[PATCH v3 0/2] serial: 8250: Apply FSL workarounds also without SERIAL_8250_CONSOLE

[PATCH v3 0/2] serial: 8250: Apply FSL workarounds also without SERIAL_8250_CONSOLE

[Uwe Kleine-König]

Hello,

this is v3 of the series that now also copes for
arch/powerpc/kernel/legacy_serial.c using fsl8250_handle_irq().

For kernel configurations that already before were correctly using the
Freescale workarounds, this is the case with this series applied, too.
So in all cases the situation doesn't get worse. The upside is that even
with the 8250 driver compiled as a module (or built-in but without
console support) the workarounds are now applied for all devices but for
the ones instantiated in arch/powerpc/kernel/legacy_serial.c. (And even
for these there might not be a problem as they might benefit from
enabling the workarounds in drivers/tty/serial/8250/8250_of.c. Not sure
though.)

Patch #1 is new here. Patch #2 only changed lightly: I restored
alphabetic order in drivers/tty/serial/8250/Kconfig.

As patch #1 is needed for patch #2 to not introduce a build failure,
both patches should be taken together. I suggest to add them to Greg's
serial tree, but the changes pending there should not conflict with this
series such that taking them both via powerpc works, too.

Best regards
Uwe

Uwe Kleine-König (2):
  powerpc/legacy_serial: Warn about 8250 devices operated without active
    FSL workarounds
  serial: 8250: Apply FSL workarounds also without SERIAL_8250_CONSOLE

 arch/powerpc/kernel/legacy_serial.c | 14 +++++++++-----
 drivers/tty/serial/8250/Kconfig     |  2 +-
 drivers/tty/serial/8250/Makefile    |  2 +-
 3 files changed, 11 insertions(+), 7 deletions(-)


base-commit: ac9a78681b921877518763ba0e89202254349d1b
-- 
2.39.2

[PATCH v3 1/2] powerpc/legacy_serial: Warn about 8250 devices operated without active FSL workarounds

[Uwe Kleine-König]

If the 8250 driver is built as a module (or built-in without console
support) the Freescale specific workaround were silently not activated.
Add a warning in this case.

Currently CONFIG_SERIAL_8250_FSL=y implies that the function
fsl8250_handle_irq() is built-in and can be used. However with the
changes of the next commit CONFIG_SERIAL_8250_FSL might be enabled also
when the 8250 driver is a module and so more care is needed when
fsl8250_handle_irq() is to be used. The code added here is able to
handle the new situation already.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/powerpc/kernel/legacy_serial.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
index c9ad12461d44..fdbd85aafeb1 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c
@@ -508,12 +508,16 @@ static void __init fixup_port_irq(int index,
 
 	port->irq = virq;
 
-#ifdef CONFIG_SERIAL_8250_FSL
-	if (of_device_is_compatible(np, "fsl,ns16550")) {
-		port->handle_irq = fsl8250_handle_irq;
-		port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
+	if (IS_ENABLED(CONFIG_SERIAL_8250) &&
+	    of_device_is_compatible(np, "fsl,ns16550")) {
+		if (IS_REACHABLE(CONFIG_SERIAL_8250)) {
+			port->handle_irq = fsl8250_handle_irq;
+			port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
+		} else {
+			pr_warn_once("Not activating Freescale specific workaround for device %pOFP\n",
+				     np);
+		}
 	}
-#endif
 }
 
 static void __init fixup_port_pio(int index,
-- 
2.39.2

[PATCH v3 2/2] serial: 8250: Apply FSL workarounds also without SERIAL_8250_CONSOLE

[Uwe Kleine-König]

The need to handle the FSL variant of 8250 in a special way is also
present without console support. So soften the dependency for
SERIAL_8250_FSL accordingly. Note that with the 8250 driver compiled as
a module, some devices still might not make use of the needed
workarounds. That affects the ports instantiated in
arch/powerpc/kernel/legacy_serial.c.

This issue was identified by Dominik Andreas Schorpp.

To cope for CONFIG_SERIAL_8250=m + CONFIG_SERIAL_8250_FSL=y, 8250_fsl.o
must be put in the same compilation unit as 8250_port.o because the
latter defines some functions needed in the former and so 8250_fsl.o
must not be built-in if 8250_port.o is available in a module.

Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Link: https://lore.kernel.org/r/20230531083230.2702181-1-u.kleine-koenig@pengutronix.de
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/serial/8250/Kconfig  | 2 +-
 drivers/tty/serial/8250/Makefile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 5313aa31930f..10c09b19c871 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -378,7 +378,7 @@ config SERIAL_8250_BCM2835AUX
 
 config SERIAL_8250_FSL
 	bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
-	depends on SERIAL_8250_CONSOLE
+	depends on SERIAL_8250
 	default PPC || ARM || ARM64
 	help
 	  Selecting this option enables a workaround for a break-detection
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 4fc2fc1f41b6..8824ba5295b6 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_SERIAL_8250)		+= 8250.o 8250_base.o
 8250_base-$(CONFIG_SERIAL_8250_DMA)	+= 8250_dma.o
 8250_base-$(CONFIG_SERIAL_8250_DWLIB)	+= 8250_dwlib.o
 8250_base-$(CONFIG_SERIAL_8250_FINTEK)	+= 8250_fintek.o
+8250_base-$(CONFIG_SERIAL_8250_FSL)	+= 8250_fsl.o
 8250_base-$(CONFIG_SERIAL_8250_PCILIB)	+= 8250_pcilib.o
 obj-$(CONFIG_SERIAL_8250_PARISC)	+= 8250_parisc.o
 obj-$(CONFIG_SERIAL_8250_PCI)		+= 8250_pci.o
@@ -28,7 +29,6 @@ obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
 obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
 obj-$(CONFIG_SERIAL_8250_PCI1XXXX)	+= 8250_pci1xxxx.o
-obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_MEN_MCB)	+= 8250_men_mcb.o
 obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
