[PATCH 0/3] the UART driver compatible with the Amlogic Meson S4 SoC

[PATCH 0/3] the UART driver compatible with the Amlogic Meson S4 SoC

[Yu Tu]

The UART driver compatible with the Amlogic Meson S4 SoC on-chip, change the
UART interrupt interface function while adding IRQF_SHARED flag. And add clear
AML_UART_TX_EN bit in meson_uart_shutdown funtion.

Yu Tu (3):
  tty: serial: meson: modify request_irq and free_irq
  tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit
  tty: serial: meson: add UART driver compatible with S4 SoC on-chip

Link:https://patchwork.kernel.org/project/linux-amlogic/patch/20211206100200.31914-1-xianwei.zhao@amlogic.com/

 drivers/tty/serial/meson_uart.c | 70 +++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 12 deletions(-)

-- 
2.33.1


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

[PATCH 0/3] the UART driver compatible with the Amlogic Meson S4 SoC

[Yu Tu]

The UART driver compatible with the Amlogic Meson S4 SoC on-chip, change the
UART interrupt interface function while adding IRQF_SHARED flag. And add clear
AML_UART_TX_EN bit in meson_uart_shutdown funtion.

Yu Tu (3):
  tty: serial: meson: modify request_irq and free_irq
  tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit
  tty: serial: meson: add UART driver compatible with S4 SoC on-chip

Link:https://patchwork.kernel.org/project/linux-amlogic/patch/20211206100200.31914-1-xianwei.zhao@amlogic.com/

 drivers/tty/serial/meson_uart.c | 70 +++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 12 deletions(-)

-- 
2.33.1


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

[PATCH 0/3] the UART driver compatible with the Amlogic Meson S4 SoC

[Yu Tu]

The UART driver compatible with the Amlogic Meson S4 SoC on-chip, change the
UART interrupt interface function while adding IRQF_SHARED flag. And add clear
AML_UART_TX_EN bit in meson_uart_shutdown funtion.

Yu Tu (3):
  tty: serial: meson: modify request_irq and free_irq
  tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit
  tty: serial: meson: add UART driver compatible with S4 SoC on-chip

Link:https://patchwork.kernel.org/project/linux-amlogic/patch/20211206100200.31914-1-xianwei.zhao@amlogic.com/

 drivers/tty/serial/meson_uart.c | 70 +++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 12 deletions(-)

-- 
2.33.1

[PATCH 1/3] tty: serial: meson: modify request_irq and free_irq

[Yu Tu]

Change request_irq to devm_request_irq and free_irq to devm_free_irq.
It's better to change the code this way.

The IRQF_SHARED interrupt flag was added because an interrupt error was
detected when the serial port was opened twice in a row on the project.

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

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index d2c08b760f83..02fafb8229d2 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -121,7 +121,7 @@ static void meson_uart_shutdown(struct uart_port *port)
 	unsigned long flags;
 	u32 val;
 
-	free_irq(port->irq, port);
+	devm_free_irq(port->dev, port->irq, port);
 
 	spin_lock_irqsave(&port->lock, flags);
 
@@ -287,8 +287,8 @@ static int meson_uart_startup(struct uart_port *port)
 	val = (AML_UART_RECV_IRQ(1) | AML_UART_XMIT_IRQ(port->fifosize / 2));
 	writel(val, port->membase + AML_UART_MISC);
 
-	ret = request_irq(port->irq, meson_uart_interrupt, 0,
-			  port->name, port);
+	ret = devm_request_irq(port->dev, port->irq, meson_uart_interrupt,
+			       IRQF_SHARED, port->name, port);
 
 	return ret;
 }
-- 
2.33.1


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

[PATCH 1/3] tty: serial: meson: modify request_irq and free_irq

[Yu Tu]

Change request_irq to devm_request_irq and free_irq to devm_free_irq.
It's better to change the code this way.

The IRQF_SHARED interrupt flag was added because an interrupt error was
detected when the serial port was opened twice in a row on the project.

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

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index d2c08b760f83..02fafb8229d2 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -121,7 +121,7 @@ static void meson_uart_shutdown(struct uart_port *port)
 	unsigned long flags;
 	u32 val;
 
-	free_irq(port->irq, port);
+	devm_free_irq(port->dev, port->irq, port);
 
 	spin_lock_irqsave(&port->lock, flags);
 
@@ -287,8 +287,8 @@ static int meson_uart_startup(struct uart_port *port)
 	val = (AML_UART_RECV_IRQ(1) | AML_UART_XMIT_IRQ(port->fifosize / 2));
 	writel(val, port->membase + AML_UART_MISC);
 
-	ret = request_irq(port->irq, meson_uart_interrupt, 0,
-			  port->name, port);
+	ret = devm_request_irq(port->dev, port->irq, meson_uart_interrupt,
+			       IRQF_SHARED, port->name, port);
 
 	return 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 1/3] tty: serial: meson: modify request_irq and free_irq

[Yu Tu]

Change request_irq to devm_request_irq and free_irq to devm_free_irq.
It's better to change the code this way.

The IRQF_SHARED interrupt flag was added because an interrupt error was
detected when the serial port was opened twice in a row on the project.

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

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index d2c08b760f83..02fafb8229d2 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -121,7 +121,7 @@ static void meson_uart_shutdown(struct uart_port *port)
 	unsigned long flags;
 	u32 val;
 
-	free_irq(port->irq, port);
+	devm_free_irq(port->dev, port->irq, port);
 
 	spin_lock_irqsave(&port->lock, flags);
 
@@ -287,8 +287,8 @@ static int meson_uart_startup(struct uart_port *port)
 	val = (AML_UART_RECV_IRQ(1) | AML_UART_XMIT_IRQ(port->fifosize / 2));
 	writel(val, port->membase + AML_UART_MISC);
 
-	ret = request_irq(port->irq, meson_uart_interrupt, 0,
-			  port->name, port);
+	ret = devm_request_irq(port->dev, port->irq, meson_uart_interrupt,
+			       IRQF_SHARED, port->name, port);
 
 	return ret;
 }
-- 
2.33.1

[PATCH 2/3] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit

[Yu Tu]

The meson_uart_shutdown function should have the opposite operation to
the meson_uart_startup function, so the shutdown of AML_UART_TX_EN is
logically missing.

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

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 02fafb8229d2..69450a461c48 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -126,8 +126,7 @@ static void meson_uart_shutdown(struct uart_port *port)
 	spin_lock_irqsave(&port->lock, flags);
 
 	val = readl(port->membase + AML_UART_CONTROL);
-	val &= ~AML_UART_RX_EN;
-	val &= ~(AML_UART_RX_INT_EN | AML_UART_TX_INT_EN);
+	val &= ~(AML_UART_RX_EN | AML_UART_TX_EN);
 	writel(val, port->membase + AML_UART_CONTROL);
 
 	spin_unlock_irqrestore(&port->lock, flags);
-- 
2.33.1


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

[PATCH 2/3] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit

[Yu Tu]

The meson_uart_shutdown function should have the opposite operation to
the meson_uart_startup function, so the shutdown of AML_UART_TX_EN is
logically missing.

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

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 02fafb8229d2..69450a461c48 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -126,8 +126,7 @@ static void meson_uart_shutdown(struct uart_port *port)
 	spin_lock_irqsave(&port->lock, flags);
 
 	val = readl(port->membase + AML_UART_CONTROL);
-	val &= ~AML_UART_RX_EN;
-	val &= ~(AML_UART_RX_INT_EN | AML_UART_TX_INT_EN);
+	val &= ~(AML_UART_RX_EN | AML_UART_TX_EN);
 	writel(val, port->membase + AML_UART_CONTROL);
 
 	spin_unlock_irqrestore(&port->lock, flags);
-- 
2.33.1


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

[PATCH 2/3] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit

[Yu Tu]

The meson_uart_shutdown function should have the opposite operation to
the meson_uart_startup function, so the shutdown of AML_UART_TX_EN is
logically missing.

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

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 02fafb8229d2..69450a461c48 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -126,8 +126,7 @@ static void meson_uart_shutdown(struct uart_port *port)
 	spin_lock_irqsave(&port->lock, flags);
 
 	val = readl(port->membase + AML_UART_CONTROL);
-	val &= ~AML_UART_RX_EN;
-	val &= ~(AML_UART_RX_INT_EN | AML_UART_TX_INT_EN);
+	val &= ~(AML_UART_RX_EN | AML_UART_TX_EN);
 	writel(val, port->membase + AML_UART_CONTROL);
 
 	spin_unlock_irqrestore(&port->lock, flags);
-- 
2.33.1

[PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Yu Tu]

The S4 SoC on-chip UART uses a 12M clock as the clock source for
calculating the baud rate of the UART. But previously, chips used 24M or
other clock sources. So add this change. The specific clock source is
determined by chip design.

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

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 69450a461c48..557c24d954a2 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -19,6 +19,7 @@
 #include <linux/serial_core.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
+#include <linux/of_device.h>
 
 /* Register offsets */
 #define AML_UART_WFIFO			0x00
@@ -68,6 +69,8 @@
 #define AML_UART_BAUD_MASK		0x7fffff
 #define AML_UART_BAUD_USE		BIT(23)
 #define AML_UART_BAUD_XTAL		BIT(24)
+#define AML_UART_BAUD_XTAL_TICK		BIT(26)
+#define AML_UART_BAUD_XTAL_DIV2		BIT(27)
 
 #define AML_UART_PORT_NUM		12
 #define AML_UART_PORT_OFFSET		6
@@ -80,6 +83,11 @@ static struct uart_driver meson_uart_driver;
 
 static struct uart_port *meson_ports[AML_UART_PORT_NUM];
 
+struct meson_uart_data {
+	/*A clock source that calculates baud rates*/
+	unsigned int xtal_tick_en;
+};
+
 static void meson_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 }
@@ -294,16 +302,29 @@ 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 *uart_data = port->private_data;
 	u32 val;
 
 	while (!meson_uart_tx_empty(port))
 		cpu_relax();
 
+	val = readl_relaxed(port->membase + AML_UART_REG5);
+	val &= ~AML_UART_BAUD_MASK;
+
 	if (port->uartclk == 24000000) {
-		val = ((port->uartclk / 3) / baud) - 1;
-		val |= AML_UART_BAUD_XTAL;
+		if (uart_data->xtal_tick_en) {
+			val = (port->uartclk / 2 + baud / 2) / baud  - 1;
+			val |= (AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_DIV2);
+		} else {
+			val = ((port->uartclk / 3) + baud / 2) / baud  - 1;
+			val &= (~(AML_UART_BAUD_XTAL_TICK |
+				AML_UART_BAUD_XTAL_DIV2));
+			val |= AML_UART_BAUD_XTAL;
+		}
 	} else {
 		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
+		val &= (~(AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_TICK |
+			AML_UART_BAUD_XTAL_DIV2));
 	}
 	val |= AML_UART_BAUD_USE;
 	writel(val, port->membase + AML_UART_REG5);
@@ -714,6 +735,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 {
 	struct resource *res_mem, *res_irq;
 	struct uart_port *port;
+	struct meson_uart_data *uart_data;
 	int ret = 0;
 	int id = -1;
 
@@ -729,6 +751,10 @@ static int meson_uart_probe(struct platform_device *pdev)
 		}
 	}
 
+	uart_data = of_device_get_match_data(&pdev->dev);
+	if (!uart_data)
+		return  -EINVAL;
+
 	if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
 		return -EINVAL;
 
@@ -770,6 +796,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	port->x_char = 0;
 	port->ops = &meson_uart_ops;
 	port->fifosize = 64;
+	port->private_data = uart_data;
 
 	meson_ports[pdev->id] = port;
 	platform_set_drvdata(pdev, port);
@@ -798,14 +825,35 @@ static int meson_uart_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct meson_uart_data meson_uart_data = {
+	.xtal_tick_en = 0,
+};
+
+static const struct meson_uart_data s4_meson_uart_data = {
+	.xtal_tick_en = 1,
+};
+
 static const struct of_device_id meson_uart_dt_match[] = {
 	/* Legacy bindings, should be removed when no more used */
-	{ .compatible = "amlogic,meson-uart" },
+	{	.compatible = "amlogic,meson-uart",
+		.data = &meson_uart_data
+	},
 	/* Stable bindings */
-	{ .compatible = "amlogic,meson6-uart" },
-	{ .compatible = "amlogic,meson8-uart" },
-	{ .compatible = "amlogic,meson8b-uart" },
-	{ .compatible = "amlogic,meson-gx-uart" },
+	{	.compatible = "amlogic,meson6-uart",
+		.data = &meson_uart_data
+	},
+	{	.compatible = "amlogic,meson8-uart",
+		.data = &meson_uart_data
+	},
+	{	.compatible = "amlogic,meson8b-uart",
+		.data = &meson_uart_data
+	},
+	{	.compatible = "amlogic,meson-gx-uart",
+		.data = &meson_uart_data
+	},
+	{	.compatible = "amlogic,meson-s4-uart",
+		.data = &s4_meson_uart_data
+	},
 	{ /* 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

[PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Yu Tu]

The S4 SoC on-chip UART uses a 12M clock as the clock source for
calculating the baud rate of the UART. But previously, chips used 24M or
other clock sources. So add this change. The specific clock source is
determined by chip design.

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

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 69450a461c48..557c24d954a2 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -19,6 +19,7 @@
 #include <linux/serial_core.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
+#include <linux/of_device.h>
 
 /* Register offsets */
 #define AML_UART_WFIFO			0x00
@@ -68,6 +69,8 @@
 #define AML_UART_BAUD_MASK		0x7fffff
 #define AML_UART_BAUD_USE		BIT(23)
 #define AML_UART_BAUD_XTAL		BIT(24)
+#define AML_UART_BAUD_XTAL_TICK		BIT(26)
+#define AML_UART_BAUD_XTAL_DIV2		BIT(27)
 
 #define AML_UART_PORT_NUM		12
 #define AML_UART_PORT_OFFSET		6
@@ -80,6 +83,11 @@ static struct uart_driver meson_uart_driver;
 
 static struct uart_port *meson_ports[AML_UART_PORT_NUM];
 
+struct meson_uart_data {
+	/*A clock source that calculates baud rates*/
+	unsigned int xtal_tick_en;
+};
+
 static void meson_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 }
@@ -294,16 +302,29 @@ 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 *uart_data = port->private_data;
 	u32 val;
 
 	while (!meson_uart_tx_empty(port))
 		cpu_relax();
 
+	val = readl_relaxed(port->membase + AML_UART_REG5);
+	val &= ~AML_UART_BAUD_MASK;
+
 	if (port->uartclk == 24000000) {
-		val = ((port->uartclk / 3) / baud) - 1;
-		val |= AML_UART_BAUD_XTAL;
+		if (uart_data->xtal_tick_en) {
+			val = (port->uartclk / 2 + baud / 2) / baud  - 1;
+			val |= (AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_DIV2);
+		} else {
+			val = ((port->uartclk / 3) + baud / 2) / baud  - 1;
+			val &= (~(AML_UART_BAUD_XTAL_TICK |
+				AML_UART_BAUD_XTAL_DIV2));
+			val |= AML_UART_BAUD_XTAL;
+		}
 	} else {
 		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
+		val &= (~(AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_TICK |
+			AML_UART_BAUD_XTAL_DIV2));
 	}
 	val |= AML_UART_BAUD_USE;
 	writel(val, port->membase + AML_UART_REG5);
@@ -714,6 +735,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 {
 	struct resource *res_mem, *res_irq;
 	struct uart_port *port;
+	struct meson_uart_data *uart_data;
 	int ret = 0;
 	int id = -1;
 
@@ -729,6 +751,10 @@ static int meson_uart_probe(struct platform_device *pdev)
 		}
 	}
 
+	uart_data = of_device_get_match_data(&pdev->dev);
+	if (!uart_data)
+		return  -EINVAL;
+
 	if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
 		return -EINVAL;
 
@@ -770,6 +796,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	port->x_char = 0;
 	port->ops = &meson_uart_ops;
 	port->fifosize = 64;
+	port->private_data = uart_data;
 
 	meson_ports[pdev->id] = port;
 	platform_set_drvdata(pdev, port);
@@ -798,14 +825,35 @@ static int meson_uart_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct meson_uart_data meson_uart_data = {
+	.xtal_tick_en = 0,
+};
+
+static const struct meson_uart_data s4_meson_uart_data = {
+	.xtal_tick_en = 1,
+};
+
 static const struct of_device_id meson_uart_dt_match[] = {
 	/* Legacy bindings, should be removed when no more used */
-	{ .compatible = "amlogic,meson-uart" },
+	{	.compatible = "amlogic,meson-uart",
+		.data = &meson_uart_data
+	},
 	/* Stable bindings */
-	{ .compatible = "amlogic,meson6-uart" },
-	{ .compatible = "amlogic,meson8-uart" },
-	{ .compatible = "amlogic,meson8b-uart" },
-	{ .compatible = "amlogic,meson-gx-uart" },
+	{	.compatible = "amlogic,meson6-uart",
+		.data = &meson_uart_data
+	},
+	{	.compatible = "amlogic,meson8-uart",
+		.data = &meson_uart_data
+	},
+	{	.compatible = "amlogic,meson8b-uart",
+		.data = &meson_uart_data
+	},
+	{	.compatible = "amlogic,meson-gx-uart",
+		.data = &meson_uart_data
+	},
+	{	.compatible = "amlogic,meson-s4-uart",
+		.data = &s4_meson_uart_data
+	},
 	{ /* 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

[PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Yu Tu]

The S4 SoC on-chip UART uses a 12M clock as the clock source for
calculating the baud rate of the UART. But previously, chips used 24M or
other clock sources. So add this change. The specific clock source is
determined by chip design.

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

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 69450a461c48..557c24d954a2 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -19,6 +19,7 @@
 #include <linux/serial_core.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
+#include <linux/of_device.h>
 
 /* Register offsets */
 #define AML_UART_WFIFO			0x00
@@ -68,6 +69,8 @@
 #define AML_UART_BAUD_MASK		0x7fffff
 #define AML_UART_BAUD_USE		BIT(23)
 #define AML_UART_BAUD_XTAL		BIT(24)
+#define AML_UART_BAUD_XTAL_TICK		BIT(26)
+#define AML_UART_BAUD_XTAL_DIV2		BIT(27)
 
 #define AML_UART_PORT_NUM		12
 #define AML_UART_PORT_OFFSET		6
@@ -80,6 +83,11 @@ static struct uart_driver meson_uart_driver;
 
 static struct uart_port *meson_ports[AML_UART_PORT_NUM];
 
+struct meson_uart_data {
+	/*A clock source that calculates baud rates*/
+	unsigned int xtal_tick_en;
+};
+
 static void meson_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 }
@@ -294,16 +302,29 @@ 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 *uart_data = port->private_data;
 	u32 val;
 
 	while (!meson_uart_tx_empty(port))
 		cpu_relax();
 
+	val = readl_relaxed(port->membase + AML_UART_REG5);
+	val &= ~AML_UART_BAUD_MASK;
+
 	if (port->uartclk == 24000000) {
-		val = ((port->uartclk / 3) / baud) - 1;
-		val |= AML_UART_BAUD_XTAL;
+		if (uart_data->xtal_tick_en) {
+			val = (port->uartclk / 2 + baud / 2) / baud  - 1;
+			val |= (AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_DIV2);
+		} else {
+			val = ((port->uartclk / 3) + baud / 2) / baud  - 1;
+			val &= (~(AML_UART_BAUD_XTAL_TICK |
+				AML_UART_BAUD_XTAL_DIV2));
+			val |= AML_UART_BAUD_XTAL;
+		}
 	} else {
 		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
+		val &= (~(AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_TICK |
+			AML_UART_BAUD_XTAL_DIV2));
 	}
 	val |= AML_UART_BAUD_USE;
 	writel(val, port->membase + AML_UART_REG5);
@@ -714,6 +735,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 {
 	struct resource *res_mem, *res_irq;
 	struct uart_port *port;
+	struct meson_uart_data *uart_data;
 	int ret = 0;
 	int id = -1;
 
@@ -729,6 +751,10 @@ static int meson_uart_probe(struct platform_device *pdev)
 		}
 	}
 
