[PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[Yu Tu]

Using the common Clock code to describe the UART baud rate
clock makes it easier for the UART driver to be compatible
with the baud rate requirements of the UART IP on different
meson chips. Add Meson S4 SoC compatible.

The test method:
Start the console and run the following commands in turn:
stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.

Since most SoCs are too old, I was able to find all the platforms myself
such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it with
G12A and S4.

Yu Tu (6):
  tty: serial: meson: Move request the register region to probe
  tty: serial: meson: Use devm_ioremap_resource to get register mapped
    memory
  tty: serial: meson: Describes the calculation of the UART baud rate
    clock using a clock frame
  tty: serial: meson: Make some bit of the REG5 register writable
  tty: serial: meson: The system stuck when you run the stty command on
    the console to change the baud rate
  tty: serial: meson: Added S4 SOC compatibility

V6 -> V7: To solve the system stuck when you run the stty command on
the console to change the baud rate.
V5 -> V6: Change error format as discussed in the email.
V4 -> V5: Change error format.
V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
in the email.
V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change
the DTS before it can be deleted
V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as
discussed in the email

Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/

 drivers/tty/serial/meson_uart.c | 221 ++++++++++++++++++++++----------
 1 file changed, 154 insertions(+), 67 deletions(-)


base-commit: a603ca60cebff8589882427a67f870ed946b3fc8
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V7 1/6] tty: serial: meson: Move request the register region to probe

[Yu Tu]

This simplifies resetting the UART controller during probe
and will make it easier to integrate the common clock code
which will require the registers at probe time as well.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
---
 drivers/tty/serial/meson_uart.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 45e00d928253..6b80e41b4cc1 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -395,24 +395,11 @@ static int meson_uart_verify_port(struct uart_port *port,
 
 static void meson_uart_release_port(struct uart_port *port)
 {
-	devm_iounmap(port->dev, port->membase);
-	port->membase = NULL;
-	devm_release_mem_region(port->dev, port->mapbase, port->mapsize);
+	/* nothing to do */
 }
 
 static int meson_uart_request_port(struct uart_port *port)
 {
-	if (!devm_request_mem_region(port->dev, port->mapbase, port->mapsize,
-				     dev_name(port->dev))) {
-		dev_err(port->dev, "Memory region busy\n");
-		return -EBUSY;
-	}
-
-	port->membase = devm_ioremap(port->dev, port->mapbase,
-					     port->mapsize);
-	if (!port->membase)
-		return -ENOMEM;
-
 	return 0;
 }
 
@@ -733,6 +720,18 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
+	if (!devm_request_mem_region(&pdev->dev, res_mem->start,
+				     resource_size(res_mem),
+				     dev_name(&pdev->dev))) {
+		dev_err(&pdev->dev, "Memory region busy\n");
+		return -EBUSY;
+	}
+
+	port->membase = devm_ioremap(&pdev->dev, res_mem->start,
+				     resource_size(res_mem));
+	if (IS_ERR(port->membase))
+		return PTR_ERR(port->membase);
+
 	ret = meson_uart_probe_clocks(pdev, port);
 	if (ret)
 		return ret;
@@ -754,10 +753,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, port);
 
 	/* reset port before registering (and possibly registering console) */
-	if (meson_uart_request_port(port) >= 0) {
-		meson_uart_reset(port);
-		meson_uart_release_port(port);
-	}
+	meson_uart_reset(port);
 
 	ret = uart_add_one_port(&meson_uart_driver, port);
 	if (ret)
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V7 2/6] tty: serial: meson: Use devm_ioremap_resource to get register mapped memory

[Yu Tu]

Replace devm_request_mem_region and devm_ioremap with
devm_ioremap_resource to make the code cleaner.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
---
 drivers/tty/serial/meson_uart.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 6b80e41b4cc1..7570958d010c 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -720,15 +720,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
-	if (!devm_request_mem_region(&pdev->dev, res_mem->start,
-				     resource_size(res_mem),
-				     dev_name(&pdev->dev))) {
-		dev_err(&pdev->dev, "Memory region busy\n");
-		return -EBUSY;
-	}
-
-	port->membase = devm_ioremap(&pdev->dev, res_mem->start,
-				     resource_size(res_mem));
+	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
 	if (IS_ERR(port->membase))
 		return PTR_ERR(port->membase);
 
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V7 3/6] tty: serial: meson: Describes the calculation of the UART baud rate clock using a clock frame

[Yu Tu]

Using the common Clock code to describe the UART baud rate clock
makes it easier for the UART driver to be compatible with the
baud rate requirements of the UART IP on different meson chips.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 194 +++++++++++++++++++++++---------
 1 file changed, 142 insertions(+), 52 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 7570958d010c..4768d51fac70 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/console.h>
 #include <linux/delay.h>
 #include <linux/init.h>
@@ -65,9 +66,7 @@
 #define AML_UART_RECV_IRQ(c)		((c) & 0xff)
 
 /* AML_UART_REG5 bits */
-#define AML_UART_BAUD_MASK		0x7fffff
 #define AML_UART_BAUD_USE		BIT(23)
-#define AML_UART_BAUD_XTAL		BIT(24)
 
 #define AML_UART_PORT_NUM		12
 #define AML_UART_PORT_OFFSET		6
@@ -76,6 +75,11 @@
 #define AML_UART_POLL_USEC		5
 #define AML_UART_TIMEOUT_USEC		10000
 
+struct meson_uart_data {
+	struct clk	*baud_clk;
+	bool		use_xtal_clk;
+};
+
 static struct uart_driver meson_uart_driver;
 
 static struct uart_port *meson_ports[AML_UART_PORT_NUM];
@@ -293,19 +297,17 @@ static int meson_uart_startup(struct uart_port *port)
 
 static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
 {
+	struct meson_uart_data *private_data = port->private_data;
 	u32 val;
 
 	while (!meson_uart_tx_empty(port))
 		cpu_relax();
 
-	if (port->uartclk == 24000000) {
-		val = ((port->uartclk / 3) / baud) - 1;
-		val |= AML_UART_BAUD_XTAL;
-	} else {
-		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
-	}
+	val = readl(port->membase + AML_UART_REG5);
 	val |= AML_UART_BAUD_USE;
 	writel(val, port->membase + AML_UART_REG5);
+
+	clk_set_rate(private_data->baud_clk, baud);
 }
 
 static void meson_uart_set_termios(struct uart_port *port,
@@ -395,11 +397,20 @@ static int meson_uart_verify_port(struct uart_port *port,
 
 static void meson_uart_release_port(struct uart_port *port)
 {
-	/* nothing to do */
+	struct meson_uart_data *private_data = port->private_data;
+
+	clk_disable_unprepare(private_data->baud_clk);
 }
 
 static int meson_uart_request_port(struct uart_port *port)
 {
+	struct meson_uart_data *private_data = port->private_data;
+	int ret;
+
+	ret = clk_prepare_enable(private_data->baud_clk);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
@@ -629,57 +640,106 @@ static struct uart_driver meson_uart_driver = {
 	.cons		= MESON_SERIAL_CONSOLE,
 };
 
-static inline struct clk *meson_uart_probe_clock(struct device *dev,
-						 const char *id)
-{
-	struct clk *clk = NULL;
-	int ret;
-
-	clk = devm_clk_get(dev, id);
-	if (IS_ERR(clk))
-		return clk;
-
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		dev_err(dev, "couldn't enable clk\n");
-		return ERR_PTR(ret);
-	}
+static const struct clk_div_table xtal_div_table[] = {
+	{ 0, 3 },
+	{ 1, 1 },
+	{ 2, 2 },
+	{ 3, 2 },
+};
 
-	devm_add_action_or_reset(dev,
-			(void(*)(void *))clk_disable_unprepare,
-			clk);
+static u32 use_xtal_mux_table;
 
-	return clk;
-}
-
-static int meson_uart_probe_clocks(struct platform_device *pdev,
-				   struct uart_port *port)
+static int meson_uart_probe_clocks(struct uart_port *port)
 {
-	struct clk *clk_xtal = NULL;
-	struct clk *clk_pclk = NULL;
-	struct clk *clk_baud = NULL;
+	struct meson_uart_data *private_data = port->private_data;
+	struct clk *clk_baud, *clk_xtal;
+	struct clk_hw *hw, *clk81_div4_hw;
+	char clk_name[32];
+	struct clk_parent_data use_xtal_mux_parents;
 
-	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
-	if (IS_ERR(clk_pclk))
-		return PTR_ERR(clk_pclk);
+	clk_baud = devm_clk_get(port->dev, "baud");
+	if (IS_ERR(clk_baud)) {
+		dev_err(port->dev, "Failed to get the 'baud' clock\n");
+		return PTR_ERR(clk_baud);
+	}
 
-	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
+	clk_xtal = devm_clk_get(port->dev, "xtal");
 	if (IS_ERR(clk_xtal))
-		return PTR_ERR(clk_xtal);
-
-	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
-	if (IS_ERR(clk_baud))
-		return PTR_ERR(clk_baud);
+		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
+				     "Failed to get the 'xtal' clock\n");
+
+	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
+		 "clk81_div4");
+	clk81_div4_hw = devm_clk_hw_register_fixed_factor(port->dev,
+							  clk_name,
+							  __clk_get_name(clk_baud),
+							  CLK_SET_RATE_NO_REPARENT,
+							  1, 4);
+	if (IS_ERR(clk81_div4_hw))
+		return PTR_ERR(clk81_div4_hw);
+
+	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
+		 "xtal_div");
+	hw = devm_clk_hw_register_divider_table(port->dev,
+						clk_name,
+						__clk_get_name(clk_baud),
+						CLK_SET_RATE_NO_REPARENT,
+						port->membase + AML_UART_REG5,
+						26, 2,
+						CLK_DIVIDER_READ_ONLY,
+						xtal_div_table, NULL);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+
+	if (private_data->use_xtal_clk) {
+		use_xtal_mux_table = 1;
+		use_xtal_mux_parents.hw = hw;
+	} else {
+		use_xtal_mux_parents.hw = clk81_div4_hw;
+	}
 
-	port->uartclk = clk_get_rate(clk_baud);
+	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
+		 "use_xtal");
+	hw = __devm_clk_hw_register_mux(port->dev, NULL,
+					clk_name,
+					1,
+					NULL, NULL,
+					&use_xtal_mux_parents,
+					CLK_SET_RATE_PARENT,
+					port->membase + AML_UART_REG5,
+					24, 0x1,
+					CLK_MUX_READ_ONLY,
+					&use_xtal_mux_table, NULL);
+
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+
+	port->uartclk = clk_hw_get_rate(hw);
+
+	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
+		 "baud_div");
+	hw = devm_clk_hw_register_divider(port->dev,
+					  clk_name,
+					  clk_hw_get_name(hw),
+					  CLK_SET_RATE_PARENT,
+					  port->membase + AML_UART_REG5,
+					  0, 23,
+					  CLK_DIVIDER_ROUND_CLOSEST,
+					  NULL);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+
+	private_data->baud_clk = hw->clk;
 
 	return 0;
 }
 
 static int meson_uart_probe(struct platform_device *pdev)
 {
+	struct meson_uart_data *private_data;
 	struct resource *res_mem;
 	struct uart_port *port;
+	struct clk *pclk;
 	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
 	int ret = 0;
 	int irq;
@@ -705,6 +765,15 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!res_mem)
 		return -ENODEV;
 
+	pclk = devm_clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(pclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(pclk),
+				     "Failed to get the 'pclk' clock\n");
+
+	ret = clk_prepare_enable(pclk);
+	if (ret)
+		return ret;
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;
@@ -724,9 +793,13 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (IS_ERR(port->membase))
 		return PTR_ERR(port->membase);
 
-	ret = meson_uart_probe_clocks(pdev, port);
-	if (ret)
-		return ret;
+	private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
+				    GFP_KERNEL);
+	if (!private_data)
+		return -ENOMEM;
+
+	if (device_get_match_data(&pdev->dev))
+		private_data->use_xtal_clk = true;
 
 	port->iotype = UPIO_MEM;
 	port->mapbase = res_mem->start;
@@ -740,6 +813,11 @@ static int meson_uart_probe(struct platform_device *pdev)
 	port->x_char = 0;
 	port->ops = &meson_uart_ops;
 	port->fifosize = fifosize;
+	port->private_data = private_data;
+
+	ret = meson_uart_probe_clocks(port);
+	if (ret)
+		return ret;
 
 	meson_ports[pdev->id] = port;
 	platform_set_drvdata(pdev, port);
@@ -766,10 +844,22 @@ static int meson_uart_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id meson_uart_dt_match[] = {
-	{ .compatible = "amlogic,meson6-uart" },
-	{ .compatible = "amlogic,meson8-uart" },
-	{ .compatible = "amlogic,meson8b-uart" },
-	{ .compatible = "amlogic,meson-gx-uart" },
+	{
+		.compatible = "amlogic,meson6-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson8-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson8b-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson-gx-uart",
+		.data = (void *)true,
+	},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 3/6] tty: serial: meson: Describes the calculation of the UART baud rate clock using a clock frame

[Jerome Brunet]

On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:

> Using the common Clock code to describe the UART baud rate clock
> makes it easier for the UART driver to be compatible with the
> baud rate requirements of the UART IP on different meson chips.
>
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---
>  drivers/tty/serial/meson_uart.c | 194 +++++++++++++++++++++++---------
>  1 file changed, 142 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 7570958d010c..4768d51fac70 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/console.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
> @@ -65,9 +66,7 @@
>  #define AML_UART_RECV_IRQ(c)		((c) & 0xff)
>  
>  /* AML_UART_REG5 bits */
> -#define AML_UART_BAUD_MASK		0x7fffff
>  #define AML_UART_BAUD_USE		BIT(23)
> -#define AML_UART_BAUD_XTAL		BIT(24)
>  
>  #define AML_UART_PORT_NUM		12
>  #define AML_UART_PORT_OFFSET		6
> @@ -76,6 +75,11 @@
>  #define AML_UART_POLL_USEC		5
>  #define AML_UART_TIMEOUT_USEC		10000
>  
> +struct meson_uart_data {
> +	struct clk	*baud_clk;
> +	bool		use_xtal_clk;
> +};
> +
>  static struct uart_driver meson_uart_driver;
>  
>  static struct uart_port *meson_ports[AML_UART_PORT_NUM];
> @@ -293,19 +297,17 @@ static int meson_uart_startup(struct uart_port *port)
>  
>  static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
>  {
> +	struct meson_uart_data *private_data = port->private_data;
>  	u32 val;
>  
>  	while (!meson_uart_tx_empty(port))
>  		cpu_relax();
>  
> -	if (port->uartclk == 24000000) {
> -		val = ((port->uartclk / 3) / baud) - 1;
> -		val |= AML_UART_BAUD_XTAL;
> -	} else {
> -		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
> -	}
> +	val = readl(port->membase + AML_UART_REG5);
>  	val |= AML_UART_BAUD_USE;
>  	writel(val, port->membase + AML_UART_REG5);
> +
> +	clk_set_rate(private_data->baud_clk, baud);
>  }
>  
>  static void meson_uart_set_termios(struct uart_port *port,
> @@ -395,11 +397,20 @@ static int meson_uart_verify_port(struct uart_port *port,
>  
>  static void meson_uart_release_port(struct uart_port *port)
>  {
> -	/* nothing to do */
> +	struct meson_uart_data *private_data = port->private_data;
> +
> +	clk_disable_unprepare(private_data->baud_clk);
>  }
>  
>  static int meson_uart_request_port(struct uart_port *port)
>  {
> +	struct meson_uart_data *private_data = port->private_data;
> +	int ret;
> +
> +	ret = clk_prepare_enable(private_data->baud_clk);
> +	if (ret)
> +		return ret;
> +

I think we already discussed that. This is yet another behavior change
Previously, enabling the clock was done at probe time.

It's fine to change it, if there is a justification, but not in the same
change as the rework of the divider

>  	return 0;
>  }
>  
> @@ -629,57 +640,106 @@ static struct uart_driver meson_uart_driver = {
>  	.cons		= MESON_SERIAL_CONSOLE,
>  };
>  
> -static inline struct clk *meson_uart_probe_clock(struct device *dev,
> -						 const char *id)
> -{
> -	struct clk *clk = NULL;
> -	int ret;
> -
> -	clk = devm_clk_get(dev, id);
> -	if (IS_ERR(clk))
> -		return clk;
> -
> -	ret = clk_prepare_enable(clk);
> -	if (ret) {
> -		dev_err(dev, "couldn't enable clk\n");
> -		return ERR_PTR(ret);
> -	}
> +static const struct clk_div_table xtal_div_table[] = {
> +	{ 0, 3 },
> +	{ 1, 1 },
> +	{ 2, 2 },
> +	{ 3, 2 },
> +};
>  
> -	devm_add_action_or_reset(dev,
> -			(void(*)(void *))clk_disable_unprepare,
> -			clk);
> +static u32 use_xtal_mux_table;
>  
> -	return clk;
> -}
> -
> -static int meson_uart_probe_clocks(struct platform_device *pdev,
> -				   struct uart_port *port)
> +static int meson_uart_probe_clocks(struct uart_port *port)
>  {
> -	struct clk *clk_xtal = NULL;
> -	struct clk *clk_pclk = NULL;
> -	struct clk *clk_baud = NULL;
> +	struct meson_uart_data *private_data = port->private_data;
> +	struct clk *clk_baud, *clk_xtal;
> +	struct clk_hw *hw, *clk81_div4_hw;
> +	char clk_name[32];
> +	struct clk_parent_data use_xtal_mux_parents;
>  
> -	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
> -	if (IS_ERR(clk_pclk))
> -		return PTR_ERR(clk_pclk);
> +	clk_baud = devm_clk_get(port->dev, "baud");
> +	if (IS_ERR(clk_baud)) {
> +		dev_err(port->dev, "Failed to get the 'baud' clock\n");
> +		return PTR_ERR(clk_baud);
> +	}

Calling devm_clk_get() would not be necessary if you used "fw_name" in
the parent table. Same bellow

>  
> -	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
> +	clk_xtal = devm_clk_get(port->dev, "xtal");
>  	if (IS_ERR(clk_xtal))
> -		return PTR_ERR(clk_xtal);
> -
> -	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
> -	if (IS_ERR(clk_baud))
> -		return PTR_ERR(clk_baud);
> +		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
> +				     "Failed to get the 'xtal' clock\n");
> +
> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
> +		 "clk81_div4");
> +	clk81_div4_hw = devm_clk_hw_register_fixed_factor(port->dev,
> +							  clk_name,
> +							  __clk_get_name(clk_baud),
> +							  CLK_SET_RATE_NO_REPARENT,
> +							  1, 4);
> +	if (IS_ERR(clk81_div4_hw))
> +		return PTR_ERR(clk81_div4_hw);

So, whatever the chip type - you register a fixed 4 divider ....

> +
> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
> +		 "xtal_div");
> +	hw = devm_clk_hw_register_divider_table(port->dev,
> +						clk_name,
> +						__clk_get_name(clk_baud),
> +						CLK_SET_RATE_NO_REPARENT,
> +						port->membase + AML_UART_REG5,
> +						26, 2,
> +						CLK_DIVIDER_READ_ONLY,
> +						xtal_div_table, NULL);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	if (private_data->use_xtal_clk) {
> +		use_xtal_mux_table = 1;
> +		use_xtal_mux_parents.hw = hw;
> +	} else {
> +		use_xtal_mux_parents.hw = clk81_div4_hw;

... which you may end up not using in the end
This is bad.

> +	}
>  
> -	port->uartclk = clk_get_rate(clk_baud);
> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
> +		 "use_xtal");
> +	hw = __devm_clk_hw_register_mux(port->dev, NULL,
> +					clk_name,
> +					1,
> +					NULL, NULL,
> +					&use_xtal_mux_parents,
> +					CLK_SET_RATE_PARENT,
> +					port->membase + AML_UART_REG5,
> +					24, 0x1,
> +					CLK_MUX_READ_ONLY,
> +					&use_xtal_mux_table, NULL);
> +
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	port->uartclk = clk_hw_get_rate(hw);
> +
> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
> +		 "baud_div");
> +	hw = devm_clk_hw_register_divider(port->dev,
> +					  clk_name,
> +					  clk_hw_get_name(hw),
> +					  CLK_SET_RATE_PARENT,
> +					  port->membase + AML_UART_REG5,
> +					  0, 23,
> +					  CLK_DIVIDER_ROUND_CLOSEST,
> +					  NULL);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	private_data->baud_clk = hw->clk;
>  
>  	return 0;
>  }
>  
>  static int meson_uart_probe(struct platform_device *pdev)
>  {
> +	struct meson_uart_data *private_data;
>  	struct resource *res_mem;
>  	struct uart_port *port;
> +	struct clk *pclk;
>  	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
>  	int ret = 0;
>  	int irq;
> @@ -705,6 +765,15 @@ static int meson_uart_probe(struct platform_device *pdev)
>  	if (!res_mem)
>  		return -ENODEV;
>  
> +	pclk = devm_clk_get(&pdev->dev, "pclk");
> +	if (IS_ERR(pclk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pclk),
> +				     "Failed to get the 'pclk' clock\n");
> +
> +	ret = clk_prepare_enable(pclk);
> +	if (ret)
> +		return ret;
> +
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
> @@ -724,9 +793,13 @@ static int meson_uart_probe(struct platform_device *pdev)
>  	if (IS_ERR(port->membase))
>  		return PTR_ERR(port->membase);
>  
> -	ret = meson_uart_probe_clocks(pdev, port);
> -	if (ret)
> -		return ret;
> +	private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
> +				    GFP_KERNEL);
> +	if (!private_data)
> +		return -ENOMEM;
> +
> +	if (device_get_match_data(&pdev->dev))
> +		private_data->use_xtal_clk = true;

As long as the device matches a compatible below, the flag will end up
'true', regardless of values in the the dt_match table.

I don't think this is the intended behavior.
It highlights that proper testing of this series is important.

Being at Amlogic, I'm sure you can test on more than just g12a and s4

>  
>  	port->iotype = UPIO_MEM;
>  	port->mapbase = res_mem->start;
> @@ -740,6 +813,11 @@ static int meson_uart_probe(struct platform_device *pdev)
>  	port->x_char = 0;
>  	port->ops = &meson_uart_ops;
>  	port->fifosize = fifosize;
> +	port->private_data = private_data;
> +
> +	ret = meson_uart_probe_clocks(port);
> +	if (ret)
> +		return ret;
>  
>  	meson_ports[pdev->id] = port;
>  	platform_set_drvdata(pdev, port);
> @@ -766,10 +844,22 @@ static int meson_uart_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id meson_uart_dt_match[] = {
> -	{ .compatible = "amlogic,meson6-uart" },
> -	{ .compatible = "amlogic,meson8-uart" },
> -	{ .compatible = "amlogic,meson8b-uart" },
> -	{ .compatible = "amlogic,meson-gx-uart" },
> +	{
> +		.compatible = "amlogic,meson6-uart",
> +		.data = (void *)false,
> +	},
> +	{
> +		.compatible = "amlogic,meson8-uart",
> +		.data = (void *)false,
> +	},
> +	{
> +		.compatible = "amlogic,meson8b-uart",
> +		.data = (void *)false,
> +	},
> +	{
> +		.compatible = "amlogic,meson-gx-uart",
> +		.data = (void *)true,
> +	},
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, meson_uart_dt_match);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 3/6] tty: serial: meson: Describes the calculation of the UART baud rate clock using a clock frame

[Yu Tu]

Hi Jerome,

On 2022/2/28 19:10, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Using the common Clock code to describe the UART baud rate clock
>> makes it easier for the UART driver to be compatible with the
>> baud rate requirements of the UART IP on different meson chips.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>>   drivers/tty/serial/meson_uart.c | 194 +++++++++++++++++++++++---------
>>   1 file changed, 142 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index 7570958d010c..4768d51fac70 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -6,6 +6,7 @@
>>    */
>>   
>>   #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>>   #include <linux/console.h>
>>   #include <linux/delay.h>
>>   #include <linux/init.h>
>> @@ -65,9 +66,7 @@
>>   #define AML_UART_RECV_IRQ(c)		((c) & 0xff)
>>   
>>   /* AML_UART_REG5 bits */
>> -#define AML_UART_BAUD_MASK		0x7fffff
>>   #define AML_UART_BAUD_USE		BIT(23)
>> -#define AML_UART_BAUD_XTAL		BIT(24)
>>   
>>   #define AML_UART_PORT_NUM		12
>>   #define AML_UART_PORT_OFFSET		6
>> @@ -76,6 +75,11 @@
>>   #define AML_UART_POLL_USEC		5
>>   #define AML_UART_TIMEOUT_USEC		10000
>>   
>> +struct meson_uart_data {
>> +	struct clk	*baud_clk;
>> +	bool		use_xtal_clk;
>> +};
>> +
>>   static struct uart_driver meson_uart_driver;
>>   
>>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>> @@ -293,19 +297,17 @@ static int meson_uart_startup(struct uart_port *port)
>>   
>>   static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
>>   {
>> +	struct meson_uart_data *private_data = port->private_data;
>>   	u32 val;
>>   
>>   	while (!meson_uart_tx_empty(port))
>>   		cpu_relax();
>>   
>> -	if (port->uartclk == 24000000) {
>> -		val = ((port->uartclk / 3) / baud) - 1;
>> -		val |= AML_UART_BAUD_XTAL;
>> -	} else {
>> -		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>> -	}
>> +	val = readl(port->membase + AML_UART_REG5);
>>   	val |= AML_UART_BAUD_USE;
>>   	writel(val, port->membase + AML_UART_REG5);
>> +
>> +	clk_set_rate(private_data->baud_clk, baud);
>>   }
>>   
>>   static void meson_uart_set_termios(struct uart_port *port,
>> @@ -395,11 +397,20 @@ static int meson_uart_verify_port(struct uart_port *port,
>>   
>>   static void meson_uart_release_port(struct uart_port *port)
>>   {
>> -	/* nothing to do */
>> +	struct meson_uart_data *private_data = port->private_data;
>> +
>> +	clk_disable_unprepare(private_data->baud_clk);
>>   }
>>   
>>   static int meson_uart_request_port(struct uart_port *port)
>>   {
>> +	struct meson_uart_data *private_data = port->private_data;
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(private_data->baud_clk);
>> +	if (ret)
>> +		return ret;
>> +
> 
> I think we already discussed that. This is yet another behavior change
> Previously, enabling the clock was done at probe time.
> 
> It's fine to change it, if there is a justification, but not in the same
> change as the rework of the divider
> 
>>   	return 0;
>>   }
>>   
>> @@ -629,57 +640,106 @@ static struct uart_driver meson_uart_driver = {
>>   	.cons		= MESON_SERIAL_CONSOLE,
>>   };
>>   
>> -static inline struct clk *meson_uart_probe_clock(struct device *dev,
>> -						 const char *id)
>> -{
>> -	struct clk *clk = NULL;
>> -	int ret;
>> -
>> -	clk = devm_clk_get(dev, id);
>> -	if (IS_ERR(clk))
>> -		return clk;
>> -
>> -	ret = clk_prepare_enable(clk);
>> -	if (ret) {
>> -		dev_err(dev, "couldn't enable clk\n");
>> -		return ERR_PTR(ret);
>> -	}
>> +static const struct clk_div_table xtal_div_table[] = {
>> +	{ 0, 3 },
>> +	{ 1, 1 },
>> +	{ 2, 2 },
>> +	{ 3, 2 },
>> +};
>>   
>> -	devm_add_action_or_reset(dev,
>> -			(void(*)(void *))clk_disable_unprepare,
>> -			clk);
>> +static u32 use_xtal_mux_table;
>>   
>> -	return clk;
>> -}
>> -
>> -static int meson_uart_probe_clocks(struct platform_device *pdev,
>> -				   struct uart_port *port)
>> +static int meson_uart_probe_clocks(struct uart_port *port)
>>   {
>> -	struct clk *clk_xtal = NULL;
>> -	struct clk *clk_pclk = NULL;
>> -	struct clk *clk_baud = NULL;
>> +	struct meson_uart_data *private_data = port->private_data;
>> +	struct clk *clk_baud, *clk_xtal;
>> +	struct clk_hw *hw, *clk81_div4_hw;
>> +	char clk_name[32];
>> +	struct clk_parent_data use_xtal_mux_parents;
>>   
>> -	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
>> -	if (IS_ERR(clk_pclk))
>> -		return PTR_ERR(clk_pclk);
>> +	clk_baud = devm_clk_get(port->dev, "baud");
>> +	if (IS_ERR(clk_baud)) {
>> +		dev_err(port->dev, "Failed to get the 'baud' clock\n");
>> +		return PTR_ERR(clk_baud);
>> +	}
> 
> Calling devm_clk_get() would not be necessary if you used "fw_name" in
> the parent table. Same bellow
Personally, I think it is good that you can understand your meaning, but 
as you are an expert in CCF, it is nice to write code in that way, but 
for people who are not unfamiliar with CCF, they may only care about the 
use of CCF.It's not pretty to use but it's easy to understand.
> 
>>   
>> -	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
>> +	clk_xtal = devm_clk_get(port->dev, "xtal");
>>   	if (IS_ERR(clk_xtal))
>> -		return PTR_ERR(clk_xtal);
>> -
>> -	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
>> -	if (IS_ERR(clk_baud))
>> -		return PTR_ERR(clk_baud);
>> +		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
>> +				     "Failed to get the 'xtal' clock\n");
>> +
>> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>> +		 "clk81_div4");
>> +	clk81_div4_hw = devm_clk_hw_register_fixed_factor(port->dev,
>> +							  clk_name,
>> +							  __clk_get_name(clk_baud),
>> +							  CLK_SET_RATE_NO_REPARENT,
>> +							  1, 4);
>> +	if (IS_ERR(clk81_div4_hw))
>> +		return PTR_ERR(clk81_div4_hw);
> 
> So, whatever the chip type - you register a fixed 4 divider ....
As you suggested last time, this CLK has been stored, but some chips are 
not used. If you have better suggestions, please let me know and I can 
make corrections later.
> 
>> +
>> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>> +		 "xtal_div");
>> +	hw = devm_clk_hw_register_divider_table(port->dev,
>> +						clk_name,
>> +						__clk_get_name(clk_baud),
>> +						CLK_SET_RATE_NO_REPARENT,
>> +						port->membase + AML_UART_REG5,
>> +						26, 2,
>> +						CLK_DIVIDER_READ_ONLY,
>> +						xtal_div_table, NULL);
>> +	if (IS_ERR(hw))
>> +		return PTR_ERR(hw);
>> +
>> +	if (private_data->use_xtal_clk) {
>> +		use_xtal_mux_table = 1;
>> +		use_xtal_mux_parents.hw = hw;
>> +	} else {
>> +		use_xtal_mux_parents.hw = clk81_div4_hw;
> 
> ... which you may end up not using in the end
> This is bad.
If you have better suggestions, please let me know and I can make 
corrections later.
> 
>> +	}
>>   
>> -	port->uartclk = clk_get_rate(clk_baud);
>> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>> +		 "use_xtal");
>> +	hw = __devm_clk_hw_register_mux(port->dev, NULL,
>> +					clk_name,
>> +					1,
>> +					NULL, NULL,
>> +					&use_xtal_mux_parents,
>> +					CLK_SET_RATE_PARENT,
>> +					port->membase + AML_UART_REG5,
>> +					24, 0x1,
>> +					CLK_MUX_READ_ONLY,
>> +					&use_xtal_mux_table, NULL);
>> +
>> +	if (IS_ERR(hw))
>> +		return PTR_ERR(hw);
>> +
>> +	port->uartclk = clk_hw_get_rate(hw);
>> +
>> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>> +		 "baud_div");
>> +	hw = devm_clk_hw_register_divider(port->dev,
>> +					  clk_name,
>> +					  clk_hw_get_name(hw),
>> +					  CLK_SET_RATE_PARENT,
>> +					  port->membase + AML_UART_REG5,
>> +					  0, 23,
>> +					  CLK_DIVIDER_ROUND_CLOSEST,
>> +					  NULL);
>> +	if (IS_ERR(hw))
>> +		return PTR_ERR(hw);
>> +
>> +	private_data->baud_clk = hw->clk;
>>   
>>   	return 0;
>>   }
>>   
>>   static int meson_uart_probe(struct platform_device *pdev)
>>   {
>> +	struct meson_uart_data *private_data;
>>   	struct resource *res_mem;
>>   	struct uart_port *port;
>> +	struct clk *pclk;
>>   	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
>>   	int ret = 0;
>>   	int irq;
>> @@ -705,6 +765,15 @@ static int meson_uart_probe(struct platform_device *pdev)
>>   	if (!res_mem)
>>   		return -ENODEV;
>>   
>> +	pclk = devm_clk_get(&pdev->dev, "pclk");
>> +	if (IS_ERR(pclk))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(pclk),
>> +				     "Failed to get the 'pclk' clock\n");
>> +
>> +	ret = clk_prepare_enable(pclk);
>> +	if (ret)
>> +		return ret;
>> +
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq < 0)
>>   		return irq;
>> @@ -724,9 +793,13 @@ static int meson_uart_probe(struct platform_device *pdev)
>>   	if (IS_ERR(port->membase))
>>   		return PTR_ERR(port->membase);
>>   
>> -	ret = meson_uart_probe_clocks(pdev, port);
>> -	if (ret)
>> -		return ret;
>> +	private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
>> +				    GFP_KERNEL);
>> +	if (!private_data)
>> +		return -ENOMEM;
>> +
>> +	if (device_get_match_data(&pdev->dev))
>> +		private_data->use_xtal_clk = true;
> 
> As long as the device matches a compatible below, the flag will end up
> 'true', regardless of values in the the dt_match table.
> 
> I don't think this is the intended behavior.
> It highlights that proper testing of this series is important.
> 
> Being at Amlogic, I'm sure you can test on more than just g12a and s4
> 
I believe with your experience this may be a real problem. I heard that 
your company has our boards. If so, can you help verify it?
>>   
>>   	port->iotype = UPIO_MEM;
>>   	port->mapbase = res_mem->start;
>> @@ -740,6 +813,11 @@ static int meson_uart_probe(struct platform_device *pdev)
>>   	port->x_char = 0;
>>   	port->ops = &meson_uart_ops;
>>   	port->fifosize = fifosize;
>> +	port->private_data = private_data;
>> +
>> +	ret = meson_uart_probe_clocks(port);
>> +	if (ret)
>> +		return ret;
>>   
>>   	meson_ports[pdev->id] = port;
>>   	platform_set_drvdata(pdev, port);
>> @@ -766,10 +844,22 @@ static int meson_uart_remove(struct platform_device *pdev)
>>   }
>>   
>>   static const struct of_device_id meson_uart_dt_match[] = {
>> -	{ .compatible = "amlogic,meson6-uart" },
>> -	{ .compatible = "amlogic,meson8-uart" },
>> -	{ .compatible = "amlogic,meson8b-uart" },
>> -	{ .compatible = "amlogic,meson-gx-uart" },
>> +	{
>> +		.compatible = "amlogic,meson6-uart",
>> +		.data = (void *)false,
>> +	},
>> +	{
>> +		.compatible = "amlogic,meson8-uart",
>> +		.data = (void *)false,
>> +	},
>> +	{
>> +		.compatible = "amlogic,meson8b-uart",
>> +		.data = (void *)false,
>> +	},
>> +	{
>> +		.compatible = "amlogic,meson-gx-uart",
>> +		.data = (void *)true,
>> +	},
>>   	{ /* sentinel */ },
>>   };
>>   MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 3/6] tty: serial: meson: Describes the calculation of the UART baud rate clock using a clock frame

[Jerome Brunet]

On Tue 01 Mar 2022 at 14:49, Yu Tu <yu.tu@amlogic.com> wrote:

> Hi Jerome,
>
> On 2022/2/28 19:10, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> 
>> On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:
>> 
>>> Using the common Clock code to describe the UART baud rate clock
>>> makes it easier for the UART driver to be compatible with the
>>> baud rate requirements of the UART IP on different meson chips.
>>>
>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>> ---
>>>   drivers/tty/serial/meson_uart.c | 194 +++++++++++++++++++++++---------
>>>   1 file changed, 142 insertions(+), 52 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>>> index 7570958d010c..4768d51fac70 100644
>>> --- a/drivers/tty/serial/meson_uart.c
>>> +++ b/drivers/tty/serial/meson_uart.c
>>> @@ -6,6 +6,7 @@
>>>    */
>>>     #include <linux/clk.h>
>>> +#include <linux/clk-provider.h>
>>>   #include <linux/console.h>
>>>   #include <linux/delay.h>
>>>   #include <linux/init.h>
>>> @@ -65,9 +66,7 @@
>>>   #define AML_UART_RECV_IRQ(c)		((c) & 0xff)
>>>     /* AML_UART_REG5 bits */
>>> -#define AML_UART_BAUD_MASK		0x7fffff
>>>   #define AML_UART_BAUD_USE		BIT(23)
>>> -#define AML_UART_BAUD_XTAL		BIT(24)
>>>     #define AML_UART_PORT_NUM		12
>>>   #define AML_UART_PORT_OFFSET		6
>>> @@ -76,6 +75,11 @@
>>>   #define AML_UART_POLL_USEC		5
>>>   #define AML_UART_TIMEOUT_USEC		10000
>>>   +struct meson_uart_data {
>>> +	struct clk	*baud_clk;
>>> +	bool		use_xtal_clk;
>>> +};
>>> +
>>>   static struct uart_driver meson_uart_driver;
>>>     static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>>> @@ -293,19 +297,17 @@ static int meson_uart_startup(struct uart_port *port)
>>>     static void meson_uart_change_speed(struct uart_port *port, unsigned
>>> long baud)
>>>   {
>>> +	struct meson_uart_data *private_data = port->private_data;
>>>   	u32 val;
>>>     	while (!meson_uart_tx_empty(port))
>>>   		cpu_relax();
>>>   -	if (port->uartclk == 24000000) {
>>> -		val = ((port->uartclk / 3) / baud) - 1;
>>> -		val |= AML_UART_BAUD_XTAL;
>>> -	} else {
>>> -		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>>> -	}
>>> +	val = readl(port->membase + AML_UART_REG5);
>>>   	val |= AML_UART_BAUD_USE;
>>>   	writel(val, port->membase + AML_UART_REG5);
>>> +
>>> +	clk_set_rate(private_data->baud_clk, baud);
>>>   }
>>>     static void meson_uart_set_termios(struct uart_port *port,
>>> @@ -395,11 +397,20 @@ static int meson_uart_verify_port(struct uart_port *port,
>>>     static void meson_uart_release_port(struct uart_port *port)
>>>   {
>>> -	/* nothing to do */
>>> +	struct meson_uart_data *private_data = port->private_data;
>>> +
>>> +	clk_disable_unprepare(private_data->baud_clk);
>>>   }
>>>     static int meson_uart_request_port(struct uart_port *port)
>>>   {
>>> +	struct meson_uart_data *private_data = port->private_data;
>>> +	int ret;
>>> +
>>> +	ret = clk_prepare_enable(private_data->baud_clk);
>>> +	if (ret)
>>> +		return ret;
>>> +
>> I think we already discussed that. This is yet another behavior change
>> Previously, enabling the clock was done at probe time.
>> It's fine to change it, if there is a justification, but not in the same
>> change as the rework of the divider
>> 
>>>   	return 0;
>>>   }
>>>   @@ -629,57 +640,106 @@ static struct uart_driver meson_uart_driver = {
>>>   	.cons		= MESON_SERIAL_CONSOLE,
>>>   };
>>>   -static inline struct clk *meson_uart_probe_clock(struct device *dev,
>>> -						 const char *id)
>>> -{
>>> -	struct clk *clk = NULL;
>>> -	int ret;
>>> -
>>> -	clk = devm_clk_get(dev, id);
>>> -	if (IS_ERR(clk))
>>> -		return clk;
>>> -
>>> -	ret = clk_prepare_enable(clk);
>>> -	if (ret) {
>>> -		dev_err(dev, "couldn't enable clk\n");
>>> -		return ERR_PTR(ret);
>>> -	}
>>> +static const struct clk_div_table xtal_div_table[] = {
>>> +	{ 0, 3 },
>>> +	{ 1, 1 },
>>> +	{ 2, 2 },
>>> +	{ 3, 2 },
>>> +};
>>>   -	devm_add_action_or_reset(dev,
>>> -			(void(*)(void *))clk_disable_unprepare,
>>> -			clk);
>>> +static u32 use_xtal_mux_table;
>>>   -	return clk;
>>> -}
>>> -
>>> -static int meson_uart_probe_clocks(struct platform_device *pdev,
>>> -				   struct uart_port *port)
>>> +static int meson_uart_probe_clocks(struct uart_port *port)
>>>   {
>>> -	struct clk *clk_xtal = NULL;
>>> -	struct clk *clk_pclk = NULL;
>>> -	struct clk *clk_baud = NULL;
>>> +	struct meson_uart_data *private_data = port->private_data;
>>> +	struct clk *clk_baud, *clk_xtal;
>>> +	struct clk_hw *hw, *clk81_div4_hw;
>>> +	char clk_name[32];
>>> +	struct clk_parent_data use_xtal_mux_parents;
>>>   -	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
>>> -	if (IS_ERR(clk_pclk))
>>> -		return PTR_ERR(clk_pclk);
>>> +	clk_baud = devm_clk_get(port->dev, "baud");
>>> +	if (IS_ERR(clk_baud)) {
>>> +		dev_err(port->dev, "Failed to get the 'baud' clock\n");
>>> +		return PTR_ERR(clk_baud);
>>> +	}
>> Calling devm_clk_get() would not be necessary if you used "fw_name" in
>> the parent table. Same bellow
> Personally, I think it is good that you can understand your meaning, but as
> you are an expert in CCF, it is nice to write code in that way, but for
> people who are not unfamiliar with CCF, they may only care about the use of
> CCF.It's not pretty to use but it's easy to understand.

There is no magic in CCF. Stephen and the other contributor took time to
add the fw_name mechanism espcially for this. I'm suggesting and you are
expected to actually look at the code and considerer it. Lack of
"expertize" is not a valid argument.

>> 
>>>   -	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
>>> +	clk_xtal = devm_clk_get(port->dev, "xtal");
>>>   	if (IS_ERR(clk_xtal))
>>> -		return PTR_ERR(clk_xtal);
>>> -
>>> -	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
>>> -	if (IS_ERR(clk_baud))
>>> -		return PTR_ERR(clk_baud);
>>> +		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
>>> +				     "Failed to get the 'xtal' clock\n");
>>> +
>>> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>>> +		 "clk81_div4");
>>> +	clk81_div4_hw = devm_clk_hw_register_fixed_factor(port->dev,
>>> +							  clk_name,
>>> +							  __clk_get_name(clk_baud),
>>> +							  CLK_SET_RATE_NO_REPARENT,
>>> +							  1, 4);
>>> +	if (IS_ERR(clk81_div4_hw))
>>> +		return PTR_ERR(clk81_div4_hw);
>> So, whatever the chip type - you register a fixed 4 divider ....
> As you suggested last time, this CLK has been stored, but some chips are
> not used. If you have better suggestions, please let me know and I can 
> make corrections later.

No, never suggested that. I suspected that 4 divider design was the same
on all SoC version. You reported it was not, So I don't get this

>> 
>>> +
>>> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>>> +		 "xtal_div");
>>> +	hw = devm_clk_hw_register_divider_table(port->dev,
>>> +						clk_name,
>>> +						__clk_get_name(clk_baud),
>>> +						CLK_SET_RATE_NO_REPARENT,
>>> +						port->membase + AML_UART_REG5,
>>> +						26, 2,
>>> +						CLK_DIVIDER_READ_ONLY,
>>> +						xtal_div_table, NULL);
>>> +	if (IS_ERR(hw))
>>> +		return PTR_ERR(hw);
>>> +
>>> +	if (private_data->use_xtal_clk) {
>>> +		use_xtal_mux_table = 1;
>>> +		use_xtal_mux_parents.hw = hw;
>>> +	} else {
>>> +		use_xtal_mux_parents.hw = clk81_div4_hw;
>> ... which you may end up not using in the end
>> This is bad.
> If you have better suggestions, please let me know and I can make
> corrections later.
>> 
>>> +	}
>>>   -	port->uartclk = clk_get_rate(clk_baud);
>>> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>>> +		 "use_xtal");
>>> +	hw = __devm_clk_hw_register_mux(port->dev, NULL,
>>> +					clk_name,
>>> +					1,
>>> +					NULL, NULL,
>>> +					&use_xtal_mux_parents,
>>> +					CLK_SET_RATE_PARENT,
>>> +					port->membase + AML_UART_REG5,
>>> +					24, 0x1,
>>> +					CLK_MUX_READ_ONLY,
>>> +					&use_xtal_mux_table, NULL);
>>> +
>>> +	if (IS_ERR(hw))
>>> +		return PTR_ERR(hw);
>>> +
>>> +	port->uartclk = clk_hw_get_rate(hw);
>>> +
>>> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>>> +		 "baud_div");
>>> +	hw = devm_clk_hw_register_divider(port->dev,
>>> +					  clk_name,
>>> +					  clk_hw_get_name(hw),
>>> +					  CLK_SET_RATE_PARENT,
>>> +					  port->membase + AML_UART_REG5,
>>> +					  0, 23,
>>> +					  CLK_DIVIDER_ROUND_CLOSEST,
>>> +					  NULL);
>>> +	if (IS_ERR(hw))
>>> +		return PTR_ERR(hw);
>>> +
>>> +	private_data->baud_clk = hw->clk;
>>>     	return 0;
>>>   }
>>>     static int meson_uart_probe(struct platform_device *pdev)
>>>   {
>>> +	struct meson_uart_data *private_data;
>>>   	struct resource *res_mem;
>>>   	struct uart_port *port;
>>> +	struct clk *pclk;
>>>   	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
>>>   	int ret = 0;
>>>   	int irq;
>>> @@ -705,6 +765,15 @@ static int meson_uart_probe(struct platform_device *pdev)
>>>   	if (!res_mem)
>>>   		return -ENODEV;
>>>   +	pclk = devm_clk_get(&pdev->dev, "pclk");
>>> +	if (IS_ERR(pclk))
>>> +		return dev_err_probe(&pdev->dev, PTR_ERR(pclk),
>>> +				     "Failed to get the 'pclk' clock\n");
>>> +
>>> +	ret = clk_prepare_enable(pclk);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>   	irq = platform_get_irq(pdev, 0);
>>>   	if (irq < 0)
>>>   		return irq;
>>> @@ -724,9 +793,13 @@ static int meson_uart_probe(struct platform_device *pdev)
>>>   	if (IS_ERR(port->membase))
>>>   		return PTR_ERR(port->membase);
>>>   -	ret = meson_uart_probe_clocks(pdev, port);
>>> -	if (ret)
>>> -		return ret;
>>> +	private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
>>> +				    GFP_KERNEL);
>>> +	if (!private_data)
>>> +		return -ENOMEM;
>>> +
>>> +	if (device_get_match_data(&pdev->dev))
>>> +		private_data->use_xtal_clk = true;
>> As long as the device matches a compatible below, the flag will end up
>> 'true', regardless of values in the the dt_match table.
>> I don't think this is the intended behavior.
>> It highlights that proper testing of this series is important.
>> Being at Amlogic, I'm sure you can test on more than just g12a and s4
>> 
> I believe with your experience this may be a real problem. I heard that
> your company has our boards. If so, can you help verify it?
>>>     	port->iotype = UPIO_MEM;
>>>   	port->mapbase = res_mem->start;
>>> @@ -740,6 +813,11 @@ static int meson_uart_probe(struct platform_device *pdev)
>>>   	port->x_char = 0;
>>>   	port->ops = &meson_uart_ops;
>>>   	port->fifosize = fifosize;
>>> +	port->private_data = private_data;
>>> +
>>> +	ret = meson_uart_probe_clocks(port);
>>> +	if (ret)
>>> +		return ret;
>>>     	meson_ports[pdev->id] = port;
>>>   	platform_set_drvdata(pdev, port);
>>> @@ -766,10 +844,22 @@ static int meson_uart_remove(struct platform_device *pdev)
>>>   }
>>>     static const struct of_device_id meson_uart_dt_match[] = {
>>> -	{ .compatible = "amlogic,meson6-uart" },
>>> -	{ .compatible = "amlogic,meson8-uart" },
>>> -	{ .compatible = "amlogic,meson8b-uart" },
>>> -	{ .compatible = "amlogic,meson-gx-uart" },
>>> +	{
>>> +		.compatible = "amlogic,meson6-uart",
>>> +		.data = (void *)false,
>>> +	},
>>> +	{
>>> +		.compatible = "amlogic,meson8-uart",
>>> +		.data = (void *)false,
>>> +	},
>>> +	{
>>> +		.compatible = "amlogic,meson8b-uart",
>>> +		.data = (void *)false,
>>> +	},
>>> +	{
>>> +		.compatible = "amlogic,meson-gx-uart",
>>> +		.data = (void *)true,
>>> +	},
>>>   	{ /* sentinel */ },
>>>   };
>>>   MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
>> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 3/6] tty: serial: meson: Describes the calculation of the UART baud rate clock using a clock frame

[Yu Tu]

Hi Jerome,

On 2022/3/1 16:26, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Tue 01 Mar 2022 at 14:49, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Hi Jerome,
>>
>> On 2022/2/28 19:10, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>>> Using the common Clock code to describe the UART baud rate clock
>>>> makes it easier for the UART driver to be compatible with the
>>>> baud rate requirements of the UART IP on different meson chips.
>>>>
>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>> ---
>>>>    drivers/tty/serial/meson_uart.c | 194 +++++++++++++++++++++++---------
>>>>    1 file changed, 142 insertions(+), 52 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>>>> index 7570958d010c..4768d51fac70 100644
>>>> --- a/drivers/tty/serial/meson_uart.c
>>>> +++ b/drivers/tty/serial/meson_uart.c
>>>> @@ -6,6 +6,7 @@
>>>>     */
>>>>      #include <linux/clk.h>
>>>> +#include <linux/clk-provider.h>
>>>>    #include <linux/console.h>
>>>>    #include <linux/delay.h>
>>>>    #include <linux/init.h>
>>>> @@ -65,9 +66,7 @@
>>>>    #define AML_UART_RECV_IRQ(c)		((c) & 0xff)
>>>>      /* AML_UART_REG5 bits */
>>>> -#define AML_UART_BAUD_MASK		0x7fffff
>>>>    #define AML_UART_BAUD_USE		BIT(23)
>>>> -#define AML_UART_BAUD_XTAL		BIT(24)
>>>>      #define AML_UART_PORT_NUM		12
>>>>    #define AML_UART_PORT_OFFSET		6
>>>> @@ -76,6 +75,11 @@
>>>>    #define AML_UART_POLL_USEC		5
>>>>    #define AML_UART_TIMEOUT_USEC		10000
>>>>    +struct meson_uart_data {
>>>> +	struct clk	*baud_clk;
>>>> +	bool		use_xtal_clk;
>>>> +};
>>>> +
>>>>    static struct uart_driver meson_uart_driver;
>>>>      static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>>>> @@ -293,19 +297,17 @@ static int meson_uart_startup(struct uart_port *port)
>>>>      static void meson_uart_change_speed(struct uart_port *port, unsigned
>>>> long baud)
>>>>    {
>>>> +	struct meson_uart_data *private_data = port->private_data;
>>>>    	u32 val;
>>>>      	while (!meson_uart_tx_empty(port))
>>>>    		cpu_relax();
>>>>    -	if (port->uartclk == 24000000) {
>>>> -		val = ((port->uartclk / 3) / baud) - 1;
>>>> -		val |= AML_UART_BAUD_XTAL;
>>>> -	} else {
>>>> -		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>>>> -	}
>>>> +	val = readl(port->membase + AML_UART_REG5);
>>>>    	val |= AML_UART_BAUD_USE;
>>>>    	writel(val, port->membase + AML_UART_REG5);
>>>> +
>>>> +	clk_set_rate(private_data->baud_clk, baud);
>>>>    }
>>>>      static void meson_uart_set_termios(struct uart_port *port,
>>>> @@ -395,11 +397,20 @@ static int meson_uart_verify_port(struct uart_port *port,
>>>>      static void meson_uart_release_port(struct uart_port *port)
>>>>    {
>>>> -	/* nothing to do */
>>>> +	struct meson_uart_data *private_data = port->private_data;
>>>> +
>>>> +	clk_disable_unprepare(private_data->baud_clk);
>>>>    }
>>>>      static int meson_uart_request_port(struct uart_port *port)
>>>>    {
>>>> +	struct meson_uart_data *private_data = port->private_data;
>>>> +	int ret;
>>>> +
>>>> +	ret = clk_prepare_enable(private_data->baud_clk);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>> I think we already discussed that. This is yet another behavior change
>>> Previously, enabling the clock was done at probe time.
>>> It's fine to change it, if there is a justification, but not in the same
>>> change as the rework of the divider
>>>
>>>>    	return 0;
>>>>    }
>>>>    @@ -629,57 +640,106 @@ static struct uart_driver meson_uart_driver = {
>>>>    	.cons		= MESON_SERIAL_CONSOLE,
>>>>    };
>>>>    -static inline struct clk *meson_uart_probe_clock(struct device *dev,
>>>> -						 const char *id)
>>>> -{
>>>> -	struct clk *clk = NULL;
>>>> -	int ret;
>>>> -
>>>> -	clk = devm_clk_get(dev, id);
>>>> -	if (IS_ERR(clk))
>>>> -		return clk;
>>>> -
>>>> -	ret = clk_prepare_enable(clk);
>>>> -	if (ret) {
>>>> -		dev_err(dev, "couldn't enable clk\n");
>>>> -		return ERR_PTR(ret);
>>>> -	}
>>>> +static const struct clk_div_table xtal_div_table[] = {
>>>> +	{ 0, 3 },
>>>> +	{ 1, 1 },
>>>> +	{ 2, 2 },
>>>> +	{ 3, 2 },
>>>> +};
>>>>    -	devm_add_action_or_reset(dev,
>>>> -			(void(*)(void *))clk_disable_unprepare,
>>>> -			clk);
>>>> +static u32 use_xtal_mux_table;
>>>>    -	return clk;
>>>> -}
>>>> -
>>>> -static int meson_uart_probe_clocks(struct platform_device *pdev,
>>>> -				   struct uart_port *port)
>>>> +static int meson_uart_probe_clocks(struct uart_port *port)
>>>>    {
>>>> -	struct clk *clk_xtal = NULL;
>>>> -	struct clk *clk_pclk = NULL;
>>>> -	struct clk *clk_baud = NULL;
>>>> +	struct meson_uart_data *private_data = port->private_data;
>>>> +	struct clk *clk_baud, *clk_xtal;
>>>> +	struct clk_hw *hw, *clk81_div4_hw;
>>>> +	char clk_name[32];
>>>> +	struct clk_parent_data use_xtal_mux_parents;
>>>>    -	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
>>>> -	if (IS_ERR(clk_pclk))
>>>> -		return PTR_ERR(clk_pclk);
>>>> +	clk_baud = devm_clk_get(port->dev, "baud");
>>>> +	if (IS_ERR(clk_baud)) {
>>>> +		dev_err(port->dev, "Failed to get the 'baud' clock\n");
>>>> +		return PTR_ERR(clk_baud);
>>>> +	}
>>> Calling devm_clk_get() would not be necessary if you used "fw_name" in
>>> the parent table. Same bellow
>> Personally, I think it is good that you can understand your meaning, but as
>> you are an expert in CCF, it is nice to write code in that way, but for
>> people who are not unfamiliar with CCF, they may only care about the use of
>> CCF.It's not pretty to use but it's easy to understand.
> 
> There is no magic in CCF. Stephen and the other contributor took time to
> add the fw_name mechanism espcially for this. I'm suggesting and you are
> expected to actually look at the code and considerer it. Lack of
> "expertize" is not a valid argument.
I agree you have a point, but isn't simplicity and ease of understanding 
the key to code design?
> 
>>>
>>>>    -	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
>>>> +	clk_xtal = devm_clk_get(port->dev, "xtal");
>>>>    	if (IS_ERR(clk_xtal))
>>>> -		return PTR_ERR(clk_xtal);
>>>> -
>>>> -	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
>>>> -	if (IS_ERR(clk_baud))
>>>> -		return PTR_ERR(clk_baud);
>>>> +		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
>>>> +				     "Failed to get the 'xtal' clock\n");
>>>> +
>>>> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>>>> +		 "clk81_div4");
>>>> +	clk81_div4_hw = devm_clk_hw_register_fixed_factor(port->dev,
>>>> +							  clk_name,
>>>> +							  __clk_get_name(clk_baud),
>>>> +							  CLK_SET_RATE_NO_REPARENT,
>>>> +							  1, 4);
>>>> +	if (IS_ERR(clk81_div4_hw))
>>>> +		return PTR_ERR(clk81_div4_hw);
>>> So, whatever the chip type - you register a fixed 4 divider ....
>> As you suggested last time, this CLK has been stored, but some chips are
>> not used. If you have better suggestions, please let me know and I can
>> make corrections later.
> 
> No, never suggested that. I suspected that 4 divider design was the same
> on all SoC version. You reported it was not, So I don't get this
> 
>>>
>>>> +
>>>> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>>>> +		 "xtal_div");
>>>> +	hw = devm_clk_hw_register_divider_table(port->dev,
>>>> +						clk_name,
>>>> +						__clk_get_name(clk_baud),
>>>> +						CLK_SET_RATE_NO_REPARENT,
>>>> +						port->membase + AML_UART_REG5,
>>>> +						26, 2,
>>>> +						CLK_DIVIDER_READ_ONLY,
>>>> +						xtal_div_table, NULL);
>>>> +	if (IS_ERR(hw))
>>>> +		return PTR_ERR(hw);
>>>> +
>>>> +	if (private_data->use_xtal_clk) {
>>>> +		use_xtal_mux_table = 1;
>>>> +		use_xtal_mux_parents.hw = hw;
>>>> +	} else {
>>>> +		use_xtal_mux_parents.hw = clk81_div4_hw;
>>> ... which you may end up not using in the end
>>> This is bad.
>> If you have better suggestions, please let me know and I can make
>> corrections later.
>>>
>>>> +	}
>>>>    -	port->uartclk = clk_get_rate(clk_baud);
>>>> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>>>> +		 "use_xtal");
>>>> +	hw = __devm_clk_hw_register_mux(port->dev, NULL,
>>>> +					clk_name,
>>>> +					1,
>>>> +					NULL, NULL,
>>>> +					&use_xtal_mux_parents,
>>>> +					CLK_SET_RATE_PARENT,
>>>> +					port->membase + AML_UART_REG5,
>>>> +					24, 0x1,
>>>> +					CLK_MUX_READ_ONLY,
>>>> +					&use_xtal_mux_table, NULL);
>>>> +
>>>> +	if (IS_ERR(hw))
>>>> +		return PTR_ERR(hw);
>>>> +
>>>> +	port->uartclk = clk_hw_get_rate(hw);
>>>> +
>>>> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>>>> +		 "baud_div");
>>>> +	hw = devm_clk_hw_register_divider(port->dev,
>>>> +					  clk_name,
>>>> +					  clk_hw_get_name(hw),
>>>> +					  CLK_SET_RATE_PARENT,
>>>> +					  port->membase + AML_UART_REG5,
>>>> +					  0, 23,
>>>> +					  CLK_DIVIDER_ROUND_CLOSEST,
>>>> +					  NULL);
>>>> +	if (IS_ERR(hw))
>>>> +		return PTR_ERR(hw);
>>>> +
>>>> +	private_data->baud_clk = hw->clk;
>>>>      	return 0;
>>>>    }
>>>>      static int meson_uart_probe(struct platform_device *pdev)
>>>>    {
>>>> +	struct meson_uart_data *private_data;
>>>>    	struct resource *res_mem;
>>>>    	struct uart_port *port;
>>>> +	struct clk *pclk;
>>>>    	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
>>>>    	int ret = 0;
>>>>    	int irq;
>>>> @@ -705,6 +765,15 @@ static int meson_uart_probe(struct platform_device *pdev)
>>>>    	if (!res_mem)
>>>>    		return -ENODEV;
>>>>    +	pclk = devm_clk_get(&pdev->dev, "pclk");
>>>> +	if (IS_ERR(pclk))
>>>> +		return dev_err_probe(&pdev->dev, PTR_ERR(pclk),
>>>> +				     "Failed to get the 'pclk' clock\n");
>>>> +
>>>> +	ret = clk_prepare_enable(pclk);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>>    	irq = platform_get_irq(pdev, 0);
>>>>    	if (irq < 0)
>>>>    		return irq;
>>>> @@ -724,9 +793,13 @@ static int meson_uart_probe(struct platform_device *pdev)
>>>>    	if (IS_ERR(port->membase))
>>>>    		return PTR_ERR(port->membase);
>>>>    -	ret = meson_uart_probe_clocks(pdev, port);
>>>> -	if (ret)
>>>> -		return ret;
>>>> +	private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
>>>> +				    GFP_KERNEL);
>>>> +	if (!private_data)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	if (device_get_match_data(&pdev->dev))
>>>> +		private_data->use_xtal_clk = true;
>>> As long as the device matches a compatible below, the flag will end up
>>> 'true', regardless of values in the the dt_match table.
>>> I don't think this is the intended behavior.
>>> It highlights that proper testing of this series is important.
>>> Being at Amlogic, I'm sure you can test on more than just g12a and s4
>>>
>> I believe with your experience this may be a real problem. I heard that
>> your company has our boards. If so, can you help verify it?
>>>>      	port->iotype = UPIO_MEM;
>>>>    	port->mapbase = res_mem->start;
>>>> @@ -740,6 +813,11 @@ static int meson_uart_probe(struct platform_device *pdev)
>>>>    	port->x_char = 0;
>>>>    	port->ops = &meson_uart_ops;
>>>>    	port->fifosize = fifosize;
>>>> +	port->private_data = private_data;
>>>> +
>>>> +	ret = meson_uart_probe_clocks(port);
>>>> +	if (ret)
>>>> +		return ret;
>>>>      	meson_ports[pdev->id] = port;
>>>>    	platform_set_drvdata(pdev, port);
>>>> @@ -766,10 +844,22 @@ static int meson_uart_remove(struct platform_device *pdev)
>>>>    }
>>>>      static const struct of_device_id meson_uart_dt_match[] = {
>>>> -	{ .compatible = "amlogic,meson6-uart" },
>>>> -	{ .compatible = "amlogic,meson8-uart" },
>>>> -	{ .compatible = "amlogic,meson8b-uart" },
>>>> -	{ .compatible = "amlogic,meson-gx-uart" },
>>>> +	{
>>>> +		.compatible = "amlogic,meson6-uart",
>>>> +		.data = (void *)false,
>>>> +	},
>>>> +	{
>>>> +		.compatible = "amlogic,meson8-uart",
>>>> +		.data = (void *)false,
>>>> +	},
>>>> +	{
>>>> +		.compatible = "amlogic,meson8b-uart",
>>>> +		.data = (void *)false,
>>>> +	},
>>>> +	{
>>>> +		.compatible = "amlogic,meson-gx-uart",
>>>> +		.data = (void *)true,
>>>> +	},
>>>>    	{ /* sentinel */ },
>>>>    };
>>>>    MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
>>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 3/6] tty: serial: meson: Describes the calculation of the UART baud rate clock using a clock frame

[Marek Szyprowski]

Hi,

On 25.02.2022 08:39, Yu Tu wrote:
> Using the common Clock code to describe the UART baud rate clock
> makes it easier for the UART driver to be compatible with the
> baud rate requirements of the UART IP on different meson chips.
>
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>

This patch landed recently in linux next-20220228 as commit 44023b8e1f14 
("tty: serial: meson: Describes the calculation of the UART baud rate 
clock using a clock frame"). It causes kernel crash on my Amlogic based 
test boards: Odroid C4/N2 and Khadas VIM3:

Unable to handle kernel paging request at virtual address 96e12b756a77c400
Mem abort info:
   ESR = 0x96000004
   EC = 0x25: DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
   FSC = 0x04: level 0 translation fault
Data abort info:
   ISV = 0, ISS = 0x00000004
   CM = 0, WnR = 0
[96e12b756a77c400] address between user and kernel address ranges
Internal error: Oops: 96000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.17.0-rc6-next-20220228+ #4588
Hardware name: Hardkernel ODROID-C4 (DT)
pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : __pi_strlen+0x14/0x150
...
Call trace:
  __pi_strlen+0x14/0x150
  kstrdup_const+0x34/0x40
  __clk_register+0x280/0x760
  clk_hw_register+0x1c/0x68
  __clk_hw_register_mux+0x180/0x1a8
  __devm_clk_hw_register_mux+0xbc/0x118
  meson_uart_probe+0x398/0x5d0
  platform_probe+0x90/0xd8
  really_probe+0xb4/0x2d0
  __driver_probe_device+0x78/0xd8
  driver_probe_device+0x3c/0x110
  __driver_attach+0xcc/0x118
  bus_for_each_dev+0x68/0xc8
  driver_attach+0x20/0x28
  bus_add_driver+0x168/0x1f8
  driver_register+0x60/0x110
  __platform_driver_register+0x24/0x30
  meson_uart_init+0x40/0x64
  do_one_initcall+0x74/0x410
  kernel_init_freeable+0x310/0x380
  kernel_init+0x20/0x128
  ret_from_fork+0x10/0x20
Code: 92402c04 b200c3e8 f13fc09f 5400088c (a9400c02)
---[ end trace 0000000000000000 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
SMP: stopping secondary CPUs
Kernel Offset: disabled
CPU features: 0x80,0000000c,19801c86
Memory Limit: none
---[ end Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x0000000b ]---

The issue is caused by using a stack for the newly registered clock. 
Making the 'use_xtal_mux_parents' static fixed the issue, but I don't 
know if this is the way of fixing it you like to apply.

> ---
>   drivers/tty/serial/meson_uart.c | 194 +++++++++++++++++++++++---------
>   1 file changed, 142 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 7570958d010c..4768d51fac70 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -6,6 +6,7 @@
>    */
>   
>   #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>   #include <linux/console.h>
>   #include <linux/delay.h>
>   #include <linux/init.h>
> @@ -65,9 +66,7 @@
>   #define AML_UART_RECV_IRQ(c)		((c) & 0xff)
>   
>   /* AML_UART_REG5 bits */
> -#define AML_UART_BAUD_MASK		0x7fffff
>   #define AML_UART_BAUD_USE		BIT(23)
> -#define AML_UART_BAUD_XTAL		BIT(24)
>   
>   #define AML_UART_PORT_NUM		12
>   #define AML_UART_PORT_OFFSET		6
> @@ -76,6 +75,11 @@
>   #define AML_UART_POLL_USEC		5
>   #define AML_UART_TIMEOUT_USEC		10000
>   
> +struct meson_uart_data {
> +	struct clk	*baud_clk;
> +	bool		use_xtal_clk;
> +};
> +
>   static struct uart_driver meson_uart_driver;
>   
>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
> @@ -293,19 +297,17 @@ static int meson_uart_startup(struct uart_port *port)
>   
>   static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
>   {
> +	struct meson_uart_data *private_data = port->private_data;
>   	u32 val;
>   
>   	while (!meson_uart_tx_empty(port))
>   		cpu_relax();
>   
> -	if (port->uartclk == 24000000) {
> -		val = ((port->uartclk / 3) / baud) - 1;
> -		val |= AML_UART_BAUD_XTAL;
> -	} else {
> -		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
> -	}
> +	val = readl(port->membase + AML_UART_REG5);
>   	val |= AML_UART_BAUD_USE;
>   	writel(val, port->membase + AML_UART_REG5);
> +
> +	clk_set_rate(private_data->baud_clk, baud);
>   }
>   
>   static void meson_uart_set_termios(struct uart_port *port,
> @@ -395,11 +397,20 @@ static int meson_uart_verify_port(struct uart_port *port,
>   
>   static void meson_uart_release_port(struct uart_port *port)
>   {
> -	/* nothing to do */
> +	struct meson_uart_data *private_data = port->private_data;
> +
> +	clk_disable_unprepare(private_data->baud_clk);
>   }
>   
>   static int meson_uart_request_port(struct uart_port *port)
>   {
> +	struct meson_uart_data *private_data = port->private_data;
> +	int ret;
> +
> +	ret = clk_prepare_enable(private_data->baud_clk);
> +	if (ret)
> +		return ret;
> +
>   	return 0;
>   }
>   
> @@ -629,57 +640,106 @@ static struct uart_driver meson_uart_driver = {
>   	.cons		= MESON_SERIAL_CONSOLE,
>   };
>   
> -static inline struct clk *meson_uart_probe_clock(struct device *dev,
> -						 const char *id)
> -{
> -	struct clk *clk = NULL;
> -	int ret;
> -
> -	clk = devm_clk_get(dev, id);
> -	if (IS_ERR(clk))
> -		return clk;
> -
> -	ret = clk_prepare_enable(clk);
> -	if (ret) {
> -		dev_err(dev, "couldn't enable clk\n");
> -		return ERR_PTR(ret);
> -	}
> +static const struct clk_div_table xtal_div_table[] = {
> +	{ 0, 3 },
> +	{ 1, 1 },
> +	{ 2, 2 },
> +	{ 3, 2 },
> +};
>   
> -	devm_add_action_or_reset(dev,
> -			(void(*)(void *))clk_disable_unprepare,
> -			clk);
> +static u32 use_xtal_mux_table;
>   
> -	return clk;
> -}
> -
> -static int meson_uart_probe_clocks(struct platform_device *pdev,
> -				   struct uart_port *port)
> +static int meson_uart_probe_clocks(struct uart_port *port)
>   {
> -	struct clk *clk_xtal = NULL;
> -	struct clk *clk_pclk = NULL;
> -	struct clk *clk_baud = NULL;
> +	struct meson_uart_data *private_data = port->private_data;
> +	struct clk *clk_baud, *clk_xtal;
> +	struct clk_hw *hw, *clk81_div4_hw;
> +	char clk_name[32];
> +	struct clk_parent_data use_xtal_mux_parents;

static struct clk_parent_data use_xtal_mux_parents;

> -	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
> -	if (IS_ERR(clk_pclk))
> -		return PTR_ERR(clk_pclk);
> +	clk_baud = devm_clk_get(port->dev, "baud");
> +	if (IS_ERR(clk_baud)) {
> +		dev_err(port->dev, "Failed to get the 'baud' clock\n");
> +		return PTR_ERR(clk_baud);
> +	}
>   
> -	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
> +	clk_xtal = devm_clk_get(port->dev, "xtal");
>   	if (IS_ERR(clk_xtal))
> -		return PTR_ERR(clk_xtal);
> -
> -	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
> -	if (IS_ERR(clk_baud))
> -		return PTR_ERR(clk_baud);
> +		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
> +				     "Failed to get the 'xtal' clock\n");
> +
> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
> +		 "clk81_div4");
> +	clk81_div4_hw = devm_clk_hw_register_fixed_factor(port->dev,
> +							  clk_name,
> +							  __clk_get_name(clk_baud),
> +							  CLK_SET_RATE_NO_REPARENT,
> +							  1, 4);
> +	if (IS_ERR(clk81_div4_hw))
> +		return PTR_ERR(clk81_div4_hw);
> +
> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
> +		 "xtal_div");
> +	hw = devm_clk_hw_register_divider_table(port->dev,
> +						clk_name,
> +						__clk_get_name(clk_baud),
> +						CLK_SET_RATE_NO_REPARENT,
> +						port->membase + AML_UART_REG5,
> +						26, 2,
> +						CLK_DIVIDER_READ_ONLY,
> +						xtal_div_table, NULL);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	if (private_data->use_xtal_clk) {
> +		use_xtal_mux_table = 1;
> +		use_xtal_mux_parents.hw = hw;
> +	} else {
> +		use_xtal_mux_parents.hw = clk81_div4_hw;
> +	}
>   
> -	port->uartclk = clk_get_rate(clk_baud);
> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
> +		 "use_xtal");
> +	hw = __devm_clk_hw_register_mux(port->dev, NULL,
> +					clk_name,
> +					1,
> +					NULL, NULL,
> +					&use_xtal_mux_parents,
> +					CLK_SET_RATE_PARENT,
> +					port->membase + AML_UART_REG5,
> +					24, 0x1,
> +					CLK_MUX_READ_ONLY,
> +					&use_xtal_mux_table, NULL);
> +
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	port->uartclk = clk_hw_get_rate(hw);
> +
> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
> +		 "baud_div");
> +	hw = devm_clk_hw_register_divider(port->dev,
> +					  clk_name,
> +					  clk_hw_get_name(hw),
> +					  CLK_SET_RATE_PARENT,
> +					  port->membase + AML_UART_REG5,
> +					  0, 23,
> +					  CLK_DIVIDER_ROUND_CLOSEST,
> +					  NULL);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	private_data->baud_clk = hw->clk;
>   
>   	return 0;
>   }
>   
>   static int meson_uart_probe(struct platform_device *pdev)
>   {
> +	struct meson_uart_data *private_data;
>   	struct resource *res_mem;
>   	struct uart_port *port;
> +	struct clk *pclk;
>   	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
>   	int ret = 0;
>   	int irq;
> @@ -705,6 +765,15 @@ static int meson_uart_probe(struct platform_device *pdev)
>   	if (!res_mem)
>   		return -ENODEV;
>   
> +	pclk = devm_clk_get(&pdev->dev, "pclk");
> +	if (IS_ERR(pclk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pclk),
> +				     "Failed to get the 'pclk' clock\n");
> +
> +	ret = clk_prepare_enable(pclk);
> +	if (ret)
> +		return ret;
> +
>   	irq = platform_get_irq(pdev, 0);
>   	if (irq < 0)
>   		return irq;
> @@ -724,9 +793,13 @@ static int meson_uart_probe(struct platform_device *pdev)
>   	if (IS_ERR(port->membase))
>   		return PTR_ERR(port->membase);
>   
> -	ret = meson_uart_probe_clocks(pdev, port);
> -	if (ret)
> -		return ret;
> +	private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
> +				    GFP_KERNEL);
> +	if (!private_data)
> +		return -ENOMEM;
> +
> +	if (device_get_match_data(&pdev->dev))
> +		private_data->use_xtal_clk = true;
>   
>   	port->iotype = UPIO_MEM;
>   	port->mapbase = res_mem->start;
> @@ -740,6 +813,11 @@ static int meson_uart_probe(struct platform_device *pdev)
>   	port->x_char = 0;
>   	port->ops = &meson_uart_ops;
>   	port->fifosize = fifosize;
> +	port->private_data = private_data;
> +
> +	ret = meson_uart_probe_clocks(port);
> +	if (ret)
> +		return ret;
>   
>   	meson_ports[pdev->id] = port;
>   	platform_set_drvdata(pdev, port);
> @@ -766,10 +844,22 @@ static int meson_uart_remove(struct platform_device *pdev)
>   }
>   
>   static const struct of_device_id meson_uart_dt_match[] = {
> -	{ .compatible = "amlogic,meson6-uart" },
> -	{ .compatible = "amlogic,meson8-uart" },
> -	{ .compatible = "amlogic,meson8b-uart" },
> -	{ .compatible = "amlogic,meson-gx-uart" },
> +	{
> +		.compatible = "amlogic,meson6-uart",
> +		.data = (void *)false,
> +	},
> +	{
> +		.compatible = "amlogic,meson8-uart",
> +		.data = (void *)false,
> +	},
> +	{
> +		.compatible = "amlogic,meson8b-uart",
> +		.data = (void *)false,
> +	},
> +	{
> +		.compatible = "amlogic,meson-gx-uart",
> +		.data = (void *)true,
> +	},
>   	{ /* sentinel */ },
>   };
>   MODULE_DEVICE_TABLE(of, meson_uart_dt_match);

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 3/6] tty: serial: meson: Describes the calculation of the UART baud rate clock using a clock frame

[Greg Kroah-Hartman]

On Tue, Mar 01, 2022 at 02:17:53PM +0100, Marek Szyprowski wrote:
> Hi,
> 
> On 25.02.2022 08:39, Yu Tu wrote:
> > Using the common Clock code to describe the UART baud rate clock
> > makes it easier for the UART driver to be compatible with the
> > baud rate requirements of the UART IP on different meson chips.
> >
> > Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> 
> This patch landed recently in linux next-20220228 as commit 44023b8e1f14 
> ("tty: serial: meson: Describes the calculation of the UART baud rate 
> clock using a clock frame"). It causes kernel crash on my Amlogic based 
> test boards: Odroid C4/N2 and Khadas VIM3:

Ok, this series is causing lots of problems, I'm just going to revert
the whole thing from my tree, thanks for letting me know.

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V7 4/6] tty: serial: meson: Make some bit of the REG5 register writable

[Yu Tu]

Make the internal clock source mux and divider writeable, allowing the
uart to deviate from the settings intially applied by the ROMCode and
using the most appropriate clocks.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 4768d51fac70..ba8dc203b9cb 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -686,7 +686,7 @@ static int meson_uart_probe_clocks(struct uart_port *port)
 						CLK_SET_RATE_NO_REPARENT,
 						port->membase + AML_UART_REG5,
 						26, 2,
-						CLK_DIVIDER_READ_ONLY,
+						CLK_DIVIDER_ROUND_CLOSEST,
 						xtal_div_table, NULL);
 	if (IS_ERR(hw))
 		return PTR_ERR(hw);
@@ -708,7 +708,7 @@ static int meson_uart_probe_clocks(struct uart_port *port)
 					CLK_SET_RATE_PARENT,
 					port->membase + AML_UART_REG5,
 					24, 0x1,
-					CLK_MUX_READ_ONLY,
+					CLK_MUX_ROUND_CLOSEST,
 					&use_xtal_mux_table, NULL);
 
 	if (IS_ERR(hw))
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V7 5/6] tty: serial: meson: The system stuck when you run the stty command on the console to change the baud rate

[Yu Tu]

Start the console and run the following commands in turn:
stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600. The
system will stuck.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index ba8dc203b9cb..d19349ead738 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -365,8 +365,13 @@ static void meson_uart_set_termios(struct uart_port *port,
 	writel(val, port->membase + AML_UART_CONTROL);
 
 	baud = uart_get_baud_rate(port, termios, old, 50, 4000000);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
 	meson_uart_change_speed(port, baud);
 
+	spin_lock_irqsave(&port->lock, flags);
+
 	port->read_status_mask = AML_UART_TX_FIFO_WERR;
 	if (iflags & INPCK)
 		port->read_status_mask |= AML_UART_PARITY_ERR |
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 5/6] tty: serial: meson: The system stuck when you run the stty command on the console to change the baud rate

[Jerome Brunet]

On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:

> Start the console and run the following commands in turn:
> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600. The
> system will stuck.
>
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---
>  drivers/tty/serial/meson_uart.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index ba8dc203b9cb..d19349ead738 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -365,8 +365,13 @@ static void meson_uart_set_termios(struct uart_port *port,
>  	writel(val, port->membase + AML_UART_CONTROL);
>  
>  	baud = uart_get_baud_rate(port, termios, old, 50, 4000000);
> +
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
>  	meson_uart_change_speed(port, baud);
>  
> +	spin_lock_irqsave(&port->lock, flags);

Already told you before, you can make meson_change_speed()
clk_set_rate() uses mutex and may sleep.

> +
>  	port->read_status_mask = AML_UART_TX_FIFO_WERR;
>  	if (iflags & INPCK)
>  		port->read_status_mask |= AML_UART_PARITY_ERR |


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 5/6] tty: serial: meson: The system stuck when you run the stty command on the console to change the baud rate

[Yu Tu]

Hi Jerome,
	Thank you for your reply.

On 2022/2/28 18:58, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Start the console and run the following commands in turn:
>> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600. The
>> system will stuck.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>>   drivers/tty/serial/meson_uart.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index ba8dc203b9cb..d19349ead738 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -365,8 +365,13 @@ static void meson_uart_set_termios(struct uart_port *port,
>>   	writel(val, port->membase + AML_UART_CONTROL);
>>   
>>   	baud = uart_get_baud_rate(port, termios, old, 50, 4000000);
>> +
>> +	spin_unlock_irqrestore(&port->lock, flags);
>> +
>>   	meson_uart_change_speed(port, baud);
>>   
>> +	spin_lock_irqsave(&port->lock, flags);
> 
> Already told you before, you can make meson_change_speed()
> clk_set_rate() uses mutex and may sleep.
Sorry, I may have only paid attention to how you asked me to change to 
using the clock before, instead of paying attention to this suggestion.
> 
>> +
>>   	port->read_status_mask = AML_UART_TX_FIFO_WERR;
>>   	if (iflags & INPCK)
>>   		port->read_status_mask |= AML_UART_PARITY_ERR |
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V7 6/6] tty: serial: meson: Added S4 SOC compatibility

[Yu Tu]

Make UART driver compatible with S4 SOC UART.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index d19349ead738..bf6be5468aaf 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -865,6 +865,10 @@ static const struct of_device_id meson_uart_dt_match[] = {
 		.compatible = "amlogic,meson-gx-uart",
 		.data = (void *)true,
 	},
+	{
+		.compatible = "amlogic,meson-s4-uart",
+		.data = (void *)true,
+	},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
-- 
2.33.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[Jerome Brunet]

On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:

> Using the common Clock code to describe the UART baud rate
> clock makes it easier for the UART driver to be compatible
> with the baud rate requirements of the UART IP on different
> meson chips. Add Meson S4 SoC compatible.
>
> The test method:
> Start the console and run the following commands in turn:
> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.
>
> Since most SoCs are too old, I was able to find all the platforms myself
> such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it with
> G12A and S4.

GXL based board are still very common an easy to come by.
I'm quite surprised that you are unable to test on this SoC family

>
> Yu Tu (6):
>   tty: serial: meson: Move request the register region to probe
>   tty: serial: meson: Use devm_ioremap_resource to get register mapped
>     memory
>   tty: serial: meson: Describes the calculation of the UART baud rate
>     clock using a clock frame
>   tty: serial: meson: Make some bit of the REG5 register writable
>   tty: serial: meson: The system stuck when you run the stty command on
>     the console to change the baud rate
>   tty: serial: meson: Added S4 SOC compatibility
>
> V6 -> V7: To solve the system stuck when you run the stty command on
> the console to change the baud rate.
> V5 -> V6: Change error format as discussed in the email.
> V4 -> V5: Change error format.
> V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
> in the email.
> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change
> the DTS before it can be deleted
> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as
> discussed in the email
>
> Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/
>
>  drivers/tty/serial/meson_uart.c | 221 ++++++++++++++++++++++----------
>  1 file changed, 154 insertions(+), 67 deletions(-)
>
>
> base-commit: a603ca60cebff8589882427a67f870ed946b3fc8


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[Yu Tu]

Hi Jerome,

On 2022/2/28 18:59, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Using the common Clock code to describe the UART baud rate
>> clock makes it easier for the UART driver to be compatible
>> with the baud rate requirements of the UART IP on different
>> meson chips. Add Meson S4 SoC compatible.
>>
>> The test method:
>> Start the console and run the following commands in turn:
>> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.
>>
>> Since most SoCs are too old, I was able to find all the platforms myself
>> such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it with
>> G12A and S4.
> 
> GXL based board are still very common an easy to come by.
> I'm quite surprised that you are unable to test on this SoC family
The fact of the matter is that the S4 is our end-2020 chip, the G12A is 
five years old, and the GXL is seven years old. If you must ask for a 
test, I will report this problem to the leadership to coordinate resources.
> 
>>
>> Yu Tu (6):
>>    tty: serial: meson: Move request the register region to probe
>>    tty: serial: meson: Use devm_ioremap_resource to get register mapped
>>      memory
>>    tty: serial: meson: Describes the calculation of the UART baud rate
>>      clock using a clock frame
>>    tty: serial: meson: Make some bit of the REG5 register writable
>>    tty: serial: meson: The system stuck when you run the stty command on
>>      the console to change the baud rate
>>    tty: serial: meson: Added S4 SOC compatibility
>>
>> V6 -> V7: To solve the system stuck when you run the stty command on
>> the console to change the baud rate.
>> V5 -> V6: Change error format as discussed in the email.
>> V4 -> V5: Change error format.
>> V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
>> in the email.
>> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change
>> the DTS before it can be deleted
>> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as
>> discussed in the email
>>
>> Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/
>>
>>   drivers/tty/serial/meson_uart.c | 221 ++++++++++++++++++++++----------
>>   1 file changed, 154 insertions(+), 67 deletions(-)
>>
>>
>> base-commit: a603ca60cebff8589882427a67f870ed946b3fc8
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[Jerome Brunet]

On Tue 01 Mar 2022 at 13:54, Yu Tu <yu.tu@amlogic.com> wrote:

> Hi Jerome,
>
> On 2022/2/28 18:59, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> 
>> On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:
>> 
>>> Using the common Clock code to describe the UART baud rate
>>> clock makes it easier for the UART driver to be compatible
>>> with the baud rate requirements of the UART IP on different
>>> meson chips. Add Meson S4 SoC compatible.
>>>
>>> The test method:
>>> Start the console and run the following commands in turn:
>>> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.
>>>
>>> Since most SoCs are too old, I was able to find all the platforms myself
>>> such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it with
>>> G12A and S4.
>> GXL based board are still very common an easy to come by.
>> I'm quite surprised that you are unable to test on this SoC family
> The fact of the matter is that the S4 is our end-2020 chip, the G12A is
> five years old, and the GXL is seven years old. If you must ask for a 
> test, I will report this problem to the leadership to coordinate resources.

The age of the SoC is irrelevant. SoCs don't get deprecated based on age
in mainline. It is not just GXL, same goes for meson8.

These SoCs are actively used. Boards with these SoCs are still sold and
easily available. See the VIM1 or the Libretech boards.

Breaking things for the the users of these SoCs is not acceptable.
So yes, looking at your series, I strongly recommend you do more tests.

>> 
>>>
>>> Yu Tu (6):
>>>    tty: serial: meson: Move request the register region to probe
>>>    tty: serial: meson: Use devm_ioremap_resource to get register mapped
>>>      memory
>>>    tty: serial: meson: Describes the calculation of the UART baud rate
>>>      clock using a clock frame
>>>    tty: serial: meson: Make some bit of the REG5 register writable
>>>    tty: serial: meson: The system stuck when you run the stty command on
>>>      the console to change the baud rate
>>>    tty: serial: meson: Added S4 SOC compatibility
>>>
>>> V6 -> V7: To solve the system stuck when you run the stty command on
>>> the console to change the baud rate.
>>> V5 -> V6: Change error format as discussed in the email.
>>> V4 -> V5: Change error format.
>>> V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
>>> in the email.
>>> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change
>>> the DTS before it can be deleted
>>> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as
>>> discussed in the email
>>>
>>> Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/
>>>
>>>   drivers/tty/serial/meson_uart.c | 221 ++++++++++++++++++++++----------
>>>   1 file changed, 154 insertions(+), 67 deletions(-)
>>>
>>>
>>> base-commit: a603ca60cebff8589882427a67f870ed946b3fc8
>> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[Yu Tu]

Hi Jerome,

On 2022/3/1 16:36, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Tue 01 Mar 2022 at 13:54, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> Hi Jerome,
>>
>> On 2022/2/28 18:59, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>>> Using the common Clock code to describe the UART baud rate
>>>> clock makes it easier for the UART driver to be compatible
>>>> with the baud rate requirements of the UART IP on different
>>>> meson chips. Add Meson S4 SoC compatible.
>>>>
>>>> The test method:
>>>> Start the console and run the following commands in turn:
>>>> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.
>>>>
>>>> Since most SoCs are too old, I was able to find all the platforms myself
>>>> such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it with
>>>> G12A and S4.
>>> GXL based board are still very common an easy to come by.
>>> I'm quite surprised that you are unable to test on this SoC family
>> The fact of the matter is that the S4 is our end-2020 chip, the G12A is
>> five years old, and the GXL is seven years old. If you must ask for a
>> test, I will report this problem to the leadership to coordinate resources.
> 
> The age of the SoC is irrelevant. SoCs don't get deprecated based on age
> in mainline. It is not just GXL, same goes for meson8.
> 
> These SoCs are actively used. Boards with these SoCs are still sold and
> easily available. See the VIM1 or the Libretech boards.
> 
> Breaking things for the the users of these SoCs is not acceptable.
> So yes, looking at your series, I strongly recommend you do more tests.
> 
You have a point there. Let's go back to the root of the problem. I aim 
to increase S4. The S4 uses 12MHZ to calculate baud. That's all.
Change it to CCF as you suggested. The changes are so large that you ask 
to test all the chips.
I also mentioned last time that using CCF would lead to a longer drive 
probe time and affect the board startup time. If this problem is not 
solved, can we reject the way you suggest using CCF?
>>>
>>>>
>>>> Yu Tu (6):
>>>>     tty: serial: meson: Move request the register region to probe
>>>>     tty: serial: meson: Use devm_ioremap_resource to get register mapped
>>>>       memory
>>>>     tty: serial: meson: Describes the calculation of the UART baud rate
>>>>       clock using a clock frame
>>>>     tty: serial: meson: Make some bit of the REG5 register writable
>>>>     tty: serial: meson: The system stuck when you run the stty command on
>>>>       the console to change the baud rate
>>>>     tty: serial: meson: Added S4 SOC compatibility
>>>>
>>>> V6 -> V7: To solve the system stuck when you run the stty command on
>>>> the console to change the baud rate.
>>>> V5 -> V6: Change error format as discussed in the email.
>>>> V4 -> V5: Change error format.
>>>> V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
>>>> in the email.
>>>> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change
>>>> the DTS before it can be deleted
>>>> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as
>>>> discussed in the email
>>>>
>>>> Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/
>>>>
>>>>    drivers/tty/serial/meson_uart.c | 221 ++++++++++++++++++++++----------
>>>>    1 file changed, 154 insertions(+), 67 deletions(-)
>>>>
>>>>
>>>> base-commit: a603ca60cebff8589882427a67f870ed946b3fc8
>>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[Neil Armstrong]

Hi,

On 01/03/2022 10:01, Yu Tu wrote:
> Hi Jerome,
> 
> On 2022/3/1 16:36, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>>
>> On Tue 01 Mar 2022 at 13:54, Yu Tu <yu.tu@amlogic.com> wrote:
>>
>>> Hi Jerome,
>>>
>>> On 2022/2/28 18:59, Jerome Brunet wrote:
>>>> [ EXTERNAL EMAIL ]
>>>>
>>>> On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>
>>>>> Using the common Clock code to describe the UART baud rate
>>>>> clock makes it easier for the UART driver to be compatible
>>>>> with the baud rate requirements of the UART IP on different
>>>>> meson chips. Add Meson S4 SoC compatible.
>>>>>
>>>>> The test method:
>>>>> Start the console and run the following commands in turn:
>>>>> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.
>>>>>
>>>>> Since most SoCs are too old, I was able to find all the platforms myself
>>>>> such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it with
>>>>> G12A and S4.
>>>> GXL based board are still very common an easy to come by.
>>>> I'm quite surprised that you are unable to test on this SoC family
>>> The fact of the matter is that the S4 is our end-2020 chip, the G12A is
>>> five years old, and the GXL is seven years old. If you must ask for a
>>> test, I will report this problem to the leadership to coordinate resources.
>>
>> The age of the SoC is irrelevant. SoCs don't get deprecated based on age
>> in mainline. It is not just GXL, same goes for meson8.
>>
>> These SoCs are actively used. Boards with these SoCs are still sold and
>> easily available. See the VIM1 or the Libretech boards.
>>
>> Breaking things for the the users of these SoCs is not acceptable.
>> So yes, looking at your series, I strongly recommend you do more tests.
>>
> You have a point there. Let's go back to the root of the problem. I aim to increase S4. The S4 uses 12MHZ to calculate baud. That's all.
> Change it to CCF as you suggested. The changes are so large that you ask to test all the chips.
> I also mentioned last time that using CCF would lead to a longer drive probe time and affect the board startup time. If this problem is not solved, can we reject the way you suggest using CCF?

I have a much simpler proposal (non-tested):

============><===================================================
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 45e00d928253..eda3fdad60d1 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -76,6 +76,12 @@
  #define AML_UART_POLL_USEC		5
  #define AML_UART_TIMEOUT_USEC		10000

+struct meson_uart_data {
+	struct clk	*clk_pclk;
+	struct clk	*clk_xtal;
+	struct clk	*clk_baud;
+};
+
  static struct uart_driver meson_uart_driver;

  static struct uart_port *meson_ports[AML_UART_PORT_NUM];
