* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 @ 2018-11-15 1:11 Guenter Roeck 2018-11-15 3:56 ` Florian Fainelli 2018-12-20 15:21 ` Greg Kroah-Hartman 0 siblings, 2 replies; 20+ messages in thread From: Guenter Roeck @ 2018-11-15 1:11 UTC (permalink / raw) To: Florian Fainelli; +Cc: Greg Kroah-Hartman, linux-kernel On Thu, Nov 01, 2018 at 11:26:06AM -0700, Florian Fainelli wrote: > It is way too easy to miss enabling SERIAL_OF_PLATFORM which would > result in the inability for the kernel to have a valid console device, > which can be seen with: > > Warning: unable to open an initial console. > > and then: > > Run /init as init process > Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000100 > > Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there > really is no drawback to defaulting this config to the value of > SERIAL_8250. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> This patch results in situations where CONFIG_SERIAL_OF_PLATFORM is now defined where it was not previously. Example mpc85xx_defconfig. This in turn results in boot failures for those configurations, with an error message of of_serial: probe of e0004500.serial failed with error -22 which wasn't seen before. Not sure if replacing a potential problem with a real one is really an improvement.` Guenter --- > 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 15c2c5463835..d7737dca0e48 100644 > --- a/drivers/tty/serial/8250/Kconfig > +++ b/drivers/tty/serial/8250/Kconfig > @@ -484,6 +484,7 @@ config SERIAL_8250_PXA > config SERIAL_OF_PLATFORM > tristate "Devicetree based probing for 8250 ports" > depends on SERIAL_8250 && OF > + default SERIAL_8250 > help > This option is used for all 8250 compatible serial ports that > are probed through devicetree, including Open Firmware based > -- > 2.7.4 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-15 1:11 [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 Guenter Roeck @ 2018-11-15 3:56 ` Florian Fainelli 2018-11-15 5:36 ` Guenter Roeck 2018-11-15 5:38 ` Guenter Roeck 2018-12-20 15:21 ` Greg Kroah-Hartman 1 sibling, 2 replies; 20+ messages in thread From: Florian Fainelli @ 2018-11-15 3:56 UTC (permalink / raw) To: Guenter Roeck; +Cc: Greg Kroah-Hartman, linux-kernel On November 14, 2018 5:11:25 PM PST, Guenter Roeck <linux@roeck-us.net> wrote: >On Thu, Nov 01, 2018 at 11:26:06AM -0700, Florian Fainelli wrote: >> It is way too easy to miss enabling SERIAL_OF_PLATFORM which would >> result in the inability for the kernel to have a valid console >device, >> which can be seen with: >> >> Warning: unable to open an initial console. >> >> and then: >> >> Run /init as init process >> Kernel panic - not syncing: Attempted to kill init! >exitcode=0x00000100 >> >> Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there >> really is no drawback to defaulting this config to the value of >> SERIAL_8250. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >This patch results in situations where CONFIG_SERIAL_OF_PLATFORM is now >defined where it was not previously. Example mpc85xx_defconfig. This in >turn results in boot failures for those configurations, with an error >message of > >of_serial: probe of e0004500.serial failed with error -22 > >which wasn't seen before. Do you know which Device Tree is being used here? The most obvious thing that could be done is to add a !PPC condition but this might be missing other platforms doing their own 8250 registration yet being OF aware (sparc?). > >Not sure if replacing a potential problem with a real one is really an >improvement.` That comment is not particularly helpful though I have an appreciation for when a change breaks things in unexpected ways and how frustrating that can be. -- Florian ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-15 3:56 ` Florian Fainelli @ 2018-11-15 5:36 ` Guenter Roeck 2018-11-15 17:19 ` Florian Fainelli 2018-11-15 5:38 ` Guenter Roeck 1 sibling, 1 reply; 20+ messages in thread From: Guenter Roeck @ 2018-11-15 5:36 UTC (permalink / raw) To: Florian Fainelli; +Cc: Greg Kroah-Hartman, linux-kernel On Wed, Nov 14, 2018 at 07:56:47PM -0800, Florian Fainelli wrote: > > > On November 14, 2018 5:11:25 PM PST, Guenter Roeck <linux@roeck-us.net> wrote: > >On Thu, Nov 01, 2018 at 11:26:06AM -0700, Florian Fainelli wrote: > >> It is way too easy to miss enabling SERIAL_OF_PLATFORM which would > >> result in the inability for the kernel to have a valid console > >device, > >> which can be seen with: > >> > >> Warning: unable to open an initial console. > >> > >> and then: > >> > >> Run /init as init process > >> Kernel panic - not syncing: Attempted to kill init! > >exitcode=0x00000100 > >> > >> Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there > >> really is no drawback to defaulting this config to the value of > >> SERIAL_8250. > >> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > >This patch results in situations where CONFIG_SERIAL_OF_PLATFORM is now > >defined where it was not previously. Example mpc85xx_defconfig. This in > >turn results in boot failures for those configurations, with an error > >message of > > > >of_serial: probe of e0004500.serial failed with error -22 > > > >which wasn't seen before. > > Do you know which Device Tree is being used here? The most obvious thing that could be done is to add a !PPC condition but this might be missing other platforms doing their own 8250 registration yet being OF aware (sparc?). This is a qemu boot test. No, I don't know what exactly is happening, except that this (emulated) system obviously does not expect CONFIG_SERIAL_OF_PLATFORM to be enabled and, afaik, the devicetree is generated internally by qemu. I would have thought that just enabling a configuration by default out of the blue might be considered problematic by itself, but maybe I am wrong. > > > > >Not sure if replacing a potential problem with a real one is really an > >improvement.` > > That comment is not particularly helpful though I have an appreciation for when a change breaks things in unexpected ways and how frustrating that can be. What is really frustrating (and let me think about dropping all those boot tests) is that one ends up having to argue if the problem is real or only applies to a presumably or possibly wrong qemu emulation, and that one ends up having to discuss the validity of the test case. Since this is "only" an emulation and thus not a "real" system, please feel free to ignore this report. I'll just drop all boot tests using this configuration once the patch hits mainline. Guenter ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-15 5:36 ` Guenter Roeck @ 2018-11-15 17:19 ` Florian Fainelli 0 siblings, 0 replies; 20+ messages in thread From: Florian Fainelli @ 2018-11-15 17:19 UTC (permalink / raw) To: Guenter Roeck; +Cc: Greg Kroah-Hartman, linux-kernel On 11/14/2018 9:36 PM, Guenter Roeck wrote: > On Wed, Nov 14, 2018 at 07:56:47PM -0800, Florian Fainelli wrote: >> >> >> On November 14, 2018 5:11:25 PM PST, Guenter Roeck <linux@roeck-us.net> wrote: >>> On Thu, Nov 01, 2018 at 11:26:06AM -0700, Florian Fainelli wrote: >>>> It is way too easy to miss enabling SERIAL_OF_PLATFORM which would >>>> result in the inability for the kernel to have a valid console >>> device, >>>> which can be seen with: >>>> >>>> Warning: unable to open an initial console. >>>> >>>> and then: >>>> >>>> Run /init as init process >>>> Kernel panic - not syncing: Attempted to kill init! >>> exitcode=0x00000100 >>>> >>>> Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there >>>> really is no drawback to defaulting this config to the value of >>>> SERIAL_8250. >>>> >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> >>> This patch results in situations where CONFIG_SERIAL_OF_PLATFORM is now >>> defined where it was not previously. Example mpc85xx_defconfig. This in >>> turn results in boot failures for those configurations, with an error >>> message of >>> >>> of_serial: probe of e0004500.serial failed with error -22 >>> >>> which wasn't seen before. >> >> Do you know which Device Tree is being used here? The most obvious thing that could be done is to add a !PPC condition but this might be missing other platforms doing their own 8250 registration yet being OF aware (sparc?). > > This is a qemu boot test. No, I don't know what exactly is happening, > except that this (emulated) system obviously does not expect > CONFIG_SERIAL_OF_PLATFORM to be enabled and, afaik, the devicetree > is generated internally by qemu. I would have thought that just enabling > a configuration by default out of the blue might be considered problematic > by itself, but maybe I am wrong. > >> >>> >>> Not sure if replacing a potential problem with a real one is really an >>> improvement.` >> >> That comment is not particularly helpful though I have an appreciation for when a change breaks things in unexpected ways and how frustrating that can be. > > What is really frustrating (and let me think about dropping all those > boot tests) is that one ends up having to argue if the problem is real > or only applies to a presumably or possibly wrong qemu emulation, and > that one ends up having to discuss the validity of the test case. What I was objecting to is your qualification of the issue, this is unfortunately not a potential/latent problem, it happens more often than not and the fact that my patch causes another platform to break is not expected and deserves fixing (which I am looking at the moment). That is all my comment was supposed to mean, and > > Since this is "only" an emulation and thus not a "real" system, > please feel free to ignore this report. I'll just drop all boot tests > using this configuration once the patch hits mainline. Sounds like we did not start on the right foot with my reply to your comment, so let's move on and just agree that this needs fixing, period. This does not question the value of your tests which are extremely valuable. -- Florian ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-15 3:56 ` Florian Fainelli 2018-11-15 5:36 ` Guenter Roeck @ 2018-11-15 5:38 ` Guenter Roeck 2018-11-15 17:19 ` Florian Fainelli 1 sibling, 1 reply; 20+ messages in thread From: Guenter Roeck @ 2018-11-15 5:38 UTC (permalink / raw) To: Florian Fainelli; +Cc: Greg Kroah-Hartman, linux-kernel On Wed, Nov 14, 2018 at 07:56:47PM -0800, Florian Fainelli wrote: > > > On November 14, 2018 5:11:25 PM PST, Guenter Roeck <linux@roeck-us.net> wrote: > >On Thu, Nov 01, 2018 at 11:26:06AM -0700, Florian Fainelli wrote: > >> It is way too easy to miss enabling SERIAL_OF_PLATFORM which would > >> result in the inability for the kernel to have a valid console > >device, > >> which can be seen with: > >> > >> Warning: unable to open an initial console. > >> > >> and then: > >> > >> Run /init as init process > >> Kernel panic - not syncing: Attempted to kill init! > >exitcode=0x00000100 > >> > >> Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there > >> really is no drawback to defaulting this config to the value of > >> SERIAL_8250. > >> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > >This patch results in situations where CONFIG_SERIAL_OF_PLATFORM is now > >defined where it was not previously. Example mpc85xx_defconfig. This in > >turn results in boot failures for those configurations, with an error > >message of > > > >of_serial: probe of e0004500.serial failed with error -22 > > > >which wasn't seen before. > > Do you know which Device Tree is being used here? The most obvious thing that could be done is to add a !PPC condition but this might be missing other platforms doing their own 8250 registration yet being OF aware (sparc?). > > > > >Not sure if replacing a potential problem with a real one is really an > >improvement.` > > That comment is not particularly helpful though I have an appreciation for when a change breaks things in unexpected ways and how frustrating that can be. Actally, never mind. I dropped the test cases. Sorry for the noise. Guenter ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-15 5:38 ` Guenter Roeck @ 2018-11-15 17:19 ` Florian Fainelli 2018-11-15 17:25 ` Guenter Roeck 0 siblings, 1 reply; 20+ messages in thread From: Florian Fainelli @ 2018-11-15 17:19 UTC (permalink / raw) To: Guenter Roeck; +Cc: Greg Kroah-Hartman, linux-kernel On 11/14/2018 9:38 PM, Guenter Roeck wrote: > On Wed, Nov 14, 2018 at 07:56:47PM -0800, Florian Fainelli wrote: >> >> >> On November 14, 2018 5:11:25 PM PST, Guenter Roeck <linux@roeck-us.net> wrote: >>> On Thu, Nov 01, 2018 at 11:26:06AM -0700, Florian Fainelli wrote: >>>> It is way too easy to miss enabling SERIAL_OF_PLATFORM which would >>>> result in the inability for the kernel to have a valid console >>> device, >>>> which can be seen with: >>>> >>>> Warning: unable to open an initial console. >>>> >>>> and then: >>>> >>>> Run /init as init process >>>> Kernel panic - not syncing: Attempted to kill init! >>> exitcode=0x00000100 >>>> >>>> Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there >>>> really is no drawback to defaulting this config to the value of >>>> SERIAL_8250. >>>> >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> >>> This patch results in situations where CONFIG_SERIAL_OF_PLATFORM is now >>> defined where it was not previously. Example mpc85xx_defconfig. This in >>> turn results in boot failures for those configurations, with an error >>> message of >>> >>> of_serial: probe of e0004500.serial failed with error -22 >>> >>> which wasn't seen before. >> >> Do you know which Device Tree is being used here? The most obvious thing that could be done is to add a !PPC condition but this might be missing other platforms doing their own 8250 registration yet being OF aware (sparc?). >> >>> >>> Not sure if replacing a potential problem with a real one is really an >>> improvement.` >> >> That comment is not particularly helpful though I have an appreciation for when a change breaks things in unexpected ways and how frustrating that can be. > > Actally, never mind. I dropped the test cases. Sorry for the noise. Why? The tests are useful, if I gave you an impression that I was just going to walk away from this issue and not look at it, then that is not happening. What I was objecting to is your qualification of the issue, this is unfortunately not a potential/latent problem, it happens more often than not. The key difference is that most subsystems that are CONFIG_OF aware will get that awareness enabled/active automatically, that is not the case with the 8250 driver, which is why this you can shoot yourself in the foot and end-up with the 8250 driver enabled, but not CONFIG_SERIAL_OF_PLATFORM. I also suspect that for historical reasons powerpc "manually" registers its 8250 ports and does not rely on CONFIG_OF_SERIAL_PLATFORM. I will try to cook a patch as quickly as possible. Thanks -- Florian ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-15 17:19 ` Florian Fainelli @ 2018-11-15 17:25 ` Guenter Roeck 2018-11-15 19:48 ` Florian Fainelli 0 siblings, 1 reply; 20+ messages in thread From: Guenter Roeck @ 2018-11-15 17:25 UTC (permalink / raw) To: Florian Fainelli; +Cc: Greg Kroah-Hartman, linux-kernel On Thu, Nov 15, 2018 at 09:19:14AM -0800, Florian Fainelli wrote: > > > On 11/14/2018 9:38 PM, Guenter Roeck wrote: > > On Wed, Nov 14, 2018 at 07:56:47PM -0800, Florian Fainelli wrote: > >> > >> > >> On November 14, 2018 5:11:25 PM PST, Guenter Roeck <linux@roeck-us.net> wrote: > >>> On Thu, Nov 01, 2018 at 11:26:06AM -0700, Florian Fainelli wrote: > >>>> It is way too easy to miss enabling SERIAL_OF_PLATFORM which would > >>>> result in the inability for the kernel to have a valid console > >>> device, > >>>> which can be seen with: > >>>> > >>>> Warning: unable to open an initial console. > >>>> > >>>> and then: > >>>> > >>>> Run /init as init process > >>>> Kernel panic - not syncing: Attempted to kill init! > >>> exitcode=0x00000100 > >>>> > >>>> Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there > >>>> really is no drawback to defaulting this config to the value of > >>>> SERIAL_8250. > >>>> > >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >>> > >>> This patch results in situations where CONFIG_SERIAL_OF_PLATFORM is now > >>> defined where it was not previously. Example mpc85xx_defconfig. This in > >>> turn results in boot failures for those configurations, with an error > >>> message of > >>> > >>> of_serial: probe of e0004500.serial failed with error -22 > >>> > >>> which wasn't seen before. > >> > >> Do you know which Device Tree is being used here? The most obvious thing that could be done is to add a !PPC condition but this might be missing other platforms doing their own 8250 registration yet being OF aware (sparc?). > >> > >>> > >>> Not sure if replacing a potential problem with a real one is really an > >>> improvement.` > >> > >> That comment is not particularly helpful though I have an appreciation for when a change breaks things in unexpected ways and how frustrating that can be. > > > > Actally, never mind. I dropped the test cases. Sorry for the noise. > > Why? The tests are useful, if I gave you an impression that I was just > going to walk away from this issue and not look at it, then that is not > happening. What I was objecting to is your qualification of the issue, > this is unfortunately not a potential/latent problem, it happens more > often than not. > I ended up adding a flag to my builders which remove the offending configuration for affected platforms/configurations, so this is no longer an issue for my build tests. Guenter ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-15 17:25 ` Guenter Roeck @ 2018-11-15 19:48 ` Florian Fainelli 2018-11-16 1:16 ` Guenter Roeck 0 siblings, 1 reply; 20+ messages in thread From: Florian Fainelli @ 2018-11-15 19:48 UTC (permalink / raw) To: Guenter Roeck, arnd; +Cc: Greg Kroah-Hartman, linux-kernel On 11/15/18 9:25 AM, Guenter Roeck wrote: > On Thu, Nov 15, 2018 at 09:19:14AM -0800, Florian Fainelli wrote: >> >> >> On 11/14/2018 9:38 PM, Guenter Roeck wrote: >>> On Wed, Nov 14, 2018 at 07:56:47PM -0800, Florian Fainelli wrote: >>>> >>>> >>>> On November 14, 2018 5:11:25 PM PST, Guenter Roeck <linux@roeck-us.net> wrote: >>>>> On Thu, Nov 01, 2018 at 11:26:06AM -0700, Florian Fainelli wrote: >>>>>> It is way too easy to miss enabling SERIAL_OF_PLATFORM which would >>>>>> result in the inability for the kernel to have a valid console >>>>> device, >>>>>> which can be seen with: >>>>>> >>>>>> Warning: unable to open an initial console. >>>>>> >>>>>> and then: >>>>>> >>>>>> Run /init as init process >>>>>> Kernel panic - not syncing: Attempted to kill init! >>>>> exitcode=0x00000100 >>>>>> >>>>>> Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there >>>>>> really is no drawback to defaulting this config to the value of >>>>>> SERIAL_8250. >>>>>> >>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>>> >>>>> This patch results in situations where CONFIG_SERIAL_OF_PLATFORM is now >>>>> defined where it was not previously. Example mpc85xx_defconfig. This in >>>>> turn results in boot failures for those configurations, with an error >>>>> message of >>>>> >>>>> of_serial: probe of e0004500.serial failed with error -22 >>>>> >>>>> which wasn't seen before. >>>> >>>> Do you know which Device Tree is being used here? The most obvious thing that could be done is to add a !PPC condition but this might be missing other platforms doing their own 8250 registration yet being OF aware (sparc?). >>>> >>>>> >>>>> Not sure if replacing a potential problem with a real one is really an >>>>> improvement.` >>>> >>>> That comment is not particularly helpful though I have an appreciation for when a change breaks things in unexpected ways and how frustrating that can be. >>> >>> Actally, never mind. I dropped the test cases. Sorry for the noise. >> >> Why? The tests are useful, if I gave you an impression that I was just >> going to walk away from this issue and not look at it, then that is not >> happening. What I was objecting to is your qualification of the issue, >> this is unfortunately not a potential/latent problem, it happens more >> often than not. >> > > I ended up adding a flag to my builders which remove the offending > configuration for affected platforms/configurations, so this is no longer > an issue for my build tests. OK, would you mind testing this below? It seems to me that 8250_of.c is incompatible with arch/powerpc/kernel/legacy_serial.c and that is what is causing the issue here. diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig index d7737dca0e48..21cb14cbd34a 100644 --- a/drivers/tty/serial/8250/Kconfig +++ b/drivers/tty/serial/8250/Kconfig @@ -483,7 +483,7 @@ config SERIAL_8250_PXA config SERIAL_OF_PLATFORM tristate "Devicetree based probing for 8250 ports" - depends on SERIAL_8250 && OF + depends on SERIAL_8250 && OF && !(PPC && PPC_UDBG_16550) default SERIAL_8250 help This option is used for all 8250 compatible serial ports that -- Florian ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-15 19:48 ` Florian Fainelli @ 2018-11-16 1:16 ` Guenter Roeck 2018-11-19 18:44 ` Florian Fainelli 0 siblings, 1 reply; 20+ messages in thread From: Guenter Roeck @ 2018-11-16 1:16 UTC (permalink / raw) To: Florian Fainelli; +Cc: arnd, Greg Kroah-Hartman, linux-kernel On Thu, Nov 15, 2018 at 11:48:20AM -0800, Florian Fainelli wrote: > > OK, would you mind testing this below? It seems to me that 8250_of.c is > incompatible with arch/powerpc/kernel/legacy_serial.c and that is what > is causing the issue here. > > diff --git a/drivers/tty/serial/8250/Kconfig > b/drivers/tty/serial/8250/Kconfig > index d7737dca0e48..21cb14cbd34a 100644 > --- a/drivers/tty/serial/8250/Kconfig > +++ b/drivers/tty/serial/8250/Kconfig > @@ -483,7 +483,7 @@ config SERIAL_8250_PXA > > config SERIAL_OF_PLATFORM > tristate "Devicetree based probing for 8250 ports" > - depends on SERIAL_8250 && OF > + depends on SERIAL_8250 && OF && !(PPC && PPC_UDBG_16550) > default SERIAL_8250 > help > This option is used for all 8250 compatible serial ports that 44x/virtex5_defconfig has both PPC_UDBG_16550 and SERIAL_OF_PLATFORM enabled and fails to boot (or display anything on the console) with this patch applied. Guenter ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-16 1:16 ` Guenter Roeck @ 2018-11-19 18:44 ` Florian Fainelli 2018-11-19 20:50 ` Guenter Roeck 0 siblings, 1 reply; 20+ messages in thread From: Florian Fainelli @ 2018-11-19 18:44 UTC (permalink / raw) To: Guenter Roeck; +Cc: arnd, Greg Kroah-Hartman, linux-kernel On 11/15/18 5:16 PM, Guenter Roeck wrote: > On Thu, Nov 15, 2018 at 11:48:20AM -0800, Florian Fainelli wrote: >> >> OK, would you mind testing this below? It seems to me that 8250_of.c is >> incompatible with arch/powerpc/kernel/legacy_serial.c and that is what >> is causing the issue here. >> >> diff --git a/drivers/tty/serial/8250/Kconfig >> b/drivers/tty/serial/8250/Kconfig >> index d7737dca0e48..21cb14cbd34a 100644 >> --- a/drivers/tty/serial/8250/Kconfig >> +++ b/drivers/tty/serial/8250/Kconfig >> @@ -483,7 +483,7 @@ config SERIAL_8250_PXA >> >> config SERIAL_OF_PLATFORM >> tristate "Devicetree based probing for 8250 ports" >> - depends on SERIAL_8250 && OF >> + depends on SERIAL_8250 && OF && !(PPC && PPC_UDBG_16550) >> default SERIAL_8250 >> help >> This option is used for all 8250 compatible serial ports that > > 44x/virtex5_defconfig has both PPC_UDBG_16550 and SERIAL_OF_PLATFORM enabled > and fails to boot (or display anything on the console) with this patch applied. Thanks for trying, can you either share or provide a link to the mpc85xx and ml507 qemu command lines that you use? I spent a good chunk of my time trying to get a kernel to boot but has failed so far. Thanks -- Florian ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-19 18:44 ` Florian Fainelli @ 2018-11-19 20:50 ` Guenter Roeck 2018-11-23 18:20 ` Guenter Roeck 0 siblings, 1 reply; 20+ messages in thread From: Guenter Roeck @ 2018-11-19 20:50 UTC (permalink / raw) To: Florian Fainelli; +Cc: arnd, Greg Kroah-Hartman, linux-kernel On Mon, Nov 19, 2018 at 10:44:30AM -0800, Florian Fainelli wrote: > On 11/15/18 5:16 PM, Guenter Roeck wrote: > > On Thu, Nov 15, 2018 at 11:48:20AM -0800, Florian Fainelli wrote: > >> > >> OK, would you mind testing this below? It seems to me that 8250_of.c is > >> incompatible with arch/powerpc/kernel/legacy_serial.c and that is what > >> is causing the issue here. > >> > >> diff --git a/drivers/tty/serial/8250/Kconfig > >> b/drivers/tty/serial/8250/Kconfig > >> index d7737dca0e48..21cb14cbd34a 100644 > >> --- a/drivers/tty/serial/8250/Kconfig > >> +++ b/drivers/tty/serial/8250/Kconfig > >> @@ -483,7 +483,7 @@ config SERIAL_8250_PXA > >> > >> config SERIAL_OF_PLATFORM > >> tristate "Devicetree based probing for 8250 ports" > >> - depends on SERIAL_8250 && OF > >> + depends on SERIAL_8250 && OF && !(PPC && PPC_UDBG_16550) > >> default SERIAL_8250 > >> help > >> This option is used for all 8250 compatible serial ports that > > > > 44x/virtex5_defconfig has both PPC_UDBG_16550 and SERIAL_OF_PLATFORM enabled > > and fails to boot (or display anything on the console) with this patch applied. > > Thanks for trying, can you either share or provide a link to the mpc85xx > and ml507 qemu command lines that you use? I spent a good chunk of my > time trying to get a kernel to boot but has failed so far. > Good to hear that this doesn't just happen to me ;-). The scripts are all at https://github.com/groeck/linux-build-test/. This includes root file systems. The one used below is at https://github.com/groeck/linux-build-test/blob/master/rootfs/ppc/rootfs.cpio.gz ml507: qemu-system-ppc -kernel vmlinux -M virtex-ml507 -m 256 -no-reboot \ -initrd rootfs.cpio -dtb arch/powerpc/boot/dts/virtex440-ml507.dtb \ --append 'rdinit=/sbin/init panic=-1 mem=256M console=ttyS0' \ -monitor none -nographic mpc85xx: qemu-system-ppc -kernel arch/powerpc/boot/uImage -M mpc8544ds -m 256 \ -no-reboot -initrd rootfs.cpio \ --append 'rdinit=/sbin/init panic=-1 mem=256M console=ttyS0' \ -monitor none -nographic Hope this helps, Guenter ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-19 20:50 ` Guenter Roeck @ 2018-11-23 18:20 ` Guenter Roeck 2018-11-27 0:08 ` Florian Fainelli 0 siblings, 1 reply; 20+ messages in thread From: Guenter Roeck @ 2018-11-23 18:20 UTC (permalink / raw) To: Florian Fainelli; +Cc: arnd, Greg Kroah-Hartman, linux-kernel On Mon, Nov 19, 2018 at 12:50:50PM -0800, Guenter Roeck wrote: > On Mon, Nov 19, 2018 at 10:44:30AM -0800, Florian Fainelli wrote: > > On 11/15/18 5:16 PM, Guenter Roeck wrote: > > > On Thu, Nov 15, 2018 at 11:48:20AM -0800, Florian Fainelli wrote: > > >> > > >> OK, would you mind testing this below? It seems to me that 8250_of.c is > > >> incompatible with arch/powerpc/kernel/legacy_serial.c and that is what > > >> is causing the issue here. > > >> > > >> diff --git a/drivers/tty/serial/8250/Kconfig > > >> b/drivers/tty/serial/8250/Kconfig > > >> index d7737dca0e48..21cb14cbd34a 100644 > > >> --- a/drivers/tty/serial/8250/Kconfig > > >> +++ b/drivers/tty/serial/8250/Kconfig > > >> @@ -483,7 +483,7 @@ config SERIAL_8250_PXA > > >> > > >> config SERIAL_OF_PLATFORM > > >> tristate "Devicetree based probing for 8250 ports" > > >> - depends on SERIAL_8250 && OF > > >> + depends on SERIAL_8250 && OF && !(PPC && PPC_UDBG_16550) > > >> default SERIAL_8250 > > >> help > > >> This option is used for all 8250 compatible serial ports that > > > > > > 44x/virtex5_defconfig has both PPC_UDBG_16550 and SERIAL_OF_PLATFORM enabled > > > and fails to boot (or display anything on the console) with this patch applied. > > > > Thanks for trying, can you either share or provide a link to the mpc85xx > > and ml507 qemu command lines that you use? I spent a good chunk of my > > time trying to get a kernel to boot but has failed so far. > > > Any update ? I still see the boot failures in next-20181123. Guenter > Good to hear that this doesn't just happen to me ;-). > > The scripts are all at https://github.com/groeck/linux-build-test/. > This includes root file systems. The one used below is at > https://github.com/groeck/linux-build-test/blob/master/rootfs/ppc/rootfs.cpio.gz > > ml507: > > qemu-system-ppc -kernel vmlinux -M virtex-ml507 -m 256 -no-reboot \ > -initrd rootfs.cpio -dtb arch/powerpc/boot/dts/virtex440-ml507.dtb \ > --append 'rdinit=/sbin/init panic=-1 mem=256M console=ttyS0' \ > -monitor none -nographic > > mpc85xx: > > qemu-system-ppc -kernel arch/powerpc/boot/uImage -M mpc8544ds -m 256 \ > -no-reboot -initrd rootfs.cpio \ > --append 'rdinit=/sbin/init panic=-1 mem=256M console=ttyS0' \ > -monitor none -nographic > > Hope this helps, > Guenter ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-23 18:20 ` Guenter Roeck @ 2018-11-27 0:08 ` Florian Fainelli 2018-12-05 5:47 ` Michael Ellerman 0 siblings, 1 reply; 20+ messages in thread From: Florian Fainelli @ 2018-11-27 0:08 UTC (permalink / raw) To: Guenter Roeck Cc: arnd, Greg Kroah-Hartman, linux-kernel, benh, paulus, mpe, linuxppc-dev +PPC folks On 11/23/18 10:20 AM, Guenter Roeck wrote: > On Mon, Nov 19, 2018 at 12:50:50PM -0800, Guenter Roeck wrote: >> On Mon, Nov 19, 2018 at 10:44:30AM -0800, Florian Fainelli wrote: >>> On 11/15/18 5:16 PM, Guenter Roeck wrote: >>>> On Thu, Nov 15, 2018 at 11:48:20AM -0800, Florian Fainelli wrote: >>>>> >>>>> OK, would you mind testing this below? It seems to me that 8250_of.c is >>>>> incompatible with arch/powerpc/kernel/legacy_serial.c and that is what >>>>> is causing the issue here. >>>>> >>>>> diff --git a/drivers/tty/serial/8250/Kconfig >>>>> b/drivers/tty/serial/8250/Kconfig >>>>> index d7737dca0e48..21cb14cbd34a 100644 >>>>> --- a/drivers/tty/serial/8250/Kconfig >>>>> +++ b/drivers/tty/serial/8250/Kconfig >>>>> @@ -483,7 +483,7 @@ config SERIAL_8250_PXA >>>>> >>>>> config SERIAL_OF_PLATFORM >>>>> tristate "Devicetree based probing for 8250 ports" >>>>> - depends on SERIAL_8250 && OF >>>>> + depends on SERIAL_8250 && OF && !(PPC && PPC_UDBG_16550) >>>>> default SERIAL_8250 >>>>> help >>>>> This option is used for all 8250 compatible serial ports that >>>> >>>> 44x/virtex5_defconfig has both PPC_UDBG_16550 and SERIAL_OF_PLATFORM enabled >>>> and fails to boot (or display anything on the console) with this patch applied. >>> >>> Thanks for trying, can you either share or provide a link to the mpc85xx >>> and ml507 qemu command lines that you use? I spent a good chunk of my >>> time trying to get a kernel to boot but has failed so far. >>> >> > > Any update ? I still see the boot failures in next-20181123. Yes, took most of last week's off sorry for the delay. I have finally been able to boot a kernel using your instructions, thanks. The problem is the following call chain: of_platform_serial_probe() -> serial8250_register_8250_port() -> up->port.uartclk == 0, return -EINVAL -> irq_dispose_mapping() and then we basically unwind what we just did with of_platform_serial_probe() including disabling the UART's IRQ, comment out the irq_dispose_mapping() and you will have a functional console again. 8250_of.c is clearly not a full replacement for legacy_serial.c (that was a first attempt), so we need to keep that code around. Making the depends even more conditional also does not sound too appealing, because while we have identified mpc85xx as being problematic, who knows about other platforms. So the best I have that does not involve a revert is this below. Ben, Michael, would that sound reasonable to you? It seems to me that there is a million ways to shoot the legacy_serial 8250 registration in the foot in any cases, and having CONFIG_SERIAL_OF_PLATFORM just made it painfully obvious. diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c index 33b34a58fc62..31353a27d714 100644 --- a/arch/powerpc/kernel/legacy_serial.c +++ b/arch/powerpc/kernel/legacy_serial.c @@ -16,7 +16,7 @@ #include <asm/pci-bridge.h> #include <asm/ppc-pci.h> -#undef DEBUG +#define DEBUG #ifdef DEBUG #define DBG(fmt...) do { printk(fmt); } while(0) @@ -70,6 +70,29 @@ static void tsi_serial_out(struct uart_port *p, int offset, int value) writeb(value, p->membase + offset); } +#ifdef CONFIG_SERIAL_OF_PLATFORM +static struct property uart_prop = { + .value = "disabled", + .name = "status", + .length = strlen("disabled"), +}; + +static void __init disable_uart_node(struct device_node *np) +{ + /* To avoid having 8250_of.c attempt to register the same device + * and failing to do, and calling irq_dispose_mapping(), just + * disable the device_node for now. + */ + of_update_property(np, &uart_prop); + pr_info("%s: disabled UART node\n", __func__); +} +#else +static inline int disable_uart_node(struct device_node *np) +{ + return 0; +} +#endif + static int __init add_legacy_port(struct device_node *np, int want_index, int iotype, phys_addr_t base, phys_addr_t taddr, unsigned long irq, @@ -80,6 +103,8 @@ static int __init add_legacy_port(struct device_node *np, int want_index, u32 shift = 0; int index; + disable_uart_node(np); + /* get clock freq. if present */ clk = of_get_property(np, "clock-frequency", NULL); if (clk && *clk) -- Florian ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-27 0:08 ` Florian Fainelli @ 2018-12-05 5:47 ` Michael Ellerman 2018-12-05 22:40 ` Florian Fainelli 0 siblings, 1 reply; 20+ messages in thread From: Michael Ellerman @ 2018-12-05 5:47 UTC (permalink / raw) To: Florian Fainelli, Guenter Roeck Cc: arnd, Greg Kroah-Hartman, linux-kernel, benh, paulus, linuxppc-dev Hi Florian, Florian Fainelli <f.fainelli@gmail.com> writes: > +PPC folks > > On 11/23/18 10:20 AM, Guenter Roeck wrote: >> On Mon, Nov 19, 2018 at 12:50:50PM -0800, Guenter Roeck wrote: >>> On Mon, Nov 19, 2018 at 10:44:30AM -0800, Florian Fainelli wrote: >>>> On 11/15/18 5:16 PM, Guenter Roeck wrote: >>>>> On Thu, Nov 15, 2018 at 11:48:20AM -0800, Florian Fainelli wrote: >>>>>> >>>>>> OK, would you mind testing this below? It seems to me that 8250_of.c is >>>>>> incompatible with arch/powerpc/kernel/legacy_serial.c and that is what >>>>>> is causing the issue here. >>>>>> >>>>>> diff --git a/drivers/tty/serial/8250/Kconfig >>>>>> b/drivers/tty/serial/8250/Kconfig >>>>>> index d7737dca0e48..21cb14cbd34a 100644 >>>>>> --- a/drivers/tty/serial/8250/Kconfig >>>>>> +++ b/drivers/tty/serial/8250/Kconfig >>>>>> @@ -483,7 +483,7 @@ config SERIAL_8250_PXA >>>>>> >>>>>> config SERIAL_OF_PLATFORM >>>>>> tristate "Devicetree based probing for 8250 ports" >>>>>> - depends on SERIAL_8250 && OF >>>>>> + depends on SERIAL_8250 && OF && !(PPC && PPC_UDBG_16550) >>>>>> default SERIAL_8250 >>>>>> help >>>>>> This option is used for all 8250 compatible serial ports that >>>>> >>>>> 44x/virtex5_defconfig has both PPC_UDBG_16550 and SERIAL_OF_PLATFORM enabled >>>>> and fails to boot (or display anything on the console) with this patch applied. >>>> >>>> Thanks for trying, can you either share or provide a link to the mpc85xx >>>> and ml507 qemu command lines that you use? I spent a good chunk of my >>>> time trying to get a kernel to boot but has failed so far. >>>> >>> >> >> Any update ? I still see the boot failures in next-20181123. > > Yes, took most of last week's off sorry for the delay. I have finally > been able to boot a kernel using your instructions, thanks. The problem > is the following call chain: > > of_platform_serial_probe() > -> serial8250_register_8250_port() > -> up->port.uartclk == 0, return -EINVAL > -> irq_dispose_mapping() > > and then we basically unwind what we just did with > of_platform_serial_probe() including disabling the UART's IRQ, comment > out the irq_dispose_mapping() and you will have a functional console > again. 8250_of.c is clearly not a full replacement for legacy_serial.c > (that was a first attempt), so we need to keep that code around. Making > the depends even more conditional also does not sound too appealing, > because while we have identified mpc85xx as being problematic, who knows > about other platforms. So the best I have that does not involve a revert > is this below. > > Ben, Michael, would that sound reasonable to you? It seems to me that > there is a million ways to shoot the legacy_serial 8250 registration in > the foot in any cases, and having CONFIG_SERIAL_OF_PLATFORM just made it > painfully obvious. We generally try to avoid changing the device tree from inside Linux, it's meant to describe the hardware as we're given it, not the state of Linux drivers etc. Perhaps this is an exceptional case but it still seems fishy. I also worry that marking the device node disabled might break something else, but that's maybe me being paranoid. I guess I don't really understand how things are supposed to work though. We have the 8250 driver and also the OF platform driver, the former has already setup the device and then the OF driver just comes along and takes over? eg. in the boot log from the mpc8544ds you see: serial8250.0: ttyS0 at MMIO 0xe0004500 (irq = 42, base_baud = 115200) is a 16550A printk: console [ttyS0] enabled printk: console [ttyS0] enabled printk: bootconsole [udbg0] disabled printk: bootconsole [udbg0] disabled of_serial: probe of e0004500.serial failed with error -22 ie. the 8250 core has already setup the device, so there should really be nothing to do? If the 8250 code could detect that it already has this port registered then maybe things would work. The patch below works for me with mpc8544ds. Whether it's worth changing the 8250 code this much to accomodate legacy_serial I'm not sure. cheers diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c index 94f3e1c64490..c7d7858fdb3d 100644 --- a/drivers/tty/serial/8250/8250_core.c +++ b/drivers/tty/serial/8250/8250_core.c @@ -905,7 +905,7 @@ static struct platform_device *serial8250_isa_devs; */ static DEFINE_MUTEX(serial_mutex); -static struct uart_8250_port *serial8250_find_match_or_unused(struct uart_port *port) +static struct uart_8250_port *serial8250_find_existing(struct uart_port *port) { int i; @@ -921,10 +921,17 @@ static struct uart_8250_port *serial8250_find_match_or_unused(struct uart_port * if (i < nr_uarts && serial8250_ports[i].port.type == PORT_UNKNOWN && serial8250_ports[i].port.iobase == 0) return &serial8250_ports[i]; + + return NULL; +} + +static struct uart_8250_port *serial8250_find_unused(struct uart_port *port) +{ + int i; + /* - * We didn't find a matching entry, so look for the first - * free entry. We look for one which hasn't been previously - * used (indicated by zero iobase). + * Look for the first free entry. We look for one which hasn't been + * previously used (indicated by zero iobase). */ for (i = 0; i < nr_uarts; i++) if (serial8250_ports[i].port.type == PORT_UNKNOWN && @@ -960,12 +967,18 @@ int serial8250_register_8250_port(struct uart_8250_port *up) struct uart_8250_port *uart; int ret = -ENOSPC; - if (up->port.uartclk == 0) - return -EINVAL; - mutex_lock(&serial_mutex); - uart = serial8250_find_match_or_unused(&up->port); + uart = serial8250_find_existing(&up->port); + if (!uart) { + if (up->port.uartclk == 0) { + ret = -EINVAL; + goto out; + } + + uart = serial8250_find_unused(&up->port); + } + if (uart && uart->port.type != PORT_8250_CIR) { if (uart->port.dev) uart_remove_one_port(&serial8250_reg, &uart->port); @@ -974,7 +987,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up) uart->port.membase = up->port.membase; uart->port.irq = up->port.irq; uart->port.irqflags = up->port.irqflags; - uart->port.uartclk = up->port.uartclk; + + if (up->port.uartclk != 0) + uart->port.uartclk = up->port.uartclk; + uart->port.fifosize = up->port.fifosize; uart->port.regshift = up->port.regshift; uart->port.iotype = up->port.iotype; @@ -1056,6 +1072,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up) ret = 0; } } +out: mutex_unlock(&serial_mutex); return ret; ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-12-05 5:47 ` Michael Ellerman @ 2018-12-05 22:40 ` Florian Fainelli 0 siblings, 0 replies; 20+ messages in thread From: Florian Fainelli @ 2018-12-05 22:40 UTC (permalink / raw) To: Michael Ellerman, Guenter Roeck Cc: arnd, Greg Kroah-Hartman, linux-kernel, benh, paulus, linuxppc-dev On 12/4/18 9:47 PM, Michael Ellerman wrote: > Hi Florian, > > Florian Fainelli <f.fainelli@gmail.com> writes: >> +PPC folks >> >> On 11/23/18 10:20 AM, Guenter Roeck wrote: >>> On Mon, Nov 19, 2018 at 12:50:50PM -0800, Guenter Roeck wrote: >>>> On Mon, Nov 19, 2018 at 10:44:30AM -0800, Florian Fainelli wrote: >>>>> On 11/15/18 5:16 PM, Guenter Roeck wrote: >>>>>> On Thu, Nov 15, 2018 at 11:48:20AM -0800, Florian Fainelli wrote: >>>>>>> >>>>>>> OK, would you mind testing this below? It seems to me that 8250_of.c is >>>>>>> incompatible with arch/powerpc/kernel/legacy_serial.c and that is what >>>>>>> is causing the issue here. >>>>>>> >>>>>>> diff --git a/drivers/tty/serial/8250/Kconfig >>>>>>> b/drivers/tty/serial/8250/Kconfig >>>>>>> index d7737dca0e48..21cb14cbd34a 100644 >>>>>>> --- a/drivers/tty/serial/8250/Kconfig >>>>>>> +++ b/drivers/tty/serial/8250/Kconfig >>>>>>> @@ -483,7 +483,7 @@ config SERIAL_8250_PXA >>>>>>> >>>>>>> config SERIAL_OF_PLATFORM >>>>>>> tristate "Devicetree based probing for 8250 ports" >>>>>>> - depends on SERIAL_8250 && OF >>>>>>> + depends on SERIAL_8250 && OF && !(PPC && PPC_UDBG_16550) >>>>>>> default SERIAL_8250 >>>>>>> help >>>>>>> This option is used for all 8250 compatible serial ports that >>>>>> >>>>>> 44x/virtex5_defconfig has both PPC_UDBG_16550 and SERIAL_OF_PLATFORM enabled >>>>>> and fails to boot (or display anything on the console) with this patch applied. >>>>> >>>>> Thanks for trying, can you either share or provide a link to the mpc85xx >>>>> and ml507 qemu command lines that you use? I spent a good chunk of my >>>>> time trying to get a kernel to boot but has failed so far. >>>>> >>>> >>> >>> Any update ? I still see the boot failures in next-20181123. >> >> Yes, took most of last week's off sorry for the delay. I have finally >> been able to boot a kernel using your instructions, thanks. The problem >> is the following call chain: >> >> of_platform_serial_probe() >> -> serial8250_register_8250_port() >> -> up->port.uartclk == 0, return -EINVAL >> -> irq_dispose_mapping() >> >> and then we basically unwind what we just did with >> of_platform_serial_probe() including disabling the UART's IRQ, comment >> out the irq_dispose_mapping() and you will have a functional console >> again. 8250_of.c is clearly not a full replacement for legacy_serial.c >> (that was a first attempt), so we need to keep that code around. Making >> the depends even more conditional also does not sound too appealing, >> because while we have identified mpc85xx as being problematic, who knows >> about other platforms. So the best I have that does not involve a revert >> is this below. >> >> Ben, Michael, would that sound reasonable to you? It seems to me that >> there is a million ways to shoot the legacy_serial 8250 registration in >> the foot in any cases, and having CONFIG_SERIAL_OF_PLATFORM just made it >> painfully obvious. > > We generally try to avoid changing the device tree from inside Linux, > it's meant to describe the hardware as we're given it, not the state of > Linux drivers etc. Perhaps this is an exceptional case but it still > seems fishy. Technically this is changing the Linux internal representation of a Device Tree node reference (device_node), which is a bit away from actual HW representation. > > I also worry that marking the device node disabled might break something > else, but that's maybe me being paranoid. The code as it is today only makes use of device_node pointers/Device Tree to find out about the UART base address and a bunch of properties, but it registers the UART as platform_device without making use of the existing OF infrastructure to do that (historical reasons really). > > I guess I don't really understand how things are supposed to work > though. We have the 8250 driver and also the OF platform driver, the > former has already setup the device and then the OF driver just comes > along and takes over? When the OF platform driver comes along it tries to register the same device instance, fails to do so because the port->clk is 0, which the 8250 core thinks is an error, and then it starts unwinding the 8250 port structure, which in turns steps on the instance that was registered manually by legacy_serial.c > > eg. in the boot log from the mpc8544ds you see: > > serial8250.0: ttyS0 at MMIO 0xe0004500 (irq = 42, base_baud = 115200) is a 16550A > printk: console [ttyS0] enabled > printk: console [ttyS0] enabled > printk: bootconsole [udbg0] disabled > printk: bootconsole [udbg0] disabled > of_serial: probe of e0004500.serial failed with error -22 > > ie. the 8250 core has already setup the device, so there should really > be nothing to do? > > If the 8250 code could detect that it already has this port registered > then maybe things would work. > > The patch below works for me with mpc8544ds. Whether it's worth changing > the 8250 code this much to accomodate legacy_serial I'm not sure. Not sure either, that was the route I initially took, but I was worried about the impact and the inability to test that across a variety of systems to make sure we would not be breaking 8250, which is really ubiquitous. Your patch looks very reasonable though, so this might be an appropriate course of action to take. It's not like we are supposed to do what is happening here :) The change in legacy_serial.c was self contained an while a hack, which quite frankly, that whole file is as well, did the job ;) > > cheers > > > diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c > index 94f3e1c64490..c7d7858fdb3d 100644 > --- a/drivers/tty/serial/8250/8250_core.c > +++ b/drivers/tty/serial/8250/8250_core.c > @@ -905,7 +905,7 @@ static struct platform_device *serial8250_isa_devs; > */ > static DEFINE_MUTEX(serial_mutex); > > -static struct uart_8250_port *serial8250_find_match_or_unused(struct uart_port *port) > +static struct uart_8250_port *serial8250_find_existing(struct uart_port *port) > { > int i; > > @@ -921,10 +921,17 @@ static struct uart_8250_port *serial8250_find_match_or_unused(struct uart_port * > if (i < nr_uarts && serial8250_ports[i].port.type == PORT_UNKNOWN && > serial8250_ports[i].port.iobase == 0) > return &serial8250_ports[i]; > + > + return NULL; > +} > + > +static struct uart_8250_port *serial8250_find_unused(struct uart_port *port) > +{ > + int i; > + > /* > - * We didn't find a matching entry, so look for the first > - * free entry. We look for one which hasn't been previously > - * used (indicated by zero iobase). > + * Look for the first free entry. We look for one which hasn't been > + * previously used (indicated by zero iobase). > */ > for (i = 0; i < nr_uarts; i++) > if (serial8250_ports[i].port.type == PORT_UNKNOWN && > @@ -960,12 +967,18 @@ int serial8250_register_8250_port(struct uart_8250_port *up) > struct uart_8250_port *uart; > int ret = -ENOSPC; > > - if (up->port.uartclk == 0) > - return -EINVAL; > - > mutex_lock(&serial_mutex); > > - uart = serial8250_find_match_or_unused(&up->port); > + uart = serial8250_find_existing(&up->port); > + if (!uart) { > + if (up->port.uartclk == 0) { > + ret = -EINVAL; > + goto out; > + } > + > + uart = serial8250_find_unused(&up->port); > + } > + > if (uart && uart->port.type != PORT_8250_CIR) { > if (uart->port.dev) > uart_remove_one_port(&serial8250_reg, &uart->port); > @@ -974,7 +987,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up) > uart->port.membase = up->port.membase; > uart->port.irq = up->port.irq; > uart->port.irqflags = up->port.irqflags; > - uart->port.uartclk = up->port.uartclk; > + > + if (up->port.uartclk != 0) > + uart->port.uartclk = up->port.uartclk; > + > uart->port.fifosize = up->port.fifosize; > uart->port.regshift = up->port.regshift; > uart->port.iotype = up->port.iotype; > @@ -1056,6 +1072,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up) > ret = 0; > } > } > +out: > mutex_unlock(&serial_mutex); > > return ret; > -- Florian ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-11-15 1:11 [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 Guenter Roeck 2018-11-15 3:56 ` Florian Fainelli @ 2018-12-20 15:21 ` Greg Kroah-Hartman 2018-12-20 17:38 ` Guenter Roeck 1 sibling, 1 reply; 20+ messages in thread From: Greg Kroah-Hartman @ 2018-12-20 15:21 UTC (permalink / raw) To: Guenter Roeck; +Cc: Florian Fainelli, linux-kernel On Wed, Nov 14, 2018 at 05:11:25PM -0800, Guenter Roeck wrote: > On Thu, Nov 01, 2018 at 11:26:06AM -0700, Florian Fainelli wrote: > > It is way too easy to miss enabling SERIAL_OF_PLATFORM which would > > result in the inability for the kernel to have a valid console device, > > which can be seen with: > > > > Warning: unable to open an initial console. > > > > and then: > > > > Run /init as init process > > Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000100 > > > > Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there > > really is no drawback to defaulting this config to the value of > > SERIAL_8250. > > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > This patch results in situations where CONFIG_SERIAL_OF_PLATFORM is now > defined where it was not previously. Example mpc85xx_defconfig. This in > turn results in boot failures for those configurations, with an error > message of > > of_serial: probe of e0004500.serial failed with error -22 > > which wasn't seen before. > > Not sure if replacing a potential problem with a real one is really an > improvement.` What ever was the result of this long thread? Should I revert something? Or was a patch proposed? totally lost, greg k-h ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-12-20 15:21 ` Greg Kroah-Hartman @ 2018-12-20 17:38 ` Guenter Roeck 2018-12-21 4:27 ` [PATCH] Revert "serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250" Florian Fainelli 2018-12-21 4:28 ` [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 Florian Fainelli 0 siblings, 2 replies; 20+ messages in thread From: Guenter Roeck @ 2018-12-20 17:38 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Florian Fainelli, linux-kernel On Thu, Dec 20, 2018 at 04:21:11PM +0100, Greg Kroah-Hartman wrote: > On Wed, Nov 14, 2018 at 05:11:25PM -0800, Guenter Roeck wrote: > > On Thu, Nov 01, 2018 at 11:26:06AM -0700, Florian Fainelli wrote: > > > It is way too easy to miss enabling SERIAL_OF_PLATFORM which would > > > result in the inability for the kernel to have a valid console device, > > > which can be seen with: > > > > > > Warning: unable to open an initial console. > > > > > > and then: > > > > > > Run /init as init process > > > Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000100 > > > > > > Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there > > > really is no drawback to defaulting this config to the value of > > > SERIAL_8250. > > > > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > This patch results in situations where CONFIG_SERIAL_OF_PLATFORM is now > > defined where it was not previously. Example mpc85xx_defconfig. This in > > turn results in boot failures for those configurations, with an error > > message of > > > > of_serial: probe of e0004500.serial failed with error -22 > > > > which wasn't seen before. > > > > Not sure if replacing a potential problem with a real one is really an > > improvement.` > > What ever was the result of this long thread? Should I revert > something? Or was a patch proposed? > The problem still exists in next-20181220. Unfortunately this is now just one failure of many in -next. I see more than 90 boot failures (out of ~330) there, not counting the build failures. And that is on a good day. Guenter ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] Revert "serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250" 2018-12-20 17:38 ` Guenter Roeck @ 2018-12-21 4:27 ` Florian Fainelli 2018-12-21 4:28 ` [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 Florian Fainelli 1 sibling, 0 replies; 20+ messages in thread From: Florian Fainelli @ 2018-12-21 4:27 UTC (permalink / raw) To: linux-vger Cc: linux-serial, mpe, arnd, gregkh, linux, benh, paulus, linuxppc-dev, Florian Fainelli, Jiri Slaby, Nishanth Menon, Tony Lindgren, Vignesh R, Lokesh Vutla, Michael Moese, open list This reverts commit 6d11023c345e369bcb9d5a68b271764e362c1f6e ("serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250") since that breaks at least mpc8544ds (PowerPC) using arch/powerpc/kernel/legacy_serial.c. See https://lkml.org/lkml/2018/12/5/1491 for discussion and analysis Fixes: 6d11023c345e ("serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/tty/serial/8250/Kconfig | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig index d7737dca0e48..15c2c5463835 100644 --- a/drivers/tty/serial/8250/Kconfig +++ b/drivers/tty/serial/8250/Kconfig @@ -484,7 +484,6 @@ config SERIAL_8250_PXA config SERIAL_OF_PLATFORM tristate "Devicetree based probing for 8250 ports" depends on SERIAL_8250 && OF - default SERIAL_8250 help This option is used for all 8250 compatible serial ports that are probed through devicetree, including Open Firmware based -- 2.19.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 2018-12-20 17:38 ` Guenter Roeck 2018-12-21 4:27 ` [PATCH] Revert "serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250" Florian Fainelli @ 2018-12-21 4:28 ` Florian Fainelli 1 sibling, 0 replies; 20+ messages in thread From: Florian Fainelli @ 2018-12-21 4:28 UTC (permalink / raw) To: Guenter Roeck, Greg Kroah-Hartman Cc: linux-kernel, Arnd Bergmann, Benjamin Herrenschmidt, Michael Ellerman, linuxppc-dev Le 12/20/18 à 9:38 AM, Guenter Roeck a écrit : > On Thu, Dec 20, 2018 at 04:21:11PM +0100, Greg Kroah-Hartman wrote: >> On Wed, Nov 14, 2018 at 05:11:25PM -0800, Guenter Roeck wrote: >>> On Thu, Nov 01, 2018 at 11:26:06AM -0700, Florian Fainelli wrote: >>>> It is way too easy to miss enabling SERIAL_OF_PLATFORM which would >>>> result in the inability for the kernel to have a valid console device, >>>> which can be seen with: >>>> >>>> Warning: unable to open an initial console. >>>> >>>> and then: >>>> >>>> Run /init as init process >>>> Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000100 >>>> >>>> Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there >>>> really is no drawback to defaulting this config to the value of >>>> SERIAL_8250. >>>> >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> >>> This patch results in situations where CONFIG_SERIAL_OF_PLATFORM is now >>> defined where it was not previously. Example mpc85xx_defconfig. This in >>> turn results in boot failures for those configurations, with an error >>> message of >>> >>> of_serial: probe of e0004500.serial failed with error -22 >>> >>> which wasn't seen before. >>> >>> Not sure if replacing a potential problem with a real one is really an >>> improvement.` >> >> What ever was the result of this long thread? Should I revert >> something? Or was a patch proposed? >> > The problem still exists in next-20181220. I submitted a tentative patch to fix the problem, discussed it with Michael and he had an alternative patch to 8250_core.c that should work equally well though I was worried more breakage could be created that way. Since clearly we have not been able to make much progress, maybe a reversion of the original patch is appropriate, yes it's now sent a as a reply to this mail! > > Unfortunately this is now just one failure of many in -next. I see more > than 90 boot failures (out of ~330) there, not counting the build failures. > And that is on a good day. > > Guenter > -- Florian ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 @ 2018-11-01 18:26 Florian Fainelli 0 siblings, 0 replies; 20+ messages in thread From: Florian Fainelli @ 2018-11-01 18:26 UTC (permalink / raw) To: linux-kernel Cc: arndb, Florian Fainelli, Greg Kroah-Hartman, Jiri Slaby, Nishanth Menon, Hendrik Brueckner, Christian Borntraeger, Michael Moese, Lokesh Vutla, open list:SERIAL DRIVERS It is way too easy to miss enabling SERIAL_OF_PLATFORM which would result in the inability for the kernel to have a valid console device, which can be seen with: Warning: unable to open an initial console. and then: Run /init as init process Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000100 Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there really is no drawback to defaulting this config to the value of SERIAL_8250. Signed-off-by: Florian Fainelli <f.fainelli@gmail.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 15c2c5463835..d7737dca0e48 100644 --- a/drivers/tty/serial/8250/Kconfig +++ b/drivers/tty/serial/8250/Kconfig @@ -484,6 +484,7 @@ config SERIAL_8250_PXA config SERIAL_OF_PLATFORM tristate "Devicetree based probing for 8250 ports" depends on SERIAL_8250 && OF + default SERIAL_8250 help This option is used for all 8250 compatible serial ports that are probed through devicetree, including Open Firmware based -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-12-21 4:28 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-15 1:11 [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 Guenter Roeck 2018-11-15 3:56 ` Florian Fainelli 2018-11-15 5:36 ` Guenter Roeck 2018-11-15 17:19 ` Florian Fainelli 2018-11-15 5:38 ` Guenter Roeck 2018-11-15 17:19 ` Florian Fainelli 2018-11-15 17:25 ` Guenter Roeck 2018-11-15 19:48 ` Florian Fainelli 2018-11-16 1:16 ` Guenter Roeck 2018-11-19 18:44 ` Florian Fainelli 2018-11-19 20:50 ` Guenter Roeck 2018-11-23 18:20 ` Guenter Roeck 2018-11-27 0:08 ` Florian Fainelli 2018-12-05 5:47 ` Michael Ellerman 2018-12-05 22:40 ` Florian Fainelli 2018-12-20 15:21 ` Greg Kroah-Hartman 2018-12-20 17:38 ` Guenter Roeck 2018-12-21 4:27 ` [PATCH] Revert "serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250" Florian Fainelli 2018-12-21 4:28 ` [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250 Florian Fainelli -- strict thread matches above, loose matches on Subject: below -- 2018-11-01 18:26 Florian Fainelli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).