[PATCH] serial: 8250: probe all 16550A variants by default

[PATCH] serial: 8250: probe all 16550A variants by default

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

On NXP T1040, the UART is typically detected as 16550A_FSL64. After said
patch, it gets detected as plain 16550A and the Linux console is
completely garbled and missing characters.

So clearly, introducing the SERIAL_8250_16550A_VARIANTS config option
has broken many existing users because it has changed the default
behavior. Restore that by adding a 'default y' to this option. Users who
care about 20 ms shorter boot time can always disable it, but stop
wasting many debugging hours for people who don't care all that much.

Fixes: dc56ecb81a0a ("serial: 8250: Support disabling mdelay-filled probes of 16550A variants")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/tty/serial/8250/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index af0688156dd0..89c7ecb55619 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -63,6 +63,7 @@ config SERIAL_8250_PNP
 config SERIAL_8250_16550A_VARIANTS
 	bool "Support for variants of the 16550A serial port"
 	depends on SERIAL_8250
+	default y
 	help
 	  The 8250 driver can probe for many variants of the venerable 16550A
 	  serial port. Doing so takes additional time at boot.
-- 
2.25.1

Re: [PATCH] serial: 8250: probe all 16550A variants by default

[Andy Shevchenko]

On Mon, May 25, 2020 at 04:02:38PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> On NXP T1040, the UART is typically detected as 16550A_FSL64. After said
> patch, it gets detected as plain 16550A and the Linux console is
> completely garbled and missing characters.
> 
> So clearly, introducing the SERIAL_8250_16550A_VARIANTS config option
> has broken many existing users because it has changed the default
> behavior. Restore that by adding a 'default y' to this option. Users who
> care about 20 ms shorter boot time can always disable it, but stop
> wasting many debugging hours for people who don't care all that much.

While this sounds like a good enough fix of the regression, the proper one
seems to have a separate quirk driver which avoids auto probe and uses
compatible strings or whatever that board can supply. 8250_port historically
has a burden of old and buggy hardware, that's why the above mentioned patch
exposed that.

That said, I have no objections, but had to note about possible approaches.

> Fixes: dc56ecb81a0a ("serial: 8250: Support disabling mdelay-filled probes of 16550A variants")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/tty/serial/8250/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index af0688156dd0..89c7ecb55619 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -63,6 +63,7 @@ config SERIAL_8250_PNP
>  config SERIAL_8250_16550A_VARIANTS
>  	bool "Support for variants of the 16550A serial port"
>  	depends on SERIAL_8250
> +	default y
>  	help
>  	  The 8250 driver can probe for many variants of the venerable 16550A
>  	  serial port. Doing so takes additional time at boot.
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250: probe all 16550A variants by default

[Josh Triplett]

On Mon, May 25, 2020 at 04:02:38PM +0300, Vladimir Oltean wrote:
> On NXP T1040, the UART is typically detected as 16550A_FSL64. After said
> patch, it gets detected as plain 16550A and the Linux console is
> completely garbled and missing characters.

Interesting that there's *new* powerpc hardware that needs these
variants. I based the patch on the fact that, on x86 at least, hardware
using these variants hasn't been made for a long time.

In the hopes of preserving at least part of the benefit of the patch,
could you please change it to `default y if !X86_64`?

>  drivers/tty/serial/8250/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index af0688156dd0..89c7ecb55619 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -63,6 +63,7 @@ config SERIAL_8250_PNP
>  config SERIAL_8250_16550A_VARIANTS
>  	bool "Support for variants of the 16550A serial port"
>  	depends on SERIAL_8250
> +	default y
>  	help
>  	  The 8250 driver can probe for many variants of the venerable 16550A
>  	  serial port. Doing so takes additional time at boot.
> -- 
> 2.25.1
> 

Re: [PATCH] serial: 8250: probe all 16550A variants by default

[Vladimir Oltean]

Hi Josh,

On Mon, 25 May 2020 at 20:28, Josh Triplett <josh@joshtriplett.org> wrote:
>
> On Mon, May 25, 2020 at 04:02:38PM +0300, Vladimir Oltean wrote:
> > On NXP T1040, the UART is typically detected as 16550A_FSL64. After said
> > patch, it gets detected as plain 16550A and the Linux console is
> > completely garbled and missing characters.
>
> Interesting that there's *new* powerpc hardware that needs these
> variants. I based the patch on the fact that, on x86 at least, hardware
> using these variants hasn't been made for a long time.
>
> In the hopes of preserving at least part of the benefit of the patch,
> could you please change it to `default y if !X86_64`?
>

