Extensible console matching & direct earlycon

setup_earlycon() will now match and register the desired earlycon from the param string (as if 'earlycon=...' had been set on the command line). Use setup_earlycon() from existing arch call sites which start an earlycon directly.

Quoted 5 lines · Peter Hurley

setup_earlycon() will now match and register the desired earlycon from the param string (as if 'earlycon=...' had been set on the command line). Use setup_earlycon() from existing arch call sites which start an earlycon directly.

Hi,

Looks like this patcheset cause regression:
when set grub console to 115200, and later kernel only have

console=uart8250,io,0x3f8

the kernel will revert baud rate to 9600 instead of keeping 115200.

in setup_earlycon: you say:

* Registers the earlycon console matching the earlycon specified
* in the param string @buf. Acceptable param strings are of the form

 *         <name>,io|mmio|mmio32,<addr>,<options>
 *         <name>,0x<addr>,<options>
 *         <name>,<options>
 *         <name>
 *
 *      Only for the third form does the earlycon setup() method receive the
 *      <options> string in the 'options' parameter; all other forms set
 *      the parameter to NULL.

so that change the old behavior that we defined in Documentation/kernel-parameters.txt

uart[8250],io,<addr>[,options] uart[8250],mmio,<addr>[,options] uart[8250],mmio32,<addr>[,options]
Start an early, polled-mode console on the 8250/16550 UART at the specified I/O port or MMIO address. MMIO inter-register address stride is either 8-bit (mmio) or 32-bit (mmio32).

The options are the same as for ttyS, above.

The old behavior: options is optional , and will use baud rate that is set by bootloader.

Please fix the problem and restore to old behavior.

The documented behavior of console=ttyS options, to which your quote refers, clearly states:

                Default is "9600n8".

Quoted 4 lines · Peter Hurley

The documented behavior of console=ttyS options, to which your quote refers, clearly states:

                Default is "9600n8".

drivers/tty/serial/8250/8250_early.c:early_serial8250_setup still have calling to probe_baud, but it is not triggered.

Here is root cause.
The gap between entries in earlycon_table cause iteration fail to find next entry, so uart8250 handler is not called proplerly.

attached patch fix the problem.

Would you please try the patch below instead?

--- >% ---
From: Peter Hurley <[email protected]>
Subject: [PATCH] earlycon: Fix __earlycon_table stride

Signed-off-by Peter Hurley <[email protected]>

---
 include/asm-generic/vmlinux.lds.h | 2 +-
 include/linux/serial_core.h       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

include/asm-generic/vmlinux.lds.h +1 -1

index 7b0ef49..2e11f31 100644
@@ -151,7 +151,7 @@
 #endif
 
 #ifdef CONFIG_SERIAL_EARLYCON
-#define EARLYCON_TABLE() . = ALIGN(8);                         \
+#define EARLYCON_TABLE() STRUCT_ALIGN();                       \
                         VMLINUX_SYMBOL(__earlycon_table) = .;  \
                         *(__earlycon_table)                    \
                         *(__earlycon_table_end)

include/linux/serial_core.h +0 -0

index 34de168..025dad9 100644

Quoted 2 lines · Peter Hurley

Would you please try the patch below instead?

Great. that works, and less lines change than my version.

Quoted 4 lines · Yinghai Lu

Quoted 2 lines · Peter Hurley

Would you please try the patch below instead?

Quoted 2 lines

Great. that works, and less lines change than my version.

still have another problem.
when using console=uart8250,io,0x3f8
it works as earlycon at first.
but after handover to normal console
it will revert back to 9600 again.

Quoted 5 lines · Yinghai Lu

Quoted 4 lines · Yinghai Lu

Quoted 2 lines · Peter Hurley

Would you please try the patch below instead?

Quoted 8 lines

Quoted 2 lines

Great. that works, and less lines change than my version.

still have another problem.
when using console=uart8250,io,0x3f8
it works as earlycon at first.
but after handover to normal console
it will revert back to 9600 again.

this regression should be caused by:

commit c7cef0a84912cab3c9df8949b034e4aa62982ec9

Author: Peter Hurley <[email protected]>
Date:   Mon Mar 9 16:27:12 2015 -0400

console: Add extensible console matching

Quoted 15 lines · Yinghai Lu

Quoted 13 lines · Yinghai Lu

Quoted 6 lines · Yinghai Lu

Quoted 2 lines · Peter Hurley

Would you please try the patch below instead?

Great. that works, and less lines change than my version.

still have another problem.
when using console=uart8250,io,0x3f8
it works as earlycon at first.
but after handover to normal console
it will revert back to 9600 again.

Quoted 7 lines

this regression should be caused by:

commit c7cef0a84912cab3c9df8949b034e4aa62982ec9

Author: Peter Hurley <[email protected]>
Date:   Mon Mar 9 16:27:12 2015 -0400

console: Add extensible console matching

Quoted 12 lines

Quoted 12 lines

Quoted 5 lines

still have another problem.
when using console=uart8250,io,0x3f8
it works as earlycon at first.
but after handover to normal console
it will revert back to 9600 again.

this regression should be caused by:

commit c7cef0a84912cab3c9df8949b034e4aa62982ec9

Author: Peter Hurley <[email protected]>
Date:   Mon Mar 9 16:27:12 2015 -0400

Quoted 2 lines

Quoted 1 line

console: Add extensible console matching

So your whole patchset will need the first patch ?

or can you just drop the first one patch ?

Quoted 14 lines · Yinghai Lu

Quoted 14 lines

Quoted 13 lines

Quoted 5 lines

still have another problem.
when using console=uart8250,io,0x3f8
it works as earlycon at first.
but after handover to normal console
it will revert back to 9600 again.

this regression should be caused by:

commit c7cef0a84912cab3c9df8949b034e4aa62982ec9

Author: Peter Hurley <[email protected]>
Date:   Mon Mar 9 16:27:12 2015 -0400

console: Add extensible console matching

Quoted 3 lines

So your whole patchset will need the first patch ?

or can you just drop the first one patch ?

Please check attached patch that fix regresion by. commit c7cef0a849 ("console: Add extensible console matching")

or you have better way?

Quoted 16 lines · Yinghai Lu

Quoted 14 lines · Yinghai Lu

Quoted 14 lines

Quoted 13 lines

Quoted 5 lines

still have another problem.
when using console=uart8250,io,0x3f8
it works as earlycon at first.
but after handover to normal console
it will revert back to 9600 again.

this regression should be caused by:

commit c7cef0a84912cab3c9df8949b034e4aa62982ec9

Author: Peter Hurley <[email protected]>
Date:   Mon Mar 9 16:27:12 2015 -0400

console: Add extensible console matching

Quoted 8 lines

Quoted 3 lines

So your whole patchset will need the first patch ?

or can you just drop the first one patch ?

Please check attached patch that fix regresion by. commit c7cef0a849 ("console: Add extensible console matching")

or you have better way?

Please re-read my first response to this problem.

Also, this expectation is an impediment when adding support for other 8250-like designs that don't have the same 8250 divisor registers (ie., _every_ new design). To properly support this requirement for just the existing 8250 hardware will require special probe_baud() functions for: dw_8250, intel byt, intel mid, omap_8250, exar 17v35 series, omap 1510.

Is specifying the line speed on the command line really a burden?

Quoted 29 lines · Peter Hurley

Quoted 16 lines · Yinghai Lu

Quoted 14 lines · Yinghai Lu

Quoted 14 lines

Quoted 13 lines

Quoted 5 lines

still have another problem.
when using console=uart8250,io,0x3f8
it works as earlycon at first.
but after handover to normal console
it will revert back to 9600 again.

this regression should be caused by:

commit c7cef0a84912cab3c9df8949b034e4aa62982ec9

Author: Peter Hurley <[email protected]>
Date:   Mon Mar 9 16:27:12 2015 -0400

console: Add extensible console matching

Quoted 8 lines

Quoted 3 lines

So your whole patchset will need the first patch ?

or can you just drop the first one patch ?

Please check attached patch that fix regresion by. commit c7cef0a849 ("console: Add extensible console matching")

or you have better way?

Please re-read my first response to this problem.

Quoted 6 lines

Also, this expectation is an impediment when adding support for other 8250-like designs that don't have the same 8250 divisor registers (ie., _every_ new design). To properly support this requirement for just the existing 8250 hardware will require special probe_baud() functions for: dw_8250, intel byt, intel mid, omap_8250, exar 17v35 series, omap 1510.

But you can not break old setup!

old behavior: console=uart8250,io,0x3f8

when bootloader or firmware has 115200 used already, earlycon, and console all keep 115200.

Now with your change, earlycon will use 115200 still, but console will switch to 9600.

Quoted 1 line

Quoted 2 lines

Is specifying the line speed on the command line really a burden?

Yes. We have two setups, and two BIOS versions. product version is using 9600 and devel/debug is using 115200.

This is a regression, and must be addressed.

Greg,

Please don't send merge request for tty-next to Linus before this regression get resolved.

On 04/03/2015 12:57 PM, Yinghai Lu wrote:

Wait -- you have earlycon in a product??

Quoted 3 lines · Peter Hurley

On 04/03/2015 12:57 PM, Yinghai Lu wrote:

Wait -- you have earlycon in a product??

What do you mean?

when you are using "console=uart8250,io,0x3f8" you will have earlycon at first, and handover to console.

I have no problem fixing this _if this is causing actual regressions_, and not simply some minor inconvenience that can be fixed by editing your grub command line.

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Quoted 6 lines · Peter Hurley

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Please separated bug fix and other documentation change to different patches.

also fix the patch title to make it clear and you need to mention which commit cause the regression.

This patch fix regression for the hand over. Thanks.

Another regression.
when user have
console=uart8250,io,0x3f8 console=uart8250,io,0x2f8

before your patchset:
port_0x3f8 is early console, and will be console later. and port_0x2f8 is ignored, because only ONE early console is allowed.
and old console setup, only handle ttyS0.

after your patchset:
port_0x3f8 is early console, and will be console later. port_0x2f8 will become default console, as new console with match method
treat all uart8250 as ttyS0.

Please fix that too.

Quoted 9 lines · Yinghai Lu

Another regression.
when user have
console=uart8250,io,0x3f8 console=uart8250,io,0x2f8

before your patchset:
port_0x3f8 is early console, and will be console later. and port_0x2f8 is ignored, because only ONE early console is allowed.
and old console setup, only handle ttyS0.

Quoted 6 lines

after your patchset:
port_0x3f8 is early console, and will be console later. port_0x2f8 will become default console, as new console with match method
treat all uart8250 as ttyS0.

Please fix that too.

That's a new feature, not a regression.

Quoted 15 lines · Peter Hurley

Quoted 14 lines · Yinghai Lu

Another regression.
when user have
console=uart8250,io,0x3f8 console=uart8250,io,0x2f8

before your patchset:
port_0x3f8 is early console, and will be console later. and port_0x2f8 is ignored, because only ONE early console is allowed.
and old console setup, only handle ttyS0.

after your patchset:
port_0x3f8 is early console, and will be console later. port_0x2f8 will become default console, as new console with match method
treat all uart8250 as ttyS0.

Quoted 3 lines

Quoted 1 line

Please fix that too.

That's a new feature, not a regression.

