serial: 8250_bcm7271: improve bcm7271 8250 port

  The 8250 bcm7271 UART is not a direct match to PORT_16550A. The
Fifo is 32 and rxtrig values are {1, 8, 16, 30}. Create a PORT_BCM7271
to better capture the HW CAPS.

Default the rxtrig level to 8.

Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
 drivers/tty/serial/8250/8250_bcm7271.c | 4 +---
 drivers/tty/serial/8250/8250_port.c    | 8 ++++++++
 include/uapi/linux/serial_core.h       | 3 +++
 3 files changed, 12 insertions(+), 3 deletions(-)
  

On 8/11/2023 3:14 PM, Justin Chen wrote:
> The 8250 bcm7271 UART is not a direct match to PORT_16550A. The
> Fifo is 32 and rxtrig values are {1, 8, 16, 30}. Create a PORT_BCM7271
> to better capture the HW CAPS.
> 
> Default the rxtrig level to 8.
> 
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
  

On Fri, Aug 11, 2023 at 03:14:01PM -0700, Justin Chen wrote:
> The 8250 bcm7271 UART is not a direct match to PORT_16550A. The
> Fifo is 32 and rxtrig values are {1, 8, 16, 30}. Create a PORT_BCM7271
> to better capture the HW CAPS.
> 
> Default the rxtrig level to 8.
> 
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
> ---
>  drivers/tty/serial/8250/8250_bcm7271.c | 4 +---
>  drivers/tty/serial/8250/8250_port.c    | 8 ++++++++
>  include/uapi/linux/serial_core.h       | 3 +++
>  3 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_bcm7271.c b/drivers/tty/serial/8250/8250_bcm7271.c
> index d4b05d7ad9e8..aa5aff046756 100644
> --- a/drivers/tty/serial/8250/8250_bcm7271.c
> +++ b/drivers/tty/serial/8250/8250_bcm7271.c
> @@ -1042,7 +1042,7 @@ static int brcmuart_probe(struct platform_device *pdev)
>  	dev_dbg(dev, "DMA is %senabled\n", priv->dma_enabled ? "" : "not ");
>  
>  	memset(&up, 0, sizeof(up));
> -	up.port.type = PORT_16550A;
> +	up.port.type = PORT_BCM7271;
>  	up.port.uartclk = clk_rate;
>  	up.port.dev = dev;
>  	up.port.mapbase = mapbase;
> @@ -1056,8 +1056,6 @@ static int brcmuart_probe(struct platform_device *pdev)
>  		| UPF_FIXED_PORT | UPF_FIXED_TYPE;
>  	up.port.dev = dev;
>  	up.port.private_data = priv;
> -	up.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
> -	up.port.fifosize = 32;
>  
>  	/* Check for a fixed line number */
>  	ret = of_alias_get_id(np, "serial");
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 16aeb1420137..a6259a264041 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -322,6 +322,14 @@ static const struct serial8250_config uart_config[] = {
>  		.rxtrig_bytes   = {2, 66, 130, 194},
>  		.flags          = UART_CAP_FIFO,
>  	},
> +	[PORT_BCM7271] = {
> +		.name		= "bcm7271_uart",
> +		.fifo_size	= 32,
> +		.tx_loadsz	= 32,
> +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01,
> +		.rxtrig_bytes	= {1, 8, 16, 30},
> +		.flags		= UART_CAP_FIFO | UART_CAP_AFE
> +	},
>  };
>  
>  /* Uart divisor latch read */
> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> index 281fa286555c..369f845a3d1d 100644
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -279,4 +279,7 @@
>  /* Sunplus UART */
>  #define PORT_SUNPLUS	123
>  
> +/* Broadcom 7271 UART */
> +#define PORT_BCM7271    124

Why is this new id required?  What in userspace is going to use it and
why can't the generic value be used instead?

thanks,

greg k-h
  

On Sat, Aug 12, 2023 at 3:50 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Aug 11, 2023 at 03:14:01PM -0700, Justin Chen wrote:
> > The 8250 bcm7271 UART is not a direct match to PORT_16550A. The
> > Fifo is 32 and rxtrig values are {1, 8, 16, 30}. Create a PORT_BCM7271
> > to better capture the HW CAPS.
> >
> > Default the rxtrig level to 8.
> >
> > Signed-off-by: Justin Chen <justin.chen@broadcom.com>
> > ---
> >  drivers/tty/serial/8250/8250_bcm7271.c | 4 +---
> >  drivers/tty/serial/8250/8250_port.c    | 8 ++++++++
> >  include/uapi/linux/serial_core.h       | 3 +++
> >  3 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_bcm7271.c b/drivers/tty/serial/8250/8250_bcm7271.c
> > index d4b05d7ad9e8..aa5aff046756 100644
> > --- a/drivers/tty/serial/8250/8250_bcm7271.c
> > +++ b/drivers/tty/serial/8250/8250_bcm7271.c
> > @@ -1042,7 +1042,7 @@ static int brcmuart_probe(struct platform_device *pdev)
> >       dev_dbg(dev, "DMA is %senabled\n", priv->dma_enabled ? "" : "not ");
> >
> >       memset(&up, 0, sizeof(up));
> > -     up.port.type = PORT_16550A;
> > +     up.port.type = PORT_BCM7271;
> >       up.port.uartclk = clk_rate;
> >       up.port.dev = dev;
> >       up.port.mapbase = mapbase;
> > @@ -1056,8 +1056,6 @@ static int brcmuart_probe(struct platform_device *pdev)
> >               | UPF_FIXED_PORT | UPF_FIXED_TYPE;
> >       up.port.dev = dev;
> >       up.port.private_data = priv;
> > -     up.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
> > -     up.port.fifosize = 32;
> >
> >       /* Check for a fixed line number */
> >       ret = of_alias_get_id(np, "serial");
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index 16aeb1420137..a6259a264041 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -322,6 +322,14 @@ static const struct serial8250_config uart_config[] = {
> >               .rxtrig_bytes   = {2, 66, 130, 194},
> >               .flags          = UART_CAP_FIFO,
> >       },
> > +     [PORT_BCM7271] = {
> > +             .name           = "bcm7271_uart",
> > +             .fifo_size      = 32,
> > +             .tx_loadsz      = 32,
> > +             .fcr            = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01,
> > +             .rxtrig_bytes   = {1, 8, 16, 30},
> > +             .flags          = UART_CAP_FIFO | UART_CAP_AFE
> > +     },
> >  };
> >
> >  /* Uart divisor latch read */
> > diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> > index 281fa286555c..369f845a3d1d 100644
> > --- a/include/uapi/linux/serial_core.h
> > +++ b/include/uapi/linux/serial_core.h
> > @@ -279,4 +279,7 @@
> >  /* Sunplus UART */
> >  #define PORT_SUNPLUS 123
> >
> > +/* Broadcom 7271 UART */
> > +#define PORT_BCM7271    124
>
> Why is this new id required?  What in userspace is going to use it and
> why can't the generic value be used instead?
>

I couldn't find a generic port that matches our FIFO size and
rxtrig_bytes. That is why I created a new one. Userspace currently
misreports what the rxtrig level is.

Thanks,
Justin

> thanks,
>
> greg k-h
  

On Sat, Aug 12, 2023 at 09:24:21PM -0700, Justin Chen wrote:
> On Sat, Aug 12, 2023 at 3:50 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Fri, Aug 11, 2023 at 03:14:01PM -0700, Justin Chen wrote:

> > > +     [PORT_BCM7271] = {
> > > +             .name           = "bcm7271_uart",

This is badly named port type.

> > > +             .fifo_size      = 32,
> > > +             .tx_loadsz      = 32,
> > > +             .fcr            = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01,
> > > +             .rxtrig_bytes   = {1, 8, 16, 30},
> > > +             .flags          = UART_CAP_FIFO | UART_CAP_AFE
> > > +     },
> > >  };

This is almost a dup of PORT_ALTR_16550_F32. Use it if you wish.
You can always rename it if it feels the right thing to do.

But why 8 and not 16 is the default rxtrig?
  

On 8/14/2023 9:28 AM, Justin Chen wrote:
> 
> 
> On 8/14/23 8:12 AM, Andy Shevchenko wrote:
>> On Sat, Aug 12, 2023 at 09:24:21PM -0700, Justin Chen wrote:
>>> On Sat, Aug 12, 2023 at 3:50 AM Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org> wrote:
>>>> On Fri, Aug 11, 2023 at 03:14:01PM -0700, Justin Chen wrote:
>>
>>>>> +     [PORT_BCM7271] = {
>>>>> +             .name           = "bcm7271_uart",
>>
>> This is badly named port type.
>>
This may be true, but it does mirror the PORT_BCM63XX naming and I do 
value consistency so it is acceptable to me. However, I will happily 
yield to a better name if one can be determined by popular consensus.

> 
> Would "Brcmstb 7271 UART" suffice?
> 
Perhaps, "Broadcom BCM7271 UART" but it seems excessively "chatty" to 
me, so as I said I am OK with the original submission.

>>>>> +             .fifo_size      = 32,
>>>>> +             .tx_loadsz      = 32,
>>>>> +             .fcr            = UART_FCR_ENABLE_FIFO | 
>>>>> UART_FCR_R_TRIG_01,
>>>>> +             .rxtrig_bytes   = {1, 8, 16, 30},
>>>>> +             .flags          = UART_CAP_FIFO | UART_CAP_AFE
>>>>> +     },
>>>>>   };
>>
>> This is almost a dup of PORT_ALTR_16550_F32. Use it if you wish.
>> You can always rename it if it feels the right thing to do.
>>
> 
> There is some other PORT_ALTR logic that I would like to avoid. I would 
> also like to avoid future changes to PORT_ALTR that wouldn't be 
> applicable to us.
I too am reluctant to introduce yet another port type, but Justin is 
correct in pointing out that the PORT_ALTR_16550_* port types include Tx 
FIFO threshold programming that is incompatible with the BCM7271 UART 
hardware. This port type does appear necessary to address fundamental 
differences in the hardware unless we are willing to scrap the 
uart_config[] array and have the individual drivers manage these 
differences (which I would also be OK with, but I am just a tail on this 
dog).

The BCM7271 UART IP does support programmable Tx FIFO thresholds in a 
different way, so if I (or someone else) decided to enable support for 
that it would appear that this new port type would be necessary at that 
time as well.

> 
>> But why 8 and not 16 is the default rxtrig?
>>
> 
> We were seeing some latency issues on our chips where 16 would cause 
> overflows. Trying to kill 2 birds with one stone. If creating another 
> port type is avoidable then alternatively I can change the default in 
> userspace.
> 
> Thanks,
> Justin

Regards,
     Doug