| Message ID | 20230704135936.14697-4-ddrokosov@sberdevices.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tty: serial: meson: support ttyS devname | expand |
On 04/07/2023 15:59, Dmitry Rokosov wrote: > It is worth noting that the devname ttyS is a widely recognized tty name > and is commonly used by many uart device drivers. Given the established > usage and compatibility concerns, it may not be feasible to change the > devname for older SoCs. However, for new definitions, it is acceptable > and even recommended to use a new devname to help ensure clarity and > avoid any potential conflicts on lower or upper software levels. In > addition, modify the meson_uart_dt match data for g12a, a1, and s4 to > their appropriate values to ensure proper devname values and > functionality. I'm not confident about modifying a global struct from a probe, I think you should add a separate meson_uart_driver/meson_serial_console couple with ttyS instead of ttyAML, refer to the right uart_driver in meson_uart_data and in probe() register it and pass it to uart_add_one_port(). Neil > > For more information please refer to IRC discussion at [1]. > > Links: > [1]: https://libera.irclog.whitequark.org/linux-amlogic/2023-07-03 > > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > --- > drivers/tty/serial/meson_uart.c | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c > index 87c0eb5f2dba..361f9326b527 100644 > --- a/drivers/tty/serial/meson_uart.c > +++ b/drivers/tty/serial/meson_uart.c > @@ -82,6 +82,7 @@ static struct uart_driver meson_uart_driver; > static struct uart_port *meson_ports[AML_UART_PORT_NUM]; > > struct meson_uart_data { > + const char *dev_name; > bool has_xtal_div2; > }; > > @@ -683,6 +684,7 @@ static int meson_uart_probe_clocks(struct platform_device *pdev, > > static int meson_uart_probe(struct platform_device *pdev) > { > + const struct meson_uart_data *priv_data; > struct resource *res_mem; > struct uart_port *port; > u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */ > @@ -729,6 +731,18 @@ static int meson_uart_probe(struct platform_device *pdev) > if (ret) > return ret; > > + priv_data = device_get_match_data(&pdev->dev); > + > + if (priv_data) { > + struct console *cons = meson_uart_driver.cons; > + > + meson_uart_driver.dev_name = priv_data->dev_name; > + > + if (cons) > + strscpy(cons->name, priv_data->dev_name, > + sizeof(cons->name)); > + } > + > if (!meson_uart_driver.state) { > ret = uart_register_driver(&meson_uart_driver); > if (ret) > @@ -748,7 +762,7 @@ static int meson_uart_probe(struct platform_device *pdev) > port->x_char = 0; > port->ops = &meson_uart_ops; > port->fifosize = fifosize; > - port->private_data = (void *)device_get_match_data(&pdev->dev); > + port->private_data = (void *)priv_data; > > meson_ports[pdev->id] = port; > platform_set_drvdata(pdev, port); > @@ -780,6 +794,17 @@ static int meson_uart_remove(struct platform_device *pdev) > } > > static struct meson_uart_data meson_g12a_uart_data = { > + .dev_name = "ttyAML", > + .has_xtal_div2 = true, > +}; > + > +static struct meson_uart_data meson_a1_uart_data = { > + .dev_name = "ttyS", > + .has_xtal_div2 = false, > +}; > + > +static struct meson_uart_data meson_s4_uart_data = { > + .dev_name = "ttyS", > .has_xtal_div2 = true, > }; > > @@ -794,7 +819,11 @@ static const struct of_device_id meson_uart_dt_match[] = { > }, > { > .compatible = "amlogic,meson-s4-uart", > - .data = (void *)&meson_g12a_uart_data, > + .data = (void *)&meson_s4_uart_data, > + }, > + { > + .compatible = "amlogic,meson-a1-uart", > + .data = (void *)&meson_a1_uart_data, > }, > { /* sentinel */ }, > };
Hello Neil, Thank you for quick feedback! On Tue, Jul 04, 2023 at 04:42:39PM +0200, neil.armstrong@linaro.org wrote: > On 04/07/2023 15:59, Dmitry Rokosov wrote: > > It is worth noting that the devname ttyS is a widely recognized tty name > > and is commonly used by many uart device drivers. Given the established > > usage and compatibility concerns, it may not be feasible to change the > > devname for older SoCs. However, for new definitions, it is acceptable > > and even recommended to use a new devname to help ensure clarity and > > avoid any potential conflicts on lower or upper software levels. In > > addition, modify the meson_uart_dt match data for g12a, a1, and s4 to > > their appropriate values to ensure proper devname values and > > functionality. > > I'm not confident about modifying a global struct from a probe, > I think you should add a separate meson_uart_driver/meson_serial_console couple > with ttyS instead of ttyAML, refer to the right uart_driver in meson_uart_data > and in probe() register it and pass it to uart_add_one_port(). Could you provide some insight into why you believe this solution may not be acceptable? It appears that the meson_uart_driver and meson_serial_console are not labeled with __init, which means it stay in memory forever. To clarify, are you suggesting a solution that involves segregating the meson_uart_driver and meson_serial_console objects for each scenario and subsequently declaring pointers to both objects within the meson_uart_data? I want to make sure that I have accurately grasped the essence of your proposed approach. > > > > For more information please refer to IRC discussion at [1]. > > > > Links: > > [1]: https://libera.irclog.whitequark.org/linux-amlogic/2023-07-03 > > > > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > > --- > > drivers/tty/serial/meson_uart.c | 33 +++++++++++++++++++++++++++++++-- > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c > > index 87c0eb5f2dba..361f9326b527 100644 > > --- a/drivers/tty/serial/meson_uart.c > > +++ b/drivers/tty/serial/meson_uart.c > > @@ -82,6 +82,7 @@ static struct uart_driver meson_uart_driver; > > static struct uart_port *meson_ports[AML_UART_PORT_NUM]; > > struct meson_uart_data { > > + const char *dev_name; > > bool has_xtal_div2; > > }; > > @@ -683,6 +684,7 @@ static int meson_uart_probe_clocks(struct platform_device *pdev, > > static int meson_uart_probe(struct platform_device *pdev) > > { > > + const struct meson_uart_data *priv_data; > > struct resource *res_mem; > > struct uart_port *port; > > u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */ > > @@ -729,6 +731,18 @@ static int meson_uart_probe(struct platform_device *pdev) > > if (ret) > > return ret; > > + priv_data = device_get_match_data(&pdev->dev); > > + > > + if (priv_data) { > > + struct console *cons = meson_uart_driver.cons; > > + > > + meson_uart_driver.dev_name = priv_data->dev_name; > > + > > + if (cons) > > + strscpy(cons->name, priv_data->dev_name, > > + sizeof(cons->name)); > > + } > > + > > if (!meson_uart_driver.state) { > > ret = uart_register_driver(&meson_uart_driver); > > if (ret) > > @@ -748,7 +762,7 @@ static int meson_uart_probe(struct platform_device *pdev) > > port->x_char = 0; > > port->ops = &meson_uart_ops; > > port->fifosize = fifosize; > > - port->private_data = (void *)device_get_match_data(&pdev->dev); > > + port->private_data = (void *)priv_data; > > meson_ports[pdev->id] = port; > > platform_set_drvdata(pdev, port); > > @@ -780,6 +794,17 @@ static int meson_uart_remove(struct platform_device *pdev) > > } > > static struct meson_uart_data meson_g12a_uart_data = { > > + .dev_name = "ttyAML", > > + .has_xtal_div2 = true, > > +}; > > + > > +static struct meson_uart_data meson_a1_uart_data = { > > + .dev_name = "ttyS", > > + .has_xtal_div2 = false, > > +}; > > + > > +static struct meson_uart_data meson_s4_uart_data = { > > + .dev_name = "ttyS", > > .has_xtal_div2 = true, > > }; > > @@ -794,7 +819,11 @@ static const struct of_device_id meson_uart_dt_match[] = { > > }, > > { > > .compatible = "amlogic,meson-s4-uart", > > - .data = (void *)&meson_g12a_uart_data, > > + .data = (void *)&meson_s4_uart_data, > > + }, > > + { > > + .compatible = "amlogic,meson-a1-uart", > > + .data = (void *)&meson_a1_uart_data, > > }, > > { /* sentinel */ }, > > }; >
Hi. On 04/07/2023 16:59, Dmitry Rokosov wrote: > Hello Neil, > > Thank you for quick feedback! > > On Tue, Jul 04, 2023 at 04:42:39PM +0200, neil.armstrong@linaro.org wrote: >> On 04/07/2023 15:59, Dmitry Rokosov wrote: >>> It is worth noting that the devname ttyS is a widely recognized tty name >>> and is commonly used by many uart device drivers. Given the established >>> usage and compatibility concerns, it may not be feasible to change the >>> devname for older SoCs. However, for new definitions, it is acceptable >>> and even recommended to use a new devname to help ensure clarity and >>> avoid any potential conflicts on lower or upper software levels. In >>> addition, modify the meson_uart_dt match data for g12a, a1, and s4 to >>> their appropriate values to ensure proper devname values and >>> functionality. >> >> I'm not confident about modifying a global struct from a probe, >> I think you should add a separate meson_uart_driver/meson_serial_console couple >> with ttyS instead of ttyAML, refer to the right uart_driver in meson_uart_data >> and in probe() register it and pass it to uart_add_one_port(). > > Could you provide some insight into why you believe this solution may > not be acceptable? It appears that the meson_uart_driver and > meson_serial_console are not labeled with __init, which means it stay in > memory forever. Yes but nothing forbids registering a g12a and an a1 uart on the same system, but you modify the meson_uart_driver/meson_serial_console struct. This could cause some issues. In practice this will never happen, but since we don't control the DT passed to the system we must make sure we take in account any scenario. > > To clarify, are you suggesting a solution that involves segregating the > meson_uart_driver and meson_serial_console objects for each scenario and > subsequently declaring pointers to both objects within the > meson_uart_data? I want to make sure that I have accurately grasped the > essence of your proposed approach. Not both, only the appropriate one. So either we make sure ttyAML and ttyS at exclusive at runtime, in this case we can modify the global meson_uart_driver/meson_serial_console, or you add a second set of uart_driver/console structs to handle this improbable case. Neil > >>> >>> For more information please refer to IRC discussion at [1]. >>> >>> Links: >>> [1]: https://libera.irclog.whitequark.org/linux-amlogic/2023-07-03 >>> >>> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> >>> --- >>> drivers/tty/serial/meson_uart.c | 33 +++++++++++++++++++++++++++++++-- >>> 1 file changed, 31 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c >>> index 87c0eb5f2dba..361f9326b527 100644 >>> --- a/drivers/tty/serial/meson_uart.c >>> +++ b/drivers/tty/serial/meson_uart.c >>> @@ -82,6 +82,7 @@ static struct uart_driver meson_uart_driver; >>> static struct uart_port *meson_ports[AML_UART_PORT_NUM]; >>> struct meson_uart_data { >>> + const char *dev_name; >>> bool has_xtal_div2; >>> }; >>> @@ -683,6 +684,7 @@ static int meson_uart_probe_clocks(struct platform_device *pdev, >>> static int meson_uart_probe(struct platform_device *pdev) >>> { >>> + const struct meson_uart_data *priv_data; >>> struct resource *res_mem; >>> struct uart_port *port; >>> u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */ >>> @@ -729,6 +731,18 @@ static int meson_uart_probe(struct platform_device *pdev) >>> if (ret) >>> return ret; >>> + priv_data = device_get_match_data(&pdev->dev); >>> + >>> + if (priv_data) { >>> + struct console *cons = meson_uart_driver.cons; >>> + >>> + meson_uart_driver.dev_name = priv_data->dev_name; >>> + >>> + if (cons) >>> + strscpy(cons->name, priv_data->dev_name, >>> + sizeof(cons->name)); >>> + } >>> + >>> if (!meson_uart_driver.state) { >>> ret = uart_register_driver(&meson_uart_driver); >>> if (ret) >>> @@ -748,7 +762,7 @@ static int meson_uart_probe(struct platform_device *pdev) >>> port->x_char = 0; >>> port->ops = &meson_uart_ops; >>> port->fifosize = fifosize; >>> - port->private_data = (void *)device_get_match_data(&pdev->dev); >>> + port->private_data = (void *)priv_data; >>> meson_ports[pdev->id] = port; >>> platform_set_drvdata(pdev, port); >>> @@ -780,6 +794,17 @@ static int meson_uart_remove(struct platform_device *pdev) >>> } >>> static struct meson_uart_data meson_g12a_uart_data = { >>> + .dev_name = "ttyAML", >>> + .has_xtal_div2 = true, >>> +}; >>> + >>> +static struct meson_uart_data meson_a1_uart_data = { >>> + .dev_name = "ttyS", >>> + .has_xtal_div2 = false, >>> +}; >>> + >>> +static struct meson_uart_data meson_s4_uart_data = { >>> + .dev_name = "ttyS", >>> .has_xtal_div2 = true, >>> }; >>> @@ -794,7 +819,11 @@ static const struct of_device_id meson_uart_dt_match[] = { >>> }, >>> { >>> .compatible = "amlogic,meson-s4-uart", >>> - .data = (void *)&meson_g12a_uart_data, >>> + .data = (void *)&meson_s4_uart_data, >>> + }, >>> + { >>> + .compatible = "amlogic,meson-a1-uart", >>> + .data = (void *)&meson_a1_uart_data, >>> }, >>> { /* sentinel */ }, >>> }; >> >
On Tue, Jul 04, 2023 at 05:29:57PM +0200, neil.armstrong@linaro.org wrote: > Hi. > > On 04/07/2023 16:59, Dmitry Rokosov wrote: > > Hello Neil, > > > > Thank you for quick feedback! > > > > On Tue, Jul 04, 2023 at 04:42:39PM +0200, neil.armstrong@linaro.org wrote: > > > On 04/07/2023 15:59, Dmitry Rokosov wrote: > > > > It is worth noting that the devname ttyS is a widely recognized tty name > > > > and is commonly used by many uart device drivers. Given the established > > > > usage and compatibility concerns, it may not be feasible to change the > > > > devname for older SoCs. However, for new definitions, it is acceptable > > > > and even recommended to use a new devname to help ensure clarity and > > > > avoid any potential conflicts on lower or upper software levels. In > > > > addition, modify the meson_uart_dt match data for g12a, a1, and s4 to > > > > their appropriate values to ensure proper devname values and > > > > functionality. > > > > > > I'm not confident about modifying a global struct from a probe, > > > I think you should add a separate meson_uart_driver/meson_serial_console couple > > > with ttyS instead of ttyAML, refer to the right uart_driver in meson_uart_data > > > and in probe() register it and pass it to uart_add_one_port(). > > > > Could you provide some insight into why you believe this solution may > > not be acceptable? It appears that the meson_uart_driver and > > meson_serial_console are not labeled with __init, which means it stay in > > memory forever. > Yes but nothing forbids registering a g12a and an a1 uart on the same system, > but you modify the meson_uart_driver/meson_serial_console struct. > This could cause some issues. > > In practice this will never happen, but since we don't control the DT passed > to the system we must make sure we take in account any scenario. > Yep, agree, this will cause an issue. > > > > To clarify, are you suggesting a solution that involves segregating the > > meson_uart_driver and meson_serial_console objects for each scenario and > > subsequently declaring pointers to both objects within the > > meson_uart_data? I want to make sure that I have accurately grasped the > > essence of your proposed approach. > > Not both, only the appropriate one. > > So either we make sure ttyAML and ttyS at exclusive at runtime, in this case we can > modify the global meson_uart_driver/meson_serial_console, or you add > a second set of uart_driver/console structs to handle this improbable case. Got it, thank you for explanation! I will prepare the v2. > > > > > > > > > > For more information please refer to IRC discussion at [1]. > > > > > > > > Links: > > > > [1]: https://libera.irclog.whitequark.org/linux-amlogic/2023-07-03 > > > > > > > > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > > > > --- > > > > drivers/tty/serial/meson_uart.c | 33 +++++++++++++++++++++++++++++++-- > > > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c > > > > index 87c0eb5f2dba..361f9326b527 100644 > > > > --- a/drivers/tty/serial/meson_uart.c > > > > +++ b/drivers/tty/serial/meson_uart.c > > > > @@ -82,6 +82,7 @@ static struct uart_driver meson_uart_driver; > > > > static struct uart_port *meson_ports[AML_UART_PORT_NUM]; > > > > struct meson_uart_data { > > > > + const char *dev_name; > > > > bool has_xtal_div2; > > > > }; > > > > @@ -683,6 +684,7 @@ static int meson_uart_probe_clocks(struct platform_device *pdev, > > > > static int meson_uart_probe(struct platform_device *pdev) > > > > { > > > > + const struct meson_uart_data *priv_data; > > > > struct resource *res_mem; > > > > struct uart_port *port; > > > > u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */ > > > > @@ -729,6 +731,18 @@ static int meson_uart_probe(struct platform_device *pdev) > > > > if (ret) > > > > return ret; > > > > + priv_data = device_get_match_data(&pdev->dev); > > > > + > > > > + if (priv_data) { > > > > + struct console *cons = meson_uart_driver.cons; > > > > + > > > > + meson_uart_driver.dev_name = priv_data->dev_name; > > > > + > > > > + if (cons) > > > > + strscpy(cons->name, priv_data->dev_name, > > > > + sizeof(cons->name)); > > > > + } > > > > + > > > > if (!meson_uart_driver.state) { > > > > ret = uart_register_driver(&meson_uart_driver); > > > > if (ret) > > > > @@ -748,7 +762,7 @@ static int meson_uart_probe(struct platform_device *pdev) > > > > port->x_char = 0; > > > > port->ops = &meson_uart_ops; > > > > port->fifosize = fifosize; > > > > - port->private_data = (void *)device_get_match_data(&pdev->dev); > > > > + port->private_data = (void *)priv_data; > > > > meson_ports[pdev->id] = port; > > > > platform_set_drvdata(pdev, port); > > > > @@ -780,6 +794,17 @@ static int meson_uart_remove(struct platform_device *pdev) > > > > } > > > > static struct meson_uart_data meson_g12a_uart_data = { > > > > + .dev_name = "ttyAML", > > > > + .has_xtal_div2 = true, > > > > +}; > > > > + > > > > +static struct meson_uart_data meson_a1_uart_data = { > > > > + .dev_name = "ttyS", > > > > + .has_xtal_div2 = false, > > > > +}; > > > > + > > > > +static struct meson_uart_data meson_s4_uart_data = { > > > > + .dev_name = "ttyS", > > > > .has_xtal_div2 = true, > > > > }; > > > > @@ -794,7 +819,11 @@ static const struct of_device_id meson_uart_dt_match[] = { > > > > }, > > > > { > > > > .compatible = "amlogic,meson-s4-uart", > > > > - .data = (void *)&meson_g12a_uart_data, > > > > + .data = (void *)&meson_s4_uart_data, > > > > + }, > > > > + { > > > > + .compatible = "amlogic,meson-a1-uart", > > > > + .data = (void *)&meson_a1_uart_data, > > > > }, > > > > { /* sentinel */ }, > > > > }; > > > > > >
On Tue, Jul 04, 2023 at 04:59:34PM +0300, Dmitry Rokosov wrote: > It is worth noting that the devname ttyS is a widely recognized tty name > and is commonly used by many uart device drivers. Given the established > usage and compatibility concerns, it may not be feasible to change the > devname for older SoCs. However, for new definitions, it is acceptable > and even recommended to use a new devname to help ensure clarity and > avoid any potential conflicts on lower or upper software levels. > In > addition, modify the meson_uart_dt match data for g12a, a1, and s4 to > their appropriate values to ensure proper devname values and > functionality. IMO, this is a separate change that should be in another patch, had to go looking through a several of unrelated $subject patches to understand how the binding patch was going to work. Cheers, Conor. > For more information please refer to IRC discussion at [1]. > > Links: > [1]: https://libera.irclog.whitequark.org/linux-amlogic/2023-07-03 > > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > --- > drivers/tty/serial/meson_uart.c | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c > index 87c0eb5f2dba..361f9326b527 100644 > --- a/drivers/tty/serial/meson_uart.c > +++ b/drivers/tty/serial/meson_uart.c > @@ -82,6 +82,7 @@ static struct uart_driver meson_uart_driver; > static struct uart_port *meson_ports[AML_UART_PORT_NUM]; > > struct meson_uart_data { > + const char *dev_name; > bool has_xtal_div2; > }; > > @@ -683,6 +684,7 @@ static int meson_uart_probe_clocks(struct platform_device *pdev, > > static int meson_uart_probe(struct platform_device *pdev) > { > + const struct meson_uart_data *priv_data; > struct resource *res_mem; > struct uart_port *port; > u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */ > @@ -729,6 +731,18 @@ static int meson_uart_probe(struct platform_device *pdev) > if (ret) > return ret; > > + priv_data = device_get_match_data(&pdev->dev); > + > + if (priv_data) { > + struct console *cons = meson_uart_driver.cons; > + > + meson_uart_driver.dev_name = priv_data->dev_name; > + > + if (cons) > + strscpy(cons->name, priv_data->dev_name, > + sizeof(cons->name)); > + } > + > if (!meson_uart_driver.state) { > ret = uart_register_driver(&meson_uart_driver); > if (ret) > @@ -748,7 +762,7 @@ static int meson_uart_probe(struct platform_device *pdev) > port->x_char = 0; > port->ops = &meson_uart_ops; > port->fifosize = fifosize; > - port->private_data = (void *)device_get_match_data(&pdev->dev); > + port->private_data = (void *)priv_data; > > meson_ports[pdev->id] = port; > platform_set_drvdata(pdev, port); > @@ -780,6 +794,17 @@ static int meson_uart_remove(struct platform_device *pdev) > } > > static struct meson_uart_data meson_g12a_uart_data = { > + .dev_name = "ttyAML", > + .has_xtal_div2 = true, > +}; > + > +static struct meson_uart_data meson_a1_uart_data = { > + .dev_name = "ttyS", > + .has_xtal_div2 = false, > +}; > + > +static struct meson_uart_data meson_s4_uart_data = { > + .dev_name = "ttyS", > .has_xtal_div2 = true, > }; > > @@ -794,7 +819,11 @@ static const struct of_device_id meson_uart_dt_match[] = { > }, > { > .compatible = "amlogic,meson-s4-uart", > - .data = (void *)&meson_g12a_uart_data, > + .data = (void *)&meson_s4_uart_data, > + }, > + { > + .compatible = "amlogic,meson-a1-uart", > + .data = (void *)&meson_a1_uart_data, > }, > { /* sentinel */ }, > }; > -- > 2.36.0 >
On Tue, Jul 04, 2023 at 05:57:15PM +0100, Conor Dooley wrote: > On Tue, Jul 04, 2023 at 04:59:34PM +0300, Dmitry Rokosov wrote: > > It is worth noting that the devname ttyS is a widely recognized tty name > > and is commonly used by many uart device drivers. Given the established > > usage and compatibility concerns, it may not be feasible to change the > > devname for older SoCs. However, for new definitions, it is acceptable > > and even recommended to use a new devname to help ensure clarity and > > avoid any potential conflicts on lower or upper software levels. > > > In > > addition, modify the meson_uart_dt match data for g12a, a1, and s4 to > > their appropriate values to ensure proper devname values and > > functionality. > > IMO, this is a separate change that should be in another patch, had to > go looking through a several of unrelated $subject patches to understand > how the binding patch was going to work. I apologize, but I'm having difficulty understanding your suggestion. Are you recommending that a distinct binding patch for meson-uart-a1 be sent as part of a separate patch series? From my perspective, isolating the binding patch may not provide all the necessary context as it is reliant on a separate 'compatible' declaration within the meson-uart driver. However, this declaration is interconnected with the devname support patchset. Therefore, it seems that all of these patches are linked together. > > For more information please refer to IRC discussion at [1]. > > > > Links: > > [1]: https://libera.irclog.whitequark.org/linux-amlogic/2023-07-03 > > > > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > > --- > > drivers/tty/serial/meson_uart.c | 33 +++++++++++++++++++++++++++++++-- > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c > > index 87c0eb5f2dba..361f9326b527 100644 > > --- a/drivers/tty/serial/meson_uart.c > > +++ b/drivers/tty/serial/meson_uart.c > > @@ -82,6 +82,7 @@ static struct uart_driver meson_uart_driver; > > static struct uart_port *meson_ports[AML_UART_PORT_NUM]; > > > > struct meson_uart_data { > > + const char *dev_name; > > bool has_xtal_div2; > > }; > > > > @@ -683,6 +684,7 @@ static int meson_uart_probe_clocks(struct platform_device *pdev, > > > > static int meson_uart_probe(struct platform_device *pdev) > > { > > + const struct meson_uart_data *priv_data; > > struct resource *res_mem; > > struct uart_port *port; > > u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */ > > @@ -729,6 +731,18 @@ static int meson_uart_probe(struct platform_device *pdev) > > if (ret) > > return ret; > > > > + priv_data = device_get_match_data(&pdev->dev); > > + > > + if (priv_data) { > > + struct console *cons = meson_uart_driver.cons; > > + > > + meson_uart_driver.dev_name = priv_data->dev_name; > > + > > + if (cons) > > + strscpy(cons->name, priv_data->dev_name, > > + sizeof(cons->name)); > > + } > > + > > if (!meson_uart_driver.state) { > > ret = uart_register_driver(&meson_uart_driver); > > if (ret) > > @@ -748,7 +762,7 @@ static int meson_uart_probe(struct platform_device *pdev) > > port->x_char = 0; > > port->ops = &meson_uart_ops; > > port->fifosize = fifosize; > > - port->private_data = (void *)device_get_match_data(&pdev->dev); > > + port->private_data = (void *)priv_data; > > > > meson_ports[pdev->id] = port; > > platform_set_drvdata(pdev, port); > > @@ -780,6 +794,17 @@ static int meson_uart_remove(struct platform_device *pdev) > > } > > > > static struct meson_uart_data meson_g12a_uart_data = { > > + .dev_name = "ttyAML", > > + .has_xtal_div2 = true, > > +}; > > + > > +static struct meson_uart_data meson_a1_uart_data = { > > + .dev_name = "ttyS", > > + .has_xtal_div2 = false, > > +}; > > + > > +static struct meson_uart_data meson_s4_uart_data = { > > + .dev_name = "ttyS", > > .has_xtal_div2 = true, > > }; > > > > @@ -794,7 +819,11 @@ static const struct of_device_id meson_uart_dt_match[] = { > > }, > > { > > .compatible = "amlogic,meson-s4-uart", > > - .data = (void *)&meson_g12a_uart_data, > > + .data = (void *)&meson_s4_uart_data, > > + }, > > + { > > + .compatible = "amlogic,meson-a1-uart", > > + .data = (void *)&meson_a1_uart_data, > > }, > > { /* sentinel */ }, > > }; > > -- > > 2.36.0 > >
On Tue, Jul 04, 2023 at 08:13:26PM +0300, Dmitry Rokosov wrote: > On Tue, Jul 04, 2023 at 05:57:15PM +0100, Conor Dooley wrote: > > On Tue, Jul 04, 2023 at 04:59:34PM +0300, Dmitry Rokosov wrote: > > > It is worth noting that the devname ttyS is a widely recognized tty name > > > and is commonly used by many uart device drivers. Given the established > > > usage and compatibility concerns, it may not be feasible to change the > > > devname for older SoCs. However, for new definitions, it is acceptable > > > and even recommended to use a new devname to help ensure clarity and > > > avoid any potential conflicts on lower or upper software levels. > > > > > In > > > addition, modify the meson_uart_dt match data for g12a, a1, and s4 to > > > their appropriate values to ensure proper devname values and > > > functionality. > > > > IMO, this is a separate change that should be in another patch, had to > > go looking through a several of unrelated $subject patches to understand > > how the binding patch was going to work. > > I apologize, but I'm having difficulty understanding your suggestion. > Are you recommending that a distinct binding patch for meson-uart-a1 be > sent as part of a separate patch series? From my perspective, isolating > the binding patch may not provide all the necessary context as it is > reliant on a separate 'compatible' declaration within the meson-uart > driver. However, this declaration is interconnected with the devname > support patchset. Therefore, it seems that all of these patches are > linked together. Maybe it is just a case of how the commit message was written, where the SoCs responsible for the changes appear only "in addition". At the moment, it seemed like an unrelated addition that was sneaking into the commit to me, who was trying to find the code change that made the DT side of things valid, Re-phrasing the commit message to explain that the a1 is the reason for this change, rather than mentioning the SoCs as an apparent afterthought would make sense to me here. As would splitting reworking the code to support devname stuff in one commit & adding the new match for the a1 in another. Whatever works for you.
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c index 87c0eb5f2dba..361f9326b527 100644 --- a/drivers/tty/serial/meson_uart.c +++ b/drivers/tty/serial/meson_uart.c @@ -82,6 +82,7 @@ static struct uart_driver meson_uart_driver; static struct uart_port *meson_ports[AML_UART_PORT_NUM]; struct meson_uart_data { + const char *dev_name; bool has_xtal_div2; }; @@ -683,6 +684,7 @@ static int meson_uart_probe_clocks(struct platform_device *pdev, static int meson_uart_probe(struct platform_device *pdev) { + const struct meson_uart_data *priv_data; struct resource *res_mem; struct uart_port *port; u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */ @@ -729,6 +731,18 @@ static int meson_uart_probe(struct platform_device *pdev) if (ret) return ret; + priv_data = device_get_match_data(&pdev->dev); + + if (priv_data) { + struct console *cons = meson_uart_driver.cons; + + meson_uart_driver.dev_name = priv_data->dev_name; + + if (cons) + strscpy(cons->name, priv_data->dev_name, + sizeof(cons->name)); + } + if (!meson_uart_driver.state) { ret = uart_register_driver(&meson_uart_driver); if (ret) @@ -748,7 +762,7 @@ static int meson_uart_probe(struct platform_device *pdev) port->x_char = 0; port->ops = &meson_uart_ops; port->fifosize = fifosize; - port->private_data = (void *)device_get_match_data(&pdev->dev); + port->private_data = (void *)priv_data; meson_ports[pdev->id] = port; platform_set_drvdata(pdev, port); @@ -780,6 +794,17 @@ static int meson_uart_remove(struct platform_device *pdev) } static struct meson_uart_data meson_g12a_uart_data = { + .dev_name = "ttyAML", + .has_xtal_div2 = true, +}; + +static struct meson_uart_data meson_a1_uart_data = { + .dev_name = "ttyS", + .has_xtal_div2 = false, +}; + +static struct meson_uart_data meson_s4_uart_data = { + .dev_name = "ttyS", .has_xtal_div2 = true, }; @@ -794,7 +819,11 @@ static const struct of_device_id meson_uart_dt_match[] = { }, { .compatible = "amlogic,meson-s4-uart", - .data = (void *)&meson_g12a_uart_data, + .data = (void *)&meson_s4_uart_data, + }, + { + .compatible = "amlogic,meson-a1-uart", + .data = (void *)&meson_a1_uart_data, }, { /* sentinel */ }, };
It is worth noting that the devname ttyS is a widely recognized tty name and is commonly used by many uart device drivers. Given the established usage and compatibility concerns, it may not be feasible to change the devname for older SoCs. However, for new definitions, it is acceptable and even recommended to use a new devname to help ensure clarity and avoid any potential conflicts on lower or upper software levels. In addition, modify the meson_uart_dt match data for g12a, a1, and s4 to their appropriate values to ensure proper devname values and functionality. For more information please refer to IRC discussion at [1]. Links: [1]: https://libera.irclog.whitequark.org/linux-amlogic/2023-07-03 Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> --- drivers/tty/serial/meson_uart.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)