So, in all seriousness, you actually have this setup and want me to fix this?

IOW, to support more than 1 earlycon in the future will require using a totally different command line parameter. Awesome.

Quoted 9 lines · Yinghai Lu

Quoted 6 lines · Peter Hurley

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Quoted 1 line

Please separated bug fix and other documentation change to different patches.

No. The documentation reflects exactly what the bug fix is for.

Quoted 3 lines

also fix the patch title to make it clear and you need to mention which commit cause the regression.

Please bisect and send bisect log so I know which commit broke your setup.

Quoted 12 lines

This patch fix regression for the hand over. Thanks.

Another regression.
when user have
console=uart8250,io,0x3f8 console=uart8250,io,0x2f8

before your patchset:
port_0x3f8 is early console, and will be console later. and port_0x2f8 is ignored, because only ONE early console is allowed.
and old console setup, only handle ttyS0.

Quoted 6 lines

after your patchset:
port_0x3f8 is early console, and will be console later. port_0x2f8 will become default console, as new console with match method
treat all uart8250 as ttyS0.

Please fix that too.

That's a new feature, not a regression.

Quoted 10 lines · Peter Hurley

Quoted 9 lines · Yinghai Lu

Quoted 6 lines · Peter Hurley

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Quoted 3 lines

Quoted 1 line

Please separated bug fix and other documentation change to different patches.

No. The documentation reflects exactly what the bug fix is for.

what is

Quoted 5 lines

Quoted 2 lines

also fix the patch title to make it clear and you need to mention which commit cause the regression.

Please bisect and send bisect log so I know which commit broke your setup.

I already gave your the patch "title". Here it is again:

commit c7cef0a84912cab3c9df8949b034e4aa62982ec9

Author: Peter Hurley <[email protected]>
Date:   Mon Mar 9 16:27:12 2015 -0400

console: Add extensible console matching

Quoted 11 lines

Quoted 11 lines

This patch fix regression for the hand over. Thanks.

Another regression.
when user have
console=uart8250,io,0x3f8 console=uart8250,io,0x2f8

before your patchset:
port_0x3f8 is early console, and will be console later. and port_0x2f8 is ignored, because only ONE early console is allowed.
and old console setup, only handle ttyS0.

Quoted 8 lines

Quoted 6 lines

after your patchset:
port_0x3f8 is early console, and will be console later. port_0x2f8 will become default console, as new console with match method
treat all uart8250 as ttyS0.

Please fix that too.

That's a new feature, not a regression.

ok, think that more.

with this patch:

[PATCH] earlycon: 8250: Fix command line regression

When you have console=uart8250,io,0x3f8 console=uart,io,0x2f8

you will skip the setup to 0x2f8, as the options is null. then if the bios or bootloader only setup baud rate for 0x3f8. the user could get nothing from 0x2f8.

Do you need to call univ8250_console_setup(co, options) to set the default baud rate ?

All this automatic behavior and aliases for the same things just adds unnecessary complexity and maintenance burden, without measurable gain.

The real tragedy is that all this was totally unnecessary; what was wrong with "earlycon=uart,io,0x3f8 console=ttyS0"?

Quoted 6 lines · Peter Hurley

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

This patch fix regression for the hand over. Thanks.

Quoted 9 lines · Yinghai Lu

Quoted 6 lines · Peter Hurley

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Quoted 1 line

This patch fix regression for the hand over. Thanks.

Just now noticed, this patch only fix partial problem. Kernel console is ok.

But later initrd/init scripts revert back to 9600 again.

Quoted 10 lines · Yinghai Lu

Quoted 9 lines · Yinghai Lu

Quoted 6 lines · Peter Hurley

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Quoted 5 lines

Quoted 1 line

This patch fix regression for the hand over. Thanks.

Just now noticed, this patch only fix partial problem. Kernel console is ok.

But later initrd/init scripts revert back to 9600 again.

Please share the init scripts.

Quoted 13 lines · Peter Hurley

Quoted 12 lines · Yinghai Lu

Quoted 10 lines · Yinghai Lu

Quoted 6 lines · Peter Hurley

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

This patch fix regression for the hand over. Thanks.

Quoted 5 lines

Quoted 3 lines

Just now noticed, this patch only fix partial problem. Kernel console is ok.

But later initrd/init scripts revert back to 9600 again.

Please share the init scripts.

I am using opensuse 13.1 rescue disk as ramdisk.

Quoted 13 lines · Peter Hurley

Quoted 12 lines · Yinghai Lu

Quoted 10 lines · Yinghai Lu

Quoted 6 lines · Peter Hurley

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

This patch fix regression for the hand over. Thanks.

Quoted 5 lines

Quoted 3 lines

Just now noticed, this patch only fix partial problem. Kernel console is ok.

But later initrd/init scripts revert back to 9600 again.

Please share the init scripts.

I am using opensuse 13.1 rescue disk as ramdisk.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Just now noticed, this patch only fix partial problem. Kernel console is ok.

But later initrd/init scripts revert back to 9600 again.

Please share the init scripts.

I am using opensuse 13.1 rescue disk as ramdisk.

NVM, I know what the problem is.

What is NVM here?

Quoted 12 lines · Yinghai Lu

Quoted 11 lines · Peter Hurley

Quoted 10 lines · Yinghai Lu

Quoted 9 lines · Yinghai Lu

Quoted 6 lines · Peter Hurley

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

This patch fix regression for the hand over. Thanks.

Just now noticed, this patch only fix partial problem. Kernel console is ok.

But later initrd/init scripts revert back to 9600 again.

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Document the required behavior of the original implementation.

Fixes c7cef0a84912cab3c9df8 ("console: Add extensible console matching")

Reported-by Yinghai Lu <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---

v3: Fixed automatic console to port line settings initialization;
open-coded serial8250_console_setup() so the baud can be probed; added sha reference in commit log

v2: Fixed regression which allowed "console=uart1337,..." to start a
console (but not an earlycon)
+ fixed earlycon= documentation related required behavior fixed by this patch

 Documentation/kernel-parameters.txt  | 18 ++++++++++++++---
 drivers/tty/serial/8250/8250_core.c  | 38 +++++++++++++++++++++++++++++++++---
 drivers/tty/serial/8250/8250_early.c | 19 ------------------
 3 files changed, 50 insertions(+), 25 deletions(-)

Documentation/kernel-parameters.txt +15 -3

index bfcb1a6..1facf0b 100644
@@ -713,10 +713,18 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 		uart[8250],io,<addr>[,options]
 		uart[8250],mmio,<addr>[,options]
+		uart[8250],mmio32,<addr>[,options]
+		uart[8250],0x<addr>[,options]
 			Start an early, polled-mode console on the 8250/16550
 			UART at the specified I/O port or MMIO address,
-			switching to the matching ttyS device later.  The
-			options are the same as for ttyS, above.
+			switching to the matching ttyS device later.
+			MMIO inter-register address stride is either 8-bit
+			(mmio) or 32-bit (mmio32).
+			If none of [io|mmio|mmio32], <addr> is assumed to be
+			equivalent to 'mmio'. 'options' are specified in the
+			same format described for ttyS above; if unspecified,
+			the h/w is not re-initialized.
+
 		hvc<n>	Use the hypervisor console device <n>. This is for
 			both Xen and PowerPC hypervisors.
 

@@ -944,11 +952,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		uart[8250],io,<addr>[,options]
 		uart[8250],mmio,<addr>[,options]
 		uart[8250],mmio32,<addr>[,options]
+		uart[8250],0x<addr>[,options]
 			Start an early, polled-mode console on the 8250/16550
 			UART at the specified I/O port or MMIO address.
 			MMIO inter-register address stride is either 8-bit
 			(mmio) or 32-bit (mmio32).
-			The options are the same as for ttyS, above.
+			If none of [io|mmio|mmio32], <addr> is assumed to be
+			equivalent to 'mmio'. 'options' are specified in the
+			same format described for "console=ttyS<n>"; if
+			unspecified, the h/w is not initialized.
 
 		pl011,<addr>
 			Start an early, polled-mode console on a pl011 serial

drivers/tty/serial/8250/8250_core.c +0 -0

index e0fb5f0..f59c7a0 100644

Quoted 78 lines · Peter Hurley

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Document the required behavior of the original implementation.

Fixes c7cef0a84912cab3c9df8 ("console: Add extensible console matching")

Reported-by Yinghai Lu <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---

v3: Fixed automatic console to port line settings initialization;
open-coded serial8250_console_setup() so the baud can be probed; added sha reference in commit log

v2: Fixed regression which allowed "console=uart1337,..." to start a
console (but not an earlycon)
+ fixed earlycon= documentation related required behavior fixed by this patch

 Documentation/kernel-parameters.txt  | 18 ++++++++++++++---
 drivers/tty/serial/8250/8250_core.c  | 38 +++++++++++++++++++++++++++++++++---
 drivers/tty/serial/8250/8250_early.c | 19 ------------------
 3 files changed, 50 insertions(+), 25 deletions(-)

Documentation/kernel-parameters.txt +15 -3

index bfcb1a6..1facf0b 100644
@@ -713,10 +713,18 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 		uart[8250],io,<addr>[,options]
 		uart[8250],mmio,<addr>[,options]
+		uart[8250],mmio32,<addr>[,options]
+		uart[8250],0x<addr>[,options]
 			Start an early, polled-mode console on the 8250/16550
 			UART at the specified I/O port or MMIO address,
-			switching to the matching ttyS device later.  The
-			options are the same as for ttyS, above.
+			switching to the matching ttyS device later.
+			MMIO inter-register address stride is either 8-bit
+			(mmio) or 32-bit (mmio32).
+			If none of [io|mmio|mmio32], <addr> is assumed to be
+			equivalent to 'mmio'. 'options' are specified in the
+			same format described for ttyS above; if unspecified,
+			the h/w is not re-initialized.
+
 		hvc<n>	Use the hypervisor console device <n>. This is for
 			both Xen and PowerPC hypervisors.
 

@@ -944,11 +952,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		uart[8250],io,<addr>[,options]
 		uart[8250],mmio,<addr>[,options]
 		uart[8250],mmio32,<addr>[,options]
+		uart[8250],0x<addr>[,options]
 			Start an early, polled-mode console on the 8250/16550
 			UART at the specified I/O port or MMIO address.
 			MMIO inter-register address stride is either 8-bit
 			(mmio) or 32-bit (mmio32).
-			The options are the same as for ttyS, above.
+			If none of [io|mmio|mmio32], <addr> is assumed to be
+			equivalent to 'mmio'. 'options' are specified in the
+			same format described for "console=ttyS<n>"; if
+			unspecified, the h/w is not initialized.
 
 		pl011,<addr>
 			Start an early, polled-mode console on a pl011 serial

drivers/tty/serial/8250/8250_core.c +0 -0

index e0fb5f0..f59c7a0 100644
@@ -3447,6 +3447,22 @@ static int univ8250_console_setup(struct console *co, char *options)
 	return serial8250_console_setup(up, options);
 }
 

drivers/tty/serial/8250/8250_core.c static int univ8250_console_setup(struct console *co, char *options) · line 3450

Quoted 1 line

drivers/tty/serial/8250/8250_core.c +1 -0

+/* FIXME: this is broken on most other 8250 h/w */

What do you mean by "most other"? What hardware does this work for? What is it broken for? What is someone supposed to think/do with this comment?

Quoted 79 lines · Greg Kroah-Hartman

Quoted 78 lines · Peter Hurley

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Document the required behavior of the original implementation.

Fixes c7cef0a84912cab3c9df8 ("console: Add extensible console matching")

Reported-by Yinghai Lu <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---

v3: Fixed automatic console to port line settings initialization;
open-coded serial8250_console_setup() so the baud can be probed; added sha reference in commit log

v2: Fixed regression which allowed "console=uart1337,..." to start a
console (but not an earlycon)
+ fixed earlycon= documentation related required behavior fixed by this patch

 Documentation/kernel-parameters.txt  | 18 ++++++++++++++---
 drivers/tty/serial/8250/8250_core.c  | 38 +++++++++++++++++++++++++++++++++---
 drivers/tty/serial/8250/8250_early.c | 19 ------------------
 3 files changed, 50 insertions(+), 25 deletions(-)

Documentation/kernel-parameters.txt +15 -3

index bfcb1a6..1facf0b 100644
@@ -713,10 +713,18 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 		uart[8250],io,<addr>[,options]
 		uart[8250],mmio,<addr>[,options]
+		uart[8250],mmio32,<addr>[,options]
+		uart[8250],0x<addr>[,options]
 			Start an early, polled-mode console on the 8250/16550
 			UART at the specified I/O port or MMIO address,
-			switching to the matching ttyS device later.  The
-			options are the same as for ttyS, above.
+			switching to the matching ttyS device later.
+			MMIO inter-register address stride is either 8-bit
+			(mmio) or 32-bit (mmio32).
+			If none of [io|mmio|mmio32], <addr> is assumed to be
+			equivalent to 'mmio'. 'options' are specified in the
+			same format described for ttyS above; if unspecified,
+			the h/w is not re-initialized.
+
 		hvc<n>	Use the hypervisor console device <n>. This is for
 			both Xen and PowerPC hypervisors.
 

@@ -944,11 +952,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		uart[8250],io,<addr>[,options]
 		uart[8250],mmio,<addr>[,options]
 		uart[8250],mmio32,<addr>[,options]
+		uart[8250],0x<addr>[,options]
 			Start an early, polled-mode console on the 8250/16550
 			UART at the specified I/O port or MMIO address.
 			MMIO inter-register address stride is either 8-bit
 			(mmio) or 32-bit (mmio32).
-			The options are the same as for ttyS, above.
+			If none of [io|mmio|mmio32], <addr> is assumed to be
+			equivalent to 'mmio'. 'options' are specified in the
+			same format described for "console=ttyS<n>"; if
+			unspecified, the h/w is not initialized.
 
 		pl011,<addr>
 			Start an early, polled-mode console on a pl011 serial

drivers/tty/serial/8250/8250_core.c +0 -0

index e0fb5f0..f59c7a0 100644
@@ -3447,6 +3447,22 @@ static int univ8250_console_setup(struct console *co, char *options)
 	return serial8250_console_setup(up, options);
 }
 

Quoted 5 lines

drivers/tty/serial/8250/8250_core.c static int univ8250_console_setup(struct console *co, char *options) · line 3450

Quoted 1 line

drivers/tty/serial/8250/8250_core.c +1 -0

+/* FIXME: this is broken on most other 8250 h/w */

What do you mean by "most other"? What hardware does this work for? What is it broken for? What is someone supposed to think/do with this comment?

It's a direct copy of the existing behavior for 8250 earlycon, which is broken for h/w with non-standard divisor registers. That's basically _every_ new design, because that's what vendors need to extend to support modern bitrates.

Affected h/w that I know of includes: 8250_dw, exar xr17v35 cards, omap8250, omap15xx, intel byt, intel mid.

But it was already broken for these and so not a regression.

Quoted 2 lines · Peter Hurley

Quoted 1 line · Greg Kroah-Hartman

On Sat, Apr 04, 2015 at 10:27:30AM -0400, Peter Hurley wrote:

Quoted 14 lines

Quoted 5 lines

Quoted 1 line

+/* FIXME: this is broken on most other 8250 h/w */

What do you mean by "most other"? What hardware does this work for? What is it broken for? What is someone supposed to think/do with this comment?

It's a direct copy of the existing behavior for 8250 earlycon, which is broken for h/w with non-standard divisor registers. That's basically _every_ new design, because that's what vendors need to extend to support modern bitrates.

Affected h/w that I know of includes: 8250_dw, exar xr17v35 cards, omap8250, omap15xx, intel byt, intel mid.

Quoted 1 line

But it was already broken for these and so not a regression.

Sorry, I know the function is a copy, you haven't broken anything that wasn't already broken. I see this comment as a "rant", but we might as well turn that "rant" into something descriptive to let other people know what is really going on here.

Sound reasonable?

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Document the required behavior of the original implementation.

Quoted 7 lines · Peter Hurley

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Quoted 1 line

Document the required behavior of the original implementation.

Greg,

Please wait for a Tested-by from Yinghai before pushing; I have no platform to test if this fixes his regression.

 Documentation/kernel-parameters.txt  | 18 +++++++++++++++---
 drivers/tty/serial/8250/8250_core.c  | 37 +++++++++++++++++++++++++++++++++---
 drivers/tty/serial/8250/8250_early.c | 19 ------------------
 3 files changed, 49 insertions(+), 25 deletions(-)

Documentation/kernel-parameters.txt +0 -0

index bfcb1a6..1facf0b 100644

Documentation/kernel-parameters.txt +8 -2

                        Start an early, polled-mode console on the 8250/16550
                        UART at the specified I/O port or MMIO address,
