Message-ID: <5674204C.1000907@hurleysoftware.com>
Date:	Fri, 18 Dec 2015 07:03:40 -0800
From:	Peter Hurley <peter@...leysoftware.com>
To:	Sebastian Frias <sf84@...oste.net>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-serial@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
	mason <slash.tmp@...e.fr>,
	Måns Rullgård <mans@...sr.com>
Subject: Re: [RFC PATCH] always probe UART HW when options are not specified

On 12/18/2015 05:53 AM, Sebastian Frias wrote:
> Hi Peter,
> 
> On 12/17/2015 09:09 PM, Peter Hurley wrote:
>>> It's confusing though, given there are multiple ways to express the same thing.
>>> I also found parts of the doc confusing in that regard as well.
>>> ie: there's also a "stdout-path" DT key.
>>
>> Yep. Thing is, once it goes into the command line and someone uses it,
>> it's permanent.
>>
>> That's why it's important to get the semantics right the first time
>> (which only looks easy from hindsight).
>>
> 
> I totally understand, we have the same constraints with our SDK's APIs but with major versions we drop old APIs that have been superseded.
> I would have thought that the switch to DT would have been a good opportunity to clean all that up, since it requires a change in the bootloader, right?

How would that have worked with field upgrades of the kernel but
not bootloader?


> Anyway, do you know of a comprehensive list of options, console=ttyS0, earlycon=uart, console=uart, stdout-path=, etc. that are tested?

Although the kernel command line parameters are documented in
Documentation/kernel-parameters.txt and the DT options are documented in
Documentation/devicetree/..., you're right; Documentation/serial-console.txt
has bit-rotted.

Some patches for that would be great.

In fact, most of the console-related documentation needs a re-org.


> I would figure that if there's no list, then it is not easy to create the testcases, and thus some end up not being tested (see further below).

It gets tested, because when I break something, I hear it.



>>
>>>> So
>>>>
>>>>     "console=ttyS0" w/o options    always initializes the h/w to 9600n81
>>>
>>> Ok, I see. So that's not the option we need then.
>>>
>>>>     "earlycon=uart" w/o options    starts a bootconsole w/o initializing the h/w
>>>>     "console=uart" w/o options     starts a bootconsole w/o initializing the h/w,
>>>>                                    then replaces that bootconsole with a regular
>>>>                                    console (whatever ttySn matched that port)
>>>>                                    In this case, the port is probed to discover
>>>>                                    the h/w settings. Those also become the initial
>>>>                                    settings for the /dev/ttySn device.
>>>
>>> Ok, sounds like that last one is the one we need, I will check that, thanks.
> 
> Ok, so that does not work.
> Actually, the kernel crashes (by the way, the is a potential crash on probe_baud if quot is zero, I had dealt with that on my patch)
> Indeed, "console=uart" will crash at a call to uart_parse_earlycon() on drivers/tty/serial/8250/8250_core.c:univ8250_console_match() due to options=NULL.
> I see that a similar call to uart_parse_earlycon() in drivers/tty/serial/earlycon.c does check for options!=NULL.

