During a IRC discussion with Neil, as reported in reference [1], an idea
emerged to provide support for a standard devname 'ttyS' in new SoCs
such as A1, S4, T7, C3 and others. The current devname 'ttyAML' is not
widely known and has caused several issues with both low and high-level
software, without any apparent justification for its implementation.
Consequently, it has been deemed necessary to introduce the 'ttyS'
devname for all new 'compatible' entries, while still retaining backward
compatibility with the old 'ttyAML' devname by supporting it in parallel
with the new approach. This patch series therefore aims to implement
these changes.
Links:
[1]: https://libera.irclog.whitequark.org/linux-amlogic/2023-07-03
Dmitry Rokosov (5):
tty: serial: meson: use dev_err_probe
tty: serial: meson: redesign the module to platform_driver
tty: serial: meson: apply ttyS devname instead of ttyAML for new SoCs
dt-bindings: serial: amlogic,meson-uart: support Amlogic A1
arm64: dts: meson: a1: change uart compatible string
.../bindings/serial/amlogic,meson-uart.yaml | 2 +
arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 4 +-
drivers/tty/serial/meson_uart.c | 83 ++++++++++---------
3 files changed, 47 insertions(+), 42 deletions(-)
--
2.36.0
Introduce meson uart serial bindings for A1 SoC family.
Signed-off-by: Dmitry Rokosov <[email protected]>
---
.../devicetree/bindings/serial/amlogic,meson-uart.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
index 01ec45b3b406..f1ae8c4934d9 100644
--- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
+++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
@@ -33,6 +33,7 @@ properties:
- amlogic,meson8b-uart
- amlogic,meson-gx-uart
- amlogic,meson-s4-uart
+ - amlogic,meson-a1-uart
- const: amlogic,meson-ao-uart
- description: Always-on power domain UART controller on G12A SoCs
items:
@@ -46,6 +47,7 @@ properties:
- amlogic,meson8b-uart
- amlogic,meson-gx-uart
- amlogic,meson-s4-uart
+ - amlogic,meson-a1-uart
- description: Everything-Else power domain UART controller on G12A SoCs
items:
- const: amlogic,meson-g12a-uart
--
2.36.0
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 <[email protected]>
---
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
In the current implementation, the meson-a1 configuration incorporates a
unique compatibility tag "amlogic,meson-a1-uart' within the meson-uart
driver due to its usage of the new console device name "ttyS".
Consequently, the previous compatibility tag designated for the
'amlogic,meson-gx-uart' configuration has become obsolete and is no
longer relevant to the current setup.
Signed-off-by: Dmitry Rokosov <[email protected]>
---
arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
index c5567031ba12..6273b9c862b3 100644
--- a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
@@ -344,7 +344,7 @@ mux {
};
uart_AO: serial@1c00 {
- compatible = "amlogic,meson-gx-uart",
+ compatible = "amlogic,meson-a1-uart",
"amlogic,meson-ao-uart";
reg = <0x0 0x1c00 0x0 0x18>;
interrupts = <GIC_SPI 25 IRQ_TYPE_EDGE_RISING>;
@@ -354,7 +354,7 @@ uart_AO: serial@1c00 {
};
uart_AO_B: serial@2000 {
- compatible = "amlogic,meson-gx-uart",
+ compatible = "amlogic,meson-a1-uart",
"amlogic,meson-ao-uart";
reg = <0x0 0x2000 0x0 0x18>;
interrupts = <GIC_SPI 26 IRQ_TYPE_EDGE_RISING>;
--
2.36.0
Actually, the meson_uart module is already a platform_driver, but it is
currently registered manually and the uart core registration is run
outside the probe() scope, which results in some restrictions. For
instance, it is not possible to communicate with the OF subsystem
because it requires an initialized device object.
To address this issue, apply module_platform_driver() instead of direct
module init/exit routines. Additionally, move uart_register_driver() to
the driver probe(), and destroy manual console registration because it's
already run in the uart_register_driver() flow.
Signed-off-by: Dmitry Rokosov <[email protected]>
---
drivers/tty/serial/meson_uart.c | 46 +++++++--------------------------
1 file changed, 10 insertions(+), 36 deletions(-)
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 169f028956ae..87c0eb5f2dba 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -621,12 +621,6 @@ static struct console meson_serial_console = {
.data = &meson_uart_driver,
};
-static int __init meson_serial_console_init(void)
-{
- register_console(&meson_serial_console);
- return 0;
-}
-
static void meson_serial_early_console_write(struct console *co,
const char *s,
u_int count)
@@ -652,9 +646,6 @@ OF_EARLYCON_DECLARE(meson, "amlogic,meson-ao-uart",
#define MESON_SERIAL_CONSOLE (&meson_serial_console)
#else
-static int __init meson_serial_console_init(void) {
- return 0;
-}
#define MESON_SERIAL_CONSOLE NULL
#endif
@@ -738,6 +729,13 @@ static int meson_uart_probe(struct platform_device *pdev)
if (ret)
return ret;
+ if (!meson_uart_driver.state) {
+ ret = uart_register_driver(&meson_uart_driver);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "failed to register meson-uart driver\n");
+ }
+
port->iotype = UPIO_MEM;
port->mapbase = res_mem->start;
port->mapsize = resource_size(res_mem);
@@ -776,6 +774,8 @@ static int meson_uart_remove(struct platform_device *pdev)
uart_remove_one_port(&meson_uart_driver, port);
meson_ports[pdev->id] = NULL;
+ uart_unregister_driver(&meson_uart_driver);
+
return 0;
}
@@ -809,33 +809,7 @@ static struct platform_driver meson_uart_platform_driver = {
},
};
-static int __init meson_uart_init(void)
-{
- int ret;
-
- ret = meson_serial_console_init();
- if (ret)
- return ret;
-
- ret = uart_register_driver(&meson_uart_driver);
- if (ret)
- return ret;
-
- ret = platform_driver_register(&meson_uart_platform_driver);
- if (ret)
- uart_unregister_driver(&meson_uart_driver);
-
- return ret;
-}
-
-static void __exit meson_uart_exit(void)
-{
- platform_driver_unregister(&meson_uart_platform_driver);
- uart_unregister_driver(&meson_uart_driver);
-}
-
-module_init(meson_uart_init);
-module_exit(meson_uart_exit);
+module_platform_driver(meson_uart_platform_driver);
MODULE_AUTHOR("Carlo Caione <[email protected]>");
MODULE_DESCRIPTION("Amlogic Meson serial port driver");
--
2.36.0
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 <[email protected]>
> ---
> 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 04/07/2023 15:59, Dmitry Rokosov wrote:
> Actually, the meson_uart module is already a platform_driver, but it is
> currently registered manually and the uart core registration is run
> outside the probe() scope, which results in some restrictions. For
> instance, it is not possible to communicate with the OF subsystem
> because it requires an initialized device object.
>
> To address this issue, apply module_platform_driver() instead of direct
> module init/exit routines. Additionally, move uart_register_driver() to
> the driver probe(), and destroy manual console registration because it's
> already run in the uart_register_driver() flow.
>
> Signed-off-by: Dmitry Rokosov <[email protected]>
> ---
> drivers/tty/serial/meson_uart.c | 46 +++++++--------------------------
> 1 file changed, 10 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 169f028956ae..87c0eb5f2dba 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -621,12 +621,6 @@ static struct console meson_serial_console = {
> .data = &meson_uart_driver,
> };
>
> -static int __init meson_serial_console_init(void)
> -{
> - register_console(&meson_serial_console);
> - return 0;
> -}
> -
> static void meson_serial_early_console_write(struct console *co,
> const char *s,
> u_int count)
> @@ -652,9 +646,6 @@ OF_EARLYCON_DECLARE(meson, "amlogic,meson-ao-uart",
>
> #define MESON_SERIAL_CONSOLE (&meson_serial_console)
> #else
> -static int __init meson_serial_console_init(void) {
> - return 0;
> -}
> #define MESON_SERIAL_CONSOLE NULL
> #endif
>
> @@ -738,6 +729,13 @@ static int meson_uart_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + if (!meson_uart_driver.state) {
> + ret = uart_register_driver(&meson_uart_driver);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to register meson-uart driver\n");
> + }
PL010 protects this in a mutex, and I think you should do the same otherwise
if multiple uart probes at the same it will do weird stuff.
> +
> port->iotype = UPIO_MEM;
> port->mapbase = res_mem->start;
> port->mapsize = resource_size(res_mem);
> @@ -776,6 +774,8 @@ static int meson_uart_remove(struct platform_device *pdev)
> uart_remove_one_port(&meson_uart_driver, port);
> meson_ports[pdev->id] = NULL;
>
> + uart_unregister_driver(&meson_uart_driver);
> +
This is dangerous, it will remove the driver even if some uart are still attached to it.
You should probably do like in pl010_remove() and remove only if the last one is removed.
> return 0;
> }
>
> @@ -809,33 +809,7 @@ static struct platform_driver meson_uart_platform_driver = {
> },
> };
>
> -static int __init meson_uart_init(void)
> -{
> - int ret;
> -
> - ret = meson_serial_console_init();
> - if (ret)
> - return ret;
> -
> - ret = uart_register_driver(&meson_uart_driver);
> - if (ret)
> - return ret;
> -
> - ret = platform_driver_register(&meson_uart_platform_driver);
> - if (ret)
> - uart_unregister_driver(&meson_uart_driver);
> -
> - return ret;
> -}
> -
> -static void __exit meson_uart_exit(void)
> -{
> - platform_driver_unregister(&meson_uart_platform_driver);
> - uart_unregister_driver(&meson_uart_driver);
> -}
> -
> -module_init(meson_uart_init);
> -module_exit(meson_uart_exit);
> +module_platform_driver(meson_uart_platform_driver);
Only pl010 uses this scheme, and I don't know why... if it works then it's ok for me.
Thanks,
Neil
>
> MODULE_AUTHOR("Carlo Caione <[email protected]>");
> MODULE_DESCRIPTION("Amlogic Meson serial port driver");
On Tue, Jul 04, 2023 at 04:46:40PM +0200, [email protected] wrote:
> On 04/07/2023 15:59, Dmitry Rokosov wrote:
> > Actually, the meson_uart module is already a platform_driver, but it is
> > currently registered manually and the uart core registration is run
> > outside the probe() scope, which results in some restrictions. For
> > instance, it is not possible to communicate with the OF subsystem
> > because it requires an initialized device object.
> >
> > To address this issue, apply module_platform_driver() instead of direct
> > module init/exit routines. Additionally, move uart_register_driver() to
> > the driver probe(), and destroy manual console registration because it's
> > already run in the uart_register_driver() flow.
> >
> > Signed-off-by: Dmitry Rokosov <[email protected]>
> > ---
> > drivers/tty/serial/meson_uart.c | 46 +++++++--------------------------
> > 1 file changed, 10 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> > index 169f028956ae..87c0eb5f2dba 100644
> > --- a/drivers/tty/serial/meson_uart.c
> > +++ b/drivers/tty/serial/meson_uart.c
> > @@ -621,12 +621,6 @@ static struct console meson_serial_console = {
> > .data = &meson_uart_driver,
> > };
> > -static int __init meson_serial_console_init(void)
> > -{
> > - register_console(&meson_serial_console);
> > - return 0;
> > -}
> > -
> > static void meson_serial_early_console_write(struct console *co,
> > const char *s,
> > u_int count)
> > @@ -652,9 +646,6 @@ OF_EARLYCON_DECLARE(meson, "amlogic,meson-ao-uart",
> > #define MESON_SERIAL_CONSOLE (&meson_serial_console)
> > #else
> > -static int __init meson_serial_console_init(void) {
> > - return 0;
> > -}
> > #define MESON_SERIAL_CONSOLE NULL
> > #endif
> > @@ -738,6 +729,13 @@ static int meson_uart_probe(struct platform_device *pdev)
> > if (ret)
> > return ret;
> > + if (!meson_uart_driver.state) {
> > + ret = uart_register_driver(&meson_uart_driver);
> > + if (ret)
> > + return dev_err_probe(&pdev->dev, ret,
> > + "failed to register meson-uart driver\n");
> > + }
>
> PL010 protects this in a mutex, and I think you should do the same otherwise
> if multiple uart probes at the same it will do weird stuff.
>
Looks like that not all drivers protect this location with a specialized
mutex object. Firstly, I think it's important to verify parallel probe()
calling and implementing mutex protection at the platform core level.
For example, I've faced with the same problem during regmap mutex based
protection.
> > +
> > port->iotype = UPIO_MEM;
> > port->mapbase = res_mem->start;
> > port->mapsize = resource_size(res_mem);
> > @@ -776,6 +774,8 @@ static int meson_uart_remove(struct platform_device *pdev)
> > uart_remove_one_port(&meson_uart_driver, port);
> > meson_ports[pdev->id] = NULL;
> > + uart_unregister_driver(&meson_uart_driver);
> > +
>
> This is dangerous, it will remove the driver even if some uart are still attached to it.
>
> You should probably do like in pl010_remove() and remove only if the last one is removed.
>
Indeed... multiple ports can be registered...
> > return 0;
> > }
> > @@ -809,33 +809,7 @@ static struct platform_driver meson_uart_platform_driver = {
> > },
> > };
> > -static int __init meson_uart_init(void)
> > -{
> > - int ret;
> > -
> > - ret = meson_serial_console_init();
> > - if (ret)
> > - return ret;
> > -
> > - ret = uart_register_driver(&meson_uart_driver);
> > - if (ret)
> > - return ret;
> > -
> > - ret = platform_driver_register(&meson_uart_platform_driver);
> > - if (ret)
> > - uart_unregister_driver(&meson_uart_driver);
> > -
> > - return ret;
> > -}
> > -
> > -static void __exit meson_uart_exit(void)
> > -{
> > - platform_driver_unregister(&meson_uart_platform_driver);
> > - uart_unregister_driver(&meson_uart_driver);
> > -}
> > -
> > -module_init(meson_uart_init);
> > -module_exit(meson_uart_exit);
> > +module_platform_driver(meson_uart_platform_driver);
>
> Only pl010 uses this scheme, and I don't know why... if it works then it's ok for me.
From my point of view, the "scheme" is using uart driver registration
from the probe() routine. Many drivers are based on such approach:
samsung-tty, timbuart, sprd, max3100, etc. Some of them are platform
drivers as well.
> > MODULE_AUTHOR("Carlo Caione <[email protected]>");
> > MODULE_DESCRIPTION("Amlogic Meson serial port driver");
>
--
Thank you,
Dmitry
Hello Neil,
Thank you for quick feedback!
On Tue, Jul 04, 2023 at 04:42:39PM +0200, [email protected] 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 <[email protected]>
> > ---
> > 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 */ },
> > };
>
--
Thank you,
Dmitry
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, [email protected] 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 <[email protected]>
>>> ---
>>> 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, [email protected] 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, [email protected] 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 <[email protected]>
> > > > ---
> > > > 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 */ },
> > > > };
> > >
> >
>
--
Thank you,
Dmitry
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 <[email protected]>
> ---
> 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 04:59:36PM +0300, Dmitry Rokosov wrote:
> In the current implementation, the meson-a1 configuration incorporates a
> unique compatibility tag "amlogic,meson-a1-uart' within the meson-uart
> driver due to its usage of the new console device name "ttyS".
> Consequently, the previous compatibility tag designated for the
> 'amlogic,meson-gx-uart' configuration has become obsolete and is no
> longer relevant to the current setup.
I don't really see why you would remove the gx-uart to be honest, and
not use it as a fallback. Neil's platform though, so his call :)
>
> Signed-off-by: Dmitry Rokosov <[email protected]>
> ---
> arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
> index c5567031ba12..6273b9c862b3 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
> @@ -344,7 +344,7 @@ mux {
> };
>
> uart_AO: serial@1c00 {
> - compatible = "amlogic,meson-gx-uart",
> + compatible = "amlogic,meson-a1-uart",
> "amlogic,meson-ao-uart";
> reg = <0x0 0x1c00 0x0 0x18>;
> interrupts = <GIC_SPI 25 IRQ_TYPE_EDGE_RISING>;
> @@ -354,7 +354,7 @@ uart_AO: serial@1c00 {
> };
>
> uart_AO_B: serial@2000 {
> - compatible = "amlogic,meson-gx-uart",
> + compatible = "amlogic,meson-a1-uart",
> "amlogic,meson-ao-uart";
> reg = <0x0 0x2000 0x0 0x18>;
> interrupts = <GIC_SPI 26 IRQ_TYPE_EDGE_RISING>;
> --
> 2.36.0
>
On Tue, Jul 04, 2023 at 06:19:20PM +0300, Dmitry Rokosov wrote:
> On Tue, Jul 04, 2023 at 04:46:40PM +0200, [email protected] wrote:
> > On 04/07/2023 15:59, Dmitry Rokosov wrote:
> > > Actually, the meson_uart module is already a platform_driver, but it is
> > > currently registered manually and the uart core registration is run
> > > outside the probe() scope, which results in some restrictions. For
> > > instance, it is not possible to communicate with the OF subsystem
> > > because it requires an initialized device object.
> > >
> > > To address this issue, apply module_platform_driver() instead of direct
> > > module init/exit routines. Additionally, move uart_register_driver() to
> > > the driver probe(), and destroy manual console registration because it's
> > > already run in the uart_register_driver() flow.
> > >
> > > Signed-off-by: Dmitry Rokosov <[email protected]>
> > > ---
> > > drivers/tty/serial/meson_uart.c | 46 +++++++--------------------------
> > > 1 file changed, 10 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> > > index 169f028956ae..87c0eb5f2dba 100644
> > > --- a/drivers/tty/serial/meson_uart.c
> > > +++ b/drivers/tty/serial/meson_uart.c
> > > @@ -621,12 +621,6 @@ static struct console meson_serial_console = {
> > > .data = &meson_uart_driver,
> > > };
> > > -static int __init meson_serial_console_init(void)
> > > -{
> > > - register_console(&meson_serial_console);
> > > - return 0;
> > > -}
> > > -
> > > static void meson_serial_early_console_write(struct console *co,
> > > const char *s,
> > > u_int count)
> > > @@ -652,9 +646,6 @@ OF_EARLYCON_DECLARE(meson, "amlogic,meson-ao-uart",
> > > #define MESON_SERIAL_CONSOLE (&meson_serial_console)
> > > #else
> > > -static int __init meson_serial_console_init(void) {
> > > - return 0;
> > > -}
> > > #define MESON_SERIAL_CONSOLE NULL
> > > #endif
> > > @@ -738,6 +729,13 @@ static int meson_uart_probe(struct platform_device *pdev)
> > > if (ret)
> > > return ret;
> > > + if (!meson_uart_driver.state) {
> > > + ret = uart_register_driver(&meson_uart_driver);
> > > + if (ret)
> > > + return dev_err_probe(&pdev->dev, ret,
> > > + "failed to register meson-uart driver\n");
> > > + }
> >
> > PL010 protects this in a mutex, and I think you should do the same otherwise
> > if multiple uart probes at the same it will do weird stuff.
> >
>
> Looks like that not all drivers protect this location with a specialized
> mutex object. Firstly, I think it's important to verify parallel probe()
> calling and implementing mutex protection at the platform core level.
> For example, I've faced with the same problem during regmap mutex based
> protection.
>
Upon examining the core logic in drivers/base/dd.c, I have observed that
driver_probe_device() is consistently executed under the device_lock().
This lock is already based on a mutex, thus ensuring parallel execution
protection:
https://elixir.bootlin.com/linux/latest/source/include/linux/device.h#L835
> > > +
> > > port->iotype = UPIO_MEM;
> > > port->mapbase = res_mem->start;
> > > port->mapsize = resource_size(res_mem);
> > > @@ -776,6 +774,8 @@ static int meson_uart_remove(struct platform_device *pdev)
> > > uart_remove_one_port(&meson_uart_driver, port);
> > > meson_ports[pdev->id] = NULL;
> > > + uart_unregister_driver(&meson_uart_driver);
> > > +
> >
> > This is dangerous, it will remove the driver even if some uart are still attached to it.
> >
> > You should probably do like in pl010_remove() and remove only if the last one is removed.
> >
>
> Indeed... multiple ports can be registered...
>
> > > return 0;
> > > }
> > > @@ -809,33 +809,7 @@ static struct platform_driver meson_uart_platform_driver = {
> > > },
> > > };
> > > -static int __init meson_uart_init(void)
> > > -{
> > > - int ret;
> > > -
> > > - ret = meson_serial_console_init();
> > > - if (ret)
> > > - return ret;
> > > -
> > > - ret = uart_register_driver(&meson_uart_driver);
> > > - if (ret)
> > > - return ret;
> > > -
> > > - ret = platform_driver_register(&meson_uart_platform_driver);
> > > - if (ret)
> > > - uart_unregister_driver(&meson_uart_driver);
> > > -
> > > - return ret;
> > > -}
> > > -
> > > -static void __exit meson_uart_exit(void)
> > > -{
> > > - platform_driver_unregister(&meson_uart_platform_driver);
> > > - uart_unregister_driver(&meson_uart_driver);
> > > -}
> > > -
> > > -module_init(meson_uart_init);
> > > -module_exit(meson_uart_exit);
> > > +module_platform_driver(meson_uart_platform_driver);
> >
> > Only pl010 uses this scheme, and I don't know why... if it works then it's ok for me.
>
> From my point of view, the "scheme" is using uart driver registration
> from the probe() routine. Many drivers are based on such approach:
> samsung-tty, timbuart, sprd, max3100, etc. Some of them are platform
> drivers as well.
>
> > > MODULE_AUTHOR("Carlo Caione <[email protected]>");
> > > MODULE_DESCRIPTION("Amlogic Meson serial port driver");
> >
>
> --
> Thank you,
> Dmitry
--
Thank you,
Dmitry
On Tue, Jul 04, 2023 at 08:08:43PM +0300, Dmitry Rokosov wrote:
> On Tue, Jul 04, 2023 at 06:02:58PM +0100, Conor Dooley wrote:
> > On Tue, Jul 04, 2023 at 04:59:36PM +0300, Dmitry Rokosov wrote:
> > > In the current implementation, the meson-a1 configuration incorporates a
> > > unique compatibility tag "amlogic,meson-a1-uart' within the meson-uart
> > > driver due to its usage of the new console device name "ttyS".
> > > Consequently, the previous compatibility tag designated for the
> > > 'amlogic,meson-gx-uart' configuration has become obsolete and is no
> > > longer relevant to the current setup.
> >
> > I don't really see why you would remove the gx-uart to be honest, and
> > not use it as a fallback. Neil's platform though, so his call :)
> >
>
> Because of amlogic,meson-gx-uart has legacy devname, we do not want to
> use it in the A1.
Which I did read in your commit message, fallback being the operative
word here.
On Tue, Jul 04, 2023 at 06:10:46PM +0100, Conor Dooley wrote:
> On Tue, Jul 04, 2023 at 08:08:43PM +0300, Dmitry Rokosov wrote:
> > On Tue, Jul 04, 2023 at 06:02:58PM +0100, Conor Dooley wrote:
> > > On Tue, Jul 04, 2023 at 04:59:36PM +0300, Dmitry Rokosov wrote:
> > > > In the current implementation, the meson-a1 configuration incorporates a
> > > > unique compatibility tag "amlogic,meson-a1-uart' within the meson-uart
> > > > driver due to its usage of the new console device name "ttyS".
> > > > Consequently, the previous compatibility tag designated for the
> > > > 'amlogic,meson-gx-uart' configuration has become obsolete and is no
> > > > longer relevant to the current setup.
> > >
> > > I don't really see why you would remove the gx-uart to be honest, and
> > > not use it as a fallback. Neil's platform though, so his call :)
> > >
> >
> > Because of amlogic,meson-gx-uart has legacy devname, we do not want to
> > use it in the A1.
>
> Which I did read in your commit message, fallback being the operative
> word here.
Although it is difficult for me to envision a situation where we would
require this fallback, but gx-uart fallback will function from a kernel
perspective (without taking into account bootloader setup or userspace
daemon script). I don't have any objections to stay gx-uart as a
fallback, will do it in the v2.
--
Thank you,
Dmitry
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 <[email protected]>
> > ---
> > 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
> >
--
Thank you,
Dmitry
On Tue, Jul 04, 2023 at 06:02:58PM +0100, Conor Dooley wrote:
> On Tue, Jul 04, 2023 at 04:59:36PM +0300, Dmitry Rokosov wrote:
> > In the current implementation, the meson-a1 configuration incorporates a
> > unique compatibility tag "amlogic,meson-a1-uart' within the meson-uart
> > driver due to its usage of the new console device name "ttyS".
> > Consequently, the previous compatibility tag designated for the
> > 'amlogic,meson-gx-uart' configuration has become obsolete and is no
> > longer relevant to the current setup.
>
> I don't really see why you would remove the gx-uart to be honest, and
> not use it as a fallback. Neil's platform though, so his call :)
>
Because of amlogic,meson-gx-uart has legacy devname, we do not want to
use it in the A1.
> >
> > Signed-off-by: Dmitry Rokosov <[email protected]>
> > ---
> > arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
> > index c5567031ba12..6273b9c862b3 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
> > +++ b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
> > @@ -344,7 +344,7 @@ mux {
> > };
> >
> > uart_AO: serial@1c00 {
> > - compatible = "amlogic,meson-gx-uart",
> > + compatible = "amlogic,meson-a1-uart",
> > "amlogic,meson-ao-uart";
> > reg = <0x0 0x1c00 0x0 0x18>;
> > interrupts = <GIC_SPI 25 IRQ_TYPE_EDGE_RISING>;
> > @@ -354,7 +354,7 @@ uart_AO: serial@1c00 {
> > };
> >
> > uart_AO_B: serial@2000 {
> > - compatible = "amlogic,meson-gx-uart",
> > + compatible = "amlogic,meson-a1-uart",
> > "amlogic,meson-ao-uart";
> > reg = <0x0 0x2000 0x0 0x18>;
> > interrupts = <GIC_SPI 26 IRQ_TYPE_EDGE_RISING>;
> > --
> > 2.36.0
> >
--
Thank you,
Dmitry
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.
Conor,
On Tue, Jul 04, 2023 at 08:19:00PM +0300, Dmitry Rokosov wrote:
> On Tue, Jul 04, 2023 at 06:10:46PM +0100, Conor Dooley wrote:
> > On Tue, Jul 04, 2023 at 08:08:43PM +0300, Dmitry Rokosov wrote:
> > > On Tue, Jul 04, 2023 at 06:02:58PM +0100, Conor Dooley wrote:
> > > > On Tue, Jul 04, 2023 at 04:59:36PM +0300, Dmitry Rokosov wrote:
> > > > > In the current implementation, the meson-a1 configuration incorporates a
> > > > > unique compatibility tag "amlogic,meson-a1-uart' within the meson-uart
> > > > > driver due to its usage of the new console device name "ttyS".
> > > > > Consequently, the previous compatibility tag designated for the
> > > > > 'amlogic,meson-gx-uart' configuration has become obsolete and is no
> > > > > longer relevant to the current setup.
> > > >
> > > > I don't really see why you would remove the gx-uart to be honest, and
> > > > not use it as a fallback. Neil's platform though, so his call :)
> > > >
> > >
> > > Because of amlogic,meson-gx-uart has legacy devname, we do not want to
> > > use it in the A1.
> >
> > Which I did read in your commit message, fallback being the operative
> > word here.
>
> Although it is difficult for me to envision a situation where we would
> require this fallback, but gx-uart fallback will function from a kernel
> perspective (without taking into account bootloader setup or userspace
> daemon script). I don't have any objections to stay gx-uart as a
> fallback, will do it in the v2.
Unfortunately, it's not possible based on schema rules. During dtbs check
I've got the such error:
arch/arm64/boot/dts/amlogic/meson-a1-ad401.dtb: serial@1c00: compatible: 'oneOf' conditional failed, one must be fixed:
['amlogic,meson-a1-uart', 'amlogic,meson-ao-uart', 'amlogic,meson-gx-uart'] is too long
'amlogic,meson-g12a-uart' was expected
'amlogic,meson-gx-uart' was expected
'amlogic,meson-ao-uart' was expected
From schema: Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
Of course, we can change dt bindings schema, but is it really needed? As
I said before, it's difficult to imagine the situation, when gx-uart
fallback will be helpful.
--
Thank you,
Dmitry
On Tue, 04 Jul 2023 16:59:35 +0300, Dmitry Rokosov wrote:
> Introduce meson uart serial bindings for A1 SoC family.
>
> Signed-off-by: Dmitry Rokosov <[email protected]>
> ---
> .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
Acked-by: Rob Herring <[email protected]>