serial: convert early_uart to earlycon for 8250

[PATCH 4/5] serial: convert early_uart to earlycon for 8250

Beacuse SERIAL_PORT_DFNS is removed from include/asm-i386/serial.h and include/asm-x86_64/serial.h. the serial8250_ports need to be probed late in serial initializing stage. the console_init=>serial8250_console_init=> register_console=>serial8250_console_setup will return -ENDEV, and console ttyS0 can not be enabled at that time.
need to wait till uart_add_one_port in drivers/serial/serial_core.c to call register_console to get console ttyS0. that is too late.

Make early_uart to use early_param, so uart console can be used earlier. Make it to be bootconsole with CON_BOOT flag, so can use console handover feature. and it will switch to corresponding normal serial console automatically.

new command line will be:

	console=uart8250,io,0x3f8,9600n8
	console=uart8250,mmio,0xff5e0000,115200n8
or
	earlycon=uart8250,io,0x3f8,9600n8
	earlycon=uart8250,mmio,0xff5e0000,115200n8

it will print in very early stage:

	Early serial console at I/O port 0x3f8 (options '9600n8')
	console [uart0] enabled
later for console it will print:
	console handover: boot [uart0] -> real [ttyS0]

 Documentation/kernel-parameters.txt |   11 +++
 arch/ia64/kernel/setup.c            |    4 -
 drivers/serial/8250.c               |   28 ++-------
 drivers/serial/8250_early.c         |  104 ++++++++++++++----------------------
 drivers/serial/Kconfig              |   10 +++
 include/asm-i386/fixmap.h           |    2 
 include/asm-i386/io.h               |   13 ++++
 include/asm-ia64/io.h               |    4 +
 include/asm-x86_64/fixmap.h         |    4 +
 include/asm-x86_64/io.h             |   13 ++++
 include/linux/console.h             |    2 
 include/linux/serial.h              |    6 --
 include/linux/serial_8250.h         |    3 +
 init/main.c                         |    5 +
 kernel/printk.c                     |   22 +++++++
 15 files changed, 133 insertions(+), 98 deletions(-)

drivers/serial/8250_early.c +5 -1

