[PATCH V6 0/5] Use CCF to describe the UART baud rate clock

[PATCH V6 0/5] 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.

Yu Tu (5):
  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: Added S4 SOC compatibility

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/20220110104214.25321-4-yu.tu@amlogic.com/

 drivers/tty/serial/meson_uart.c | 217 ++++++++++++++++++++++----------
 1 file changed, 149 insertions(+), 68 deletions(-)


base-commit: 4d66020dcef83314092f2c8c89152a8d122627e2
-- 
2.33.1


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

[PATCH V6 1/5] 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>
---
 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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH V6 1/5] tty: serial: meson: Move request the register region to probe

[Jiri Slaby]

On 18. 01. 22, 4:09, Yu Tu wrote:
> 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>

thanks,
-- 
js
suse labs

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

[PATCH V6 2/5] 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>
---
 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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH V6 2/5] tty: serial: meson: Use devm_ioremap_resource to get register mapped memory

[Jiri Slaby]

On 18. 01. 22, 4:09, Yu Tu wrote:
> 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>

thanks,
-- 
js

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

[PATCH V6 3/5] 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 | 195 +++++++++++++++++++++++---------
 1 file changed, 142 insertions(+), 53 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 7570958d010c..92fa91c825e6 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,105 @@ 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);
-	}
-
-	devm_add_action_or_reset(dev,
-			(void(*)(void *))clk_disable_unprepare,
-			clk);
-
-	return clk;
-}
+static struct clk_div_table xtal_div_table[] = {
+	{0, 3},
+	{1, 1},
+	{2, 2},
+	{3, 2},
+};
 
-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;
+	char clk_name[32];
+	struct clk_parent_data use_xtal_mux_parents[2] = {
+		{ .index = -1, },
+		{ .index = -1, },
+	};
 
-	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");
+
+	if (private_data->use_xtal_clk) {
+		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);
+
+		use_xtal_mux_parents[1].hw = hw;
+	} else {
+		snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
+			 "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(hw))
+			return PTR_ERR(hw);
+
+		use_xtal_mux_parents[0].hw = 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,
+					ARRAY_SIZE(use_xtal_mux_parents),
+					NULL, NULL,
+					use_xtal_mux_parents,
+					CLK_SET_RATE_PARENT,
+					port->membase + AML_UART_REG5,
+					24, 0x1,
+					CLK_MUX_READ_ONLY,
+					NULL, 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 +764,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 +792,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 +812,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 +843,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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Jiri Slaby]

