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

[PATCH V2 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.

ChangeLog:
v1->v2:
 * Patch 2/4/5 chagned, other no changes.
   See individuals for details.

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                    | 147 ++++++++++++++++++---
 2 files changed, 134 insertions(+), 15 deletions(-)

-- 
2.7.4

[PATCH V2 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.

ChangeLog:
v1->v2:
 * Patch 2/4/5 chagned, other no changes.
   See individuals for details.

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                    | 147 ++++++++++++++++++---
 2 files changed, 134 insertions(+), 15 deletions(-)

-- 
2.7.4

[PATCH V2 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.

ChangeLog:
v1->v2:
 * Patch 2/4/5 chagned, other no changes.
   See individuals for details.

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                    | 147 ++++++++++++++++++---
 2 files changed, 134 insertions(+), 15 deletions(-)

-- 
2.7.4

[PATCH V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

---
ChangeLog:
v1->v2:
 * make all soc_data const
---
 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..7204103 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 const struct lpuart_soc_data vf_data = {
+	.is_32 = false,
+};
+
+static const 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 V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

---
ChangeLog:
v1->v2:
 * make all soc_data const
---
 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..7204103 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 const struct lpuart_soc_data vf_data = {
+	.is_32 = false,
+};
+
+static const 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 V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

---
ChangeLog:
v1->v2:
 * make all soc_data const
---
 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..7204103 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 const struct lpuart_soc_data vf_data = {
+	.is_32 = false,
+};
+
+static const 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

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

[Andy Shevchenko]

On Mon, May 15, 2017 at 10:48 AM, Dong Aisheng <aisheng.dong@nxp.com> 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.

For the rest it might be okay, but don't we have specific properties
for register width and offset for UART? I think I saw something like
that in other drivers.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, May 15, 2017 at 10:48 AM, Dong Aisheng <aisheng.dong@nxp.com> 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.

For the rest it might be okay, but don't we have specific properties
for register width and offset for UART? I think I saw something like
that in other drivers.

-- 
With Best Regards,
Andy Shevchenko

[PATCH V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@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 7204103..e72b397 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 const struct lpuart_soc_data vf_data = {
@@ -268,6 +271,7 @@ static const struct lpuart_soc_data vf_data = {
 
 static const 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 V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@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 7204103..e72b397 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 const struct lpuart_soc_data vf_data = {
@@ -268,6 +271,7 @@ static const struct lpuart_soc_data vf_data = {
 
 static const 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 V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@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 7204103..e72b397 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 const struct lpuart_soc_data vf_data = {
@@ -268,6 +271,7 @@ static const struct lpuart_soc_data vf_data = {
 
 static const 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

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

[Andy Shevchenko]

On Mon, May 15, 2017 at 10:48 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> It's based on the exist lpuart32 read/write implementation.

Same as per previous comment (perhaps not in other UART driver, but
some might have been using device property for that.


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, May 15, 2017 at 10:48 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> It's based on the exist lpuart32 read/write implementation.

Same as per previous comment (perhaps not in other UART driver, but
some might have been using device property for that.


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, May 15, 2017 at 10:48 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> It's based on the exist lpuart32 read/write implementation.

Same as per previous comment (perhaps not in other UART driver, but
some might have been using device property for that.


-- 
With Best Regards,
Andy Shevchenko

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

[Dong Aisheng]

On Mon, May 15, 2017 at 04:36:25PM +0300, Andy Shevchenko wrote:
> On Mon, May 15, 2017 at 10:48 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> > It's based on the exist lpuart32 read/write implementation.
> 
> Same as per previous comment (perhaps not in other UART driver, but
> some might have been using device property for that.
> 

The reason why we do that is: a) we don't want to make a churn on the
exist users. b) it's not really necessary to use dt property
when we already have a soc_data as it has no much benefit but introduce
overhead.

Regards
Dong Aisheng

> 
> -- 
> With Best Regards,
> Andy Shevchenko

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

[Dong Aisheng]

On Mon, May 15, 2017 at 04:36:25PM +0300, Andy Shevchenko wrote:
> On Mon, May 15, 2017 at 10:48 AM, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> > It's based on the exist lpuart32 read/write implementation.
> 
> Same as per previous comment (perhaps not in other UART driver, but
> some might have been using device property for that.
> 

The reason why we do that is: a) we don't want to make a churn on the
exist users. b) it's not really necessary to use dt property
when we already have a soc_data as it has no much benefit but introduce
overhead.

Regards
Dong Aisheng

> 
> -- 
> With Best Regards,
> Andy Shevchenko

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

[Nikita Yushchenko]

> @@ -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;

Setting a global variable in per-device routine is quite bad design.

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

[Nikita Yushchenko]

> @@ -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;

Setting a global variable in per-device routine is quite bad design.

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

[Dong Aisheng]

On Tue, May 16, 2017 at 02:08:12PM +0300, Nikita Yushchenko wrote:
> > @@ -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;
> 
> Setting a global variable in per-device routine is quite bad design.
> 

There is a reason for that we don't want to change the exist 
lpuart32_read[write] API which is widely used in driver.
Making a global lpuart_is_be is the simplest way to do it.

Any strong blocking reason?

Regards
Dong Aisheng

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

[Dong Aisheng]

On Tue, May 16, 2017 at 02:08:12PM +0300, Nikita Yushchenko wrote:
> > @@ -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;
> 
> Setting a global variable in per-device routine is quite bad design.
> 

There is a reason for that we don't want to change the exist 
lpuart32_read[write] API which is widely used in driver.
Making a global lpuart_is_be is the simplest way to do it.

Any strong blocking reason?

Regards
Dong Aisheng

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

[Nikita Yushchenko]

>>> @@ -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;
>>
>> Setting a global variable in per-device routine is quite bad design.
>>
> 
> There is a reason for that we don't want to change the exist 
> lpuart32_read[write] API which is widely used in driver.
> Making a global lpuart_is_be is the simplest way to do it.
> 
> Any strong blocking reason?

Code should be consistent.

There is no good reason to have sport->lpuart32 inside sport, but
lpuart_is_be outside of it. Both these values describe properties of
particular device, and thus should be in per-device structure.

If that implies adding sport arg to lpuart32_(read|write), just do that.

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

[Nikita Yushchenko]

>>> @@ -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;
>>
>> Setting a global variable in per-device routine is quite bad design.
>>
> 
> There is a reason for that we don't want to change the exist 
> lpuart32_read[write] API which is widely used in driver.
> Making a global lpuart_is_be is the simplest way to do it.
> 
> Any strong blocking reason?

Code should be consistent.

There is no good reason to have sport->lpuart32 inside sport, but
lpuart_is_be outside of it. Both these values describe properties of
particular device, and thus should be in per-device structure.

If that implies adding sport arg to lpuart32_(read|write), just do that.

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

[A.S. Dong]

> -----Original Message-----
> From: Nikita Yushchenko [mailto:nikita.yoush@cogentembedded.com]
> Sent: Wednesday, May 17, 2017 1:44 PM
> To: Dong Aisheng
> Cc: A.S. Dong; linux-serial@vger.kernel.org; Andy Duan;
> gregkh@linuxfoundation.org; Y.B. Lu; linux-kernel@vger.kernel.org;
> stefan@agner.ch; Mingkai Hu; jslaby@suse.com; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit
> register support
> 
> >>> @@ -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;
> >>
> >> Setting a global variable in per-device routine is quite bad design.
> >>
> >
> > There is a reason for that we don't want to change the exist
> > lpuart32_read[write] API which is widely used in driver.
> > Making a global lpuart_is_be is the simplest way to do it.
> >
> > Any strong blocking reason?
> 
> Code should be consistent.
> 

Yes.

> There is no good reason to have sport->lpuart32 inside sport, but
> lpuart_is_be outside of it. Both these values describe properties of
> particular device, and thus should be in per-device structure.
> 

That's for special case, normally we wouldn't do that.

> If that implies adding sport arg to lpuart32_(read|write), just do that.

There's another reason that we have to deal with earlycon which is
executed much early before driver probe.

And I need specificly align the endian data.
e.g.
static int __init lpuart32_early_console_setup(struct earlycon_device *device,
                                          const char *opt)
{
        if (!device->port.membase)
                return -ENODEV;

        lpuart_is_be = true;
        device->con->write = lpuart32_early_write;
        return 0;
}

static int __init lpuart32_imx_early_console_setup(struct earlycon_device *device,
                                                   const char *opt)
{
        if (!device->port.membase)
                return -ENODEV;

        lpuart_is_be = false;
        device->port.membase += IMX_REG_OFF;
        device->con->write = lpuart32_early_write;

        return 0;
}

Regards
Dong Aisheng

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

[A.S. Dong]

> -----Original Message-----
> From: Nikita Yushchenko [mailto:nikita.yoush at cogentembedded.com]
> Sent: Wednesday, May 17, 2017 1:44 PM
> To: Dong Aisheng
> Cc: A.S. Dong; linux-serial at vger.kernel.org; Andy Duan;
> gregkh at linuxfoundation.org; Y.B. Lu; linux-kernel at vger.kernel.org;
> stefan at agner.ch; Mingkai Hu; jslaby at suse.com; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit
> register support
> 
> >>> @@ -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;
> >>
> >> Setting a global variable in per-device routine is quite bad design.
> >>
> >
> > There is a reason for that we don't want to change the exist
> > lpuart32_read[write] API which is widely used in driver.
> > Making a global lpuart_is_be is the simplest way to do it.
> >
> > Any strong blocking reason?
> 
> Code should be consistent.
> 

Yes.

> There is no good reason to have sport->lpuart32 inside sport, but
> lpuart_is_be outside of it. Both these values describe properties of
> particular device, and thus should be in per-device structure.
> 

That's for special case, normally we wouldn't do that.

> If that implies adding sport arg to lpuart32_(read|write), just do that.

There's another reason that we have to deal with earlycon which is
executed much early before driver probe.

And I need specificly align the endian data.
e.g.
static int __init lpuart32_early_console_setup(struct earlycon_device *device,
                                          const char *opt)
{
        if (!device->port.membase)
                return -ENODEV;

        lpuart_is_be = true;
        device->con->write = lpuart32_early_write;
        return 0;
}

static int __init lpuart32_imx_early_console_setup(struct earlycon_device *device,
                                                   const char *opt)
{
        if (!device->port.membase)
                return -ENODEV;

        lpuart_is_be = false;
        device->port.membase += IMX_REG_OFF;
        device->con->write = lpuart32_early_write;

        return 0;
}

Regards
Dong Aisheng

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

[A.S. Dong]

> -----Original Message-----
> From: Nikita Yushchenko [mailto:nikita.yoush@cogentembedded.com]
> Sent: Wednesday, May 17, 2017 1:44 PM
> To: Dong Aisheng
> Cc: A.S. Dong; linux-serial@vger.kernel.org; Andy Duan;
> gregkh@linuxfoundation.org; Y.B. Lu; linux-kernel@vger.kernel.org;
> stefan@agner.ch; Mingkai Hu; jslaby@suse.com; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit
> register support
> 
> >>> @@ -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;
> >>
> >> Setting a global variable in per-device routine is quite bad design.
> >>
> >
> > There is a reason for that we don't want to change the exist
> > lpuart32_read[write] API which is widely used in driver.
> > Making a global lpuart_is_be is the simplest way to do it.
> >
> > Any strong blocking reason?
> 
> Code should be consistent.
> 

Yes.

> There is no good reason to have sport->lpuart32 inside sport, but
> lpuart_is_be outside of it. Both these values describe properties of
> particular device, and thus should be in per-device structure.
> 

That's for special case, normally we wouldn't do that.

> If that implies adding sport arg to lpuart32_(read|write), just do that.

There's another reason that we have to deal with earlycon which is
executed much early before driver probe.

And I need specificly align the endian data.
e.g.
static int __init lpuart32_early_console_setup(struct earlycon_device *device,
                                          const char *opt)
{
        if (!device->port.membase)
                return -ENODEV;

        lpuart_is_be = true;
        device->con->write = lpuart32_early_write;
        return 0;
}

static int __init lpuart32_imx_early_console_setup(struct earlycon_device *device,
                                                   const char *opt)
{
        if (!device->port.membase)
                return -ENODEV;

        lpuart_is_be = false;
        device->port.membase += IMX_REG_OFF;
        device->con->write = lpuart32_early_write;

        return 0;
}

Regards
Dong Aisheng

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

[Nikita Yushchenko]

>> Code should be consistent.
>>
> 
> Yes.
> 
>> There is no good reason to have sport->lpuart32 inside sport, but
>> lpuart_is_be outside of it. Both these values describe properties of
>> particular device, and thus should be in per-device structure.
>>
> 
> That's for special case, normally we wouldn't do that.

For me this "special case" looks like "let's break data structure
consistency to reuse several lines of code".

With code snippets you show, it looks even worse: you assign same global
variable in several places for different uses. implicitly assuming that
it is for same device. Which can be true in your current system, but not
elsewhere (e.g. why not having lpuart programmed into fpga)?

Alternative solution could be - have separate write path for earlycon.
At a glance, it is dozen lines of code.

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

[Nikita Yushchenko]

>> Code should be consistent.
>>
> 
> Yes.
> 
>> There is no good reason to have sport->lpuart32 inside sport, but
>> lpuart_is_be outside of it. Both these values describe properties of
>> particular device, and thus should be in per-device structure.
>>
> 
> That's for special case, normally we wouldn't do that.

For me this "special case" looks like "let's break data structure
consistency to reuse several lines of code".

With code snippets you show, it looks even worse: you assign same global
variable in several places for different uses. implicitly assuming that
it is for same device. Which can be true in your current system, but not
elsewhere (e.g. why not having lpuart programmed into fpga)?

Alternative solution could be - have separate write path for earlycon.
At a glance, it is dozen lines of code.

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

[Nikita Yushchenko]

>> Code should be consistent.
>>
> 
> Yes.
> 
>> There is no good reason to have sport->lpuart32 inside sport, but
>> lpuart_is_be outside of it. Both these values describe properties of
>> particular device, and thus should be in per-device structure.
>>
> 
> That's for special case, normally we wouldn't do that.

For me this "special case" looks like "let's break data structure
consistency to reuse several lines of code".

With code snippets you show, it looks even worse: you assign same global
variable in several places for different uses. implicitly assuming that
it is for same device. Which can be true in your current system, but not
elsewhere (e.g. why not having lpuart programmed into fpga)?

Alternative solution could be - have separate write path for earlycon.
At a glance, it is dozen lines of code.

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

[Dong Aisheng]

On Wed, May 17, 2017 at 09:25:51AM +0300, Nikita Yushchenko wrote:
> >> Code should be consistent.
> >>
> > 
> > Yes.
> > 
> >> There is no good reason to have sport->lpuart32 inside sport, but
> >> lpuart_is_be outside of it. Both these values describe properties of
> >> particular device, and thus should be in per-device structure.
> >>
> > 
> > That's for special case, normally we wouldn't do that.
> 
> For me this "special case" looks like "let's break data structure
> consistency to reuse several lines of code".
> 
> With code snippets you show, it looks even worse: you assign same global
> variable in several places for different uses. 

If you mean lpuart_is_be, it's not for different uses.
The purpose is the same to align the correct endian but in two places.

> implicitly assuming that
> it is for same device. Which can be true in your current system, but not
> elsewhere (e.g. why not having lpuart programmed into fpga)?
> 

Sorry, What issues for fpga?

> Alternative solution could be - have separate write path for earlycon.

It looks to me having the same issue with a separate write patch
for earlycon as we still need distinguish Little or Big endian
for Layerscape and IMX.

> At a glance, it is dozen lines of code.

Would you please show some sample code?
Then we probably may understand better with each other.

Anyway, thanks for detailed review.

Regards
Dong Aisheng

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

[Dong Aisheng]

On Wed, May 17, 2017 at 09:25:51AM +0300, Nikita Yushchenko wrote:
> >> Code should be consistent.
> >>
> > 
> > Yes.
> > 
> >> There is no good reason to have sport->lpuart32 inside sport, but
> >> lpuart_is_be outside of it. Both these values describe properties of
> >> particular device, and thus should be in per-device structure.
> >>
> > 
> > That's for special case, normally we wouldn't do that.
> 
> For me this "special case" looks like "let's break data structure
> consistency to reuse several lines of code".
> 
> With code snippets you show, it looks even worse: you assign same global
> variable in several places for different uses. 

If you mean lpuart_is_be, it's not for different uses.
The purpose is the same to align the correct endian but in two places.

> implicitly assuming that
> it is for same device. Which can be true in your current system, but not
> elsewhere (e.g. why not having lpuart programmed into fpga)?
> 

Sorry, What issues for fpga?

> Alternative solution could be - have separate write path for earlycon.

It looks to me having the same issue with a separate write patch
for earlycon as we still need distinguish Little or Big endian
for Layerscape and IMX.

> At a glance, it is dozen lines of code.

Would you please show some sample code?
Then we probably may understand better with each other.

Anyway, thanks for detailed review.

Regards
Dong Aisheng

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

[Dong Aisheng]

On Wed, May 17, 2017 at 09:25:51AM +0300, Nikita Yushchenko wrote:
> >> Code should be consistent.
> >>
> > 
> > Yes.
> > 
> >> There is no good reason to have sport->lpuart32 inside sport, but
> >> lpuart_is_be outside of it. Both these values describe properties of
> >> particular device, and thus should be in per-device structure.
> >>
> > 
> > That's for special case, normally we wouldn't do that.
> 
> For me this "special case" looks like "let's break data structure
> consistency to reuse several lines of code".
> 
> With code snippets you show, it looks even worse: you assign same global
> variable in several places for different uses. 

If you mean lpuart_is_be, it's not for different uses.
The purpose is the same to align the correct endian but in two places.

> implicitly assuming that
> it is for same device. Which can be true in your current system, but not
> elsewhere (e.g. why not having lpuart programmed into fpga)?
> 

Sorry, What issues for fpga?

> Alternative solution could be - have separate write path for earlycon.

It looks to me having the same issue with a separate write patch
for earlycon as we still need distinguish Little or Big endian
for Layerscape and IMX.

> At a glance, it is dozen lines of code.

Would you please show some sample code?
Then we probably may understand better with each other.

Anyway, thanks for detailed review.

Regards
Dong Aisheng

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

[Nikita Yushchenko]

Hi

My view of your statement is:
- you currently assume only a few cases for this driver - builtin UART
in vf610, ls1012a and imx7,
- in each of these cases, all lpuart instances share same endian, thus
having that in global var works for these cases,
- having that in global var makes it possible for you to write less
lines of code

My complain is:
- in Linux, we are trying to keep drivers generic,
- in Linux, having less lines of code has never been sufficient to break
basic data structure consistency,
- having driver to keep per-device capability in global var is a clear
case of breaking consistency.


>>> That's for special case, normally we wouldn't do that.
>>
>> For me this "special case" looks like "let's break data structure
>> consistency to reuse several lines of code".
>>
>> With code snippets you show, it looks even worse: you assign same global
>> variable in several places for different uses. 
> 
> If you mean lpuart_is_be, it's not for different uses.
> The purpose is the same to align the correct endian but in two places.

_probe() routine called for device X alters state already in use for
device Y.

> 
>> implicitly assuming that
>> it is for same device. Which can be true in your current system, but not
>> elsewhere (e.g. why not having lpuart programmed into fpga)?
>>
> 
> Sorry, What issues for fpga?

Connect FPGA to IMX7 based system and program LS1012a version of lpuart
core into it.  Have your console on system UART broken at time when
driver gets registered.


> 
>> Alternative solution could be - have separate write path for earlycon.
> 
> It looks to me having the same issue with a separate write patch
> for earlycon as we still need distinguish Little or Big endian
> for Layerscape and IMX.
> 
>> At a glance, it is dozen lines of code.
> 
> Would you please show some sample code?

Do not reuse lpuart32_console_putchar() in earlycon code.

Have two sets of early_setup/early_write/putchar - for BE and
defaut-endian earlycon. And in these putchar's do not use
lpuart_(read|write).


As far as I can see, fsl_lpuart.c already has two drivers in one -
there is separate set of routines for 8bit and 32bit cases.
And those routines that are common, have if blocks that separate cases.
I think these drivers will be cleaner if separated.
However that's completely different story.

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

[Nikita Yushchenko]

Hi

My view of your statement is:
- you currently assume only a few cases for this driver - builtin UART
in vf610, ls1012a and imx7,
- in each of these cases, all lpuart instances share same endian, thus
having that in global var works for these cases,
- having that in global var makes it possible for you to write less
lines of code

My complain is:
- in Linux, we are trying to keep drivers generic,
- in Linux, having less lines of code has never been sufficient to break
basic data structure consistency,
- having driver to keep per-device capability in global var is a clear
case of breaking consistency.


>>> That's for special case, normally we wouldn't do that.
>>
>> For me this "special case" looks like "let's break data structure
>> consistency to reuse several lines of code".
>>
>> With code snippets you show, it looks even worse: you assign same global
>> variable in several places for different uses. 
> 
> If you mean lpuart_is_be, it's not for different uses.
> The purpose is the same to align the correct endian but in two places.

_probe() routine called for device X alters state already in use for
device Y.

> 
>> implicitly assuming that
>> it is for same device. Which can be true in your current system, but not
>> elsewhere (e.g. why not having lpuart programmed into fpga)?
>>
> 
> Sorry, What issues for fpga?

Connect FPGA to IMX7 based system and program LS1012a version of lpuart
core into it.  Have your console on system UART broken at time when
driver gets registered.


> 
>> Alternative solution could be - have separate write path for earlycon.
> 
> It looks to me having the same issue with a separate write patch
> for earlycon as we still need distinguish Little or Big endian
> for Layerscape and IMX.
> 
>> At a glance, it is dozen lines of code.
> 
> Would you please show some sample code?

Do not reuse lpuart32_console_putchar() in earlycon code.

Have two sets of early_setup/early_write/putchar - for BE and
defaut-endian earlycon. And in these putchar's do not use
lpuart_(read|write).


As far as I can see, fsl_lpuart.c already has two drivers in one -
there is separate set of routines for 8bit and 32bit cases.
And those routines that are common, have if blocks that separate cases.
I think these drivers will be cleaner if separated.
However that's completely different story.

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

[Nikita Yushchenko]

Hi

My view of your statement is:
- you currently assume only a few cases for this driver - builtin UART
in vf610, ls1012a and imx7,
- in each of these cases, all lpuart instances share same endian, thus
having that in global var works for these cases,
- having that in global var makes it possible for you to write less
lines of code

My complain is:
- in Linux, we are trying to keep drivers generic,
- in Linux, having less lines of code has never been sufficient to break
basic data structure consistency,
- having driver to keep per-device capability in global var is a clear
case of breaking consistency.


>>> That's for special case, normally we wouldn't do that.
>>
>> For me this "special case" looks like "let's break data structure
>> consistency to reuse several lines of code".
>>
>> With code snippets you show, it looks even worse: you assign same global
>> variable in several places for different uses. 
> 
> If you mean lpuart_is_be, it's not for different uses.
> The purpose is the same to align the correct endian but in two places.

_probe() routine called for device X alters state already in use for
device Y.

> 
>> implicitly assuming that
>> it is for same device. Which can be true in your current system, but not
>> elsewhere (e.g. why not having lpuart programmed into fpga)?
>>
> 
> Sorry, What issues for fpga?

Connect FPGA to IMX7 based system and program LS1012a version of lpuart
core into it.  Have your console on system UART broken at time when
driver gets registered.


> 
>> Alternative solution could be - have separate write path for earlycon.
> 
> It looks to me having the same issue with a separate write patch
> for earlycon as we still need distinguish Little or Big endian
> for Layerscape and IMX.
> 
>> At a glance, it is dozen lines of code.
> 
> Would you please show some sample code?

Do not reuse lpuart32_console_putchar() in earlycon code.

Have two sets of early_setup/early_write/putchar - for BE and
defaut-endian earlycon. And in these putchar's do not use
lpuart_(read|write).


As far as I can see, fsl_lpuart.c already has two drivers in one -
there is separate set of routines for 8bit and 32bit cases.
And those routines that are common, have if blocks that separate cases.
I think these drivers will be cleaner if separated.
However that's completely different story.

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

[Dong Aisheng]

On Wed, May 17, 2017 at 11:04:04AM +0300, Nikita Yushchenko wrote:
> Hi
> 
> My view of your statement is:
> - you currently assume only a few cases for this driver - builtin UART
> in vf610, ls1012a and imx7,
> - in each of these cases, all lpuart instances share same endian, thus
> having that in global var works for these cases,
> - having that in global var makes it possible for you to write less
> lines of code
> 
> My complain is:
> - in Linux, we are trying to keep drivers generic,
> - in Linux, having less lines of code has never been sufficient to break
> basic data structure consistency,
> - having driver to keep per-device capability in global var is a clear
> case of breaking consistency.
> 

Yes, i do understand your concern and i absolutely agree with the rule
you mentioned.

> 
> >>> That's for special case, normally we wouldn't do that.
> >>
> >> For me this "special case" looks like "let's break data structure
> >> consistency to reuse several lines of code".
> >>
> >> With code snippets you show, it looks even worse: you assign same global
> >> variable in several places for different uses. 
> > 
> > If you mean lpuart_is_be, it's not for different uses.
> > The purpose is the same to align the correct endian but in two places.
> 
> _probe() routine called for device X alters state already in use for
> device Y.
> 

Okay, you're saying two different types of devices appeared in one SoC.

> > 
> >> implicitly assuming that
> >> it is for same device. Which can be true in your current system, but not
> >> elsewhere (e.g. why not having lpuart programmed into fpga)?
> >>
> > 
> > Sorry, What issues for fpga?
> 
> Connect FPGA to IMX7 based system and program LS1012a version of lpuart
> core into it.  Have your console on system UART broken at time when
> driver gets registered.
> 

Well, theoretically it may happen.

> 
> > 
> >> Alternative solution could be - have separate write path for earlycon.
> > 
> > It looks to me having the same issue with a separate write patch
> > for earlycon as we still need distinguish Little or Big endian
> > for Layerscape and IMX.
> > 
> >> At a glance, it is dozen lines of code.
> > 
> > Would you please show some sample code?
> 
> Do not reuse lpuart32_console_putchar() in earlycon code.
> 
> Have two sets of early_setup/early_write/putchar - for BE and
> defaut-endian earlycon. And in these putchar's do not use
> lpuart_(read|write).
> 

Isn't that introducing another consistency break after fix one
consistency break?

If doing that, we then have two register read/write APIs.
One for normal driver operation by dynamically checking lpuart_is_be
property to distinguish the endian difference problem.
Another is specifically implemented for only early console read/write
and use hardcoded way to read/write register directly instead of using
the standard API lpuart32_read/write, like follows:
e.g.
lpuart32_le_console_write() {
	 writel();
}

lpuart32_be_console_write() {
	 iowrite32be()
}
This also makes the driver a bit strange and ugly.

It looks to me both way are trade offs and the later one seems sacrifice
more. And i doubt if it's really necessary for probably a no real gain
purpose as the FPGA you mentioned is a theoretical case and less
possibility to exist.

I'm still wondering how about keep using the exist way and adding more
information in code to explain why use a global var?

Regards
Dong Aisheng

> 
> As far as I can see, fsl_lpuart.c already has two drivers in one -
> there is separate set of routines for 8bit and 32bit cases.
> And those routines that are common, have if blocks that separate cases.
> I think these drivers will be cleaner if separated.
> However that's completely different story.

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

[Dong Aisheng]

On Wed, May 17, 2017 at 11:04:04AM +0300, Nikita Yushchenko wrote:
> Hi
> 
> My view of your statement is:
> - you currently assume only a few cases for this driver - builtin UART
> in vf610, ls1012a and imx7,
> - in each of these cases, all lpuart instances share same endian, thus
> having that in global var works for these cases,
> - having that in global var makes it possible for you to write less
> lines of code
> 
> My complain is:
> - in Linux, we are trying to keep drivers generic,
> - in Linux, having less lines of code has never been sufficient to break
> basic data structure consistency,
> - having driver to keep per-device capability in global var is a clear
> case of breaking consistency.
> 

Yes, i do understand your concern and i absolutely agree with the rule
you mentioned.

> 
> >>> That's for special case, normally we wouldn't do that.
> >>
> >> For me this "special case" looks like "let's break data structure
> >> consistency to reuse several lines of code".
> >>
> >> With code snippets you show, it looks even worse: you assign same global
> >> variable in several places for different uses. 
> > 
> > If you mean lpuart_is_be, it's not for different uses.
> > The purpose is the same to align the correct endian but in two places.
> 
> _probe() routine called for device X alters state already in use for
> device Y.
> 

Okay, you're saying two different types of devices appeared in one SoC.

> > 
> >> implicitly assuming that
> >> it is for same device. Which can be true in your current system, but not
> >> elsewhere (e.g. why not having lpuart programmed into fpga)?
> >>
> > 
> > Sorry, What issues for fpga?
> 
> Connect FPGA to IMX7 based system and program LS1012a version of lpuart
> core into it.  Have your console on system UART broken at time when
> driver gets registered.
> 

Well, theoretically it may happen.

> 
> > 
> >> Alternative solution could be - have separate write path for earlycon.
> > 
> > It looks to me having the same issue with a separate write patch
> > for earlycon as we still need distinguish Little or Big endian
> > for Layerscape and IMX.
> > 
> >> At a glance, it is dozen lines of code.
> > 
> > Would you please show some sample code?
> 
> Do not reuse lpuart32_console_putchar() in earlycon code.
> 
> Have two sets of early_setup/early_write/putchar - for BE and
> defaut-endian earlycon. And in these putchar's do not use
> lpuart_(read|write).
> 

Isn't that introducing another consistency break after fix one
consistency break?

If doing that, we then have two register read/write APIs.
One for normal driver operation by dynamically checking lpuart_is_be
property to distinguish the endian difference problem.
Another is specifically implemented for only early console read/write
and use hardcoded way to read/write register directly instead of using
the standard API lpuart32_read/write, like follows:
e.g.
lpuart32_le_console_write() {
	 writel();
}

lpuart32_be_console_write() {
	 iowrite32be()
}
This also makes the driver a bit strange and ugly.

It looks to me both way are trade offs and the later one seems sacrifice
more. And i doubt if it's really necessary for probably a no real gain
purpose as the FPGA you mentioned is a theoretical case and less
possibility to exist.

I'm still wondering how about keep using the exist way and adding more
information in code to explain why use a global var?

Regards
Dong Aisheng

> 
> As far as I can see, fsl_lpuart.c already has two drivers in one -
> there is separate set of routines for 8bit and 32bit cases.
> And those routines that are common, have if blocks that separate cases.
> I think these drivers will be cleaner if separated.
> However that's completely different story.

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

[Dong Aisheng]

On Wed, May 17, 2017 at 11:04:04AM +0300, Nikita Yushchenko wrote:
> Hi
> 
> My view of your statement is:
> - you currently assume only a few cases for this driver - builtin UART
> in vf610, ls1012a and imx7,
> - in each of these cases, all lpuart instances share same endian, thus
> having that in global var works for these cases,
> - having that in global var makes it possible for you to write less
> lines of code
> 
> My complain is:
> - in Linux, we are trying to keep drivers generic,
> - in Linux, having less lines of code has never been sufficient to break
> basic data structure consistency,
> - having driver to keep per-device capability in global var is a clear
> case of breaking consistency.
> 

Yes, i do understand your concern and i absolutely agree with the rule
you mentioned.

> 
> >>> That's for special case, normally we wouldn't do that.
> >>
> >> For me this "special case" looks like "let's break data structure
> >> consistency to reuse several lines of code".
> >>
> >> With code snippets you show, it looks even worse: you assign same global
> >> variable in several places for different uses. 
> > 
> > If you mean lpuart_is_be, it's not for different uses.
> > The purpose is the same to align the correct endian but in two places.
> 
> _probe() routine called for device X alters state already in use for
> device Y.
> 

Okay, you're saying two different types of devices appeared in one SoC.

> > 
> >> implicitly assuming that
> >> it is for same device. Which can be true in your current system, but not
> >> elsewhere (e.g. why not having lpuart programmed into fpga)?
> >>
> > 
> > Sorry, What issues for fpga?
> 
> Connect FPGA to IMX7 based system and program LS1012a version of lpuart
> core into it.  Have your console on system UART broken at time when
> driver gets registered.
> 

Well, theoretically it may happen.

> 
> > 
> >> Alternative solution could be - have separate write path for earlycon.
> > 
> > It looks to me having the same issue with a separate write patch
> > for earlycon as we still need distinguish Little or Big endian
> > for Layerscape and IMX.
> > 
> >> At a glance, it is dozen lines of code.
> > 
> > Would you please show some sample code?
> 
> Do not reuse lpuart32_console_putchar() in earlycon code.
> 
> Have two sets of early_setup/early_write/putchar - for BE and
> defaut-endian earlycon. And in these putchar's do not use
> lpuart_(read|write).
> 

Isn't that introducing another consistency break after fix one
consistency break?

If doing that, we then have two register read/write APIs.
One for normal driver operation by dynamically checking lpuart_is_be
property to distinguish the endian difference problem.
Another is specifically implemented for only early console read/write
and use hardcoded way to read/write register directly instead of using
the standard API lpuart32_read/write, like follows:
e.g.
lpuart32_le_console_write() {
	 writel();
}

lpuart32_be_console_write() {
	 iowrite32be()
}
This also makes the driver a bit strange and ugly.

It looks to me both way are trade offs and the later one seems sacrifice
more. And i doubt if it's really necessary for probably a no real gain
purpose as the FPGA you mentioned is a theoretical case and less
possibility to exist.

I'm still wondering how about keep using the exist way and adding more
information in code to explain why use a global var?

Regards
Dong Aisheng

> 
> As far as I can see, fsl_lpuart.c already has two drivers in one -
> there is separate set of routines for 8bit and 32bit cases.
> And those routines that are common, have if blocks that separate cases.
> I think these drivers will be cleaner if separated.
> However that's completely different story.

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

[Nikita Yushchenko]

Hi,

>>>> Alternative solution could be - have separate write path for earlycon.
>>>
>>> It looks to me having the same issue with a separate write patch
>>> for earlycon as we still need distinguish Little or Big endian
>>> for Layerscape and IMX.
>>>
>>>> At a glance, it is dozen lines of code.
>>>
>>> Would you please show some sample code?
>>
>> Do not reuse lpuart32_console_putchar() in earlycon code.
>>
>> Have two sets of early_setup/early_write/putchar - for BE and
>> defaut-endian earlycon. And in these putchar's do not use
>> lpuart_(read|write).
>>
> 
> Isn't that introducing another consistency break after fix one
> consistency break?
> 
> If doing that, we then have two register read/write APIs.
> One for normal driver operation by dynamically checking lpuart_is_be
> property to distinguish the endian difference problem.
> Another is specifically implemented for only early console read/write
> and use hardcoded way to read/write register directly instead of using
> the standard API lpuart32_read/write, like follows:
> e.g.
> lpuart32_le_console_write() {
> 	 writel();
> }
> 
> lpuart32_be_console_write() {
> 	 iowrite32be()
> }
> This also makes the driver a bit strange and ugly.
> 
> It looks to me both way are trade offs and the later one seems sacrifice
> more. And i doubt if it's really necessary for probably a no real gain
> purpose as the FPGA you mentioned is a theoretical case and less
> possibility to exist.
> 
> I'm still wondering how about keep using the exist way and adding more
> information in code to explain why use a global var?

I've checked other driver under drivers/tty/serial/, for examples of
similar cases.

Please look at serial8250_early_in() / serial8250_early_out() ?
These do handle different endian, via port->iotype

Another example is drivers/tty/serial/samsung.c, where
port->private_data is initialized and used.

Nikita

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

[Nikita Yushchenko]

Hi,

>>>> Alternative solution could be - have separate write path for earlycon.
>>>
>>> It looks to me having the same issue with a separate write patch
>>> for earlycon as we still need distinguish Little or Big endian
>>> for Layerscape and IMX.
>>>
>>>> At a glance, it is dozen lines of code.
>>>
>>> Would you please show some sample code?
>>
>> Do not reuse lpuart32_console_putchar() in earlycon code.
>>
>> Have two sets of early_setup/early_write/putchar - for BE and
>> defaut-endian earlycon. And in these putchar's do not use
>> lpuart_(read|write).
>>
> 
> Isn't that introducing another consistency break after fix one
> consistency break?
> 
> If doing that, we then have two register read/write APIs.
> One for normal driver operation by dynamically checking lpuart_is_be
> property to distinguish the endian difference problem.
> Another is specifically implemented for only early console read/write
> and use hardcoded way to read/write register directly instead of using
> the standard API lpuart32_read/write, like follows:
> e.g.
> lpuart32_le_console_write() {
> 	 writel();
> }
> 
> lpuart32_be_console_write() {
> 	 iowrite32be()
> }
> This also makes the driver a bit strange and ugly.
> 
> It looks to me both way are trade offs and the later one seems sacrifice
> more. And i doubt if it's really necessary for probably a no real gain
> purpose as the FPGA you mentioned is a theoretical case and less
> possibility to exist.
> 
> I'm still wondering how about keep using the exist way and adding more
> information in code to explain why use a global var?

I've checked other driver under drivers/tty/serial/, for examples of
similar cases.

Please look at serial8250_early_in() / serial8250_early_out() ?
These do handle different endian, via port->iotype

Another example is drivers/tty/serial/samsung.c, where
port->private_data is initialized and used.

Nikita

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

[Nikita Yushchenko]

Hi,

>>>> Alternative solution could be - have separate write path for earlycon.
>>>
>>> It looks to me having the same issue with a separate write patch
>>> for earlycon as we still need distinguish Little or Big endian
>>> for Layerscape and IMX.
>>>
>>>> At a glance, it is dozen lines of code.
>>>
>>> Would you please show some sample code?
>>
>> Do not reuse lpuart32_console_putchar() in earlycon code.
>>
>> Have two sets of early_setup/early_write/putchar - for BE and
>> defaut-endian earlycon. And in these putchar's do not use
>> lpuart_(read|write).
>>
> 
> Isn't that introducing another consistency break after fix one
> consistency break?
> 
> If doing that, we then have two register read/write APIs.
> One for normal driver operation by dynamically checking lpuart_is_be
> property to distinguish the endian difference problem.
> Another is specifically implemented for only early console read/write
> and use hardcoded way to read/write register directly instead of using
> the standard API lpuart32_read/write, like follows:
> e.g.
> lpuart32_le_console_write() {
> 	 writel();
> }
> 
> lpuart32_be_console_write() {
> 	 iowrite32be()
> }
> This also makes the driver a bit strange and ugly.
> 
> It looks to me both way are trade offs and the later one seems sacrifice
> more. And i doubt if it's really necessary for probably a no real gain
> purpose as the FPGA you mentioned is a theoretical case and less
> possibility to exist.
> 
> I'm still wondering how about keep using the exist way and adding more
> information in code to explain why use a global var?

I've checked other driver under drivers/tty/serial/, for examples of
similar cases.

Please look at serial8250_early_in() / serial8250_early_out() ?
These do handle different endian, via port->iotype

Another example is drivers/tty/serial/samsung.c, where
port->private_data is initialized and used.

Nikita

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

[Dong Aisheng]

Hi Nikita,

On Tue, May 23, 2017 at 08:24:18AM +0300, Nikita Yushchenko wrote:
> Hi,
> 
> >>>> Alternative solution could be - have separate write path for earlycon.
> >>>
> >>> It looks to me having the same issue with a separate write patch
> >>> for earlycon as we still need distinguish Little or Big endian
> >>> for Layerscape and IMX.
> >>>
> >>>> At a glance, it is dozen lines of code.
> >>>
> >>> Would you please show some sample code?
> >>
> >> Do not reuse lpuart32_console_putchar() in earlycon code.
> >>
> >> Have two sets of early_setup/early_write/putchar - for BE and
> >> defaut-endian earlycon. And in these putchar's do not use
> >> lpuart_(read|write).
> >>
> > 
> > Isn't that introducing another consistency break after fix one
> > consistency break?
> > 
> > If doing that, we then have two register read/write APIs.
> > One for normal driver operation by dynamically checking lpuart_is_be
> > property to distinguish the endian difference problem.
> > Another is specifically implemented for only early console read/write
> > and use hardcoded way to read/write register directly instead of using
> > the standard API lpuart32_read/write, like follows:
> > e.g.
> > lpuart32_le_console_write() {
> > 	 writel();
> > }
> > 
> > lpuart32_be_console_write() {
> > 	 iowrite32be()
> > }
> > This also makes the driver a bit strange and ugly.
> > 
> > It looks to me both way are trade offs and the later one seems sacrifice
> > more. And i doubt if it's really necessary for probably a no real gain
> > purpose as the FPGA you mentioned is a theoretical case and less
> > possibility to exist.
> > 
> > I'm still wondering how about keep using the exist way and adding more
> > information in code to explain why use a global var?
> 
> I've checked other driver under drivers/tty/serial/, for examples of
> similar cases.
> 
> Please look at serial8250_early_in() / serial8250_early_out() ?
> These do handle different endian, via port->iotype
> 

Well, that does inspire me.

With struct uart_port's iotype member, the global lpuart_is_be can be
gone. We can update lpuart32_read/write API to take the reference of
of structure uart_port, then the API knows the endian information and
can use the correct further IO accessor accordingly.

And most importantly, it also works with earlycon.

> Another example is drivers/tty/serial/samsung.c, where
> port->private_data is initialized and used.
> 

If using iotype, seems no need private_data anymore.
Will try and send the new series later with you CCed to help review.

Thanks for the advice.

Regards
Dong Aisheng

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

[Dong Aisheng]

Hi Nikita,

On Tue, May 23, 2017 at 08:24:18AM +0300, Nikita Yushchenko wrote:
> Hi,
> 
> >>>> Alternative solution could be - have separate write path for earlycon.
> >>>
> >>> It looks to me having the same issue with a separate write patch
> >>> for earlycon as we still need distinguish Little or Big endian
> >>> for Layerscape and IMX.
> >>>
> >>>> At a glance, it is dozen lines of code.
> >>>
> >>> Would you please show some sample code?
> >>
> >> Do not reuse lpuart32_console_putchar() in earlycon code.
> >>
> >> Have two sets of early_setup/early_write/putchar - for BE and
> >> defaut-endian earlycon. And in these putchar's do not use
> >> lpuart_(read|write).
> >>
> > 
> > Isn't that introducing another consistency break after fix one
> > consistency break?
> > 
> > If doing that, we then have two register read/write APIs.
> > One for normal driver operation by dynamically checking lpuart_is_be
> > property to distinguish the endian difference problem.
> > Another is specifically implemented for only early console read/write
> > and use hardcoded way to read/write register directly instead of using
> > the standard API lpuart32_read/write, like follows:
> > e.g.
> > lpuart32_le_console_write() {
> > 	 writel();
> > }
> > 
> > lpuart32_be_console_write() {
> > 	 iowrite32be()
> > }
> > This also makes the driver a bit strange and ugly.
> > 
> > It looks to me both way are trade offs and the later one seems sacrifice
> > more. And i doubt if it's really necessary for probably a no real gain
> > purpose as the FPGA you mentioned is a theoretical case and less
> > possibility to exist.
> > 
> > I'm still wondering how about keep using the exist way and adding more
> > information in code to explain why use a global var?
> 
> I've checked other driver under drivers/tty/serial/, for examples of
> similar cases.
> 
> Please look at serial8250_early_in() / serial8250_early_out() ?
> These do handle different endian, via port->iotype
> 

Well, that does inspire me.

With struct uart_port's iotype member, the global lpuart_is_be can be
gone. We can update lpuart32_read/write API to take the reference of
of structure uart_port, then the API knows the endian information and
can use the correct further IO accessor accordingly.

And most importantly, it also works with earlycon.

> Another example is drivers/tty/serial/samsung.c, where
> port->private_data is initialized and used.
> 

If using iotype, seems no need private_data anymore.
Will try and send the new series later with you CCed to help review.

Thanks for the advice.

Regards
Dong Aisheng

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

[Dong Aisheng]

Hi Nikita,

On Tue, May 23, 2017 at 08:24:18AM +0300, Nikita Yushchenko wrote:
> Hi,
> 
> >>>> Alternative solution could be - have separate write path for earlycon.
> >>>
> >>> It looks to me having the same issue with a separate write patch
> >>> for earlycon as we still need distinguish Little or Big endian
> >>> for Layerscape and IMX.
> >>>
> >>>> At a glance, it is dozen lines of code.
> >>>
> >>> Would you please show some sample code?
> >>
> >> Do not reuse lpuart32_console_putchar() in earlycon code.
> >>
> >> Have two sets of early_setup/early_write/putchar - for BE and
> >> defaut-endian earlycon. And in these putchar's do not use
> >> lpuart_(read|write).
> >>
> > 
> > Isn't that introducing another consistency break after fix one
> > consistency break?
> > 
> > If doing that, we then have two register read/write APIs.
> > One for normal driver operation by dynamically checking lpuart_is_be
> > property to distinguish the endian difference problem.
> > Another is specifically implemented for only early console read/write
> > and use hardcoded way to read/write register directly instead of using
> > the standard API lpuart32_read/write, like follows:
> > e.g.
> > lpuart32_le_console_write() {
> > 	 writel();
> > }
> > 
> > lpuart32_be_console_write() {
> > 	 iowrite32be()
> > }
> > This also makes the driver a bit strange and ugly.
> > 
> > It looks to me both way are trade offs and the later one seems sacrifice
> > more. And i doubt if it's really necessary for probably a no real gain
> > purpose as the FPGA you mentioned is a theoretical case and less
> > possibility to exist.
> > 
> > I'm still wondering how about keep using the exist way and adding more
> > information in code to explain why use a global var?
> 
> I've checked other driver under drivers/tty/serial/, for examples of
> similar cases.
> 
> Please look at serial8250_early_in() / serial8250_early_out() ?
> These do handle different endian, via port->iotype
> 

Well, that does inspire me.

With struct uart_port's iotype member, the global lpuart_is_be can be
gone. We can update lpuart32_read/write API to take the reference of
of structure uart_port, then the API knows the endian information and
can use the correct further IO accessor accordingly.

And most importantly, it also works with earlycon.

> Another example is drivers/tty/serial/samsung.c, where
> port->private_data is initialized and used.
> 

If using iotype, seems no need private_data anymore.
Will try and send the new series later with you CCed to help review.

Thanks for the advice.

Regards
Dong Aisheng

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

[Nikita Yushchenko]

>  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);
>  }

What if this is ever executed on big endian system?

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

[Nikita Yushchenko]

>  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);
>  }

What if this is ever executed on big endian system?

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

[Dong Aisheng]

On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
> >  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);
> >  }
> 
> What if this is ever executed on big endian system?
> 

Sorry, not catching the point...

What issues will meet?

Regards
Dong Aisheng

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

[Dong Aisheng]

On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
> >  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);
> >  }
> 
> What if this is ever executed on big endian system?
> 

Sorry, not catching the point...

What issues will meet?

Regards
Dong Aisheng

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

[Nikita Yushchenko]

17.05.2017 06:39, Dong Aisheng wrote:
> On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
>>>  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);
>>>  }
>>
>> What if this is ever executed on big endian system?
>>
> 
> Sorry, not catching the point...
> 
> What issues will meet?

Isn't writel() in host endian?

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

[Nikita Yushchenko]

17.05.2017 06:39, Dong Aisheng wrote:
> On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
>>>  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);
>>>  }
>>
>> What if this is ever executed on big endian system?
>>
> 
> Sorry, not catching the point...
> 
> What issues will meet?