+	uart_data = of_device_get_match_data(&pdev->dev);
+	if (!uart_data)
+		return  -EINVAL;
+
 	if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
 		return -EINVAL;
 
@@ -770,6 +796,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	port->x_char = 0;
 	port->ops = &meson_uart_ops;
 	port->fifosize = 64;
+	port->private_data = uart_data;
 
 	meson_ports[pdev->id] = port;
 	platform_set_drvdata(pdev, port);
@@ -798,14 +825,35 @@ static int meson_uart_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct meson_uart_data meson_uart_data = {
+	.xtal_tick_en = 0,
+};
+
+static const struct meson_uart_data s4_meson_uart_data = {
+	.xtal_tick_en = 1,
+};
+
 static const struct of_device_id meson_uart_dt_match[] = {
 	/* Legacy bindings, should be removed when no more used */
-	{ .compatible = "amlogic,meson-uart" },
+	{	.compatible = "amlogic,meson-uart",
+		.data = &meson_uart_data
+	},
 	/* Stable bindings */
-	{ .compatible = "amlogic,meson6-uart" },
-	{ .compatible = "amlogic,meson8-uart" },
-	{ .compatible = "amlogic,meson8b-uart" },
-	{ .compatible = "amlogic,meson-gx-uart" },
+	{	.compatible = "amlogic,meson6-uart",
+		.data = &meson_uart_data
+	},
+	{	.compatible = "amlogic,meson8-uart",
+		.data = &meson_uart_data
+	},
+	{	.compatible = "amlogic,meson8b-uart",
+		.data = &meson_uart_data
+	},
+	{	.compatible = "amlogic,meson-gx-uart",
+		.data = &meson_uart_data
+	},
+	{	.compatible = "amlogic,meson-s4-uart",
+		.data = &s4_meson_uart_data
+	},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
-- 
2.33.1

Re: [PATCH 1/3] tty: serial: meson: modify request_irq and free_irq

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:16:32PM +0800, Yu Tu wrote:
> Change request_irq to devm_request_irq and free_irq to devm_free_irq.
> It's better to change the code this way.

Why?  What did this fix up?  You still are manually requesting and
freeing the irq.  What bug did you fix?

> 
> The IRQF_SHARED interrupt flag was added because an interrupt error was
> detected when the serial port was opened twice in a row on the project.

That is a different change.  Make that a different patch.

thanks,

greg k-h

Re: [PATCH 1/3] tty: serial: meson: modify request_irq and free_irq

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:16:32PM +0800, Yu Tu wrote:
> Change request_irq to devm_request_irq and free_irq to devm_free_irq.
> It's better to change the code this way.

Why?  What did this fix up?  You still are manually requesting and
freeing the irq.  What bug did you fix?

> 
> The IRQF_SHARED interrupt flag was added because an interrupt error was
> detected when the serial port was opened twice in a row on the project.

That is a different change.  Make that a different patch.

thanks,

greg k-h

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

Re: [PATCH 1/3] tty: serial: meson: modify request_irq and free_irq

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:16:32PM +0800, Yu Tu wrote:
> Change request_irq to devm_request_irq and free_irq to devm_free_irq.
> It's better to change the code this way.

Why?  What did this fix up?  You still are manually requesting and
freeing the irq.  What bug did you fix?

> 
> The IRQF_SHARED interrupt flag was added because an interrupt error was
> detected when the serial port was opened twice in a row on the project.

That is a different change.  Make that a different patch.

thanks,

greg k-h

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

Re: [PATCH 0/3] the UART driver compatible with the Amlogic Meson S4 SoC

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:16:31PM +0800, Yu Tu wrote:
> The UART driver compatible with the Amlogic Meson S4 SoC on-chip, change the
> UART interrupt interface function while adding IRQF_SHARED flag. And add clear
> AML_UART_TX_EN bit in meson_uart_shutdown funtion.
> 
> Yu Tu (3):
>   tty: serial: meson: modify request_irq and free_irq
>   tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit
>   tty: serial: meson: add UART driver compatible with S4 SoC on-chip
> 
> Link:https://patchwork.kernel.org/project/linux-amlogic/patch/20211206100200.31914-1-xianwei.zhao@amlogic.com/

What is this link for?

And why patchwork?

Please just use lore.kernel.org links for mailing list threads.

thanks,

greg k-h

Re: [PATCH 0/3] the UART driver compatible with the Amlogic Meson S4 SoC

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:16:31PM +0800, Yu Tu wrote:
> The UART driver compatible with the Amlogic Meson S4 SoC on-chip, change the
> UART interrupt interface function while adding IRQF_SHARED flag. And add clear
> AML_UART_TX_EN bit in meson_uart_shutdown funtion.
> 
> Yu Tu (3):
>   tty: serial: meson: modify request_irq and free_irq
>   tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit
>   tty: serial: meson: add UART driver compatible with S4 SoC on-chip
> 
> Link:https://patchwork.kernel.org/project/linux-amlogic/patch/20211206100200.31914-1-xianwei.zhao@amlogic.com/

What is this link for?

And why patchwork?

Please just use lore.kernel.org links for mailing list threads.

thanks,

greg k-h

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

Re: [PATCH 0/3] the UART driver compatible with the Amlogic Meson S4 SoC

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:16:31PM +0800, Yu Tu wrote:
> The UART driver compatible with the Amlogic Meson S4 SoC on-chip, change the
> UART interrupt interface function while adding IRQF_SHARED flag. And add clear
> AML_UART_TX_EN bit in meson_uart_shutdown funtion.
> 
> Yu Tu (3):
>   tty: serial: meson: modify request_irq and free_irq
>   tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit
>   tty: serial: meson: add UART driver compatible with S4 SoC on-chip
> 
> Link:https://patchwork.kernel.org/project/linux-amlogic/patch/20211206100200.31914-1-xianwei.zhao@amlogic.com/

What is this link for?

And why patchwork?

Please just use lore.kernel.org links for mailing list threads.

thanks,

greg k-h

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

Re: [PATCH 2/3] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:16:33PM +0800, Yu Tu wrote:
> The meson_uart_shutdown function should have the opposite operation to
> the meson_uart_startup function, so the shutdown of AML_UART_TX_EN is
> logically missing.
> 
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>

What commit does this fix?  Should it go to stable kernels?  Please put
that in here if needed.

thanks,

greg k-h

Re: [PATCH 2/3] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:16:33PM +0800, Yu Tu wrote:
> The meson_uart_shutdown function should have the opposite operation to
> the meson_uart_startup function, so the shutdown of AML_UART_TX_EN is
> logically missing.
> 
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>

What commit does this fix?  Should it go to stable kernels?  Please put
that in here if needed.

thanks,

greg k-h

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

Re: [PATCH 2/3] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:16:33PM +0800, Yu Tu wrote:
> The meson_uart_shutdown function should have the opposite operation to
> the meson_uart_startup function, so the shutdown of AML_UART_TX_EN is
> logically missing.
> 
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>

What commit does this fix?  Should it go to stable kernels?  Please put
that in here if needed.

thanks,

greg k-h

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

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:16:34PM +0800, Yu Tu wrote:
> The S4 SoC on-chip UART uses a 12M clock as the clock source for
> calculating the baud rate of the UART. But previously, chips used 24M or
> other clock sources. So add this change. The specific clock source is
> determined by chip design.
> 
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---
>  drivers/tty/serial/meson_uart.c | 62 +++++++++++++++++++++++++++++----
>  1 file changed, 55 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 69450a461c48..557c24d954a2 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -19,6 +19,7 @@
>  #include <linux/serial_core.h>
>  #include <linux/tty.h>
>  #include <linux/tty_flip.h>
> +#include <linux/of_device.h>
>  
>  /* Register offsets */
>  #define AML_UART_WFIFO			0x00
> @@ -68,6 +69,8 @@
>  #define AML_UART_BAUD_MASK		0x7fffff
>  #define AML_UART_BAUD_USE		BIT(23)
>  #define AML_UART_BAUD_XTAL		BIT(24)
> +#define AML_UART_BAUD_XTAL_TICK		BIT(26)
> +#define AML_UART_BAUD_XTAL_DIV2		BIT(27)
>  
>  #define AML_UART_PORT_NUM		12
>  #define AML_UART_PORT_OFFSET		6
> @@ -80,6 +83,11 @@ static struct uart_driver meson_uart_driver;
>  
>  static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>  
> +struct meson_uart_data {
> +	/*A clock source that calculates baud rates*/

Please use spaces in your comments.

> +	unsigned int xtal_tick_en;

What is "_en" for?

"enabled"?

Spell it out please.

And why an unsigned int for a boolean flag?

> +};
> +
>  static void meson_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  {
>  }
> @@ -294,16 +302,29 @@ 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 *uart_data = port->private_data;
>  	u32 val;
>  
>  	while (!meson_uart_tx_empty(port))
>  		cpu_relax();
>  
> +	val = readl_relaxed(port->membase + AML_UART_REG5);
> +	val &= ~AML_UART_BAUD_MASK;
> +
>  	if (port->uartclk == 24000000) {
> -		val = ((port->uartclk / 3) / baud) - 1;
> -		val |= AML_UART_BAUD_XTAL;
> +		if (uart_data->xtal_tick_en) {
> +			val = (port->uartclk / 2 + baud / 2) / baud  - 1;
> +			val |= (AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_DIV2);
> +		} else {
> +			val = ((port->uartclk / 3) + baud / 2) / baud  - 1;
> +			val &= (~(AML_UART_BAUD_XTAL_TICK |
> +				AML_UART_BAUD_XTAL_DIV2));
> +			val |= AML_UART_BAUD_XTAL;
> +		}
>  	} else {
>  		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
> +		val &= (~(AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_TICK |
> +			AML_UART_BAUD_XTAL_DIV2));
>  	}
>  	val |= AML_UART_BAUD_USE;
>  	writel(val, port->membase + AML_UART_REG5);
> @@ -714,6 +735,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>  {
>  	struct resource *res_mem, *res_irq;
>  	struct uart_port *port;
> +	struct meson_uart_data *uart_data;
>  	int ret = 0;
>  	int id = -1;
>  
> @@ -729,6 +751,10 @@ static int meson_uart_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	uart_data = of_device_get_match_data(&pdev->dev);
> +	if (!uart_data)
> +		return  -EINVAL;

Wrong spacing.

Always use checkpatch.pl on your patches before sending them out.

And did you just break existing systems?  Do you know if all older ones
will still work with that call?

> +
>  	if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
>  		return -EINVAL;
>  
> @@ -770,6 +796,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>  	port->x_char = 0;
>  	port->ops = &meson_uart_ops;
>  	port->fifosize = 64;
> +	port->private_data = uart_data;
>  
>  	meson_ports[pdev->id] = port;
>  	platform_set_drvdata(pdev, port);
> @@ -798,14 +825,35 @@ static int meson_uart_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct meson_uart_data meson_uart_data = {
> +	.xtal_tick_en = 0,
> +};
> +
> +static const struct meson_uart_data s4_meson_uart_data = {
> +	.xtal_tick_en = 1,
> +};

As your whole structure just has one bit, why not just use that as the
data value, instead of a structure?  No need to be complex here at all.

thanks,

greg k-h

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:16:34PM +0800, Yu Tu wrote:
> The S4 SoC on-chip UART uses a 12M clock as the clock source for
> calculating the baud rate of the UART. But previously, chips used 24M or
> other clock sources. So add this change. The specific clock source is
> determined by chip design.
> 
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---
>  drivers/tty/serial/meson_uart.c | 62 +++++++++++++++++++++++++++++----
>  1 file changed, 55 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 69450a461c48..557c24d954a2 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -19,6 +19,7 @@
>  #include <linux/serial_core.h>
>  #include <linux/tty.h>
>  #include <linux/tty_flip.h>
> +#include <linux/of_device.h>
>  
>  /* Register offsets */
>  #define AML_UART_WFIFO			0x00
> @@ -68,6 +69,8 @@
>  #define AML_UART_BAUD_MASK		0x7fffff
>  #define AML_UART_BAUD_USE		BIT(23)
>  #define AML_UART_BAUD_XTAL		BIT(24)
> +#define AML_UART_BAUD_XTAL_TICK		BIT(26)
> +#define AML_UART_BAUD_XTAL_DIV2		BIT(27)
>  
>  #define AML_UART_PORT_NUM		12
>  #define AML_UART_PORT_OFFSET		6
> @@ -80,6 +83,11 @@ static struct uart_driver meson_uart_driver;
>  
>  static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>  
> +struct meson_uart_data {
> +	/*A clock source that calculates baud rates*/

Please use spaces in your comments.

> +	unsigned int xtal_tick_en;

What is "_en" for?

"enabled"?

Spell it out please.

And why an unsigned int for a boolean flag?

> +};
> +
>  static void meson_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  {
>  }
> @@ -294,16 +302,29 @@ 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 *uart_data = port->private_data;
>  	u32 val;
>  
>  	while (!meson_uart_tx_empty(port))
>  		cpu_relax();
>  
> +	val = readl_relaxed(port->membase + AML_UART_REG5);
> +	val &= ~AML_UART_BAUD_MASK;
> +
>  	if (port->uartclk == 24000000) {
> -		val = ((port->uartclk / 3) / baud) - 1;
> -		val |= AML_UART_BAUD_XTAL;
> +		if (uart_data->xtal_tick_en) {
> +			val = (port->uartclk / 2 + baud / 2) / baud  - 1;
> +			val |= (AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_DIV2);
> +		} else {
> +			val = ((port->uartclk / 3) + baud / 2) / baud  - 1;
> +			val &= (~(AML_UART_BAUD_XTAL_TICK |
> +				AML_UART_BAUD_XTAL_DIV2));
> +			val |= AML_UART_BAUD_XTAL;
> +		}
>  	} else {
>  		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
> +		val &= (~(AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_TICK |
> +			AML_UART_BAUD_XTAL_DIV2));
>  	}
>  	val |= AML_UART_BAUD_USE;
>  	writel(val, port->membase + AML_UART_REG5);
> @@ -714,6 +735,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>  {
>  	struct resource *res_mem, *res_irq;
>  	struct uart_port *port;
> +	struct meson_uart_data *uart_data;
>  	int ret = 0;
>  	int id = -1;
>  
> @@ -729,6 +751,10 @@ static int meson_uart_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	uart_data = of_device_get_match_data(&pdev->dev);
> +	if (!uart_data)
> +		return  -EINVAL;

Wrong spacing.

Always use checkpatch.pl on your patches before sending them out.

And did you just break existing systems?  Do you know if all older ones
will still work with that call?

> +
>  	if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
>  		return -EINVAL;
>  
> @@ -770,6 +796,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>  	port->x_char = 0;
>  	port->ops = &meson_uart_ops;
>  	port->fifosize = 64;
> +	port->private_data = uart_data;
>  
>  	meson_ports[pdev->id] = port;
>  	platform_set_drvdata(pdev, port);
> @@ -798,14 +825,35 @@ static int meson_uart_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct meson_uart_data meson_uart_data = {
> +	.xtal_tick_en = 0,
> +};
> +
> +static const struct meson_uart_data s4_meson_uart_data = {
> +	.xtal_tick_en = 1,
> +};

As your whole structure just has one bit, why not just use that as the
data value, instead of a structure?  No need to be complex here at all.

thanks,

greg k-h

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

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:16:34PM +0800, Yu Tu wrote:
> The S4 SoC on-chip UART uses a 12M clock as the clock source for
> calculating the baud rate of the UART. But previously, chips used 24M or
> other clock sources. So add this change. The specific clock source is
> determined by chip design.
> 
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---
>  drivers/tty/serial/meson_uart.c | 62 +++++++++++++++++++++++++++++----
>  1 file changed, 55 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 69450a461c48..557c24d954a2 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -19,6 +19,7 @@
>  #include <linux/serial_core.h>
>  #include <linux/tty.h>
>  #include <linux/tty_flip.h>
> +#include <linux/of_device.h>
>  
>  /* Register offsets */
>  #define AML_UART_WFIFO			0x00
> @@ -68,6 +69,8 @@
>  #define AML_UART_BAUD_MASK		0x7fffff
>  #define AML_UART_BAUD_USE		BIT(23)
>  #define AML_UART_BAUD_XTAL		BIT(24)
> +#define AML_UART_BAUD_XTAL_TICK		BIT(26)
> +#define AML_UART_BAUD_XTAL_DIV2		BIT(27)
>  
>  #define AML_UART_PORT_NUM		12
>  #define AML_UART_PORT_OFFSET		6
> @@ -80,6 +83,11 @@ static struct uart_driver meson_uart_driver;
>  
>  static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>  
> +struct meson_uart_data {
> +	/*A clock source that calculates baud rates*/

Please use spaces in your comments.

> +	unsigned int xtal_tick_en;

What is "_en" for?

"enabled"?

Spell it out please.

And why an unsigned int for a boolean flag?

> +};
> +
>  static void meson_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  {
>  }
> @@ -294,16 +302,29 @@ 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 *uart_data = port->private_data;
>  	u32 val;
>  
>  	while (!meson_uart_tx_empty(port))
>  		cpu_relax();
>  
> +	val = readl_relaxed(port->membase + AML_UART_REG5);
> +	val &= ~AML_UART_BAUD_MASK;
> +
>  	if (port->uartclk == 24000000) {
> -		val = ((port->uartclk / 3) / baud) - 1;
> -		val |= AML_UART_BAUD_XTAL;
> +		if (uart_data->xtal_tick_en) {
> +			val = (port->uartclk / 2 + baud / 2) / baud  - 1;
> +			val |= (AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_DIV2);
> +		} else {
> +			val = ((port->uartclk / 3) + baud / 2) / baud  - 1;
> +			val &= (~(AML_UART_BAUD_XTAL_TICK |
> +				AML_UART_BAUD_XTAL_DIV2));
> +			val |= AML_UART_BAUD_XTAL;
> +		}
>  	} else {
>  		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
> +		val &= (~(AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_TICK |
> +			AML_UART_BAUD_XTAL_DIV2));
>  	}
>  	val |= AML_UART_BAUD_USE;
>  	writel(val, port->membase + AML_UART_REG5);
> @@ -714,6 +735,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>  {
>  	struct resource *res_mem, *res_irq;
>  	struct uart_port *port;
> +	struct meson_uart_data *uart_data;
>  	int ret = 0;
>  	int id = -1;
>  
> @@ -729,6 +751,10 @@ static int meson_uart_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	uart_data = of_device_get_match_data(&pdev->dev);
> +	if (!uart_data)
> +		return  -EINVAL;

Wrong spacing.

Always use checkpatch.pl on your patches before sending them out.

And did you just break existing systems?  Do you know if all older ones
will still work with that call?

> +
>  	if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
>  		return -EINVAL;
>  
> @@ -770,6 +796,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>  	port->x_char = 0;
>  	port->ops = &meson_uart_ops;
>  	port->fifosize = 64;
> +	port->private_data = uart_data;
>  
>  	meson_ports[pdev->id] = port;
>  	platform_set_drvdata(pdev, port);
> @@ -798,14 +825,35 @@ static int meson_uart_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct meson_uart_data meson_uart_data = {
> +	.xtal_tick_en = 0,
> +};
> +
> +static const struct meson_uart_data s4_meson_uart_data = {
> +	.xtal_tick_en = 1,
> +};

As your whole structure just has one bit, why not just use that as the
data value, instead of a structure?  No need to be complex here at all.

thanks,

greg k-h

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

Re: [PATCH 0/3] the UART driver compatible with the Amlogic Meson S4 SoC

[Yu Tu]

Hi Greg K-H,
	Thank you very much for your reply.I'll amend it as you suggest.

On 2021/12/21 15:30, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Dec 21, 2021 at 03:16:31PM +0800, Yu Tu wrote:
>> The UART driver compatible with the Amlogic Meson S4 SoC on-chip, change the
>> UART interrupt interface function while adding IRQF_SHARED flag. And add clear
>> AML_UART_TX_EN bit in meson_uart_shutdown funtion.
>>
>> Yu Tu (3):
>>    tty: serial: meson: modify request_irq and free_irq
>>    tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit
>>    tty: serial: meson: add UART driver compatible with S4 SoC on-chip
>>
>> Link:https://patchwork.kernel.org/project/linux-amlogic/patch/20211206100200.31914-1-xianwei.zhao@amlogic.com/
> 
> What is this link for?
> 
> And why patchwork?
> 
> Please just use lore.kernel.org links for mailing list threads.
> 
> thanks,
> 
> greg k-h
> 
I will change the address you suggested as follows,
Link: 
https://lore.kernel.org/linux-amlogic/20211206100200.31914-1-xianwei.zhao@amlogic.com/

Re: [PATCH 0/3] the UART driver compatible with the Amlogic Meson S4 SoC

[Yu Tu]

Hi Greg K-H,
	Thank you very much for your reply.I'll amend it as you suggest.

On 2021/12/21 15:30, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Dec 21, 2021 at 03:16:31PM +0800, Yu Tu wrote:
>> The UART driver compatible with the Amlogic Meson S4 SoC on-chip, change the
>> UART interrupt interface function while adding IRQF_SHARED flag. And add clear
>> AML_UART_TX_EN bit in meson_uart_shutdown funtion.
>>
>> Yu Tu (3):
>>    tty: serial: meson: modify request_irq and free_irq
>>    tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit
>>    tty: serial: meson: add UART driver compatible with S4 SoC on-chip
>>
>> Link:https://patchwork.kernel.org/project/linux-amlogic/patch/20211206100200.31914-1-xianwei.zhao@amlogic.com/
> 
> What is this link for?
> 
> And why patchwork?
> 
> Please just use lore.kernel.org links for mailing list threads.
> 
> thanks,
> 
> greg k-h
> 
I will change the address you suggested as follows,
Link: 
https://lore.kernel.org/linux-amlogic/20211206100200.31914-1-xianwei.zhao@amlogic.com/

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

Re: [PATCH 0/3] the UART driver compatible with the Amlogic Meson S4 SoC

[Yu Tu]

Hi Greg K-H,
	Thank you very much for your reply.I'll amend it as you suggest.

On 2021/12/21 15:30, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Dec 21, 2021 at 03:16:31PM +0800, Yu Tu wrote:
>> The UART driver compatible with the Amlogic Meson S4 SoC on-chip, change the
>> UART interrupt interface function while adding IRQF_SHARED flag. And add clear
>> AML_UART_TX_EN bit in meson_uart_shutdown funtion.
>>
>> Yu Tu (3):
>>    tty: serial: meson: modify request_irq and free_irq
>>    tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit
>>    tty: serial: meson: add UART driver compatible with S4 SoC on-chip
>>
>> Link:https://patchwork.kernel.org/project/linux-amlogic/patch/20211206100200.31914-1-xianwei.zhao@amlogic.com/
> 
> What is this link for?
> 
> And why patchwork?
> 
> Please just use lore.kernel.org links for mailing list threads.
> 
> thanks,
> 
> greg k-h
> 
I will change the address you suggested as follows,
Link: 
https://lore.kernel.org/linux-amlogic/20211206100200.31914-1-xianwei.zhao@amlogic.com/

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

Re: [PATCH 1/3] tty: serial: meson: modify request_irq and free_irq

[Yu Tu]

On 2021/12/21 15:30, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Dec 21, 2021 at 03:16:32PM +0800, Yu Tu wrote:
>> Change request_irq to devm_request_irq and free_irq to devm_free_irq.
>> It's better to change the code this way.
> 
> Why?  What did this fix up?  You still are manually requesting and
> freeing the irq.  What bug did you fix?
> 
I think this is exactly what you said. It's not necessary.
>>
>> The IRQF_SHARED interrupt flag was added because an interrupt error was
>> detected when the serial port was opened twice in a row on the project.
> 
> That is a different change.  Make that a different patch.
> 
The main purpose of this change is that I found some users in the actual 
project with the following usages:
(1)open(/dev/ttyAML0);
(2)open(/dev/ttyAML0);
The open function calls the meson_uart_startup function. If this is the 
case, an interrupt error is reported.So So the IRQF_SHARED flag was added.

I'm going to do this for now, remove free_irq and request_irq 
function,then add devm_request_irq in meson_uart_probe function.
This solves the above problem without adding IRQF_SHARED.
> thanks,
> 
> greg k-h
> 

Re: [PATCH 1/3] tty: serial: meson: modify request_irq and free_irq

[Yu Tu]

On 2021/12/21 15:30, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Dec 21, 2021 at 03:16:32PM +0800, Yu Tu wrote:
>> Change request_irq to devm_request_irq and free_irq to devm_free_irq.
>> It's better to change the code this way.
> 
> Why?  What did this fix up?  You still are manually requesting and
> freeing the irq.  What bug did you fix?
> 
I think this is exactly what you said. It's not necessary.
>>
>> The IRQF_SHARED interrupt flag was added because an interrupt error was
>> detected when the serial port was opened twice in a row on the project.
> 
> That is a different change.  Make that a different patch.
> 
The main purpose of this change is that I found some users in the actual 
project with the following usages:
(1)open(/dev/ttyAML0);
(2)open(/dev/ttyAML0);
The open function calls the meson_uart_startup function. If this is the 
case, an interrupt error is reported.So So the IRQF_SHARED flag was added.

I'm going to do this for now, remove free_irq and request_irq 
function,then add devm_request_irq in meson_uart_probe function.
This solves the above problem without adding IRQF_SHARED.
> thanks,
> 
> greg k-h
> 


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

Re: [PATCH 1/3] tty: serial: meson: modify request_irq and free_irq

[Yu Tu]

On 2021/12/21 15:30, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Dec 21, 2021 at 03:16:32PM +0800, Yu Tu wrote:
>> Change request_irq to devm_request_irq and free_irq to devm_free_irq.
>> It's better to change the code this way.
> 
> Why?  What did this fix up?  You still are manually requesting and
> freeing the irq.  What bug did you fix?
> 
I think this is exactly what you said. It's not necessary.
>>
>> The IRQF_SHARED interrupt flag was added because an interrupt error was
>> detected when the serial port was opened twice in a row on the project.
> 
> That is a different change.  Make that a different patch.
> 
The main purpose of this change is that I found some users in the actual 
project with the following usages:
(1)open(/dev/ttyAML0);
(2)open(/dev/ttyAML0);
The open function calls the meson_uart_startup function. If this is the 
case, an interrupt error is reported.So So the IRQF_SHARED flag was added.

I'm going to do this for now, remove free_irq and request_irq 
function,then add devm_request_irq in meson_uart_probe function.
This solves the above problem without adding IRQF_SHARED.
> thanks,
> 
> greg k-h
> 


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

Re: [PATCH 2/3] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit

[Yu Tu]

On 2021/12/21 15:32, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Dec 21, 2021 at 03:16:33PM +0800, Yu Tu wrote:
>> The meson_uart_shutdown function should have the opposite operation to
>> the meson_uart_startup function, so the shutdown of AML_UART_TX_EN is
>> logically missing.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> 
> What commit does this fix?  Should it go to stable kernels?  Please put
> that in here if needed.
> 
It has not yet been revealed that this is a bug. The reason I changed it 
was because I thought it was an improvement and it might be a hidden 
bug.The AML_UART_TX_EN bit is conspicuously missing when comparing the 
meson_uart_shutdown and meson_uart_startup functions.So I think it's 
best to add.
> thanks,
> 
> greg k-h
> 

Re: [PATCH 2/3] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit

[Yu Tu]

On 2021/12/21 15:32, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Dec 21, 2021 at 03:16:33PM +0800, Yu Tu wrote:
>> The meson_uart_shutdown function should have the opposite operation to
>> the meson_uart_startup function, so the shutdown of AML_UART_TX_EN is
>> logically missing.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> 
> What commit does this fix?  Should it go to stable kernels?  Please put
> that in here if needed.
> 
It has not yet been revealed that this is a bug. The reason I changed it 
was because I thought it was an improvement and it might be a hidden 
bug.The AML_UART_TX_EN bit is conspicuously missing when comparing the 
meson_uart_shutdown and meson_uart_startup functions.So I think it's 
best to add.
> thanks,
> 
> greg k-h
> 

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

Re: [PATCH 2/3] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit

[Yu Tu]

On 2021/12/21 15:32, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Dec 21, 2021 at 03:16:33PM +0800, Yu Tu wrote:
>> The meson_uart_shutdown function should have the opposite operation to
>> the meson_uart_startup function, so the shutdown of AML_UART_TX_EN is
>> logically missing.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> 
> What commit does this fix?  Should it go to stable kernels?  Please put
> that in here if needed.
> 
It has not yet been revealed that this is a bug. The reason I changed it 
was because I thought it was an improvement and it might be a hidden 
bug.The AML_UART_TX_EN bit is conspicuously missing when comparing the 
meson_uart_shutdown and meson_uart_startup functions.So I think it's 
best to add.
> thanks,
> 
> greg k-h
> 

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

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Yu Tu]

On 2021/12/21 15:34, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Dec 21, 2021 at 03:16:34PM +0800, Yu Tu wrote:
>> The S4 SoC on-chip UART uses a 12M clock as the clock source for
>> calculating the baud rate of the UART. But previously, chips used 24M or
>> other clock sources. So add this change. The specific clock source is
>> determined by chip design.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>>   drivers/tty/serial/meson_uart.c | 62 +++++++++++++++++++++++++++++----
>>   1 file changed, 55 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index 69450a461c48..557c24d954a2 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/serial_core.h>
>>   #include <linux/tty.h>
>>   #include <linux/tty_flip.h>
>> +#include <linux/of_device.h>
>>   
>>   /* Register offsets */
>>   #define AML_UART_WFIFO			0x00
>> @@ -68,6 +69,8 @@
>>   #define AML_UART_BAUD_MASK		0x7fffff
>>   #define AML_UART_BAUD_USE		BIT(23)
>>   #define AML_UART_BAUD_XTAL		BIT(24)
>> +#define AML_UART_BAUD_XTAL_TICK		BIT(26)
>> +#define AML_UART_BAUD_XTAL_DIV2		BIT(27)
>>   
>>   #define AML_UART_PORT_NUM		12
>>   #define AML_UART_PORT_OFFSET		6
>> @@ -80,6 +83,11 @@ static struct uart_driver meson_uart_driver;
>>   
>>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>>   
>> +struct meson_uart_data {
>> +	/*A clock source that calculates baud rates*/
> 
> Please use spaces in your comments.

I will correct this mistake in the next patch.

> 
>> +	unsigned int xtal_tick_en;
> 
> What is "_en" for?
> 
> "enabled"?
> 
> Spell it out please.
You're right.I will correct as you suggested.
> 
> And why an unsigned int for a boolean flag?
It is my thoughtless, I will correct.
> 
>> +};
>> +
>>   static void meson_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>>   {
>>   }
>> @@ -294,16 +302,29 @@ 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 *uart_data = port->private_data;
>>   	u32 val;
>>   
>>   	while (!meson_uart_tx_empty(port))
>>   		cpu_relax();
>>   
>> +	val = readl_relaxed(port->membase + AML_UART_REG5);
>> +	val &= ~AML_UART_BAUD_MASK;
>> +
>>   	if (port->uartclk == 24000000) {
>> -		val = ((port->uartclk / 3) / baud) - 1;
>> -		val |= AML_UART_BAUD_XTAL;
>> +		if (uart_data->xtal_tick_en) {
>> +			val = (port->uartclk / 2 + baud / 2) / baud  - 1;
>> +			val |= (AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_DIV2);
>> +		} else {
>> +			val = ((port->uartclk / 3) + baud / 2) / baud  - 1;
>> +			val &= (~(AML_UART_BAUD_XTAL_TICK |
>> +				AML_UART_BAUD_XTAL_DIV2));
>> +			val |= AML_UART_BAUD_XTAL;
>> +		}
>>   	} else {
>>   		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>> +		val &= (~(AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_TICK |
>> +			AML_UART_BAUD_XTAL_DIV2));
>>   	}
>>   	val |= AML_UART_BAUD_USE;
>>   	writel(val, port->membase + AML_UART_REG5);
>> @@ -714,6 +735,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>>   {
>>   	struct resource *res_mem, *res_irq;
>>   	struct uart_port *port;
>> +	struct meson_uart_data *uart_data;
>>   	int ret = 0;
>>   	int id = -1;
>>   
>> @@ -729,6 +751,10 @@ static int meson_uart_probe(struct platform_device *pdev)
>>   		}
>>   	}
>>   
>> +	uart_data = of_device_get_match_data(&pdev->dev);
>> +	if (!uart_data)
>> +		return  -EINVAL;
> 
> Wrong spacing.
> 
> Always use checkpatch.pl on your patches before sending them out.
Sorry, this is a rookie mistake.But I did check it locally before 
sending it. I will follow your advice strictly later.
> 
> And did you just break existing systems?  Do you know if all older ones
> will still work with that call?
> 
It does affect older systems, but the new and older baud rates are not 
the same. I checked the documents before I made any changes. So this 
change is compatible with the older.
>> +
>>   	if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
>>   		return -EINVAL;
>>   
>> @@ -770,6 +796,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>>   	port->x_char = 0;
>>   	port->ops = &meson_uart_ops;
>>   	port->fifosize = 64;
>> +	port->private_data = uart_data;
>>   
>>   	meson_ports[pdev->id] = port;
>>   	platform_set_drvdata(pdev, port);
>> @@ -798,14 +825,35 @@ static int meson_uart_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> +static const struct meson_uart_data meson_uart_data = {
>> +	.xtal_tick_en = 0,
>> +};
>> +
>> +static const struct meson_uart_data s4_meson_uart_data = {
>> +	.xtal_tick_en = 1,
>> +};
> 
> As your whole structure just has one bit, why not just use that as the
> data value, instead of a structure?  No need to be complex here at all.
> 
It is my thoughtless, I will correct.
> thanks,
> 
> greg k-h
> 

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Yu Tu]

On 2021/12/21 15:34, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Dec 21, 2021 at 03:16:34PM +0800, Yu Tu wrote:
>> The S4 SoC on-chip UART uses a 12M clock as the clock source for
>> calculating the baud rate of the UART. But previously, chips used 24M or
>> other clock sources. So add this change. The specific clock source is
>> determined by chip design.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>>   drivers/tty/serial/meson_uart.c | 62 +++++++++++++++++++++++++++++----
>>   1 file changed, 55 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index 69450a461c48..557c24d954a2 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/serial_core.h>
>>   #include <linux/tty.h>
>>   #include <linux/tty_flip.h>
>> +#include <linux/of_device.h>
>>   
>>   /* Register offsets */
>>   #define AML_UART_WFIFO			0x00
>> @@ -68,6 +69,8 @@
>>   #define AML_UART_BAUD_MASK		0x7fffff
>>   #define AML_UART_BAUD_USE		BIT(23)
>>   #define AML_UART_BAUD_XTAL		BIT(24)
>> +#define AML_UART_BAUD_XTAL_TICK		BIT(26)
>> +#define AML_UART_BAUD_XTAL_DIV2		BIT(27)
>>   
>>   #define AML_UART_PORT_NUM		12
>>   #define AML_UART_PORT_OFFSET		6
>> @@ -80,6 +83,11 @@ static struct uart_driver meson_uart_driver;
>>   
>>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>>   
>> +struct meson_uart_data {
>> +	/*A clock source that calculates baud rates*/
> 
> Please use spaces in your comments.

I will correct this mistake in the next patch.

> 
>> +	unsigned int xtal_tick_en;
> 
> What is "_en" for?
> 
> "enabled"?
> 
> Spell it out please.
You're right.I will correct as you suggested.
> 
> And why an unsigned int for a boolean flag?
It is my thoughtless, I will correct.
> 
>> +};
>> +
>>   static void meson_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>>   {
>>   }
>> @@ -294,16 +302,29 @@ 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 *uart_data = port->private_data;
>>   	u32 val;
>>   
>>   	while (!meson_uart_tx_empty(port))
>>   		cpu_relax();
>>   
>> +	val = readl_relaxed(port->membase + AML_UART_REG5);
>> +	val &= ~AML_UART_BAUD_MASK;
>> +
>>   	if (port->uartclk == 24000000) {
>> -		val = ((port->uartclk / 3) / baud) - 1;
>> -		val |= AML_UART_BAUD_XTAL;
>> +		if (uart_data->xtal_tick_en) {
>> +			val = (port->uartclk / 2 + baud / 2) / baud  - 1;
>> +			val |= (AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_DIV2);
>> +		} else {
>> +			val = ((port->uartclk / 3) + baud / 2) / baud  - 1;
>> +			val &= (~(AML_UART_BAUD_XTAL_TICK |
>> +				AML_UART_BAUD_XTAL_DIV2));
>> +			val |= AML_UART_BAUD_XTAL;
>> +		}
>>   	} else {
>>   		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>> +		val &= (~(AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_TICK |
>> +			AML_UART_BAUD_XTAL_DIV2));
>>   	}
>>   	val |= AML_UART_BAUD_USE;
>>   	writel(val, port->membase + AML_UART_REG5);
>> @@ -714,6 +735,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>>   {
>>   	struct resource *res_mem, *res_irq;
>>   	struct uart_port *port;
>> +	struct meson_uart_data *uart_data;
>>   	int ret = 0;
>>   	int id = -1;
>>   
>> @@ -729,6 +751,10 @@ static int meson_uart_probe(struct platform_device *pdev)
>>   		}
>>   	}
>>   
>> +	uart_data = of_device_get_match_data(&pdev->dev);
>> +	if (!uart_data)
>> +		return  -EINVAL;
> 
> Wrong spacing.
> 
> Always use checkpatch.pl on your patches before sending them out.
Sorry, this is a rookie mistake.But I did check it locally before 
sending it. I will follow your advice strictly later.
> 
> And did you just break existing systems?  Do you know if all older ones
> will still work with that call?
> 
It does affect older systems, but the new and older baud rates are not 
the same. I checked the documents before I made any changes. So this 
change is compatible with the older.
>> +
>>   	if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
>>   		return -EINVAL;
>>   
>> @@ -770,6 +796,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>>   	port->x_char = 0;
>>   	port->ops = &meson_uart_ops;
>>   	port->fifosize = 64;
>> +	port->private_data = uart_data;
>>   
>>   	meson_ports[pdev->id] = port;
>>   	platform_set_drvdata(pdev, port);
>> @@ -798,14 +825,35 @@ static int meson_uart_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> +static const struct meson_uart_data meson_uart_data = {
>> +	.xtal_tick_en = 0,
>> +};
>> +
>> +static const struct meson_uart_data s4_meson_uart_data = {
>> +	.xtal_tick_en = 1,
>> +};
> 
> As your whole structure just has one bit, why not just use that as the
> data value, instead of a structure?  No need to be complex here at all.
> 
It is my thoughtless, I will correct.
> thanks,
> 
> greg k-h
> 

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

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Yu Tu]