On 18. 01. 22, 4:09, 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.
...
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
...
> @@ -629,57 +640,105 @@ 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);
> -	}
> -
> -	devm_add_action_or_reset(dev,
> -			(void(*)(void *))clk_disable_unprepare,
> -			clk);
> -
> -	return clk;
> -}
> +static struct clk_div_table xtal_div_table[] = {

This can be const, right?

> +	{0, 3},
> +	{1, 1},
> +	{2, 2},
> +	{3, 2},

Not sure if you didn't remove too much whitespace. I think it should be 
like: "{ 0, 3 },". But I actually don't care, it's a minor thing.

I cannot comment on the rest (clk and OF part) as my knowledge is pretty 
limited there. Leaving up to others.

thanks,
-- 
js
suse labs

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

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

[Yu Tu]

Hi Jiri,
	Thank you very much for your patient reply.I learned a lot from your 
response.

On 2022/1/18 17:39, Jiri Slaby wrote:
> [ EXTERNAL EMAIL ]
> 
> On 18. 01. 22, 4:09, 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.
> ...
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
> ...
>> @@ -629,57 +640,105 @@ 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);
>> -    }
>> -
>> -    devm_add_action_or_reset(dev,
>> -            (void(*)(void *))clk_disable_unprepare,
>> -            clk);
>> -
>> -    return clk;
>> -}
>> +static struct clk_div_table xtal_div_table[] = {
> 
> This can be const, right?
You are right.
> 
>> +    {0, 3},
>> +    {1, 1},
>> +    {2, 2},
>> +    {3, 2},
> 
> Not sure if you didn't remove too much whitespace. I think it should be 
> like: "{ 0, 3 },". But I actually don't care, it's a minor thing.
> 
Ok, I will correct it if it needs to be changed.
> I cannot comment on the rest (clk and OF part) as my knowledge is pretty 
> limited there. Leaving up to others.
> 
Anyway, thanks for your reply.
> thanks,

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

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

[Jerome Brunet]

On Tue 18 Jan 2022 at 10:39, Jiri Slaby <jirislaby@kernel.org> wrote:

> On 18. 01. 22, 4:09, 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.
> ...
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
> ...
>> @@ -629,57 +640,105 @@ 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);
>> -	}
>> -
>> -	devm_add_action_or_reset(dev,
>> -			(void(*)(void *))clk_disable_unprepare,
>> -			clk);
>> -
>> -	return clk;
>> -}
>> +static struct clk_div_table xtal_div_table[] = {
>
> This can be const, right?
>
>> +	{0, 3},
>> +	{1, 1},
>> +	{2, 2},
>> +	{3, 2},
>
> Not sure if you didn't remove too much whitespace. I think it should be
> like: "{ 0, 3 },". But I actually don't care, it's a minor thing.

Seconds
It worth fixing in the next version

>
> I cannot comment on the rest (clk and OF part) as my knowledge is pretty
> limited there. Leaving up to others.
>
> thanks,


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

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

[Yu Tu]

On 2022/1/21 5:48, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Tue 18 Jan 2022 at 10:39, Jiri Slaby <jirislaby@kernel.org> wrote:
> 
>> On 18. 01. 22, 4:09, 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.
>> ...
>>> --- a/drivers/tty/serial/meson_uart.c
>>> +++ b/drivers/tty/serial/meson_uart.c
>> ...
>>> @@ -629,57 +640,105 @@ 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);
>>> -	}
>>> -
>>> -	devm_add_action_or_reset(dev,
>>> -			(void(*)(void *))clk_disable_unprepare,
>>> -			clk);
>>> -
>>> -	return clk;
>>> -}
>>> +static struct clk_div_table xtal_div_table[] = {
>>
>> This can be const, right?
>>
>>> +	{0, 3},
>>> +	{1, 1},
>>> +	{2, 2},
>>> +	{3, 2},
>>
>> Not sure if you didn't remove too much whitespace. I think it should be
>> like: "{ 0, 3 },". But I actually don't care, it's a minor thing.
> 
> Seconds
> It worth fixing in the next version
> 
I will correct in a version.
>>
>> I cannot comment on the rest (clk and OF part) as my knowledge is pretty
>> limited there. Leaving up to others.
>>
>> thanks,
> 

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

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

[Jerome Brunet]

On Tue 18 Jan 2022 at 11:09, 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 | 195 +++++++++++++++++++++++---------
>  1 file changed, 142 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 7570958d010c..92fa91c825e6 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,105 @@ 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);
> -	}
> -
> -	devm_add_action_or_reset(dev,
> -			(void(*)(void *))clk_disable_unprepare,
> -			clk);
> -
> -	return clk;
> -}
> +static struct clk_div_table xtal_div_table[] = {
> +	{0, 3},
> +	{1, 1},
> +	{2, 2},
> +	{3, 2},
> +};
>  
> -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;
> +	char clk_name[32];
> +	struct clk_parent_data use_xtal_mux_parents[2] = {
> +		{ .index = -1, },
> +		{ .index = -1, },
> +	};

You are using hw pointers later, you don't need to init the index to -1
I think

>  
> -	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");
> +
> +	if (private_data->use_xtal_clk) {
> +		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);
> +
> +		use_xtal_mux_parents[1].hw = hw;
> +	} else {
> +		snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
> +			 "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(hw))
> +			return PTR_ERR(hw);
> +
> +		use_xtal_mux_parents[0].hw = hw;
> +	}

The above is still wrong.

use_xtal_mux_parents initialize both parent to nothing
And you init the parent in the conditional above.
It is means only one path is actually set instead of both.

