Re: [PATCH] serial: 8250_bcm7271: improve bcm7271 8250 port

[Doug Berger]

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@xxxxxxxxxxxxxxxxxxx> 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