On 2021/12/21 15:34, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Dec 21, 2021 at 03:16:34PM +0800, Yu Tu wrote:
>> The S4 SoC on-chip UART uses a 12M clock as the clock source for
>> calculating the baud rate of the UART. But previously, chips used 24M or
>> other clock sources. So add this change. The specific clock source is
>> determined by chip design.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>>   drivers/tty/serial/meson_uart.c | 62 +++++++++++++++++++++++++++++----
>>   1 file changed, 55 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index 69450a461c48..557c24d954a2 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/serial_core.h>
>>   #include <linux/tty.h>
>>   #include <linux/tty_flip.h>
>> +#include <linux/of_device.h>
>>   
>>   /* Register offsets */
>>   #define AML_UART_WFIFO			0x00
>> @@ -68,6 +69,8 @@
>>   #define AML_UART_BAUD_MASK		0x7fffff
>>   #define AML_UART_BAUD_USE		BIT(23)
>>   #define AML_UART_BAUD_XTAL		BIT(24)
>> +#define AML_UART_BAUD_XTAL_TICK		BIT(26)
>> +#define AML_UART_BAUD_XTAL_DIV2		BIT(27)
>>   
>>   #define AML_UART_PORT_NUM		12
>>   #define AML_UART_PORT_OFFSET		6
>> @@ -80,6 +83,11 @@ static struct uart_driver meson_uart_driver;
>>   
>>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>>   
>> +struct meson_uart_data {
>> +	/*A clock source that calculates baud rates*/
> 
> Please use spaces in your comments.

I will correct this mistake in the next patch.

> 
>> +	unsigned int xtal_tick_en;
> 
> What is "_en" for?
> 
> "enabled"?
> 
> Spell it out please.
You're right.I will correct as you suggested.
> 
> And why an unsigned int for a boolean flag?
It is my thoughtless, I will correct.
> 
>> +};
>> +
>>   static void meson_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>>   {
>>   }
>> @@ -294,16 +302,29 @@ 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 *uart_data = port->private_data;
>>   	u32 val;
>>   
>>   	while (!meson_uart_tx_empty(port))
>>   		cpu_relax();
>>   
>> +	val = readl_relaxed(port->membase + AML_UART_REG5);
>> +	val &= ~AML_UART_BAUD_MASK;
>> +
>>   	if (port->uartclk == 24000000) {
>> -		val = ((port->uartclk / 3) / baud) - 1;
>> -		val |= AML_UART_BAUD_XTAL;
>> +		if (uart_data->xtal_tick_en) {
>> +			val = (port->uartclk / 2 + baud / 2) / baud  - 1;
>> +			val |= (AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_DIV2);
>> +		} else {
>> +			val = ((port->uartclk / 3) + baud / 2) / baud  - 1;
>> +			val &= (~(AML_UART_BAUD_XTAL_TICK |
>> +				AML_UART_BAUD_XTAL_DIV2));
>> +			val |= AML_UART_BAUD_XTAL;
>> +		}
>>   	} else {
>>   		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>> +		val &= (~(AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_TICK |
>> +			AML_UART_BAUD_XTAL_DIV2));
>>   	}
>>   	val |= AML_UART_BAUD_USE;
>>   	writel(val, port->membase + AML_UART_REG5);
>> @@ -714,6 +735,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>>   {
>>   	struct resource *res_mem, *res_irq;
>>   	struct uart_port *port;
>> +	struct meson_uart_data *uart_data;
>>   	int ret = 0;
>>   	int id = -1;
>>   
>> @@ -729,6 +751,10 @@ static int meson_uart_probe(struct platform_device *pdev)
>>   		}
>>   	}
>>   
>> +	uart_data = of_device_get_match_data(&pdev->dev);
>> +	if (!uart_data)
>> +		return  -EINVAL;
> 
> Wrong spacing.
> 
> Always use checkpatch.pl on your patches before sending them out.
Sorry, this is a rookie mistake.But I did check it locally before 
sending it. I will follow your advice strictly later.
> 
> And did you just break existing systems?  Do you know if all older ones
> will still work with that call?
> 
It does affect older systems, but the new and older baud rates are not 
the same. I checked the documents before I made any changes. So this 
change is compatible with the older.
>> +
>>   	if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
>>   		return -EINVAL;
>>   
>> @@ -770,6 +796,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>>   	port->x_char = 0;
>>   	port->ops = &meson_uart_ops;
>>   	port->fifosize = 64;
>> +	port->private_data = uart_data;
>>   
>>   	meson_ports[pdev->id] = port;
>>   	platform_set_drvdata(pdev, port);
>> @@ -798,14 +825,35 @@ static int meson_uart_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> +static const struct meson_uart_data meson_uart_data = {
>> +	.xtal_tick_en = 0,
>> +};
>> +
>> +static const struct meson_uart_data s4_meson_uart_data = {
>> +	.xtal_tick_en = 1,
>> +};
> 
> As your whole structure just has one bit, why not just use that as the
> data value, instead of a structure?  No need to be complex here at all.
> 
It is my thoughtless, I will correct.
> thanks,
> 
> greg k-h
> 

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

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Martin Blumenstingl]

