On 12/17/2015 07:15 AM, Sebastian Frias wrote:
> ---
>
> I think there are a few minor bugs on the 8250 UART code.
>
> Below you can find a patch with a proposed solution.
>
> In a nutshell:
> - probe_baud from 87515772c33ee8a0cc08d984a7d2401eeff074cd was
> converted into probe_port so that it reads all the parameters that
> uart_set_options require (namely baud, parity, bits, flow).
> - reading/writing to UART_DLL/UART_DLM directly are converted to
> using the read_dl/write_dl callbacks.
> - the port is always probed if there are no options (*).
Because I don't want to probe the port at all.
But must when using the
earlycon=ttyS0,....
command-line (because the original hack expects that behavior).
Regards,
Peter Hurley
> (*): I'm not sure why commit 87515772c33ee8a0cc08d984a7d2401eeff074cd
> makes a difference in that regard, especially considering the commit
> log states that if there are no options, the hardware is assumed to
> be already initialised. Since uart_set_options is always called, the
> current hardware setup could be overwritten with different parameters
> if the actual hardware is not probed.
>
> ---
> drivers/tty/serial/8250/8250_core.c | 84 ++++++++++++++++++++++++++---------
> 1 file changed, 63 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 2c46a21..624667f 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -791,22 +791,19 @@ static int size_fifo(struct uart_8250_port *up)
> */
> static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
> {
> - unsigned char old_dll, old_dlm, old_lcr;
> + unsigned char old_lcr;
> unsigned int id;
> + unsigned int old_dl;
>
> old_lcr = serial_in(p, UART_LCR);
> - serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
> -
> - old_dll = serial_in(p, UART_DLL);
> - old_dlm = serial_in(p, UART_DLM);
>
> - serial_out(p, UART_DLL, 0);
> - serial_out(p, UART_DLM, 0);
> + serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
>
> - id = serial_in(p, UART_DLL) | serial_in(p, UART_DLM) << 8;
> + old_dl = serial_dl_read(p);
> + serial_dl_write(p, 0);
> + id = serial_dl_read(p);
> + serial_dl_write(p, old_dl);
>
> - serial_out(p, UART_DLL, old_dll);
> - serial_out(p, UART_DLM, old_dlm);
> serial_out(p, UART_LCR, old_lcr);
>
> return id;
> @@ -3440,22 +3437,67 @@ static void univ8250_console_write(struct console *co, const char *s,
> serial8250_console_write(up, s, count);
> }
>
> -static unsigned int probe_baud(struct uart_port *port)
> +static int probe_port(struct uart_port *port, int *parity, int *bits, int *flow)
> {
> - unsigned char lcr, dll, dlm;
> + struct uart_8250_port *up = up_to_u8250p(port);
> + unsigned char lcr, efr;
> 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);
> + quot = serial_dl_read(up);
> + serial_port_out(port, UART_LCR, UART_LCR_CONF_MODE_B);
> + if (port->flags & UPF_EXAR_EFR)
> + efr = serial_port_in(port, UART_XR_EFR);
> + else
> + efr = serial_port_in(port, UART_EFR);
> serial_port_out(port, UART_LCR, lcr);
>
> - quot = (dlm << 8) | dll;
> - return (port->uartclk / 16) / quot;
> +//word length select mask
> +#define WLS_MASK (0x3)
> +//parity enable
> +#define PEN (0x8)
> +//even parity select
> +#define EPS (0x10)
> +
> + switch (lcr & WLS_MASK) {
> + case 0: // 5bits
> + case 1: // 6bits
> + // Not supported by drivers/tty/serial/serial_core.c:uart_set_options() anyway
> + WARN(true, "%s: probed uart word length (%u bits) is not supported by uart_set_options()\n", __FUNCTION__, (lcr & WLS_MASK) ? 5 : 6 );
> + break;
> + case 2: // 7bits
> + *bits = 7;
> + break;
> + case 3: // 8bits
> + *bits = 8;
> + break;
> + };
> +
> + if (lcr & PEN)
> + {
> + if (lcr & EPS)
> + *parity = 'e';
> + else
> + *parity = 'o';
> + }
> + else
> + *parity = 'n';
> +
> + if (efr & UART_EFR_CTS)
> + *flow = 'r';
> + else
> + *flow = 'n';
> +
> + if (quot)
> + return (port->uartclk / 16) / quot;
> + else
> + WARN(true, "%s: quot is zero!\n", __FUNCTION__);
> +
> + return -1;
> }
>
> -static int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
> +static int serial8250_console_setup(struct uart_port *port, char *options)
> {
> int baud = 9600;
> int bits = 8;
> @@ -3467,8 +3509,8 @@ static int serial8250_console_setup(struct uart_port *port, char *options, bool
>
> if (options)
> uart_parse_options(options, &baud, &parity, &bits, &flow);
> - else if (probe)
> - baud = probe_baud(port);
> + else
> + baud = probe_port(port, &parity, &bits, &flow);
>
> return uart_set_options(port, port->cons, baud, parity, bits, flow);
> }
> @@ -3488,7 +3530,7 @@ static int univ8250_console_setup(struct console *co, char *options)
> /* link port to console */
> port->cons = co;
>
> - return serial8250_console_setup(port, options, false);
> + return serial8250_console_setup(port, options);
> }
>
> /**
> @@ -3537,7 +3579,7 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
>
> co->index = i;
> port->cons = co;
> - return serial8250_console_setup(port, options, true);
> + return serial8250_console_setup(port, options);
> }
>
> return -ENODEV;
> --
> 1.7.10.4
>
On 12/17/2015 05:29 PM, Peter Hurley wrote:
> On 12/17/2015 07:15 AM, Sebastian Frias wrote:
>> ---
>>
>> I think there are a few minor bugs on the 8250 UART code.
>>
>> Below you can find a patch with a proposed solution.
>>
>> In a nutshell:
>> - probe_baud from 87515772c33ee8a0cc08d984a7d2401eeff074cd was
>> converted into probe_port so that it reads all the parameters that
>> uart_set_options require (namely baud, parity, bits, flow).
>> - reading/writing to UART_DLL/UART_DLM directly are converted to
>> using the read_dl/write_dl callbacks.
>> - the port is always probed if there are no options (*).
>
> Because I don't want to probe the port at all.
>
> But must when using the
> earlycon=ttyS0,....
>
> command-line (because the original hack expects that behavior).
Ok, we are using:
"console=ttyS0 earlyprintk"
and the 8250 (with CONFIG_SERIAL_8250_RT288X=y) driver.
The hardware is setup prior to Linux boot.
We don't want Linux to change the UART settings, just to pick up
whatever settings the UART has and take over UART.
How do you suggest we do that? Right now, since it does not probe, it
just messes up the UART config setup before booting Linux.
While on the subject, do you think you could explain the difference (or
similarity) between:
- "console=ttyS0"
- "console=uart"
- "earlycon=uart"
and how they relate to "earlyprintk" (if at all)?
Maybe some of those options are soon to be deprecated and we'd like to
stick with the standard and future-proof way.
>
> Regards,
> Peter Hurley
>
>> (*): I'm not sure why commit 87515772c33ee8a0cc08d984a7d2401eeff074cd
>> makes a difference in that regard, especially considering the commit
>> log states that if there are no options, the hardware is assumed to
>> be already initialised. Since uart_set_options is always called, the
>> current hardware setup could be overwritten with different parameters
>> if the actual hardware is not probed.
>>
>> ---
>> drivers/tty/serial/8250/8250_core.c | 84 ++++++++++++++++++++++++++---------
>> 1 file changed, 63 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index 2c46a21..624667f 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -791,22 +791,19 @@ static int size_fifo(struct uart_8250_port *up)
>> */
>> static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
>> {
>> - unsigned char old_dll, old_dlm, old_lcr;
>> + unsigned char old_lcr;
>> unsigned int id;
>> + unsigned int old_dl;
>>
>> old_lcr = serial_in(p, UART_LCR);
>> - serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
>> -
>> - old_dll = serial_in(p, UART_DLL);
>> - old_dlm = serial_in(p, UART_DLM);
>>
>> - serial_out(p, UART_DLL, 0);
>> - serial_out(p, UART_DLM, 0);
>> + serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
>>
>> - id = serial_in(p, UART_DLL) | serial_in(p, UART_DLM) << 8;
>> + old_dl = serial_dl_read(p);
>> + serial_dl_write(p, 0);
>> + id = serial_dl_read(p);
>> + serial_dl_write(p, old_dl);
>>
>> - serial_out(p, UART_DLL, old_dll);
>> - serial_out(p, UART_DLM, old_dlm);
>> serial_out(p, UART_LCR, old_lcr);
>>
>> return id;
>> @@ -3440,22 +3437,67 @@ static void univ8250_console_write(struct console *co, const char *s,
>> serial8250_console_write(up, s, count);
>> }
>>
>> -static unsigned int probe_baud(struct uart_port *port)
>> +static int probe_port(struct uart_port *port, int *parity, int *bits, int *flow)
>> {
>> - unsigned char lcr, dll, dlm;
>> + struct uart_8250_port *up = up_to_u8250p(port);
>> + unsigned char lcr, efr;
>> 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);
>> + quot = serial_dl_read(up);
>> + serial_port_out(port, UART_LCR, UART_LCR_CONF_MODE_B);
>> + if (port->flags & UPF_EXAR_EFR)
>> + efr = serial_port_in(port, UART_XR_EFR);
>> + else
>> + efr = serial_port_in(port, UART_EFR);
>> serial_port_out(port, UART_LCR, lcr);
>>
>> - quot = (dlm << 8) | dll;
>> - return (port->uartclk / 16) / quot;
>> +//word length select mask
>> +#define WLS_MASK (0x3)
>> +//parity enable
>> +#define PEN (0x8)
>> +//even parity select
>> +#define EPS (0x10)
>> +
>> + switch (lcr & WLS_MASK) {
>> + case 0: // 5bits
>> + case 1: // 6bits
>> + // Not supported by drivers/tty/serial/serial_core.c:uart_set_options() anyway
>> + WARN(true, "%s: probed uart word length (%u bits) is not supported by uart_set_options()\n", __FUNCTION__, (lcr & WLS_MASK) ? 5 : 6 );
>> + break;
>> + case 2: // 7bits
>> + *bits = 7;
>> + break;
>> + case 3: // 8bits
>> + *bits = 8;
>> + break;
>> + };
>> +
>> + if (lcr & PEN)
>> + {
>> + if (lcr & EPS)
>> + *parity = 'e';
>> + else
>> + *parity = 'o';
>> + }
>> + else
>> + *parity = 'n';
>> +
>> + if (efr & UART_EFR_CTS)
>> + *flow = 'r';
>> + else
>> + *flow = 'n';
>> +
>> + if (quot)
>> + return (port->uartclk / 16) / quot;
>> + else
>> + WARN(true, "%s: quot is zero!\n", __FUNCTION__);
>> +
>> + return -1;
>> }
>>
>> -static int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
>> +static int serial8250_console_setup(struct uart_port *port, char *options)
>> {
>> int baud = 9600;
>> int bits = 8;
>> @@ -3467,8 +3509,8 @@ static int serial8250_console_setup(struct uart_port *port, char *options, bool
>>
>> if (options)
>> uart_parse_options(options, &baud, &parity, &bits, &flow);
>> - else if (probe)
>> - baud = probe_baud(port);
>> + else
>> + baud = probe_port(port, &parity, &bits, &flow);
>>
>> return uart_set_options(port, port->cons, baud, parity, bits, flow);
>> }
>> @@ -3488,7 +3530,7 @@ static int univ8250_console_setup(struct console *co, char *options)
>> /* link port to console */
>> port->cons = co;
>>
>> - return serial8250_console_setup(port, options, false);
>> + return serial8250_console_setup(port, options);
>> }
>>
>> /**
>> @@ -3537,7 +3579,7 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
>>
>> co->index = i;
>> port->cons = co;
>> - return serial8250_console_setup(port, options, true);
>> + return serial8250_console_setup(port, options);
>> }
>>
>> return -ENODEV;
>> --
>> 1.7.10.4
>>
>
On Thu, Dec 17, 2015 at 05:48:42PM +0100, Sebastian Frias wrote:
> On 12/17/2015 05:29 PM, Peter Hurley wrote:
> >On 12/17/2015 07:15 AM, Sebastian Frias wrote:
> >>---
> >>
> >>I think there are a few minor bugs on the 8250 UART code.
> >>
> >>Below you can find a patch with a proposed solution.
> >>
> >>In a nutshell:
> >>- probe_baud from 87515772c33ee8a0cc08d984a7d2401eeff074cd was
> >>converted into probe_port so that it reads all the parameters that
> >>uart_set_options require (namely baud, parity, bits, flow).
> >>- reading/writing to UART_DLL/UART_DLM directly are converted to
> >>using the read_dl/write_dl callbacks.
> >>- the port is always probed if there are no options (*).
> >
> >Because I don't want to probe the port at all.
> >
> >But must when using the
> > earlycon=ttyS0,....
> >
> >command-line (because the original hack expects that behavior).
>
> Ok, we are using:
>
> "console=ttyS0 earlyprintk"
>
> and the 8250 (with CONFIG_SERIAL_8250_RT288X=y) driver.
>
> The hardware is setup prior to Linux boot.
> We don't want Linux to change the UART settings, just to pick up whatever
> settings the UART has and take over UART.
Don't do that :)
Linux can't "know" what happened before it started to the hardware and
expect to work properly.
> How do you suggest we do that? Right now, since it does not probe, it just
> messes up the UART config setup before booting Linux.
pass in the same settings as you previously set up, that way there is no
change.
thanks,
greg k-h
Hi Sebastian,
On 12/17/2015 08:48 AM, Sebastian Frias wrote:
> On 12/17/2015 05:29 PM, Peter Hurley wrote:
>> On 12/17/2015 07:15 AM, Sebastian Frias wrote:
>>> ---
>>>
>>> I think there are a few minor bugs on the 8250 UART code.
>>>
>>> Below you can find a patch with a proposed solution.
>>>
>>> In a nutshell:
>>> - probe_baud from 87515772c33ee8a0cc08d984a7d2401eeff074cd was
>>> converted into probe_port so that it reads all the parameters that
>>> uart_set_options require (namely baud, parity, bits, flow).
>>> - reading/writing to UART_DLL/UART_DLM directly are converted to
>>> using the read_dl/write_dl callbacks.
>>> - the port is always probed if there are no options (*).
>>
>> Because I don't want to probe the port at all.
>>
>> But must when using the
>> earlycon=ttyS0,....
Sorry, ignore this. I meant "console=uart"
>> command-line (because the original hack expects that behavior).
>
> Ok, we are using:
>
> "console=ttyS0 earlyprintk"
>
> and the 8250 (with CONFIG_SERIAL_8250_RT288X=y) driver.
>
> The hardware is setup prior to Linux boot.
> We don't want Linux to change the UART settings, just to pick up whatever settings the UART has and take over UART.
>
> How do you suggest we do that? Right now, since it does not probe, it just messes up the UART config setup before booting Linux.
>
> While on the subject, do you think you could explain the difference (or similarity) between:
> - "console=ttyS0"
> - "console=uart"
> - "earlycon=uart"
> and how they relate to "earlyprintk" (if at all)?
>
> Maybe some of those options are soon to be deprecated and we'd like to stick with the standard and future-proof way.
All of the above choices are future-proof because kernel command
line options are considered userspace.
So
"console=ttyS0" w/o options always initializes the h/w to 9600n81
"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.
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.
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?
Regards,
Peter Hurley
>>> (*): I'm not sure why commit 87515772c33ee8a0cc08d984a7d2401eeff074cd
>>> makes a difference in that regard, especially considering the commit
>>> log states that if there are no options, the hardware is assumed to
>>> be already initialised. Since uart_set_options is always called, the
>>> current hardware setup could be overwritten with different parameters
>>> if the actual hardware is not probed.
>>>
>>> ---
>>> drivers/tty/serial/8250/8250_core.c | 84 ++++++++++++++++++++++++++---------
>>> 1 file changed, 63 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>>> index 2c46a21..624667f 100644
>>> --- a/drivers/tty/serial/8250/8250_core.c
>>> +++ b/drivers/tty/serial/8250/8250_core.c
>>> @@ -791,22 +791,19 @@ static int size_fifo(struct uart_8250_port *up)
>>> */
>>> static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
>>> {
>>> - unsigned char old_dll, old_dlm, old_lcr;
>>> + unsigned char old_lcr;
>>> unsigned int id;
>>> + unsigned int old_dl;
>>>
>>> old_lcr = serial_in(p, UART_LCR);
>>> - serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
>>> -
>>> - old_dll = serial_in(p, UART_DLL);
>>> - old_dlm = serial_in(p, UART_DLM);
>>>
>>> - serial_out(p, UART_DLL, 0);
>>> - serial_out(p, UART_DLM, 0);
>>> + serial_out(p, UART_LCR, UART_LCR_CONF_MODE_A);
>>>
>>> - id = serial_in(p, UART_DLL) | serial_in(p, UART_DLM) << 8;
>>> + old_dl = serial_dl_read(p);
>>> + serial_dl_write(p, 0);
>>> + id = serial_dl_read(p);
>>> + serial_dl_write(p, old_dl);
>>>
>>> - serial_out(p, UART_DLL, old_dll);
>>> - serial_out(p, UART_DLM, old_dlm);
>>> serial_out(p, UART_LCR, old_lcr);
>>>
>>> return id;
>>> @@ -3440,22 +3437,67 @@ static void univ8250_console_write(struct console *co, const char *s,
>>> serial8250_console_write(up, s, count);
>>> }
>>>
>>> -static unsigned int probe_baud(struct uart_port *port)
>>> +static int probe_port(struct uart_port *port, int *parity, int *bits, int *flow)
>>> {
>>> - unsigned char lcr, dll, dlm;
>>> + struct uart_8250_port *up = up_to_u8250p(port);
>>> + unsigned char lcr, efr;
>>> 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);
>>> + quot = serial_dl_read(up);
>>> + serial_port_out(port, UART_LCR, UART_LCR_CONF_MODE_B);
>>> + if (port->flags & UPF_EXAR_EFR)
>>> + efr = serial_port_in(port, UART_XR_EFR);
>>> + else
>>> + efr = serial_port_in(port, UART_EFR);
>>> serial_port_out(port, UART_LCR, lcr);
>>>
>>> - quot = (dlm << 8) | dll;
>>> - return (port->uartclk / 16) / quot;
>>> +//word length select mask
>>> +#define WLS_MASK (0x3)
>>> +//parity enable
>>> +#define PEN (0x8)
>>> +//even parity select
>>> +#define EPS (0x10)
>>> +
>>> + switch (lcr & WLS_MASK) {
>>> + case 0: // 5bits
>>> + case 1: // 6bits
>>> + // Not supported by drivers/tty/serial/serial_core.c:uart_set_options() anyway
>>> + WARN(true, "%s: probed uart word length (%u bits) is not supported by uart_set_options()\n", __FUNCTION__, (lcr & WLS_MASK) ? 5 : 6 );
>>> + break;
>>> + case 2: // 7bits
>>> + *bits = 7;
>>> + break;
>>> + case 3: // 8bits
>>> + *bits = 8;
>>> + break;
>>> + };
>>> +
>>> + if (lcr & PEN)
>>> + {
>>> + if (lcr & EPS)
>>> + *parity = 'e';
>>> + else
>>> + *parity = 'o';
>>> + }
>>> + else
>>> + *parity = 'n';
>>> +
>>> + if (efr & UART_EFR_CTS)
>>> + *flow = 'r';
>>> + else
>>> + *flow = 'n';
>>> +
>>> + if (quot)
>>> + return (port->uartclk / 16) / quot;
>>> + else
>>> + WARN(true, "%s: quot is zero!\n", __FUNCTION__);
>>> +
>>> + return -1;
>>> }
>>>
>>> -static int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
>>> +static int serial8250_console_setup(struct uart_port *port, char *options)
>>> {
>>> int baud = 9600;
>>> int bits = 8;
>>> @@ -3467,8 +3509,8 @@ static int serial8250_console_setup(struct uart_port *port, char *options, bool
>>>
>>> if (options)
>>> uart_parse_options(options, &baud, &parity, &bits, &flow);
>>> - else if (probe)
>>> - baud = probe_baud(port);
>>> + else
>>> + baud = probe_port(port, &parity, &bits, &flow);
>>>
>>> return uart_set_options(port, port->cons, baud, parity, bits, flow);
>>> }
>>> @@ -3488,7 +3530,7 @@ static int univ8250_console_setup(struct console *co, char *options)
>>> /* link port to console */
>>> port->cons = co;
>>>
>>> - return serial8250_console_setup(port, options, false);
>>> + return serial8250_console_setup(port, options);
>>> }
>>>
>>> /**
>>> @@ -3537,7 +3579,7 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
>>>
>>> co->index = i;
>>> port->cons = co;
>>> - return serial8250_console_setup(port, options, true);
>>> + return serial8250_console_setup(port, options);
>>> }
>>>
>>> return -ENODEV;
>>> --
>>> 1.7.10.4
>>>
>>
Hi Peter,
On 12/17/2015 06:48 PM, Peter Hurley wrote:
> Hi Sebastian,
>
> On 12/17/2015 08:48 AM, Sebastian Frias wrote:
>> On 12/17/2015 05:29 PM, Peter Hurley wrote:
>>> On 12/17/2015 07:15 AM, Sebastian Frias wrote:
>>>> ---
>>>>
>>>> I think there are a few minor bugs on the 8250 UART code.
>>>>
>>>> Below you can find a patch with a proposed solution.
>>>>
>>>> In a nutshell:
>>>> - probe_baud from 87515772c33ee8a0cc08d984a7d2401eeff074cd was
>>>> converted into probe_port so that it reads all the parameters that
>>>> uart_set_options require (namely baud, parity, bits, flow).
>>>> - reading/writing to UART_DLL/UART_DLM directly are converted to
>>>> using the read_dl/write_dl callbacks.
>>>> - the port is always probed if there are no options (*).
>>>
>>> Because I don't want to probe the port at all.
>>>
>>> But must when using the
>>> earlycon=ttyS0,....
>
> Sorry, ignore this. I meant "console=uart"
>
>>> command-line (because the original hack expects that behavior).
>>
>> Ok, we are using:
>>
>> "console=ttyS0 earlyprintk"
>>
>> and the 8250 (with CONFIG_SERIAL_8250_RT288X=y) driver.
>>
>> The hardware is setup prior to Linux boot.
>> We don't want Linux to change the UART settings, just to pick up whatever settings the UART has and take over UART.
>>
>> How do you suggest we do that? Right now, since it does not probe, it just messes up the UART config setup before booting Linux.
>>
>> While on the subject, do you think you could explain the difference (or similarity) between:
>> - "console=ttyS0"
>> - "console=uart"
>> - "earlycon=uart"
>> and how they relate to "earlyprintk" (if at all)?
>>
>> Maybe some of those options are soon to be deprecated and we'd like to stick with the standard and future-proof way.
>
> All of the above choices are future-proof because kernel command
> line options are considered userspace.
>
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.
> 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.
>
> 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?
>
> 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.
In our case, just forcing the probe was enough because only the baud
rate could change, but if other parameters were to differ, set_termios
would overwrite them with the "n8r" defaults.
Thanks, regards,
Sebastian
On 12/17/2015 06:21 PM, Greg Kroah-Hartman wrote:
> On Thu, Dec 17, 2015 at 05:48:42PM +0100, Sebastian Frias wrote:
>> On 12/17/2015 05:29 PM, Peter Hurley wrote:
>>> On 12/17/2015 07:15 AM, Sebastian Frias wrote:
>>>> ---
>>>>
>>>> I think there are a few minor bugs on the 8250 UART code.
>>>>
>>>> Below you can find a patch with a proposed solution.
>>>>
>>>> In a nutshell:
>>>> - probe_baud from 87515772c33ee8a0cc08d984a7d2401eeff074cd was
>>>> converted into probe_port so that it reads all the parameters that
>>>> uart_set_options require (namely baud, parity, bits, flow).
>>>> - reading/writing to UART_DLL/UART_DLM directly are converted to
>>>> using the read_dl/write_dl callbacks.
>>>> - the port is always probed if there are no options (*).
>>>
>>> Because I don't want to probe the port at all.
>>>
>>> But must when using the
>>> earlycon=ttyS0,....
>>>
>>> command-line (because the original hack expects that behavior).
>>
>> Ok, we are using:
>>
>> "console=ttyS0 earlyprintk"
>>
>> and the 8250 (with CONFIG_SERIAL_8250_RT288X=y) driver.
>>
>> The hardware is setup prior to Linux boot.
>> We don't want Linux to change the UART settings, just to pick up whatever
>> settings the UART has and take over UART.
>
> Don't do that :)
> Linux can't "know" what happened before it started to the hardware and
> expect to work properly.
But it seems it was designed to work in that case :)
Commit 87515772c33ee8a0cc08d984a7d2401eeff074cd says that is not
documented, but was made so that it works.
I can understand that normally Linux should take over all devices it is
supposed to handle, initialise them, etc. and then it expects to retain
full ownership of those devices.
But UART end-points need to agree on the communication parameters
beforehand (and do not re-negotiate them mutually on-the-fly), that's
why it seems important for Linux to avoid changing the parameters of an
already configured UART.
If Linux does not "know" what happened before to some device, then maybe
it's better if it does not touch it (or to be able to tell it that it
should not touch the device)
The proposed solution of probing and setting up the same way also works.
>
>> How do you suggest we do that? Right now, since it does not probe, it just
>> messes up the UART config setup before booting Linux.
>
> pass in the same settings as you previously set up, that way there is no
> change.
We may not know them.
Indeed, when running under an SoC emulator, clocks sometimes run at
arbitrary speeds, so if we hard-code the parameters, then that Linux
Image+DT combo are bound to that specific clock.
Other times the clocks have strange ratios or the clock generators may
not even be there, we may not even have a bootloader.
However, and at least in our case, when the SoC design is started in the
emulator, the UART is setup automatically, which allows any device
booting up to simply write to the UART and have its logs output.
In the past we used to have a forked Linux where some #ifdef would just
bypass the UART init.
In Linux 4.x we found that bypassing the call to set_termios would do
the trick.
But if we could avoid having any #ifdef at all and just use regular
Linux features, it would be better.
Does that makes sense?
Thanks, regards,
Sebastian
>
> thanks,
>
> greg k-h
>
On 12/17/2015 10:21 AM, Sebastian Frias wrote:
> On 12/17/2015 06:48 PM, Peter Hurley wrote:
>> On 12/17/2015 08:48 AM, Sebastian Frias wrote:
>>> On 12/17/2015 05:29 PM, Peter Hurley wrote:
>>>> On 12/17/2015 07:15 AM, Sebastian Frias wrote:
>>>>> ---
>>>>>
>>>>> I think there are a few minor bugs on the 8250 UART code.
>>>>>
>>>>> Below you can find a patch with a proposed solution.
>>>>>
>>>>> In a nutshell:
>>>>> - probe_baud from 87515772c33ee8a0cc08d984a7d2401eeff074cd was
>>>>> converted into probe_port so that it reads all the parameters that
>>>>> uart_set_options require (namely baud, parity, bits, flow).
>>>>> - reading/writing to UART_DLL/UART_DLM directly are converted to
>>>>> using the read_dl/write_dl callbacks.
>>>>> - the port is always probed if there are no options (*).
>>>>
>>>> Because I don't want to probe the port at all.
>>>>
>>>> But must when using the
>>>> earlycon=ttyS0,....
>>
>> Sorry, ignore this. I meant "console=uart"
>>
>>>> command-line (because the original hack expects that behavior).
>>>
>>> Ok, we are using:
>>>
>>> "console=ttyS0 earlyprintk"
>>>
>>> and the 8250 (with CONFIG_SERIAL_8250_RT288X=y) driver.
>>>
>>> The hardware is setup prior to Linux boot.
>>> We don't want Linux to change the UART settings, just to pick up whatever settings the UART has and take over UART.
>>>
>>> How do you suggest we do that? Right now, since it does not probe, it just messes up the UART config setup before booting Linux.
>>>
>>> While on the subject, do you think you could explain the difference (or similarity) between:
>>> - "console=ttyS0"
>>> - "console=uart"
>>> - "earlycon=uart"
>>> and how they relate to "earlyprintk" (if at all)?
>>>
>>> Maybe some of those options are soon to be deprecated and we'd like to stick with the standard and future-proof way.
>>
>> All of the above choices are future-proof because kernel command
>> line options are considered userspace.
>>
>
> 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).
>> 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.
>
>>
>> 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.
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.
> In our case, just forcing the probe was enough because only the baud
> rate could change, but if other parameters were to differ,
> set_termios would overwrite them with the "n8r" defaults.
Ok.
Regards,
Peter Hurley
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
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
Hi Peter,
On 12/18/2015 04:03 PM, Peter Hurley wrote:
>>
>> 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?
It wouldn't, that was my point.
You would have decided a limit of support.
I'm sure that from time to time, some support is dropped, leading to
some HW having its support "removed" from mainline and maybe supported
on some backports branch.
Right?
>
>
>> 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 understand, I would like to help, but right now I'm not sure I
understood all of these options nor their history.
Do you have any suggestions?
>
>
>> 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.
:)
That's true if all test cases are exercised by people that would take
the time to report back to you, and if they are always testing the
latest code (or at least where commits go).
In general, when a company ships a product, they will attempt to keep
all changes to the minimum.
The are several reasons for that, product certifications (they'd need to
be redone under some circumstances) and middleware developed by third
companies or contractors provided as binary (which would ask for money
to rebuild/port), seem the most common, and that forces companies to
support really old kernels, which may not receive any more patches from
the community and that they need to keep fixing it internally. That
brings us back to the question whether or not you would get feedback,
and in those circumstances I think you probably wouldn't.
All that to say that I would not be so sure about the "community"
reporting issues, for sure they do, but with limitations.
I understand that the "open source" philosophy is that there's many
people looking at the code and will find issues, report them or fix them.
But in my experience, if something is not tested continuously, it will
inexorably be broken at some point.
Another argument in favor of testcases is that they also serve as
documentation.
Don't get me wrong, I'm just trying to show how, under some
circumstances, the "many eyes" hypothesis is not verified, and that
testing does not comes for free.
Although I'm sure I'm just repeating something that has already been said.
>> 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.
Do you mean they are mandatory?
How do they relate to the keys present on the DT? Because the device is
already described in the DT:
uart: serial@10700 {
compatible = "ralink,rt2880-uart";
reg = <0x10700 0x30>;
interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
clock-frequency = <7372800>;
reg-shift = <2>;
};
Are we supposed to duplicate such information (ie: addr) in the
commandline as well?
>> 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.
Is the crash expected then?
If the 'options' are really mandatory, then the crash could be avoided by:
if (!options)
return -ENODEV;
however, as I stated earlier, that would prevent the console from
working even if the port is properly described in the DT :(
Let's say there's a non-standard (ie: incompatible with 8250) UART
driver which is described in DT and whose driver would allow for it to
be used as console, what would the kernel command line for it be?
"console=uart" or "console=ttyS0"?
Alternatively, what's the most generic way (kernel command line) to
instruct the console to work on some device that is described by the DT?
>
>> ...
>> 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.
>
See my previous comment.
>
>>
>> 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?
If that were the case, then the statement "if unspecified, the h/w is
not re-initialized" would be misleading, right?
>> 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
>
For example, I logged the boot of our 4.1.13+ kernel with command line
"console=uart mem=256M earlyprintk debug ignore_loglevel" and I saw that
arch/arm/kernel/setup.c:setup_arch() would call parse_early_param()
resulting in:
A) drivers/tty/serial/8250/8250_early.c:early_serial8250_setup()
rejecting the initialisation (most likely due to incomplete options)
until
B) drivers/tty/serial/8250/8250_early.c:setup_early_printk() registering
a console hooked up to printch() (written in assembly and available very
early), essentially using the arch-dependent UART facility.
Would A) be what you call "if it finds an earlycon match"?
>
>> 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.
I understand, thanks.
So, before earlycon there was a "gap" between earlyprintk and console?
Does the notion of "early" between earlycon and console just affects
when the logs starts being sent to the UART? Or is earlycon somehow
limited, justifying the need for another more complex driver (console)
to be brought up after a while?
>
>
>> 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.
>
What is confusing is that now we have DT which is supposed to deal with
describing the HW, and while I understand it is somewhat new and thus
before it we needed to pass options in the command line, now that we
have DT, there should be a way to tell it to use the HW description from DT.
Am I missing something obvious?
>> 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.
>
Thanks for your understanding.
>> 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.
>
Ok, I will do so later on.
Thanks, regards,
Sebastian
On 12/21/2015 05:50 PM, Sebastian Frias wrote:
>> 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.
>
> Do you mean they are mandatory?
> How do they relate to the keys present on the DT? Because the device is
> already described in the DT:
>
> uart: serial@10700 {
> compatible = "ralink,rt2880-uart";
> reg = <0x10700 0x30>;
> interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
> clock-frequency = <7372800>;
> reg-shift = <2>;
> };
>
> Are we supposed to duplicate such information (ie: addr) in the
> commandline as well?
>
By the way, I tried using the following command line
'console=uart,mmio32,0x10700,115200n8r mem=256M earlyprintk debug
ignore_loglevel' but I think I'm still missing something regarding this,
because it does not work, here's a log of Linux 4.1.13+:
1: [arch/arm/kernel/setup.c:932] setup_arch(): r1 = 0xf34, r2 =
0x803b3b50, cmdline ''
1: [arch/arm/kernel/devtree.c:195] arch_get_next_mach(): infoBegin
0xc024faac, mdesc 0xc024faac
1: [arch/arm/kernel/devtree.c:195] arch_get_next_mach(): infoBegin
0xc024faac, mdesc 0xc024fb14
1: [drivers/of/fdt.c:106] of_fdt_is_compatible(): sigma,tango4 vs
sigma,vantage-1172
1: [arch/arm/kernel/devtree.c:195] arch_get_next_mach(): infoBegin
0xc024faac, mdesc 0xc024fb7c
1: [drivers/of/fdt.c:749] of_flat_dt_match_machine(): Machine model:
Sigma Designs SMP8758 Vantage-1172 Rev E1
1: [arch/arm/kernel/devtree.c:256] setup_machine_fdt(): about to call
early_init_dt_scan_nodes
1: [drivers/of/fdt.c:1057] early_init_dt_scan_nodes(): bootcmdline ''
1: [drivers/of/fdt.c:1062] early_init_dt_scan_nodes(): bootcmdline after
of_scan_flat_dt 'console=uart,mmio32,0x10700,115200n8r mem=256M
earlyprintk debug ignore_loglevel'
1: [arch/arm/kernel/setup.c:946] setup_arch(): using DT at r2
1: [arch/arm/kernel/setup.c:949] setup_arch(): machine name Sigma Tango DT
1: [arch/arm/kernel/setup.c:967] setup_arch(): before parse_early_param
1: [init/main.c:468] parse_early_param(): bootcmdline
'console=uart,mmio32,0x10700,115200n8r mem=256M earlyprintk debug
ignore_loglevel'
1: [init/main.c:473] parse_early_param(): mark
1: [init/main.c:428] do_early_param(): enter with: 'console'
'uart,mmio32,0x10700,115200n8r'
1: [init/main.c:446] do_early_param(): 'earlycon', setup_func 0xc024b868
1: [drivers/tty/serial/earlycon.c:214] param_setup_earlycon():
'uart,mmio32,0x10700,115200n8r'
1: [drivers/tty/serial/earlycon.c:181] setup_earlycon():
'uart,mmio32,0x10700,115200n8r'
1: [drivers/tty/serial/earlycon.c:132] register_earlycon():
'mmio32,0x10700,115200n8r' 'uart'
1: [drivers/tty/serial/earlycon.c:86] parse_options():
'mmio32,0x10700,115200n8r'
1: [drivers/tty/serial/earlycon.c:91] parse_options(): addr 0x00010700,
options '115200n8r'
1: [drivers/tty/serial/earlycon.c:117] parse_options(): Early serial
console at MMIO32 0x10700 (options '115200n8r')
1: [drivers/tty/serial/earlycon.c:138] register_earlycon(): '(null)'
1: [drivers/tty/serial/earlycon.c:62] earlycon_map(): paddr 0x00010700
size 64
As you can see, now that the options are more complete, the match for
"earlycon" succeeds, but it does not work, the last log is in
earlycon_map() function.
When using just "console=uart", the "earlycon" match will fail but a
match for "earlyprink" will succeed. That one will hook printch() from
the arch-dependent code to a somewhat simpler 'earlycon'
(arch/arm/kernel/early_printk.c).
I think the code is the same on mainline.
Also, regarding my previous question about using a HW described in DT
for earlycon, I noticed there's a of_setup_earlycon() in
drivers/tty/serial/earlycon.c but that is hooked to
drivers/of/fdt.c:setup_of_earlycon() which is not called. I do have
CONFIG_SERIAL_EARLYCON=y.
I also have a similar issue (ie: get blocked right after the call to
earlycon_map function, log is obviously different from above) if attempt
to use of_setup_earlycon.
My DT has roughly:
aliases {
serial0 = &uart;
};
chosen {
bootargs = "earlycon console mem=256M earlyprintk debug
ignore_loglevel";
stdout-path = "serial0:115200n8";
};
uart: serial@10700 {
compatible = "ralink,rt2880-uart";
reg = <0x10700 0x30>;
interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
clock-frequency = <7372800>;
reg-shift = <2>;
};
and then I hack drivers/tty/serial/8250/8250_early.c by adding:
OF_EARLYCON_DECLARE(rt2880, "ralink,rt2880-uart", early_serial8250_setup);
at the end of the file, trying to mimic commit
d05f15707bb7659d2b863fafa1a918f286d74a63
I'm still trying to figure out the right bootargs, so that's why both
"earlycon" and "console" are there. Suggestions welcome.
Thanks in advance and Merry Christmas!