You need to use the format documented in Documentation/kernel-parameters.text:

	console=	[KNL] Output console device and options.

		uart[8250],io,<addr>[,options]
		uart[8250],mmio,<addr>[,options]
		uart[8250],mmio16,<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.
			MMIO inter-register address stride is either 8-bit
			(mmio), 16-bit (mmio16), or 32-bit (mmio32).
			If none of [io|mmio|mmio16|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.

The iotype and the uart address are not options.
Otherwise, on console startup the driver doesn't know which uart to match.



> If we add a simple:
>     
>     if (!options)
>         return -ENODEV;
> 
> then the kernel does not crashes but it appears that the console is not properly brought up, and once that we reach:
> 
> [    0.353378] bootconsole [earlycon0] disabled
> 
> we loose it (ie: there are no more logs)
> 
> I think the whole process is too involved and I'm not sure I understand it all.
> univ8250_console_match() seems to be called twice, here's the calltrace:
> 
> console_init()
>    register_console()
>       univ8250_console_match()

Console matching failed here: probably because the driver's not yet initialized.
Only ISA type ports/early_serial_setup() ports can load a console here
because driver probing hasn't yet happened. This is very early here.

> ...
> kernel_init()
>    ...
>       of_platform_serial_driver_init()
>       ...
>          of_platform_serial_probe()
>             serial8250_register_8250_port()
>                uart_add_one_port()
>                   register_console()
>                      univ8250_console_match()

This is where your console will take over from earlycon.


> 
> Since options=NULL both times, I think the console is never brought up properly.
> 
> I thus used a less obvious (at first) solution:
> 
>     if (!options)
>         return univ8250_console_setup(co, options);
> 
> however, since univ8250_console_setup() does not forces a probe, and options=NULL, it overwrites the UART config with '9600n8r'.
> 
> So, I still think we need to change serial8250_console_setup() and the "rfc patch" I had proposed is still ok for this.

Again, what about the existing installations that have a kernel command line
like "console=ttyS0" and expect 9600n81 line settings?

How are you going to go around and update all those command lines?


> I can remove the probing of the parity, bits, etc. but it looks like it would end up in a half cooked patch, in the sense that sentences like:
> 
>      "console=uart" w/o options     starts a bootconsole w/o initializing the h/w,
> 
> would come with some undocumented limitations.
> 
> Let me know what you think.
> 
>>>
>>>>
>>>> earlyprintk is implemented by arch-specific code, whereas earlycon is implemented
>>>> by the serial driver code.
>>>>
>>>> Since earlyprintk is implemented in the arch code, it can be tweaked for
>>>> earlier use than early param parsing. There were some patches earlier this
>>>> year for x86 to initialize earlyprintk very early; not sure if they were
>>>> ever upstreamed. On ARM, earlyprintk is debug_ll.
>>>
>>> So there are 3 levels of console?
>>> earlyprintk: before early param
>>> earlycon: early param?
>>> console: after early param?
>>>
>>> What's the use case for earlycon if earlyprink is operational by then?
>>
>> They serve different masters.
>>
>> Earlyprintk can be crucial for debugging arch-dependent code. For example,
>> earlycon expects page tables to be setup, whereas earlyprintk on many
>> arches does not. Earlyprintk is not tied to the driver source at all.
>>
>> Earlycon is arch-independent and lives with the serial drivers. This makes
>> it more suitable to support different flavors of serial h/w. Earlycon is
>> now the boot console for driver developers and post-early init.
> 
> Ok, thanks for the explanation.
> Out of curiosity:
> Do you know what is the difference between "earlycon" and "console"?

earlycon= starts a boot console only
console= will start a boot console if it finds an earlycon match and then
         start a regular console that "takes over" from the boot console


> I mean, why would one need "earlycon" if there's already "earlyprintk"?

You need to think about this from other developers' points-of-view.

Suppose there was no earlycon, and you needed to initialize your
8250-work-alike-but-not-clone? Are you going to add RT2880 register layout
to all the various arches for earlyprintk support? Trust me, those arches are
going to be unhappy about that. Multiply that by all the serial consoles
and that's an insurmountable problem.

Whereas adding earlycon support for every arch at once is trivial.


> Why does it matter if support is in arch-dependent or arch-independent code?, as long as it works, it shouldn't matter, right?
> Why couldn't driver developers use the "earlyprintk" facilities?

Sure, if earlyprintk works for you, by all means, please use it.

But it strikes me that it actually doesn't work for you because earlyprintk
doesn't do console hand-off, which is what you want.


> Sorry for all the questions, I'm just curious about all these facilities.
> I mean, maintaining all of them requires work and is error prone (as the crash above shows), so I'd like to understand why are you guys keeping them all.

No need to apologize for questions.


>> I've noticed an increasing tendency for shipping product to also use
>> earlyprintk/earlycon; I think this is a terrible idea. Boot consoles should
>> be for debugging only.
>>
>>
>>
>>>> Feel free to submit regular patches; reading the divisor via the 8250 port
>>>> driver is definitely a good idea.
>>>>
>>>> Not to sure about probing for other than baud though; do you really want
>>>> 7 data bits and even parity?  Or are you just trying to get enable h/w
>>>> flow?
>>>>
>>>
>>> Actually, I was doing that for completeness, I can remove that part
>>> of the code if it is wrong or unnecessary, although I'd thought that
>>> one always wanted correct code.
>>
>> Well, it's just one more thing to have to maintain, so if you don't actually
>> need those features, I'd rather not add that.
> 
> Ok, what about posting that as a separate patch in case somebody else needs it, would that be ok with you?

Definitely; patches are always welcome.

Plus Greg may disagree and want to take up the patch anyway.

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists