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

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

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 2331 bytes --]

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
+
 #endif /* _UAPILINUX_SERIAL_CORE_H */
-- 
2.7.4


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

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

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 385 bytes --]



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>
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

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

[Greg Kroah-Hartman]

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

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

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 3294 bytes --]

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

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

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]



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.
> 

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

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

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

[Doug Berger]

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>
Acked-by: Doug Berger <opendmb@gmail.com>

Thanks Justin!
--
Doug

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

[Andy Shevchenko]

On Mon, Aug 14, 2023 at 11:09:46AM -0700, Doug Berger wrote:
> 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.

I'm not okay, sorry. But your variant seems the best from all proposed.

> > > > > > +             .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.

All these details are missing in the initial submission. How should we know all
that? Please, amend the commit message accordingly.

> > > 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.

Also choose the number less than 124, IIRC we have gaps that may be filled.

-- 
With Best Regards,
Andy Shevchenko