Hello,

On Tue, Dec 21, 2021 at 8:17 AM Yu Tu <yu.tu@amlogic.com> wrote:
>
> The S4 SoC on-chip UART uses a 12M clock as the clock source for
> calculating the baud rate of the UART. But previously, chips used 24M or
> other clock sources. So add this change. The specific clock source is
> determined by chip design.
Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
If there's still a 24MHz XTAL then I think this description is not
correct - at least based on how I understand the UART controller.

SoCs up to GXL and GXM had an internal divide-by-3 (clock divider) in
the UART controller IP and an external 24MHz XTAL.
This was not configurable, so the clock for all baud-rates had to be
derived from an 8MHz (24MHz divided by 3) clock.

With the A311D (G12B, which is still using an external 24MHz XTAL) SoC
the UART controller gained two new bits - with configurable dividers -
according to the public datasheets:
UART_EE_A_REG5[26]:
- 0x0: divide the input clock by 3 (meaning: this internally works
with an 8MHz clock)
- 0x1: use the input clock directly without further division (meaning:
this internally work with an 24MHz clock)
UART_EE_A_REG5[27]:
- 0x0: use the clock as configured in UART_EE_A_REG5[26]
- 0x1: divide the input clock by 2 (meaning: this internally works
with an 12MHz clock)

While writing this email I did some investigation and found that
UART_EE_A_REG5[26] is used in the vendor kernel even for GXL and GXM
SoCs.
So this probably has been introduced with the GXL generation (and thus
is missing on GXBB and earlier SoCs).
Also UART_EE_A_REG5[27] seems to have been introduced with the G12A
generation of SoCs (not surprising since G12A and G12B peripherals are
very similar).

Does the UART controller not work with divide-by-3 (as we have it
today) or are these configurable dividers to reduce jitter?


Best regards,
Martin

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Martin Blumenstingl]

Hello,

On Tue, Dec 21, 2021 at 8:17 AM Yu Tu <yu.tu@amlogic.com> wrote:
>
> The S4 SoC on-chip UART uses a 12M clock as the clock source for
> calculating the baud rate of the UART. But previously, chips used 24M or
> other clock sources. So add this change. The specific clock source is
> determined by chip design.
Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
If there's still a 24MHz XTAL then I think this description is not
correct - at least based on how I understand the UART controller.

SoCs up to GXL and GXM had an internal divide-by-3 (clock divider) in
the UART controller IP and an external 24MHz XTAL.
This was not configurable, so the clock for all baud-rates had to be
derived from an 8MHz (24MHz divided by 3) clock.

With the A311D (G12B, which is still using an external 24MHz XTAL) SoC
the UART controller gained two new bits - with configurable dividers -
according to the public datasheets:
UART_EE_A_REG5[26]:
- 0x0: divide the input clock by 3 (meaning: this internally works
with an 8MHz clock)
- 0x1: use the input clock directly without further division (meaning:
this internally work with an 24MHz clock)
UART_EE_A_REG5[27]:
- 0x0: use the clock as configured in UART_EE_A_REG5[26]
- 0x1: divide the input clock by 2 (meaning: this internally works
with an 12MHz clock)

While writing this email I did some investigation and found that
UART_EE_A_REG5[26] is used in the vendor kernel even for GXL and GXM
SoCs.
So this probably has been introduced with the GXL generation (and thus
is missing on GXBB and earlier SoCs).
Also UART_EE_A_REG5[27] seems to have been introduced with the G12A
generation of SoCs (not surprising since G12A and G12B peripherals are
very similar).

Does the UART controller not work with divide-by-3 (as we have it
today) or are these configurable dividers to reduce jitter?


Best regards,
Martin

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

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Martin Blumenstingl]

Hello,

On Tue, Dec 21, 2021 at 8:17 AM Yu Tu <yu.tu@amlogic.com> wrote:
>
> The S4 SoC on-chip UART uses a 12M clock as the clock source for
> calculating the baud rate of the UART. But previously, chips used 24M or
> other clock sources. So add this change. The specific clock source is
> determined by chip design.
Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
If there's still a 24MHz XTAL then I think this description is not
correct - at least based on how I understand the UART controller.

SoCs up to GXL and GXM had an internal divide-by-3 (clock divider) in
the UART controller IP and an external 24MHz XTAL.
This was not configurable, so the clock for all baud-rates had to be
derived from an 8MHz (24MHz divided by 3) clock.

With the A311D (G12B, which is still using an external 24MHz XTAL) SoC
the UART controller gained two new bits - with configurable dividers -
according to the public datasheets:
UART_EE_A_REG5[26]:
- 0x0: divide the input clock by 3 (meaning: this internally works
with an 8MHz clock)
- 0x1: use the input clock directly without further division (meaning:
this internally work with an 24MHz clock)
UART_EE_A_REG5[27]:
- 0x0: use the clock as configured in UART_EE_A_REG5[26]
- 0x1: divide the input clock by 2 (meaning: this internally works
with an 12MHz clock)

While writing this email I did some investigation and found that
UART_EE_A_REG5[26] is used in the vendor kernel even for GXL and GXM
SoCs.
So this probably has been introduced with the GXL generation (and thus
is missing on GXBB and earlier SoCs).
Also UART_EE_A_REG5[27] seems to have been introduced with the G12A
generation of SoCs (not surprising since G12A and G12B peripherals are
very similar).

Does the UART controller not work with divide-by-3 (as we have it
today) or are these configurable dividers to reduce jitter?


Best regards,
Martin

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

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Yu Tu]

Hi Martin,
	Thank you very much for your reply.

On 2021/12/25 1:25, Martin Blumenstingl wrote:
> [ EXTERNAL EMAIL ]
> 
> Hello,
> 
> On Tue, Dec 21, 2021 at 8:17 AM Yu Tu <yu.tu@amlogic.com> wrote:
>>
>> The S4 SoC on-chip UART uses a 12M clock as the clock source for
>> calculating the baud rate of the UART. But previously, chips used 24M or
>> other clock sources. So add this change. The specific clock source is
>> determined by chip design.
> Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
> If there's still a 24MHz XTAL then I think this description is not
> correct - at least based on how I understand the UART controller.
> 
The S4 SoC uses 12MHz(UART_EE_A_REG5[27]=0x1,the bit is set in romcode). 
This register description is the same as the G12A and G12B you know.

> SoCs up to GXL and GXM had an internal divide-by-3 (clock divider) in
> the UART controller IP and an external 24MHz XTAL.
> This was not configurable, so the clock for all baud-rates had to be
> derived from an 8MHz (24MHz divided by 3) clock.
> 
> With the A311D (G12B, which is still using an external 24MHz XTAL) SoC
> the UART controller gained two new bits - with configurable dividers -
> according to the public datasheets:
> UART_EE_A_REG5[26]:
> - 0x0: divide the input clock by 3 (meaning: this internally works
> with an 8MHz clock)
> - 0x1: use the input clock directly without further division (meaning:
> this internally work with an 24MHz clock)
> UART_EE_A_REG5[27]:
> - 0x0: use the clock as configured in UART_EE_A_REG5[26]
> - 0x1: divide the input clock by 2 (meaning: this internally works
> with an 12MHz clock)
> 
> While writing this email I did some investigation and found that
> UART_EE_A_REG5[26] is used in the vendor kernel even for GXL and GXM
> SoCs.
> So this probably has been introduced with the GXL generation (and thus
> is missing on GXBB and earlier SoCs).
> Also UART_EE_A_REG5[27] seems to have been introduced with the G12A
> generation of SoCs (not surprising since G12A and G12B peripherals are
> very similar).
> 
> Does the UART controller not work with divide-by-3 (as we have it
> today) or are these configurable dividers to reduce jitter?
> 
The UART controller can work with divide-by-3.
The chip history as you described above, the current reason for using 
12MHz clock is really what you call reduce jitter. The UART mainly 
connects to Bluetooth and uses typical baud rates of 2Mhz, 3MHz and 
4MHz, so 12MHz is used as the clock source.
> 
> Best regards,
> Martin
> 

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Yu Tu]

Hi Martin,
	Thank you very much for your reply.

On 2021/12/25 1:25, Martin Blumenstingl wrote:
> [ EXTERNAL EMAIL ]
> 
> Hello,
> 
> On Tue, Dec 21, 2021 at 8:17 AM Yu Tu <yu.tu@amlogic.com> wrote:
>>
>> The S4 SoC on-chip UART uses a 12M clock as the clock source for
>> calculating the baud rate of the UART. But previously, chips used 24M or
>> other clock sources. So add this change. The specific clock source is
>> determined by chip design.
> Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
> If there's still a 24MHz XTAL then I think this description is not
> correct - at least based on how I understand the UART controller.
> 
The S4 SoC uses 12MHz(UART_EE_A_REG5[27]=0x1,the bit is set in romcode). 
This register description is the same as the G12A and G12B you know.

> SoCs up to GXL and GXM had an internal divide-by-3 (clock divider) in
> the UART controller IP and an external 24MHz XTAL.
> This was not configurable, so the clock for all baud-rates had to be
> derived from an 8MHz (24MHz divided by 3) clock.
> 
> With the A311D (G12B, which is still using an external 24MHz XTAL) SoC
> the UART controller gained two new bits - with configurable dividers -
> according to the public datasheets:
> UART_EE_A_REG5[26]:
> - 0x0: divide the input clock by 3 (meaning: this internally works
> with an 8MHz clock)
> - 0x1: use the input clock directly without further division (meaning:
> this internally work with an 24MHz clock)
> UART_EE_A_REG5[27]:
> - 0x0: use the clock as configured in UART_EE_A_REG5[26]
> - 0x1: divide the input clock by 2 (meaning: this internally works
> with an 12MHz clock)
> 
> While writing this email I did some investigation and found that
> UART_EE_A_REG5[26] is used in the vendor kernel even for GXL and GXM
> SoCs.
> So this probably has been introduced with the GXL generation (and thus
> is missing on GXBB and earlier SoCs).
> Also UART_EE_A_REG5[27] seems to have been introduced with the G12A
> generation of SoCs (not surprising since G12A and G12B peripherals are
> very similar).
> 
> Does the UART controller not work with divide-by-3 (as we have it
> today) or are these configurable dividers to reduce jitter?
> 
The UART controller can work with divide-by-3.
The chip history as you described above, the current reason for using 
12MHz clock is really what you call reduce jitter. The UART mainly 
connects to Bluetooth and uses typical baud rates of 2Mhz, 3MHz and 
4MHz, so 12MHz is used as the clock source.
> 
> Best regards,
> Martin
> 

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

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Yu Tu]

