| Message ID | 20210723223152.648326-5-sean.anderson@seco.com |
|---|---|
| State | New |
| Headers | show |
| Series | tty: serial: uartlite: Disable changing fixed parameters | expand |
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@seco.com> > --- > > 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. --Sean > > dev_set_drvdata(dev, port); > > @@ -756,18 +770,58 @@ static int ulite_probe(struct platform_device *pdev) > struct uartlite_data *pdata; > int irq, ret; > int id = pdev->id; > -#ifdef CONFIG_OF > - const __be32 *prop; > > - prop = of_get_property(pdev->dev.of_node, "port-number", NULL); > - if (prop) > - id = be32_to_cpup(prop); > -#endif > pdata = devm_kzalloc(&pdev->dev, sizeof(struct uartlite_data), > GFP_KERNEL); > if (!pdata) > return -ENOMEM; > > + if (IS_ENABLED(CONFIG_OF)) { > + const char *prop; > + struct device_node *np = pdev->dev.of_node; > + u32 val; > + > + prop = "port-number"; > + ret = of_property_read_u32(np, prop, &id); > + if (ret && ret != -EINVAL) > +of_err: > + return dev_err_probe(&pdev->dev, ret, > + "could not read %s\n", prop); > + > + prop = "current-speed"; > + ret = of_property_read_u32(np, prop, &pdata->baud); > + if (ret) > + goto of_err; > + > + prop = "xlnx,use-parity"; > + ret = of_property_read_u32(np, prop, &val); > + if (ret && ret != -EINVAL) > + goto of_err; > + > + if (val) { > + prop = "xlnx,odd-parity"; > + ret = of_property_read_u32(np, prop, &val); > + if (ret) > + goto of_err; > + > + if (val) > + pdata->parity = 'o'; > + else > + pdata->parity = 'e'; > + } else { > + pdata->parity = 'n'; > + } > + > + prop = "xlnx,data-bits"; > + ret = of_property_read_u32(np, prop, &pdata->bits); > + if (ret && ret != -EINVAL) > + goto of_err; > + } else { > + pdata->baud = 9600; > + pdata->parity = 'n'; > + pdata->bits = 8; > + } > + > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!res) > return -ENODEV; >
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@seco.com> > > --- > > > > 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
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'); dev_set_drvdata(dev, port); @@ -756,18 +770,58 @@ static int ulite_probe(struct platform_device *pdev) struct uartlite_data *pdata; int irq, ret; int id = pdev->id; -#ifdef CONFIG_OF - const __be32 *prop; - prop = of_get_property(pdev->dev.of_node, "port-number", NULL); - if (prop) - id = be32_to_cpup(prop); -#endif pdata = devm_kzalloc(&pdev->dev, sizeof(struct uartlite_data), GFP_KERNEL); if (!pdata) return -ENOMEM; + if (IS_ENABLED(CONFIG_OF)) { + const char *prop; + struct device_node *np = pdev->dev.of_node; + u32 val; + + prop = "port-number"; + ret = of_property_read_u32(np, prop, &id); + if (ret && ret != -EINVAL) +of_err: + return dev_err_probe(&pdev->dev, ret, + "could not read %s\n", prop); + + prop = "current-speed"; + ret = of_property_read_u32(np, prop, &pdata->baud); + if (ret) + goto of_err; + + prop = "xlnx,use-parity"; + ret = of_property_read_u32(np, prop, &val); + if (ret && ret != -EINVAL) + goto of_err; + + if (val) { + prop = "xlnx,odd-parity"; + ret = of_property_read_u32(np, prop, &val); + if (ret) + goto of_err; + + if (val) + pdata->parity = 'o'; + else + pdata->parity = 'e'; + } else { + pdata->parity = 'n'; + } + + prop = "xlnx,data-bits"; + ret = of_property_read_u32(np, prop, &pdata->bits); + if (ret && ret != -EINVAL) + goto of_err; + } else { + pdata->baud = 9600; + pdata->parity = 'n'; + pdata->bits = 8; + } + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) return -ENODEV;
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@seco.com> --- drivers/tty/serial/uartlite.c | 66 +++++++++++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 6 deletions(-)