[PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Bin Gao]

The arch independent uart8250 early console driver has good
support for memory mapped and io port based 8250 uarts. Since
pci is arch independent so it's natural to extend uart8250 to
support mem, io and pci. Hence pci uart early console in
arch/x86/kernel_printk.c by the following commit:
'commit 5140fda16051 ("Specify PCI based UART for earlyprintk")'
is removed. And its equivalent function will be available from
uart8250 early console driver.

Signed-off-by: Bin Gao <bin.gao@intel.com>
---
Changes in v5:
 - moved earlyprintk= (an alias to earlycon=) from patch 1/2 to patch 2/2
Changes in v2-v4:
 - no changes but keep going with patch 1/2
 arch/x86/kernel/early_printk.c | 180 ++++-------------------------------------
 drivers/tty/serial/earlycon.c  |   9 +++
 2 files changed, 24 insertions(+), 165 deletions(-)

diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index 89427d8..00c2e2a 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -19,7 +19,6 @@
 #include <linux/usb/ehci_def.h>
 #include <linux/efi.h>
 #include <asm/efi.h>
-#include <asm/pci_x86.h>
 
 /* Simple VGA output */
 #define VGABASE		(__ISA_IO_base + 0xb8000)
@@ -77,7 +76,7 @@ static struct console early_vga_console = {
 
 /* Serial functions loosely based on a similar package from Klaus P. Gerlicher */
 
-static unsigned long early_serial_base = 0x3f8;  /* ttyS0 */
+static int early_serial_base = 0x3f8;  /* ttyS0 */
 
 #define XMTRDY          0x20
 
@@ -95,26 +94,13 @@ static unsigned long early_serial_base = 0x3f8;  /* ttyS0 */
 #define DLL             0       /*  Divisor Latch Low         */
 #define DLH             1       /*  Divisor latch High        */
 
-static unsigned int io_serial_in(unsigned long addr, int offset)
-{
-	return inb(addr + offset);
-}
-
-static void io_serial_out(unsigned long addr, int offset, int value)
-{
-	outb(value, addr + offset);
-}
-
-static unsigned int (*serial_in)(unsigned long addr, int offset) = io_serial_in;
-static void (*serial_out)(unsigned long addr, int offset, int value) = io_serial_out;
-
 static int early_serial_putc(unsigned char ch)
 {
 	unsigned timeout = 0xffff;
 
-	while ((serial_in(early_serial_base, LSR) & XMTRDY) == 0 && --timeout)
+	while ((inb(early_serial_base + LSR) & XMTRDY) == 0 && --timeout)
 		cpu_relax();
-	serial_out(early_serial_base, TXR, ch);
+	outb(ch, early_serial_base + TXR);
 	return timeout ? 0 : -1;
 }
 
@@ -128,28 +114,13 @@ static void early_serial_write(struct console *con, const char *s, unsigned n)
 	}
 }
 
-static __init void early_serial_hw_init(unsigned divisor)
-{
-	unsigned char c;
-
-	serial_out(early_serial_base, LCR, 0x3);	/* 8n1 */
-	serial_out(early_serial_base, IER, 0);	/* no interrupt */
-	serial_out(early_serial_base, FCR, 0);	/* no fifo */
-	serial_out(early_serial_base, MCR, 0x3);	/* DTR + RTS */
-
-	c = serial_in(early_serial_base, LCR);
-	serial_out(early_serial_base, LCR, c | DLAB);
-	serial_out(early_serial_base, DLL, divisor & 0xff);
-	serial_out(early_serial_base, DLH, (divisor >> 8) & 0xff);
-	serial_out(early_serial_base, LCR, c & ~DLAB);
-}
-
 #define DEFAULT_BAUD 9600
 
 static __init void early_serial_init(char *s)
 {
+	unsigned char c;
 	unsigned divisor;
-	unsigned long baud = DEFAULT_BAUD;
+	unsigned baud = DEFAULT_BAUD;
 	char *e;
 
 	if (*s == ',')
@@ -174,138 +145,23 @@ static __init void early_serial_init(char *s)
 			s++;
 	}
 
