Re: [RFC PATCH] always probe UART HW when options are not specified

Re: [RFC PATCH] always probe UART HW when options are not specified

[Måns Rullgård]

Sebastian Frias <sf84@laposte.net> writes:

> - reading/writing to UART_DLL/UART_DLM directly are converted to using
> the read_dl/write_dl callbacks.

This should be an obvious fix.  Please send that as a separate patch.

-- 
Måns Rullgård

Re: [RFC PATCH] always probe UART HW when options are not specified

[Peter Hurley]

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
> 

Re: [RFC PATCH] always probe UART HW when options are not specified

[Sebastian Frias]

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

Re: [RFC PATCH] always probe UART HW when options are not specified

[Greg Kroah-Hartman]

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

Re: [RFC PATCH] always probe UART HW when options are not specified

[Peter Hurley]

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

Re: [RFC PATCH] always probe UART HW when options are not specified

[Sebastian Frias]

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

Re: [RFC PATCH] always probe UART HW when options are not specified

[Sebastian Frias]

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
>

Re: [RFC PATCH] always probe UART HW when options are not specified

[Peter Hurley]

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

Re: [RFC PATCH] always probe UART HW when options are not specified

[Sebastian Frias]

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

Re: [RFC PATCH] always probe UART HW when options are not specified

[Peter Hurley]

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

Re: [RFC PATCH] always probe UART HW when options are not specified

[Sebastian Frias]

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

Re: [RFC PATCH] always probe UART HW when options are not specified

[Sebastian Frias]

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!

Re: [RFC PATCH] always probe UART HW when options are not specified

[Sebastian Frias]

On 12/22/2015 06:56 PM, Sebastian Frias wrote:
> 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.
>

Does anybody has comments or suggestions regarding this thread and the 
issue above?

Re: [RFC PATCH] always probe UART HW when options are not specified

[Peter Hurley]

On 01/11/2016 07:07 AM, Sebastian Frias wrote:
> On 12/22/2015 06:56 PM, Sebastian Frias wrote:
>> 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);

There is no support for this uart in 8250 earlycon; the registers
need remapped.

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

Just 'earlycon' triggers the attempted registration of earlycon matching the
compatible string of the stdout-path node.

The empty 'console' in bootargs is doing nothing.


> Does anybody has comments or suggestions regarding this thread and the issue above?

Regards,
Peter Hurley

Re: [RFC PATCH] always probe UART HW when options are not specified

[Sebastian Frias]

Hi Peter,

On 01/11/2016 05:11 PM, Peter Hurley wrote:
> On 01/11/2016 07:07 AM, Sebastian Frias wrote:
>> On 12/22/2015 06:56 PM, Sebastian Frias wrote:
>>>
>>>       OF_EARLYCON_DECLARE(rt2880, "ralink,rt2880-uart",
>>> early_serial8250_setup);
>
> There is no support for this uart in 8250 earlycon; the registers
> need remapped.

