[-next] serial: 8250: Match legacy NS16550A UARTs

[-next] serial: 8250: Match legacy NS16550A UARTs

[Al Cooper]

From: Florian Fainelli <f.fainelli@gmail.com>

Older 32-bit only Broadcom STB chips used a NS16550A compatible UART,
the 8250_bcm7271.c driver can drive those UARTs just fine provided that
we let it match the appropriate compatible string.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/tty/serial/8250/8250_bcm7271.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_bcm7271.c b/drivers/tty/serial/8250/8250_bcm7271.c
index 725a450058f8..023a2de8b2d6 100644
--- a/drivers/tty/serial/8250/8250_bcm7271.c
+++ b/drivers/tty/serial/8250/8250_bcm7271.c
@@ -204,6 +204,13 @@ static const u32 brcmstb_rate_table_7278[] = {
 	MHZ(48),
 };
 
+static const u32 brcmstb_rate_table_16550a[] = {
+	MHZ(81),
+	0,
+	0,
+	0,
+};
+
 struct brcmuart_priv {
 	int		line;
 	struct clk	*baud_mux_clk;
@@ -865,6 +872,10 @@ static const struct of_device_id brcmuart_dt_ids[] = {
 		.compatible = "brcm,bcm7271-uart",
 		.data = brcmstb_rate_table,
 	},
+	{
+		.compatible = "ns16550a",
+		.data = brcmstb_rate_table_16550a,
+	},
 	{},
 };
 
-- 
2.17.1

Re: [-next] serial: 8250: Match legacy NS16550A UARTs

[Greg Kroah-Hartman]

On Wed, Apr 14, 2021 at 09:45:39AM -0400, Al Cooper wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> 
> Older 32-bit only Broadcom STB chips used a NS16550A compatible UART,
> the 8250_bcm7271.c driver can drive those UARTs just fine provided that
> we let it match the appropriate compatible string.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> Reviewed-by: Al Cooper <alcooperx@gmail.com>

When forwarding on patches from others, you need to sign-off on them,
not just say "reviewed-by" as I am obtaining the patch from you, not
Florian.

Please fix up.

thanks,

greg k-h

Re: [-next] serial: 8250: Match legacy NS16550A UARTs

[Andy Shevchenko]

On Wed, Apr 14, 2021 at 7:13 PM Al Cooper <alcooperx@gmail.com> wrote:
>
> From: Florian Fainelli <f.fainelli@gmail.com>
>
> Older 32-bit only Broadcom STB chips used a NS16550A compatible UART,
> the 8250_bcm7271.c driver can drive those UARTs just fine provided that
> we let it match the appropriate compatible string.

This sounds not correct to me, ns16550a is a national semiconductor product.

Why is it here and not in generic 8250_of?

> +       {
> +               .compatible = "ns16550a",
> +               .data = brcmstb_rate_table_16550a,
> +       },


-- 
With Best Regards,
Andy Shevchenko

Re: [-next] serial: 8250: Match legacy NS16550A UARTs

[Alan Cooper]

On Thu, Apr 15, 2021 at 6:44 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Apr 14, 2021 at 7:13 PM Al Cooper <alcooperx@gmail.com> wrote:
> >
> > From: Florian Fainelli <f.fainelli@gmail.com>
> >
> > Older 32-bit only Broadcom STB chips used a NS16550A compatible UART,
> > the 8250_bcm7271.c driver can drive those UARTs just fine provided that
> > we let it match the appropriate compatible string.
>
> This sounds not correct to me, ns16550a is a national semiconductor product.
>
> Why is it here and not in generic 8250_of?

Andy,

While double checking in preparation for this response I was surprised
to find that the problem that this was meant to solve no longer
exists. The original issue was a result of the two UART drivers
returning probe DEFERs when getting their clocks. This was happening
because we use SCMI based clocks and they were coming up late in the
initialization process. The problem no longer exists because the SCMI
clock driver is now being called by subsys_init instead of module_init
and the drivers don't DEFER on clocks.

Here's a more detailed description of the problem we were trying to solve.

Our SoC's have always had a NS16650A UART core and older SoC's would
have a compatible string of: 'compatible = ""ns16550a"' and use the
8250_of driver. Our newer SoC's have added enhancements to the base
core to add support for DMA and accurate high speed baud rates and use
this newer 8250_bcm7271 driver. The Device Tree node for our enhanced
UARTs has a compatible string of: 'compatible = "brcm,bcm7271-uart",
"ns16550a"''. With both drivers running and the link order setup so
that the 8250_bcm7217 driver is initialized before the 8250_of driver,
we should bind the 8250_bcm7271 driver to the enhanced UART, or for
upstream kernels that don't have the 8250_bcm7271 driver, we bind to
the 8250_of driver.

The problem is that when both the 8250_of and 8250_bcm7271 drivers
were running, occasionally the 8250_of driver would be bound to the
enhanced UART instead of the 8250_bcm7271 driver. This was happening
because we use SCMI based clocks which come up late in initialization
and cause probe DEFER's when the two drivers get their clocks.
Occasionally the SCMI clock would become ready between the
8250_bcm7271 probe and the 8250_of probe and the 8250_of driver would
be bound. To fix this we decided to config only our 8250_bcm7271
driver and added "ns16665a0" to the compatible string so the driver
would work on our older system.

I was surprised by a couple of things while working on this issue.
1. The compatible string left to right order has no effect on which
driver is bound for builtin drivers.
2. I thought that the new "device links" added by the OF framework for
clocks would fix this order issue but it did not because the link was
made to the parent of the SCMI clock protocol which comes up long
before the clock protocol.

Thanks
Al