-	if (*s) {
-		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
-			baud = DEFAULT_BAUD;
-	}
-
-	/* Convert from baud to divisor value */
-	divisor = 115200 / baud;
-
-	/* These will always be IO based ports */
-	serial_in = io_serial_in;
-	serial_out = io_serial_out;
-
-	/* Set up the HW */
-	early_serial_hw_init(divisor);
-}
-
-#ifdef CONFIG_PCI
-static void mem32_serial_out(unsigned long addr, int offset, int value)
-{
-	u32 *vaddr = (u32 *)addr;
-	/* shift implied by pointer type */
-	writel(value, vaddr + offset);
-}
-
-static unsigned int mem32_serial_in(unsigned long addr, int offset)
-{
-	u32 *vaddr = (u32 *)addr;
-	/* shift implied by pointer type */
-	return readl(vaddr + offset);
-}
-
-/*
- * early_pci_serial_init()
- *
- * This function is invoked when the early_printk param starts with "pciserial"
- * The rest of the param should be ",B:D.F,baud" where B, D & F describe the
- * location of a PCI device that must be a UART device.
- */
-static __init void early_pci_serial_init(char *s)
-{
-	unsigned divisor;
-	unsigned long baud = DEFAULT_BAUD;
-	u8 bus, slot, func;
-	u32 classcode, bar0;
-	u16 cmdreg;
-	char *e;
-
-
-	/*
-	 * First, part the param to get the BDF values
-	 */
-	if (*s == ',')
-		++s;
-
-	if (*s == 0)
-		return;
-
-	bus = (u8)simple_strtoul(s, &e, 16);
-	s = e;
-	if (*s != ':')
-		return;
-	++s;
-	slot = (u8)simple_strtoul(s, &e, 16);
-	s = e;
-	if (*s != '.')
-		return;
-	++s;
-	func = (u8)simple_strtoul(s, &e, 16);
-	s = e;
-
-	/* A baud might be following */
-	if (*s == ',')
-		s++;
-
-	/*
-	 * Second, find the device from the BDF
-	 */
-	cmdreg = read_pci_config(bus, slot, func, PCI_COMMAND);
-	classcode = read_pci_config(bus, slot, func, PCI_CLASS_REVISION);
-	bar0 = read_pci_config(bus, slot, func, PCI_BASE_ADDRESS_0);
-
-	/*
-	 * Verify it is a UART type device
-	 */
-	if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
-	     (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
-	   (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */
-		return;
-
-	/*
-	 * Determine if it is IO or memory mapped
-	 */
-	if (bar0 & 0x01) {
-		/* it is IO mapped */
-		serial_in = io_serial_in;
-		serial_out = io_serial_out;
-		early_serial_base = bar0&0xfffffffc;
-		write_pci_config(bus, slot, func, PCI_COMMAND,
-						cmdreg|PCI_COMMAND_IO);
-	} else {
-		/* It is memory mapped - assume 32-bit alignment */
-		serial_in = mem32_serial_in;
-		serial_out = mem32_serial_out;
-		/* WARNING! assuming the address is always in the first 4G */
-		early_serial_base =
-			(unsigned long)early_ioremap(bar0 & 0xfffffff0, 0x10);
-		write_pci_config(bus, slot, func, PCI_COMMAND,
-						cmdreg|PCI_COMMAND_MEMORY);
-	}
+	outb(0x3, early_serial_base + LCR);	/* 8n1 */
+	outb(0, early_serial_base + IER);	/* no interrupt */
+	outb(0, early_serial_base + FCR);	/* no fifo */
+	outb(0x3, early_serial_base + MCR);	/* DTR + RTS */
 
-	/*
-	 * Lastly, initalize the hardware
-	 */
 	if (*s) {
-		if (strcmp(s, "nocfg") == 0)
-			/* Sometimes, we want to leave the UART alone
-			 * and assume the BIOS has set it up correctly.
-			 * "nocfg" tells us this is the case, and we
-			 * should do no more setup.
-			 */
-			return;
 		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
 			baud = DEFAULT_BAUD;
 	}
 
-	/* Convert from baud to divisor value */
 	divisor = 115200 / baud;
-
-	/* Set up the HW */
-	early_serial_hw_init(divisor);
+	c = inb(early_serial_base + LCR);
+	outb(c | DLAB, early_serial_base + LCR);
+	outb(divisor & 0xff, early_serial_base + DLL);
+	outb((divisor >> 8) & 0xff, early_serial_base + DLH);
+	outb(c & ~DLAB, early_serial_base + LCR);
 }
-#endif
 
 static struct console early_serial_console = {
 	.name =		"earlyser",
@@ -326,6 +182,7 @@ static inline void early_console_register(struct console *con, int keep_early)
 		early_console->flags &= ~CON_BOOT;
 	else
 		early_console->flags |= CON_BOOT;
+
 	register_console(early_console);
 }
 
@@ -353,13 +210,6 @@ static int __init setup_early_printk(char *buf)
 			early_serial_init(buf + 4);
 			early_console_register(&early_serial_console, keep);
 		}
-#ifdef CONFIG_PCI
-		if (!strncmp(buf, "pciserial", 9)) {
-			early_pci_serial_init(buf + 9);
-			early_console_register(&early_serial_console, keep);
-			buf += 9; /* Keep from match the above "serial" */
-		}
-#endif
 		if (!strncmp(buf, "vga", 3) &&
 		    boot_params.screen_info.orig_video_isVGA == 1) {
 			max_xpos = boot_params.screen_info.orig_video_cols;
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index 6dc471e..63ae60e 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -193,6 +193,15 @@ static int __init param_setup_earlycon(char *buf)
 }
 early_param("earlycon", param_setup_earlycon);
 
+/* x86 uses "earlyprintk=xxx", so we keep the compatibility here */
+#ifdef CONFIG_X86
+static int __init param_setup_earlycon_x86(char *buf)
+{
+	return param_setup_earlycon(buf);
+}
+early_param("earlyprintk", param_setup_earlycon_x86);
+#endif
+
 int __init of_setup_earlycon(unsigned long addr,
 			     int (*setup)(struct earlycon_device *, const char *))
 {
-- 
1.9.1

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Ingo Molnar]

* Bin Gao <bin.gao@linux.intel.com> wrote:

> The arch independent uart8250 early console driver has good support for memory 
> mapped and io port based 8250 uarts. Since pci is arch independent so it's 
> natural to extend uart8250 to support mem, io and pci. Hence pci uart early 
> console in arch/x86/kernel_printk.c by the following commit: 'commit 
> 5140fda16051 ("Specify PCI based UART for earlyprintk")' is removed. And its 
> equivalent function will be available from uart8250 early console driver.

In what way have you tested this change, does serial-earlyprintk still work on x86 
after the change?

Thanks,

	Ingo

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Bin Gao]