Hi Martin,
	Thank you very much for your reply.

On 2021/12/25 1:25, Martin Blumenstingl wrote:
> [ EXTERNAL EMAIL ]
> 
> Hello,
> 
> On Tue, Dec 21, 2021 at 8:17 AM Yu Tu <yu.tu@amlogic.com> wrote:
>>
>> The S4 SoC on-chip UART uses a 12M clock as the clock source for
>> calculating the baud rate of the UART. But previously, chips used 24M or
>> other clock sources. So add this change. The specific clock source is
>> determined by chip design.
> Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
> If there's still a 24MHz XTAL then I think this description is not
> correct - at least based on how I understand the UART controller.
> 
The S4 SoC uses 12MHz(UART_EE_A_REG5[27]=0x1,the bit is set in romcode). 
This register description is the same as the G12A and G12B you know.

> SoCs up to GXL and GXM had an internal divide-by-3 (clock divider) in
> the UART controller IP and an external 24MHz XTAL.
> This was not configurable, so the clock for all baud-rates had to be
> derived from an 8MHz (24MHz divided by 3) clock.
> 
> With the A311D (G12B, which is still using an external 24MHz XTAL) SoC
> the UART controller gained two new bits - with configurable dividers -
> according to the public datasheets:
> UART_EE_A_REG5[26]:
> - 0x0: divide the input clock by 3 (meaning: this internally works
> with an 8MHz clock)
> - 0x1: use the input clock directly without further division (meaning:
> this internally work with an 24MHz clock)
> UART_EE_A_REG5[27]:
> - 0x0: use the clock as configured in UART_EE_A_REG5[26]
> - 0x1: divide the input clock by 2 (meaning: this internally works
> with an 12MHz clock)
> 
> While writing this email I did some investigation and found that
> UART_EE_A_REG5[26] is used in the vendor kernel even for GXL and GXM
> SoCs.
> So this probably has been introduced with the GXL generation (and thus
> is missing on GXBB and earlier SoCs).
> Also UART_EE_A_REG5[27] seems to have been introduced with the G12A
> generation of SoCs (not surprising since G12A and G12B peripherals are
> very similar).
> 
> Does the UART controller not work with divide-by-3 (as we have it
> today) or are these configurable dividers to reduce jitter?
> 
The UART controller can work with divide-by-3.
The chip history as you described above, the current reason for using 
12MHz clock is really what you call reduce jitter. The UART mainly 
connects to Bluetooth and uses typical baud rates of 2Mhz, 3MHz and 
4MHz, so 12MHz is used as the clock source.
> 
> Best regards,
> Martin
> 

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

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Jerome Brunet]

On Mon 27 Dec 2021 at 14:56, Yu Tu <yu.tu@amlogic.com> wrote:

> Hi Martin,
> 	Thank you very much for your reply.
>
> On 2021/12/25 1:25, Martin Blumenstingl wrote:
>> [ EXTERNAL EMAIL ]
>> Hello,
>> On Tue, Dec 21, 2021 at 8:17 AM Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>> The S4 SoC on-chip UART uses a 12M clock as the clock source for
>>> calculating the baud rate of the UART. But previously, chips used 24M or
>>> other clock sources. So add this change. The specific clock source is
>>> determined by chip design.
>> Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
>> If there's still a 24MHz XTAL then I think this description is not
>> correct - at least based on how I understand the UART controller.
>> 
> The S4 SoC uses 12MHz(UART_EE_A_REG5[27]=0x1,the bit is set in
> romcode). This register description is the same as the G12A and G12B you
> know.
>
>> SoCs up to GXL and GXM had an internal divide-by-3 (clock divider) in
>> the UART controller IP and an external 24MHz XTAL.
>> This was not configurable, so the clock for all baud-rates had to be
>> derived from an 8MHz (24MHz divided by 3) clock.
>> With the A311D (G12B, which is still using an external 24MHz XTAL) SoC
>> the UART controller gained two new bits - with configurable dividers -
>> according to the public datasheets:
>> UART_EE_A_REG5[26]:
>> - 0x0: divide the input clock by 3 (meaning: this internally works
>> with an 8MHz clock)
>> - 0x1: use the input clock directly without further division (meaning:
>> this internally work with an 24MHz clock)
>> UART_EE_A_REG5[27]:
>> - 0x0: use the clock as configured in UART_EE_A_REG5[26]
>> - 0x1: divide the input clock by 2 (meaning: this internally works
>> with an 12MHz clock)
>> While writing this email I did some investigation and found that
>> UART_EE_A_REG5[26] is used in the vendor kernel even for GXL and GXM
>> SoCs.
>> So this probably has been introduced with the GXL generation (and thus
>> is missing on GXBB and earlier SoCs).
>> Also UART_EE_A_REG5[27] seems to have been introduced with the G12A
>> generation of SoCs (not surprising since G12A and G12B peripherals are
>> very similar).
>> Does the UART controller not work with divide-by-3 (as we have it
>> today) or are these configurable dividers to reduce jitter?
>> 
> The UART controller can work with divide-by-3.
> The chip history as you described above, the current reason for using 12MHz
> clock is really what you call reduce jitter. The UART mainly connects to
> Bluetooth and uses typical baud rates of 2Mhz, 3MHz and 4MHz, so 12MHz is
> used as the clock source.

Looks to me that the clock divider above should be modelled properly
with CCF. If you wish the initial Romcode setting to remain untouched,
then don't put CLK_SET_RATE_PARENT to stop rate propagation.

CCF will figure out what the internal rate is. You don't need to device
tree data if things are done properly

>> Best regards,
>> Martin
>> 

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Jerome Brunet]

On Mon 27 Dec 2021 at 14:56, Yu Tu <yu.tu@amlogic.com> wrote:

> Hi Martin,
> 	Thank you very much for your reply.
>
> On 2021/12/25 1:25, Martin Blumenstingl wrote:
>> [ EXTERNAL EMAIL ]
>> Hello,
>> On Tue, Dec 21, 2021 at 8:17 AM Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>> The S4 SoC on-chip UART uses a 12M clock as the clock source for
>>> calculating the baud rate of the UART. But previously, chips used 24M or
>>> other clock sources. So add this change. The specific clock source is
>>> determined by chip design.
>> Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
>> If there's still a 24MHz XTAL then I think this description is not
>> correct - at least based on how I understand the UART controller.
>> 
> The S4 SoC uses 12MHz(UART_EE_A_REG5[27]=0x1,the bit is set in
> romcode). This register description is the same as the G12A and G12B you
> know.
>
>> SoCs up to GXL and GXM had an internal divide-by-3 (clock divider) in
>> the UART controller IP and an external 24MHz XTAL.
>> This was not configurable, so the clock for all baud-rates had to be
>> derived from an 8MHz (24MHz divided by 3) clock.
>> With the A311D (G12B, which is still using an external 24MHz XTAL) SoC
>> the UART controller gained two new bits - with configurable dividers -
>> according to the public datasheets:
>> UART_EE_A_REG5[26]:
>> - 0x0: divide the input clock by 3 (meaning: this internally works
>> with an 8MHz clock)
>> - 0x1: use the input clock directly without further division (meaning:
>> this internally work with an 24MHz clock)
>> UART_EE_A_REG5[27]:
>> - 0x0: use the clock as configured in UART_EE_A_REG5[26]
>> - 0x1: divide the input clock by 2 (meaning: this internally works
>> with an 12MHz clock)
>> While writing this email I did some investigation and found that
>> UART_EE_A_REG5[26] is used in the vendor kernel even for GXL and GXM
>> SoCs.
>> So this probably has been introduced with the GXL generation (and thus
>> is missing on GXBB and earlier SoCs).
>> Also UART_EE_A_REG5[27] seems to have been introduced with the G12A
>> generation of SoCs (not surprising since G12A and G12B peripherals are
>> very similar).
>> Does the UART controller not work with divide-by-3 (as we have it
>> today) or are these configurable dividers to reduce jitter?
>> 
> The UART controller can work with divide-by-3.
> The chip history as you described above, the current reason for using 12MHz
> clock is really what you call reduce jitter. The UART mainly connects to
> Bluetooth and uses typical baud rates of 2Mhz, 3MHz and 4MHz, so 12MHz is
> used as the clock source.

Looks to me that the clock divider above should be modelled properly
with CCF. If you wish the initial Romcode setting to remain untouched,
then don't put CLK_SET_RATE_PARENT to stop rate propagation.

CCF will figure out what the internal rate is. You don't need to device
tree data if things are done properly

>> Best regards,
>> Martin
>> 


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

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Jerome Brunet]

On Mon 27 Dec 2021 at 14:56, Yu Tu <yu.tu@amlogic.com> wrote:

> Hi Martin,
> 	Thank you very much for your reply.
>
> On 2021/12/25 1:25, Martin Blumenstingl wrote:
>> [ EXTERNAL EMAIL ]
>> Hello,
>> On Tue, Dec 21, 2021 at 8:17 AM Yu Tu <yu.tu@amlogic.com> wrote:
>>>
>>> The S4 SoC on-chip UART uses a 12M clock as the clock source for
>>> calculating the baud rate of the UART. But previously, chips used 24M or
>>> other clock sources. So add this change. The specific clock source is
>>> determined by chip design.
>> Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
>> If there's still a 24MHz XTAL then I think this description is not
>> correct - at least based on how I understand the UART controller.
>> 
> The S4 SoC uses 12MHz(UART_EE_A_REG5[27]=0x1,the bit is set in
> romcode). This register description is the same as the G12A and G12B you
> know.
>
>> SoCs up to GXL and GXM had an internal divide-by-3 (clock divider) in
>> the UART controller IP and an external 24MHz XTAL.
>> This was not configurable, so the clock for all baud-rates had to be
>> derived from an 8MHz (24MHz divided by 3) clock.
>> With the A311D (G12B, which is still using an external 24MHz XTAL) SoC
>> the UART controller gained two new bits - with configurable dividers -
>> according to the public datasheets:
>> UART_EE_A_REG5[26]:
>> - 0x0: divide the input clock by 3 (meaning: this internally works
>> with an 8MHz clock)
>> - 0x1: use the input clock directly without further division (meaning:
>> this internally work with an 24MHz clock)
>> UART_EE_A_REG5[27]:
>> - 0x0: use the clock as configured in UART_EE_A_REG5[26]
>> - 0x1: divide the input clock by 2 (meaning: this internally works
>> with an 12MHz clock)
>> While writing this email I did some investigation and found that
>> UART_EE_A_REG5[26] is used in the vendor kernel even for GXL and GXM
>> SoCs.
>> So this probably has been introduced with the GXL generation (and thus
>> is missing on GXBB and earlier SoCs).
>> Also UART_EE_A_REG5[27] seems to have been introduced with the G12A
>> generation of SoCs (not surprising since G12A and G12B peripherals are
>> very similar).
>> Does the UART controller not work with divide-by-3 (as we have it
>> today) or are these configurable dividers to reduce jitter?
>> 
> The UART controller can work with divide-by-3.
> The chip history as you described above, the current reason for using 12MHz
> clock is really what you call reduce jitter. The UART mainly connects to
> Bluetooth and uses typical baud rates of 2Mhz, 3MHz and 4MHz, so 12MHz is
> used as the clock source.

Looks to me that the clock divider above should be modelled properly
with CCF. If you wish the initial Romcode setting to remain untouched,
then don't put CLK_SET_RATE_PARENT to stop rate propagation.

CCF will figure out what the internal rate is. You don't need to device
tree data if things are done properly

>> Best regards,
>> Martin
>> 


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

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Martin Blumenstingl]

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

Hello,

On Mon, Dec 27, 2021 at 7:56 AM Yu Tu <yu.tu@amlogic.com> wrote:
[...]
> > Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
> > If there's still a 24MHz XTAL then I think this description is not
> > correct - at least based on how I understand the UART controller.
> >
> The S4 SoC uses 12MHz(UART_EE_A_REG5[27]=0x1,the bit is set in romcode).
> This register description is the same as the G12A and G12B you know.
Thank you for this explanation!
So the problem is that we're not touching bit 26 and bit 27 - and with
the updated romcode you would not get any serial output since the
divider is calculated off the wrong clock.

I agree with Jerome that we shouldn't put a flag in device-tree.

Also I did some experimenting with Jerome's idea to implement the
clocks using CCF (common clock framework), see the attached patches.
It was a bit tricky because some initial clean-ups were needed in the
serial driver.
Note: I have only briefly tested this on a 32-bit Meson8m2 SoC, see my
attached patches and the clk_summary debugfs output.
In fact, I expect that there are some issues with at least one of the
patches as the whole bit 26 and bit 27 code is untested.

Do you see any problems with this patch?
Could you try to implement CCF support with the idea from the attached
patches (you don't need to re-use them, I just wrote them to make it
clearer in our discussion what we're talking about).


Best regards,
Martin

[-- Attachment #2: 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 #3: 0001-tty-serial-meson-Drop-the-legacy-compatible-strings-.patch --]
[-- Type: text/x-patch, Size: 2747 bytes --]

From 7fe5c609405bd67a2f8ad065d41db4385bbd376a Mon Sep 17 00:00:00 2001
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: Fri, 24 Dec 2021 23:46:37 +0100
Subject: [PATCH 1/3] tty: serial: meson: Drop the legacy compatible strings
 and clock code

All mainline .dts files have been using the stable UART since Linux
4.16. Drop the legacy compatible strings and related clock code.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/tty/serial/meson_uart.c | 34 ++-------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index efee3935917f..e54832401e9d 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -622,10 +622,7 @@ meson_serial_early_console_setup(struct earlycon_device *device, const char *opt
 	device->con->write = meson_serial_early_console_write;
 	return 0;
 }
-/* Legacy bindings, should be removed when no more used */
-OF_EARLYCON_DECLARE(meson, "amlogic,meson-uart",
-		    meson_serial_early_console_setup);
-/* Stable bindings */
+
 OF_EARLYCON_DECLARE(meson, "amlogic,meson-ao-uart",
 		    meson_serial_early_console_setup);
 
@@ -668,25 +665,6 @@ static inline struct clk *meson_uart_probe_clock(struct device *dev,
 	return clk;
 }
 
-/*
- * This function gets clocks in the legacy non-stable DT bindings.
- * This code will be remove once all the platforms switch to the
- * new DT bindings.
- */
-static int meson_uart_probe_clocks_legacy(struct platform_device *pdev,
-					  struct uart_port *port)
-{
-	struct clk *clk = NULL;
-
-	clk = meson_uart_probe_clock(&pdev->dev, NULL);
-	if (IS_ERR(clk))
-		return PTR_ERR(clk);
-
-	port->uartclk = clk_get_rate(clk);
-
-	return 0;
-}
-
 static int meson_uart_probe_clocks(struct platform_device *pdev,
 				   struct uart_port *port)
 {
@@ -754,12 +732,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
-	/* Use legacy way until all platforms switch to new bindings */
-	if (of_device_is_compatible(pdev->dev.of_node, "amlogic,meson-uart"))
-		ret = meson_uart_probe_clocks_legacy(pdev, port);
-	else
-		ret = meson_uart_probe_clocks(pdev, port);
-
+	ret = meson_uart_probe_clocks(pdev, port);
 	if (ret)
 		return ret;
 
@@ -804,9 +777,6 @@ static int meson_uart_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id meson_uart_dt_match[] = {
-	/* Legacy bindings, should be removed when no more used */
-	{ .compatible = "amlogic,meson-uart" },
-	/* Stable bindings */
 	{ .compatible = "amlogic,meson6-uart" },
 	{ .compatible = "amlogic,meson8-uart" },
 	{ .compatible = "amlogic,meson8b-uart" },
-- 
2.34.1


[-- Attachment #4: 0002-tty-serial-meson-Request-the-register-region-in-meso.patch --]
[-- Type: text/x-patch, Size: 2230 bytes --]

From 3ba1deab633c7e4ef7067873da963b41154b93a5 Mon Sep 17 00:00:00 2001
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: Mon, 27 Dec 2021 19:48:43 +0100
Subject: [PATCH 2/3] tty: serial: meson: Request the register region in
 meson_uart_probe()

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: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/tty/serial/meson_uart.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index e54832401e9d..e17bab1b3366 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;
 }
 
@@ -732,6 +719,10 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
+	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
+	if (IS_ERR(port->membase))
+		return PTR_ERR(port->membase);
+
 	ret = meson_uart_probe_clocks(pdev, port);
 	if (ret)
 		return ret;
@@ -753,10 +744,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.34.1


[-- Attachment #5: 0003-tty-serial-meson-UART-clk-test-TODO.patch --]
[-- Type: text/x-patch, Size: 13748 bytes --]

From 271d005efd5ec36f7f376e747dfa966ffd68d727 Mon Sep 17 00:00:00 2001
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: Sat, 25 Dec 2021 01:32:50 +0100
Subject: [PATCH 3/3] tty: serial: meson: UART clk test - TODO

TODO

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/tty/serial/Kconfig      |   1 +
 drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------
 2 files changed, 257 insertions(+), 55 deletions(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index fc543ac97c13..8e1c9c5c2cdf 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -199,6 +199,7 @@ config SERIAL_KGDB_NMI
 config SERIAL_MESON
 	tristate "Meson serial port support"
 	depends on ARCH_MESON || COMPILE_TEST
+	depends on COMMON_CLK
 	select SERIAL_CORE
 	help
 	  This enables the driver for the on-chip UARTs of the Amlogic
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index e17bab1b3366..90068c8f75c8 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,21 @@
 #define AML_UART_POLL_USEC		5
 #define AML_UART_TIMEOUT_USEC		10000
 
+struct meson_uart_data {
+	struct uart_port	port;
+	struct clk		*pclk;
+	struct clk		*baud_clk;
+	struct clk_divider	baud_div;
+	struct clk_mux		use_xtal_mux;
+	struct clk_mux		xtal_clk_sel_mux;
+	struct clk_mux		xtal2_clk_sel_mux;
+	struct clk_fixed_factor	xtal_div2;
+	struct clk_fixed_factor	xtal_div3;
+	struct clk_fixed_factor	clk81_div4;
+	bool			no_clk81_input;
+	bool			has_xtal_clk_sel;
+};
+
 static struct uart_driver meson_uart_driver;
 
 static struct uart_port *meson_ports[AML_UART_PORT_NUM];
@@ -268,14 +282,11 @@ static void meson_uart_reset(struct uart_port *port)
 static int meson_uart_startup(struct uart_port *port)
 {
 	u32 val;
-	int ret = 0;
+	int ret;
 
-	val = readl(port->membase + AML_UART_CONTROL);
-	val |= AML_UART_CLEAR_ERR;
-	writel(val, port->membase + AML_UART_CONTROL);
-	val &= ~AML_UART_CLEAR_ERR;
-	writel(val, port->membase + AML_UART_CONTROL);
+	meson_uart_reset(port);
 
+	val = readl(port->membase + AML_UART_CONTROL);
 	val |= (AML_UART_RX_EN | AML_UART_TX_EN);
 	writel(val, port->membase + AML_UART_CONTROL);
 
@@ -293,19 +304,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 +404,27 @@ 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);
+	clk_disable_unprepare(private_data->pclk);
 }
 
 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->pclk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(private_data->baud_clk);
+	if (ret) {
+		clk_disable_unprepare(private_data->pclk);
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -629,56 +654,175 @@ 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)
+static int meson_uart_register_clk(struct uart_port *port,
+				   const char *name_suffix,
+				   const struct clk_parent_data *parent_data,
+				   unsigned int num_parents,
+				   const struct clk_ops *ops,
+				   struct clk_hw *hw)
 {
-	struct clk *clk = NULL;
+	struct clk_init_data init = { };
+	char clk_name[32];
 	int ret;
 
-	clk = devm_clk_get(dev, id);
-	if (IS_ERR(clk))
-		return clk;
+	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
+		 name_suffix);
 
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		dev_err(dev, "couldn't enable clk\n");
-		return ERR_PTR(ret);
-	}
+	init.name = clk_name;
+	init.ops = ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	init.parent_data = parent_data;
+	init.num_parents = num_parents;
+
+	hw->init = &init;
 
-	devm_add_action_or_reset(dev,
-			(void(*)(void *))clk_disable_unprepare,
-			clk);
+	ret = devm_clk_hw_register(port->dev, hw);
+	if (ret)
+		return dev_err_probe(port->dev, ret,
+				     "Failed to register the '%s' clock\n",
+				     clk_name);
 
-	return clk;
+	return ret;
 }
 
-static int meson_uart_probe_clocks(struct platform_device *pdev,
-				   struct uart_port *port)
-{
-	struct clk *clk_xtal = NULL;
-	struct clk *clk_pclk = NULL;
-	struct clk *clk_baud = NULL;
+static int meson_uart_probe_clocks(struct uart_port *port,
+				   bool register_clk81_div4)
+{
+	struct meson_uart_data *private_data = port->private_data;
+	struct clk_parent_data use_xtal_mux_parents[2] = {
+		{ .index = -1, },
+		{ .index = -1, },
+	};
+	struct clk_parent_data xtal_clk_sel_mux_parents[2] = { };
+	struct clk_parent_data xtal2_clk_sel_mux_parents[2] = { };
+	struct clk_parent_data xtal_div_parent = { .fw_name = "xtal", };
+	struct clk_parent_data clk81_div_parent = { .fw_name = "baud", };
+	struct clk_parent_data baud_div_parent = { };
+	struct clk *clk_baud, *clk_xtal;
+	int ret;
 
-	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
-	if (IS_ERR(clk_pclk))
-		return PTR_ERR(clk_pclk);
+	private_data->pclk = devm_clk_get(port->dev, "pclk");
+	if (IS_ERR(private_data->pclk))
+		return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
+				     "Failed to get the 'pclk' clock\n");
+
+	clk_baud = devm_clk_get(port->dev, "baud");
+	if (IS_ERR(clk_baud))
+		return dev_err_probe(port->dev, PTR_ERR(clk_baud),
+				     "Failed to get the 'baud' clock\n");
 
-	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);
+		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
+				     "Failed to get the 'xtal' clock\n");
+
+	private_data->xtal_div3.mult = 1;
+	private_data->xtal_div3.div = 3;
+	ret = meson_uart_register_clk(port, "xtal_div3", &xtal_div_parent,
+				      1, &clk_fixed_factor_ops,
+				      &private_data->xtal_div3.hw);
+	if (ret)
+		return ret;
 
-	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
-	if (IS_ERR(clk_baud))
-		return PTR_ERR(clk_baud);
+	if (register_clk81_div4) {
+		private_data->clk81_div4.mult = 1;
+		private_data->clk81_div4.div = 4;
+		ret = meson_uart_register_clk(port, "clk81_div4",
+					      &clk81_div_parent, 1,
+					      &clk_fixed_factor_ops,
+					      &private_data->clk81_div4.hw);
+		if (ret)
+			return ret;
+
+		use_xtal_mux_parents[0].hw = &private_data->clk81_div4.hw;
+	}
 
-	port->uartclk = clk_get_rate(clk_baud);
+	if (private_data->has_xtal_clk_sel) {
+		private_data->xtal_div2.mult = 1;
+		private_data->xtal_div2.div = 2;
+		ret = meson_uart_register_clk(port, "xtal_div2",
+					      &xtal_div_parent, 1,
+					      &clk_fixed_factor_ops,
+					      &private_data->xtal_div2.hw);
+		if (ret)
+			return ret;
+
+		xtal_clk_sel_mux_parents[0].hw = &private_data->xtal_div3.hw;
+		xtal_clk_sel_mux_parents[1].fw_name = "xtal";
+
+		private_data->xtal_clk_sel_mux.reg = port->membase + AML_UART_REG5;
+		private_data->xtal_clk_sel_mux.mask = 0x1;
+		private_data->xtal_clk_sel_mux.shift = 26;
+		private_data->xtal_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST;
+		ret = meson_uart_register_clk(port, "xtal_clk_sel",
+					      xtal_clk_sel_mux_parents,
+					      ARRAY_SIZE(xtal_clk_sel_mux_parents),
+					      &clk_mux_ops,
+					      &private_data->xtal_clk_sel_mux.hw);
+		if (ret)
+			return ret;
+
+		xtal2_clk_sel_mux_parents[0].hw = &private_data->xtal_clk_sel_mux.hw;
+		xtal2_clk_sel_mux_parents[1].hw = &private_data->xtal_div2.hw;
+
+		private_data->xtal2_clk_sel_mux.reg = port->membase + AML_UART_REG5;
+		private_data->xtal2_clk_sel_mux.mask = 0x1;
+		private_data->xtal2_clk_sel_mux.shift = 27;
+		private_data->xtal2_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST;
+		ret = meson_uart_register_clk(port, "xtal2_clk_sel",
+					      xtal2_clk_sel_mux_parents,
+					      ARRAY_SIZE(xtal2_clk_sel_mux_parents),
+					      &clk_mux_ops,
+					      &private_data->xtal2_clk_sel_mux.hw);
+		if (ret)
+			return ret;
+
+		use_xtal_mux_parents[1].hw = &private_data->xtal2_clk_sel_mux.hw;
+	} else {
+		use_xtal_mux_parents[1].hw = &private_data->xtal_div3.hw;
+	}
+
+	private_data->use_xtal_mux.reg = port->membase + AML_UART_REG5;
+	private_data->use_xtal_mux.mask = 0x1;
+	private_data->use_xtal_mux.shift = 24;
+	private_data->use_xtal_mux.flags = CLK_MUX_ROUND_CLOSEST;
+	ret = meson_uart_register_clk(port, "use_xtal", use_xtal_mux_parents,
+				      ARRAY_SIZE(use_xtal_mux_parents),
+				      &clk_mux_ops,
+				      &private_data->use_xtal_mux.hw);
+	if (ret)
+		return ret;
+
+	baud_div_parent.hw = &private_data->use_xtal_mux.hw;
+
+	private_data->baud_div.reg = port->membase + AML_UART_REG5;
+	private_data->baud_div.shift = 0;
+	private_data->baud_div.width = 23;
+	private_data->baud_div.flags = CLK_DIVIDER_ROUND_CLOSEST;
+	ret = meson_uart_register_clk(port, "baud_div",
+				      &baud_div_parent, 1,
+				      &clk_divider_ops,
+				      &private_data->baud_div.hw);
+	if (ret)
+		return ret;
+
+	private_data->baud_clk = devm_clk_hw_get_clk(port->dev,
+						     &private_data->baud_div.hw,
+						     "baud_rate");
+	if (IS_ERR(private_data->baud_clk))
+		return dev_err_probe(port->dev,
+				     PTR_ERR(private_data->baud_clk),
+				     "Failed to request the 'baud_rate' clock\n");
 
 	return 0;
 }
 
 static int meson_uart_probe(struct platform_device *pdev)
 {
+	struct meson_uart_data *private_data;
 	struct resource *res_mem, *res_irq;
+	struct clk *clk_baud, *clk_xtal;
+	bool register_clk81_div4;
 	struct uart_port *port;
 	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
 	int ret = 0;
@@ -715,18 +859,37 @@ static int meson_uart_probe(struct platform_device *pdev)
 		return -EBUSY;
 	}
 
-	port = devm_kzalloc(&pdev->dev, sizeof(struct uart_port), GFP_KERNEL);
-	if (!port)
+	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->has_xtal_clk_sel = true;
+
+	private_data->pclk = devm_clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(private_data->pclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(private_data->pclk),
+				     "Failed to get the 'pclk' clock\n");
+
+	clk_baud = devm_clk_get(&pdev->dev, "baud");
+	if (IS_ERR(clk_baud))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk_baud),
+				     "Failed to get the 'baud' clock\n");
+
+	clk_xtal = devm_clk_get(&pdev->dev, "xtal");
+	if (IS_ERR(clk_xtal))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk_xtal),
+				     "Failed to get the 'xtal' clock\n");
+
+	register_clk81_div4 = clk_get_rate(clk_xtal) != clk_get_rate(clk_baud);
+
+	port = &private_data->port;
+
 	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
 	if (IS_ERR(port->membase))
 		return PTR_ERR(port->membase);
 
-	ret = meson_uart_probe_clocks(pdev, port);
-	if (ret)
-		return ret;
-
 	port->iotype = UPIO_MEM;
 	port->mapbase = res_mem->start;
 	port->mapsize = resource_size(res_mem);
@@ -739,6 +902,12 @@ static int meson_uart_probe(struct platform_device *pdev)
 	port->x_char = 0;
 	port->ops = &meson_uart_ops;
 	port->fifosize = fifosize;
+	port->uartclk = clk_get_rate(clk_baud);
+	port->private_data = private_data;
+
+	ret = meson_uart_probe_clocks(port, register_clk81_div4);
+	if (ret)
+		return ret;
 
 	meson_ports[pdev->id] = port;
 	platform_set_drvdata(pdev, port);
@@ -765,10 +934,42 @@ 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-gxbb-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson-gxl-uart",
+		.data = (void *)true,
+	},
+	{
+		.compatible = "amlogic,meson-g12a-uart",
+		.data = (void *)true,
+	},
+	{
+		.compatible = "amlogic,meson-s4-uart",
+		.data = (void *)true,
+	},
+	/*
+	 * deprecated, don't use anymore because it doesn't differentiate
+	 * between GXBB and GXL which have different revisions of the UART IP.
+	 */
+	{
+		.compatible = "amlogic,meson-gx-uart",
+		.data = (void *)false,
+	},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
-- 
2.34.1

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Martin Blumenstingl]

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

Hello,

On Mon, Dec 27, 2021 at 7:56 AM Yu Tu <yu.tu@amlogic.com> wrote:
[...]
> > Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
> > If there's still a 24MHz XTAL then I think this description is not
> > correct - at least based on how I understand the UART controller.
> >
> The S4 SoC uses 12MHz(UART_EE_A_REG5[27]=0x1,the bit is set in romcode).
> This register description is the same as the G12A and G12B you know.
Thank you for this explanation!
So the problem is that we're not touching bit 26 and bit 27 - and with
the updated romcode you would not get any serial output since the
divider is calculated off the wrong clock.

I agree with Jerome that we shouldn't put a flag in device-tree.

Also I did some experimenting with Jerome's idea to implement the
clocks using CCF (common clock framework), see the attached patches.
It was a bit tricky because some initial clean-ups were needed in the
serial driver.
Note: I have only briefly tested this on a 32-bit Meson8m2 SoC, see my
attached patches and the clk_summary debugfs output.
In fact, I expect that there are some issues with at least one of the
patches as the whole bit 26 and bit 27 code is untested.

Do you see any problems with this patch?
Could you try to implement CCF support with the idea from the attached
patches (you don't need to re-use them, I just wrote them to make it
clearer in our discussion what we're talking about).


Best regards,
Martin

[-- Attachment #2: 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 #3: 0001-tty-serial-meson-Drop-the-legacy-compatible-strings-.patch --]
[-- Type: text/x-patch, Size: 2747 bytes --]

From 7fe5c609405bd67a2f8ad065d41db4385bbd376a Mon Sep 17 00:00:00 2001
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: Fri, 24 Dec 2021 23:46:37 +0100
Subject: [PATCH 1/3] tty: serial: meson: Drop the legacy compatible strings
 and clock code

All mainline .dts files have been using the stable UART since Linux
4.16. Drop the legacy compatible strings and related clock code.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/tty/serial/meson_uart.c | 34 ++-------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index efee3935917f..e54832401e9d 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -622,10 +622,7 @@ meson_serial_early_console_setup(struct earlycon_device *device, const char *opt
 	device->con->write = meson_serial_early_console_write;
 	return 0;
 }
-/* Legacy bindings, should be removed when no more used */
-OF_EARLYCON_DECLARE(meson, "amlogic,meson-uart",
-		    meson_serial_early_console_setup);
-/* Stable bindings */
+
 OF_EARLYCON_DECLARE(meson, "amlogic,meson-ao-uart",
 		    meson_serial_early_console_setup);
 
@@ -668,25 +665,6 @@ static inline struct clk *meson_uart_probe_clock(struct device *dev,
 	return clk;
 }
 
-/*
- * This function gets clocks in the legacy non-stable DT bindings.
- * This code will be remove once all the platforms switch to the
- * new DT bindings.
- */
-static int meson_uart_probe_clocks_legacy(struct platform_device *pdev,
-					  struct uart_port *port)
-{
-	struct clk *clk = NULL;
-
-	clk = meson_uart_probe_clock(&pdev->dev, NULL);
-	if (IS_ERR(clk))
-		return PTR_ERR(clk);
-
-	port->uartclk = clk_get_rate(clk);
-
-	return 0;
-}
-
 static int meson_uart_probe_clocks(struct platform_device *pdev,
 				   struct uart_port *port)
 {
@@ -754,12 +732,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
-	/* Use legacy way until all platforms switch to new bindings */
-	if (of_device_is_compatible(pdev->dev.of_node, "amlogic,meson-uart"))
-		ret = meson_uart_probe_clocks_legacy(pdev, port);
-	else
-		ret = meson_uart_probe_clocks(pdev, port);
-
+	ret = meson_uart_probe_clocks(pdev, port);
 	if (ret)
 		return ret;
 
@@ -804,9 +777,6 @@ static int meson_uart_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id meson_uart_dt_match[] = {
-	/* Legacy bindings, should be removed when no more used */
-	{ .compatible = "amlogic,meson-uart" },
-	/* Stable bindings */
 	{ .compatible = "amlogic,meson6-uart" },
 	{ .compatible = "amlogic,meson8-uart" },
 	{ .compatible = "amlogic,meson8b-uart" },
-- 
2.34.1


[-- Attachment #4: 0002-tty-serial-meson-Request-the-register-region-in-meso.patch --]
[-- Type: text/x-patch, Size: 2230 bytes --]

From 3ba1deab633c7e4ef7067873da963b41154b93a5 Mon Sep 17 00:00:00 2001
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: Mon, 27 Dec 2021 19:48:43 +0100
Subject: [PATCH 2/3] tty: serial: meson: Request the register region in
 meson_uart_probe()

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: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/tty/serial/meson_uart.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index e54832401e9d..e17bab1b3366 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;
 }
 
@@ -732,6 +719,10 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
+	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
+	if (IS_ERR(port->membase))
+		return PTR_ERR(port->membase);
+
 	ret = meson_uart_probe_clocks(pdev, port);
 	if (ret)
 		return ret;
@@ -753,10 +744,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.34.1


[-- Attachment #5: 0003-tty-serial-meson-UART-clk-test-TODO.patch --]
[-- Type: text/x-patch, Size: 13748 bytes --]

From 271d005efd5ec36f7f376e747dfa966ffd68d727 Mon Sep 17 00:00:00 2001
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: Sat, 25 Dec 2021 01:32:50 +0100
Subject: [PATCH 3/3] tty: serial: meson: UART clk test - TODO

TODO

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/tty/serial/Kconfig      |   1 +
 drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------
 2 files changed, 257 insertions(+), 55 deletions(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index fc543ac97c13..8e1c9c5c2cdf 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -199,6 +199,7 @@ config SERIAL_KGDB_NMI
 config SERIAL_MESON
 	tristate "Meson serial port support"
 	depends on ARCH_MESON || COMPILE_TEST
+	depends on COMMON_CLK
 	select SERIAL_CORE
 	help
 	  This enables the driver for the on-chip UARTs of the Amlogic
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index e17bab1b3366..90068c8f75c8 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,21 @@
 #define AML_UART_POLL_USEC		5
 #define AML_UART_TIMEOUT_USEC		10000
 
+struct meson_uart_data {
+	struct uart_port	port;
+	struct clk		*pclk;
+	struct clk		*baud_clk;
+	struct clk_divider	baud_div;
+	struct clk_mux		use_xtal_mux;
+	struct clk_mux		xtal_clk_sel_mux;
+	struct clk_mux		xtal2_clk_sel_mux;
+	struct clk_fixed_factor	xtal_div2;
+	struct clk_fixed_factor	xtal_div3;
+	struct clk_fixed_factor	clk81_div4;
+	bool			no_clk81_input;
+	bool			has_xtal_clk_sel;
+};
+
 static struct uart_driver meson_uart_driver;
 
 static struct uart_port *meson_ports[AML_UART_PORT_NUM];
@@ -268,14 +282,11 @@ static void meson_uart_reset(struct uart_port *port)
 static int meson_uart_startup(struct uart_port *port)
 {
 	u32 val;
-	int ret = 0;
+	int ret;
 
-	val = readl(port->membase + AML_UART_CONTROL);
-	val |= AML_UART_CLEAR_ERR;
-	writel(val, port->membase + AML_UART_CONTROL);
-	val &= ~AML_UART_CLEAR_ERR;
-	writel(val, port->membase + AML_UART_CONTROL);
+	meson_uart_reset(port);
 
+	val = readl(port->membase + AML_UART_CONTROL);
 	val |= (AML_UART_RX_EN | AML_UART_TX_EN);
 	writel(val, port->membase + AML_UART_CONTROL);
 
@@ -293,19 +304,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 +404,27 @@ 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);
+	clk_disable_unprepare(private_data->pclk);
 }
 
 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->pclk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(private_data->baud_clk);
+	if (ret) {
+		clk_disable_unprepare(private_data->pclk);
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -629,56 +654,175 @@ 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)
+static int meson_uart_register_clk(struct uart_port *port,
+				   const char *name_suffix,
+				   const struct clk_parent_data *parent_data,
+				   unsigned int num_parents,
+				   const struct clk_ops *ops,
+				   struct clk_hw *hw)
 {
-	struct clk *clk = NULL;
+	struct clk_init_data init = { };
+	char clk_name[32];
 	int ret;
 
-	clk = devm_clk_get(dev, id);
-	if (IS_ERR(clk))
-		return clk;
+	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
+		 name_suffix);
 
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		dev_err(dev, "couldn't enable clk\n");
-		return ERR_PTR(ret);
-	}
+	init.name = clk_name;
+	init.ops = ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	init.parent_data = parent_data;
+	init.num_parents = num_parents;
+
+	hw->init = &init;
 
-	devm_add_action_or_reset(dev,
-			(void(*)(void *))clk_disable_unprepare,
-			clk);
+	ret = devm_clk_hw_register(port->dev, hw);
+	if (ret)
+		return dev_err_probe(port->dev, ret,
+				     "Failed to register the '%s' clock\n",
+				     clk_name);
 
-	return clk;
+	return ret;
 }
 
-static int meson_uart_probe_clocks(struct platform_device *pdev,
-				   struct uart_port *port)
-{
-	struct clk *clk_xtal = NULL;
-	struct clk *clk_pclk = NULL;
-	struct clk *clk_baud = NULL;
+static int meson_uart_probe_clocks(struct uart_port *port,
+				   bool register_clk81_div4)
+{
+	struct meson_uart_data *private_data = port->private_data;
+	struct clk_parent_data use_xtal_mux_parents[2] = {
+		{ .index = -1, },
+		{ .index = -1, },
+	};
+	struct clk_parent_data xtal_clk_sel_mux_parents[2] = { };
+	struct clk_parent_data xtal2_clk_sel_mux_parents[2] = { };
+	struct clk_parent_data xtal_div_parent = { .fw_name = "xtal", };
+	struct clk_parent_data clk81_div_parent = { .fw_name = "baud", };
+	struct clk_parent_data baud_div_parent = { };
+	struct clk *clk_baud, *clk_xtal;
+	int ret;
 
-	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
-	if (IS_ERR(clk_pclk))
-		return PTR_ERR(clk_pclk);
+	private_data->pclk = devm_clk_get(port->dev, "pclk");
+	if (IS_ERR(private_data->pclk))
+		return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
+				     "Failed to get the 'pclk' clock\n");
+
+	clk_baud = devm_clk_get(port->dev, "baud");
+	if (IS_ERR(clk_baud))
+		return dev_err_probe(port->dev, PTR_ERR(clk_baud),
+				     "Failed to get the 'baud' clock\n");
 
-	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);
+		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
+				     "Failed to get the 'xtal' clock\n");
+
+	private_data->xtal_div3.mult = 1;
+	private_data->xtal_div3.div = 3;
+	ret = meson_uart_register_clk(port, "xtal_div3", &xtal_div_parent,
+				      1, &clk_fixed_factor_ops,
+				      &private_data->xtal_div3.hw);
+	if (ret)
+		return ret;
 
-	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
-	if (IS_ERR(clk_baud))
-		return PTR_ERR(clk_baud);
+	if (register_clk81_div4) {
+		private_data->clk81_div4.mult = 1;
+		private_data->clk81_div4.div = 4;
+		ret = meson_uart_register_clk(port, "clk81_div4",
+					      &clk81_div_parent, 1,
+					      &clk_fixed_factor_ops,
+					      &private_data->clk81_div4.hw);
+		if (ret)
+			return ret;
+
+		use_xtal_mux_parents[0].hw = &private_data->clk81_div4.hw;
+	}
 
-	port->uartclk = clk_get_rate(clk_baud);
+	if (private_data->has_xtal_clk_sel) {
+		private_data->xtal_div2.mult = 1;
+		private_data->xtal_div2.div = 2;
+		ret = meson_uart_register_clk(port, "xtal_div2",
+					      &xtal_div_parent, 1,
+					      &clk_fixed_factor_ops,
+					      &private_data->xtal_div2.hw);
+		if (ret)
+			return ret;
+
+		xtal_clk_sel_mux_parents[0].hw = &private_data->xtal_div3.hw;
+		xtal_clk_sel_mux_parents[1].fw_name = "xtal";
+
+		private_data->xtal_clk_sel_mux.reg = port->membase + AML_UART_REG5;
+		private_data->xtal_clk_sel_mux.mask = 0x1;
+		private_data->xtal_clk_sel_mux.shift = 26;
+		private_data->xtal_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST;
+		ret = meson_uart_register_clk(port, "xtal_clk_sel",
+					      xtal_clk_sel_mux_parents,
+					      ARRAY_SIZE(xtal_clk_sel_mux_parents),
+					      &clk_mux_ops,
+					      &private_data->xtal_clk_sel_mux.hw);
+		if (ret)
+			return ret;
+
+		xtal2_clk_sel_mux_parents[0].hw = &private_data->xtal_clk_sel_mux.hw;
+		xtal2_clk_sel_mux_parents[1].hw = &private_data->xtal_div2.hw;
+
+		private_data->xtal2_clk_sel_mux.reg = port->membase + AML_UART_REG5;
+		private_data->xtal2_clk_sel_mux.mask = 0x1;
+		private_data->xtal2_clk_sel_mux.shift = 27;
+		private_data->xtal2_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST;
+		ret = meson_uart_register_clk(port, "xtal2_clk_sel",
+					      xtal2_clk_sel_mux_parents,
+					      ARRAY_SIZE(xtal2_clk_sel_mux_parents),
+					      &clk_mux_ops,
+					      &private_data->xtal2_clk_sel_mux.hw);
+		if (ret)
+			return ret;
+
+		use_xtal_mux_parents[1].hw = &private_data->xtal2_clk_sel_mux.hw;
+	} else {
+		use_xtal_mux_parents[1].hw = &private_data->xtal_div3.hw;
+	}
+
+	private_data->use_xtal_mux.reg = port->membase + AML_UART_REG5;
+	private_data->use_xtal_mux.mask = 0x1;
+	private_data->use_xtal_mux.shift = 24;
+	private_data->use_xtal_mux.flags = CLK_MUX_ROUND_CLOSEST;
+	ret = meson_uart_register_clk(port, "use_xtal", use_xtal_mux_parents,
+				      ARRAY_SIZE(use_xtal_mux_parents),
+				      &clk_mux_ops,
+				      &private_data->use_xtal_mux.hw);
+	if (ret)
+		return ret;
+
+	baud_div_parent.hw = &private_data->use_xtal_mux.hw;
+
+	private_data->baud_div.reg = port->membase + AML_UART_REG5;
+	private_data->baud_div.shift = 0;
+	private_data->baud_div.width = 23;
+	private_data->baud_div.flags = CLK_DIVIDER_ROUND_CLOSEST;
+	ret = meson_uart_register_clk(port, "baud_div",
+				      &baud_div_parent, 1,
+				      &clk_divider_ops,
+				      &private_data->baud_div.hw);
+	if (ret)
+		return ret;
+
+	private_data->baud_clk = devm_clk_hw_get_clk(port->dev,
+						     &private_data->baud_div.hw,
+						     "baud_rate");
+	if (IS_ERR(private_data->baud_clk))
+		return dev_err_probe(port->dev,
+				     PTR_ERR(private_data->baud_clk),
+				     "Failed to request the 'baud_rate' clock\n");
 
 	return 0;
 }
 
 static int meson_uart_probe(struct platform_device *pdev)
 {
+	struct meson_uart_data *private_data;
 	struct resource *res_mem, *res_irq;
+	struct clk *clk_baud, *clk_xtal;
+	bool register_clk81_div4;
 	struct uart_port *port;
 	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
 	int ret = 0;
@@ -715,18 +859,37 @@ static int meson_uart_probe(struct platform_device *pdev)
 		return -EBUSY;
 	}
 
-	port = devm_kzalloc(&pdev->dev, sizeof(struct uart_port), GFP_KERNEL);
-	if (!port)
+	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->has_xtal_clk_sel = true;
+
+	private_data->pclk = devm_clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(private_data->pclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(private_data->pclk),
+				     "Failed to get the 'pclk' clock\n");
+
+	clk_baud = devm_clk_get(&pdev->dev, "baud");
+	if (IS_ERR(clk_baud))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk_baud),
+				     "Failed to get the 'baud' clock\n");
+
+	clk_xtal = devm_clk_get(&pdev->dev, "xtal");
+	if (IS_ERR(clk_xtal))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk_xtal),
+				     "Failed to get the 'xtal' clock\n");
+
+	register_clk81_div4 = clk_get_rate(clk_xtal) != clk_get_rate(clk_baud);
+
+	port = &private_data->port;
+
 	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
 	if (IS_ERR(port->membase))
 		return PTR_ERR(port->membase);
 
-	ret = meson_uart_probe_clocks(pdev, port);
-	if (ret)
-		return ret;
-
 	port->iotype = UPIO_MEM;
 	port->mapbase = res_mem->start;
 	port->mapsize = resource_size(res_mem);
@@ -739,6 +902,12 @@ static int meson_uart_probe(struct platform_device *pdev)
 	port->x_char = 0;
 	port->ops = &meson_uart_ops;
 	port->fifosize = fifosize;
+	port->uartclk = clk_get_rate(clk_baud);
+	port->private_data = private_data;
+
+	ret = meson_uart_probe_clocks(port, register_clk81_div4);
+	if (ret)
+		return ret;
 
 	meson_ports[pdev->id] = port;
 	platform_set_drvdata(pdev, port);
@@ -765,10 +934,42 @@ 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-gxbb-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson-gxl-uart",
+		.data = (void *)true,
+	},
+	{
+		.compatible = "amlogic,meson-g12a-uart",
+		.data = (void *)true,
+	},
+	{
+		.compatible = "amlogic,meson-s4-uart",
+		.data = (void *)true,
+	},
+	/*
+	 * deprecated, don't use anymore because it doesn't differentiate
+	 * between GXBB and GXL which have different revisions of the UART IP.
+	 */
+	{
+		.compatible = "amlogic,meson-gx-uart",
+		.data = (void *)false,
+	},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
-- 
2.34.1