Ok, two questions then:
1) If the UART is not supported on 8250 earlycon, what is the 
suggested/advised solution? Using just "earlyprintk"?
2) What would it take to make the "rt2880" work with the 8250 earlycon? 
I mean, it is already pretty much supported in there, what would be 
missing? (I don't see why it blocks on earlycon_map) And would it be 
worth doing?

>
>>> 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.
>
> Just 'earlycon' triggers the attempted registration of earlycon matching the
> compatible string of the stdout-path node.
>
> The empty 'console' in bootargs is doing nothing.

Ok, thanks.

So, just to recap.
We would like to understand what is the right way of doing this:

- we are using 8250 (rt288x variant) UART: CONFIG_SERIAL_8250_RT288X=y
- the UART 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 the UART.

There were two replies to that, one by Greg Kroah-Hartman 
(http://www.spinics.net/lists/linux-serial/msg20278.html) and one by 
you, where you suggested we use "console=uart", but as I reported 
(http://www.spinics.net/lists/linux-serial/msg20307.html) it does not 
work, you replied that iotype and mmio are not optional but mandatory 
(http://www.spinics.net/lists/linux-serial/msg20310.html), and I 
wondered if it was really necessary to duplicate data that is already on 
the DT among other questions 
(http://www.spinics.net/lists/linux-serial/msg20383.html), like how are 
DT described drivers supposed to interact with the 
"console="/"earlycon=" commandlines, or, the contradiction between 
"console=ttyS0" means '9600n81' and "if unspecified [the uart options], 
the h/w is not re-initialized"

So, for us, it is still not clear what is the recommended way of 
achieving our goal above, and it seems it is not clear what does 
"console=ttyS0" is supposed to do, hardcode ('9600n81') or probe ('the 
h/w is not re-initialized')
Any help will be appreciated.

Thanks, best regards,

Sebastian

Re: [RFC PATCH] always probe UART HW when options are not specified

[Peter Hurley]

On 01/11/2016 09:56 AM, Sebastian Frias wrote:
> Hi Peter,
> 
> On 01/11/2016 05:11 PM, Peter Hurley wrote:
>> On 01/11/2016 07:07 AM, Sebastian Frias wrote:
>>> On 12/22/2015 06:56 PM, Sebastian Frias wrote:
>>>>
>>>>       OF_EARLYCON_DECLARE(rt2880, "ralink,rt2880-uart",
>>>> early_serial8250_setup);
>>
>> There is no support for this uart in 8250 earlycon; the registers
>> need remapped.
> 
> Ok, two questions then:
> 1) If the UART is not supported on 8250 earlycon, what is the
> suggested/advised solution? Using just "earlyprintk"?

I don't have enough information to suggest what you "should" use
here.

Is this going to be a shipping product?
Is it single-core?
etc.

And what is your purpose for outputting early boot information
before loading the serial driver which does provide console output?


> 2) What would it take to make the "rt2880" work with the 8250
> earlycon? I mean, it is already pretty much supported in there, what
> would be missing? (I don't see why it blocks on earlycon_map) And
> would it be worth doing?

The rt2880 does not have the same register locations as a 8250.
The 8250 port driver remaps all register accesses with a LUT.

Adding support would be trivial.

>>>> 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.
>>
>> Just 'earlycon' triggers the attempted registration of earlycon matching the
>> compatible string of the stdout-path node.
>>
>> The empty 'console' in bootargs is doing nothing.
> 
> Ok, thanks.
> 
> So, just to recap.
> We would like to understand what is the right way of doing this:
> 
> - we are using 8250 (rt288x variant) UART: CONFIG_SERIAL_8250_RT288X=y
> - the UART 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 the UART. 
> There were two replies to that, one by Greg Kroah-Hartman
> (http://www.spinics.net/lists/linux-serial/msg20278.html) and one by
> you, where you suggested we use "console=uart", but as I reported
> (http://www.spinics.net/lists/linux-serial/msg20307.html) it does not
> work, you replied that iotype and mmio are not optional but mandatory
> (http://www.spinics.net/lists/linux-serial/msg20310.html), and I
> wondered if it was really necessary to duplicate data that is already
> on the DT among other questions

At the time, I didn't know you were describing your h/w with DT.

If you use the console command line (console= or earlycon=) to start
an earlycon, then the uart address and iotype are mandatory.
For this usage, earlycon matching is attempted with every EARLYCON_DECLARE().

If you use plain "earlycon" on the command line, that will attempt to
register the uart described by stdout-path in DT. For this usage,
earlycon matching is attempted with the compatible string of every
OF_EARLYCON_DECLARE().

> (http://www.spinics.net/lists/linux-serial/msg20383.html), like how
> are DT described drivers supposed to interact with the
> "console="/"earlycon=" commandlines

They don't; those are orthogonal.

>, or, the contradiction between
> "console=ttyS0" means '9600n81' and "if unspecified [the uart
> options], the h/w is not re-initialized"> 

I thought I was clear on that: "console=ttyS0" initializes the h/w
to 9600n81 *because there are already existing users that must not break*.

"console=uart,..." probes the h/w
*because there are already existing users that must not break *

> So, for us, it is still not clear what is the recommended way of
> achieving our goal above, and it seems it is not clear what does
> "console=ttyS0" is supposed to do, hardcode ('9600n81') or probe
> ('the h/w is not re-initialized')

The DT way will be simplest at this point because you won't
have to write console handover matching for "console=rt288x,..."

With DT (ie, stdout-path) earlycon, when a serial driver loads,
an attempt is made to cross-reference any existing console with
the node that is loading and will do a console takeover from
a running earlycon for a matching uart node.

There is a bug with DT earlycon though.
If you have a dummy console that loads, the DT earlycon is
disabled at that point because boot consoles are disabled when
"real" consoles load.

> Any help will be appreciated.

Regards,
Peter Hurley

Re: [RFC PATCH] always probe UART HW when options are not specified

[Peter Hurley]

On 01/11/2016 11:06 AM, Peter Hurley wrote:
> On 01/11/2016 09:56 AM, Sebastian Frias wrote:
>> On 01/11/2016 05:11 PM, Peter Hurley wrote:
>>> On 01/11/2016 07:07 AM, Sebastian Frias wrote:
>>>> On 12/22/2015 06:56 PM, Sebastian Frias wrote:

[...]

>> 2) What would it take to make the "rt2880" work with the 8250
>> earlycon? I mean, it is already pretty much supported in there, what
>> would be missing? (I don't see why it blocks on earlycon_map) And
>> would it be worth doing?
> 
> The rt2880 does not have the same register locations as a 8250.
> The 8250 port driver remaps all register accesses with a LUT.
> 
> Adding support would be trivial.

Please test.

--- >% ---
Subject: [PATCH] serial: 8250: Add Au1x00/RT288x earlycon support

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/8250/8250_early.c | 52 ++++++++++++++++++++++++++++++++++++
 include/uapi/linux/serial_reg.h      | 13 +++++++++
 2 files changed, 65 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
index af62131..307fb23 100644
--- a/drivers/tty/serial/8250/8250_early.c
+++ b/drivers/tty/serial/8250/8250_early.c
@@ -145,3 +145,55 @@ EARLYCON_DECLARE(uart8250, early_serial8250_setup);
 EARLYCON_DECLARE(uart, early_serial8250_setup);
 OF_EARLYCON_DECLARE(ns16550, "ns16550", early_serial8250_setup);
 OF_EARLYCON_DECLARE(ns16550a, "ns16550a", early_serial8250_setup);
+
+
+#ifdef CONFIG_SERIAL_8250_RT288X
+
+static void __init rt288x_putc(struct uart_port *port, int c)
+{
+	unsigned int status;
+
+	serial8250_early_out(port, AU1x00_TX, c);
+
+	for (;;) {
+		status = serial8250_early_in(port, AU1x00_LSR);
+		if ((status & BOTH_EMPTY) == BOTH_EMPTY)
+			break;
+		cpu_relax();
+	}
+}
+
+static void __init early_rt288x_write(struct console *console,
+				      const char *s, unsigned int count)
+{
+	struct earlycon_device *device = console->data;
+	struct uart_port *port = &device->port;
+
+	uart_console_write(port, s, count, rt288x_putc);
+}
+
+static int __init early_rt288x_setup(struct earlycon_device *device,
+				     const char *options)
+{
+	struct uart_port *port = &device->port;
+	unsigned int ier;
+
+	if (!(device->port.membase || device->port.iobase))
+		return -ENODEV;
+
+	/* Don't support direct init or any line-setting options */
+	if (device->baud)
+		return -EINVAL;
+
+	/* assume the device was initialized, only mask interrupts */
+	ier = serial8250_early_in(port, AU1x00_IER);
+	serial8250_early_out(port, AU1x00_IER, ier);
+
+	device->con->write = early_rt288x_write;
+	return 0;
+}
+
+EARLYCON_DECLARE(rt288x, early_rt288x_setup);
+OF_EARLYCON_DECLARE(rt288x, "ralink,rt2880-uart", early_rt288x_setup);
+
+#endif /* CONFIG_SERIAL_8250_RT288X */
diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
index 1e5ac4e7..4d068ac 100644
--- a/include/uapi/linux/serial_reg.h
+++ b/include/uapi/linux/serial_reg.h
@@ -376,5 +376,18 @@
 #define UART_EXAR_TXTRG		0x0a	/* Tx FIFO trigger level write-only */
 #define UART_EXAR_RXTRG		0x0b	/* Rx FIFO trigger level write-only */
 
+/*
+ * Register offsets for Au1x00/RT288x 8250-workalikes
+ */
+#define AU1x00_RX	0
+#define AU1x00_TX	1
+#define AU1x00_IER	2
+#define AU1x00_IIR	3
+#define AU1x00_FCR	4
+#define AU1x00_LCR	5
+#define AU1x00_MCR	6
+#define AU1x00_LSR	7
+#define AU1x00_MSR	8
+
 #endif /* _LINUX_SERIAL_REG_H */
 
-- 
2.7.0

Re: [RFC PATCH] always probe UART HW when options are not specified

[Peter Hurley]

On 01/11/2016 11:57 AM, Peter Hurley wrote:
> On 01/11/2016 11:06 AM, Peter Hurley wrote:
>> On 01/11/2016 09:56 AM, Sebastian Frias wrote:
>>> On 01/11/2016 05:11 PM, Peter Hurley wrote:
>>>> On 01/11/2016 07:07 AM, Sebastian Frias wrote:
>>>>> On 12/22/2015 06:56 PM, Sebastian Frias wrote:
> 
> [...]
> 
>>> 2) What would it take to make the "rt2880" work with the 8250
>>> earlycon? I mean, it is already pretty much supported in there, what
>>> would be missing? (I don't see why it blocks on earlycon_map) And
>>> would it be worth doing?
>>
>> The rt2880 does not have the same register locations as a 8250.
>> The 8250 port driver remaps all register accesses with a LUT.
>>
>> Adding support would be trivial.
> 
> Please test.

To get this working with DT, you may need to apply part or all of
my "Earlycon cleanup" series from Apr last year that adds the 
necessary support for things like "reg-shift" DT properties.

I'll see what I can do about cleaning up and re-submitting that series.

Regards,
Peter Hurley


> --- >% ---
> Subject: [PATCH] serial: 8250: Add Au1x00/RT288x earlycon support
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/serial/8250/8250_early.c | 52 ++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/serial_reg.h      | 13 +++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
> index af62131..307fb23 100644
> --- a/drivers/tty/serial/8250/8250_early.c
> +++ b/drivers/tty/serial/8250/8250_early.c
> @@ -145,3 +145,55 @@ EARLYCON_DECLARE(uart8250, early_serial8250_setup);
>  EARLYCON_DECLARE(uart, early_serial8250_setup);
>  OF_EARLYCON_DECLARE(ns16550, "ns16550", early_serial8250_setup);
>  OF_EARLYCON_DECLARE(ns16550a, "ns16550a", early_serial8250_setup);
> +
> +
> +#ifdef CONFIG_SERIAL_8250_RT288X
> +
> +static void __init rt288x_putc(struct uart_port *port, int c)
> +{
> +	unsigned int status;
> +
> +	serial8250_early_out(port, AU1x00_TX, c);
> +
> +	for (;;) {
> +		status = serial8250_early_in(port, AU1x00_LSR);
> +		if ((status & BOTH_EMPTY) == BOTH_EMPTY)
> +			break;
> +		cpu_relax();
> +	}
> +}
> +
> +static void __init early_rt288x_write(struct console *console,
> +				      const char *s, unsigned int count)
> +{
> +	struct earlycon_device *device = console->data;
> +	struct uart_port *port = &device->port;
> +
> +	uart_console_write(port, s, count, rt288x_putc);
> +}
> +
> +static int __init early_rt288x_setup(struct earlycon_device *device,
> +				     const char *options)
> +{
> +	struct uart_port *port = &device->port;
> +	unsigned int ier;
> +
> +	if (!(device->port.membase || device->port.iobase))
> +		return -ENODEV;
> +
> +	/* Don't support direct init or any line-setting options */
> +	if (device->baud)
> +		return -EINVAL;
> +
> +	/* assume the device was initialized, only mask interrupts */
> +	ier = serial8250_early_in(port, AU1x00_IER);
> +	serial8250_early_out(port, AU1x00_IER, ier);
> +
> +	device->con->write = early_rt288x_write;
> +	return 0;
> +}
> +
> +EARLYCON_DECLARE(rt288x, early_rt288x_setup);
> +OF_EARLYCON_DECLARE(rt288x, "ralink,rt2880-uart", early_rt288x_setup);
> +
> +#endif /* CONFIG_SERIAL_8250_RT288X */
> diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
> index 1e5ac4e7..4d068ac 100644
> --- a/include/uapi/linux/serial_reg.h
> +++ b/include/uapi/linux/serial_reg.h
> @@ -376,5 +376,18 @@
>  #define UART_EXAR_TXTRG		0x0a	/* Tx FIFO trigger level write-only */
>  #define UART_EXAR_RXTRG		0x0b	/* Rx FIFO trigger level write-only */
>  
> +/*
> + * Register offsets for Au1x00/RT288x 8250-workalikes
> + */
> +#define AU1x00_RX	0
> +#define AU1x00_TX	1
> +#define AU1x00_IER	2
> +#define AU1x00_IIR	3
> +#define AU1x00_FCR	4
> +#define AU1x00_LCR	5
> +#define AU1x00_MCR	6
> +#define AU1x00_LSR	7
> +#define AU1x00_MSR	8
> +
>  #endif /* _LINUX_SERIAL_REG_H */
>  
> 

Re: [RFC PATCH] always probe UART HW when options are not specified

[Mason]

On 11/01/2016 20:06, Peter Hurley wrote:

> I don't have enough information to suggest what you "should" use
> here.
> 
> Is this going to be a shipping product?
> Is it single-core?
> etc.
> 
> And what is your purpose for outputting early boot information
> before loading the serial driver which does provide console output?

I'm not sure exactly which board Sebastian has in mind, but I've been
using the SERIAL_8250_RT288X driver on my Tango4 board.

Note: calling the UART driver "Au1x00/RT288x" is a bit of a misnomer,
as these are names of SoCs using that distinct register layout.
The actual IP is probably the 16550-compatible Palmchip BK-3103.
https://sites.google.com/a/palmchiptech.com/palmchiptech/product-services/hardware-services/ip-cores/bk-3103
(Not sure that this website is legitimate, though.)

When I need earlyprintk support, I use this patch from Mans:
http://thread.gmane.org/gmane.linux.kernel/2081016

Regards.

Re: [RFC PATCH] always probe UART HW when options are not specified

[Sebastian Frias]

Hi Peter,

On 01/11/2016 08:06 PM, Peter Hurley wrote:
> On 01/11/2016 09:56 AM, Sebastian Frias wrote:
>> Hi Peter,
>>
>> On 01/11/2016 05:11 PM, Peter Hurley wrote:
>>> On 01/11/2016 07:07 AM, Sebastian Frias wrote:
>>>> On 12/22/2015 06:56 PM, Sebastian Frias wrote:
>>>>>
>>>>>        OF_EARLYCON_DECLARE(rt2880, "ralink,rt2880-uart",
>>>>> early_serial8250_setup);
>>>
>>> There is no support for this uart in 8250 earlycon; the registers
>>> need remapped.
>>
>> Ok, two questions then:
>> 1) If the UART is not supported on 8250 earlycon, what is the
>> suggested/advised solution? Using just "earlyprintk"?
>
> I don't have enough information to suggest what you "should" use
> here.
>
> Is this going to be a shipping product?
> Is it single-core?
> etc.
>
> And what is your purpose for outputting early boot information
> before loading the serial driver which does provide console output?
>

No, it is not for production, just for debug, but we would like to 
understand if there is a standard way of doing so, so that whenever 
somebody ask us for "early print", we can provide with the good way.
I know we can always provide with hacks, I'm just wondering if there's a 
"standard way".

>
>> 2) What would it take to make the "rt2880" work with the 8250
>> earlycon? I mean, it is already pretty much supported in there, what
>> would be missing? (I don't see why it blocks on earlycon_map) And
>> would it be worth doing?
>
> The rt2880 does not have the same register locations as a 8250.
> The 8250 port driver remaps all register accesses with a LUT.
>
> Adding support would be trivial.

Ok, I will see if I can find some commit that does something similar to 
get some inspiration.

>
>>>>> 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.
>>>
>>> Just 'earlycon' triggers the attempted registration of earlycon matching the
>>> compatible string of the stdout-path node.
>>>
>>> The empty 'console' in bootargs is doing nothing.
>>
>> Ok, thanks.
>>
>> So, just to recap.
>> We would like to understand what is the right way of doing this:
>>
>> - we are using 8250 (rt288x variant) UART: CONFIG_SERIAL_8250_RT288X=y
>> - the UART 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 the UART.
>> There were two replies to that, one by Greg Kroah-Hartman
>> (http://www.spinics.net/lists/linux-serial/msg20278.html) and one by
>> you, where you suggested we use "console=uart", but as I reported
>> (http://www.spinics.net/lists/linux-serial/msg20307.html) it does not
>> work, you replied that iotype and mmio are not optional but mandatory
>> (http://www.spinics.net/lists/linux-serial/msg20310.html), and I
>> wondered if it was really necessary to duplicate data that is already
>> on the DT among other questions
>
> At the time, I didn't know you were describing your h/w with DT.

Oh, sorry for the inconvenience then.

>
> If you use the console command line (console= or earlycon=) to start
> an earlycon, then the uart address and iotype are mandatory.
> For this usage, earlycon matching is attempted with every EARLYCON_DECLARE().
>
> If you use plain "earlycon" on the command line, that will attempt to
> register the uart described by stdout-path in DT. For this usage,
> earlycon matching is attempted with the compatible string of every
> OF_EARLYCON_DECLARE().

I see, and since rt288x variant is not fully supported (no 
OF_EARLYCON_DECLARE) "earlycon" fails.

>
>> (http://www.spinics.net/lists/linux-serial/msg20383.html), like how
>> are DT described drivers supposed to interact with the
>> "console="/"earlycon=" commandlines
>
> They don't; those are orthogonal.

Ok.

>
>> , or, the contradiction between
>> "console=ttyS0" means '9600n81' and "if unspecified [the uart
>> options], the h/w is not re-initialized">
>
> I thought I was clear on that: "console=ttyS0" initializes the h/w
> to 9600n81 *because there are already existing users that must not break*.
>
> "console=uart,..." probes the h/w
> *because there are already existing users that must not break *

Thanks, I had misunderstood.

>
>> So, for us, it is still not clear what is the recommended way of
>> achieving our goal above, and it seems it is not clear what does
>> "console=ttyS0" is supposed to do, hardcode ('9600n81') or probe
>> ('the h/w is not re-initialized')
>
> The DT way will be simplest at this point because you won't
> have to write console handover matching for "console=rt288x,..."
>
> With DT (ie, stdout-path) earlycon, when a serial driver loads,
> an attempt is made to cross-reference any existing console with
> the node that is loading and will do a console takeover from
> a running earlycon for a matching uart node.
>
> There is a bug with DT earlycon though.
> If you have a dummy console that loads, the DT earlycon is
> disabled at that point because boot consoles are disabled when
> "real" consoles load.

I'm sorry for my ignorance, but what is a "dummy console"? Under what 
circumstances would it load and this bug be seen?

Regards,

Sebastian

Re: [RFC PATCH] always probe UART HW when options are not specified

[Sebastian Frias]

On 01/12/2016 10:37 AM, Mason wrote:
> On 11/01/2016 20:06, Peter Hurley wrote:
>
>> I don't have enough information to suggest what you "should" use
>> here.
>>
>> Is this going to be a shipping product?
>> Is it single-core?
>> etc.
>>
>> And what is your purpose for outputting early boot information
>> before loading the serial driver which does provide console output?
>
> I'm not sure exactly which board Sebastian has in mind, but I've been
> using the SERIAL_8250_RT288X driver on my Tango4 board.

For the record, I'm using a SoC emulator, and thus do not have a 
bootloader per se and there are a bunch of other things that I cannot 
count on.
The emulator has the UART pre-setup, so I just need Linux to take over 
without changing the parameters.
Ideally, I would like to have the same image of Linux+DT to start in any 
instance of the emulator or real chips, regardless of the clock ratios, 
that's why I sort of need Linux to not change the UART speed, which is 
quite tricky because there are no clock generators in the emulator.

NOTE: on my tree I'm using the patch I previously submitted here, so 
Linux is probing the UART so it works for all my cases, but I would like 
to go back to a standard tree.

>
> Note: calling the UART driver "Au1x00/RT288x" is a bit of a misnomer,
> as these are names of SoCs using that distinct register layout.
> The actual IP is probably the 16550-compatible Palmchip BK-3103.
> https://sites.google.com/a/palmchiptech.com/palmchiptech/product-services/hardware-services/ip-cores/bk-3103
> (Not sure that this website is legitimate, though.)
>
> When I need earlyprintk support, I use this patch from Mans:
> http://thread.gmane.org/gmane.linux.kernel/2081016
>
> Regards.
>

Re: [RFC PATCH] always probe UART HW when options are not specified

[Peter Hurley]

On 01/12/2016 06:22 AM, Sebastian Frias wrote:
> On 01/12/2016 10:37 AM, Mason wrote:
>> On 11/01/2016 20:06, Peter Hurley wrote:
>>
>>> I don't have enough information to suggest what you "should" use
>>> here.
>>>
>>> Is this going to be a shipping product?
>>> Is it single-core?
>>> etc.
>>>
>>> And what is your purpose for outputting early boot information
>>> before loading the serial driver which does provide console output?
>>
>> I'm not sure exactly which board Sebastian has in mind, but I've been
>> using the SERIAL_8250_RT288X driver on my Tango4 board.
> 
> For the record, I'm using a SoC emulator, and thus do not have a bootloader per se and there are a bunch of other things that I cannot count on.
> The emulator has the UART pre-setup, so I just need Linux to take over without changing the parameters.
> Ideally, I would like to have the same image of Linux+DT to start in any instance of the emulator or real chips, regardless of the clock ratios, that's why I sort of need Linux to not change the UART speed, which is quite tricky because there are no clock generators in the emulator.

Got it, thanks for the info.
Please test the series I just cc'd you on plus the patch I sent
you yesterday.

That should get you an earlycon up and running on that simulator;
let me know if it doesn't and we'll go from there.

> NOTE: on my tree I'm using the patch I previously submitted here, so Linux is probing the UART so it works for all my cases, but I would like to go back to a standard tree.

I expect that patch to go into -next sometime during the 4.5-rc cycle.


Regards,
Peter Hurley

>> Note: calling the UART driver "Au1x00/RT288x" is a bit of a misnomer,
>> as these are names of SoCs using that distinct register layout.
>> The actual IP is probably the 16550-compatible Palmchip BK-3103.
>> https://sites.google.com/a/palmchiptech.com/palmchiptech/product-services/hardware-services/ip-cores/bk-3103
>> (Not sure that this website is legitimate, though.)
>>
>> When I need earlyprintk support, I use this patch from Mans:
>> http://thread.gmane.org/gmane.linux.kernel/2081016
>>
>> Regards.
>>

Re: [RFC PATCH] always probe UART HW when options are not specified

[Peter Hurley]

Hi Sebastian,

On 01/12/2016 06:14 AM, Sebastian Frias wrote:
> On 01/11/2016 08:06 PM, Peter Hurley wrote:
>> On 01/11/2016 09:56 AM, Sebastian Frias wrote:
>>> On 01/11/2016 05:11 PM, Peter Hurley wrote:
>>>> On 01/11/2016 07:07 AM, Sebastian Frias wrote:
>>>>> On 12/22/2015 06:56 PM, Sebastian Frias wrote:
>>>>>>
>>>>>>        OF_EARLYCON_DECLARE(rt2880, "ralink,rt2880-uart",
>>>>>> early_serial8250_setup);
>>>>
>>>> There is no support for this uart in 8250 earlycon; the registers
>>>> need remapped.
>>>
>>> Ok, two questions then:
>>> 1) If the UART is not supported on 8250 earlycon, what is the
>>> suggested/advised solution? Using just "earlyprintk"?
>>
>> I don't have enough information to suggest what you "should" use
>> here.
>>
>> Is this going to be a shipping product?
>> Is it single-core?
>> etc.
>>
>> And what is your purpose for outputting early boot information
>> before loading the serial driver which does provide console output?
>>
> 
> No, it is not for production, just for debug, but we would like to understand if there is a standard way of doing so, so that whenever somebody ask us for "early print", we can provide with the good way.
> I know we can always provide with hacks, I'm just wondering if there's a "standard way".

Ok.

The "standard" solution is either command-line or DT earlycon (which
we've established isn't working for your setup).

Let's fix that so that both of those options work.


>>> 2) What would it take to make the "rt2880" work with the 8250
>>> earlycon? I mean, it is already pretty much supported in there, what
>>> would be missing? (I don't see why it blocks on earlycon_map) And
>>> would it be worth doing?
>>
>> The rt2880 does not have the same register locations as a 8250.
>> The 8250 port driver remaps all register accesses with a LUT.
>>
>> Adding support would be trivial.
> 
> Ok, I will see if I can find some commit that does something similar to get some inspiration.

I sent you a patch yesterday that adds earlycon support for
this uart (so command line should work), plus the series I sent today
should get DT earlycon up and running. Please test both.


>>>>>> 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.
>>>>
>>>> Just 'earlycon' triggers the attempted registration of earlycon matching the
>>>> compatible string of the stdout-path node.
>>>>
>>>> The empty 'console' in bootargs is doing nothing.
>>>
>>> Ok, thanks.
>>>
>>> So, just to recap.
>>> We would like to understand what is the right way of doing this:
>>>
>>> - we are using 8250 (rt288x variant) UART: CONFIG_SERIAL_8250_RT288X=y
>>> - the UART 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 the UART.
>>> There were two replies to that, one by Greg Kroah-Hartman
>>> (http://www.spinics.net/lists/linux-serial/msg20278.html) and one by
>>> you, where you suggested we use "console=uart", but as I reported
>>> (http://www.spinics.net/lists/linux-serial/msg20307.html) it does not
>>> work, you replied that iotype and mmio are not optional but mandatory
>>> (http://www.spinics.net/lists/linux-serial/msg20310.html), and I
>>> wondered if it was really necessary to duplicate data that is already
>>> on the DT among other questions
>>
>> At the time, I didn't know you were describing your h/w with DT.
> 
> Oh, sorry for the inconvenience then.

No big deal, I was just explaining the earlier advice.


>> If you use the console command line (console= or earlycon=) to start
>> an earlycon, then the uart address and iotype are mandatory.
>> For this usage, earlycon matching is attempted with every EARLYCON_DECLARE().
>>
>> If you use plain "earlycon" on the command line, that will attempt to
>> register the uart described by stdout-path in DT. For this usage,
>> earlycon matching is attempted with the compatible string of every
>> OF_EARLYCON_DECLARE().
> 
> I see, and since rt288x variant is not fully supported (no OF_EARLYCON_DECLARE) "earlycon" fails.

Yes.

>>> (http://www.spinics.net/lists/linux-serial/msg20383.html), like how
>>> are DT described drivers supposed to interact with the
>>> "console="/"earlycon=" commandlines
>>
>> They don't; those are orthogonal.
> 
> Ok.
> 
>>
>>> , or, the contradiction between
>>> "console=ttyS0" means '9600n81' and "if unspecified [the uart
>>> options], the h/w is not re-initialized">
>>
>> I thought I was clear on that: "console=ttyS0" initializes the h/w
>> to 9600n81 *because there are already existing users that must not break*.
>>
>> "console=uart,..." probes the h/w
>> *because there are already existing users that must not break *
> 
> Thanks, I had misunderstood.
> 
>>
>>> So, for us, it is still not clear what is the recommended way of
>>> achieving our goal above, and it seems it is not clear what does
>>> "console=ttyS0" is supposed to do, hardcode ('9600n81') or probe
>>> ('the h/w is not re-initialized')
>>
>> The DT way will be simplest at this point because you won't
>> have to write console handover matching for "console=rt288x,..."
>>
>> With DT (ie, stdout-path) earlycon, when a serial driver loads,
>> an attempt is made to cross-reference any existing console with
>> the node that is loading and will do a console takeover from
>> a running earlycon for a matching uart node.
>>
>> There is a bug with DT earlycon though.
>> If you have a dummy console that loads, the DT earlycon is
>> disabled at that point because boot consoles are disabled when
>> "real" consoles load.
> 
> I'm sorry for my ignorance, but what is a "dummy console"? Under what circumstances would it load and this bug be seen?

The dummy console is a hack used to preserve the priority of the
display console in the event no console is specified.

Here's an example:
[    0.000000] Booting Linux on physical CPU 0x0
[    0.000000] Initializing cgroup subsys cpuset
[    0.000000] Initializing cgroup subsys cpu
[    0.000000] Initializing cgroup subsys cpuacct
[    0.000000] Linux version 4.4.0-rc6+ (peter@thor) (gcc version 4.8.2 (Ubuntu/Linaro 4.8.2-16ubuntu4) ) #86 PREEMPT Tue Jan 12 12:30:39 PST 2016
[    0.000000] CPU: ARMv7 Processor [413fc082] revision 2 (ARMv7), cr=10c5387d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
[    0.000000] Machine model: TI AM335x BeagleBone Black
[    0.000000] earlycon: omap8250 at MMIO 0x44e09000 (options '')
[    0.000000] bootconsole [omap8250] enabled
[    0.000000] cma: Reserved 16 MiB at 0x9f000000
[    0.000000] Memory policy: Data cache writeback
[    0.000000] CPU: All CPU(s) started in SVC mode.
[    0.000000] AM335X ES2.1 (sgx neon )
[    0.000000] Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 129920
[    0.000000] Kernel command line: earlycon root=/dev/mmcblk0p2 ro fixrtc rootfstype=ext4 rootwait
[    0.000000] PID hash table entries: 2048 (order: 1, 8192 bytes)
[    0.000000] Dentry cache hash table entries: 65536 (order: 6, 262144 bytes)
[    0.000000] Inode-cache hash table entries: 32768 (order: 5, 131072 bytes)
[    0.000000] Memory: 486332K/524288K available (10582K kernel code, 862K rwdata, 3396K rodata, 596K init, 871K bss, 21572K reserved, 16384K cma-reserved, 0K highmem)
[    0.000000] Virtual kernel memory layout:
[    0.000000]     vector  : 0xffff0000 - 0xffff1000   (   4 kB)
[    0.000000]     fixmap  : 0xffc00000 - 0xfff00000   (3072 kB)
[    0.000000]     vmalloc : 0xe0800000 - 0xff800000   ( 496 MB)
[    0.000000]     lowmem  : 0xc0000000 - 0xe0000000   ( 512 MB)
[    0.000000]     pkmap   : 0xbfe00000 - 0xc0000000   (   2 MB)
[    0.000000]     modules : 0xbf000000 - 0xbfe00000   (  14 MB)
[    0.000000]       .text : 0xc0008000 - 0xc0daebf0   (13979 kB)
[    0.000000]       .init : 0xc0daf000 - 0xc0e44000   ( 596 kB)
[    0.000000]       .data : 0xc0e44000 - 0xc0f1bb3c   ( 863 kB)
[    0.000000]        .bss : 0xc0f1e000 - 0xc0ff7d84   ( 872 kB)
[    0.000000] Preemptible hierarchical RCU implementation.
[    0.000000]  Build-time adjustment of leaf fanout to 32.
[    0.000000] NR_IRQS:16 nr_irqs:16 16
[    0.000000] IRQ: Found an INTC at 0xfa200000 (revision 5.0) with 128 interrupts
[    0.000000] OMAP clockevent source: timer2 at 24000000 Hz
[    0.000014] sched_clock: 32 bits at 24MHz, resolution 41ns, wraps every 89478484971ns
[    0.008083] clocksource: timer1: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns
[    0.017570] OMAP clocksource: timer1 at 24000000 Hz
[    0.022969] Console: colour dummy device 80x30
[    0.027570] console [tty0] enabled
[    0.031100] bootconsole [omap8250] disabled
Ubuntu 14.04.3 LTS black ttyS0

black login:


As you can see, there's boot console but no regular console until
getty spawns.

I've fixed the problem once, but that broke some setups and had to be
reverted so I need to rethink an alternate approach. A unified, coherent
solution is elusive because of the many different possible console setups.

Regards,
Peter Hurley

Re: [RFC PATCH] always probe UART HW when options are not specified

[Mason]

On 12/01/2016 20:47, Peter Hurley wrote:

> I expect that patch to go into -next sometime during the 4.5-rc cycle.

Do you mean the "Add Au1x00/RT288x earlycon support" patch?

I find it strange that you have rt288x_putc while also
using AU1x00_FOO. Seems like mixing naming styles.

Did you see my post about the underlying IP block?
Might be appropriate to use that name there?

Regards.

Re: [RFC PATCH] always probe UART HW when options are not specified

[Peter Hurley]

On 01/12/2016 02:26 PM, Mason wrote:
> On 12/01/2016 20:47, Peter Hurley wrote:
> 
>> I expect that patch to go into -next sometime during the 4.5-rc cycle.
> 
> Do you mean the "Add Au1x00/RT288x earlycon support" patch?

No. This one http://www.spinics.net/lists/linux-serial/msg20328.html

> I find it strange that you have rt288x_putc while also
> using AU1x00_FOO. Seems like mixing naming styles.

I wrote it in 10 mins.

> Did you see my post about the underlying IP block?
> Might be appropriate to use that name there?

I did, and thanks for reminding me. I did follow that email thread
a while back about the provenance of this register layout.

I'd rather not go back through the source and rename the symbols.
Afaict, the Au1x00 landed first in source and then the RT288x afterwards.
So I'd like to stick with those.

I named the register layout on the basis of the original usage
which started in the 8250 port driver with the au_* accessor functions.

The earlycon functions I named that way because that's the name of
the earlycon (which originates from the DT compatible key which can't be
changed).

I do want people to be able to find other relevant code by grepping
these names, so I was thinking of adding a follow-on patch that uses the
AU1x00_ register names in the 8250 port driver LUT.

Regards,
Peter Hurley

Re: [RFC PATCH] always probe UART HW when options are not specified

[Sebastian Frias]

Hi Peter,

On 01/12/2016 08:47 PM, Peter Hurley wrote:
> On 01/12/2016 06:22 AM, Sebastian Frias wrote:
>>
>> For the record, I'm using a SoC emulator, and thus do not have a bootloader per se and there are a bunch of other things that I cannot count on.
>> The emulator has the UART pre-setup, so I just need Linux to take over without changing the parameters.
>> Ideally, I would like to have the same image of Linux+DT to start in any instance of the emulator or real chips, regardless of the clock ratios, that's why I sort of need Linux to not change the UART speed, which is quite tricky because there are no clock generators in the emulator.
>
> Got it, thanks for the info.
> Please test the series I just cc'd you on plus the patch I sent
> you yesterday.
>
> That should get you an earlycon up and running on that simulator;
> let me know if it doesn't and we'll go from there.

Ok, thanks.
I will try as soon as we finish rebasing our changes on top of 
"mainline" (or HEAD, or is it "-next"? I don't know how you guys call 
the most recent code base for Linux)

Actually, I tried yesterday on a 4.1.13 but 
"[drivers/tty/serial/earlycon.c:62] earlycon_map()" was still the last 
message I got, just as with the OF_EARLYCON_DECLARE hack I had 
previously talk about, and so I would like to test on the same 
conditions than you, mainline.

>
>> NOTE: on my tree I'm using the patch I previously submitted here, so Linux is probing the UART so it works for all my cases, but I would like to go back to a standard tree.
>
> I expect that patch to go into -next sometime during the 4.5-rc cycle.

Sounds good!
Thanks,

Sebastian

>
>
> Regards,
> Peter Hurley
>
>>> Note: calling the UART driver "Au1x00/RT288x" is a bit of a misnomer,
>>> as these are names of SoCs using that distinct register layout.
>>> The actual IP is probably the 16550-compatible Palmchip BK-3103.
>>> https://sites.google.com/a/palmchiptech.com/palmchiptech/product-services/hardware-services/ip-cores/bk-3103
>>> (Not sure that this website is legitimate, though.)
>>>
>>> When I need earlyprintk support, I use this patch from Mans:
>>> http://thread.gmane.org/gmane.linux.kernel/2081016
>>>
>>> Regards.
>>>
>

Re: [RFC PATCH] always probe UART HW when options are not specified

[Peter Hurley]

On 01/13/2016 03:14 AM, Sebastian Frias wrote:
> Hi Peter,
> 
> On 01/12/2016 08:47 PM, Peter Hurley wrote:
>> On 01/12/2016 06:22 AM, Sebastian Frias wrote:
>>>
>>> For the record, I'm using a SoC emulator, and thus do not have a bootloader per se and there are a bunch of other things that I cannot count on.
>>> The emulator has the UART pre-setup, so I just need Linux to take over without changing the parameters.
>>> Ideally, I would like to have the same image of Linux+DT to start in any instance of the emulator or real chips, regardless of the clock ratios, that's why I sort of need Linux to not change the UART speed, which is quite tricky because there are no clock generators in the emulator.
>>
>> Got it, thanks for the info.
>> Please test the series I just cc'd you on plus the patch I sent
>> you yesterday.
>>
>> That should get you an earlycon up and running on that simulator;
>> let me know if it doesn't and we'll go from there.
> 
> Ok, thanks.
> I will try as soon as we finish rebasing our changes on top of
> "mainline" (or HEAD, or is it "-next"? I don't know how you guys call
> the most recent code base for Linux)

The basic tree organization is

   Linus's tree  <---- linux-next[date]  <----- maintainers' trees
   (mainline)          (next)                   (eg., Greg's tty tree)

The patches I sent you (along with any required modifications during the
review cycle) are based on the tty-next branch in Greg's tty tree here
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git

As of this email, Greg's tree is based on Linus's 4.4-rc6 which should
be stable enough for you to test these patches on.

> Actually, I tried yesterday on a 4.1.13 but
> "[drivers/tty/serial/earlycon.c:62] earlycon_map()" was still the
> last message I got, just as with the OF_EARLYCON_DECLARE hack I had
> previously talk about, and so I would like to test on the same
> conditions than you, mainline.

So just to confirm, you applied "8250: Add Au1x00/RT288x earlycon support"
to 4.1.13 and the earlycon didn't come up when you used the kernel
command line option like so:

	earlycon=rt288x,mmio32,0x10700

Is that correct?

Re: [RFC PATCH] always probe UART HW when options are not specified

[Sebastian Frias]

Hi Peter,

On 01/13/2016 05:34 PM, Peter Hurley wrote:
> On 01/13/2016 03:14 AM, Sebastian Frias wrote:
>> Hi Peter,
>>
>> On 01/12/2016 08:47 PM, Peter Hurley wrote:
>>> On 01/12/2016 06:22 AM, Sebastian Frias wrote:
>>>>
>>>> For the record, I'm using a SoC emulator, and thus do not have a bootloader per se and there are a bunch of other things that I cannot count on.
>>>> The emulator has the UART pre-setup, so I just need Linux to take over without changing the parameters.
>>>> Ideally, I would like to have the same image of Linux+DT to start in any instance of the emulator or real chips, regardless of the clock ratios, that's why I sort of need Linux to not change the UART speed, which is quite tricky because there are no clock generators in the emulator.
>>>
>>> Got it, thanks for the info.
>>> Please test the series I just cc'd you on plus the patch I sent
>>> you yesterday.
>>>
>>> That should get you an earlycon up and running on that simulator;
>>> let me know if it doesn't and we'll go from there.
>>
>> Ok, thanks.
>> I will try as soon as we finish rebasing our changes on top of
>> "mainline" (or HEAD, or is it "-next"? I don't know how you guys call
>> the most recent code base for Linux)
>
> The basic tree organization is
>
>     Linus's tree  <---- linux-next[date]  <----- maintainers' trees
>     (mainline)          (next)                   (eg., Greg's tty tree)
>
> The patches I sent you (along with any required modifications during the
> review cycle) are based on the tty-next branch in Greg's tty tree here
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
>
> As of this email, Greg's tree is based on Linus's 4.4-rc6 which should
> be stable enough for you to test these patches on.

Thanks for your explanation :-)

>
>> Actually, I tried yesterday on a 4.1.13 but
>> "[drivers/tty/serial/earlycon.c:62] earlycon_map()" was still the
>> last message I got, just as with the OF_EARLYCON_DECLARE hack I had
>> previously talk about, and so I would like to test on the same
>> conditions than you, mainline.
>
> So just to confirm, you applied "8250: Add Au1x00/RT288x earlycon support"
> to 4.1.13 and the earlycon didn't come up when you used the kernel
> command line option like so:
>
> 	earlycon=rt288x,mmio32,0x10700
>
> Is that correct?
>

Sort of, I applied your patch "8250: Add Au1x00/RT288x earlycon 
support", and then tried with something like:

	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>;
	};

I can see that the code is correctly picking up the rt288x from the DT 
(I put logs):

...
[drivers/of/fdt.c:834] early_init_dt_scan_chosen_serial(): 
'ralink,rt2880-uart'
[drivers/tty/serial/earlycon.c:239] of_setup_earlycon(): addr=00010700 
setup@c024c14c
[drivers/tty/serial/earlycon.c:62] earlycon_map(): paddr 0x00010700 size 
4096

(last log visible on UART)

where 0xc024c14c resolves to 'early_rt288x_setup' from your patch (in 
the disassembly)

This happens when parsing the first token of the bootargs, "earlycon".

I will let you know how it goes the tests on 4.4.

Best regards,

Sebastian