[PATCH v1 1/1] serial: 8250: Move CE4100 quirks to a module under 8250 driver

[PATCH v1 1/1] serial: 8250: Move CE4100 quirks to a module under 8250 driver

[Andy Shevchenko]

There is inconvenient for maintainers and maintainership to have
some quirks under architectural code. Move it to the specific quirk
file like other 8250-compatible drivers do.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

The next step can be considered on how to get rid of
serial8250_set_isa_configurator() ugly hook.

 arch/x86/include/asm/ce4100.h                 |  6 ++
 arch/x86/platform/ce4100/ce4100.c             | 98 -------------------
 .../tty/serial/8250/8250_ce4100.c             | 92 +++--------------
 drivers/tty/serial/8250/Makefile              |  1 +
 4 files changed, 23 insertions(+), 174 deletions(-)
 copy arch/x86/platform/ce4100/ce4100.c => drivers/tty/serial/8250/8250_ce4100.c (51%)

diff --git a/arch/x86/include/asm/ce4100.h b/arch/x86/include/asm/ce4100.h
index 2930f560d7f3..e1f965bb1e31 100644
--- a/arch/x86/include/asm/ce4100.h
+++ b/arch/x86/include/asm/ce4100.h
@@ -4,4 +4,10 @@
 
 int ce4100_pci_init(void);
 
+#ifdef CONFIG_SERIAL_8250
+void __init sdv_serial_fixup(void);
+#else
+static inline void sdv_serial_fixup(void) {};
+#endif
+
 #endif
diff --git a/arch/x86/platform/ce4100/ce4100.c b/arch/x86/platform/ce4100/ce4100.c
index 08492bea9713..aaa7017416f7 100644
--- a/arch/x86/platform/ce4100/ce4100.c
+++ b/arch/x86/platform/ce4100/ce4100.c
@@ -5,19 +5,12 @@
  * (C) Copyright 2010 Intel Corporation
  */
 #include <linux/init.h>
-#include <linux/kernel.h>
-#include <linux/irq.h>
 #include <linux/reboot.h>
-#include <linux/serial_reg.h>
-#include <linux/serial_8250.h>
 
 #include <asm/ce4100.h>
 #include <asm/prom.h>
 #include <asm/setup.h>
-#include <asm/i8259.h>
 #include <asm/io.h>