[-- Attachment #6: 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 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Martin Blumenstingl]

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

Hello,

On Mon, Dec 27, 2021 at 7:56 AM Yu Tu <yu.tu@amlogic.com> wrote:
[...]
> > Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
> > If there's still a 24MHz XTAL then I think this description is not
> > correct - at least based on how I understand the UART controller.
> >
> The S4 SoC uses 12MHz(UART_EE_A_REG5[27]=0x1,the bit is set in romcode).
> This register description is the same as the G12A and G12B you know.
Thank you for this explanation!
So the problem is that we're not touching bit 26 and bit 27 - and with
the updated romcode you would not get any serial output since the
divider is calculated off the wrong clock.

I agree with Jerome that we shouldn't put a flag in device-tree.

Also I did some experimenting with Jerome's idea to implement the
clocks using CCF (common clock framework), see the attached patches.
It was a bit tricky because some initial clean-ups were needed in the
serial driver.
Note: I have only briefly tested this on a 32-bit Meson8m2 SoC, see my
attached patches and the clk_summary debugfs output.
In fact, I expect that there are some issues with at least one of the
patches as the whole bit 26 and bit 27 code is untested.

Do you see any problems with this patch?
Could you try to implement CCF support with the idea from the attached
patches (you don't need to re-use them, I just wrote them to make it
clearer in our discussion what we're talking about).


Best regards,
Martin

[-- Attachment #2: 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 #3: 0001-tty-serial-meson-Drop-the-legacy-compatible-strings-.patch --]
[-- Type: text/x-patch, Size: 2747 bytes --]

From 7fe5c609405bd67a2f8ad065d41db4385bbd376a Mon Sep 17 00:00:00 2001
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: Fri, 24 Dec 2021 23:46:37 +0100
Subject: [PATCH 1/3] tty: serial: meson: Drop the legacy compatible strings
 and clock code

All mainline .dts files have been using the stable UART since Linux
4.16. Drop the legacy compatible strings and related clock code.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/tty/serial/meson_uart.c | 34 ++-------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index efee3935917f..e54832401e9d 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -622,10 +622,7 @@ meson_serial_early_console_setup(struct earlycon_device *device, const char *opt
 	device->con->write = meson_serial_early_console_write;
 	return 0;
 }
-/* Legacy bindings, should be removed when no more used */
-OF_EARLYCON_DECLARE(meson, "amlogic,meson-uart",
-		    meson_serial_early_console_setup);
-/* Stable bindings */
+
 OF_EARLYCON_DECLARE(meson, "amlogic,meson-ao-uart",
 		    meson_serial_early_console_setup);
 
@@ -668,25 +665,6 @@ static inline struct clk *meson_uart_probe_clock(struct device *dev,
 	return clk;
 }
 
-/*
- * This function gets clocks in the legacy non-stable DT bindings.
- * This code will be remove once all the platforms switch to the
- * new DT bindings.
- */
-static int meson_uart_probe_clocks_legacy(struct platform_device *pdev,
-					  struct uart_port *port)
-{
-	struct clk *clk = NULL;
-
-	clk = meson_uart_probe_clock(&pdev->dev, NULL);
-	if (IS_ERR(clk))
-		return PTR_ERR(clk);
-
-	port->uartclk = clk_get_rate(clk);
-
-	return 0;
-}
-
 static int meson_uart_probe_clocks(struct platform_device *pdev,
 				   struct uart_port *port)
 {
@@ -754,12 +732,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
-	/* Use legacy way until all platforms switch to new bindings */
-	if (of_device_is_compatible(pdev->dev.of_node, "amlogic,meson-uart"))
-		ret = meson_uart_probe_clocks_legacy(pdev, port);
-	else
-		ret = meson_uart_probe_clocks(pdev, port);
-
+	ret = meson_uart_probe_clocks(pdev, port);
 	if (ret)
 		return ret;
 
@@ -804,9 +777,6 @@ static int meson_uart_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id meson_uart_dt_match[] = {
-	/* Legacy bindings, should be removed when no more used */
-	{ .compatible = "amlogic,meson-uart" },
-	/* Stable bindings */
 	{ .compatible = "amlogic,meson6-uart" },
 	{ .compatible = "amlogic,meson8-uart" },
 	{ .compatible = "amlogic,meson8b-uart" },
-- 
2.34.1


[-- Attachment #4: 0002-tty-serial-meson-Request-the-register-region-in-meso.patch --]
[-- Type: text/x-patch, Size: 2230 bytes --]

From 3ba1deab633c7e4ef7067873da963b41154b93a5 Mon Sep 17 00:00:00 2001
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: Mon, 27 Dec 2021 19:48:43 +0100
Subject: [PATCH 2/3] tty: serial: meson: Request the register region in
 meson_uart_probe()

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: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/tty/serial/meson_uart.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index e54832401e9d..e17bab1b3366 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;
 }
 
@@ -732,6 +719,10 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
+	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
+	if (IS_ERR(port->membase))
+		return PTR_ERR(port->membase);
+
 	ret = meson_uart_probe_clocks(pdev, port);
 	if (ret)
 		return ret;
@@ -753,10 +744,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.34.1


[-- Attachment #5: 0003-tty-serial-meson-UART-clk-test-TODO.patch --]
[-- Type: text/x-patch, Size: 13748 bytes --]

From 271d005efd5ec36f7f376e747dfa966ffd68d727 Mon Sep 17 00:00:00 2001
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: Sat, 25 Dec 2021 01:32:50 +0100
Subject: [PATCH 3/3] tty: serial: meson: UART clk test - TODO

TODO

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/tty/serial/Kconfig      |   1 +
 drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------
 2 files changed, 257 insertions(+), 55 deletions(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index fc543ac97c13..8e1c9c5c2cdf 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -199,6 +199,7 @@ config SERIAL_KGDB_NMI
 config SERIAL_MESON
 	tristate "Meson serial port support"
 	depends on ARCH_MESON || COMPILE_TEST
+	depends on COMMON_CLK
 	select SERIAL_CORE
 	help
 	  This enables the driver for the on-chip UARTs of the Amlogic
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index e17bab1b3366..90068c8f75c8 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,21 @@
 #define AML_UART_POLL_USEC		5
 #define AML_UART_TIMEOUT_USEC		10000
 
+struct meson_uart_data {
+	struct uart_port	port;
+	struct clk		*pclk;
+	struct clk		*baud_clk;
+	struct clk_divider	baud_div;
+	struct clk_mux		use_xtal_mux;
+	struct clk_mux		xtal_clk_sel_mux;
+	struct clk_mux		xtal2_clk_sel_mux;
+	struct clk_fixed_factor	xtal_div2;
+	struct clk_fixed_factor	xtal_div3;
+	struct clk_fixed_factor	clk81_div4;
+	bool			no_clk81_input;
+	bool			has_xtal_clk_sel;
+};
+
 static struct uart_driver meson_uart_driver;
 
 static struct uart_port *meson_ports[AML_UART_PORT_NUM];
@@ -268,14 +282,11 @@ static void meson_uart_reset(struct uart_port *port)
 static int meson_uart_startup(struct uart_port *port)
 {
 	u32 val;
-	int ret = 0;
+	int ret;
 
-	val = readl(port->membase + AML_UART_CONTROL);
-	val |= AML_UART_CLEAR_ERR;
-	writel(val, port->membase + AML_UART_CONTROL);
-	val &= ~AML_UART_CLEAR_ERR;
-	writel(val, port->membase + AML_UART_CONTROL);
+	meson_uart_reset(port);
 
+	val = readl(port->membase + AML_UART_CONTROL);
 	val |= (AML_UART_RX_EN | AML_UART_TX_EN);
 	writel(val, port->membase + AML_UART_CONTROL);
 
@@ -293,19 +304,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 +404,27 @@ 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);
+	clk_disable_unprepare(private_data->pclk);
 }
 
 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->pclk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(private_data->baud_clk);
+	if (ret) {
+		clk_disable_unprepare(private_data->pclk);
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -629,56 +654,175 @@ 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)
+static int meson_uart_register_clk(struct uart_port *port,
+				   const char *name_suffix,
+				   const struct clk_parent_data *parent_data,
+				   unsigned int num_parents,
+				   const struct clk_ops *ops,
+				   struct clk_hw *hw)
 {
-	struct clk *clk = NULL;
+	struct clk_init_data init = { };
+	char clk_name[32];
 	int ret;
 
-	clk = devm_clk_get(dev, id);
-	if (IS_ERR(clk))
-		return clk;
+	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
+		 name_suffix);
 
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		dev_err(dev, "couldn't enable clk\n");
-		return ERR_PTR(ret);
-	}
+	init.name = clk_name;
+	init.ops = ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	init.parent_data = parent_data;
+	init.num_parents = num_parents;
+
+	hw->init = &init;
 
-	devm_add_action_or_reset(dev,
-			(void(*)(void *))clk_disable_unprepare,
-			clk);
+	ret = devm_clk_hw_register(port->dev, hw);
+	if (ret)
+		return dev_err_probe(port->dev, ret,
+				     "Failed to register the '%s' clock\n",
+				     clk_name);
 
-	return clk;
+	return ret;
 }
 
-static int meson_uart_probe_clocks(struct platform_device *pdev,
-				   struct uart_port *port)
-{
-	struct clk *clk_xtal = NULL;
-	struct clk *clk_pclk = NULL;
-	struct clk *clk_baud = NULL;
+static int meson_uart_probe_clocks(struct uart_port *port,
+				   bool register_clk81_div4)
+{
+	struct meson_uart_data *private_data = port->private_data;
+	struct clk_parent_data use_xtal_mux_parents[2] = {
+		{ .index = -1, },
+		{ .index = -1, },
+	};
+	struct clk_parent_data xtal_clk_sel_mux_parents[2] = { };
+	struct clk_parent_data xtal2_clk_sel_mux_parents[2] = { };
+	struct clk_parent_data xtal_div_parent = { .fw_name = "xtal", };
+	struct clk_parent_data clk81_div_parent = { .fw_name = "baud", };
+	struct clk_parent_data baud_div_parent = { };
+	struct clk *clk_baud, *clk_xtal;
+	int ret;
 
-	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
-	if (IS_ERR(clk_pclk))
-		return PTR_ERR(clk_pclk);
+	private_data->pclk = devm_clk_get(port->dev, "pclk");
+	if (IS_ERR(private_data->pclk))
+		return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
+				     "Failed to get the 'pclk' clock\n");
+
+	clk_baud = devm_clk_get(port->dev, "baud");
+	if (IS_ERR(clk_baud))
+		return dev_err_probe(port->dev, PTR_ERR(clk_baud),
+				     "Failed to get the 'baud' clock\n");
 
-	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);
+		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
+				     "Failed to get the 'xtal' clock\n");
+
+	private_data->xtal_div3.mult = 1;
+	private_data->xtal_div3.div = 3;
+	ret = meson_uart_register_clk(port, "xtal_div3", &xtal_div_parent,
+				      1, &clk_fixed_factor_ops,
+				      &private_data->xtal_div3.hw);
+	if (ret)
+		return ret;
 
-	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
-	if (IS_ERR(clk_baud))
-		return PTR_ERR(clk_baud);
+	if (register_clk81_div4) {
+		private_data->clk81_div4.mult = 1;
+		private_data->clk81_div4.div = 4;
+		ret = meson_uart_register_clk(port, "clk81_div4",
+					      &clk81_div_parent, 1,
+					      &clk_fixed_factor_ops,
+					      &private_data->clk81_div4.hw);
+		if (ret)
+			return ret;
+
+		use_xtal_mux_parents[0].hw = &private_data->clk81_div4.hw;
+	}
 
-	port->uartclk = clk_get_rate(clk_baud);
+	if (private_data->has_xtal_clk_sel) {
+		private_data->xtal_div2.mult = 1;
+		private_data->xtal_div2.div = 2;
+		ret = meson_uart_register_clk(port, "xtal_div2",
+					      &xtal_div_parent, 1,
+					      &clk_fixed_factor_ops,
+					      &private_data->xtal_div2.hw);
+		if (ret)
+			return ret;
+
+		xtal_clk_sel_mux_parents[0].hw = &private_data->xtal_div3.hw;
+		xtal_clk_sel_mux_parents[1].fw_name = "xtal";
+
+		private_data->xtal_clk_sel_mux.reg = port->membase + AML_UART_REG5;
+		private_data->xtal_clk_sel_mux.mask = 0x1;
+		private_data->xtal_clk_sel_mux.shift = 26;
+		private_data->xtal_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST;
+		ret = meson_uart_register_clk(port, "xtal_clk_sel",
+					      xtal_clk_sel_mux_parents,
+					      ARRAY_SIZE(xtal_clk_sel_mux_parents),
+					      &clk_mux_ops,
+					      &private_data->xtal_clk_sel_mux.hw);
+		if (ret)
+			return ret;
+
+		xtal2_clk_sel_mux_parents[0].hw = &private_data->xtal_clk_sel_mux.hw;
+		xtal2_clk_sel_mux_parents[1].hw = &private_data->xtal_div2.hw;
+
+		private_data->xtal2_clk_sel_mux.reg = port->membase + AML_UART_REG5;
+		private_data->xtal2_clk_sel_mux.mask = 0x1;
+		private_data->xtal2_clk_sel_mux.shift = 27;
+		private_data->xtal2_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST;
+		ret = meson_uart_register_clk(port, "xtal2_clk_sel",
+					      xtal2_clk_sel_mux_parents,
+					      ARRAY_SIZE(xtal2_clk_sel_mux_parents),
+					      &clk_mux_ops,
+					      &private_data->xtal2_clk_sel_mux.hw);
+		if (ret)
+			return ret;
+
+		use_xtal_mux_parents[1].hw = &private_data->xtal2_clk_sel_mux.hw;
+	} else {
+		use_xtal_mux_parents[1].hw = &private_data->xtal_div3.hw;
+	}
+
+	private_data->use_xtal_mux.reg = port->membase + AML_UART_REG5;
+	private_data->use_xtal_mux.mask = 0x1;
+	private_data->use_xtal_mux.shift = 24;
+	private_data->use_xtal_mux.flags = CLK_MUX_ROUND_CLOSEST;
+	ret = meson_uart_register_clk(port, "use_xtal", use_xtal_mux_parents,
+				      ARRAY_SIZE(use_xtal_mux_parents),
+				      &clk_mux_ops,
+				      &private_data->use_xtal_mux.hw);
+	if (ret)
+		return ret;
+
+	baud_div_parent.hw = &private_data->use_xtal_mux.hw;
+
+	private_data->baud_div.reg = port->membase + AML_UART_REG5;
+	private_data->baud_div.shift = 0;
+	private_data->baud_div.width = 23;
+	private_data->baud_div.flags = CLK_DIVIDER_ROUND_CLOSEST;
+	ret = meson_uart_register_clk(port, "baud_div",
+				      &baud_div_parent, 1,
+				      &clk_divider_ops,
+				      &private_data->baud_div.hw);
+	if (ret)
+		return ret;
+
+	private_data->baud_clk = devm_clk_hw_get_clk(port->dev,
+						     &private_data->baud_div.hw,
+						     "baud_rate");
+	if (IS_ERR(private_data->baud_clk))
+		return dev_err_probe(port->dev,
+				     PTR_ERR(private_data->baud_clk),
+				     "Failed to request the 'baud_rate' clock\n");
 
 	return 0;
 }
 
 static int meson_uart_probe(struct platform_device *pdev)
 {
+	struct meson_uart_data *private_data;
 	struct resource *res_mem, *res_irq;
+	struct clk *clk_baud, *clk_xtal;
+	bool register_clk81_div4;
 	struct uart_port *port;
 	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
 	int ret = 0;
@@ -715,18 +859,37 @@ static int meson_uart_probe(struct platform_device *pdev)
 		return -EBUSY;
 	}
 
-	port = devm_kzalloc(&pdev->dev, sizeof(struct uart_port), GFP_KERNEL);
-	if (!port)
+	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->has_xtal_clk_sel = true;
+
+	private_data->pclk = devm_clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(private_data->pclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(private_data->pclk),
+				     "Failed to get the 'pclk' clock\n");
+
+	clk_baud = devm_clk_get(&pdev->dev, "baud");
+	if (IS_ERR(clk_baud))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk_baud),
+				     "Failed to get the 'baud' clock\n");
+
+	clk_xtal = devm_clk_get(&pdev->dev, "xtal");
+	if (IS_ERR(clk_xtal))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk_xtal),
+				     "Failed to get the 'xtal' clock\n");
+
+	register_clk81_div4 = clk_get_rate(clk_xtal) != clk_get_rate(clk_baud);
+
+	port = &private_data->port;
+
 	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
 	if (IS_ERR(port->membase))
 		return PTR_ERR(port->membase);
 
-	ret = meson_uart_probe_clocks(pdev, port);
-	if (ret)
-		return ret;
-
 	port->iotype = UPIO_MEM;
 	port->mapbase = res_mem->start;
 	port->mapsize = resource_size(res_mem);
@@ -739,6 +902,12 @@ static int meson_uart_probe(struct platform_device *pdev)
 	port->x_char = 0;
 	port->ops = &meson_uart_ops;
 	port->fifosize = fifosize;
+	port->uartclk = clk_get_rate(clk_baud);
+	port->private_data = private_data;
+
+	ret = meson_uart_probe_clocks(port, register_clk81_div4);
+	if (ret)
+		return ret;
 
 	meson_ports[pdev->id] = port;
 	platform_set_drvdata(pdev, port);
@@ -765,10 +934,42 @@ 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-gxbb-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson-gxl-uart",
+		.data = (void *)true,
+	},
+	{
+		.compatible = "amlogic,meson-g12a-uart",
+		.data = (void *)true,
+	},
+	{
+		.compatible = "amlogic,meson-s4-uart",
+		.data = (void *)true,
+	},
+	/*
+	 * deprecated, don't use anymore because it doesn't differentiate
+	 * between GXBB and GXL which have different revisions of the UART IP.
+	 */
+	{
+		.compatible = "amlogic,meson-gx-uart",
+		.data = (void *)false,
+	},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
-- 
2.34.1


[-- Attachment #6: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Yu Tu]

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

Hi Martin and Jerome，
	Thank you very much for your reply. I have learned a lot from your 
communication.

On 2021/12/28 4:04, Martin Blumenstingl wrote:
> [ EXTERNAL EMAIL ]
> 
> Hello,
> 
> On Mon, Dec 27, 2021 at 7:56 AM Yu Tu <yu.tu@amlogic.com> wrote:
> [...]
>>> Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
>>> If there's still a 24MHz XTAL then I think this description is not
>>> correct - at least based on how I understand the UART controller.
>>>
>> The S4 SoC uses 12MHz(UART_EE_A_REG5[27]=0x1,the bit is set in romcode).
>> This register description is the same as the G12A and G12B you know.
> Thank you for this explanation!
> So the problem is that we're not touching bit 26 and bit 27 - and with
> the updated romcode you would not get any serial output since the
> divider is calculated off the wrong clock.
> 
> I agree with Jerome that we shouldn't put a flag in device-tree.
> 
> Also I did some experimenting with Jerome's idea to implement the
> clocks using CCF (common clock framework), see the attached patches.
> It was a bit tricky because some initial clean-ups were needed in the
> serial driver.
> Note: I have only briefly tested this on a 32-bit Meson8m2 SoC, see my
> attached patches and the clk_summary debugfs output.
> In fact, I expect that there are some issues with at least one of the
> patches as the whole bit 26 and bit 27 code is untested.
> 
> Do you see any problems with this patch?
> Could you try to implement CCF support with the idea from the attached
> patches (you don't need to re-use them, I just wrote them to make it
> clearer in our discussion what we're talking about).
> 
I couldn't agree with you more. I have verified it on a 64-bit S4 
platform. Please refer to the attachment for verification output 
information.
I will prepare the second version of patch according to the example 
ideas you provided.
> 
> Best regards,
> Martin

[-- Attachment #2: s4-clk-debug-output.txt --]
[-- Type: text/plain, Size: 1440 bytes --]

# cat /sys/kernel/debug/clk/clk_summary 
                                 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_div2         1        1        0    12000000          0     0  50000         Y
       fe07a000.serial#xtal2_clk_sel       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
    fe07a000.serial#xtal_div3         0        0        0     8000000          0     0  50000         Y
       fe07a000.serial#xtal_clk_sel       0        0        0     8000000          0     0  50000         Y

# 
# stty -F /dev/ttyAML0  115200

# stty -F /dev/ttyAML0 
speed 115200 baud; line = 0;
intr = ^C; quit = ^\; erase = ^?; kill = ^U; eof = ^D; eol = <undef>;
eol2 = <undef>; swtch = <undef>; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R;
werase = ^W; lnext = ^V; flush = ^O; min = 1; time = 0;
-brkint ixoff -imaxbel

Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Yu Tu]

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

Hi Martin and Jerome，
	Thank you very much for your reply. I have learned a lot from your 
communication.

On 2021/12/28 4:04, Martin Blumenstingl wrote:
> [ EXTERNAL EMAIL ]
> 
> Hello,
> 
> On Mon, Dec 27, 2021 at 7:56 AM Yu Tu <yu.tu@amlogic.com> wrote:
> [...]
>>> Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
>>> If there's still a 24MHz XTAL then I think this description is not
>>> correct - at least based on how I understand the UART controller.
>>>
>> The S4 SoC uses 12MHz(UART_EE_A_REG5[27]=0x1,the bit is set in romcode).
>> This register description is the same as the G12A and G12B you know.
> Thank you for this explanation!
> So the problem is that we're not touching bit 26 and bit 27 - and with
> the updated romcode you would not get any serial output since the
> divider is calculated off the wrong clock.
> 
> I agree with Jerome that we shouldn't put a flag in device-tree.
> 
> Also I did some experimenting with Jerome's idea to implement the
> clocks using CCF (common clock framework), see the attached patches.
> It was a bit tricky because some initial clean-ups were needed in the
> serial driver.
> Note: I have only briefly tested this on a 32-bit Meson8m2 SoC, see my
> attached patches and the clk_summary debugfs output.
> In fact, I expect that there are some issues with at least one of the
> patches as the whole bit 26 and bit 27 code is untested.
> 
> Do you see any problems with this patch?
> Could you try to implement CCF support with the idea from the attached
> patches (you don't need to re-use them, I just wrote them to make it
> clearer in our discussion what we're talking about).
> 
I couldn't agree with you more. I have verified it on a 64-bit S4 
platform. Please refer to the attachment for verification output 
information.
I will prepare the second version of patch according to the example 
ideas you provided.
> 
> Best regards,
> Martin

[-- Attachment #2: s4-clk-debug-output.txt --]
[-- Type: text/plain, Size: 1440 bytes --]

# cat /sys/kernel/debug/clk/clk_summary 
                                 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_div2         1        1        0    12000000          0     0  50000         Y
       fe07a000.serial#xtal2_clk_sel       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
    fe07a000.serial#xtal_div3         0        0        0     8000000          0     0  50000         Y
       fe07a000.serial#xtal_clk_sel       0        0        0     8000000          0     0  50000         Y

# 
# stty -F /dev/ttyAML0  115200

# stty -F /dev/ttyAML0 
speed 115200 baud; line = 0;
intr = ^C; quit = ^\; erase = ^?; kill = ^U; eof = ^D; eol = <undef>;
eol2 = <undef>; swtch = <undef>; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R;
werase = ^W; lnext = ^V; flush = ^O; min = 1; time = 0;
-brkint ixoff -imaxbel

[-- Attachment #3: 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 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip

[Yu Tu]

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

Hi Martin and Jerome，
	Thank you very much for your reply. I have learned a lot from your 
communication.

On 2021/12/28 4:04, Martin Blumenstingl wrote:
> [ EXTERNAL EMAIL ]
> 
> Hello,
> 
> On Mon, Dec 27, 2021 at 7:56 AM Yu Tu <yu.tu@amlogic.com> wrote:
> [...]
>>> Does the new S4 SoC use an external 12MHz XTAL or does it use a 24MHz XTAL?
>>> If there's still a 24MHz XTAL then I think this description is not
>>> correct - at least based on how I understand the UART controller.
>>>
>> The S4 SoC uses 12MHz(UART_EE_A_REG5[27]=0x1,the bit is set in romcode).
>> This register description is the same as the G12A and G12B you know.
> Thank you for this explanation!
> So the problem is that we're not touching bit 26 and bit 27 - and with
> the updated romcode you would not get any serial output since the
> divider is calculated off the wrong clock.
> 
> I agree with Jerome that we shouldn't put a flag in device-tree.
> 
> Also I did some experimenting with Jerome's idea to implement the
> clocks using CCF (common clock framework), see the attached patches.
> It was a bit tricky because some initial clean-ups were needed in the
> serial driver.
> Note: I have only briefly tested this on a 32-bit Meson8m2 SoC, see my
> attached patches and the clk_summary debugfs output.
> In fact, I expect that there are some issues with at least one of the
> patches as the whole bit 26 and bit 27 code is untested.
> 
> Do you see any problems with this patch?
> Could you try to implement CCF support with the idea from the attached
> patches (you don't need to re-use them, I just wrote them to make it
> clearer in our discussion what we're talking about).
> 
I couldn't agree with you more. I have verified it on a 64-bit S4 
platform. Please refer to the attachment for verification output 
information.
I will prepare the second version of patch according to the example 
ideas you provided.
> 
> Best regards,
> Martin

[-- Attachment #2: s4-clk-debug-output.txt --]
[-- Type: text/plain, Size: 1440 bytes --]

# cat /sys/kernel/debug/clk/clk_summary 
                                 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_div2         1        1        0    12000000          0     0  50000         Y
       fe07a000.serial#xtal2_clk_sel       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
    fe07a000.serial#xtal_div3         0        0        0     8000000          0     0  50000         Y
       fe07a000.serial#xtal_clk_sel       0        0        0     8000000          0     0  50000         Y

# 
# stty -F /dev/ttyAML0  115200

# stty -F /dev/ttyAML0 
speed 115200 baud; line = 0;
intr = ^C; quit = ^\; erase = ^?; kill = ^U; eof = ^D; eol = <undef>;
eol2 = <undef>; swtch = <undef>; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R;
werase = ^W; lnext = ^V; flush = ^O; min = 1; time = 0;
-brkint ixoff -imaxbel

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

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