[PATCH 0/6] tty: serial: lpuart: add imx7ulp support

[PATCH 0/6] tty: serial: lpuart: add imx7ulp support

[Dong Aisheng]

This patch series mainly intends to add imx7ulp support which is also
using FSL lpuart.

The lpuart in imx7ulp is basically the same as ls1021a. It's also
32 bit width register, but unlike ls1021a, it's little endian.
Besides that, imx7ulp lpuart has a minor different register layout
from ls1021a that it has four extra registers (verid, param, global,
pincfg) located at the beginning of register map, which are currently
not used by the driver and less to be used later.

Furthermore, this patch serial also add a new more accurate baud rate
calculation method as MX7ULP can't divide a suitable baud rate
with the default setting.

Currently the new baud rate calculation is only enabled on MX7ULP.
However, i guess the Layerscape may also be able to use it as there
seems to be no difference in baud rate setting register after checking
the Layerscape Reference Manual.

As i don't have Layerscape boards, i can't test it, so i only enable it
for MX7ULP by default to avoid a potential break.

I copied LayerScape guys in this series and hope they can help test later.
If it works on Layerscape as well, then they can switch to the new setting
too and totally remove the old stuff.

Dong Aisheng (6):
  tty: serial: lpuart: introduce lpuart_soc_data to represent SoC
    property
  tty: serial: lpuart: add little endian 32 bit register support
  dt-bindings: serial: fsl-lpuart: add i.MX7ULP support
  tty: serial: lpuart: add imx7ulp support
  tty: serial: lpuart: add earlycon support for imx7ulp
  tty: serial: lpuart: add a more accurate baud rate calculation method

 .../devicetree/bindings/serial/fsl-lpuart.txt      |   2 +
 drivers/tty/serial/fsl_lpuart.c                    | 149 ++++++++++++++++++---
 2 files changed, 136 insertions(+), 15 deletions(-)

-- 
2.7.4

[PATCH 1/6] tty: serial: lpuart: introduce lpuart_soc_data to represent SoC property

[Dong Aisheng]

This is used to dynamically check the SoC specific lpuart properies.
Currently only the checking of 32 bit register width is added which
functions the same as before. More will be added later for supporting
new chips.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Fugang Duan <fugang.duan@nxp.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/tty/serial/fsl_lpuart.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 15df1ba7..cd4e905 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -258,13 +258,21 @@ struct lpuart_port {
 	wait_queue_head_t	dma_wait;
 };
 
+struct lpuart_soc_data {
+	bool	is_32;
+};
+
+static struct lpuart_soc_data vf_data = {
+	.is_32 = false,
+};
+
+static struct lpuart_soc_data ls_data = {
+	.is_32 = true,
+};
+
 static const struct of_device_id lpuart_dt_ids[] = {
-	{
-		.compatible = "fsl,vf610-lpuart",
-	},
-	{
-		.compatible = "fsl,ls1021a-lpuart",
-	},
+	{ .compatible = "fsl,vf610-lpuart", .data = &vf_data, },
+	{ .compatible = "fsl,ls1021a-lpuart", .data = &ls_data, },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
@@ -1971,6 +1979,9 @@ static struct uart_driver lpuart_reg = {
 
 static int lpuart_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *of_id = of_match_device(lpuart_dt_ids,
+							   &pdev->dev);
+	const struct lpuart_soc_data *sdata = of_id->data;
 	struct device_node *np = pdev->dev.of_node;
 	struct lpuart_port *sport;
 	struct resource *res;
@@ -1988,7 +1999,7 @@ static int lpuart_probe(struct platform_device *pdev)
 		return ret;
 	}
 	sport->port.line = ret;
-	sport->lpuart32 = of_device_is_compatible(np, "fsl,ls1021a-lpuart");
+	sport->lpuart32 = sdata->is_32;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	sport->port.membase = devm_ioremap_resource(&pdev->dev, res);
-- 
2.7.4

[PATCH 2/6] tty: serial: lpuart: add little endian 32 bit register support

[Dong Aisheng]

It's based on the exist lpuart32 read/write implementation.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com> (supporter:TTY LAYER)
Cc: Fugang Duan <fugang.duan@nxp.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/tty/serial/fsl_lpuart.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index cd4e905..bddd041 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -231,6 +231,8 @@
 #define DEV_NAME	"ttyLP"
 #define UART_NR		6
 
+static bool lpuart_is_be;
+
 struct lpuart_port {
 	struct uart_port	port;
 	struct clk		*clk;
@@ -260,6 +262,7 @@ struct lpuart_port {
 
 struct lpuart_soc_data {
 	bool	is_32;
+	bool	is_be;
 };
 
 static struct lpuart_soc_data vf_data = {
@@ -268,6 +271,7 @@ static struct lpuart_soc_data vf_data = {
 
 static struct lpuart_soc_data ls_data = {
 	.is_32 = true,
+	.is_be = true,
 };
 
 static const struct of_device_id lpuart_dt_ids[] = {
@@ -282,12 +286,15 @@ static void lpuart_dma_tx_complete(void *arg);
 
 static u32 lpuart32_read(void __iomem *addr)
 {
-	return ioread32be(addr);
+	return lpuart_is_be ? ioread32be(addr) : readl(addr);
 }
 
 static void lpuart32_write(u32 val, void __iomem *addr)
 {
-	iowrite32be(val, addr);
+	if (lpuart_is_be)
+		iowrite32be(val, addr);
+	else
+		writel(val, addr);
 }
 
 static void lpuart_stop_tx(struct uart_port *port)
@@ -2000,6 +2007,7 @@ static int lpuart_probe(struct platform_device *pdev)
 	}
 	sport->port.line = ret;
 	sport->lpuart32 = sdata->is_32;
+	lpuart_is_be = sdata->is_be;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	sport->port.membase = devm_ioremap_resource(&pdev->dev, res);
-- 
2.7.4

[PATCH 3/6] dt-bindings: serial: fsl-lpuart: add i.MX7ULP support

[Dong Aisheng]

The lpuart of imx7ulp is basically the same as ls1021a. It's also
32 bit width register, but unlike ls1021a, it's little endian.
Besides that, imx7ulp lpuart has a minor different register layout
from ls1021a.

Cc: devicetree@vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Fugang Duan <fugang.duan@nxp.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 Documentation/devicetree/bindings/serial/fsl-lpuart.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/fsl-lpuart.txt b/Documentation/devicetree/bindings/serial/fsl-lpuart.txt
index c95005e..a1252a0 100644
--- a/Documentation/devicetree/bindings/serial/fsl-lpuart.txt
+++ b/Documentation/devicetree/bindings/serial/fsl-lpuart.txt
@@ -6,6 +6,8 @@ Required properties:
     on Vybrid vf610 SoC with 8-bit register organization
   - "fsl,ls1021a-lpuart" for lpuart compatible with the one integrated
     on LS1021A SoC with 32-bit big-endian register organization
+  - "fsl,imx7ulp-lpuart" for lpuart compatible with the one integrated
+    on i.MX7ULP SoC with 32-bit little-endian register organization
 - reg : Address and length of the register set for the device
 - interrupts : Should contain uart interrupt
 - clocks : phandle + clock specifier pairs, one for each entry in clock-names
-- 
2.7.4

[PATCH 4/6] tty: serial: lpuart: add imx7ulp support

[Dong Aisheng]

The lpuart of imx7ulp is basically the same as ls1021a. It's also
32 bit width register, but unlike ls1021a, it's little endian.
Besides that, imx7ulp lpuart has a minor different register layout
from ls1021a that it has four extra registers (verid, param, global,
pincfg) located at the beginning of register map, which are currently
not used by the driver and less to be used later.

To ease the register difference handling, we add a reg_off member
in lpuart_soc_data structure to represent if the normal
lpuart32_{read|write} requires plus a offset to hide the issue.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Fugang Duan <fugang.duan@nxp.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/tty/serial/fsl_lpuart.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index bddd041..1cdb3f9 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -231,7 +231,11 @@
 #define DEV_NAME	"ttyLP"
 #define UART_NR		6
 
+/* IMX lpuart has four extra unused regs located at the beginning */
+#define IMX_REG_OFF	0x10
+
 static bool lpuart_is_be;
+static u8 lpuart_reg_off;
 
 struct lpuart_port {
 	struct uart_port	port;
@@ -263,6 +267,7 @@ struct lpuart_port {
 struct lpuart_soc_data {
 	bool	is_32;
 	bool	is_be;
+	u8	reg_off;
 };
 
 static struct lpuart_soc_data vf_data = {
@@ -272,11 +277,19 @@ static struct lpuart_soc_data vf_data = {
 static struct lpuart_soc_data ls_data = {
 	.is_32 = true,
 	.is_be = true,
+	.reg_off = 0x0,
+};
+
+static struct lpuart_soc_data imx_data = {
+	.is_32 = true,
+	.is_be = false,
+	.reg_off = IMX_REG_OFF,
 };
 
 static const struct of_device_id lpuart_dt_ids[] = {
 	{ .compatible = "fsl,vf610-lpuart", .data = &vf_data, },
 	{ .compatible = "fsl,ls1021a-lpuart", .data = &ls_data, },
+	{ .compatible = "fsl,imx7ulp-lpuart", .data = &imx_data, },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
@@ -286,15 +299,16 @@ static void lpuart_dma_tx_complete(void *arg);
 
 static u32 lpuart32_read(void __iomem *addr)
 {
-	return lpuart_is_be ? ioread32be(addr) : readl(addr);
+	return lpuart_is_be ? ioread32be(addr + lpuart_reg_off) :
+			      readl(addr + lpuart_reg_off);
 }
 
 static void lpuart32_write(u32 val, void __iomem *addr)
 {
 	if (lpuart_is_be)
-		iowrite32be(val, addr);
+		iowrite32be(val, addr + lpuart_reg_off);
 	else
-		writel(val, addr);
+		writel(val, addr + lpuart_reg_off);
 }
 
 static void lpuart_stop_tx(struct uart_port *port)
@@ -2008,6 +2022,7 @@ static int lpuart_probe(struct platform_device *pdev)
 	sport->port.line = ret;
 	sport->lpuart32 = sdata->is_32;
 	lpuart_is_be = sdata->is_be;
+	lpuart_reg_off = sdata->reg_off;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	sport->port.membase = devm_ioremap_resource(&pdev->dev, res);
-- 
2.7.4

[PATCH 5/6] tty: serial: lpuart: add earlycon support for imx7ulp

[Dong Aisheng]

earlycon is executed quite early before the device tree probe,
so we need configure the correct reg_off for imx7ulp during
early console setup.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Fugang Duan <fugang.duan@nxp.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/tty/serial/fsl_lpuart.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 1cdb3f9..5b485e8 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1978,8 +1978,20 @@ static int __init lpuart32_early_console_setup(struct earlycon_device *device,
 	return 0;
 }
 
+static int __init lpuart32_imx_early_console_setup(struct earlycon_device *device,
+						   const char *opt)
+{
+	if (!device->port.membase)
+		return -ENODEV;
+
+	lpuart_reg_off = IMX_REG_OFF;
+	device->con->write = lpuart32_early_write;
+
+	return 0;
+}
 OF_EARLYCON_DECLARE(lpuart, "fsl,vf610-lpuart", lpuart_early_console_setup);
 OF_EARLYCON_DECLARE(lpuart32, "fsl,ls1021a-lpuart", lpuart32_early_console_setup);
+OF_EARLYCON_DECLARE(lpuart32, "fsl,imx7ulp-lpuart", lpuart32_imx_early_console_setup);
 EARLYCON_DECLARE(lpuart, lpuart_early_console_setup);
 EARLYCON_DECLARE(lpuart32, lpuart32_early_console_setup);
 
-- 
2.7.4

[PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate calculation method

[Dong Aisheng]

On new LPUART versions, the oversampling ratio for the receiver can be
changed from 4x (00011) to 32x (11111) which could help us get a more
accurate baud rate divider.

The idea is to use the best OSR (over-sampling rate) possible.
Note, OSR is typically hard-set to 16 in other LPUART instantiations.
Loop to find the best OSR value possible, one that generates minimum
baud diff iterate through the rest of the supported values of OSR.

Currently only i.MX7ULP is using it.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Fugang Duan <fugang.duan@nxp.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/tty/serial/fsl_lpuart.c | 85 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 79 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 5b485e8..6b2abb7 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -140,6 +140,8 @@
 #define UARTBAUD_SBNS		0x00002000
 #define UARTBAUD_SBR		0x00000000
 #define UARTBAUD_SBR_MASK	0x1fff
+#define UARTBAUD_OSR_MASK       0x1f
+#define UARTBAUD_OSR_SHIFT      24
 
 #define UARTSTAT_LBKDIF		0x80000000
 #define UARTSTAT_RXEDGIF	0x40000000
@@ -1508,6 +1510,72 @@ lpuart_set_termios(struct uart_port *port, struct ktermios *termios,
 }
 
 static void
+lpuart32_serial_setbrg(struct lpuart_port *sport, unsigned int baudrate)
+{
+	u32 sbr, osr, baud_diff, tmp_osr, tmp_sbr, tmp_diff, tmp;
+	u32 clk = sport->port.uartclk;
+
+	/*
+	 * The idea is to use the best OSR (over-sampling rate) possible.
+	 * Note, OSR is typically hard-set to 16 in other LPUART instantiations.
+	 * Loop to find the best OSR value possible, one that generates minimum
+	 * baud_diff iterate through the rest of the supported values of OSR.
+	 *
+	 * Calculation Formula:
+	 *  Baud Rate = baud clock / ((OSR+1) × SBR)
+	 */
+	baud_diff = baudrate;
+	osr = 0;
+	sbr = 0;
+
+	for (tmp_osr = 4; tmp_osr <= 32; tmp_osr++) {
+		/* calculate the temporary sbr value  */
+		tmp_sbr = (clk / (baudrate * tmp_osr));
+		if (tmp_sbr == 0)
+			tmp_sbr = 1;
+
+		/*
+		 * calculate the baud rate difference based on the temporary
+		 * osr and sbr values
+		 */
+		tmp_diff = clk / (tmp_osr * tmp_sbr) - baudrate;
+
+		/* select best values between sbr and sbr+1 */
+		tmp = clk / (tmp_osr * (tmp_sbr + 1));
+		if (tmp_diff > (baudrate - tmp)) {
+			tmp_diff = baudrate - tmp;
+			tmp_sbr++;
+		}
+
+		if (tmp_diff <= baud_diff) {
+			baud_diff = tmp_diff;
+			osr = tmp_osr;
+			sbr = tmp_sbr;
+		}
+	}
+
+	/* handle buadrate outside acceptable rate */
+	if (baud_diff > ((baudrate / 100) * 3))
+		dev_warn(sport->port.dev,
+			 "unacceptable baud rate difference of more than 3%%\n");
+
+	tmp = lpuart32_read(sport->port.membase + UARTBAUD);
+
+	if ((osr > 3) && (osr < 8))
+		tmp |= UARTBAUD_BOTHEDGE;
+
+	tmp &= ~(UARTBAUD_OSR_MASK << UARTBAUD_OSR_SHIFT);
+	tmp |= (((osr-1) & UARTBAUD_OSR_MASK) << UARTBAUD_OSR_SHIFT);
+
+	tmp &= ~UARTBAUD_SBR_MASK;
+	tmp |= sbr & UARTBAUD_SBR_MASK;
+
+	tmp &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);
+
+	lpuart32_write(tmp, sport->port.membase + UARTBAUD);
+}
+
+static void
 lpuart32_set_termios(struct uart_port *port, struct ktermios *termios,
 		   struct ktermios *old)
 {
@@ -1613,12 +1681,17 @@ lpuart32_set_termios(struct uart_port *port, struct ktermios *termios,
 	lpuart32_write(old_ctrl & ~(UARTCTRL_TE | UARTCTRL_RE),
 			sport->port.membase + UARTCTRL);
 
-	sbr = sport->port.uartclk / (16 * baud);
-	bd &= ~UARTBAUD_SBR_MASK;
-	bd |= sbr & UARTBAUD_SBR_MASK;
-	bd |= UARTBAUD_BOTHEDGE;
-	bd &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);
-	lpuart32_write(bd, sport->port.membase + UARTBAUD);
+	if (of_device_is_compatible(port->dev->of_node, "fsl,imx7ulp-lpuart")) {
+		lpuart32_serial_setbrg(sport, baud);
+	} else {
+		sbr = sport->port.uartclk / (16 * baud);
+		bd &= ~UARTBAUD_SBR_MASK;
+		bd |= sbr & UARTBAUD_SBR_MASK;
+		bd |= UARTBAUD_BOTHEDGE;
+		bd &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);
+		lpuart32_write(bd, sport->port.membase + UARTBAUD);
+	}
+
 	lpuart32_write(modem, sport->port.membase + UARTMODIR);
 	lpuart32_write(ctrl, sport->port.membase + UARTCTRL);
 	/* restore control register */
-- 
2.7.4

RE: [PATCH 0/6] tty: serial: lpuart: add imx7ulp support

[Andy Duan]

From: Dong Aisheng <aisheng.dong@nxp.com> Sent: Tuesday, May 09, 2017 3:51 PM
>To: linux-serial@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>gregkh@linuxfoundation.org; jslaby@suse.com; Andy Duan
><fugang.duan@nxp.com>; stefan@agner.ch; Mingkai Hu
><mingkai.hu@nxp.com>; Y.B. Lu <yangbo.lu@nxp.com>; A.S. Dong
><aisheng.dong@nxp.com>
>Subject: [PATCH 0/6] tty: serial: lpuart: add imx7ulp support
>
>This patch series mainly intends to add imx7ulp support which is also using FSL
>lpuart.
>
>The lpuart in imx7ulp is basically the same as ls1021a. It's also
>32 bit width register, but unlike ls1021a, it's little endian.
>Besides that, imx7ulp lpuart has a minor different register layout from ls1021a
>that it has four extra registers (verid, param, global,
>pincfg) located at the beginning of register map, which are currently not used
>by the driver and less to be used later.
>
>Furthermore, this patch serial also add a new more accurate baud rate
>calculation method as MX7ULP can't divide a suitable baud rate with the
>default setting.
>
>Currently the new baud rate calculation is only enabled on MX7ULP.
>However, i guess the Layerscape may also be able to use it as there seems to
>be no difference in baud rate setting register after checking the Layerscape
>Reference Manual.
>
>As i don't have Layerscape boards, i can't test it, so i only enable it for MX7ULP
>by default to avoid a potential break.
>
>I copied LayerScape guys in this series and hope they can help test later.
>If it works on Layerscape as well, then they can switch to the new setting too
>and totally remove the old stuff.
>
>Dong Aisheng (6):
>  tty: serial: lpuart: introduce lpuart_soc_data to represent SoC
>    property
>  tty: serial: lpuart: add little endian 32 bit register support
>  dt-bindings: serial: fsl-lpuart: add i.MX7ULP support
>  tty: serial: lpuart: add imx7ulp support
>  tty: serial: lpuart: add earlycon support for imx7ulp
>  tty: serial: lpuart: add a more accurate baud rate calculation method
>
> .../devicetree/bindings/serial/fsl-lpuart.txt      |   2 +
> drivers/tty/serial/fsl_lpuart.c                    | 149 ++++++++++++++++++---
> 2 files changed, 136 insertions(+), 15 deletions(-)
>
>--
>2.7.4

The series looks fine.

Acked-by: Fugang Duan <fugang.duan@nxp.com>

Re: [PATCH 1/6] tty: serial: lpuart: introduce lpuart_soc_data to represent SoC property

[Stefan Agner]

On 2017-05-09 00:50, Dong Aisheng wrote:
> This is used to dynamically check the SoC specific lpuart properies.
> Currently only the checking of 32 bit register width is added which
> functions the same as before. More will be added later for supporting
> new chips.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: Fugang Duan <fugang.duan@nxp.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
> Cc: Yangbo Lu <yangbo.lu@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/tty/serial/fsl_lpuart.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index 15df1ba7..cd4e905 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -258,13 +258,21 @@ struct lpuart_port {
>  	wait_queue_head_t	dma_wait;
>  };
>  
> +struct lpuart_soc_data {
> +	bool	is_32;
> +};
> +
> +static struct lpuart_soc_data vf_data = {
> +	.is_32 = false,
> +};
> +
> +static struct lpuart_soc_data ls_data = {
> +	.is_32 = true,
> +};

This could be const I guess?

--
Stefan

> +
>  static const struct of_device_id lpuart_dt_ids[] = {
> -	{
> -		.compatible = "fsl,vf610-lpuart",
> -	},
> -	{
> -		.compatible = "fsl,ls1021a-lpuart",
> -	},
> +	{ .compatible = "fsl,vf610-lpuart", .data = &vf_data, },
> +	{ .compatible = "fsl,ls1021a-lpuart", .data = &ls_data, },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
> @@ -1971,6 +1979,9 @@ static struct uart_driver lpuart_reg = {
>  
>  static int lpuart_probe(struct platform_device *pdev)
>  {
> +	const struct of_device_id *of_id = of_match_device(lpuart_dt_ids,
> +							   &pdev->dev);
> +	const struct lpuart_soc_data *sdata = of_id->data;
>  	struct device_node *np = pdev->dev.of_node;
>  	struct lpuart_port *sport;
>  	struct resource *res;
> @@ -1988,7 +1999,7 @@ static int lpuart_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  	sport->port.line = ret;
> -	sport->lpuart32 = of_device_is_compatible(np, "fsl,ls1021a-lpuart");
> +	sport->lpuart32 = sdata->is_32;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	sport->port.membase = devm_ioremap_resource(&pdev->dev, res);

Re: [PATCH 2/6] tty: serial: lpuart: add little endian 32 bit register support

[Stefan Agner]

On 2017-05-09 00:50, Dong Aisheng wrote:
> It's based on the exist lpuart32 read/write implementation.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com> (supporter:TTY LAYER)
> Cc: Fugang Duan <fugang.duan@nxp.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
> Cc: Yangbo Lu <yangbo.lu@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/tty/serial/fsl_lpuart.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index cd4e905..bddd041 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -231,6 +231,8 @@
>  #define DEV_NAME	"ttyLP"
>  #define UART_NR		6
>  
> +static bool lpuart_is_be;
> +

Other LS1021a IP's such as SPI use the big-endian device tree property
along with regmap.

See e.g.
drivers/spi/spi-fsl-dspi.c

(Used in vf610 in little endian mode and ls1021a in big endian)

Not sure if we want to switch to regmap, but you can also get the
property using of_get_property.

The ls1021a lpuart node do not specify big-endian at the moment (would
probably good to add it), so I would leave big-endian the driver default
and check for little-endian for the new device and check whether that is
specified:
of_get_property(dn, "little-endian", NULL)

--
Stefan

>  struct lpuart_port {
>  	struct uart_port	port;
>  	struct clk		*clk;
> @@ -260,6 +262,7 @@ struct lpuart_port {
>  
>  struct lpuart_soc_data {
>  	bool	is_32;
> +	bool	is_be;
>  };
>  
>  static struct lpuart_soc_data vf_data = {
> @@ -268,6 +271,7 @@ static struct lpuart_soc_data vf_data = {
>  
>  static struct lpuart_soc_data ls_data = {
>  	.is_32 = true,
> +	.is_be = true,
>  };
>  
>  static const struct of_device_id lpuart_dt_ids[] = {
> @@ -282,12 +286,15 @@ static void lpuart_dma_tx_complete(void *arg);
>  
>  static u32 lpuart32_read(void __iomem *addr)
>  {
> -	return ioread32be(addr);
> +	return lpuart_is_be ? ioread32be(addr) : readl(addr);
>  }
>  
>  static void lpuart32_write(u32 val, void __iomem *addr)
>  {
> -	iowrite32be(val, addr);
> +	if (lpuart_is_be)
> +		iowrite32be(val, addr);
> +	else
> +		writel(val, addr);
>  }
>  
>  static void lpuart_stop_tx(struct uart_port *port)
> @@ -2000,6 +2007,7 @@ static int lpuart_probe(struct platform_device *pdev)
>  	}
>  	sport->port.line = ret;
>  	sport->lpuart32 = sdata->is_32;
> +	lpuart_is_be = sdata->is_be;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	sport->port.membase = devm_ioremap_resource(&pdev->dev, res);

Re: [PATCH 4/6] tty: serial: lpuart: add imx7ulp support

[Stefan Agner]

On 2017-05-09 00:50, Dong Aisheng wrote:
> The lpuart of imx7ulp is basically the same as ls1021a. It's also
> 32 bit width register, but unlike ls1021a, it's little endian.
> Besides that, imx7ulp lpuart has a minor different register layout
> from ls1021a that it has four extra registers (verid, param, global,
> pincfg) located at the beginning of register map, which are currently
> not used by the driver and less to be used later.
> 
> To ease the register difference handling, we add a reg_off member
> in lpuart_soc_data structure to represent if the normal
> lpuart32_{read|write} requires plus a offset to hide the issue.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: Fugang Duan <fugang.duan@nxp.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
> Cc: Yangbo Lu <yangbo.lu@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/tty/serial/fsl_lpuart.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index bddd041..1cdb3f9 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -231,7 +231,11 @@
>  #define DEV_NAME	"ttyLP"
>  #define UART_NR		6
>  
> +/* IMX lpuart has four extra unused regs located at the beginning */
> +#define IMX_REG_OFF	0x10
> +
>  static bool lpuart_is_be;
> +static u8 lpuart_reg_off;

Global variables? That hardly works once you have two UARTs...

Instead of adding a fixed offset to any write you could just add the
offset to sport->port.membase...

--
Stefan

>  
>  struct lpuart_port {
>  	struct uart_port	port;
> @@ -263,6 +267,7 @@ struct lpuart_port {
>  struct lpuart_soc_data {
>  	bool	is_32;
>  	bool	is_be;
> +	u8	reg_off;
>  };
>  
>  static struct lpuart_soc_data vf_data = {
> @@ -272,11 +277,19 @@ static struct lpuart_soc_data vf_data = {
>  static struct lpuart_soc_data ls_data = {
>  	.is_32 = true,
>  	.is_be = true,
> +	.reg_off = 0x0,
> +};
> +
> +static struct lpuart_soc_data imx_data = {
> +	.is_32 = true,
> +	.is_be = false,
> +	.reg_off = IMX_REG_OFF,
>  };
>  
>  static const struct of_device_id lpuart_dt_ids[] = {
>  	{ .compatible = "fsl,vf610-lpuart", .data = &vf_data, },
>  	{ .compatible = "fsl,ls1021a-lpuart", .data = &ls_data, },
> +	{ .compatible = "fsl,imx7ulp-lpuart", .data = &imx_data, },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
> @@ -286,15 +299,16 @@ static void lpuart_dma_tx_complete(void *arg);
>  
>  static u32 lpuart32_read(void __iomem *addr)
>  {
> -	return lpuart_is_be ? ioread32be(addr) : readl(addr);
> +	return lpuart_is_be ? ioread32be(addr + lpuart_reg_off) :
> +			      readl(addr + lpuart_reg_off);
>  }
>  
>  static void lpuart32_write(u32 val, void __iomem *addr)
>  {
>  	if (lpuart_is_be)
> -		iowrite32be(val, addr);
> +		iowrite32be(val, addr + lpuart_reg_off);
>  	else
> -		writel(val, addr);
> +		writel(val, addr + lpuart_reg_off);
>  }
>  
>  static void lpuart_stop_tx(struct uart_port *port)
> @@ -2008,6 +2022,7 @@ static int lpuart_probe(struct platform_device *pdev)
>  	sport->port.line = ret;
>  	sport->lpuart32 = sdata->is_32;
>  	lpuart_is_be = sdata->is_be;
> +	lpuart_reg_off = sdata->reg_off;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	sport->port.membase = devm_ioremap_resource(&pdev->dev, res);

Re: [PATCH 1/6] tty: serial: lpuart: introduce lpuart_soc_data to represent SoC property

[Dong Aisheng]

Hi Stefan,

On Wed, May 10, 2017 at 11:50 AM, Stefan Agner <stefan@agner.ch> wrote:
> On 2017-05-09 00:50, Dong Aisheng wrote:
>> This is used to dynamically check the SoC specific lpuart properies.
>> Currently only the checking of 32 bit register width is added which
>> functions the same as before. More will be added later for supporting
>> new chips.
>>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Jiri Slaby <jslaby@suse.com>
>> Cc: Fugang Duan <fugang.duan@nxp.com>
>> Cc: Stefan Agner <stefan@agner.ch>
>> Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
>> Cc: Yangbo Lu <yangbo.lu@nxp.com>
>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> ---
>>  drivers/tty/serial/fsl_lpuart.c | 25 ++++++++++++++++++-------
>>  1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
>> index 15df1ba7..cd4e905 100644
>> --- a/drivers/tty/serial/fsl_lpuart.c
>> +++ b/drivers/tty/serial/fsl_lpuart.c
>> @@ -258,13 +258,21 @@ struct lpuart_port {
>>       wait_queue_head_t       dma_wait;
>>  };
>>
>> +struct lpuart_soc_data {
>> +     bool    is_32;
>> +};
>> +
>> +static struct lpuart_soc_data vf_data = {
>> +     .is_32 = false,
>> +};
>> +
>> +static struct lpuart_soc_data ls_data = {
>> +     .is_32 = true,
>> +};
>
> This could be const I guess?
>

Yes, of course.
Thanks for the pointing out.

Regards
Dong Aisheng

> --
> Stefan
>
>> +
>>  static const struct of_device_id lpuart_dt_ids[] = {
>> -     {
>> -             .compatible = "fsl,vf610-lpuart",
>> -     },
>> -     {
>> -             .compatible = "fsl,ls1021a-lpuart",
>> -     },
>> +     { .compatible = "fsl,vf610-lpuart", .data = &vf_data, },
>> +     { .compatible = "fsl,ls1021a-lpuart", .data = &ls_data, },
>>       { /* sentinel */ }
>>  };
>>  MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
>> @@ -1971,6 +1979,9 @@ static struct uart_driver lpuart_reg = {
>>
>>  static int lpuart_probe(struct platform_device *pdev)
>>  {
>> +     const struct of_device_id *of_id = of_match_device(lpuart_dt_ids,
>> +                                                        &pdev->dev);
>> +     const struct lpuart_soc_data *sdata = of_id->data;
>>       struct device_node *np = pdev->dev.of_node;
>>       struct lpuart_port *sport;
>>       struct resource *res;
>> @@ -1988,7 +1999,7 @@ static int lpuart_probe(struct platform_device *pdev)
>>               return ret;
>>       }
>>       sport->port.line = ret;
>> -     sport->lpuart32 = of_device_is_compatible(np, "fsl,ls1021a-lpuart");
>> +     sport->lpuart32 = sdata->is_32;
>>
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       sport->port.membase = devm_ioremap_resource(&pdev->dev, res);

Re: [PATCH 4/6] tty: serial: lpuart: add imx7ulp support

[Dong Aisheng]

Hi Stefan,

On Wed, May 10, 2017 at 12:10 PM, Stefan Agner <stefan@agner.ch> wrote:
> On 2017-05-09 00:50, Dong Aisheng wrote:
>> The lpuart of imx7ulp is basically the same as ls1021a. It's also
>> 32 bit width register, but unlike ls1021a, it's little endian.
>> Besides that, imx7ulp lpuart has a minor different register layout
>> from ls1021a that it has four extra registers (verid, param, global,
>> pincfg) located at the beginning of register map, which are currently
>> not used by the driver and less to be used later.
>>
>> To ease the register difference handling, we add a reg_off member
>> in lpuart_soc_data structure to represent if the normal
>> lpuart32_{read|write} requires plus a offset to hide the issue.
>>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Jiri Slaby <jslaby@suse.com>
>> Cc: Fugang Duan <fugang.duan@nxp.com>
>> Cc: Stefan Agner <stefan@agner.ch>
>> Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
>> Cc: Yangbo Lu <yangbo.lu@nxp.com>
>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> ---
>>  drivers/tty/serial/fsl_lpuart.c | 21 ++++++++++++++++++---
>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
>> index bddd041..1cdb3f9 100644
>> --- a/drivers/tty/serial/fsl_lpuart.c
>> +++ b/drivers/tty/serial/fsl_lpuart.c
>> @@ -231,7 +231,11 @@
>>  #define DEV_NAME     "ttyLP"
>>  #define UART_NR              6
>>
>> +/* IMX lpuart has four extra unused regs located at the beginning */
>> +#define IMX_REG_OFF  0x10
>> +
>>  static bool lpuart_is_be;
>> +static u8 lpuart_reg_off;
>
> Global variables? That hardly works once you have two UARTs...
>

lpuart_reg_off is SoC specific and there's no two UART
with two different reg offset.

> Instead of adding a fixed offset to any write you could just add the
> offset to sport->port.membase...
>

That's intended as i don't want the changes to be too intrusive.
If adding offset in sport->port.xxx, then we have to change the basic
lpuart32_read/write API which is called through the whole driver.

Regards
Dong Aisheng

> --
> Stefan
>
>>
>>  struct lpuart_port {
>>       struct uart_port        port;
>> @@ -263,6 +267,7 @@ struct lpuart_port {
>>  struct lpuart_soc_data {
>>       bool    is_32;
>>       bool    is_be;
>> +     u8      reg_off;
>>  };
>>
>>  static struct lpuart_soc_data vf_data = {
>> @@ -272,11 +277,19 @@ static struct lpuart_soc_data vf_data = {
>>  static struct lpuart_soc_data ls_data = {
>>       .is_32 = true,
>>       .is_be = true,
>> +     .reg_off = 0x0,
>> +};
>> +
>> +static struct lpuart_soc_data imx_data = {
>> +     .is_32 = true,
>> +     .is_be = false,
>> +     .reg_off = IMX_REG_OFF,
>>  };
>>
>>  static const struct of_device_id lpuart_dt_ids[] = {
>>       { .compatible = "fsl,vf610-lpuart", .data = &vf_data, },
>>       { .compatible = "fsl,ls1021a-lpuart", .data = &ls_data, },
>> +     { .compatible = "fsl,imx7ulp-lpuart", .data = &imx_data, },
>>       { /* sentinel */ }
>>  };
>>  MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
>> @@ -286,15 +299,16 @@ static void lpuart_dma_tx_complete(void *arg);
>>
>>  static u32 lpuart32_read(void __iomem *addr)
>>  {
>> -     return lpuart_is_be ? ioread32be(addr) : readl(addr);
>> +     return lpuart_is_be ? ioread32be(addr + lpuart_reg_off) :
>> +                           readl(addr + lpuart_reg_off);
>>  }
>>
>>  static void lpuart32_write(u32 val, void __iomem *addr)
>>  {
>>       if (lpuart_is_be)
>> -             iowrite32be(val, addr);
>> +             iowrite32be(val, addr + lpuart_reg_off);
>>       else
>> -             writel(val, addr);
>> +             writel(val, addr + lpuart_reg_off);
>>  }
>>
>>  static void lpuart_stop_tx(struct uart_port *port)
>> @@ -2008,6 +2022,7 @@ static int lpuart_probe(struct platform_device *pdev)
>>       sport->port.line = ret;
>>       sport->lpuart32 = sdata->is_32;
>>       lpuart_is_be = sdata->is_be;
>> +     lpuart_reg_off = sdata->reg_off;
>>
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       sport->port.membase = devm_ioremap_resource(&pdev->dev, res);

Re: [PATCH 2/6] tty: serial: lpuart: add little endian 32 bit register support

[Dong Aisheng]

On Wed, May 10, 2017 at 11:58 AM, Stefan Agner <stefan@agner.ch> wrote:
> On 2017-05-09 00:50, Dong Aisheng wrote:
>> It's based on the exist lpuart32 read/write implementation.
>>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Jiri Slaby <jslaby@suse.com> (supporter:TTY LAYER)
>> Cc: Fugang Duan <fugang.duan@nxp.com>
>> Cc: Stefan Agner <stefan@agner.ch>
>> Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
>> Cc: Yangbo Lu <yangbo.lu@nxp.com>
>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> ---
>>  drivers/tty/serial/fsl_lpuart.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
>> index cd4e905..bddd041 100644
>> --- a/drivers/tty/serial/fsl_lpuart.c
>> +++ b/drivers/tty/serial/fsl_lpuart.c
>> @@ -231,6 +231,8 @@
>>  #define DEV_NAME     "ttyLP"
>>  #define UART_NR              6
>>
>> +static bool lpuart_is_be;
>> +
>
> Other LS1021a IP's such as SPI use the big-endian device tree property
> along with regmap.
>
> See e.g.
> drivers/spi/spi-fsl-dspi.c
>
> (Used in vf610 in little endian mode and ls1021a in big endian)
>
> Not sure if we want to switch to regmap, but you can also get the
> property using of_get_property.
>
> The ls1021a lpuart node do not specify big-endian at the moment (would
> probably good to add it),

That's why i did not check for little-endian initially.

> so I would leave big-endian the driver default
> and check for little-endian for the new device and check whether that is
> specified:
> of_get_property(dn, "little-endian", NULL)

As we already have the platform data, it's not too necessary to get the endian
from device tree.

Regards
Dong Aisheng

>
> --
> Stefan
>
>>  struct lpuart_port {
>>       struct uart_port        port;
>>       struct clk              *clk;
>> @@ -260,6 +262,7 @@ struct lpuart_port {
>>
>>  struct lpuart_soc_data {
>>       bool    is_32;
>> +     bool    is_be;
>>  };
>>
>>  static struct lpuart_soc_data vf_data = {
>> @@ -268,6 +271,7 @@ static struct lpuart_soc_data vf_data = {
>>
>>  static struct lpuart_soc_data ls_data = {
>>       .is_32 = true,
>> +     .is_be = true,
>>  };
>>
>>  static const struct of_device_id lpuart_dt_ids[] = {
>> @@ -282,12 +286,15 @@ static void lpuart_dma_tx_complete(void *arg);
>>
>>  static u32 lpuart32_read(void __iomem *addr)
>>  {
>> -     return ioread32be(addr);
>> +     return lpuart_is_be ? ioread32be(addr) : readl(addr);
>>  }
>>
>>  static void lpuart32_write(u32 val, void __iomem *addr)
>>  {
>> -     iowrite32be(val, addr);
>> +     if (lpuart_is_be)
>> +             iowrite32be(val, addr);
>> +     else
>> +             writel(val, addr);
>>  }
>>
>>  static void lpuart_stop_tx(struct uart_port *port)
>> @@ -2000,6 +2007,7 @@ static int lpuart_probe(struct platform_device *pdev)
>>       }
>>       sport->port.line = ret;
>>       sport->lpuart32 = sdata->is_32;
>> +     lpuart_is_be = sdata->is_be;
>>
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       sport->port.membase = devm_ioremap_resource(&pdev->dev, res);

Re: [PATCH 4/6] tty: serial: lpuart: add imx7ulp support

[Stefan Agner]

On 2017-05-09 23:14, Dong Aisheng wrote:
> Hi Stefan,
> 
> On Wed, May 10, 2017 at 12:10 PM, Stefan Agner <stefan@agner.ch> wrote:
>> On 2017-05-09 00:50, Dong Aisheng wrote:
>>> The lpuart of imx7ulp is basically the same as ls1021a. It's also
>>> 32 bit width register, but unlike ls1021a, it's little endian.
>>> Besides that, imx7ulp lpuart has a minor different register layout
>>> from ls1021a that it has four extra registers (verid, param, global,
>>> pincfg) located at the beginning of register map, which are currently
>>> not used by the driver and less to be used later.
>>>
>>> To ease the register difference handling, we add a reg_off member
>>> in lpuart_soc_data structure to represent if the normal
>>> lpuart32_{read|write} requires plus a offset to hide the issue.
>>>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Jiri Slaby <jslaby@suse.com>
>>> Cc: Fugang Duan <fugang.duan@nxp.com>
>>> Cc: Stefan Agner <stefan@agner.ch>
>>> Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
>>> Cc: Yangbo Lu <yangbo.lu@nxp.com>
>>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>>> ---
>>>  drivers/tty/serial/fsl_lpuart.c | 21 ++++++++++++++++++---
>>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
>>> index bddd041..1cdb3f9 100644
>>> --- a/drivers/tty/serial/fsl_lpuart.c
>>> +++ b/drivers/tty/serial/fsl_lpuart.c
>>> @@ -231,7 +231,11 @@
>>>  #define DEV_NAME     "ttyLP"
>>>  #define UART_NR              6
>>>
>>> +/* IMX lpuart has four extra unused regs located at the beginning */
>>> +#define IMX_REG_OFF  0x10
>>> +
>>>  static bool lpuart_is_be;
>>> +static u8 lpuart_reg_off;
>>
>> Global variables? That hardly works once you have two UARTs...
>>
> 
> lpuart_reg_off is SoC specific and there's no two UART
> with two different reg offset.
> 
>> Instead of adding a fixed offset to any write you could just add the
>> offset to sport->port.membase...
>>
> 
> That's intended as i don't want the changes to be too intrusive.
> If adding offset in sport->port.xxx, then we have to change the basic
> lpuart32_read/write API which is called through the whole driver.

Every lpuart32_write/read call passes port.membase, so if you offset
port.membase when it is assigned in probe you should be fine not?

--
Stefan

> 
> Regards
> Dong Aisheng
> 
>> --
>> Stefan
>>
>>>
>>>  struct lpuart_port {
>>>       struct uart_port        port;
>>> @@ -263,6 +267,7 @@ struct lpuart_port {
>>>  struct lpuart_soc_data {
>>>       bool    is_32;
>>>       bool    is_be;
>>> +     u8      reg_off;
>>>  };
>>>
>>>  static struct lpuart_soc_data vf_data = {
>>> @@ -272,11 +277,19 @@ static struct lpuart_soc_data vf_data = {
>>>  static struct lpuart_soc_data ls_data = {
>>>       .is_32 = true,
>>>       .is_be = true,
>>> +     .reg_off = 0x0,
>>> +};
>>> +
>>> +static struct lpuart_soc_data imx_data = {
>>> +     .is_32 = true,
>>> +     .is_be = false,
>>> +     .reg_off = IMX_REG_OFF,
>>>  };
>>>
>>>  static const struct of_device_id lpuart_dt_ids[] = {
>>>       { .compatible = "fsl,vf610-lpuart", .data = &vf_data, },
>>>       { .compatible = "fsl,ls1021a-lpuart", .data = &ls_data, },
>>> +     { .compatible = "fsl,imx7ulp-lpuart", .data = &imx_data, },
>>>       { /* sentinel */ }
>>>  };
>>>  MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
>>> @@ -286,15 +299,16 @@ static void lpuart_dma_tx_complete(void *arg);
>>>
>>>  static u32 lpuart32_read(void __iomem *addr)
>>>  {
>>> -     return lpuart_is_be ? ioread32be(addr) : readl(addr);
>>> +     return lpuart_is_be ? ioread32be(addr + lpuart_reg_off) :
>>> +                           readl(addr + lpuart_reg_off);
>>>  }
>>>
>>>  static void lpuart32_write(u32 val, void __iomem *addr)
>>>  {
>>>       if (lpuart_is_be)
>>> -             iowrite32be(val, addr);
>>> +             iowrite32be(val, addr + lpuart_reg_off);
>>>       else
>>> -             writel(val, addr);
>>> +             writel(val, addr + lpuart_reg_off);
>>>  }
>>>
>>>  static void lpuart_stop_tx(struct uart_port *port)
>>> @@ -2008,6 +2022,7 @@ static int lpuart_probe(struct platform_device *pdev)
>>>       sport->port.line = ret;
>>>       sport->lpuart32 = sdata->is_32;
>>>       lpuart_is_be = sdata->is_be;
>>> +     lpuart_reg_off = sdata->reg_off;
>>>
>>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>       sport->port.membase = devm_ioremap_resource(&pdev->dev, res);

Re: [PATCH 4/6] tty: serial: lpuart: add imx7ulp support

[Dong Aisheng]

On Wed, May 10, 2017 at 01:37:07PM -0700, Stefan Agner wrote:
> On 2017-05-09 23:14, Dong Aisheng wrote:
> > Hi Stefan,
> > 
> > On Wed, May 10, 2017 at 12:10 PM, Stefan Agner <stefan@agner.ch> wrote:
> >> On 2017-05-09 00:50, Dong Aisheng wrote:
> >>> The lpuart of imx7ulp is basically the same as ls1021a. It's also
> >>> 32 bit width register, but unlike ls1021a, it's little endian.
> >>> Besides that, imx7ulp lpuart has a minor different register layout
> >>> from ls1021a that it has four extra registers (verid, param, global,
> >>> pincfg) located at the beginning of register map, which are currently
> >>> not used by the driver and less to be used later.
> >>>
> >>> To ease the register difference handling, we add a reg_off member
> >>> in lpuart_soc_data structure to represent if the normal
> >>> lpuart32_{read|write} requires plus a offset to hide the issue.
> >>>
> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>> Cc: Jiri Slaby <jslaby@suse.com>
> >>> Cc: Fugang Duan <fugang.duan@nxp.com>
> >>> Cc: Stefan Agner <stefan@agner.ch>
> >>> Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
> >>> Cc: Yangbo Lu <yangbo.lu@nxp.com>
> >>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >>> ---
> >>>  drivers/tty/serial/fsl_lpuart.c | 21 ++++++++++++++++++---
> >>>  1 file changed, 18 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> >>> index bddd041..1cdb3f9 100644
> >>> --- a/drivers/tty/serial/fsl_lpuart.c
> >>> +++ b/drivers/tty/serial/fsl_lpuart.c
> >>> @@ -231,7 +231,11 @@
> >>>  #define DEV_NAME     "ttyLP"
> >>>  #define UART_NR              6
> >>>
> >>> +/* IMX lpuart has four extra unused regs located at the beginning */
> >>> +#define IMX_REG_OFF  0x10
> >>> +
> >>>  static bool lpuart_is_be;
> >>> +static u8 lpuart_reg_off;
> >>
> >> Global variables? That hardly works once you have two UARTs...
> >>
> > 
> > lpuart_reg_off is SoC specific and there's no two UART
> > with two different reg offset.
> > 
> >> Instead of adding a fixed offset to any write you could just add the
> >> offset to sport->port.membase...
> >>
> > 
> > That's intended as i don't want the changes to be too intrusive.
> > If adding offset in sport->port.xxx, then we have to change the basic
> > lpuart32_read/write API which is called through the whole driver.
> 
> Every lpuart32_write/read call passes port.membase, so if you offset
> port.membase when it is assigned in probe you should be fine not?
> 

Got your point now and that do seem better.
Thanks for the suggestion.
Will change in V2.

Regards
Dong Aisheng

Re: [PATCH 3/6] dt-bindings: serial: fsl-lpuart: add i.MX7ULP support

[Rob Herring]

On Tue, May 09, 2017 at 03:50:45PM +0800, Dong Aisheng wrote:
> The lpuart of imx7ulp is basically the same as ls1021a. It's also
> 32 bit width register, but unlike ls1021a, it's little endian.
> Besides that, imx7ulp lpuart has a minor different register layout
> from ls1021a.
> 
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: Fugang Duan <fugang.duan@nxp.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
> Cc: Yangbo Lu <yangbo.lu@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  Documentation/devicetree/bindings/serial/fsl-lpuart.txt | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate calculation method

[Andy Shevchenko]

On Tue, May 9, 2017 at 10:50 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> On new LPUART versions, the oversampling ratio for the receiver can be
> changed from 4x (00011) to 32x (11111) which could help us get a more
> accurate baud rate divider.
>
> The idea is to use the best OSR (over-sampling rate) possible.
> Note, OSR is typically hard-set to 16 in other LPUART instantiations.
> Loop to find the best OSR value possible, one that generates minimum
> baud diff iterate through the rest of the supported values of OSR.

> +lpuart32_serial_setbrg(struct lpuart_port *sport, unsigned int baudrate)
> +{
> +       u32 sbr, osr, baud_diff, tmp_osr, tmp_sbr, tmp_diff, tmp;
> +       u32 clk = sport->port.uartclk;
> +
> +       /*
> +        * The idea is to use the best OSR (over-sampling rate) possible.
> +        * Note, OSR is typically hard-set to 16 in other LPUART instantiations.
> +        * Loop to find the best OSR value possible, one that generates minimum
> +        * baud_diff iterate through the rest of the supported values of OSR.
> +        *
> +        * Calculation Formula:
> +        *  Baud Rate = baud clock / ((OSR+1) × SBR)
> +        */
> +       baud_diff = baudrate;
> +       osr = 0;
> +       sbr = 0;
> +

> +       for (tmp_osr = 4; tmp_osr <= 32; tmp_osr++) {

I _think_ you may simplify this and avoid for-loop if you reconsider approach.

> +               /* calculate the temporary sbr value  */
> +               tmp_sbr = (clk / (baudrate * tmp_osr));
> +               if (tmp_sbr == 0)
> +                       tmp_sbr = 1;
> +
> +               /*
> +                * calculate the baud rate difference based on the temporary
> +                * osr and sbr values
> +                */

> +               tmp_diff = clk / (tmp_osr * tmp_sbr) - baudrate;

(32 - 4 + 1) times division is called...

> +
> +               /* select best values between sbr and sbr+1 */
> +               tmp = clk / (tmp_osr * (tmp_sbr + 1));
> +               if (tmp_diff > (baudrate - tmp)) {
> +                       tmp_diff = baudrate - tmp;
> +                       tmp_sbr++;
> +               }
> +
> +               if (tmp_diff <= baud_diff) {
> +                       baud_diff = tmp_diff;
> +                       osr = tmp_osr;
> +                       sbr = tmp_sbr;
> +               }
> +       }

> +       /* handle buadrate outside acceptable rate */
> +       if (baud_diff > ((baudrate / 100) * 3))
> +               dev_warn(sport->port.dev,
> +                        "unacceptable baud rate difference of more than 3%%\n");

Shouldn't you fall back to previous setting?

> +
> +       tmp = lpuart32_read(sport->port.membase + UARTBAUD);
> +

> +       if ((osr > 3) && (osr < 8))

Isn't it

if (osr ^ BIT(2) < BIT(2))

?

> +               tmp |= UARTBAUD_BOTHEDGE;

> +}

> +       if (of_device_is_compatible(port->dev->of_node, "fsl,imx7ulp-lpuart")) {
> +               lpuart32_serial_setbrg(sport, baud);

> +       } else {
> +               sbr = sport->port.uartclk / (16 * baud);
> +               bd &= ~UARTBAUD_SBR_MASK;
> +               bd |= sbr & UARTBAUD_SBR_MASK;
> +               bd |= UARTBAUD_BOTHEDGE;
> +               bd &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);
> +               lpuart32_write(bd, sport->port.membase + UARTBAUD);
> +       }

Perhaps it makes sense to split this to a helper function as well (in
a separate patch).

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate calculation method

[A.S. Dong]

Hi Andy,

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Sunday, May 28, 2017 8:04 AM
> To: A.S. Dong
> Cc: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm
> Mailing List; Greg Kroah-Hartman; Jiri Slaby; Andy Duan; Stefan Agner;
> Mingkai Hu; Y.B. Lu
> Subject: Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud
> rate calculation method
> 
> On Tue, May 9, 2017 at 10:50 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> > On new LPUART versions, the oversampling ratio for the receiver can be
> > changed from 4x (00011) to 32x (11111) which could help us get a more
> > accurate baud rate divider.
> >
> > The idea is to use the best OSR (over-sampling rate) possible.
> > Note, OSR is typically hard-set to 16 in other LPUART instantiations.
> > Loop to find the best OSR value possible, one that generates minimum
> > baud diff iterate through the rest of the supported values of OSR.
> 
> > +lpuart32_serial_setbrg(struct lpuart_port *sport, unsigned int
> > +baudrate) {
> > +       u32 sbr, osr, baud_diff, tmp_osr, tmp_sbr, tmp_diff, tmp;
> > +       u32 clk = sport->port.uartclk;
> > +
> > +       /*
> > +        * The idea is to use the best OSR (over-sampling rate) possible.
> > +        * Note, OSR is typically hard-set to 16 in other LPUART
> instantiations.
> > +        * Loop to find the best OSR value possible, one that generates
> minimum
> > +        * baud_diff iterate through the rest of the supported values of
> OSR.
> > +        *
> > +        * Calculation Formula:
> > +        *  Baud Rate = baud clock / ((OSR+1) × SBR)
> > +        */
> > +       baud_diff = baudrate;
> > +       osr = 0;
> > +       sbr = 0;
> > +
> 
> > +       for (tmp_osr = 4; tmp_osr <= 32; tmp_osr++) {
> 
> I _think_ you may simplify this and avoid for-loop if you reconsider
> approach.
> 

The algorithm is that we have to iterate all possible OSCs and find
the one with minimum baud_diff.

I'm not sure what alternative approach did you mean?

But there is indeed a optimization way, see below.

> > +               /* calculate the temporary sbr value  */
> > +               tmp_sbr = (clk / (baudrate * tmp_osr));
> > +               if (tmp_sbr == 0)
> > +                       tmp_sbr = 1;
> > +
> > +               /*
> > +                * calculate the baud rate difference based on the
> temporary
> > +                * osr and sbr values
> > +                */
> 
> > +               tmp_diff = clk / (tmp_osr * tmp_sbr) - baudrate;
> 
> (32 - 4 + 1) times division is called...
> 

Yes.

> > +
> > +               /* select best values between sbr and sbr+1 */
> > +               tmp = clk / (tmp_osr * (tmp_sbr + 1));
> > +               if (tmp_diff > (baudrate - tmp)) {
> > +                       tmp_diff = baudrate - tmp;
> > +                       tmp_sbr++;
> > +               }
> > +
> > +               if (tmp_diff <= baud_diff) {
> > +                       baud_diff = tmp_diff;
> > +                       osr = tmp_osr;
> > +                       sbr = tmp_sbr;

To optimize the looping, we probably could do:
If (!daud_diff)
	Break;

> > +               }
> > +       }
> 
> > +       /* handle buadrate outside acceptable rate */
> > +       if (baud_diff > ((baudrate / 100) * 3))
> > +               dev_warn(sport->port.dev,
> > +                        "unacceptable baud rate difference of more
> > + than 3%%\n");
> 
> Shouldn't you fall back to previous setting?
> 

Hmmm.. Is there defined semantic to do that or is there any other ones
doing that way?

I see most drivers not doing that.

> > +
> > +       tmp = lpuart32_read(sport->port.membase + UARTBAUD);
> > +
> 
> > +       if ((osr > 3) && (osr < 8))
> 
> Isn't it
> 
> if (osr ^ BIT(2) < BIT(2))
> 
> ?
> 

That is obvious hard to understand and I'd rather keep a more explicit way.

> > +               tmp |= UARTBAUD_BOTHEDGE;
> 
> > +}
> 
> > +       if (of_device_is_compatible(port->dev->of_node, "fsl,imx7ulp-
> lpuart")) {
> > +               lpuart32_serial_setbrg(sport, baud);
> 
> > +       } else {
> > +               sbr = sport->port.uartclk / (16 * baud);
> > +               bd &= ~UARTBAUD_SBR_MASK;
> > +               bd |= sbr & UARTBAUD_SBR_MASK;
> > +               bd |= UARTBAUD_BOTHEDGE;
> > +               bd &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);
> > +               lpuart32_write(bd, sport->port.membase + UARTBAUD);
> > +       }
> 
> Perhaps it makes sense to split this to a helper function as well (in a
> separate patch).
> 

That will be removed according to Stefan's suggestion to get LS platforms
Start to test.

Regards
Dong Aisheng

Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate calculation method

[Andy Shevchenko]

On Wed, May 31, 2017 at 5:18 PM, A.S. Dong <aisheng.dong@nxp.com> wrote:
>> On Tue, May 9, 2017 at 10:50 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:

By some reason my previous message went privately.
It didn't have anything major anyway and here I'm suggesting
optimization of finding factors of the formula in use. See below.

>> > +       u32 sbr, osr, baud_diff, tmp_osr, tmp_sbr, tmp_diff, tmp;
>> > +       u32 clk = sport->port.uartclk;
>> > +
>> > +       /*
>> > +        * The idea is to use the best OSR (over-sampling rate) possible.
>> > +        * Note, OSR is typically hard-set to 16 in other LPUART
>> instantiations.
>> > +        * Loop to find the best OSR value possible, one that generates
>> minimum
>> > +        * baud_diff iterate through the rest of the supported values of
>> OSR.
>> > +        *
>> > +        * Calculation Formula:
>> > +        *  Baud Rate = baud clock / ((OSR+1) × SBR)
>> > +        */
>> > +       baud_diff = baudrate;
>> > +       osr = 0;
>> > +       sbr = 0;
>> > +
>>
>> > +       for (tmp_osr = 4; tmp_osr <= 32; tmp_osr++) {

I missed one thing, what happened by default to OSR? What is the value in use?

>> I _think_ you may simplify this and avoid for-loop if you reconsider
>> approach.

> But there is indeed a optimization way, see below.

> To optimize the looping, we probably could do:
> If (!baud_diff)
>         Break;

It's a small one, we may have more interesting approach.

So, the algo is the following:

Assume the ranges like this:
OSR = [4 ... 32]
SBR = [2 ... 8192]

Then:

1. Get ratio factor as
      ratio = CLK / desired baud rate
2. If ratio < 8192 * 9 / 2, just use (ratio / 4, 4) as (OSR, SBR)
setting. (Needs clarification on OSR < 4)
3. if ratio >= 8192 * 31, just use those two numbers (8192, 31). You
can't do anything better there.
4. Otherwise, get a minimum required factor of OSR
      osr_min = ratio / 8192
5. Start your loop from osr_min + 1 to 31.

6 (optional). Of course you may not consider baud_diff > osr_min, it's
I suppose obvious

P.S. Note, all divisions by 2^n are just simple right shifts. Diffs
are calculated as multiplication of OSR and SBR in comparison to
ratio. One division so far.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate calculation method

[A.S. Dong]

Hi Andy,

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Saturday, June 03, 2017 1:11 AM
> To: A.S. Dong
> Cc: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm
> Mailing List; Greg Kroah-Hartman; Jiri Slaby; Andy Duan; Stefan Agner;
> Mingkai Hu; Y.B. Lu
> Subject: Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud
> rate calculation method
> 
> On Wed, May 31, 2017 at 5:18 PM, A.S. Dong <aisheng.dong@nxp.com> wrote:
> >> On Tue, May 9, 2017 at 10:50 AM, Dong Aisheng <aisheng.dong@nxp.com>
> wrote:
> 
> By some reason my previous message went privately.
> It didn't have anything major anyway and here I'm suggesting optimization
> of finding factors of the formula in use. See below.
> 
> >> > +       u32 sbr, osr, baud_diff, tmp_osr, tmp_sbr, tmp_diff, tmp;
> >> > +       u32 clk = sport->port.uartclk;
> >> > +
> >> > +       /*
> >> > +        * The idea is to use the best OSR (over-sampling rate)
> possible.
> >> > +        * Note, OSR is typically hard-set to 16 in other LPUART
> >> instantiations.
> >> > +        * Loop to find the best OSR value possible, one that
> >> > + generates
> >> minimum
> >> > +        * baud_diff iterate through the rest of the supported
> >> > + values of
> >> OSR.
> >> > +        *
> >> > +        * Calculation Formula:
> >> > +        *  Baud Rate = baud clock / ((OSR+1) × SBR)
> >> > +        */
> >> > +       baud_diff = baudrate;
> >> > +       osr = 0;
> >> > +       sbr = 0;
> >> > +
> >>
> >> > +       for (tmp_osr = 4; tmp_osr <= 32; tmp_osr++) {
> 
> I missed one thing, what happened by default to OSR? What is the value in
> use?
> 

No valid default value. (osc/sbr are 0 by default)
If no proper osc and sbr calculated, a WARNING will show.

> >> I _think_ you may simplify this and avoid for-loop if you reconsider
> >> approach.
> 
> > But there is indeed a optimization way, see below.
> 
> > To optimize the looping, we probably could do:
> > If (!baud_diff)
> >         Break;
> 
> It's a small one, we may have more interesting approach.
> 
> So, the algo is the following:
> 
> Assume the ranges like this:
> OSR = [4 ... 32]
> SBR = [2 ... 8192]
> 

Baud Rate = baud clock / ((OSR+1) × SBR)

In HW:
OSR range : 3 – 31
SBR range: 1 – 8191

> Then:
> 
> 1. Get ratio factor as
>       ratio = CLK / desired baud rate
> 2. If ratio < 8192 * 9 / 2, just use (ratio / 4, 4) as (OSR, SBR) setting.
> (Needs clarification on OSR < 4) 

Sorry that I'm a bit mess here.
What is 8192 * 9 /2 meaning?

And for (ratio / 4, 4) as (OSR,SBR), take 115200 as an example:
Assuming baud clock 24Mhz.

Ratio = 24000000 / 115200 = 208
OSR = Ratio / 4 = 52 
Then OSR is out of range which seems wrong.

> 3. if ratio >= 8192 * 31, just use those
> two numbers (8192, 31). You can't do anything better there.

This actually may not happen.
Even take a 9600 as example, the clk becomes:
8191 * 31 * 9600 = 2.4GHz
Which is theoretically not exist.

> 4. Otherwise, get a minimum required factor of OSR
>       osr_min = ratio / 8192
> 5. Start your loop from osr_min + 1 to 31.
> 
> 6 (optional). Of course you may not consider baud_diff > osr_min, it's I
> suppose obvious
> 
> P.S. Note, all divisions by 2^n are just simple right shifts. Diffs are
> calculated as multiplication of OSR and SBR in comparison to ratio. One
> division so far.
> 

I'm not quite understand the approach.

How about you send a separate baud algorithm improvement patch later?
Then it first can provide us a good patch history and also better to
understand for review.

Last, very appreciate for your kind and professional review.

Regards
Dong Aisheng

Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate calculation method

[Andy Shevchenko]

On Fri, Jun 9, 2017 at 11:01 AM, A.S. Dong <aisheng.dong@nxp.com> wrote:

>> >> > +       u32 sbr, osr, baud_diff, tmp_osr, tmp_sbr, tmp_diff, tmp;
>> >> > +       u32 clk = sport->port.uartclk;
>> >> > +
>> >> > +       /*
>> >> > +        * The idea is to use the best OSR (over-sampling rate)
>> possible.
>> >> > +        * Note, OSR is typically hard-set to 16 in other LPUART
>> >> instantiations.
>> >> > +        * Loop to find the best OSR value possible, one that
>> >> > + generates
>> >> minimum
>> >> > +        * baud_diff iterate through the rest of the supported
>> >> > + values of
>> >> OSR.
>> >> > +        *
>> >> > +        * Calculation Formula:
>> >> > +        *  Baud Rate = baud clock / ((OSR+1) × SBR)
>> >> > +        */
>> >> > +       baud_diff = baudrate;
>> >> > +       osr = 0;
>> >> > +       sbr = 0;
>> >> > +
>> >>
>> >> > +       for (tmp_osr = 4; tmp_osr <= 32; tmp_osr++) {
>>
>> I missed one thing, what happened by default to OSR? What is the value in
>> use?
>>
>
> No valid default value. (osc/sbr are 0 by default)
> If no proper osc and sbr calculated, a WARNING will show.

Okay, so, it means the maximum supported speed is UART clock / 4. Correct?

>> So, the algo is the following:
>>
>> Assume the ranges like this:
>> OSR = [4 ... 32]
>> SBR = [2 ... 8192]
>>
>
> Baud Rate = baud clock / ((OSR+1) × SBR)
>
> In HW:
> OSR range : 3 – 31
> SBR range: 1 – 8191

I've read that, but think outside the box.

>> Then:
>>
>> 1. Get ratio factor as
>>       ratio = CLK / desired baud rate
>> 2. If ratio < 8192 * 9 / 2, just use (ratio / 4, 4) as (OSR, SBR) setting.
>> (Needs clarification on OSR < 4)
>
> Sorry that I'm a bit mess here.
> What is 8192 * 9 /2 meaning?

I forgot the details...

> And for (ratio / 4, 4) as (OSR,SBR), take 115200 as an example:
> Assuming baud clock 24Mhz.
>
> Ratio = 24000000 / 115200 = 208
> OSR = Ratio / 4 = 52
> Then OSR is out of range which seems wrong.

...yes...

>> 3. if ratio >= 8192 * 31, just use those
>> two numbers (8192, 31). You can't do anything better there.
>
> This actually may not happen.
> Even take a 9600 as example, the clk becomes:
> 8191 * 31 * 9600 = 2.4GHz
> Which is theoretically not exist.
>
>> 4. Otherwise, get a minimum required factor of OSR
>>       osr_min = ratio / 8192
>> 5. Start your loop from osr_min + 1 to 31.
>>
>> 6 (optional). Of course you may not consider baud_diff > osr_min, it's I
>> suppose obvious
>>
>> P.S. Note, all divisions by 2^n are just simple right shifts. Diffs are
>> calculated as multiplication of OSR and SBR in comparison to ratio. One
>> division so far.

> I'm not quite understand the approach.

...lemme prepare a python script demonstrating it.

> How about you send a separate baud algorithm improvement patch later?

Why not to do it right a way?

Just describe it in a comment if you afraid of reader can't understand
from the code.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate calculation method

[A.S. Dong]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Friday, June 09, 2017 5:26 PM
> To: A.S. Dong
> Cc: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm
> Mailing List; Greg Kroah-Hartman; Jiri Slaby; Andy Duan; Stefan Agner;
> Mingkai Hu; Y.B. Lu; Dong Aisheng
> Subject: Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud
> rate calculation method
> 
> On Fri, Jun 9, 2017 at 11:01 AM, A.S. Dong <aisheng.dong@nxp.com> wrote:
> 
> >> >> > +       u32 sbr, osr, baud_diff, tmp_osr, tmp_sbr, tmp_diff, tmp;
> >> >> > +       u32 clk = sport->port.uartclk;
> >> >> > +
> >> >> > +       /*
> >> >> > +        * The idea is to use the best OSR (over-sampling rate)
> >> possible.
> >> >> > +        * Note, OSR is typically hard-set to 16 in other LPUART
> >> >> instantiations.
> >> >> > +        * Loop to find the best OSR value possible, one that
> >> >> > + generates
> >> >> minimum
> >> >> > +        * baud_diff iterate through the rest of the supported
> >> >> > + values of
> >> >> OSR.
> >> >> > +        *
> >> >> > +        * Calculation Formula:
> >> >> > +        *  Baud Rate = baud clock / ((OSR+1) × SBR)
> >> >> > +        */
> >> >> > +       baud_diff = baudrate;
> >> >> > +       osr = 0;
> >> >> > +       sbr = 0;
> >> >> > +
> >> >>
> >> >> > +       for (tmp_osr = 4; tmp_osr <= 32; tmp_osr++) {
> >>
> >> I missed one thing, what happened by default to OSR? What is the
> >> value in use?
> >>
> >
> > No valid default value. (osc/sbr are 0 by default) If no proper osc
> > and sbr calculated, a WARNING will show.
> 
> Okay, so, it means the maximum supported speed is UART clock / 4. Correct?
> 

Yes.

> >> So, the algo is the following:
> >>
> >> Assume the ranges like this:
> >> OSR = [4 ... 32]
> >> SBR = [2 ... 8192]
> >>
> >
> > Baud Rate = baud clock / ((OSR+1) × SBR)
> >
> > In HW:
> > OSR range : 3 – 31
> > SBR range: 1 – 8191
> 
> I've read that, but think outside the box.
> 
> >> Then:
> >>
> >> 1. Get ratio factor as
> >>       ratio = CLK / desired baud rate 2. If ratio < 8192 * 9 / 2,
> >> just use (ratio / 4, 4) as (OSR, SBR) setting.
> >> (Needs clarification on OSR < 4)
> >
> > Sorry that I'm a bit mess here.
> > What is 8192 * 9 /2 meaning?
> 
> I forgot the details...
> 
> > And for (ratio / 4, 4) as (OSR,SBR), take 115200 as an example:
> > Assuming baud clock 24Mhz.
> >
> > Ratio = 24000000 / 115200 = 208
> > OSR = Ratio / 4 = 52
> > Then OSR is out of range which seems wrong.
> 
> ...yes...
> 
> >> 3. if ratio >= 8192 * 31, just use those two numbers (8192, 31). You
> >> can't do anything better there.
> >
> > This actually may not happen.
> > Even take a 9600 as example, the clk becomes:
> > 8191 * 31 * 9600 = 2.4GHz
> > Which is theoretically not exist.
> >
> >> 4. Otherwise, get a minimum required factor of OSR
> >>       osr_min = ratio / 8192
> >> 5. Start your loop from osr_min + 1 to 31.
> >>
> >> 6 (optional). Of course you may not consider baud_diff > osr_min,
> >> it's I suppose obvious
> >>
> >> P.S. Note, all divisions by 2^n are just simple right shifts. Diffs
> >> are calculated as multiplication of OSR and SBR in comparison to
> >> ratio. One division so far.
> 
> > I'm not quite understand the approach.
> 
> ...lemme prepare a python script demonstrating it.
> 

Great, thanks

> > How about you send a separate baud algorithm improvement patch later?
> 
> Why not to do it right a way?
> 

Because I thought that could be a separate patch which is doing algorithm
improvement, then we can have the full history and a clear comparison.

And also we are still not sure whether it works, we don't want to block on it
too long.

But if you're pretty sure about it, I would wait for some more time.

However, personally I would still rather keep them in two separate Patches
for clearer history and comparison.

> Just describe it in a comment if you afraid of reader can't understand
> from the code.
> 

That is good.

Regards
Dong Aisheng

> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate calculation method

[Andy Shevchenko]

On Fri, Jun 9, 2017 at 5:20 PM, A.S. Dong <aisheng.dong@nxp.com> wrote:
>> > How about you send a separate baud algorithm improvement patch later?
>>
>> Why not to do it right a way?
>>
>
> Because I thought that could be a separate patch which is doing algorithm
> improvement, then we can have the full history and a clear comparison.
>
> And also we are still not sure whether it works, we don't want to block on it
> too long.
>
> But if you're pretty sure about it, I would wait for some more time.
>
> However, personally I would still rather keep them in two separate Patches
> for clearer history and comparison.

Since we already near to -rc5, I would rather agree with you.
So, please proceed with your approach and we can modify it in relaxing
mode later on.

Thanks, and sorry for the delay!

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud rate calculation method

[A.S. Dong]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Friday, June 09, 2017 11:49 PM
> To: A.S. Dong
> Cc: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm
> Mailing List; Greg Kroah-Hartman; Jiri Slaby; Andy Duan; Stefan Agner;
> Mingkai Hu; Y.B. Lu; Dong Aisheng
> Subject: Re: [PATCH 6/6] tty: serial: lpuart: add a more accurate baud
> rate calculation method
> 
> On Fri, Jun 9, 2017 at 5:20 PM, A.S. Dong <aisheng.dong@nxp.com> wrote:
> >> > How about you send a separate baud algorithm improvement patch later?
> >>
> >> Why not to do it right a way?
> >>
> >
> > Because I thought that could be a separate patch which is doing
> > algorithm improvement, then we can have the full history and a clear
> comparison.
> >
> > And also we are still not sure whether it works, we don't want to
> > block on it too long.
> >
> > But if you're pretty sure about it, I would wait for some more time.
> >
> > However, personally I would still rather keep them in two separate
> > Patches for clearer history and comparison.
> 
> Since we already near to -rc5, I would rather agree with you.
> So, please proceed with your approach and we can modify it in relaxing
> mode later on.
> 
> Thanks, and sorry for the delay!

Never mind.
A professional review is always respectable and appreciated.

Thanks

Regards
Dong Aisheng