Isn't writel() in host endian?

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

[Dong Aisheng]

On Wed, May 17, 2017 at 08:37:41AM +0300, Nikita Yushchenko wrote:
> 
> 
> 17.05.2017 06:39, Dong Aisheng wrote:
> > On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
> >>>  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);
> >>>  }
> >>
> >> What if this is ever executed on big endian system?
> >>
> > 
> > Sorry, not catching the point...
> > 
> > What issues will meet?
> 
> Isn't writel() in host endian?

On big endian systems, it is supposed to run iowrite32be.

Regards
Dong Aisheng

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

[Dong Aisheng]

On Wed, May 17, 2017 at 08:37:41AM +0300, Nikita Yushchenko wrote:
> 
> 
> 17.05.2017 06:39, Dong Aisheng wrote:
> > On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
> >>>  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);
> >>>  }
> >>
> >> What if this is ever executed on big endian system?
> >>
> > 
> > Sorry, not catching the point...
> > 
> > What issues will meet?
> 
> Isn't writel() in host endian?

On big endian systems, it is supposed to run iowrite32be.

Regards
Dong Aisheng

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

[Nikita Yushchenko]

>>>>>  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);
>>>>>  }
>>>>
>>>> What if this is ever executed on big endian system?
>>>>
>>>
>>> Sorry, not catching the point...
>>>
>>> What issues will meet?
>>
>> Isn't writel() in host endian?
> 
> On big endian systems, it is supposed to run iowrite32be.