Why don't you add CONFIG_SERIAL_8250_16550A_VARIANTS=n in x86_64_defconfig?

> >  drivers/tty/serial/8250/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > index af0688156dd0..89c7ecb55619 100644
> > --- a/drivers/tty/serial/8250/Kconfig
> > +++ b/drivers/tty/serial/8250/Kconfig
> > @@ -63,6 +63,7 @@ config SERIAL_8250_PNP
> >  config SERIAL_8250_16550A_VARIANTS
> >       bool "Support for variants of the 16550A serial port"
> >       depends on SERIAL_8250
> > +     default y
> >       help
> >         The 8250 driver can probe for many variants of the venerable 16550A
> >         serial port. Doing so takes additional time at boot.
> > --
> > 2.25.1
> >

Thanks,
-Vladimir

Re: [PATCH] serial: 8250: probe all 16550A variants by default

[Josh Triplett]

On Mon, May 25, 2020 at 09:52:54PM +0300, Vladimir Oltean wrote:
> Hi Josh,
> 
> On Mon, 25 May 2020 at 20:28, Josh Triplett <josh@joshtriplett.org> wrote:
> >
> > On Mon, May 25, 2020 at 04:02:38PM +0300, Vladimir Oltean wrote:
> > > On NXP T1040, the UART is typically detected as 16550A_FSL64. After said
> > > patch, it gets detected as plain 16550A and the Linux console is
> > > completely garbled and missing characters.
> >
> > Interesting that there's *new* powerpc hardware that needs these
> > variants. I based the patch on the fact that, on x86 at least, hardware
> > using these variants hasn't been made for a long time.
> >
> > In the hopes of preserving at least part of the benefit of the patch,
> > could you please change it to `default y if !X86_64`?
> >
> 
> Why don't you add CONFIG_SERIAL_8250_16550A_VARIANTS=n in x86_64_defconfig?

In general, it seems preferable to me when the defconfig files differ
less from the defaults encoded in Kconfig.

You're proposing a change to the default; could you please include one
or the other additional change in your patch to preserve the behavior on
x86_64?

> > >  drivers/tty/serial/8250/Kconfig | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > > index af0688156dd0..89c7ecb55619 100644
> > > --- a/drivers/tty/serial/8250/Kconfig
> > > +++ b/drivers/tty/serial/8250/Kconfig
> > > @@ -63,6 +63,7 @@ config SERIAL_8250_PNP
> > >  config SERIAL_8250_16550A_VARIANTS
> > >       bool "Support for variants of the 16550A serial port"
> > >       depends on SERIAL_8250
> > > +     default y
> > >       help
> > >         The 8250 driver can probe for many variants of the venerable 16550A
> > >         serial port. Doing so takes additional time at boot.
> > > --
> > > 2.25.1
> > >
> 
> Thanks,
> -Vladimir

Re: [PATCH] serial: 8250: probe all 16550A variants by default

[Maxim Kochetkov]

This change breaks all my devices: OMAP-L138 (davinci based), LS1021A, 
T1040, Marvell (kirkwood2 based). Only enabling VARIANTS on all my 
devices fix the issue.

25.05.2020 20:28, Josh Triplett wrote:
> On Mon, May 25, 2020 at 04:02:38PM +0300, Vladimir Oltean wrote:
>> On NXP T1040, the UART is typically detected as 16550A_FSL64. After said
>> patch, it gets detected as plain 16550A and the Linux console is
>> completely garbled and missing characters.
> Interesting that there's*new*  powerpc hardware that needs these
> variants. I based the patch on the fact that, on x86 at least, hardware
> using these variants hasn't been made for a long time.

Re: [PATCH] serial: 8250: probe all 16550A variants by default

[Josh Triplett]

On Tue, May 26, 2020 at 10:05:25AM +0300, Maxim Kochetkov wrote:
> This change breaks all my devices: OMAP-L138 (davinci based), LS1021A,
> T1040, Marvell (kirkwood2 based). Only enabling VARIANTS on all my devices
> fix the issue.

Preparing a patch right now, with the appropriate Fixes tag so it should
end up on any kernel that has the original patch.

- Josh Triplett