-- 
2.39.2

Re: [PATCH v3 2/2] serial: 8250: Apply FSL workarounds also without SERIAL_8250_CONSOLE

[Ilpo Järvinen]

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

On Mon, 5 Jun 2023, Uwe Kleine-König wrote:

> The need to handle the FSL variant of 8250 in a special way is also
> present without console support. So soften the dependency for
> SERIAL_8250_FSL accordingly. Note that with the 8250 driver compiled as
> a module, some devices still might not make use of the needed
> workarounds. That affects the ports instantiated in
> arch/powerpc/kernel/legacy_serial.c.
> 
> This issue was identified by Dominik Andreas Schorpp.
> 
> To cope for CONFIG_SERIAL_8250=m + CONFIG_SERIAL_8250_FSL=y, 8250_fsl.o
> must be put in the same compilation unit as 8250_port.o because the
> latter defines some functions needed in the former and so 8250_fsl.o
> must not be built-in if 8250_port.o is available in a module.
> 
> Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Link: https://lore.kernel.org/r/20230531083230.2702181-1-u.kleine-koenig@pengutronix.de
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/tty/serial/8250/Kconfig  | 2 +-
>  drivers/tty/serial/8250/Makefile | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 5313aa31930f..10c09b19c871 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -378,7 +378,7 @@ config SERIAL_8250_BCM2835AUX
>  
>  config SERIAL_8250_FSL
>  	bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
> -	depends on SERIAL_8250_CONSOLE
> +	depends on SERIAL_8250

Just one additional thought: After the adding the arch side 
workaround/hack, SERIAL_8250_FSL could become a tristate?

(1/2 might need a small change to take into account that it can now be a 
module).

>  	default PPC || ARM || ARM64
>  	help
>  	  Selecting this option enables a workaround for a break-detection
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index 4fc2fc1f41b6..8824ba5295b6 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_SERIAL_8250)		+= 8250.o 8250_base.o
>  8250_base-$(CONFIG_SERIAL_8250_DMA)	+= 8250_dma.o
>  8250_base-$(CONFIG_SERIAL_8250_DWLIB)	+= 8250_dwlib.o
>  8250_base-$(CONFIG_SERIAL_8250_FINTEK)	+= 8250_fintek.o
> +8250_base-$(CONFIG_SERIAL_8250_FSL)	+= 8250_fsl.o
>  8250_base-$(CONFIG_SERIAL_8250_PCILIB)	+= 8250_pcilib.o
>  obj-$(CONFIG_SERIAL_8250_PARISC)	+= 8250_parisc.o
>  obj-$(CONFIG_SERIAL_8250_PCI)		+= 8250_pci.o
> @@ -28,7 +29,6 @@ obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
>  obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
>  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
>  obj-$(CONFIG_SERIAL_8250_PCI1XXXX)	+= 8250_pci1xxxx.o
> -obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
>  obj-$(CONFIG_SERIAL_8250_MEN_MCB)	+= 8250_men_mcb.o
>  obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
>  obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o

-- 
 i.

Re: [PATCH v3 2/2] serial: 8250: Apply FSL workarounds also without SERIAL_8250_CONSOLE

[Uwe Kleine-König]

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

On Mon, Jun 05, 2023 at 04:22:55PM +0300, Ilpo Järvinen wrote:
> On Mon, 5 Jun 2023, Uwe Kleine-König wrote:
> 
> > The need to handle the FSL variant of 8250 in a special way is also
> > present without console support. So soften the dependency for
> > SERIAL_8250_FSL accordingly. Note that with the 8250 driver compiled as
> > a module, some devices still might not make use of the needed
> > workarounds. That affects the ports instantiated in
> > arch/powerpc/kernel/legacy_serial.c.
> > 
> > This issue was identified by Dominik Andreas Schorpp.
> > 
> > To cope for CONFIG_SERIAL_8250=m + CONFIG_SERIAL_8250_FSL=y, 8250_fsl.o
> > must be put in the same compilation unit as 8250_port.o because the
> > latter defines some functions needed in the former and so 8250_fsl.o
> > must not be built-in if 8250_port.o is available in a module.
> > 
> > Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Link: https://lore.kernel.org/r/20230531083230.2702181-1-u.kleine-koenig@pengutronix.de
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/tty/serial/8250/Kconfig  | 2 +-
> >  drivers/tty/serial/8250/Makefile | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > index 5313aa31930f..10c09b19c871 100644
> > --- a/drivers/tty/serial/8250/Kconfig
> > +++ b/drivers/tty/serial/8250/Kconfig
> > @@ -378,7 +378,7 @@ config SERIAL_8250_BCM2835AUX
> >  
> >  config SERIAL_8250_FSL
> >  	bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
> > -	depends on SERIAL_8250_CONSOLE
> > +	depends on SERIAL_8250
> 
> Just one additional thought: After the adding the arch side 
> workaround/hack, SERIAL_8250_FSL could become a tristate?

I see no benefit for a module separate from 8250_base.ko. There are
dependencies in both directions between 8250_port.o and 8250_fsl.o[1].
So in my book a bool SERIAL_8250_FSL that modifies 8250_base.ko (with
SERIAL_8250=m) is fine.

Best regards
Uwe

[1] 8250_port.o uses fsl8250_handle_irq() from 8250_fsl.o, and
8250_fsl.o uses serial8250_modem_status from 8250_port.o.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/2] serial: 8250: Apply FSL workarounds also without SERIAL_8250_CONSOLE

[Ilpo Järvinen]

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

On Mon, 5 Jun 2023, Uwe Kleine-König wrote:

> On Mon, Jun 05, 2023 at 04:22:55PM +0300, Ilpo Järvinen wrote:
> > On Mon, 5 Jun 2023, Uwe Kleine-König wrote:
> > 
> > > The need to handle the FSL variant of 8250 in a special way is also
> > > present without console support. So soften the dependency for
> > > SERIAL_8250_FSL accordingly. Note that with the 8250 driver compiled as
> > > a module, some devices still might not make use of the needed
> > > workarounds. That affects the ports instantiated in
> > > arch/powerpc/kernel/legacy_serial.c.
> > > 
> > > This issue was identified by Dominik Andreas Schorpp.
> > > 
> > > To cope for CONFIG_SERIAL_8250=m + CONFIG_SERIAL_8250_FSL=y, 8250_fsl.o
> > > must be put in the same compilation unit as 8250_port.o because the
> > > latter defines some functions needed in the former and so 8250_fsl.o
> > > must not be built-in if 8250_port.o is available in a module.
> > > 
> > > Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Link: https://lore.kernel.org/r/20230531083230.2702181-1-u.kleine-koenig@pengutronix.de
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/tty/serial/8250/Kconfig  | 2 +-
> > >  drivers/tty/serial/8250/Makefile | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > > index 5313aa31930f..10c09b19c871 100644
> > > --- a/drivers/tty/serial/8250/Kconfig
> > > +++ b/drivers/tty/serial/8250/Kconfig
> > > @@ -378,7 +378,7 @@ config SERIAL_8250_BCM2835AUX
> > >  
> > >  config SERIAL_8250_FSL
> > >  	bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
> > > -	depends on SERIAL_8250_CONSOLE
> > > +	depends on SERIAL_8250
> > 
> > Just one additional thought: After the adding the arch side 
> > workaround/hack, SERIAL_8250_FSL could become a tristate?
> 
> I see no benefit for a module separate from 8250_base.ko. There are
> dependencies in both directions between 8250_port.o and 8250_fsl.o[1].
> So in my book a bool SERIAL_8250_FSL that modifies 8250_base.ko (with
> SERIAL_8250=m) is fine.
> 
> Best regards
> Uwe
> 
> [1] 8250_port.o uses fsl8250_handle_irq() from 8250_fsl.o

Is that after some fix which isn't in tty-next? I see only these:

$ git grep -l fsl8250_handle_irq
arch/powerpc/kernel/legacy_serial.c
drivers/tty/serial/8250/8250_fsl.c
drivers/tty/serial/8250/8250_of.c
include/linux/serial_8250.h

No users of fsl8250_handle_irq in 8250_port.c.

-- 
 i.

>, and 8250_fsl.o uses serial8250_modem_status from 8250_port.o.

Re: [PATCH v3 2/2] serial: 8250: Apply FSL workarounds also without SERIAL_8250_CONSOLE

[Andy Shevchenko]

On Mon, Jun 05, 2023 at 04:44:08PM +0300, Ilpo Järvinen wrote:
> On Mon, 5 Jun 2023, Uwe Kleine-König wrote:
> > On Mon, Jun 05, 2023 at 04:22:55PM +0300, Ilpo Järvinen wrote:
> > > On Mon, 5 Jun 2023, Uwe Kleine-König wrote:
> > > > The need to handle the FSL variant of 8250 in a special way is also
> > > > present without console support. So soften the dependency for
> > > > SERIAL_8250_FSL accordingly. Note that with the 8250 driver compiled as
> > > > a module, some devices still might not make use of the needed
> > > > workarounds. That affects the ports instantiated in
> > > > arch/powerpc/kernel/legacy_serial.c.
> > > > 
> > > > This issue was identified by Dominik Andreas Schorpp.
> > > > 
> > > > To cope for CONFIG_SERIAL_8250=m + CONFIG_SERIAL_8250_FSL=y, 8250_fsl.o
> > > > must be put in the same compilation unit as 8250_port.o because the
> > > > latter defines some functions needed in the former and so 8250_fsl.o
> > > > must not be built-in if 8250_port.o is available in a module.
> > > > 
> > > > Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > Link: https://lore.kernel.org/r/20230531083230.2702181-1-u.kleine-koenig@pengutronix.de
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  drivers/tty/serial/8250/Kconfig  | 2 +-
> > > >  drivers/tty/serial/8250/Makefile | 2 +-
> > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > > > index 5313aa31930f..10c09b19c871 100644
> > > > --- a/drivers/tty/serial/8250/Kconfig
> > > > +++ b/drivers/tty/serial/8250/Kconfig
> > > > @@ -378,7 +378,7 @@ config SERIAL_8250_BCM2835AUX
> > > >  
> > > >  config SERIAL_8250_FSL
> > > >  	bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
> > > > -	depends on SERIAL_8250_CONSOLE
> > > > +	depends on SERIAL_8250
> > > 
> > > Just one additional thought: After the adding the arch side 
> > > workaround/hack, SERIAL_8250_FSL could become a tristate?
> > 
> > I see no benefit for a module separate from 8250_base.ko. There are
> > dependencies in both directions between 8250_port.o and 8250_fsl.o[1].
> > So in my book a bool SERIAL_8250_FSL that modifies 8250_base.ko (with
> > SERIAL_8250=m) is fine.