On Tue, Jun 02, 2015 at 01:54:33PM +0200, Ingo Molnar wrote:
> 
> * Bin Gao <bin.gao@linux.intel.com> wrote:
> 
> > The arch independent uart8250 early console driver has good support for memory 
> > mapped and io port based 8250 uarts. Since pci is arch independent so it's 
> > natural to extend uart8250 to support mem, io and pci. Hence pci uart early 
> > console in arch/x86/kernel_printk.c by the following commit: 'commit 
> > 5140fda16051 ("Specify PCI based UART for earlyprintk")' is removed. And its 
> > equivalent function will be available from uart8250 early console driver.
> 
> In what way have you tested this change, does serial-earlyprintk still work on x86 
> after the change?
> 
> Thanks,
> 
> 	Ingo

Yes, this patch was tested with earlyprintk=serial,0x3f8 on my x86 box and
the early console worked fine.

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Yinghai Lu]

On Tue, Jun 2, 2015 at 12:20 PM, Bin Gao <bin.gao@linux.intel.com> wrote:
>>
>> In what way have you tested this change, does serial-earlyprintk still work on x86
>> after the change?
>
> Yes, this patch was tested with earlyprintk=serial,0x3f8 on my x86 box and
> the early console worked fine.

how about
earlyprintk=serial,ttyS0
earlyprintk=ttyS0

Thanks

Yinghai

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Thomas Gleixner]

On Tue, 2 Jun 2015, Bin Gao wrote:
> On Tue, Jun 02, 2015 at 01:54:33PM +0200, Ingo Molnar wrote:
> > 
> > * Bin Gao <bin.gao@linux.intel.com> wrote:
> > 
> > > The arch independent uart8250 early console driver has good support for memory 
> > > mapped and io port based 8250 uarts. Since pci is arch independent so it's 
> > > natural to extend uart8250 to support mem, io and pci. Hence pci uart early 
> > > console in arch/x86/kernel_printk.c by the following commit: 'commit 
> > > 5140fda16051 ("Specify PCI based UART for earlyprintk")' is removed. And its 
> > > equivalent function will be available from uart8250 early console driver.
> > 
> > In what way have you tested this change, does serial-earlyprintk still work on x86 
> > after the change?
> > 
> > Thanks,
> > 
> > 	Ingo
> 
> Yes, this patch was tested with earlyprintk=serial,0x3f8 on my x86 box and
> the early console worked fine.

What about the memory mapped uarts which have been source of trouble
in the past?

Thanks,

	tglx

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Ingo Molnar]

* Bin Gao <bin.gao@linux.intel.com> wrote:

> On Tue, Jun 02, 2015 at 01:54:33PM +0200, Ingo Molnar wrote:
> > 
> > * Bin Gao <bin.gao@linux.intel.com> wrote:
> > 
> > > The arch independent uart8250 early console driver has good support for memory 
> > > mapped and io port based 8250 uarts. Since pci is arch independent so it's 
> > > natural to extend uart8250 to support mem, io and pci. Hence pci uart early 
> > > console in arch/x86/kernel_printk.c by the following commit: 'commit 
> > > 5140fda16051 ("Specify PCI based UART for earlyprintk")' is removed. And its 
> > > equivalent function will be available from uart8250 early console driver.
> > 
> > In what way have you tested this change, does serial-earlyprintk still work on x86 
> > after the change?
> > 
> > Thanks,
> > 
> > 	Ingo
> 
> Yes, this patch was tested with earlyprintk=serial,0x3f8 on my x86 box and
> the early console worked fine.

So the format on x86 used to be:

  earlyprintk=serial,ttyS0,115200

does that work too?

Thanks,

	Ingo

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Bin Gao]

On Tue, Jun 02, 2015 at 11:46:02PM +0200, Ingo Molnar wrote:
> So the format on x86 used to be:
> 
>   earlyprintk=serial,ttyS0,115200
> 
> does that work too?
> 
> Thanks,
> 
> 	Ingo
Just tested it, and yes it works.

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Bin Gao]

On Tue, Jun 02, 2015 at 11:07:39PM +0200, Thomas Gleixner wrote:
> What about the memory mapped uarts which have been source of trouble
> in the past?
> 
> Thanks,
> 
> 	tglx

Not sure which specific early console you are referring to.
Currently we have serial, ttyS, vga, dbgp, xen, efi and pciserial in
arch/x86/kernel/early_printk.c, and only pciserial is memory mapped
(but it's being removed).

And this patch only touches pciserial/serial parts so ideally shouldn't
have impact on other early consoles.

-Bin

RE: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Anderson, Stuart R]

Bin, et al,

What we are losing here is the ability to specify a UART by its bus address instead of just supplying the memory or io address. There are some cases where this is useful, though I admit it is probably not going to be widely used. I have seen a platform where the location of the UART moves depending on the firmware version, but the bus address (B:D.F) did not change. There are also some platforms where you do not know the address until you boot the OS and can use the UART to login and find the address of the UART (oops. Chicken and egg problem).

Also, I was going to soon send a patch to allow "pciserial32" for the case where the UART registers are 32-bit aligned instead of 8-bit aligned.

Stuart

-----Original Message-----
From: Bin Gao [mailto:bin.gao@linux.intel.com] 
Sent: Tuesday, June 02, 2015 3:43 PM
To: Thomas Gleixner
Cc: Ingo Molnar; Ingo Molnar; H. Peter Anvin; Greg Kroah-Hartman; One Thousand Gnomes; Peter Hurley; Jiri Slaby; Borislav Petkov; Anderson, Stuart R; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

On Tue, Jun 02, 2015 at 11:07:39PM +0200, Thomas Gleixner wrote:
> What about the memory mapped uarts which have been source of trouble 
> in the past?
> 
> Thanks,
> 
> 	tglx

