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

[Yinghai Lu <Yinghai.Lu@Sun.COM>]

Andrew Morton wrote:
> On Tue, 29 May 2007 18:43:59 -0700
> Yinghai Lu <Yinghai.Lu@Sun.COM> 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]
>>
>>
>>  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.
> 
>> @@ -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?
earycon, when you can not get fix_ioremap work, you can not get anything from boot.
> 
>>  		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.
> 

I put that into setup_early_serial8250_console.

-int __init early_serial_console_init(char *cmdline)
+static int __init setup_early_serial8250_console(char *cmdline)
  {
         char *options;
         int err;

-       options = strstr(cmdline, "console=uart,");
+       options = strstr(cmdline, "uart8250,");
         if (!options)
-               return -ENODEV;
+               return 0;

         options = strchr(cmdline, ',') + 1;
-       if ((err = early_uart_setup(NULL, options)) < 0)
+       if ((err = early_serial8250_setup(options)) < 0)
                 return err;
-       return early_uart_console_init();
+
+       early_serial8250_console.flags |= CON_BOOT;
+       register_console(&early_serial8250_console);
+
+       return 0;
  }

we may use
 >> +	.flags	= CON_PRINTBUFFER | CAN_BOOT;

instead that line.


>> +	.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)
>> @@ -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.
> 
>> +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).
> 
>> +{
>> +	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.

current only one calling, later will use that for usb debug cable.

> 
>>  /* Use early IO mappings for DMI because it's initialized early */
>>  #define dmi_ioremap bt_ioremap
>>  #define dmi_iounmap bt_iounmap
>> diff --git a/include/asm-ia64/io.h b/include/asm-ia64/io.h
>> index eb17a86..e29eaf8 100644
>> --- a/include/asm-ia64/io.h
>> +++ b/include/asm-ia64/io.h
>> @@ -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?
> 
>>  
>> +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) {
>> +				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?
with __init, the compiler will cry about .init.text and .text missection.

YH