Your code states, "force BE if lpuart_is_be, don't care otherwise".
This semantics looks questionable for code reviewer.
If driver handles endian, should't it be explicit in both cases?
And if indeed driver means handling BE explicitly, but don't caring
otherwise, maybe variable name should suggest that (i.e. "force_be")?

Although driver maintainer could think differently. I won't insist on this.

Nikita

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

[Nikita Yushchenko]

>>>>>  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);
>>>>>  }
>>>>
>>>> What if this is ever executed on big endian system?
>>>>
>>>
>>> Sorry, not catching the point...
>>>
>>> What issues will meet?
>>
>> Isn't writel() in host endian?
> 
> On big endian systems, it is supposed to run iowrite32be.

Your code states, "force BE if lpuart_is_be, don't care otherwise".
This semantics looks questionable for code reviewer.
If driver handles endian, should't it be explicit in both cases?
And if indeed driver means handling BE explicitly, but don't caring
otherwise, maybe variable name should suggest that (i.e. "force_be")?

Although driver maintainer could think differently. I won't insist on this.

Nikita

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

[Dong Aisheng]

On Wed, May 17, 2017 at 08:50:39AM +0300, Nikita Yushchenko wrote:
> >>>>>  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);
> >>>>>  }
> >>>>
> >>>> What if this is ever executed on big endian system?
> >>>>
> >>>
> >>> Sorry, not catching the point...
> >>>
> >>> What issues will meet?
> >>
> >> Isn't writel() in host endian?
> > 
> > On big endian systems, it is supposed to run iowrite32be.
> 
> Your code states, "force BE if lpuart_is_be, don't care otherwise".
> This semantics looks questionable for code reviewer.
> If driver handles endian, should't it be explicit in both cases?
> And if indeed driver means handling BE explicitly, but don't caring
> otherwise, maybe variable name should suggest that (i.e. "force_be")?
> 