Not sure which specific early console you are referring to.
Currently we have serial, ttyS, vga, dbgp, xen, efi and pciserial in arch/x86/kernel/early_printk.c, and only pciserial is memory mapped (but it's being removed).

And this patch only touches pciserial/serial parts so ideally shouldn't have impact on other early consoles.

-Bin

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Bin Gao]

On Tue, Jun 02, 2015 at 01:37:52PM -0700, Yinghai Lu wrote:
> how about
> earlyprintk=serial,ttyS0
> earlyprintk=ttyS0
> 
> Thanks
> 
> Yinghai

Yes, both work.

-Bin

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Bin Gao]

On Wed, Jun 03, 2015 at 12:16:36AM +0000, Anderson, Stuart R wrote:
> Bin, et al,
> 
> What we are losing here is the ability to specify a UART by its bus address
> instead of just supplying the memory or io address. There are some cases
> where this is useful, though I admit it is probably not going to be widely
> used. I have seen a platform where the location of the UART moves depending
> on the firmware version, but the bus address (B:D.F) did not change. There
> are also some platforms where you do not know the address until you boot the
> OS and can use the UART to login and find the address of the UART (oops.
> Chicken and egg problem).
> 
> Also, I was going to soon send a patch to allow "pciserial32" for the case where the UART registers are 32-bit aligned instead of 8-bit aligned.
> 
> Stuart
> 

Theare are several reasons that we want to move it to serial_core.
First, pci is arch independent, so putting these codes in serial_core
(which is arch independent) makes more sense.
Second, B:D.F may change across SoCs, then to support new SoC we only
need change command line but don't need to change the code.
Lastly, there could be other non-x86 platforms using it in the future.

-Bin

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Peter Hurley]

On 05/29/2015 02:41 PM, Bin Gao wrote:
> The arch independent uart8250 early console driver has good
> support for memory mapped and io port based 8250 uarts. Since
> pci is arch independent so it's natural to extend uart8250 to
> support mem, io and pci. Hence pci uart early console in
> arch/x86/kernel_printk.c by the following commit:
> 'commit 5140fda16051 ("Specify PCI based UART for earlyprintk")'
> is removed. And its equivalent function will be available from
> uart8250 early console driver.
> 
> Signed-off-by: Bin Gao <bin.gao@intel.com>
> ---
> Changes in v5:
>  - moved earlyprintk= (an alias to earlycon=) from patch 1/2 to patch 2/2
> Changes in v2-v4:
>  - no changes but keep going with patch 1/2
>  arch/x86/kernel/early_printk.c | 180 ++++-------------------------------------
>  drivers/tty/serial/earlycon.c  |   9 +++
>  2 files changed, 24 insertions(+), 165 deletions(-)
> 
> diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> index 89427d8..00c2e2a 100644
> --- a/arch/x86/kernel/early_printk.c
> +++ b/arch/x86/kernel/early_printk.c
> @@ -19,7 +19,6 @@
>  #include <linux/usb/ehci_def.h>
>  #include <linux/efi.h>
>  #include <asm/efi.h>
> -#include <asm/pci_x86.h>
>  
>  /* Simple VGA output */
>  #define VGABASE		(__ISA_IO_base + 0xb8000)
> @@ -77,7 +76,7 @@ static struct console early_vga_console = {
>  
>  /* Serial functions loosely based on a similar package from Klaus P. Gerlicher */
>  
> -static unsigned long early_serial_base = 0x3f8;  /* ttyS0 */
> +static int early_serial_base = 0x3f8;  /* ttyS0 */
>  
>  #define XMTRDY          0x20
>  
> @@ -95,26 +94,13 @@ static unsigned long early_serial_base = 0x3f8;  /* ttyS0 */
>  #define DLL             0       /*  Divisor Latch Low         */
>  #define DLH             1       /*  Divisor latch High        */
>  
> -static unsigned int io_serial_in(unsigned long addr, int offset)
> -{
> -	return inb(addr + offset);
> -}
> -
> -static void io_serial_out(unsigned long addr, int offset, int value)
> -{
> -	outb(value, addr + offset);
> -}
> -
> -static unsigned int (*serial_in)(unsigned long addr, int offset) = io_serial_in;
> -static void (*serial_out)(unsigned long addr, int offset, int value) = io_serial_out;
> -
>  static int early_serial_putc(unsigned char ch)
>  {
>  	unsigned timeout = 0xffff;
>  
> -	while ((serial_in(early_serial_base, LSR) & XMTRDY) == 0 && --timeout)
> +	while ((inb(early_serial_base + LSR) & XMTRDY) == 0 && --timeout)
>  		cpu_relax();
> -	serial_out(early_serial_base, TXR, ch);
> +	outb(ch, early_serial_base + TXR);
>  	return timeout ? 0 : -1;
>  }
>  
> @@ -128,28 +114,13 @@ static void early_serial_write(struct console *con, const char *s, unsigned n)
>  	}
>  }
>  
> -static __init void early_serial_hw_init(unsigned divisor)
> -{
> -	unsigned char c;
> -
> -	serial_out(early_serial_base, LCR, 0x3);	/* 8n1 */
> -	serial_out(early_serial_base, IER, 0);	/* no interrupt */
> -	serial_out(early_serial_base, FCR, 0);	/* no fifo */
> -	serial_out(early_serial_base, MCR, 0x3);	/* DTR + RTS */
> -
> -	c = serial_in(early_serial_base, LCR);
> -	serial_out(early_serial_base, LCR, c | DLAB);
> -	serial_out(early_serial_base, DLL, divisor & 0xff);
> -	serial_out(early_serial_base, DLH, (divisor >> 8) & 0xff);
> -	serial_out(early_serial_base, LCR, c & ~DLAB);
> -}
> -
>  #define DEFAULT_BAUD 9600
>  
>  static __init void early_serial_init(char *s)
>  {
> +	unsigned char c;
>  	unsigned divisor;
> -	unsigned long baud = DEFAULT_BAUD;
> +	unsigned baud = DEFAULT_BAUD;
>  	char *e;
>  
>  	if (*s == ',')
> @@ -174,138 +145,23 @@ static __init void early_serial_init(char *s)
>  			s++;
>  	}
>  
> -	if (*s) {
> -		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
> -			baud = DEFAULT_BAUD;
> -	}
> -
> -	/* Convert from baud to divisor value */
> -	divisor = 115200 / baud;
> -
> -	/* These will always be IO based ports */
> -	serial_in = io_serial_in;
> -	serial_out = io_serial_out;
> -
> -	/* Set up the HW */
> -	early_serial_hw_init(divisor);
> -}
> -
> -#ifdef CONFIG_PCI
> -static void mem32_serial_out(unsigned long addr, int offset, int value)
> -{
> -	u32 *vaddr = (u32 *)addr;
> -	/* shift implied by pointer type */
> -	writel(value, vaddr + offset);
> -}
> -
> -static unsigned int mem32_serial_in(unsigned long addr, int offset)
> -{
> -	u32 *vaddr = (u32 *)addr;
> -	/* shift implied by pointer type */
> -	return readl(vaddr + offset);
> -}
> -
> -/*
> - * early_pci_serial_init()
> - *
> - * This function is invoked when the early_printk param starts with "pciserial"
> - * The rest of the param should be ",B:D.F,baud" where B, D & F describe the
> - * location of a PCI device that must be a UART device.
> - */
> -static __init void early_pci_serial_init(char *s)
> -{
> -	unsigned divisor;
> -	unsigned long baud = DEFAULT_BAUD;
> -	u8 bus, slot, func;
> -	u32 classcode, bar0;
> -	u16 cmdreg;
> -	char *e;
> -
> -
> -	/*
> -	 * First, part the param to get the BDF values
> -	 */
> -	if (*s == ',')
> -		++s;
> -
> -	if (*s == 0)
> -		return;
> -
> -	bus = (u8)simple_strtoul(s, &e, 16);
> -	s = e;
> -	if (*s != ':')
> -		return;
> -	++s;
> -	slot = (u8)simple_strtoul(s, &e, 16);
> -	s = e;
> -	if (*s != '.')
> -		return;
> -	++s;
> -	func = (u8)simple_strtoul(s, &e, 16);
> -	s = e;
> -
> -	/* A baud might be following */
> -	if (*s == ',')
> -		s++;
> -
> -	/*
> -	 * Second, find the device from the BDF
> -	 */
> -	cmdreg = read_pci_config(bus, slot, func, PCI_COMMAND);
> -	classcode = read_pci_config(bus, slot, func, PCI_CLASS_REVISION);
> -	bar0 = read_pci_config(bus, slot, func, PCI_BASE_ADDRESS_0);
> -
> -	/*
> -	 * Verify it is a UART type device
> -	 */
> -	if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
> -	     (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
> -	   (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */
> -		return;
> -
> -	/*
> -	 * Determine if it is IO or memory mapped
> -	 */
> -	if (bar0 & 0x01) {
> -		/* it is IO mapped */
> -		serial_in = io_serial_in;
> -		serial_out = io_serial_out;
> -		early_serial_base = bar0&0xfffffffc;
> -		write_pci_config(bus, slot, func, PCI_COMMAND,
> -						cmdreg|PCI_COMMAND_IO);
> -	} else {
> -		/* It is memory mapped - assume 32-bit alignment */
> -		serial_in = mem32_serial_in;
> -		serial_out = mem32_serial_out;
> -		/* WARNING! assuming the address is always in the first 4G */
> -		early_serial_base =
> -			(unsigned long)early_ioremap(bar0 & 0xfffffff0, 0x10);
> -		write_pci_config(bus, slot, func, PCI_COMMAND,
> -						cmdreg|PCI_COMMAND_MEMORY);
> -	}
> +	outb(0x3, early_serial_base + LCR);	/* 8n1 */
> +	outb(0, early_serial_base + IER);	/* no interrupt */
> +	outb(0, early_serial_base + FCR);	/* no fifo */
> +	outb(0x3, early_serial_base + MCR);	/* DTR + RTS */
>  
> -	/*
> -	 * Lastly, initalize the hardware
> -	 */
>  	if (*s) {
> -		if (strcmp(s, "nocfg") == 0)
> -			/* Sometimes, we want to leave the UART alone
> -			 * and assume the BIOS has set it up correctly.
> -			 * "nocfg" tells us this is the case, and we
> -			 * should do no more setup.
> -			 */
> -			return;
>  		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
>  			baud = DEFAULT_BAUD;
>  	}
>  
> -	/* Convert from baud to divisor value */
>  	divisor = 115200 / baud;
> -
> -	/* Set up the HW */
> -	early_serial_hw_init(divisor);
> +	c = inb(early_serial_base + LCR);
> +	outb(c | DLAB, early_serial_base + LCR);
> +	outb(divisor & 0xff, early_serial_base + DLL);
> +	outb((divisor >> 8) & 0xff, early_serial_base + DLH);
> +	outb(c & ~DLAB, early_serial_base + LCR);
>  }
> -#endif
>  
>  static struct console early_serial_console = {
>  	.name =		"earlyser",
> @@ -326,6 +182,7 @@ static inline void early_console_register(struct console *con, int keep_early)
>  		early_console->flags &= ~CON_BOOT;
>  	else
>  		early_console->flags |= CON_BOOT;
> +
>  	register_console(early_console);
>  }
>  
> @@ -353,13 +210,6 @@ static int __init setup_early_printk(char *buf)
>  			early_serial_init(buf + 4);
>  			early_console_register(&early_serial_console, keep);
>  		}
> -#ifdef CONFIG_PCI
> -		if (!strncmp(buf, "pciserial", 9)) {
> -			early_pci_serial_init(buf + 9);
> -			early_console_register(&early_serial_console, keep);
> -			buf += 9; /* Keep from match the above "serial" */
> -		}
> -#endif
>  		if (!strncmp(buf, "vga", 3) &&
>  		    boot_params.screen_info.orig_video_isVGA == 1) {
>  			max_xpos = boot_params.screen_info.orig_video_cols;
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index 6dc471e..63ae60e 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -193,6 +193,15 @@ static int __init param_setup_earlycon(char *buf)
>  }
>  early_param("earlycon", param_setup_earlycon);
>  
> +/* x86 uses "earlyprintk=xxx", so we keep the compatibility here */
> +#ifdef CONFIG_X86
> +static int __init param_setup_earlycon_x86(char *buf)
> +{
> +	return param_setup_earlycon(buf);
> +}
> +early_param("earlyprintk", param_setup_earlycon_x86);

I'm concerned that this effectively makes earlyprintk= a synonym for
earlycon=, which may have unforeseen consequences. I'd rather this
specifically parse for replacement functionality, ie., only command line
parameters of the form:

	earlyprintk=pciserial,...

Regards,
Peter Hurley

> +#endif
> +
>  int __init of_setup_earlycon(unsigned long addr,
>  			     int (*setup)(struct earlycon_device *, const char *))
>  {
> 

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Peter Hurley]

Hi Stuart,

On 06/02/2015 08:16 PM, Anderson, Stuart R wrote:
> Bin, et al,
> 
> What we are losing here is the ability to specify a UART by its bus address instead of just supplying the memory or io address. There are some cases where this is useful, though I admit it is probably not going to be widely used. I have seen a platform where the location of the UART moves depending on the firmware version, but the bus address (B:D.F) did not change. There are also some platforms where you do not know the address until you boot the OS and can use the UART to login and find the address of the UART (oops. Chicken and egg problem).

The patch title and commit log are somewhat misleading.
What this patch actually does is re-implement pciserial earlyprintk
in terms of earlycon instead, so the functionality is retained.

Patch 1/2 adds pciserial support to earlycon with command line parameters
of the forms

	earlycon=uart8250,pci[32],<B:D.F>,<options>

This patch 2/2
1. removes the x86-only pciserial bootconsole implementation, and
2. wires that behavior up to command line parameters of the forms

	earlyprintk=uart8250,pci,<B:D.F>,<options>

which is not exactly what was suggested; rather that the existing
earlyprintk= command line format for pciserial should be transparently
handled by earlycon.


Regards,
Peter Hurley

> Also, I was going to soon send a patch to allow "pciserial32" for the case where the UART registers are 32-bit aligned instead of 8-bit aligned.
> 
> Stuart
> 
> -----Original Message-----
> From: Bin Gao [mailto:bin.gao@linux.intel.com] 
> Sent: Tuesday, June 02, 2015 3:43 PM
> To: Thomas Gleixner
> Cc: Ingo Molnar; Ingo Molnar; H. Peter Anvin; Greg Kroah-Hartman; One Thousand Gnomes; Peter Hurley; Jiri Slaby; Borislav Petkov; Anderson, Stuart R; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c
> 
> On Tue, Jun 02, 2015 at 11:07:39PM +0200, Thomas Gleixner wrote:
>> What about the memory mapped uarts which have been source of trouble 
>> in the past?
>>
>> Thanks,
>>
>> 	tglx
> 
> Not sure which specific early console you are referring to.
> Currently we have serial, ttyS, vga, dbgp, xen, efi and pciserial in arch/x86/kernel/early_printk.c, and only pciserial is memory mapped (but it's being removed).
> 
> And this patch only touches pciserial/serial parts so ideally shouldn't have impact on other early consoles.
> 
> -Bin
> 

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Bin Gao]

On Wed, Jun 03, 2015 at 08:35:29AM -0400, Peter Hurley wrote:
> > +/* x86 uses "earlyprintk=xxx", so we keep the compatibility here */
> > +#ifdef CONFIG_X86
> > +static int __init param_setup_earlycon_x86(char *buf)
> > +{
> > +	return param_setup_earlycon(buf);
> > +}
> > +early_param("earlyprintk", param_setup_earlycon_x86);
> 
> I'm concerned that this effectively makes earlyprintk= a synonym for
> earlycon=, which may have unforeseen consequences. I'd rather this
> specifically parse for replacement functionality, ie., only command line
> parameters of the form:
> 
> 	earlyprintk=pciserial,...
> 
> Regards,
> Peter Hurley
> 

Something like this: ?

/*
 * x86 uses "earlyprintk=xxx", so we keep the compatibility here.
 * But we only handle the earlyprintk=uart8250,pci[32]B:D.F[,options] case.
 */
#ifdef CONFIG_X86
static int __init param_setup_earlycon_x86(char *buf)
{
        if (strncmp("uart8250,pci", 12))
                return -EINVAL;
 
        return param_setup_earlycon(buf);
}
early_param("earlyprintk", param_setup_earlycon_x86);

Thanks,
Bin

Re: [PATCH v5 2/2] arch/x86: remove pci uart early console from early_prink.c

[Thomas Gleixner]

On Tue, 2 Jun 2015, Bin Gao wrote:

> On Tue, Jun 02, 2015 at 11:07:39PM +0200, Thomas Gleixner wrote:
> > What about the memory mapped uarts which have been source of trouble
> > in the past?
> > 
> > Thanks,
> > 
> > 	tglx
> 
> Not sure which specific early console you are referring to.
> Currently we have serial, ttyS, vga, dbgp, xen, efi and pciserial in
> arch/x86/kernel/early_printk.c, and only pciserial is memory mapped
> (but it's being removed).

Removed? So you kill the only way to get early serial output on
certain machines?
 
Thanks,

	tglx

[PATCH v6 2/2] arch/x86: remove pci uart early console from early_prink.c

[Bin Gao]

The arch independent uart8250 early console driver has good
support for memory mapped and io port based 8250 uarts. Since
pci is arch independent so it's natural to extend uart8250 to
support mem, io and pci. Hence pci uart early console in
arch/x86/kernel_printk.c by the following commit:
'commit 5140fda16051 ("Specify PCI based UART for earlyprintk")'
is removed. And its equivalent function will be available from
uart8250 early console driver.

Signed-off-by: Bin Gao <bin.gao@intel.com>
---
Changes in v6:
 - limited the early parameter 'earlyprintk' in drivers/tty/serial/earlycon.c
   to uart8250,pci[32][,options] only
Changes in v5:
 - moved earlyprintk= (an alias to earlycon=) from patch 1/2 to patch 2/2
Changes in v2-v4: None
 arch/x86/kernel/early_printk.c | 180 ++++-------------------------------------
 drivers/tty/serial/earlycon.c  |  15 ++++
 2 files changed, 30 insertions(+), 165 deletions(-)

diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index 89427d8..00c2e2a 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -19,7 +19,6 @@
 #include <linux/usb/ehci_def.h>
 #include <linux/efi.h>
 #include <asm/efi.h>
-#include <asm/pci_x86.h>
 
 /* Simple VGA output */
 #define VGABASE		(__ISA_IO_base + 0xb8000)
@@ -77,7 +76,7 @@ static struct console early_vga_console = {
 
 /* Serial functions loosely based on a similar package from Klaus P. Gerlicher */
 
-static unsigned long early_serial_base = 0x3f8;  /* ttyS0 */
+static int early_serial_base = 0x3f8;  /* ttyS0 */
 
 #define XMTRDY          0x20
 
@@ -95,26 +94,13 @@ static unsigned long early_serial_base = 0x3f8;  /* ttyS0 */
 #define DLL             0       /*  Divisor Latch Low         */
 #define DLH             1       /*  Divisor latch High        */
 
-static unsigned int io_serial_in(unsigned long addr, int offset)
-{
-	return inb(addr + offset);
-}
-
-static void io_serial_out(unsigned long addr, int offset, int value)
-{
-	outb(value, addr + offset);
-}
-
-static unsigned int (*serial_in)(unsigned long addr, int offset) = io_serial_in;
-static void (*serial_out)(unsigned long addr, int offset, int value) = io_serial_out;
-
 static int early_serial_putc(unsigned char ch)
 {
 	unsigned timeout = 0xffff;
 
-	while ((serial_in(early_serial_base, LSR) & XMTRDY) == 0 && --timeout)
+	while ((inb(early_serial_base + LSR) & XMTRDY) == 0 && --timeout)
 		cpu_relax();
-	serial_out(early_serial_base, TXR, ch);
+	outb(ch, early_serial_base + TXR);
 	return timeout ? 0 : -1;
 }
 
@@ -128,28 +114,13 @@ static void early_serial_write(struct console *con, const char *s, unsigned n)
 	}
 }
 
-static __init void early_serial_hw_init(unsigned divisor)
-{
-	unsigned char c;
-
-	serial_out(early_serial_base, LCR, 0x3);	/* 8n1 */
-	serial_out(early_serial_base, IER, 0);	/* no interrupt */
-	serial_out(early_serial_base, FCR, 0);	/* no fifo */
-	serial_out(early_serial_base, MCR, 0x3);	/* DTR + RTS */
-
-	c = serial_in(early_serial_base, LCR);
-	serial_out(early_serial_base, LCR, c | DLAB);
-	serial_out(early_serial_base, DLL, divisor & 0xff);
-	serial_out(early_serial_base, DLH, (divisor >> 8) & 0xff);
-	serial_out(early_serial_base, LCR, c & ~DLAB);
-}
-
 #define DEFAULT_BAUD 9600
 
 static __init void early_serial_init(char *s)
 {
+	unsigned char c;
 	unsigned divisor;
-	unsigned long baud = DEFAULT_BAUD;
+	unsigned baud = DEFAULT_BAUD;
 	char *e;
 
 	if (*s == ',')
@@ -174,138 +145,23 @@ static __init void early_serial_init(char *s)
 			s++;
 	}
 
-	if (*s) {
-		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
-			baud = DEFAULT_BAUD;
-	}
-
-	/* Convert from baud to divisor value */
-	divisor = 115200 / baud;
-
-	/* These will always be IO based ports */
-	serial_in = io_serial_in;
-	serial_out = io_serial_out;
-
-	/* Set up the HW */
-	early_serial_hw_init(divisor);
-}
-
-#ifdef CONFIG_PCI
-static void mem32_serial_out(unsigned long addr, int offset, int value)
-{
-	u32 *vaddr = (u32 *)addr;
-	/* shift implied by pointer type */
-	writel(value, vaddr + offset);
-}
-
-static unsigned int mem32_serial_in(unsigned long addr, int offset)
-{
-	u32 *vaddr = (u32 *)addr;
-	/* shift implied by pointer type */
-	return readl(vaddr + offset);
-}
-
-/*
- * early_pci_serial_init()
- *
- * This function is invoked when the early_printk param starts with "pciserial"
- * The rest of the param should be ",B:D.F,baud" where B, D & F describe the
- * location of a PCI device that must be a UART device.
- */
-static __init void early_pci_serial_init(char *s)
-{
-	unsigned divisor;
-	unsigned long baud = DEFAULT_BAUD;
-	u8 bus, slot, func;
-	u32 classcode, bar0;
-	u16 cmdreg;
-	char *e;
-
-
-	/*
-	 * First, part the param to get the BDF values
-	 */
-	if (*s == ',')
-		++s;
-
-	if (*s == 0)
-		return;
-
-	bus = (u8)simple_strtoul(s, &e, 16);
-	s = e;
-	if (*s != ':')
-		return;
-	++s;
-	slot = (u8)simple_strtoul(s, &e, 16);
-	s = e;
-	if (*s != '.')
-		return;
-	++s;
-	func = (u8)simple_strtoul(s, &e, 16);
-	s = e;
-
-	/* A baud might be following */
-	if (*s == ',')
-		s++;
-
-	/*
-	 * Second, find the device from the BDF
-	 */
-	cmdreg = read_pci_config(bus, slot, func, PCI_COMMAND);
-	classcode = read_pci_config(bus, slot, func, PCI_CLASS_REVISION);
-	bar0 = read_pci_config(bus, slot, func, PCI_BASE_ADDRESS_0);
-
-	/*
-	 * Verify it is a UART type device
-	 */
-	if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
-	     (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
-	   (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */
-		return;
-
-	/*
-	 * Determine if it is IO or memory mapped
-	 */
-	if (bar0 & 0x01) {
-		/* it is IO mapped */
-		serial_in = io_serial_in;
-		serial_out = io_serial_out;
-		early_serial_base = bar0&0xfffffffc;
-		write_pci_config(bus, slot, func, PCI_COMMAND,
-						cmdreg|PCI_COMMAND_IO);
-	} else {
-		/* It is memory mapped - assume 32-bit alignment */
-		serial_in = mem32_serial_in;
-		serial_out = mem32_serial_out;
-		/* WARNING! assuming the address is always in the first 4G */
-		early_serial_base =
-			(unsigned long)early_ioremap(bar0 & 0xfffffff0, 0x10);
-		write_pci_config(bus, slot, func, PCI_COMMAND,
-						cmdreg|PCI_COMMAND_MEMORY);
-	}
+	outb(0x3, early_serial_base + LCR);	/* 8n1 */
+	outb(0, early_serial_base + IER);	/* no interrupt */
+	outb(0, early_serial_base + FCR);	/* no fifo */
+	outb(0x3, early_serial_base + MCR);	/* DTR + RTS */
 
-	/*
-	 * Lastly, initalize the hardware
-	 */
 	if (*s) {
-		if (strcmp(s, "nocfg") == 0)
-			/* Sometimes, we want to leave the UART alone
-			 * and assume the BIOS has set it up correctly.
-			 * "nocfg" tells us this is the case, and we
-			 * should do no more setup.
-			 */
-			return;
 		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
 			baud = DEFAULT_BAUD;
 	}
 
-	/* Convert from baud to divisor value */
 	divisor = 115200 / baud;
-
-	/* Set up the HW */
-	early_serial_hw_init(divisor);
+	c = inb(early_serial_base + LCR);
+	outb(c | DLAB, early_serial_base + LCR);
+	outb(divisor & 0xff, early_serial_base + DLL);
+	outb((divisor >> 8) & 0xff, early_serial_base + DLH);
+	outb(c & ~DLAB, early_serial_base + LCR);
 }
-#endif
 
 static struct console early_serial_console = {
 	.name =		"earlyser",
@@ -326,6 +182,7 @@ static inline void early_console_register(struct console *con, int keep_early)
 		early_console->flags &= ~CON_BOOT;
 	else
 		early_console->flags |= CON_BOOT;
+
 	register_console(early_console);
 }
 
@@ -353,13 +210,6 @@ static int __init setup_early_printk(char *buf)
 			early_serial_init(buf + 4);
 			early_console_register(&early_serial_console, keep);
 		}
-#ifdef CONFIG_PCI
-		if (!strncmp(buf, "pciserial", 9)) {
-			early_pci_serial_init(buf + 9);
-			early_console_register(&early_serial_console, keep);
-			buf += 9; /* Keep from match the above "serial" */
-		}
-#endif
 		if (!strncmp(buf, "vga", 3) &&
 		    boot_params.screen_info.orig_video_isVGA == 1) {
 			max_xpos = boot_params.screen_info.orig_video_cols;
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index 6dc471e..4ab822c 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -193,6 +193,21 @@ static int __init param_setup_earlycon(char *buf)
 }
 early_param("earlycon", param_setup_earlycon);
 
+/*
+ * X86 uses "earlyprintk=xxx", so we keep the compatibility here.
+ * But we only handle the earlyprintk=uart8250,pci[32][,options] case.
+ */
+#ifdef CONFIG_X86
+static int __init param_setup_earlycon_x86(char *buf)
+{
+	if (strncmp(buf, "uart8250,pci", 12))
+		return -EINVAL;
+
+	return param_setup_earlycon(buf);
+}
+early_param("earlyprintk", param_setup_earlycon_x86);
+#endif
+
 int __init of_setup_earlycon(unsigned long addr,
 			     int (*setup)(struct earlycon_device *, const char *))
 {
-- 
1.9.1