lkml.org 
[lkml]   [2015]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] always probe UART HW when options are not specified
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?

Anyway, do you know of a comprehensive list of options, console=ttyS0,
earlycon=uart, console=uart, stdout-path=, etc. that are tested?
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).

>
>>> 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.

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()
...
kernel_init()
...
of_platform_serial_driver_init()
...
of_platform_serial_probe()
serial8250_register_8250_port()
uart_add_one_port()
register_console()
univ8250_console_match()

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.
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"?
I mean, why would one need "earlycon" if there's already "earlyprintk"?
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?

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.

>
> 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?

Thanks, regards,


Sebastian


\
 
 \ /
  Last update: 2015-12-18 15:21    [W:0.146 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site