-#include <asm/io_apic.h>
-#include <asm/emergency-restart.h>
 
 /*
  * The CE4100 platform has an internal 8051 Microcontroller which is
@@ -31,97 +24,6 @@ static void ce4100_power_off(void)
 	outb(0x4, 0xcf9);
 }
 
-#ifdef CONFIG_SERIAL_8250
-
-static unsigned int mem_serial_in(struct uart_port *p, int offset)
-{
-	offset = offset << p->regshift;
-	return readl(p->membase + offset);
-}
-
-/*
- * The UART Tx interrupts are not set under some conditions and therefore serial
- * transmission hangs. This is a silicon issue and has not been root caused. The
- * workaround for this silicon issue checks UART_LSR_THRE bit and UART_LSR_TEMT
- * bit of LSR register in interrupt handler to see whether at least one of these
- * two bits is set, if so then process the transmit request. If this workaround
- * is not applied, then the serial transmission may hang. This workaround is for
- * errata number 9 in Errata - B step.
-*/
-
-static u32 ce4100_mem_serial_in(struct uart_port *p, unsigned int offset)
-{
-	u32 ret, ier, lsr;
-
-	if (offset != UART_IIR)
-		return mem_serial_in(p, offset);
-
-	offset <<= p->regshift;
-
-	ret = readl(p->membase + offset);
-	if (!(ret & UART_IIR_NO_INT))
-		return ret;
-
-	/* see if the TX interrupt should have really set */
-	ier = mem_serial_in(p, UART_IER);
-	/* see if the UART's XMIT interrupt is enabled */
-	if (!(ier & UART_IER_THRI))
-		return ret;
-
-	lsr = mem_serial_in(p, UART_LSR);
-	/* now check to see if the UART should be generating an interrupt (but isn't) */
-	if (lsr & (UART_LSR_THRE | UART_LSR_TEMT))
-		ret &= ~UART_IIR_NO_INT;
-
-	return ret;
-}
-
-static void ce4100_mem_serial_out(struct uart_port *p, unsigned int offset, u32 value)
-{
-	offset <<= p->regshift;
-	writel(value, p->membase + offset);
-}
-
-static void ce4100_serial_fixup(int port, struct uart_port *up,
-	u32 *capabilities)
-{
-#ifdef CONFIG_EARLY_PRINTK
-	/*
-	 * Over ride the legacy port configuration that comes from
-	 * asm/serial.h. Using the ioport driver then switching to the
-	 * PCI memmaped driver hangs the IOAPIC
-	 */
-	if (up->iotype !=  UPIO_MEM32) {
-		up->uartclk  = 14745600;
-		up->mapbase = 0xdffe0200;
-		set_fixmap_nocache(FIX_EARLYCON_MEM_BASE,
-				up->mapbase & PAGE_MASK);
-		up->membase =
-			(void __iomem *)__fix_to_virt(FIX_EARLYCON_MEM_BASE);
-		up->membase += up->mapbase & ~PAGE_MASK;
-		up->mapbase += port * 0x100;
-		up->membase += port * 0x100;
-		up->iotype   = UPIO_MEM32;
-		up->regshift = 2;
-		up->irq = 4;
-	}
-#endif
-	up->iobase = 0;
-	up->serial_in = ce4100_mem_serial_in;
-	up->serial_out = ce4100_mem_serial_out;
-
-	*capabilities |= (1 << 12);
-}
-
-static __init void sdv_serial_fixup(void)
-{
-	serial8250_set_isa_configurator(ce4100_serial_fixup);
-}
-
-#else
-static inline void sdv_serial_fixup(void) {};
-#endif
-
 static void __init sdv_arch_setup(void)
 {
 	sdv_serial_fixup();
diff --git a/arch/x86/platform/ce4100/ce4100.c b/drivers/tty/serial/8250/8250_ce4100.c
similarity index 51%
copy from arch/x86/platform/ce4100/ce4100.c
copy to drivers/tty/serial/8250/8250_ce4100.c
index 08492bea9713..3dd88f372a51 100644
--- a/arch/x86/platform/ce4100/ce4100.c
+++ b/drivers/tty/serial/8250/8250_ce4100.c
@@ -4,34 +4,17 @@
  *
  * (C) Copyright 2010 Intel Corporation
  */
+
 #include <linux/init.h>
-#include <linux/kernel.h>
-#include <linux/irq.h>
-#include <linux/reboot.h>
-#include <linux/serial_reg.h>
-#include <linux/serial_8250.h>
+#include <linux/io.h>
+#include <linux/types.h>
 
 #include <asm/ce4100.h>
-#include <asm/prom.h>
-#include <asm/setup.h>
-#include <asm/i8259.h>
-#include <asm/io.h>
-#include <asm/io_apic.h>
-#include <asm/emergency-restart.h>
+#include <asm/fixmap.h>
+#include <asm/page.h>
 
-/*
- * The CE4100 platform has an internal 8051 Microcontroller which is
- * responsible for signaling to the external Power Management Unit the
- * intention to reset, reboot or power off the system. This 8051 device has
- * its command register mapped at I/O port 0xcf9 and the value 0x4 is used
- * to power off the system.
- */
-static void ce4100_power_off(void)
-{
-	outb(0x4, 0xcf9);
-}
-
-#ifdef CONFIG_SERIAL_8250
+#include <linux/serial_reg.h>
+#include <linux/serial_8250.h>
 
 static unsigned int mem_serial_in(struct uart_port *p, int offset)
 {
@@ -48,7 +31,6 @@ static unsigned int mem_serial_in(struct uart_port *p, int offset)
  * is not applied, then the serial transmission may hang. This workaround is for
  * errata number 9 in Errata - B step.
 */
-
 static u32 ce4100_mem_serial_in(struct uart_port *p, unsigned int offset)
 {
 	u32 ret, ier, lsr;
@@ -82,26 +64,23 @@ static void ce4100_mem_serial_out(struct uart_port *p, unsigned int offset, u32
 	writel(value, p->membase + offset);
 }
 
-static void ce4100_serial_fixup(int port, struct uart_port *up,
-	u32 *capabilities)
+static void ce4100_serial_fixup(int port, struct uart_port *up, u32 *capabilities)
 {
 #ifdef CONFIG_EARLY_PRINTK
 	/*
-	 * Over ride the legacy port configuration that comes from
+	 * Override the legacy port configuration that comes from
 	 * asm/serial.h. Using the ioport driver then switching to the
-	 * PCI memmaped driver hangs the IOAPIC
+	 * PCI memmaped driver hangs the IOAPIC.
 	 */
-	if (up->iotype !=  UPIO_MEM32) {
-		up->uartclk  = 14745600;
+	if (up->iotype != UPIO_MEM32) {
+		up->uartclk = 14745600;
 		up->mapbase = 0xdffe0200;
-		set_fixmap_nocache(FIX_EARLYCON_MEM_BASE,
-				up->mapbase & PAGE_MASK);
-		up->membase =
-			(void __iomem *)__fix_to_virt(FIX_EARLYCON_MEM_BASE);
+		set_fixmap_nocache(FIX_EARLYCON_MEM_BASE, up->mapbase & PAGE_MASK);
+		up->membase = (void __iomem *)__fix_to_virt(FIX_EARLYCON_MEM_BASE);
 		up->membase += up->mapbase & ~PAGE_MASK;
 		up->mapbase += port * 0x100;
 		up->membase += port * 0x100;
-		up->iotype   = UPIO_MEM32;
+		up->iotype = UPIO_MEM32;
 		up->regshift = 2;
 		up->irq = 4;
 	}
@@ -113,46 +92,7 @@ static void ce4100_serial_fixup(int port, struct uart_port *up,
 	*capabilities |= (1 << 12);
 }
 
-static __init void sdv_serial_fixup(void)
+void __init sdv_serial_fixup(void)
 {
 	serial8250_set_isa_configurator(ce4100_serial_fixup);
 }
-
-#else
-static inline void sdv_serial_fixup(void) {};
-#endif
-
-static void __init sdv_arch_setup(void)
-{
-	sdv_serial_fixup();
-}
-
-static void sdv_pci_init(void)
-{
-	x86_of_pci_init();
-}
-
-/*
- * CE4100 specific x86_init function overrides and early setup
- * calls.
- */
-void __init x86_ce4100_early_setup(void)
-{
-	x86_init.oem.arch_setup			= sdv_arch_setup;
-	x86_init.resources.probe_roms		= x86_init_noop;
-	x86_init.mpparse.find_mptable		= x86_init_noop;
-	x86_init.mpparse.early_parse_smp_cfg	= x86_init_noop;
-	x86_init.pci.init			= ce4100_pci_init;
-	x86_init.pci.init_irq			= sdv_pci_init;
-
-	/*
-	 * By default, the reboot method is ACPI which is supported by the
-	 * CE4100 bootloader CEFDK using FADT.ResetReg Address and ResetValue
-	 * the bootloader will however issue a system power off instead of
-	 * reboot. By using BOOT_KBD we ensure proper system reboot as
-	 * expected.
-	 */
-	reboot_type = BOOT_KBD;
-
-	pm_power_off = ce4100_power_off;
-}
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index b04eeda03b23..e61dc3f4ca50 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SERIAL_8250_ASPEED_VUART)	+= 8250_aspeed_vuart.o
 obj-$(CONFIG_SERIAL_8250_BCM2835AUX)	+= 8250_bcm2835aux.o
 obj-$(CONFIG_SERIAL_8250_BCM7271)	+= 8250_bcm7271.o
 obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
+obj-$(CONFIG_X86_INTEL_CE)		+= 8250_ce4100.o
 obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
-- 
2.47.2

Re: [PATCH v1 1/1] serial: 8250: Move CE4100 quirks to a module under 8250 driver

[Jiri Slaby]

On 27. 06. 25, 20:25, Andy Shevchenko wrote:
> There is inconvenient for maintainers and maintainership to have
> some quirks under architectural code. Move it to the specific quirk
> file like other 8250-compatible drivers do.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Nice.

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

Just two nits (one suggestion actually) below. Ignore if you won't 
resubmit for some other reasons...

> --- a/arch/x86/include/asm/ce4100.h
> +++ b/arch/x86/include/asm/ce4100.h
> @@ -4,4 +4,10 @@
>   
>   int ce4100_pci_init(void);
>   
> +#ifdef CONFIG_SERIAL_8250
> +void __init sdv_serial_fixup(void);
> +#else
> +static inline void sdv_serial_fixup(void) {};

Superfluous ;.

> +#endif
> +
>   #endif
> --- a/arch/x86/platform/ce4100/ce4100.c
> +++ b/arch/x86/platform/ce4100/ce4100.c
...
> @@ -31,97 +24,6 @@ static void ce4100_power_off(void)
...
> -static u32 ce4100_mem_serial_in(struct uart_port *p, unsigned int offset)
> -{
> -	u32 ret, ier, lsr;
> -
> -	if (offset != UART_IIR)
> -		return mem_serial_in(p, offset);
> -
> -	offset <<= p->regshift;
> -
> -	ret = readl(p->membase + offset);

Just noticed: why the two above lines are not one:
ret = mem_serial_in()?

Or in fact the whole function intro:
ret = mem_serial_in(p, offset);
if (offset != UART_IIR)
   return ret;

thanks,
-- 
js
suse labs

Re: [PATCH v1 1/1] serial: 8250: Move CE4100 quirks to a module under 8250 driver

[Andy Shevchenko]

On Mon, Jun 30, 2025 at 01:27:10PM +0200, Jiri Slaby wrote:
> On 27. 06. 25, 20:25, Andy Shevchenko wrote:
> > There is inconvenient for maintainers and maintainership to have
> > some quirks under architectural code. Move it to the specific quirk
> > file like other 8250-compatible drivers do.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Nice.
> 
> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

Thanks, but it seems a bit late :-)

> Just two nits (one suggestion actually) below. Ignore if you won't resubmit
> for some other reasons...

Sure, see above.

...

> > +static inline void sdv_serial_fixup(void) {};
> 
> Superfluous ;.

Indeed. But I don't want to update just this (as it now may require
a separate change for x86).

...

> > -static u32 ce4100_mem_serial_in(struct uart_port *p, unsigned int offset)
> > -{
> > -	u32 ret, ier, lsr;
> > -
> > -	if (offset != UART_IIR)
> > -		return mem_serial_in(p, offset);
> > -
> > -	offset <<= p->regshift;
> > -
> > -	ret = readl(p->membase + offset);
> 
> Just noticed: why the two above lines are not one:
> ret = mem_serial_in()?
> 
> Or in fact the whole function intro:
> ret = mem_serial_in(p, offset);
> if (offset != UART_IIR)
>   return ret;

Hmm... Sounds like an equivalent. I probably can send a followup for this
suggestion.

-- 
With Best Regards,
Andy Shevchenko