+	.index	= -1,
+};
+
+static int __init early_serial8250_setup(char *options)
 {
-	struct early_uart_device *device = &early_device;
+	struct early_serial8250_device *device = &early_device;
 	int err;
 
 	if (device->port.membase || device->port.iobase)

include/asm-i386/io.h +0 -0

 /* Use early IO mappings for DMI because it's initialized early */
 #define dmi_ioremap bt_ioremap
 #define dmi_iounmap bt_iounmap

include/asm-ia64/io.h +0 -0

index eb17a86..e29eaf8 100644

include/asm-ia64/io.h +8 -0

+int update_console_cmdline(char *name, int idx, char *name_new, int idx_new, char *options)
+{
+	struct console_cmdline *c;
+	int i;
+
+	for (i = 0; i < MAX_CMDLINECONSOLES && console_cmdline[i].name[0]; i++)
+		if (strcmp(console_cmdline[i].name, name) == 0 &&
+			  console_cmdline[i].index == idx) {

Quoted 29 lines · On Tue, 29 May 2007 18:43:59 -0700 Yinghai Lu <[email protected]> wrote:

[PATCH 4/5] serial: convert early_uart to earlycon for 8250

Beacuse SERIAL_PORT_DFNS is removed from include/asm-i386/serial.h and include/asm-x86_64/serial.h. the serial8250_ports need to be probed late in serial initializing stage. the console_init=>serial8250_console_init=> register_console=>serial8250_console_setup will return -ENDEV, and console ttyS0 can not be enabled at that time.
need to wait till uart_add_one_port in drivers/serial/serial_core.c to call register_console to get console ttyS0. that is too late.

Make early_uart to use early_param, so uart console can be used earlier. Make it to be bootconsole with CON_BOOT flag, so can use console handover feature. and it will switch to corresponding normal serial console automatically.

new command line will be:

	console=uart8250,io,0x3f8,9600n8
	console=uart8250,mmio,0xff5e0000,115200n8
or
	earlycon=uart8250,io,0x3f8,9600n8
	earlycon=uart8250,mmio,0xff5e0000,115200n8

it will print in very early stage:

	Early serial console at I/O port 0x3f8 (options '9600n8')
	console [uart0] enabled
later for console it will print:
	console handover: boot [uart0] -> real [ttyS0]

Quoted 16 lines

 Documentation/kernel-parameters.txt |   11 +++
 arch/ia64/kernel/setup.c            |    4 -
 drivers/serial/8250.c               |   28 ++-------
 drivers/serial/8250_early.c         |  104 ++++++++++++++----------------------
 drivers/serial/Kconfig              |   10 +++
 include/asm-i386/fixmap.h           |    2 
 include/asm-i386/io.h               |   13 ++++
 include/asm-ia64/io.h               |    4 +
 include/asm-x86_64/fixmap.h         |    4 +
 include/asm-x86_64/io.h             |   13 ++++
 include/linux/console.h             |    2 
 include/linux/serial.h              |    6 --
 include/linux/serial_8250.h         |    3 +
 init/main.c                         |    5 +
 kernel/printk.c                     |   22 +++++++
 15 files changed, 133 insertions(+), 98 deletions(-)

This is a lot of tricky stuff in areas which I'm not very familar with. Need help, am not getting it.

drivers/serial/8250_early.c static int __init parse_options(struct early_uart_device *device, char *options) · line 143

Quoted 11 lines

drivers/serial/8250_early.c +1 -6

@@ -143,12 +140,7 @@ static int __init parse_options(struct early_uart_device *device, char *options)
 	if (!strncmp(options, "mmio,", 5)) {
 		port->iotype = UPIO_MEM;
 		port->mapbase = simple_strtoul(options + 5, &options, 0);
-		port->membase = ioremap(port->mapbase, mapsize);
-		if (!port->membase) {
-			printk(KERN_ERR "%s: Couldn't ioremap 0x%lx\n",
-				__FUNCTION__, port->mapbase);
-			return -ENOMEM;
-		}
+		port->membase = fix_ioremap(FIX_EARLYCON_MEM_BASE, port->mapbase);

A number of useful-looking warnigns such as the above got removed. How come?

drivers/serial/8250_early.c static int __init parse_options(struct early_uart_device *device, char *options) · line 173

Quoted 13 lines

drivers/serial/8250_early.c +4 -1

 		mmio = 1;
 	} else if (!strncmp(options, "io,", 3)) {
 		port->iotype = UPIO_PORT;

@@ -175,9 +167,16 @@ static int __init parse_options(struct early_uart_device *device, char *options)
 	return 0;
 }
 
-static int __init early_uart_setup(struct console *console, char *options)
+static struct console early_serial8250_console __initdata = {
+	.name	= "uart",
+	.write	= early_serial8250_write,
+	.flags	= CON_PRINTBUFFER,

Should this have CON_BOOT set as well?

See ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.21-rc7/2.6.21-rc7-mm2/broken-out/fixes-and-cleanups-for-earlyprintk-aka-boot-console.patch, which is now in mainline.

Review static int __init parse_options(struct early_uart_device *device, char *options) · line 174

Quoted 1 line

(quoted diff) +1 -0

+	.flags	= CON_PRINTBUFFER | CAN_BOOT;

Quoted 11 lines

drivers/serial/8250_early.c +5 -1

+	.index	= -1,
+};
+
+static int __init early_serial8250_setup(char *options)
 {
-	struct early_uart_device *device = &early_device;
+	struct early_serial8250_device *device = &early_device;
 	int err;
 
 	if (device->port.membase || device->port.iobase)

drivers/serial/8250_early.c static int __init early_uart_setup(struct console *console, char *options) · line 199

Quoted 10 lines

drivers/serial/8250_early.c +0 -7

@@ -190,61 +189,40 @@ static int __init early_uart_setup(struct console *console, char *options)
 	return 0;
 }
 
-static struct console early_uart_console __initdata = {
-	.name	= "uart",
-	.write	= early_uart_write,
-	.setup	= early_uart_setup,
-	.flags	= CON_PRINTBUFFER,
-	.index	= -1,
-};

hm, that didn't have CON_BOOT.

Review static int __init early_uart_setup(struct console *console, char *options) · line 192

Quoted 1 line

(quoted diff) +1 -0

+static inline void __iomem * fix_ioremap (unsigned idx, unsigned long phys)

I don't suppose it'd help much if I were to complain about the coding style (no space after the * or after the function name, please).

include/asm-i386/io.h extern void iounmap(volatile void __iomem *addr); · line 144

Quoted 8 lines

include/asm-i386/io.h +8 -0

+{
+	void __iomem * vaddr;
+	set_fixmap_nocache(idx, phys & PAGE_MASK);
+	vaddr = (void __iomem *)__fix_to_virt(idx);
+	vaddr += phys & ~PAGE_MASK;
+
+	return vaddr;
+}

Probably this was a bit too large for inlining.

Quoted 9 lines

include/asm-i386/io.h +0 -0

 /* Use early IO mappings for DMI because it's initialized early */
 #define dmi_ioremap bt_ioremap
 #define dmi_iounmap bt_iounmap

include/asm-ia64/io.h +0 -0

index eb17a86..e29eaf8 100644

include/asm-ia64/io.h extern void __iomem * ioremap(unsigned long offset, unsigned long size); · line 432

Quoted 10 lines

include/asm-ia64/io.h +4 -0

@@ -423,6 +423,10 @@ extern void __iomem * ioremap(unsigned long offset, unsigned long size);
 extern void __iomem * ioremap_nocache (unsigned long offset, unsigned long size);
 extern void iounmap (volatile void __iomem *addr);
 
+/* for console=uart8250,mmio,0xffe5000,9600n8 */
+#define FIX_EARLYCON_MEM_BASE 1
+#define fix_ioremap(idx, phys) ioremap(phys, 64)
+
 /* Use normal IO mappings for DMI */
 #define dmi_ioremap ioremap
 #define dmi_iounmap(x,l) iounmap(x)

So we touch ia64 as well?

Quoted 9 lines

include/asm-ia64/io.h +8 -0

+int update_console_cmdline(char *name, int idx, char *name_new, int idx_new, char *options)
+{
+	struct console_cmdline *c;
+	int i;
+
+	for (i = 0; i < MAX_CMDLINECONSOLES && console_cmdline[i].name[0]; i++)
+		if (strcmp(console_cmdline[i].name, name) == 0 &&
+			  console_cmdline[i].index == idx) {

include/asm-ia64/io.h

Quoted 10 lines

include/asm-ia64/io.h +10 -0

+				c = &console_cmdline[i];
+				memcpy(c->name, name_new, sizeof(c->name));
+				c->name[sizeof(c->name) - 1] = 0;
+				c->options = options;
+				c->index = idx_new;
+				return i;
+		}
+	/* not found */
+	return -1;
+}

Shouldn't this be __init?

serial8250_find_port_for_earlycon() had its __init removed. Why?

Quoted 21 lines

Quoted 8 lines

kernel/printk.c +8 -0

+int update_console_cmdline(char *name, int idx, char *name_new, int idx_new, char *options)
+{
+	struct console_cmdline *c;
+	int i;
+
+	for (i = 0; i < MAX_CMDLINECONSOLES && console_cmdline[i].name[0]; i++)
+		if (strcmp(console_cmdline[i].name, name) == 0 &&
+			  console_cmdline[i].index == idx) {

kernel/printk.c int __init add_preferred_console(char *name, int idx, char *options) · line 746

Quoted 10 lines

kernel/printk.c +10 -0

+				c = &console_cmdline[i];
+				memcpy(c->name, name_new, sizeof(c->name));
+				c->name[sizeof(c->name) - 1] = 0;
+				c->options = options;
+				c->index = idx_new;
+				return i;
+		}
+	/* not found */
+	return -1;
+}

Shouldn't this be __init?

Quoted 1 line

serial8250_find_port_for_earlycon() had its __init removed. Why?

with __init, the compiler will cry about .init.text and .text missection.

Quoted 21 lines · On Wed, 30 May 2007 12:58:11 -0700 Yinghai Lu <[email protected]> wrote:

Quoted 21 lines

Quoted 8 lines

kernel/printk.c +8 -0

+int update_console_cmdline(char *name, int idx, char *name_new, int idx_new, char *options)
+{
+	struct console_cmdline *c;
+	int i;
+
+	for (i = 0; i < MAX_CMDLINECONSOLES && console_cmdline[i].name[0]; i++)
+		if (strcmp(console_cmdline[i].name, name) == 0 &&
+			  console_cmdline[i].index == idx) {

kernel/printk.c int __init add_preferred_console(char *name, int idx, char *options) · line 746

Quoted 10 lines

kernel/printk.c +10 -0

+				c = &console_cmdline[i];
+				memcpy(c->name, name_new, sizeof(c->name));
+				c->name[sizeof(c->name) - 1] = 0;
+				c->options = options;
+				c->index = idx_new;
+				return i;
+		}
+	/* not found */
+	return -1;
+}

Shouldn't this be __init?

Quoted 2 lines

Quoted 1 line

serial8250_find_port_for_earlycon() had its __init removed. Why?

with __init, the compiler will cry about .init.text and .text missection.

But was this the correct fix for that? afaict the only caller of this function is __init anyway.

Quoted 27 lines · Andrew Morton

Quoted 23 lines · On Wed, 30 May 2007 12:58:11 -0700 Yinghai Lu <[email protected]> wrote:

Quoted 21 lines

Quoted 8 lines

kernel/printk.c +8 -0

+int update_console_cmdline(char *name, int idx, char *name_new, int idx_new, char *options)
+{
+	struct console_cmdline *c;
+	int i;
+
+	for (i = 0; i < MAX_CMDLINECONSOLES && console_cmdline[i].name[0]; i++)
+		if (strcmp(console_cmdline[i].name, name) == 0 &&
+			  console_cmdline[i].index == idx) {

kernel/printk.c int __init add_preferred_console(char *name, int idx, char *options) · line 746

Quoted 10 lines

kernel/printk.c +10 -0

+				c = &console_cmdline[i];
+				memcpy(c->name, name_new, sizeof(c->name));
+				c->name[sizeof(c->name) - 1] = 0;
+				c->options = options;
+				c->index = idx_new;
+				return i;
+		}
+	/* not found */
+	return -1;
+}

Shouldn't this be __init?

Quoted 1 line

serial8250_find_port_for_earlycon() had its __init removed. Why?

with __init, the compiler will cry about .init.text and .text missection.

Quoted 3 lines

But was this the correct fix for that? afaict the only caller of this function is __init anyway.

You are right.
serial8250_find_port_for_earlycon and
update_console_cmdline in kernel/printk.c
could be __init

[PATCH 4/5] serial: convert early_uart to earlycon for 8250

Quoted 1 line · On Tue, 29 May 2007 18:43:59 -0700 Yinghai Lu <[email protected]> wrote:

[PATCH 4/5] serial: convert early_uart to earlycon for 8250

drivers/serial/8250_early.c: In function 'parse_options':
drivers/serial/8250_early.c:143: error: 'FIX_EARLYCON_MEM_BASE' undeclared (first use in this function) drivers/serial/8250_early.c:143: error: (Each undeclared identifier is reported only once

That was with http://userweb.kernel.org/~akpm/config-sony.txt, but this is going to break a whole pile of architectures (alpha, powerpc, others).

Quoted 4 lines · Andrew Morton

Quoted 1 line · On Tue, 29 May 2007 18:43:59 -0700 Yinghai Lu <[email protected]> wrote:

[PATCH 4/5] serial: convert early_uart to earlycon for 8250

Quoted 6 lines

drivers/serial/8250_early.c: In function 'parse_options':
drivers/serial/8250_early.c:143: error: 'FIX_EARLYCON_MEM_BASE' undeclared (first use in this function) drivers/serial/8250_early.c:143: error: (Each undeclared identifier is reported only once

That was with http://userweb.kernel.org/~akpm/config-sony.txt, but this is going to break a whole pile of architectures (alpha, powerpc, others).

alpha, powerpc, others may need sth like ia64, if they do not support fixmap.

Quoted 9 lines · Yinghai Lu

Quoted 8 lines · Andrew Morton

Quoted 1 line · On Tue, 29 May 2007 18:43:59 -0700 Yinghai Lu <[email protected]> wrote:

[PATCH 4/5] serial: convert early_uart to earlycon for 8250

drivers/serial/8250_early.c: In function 'parse_options':
drivers/serial/8250_early.c:143: error: 'FIX_EARLYCON_MEM_BASE' undeclared (first use in this function) drivers/serial/8250_early.c:143: error: (Each undeclared identifier is reported only once

Quoted 6 lines

Quoted 2 lines

That was with http://userweb.kernel.org/~akpm/config-sony.txt, but this is going to break a whole pile of architectures (alpha, powerpc, others).

alpha, powerpc, others may need sth like ia64, if they do not support fixmap.

or create dummy fixmap.h that doesn't support fixmap.

Quoted 13 lines · On Wed, 30 May 2007 21:36:41 -0700 "Yinghai Lu" <[email protected]> wrote:

Quoted 12 lines · Yinghai Lu

Quoted 10 lines · Andrew Morton

Quoted 1 line · On Tue, 29 May 2007 18:43:59 -0700 Yinghai Lu <[email protected]> wrote:

[PATCH 4/5] serial: convert early_uart to earlycon for 8250

drivers/serial/8250_early.c: In function 'parse_options':
drivers/serial/8250_early.c:143: error: 'FIX_EARLYCON_MEM_BASE' undeclared (first use in this function) drivers/serial/8250_early.c:143: error: (Each undeclared identifier is reported only once

That was with http://userweb.kernel.org/~akpm/config-sony.txt, but this is going to break a whole pile of architectures (alpha, powerpc, others).

Quoted 3 lines

Quoted 1 line

alpha, powerpc, others may need sth like ia64, if they do not support fixmap.

or create dummy fixmap.h that doesn't support fixmap.

You could require that the architecture implement some specific function for mapping the early-uart.

Quoted 2 lines · Andrew Morton

You could require that the architecture implement some specific function for mapping the early-uart.

We could, but we already have bt_ioremap, dmi_ioremap, and early_ioremap. Do we really want to proliferate more flavors of "early ioremap"? I'd rather consolidate the ones we already have.

Quoted 8 lines · Bjorn Helgaas

Quoted 2 lines · Andrew Morton

You could require that the architecture implement some specific function for mapping the early-uart.

We could, but we already have bt_ioremap, dmi_ioremap, and early_ioremap. Do we really want to proliferate more flavors of "early ioremap"? I'd rather consolidate the ones we already have.

not really.

dhcp-umpk12-63-201:/home/yhlu/xx/xx/kernel/linux-2.6/include # grep bt_ioremap * -r
asm-i386/dmi.h:#define dmi_ioremap bt_ioremap
asm-i386/io.h: * bt_ioremap() and bt_iounmap() are for temporary early boot-time asm-i386/io.h:extern void *bt_ioremap(unsigned long offset, unsigned long size); asm-i386/io.h:#define dmi_ioremap bt_ioremap

dhcp-umpk12-63-201:/home/yhlu/xx/xx/kernel/linux-2.6/include # grep early_ioremap * -r
asm-x86_64/dmi.h:#define dmi_ioremap early_ioremap asm-x86_64/io.h:extern void *early_ioremap(unsigned long addr, unsigned long size);

dhcp-umpk12-63-201:/home/yhlu/xx/xx/kernel/linux-2.6/include # grep dmi_ioremap * -r
asm-i386/dmi.h:#define dmi_ioremap bt_ioremap asm-i386/io.h:#define dmi_ioremap bt_ioremap asm-ia64/io.h:#define dmi_ioremap ioremap asm-x86_64/dmi.h:extern void *dmi_ioremap(unsigned long addr, unsigned long size);
asm-x86_64/dmi.h:#define dmi_ioremap early_ioremap

Quoted 1 line

not really.

I'm sorry, I don't understand the point you're making.

Andrew's proposal was that we could add early_uart_ioremap to the list of bt_ioremap and dmi_ioremap.

they will need

include/asm-x86_64/io.h +3 -0

index de2cd9a..e2d66de 100644
@@ -145,6 +145,19 @@ extern void early_iounmap(void *addr, unsigned long size);
 extern void __iomem * ioremap_nocache (unsigned long offset, unsigned
 long size);
 extern void iounmap(volatile void __iomem *addr);
 
+#include <asm/pgtable.h>
+#include <asm/fixmap.h>
+

Quoted 13 lines · On Wed, 30 May 2007 21:34:35 -0700 "Yinghai Lu" <[email protected]> wrote:

they will need

include/asm-x86_64/io.h +3 -0

index de2cd9a..e2d66de 100644
@@ -145,6 +145,19 @@ extern void early_iounmap(void *addr, unsigned long size);
 extern void __iomem * ioremap_nocache (unsigned long offset, unsigned
 long size);
 extern void iounmap(volatile void __iomem *addr);
 
+#include <asm/pgtable.h>
+#include <asm/fixmap.h>
+

include/asm-x86_64/io.h extern void early_iounmap(void *addr, unsigned long size); · line 161

Quoted 10 lines

include/asm-x86_64/io.h +10 -0

+static inline void __iomem * fix_ioremap (unsigned idx, unsigned long phys)
+{
+       void __iomem * vaddr;
+       set_fixmap_nocache(idx, phys & PAGE_MASK);
+       vaddr = (void __iomem *)__fix_to_virt(idx);
+       vaddr += phys & ~PAGE_MASK;
+
+       return vaddr;
+}
+

I hope not - I just removed that inlined function. It's too large and the inclusion of pgtable.h in io.h causes all sorts of problems.

This function should go into arch/.../mm/ioremap.c

Quoted 29 lines · Andrew Morton

Quoted 13 lines · On Wed, 30 May 2007 21:34:35 -0700 "Yinghai Lu" <[email protected]> wrote:

they will need

include/asm-x86_64/io.h +3 -0

index de2cd9a..e2d66de 100644
@@ -145,6 +145,19 @@ extern void early_iounmap(void *addr, unsigned long size);
 extern void __iomem * ioremap_nocache (unsigned long offset, unsigned
 long size);
 extern void iounmap(volatile void __iomem *addr);
 
+#include <asm/pgtable.h>
+#include <asm/fixmap.h>
+

include/asm-x86_64/io.h extern void early_iounmap(void *addr, unsigned long size); · line 161

Quoted 10 lines

include/asm-x86_64/io.h +10 -0

+static inline void __iomem * fix_ioremap (unsigned idx, unsigned long phys)
+{
+       void __iomem * vaddr;
+       set_fixmap_nocache(idx, phys & PAGE_MASK);
+       vaddr = (void __iomem *)__fix_to_virt(idx);
+       vaddr += phys & ~PAGE_MASK;
+
+       return vaddr;
+}
+

I hope not - I just removed that inlined function. It's too large and the inclusion of pgtable.h in io.h causes all sorts of problems.

Quoted 2 lines

This function should go into arch/.../mm/ioremap.c

agree, but need to keep

#include <asm/fixmap.h>

and will create dummy fixmap.h for arch that doesn't support fixmap