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

[Sebastian Frias <sf84@xxxxxxxxxxx>]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html