-                       switching to the matching ttyS device later.  The
-                       options are the same as for ttyS, above.
+                       switching to the matching ttyS device later.
+                       MMIO inter-register address stride is either 8-bit
+                       (mmio) or 32-bit (mmio32).
+                       If none of [io|mmio|mmio32], <addr> is assumed to be
+                       equivalent to 'mmio'. 'options' are specified in the
+                       same format described for ttyS above; if unspecified,
+                       the h/w is not re-initialized.
+
                hvc<n>  Use the hypervisor console device <n>. This is for
                        both Xen and PowerPC hypervisors.
 

Documentation/kernel-parameters.txt +4 -1

                        Start an early, polled-mode console on the 8250/16550
                        UART at the specified I/O port or MMIO address.
                        MMIO inter-register address stride is either 8-bit
                        (mmio) or 32-bit (mmio32).
-                       The options are the same as for ttyS, above.
+                       If none of [io|mmio|mmio32], <addr> is assumed to be
+                       equivalent to 'mmio'. 'options' are specified in the
+                       same format described for "console=ttyS<n>"; if
+                       unspecified, the h/w is not initialized.
 
                pl011,<addr>
                        Start an early, polled-mode console on a pl011 serial

drivers/tty/serial/8250/8250_core.c +5 -0

index e0fb5f0..456606f 100644
@@ -3447,6 +3447,21 @@ static int univ8250_console_setup(struct console *co, char *options)
        return serial8250_console_setup(up, options);
 }
 