@@ -293,16 +299,17 @@ static int meson_uart_startup(struct uart_port *port)

  static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
  {
+	struct meson_uart_data *private_data = port->private_data;
  	u32 val;

  	while (!meson_uart_tx_empty(port))
  		cpu_relax();

-	if (port->uartclk == 24000000) {
-		val = ((port->uartclk / 3) / baud) - 1;
+	if (clk_is_match(private_data->clk_baud, private_data->clk_xtal)) {
+		val = ((clk_get_rate(private_data->clk_baud) / 3) / baud) - 1;
  		val |= AML_UART_BAUD_XTAL;
  	} else {
-		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
+		val = ((clk_get_rate(private_data->clk_baud) * 10 / (baud * 4) + 5) / 10) - 1;
  	}
  	val |= AML_UART_BAUD_USE;
  	writel(val, port->membase + AML_UART_REG5);
@@ -666,31 +673,26 @@ static inline struct clk *meson_uart_probe_clock(struct device *dev,
  }

  static int meson_uart_probe_clocks(struct platform_device *pdev,
-				   struct uart_port *port)
+				   struct meson_uart_data *private_data)
  {
-	struct clk *clk_xtal = NULL;
-	struct clk *clk_pclk = NULL;
-	struct clk *clk_baud = NULL;
-
-	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
-	if (IS_ERR(clk_pclk))
-		return PTR_ERR(clk_pclk);
+	private_data->clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
+	if (IS_ERR(private_data->clk_pclk))
+		return PTR_ERR(private_data->clk_pclk);

-	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
-	if (IS_ERR(clk_xtal))
-		return PTR_ERR(clk_xtal);
+	private_data->clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
+	if (IS_ERR(private_data->clk_xtal))
+		return PTR_ERR(private_data->clk_xtal);

-	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
-	if (IS_ERR(clk_baud))
-		return PTR_ERR(clk_baud);
-
-	port->uartclk = clk_get_rate(clk_baud);
+	private_data->clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
+	if (IS_ERR(private_data->clk_baud))
+		return PTR_ERR(private_data->clk_baud);

  	return 0;
  }

  static int meson_uart_probe(struct platform_device *pdev)
  {
+	struct meson_uart_data *private_data;
  	struct resource *res_mem;
  	struct uart_port *port;
  	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
@@ -714,6 +716,11 @@ static int meson_uart_probe(struct platform_device *pdev)
  	if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
  		return -EINVAL;

+	private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
+				    GFP_KERNEL);
+	if (!private_data)
+		return -ENOMEM;
+
  	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	if (!res_mem)
  		return -ENODEV;
@@ -733,7 +740,7 @@ static int meson_uart_probe(struct platform_device *pdev)
  	if (!port)
  		return -ENOMEM;

-	ret = meson_uart_probe_clocks(pdev, port);
+	ret = meson_uart_probe_clocks(pdev, private_data);
  	if (ret)
  		return ret;

@@ -749,6 +756,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 = private_data;

  	meson_ports[pdev->id] = port;
  	platform_set_drvdata(pdev, port);
============><===================================================

Neil

>>>>
>>>>>
>>>>> Yu Tu (6):
>>>>>     tty: serial: meson: Move request the register region to probe
>>>>>     tty: serial: meson: Use devm_ioremap_resource to get register mapped
>>>>>       memory
>>>>>     tty: serial: meson: Describes the calculation of the UART baud rate
>>>>>       clock using a clock frame
>>>>>     tty: serial: meson: Make some bit of the REG5 register writable
>>>>>     tty: serial: meson: The system stuck when you run the stty command on
>>>>>       the console to change the baud rate
>>>>>     tty: serial: meson: Added S4 SOC compatibility
>>>>>
>>>>> V6 -> V7: To solve the system stuck when you run the stty command on
>>>>> the console to change the baud rate.
>>>>> V5 -> V6: Change error format as discussed in the email.
>>>>> V4 -> V5: Change error format.
>>>>> V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
>>>>> in the email.
>>>>> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change
>>>>> the DTS before it can be deleted
>>>>> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as
>>>>> discussed in the email
>>>>>
>>>>> Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/
>>>>>
>>>>>    drivers/tty/serial/meson_uart.c | 221 ++++++++++++++++++++++----------
>>>>>    1 file changed, 154 insertions(+), 67 deletions(-)
>>>>>
>>>>>
>>>>> base-commit: a603ca60cebff8589882427a67f870ed946b3fc8
>>>>
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[Yu Tu]

Hi Neil,
	Thank you very much for your advice.

On 2022/3/1 17:27, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> On 01/03/2022 10:01, Yu Tu wrote:
>> Hi Jerome,
>>
>> On 2022/3/1 16:36, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>>
>>> On Tue 01 Mar 2022 at 13:54, Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>>> Hi Jerome,
>>>>
>>>> On 2022/2/28 18:59, Jerome Brunet wrote:
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>
>>>>>> Using the common Clock code to describe the UART baud rate
>>>>>> clock makes it easier for the UART driver to be compatible
>>>>>> with the baud rate requirements of the UART IP on different
>>>>>> meson chips. Add Meson S4 SoC compatible.
>>>>>>
>>>>>> The test method:
>>>>>> Start the console and run the following commands in turn:
>>>>>> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.
>>>>>>
>>>>>> Since most SoCs are too old, I was able to find all the platforms 
>>>>>> myself
>>>>>> such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it with
>>>>>> G12A and S4.
>>>>> GXL based board are still very common an easy to come by.
>>>>> I'm quite surprised that you are unable to test on this SoC family
>>>> The fact of the matter is that the S4 is our end-2020 chip, the G12A is
>>>> five years old, and the GXL is seven years old. If you must ask for a
>>>> test, I will report this problem to the leadership to coordinate 
>>>> resources.
>>>
>>> The age of the SoC is irrelevant. SoCs don't get deprecated based on age
>>> in mainline. It is not just GXL, same goes for meson8.
>>>
>>> These SoCs are actively used. Boards with these SoCs are still sold and
>>> easily available. See the VIM1 or the Libretech boards.
>>>
>>> Breaking things for the the users of these SoCs is not acceptable.
>>> So yes, looking at your series, I strongly recommend you do more tests.
>>>
>> You have a point there. Let's go back to the root of the problem. I 
>> aim to increase S4. The S4 uses 12MHZ to calculate baud. That's all.
>> Change it to CCF as you suggested. The changes are so large that you 
>> ask to test all the chips.
>> I also mentioned last time that using CCF would lead to a longer drive 
>> probe time and affect the board startup time. If this problem is not 
>> solved, can we reject the way you suggest using CCF?
> 
> I have a much simpler proposal (non-tested):
> 
I can test it for you. But if you need to add S4 chips. Again, we need 
to add a judgment. Is that acceptable to you?

What we've been talking about for a long time is how to make the least 
changes and the least impact for the S4.

The fact is that Amlogic has recommended 12MHz for baud rates for all 
chips starting with the G12A. I really doubt whether the G12A/B and SM1 
were tested before. So if we want to upload subsequent Amlogic chips, 
not only the S4 needs to solve the 12MHz problem, but other chips can't 
get around this area.
> ============><===================================================
> diff --git a/drivers/tty/serial/meson_uart.c 
> b/drivers/tty/serial/meson_uart.c
> index 45e00d928253..eda3fdad60d1 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -76,6 +76,12 @@
>   #define AML_UART_POLL_USEC        5
>   #define AML_UART_TIMEOUT_USEC        10000
> 
> +struct meson_uart_data {
> +    struct clk    *clk_pclk;
> +    struct clk    *clk_xtal;
> +    struct clk    *clk_baud;
> +};
> +
>   static struct uart_driver meson_uart_driver;
> 
>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
> @@ -293,16 +299,17 @@ static int meson_uart_startup(struct uart_port *port)
> 
>   static void meson_uart_change_speed(struct uart_port *port, unsigned 
> long baud)
>   {
> +    struct meson_uart_data *private_data = port->private_data;
>       u32 val;
> 
>       while (!meson_uart_tx_empty(port))
>           cpu_relax();
> 
> -    if (port->uartclk == 24000000) {
> -        val = ((port->uartclk / 3) / baud) - 1;
> +    if (clk_is_match(private_data->clk_baud, private_data->clk_xtal)) {
> +        val = ((clk_get_rate(private_data->clk_baud) / 3) / baud) - 1;
>           val |= AML_UART_BAUD_XTAL;
>       } else {
> -        val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
> +        val = ((clk_get_rate(private_data->clk_baud) * 10 / (baud * 4) 
> + 5) / 10) - 1;
>       }
>       val |= AML_UART_BAUD_USE;
>       writel(val, port->membase + AML_UART_REG5);
> @@ -666,31 +673,26 @@ static inline struct clk 
> *meson_uart_probe_clock(struct device *dev,
>   }
> 
>   static int meson_uart_probe_clocks(struct platform_device *pdev,
> -                   struct uart_port *port)
> +                   struct meson_uart_data *private_data)
>   {
> -    struct clk *clk_xtal = NULL;
> -    struct clk *clk_pclk = NULL;
> -    struct clk *clk_baud = NULL;
> -
> -    clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
> -    if (IS_ERR(clk_pclk))
> -        return PTR_ERR(clk_pclk);
> +    private_data->clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
> +    if (IS_ERR(private_data->clk_pclk))
> +        return PTR_ERR(private_data->clk_pclk);
> 
> -    clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
> -    if (IS_ERR(clk_xtal))
> -        return PTR_ERR(clk_xtal);
> +    private_data->clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
> +    if (IS_ERR(private_data->clk_xtal))
> +        return PTR_ERR(private_data->clk_xtal);
> 
> -    clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
> -    if (IS_ERR(clk_baud))
> -        return PTR_ERR(clk_baud);
> -
> -    port->uartclk = clk_get_rate(clk_baud);
> +    private_data->clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
> +    if (IS_ERR(private_data->clk_baud))
> +        return PTR_ERR(private_data->clk_baud);
> 
>       return 0;
>   }
> 
>   static int meson_uart_probe(struct platform_device *pdev)
>   {
> +    struct meson_uart_data *private_data;
>       struct resource *res_mem;
>       struct uart_port *port;
>       u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
> @@ -714,6 +716,11 @@ static int meson_uart_probe(struct platform_device 
> *pdev)
>       if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
>           return -EINVAL;
> 
> +    private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
> +                    GFP_KERNEL);
> +    if (!private_data)
> +        return -ENOMEM;
> +
>       res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>       if (!res_mem)
>           return -ENODEV;
> @@ -733,7 +740,7 @@ static int meson_uart_probe(struct platform_device 
> *pdev)
>       if (!port)
>           return -ENOMEM;
> 
> -    ret = meson_uart_probe_clocks(pdev, port);
> +    ret = meson_uart_probe_clocks(pdev, private_data);
>       if (ret)
>           return ret;
> 
> @@ -749,6 +756,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 = private_data;
> 
>       meson_ports[pdev->id] = port;
>       platform_set_drvdata(pdev, port);
> ============><===================================================
> 
> Neil
> 
>>>>>
>>>>>>
>>>>>> Yu Tu (6):
>>>>>>     tty: serial: meson: Move request the register region to probe
>>>>>>     tty: serial: meson: Use devm_ioremap_resource to get register 
>>>>>> mapped
>>>>>>       memory
>>>>>>     tty: serial: meson: Describes the calculation of the UART baud 
>>>>>> rate
>>>>>>       clock using a clock frame
>>>>>>     tty: serial: meson: Make some bit of the REG5 register writable
>>>>>>     tty: serial: meson: The system stuck when you run the stty 
>>>>>> command on
>>>>>>       the console to change the baud rate
>>>>>>     tty: serial: meson: Added S4 SOC compatibility
>>>>>>
>>>>>> V6 -> V7: To solve the system stuck when you run the stty command on
>>>>>> the console to change the baud rate.
>>>>>> V5 -> V6: Change error format as discussed in the email.
>>>>>> V4 -> V5: Change error format.
>>>>>> V3 -> V4: Change CCF to describe the UART baud rate clock as 
>>>>>> discussed
>>>>>> in the email.
>>>>>> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it 
>>>>>> must change
>>>>>> the DTS before it can be deleted
>>>>>> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some 
>>>>>> changes as
>>>>>> discussed in the email
>>>>>>
>>>>>> Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/ 
>>>>>>
>>>>>>
>>>>>>    drivers/tty/serial/meson_uart.c | 221 
>>>>>> ++++++++++++++++++++++----------
>>>>>>    1 file changed, 154 insertions(+), 67 deletions(-)
>>>>>>
>>>>>>
>>>>>> base-commit: a603ca60cebff8589882427a67f870ed946b3fc8
>>>>>
>>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[Yu Tu]

Hi Greg,
	I have no problem with trying to revert this list of submissions. I'm 
sorry about that.

On 2022/3/1 18:12, Yu Tu wrote:
> Hi Neil,
>      Thank you very much for your advice.
> 
> On 2022/3/1 17:27, Neil Armstrong wrote:
>> [ EXTERNAL EMAIL ]
>>
>> Hi,
>>
>> On 01/03/2022 10:01, Yu Tu wrote:
>>> Hi Jerome,
>>>
>>> On 2022/3/1 16:36, Jerome Brunet wrote:
>>>> [ EXTERNAL EMAIL ]
>>>>
>>>>
>>>> On Tue 01 Mar 2022 at 13:54, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>
>>>>> Hi Jerome,
>>>>>
>>>>> On 2022/2/28 18:59, Jerome Brunet wrote:
>>>>>> [ EXTERNAL EMAIL ]
>>>>>>
>>>>>> On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>>
>>>>>>> Using the common Clock code to describe the UART baud rate
>>>>>>> clock makes it easier for the UART driver to be compatible
>>>>>>> with the baud rate requirements of the UART IP on different
>>>>>>> meson chips. Add Meson S4 SoC compatible.
>>>>>>>
>>>>>>> The test method:
>>>>>>> Start the console and run the following commands in turn:
>>>>>>> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.
>>>>>>>
>>>>>>> Since most SoCs are too old, I was able to find all the platforms 
>>>>>>> myself
>>>>>>> such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it 
>>>>>>> with
>>>>>>> G12A and S4.
>>>>>> GXL based board are still very common an easy to come by.
>>>>>> I'm quite surprised that you are unable to test on this SoC family
>>>>> The fact of the matter is that the S4 is our end-2020 chip, the 
>>>>> G12A is
>>>>> five years old, and the GXL is seven years old. If you must ask for a
>>>>> test, I will report this problem to the leadership to coordinate 
>>>>> resources.
>>>>
>>>> The age of the SoC is irrelevant. SoCs don't get deprecated based on 
>>>> age
>>>> in mainline. It is not just GXL, same goes for meson8.
>>>>
>>>> These SoCs are actively used. Boards with these SoCs are still sold and
>>>> easily available. See the VIM1 or the Libretech boards.
>>>>
>>>> Breaking things for the the users of these SoCs is not acceptable.
>>>> So yes, looking at your series, I strongly recommend you do more tests.
>>>>
>>> You have a point there. Let's go back to the root of the problem. I 
>>> aim to increase S4. The S4 uses 12MHZ to calculate baud. That's all.
>>> Change it to CCF as you suggested. The changes are so large that you 
>>> ask to test all the chips.
>>> I also mentioned last time that using CCF would lead to a longer 
>>> drive probe time and affect the board startup time. If this problem 
>>> is not solved, can we reject the way you suggest using CCF?
>>
>> I have a much simpler proposal (non-tested):
>>
> I can test it for you. But if you need to add S4 chips. Again, we need 
> to add a judgment. Is that acceptable to you?
> 
> What we've been talking about for a long time is how to make the least 
> changes and the least impact for the S4.
> 
> The fact is that Amlogic has recommended 12MHz for baud rates for all 
> chips starting with the G12A. I really doubt whether the G12A/B and SM1 
> were tested before. So if we want to upload subsequent Amlogic chips, 
> not only the S4 needs to solve the 12MHz problem, but other chips can't 
> get around this area.
>> ============><===================================================
>> diff --git a/drivers/tty/serial/meson_uart.c 
>> b/drivers/tty/serial/meson_uart.c
>> index 45e00d928253..eda3fdad60d1 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -76,6 +76,12 @@
>>   #define AML_UART_POLL_USEC        5
>>   #define AML_UART_TIMEOUT_USEC        10000
>>
>> +struct meson_uart_data {
>> +    struct clk    *clk_pclk;
>> +    struct clk    *clk_xtal;
>> +    struct clk    *clk_baud;
>> +};
>> +
>>   static struct uart_driver meson_uart_driver;
>>
>>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>> @@ -293,16 +299,17 @@ static int meson_uart_startup(struct uart_port 
>> *port)
>>
>>   static void meson_uart_change_speed(struct uart_port *port, unsigned 
>> long baud)
>>   {
>> +    struct meson_uart_data *private_data = port->private_data;
>>       u32 val;
>>
>>       while (!meson_uart_tx_empty(port))
>>           cpu_relax();
>>
>> -    if (port->uartclk == 24000000) {
>> -        val = ((port->uartclk / 3) / baud) - 1;
>> +    if (clk_is_match(private_data->clk_baud, private_data->clk_xtal)) {
>> +        val = ((clk_get_rate(private_data->clk_baud) / 3) / baud) - 1;
>>           val |= AML_UART_BAUD_XTAL;
>>       } else {
>> -        val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>> +        val = ((clk_get_rate(private_data->clk_baud) * 10 / (baud * 
>> 4) + 5) / 10) - 1;
>>       }
>>       val |= AML_UART_BAUD_USE;
>>       writel(val, port->membase + AML_UART_REG5);
>> @@ -666,31 +673,26 @@ static inline struct clk 
>> *meson_uart_probe_clock(struct device *dev,
>>   }
>>
>>   static int meson_uart_probe_clocks(struct platform_device *pdev,
>> -                   struct uart_port *port)
>> +                   struct meson_uart_data *private_data)
>>   {
>> -    struct clk *clk_xtal = NULL;
>> -    struct clk *clk_pclk = NULL;
>> -    struct clk *clk_baud = NULL;
>> -
>> -    clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
>> -    if (IS_ERR(clk_pclk))
>> -        return PTR_ERR(clk_pclk);
>> +    private_data->clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
>> +    if (IS_ERR(private_data->clk_pclk))
>> +        return PTR_ERR(private_data->clk_pclk);
>>
>> -    clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
>> -    if (IS_ERR(clk_xtal))
>> -        return PTR_ERR(clk_xtal);
>> +    private_data->clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
>> +    if (IS_ERR(private_data->clk_xtal))
>> +        return PTR_ERR(private_data->clk_xtal);
>>
>> -    clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
>> -    if (IS_ERR(clk_baud))
>> -        return PTR_ERR(clk_baud);
>> -
>> -    port->uartclk = clk_get_rate(clk_baud);
>> +    private_data->clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
>> +    if (IS_ERR(private_data->clk_baud))
>> +        return PTR_ERR(private_data->clk_baud);
>>
>>       return 0;
>>   }
>>
>>   static int meson_uart_probe(struct platform_device *pdev)
>>   {
>> +    struct meson_uart_data *private_data;
>>       struct resource *res_mem;
>>       struct uart_port *port;
>>       u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
>> @@ -714,6 +716,11 @@ static int meson_uart_probe(struct 
>> platform_device *pdev)
>>       if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
>>           return -EINVAL;
>>
>> +    private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
>> +                    GFP_KERNEL);
>> +    if (!private_data)
>> +        return -ENOMEM;
>> +
>>       res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       if (!res_mem)
>>           return -ENODEV;
>> @@ -733,7 +740,7 @@ static int meson_uart_probe(struct platform_device 
>> *pdev)
>>       if (!port)
>>           return -ENOMEM;
>>
>> -    ret = meson_uart_probe_clocks(pdev, port);
>> +    ret = meson_uart_probe_clocks(pdev, private_data);
>>       if (ret)
>>           return ret;
>>
>> @@ -749,6 +756,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 = private_data;
>>
>>       meson_ports[pdev->id] = port;
>>       platform_set_drvdata(pdev, port);
>> ============><===================================================
>>
>> Neil
>>
>>>>>>
>>>>>>>
>>>>>>> Yu Tu (6):
>>>>>>>     tty: serial: meson: Move request the register region to probe
>>>>>>>     tty: serial: meson: Use devm_ioremap_resource to get register 
>>>>>>> mapped
>>>>>>>       memory
>>>>>>>     tty: serial: meson: Describes the calculation of the UART 
>>>>>>> baud rate
>>>>>>>       clock using a clock frame
>>>>>>>     tty: serial: meson: Make some bit of the REG5 register writable
>>>>>>>     tty: serial: meson: The system stuck when you run the stty 
>>>>>>> command on
>>>>>>>       the console to change the baud rate
>>>>>>>     tty: serial: meson: Added S4 SOC compatibility
>>>>>>>
>>>>>>> V6 -> V7: To solve the system stuck when you run the stty command on
>>>>>>> the console to change the baud rate.
>>>>>>> V5 -> V6: Change error format as discussed in the email.
>>>>>>> V4 -> V5: Change error format.
>>>>>>> V3 -> V4: Change CCF to describe the UART baud rate clock as 
>>>>>>> discussed
>>>>>>> in the email.
>>>>>>> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it 
>>>>>>> must change
>>>>>>> the DTS before it can be deleted
>>>>>>> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some 
>>>>>>> changes as
>>>>>>> discussed in the email
>>>>>>>
>>>>>>> Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/ 
>>>>>>>
>>>>>>>
>>>>>>>    drivers/tty/serial/meson_uart.c | 221 
>>>>>>> ++++++++++++++++++++++----------
>>>>>>>    1 file changed, 154 insertions(+), 67 deletions(-)
>>>>>>>
>>>>>>>
>>>>>>> base-commit: a603ca60cebff8589882427a67f870ed946b3fc8
>>>>>>
>>>>
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[Neil Armstrong]

Hi,

Le 25/02/2022 à 08:39, Yu Tu a écrit :
> Using the common Clock code to describe the UART baud rate
> clock makes it easier for the UART driver to be compatible
> with the baud rate requirements of the UART IP on different
> meson chips. Add Meson S4 SoC compatible.
> 
> The test method:
> Start the console and run the following commands in turn:
> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.
> 
> Since most SoCs are too old, I was able to find all the platforms myself
> such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it with
> G12A and S4.
> 
> Yu Tu (6):
>    tty: serial: meson: Move request the register region to probe
>    tty: serial: meson: Use devm_ioremap_resource to get register mapped
>      memory
>    tty: serial: meson: Describes the calculation of the UART baud rate
>      clock using a clock frame
>    tty: serial: meson: Make some bit of the REG5 register writable
>    tty: serial: meson: The system stuck when you run the stty command on
>      the console to change the baud rate
>    tty: serial: meson: Added S4 SOC compatibility
> 
> V6 -> V7: To solve the system stuck when you run the stty command on
> the console to change the baud rate.
> V5 -> V6: Change error format as discussed in the email.
> V4 -> V5: Change error format.
> V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
> in the email.
> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change
> the DTS before it can be deleted
> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as
> discussed in the email
> 
> Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/
> 
>   drivers/tty/serial/meson_uart.c | 221 ++++++++++++++++++++++----------
>   1 file changed, 154 insertions(+), 67 deletions(-)
> 
> 
> base-commit: a603ca60cebff8589882427a67f870ed946b3fc8

Could you send the emails To Kevin, Jerome, Martin & me, and put the various lists in CC instead ? otherwise we are not notified when the patch is accepted by the tty maintainer.

Thanks,
Neil


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[Yu Tu]

Hi Neil,

On 2022/3/1 15:25, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> Le 25/02/2022 à 08:39, Yu Tu a écrit :
>> Using the common Clock code to describe the UART baud rate
>> clock makes it easier for the UART driver to be compatible
>> with the baud rate requirements of the UART IP on different
>> meson chips. Add Meson S4 SoC compatible.
>>
>> The test method:
>> Start the console and run the following commands in turn:
>> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.
>>
>> Since most SoCs are too old, I was able to find all the platforms myself
>> such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it with
>> G12A and S4.
>>
>> Yu Tu (6):
>>    tty: serial: meson: Move request the register region to probe
>>    tty: serial: meson: Use devm_ioremap_resource to get register mapped
>>      memory
>>    tty: serial: meson: Describes the calculation of the UART baud rate
>>      clock using a clock frame
>>    tty: serial: meson: Make some bit of the REG5 register writable
>>    tty: serial: meson: The system stuck when you run the stty command on
>>      the console to change the baud rate
>>    tty: serial: meson: Added S4 SOC compatibility
>>
>> V6 -> V7: To solve the system stuck when you run the stty command on
>> the console to change the baud rate.
>> V5 -> V6: Change error format as discussed in the email.
>> V4 -> V5: Change error format.
>> V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
>> in the email.
>> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must 
>> change
>> the DTS before it can be deleted
>> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some 
>> changes as
>> discussed in the email
>>
>> Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/ 
>>
>>
>>   drivers/tty/serial/meson_uart.c | 221 ++++++++++++++++++++++----------
>>   1 file changed, 154 insertions(+), 67 deletions(-)
>>
>>
>> base-commit: a603ca60cebff8589882427a67f870ed946b3fc8
> 
> Could you send the emails To Kevin, Jerome, Martin & me, and put the 
> various lists in CC instead ? otherwise we are not notified when the 
> patch is accepted by the tty maintainer.
The fact is that sending is adding you up, you see
Link: 
https://lore.kernel.org/linux-amlogic/20220225073922.3947-1-yu.tu@amlogic.com/
> 
> Thanks,
> Neil
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[Neil Armstrong]

Hi,

On 01/03/2022 08:57, Yu Tu wrote:
> Hi Neil,
> 
> On 2022/3/1 15:25, Neil Armstrong wrote:
>> [ EXTERNAL EMAIL ]
>>
>> Hi,
>>
>> Le 25/02/2022 à 08:39, Yu Tu a écrit :
>>> Using the common Clock code to describe the UART baud rate
>>> clock makes it easier for the UART driver to be compatible
>>> with the baud rate requirements of the UART IP on different
>>> meson chips. Add Meson S4 SoC compatible.
>>>
>>> The test method:
>>> Start the console and run the following commands in turn:
>>> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.
>>>
>>> Since most SoCs are too old, I was able to find all the platforms myself
>>> such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it with
>>> G12A and S4.
>>>
>>> Yu Tu (6):
>>>    tty: serial: meson: Move request the register region to probe
>>>    tty: serial: meson: Use devm_ioremap_resource to get register mapped
>>>      memory
>>>    tty: serial: meson: Describes the calculation of the UART baud rate
>>>      clock using a clock frame
>>>    tty: serial: meson: Make some bit of the REG5 register writable
>>>    tty: serial: meson: The system stuck when you run the stty command on
>>>      the console to change the baud rate
>>>    tty: serial: meson: Added S4 SOC compatibility
>>>
>>> V6 -> V7: To solve the system stuck when you run the stty command on
>>> the console to change the baud rate.
>>> V5 -> V6: Change error format as discussed in the email.
>>> V4 -> V5: Change error format.
>>> V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
>>> in the email.
>>> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change
>>> the DTS before it can be deleted
>>> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as
>>> discussed in the email
>>>
>>> Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/
>>>
>>>   drivers/tty/serial/meson_uart.c | 221 ++++++++++++++++++++++----------
>>>   1 file changed, 154 insertions(+), 67 deletions(-)
>>>
>>>
>>> base-commit: a603ca60cebff8589882427a67f870ed946b3fc8
>>
>> Could you send the emails To Kevin, Jerome, Martin & me, and put the various lists in CC instead ? otherwise we are not notified when the patch is accepted by the tty maintainer.
> The fact is that sending is adding you up, you see
> Link: https://lore.kernel.org/linux-amlogic/20220225073922.3947-1-yu.tu@amlogic.com/

It's not the point, the order of recipient in Cc and To is important since they are used
by maintainers to track patch for review and to notify when patches are taken in their tree.

So please make sure the recipients are correctly set before sending the patches.

Neil

>>
>> Thanks,
>> Neil
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V7 0/6] Use CCF to describe the UART baud rate clock

[Yu Tu]

Hi Neil,

On 2022/3/1 16:37, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> On 01/03/2022 08:57, Yu Tu wrote:
>> Hi Neil,
>>
>> On 2022/3/1 15:25, Neil Armstrong wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Hi,
>>>
>>> Le 25/02/2022 à 08:39, Yu Tu a écrit :
>>>> Using the common Clock code to describe the UART baud rate
>>>> clock makes it easier for the UART driver to be compatible
>>>> with the baud rate requirements of the UART IP on different
>>>> meson chips. Add Meson S4 SoC compatible.
>>>>
>>>> The test method:
>>>> Start the console and run the following commands in turn:
>>>> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.
>>>>
>>>> Since most SoCs are too old, I was able to find all the platforms 
>>>> myself
>>>> such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it with
>>>> G12A and S4.
>>>>
>>>> Yu Tu (6):
>>>>    tty: serial: meson: Move request the register region to probe
>>>>    tty: serial: meson: Use devm_ioremap_resource to get register mapped
>>>>      memory
>>>>    tty: serial: meson: Describes the calculation of the UART baud rate
>>>>      clock using a clock frame
>>>>    tty: serial: meson: Make some bit of the REG5 register writable
>>>>    tty: serial: meson: The system stuck when you run the stty 
>>>> command on
>>>>      the console to change the baud rate
>>>>    tty: serial: meson: Added S4 SOC compatibility
>>>>
>>>> V6 -> V7: To solve the system stuck when you run the stty command on
>>>> the console to change the baud rate.
>>>> V5 -> V6: Change error format as discussed in the email.
>>>> V4 -> V5: Change error format.
>>>> V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
>>>> in the email.
>>>> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must 
>>>> change
>>>> the DTS before it can be deleted
>>>> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some 
>>>> changes as
>>>> discussed in the email
>>>>
>>>> Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/ 
>>>>
>>>>
>>>>   drivers/tty/serial/meson_uart.c | 221 
>>>> ++++++++++++++++++++++----------
>>>>   1 file changed, 154 insertions(+), 67 deletions(-)
>>>>
>>>>
>>>> base-commit: a603ca60cebff8589882427a67f870ed946b3fc8
>>>
>>> Could you send the emails To Kevin, Jerome, Martin & me, and put the 
>>> various lists in CC instead ? otherwise we are not notified when the 
>>> patch is accepted by the tty maintainer.
>> The fact is that sending is adding you up, you see
>> Link: 
>> https://lore.kernel.org/linux-amlogic/20220225073922.3947-1-yu.tu@amlogic.com/ 
>>
> 
> It's not the point, the order of recipient in Cc and To is important 
> since they are used
> by maintainers to track patch for review and to notify when patches are 
> taken in their tree.
> 
> So please make sure the recipients are correctly set before sending the 
> patches.
> 
I see what you mean. Mailing lists should all use To.
> Neil
> 
>>>
>>> Thanks,
>>> Neil
>>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel