| Message ID | 1691792050-25042-1-git-send-email-justin.chen@broadcom.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: 8250_bcm7271: improve bcm7271 8250 port | expand |
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/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
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
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
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.
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 */
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(-)