+static unsigned int probe_baud(struct uart_port *port)
+{
+       unsigned char lcr, dll, dlm;
+       unsigned int quot;
+

drivers/tty/serial/8250/8250_core.c +0 -0

 /**
  *     univ8250_console_match - non-standard console matching
  *     @co:      registering console

@@ -3455,13 +3470,18 @@ static int univ8250_console_setup(struct console *co, char *options)
  *     @options: ptr to option string from console command line
  *
  *     Only attempts to match console command lines of the form:

drivers/tty/serial/8250/8250_core.c +4 -0

+ *     If no <options> are specified, then assume the h/w is already setup.
+ *     This was the undocumented behavior of the original implementation so
+ *     it is cast in stone forever.
+ *
  *     Returns 0 if console matches; otherwise non-zero to use default matching
  */
 static int univ8250_console_match(struct console *co, char *name, int idx,

drivers/tty/serial/8250/8250_core.c +1 -0

        if (uart_parse_earlycon(options, &iotype, &addr, &options))
                return -ENODEV;
 
        /* try to match the port specified on the command line */
        for (i = 0; i < nr_uarts; i++) {
                struct uart_port *port = &serial8250_ports[i].port;
+               int baud, bits = 8, parity = 'n', flow = 'n';
 
                if (port->iotype != iotype)
                        continue;

@@ -3490,8 +3514,15 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
                if (iotype == UPIO_PORT && port->iobase != addr)
                        continue;
 

drivers/tty/serial/8250/8250_core.c +0 -0

        }
 
        return -ENODEV;

drivers/tty/serial/8250/8250_early.c +0 -15

index e95ebfe..8e11968 100644
@@ -105,21 +105,6 @@ static void __init early_serial8250_write(struct console *console,
                serial8250_early_out(port, UART_IER, ier);
 }
 
-static unsigned int __init probe_baud(struct uart_port *port)
-{
-       unsigned char lcr, dll, dlm;
-       unsigned int quot;
-
-       lcr = serial8250_early_in(port, UART_LCR);
-       serial8250_early_out(port, UART_LCR, lcr | UART_LCR_DLAB);
-       dll = serial8250_early_in(port, UART_DLL);
-       dlm = serial8250_early_in(port, UART_DLM);
-       serial8250_early_out(port, UART_LCR, lcr);
-
-       quot = (dlm << 8) | dll;
-       return (port->uartclk / 16) / quot;
-}
-
 static void __init init_port(struct earlycon_device *device)
 {
        struct uart_port *port = &device->port;

@@ -151,10 +136,6 @@ static int __init early_serial8250_setup(struct earlycon_device *device,
                struct uart_port *port = &device->port;

On Sat, Apr 4, 2015 at 10:19 AM, Peter Hurley <[email protected]> wrote:
...

Quoted 10 lines

 Documentation/kernel-parameters.txt  | 18 +++++++++++++++---
 drivers/tty/serial/8250/8250_core.c  | 37 +++++++++++++++++++++++++++++++++---
 drivers/tty/serial/8250/8250_early.c | 19 ------------------
 3 files changed, 49 insertions(+), 25 deletions(-)

Documentation/kernel-parameters.txt +0 -0

index bfcb1a6..1facf0b 100644

Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes can also be entirely omitted. · line 717

Quoted 5 lines

Documentation/kernel-parameters.txt +2 -0

@@ -713,10 +713,18 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
                uart[8250],io,<addr>[,options]
                uart[8250],mmio,<addr>[,options]
+               uart[8250],mmio32,<addr>[,options]
+               uart[8250],0x<addr>[,options]

put this to other patch please.

Quoted 16 lines

Documentation/kernel-parameters.txt +8 -2

                        Start an early, polled-mode console on the 8250/16550
                        UART at the specified I/O port or MMIO address,
-                       switching to the matching ttyS device later.  The
-                       options are the same as for ttyS, above.
+                       switching to the matching ttyS device later.
+                       MMIO inter-register address stride is either 8-bit
+                       (mmio) or 32-bit (mmio32).
+                       If none of [io|mmio|mmio32], <addr> is assumed to be
+                       equivalent to 'mmio'. 'options' are specified in the
+                       same format described for ttyS above; if unspecified,
+                       the h/w is not re-initialized.
+
                hvc<n>  Use the hypervisor console device <n>. This is for
                        both Xen and PowerPC hypervisors.
 

Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes can also be entirely omitted. · line 955

Quoted 4 lines

Documentation/kernel-parameters.txt +1 -0

@@ -944,11 +952,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
                uart[8250],io,<addr>[,options]
                uart[8250],mmio,<addr>[,options]
                uart[8250],mmio32,<addr>[,options]
+               uart[8250],0x<addr>[,options]

and this to another patch please.

Quoted 30 lines

Documentation/kernel-parameters.txt +4 -1

                        Start an early, polled-mode console on the 8250/16550
                        UART at the specified I/O port or MMIO address.
                        MMIO inter-register address stride is either 8-bit
                        (mmio) or 32-bit (mmio32).
-                       The options are the same as for ttyS, above.
+                       If none of [io|mmio|mmio32], <addr> is assumed to be
+                       equivalent to 'mmio'. 'options' are specified in the
+                       same format described for "console=ttyS<n>"; if
+                       unspecified, the h/w is not initialized.
 
                pl011,<addr>
                        Start an early, polled-mode console on a pl011 serial

drivers/tty/serial/8250/8250_core.c +10 -0

index e0fb5f0..456606f 100644
@@ -3447,6 +3447,21 @@ static int univ8250_console_setup(struct console *co, char *options)
        return serial8250_console_setup(up, options);
 }
 
+static unsigned int probe_baud(struct uart_port *port)
+{
+       unsigned char lcr, dll, dlm;
+       unsigned int quot;
+
+       lcr = serial_port_in(port, UART_LCR);
+       serial_port_out(port, UART_LCR, lcr | UART_LCR_DLAB);
+       dll = serial_port_in(port, UART_DLL);
+       dlm = serial_port_in(port, UART_DLM);
+       serial_port_out(port, UART_LCR, lcr);

drivers/tty/serial/8250/8250_core.c static int univ8250_console_setup(struct console *co, char *options) · line 3464

Quoted 5 lines

drivers/tty/serial/8250/8250_core.c +5 -0

+
+       quot = (dlm << 8) | dll;
+       return (port->uartclk / 16) / quot;
+}
+

You said some "newer" chips do not support probe baud. why do you move code to core ?

I was thinking some embedded guys could comment out 8280_early.c, now you get extra work for them to comment out code from 8250_core.c.

Quoted 7 lines

drivers/tty/serial/8250/8250_core.c +0 -0

 /**
  *     univ8250_console_match - non-standard console matching
  *     @co:      registering console

@@ -3455,13 +3470,18 @@ static int univ8250_console_setup(struct console *co, char *options)
  *     @options: ptr to option string from console command line
  *
  *     Only attempts to match console command lines of the form:

drivers/tty/serial/8250/8250_core.c static int univ8250_console_setup(struct console *co, char *options) · line 3480

Quoted 10 lines

drivers/tty/serial/8250/8250_core.c +3 -2

- *         console=uart<>,io|mmio|mmio32,<addr>,<options>
- *         console=uart<>,<addr>,options
+ *         console=uart[8250],io|mmio|mmio32,<addr>[,<options>]
+ *         console=uart[8250],0x<addr>[,<options>]
  *     This form is used to register an initial earlycon boot console and
  *     replace it with the serial8250_console at 8250 driver init.
  *
  *     Performs console setup for a match (as required by interface)
  *
+ *     ** HACK ALERT **

That is not HACK original.

but your fix make it to be hack.

Quoted 11 lines

drivers/tty/serial/8250/8250_core.c +4 -0

+ *     If no <options> are specified, then assume the h/w is already setup.
+ *     This was the undocumented behavior of the original implementation so
+ *     it is cast in stone forever.
+ *
  *     Returns 0 if console matches; otherwise non-zero to use default matching
  */
 static int univ8250_console_match(struct console *co, char *name, int idx,

@@ -3475,12 +3495,16 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
        if (strncmp(name, match, 4) != 0)
                return -ENODEV;
 

drivers/tty/serial/8250/8250_core.c static int univ8250_console_match(struct · line 3495

Quoted 3 lines

drivers/tty/serial/8250/8250_core.c +3 -0

+       if (idx && idx != 8250)
+               return -ENODEV;
+

Your fix is getting ugly now.

Quoted 14 lines

drivers/tty/serial/8250/8250_core.c +1 -0

        if (uart_parse_earlycon(options, &iotype, &addr, &options))
                return -ENODEV;
 
        /* try to match the port specified on the command line */
        for (i = 0; i < nr_uarts; i++) {
                struct uart_port *port = &serial8250_ports[i].port;
+               int baud, bits = 8, parity = 'n', flow = 'n';
 
                if (port->iotype != iotype)
                        continue;

@@ -3490,8 +3514,15 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
                if (iotype == UPIO_PORT && port->iobase != addr)
                        continue;
 

drivers/tty/serial/8250/8250_core.c static int univ8250_console_match(struct console *co, char *name, int idx, · line 3525

Quoted 10 lines

drivers/tty/serial/8250/8250_core.c +8 -1

+               /* link port to console */
                co->index = i;
-               return univ8250_console_setup(co, options);
+               port->cons = co;
+
+               if (options && *options)
+                       uart_parse_options(options, &baud, &parity, &bits, &flow);
+               else
+                       baud = probe_baud(port);
+               return uart_set_options(port, port->cons, baud, parity, bits, flow);

what a mess.

drivers/tty/serial/8250/8250_core.c +0 -18

 
-static int serial8250_console_early_setup(void)
-{
-	return serial8250_find_port_for_earlycon();
-}

@@ -3347,7 +3392,7 @@ static struct console serial8250_console = {
 	.write		= serial8250_console_write,
 	.device		= uart_console_device,
 	.setup		= serial8250_console_setup,
-	.early_setup	= serial8250_console_early_setup,
 	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
 	.index		= -1,
 	.data		= &serial8250_reg,

@@ -3361,19 +3406,6 @@ static int __init serial8250_console_init(void)
 
-int serial8250_find_port(struct uart_port *p)
-{
-	int line;
-	struct uart_port *port;
-
-	for (line = 0; line < nr_uarts; line++) {
-		port = &serial8250_ports[line].port;
-		if (uart_match_port(p, port))
-			return line;
-	}
-	return -ENODEV;
-}
-
 

drivers/tty/serial/8250/8250_early.c +0 -23

 
-
-int serial8250_find_port_for_earlycon(void)
-{
-	struct earlycon_device *device = early_device;
-	struct uart_port *port = device ? &device->port : NULL;
-	int line;
-	int ret;
-
-	if (!port || (!port->membase && !port->iobase))
-		return -ENODEV;
-
-	line = serial8250_find_port(port);
-	if (line < 0)
-		return -ENODEV;
-
-	ret = update_console_cmdline("uart", 8250,
-			     "ttyS", line, device->options);
-	if (ret < 0)
-		ret = update_console_cmdline("uart", 0,
-				     "ttyS", line, device->options);
-
-	return ret;
-}

include/linux/console.h +0 -1

 
-extern int update_console_cmdline(char *name, int idx, char *name_new, int idx_new, char *options);

include/linux/serial_8250.h +0 -2

 
-extern int serial8250_find_port(struct uart_port *p);
-extern int serial8250_find_port_for_earlycon(void);

kernel/printk/printk.c +0 -21

 
-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, c = console_cmdline;
-	     i < MAX_CMDLINECONSOLES && c->name[0];
-	     i++, c++)
-		if (strcmp(c->name, name) == 0 && c->index == idx) {
-			strlcpy(c->name, name_new, sizeof(c->name));
-			c->options = options;
-			c->index = idx_new;
-			return i;
-		}
-	/* not found */
-	return -1;
-}
-

@@ -2436,9 +2418,6 @@ void register_console(struct console *newcon)
 
-	if (newcon->early_setup)
-		newcon->early_setup();
-
 
 

Now sure if it is safe to move down probe_baud, when serial port is still used.

Quoted 33 lines

drivers/tty/serial/8250/8250_core.c +0 -0

        }
 
        return -ENODEV;

drivers/tty/serial/8250/8250_early.c +0 -15

index e95ebfe..8e11968 100644
@@ -105,21 +105,6 @@ static void __init early_serial8250_write(struct console *console,
                serial8250_early_out(port, UART_IER, ier);
 }
 
-static unsigned int __init probe_baud(struct uart_port *port)
-{
-       unsigned char lcr, dll, dlm;
-       unsigned int quot;
-
-       lcr = serial8250_early_in(port, UART_LCR);
-       serial8250_early_out(port, UART_LCR, lcr | UART_LCR_DLAB);
-       dll = serial8250_early_in(port, UART_DLL);
-       dlm = serial8250_early_in(port, UART_DLM);
-       serial8250_early_out(port, UART_LCR, lcr);
-
-       quot = (dlm << 8) | dll;
-       return (port->uartclk / 16) / quot;
-}
-
 static void __init init_port(struct earlycon_device *device)
 {
        struct uart_port *port = &device->port;

@@ -151,10 +136,6 @@ static int __init early_serial8250_setup(struct earlycon_device *device,
                struct uart_port *port = &device->port;
                unsigned int ier;
 

drivers/tty/serial/8250/8250_early.c static int __init early_serial8250_setup(struct earlycon_device *device,

Quoted 8 lines

drivers/tty/serial/8250/8250_early.c +0 -5

-               device->baud = probe_baud(&device->port);
-               snprintf(device->options, sizeof(device->options), "%u",
-                        device->baud);
-
                /* assume the device was initialized, only mask interrupts */
                ier = serial8250_early_in(port, UART_IER);
                serial8250_early_out(port, UART_IER, ier & UART_IER_UUE);
--

Greg, Please consider apply my fix as attached, it is much cleaner.

On 04/05/2015 03:09 AM, Yinghai Lu wrote:

On what planet is 27 loc across 4 source files cleaner than 6 loc that might be reducible to 2?

Restore undocumented behavior of kernel command line parameters of the forms:

    console=uart[8250],io|mmio|mmio32,<addr>[,options]
    console=uart[8250],<addr>[,options]

where 'options' have not been specified; in this case, the hardware is assumed to be initialized.

Document the required behavior of the original implementation.

Fixes c7cef0a84912cab3c9df8 ("console: Add extensible console matching")

Reported-by Yinghai Lu <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
v5: Refactored serial8250_console_setup() & univ8250_console_setup()
to eliminate open-coding; net +1 LOC (excludes documentation)

v4: Removed FIXME comment

v3: Fixed automatic console to port line settings initialization;
open-coded serial8250_console_setup() so the baud can be probed; added sha reference in commit log

v2: Fixed regression which allowed "console=uart1337,..." to start a
console (but not an earlycon)
+ fixed earlycon= documentation related required behavior fixed by this patch

 Documentation/kernel-parameters.txt  | 18 ++++++++++++---
 drivers/tty/serial/8250/8250_core.c  | 45 ++++++++++++++++++++++++++++--------
 drivers/tty/serial/8250/8250_early.c | 19 ---------------
 3 files changed, 50 insertions(+), 32 deletions(-)

Documentation/kernel-parameters.txt +0 -0

index bfcb1a6..1facf0b 100644