>
> > +       {
> > +               .compatible = "ns16550a",
> > +               .data = brcmstb_rate_table_16550a,
> > +       },
>
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [-next] serial: 8250: Match legacy NS16550A UARTs

[Andy Shevchenko]

On Wed, Apr 21, 2021 at 10:04 PM Alan Cooper <alcooperx@gmail.com> wrote:
> On Thu, Apr 15, 2021 at 6:44 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Apr 14, 2021 at 7:13 PM Al Cooper <alcooperx@gmail.com> wrote:

> The problem is that when both the 8250_of and 8250_bcm7271 drivers
> were running, occasionally the 8250_of driver would be bound to the
> enhanced UART instead of the 8250_bcm7271 driver. This was happening
> because we use SCMI based clocks which come up late in initialization
> and cause probe DEFER's when the two drivers get their clocks.
> Occasionally the SCMI clock would become ready between the
> 8250_bcm7271 probe and the 8250_of probe and the 8250_of driver would
> be bound. To fix this we decided to config only our 8250_bcm7271
> driver and added "ns16665a0" to the compatible string so the driver
> would work on our older system.

Interesting reading.

As far as I understand the 8250 approach (*), you blacklist (or
whatever naming you prefer, b/c 8250_of seems does not have such) the
binding based on the presence of the specific compatible string.

I.o.w. in 8250_of you need to check if you are trying to probe the
device which has both compatible strings. In that case you simply
return -ENODEV.

*) 8250_pci does like this.

-- 
With Best Regards,
Andy Shevchenko

Re: [-next] serial: 8250: Match legacy NS16550A UARTs

[Florian Fainelli]

On 4/21/2021 12:57 PM, Andy Shevchenko wrote:
> On Wed, Apr 21, 2021 at 10:04 PM Alan Cooper <alcooperx@gmail.com> wrote:
>> On Thu, Apr 15, 2021 at 6:44 AM Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Wed, Apr 14, 2021 at 7:13 PM Al Cooper <alcooperx@gmail.com> wrote:
> 
>> The problem is that when both the 8250_of and 8250_bcm7271 drivers
>> were running, occasionally the 8250_of driver would be bound to the
>> enhanced UART instead of the 8250_bcm7271 driver. This was happening
>> because we use SCMI based clocks which come up late in initialization
>> and cause probe DEFER's when the two drivers get their clocks.
>> Occasionally the SCMI clock would become ready between the
>> 8250_bcm7271 probe and the 8250_of probe and the 8250_of driver would
>> be bound. To fix this we decided to config only our 8250_bcm7271
>> driver and added "ns16665a0" to the compatible string so the driver
>> would work on our older system.
> 
> Interesting reading.
> 
> As far as I understand the 8250 approach (*), you blacklist (or
> whatever naming you prefer, b/c 8250_of seems does not have such) the
> binding based on the presence of the specific compatible string.
> 
> I.o.w. in 8250_of you need to check if you are trying to probe the
> device which has both compatible strings. In that case you simply
> return -ENODEV.

Yes we had a downstream patch not submitted that did exactly that:

+       if (IS_ENABLED(CONFIG_SERIAL_8250_BCM7271) &&
+           of_device_is_compatible(ofdev->dev.of_node,
"brcm,bcm7271-uart"))
+               return -ENODEV;
+

but thanks to Al's findings it does not appear to be needed anymore, we
could submit it somehow if you feel like other scenarios like having
SCMI and the UART drivers as modules.
-- 
Florian

Re: [-next] serial: 8250: Match legacy NS16550A UARTs

[Andy Shevchenko]

On Wed, Apr 21, 2021 at 11:00 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 4/21/2021 12:57 PM, Andy Shevchenko wrote:
> > On Wed, Apr 21, 2021 at 10:04 PM Alan Cooper <alcooperx@gmail.com> wrote:
> >> On Thu, Apr 15, 2021 at 6:44 AM Andy Shevchenko
> >> <andy.shevchenko@gmail.com> wrote:
> >>> On Wed, Apr 14, 2021 at 7:13 PM Al Cooper <alcooperx@gmail.com> wrote:
> >
> >> The problem is that when both the 8250_of and 8250_bcm7271 drivers
> >> were running, occasionally the 8250_of driver would be bound to the
> >> enhanced UART instead of the 8250_bcm7271 driver. This was happening
> >> because we use SCMI based clocks which come up late in initialization
> >> and cause probe DEFER's when the two drivers get their clocks.
> >> Occasionally the SCMI clock would become ready between the
> >> 8250_bcm7271 probe and the 8250_of probe and the 8250_of driver would
> >> be bound. To fix this we decided to config only our 8250_bcm7271
> >> driver and added "ns16665a0" to the compatible string so the driver
> >> would work on our older system.
> >
> > Interesting reading.
> >
> > As far as I understand the 8250 approach (*), you blacklist (or
> > whatever naming you prefer, b/c 8250_of seems does not have such) the
> > binding based on the presence of the specific compatible string.
> >
> > I.o.w. in 8250_of you need to check if you are trying to probe the
> > device which has both compatible strings. In that case you simply
> > return -ENODEV.
>
> Yes we had a downstream patch not submitted that did exactly that:
>
> +       if (IS_ENABLED(CONFIG_SERIAL_8250_BCM7271) &&
> +           of_device_is_compatible(ofdev->dev.of_node,
> "brcm,bcm7271-uart"))
> +               return -ENODEV;
> +
>
> but thanks to Al's findings it does not appear to be needed anymore, we
> could submit it somehow if you feel like other scenarios like having
> SCMI and the UART drivers as modules.

I suggest to upstream it anyway.It will make the kernel robust.

-- 
With Best Regards,
Andy Shevchenko