> > [1] 8250_port.o uses fsl8250_handle_irq() from 8250_fsl.o
> 
> Is that after some fix which isn't in tty-next? I see only these:
> 
> $ git grep -l fsl8250_handle_irq
> arch/powerpc/kernel/legacy_serial.c
> drivers/tty/serial/8250/8250_fsl.c
> drivers/tty/serial/8250/8250_of.c
> include/linux/serial_8250.h
> 
> No users of fsl8250_handle_irq in 8250_port.c.

> >, and 8250_fsl.o uses serial8250_modem_status from 8250_port.o.

I don't like 8250_base to be fattened by some stuff that has no
generic meaning. Can we avoid putting every quirk there?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] serial: 8250: Apply FSL workarounds also without SERIAL_8250_CONSOLE

[Uwe Kleine-König]

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

Hello Ilpo,

On Mon, Jun 05, 2023 at 04:44:08PM +0300, Ilpo Järvinen wrote:
> On Mon, 5 Jun 2023, Uwe Kleine-König wrote:
> > On Mon, Jun 05, 2023 at 04:22:55PM +0300, Ilpo Järvinen wrote:
> > > On Mon, 5 Jun 2023, Uwe Kleine-König wrote:
> > > 
> > > > The need to handle the FSL variant of 8250 in a special way is also
> > > > present without console support. So soften the dependency for
> > > > SERIAL_8250_FSL accordingly. Note that with the 8250 driver compiled as
> > > > a module, some devices still might not make use of the needed
> > > > workarounds. That affects the ports instantiated in
> > > > arch/powerpc/kernel/legacy_serial.c.
> > > > 
> > > > This issue was identified by Dominik Andreas Schorpp.
> > > > 
> > > > To cope for CONFIG_SERIAL_8250=m + CONFIG_SERIAL_8250_FSL=y, 8250_fsl.o
> > > > must be put in the same compilation unit as 8250_port.o because the
> > > > latter defines some functions needed in the former and so 8250_fsl.o
> > > > must not be built-in if 8250_port.o is available in a module.
> > > > 
> > > > Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > Link: https://lore.kernel.org/r/20230531083230.2702181-1-u.kleine-koenig@pengutronix.de
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  drivers/tty/serial/8250/Kconfig  | 2 +-
> > > >  drivers/tty/serial/8250/Makefile | 2 +-
> > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > > > index 5313aa31930f..10c09b19c871 100644
> > > > --- a/drivers/tty/serial/8250/Kconfig
> > > > +++ b/drivers/tty/serial/8250/Kconfig
> > > > @@ -378,7 +378,7 @@ config SERIAL_8250_BCM2835AUX
> > > >  
> > > >  config SERIAL_8250_FSL
> > > >  	bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
> > > > -	depends on SERIAL_8250_CONSOLE
> > > > +	depends on SERIAL_8250
> > > 
> > > Just one additional thought: After the adding the arch side 
> > > workaround/hack, SERIAL_8250_FSL could become a tristate?
> > 
> > I see no benefit for a module separate from 8250_base.ko. There are
> > dependencies in both directions between 8250_port.o and 8250_fsl.o[1].
> > So in my book a bool SERIAL_8250_FSL that modifies 8250_base.ko (with
> > SERIAL_8250=m) is fine.
> > 
> > Best regards
> > Uwe
> > 
> > [1] 8250_port.o uses fsl8250_handle_irq() from 8250_fsl.o
> 
> Is that after some fix which isn't in tty-next? I see only these:
> 
> $ git grep -l fsl8250_handle_irq
> arch/powerpc/kernel/legacy_serial.c
> drivers/tty/serial/8250/8250_fsl.c
> drivers/tty/serial/8250/8250_of.c
> include/linux/serial_8250.h
> 
> No users of fsl8250_handle_irq in 8250_port.c.

Ah right, I was too quick:

	8250_of.o uses fsl8250_handle_irq() from 8250_fsl.o
	8250_fsl.o uses serial8250_modem_status() from 8250_port.o (which is in 8250_base.o)


However linking 8250_fsl.o in 8250_of.o isn't a good solution either as
8250_fsl.o should also be available with CONFIG_SERIAL_OF_PLATFORM
disabled to provide the ACPI driver. And as 8250_of.o already depends on
8250_port.o (e.g. via serial8250_em485_config()) adding 8250_fsl.o
together with 8250_port.o into 8250_base.ko is fine and doesn't add new
dependencies.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/2] serial: 8250: Apply FSL workarounds also without SERIAL_8250_CONSOLE

[Ilpo Järvinen]

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

On Mon, 5 Jun 2023, Uwe Kleine-König wrote:

> Hello Ilpo,
> 
> On Mon, Jun 05, 2023 at 04:44:08PM +0300, Ilpo Järvinen wrote:
> > On Mon, 5 Jun 2023, Uwe Kleine-König wrote:
> > > On Mon, Jun 05, 2023 at 04:22:55PM +0300, Ilpo Järvinen wrote:
> > > > On Mon, 5 Jun 2023, Uwe Kleine-König wrote:
> > > > 
> > > > > The need to handle the FSL variant of 8250 in a special way is also
> > > > > present without console support. So soften the dependency for
> > > > > SERIAL_8250_FSL accordingly. Note that with the 8250 driver compiled as
> > > > > a module, some devices still might not make use of the needed
> > > > > workarounds. That affects the ports instantiated in
> > > > > arch/powerpc/kernel/legacy_serial.c.
> > > > > 
> > > > > This issue was identified by Dominik Andreas Schorpp.
> > > > > 
> > > > > To cope for CONFIG_SERIAL_8250=m + CONFIG_SERIAL_8250_FSL=y, 8250_fsl.o
> > > > > must be put in the same compilation unit as 8250_port.o because the
> > > > > latter defines some functions needed in the former and so 8250_fsl.o
> > > > > must not be built-in if 8250_port.o is available in a module.
> > > > > 
> > > > > Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > > Link: https://lore.kernel.org/r/20230531083230.2702181-1-u.kleine-koenig@pengutronix.de
> > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > ---
> > > > >  drivers/tty/serial/8250/Kconfig  | 2 +-
> > > > >  drivers/tty/serial/8250/Makefile | 2 +-
> > > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > > > > index 5313aa31930f..10c09b19c871 100644
> > > > > --- a/drivers/tty/serial/8250/Kconfig
> > > > > +++ b/drivers/tty/serial/8250/Kconfig
> > > > > @@ -378,7 +378,7 @@ config SERIAL_8250_BCM2835AUX
> > > > >  
> > > > >  config SERIAL_8250_FSL
> > > > >  	bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
> > > > > -	depends on SERIAL_8250_CONSOLE
> > > > > +	depends on SERIAL_8250
> > > > 
> > > > Just one additional thought: After the adding the arch side 
> > > > workaround/hack, SERIAL_8250_FSL could become a tristate?
> > > 
> > > I see no benefit for a module separate from 8250_base.ko. There are
> > > dependencies in both directions between 8250_port.o and 8250_fsl.o[1].
> > > So in my book a bool SERIAL_8250_FSL that modifies 8250_base.ko (with
> > > SERIAL_8250=m) is fine.
> > > 
> > > Best regards
> > > Uwe
> > > 
> > > [1] 8250_port.o uses fsl8250_handle_irq() from 8250_fsl.o
> > 
> > Is that after some fix which isn't in tty-next? I see only these:
> > 
> > $ git grep -l fsl8250_handle_irq
> > arch/powerpc/kernel/legacy_serial.c
> > drivers/tty/serial/8250/8250_fsl.c
> > drivers/tty/serial/8250/8250_of.c
> > include/linux/serial_8250.h
> > 
> > No users of fsl8250_handle_irq in 8250_port.c.
> 
> Ah right, I was too quick:
> 
> 	8250_of.o uses fsl8250_handle_irq() from 8250_fsl.o
> 	8250_fsl.o uses serial8250_modem_status() from 8250_port.o (which is in 8250_base.o)
> 
> 
> However linking 8250_fsl.o in 8250_of.o isn't a good solution either as
> 8250_fsl.o should also be available with CONFIG_SERIAL_OF_PLATFORM
> disabled to provide the ACPI driver. And as 8250_of.o already depends on
> 8250_port.o (e.g. via serial8250_em485_config()) adding 8250_fsl.o
> together with 8250_port.o into 8250_base.ko is fine and doesn't add new
> dependencies.

So we have dependencies one-way only:

8250_port

/\    |\
        \
8250_fsl \
         /
/\      /

8250_of

There's no loop here, both they be indepedent modules and configured 
independently (with a correct IS_*() in 8250_of.c).

-- 
 i.