For lpuart32, only two cases that LS platforms is Big endian while
IMX is little endian.

It's SoC IP native property, i don't think force_be is better.

Regards
Dong Aisheng

> Although driver maintainer could think differently. I won't insist on this.
> 
> Nikita

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

[Dong Aisheng]

On Wed, May 17, 2017 at 08:50:39AM +0300, Nikita Yushchenko wrote:
> >>>>>  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);
> >>>>>  }
> >>>>
> >>>> What if this is ever executed on big endian system?
> >>>>
> >>>
> >>> Sorry, not catching the point...
> >>>
> >>> What issues will meet?
> >>
> >> Isn't writel() in host endian?
> > 
> > On big endian systems, it is supposed to run iowrite32be.
> 
> Your code states, "force BE if lpuart_is_be, don't care otherwise".
> This semantics looks questionable for code reviewer.
> If driver handles endian, should't it be explicit in both cases?
> And if indeed driver means handling BE explicitly, but don't caring
> otherwise, maybe variable name should suggest that (i.e. "force_be")?
> 

For lpuart32, only two cases that LS platforms is Big endian while
IMX is little endian.

It's SoC IP native property, i don't think force_be is better.

Regards
Dong Aisheng

> Although driver maintainer could think differently. I won't insist on this.
> 
> Nikita

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

[Andy Shevchenko]

On Wed, May 17, 2017 at 8:43 AM, Dong Aisheng <dongas86@gmail.com> wrote:
> On Wed, May 17, 2017 at 08:37:41AM +0300, Nikita Yushchenko wrote:
>>
>>
>> 17.05.2017 06:39, Dong Aisheng wrote:
>> > On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
>> >>>  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);
>> >>>  }
>> >>
>> >> What if this is ever executed on big endian system?
>> >>
>> >
>> > Sorry, not catching the point...
>> >
>> > What issues will meet?
>>
>> Isn't writel() in host endian?
>
> On big endian systems, it is supposed to run iowrite32be.

It looks like you substituting *bus* side with CPU *side* of communication.

If you are talking about CPU side
__raw_readl() / __raw_writel() will do the trick.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, May 17, 2017 at 8:43 AM, Dong Aisheng <dongas86@gmail.com> wrote:
> On Wed, May 17, 2017 at 08:37:41AM +0300, Nikita Yushchenko wrote:
>>
>>
>> 17.05.2017 06:39, Dong Aisheng wrote:
>> > On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
>> >>>  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);
>> >>>  }
>> >>
>> >> What if this is ever executed on big endian system?
>> >>
>> >
>> > Sorry, not catching the point...
>> >
>> > What issues will meet?
>>
>> Isn't writel() in host endian?
>
> On big endian systems, it is supposed to run iowrite32be.

It looks like you substituting *bus* side with CPU *side* of communication.

If you are talking about CPU side
__raw_readl() / __raw_writel() will do the trick.

-- 
With Best Regards,
Andy Shevchenko

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

[Dong Aisheng]

Hi Andy,

On Wed, May 17, 2017 at 12:55:59PM +0300, Andy Shevchenko wrote:
> On Wed, May 17, 2017 at 8:43 AM, Dong Aisheng <dongas86@gmail.com> wrote:
> > On Wed, May 17, 2017 at 08:37:41AM +0300, Nikita Yushchenko wrote:
> >>
> >>
> >> 17.05.2017 06:39, Dong Aisheng wrote:
> >> > On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
> >> >>>  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);
> >> >>>  }
> >> >>
> >> >> What if this is ever executed on big endian system?
> >> >>
> >> >
> >> > Sorry, not catching the point...
> >> >
> >> > What issues will meet?
> >>
> >> Isn't writel() in host endian?
> >
> > On big endian systems, it is supposed to run iowrite32be.
> 
> It looks like you substituting *bus* side with CPU *side* of communication.
> 
> If you are talking about CPU side
> __raw_readl() / __raw_writel() will do the trick.
> 

Yes, you're right.
To be more accurate, for bus side, if it's BE lpuart it should use
ioread32be/iowrite32be. If it's LE lpuart, it should use readl/writel.

