On Mon, Jul 26, 2021 at 04:53:39PM -0400, Sean Anderson wrote:
>
>
> On 7/23/21 6:31 PM, Sean Anderson wrote:
> > This reads the various new devicetree parameters to discover how the
> > uart was configured when it was synthesized. Note that these properties
> > are fixed and undiscoverable. Once we have determined how the uart is
> > configured, we set the termios to let users know, and to initialize the
> > timeout to the correct value.
> >
> > The defaults match ulite_console_setup. xlnx,use-parity,
> > xlnx,odd-parity, and xlnx,data-bits are optional since there were
> > in-tree users (and presumably out-of-tree users) who did not set them.
> >
> > Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx>
> > ---
> >
> > drivers/tty/serial/uartlite.c | 66 +++++++++++++++++++++++++++++++----
> > 1 file changed, 60 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> > index f42ccc40ffa6..39c17ab206ca 100644
> > --- a/drivers/tty/serial/uartlite.c
> > +++ b/drivers/tty/serial/uartlite.c
> > @@ -60,9 +60,20 @@
> > static struct uart_port *console_port;
> > #endif
> >
> > +/**
> > + * struct uartlite_data: Driver private data
> > + * reg_ops: Functions to read/write registers
> > + * clk: Our parent clock, if present
> > + * baud: The baud rate configured when this device was synthesized
> > + * parity: The parity settings, like for uart_set_options()
> > + * bits: The number of data bits
> > + */
> > struct uartlite_data {
> > const struct uartlite_reg_ops *reg_ops;
> > struct clk *clk;
> > + int baud;
> > + int parity;
> > + int bits;
> > };
> >
> > struct uartlite_reg_ops {
> > @@ -652,6 +663,9 @@ static int ulite_assign(struct device *dev, int id, u32 base, int irq,
> > port->type = PORT_UNKNOWN;
> > port->line = id;
> > port->private_data = pdata;
> > + /* Initialize the termios to what was configured at synthesis-time */
> > + uart_set_options(port, NULL, pdata->baud, pdata->parity, pdata->bits,
> > + 'n');
>
> I did some testing today, and discovered that the termios are not set
> properly. I think I missed this the first time around because on
> Microblaze QEMU this UART is the console, and the baud etc. gets set
> properly because of stdout-path (or bootargs). However, uart_set_options
> doesn't actually do anything with the termios when co is NULL.
>
> The initial termios are set up by tty_init_termios (called from
> tty_init_dev). They come from either tty->driver->init_termios, or from
> tty->driver->termios[idx]. There is only one init_termios per-driver,
> so we would need to have multiple drivers if we wanted to have multiple
> UARTs with different (e.g.) bauds.
>
> The indexed termios are designed to keep the settings from the previous
> time the tty was opened. So I think (ab)using them is not too terrible,
> especially since we will only set them once. Unfortunately, we cannot
> use tty_save_termios to initialize the indexed termio, since the tty is
> not set until tty_port_open, which is called after tty_init_dev.
>
> Based on this, I think the neatest cut would be something like
>
> /* perhaps just do this in ulite_assign? */
> ulite_request_port () {
> /* ... */
> termios = &ulite_uart_driver.tty_driver->termios[port->line];
> termios = kzalloc(sizeof(*termios));
> if (!termios)
> /* ... */
> termios.c_cflags = /* ... */
> /* etc */
> }
>
> Unfortunately, according to include/linux/serial_core.h, tty_driver is
> not supposed to be touched by the low-level driver. But I think we have
> a bit of an unusual case here with a device that can't change baud. If
> anyone has other suggestions, I'm all for them.
If a driver can not support changes in baud rates, then just ignore all
changes to baud rates as there will not be an issue for anyone :)
thanks,
greg k-h