The mux always has 2 sources - Both should be set regardless of the HW version
You just add
* /4 on path 0 on legacy SoC
* the funky divider on path 1 on newer SoC.

>  
> -	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,
> +					ARRAY_SIZE(use_xtal_mux_parents),
> +					NULL, NULL,
> +					use_xtal_mux_parents,
> +					CLK_SET_RATE_PARENT,
> +					port->membase + AML_UART_REG5,
> +					24, 0x1,
> +					CLK_MUX_READ_ONLY,
> +					NULL, 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 +764,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;
> +

I think this is unbalanced. 

>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
> @@ -724,9 +792,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 +812,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 +843,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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Yu Tu]

Hi Jerome ,
	Thank you very much for your reply.
	At present, it is found that after using CCF to describe baud rate 
clock, when using stty to change baud rate, the situation will be stuck. 
  If initcall_debug is enabled, the initialization of the UART driver 
takes longer . Shall we discuss this first and change it as you suggest? 
I have replied to Kevin about this question yesterday.
Do you need to discuss it? What should I do next?

On 2022/1/21 5:40, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Tue 18 Jan 2022 at 11:09, 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 | 195 +++++++++++++++++++++++---------
>>   1 file changed, 142 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index 7570958d010c..92fa91c825e6 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,105 @@ 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);
>> -	}
>> -
>> -	devm_add_action_or_reset(dev,
>> -			(void(*)(void *))clk_disable_unprepare,
>> -			clk);
>> -
>> -	return clk;
>> -}
>> +static struct clk_div_table xtal_div_table[] = {
>> +	{0, 3},
>> +	{1, 1},
>> +	{2, 2},
>> +	{3, 2},
>> +};
>>   
>> -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;
>> +	char clk_name[32];
>> +	struct clk_parent_data use_xtal_mux_parents[2] = {
>> +		{ .index = -1, },
>> +		{ .index = -1, },
>> +	};
> 
> You are using hw pointers later, you don't need to init the index to -1
> I think
> 
>>   
>> -	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");
>> +
>> +	if (private_data->use_xtal_clk) {
>> +		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);
>> +
>> +		use_xtal_mux_parents[1].hw = hw;
>> +	} else {
>> +		snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>> +			 "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(hw))
>> +			return PTR_ERR(hw);
>> +
>> +		use_xtal_mux_parents[0].hw = hw;
>> +	}
> 
> The above is still wrong.
> 
> use_xtal_mux_parents initialize both parent to nothing
> And you init the parent in the conditional above.
> It is means only one path is actually set instead of both.
> 
> The mux always has 2 sources - Both should be set regardless of the HW version
> You just add
> * /4 on path 0 on legacy SoC
> * the funky divider on path 1 on newer SoC.
> 
>>   
>> -	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,
>> +					ARRAY_SIZE(use_xtal_mux_parents),
>> +					NULL, NULL,
>> +					use_xtal_mux_parents,
>> +					CLK_SET_RATE_PARENT,
>> +					port->membase + AML_UART_REG5,
>> +					24, 0x1,
>> +					CLK_MUX_READ_ONLY,
>> +					NULL, 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 +764,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;
>> +
> 
> I think this is unbalanced.
> 
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq < 0)
>>   		return irq;
>> @@ -724,9 +792,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 +812,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 +843,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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Yu Tu]

Hi Jerome,
     Thank you very much for your reply. At present, the problem of 
switching UART baud rate stuck has been solved. I'm ready to send the 
next edition. However, I still don't understand your suggestion and need 
to communicate with you.

On 2022/1/21 5:40, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Tue 18 Jan 2022 at 11:09, 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 | 195 +++++++++++++++++++++++---------
>>   1 file changed, 142 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index 7570958d010c..92fa91c825e6 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,105 @@ 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);
>> -	}
>> -
>> -	devm_add_action_or_reset(dev,
>> -			(void(*)(void *))clk_disable_unprepare,
>> -			clk);
>> -
>> -	return clk;
>> -}
>> +static struct clk_div_table xtal_div_table[] = {
>> +	{0, 3},
>> +	{1, 1},
>> +	{2, 2},
>> +	{3, 2},
>> +};
>>   
>> -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;
>> +	char clk_name[32];
>> +	struct clk_parent_data use_xtal_mux_parents[2] = {
>> +		{ .index = -1, },
>> +		{ .index = -1, },
>> +	};
> 
> You are using hw pointers later, you don't need to init the index to -1
> I think
> 
I'm going to delete them in the next version.
like: struct clk_parent_data use_xtal_mux_parents[2] = { };
>>   
>> -	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");
>> +
>> +	if (private_data->use_xtal_clk) {
>> +		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);
>> +
>> +		use_xtal_mux_parents[1].hw = hw;
>> +	} else {
>> +		snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>> +			 "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(hw))
>> +			return PTR_ERR(hw);
>> +
>> +		use_xtal_mux_parents[0].hw = hw;
>> +	}
> 
> The above is still wrong.
> 
> use_xtal_mux_parents initialize both parent to nothing
> And you init the parent in the conditional above.
> It is means only one path is actually set instead of both.
> 
> The mux always has 2 sources - Both should be set regardless of the HW version
> You just add
> * /4 on path 0 on legacy SoC
> * the funky divider on path 1 on newer SoC.
> 
You do have a point, so I was wondering if you had any suggestions.
>>   
>> -	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,
>> +					ARRAY_SIZE(use_xtal_mux_parents),
>> +					NULL, NULL,
>> +					use_xtal_mux_parents,
>> +					CLK_SET_RATE_PARENT,
>> +					port->membase + AML_UART_REG5,
>> +					24, 0x1,
>> +					CLK_MUX_READ_ONLY,
>> +					NULL, 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 +764,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;
>> +
> 
> I think this is unbalanced.
I don't understand what you mean. How should I change it? Can you tell 
me more specifically.
> 
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq < 0)
>>   		return irq;
>> @@ -724,9 +792,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 +812,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 +843,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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Yu Tu]

The UART_REG5 register defaults to 0. The console port is set in
ROMCODE. But other UART ports default to 0, so make bit24 and
bit[26,27] writable so that the UART can choose a more
appropriate clock.

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 92fa91c825e6..4e7b2b38ab0a 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -678,7 +678,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,
 					NULL, NULL);
 	if (IS_ERR(hw))
 		return PTR_ERR(hw);
-- 
2.33.1


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

Re: [PATCH V6 4/5] tty: serial: meson: Make some bit of the REG5 register writable

[Jerome Brunet]

On Tue 18 Jan 2022 at 11:09, Yu Tu <yu.tu@amlogic.com> wrote:

> The UART_REG5 register defaults to 0. The console port is set in
> ROMCODE. But other UART ports default to 0, so make bit24 and
> bit[26,27] writable so that the UART can choose a more
> appropriate clock.

Suggestion: Instead of talking bits (which is a bit cryptic) tell us
what is actually does

Something like:
 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 92fa91c825e6..4e7b2b38ab0a 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -678,7 +678,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,
>  					NULL, NULL);
>  	if (IS_ERR(hw))
>  		return PTR_ERR(hw);


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

Re: [PATCH V6 4/5] tty: serial: meson: Make some bit of the REG5 register writable

[Yu Tu]

Hi Jerome,
	Thank you very much for your reply. At present, the problem of 
switching uART baud rate stuck has been solved. I'm ready to send the 
next edition.

On 2022/1/21 5:49, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Tue 18 Jan 2022 at 11:09, Yu Tu <yu.tu@amlogic.com> wrote:
> 
>> The UART_REG5 register defaults to 0. The console port is set in
>> ROMCODE. But other UART ports default to 0, so make bit24 and
>> bit[26,27] writable so that the UART can choose a more
>> appropriate clock.
> 
> Suggestion: Instead of talking bits (which is a bit cryptic) tell us
> what is actually does
> 
> Something like:
>   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
> 
Your description is better, I will follow your suggestion in the next 
edition.
>>
>> 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 92fa91c825e6..4e7b2b38ab0a 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -678,7 +678,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,
>>   					NULL, NULL);
>>   	if (IS_ERR(hw))
>>   		return PTR_ERR(hw);
> 

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