And for CPU side, endian is transparent to driver users as they're
hided in io accessors implementation.

Regards
Dong Aisheng

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

[Dong Aisheng]

Hi Andy,

On Wed, May 17, 2017 at 12:55:59PM +0300, Andy Shevchenko wrote:
> On Wed, May 17, 2017 at 8:43 AM, Dong Aisheng <dongas86@gmail.com> wrote:
> > On Wed, May 17, 2017 at 08:37:41AM +0300, Nikita Yushchenko wrote:
> >>
> >>
> >> 17.05.2017 06:39, Dong Aisheng wrote:
> >> > On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
> >> >>>  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);
> >> >>>  }
> >> >>
> >> >> What if this is ever executed on big endian system?
> >> >>
> >> >
> >> > Sorry, not catching the point...
> >> >
> >> > What issues will meet?
> >>
> >> Isn't writel() in host endian?
> >
> > On big endian systems, it is supposed to run iowrite32be.
> 
> It looks like you substituting *bus* side with CPU *side* of communication.
> 
> If you are talking about CPU side
> __raw_readl() / __raw_writel() will do the trick.
> 

Yes, you're right.
To be more accurate, for bus side, if it's BE lpuart it should use
ioread32be/iowrite32be. If it's LE lpuart, it should use readl/writel.

And for CPU side, endian is transparent to driver users as they're
hided in io accessors implementation.

Regards
Dong Aisheng

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

[Andy Shevchenko]

On Tue, May 16, 2017 at 2:15 PM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:
>>  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);
>>  }
>
> What if this is ever executed on big endian system?

Above is apparently about bus side of communication and it looks like
it needs to be CPU side. Is this what you are trying to discuss?


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, May 16, 2017 at 2:15 PM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:
>>  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);
>>  }
>
> What if this is ever executed on big endian system?

Above is apparently about bus side of communication and it looks like
it needs to be CPU side. Is this what you are trying to discuss?


-- 
With Best Regards,
Andy Shevchenko

[PATCH V2 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Fugang Duan <fugang.duan@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 V2 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 at vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Fugang Duan <fugang.duan@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 V2 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Fugang Duan <fugang.duan@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 V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

---
ChangeLog:
v1->v2:
 * remove lpuart_reg_off according to Stefan's suggestion
---
 drivers/tty/serial/fsl_lpuart.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index e72b397..a3ee825 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -231,6 +231,9 @@
 #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;
 
 struct lpuart_port {
@@ -263,6 +266,7 @@ struct lpuart_port {
 struct lpuart_soc_data {
 	bool	is_32;
 	bool	is_be;
+	u8	reg_off;
 };
 
 static const struct lpuart_soc_data vf_data = {
@@ -272,11 +276,19 @@ static const struct lpuart_soc_data vf_data = {
 static const 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);
@@ -2014,6 +2026,7 @@ static int lpuart_probe(struct platform_device *pdev)
 	if (IS_ERR(sport->port.membase))
 		return PTR_ERR(sport->port.membase);
 
+	sport->port.membase += sdata->reg_off;
 	sport->port.mapbase = res->start;
 	sport->port.dev = &pdev->dev;
 	sport->port.type = PORT_LPUART;
-- 
2.7.4

[PATCH V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

---
ChangeLog:
v1->v2:
 * remove lpuart_reg_off according to Stefan's suggestion
---
 drivers/tty/serial/fsl_lpuart.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index e72b397..a3ee825 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -231,6 +231,9 @@
 #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;
 
 struct lpuart_port {
@@ -263,6 +266,7 @@ struct lpuart_port {
 struct lpuart_soc_data {
 	bool	is_32;
 	bool	is_be;
+	u8	reg_off;
 };
 
 static const struct lpuart_soc_data vf_data = {
@@ -272,11 +276,19 @@ static const struct lpuart_soc_data vf_data = {
 static const 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);
@@ -2014,6 +2026,7 @@ static int lpuart_probe(struct platform_device *pdev)
 	if (IS_ERR(sport->port.membase))
 		return PTR_ERR(sport->port.membase);
 
+	sport->port.membase += sdata->reg_off;
 	sport->port.mapbase = res->start;
 	sport->port.dev = &pdev->dev;
 	sport->port.type = PORT_LPUART;
-- 
2.7.4

[PATCH V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

---
ChangeLog:
v1->v2:
 * remove lpuart_reg_off according to Stefan's suggestion
---
 drivers/tty/serial/fsl_lpuart.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index e72b397..a3ee825 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -231,6 +231,9 @@
 #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;
 
 struct lpuart_port {
@@ -263,6 +266,7 @@ struct lpuart_port {
 struct lpuart_soc_data {
 	bool	is_32;
 	bool	is_be;
+	u8	reg_off;
 };
 
 static const struct lpuart_soc_data vf_data = {
@@ -272,11 +276,19 @@ static const struct lpuart_soc_data vf_data = {
 static const 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);
@@ -2014,6 +2026,7 @@ static int lpuart_probe(struct platform_device *pdev)
 	if (IS_ERR(sport->port.membase))
 		return PTR_ERR(sport->port.membase);
 
+	sport->port.membase += sdata->reg_off;
 	sport->port.mapbase = res->start;
 	sport->port.dev = &pdev->dev;
 	sport->port.type = PORT_LPUART;
-- 
2.7.4

[PATCH V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

---
Change Log:
v1->v2:
 * updated due to lpuart_reg_off removed
---
 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 a3ee825..107d0911 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1976,8 +1976,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;
+
+	device->port.membase += 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 V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

---
Change Log:
v1->v2:
 * updated due to lpuart_reg_off removed
---
 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 a3ee825..107d0911 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1976,8 +1976,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;
+
+	device->port.membase += 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 V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

---
Change Log:
v1->v2:
 * updated due to lpuart_reg_off removed
---
 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 a3ee825..107d0911 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1976,8 +1976,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;
+
+	device->port.membase += 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 V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@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 107d0911..bda4b0c 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
@@ -1506,6 +1508,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)
 {
@@ -1611,12 +1679,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

[PATCH V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@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 107d0911..bda4b0c 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
@@ -1506,6 +1508,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)
 {
@@ -1611,12 +1679,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

[PATCH V2 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: Stefan Agner <stefan@agner.ch>
Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Fugang Duan <fugang.duan@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 107d0911..bda4b0c 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
@@ -1506,6 +1508,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)
 {
@@ -1611,12 +1679,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 V2 6/6] tty: serial: lpuart: add a more accurate baud rate calculation method

[Stefan Agner]

On 2017-05-15 00:48, Dong Aisheng 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.
> 
> Currently only i.MX7ULP is using it.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
> Cc: Yangbo Lu <yangbo.lu@nxp.com>
> Acked-by: Fugang Duan <fugang.duan@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 107d0911..bda4b0c 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
> @@ -1506,6 +1508,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)
>  {
> @@ -1611,12 +1679,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")) {

Shouldn't we be consequent here and also use a flag in the soc data
instead of of_device_is_compatible...?

Btw, instead of using 3 bools, I would prefer using a single flags like
your patchset is proposing for the GPIO driver, what do you think?

--
Stefan


> +		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 */

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

[Stefan Agner]

On 2017-05-15 00:48, Dong Aisheng 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.
> 
> Currently only i.MX7ULP is using it.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
> Cc: Yangbo Lu <yangbo.lu@nxp.com>
> Acked-by: Fugang Duan <fugang.duan@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 107d0911..bda4b0c 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
> @@ -1506,6 +1508,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)
>  {
> @@ -1611,12 +1679,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")) {

Shouldn't we be consequent here and also use a flag in the soc data
instead of of_device_is_compatible...?

Btw, instead of using 3 bools, I would prefer using a single flags like
your patchset is proposing for the GPIO driver, what do you think?

--
Stefan


> +		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 */

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

[Dong Aisheng]

On Mon, May 15, 2017 at 10:06:41AM -0700, Stefan Agner wrote:
> On 2017-05-15 00:48, Dong Aisheng 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.
> > 
> > Currently only i.MX7ULP is using it.
> > 
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jiri Slaby <jslaby@suse.com>
> > Cc: Stefan Agner <stefan@agner.ch>
> > Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
> > Cc: Yangbo Lu <yangbo.lu@nxp.com>
> > Acked-by: Fugang Duan <fugang.duan@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 107d0911..bda4b0c 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
> > @@ -1506,6 +1508,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)
> >  {
> > @@ -1611,12 +1679,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")) {
> 
> Shouldn't we be consequent here and also use a flag in the soc data
> instead of of_device_is_compatible...?
> 

The original purpose is that this is a temporary code and supposed will
be deleted later once LS platforms confirmed the new baud setting API
works for them as well.

That's why i did not make it a property, as i stated in the cover letter.

> Btw, instead of using 3 bools, I would prefer using a single flags like
> your patchset is proposing for the GPIO driver, what do you think?
> 

Yes, good suggestion.
Probably we could convert the below two.
.is_32 = true,
.is_be = true,

But reg_off seems better to be kept.

Regards
Dong Aisheng

> --
> Stefan
> 
> 
> > +		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 */

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

[Dong Aisheng]

On Mon, May 15, 2017 at 10:06:41AM -0700, Stefan Agner wrote:
> On 2017-05-15 00:48, Dong Aisheng 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.
> > 
> > Currently only i.MX7ULP is using it.
> > 
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jiri Slaby <jslaby@suse.com>
> > Cc: Stefan Agner <stefan@agner.ch>
> > Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
> > Cc: Yangbo Lu <yangbo.lu@nxp.com>
> > Acked-by: Fugang Duan <fugang.duan@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 107d0911..bda4b0c 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
> > @@ -1506,6 +1508,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)
> >  {
> > @@ -1611,12 +1679,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")) {
> 
> Shouldn't we be consequent here and also use a flag in the soc data
> instead of of_device_is_compatible...?
> 

The original purpose is that this is a temporary code and supposed will
be deleted later once LS platforms confirmed the new baud setting API
works for them as well.

That's why i did not make it a property, as i stated in the cover letter.

> Btw, instead of using 3 bools, I would prefer using a single flags like
> your patchset is proposing for the GPIO driver, what do you think?
> 

Yes, good suggestion.
Probably we could convert the below two.
.is_32 = true,
.is_be = true,

But reg_off seems better to be kept.

Regards
Dong Aisheng

> --
> Stefan
> 
> 
> > +		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 */

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

[Stefan Agner]

On 2017-05-16 20:47, Dong Aisheng wrote:
> On Mon, May 15, 2017 at 10:06:41AM -0700, Stefan Agner wrote:
>> On 2017-05-15 00:48, Dong Aisheng 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.
>> >
>> > Currently only i.MX7ULP is using it.
>> >
>> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > Cc: Jiri Slaby <jslaby@suse.com>
>> > Cc: Stefan Agner <stefan@agner.ch>
>> > Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
>> > Cc: Yangbo Lu <yangbo.lu@nxp.com>
>> > Acked-by: Fugang Duan <fugang.duan@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 107d0911..bda4b0c 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
>> > @@ -1506,6 +1508,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)
>> >  {
>> > @@ -1611,12 +1679,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")) {
>>
>> Shouldn't we be consequent here and also use a flag in the soc data
>> instead of of_device_is_compatible...?
>>
> 
> The original purpose is that this is a temporary code and supposed will
> be deleted later once LS platforms confirmed the new baud setting API
> works for them as well.
> 
> That's why i did not make it a property, as i stated in the cover letter.

Ok, I see that is a good reason to not define a new feature property
now... 

But, if you are reasonable sure it should work, I am inclined to say
just enable it for LS1021a so it also really gets tested...

> 
>> Btw, instead of using 3 bools, I would prefer using a single flags like
>> your patchset is proposing for the GPIO driver, what do you think?
>>
> 
> Yes, good suggestion.
> Probably we could convert the below two.
> .is_32 = true,
> .is_be = true,
> 
> But reg_off seems better to be kept.

Sounds good!

--
Stefan

> 
> Regards
> Dong Aisheng
> 
>> --
>> Stefan
>>
>>
>> > +		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 */

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

[Stefan Agner]

On 2017-05-16 20:47, Dong Aisheng wrote:
> On Mon, May 15, 2017 at 10:06:41AM -0700, Stefan Agner wrote:
>> On 2017-05-15 00:48, Dong Aisheng 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.
>> >
>> > Currently only i.MX7ULP is using it.
>> >
>> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > Cc: Jiri Slaby <jslaby@suse.com>
>> > Cc: Stefan Agner <stefan@agner.ch>
>> > Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
>> > Cc: Yangbo Lu <yangbo.lu@nxp.com>
>> > Acked-by: Fugang Duan <fugang.duan@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 107d0911..bda4b0c 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
>> > @@ -1506,6 +1508,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)
>> >  {
>> > @@ -1611,12 +1679,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")) {
>>
>> Shouldn't we be consequent here and also use a flag in the soc data
>> instead of of_device_is_compatible...?
>>
> 
> The original purpose is that this is a temporary code and supposed will
> be deleted later once LS platforms confirmed the new baud setting API
> works for them as well.
> 
> That's why i did not make it a property, as i stated in the cover letter.

Ok, I see that is a good reason to not define a new feature property
now... 

But, if you are reasonable sure it should work, I am inclined to say
just enable it for LS1021a so it also really gets tested...

> 
>> Btw, instead of using 3 bools, I would prefer using a single flags like
>> your patchset is proposing for the GPIO driver, what do you think?
>>
> 
> Yes, good suggestion.
> Probably we could convert the below two.
> .is_32 = true,
> .is_be = true,
> 
> But reg_off seems better to be kept.

Sounds good!

--
Stefan

> 
> Regards
> Dong Aisheng
> 
>> --
>> Stefan
>>
>>
>> > +		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 */

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

[Dong Aisheng]

On Wed, May 17, 2017 at 10:35:43AM -0700, Stefan Agner wrote:
> On 2017-05-16 20:47, Dong Aisheng wrote:
> > On Mon, May 15, 2017 at 10:06:41AM -0700, Stefan Agner wrote:
> >> On 2017-05-15 00:48, Dong Aisheng 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.
> >> >
> >> > Currently only i.MX7ULP is using it.
> >> >
> >> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> > Cc: Jiri Slaby <jslaby@suse.com>
> >> > Cc: Stefan Agner <stefan@agner.ch>
> >> > Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
> >> > Cc: Yangbo Lu <yangbo.lu@nxp.com>
> >> > Acked-by: Fugang Duan <fugang.duan@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 107d0911..bda4b0c 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
> >> > @@ -1506,6 +1508,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)
> >> >  {
> >> > @@ -1611,12 +1679,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")) {
> >>
> >> Shouldn't we be consequent here and also use a flag in the soc data
> >> instead of of_device_is_compatible...?
> >>
> > 
> > The original purpose is that this is a temporary code and supposed will
> > be deleted later once LS platforms confirmed the new baud setting API
> > works for them as well.
> > 
> > That's why i did not make it a property, as i stated in the cover letter.
> 
> Ok, I see that is a good reason to not define a new feature property
> now... 
> 
> But, if you are reasonable sure it should work, I am inclined to say
> just enable it for LS1021a so it also really gets tested...
> 

Okay, i'll take this suggestion to get things started.

Thanks

Regards
Dong Aisheng

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

[Dong Aisheng]

On Wed, May 17, 2017 at 10:35:43AM -0700, Stefan Agner wrote:
> On 2017-05-16 20:47, Dong Aisheng wrote:
> > On Mon, May 15, 2017 at 10:06:41AM -0700, Stefan Agner wrote:
> >> On 2017-05-15 00:48, Dong Aisheng 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.
> >> >
> >> > Currently only i.MX7ULP is using it.
> >> >
> >> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> > Cc: Jiri Slaby <jslaby@suse.com>
> >> > Cc: Stefan Agner <stefan@agner.ch>
> >> > Cc: Mingkai Hu <Mingkai.Hu@nxp.com>
> >> > Cc: Yangbo Lu <yangbo.lu@nxp.com>
> >> > Acked-by: Fugang Duan <fugang.duan@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 107d0911..bda4b0c 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
> >> > @@ -1506,6 +1508,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)
> >> >  {
> >> > @@ -1611,12 +1679,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")) {
> >>
> >> Shouldn't we be consequent here and also use a flag in the soc data
> >> instead of of_device_is_compatible...?
> >>
> > 
> > The original purpose is that this is a temporary code and supposed will
> > be deleted later once LS platforms confirmed the new baud setting API
> > works for them as well.
> > 
> > That's why i did not make it a property, as i stated in the cover letter.
> 
> Ok, I see that is a good reason to not define a new feature property
> now... 
> 
> But, if you are reasonable sure it should work, I am inclined to say
> just enable it for LS1021a so it also really gets tested...
> 

Okay, i'll take this suggestion to get things started.

Thanks

Regards
Dong Aisheng