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 <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
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 <[email protected]>
> ---
> 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
<[email protected]> 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 <[email protected]>
> > ---
> > 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
> <[email protected]> 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?
--
With Best Regards,
Andy Shevchenko
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
>> <[email protected]> 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.
>
Would "Brcmstb 7271 UART" suffice?
>>>> + .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.
> 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