[PATCH V6 5/5] 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 4e7b2b38ab0a..af95a4676d28 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -859,6 +859,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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Kevin Hilman]

Hello,

Yu Tu <yu.tu@amlogic.com> writes:

> 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.

Could you describe how this was tested and on which SoCs?  There seem to
be some changes in this series that might affect previous SoCs.

Thanks,

Kevin



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

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

[Yu Tu]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

Hi Kevin,
	Thank you very much for your reply.

On 2022/1/20 6:37, Kevin Hilman wrote:
> [ EXTERNAL EMAIL ]
> 
> Hello,
> 
> Yu Tu <yu.tu@amlogic.com> writes:
> 
>> 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.
> 
> Could you describe how this was tested and on which SoCs?  There seem to
> be some changes in this series that might affect previous SoCs.
> 
For me, the board starts normally and prints. My intention was to add 
the S4 SOC UART compatible, but for the S4 our baud rate clock is 
calculated at 12MHz by default.So a series of changes were made at your 
suggestion.

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.But when I talked to Martin earlier he tried meson8b's log.
The test patch is in the attachment.

I have found that on some boards with this change, the initcall_debug 
Uart driver takes longer to initialize. Running the stty command to 
change the baud rate at the same time may cause a jam.

I'd love to know what else you suggest.

> Thanks,
> 
> Kevin
> 
> 

[-- Attachment #2: g12a-clk-debug-output --]
[-- Type: text/plain, Size: 1105 bytes --]

# cat /sys/kernel/debug/clk//clk_summary | head
                                 enable  prepare  protect                                duty  hardware
   clock                          count    count    count        rate   accuracy phase  cycle    enable
-------------------------------------------------------------------------------------------------------
 xtal                                 6        6        0    24000000          0     0  50000         Y
    ff803000.serial#xtal_div          1        1        0    12000000          0     0  50000         Y
       ff803000.serial#use_xtal       1        1        0    12000000          0     0  50000         Y
          ff803000.serial#baud_div       1        1        0      115385          0     0  50000         Y
    cts_oscin                         0        0        0    24000000          0     0  50000         Y
       g12a_ao_cec_pre                0        0        0    24000000          0     0  50000         N
          g12a_ao_cec_div             0        0        0       32742          0     0  50000         Y

[-- Attachment #3: clk-debug-output.txt --]
[-- Type: text/plain, Size: 1702 bytes --]

# cat /sys/kernel/debug/clk/clk_summary 
                                 enable  prepare  protect                                duty  hardware
   clock                          count    count    count        rate   accuracy phase  cycle    enable
-------------------------------------------------------------------------------------------------------
[...]
 xtal                                 6        6        2    24000000          0     0  50000         Y
[...]
    c81004c0.serial#xtal_div3         0        0        0     8000000          0     0  50000         Y
[...]
    fixed_pll_dco                     1        1        0  2550000000          0     0  50000         Y
       fixed_pll                      1        1        0  2550000000          0     0  50000         Y
[...]
          fclk_div3_div               1        1        0   850000000          0     0  50000         Y
             fclk_div3                2        2        0   850000000          0     0  50000         Y
[...]
                mpeg_clk_sel          1        1        0   850000000          0     0  50000         Y
                   mpeg_clk_div       1        1        0   141666667          0     0  50000         Y
                      clk81          17       20        0   141666667          0     0  50000         Y
[...]
                         c81004c0.serial#clk81_div4       1        1        0    35416666          0     0  50000         Y
                            c81004c0.serial#use_xtal       1        1        0    35416666          0     0  50000         Y
                               c81004c0.serial#baud_div       1        1        0      115364          0     0  50000         Y

[-- Attachment #4: s4-clk-debug-output --]
[-- Type: text/plain, Size: 1104 bytes --]

# cat /sys/kernel/debug/clk/clk_summary | head
                                 enable  prepare  protect                                duty  hardware
   clock                          count    count    count        rate   accuracy phase  cycle    enable
-------------------------------------------------------------------------------------------------------
 xtal                                 7        7        0    24000000          0     0  50000         Y
    fe07a000.serial#xtal_div          1        1        0    12000000          0     0  50000         Y
       fe07a000.serial#use_xtal       1        1        0    12000000          0     0  50000         Y
          fe07a000.serial#baud_div       1        1        0      923077          0     0  50000         Y
    hdcp22_skpclk_mux                 0        0        0    24000000          0     0  50000         Y
       hdcp22_skpclk_div              0        0        0    24000000          0     0  50000         Y
          hdcp22_skpclk_gate          0        0        0    24000000          0     0  50000         N

[-- Attachment #5: Type: text/plain, Size: 167 bytes --]

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

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

[Kevin Hilman]

Yu Tu <yu.tu@amlogic.com> writes:

> Hi Kevin,
> 	Thank you very much for your reply.
>
> On 2022/1/20 6:37, Kevin Hilman wrote:
>> [ EXTERNAL EMAIL ]
>> 
>> Hello,
>> 
>> Yu Tu <yu.tu@amlogic.com> writes:
>> 
>>> 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.
>> 
>> Could you describe how this was tested and on which SoCs?  There seem to
>> be some changes in this series that might affect previous SoCs.
>> 
> For me, the board starts normally and prints. My intention was to add 
> the S4 SOC UART compatible, but for the S4 our baud rate clock is 
> calculated at 12MHz by default.So a series of changes were made at your 
> suggestion.
>
> 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.But when I talked to Martin earlier he tried meson8b's log.
> The test patch is in the attachment.
>
> I have found that on some boards with this change, the initcall_debug 
> Uart driver takes longer to initialize. Running the stty command to 
> change the baud rate at the same time may cause a jam.

This kind of detail is important to document in the cover letter,
including a bit more detail on how to reproduce so that other can help
test or may have ideas for how to solve.

> I'd love to know what else you suggest.

I don't expect you to be able to test on all SoCs, but just to list what
SoCs and which boards you tested on.  This way, those who have other
boards can help test and we can have a better idea of how this was
tested before merging.

Thanks,

Kevin

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

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

[Yu Tu]

  Hi Kevin,
	First of all,thank you very much for your reply.Due to the Chinese 
Spring Festival holiday recently, so i just reply to you now。

On 2022/1/25 3:58, Kevin Hilman wrote:
> [ EXTERNAL EMAIL ]
> 
> Yu Tu <yu.tu@amlogic.com> writes:
> 
>> Hi Kevin,
>> 	Thank you very much for your reply.
>>
>> On 2022/1/20 6:37, Kevin Hilman wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Hello,
>>>
>>> Yu Tu <yu.tu@amlogic.com> writes:
>>>
>>>> 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.
>>>
>>> Could you describe how this was tested and on which SoCs?  There seem to
>>> be some changes in this series that might affect previous SoCs.
>>>
>> For me, the board starts normally and prints. My intention was to add
>> the S4 SOC UART compatible, but for the S4 our baud rate clock is
>> calculated at 12MHz by default.So a series of changes were made at your
>> suggestion.
>>
>> 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.But when I talked to Martin earlier he tried meson8b's log.
>> The test patch is in the attachment.
>>
>> I have found that on some boards with this change, the initcall_debug
>> Uart driver takes longer to initialize. Running the stty command to
>> change the baud rate at the same time may cause a jam.
> 
> This kind of detail is important to document in the cover letter,
> including a bit more detail on how to reproduce so that other can help
> test or may have ideas for how to solve.
> 
The problem recurrence method is described below:
First, add the patch I changed. It then launches normally to the console 
command line.  Finally, run the stty command to change buad rate. The 
detailed commands are as follows:
stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600 .Alternate 
execution can reproduce.

>> I'd love to know what else you suggest.
> 
> I don't expect you to be able to test on all SoCs, but just to list what
> SoCs and which boards you tested on.  This way, those who have other
> boards can help test and we can have a better idea of how this was
> tested before merging.
> 
I only have S4 and G12A so far, so I've only tested on those two platforms.

> Thanks,
> 
> Kevin
> 

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