[PATCH v4 0/6] Update Renesas {EMMA mobile, RZ/V2M} UART Port type

[PATCH v4 0/6] Update Renesas {EMMA mobile, RZ/V2M} UART Port type

[Biju Das]

The Renesas {EMMA mobile, RZ/V2M} UART compatible with the general-purpose
16750 UART chip. This patch updates Renesas {EMMA mobile, RZ/V2M} UART type
from 16550a->16750 and also enables 64-bytes fifo.

This patch series also simplifies 8250_em_probe() and also updates the
register handling for the below restriction mentioned in the hardware
manual.

40.6.1 Point for Caution when Changing the Register Settings:

When changing the settings of the following registers, a PRESETn master
reset or FIFO reset + SW reset (FCR[2],FCR[1], HCR0[7]) must be input to
re-initialize them.

Target Registers: FCR, LCR, MCR, DLL, DLM, HCR0.

v3->v4:
 * Split patch#1 to 3 patches and removed Rb tags.
 * Replaced dev_err->dev_err_probe() in probe()
 * Added a local variable 'dev' to replace '&pdev->dev' in probe().
 * Patch#2 removes unused header file slab.h.
 * Patch#3 simplifies clk handling in probe()/remove()
 * Removed sclk from struct serial8250_em_priv instead used local
   variable in probe()
 * Replaced devm_clk_get->devm_clk_get_enabled() and removed
   clk_prepare_enable/clk_disable_unprepare from probe()/remove().
 * Both {RZ/V2M, EMMA mobile} SoC is Register-compatible
   with the general-purpose 16750 UART chip. So started using
   generic compatible and removed struct serial8250_em_hw_info.
 * Introduced pseudo offset for UART_FCR(UART_FCR_EM) and used
   UART_FCR_EM in serial8250_em_serial{_in/_out} function to read/write
   FCR register.
 * Updated register update restriction to EMMA mobile SoC as well.
 * Replaced readl->serial8250_em_serial_in() for reading fcr register
   in serial8250_em_reg_update().
 * Added serial8250_em_serial_out_helper() to simplify the
   code and used for register writes in serial8250_em_reg_update().
v2->v3:
 * Dropped sclk from priv.
 * Dropped unneeded clk_disable_unprepare from remove().
 * Retained Rb tags from Geert,Andy and Ilpo as the changes are trivial.
 * Replaced of_device_get_match_data->device_get_match_data
 * Dropped struct serial8250_em_hw_info *info from priv and started
   using a local variable info in probe().
 * Retained Rb tag from Ilpo as changes are trivial.
 * Replaced readl/writel()->serial8250_em_serial_in/out() in serial8250_rzv2m_
   reg_update() to avoid duplication of offset trickery.
 * Added support for HCR0 read/write in serial8250_em_{serial_in, serial_out}
 * Added UART_LCR macro support in serial8250_em_serial_in() to read LCR
 * Reordered serial8250_em_{serial_in, serial_out} above serial8250_rzv2m_
   reg_update().
 * Replaced priv->info->serial_out to info->serial_out.
v1->v2:
 * Dropped patch#1 from previous series
 * Replaced devm_clk_get->devm_clk_get_enabled() and updated clk
   handling in probe().
 * Added Rb tag from Geert.
 * Added patch for updating Renesas RZ/V2M UART type from 16550a->16750
   and also enables 64-bytes fifo.
 * Used .data for taking h/w differences between EMMA mobile and RZ/V2M UART.
 * Added serial_out() to struct serial8250_em_hw_info for the write register
   differences between EMMA mobile and RZ/V2M UART.

Biju Das (6):
  serial: 8250_em: Use dev_err_probe()
  serial: 8250_em: Drop slab.h
  serial: 8250_em: Use devm_clk_get_enabled()
  serial: 8250_em: Update port type as PORT_16750
  serial: 8250_em: Use pseudo offset for UART_FCR
  serial: 8250_em: Add serial8250_em_{reg_update(),out_helper()}

 drivers/tty/serial/8250/8250_em.c | 110 +++++++++++++++++++++++-------
 1 file changed, 85 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH v4 1/6] serial: 8250_em: Use dev_err_probe()

[Biju Das]

This patch simplifies probe() by using dev_err() instead
of dev_err_probe() and added a local variable 'dev' to
replace '&pdev->dev'.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3->v4:
 * Split from patch#1 
 * Replaced dev_err->dev_err_probe() in probe()
 * Added a local variable 'dev' to replace '&pdev->dev' in probe().
v2->v3:
 * Dropped sclk from priv.
 * Dropped unneeded clk_disable_unprepare from remove().
 * Retained Rb tags from Geert and Andy as the changes are trivial.
v1->v2:
 * replaced devm_clk_get->devm_clk_get_enabled() and updated clk
   handling in probe().
 * Added Rb tag from Geert.
---
 drivers/tty/serial/8250/8250_em.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
index f8e99995eee9..785153d8502d 100644
--- a/drivers/tty/serial/8250/8250_em.c
+++ b/drivers/tty/serial/8250/8250_em.c
@@ -79,6 +79,7 @@ static void serial8250_em_serial_dl_write(struct uart_8250_port *up, int value)
 static int serial8250_em_probe(struct platform_device *pdev)
 {
 	struct serial8250_em_priv *priv;
+	struct device *dev = &pdev->dev;
 	struct uart_8250_port up;
 	struct resource *regs;
 	int irq, ret;
@@ -88,27 +89,23 @@ static int serial8250_em_probe(struct platform_device *pdev)
 		return irq;
 
 	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!regs) {
-		dev_err(&pdev->dev, "missing registers\n");
-		return -EINVAL;
-	}
+	if (!regs)
+		return dev_err_probe(dev, -EINVAL, "missing registers\n");
 
-	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	priv->sclk = devm_clk_get(&pdev->dev, "sclk");
-	if (IS_ERR(priv->sclk)) {
-		dev_err(&pdev->dev, "unable to get clock\n");
-		return PTR_ERR(priv->sclk);
-	}
+	priv->sclk = devm_clk_get(dev, "sclk");
+	if (IS_ERR(priv->sclk))
+		return dev_err_probe(dev, PTR_ERR(priv->sclk), "unable to get clock\n");
 
 	memset(&up, 0, sizeof(up));
 	up.port.mapbase = regs->start;
 	up.port.irq = irq;
 	up.port.type = PORT_UNKNOWN;
 	up.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP;
-	up.port.dev = &pdev->dev;
+	up.port.dev = dev;
 	up.port.private_data = priv;
 
 	clk_prepare_enable(priv->sclk);
@@ -122,9 +119,8 @@ static int serial8250_em_probe(struct platform_device *pdev)
 
 	ret = serial8250_register_8250_port(&up);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "unable to register 8250 port\n");
 		clk_disable_unprepare(priv->sclk);
-		return ret;
+		return dev_err_probe(dev, ret, "unable to register 8250 port\n");
 	}
 
 	priv->line = ret;
-- 
2.25.1

[PATCH v4 2/6] serial: 8250_em: Drop slab.h

[Biju Das]

This patch removes the unused header file slab.h.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4:
 * Split from patch#1
 * Removed the unused header file slab.h.
---
 drivers/tty/serial/8250/8250_em.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
index 785153d8502d..7bbf206ff476 100644
--- a/drivers/tty/serial/8250/8250_em.c
+++ b/drivers/tty/serial/8250/8250_em.c
@@ -13,7 +13,6 @@
 #include <linux/serial_reg.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
-#include <linux/slab.h>
 
 #include "8250.h"
 
-- 
2.25.1

[PATCH v4 3/6] serial: 8250_em: Use devm_clk_get_enabled()

[Biju Das]

This patch simplifies clk handling in probe()/remove() by replacing
devm_clk_get()->devm_clk_get_enabled().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4:
 * New patch. Split from patch#1.
 * Removed sclk from struct serial8250_em_priv instead used local
   variable in probe().
 * Replaced devm_clk_get->devm_clk_get_enabled() and removed
   clk_prepare_enable/clk_disable_unprepare from probe()/remove().
---
 drivers/tty/serial/8250/8250_em.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
index 7bbf206ff476..9781bf73ed0c 100644
--- a/drivers/tty/serial/8250/8250_em.c
+++ b/drivers/tty/serial/8250/8250_em.c
@@ -20,7 +20,6 @@
 #define UART_DLM_EM 10
 
 struct serial8250_em_priv {
-	struct clk *sclk;
 	int line;
 };
 
@@ -81,6 +80,7 @@ static int serial8250_em_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct uart_8250_port up;
 	struct resource *regs;
+	struct clk *sclk;
 	int irq, ret;
 
 	irq = platform_get_irq(pdev, 0);
@@ -95,9 +95,9 @@ static int serial8250_em_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
-	priv->sclk = devm_clk_get(dev, "sclk");
-	if (IS_ERR(priv->sclk))
-		return dev_err_probe(dev, PTR_ERR(priv->sclk), "unable to get clock\n");
+	sclk = devm_clk_get_enabled(dev, "sclk");
+	if (IS_ERR(sclk))
+		return dev_err_probe(dev, PTR_ERR(sclk), "unable to get clock\n");
 
 	memset(&up, 0, sizeof(up));
 	up.port.mapbase = regs->start;
@@ -107,8 +107,7 @@ static int serial8250_em_probe(struct platform_device *pdev)
 	up.port.dev = dev;
 	up.port.private_data = priv;
 
-	clk_prepare_enable(priv->sclk);
-	up.port.uartclk = clk_get_rate(priv->sclk);
+	up.port.uartclk = clk_get_rate(sclk);
 
 	up.port.iotype = UPIO_MEM32;
 	up.port.serial_in = serial8250_em_serial_in;
@@ -117,10 +116,8 @@ static int serial8250_em_probe(struct platform_device *pdev)
 	up.dl_write = serial8250_em_serial_dl_write;
 
 	ret = serial8250_register_8250_port(&up);
-	if (ret < 0) {
-		clk_disable_unprepare(priv->sclk);
+	if (ret < 0)
 		return dev_err_probe(dev, ret, "unable to register 8250 port\n");
-	}
 
 	priv->line = ret;
 	platform_set_drvdata(pdev, priv);
@@ -132,7 +129,6 @@ static int serial8250_em_remove(struct platform_device *pdev)
 	struct serial8250_em_priv *priv = platform_get_drvdata(pdev);
 
 	serial8250_unregister_port(priv->line);
-	clk_disable_unprepare(priv->sclk);
 	return 0;
 }
 
-- 
2.25.1

[PATCH v4 4/6] serial: 8250_em: Update port type as PORT_16750

[Biju Das]

The UART IP found on {RZ/V2M, EMMA mobile} SoC is Register-compatible
with the general-purpose 16750 UART chip. This patch updates port type
from 16550A->16750 and also enables 64-bytes fifo support.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3->v4:
 * Both {RZ/V2M, EMMA mobile} SoC is Register-compatible
   with the general-purpose 16750 UART chip. So started using
   generic compatible and removed struct serial8250_em_hw_info.
 * Removed Rb tag from Ilpo as it is new change.
v2->v3:
 * Replaced of_device_get_match_data()->device_get_match_data().
 * Replaced of_device.h->property.h
 * Dropped struct serial8250_em_hw_info *info from priv and started
   using a local variable info in probe().
 * Retained Rb tag from Ilpo as changes are trivial.
v2:
 * New patch
---
 drivers/tty/serial/8250/8250_em.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
index 9781bf73ed0c..499d7a8847ec 100644
--- a/drivers/tty/serial/8250/8250_em.c
+++ b/drivers/tty/serial/8250/8250_em.c
@@ -102,8 +102,8 @@ static int serial8250_em_probe(struct platform_device *pdev)
 	memset(&up, 0, sizeof(up));
 	up.port.mapbase = regs->start;
 	up.port.irq = irq;
-	up.port.type = PORT_UNKNOWN;
-	up.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP;
+	up.port.type = PORT_16750;
+	up.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP | UPF_FIXED_TYPE;
 	up.port.dev = dev;
 	up.port.private_data = priv;
 
-- 
2.25.1

[PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR

[Biju Das]

UART_FCR shares the same offset with UART_IIR. We cannot use
UART_FCR in serial8250_em_serial_in() as it overlaps with
UART_IIR.

This patch introduces a macro UART_FCR_EM with a high value to
avoid overlapping with existing UART_* register defines.

This will help us to use UART_FCR_EM consistently in serial8250_em_
serial{_in/_out} function to read/write FCR register.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v4:
 * New patch. Used UART_FCR_EM for read/write of FCR register.
---
 drivers/tty/serial/8250/8250_em.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
index 499d7a8847ec..4165fd3bb17a 100644
--- a/drivers/tty/serial/8250/8250_em.c
+++ b/drivers/tty/serial/8250/8250_em.c
@@ -19,6 +19,13 @@
 #define UART_DLL_EM 9
 #define UART_DLM_EM 10
 
+/*
+ * A high value for UART_FCR_EM avoids overlapping with existing UART_*
+ * register defines. UART_FCR_EM_HW is the real HW register offset.
+ */
+#define UART_FCR_EM 0x10003
+#define UART_FCR_EM_HW 3
+
 struct serial8250_em_priv {
 	int line;
 };
@@ -29,12 +36,14 @@ static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
 	case UART_TX: /* TX @ 0x00 */
 		writeb(value, p->membase);
 		break;
-	case UART_FCR: /* FCR @ 0x0c (+1) */
 	case UART_LCR: /* LCR @ 0x10 (+1) */
 	case UART_MCR: /* MCR @ 0x14 (+1) */
 	case UART_SCR: /* SCR @ 0x20 (+1) */
 		writel(value, p->membase + ((offset + 1) << 2));
 		break;
+	case UART_FCR_EM:
+		writel(value, p->membase + (UART_FCR_EM_HW << 2));
+		break;
 	case UART_IER: /* IER @ 0x04 */
 		value &= 0x0f; /* only 4 valid bits - not Xscale */
 		fallthrough;
@@ -54,6 +63,8 @@ static unsigned int serial8250_em_serial_in(struct uart_port *p, int offset)
 	case UART_MSR: /* MSR @ 0x1c (+1) */
 	case UART_SCR: /* SCR @ 0x20 (+1) */
 		return readl(p->membase + ((offset + 1) << 2));
+	case UART_FCR_EM:
+		return readl(p->membase + (UART_FCR_EM_HW << 2));
 	case UART_IER: /* IER @ 0x04 */
 	case UART_IIR: /* IIR @ 0x08 */
 	case UART_DLL_EM: /* DLL @ 0x24 (+9) */
-- 
2.25.1

[PATCH v4 6/6] serial: 8250_em: Add serial8250_em_{reg_update(),out_helper()}

[Biju Das]

As per RZ/V2M hardware manual(Rev.1.30 Jun, 2022), UART IP has a
restriction as mentioned below.

40.6.1 Point for Caution when Changing the Register Settings:

When changing the settings of the following registers, a PRESETn master
reset or FIFO reset + SW reset (FCR[2],FCR[1], HCR0[7]) must be input to
re-initialize them.

Target Registers: FCR, LCR, MCR, DLL, DLM, HCR0.

This patch adds serial8250_em_reg_update() and serial8250_em_serial_
out_helper to handle it.

DLL/DLM register can be updated only by setting LCR[7]. So the
updation of LCR[7] will perform reset for DLL/DLM register changes.

EMMA mobile has the same register set as RZ/V2M and this patch is tested on
EMEV2 board. So, there is no harm in applying the same restriction here as
well as the HW manual for EMMA mobile is not updated for a long time.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3->v4:
 * Updated restriction to EMMA mobile SoC as well.
 * Replaced readl->serial8250_em_serial_in() for reading fcr register
   in serial8250_em_reg_update().
 * Added serial8250_em_serial_out_helper() to simplify the
   code and used for register writes in serial8250_em_reg_update().
v2->v3:
 * Replaced readl/writel()->serial8250_em_serial_in/out() in serial8250_rzv2m_
   reg_update() to avoid duplication of offset trickery.
 * Added support for HCR0 read/write in serial8250_em_{serial_in, serial_out}
 * Added UART_LCR macro support in serial8250_em_serial_in() to read LCR
 * Reordered serial8250_em_{serial_in, serial_out} above serial8250_rzv2m_
   reg_update().
 * Replaced priv->info->serial_out to info->serial_out.
v1->v2:
 * Added serial_out to struct serial8250_em_hw_info
---
 drivers/tty/serial/8250/8250_em.c | 60 ++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
index 4165fd3bb17a..19b86853874a 100644
--- a/drivers/tty/serial/8250/8250_em.c
+++ b/drivers/tty/serial/8250/8250_em.c
@@ -18,6 +18,7 @@
 
 #define UART_DLL_EM 9
 #define UART_DLM_EM 10
+#define UART_HCR0_EM 11
 
 /*
  * A high value for UART_FCR_EM avoids overlapping with existing UART_*
@@ -26,11 +27,13 @@
 #define UART_FCR_EM 0x10003
 #define UART_FCR_EM_HW 3
 
+#define UART_HCR0_EM_SW_RESET	BIT(7) /* SW Reset */
+
 struct serial8250_em_priv {
 	int line;
 };
 
-static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
+static void serial8250_em_serial_out_helper(struct uart_port *p, int offset, int value)
 {
 	switch (offset) {
 	case UART_TX: /* TX @ 0x00 */
@@ -49,6 +52,7 @@ static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
 		fallthrough;
 	case UART_DLL_EM: /* DLL @ 0x24 (+9) */
 	case UART_DLM_EM: /* DLM @ 0x28 (+9) */
+	case UART_HCR0_EM: /* HCR0 @ 0x2c */
 		writel(value, p->membase + (offset << 2));
 	}
 }
@@ -58,6 +62,7 @@ static unsigned int serial8250_em_serial_in(struct uart_port *p, int offset)
 	switch (offset) {
 	case UART_RX: /* RX @ 0x00 */
 		return readb(p->membase);
+	case UART_LCR: /* LCR @ 0x10 (+1) */
 	case UART_MCR: /* MCR @ 0x14 (+1) */
 	case UART_LSR: /* LSR @ 0x18 (+1) */
 	case UART_MSR: /* MSR @ 0x1c (+1) */
@@ -69,11 +74,64 @@ static unsigned int serial8250_em_serial_in(struct uart_port *p, int offset)
 	case UART_IIR: /* IIR @ 0x08 */
 	case UART_DLL_EM: /* DLL @ 0x24 (+9) */
 	case UART_DLM_EM: /* DLM @ 0x28 (+9) */
+	case UART_HCR0_EM: /* HCR0 @ 0x2c */
 		return readl(p->membase + (offset << 2));
 	}
 	return 0;
 }
 
+static void serial8250_em_reg_update(struct uart_port *p, int off, int value)
+{
+	unsigned int ier, fcr, lcr, mcr, hcr0;
+
+	ier = serial8250_em_serial_in(p, UART_IER);
+	fcr = serial8250_em_serial_in(p, UART_FCR_EM);
+	lcr = serial8250_em_serial_in(p, UART_LCR);
+	mcr = serial8250_em_serial_in(p, UART_MCR);
+	hcr0 = serial8250_em_serial_in(p, UART_HCR0_EM);
+
+	serial8250_em_serial_out_helper(p, UART_FCR_EM, fcr | UART_FCR_CLEAR_RCVR |
+				 UART_FCR_CLEAR_XMIT);
+	serial8250_em_serial_out_helper(p, UART_HCR0_EM, hcr0 | UART_HCR0_EM_SW_RESET);
+	serial8250_em_serial_out_helper(p, UART_HCR0_EM, hcr0 & ~UART_HCR0_EM_SW_RESET);
+
+	switch (off) {
+	case UART_FCR_EM:
+		fcr = value;
+		break;
+	case UART_LCR:
+		lcr = value;
+		break;
+	case UART_MCR:
+		mcr = value;
+	}
+
+	serial8250_em_serial_out_helper(p, UART_IER, ier);
+	serial8250_em_serial_out_helper(p, UART_FCR_EM, fcr);
+	serial8250_em_serial_out_helper(p, UART_MCR, mcr);
+	serial8250_em_serial_out_helper(p, UART_LCR, lcr);
+	serial8250_em_serial_out_helper(p, UART_HCR0_EM, hcr0);
+}
+
+static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
+{
+	switch (offset) {
+	case UART_TX:
+	case UART_SCR:
+	case UART_IER:
+	case UART_DLL_EM:
+	case UART_DLM_EM:
+		serial8250_em_serial_out_helper(p, offset, value);
+		break;
+	case UART_FCR:
+		serial8250_em_reg_update(p, UART_FCR_EM, value);
+		break;
+	case UART_LCR:
+	case UART_MCR:
+		serial8250_em_reg_update(p, offset, value);
+	}
+}
+
 static int serial8250_em_serial_dl_read(struct uart_8250_port *up)
 {
 	return serial_in(up, UART_DLL_EM) | serial_in(up, UART_DLM_EM) << 8;
-- 
2.25.1

Re: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR

[Ilpo Järvinen]

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

On Fri, 17 Feb 2023, Biju Das wrote:

> UART_FCR shares the same offset with UART_IIR. We cannot use
> UART_FCR in serial8250_em_serial_in() as it overlaps with
> UART_IIR.
> 
> This patch introduces a macro UART_FCR_EM with a high value to
> avoid overlapping with existing UART_* register defines.
> 
> This will help us to use UART_FCR_EM consistently in serial8250_em_
> serial{_in/_out} function to read/write FCR register.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> v4:
>  * New patch. Used UART_FCR_EM for read/write of FCR register.
> ---
>  drivers/tty/serial/8250/8250_em.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
> index 499d7a8847ec..4165fd3bb17a 100644
> --- a/drivers/tty/serial/8250/8250_em.c
> +++ b/drivers/tty/serial/8250/8250_em.c
> @@ -19,6 +19,13 @@
>  #define UART_DLL_EM 9
>  #define UART_DLM_EM 10
>  
> +/*
> + * A high value for UART_FCR_EM avoids overlapping with existing UART_*
> + * register defines. UART_FCR_EM_HW is the real HW register offset.
> + */
> +#define UART_FCR_EM 0x10003
> +#define UART_FCR_EM_HW 3
> +
>  struct serial8250_em_priv {
>  	int line;
>  };
> @@ -29,12 +36,14 @@ static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
>  	case UART_TX: /* TX @ 0x00 */
>  		writeb(value, p->membase);
>  		break;
> -	case UART_FCR: /* FCR @ 0x0c (+1) */

8250_port wants this to remain in place, I think. Otherwise it's attempts 
to set UART_FCR will end up into wrong destination.

-- 
 i.


>  	case UART_LCR: /* LCR @ 0x10 (+1) */
>  	case UART_MCR: /* MCR @ 0x14 (+1) */
>  	case UART_SCR: /* SCR @ 0x20 (+1) */
>  		writel(value, p->membase + ((offset + 1) << 2));
>  		break;
> +	case UART_FCR_EM:
> +		writel(value, p->membase + (UART_FCR_EM_HW << 2));
> +		break;
>  	case UART_IER: /* IER @ 0x04 */
>  		value &= 0x0f; /* only 4 valid bits - not Xscale */
>  		fallthrough;
> @@ -54,6 +63,8 @@ static unsigned int serial8250_em_serial_in(struct uart_port *p, int offset)
>  	case UART_MSR: /* MSR @ 0x1c (+1) */
>  	case UART_SCR: /* SCR @ 0x20 (+1) */
>  		return readl(p->membase + ((offset + 1) << 2));
> +	case UART_FCR_EM:
> +		return readl(p->membase + (UART_FCR_EM_HW << 2));
>  	case UART_IER: /* IER @ 0x04 */
>  	case UART_IIR: /* IIR @ 0x08 */
>  	case UART_DLL_EM: /* DLL @ 0x24 (+9) */
> 

RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR

[Biju Das]

Hi Ilpo,

Thanks for the feedback.

> Subject: Re: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR
> 
> On Fri, 17 Feb 2023, Biju Das wrote:
> 
> > UART_FCR shares the same offset with UART_IIR. We cannot use UART_FCR
> > in serial8250_em_serial_in() as it overlaps with UART_IIR.
> >
> > This patch introduces a macro UART_FCR_EM with a high value to avoid
> > overlapping with existing UART_* register defines.
> >
> > This will help us to use UART_FCR_EM consistently in serial8250_em_
> > serial{_in/_out} function to read/write FCR register.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> > v4:
> >  * New patch. Used UART_FCR_EM for read/write of FCR register.
> > ---
> >  drivers/tty/serial/8250/8250_em.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_em.c
> > b/drivers/tty/serial/8250/8250_em.c
> > index 499d7a8847ec..4165fd3bb17a 100644
> > --- a/drivers/tty/serial/8250/8250_em.c
> > +++ b/drivers/tty/serial/8250/8250_em.c
> > @@ -19,6 +19,13 @@
> >  #define UART_DLL_EM 9
> >  #define UART_DLM_EM 10
> >
> > +/*
> > + * A high value for UART_FCR_EM avoids overlapping with existing
> > +UART_*
> > + * register defines. UART_FCR_EM_HW is the real HW register offset.
> > + */
> > +#define UART_FCR_EM 0x10003
> > +#define UART_FCR_EM_HW 3
> > +
> >  struct serial8250_em_priv {
> >  	int line;
> >  };
> > @@ -29,12 +36,14 @@ static void serial8250_em_serial_out(struct uart_port
> *p, int offset, int value)
> >  	case UART_TX: /* TX @ 0x00 */
> >  		writeb(value, p->membase);
> >  		break;
> > -	case UART_FCR: /* FCR @ 0x0c (+1) */
> 
> 8250_port wants this to remain in place, I think. Otherwise it's attempts to
> set UART_FCR will end up into wrong destination.

Oops, next patch has this change.

+	case UART_FCR:
+		serial8250_em_reg_update(p, UART_FCR_EM, value);

I just need to keep UART_FCR for this patch and 
remove it from "serial8250_em_serial_out_helper" on the next patch

Cheers,
Biju


> 
> --
>  i.
> 
> 
> >  	case UART_LCR: /* LCR @ 0x10 (+1) */
> >  	case UART_MCR: /* MCR @ 0x14 (+1) */
> >  	case UART_SCR: /* SCR @ 0x20 (+1) */
> >  		writel(value, p->membase + ((offset + 1) << 2));
> >  		break;
> > +	case UART_FCR_EM:
> > +		writel(value, p->membase + (UART_FCR_EM_HW << 2));
> > +		break;
> >  	case UART_IER: /* IER @ 0x04 */
> >  		value &= 0x0f; /* only 4 valid bits - not Xscale */
> >  		fallthrough;
> > @@ -54,6 +63,8 @@ static unsigned int serial8250_em_serial_in(struct
> uart_port *p, int offset)
> >  	case UART_MSR: /* MSR @ 0x1c (+1) */
> >  	case UART_SCR: /* SCR @ 0x20 (+1) */
> >  		return readl(p->membase + ((offset + 1) << 2));
> > +	case UART_FCR_EM:
> > +		return readl(p->membase + (UART_FCR_EM_HW << 2));
> >  	case UART_IER: /* IER @ 0x04 */
> >  	case UART_IIR: /* IIR @ 0x08 */
> >  	case UART_DLL_EM: /* DLL @ 0x24 (+9) */
> >

RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR

[Ilpo Järvinen]

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

On Fri, 17 Feb 2023, Biju Das wrote:

> Hi Ilpo,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR
> > 
> > On Fri, 17 Feb 2023, Biju Das wrote:
> > 
> > > UART_FCR shares the same offset with UART_IIR. We cannot use UART_FCR
> > > in serial8250_em_serial_in() as it overlaps with UART_IIR.
> > >
> > > This patch introduces a macro UART_FCR_EM with a high value to avoid
> > > overlapping with existing UART_* register defines.
> > >
> > > This will help us to use UART_FCR_EM consistently in serial8250_em_
> > > serial{_in/_out} function to read/write FCR register.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > ---
> > > v4:
> > >  * New patch. Used UART_FCR_EM for read/write of FCR register.
> > > ---
> > >  drivers/tty/serial/8250/8250_em.c | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_em.c
> > > b/drivers/tty/serial/8250/8250_em.c
> > > index 499d7a8847ec..4165fd3bb17a 100644
> > > --- a/drivers/tty/serial/8250/8250_em.c
> > > +++ b/drivers/tty/serial/8250/8250_em.c
> > > @@ -19,6 +19,13 @@
> > >  #define UART_DLL_EM 9
> > >  #define UART_DLM_EM 10
> > >
> > > +/*
> > > + * A high value for UART_FCR_EM avoids overlapping with existing
> > > +UART_*
> > > + * register defines. UART_FCR_EM_HW is the real HW register offset.
> > > + */
> > > +#define UART_FCR_EM 0x10003
> > > +#define UART_FCR_EM_HW 3
> > > +
> > >  struct serial8250_em_priv {
> > >  	int line;
> > >  };
> > > @@ -29,12 +36,14 @@ static void serial8250_em_serial_out(struct uart_port
> > *p, int offset, int value)
> > >  	case UART_TX: /* TX @ 0x00 */
> > >  		writeb(value, p->membase);
> > >  		break;
> > > -	case UART_FCR: /* FCR @ 0x0c (+1) */
> > 
> > 8250_port wants this to remain in place, I think. Otherwise it's attempts to
> > set UART_FCR will end up into wrong destination.
> 
> Oops, next patch has this change.
> 
> +	case UART_FCR:
> +		serial8250_em_reg_update(p, UART_FCR_EM, value);
> 
> I just need to keep UART_FCR for this patch and 
> remove it from "serial8250_em_serial_out_helper" on the next patch

IMHO, the extra layer + _helper seems pretty unnecessary. I don't see any 
useful thing it achieves over just having to serial_out functions.

-- 
 i.

RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR

[Biju Das]

HI Ilpo,

> Subject: RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR
> 
> On Fri, 17 Feb 2023, Biju Das wrote:
> 
> > Hi Ilpo,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for
> > > UART_FCR
> > >
> > > On Fri, 17 Feb 2023, Biju Das wrote:
> > >
> > > > UART_FCR shares the same offset with UART_IIR. We cannot use
> > > > UART_FCR in serial8250_em_serial_in() as it overlaps with UART_IIR.
> > > >
> > > > This patch introduces a macro UART_FCR_EM with a high value to
> > > > avoid overlapping with existing UART_* register defines.
> > > >
> > > > This will help us to use UART_FCR_EM consistently in
> > > > serial8250_em_ serial{_in/_out} function to read/write FCR register.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > ---
> > > > v4:
> > > >  * New patch. Used UART_FCR_EM for read/write of FCR register.
> > > > ---
> > > >  drivers/tty/serial/8250/8250_em.c | 13 ++++++++++++-
> > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/tty/serial/8250/8250_em.c
> > > > b/drivers/tty/serial/8250/8250_em.c
> > > > index 499d7a8847ec..4165fd3bb17a 100644
> > > > --- a/drivers/tty/serial/8250/8250_em.c
> > > > +++ b/drivers/tty/serial/8250/8250_em.c
> > > > @@ -19,6 +19,13 @@
> > > >  #define UART_DLL_EM 9
> > > >  #define UART_DLM_EM 10
> > > >
> > > > +/*
> > > > + * A high value for UART_FCR_EM avoids overlapping with existing
> > > > +UART_*
> > > > + * register defines. UART_FCR_EM_HW is the real HW register offset.
> > > > + */
> > > > +#define UART_FCR_EM 0x10003
> > > > +#define UART_FCR_EM_HW 3
> > > > +
> > > >  struct serial8250_em_priv {
> > > >  	int line;
> > > >  };
> > > > @@ -29,12 +36,14 @@ static void serial8250_em_serial_out(struct
> > > > uart_port
> > > *p, int offset, int value)
> > > >  	case UART_TX: /* TX @ 0x00 */
> > > >  		writeb(value, p->membase);
> > > >  		break;
> > > > -	case UART_FCR: /* FCR @ 0x0c (+1) */
> > >
> > > 8250_port wants this to remain in place, I think. Otherwise it's
> > > attempts to set UART_FCR will end up into wrong destination.
> >
> > Oops, next patch has this change.
> >
> > +	case UART_FCR:
> > +		serial8250_em_reg_update(p, UART_FCR_EM, value);
> >
> > I just need to keep UART_FCR for this patch and remove it from
> > "serial8250_em_serial_out_helper" on the next patch
> 
> IMHO, the extra layer + _helper seems pretty unnecessary. I don't see any
> useful thing it achieves over just having to serial_out functions.

Without helper, it will become cyclic right??

serial8250_em_serial_out() needs to call serial8250_em_reg_update()

serial8250_em_reg_update() will call serial8250_em_serial_out() for writes??

Cheers,
Biju

RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR

[Ilpo Järvinen]

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

On Fri, 17 Feb 2023, Biju Das wrote:

> HI Ilpo,
> 
> > Subject: RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR
> > 
> > On Fri, 17 Feb 2023, Biju Das wrote:
> > 
> > > Hi Ilpo,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for
> > > > UART_FCR
> > > >
> > > > On Fri, 17 Feb 2023, Biju Das wrote:
> > > >
> > > > > UART_FCR shares the same offset with UART_IIR. We cannot use
> > > > > UART_FCR in serial8250_em_serial_in() as it overlaps with UART_IIR.
> > > > >
> > > > > This patch introduces a macro UART_FCR_EM with a high value to
> > > > > avoid overlapping with existing UART_* register defines.
> > > > >
> > > > > This will help us to use UART_FCR_EM consistently in
> > > > > serial8250_em_ serial{_in/_out} function to read/write FCR register.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > > ---
> > > > > v4:
> > > > >  * New patch. Used UART_FCR_EM for read/write of FCR register.
> > > > > ---
> > > > >  drivers/tty/serial/8250/8250_em.c | 13 ++++++++++++-
> > > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/tty/serial/8250/8250_em.c
> > > > > b/drivers/tty/serial/8250/8250_em.c
> > > > > index 499d7a8847ec..4165fd3bb17a 100644
> > > > > --- a/drivers/tty/serial/8250/8250_em.c
> > > > > +++ b/drivers/tty/serial/8250/8250_em.c
> > > > > @@ -19,6 +19,13 @@
> > > > >  #define UART_DLL_EM 9
> > > > >  #define UART_DLM_EM 10
> > > > >
> > > > > +/*
> > > > > + * A high value for UART_FCR_EM avoids overlapping with existing
> > > > > +UART_*
> > > > > + * register defines. UART_FCR_EM_HW is the real HW register offset.
> > > > > + */
> > > > > +#define UART_FCR_EM 0x10003
> > > > > +#define UART_FCR_EM_HW 3
> > > > > +
> > > > >  struct serial8250_em_priv {
> > > > >  	int line;
> > > > >  };
> > > > > @@ -29,12 +36,14 @@ static void serial8250_em_serial_out(struct
> > > > > uart_port
> > > > *p, int offset, int value)
> > > > >  	case UART_TX: /* TX @ 0x00 */
> > > > >  		writeb(value, p->membase);
> > > > >  		break;
> > > > > -	case UART_FCR: /* FCR @ 0x0c (+1) */
> > > >
> > > > 8250_port wants this to remain in place, I think. Otherwise it's
> > > > attempts to set UART_FCR will end up into wrong destination.
> > >
> > > Oops, next patch has this change.
> > >
> > > +	case UART_FCR:
> > > +		serial8250_em_reg_update(p, UART_FCR_EM, value);
> > >
> > > I just need to keep UART_FCR for this patch and remove it from
> > > "serial8250_em_serial_out_helper" on the next patch
> > 
> > IMHO, the extra layer + _helper seems pretty unnecessary. I don't see any
> > useful thing it achieves over just having to serial_out functions.
> 
> Without helper, it will become cyclic right??
> 
> serial8250_em_serial_out() needs to call serial8250_em_reg_update()
> 
> serial8250_em_reg_update() will call serial8250_em_serial_out() for writes??

With your most recent code, yes it seems to happen. But why did you remove 
the separate serial_out for RZ. There wasn't any cyclicness when it 
called serial8250_em_serial_out().

I'm a bit lost now, are you saying that this change is needed for all 
devices 8250_em supports (which is different from what you initially 
presented in the early versions of this patchset)?

-- 
 i.

RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR

[Biju Das]

Hi Ilpo,

Thanks for the feedback.

> Subject: RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR
> 
> On Fri, 17 Feb 2023, Biju Das wrote:
> 
> > HI Ilpo,
> >
> > > Subject: RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for
> > > UART_FCR
> > >
> > > On Fri, 17 Feb 2023, Biju Das wrote:
> > >
> > > > Hi Ilpo,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: Re: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset
> > > > > for UART_FCR
> > > > >
> > > > > On Fri, 17 Feb 2023, Biju Das wrote:
> > > > >
> > > > > > UART_FCR shares the same offset with UART_IIR. We cannot use
> > > > > > UART_FCR in serial8250_em_serial_in() as it overlaps with
> UART_IIR.
> > > > > >
> > > > > > This patch introduces a macro UART_FCR_EM with a high value to
> > > > > > avoid overlapping with existing UART_* register defines.
> > > > > >
> > > > > > This will help us to use UART_FCR_EM consistently in
> > > > > > serial8250_em_ serial{_in/_out} function to read/write FCR
> register.
> > > > > >
> > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > > > ---
> > > > > > v4:
> > > > > >  * New patch. Used UART_FCR_EM for read/write of FCR register.
> > > > > > ---
> > > > > >  drivers/tty/serial/8250/8250_em.c | 13 ++++++++++++-
> > > > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/tty/serial/8250/8250_em.c
> > > > > > b/drivers/tty/serial/8250/8250_em.c
> > > > > > index 499d7a8847ec..4165fd3bb17a 100644
> > > > > > --- a/drivers/tty/serial/8250/8250_em.c
> > > > > > +++ b/drivers/tty/serial/8250/8250_em.c
> > > > > > @@ -19,6 +19,13 @@
> > > > > >  #define UART_DLL_EM 9
> > > > > >  #define UART_DLM_EM 10
> > > > > >
> > > > > > +/*
> > > > > > + * A high value for UART_FCR_EM avoids overlapping with
> > > > > > +existing
> > > > > > +UART_*
> > > > > > + * register defines. UART_FCR_EM_HW is the real HW register
> offset.
> > > > > > + */
> > > > > > +#define UART_FCR_EM 0x10003
> > > > > > +#define UART_FCR_EM_HW 3
> > > > > > +
> > > > > >  struct serial8250_em_priv {
> > > > > >  	int line;
> > > > > >  };
> > > > > > @@ -29,12 +36,14 @@ static void
> > > > > > serial8250_em_serial_out(struct uart_port
> > > > > *p, int offset, int value)
> > > > > >  	case UART_TX: /* TX @ 0x00 */
> > > > > >  		writeb(value, p->membase);
> > > > > >  		break;
> > > > > > -	case UART_FCR: /* FCR @ 0x0c (+1) */
> > > > >
> > > > > 8250_port wants this to remain in place, I think. Otherwise it's
> > > > > attempts to set UART_FCR will end up into wrong destination.
> > > >
> > > > Oops, next patch has this change.
> > > >
> > > > +	case UART_FCR:
> > > > +		serial8250_em_reg_update(p, UART_FCR_EM, value);
> > > >
> > > > I just need to keep UART_FCR for this patch and remove it from
> > > > "serial8250_em_serial_out_helper" on the next patch
> > >
> > > IMHO, the extra layer + _helper seems pretty unnecessary. I don't
> > > see any useful thing it achieves over just having to serial_out
> functions.
> >
> > Without helper, it will become cyclic right??
> >
> > serial8250_em_serial_out() needs to call serial8250_em_reg_update()
> >
> > serial8250_em_reg_update() will call serial8250_em_serial_out() for
> writes??
> 
> With your most recent code, yes it seems to happen. But why did you remove
> the separate serial_out for RZ. There wasn't any cyclicness when it called
> serial8250_em_serial_out().
> 
> I'm a bit lost now, are you saying that this change is needed for all
> devices 8250_em supports (which is different from what you initially
> presented in the early versions of this patchset)?

Yes, That is correct. Please see the discussion thread related to this[1].

Geert pointed out that UART register sets between RZ/V2M and
EMMA mobile SoC are almost identical.

"According to R19UH0040EJ0400 Rev.4.00 it is available on EMEV2, and the
layout looks identical to RZ/V2M."

Niklas tested previous patch series on EMEV2 board and 
It detected port type as 16750 and done read/write test
with 64-bytes fifo enabled.

EMMA mobile SoC is very old and hardware manual
is not updated for long time related.

So we guess it is safe to apply this restriction for this SoC
aswell, since there is no regression.

[1]
https://lore.kernel.org/linux-renesas-soc/8585f736-cf3b-b63c-753f-892d4051ada3@linux.intel.com/T/#mf5e3059e792ab1a5dd96a769b9c69ae6befb25c5

Cheers,
Biju

RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR

[Ilpo Järvinen]

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

On Fri, 17 Feb 2023, Biju Das wrote:

> Hi Ilpo,
> 
> Thanks for the feedback.
> 
> > Subject: RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR
> > 
> > On Fri, 17 Feb 2023, Biju Das wrote:
> > 
> > > HI Ilpo,
> > >
> > > > Subject: RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for
> > > > UART_FCR
> > > >
> > > > On Fri, 17 Feb 2023, Biju Das wrote:
> > > >
> > > > > Hi Ilpo,
> > > > >
> > > > > Thanks for the feedback.
> > > > >
> > > > > > Subject: Re: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset
> > > > > > for UART_FCR
> > > > > >
> > > > > > On Fri, 17 Feb 2023, Biju Das wrote:
> > > > > >
> > > > > > > UART_FCR shares the same offset with UART_IIR. We cannot use
> > > > > > > UART_FCR in serial8250_em_serial_in() as it overlaps with
> > UART_IIR.
> > > > > > >
> > > > > > > This patch introduces a macro UART_FCR_EM with a high value to
> > > > > > > avoid overlapping with existing UART_* register defines.
> > > > > > >
> > > > > > > This will help us to use UART_FCR_EM consistently in
> > > > > > > serial8250_em_ serial{_in/_out} function to read/write FCR
> > register.
> > > > > > >
> > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > > > > ---
> > > > > > > v4:
> > > > > > >  * New patch. Used UART_FCR_EM for read/write of FCR register.
> > > > > > > ---
> > > > > > >  drivers/tty/serial/8250/8250_em.c | 13 ++++++++++++-
> > > > > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/tty/serial/8250/8250_em.c
> > > > > > > b/drivers/tty/serial/8250/8250_em.c
> > > > > > > index 499d7a8847ec..4165fd3bb17a 100644
> > > > > > > --- a/drivers/tty/serial/8250/8250_em.c
> > > > > > > +++ b/drivers/tty/serial/8250/8250_em.c
> > > > > > > @@ -19,6 +19,13 @@
> > > > > > >  #define UART_DLL_EM 9
> > > > > > >  #define UART_DLM_EM 10
> > > > > > >
> > > > > > > +/*
> > > > > > > + * A high value for UART_FCR_EM avoids overlapping with
> > > > > > > +existing
> > > > > > > +UART_*
> > > > > > > + * register defines. UART_FCR_EM_HW is the real HW register
> > offset.
> > > > > > > + */
> > > > > > > +#define UART_FCR_EM 0x10003
> > > > > > > +#define UART_FCR_EM_HW 3
> > > > > > > +
> > > > > > >  struct serial8250_em_priv {
> > > > > > >  	int line;
> > > > > > >  };
> > > > > > > @@ -29,12 +36,14 @@ static void
> > > > > > > serial8250_em_serial_out(struct uart_port
> > > > > > *p, int offset, int value)
> > > > > > >  	case UART_TX: /* TX @ 0x00 */
> > > > > > >  		writeb(value, p->membase);
> > > > > > >  		break;
> > > > > > > -	case UART_FCR: /* FCR @ 0x0c (+1) */
> > > > > >
> > > > > > 8250_port wants this to remain in place, I think. Otherwise it's
> > > > > > attempts to set UART_FCR will end up into wrong destination.
> > > > >
> > > > > Oops, next patch has this change.
> > > > >
> > > > > +	case UART_FCR:
> > > > > +		serial8250_em_reg_update(p, UART_FCR_EM, value);
> > > > >
> > > > > I just need to keep UART_FCR for this patch and remove it from
> > > > > "serial8250_em_serial_out_helper" on the next patch
> > > >
> > > > IMHO, the extra layer + _helper seems pretty unnecessary. I don't
> > > > see any useful thing it achieves over just having to serial_out
> > functions.
> > >
> > > Without helper, it will become cyclic right??
> > >
> > > serial8250_em_serial_out() needs to call serial8250_em_reg_update()
> > >
> > > serial8250_em_reg_update() will call serial8250_em_serial_out() for
> > writes??
> > 
> > With your most recent code, yes it seems to happen. But why did you remove
> > the separate serial_out for RZ. There wasn't any cyclicness when it called
> > serial8250_em_serial_out().
> > 
> > I'm a bit lost now, are you saying that this change is needed for all
> > devices 8250_em supports (which is different from what you initially
> > presented in the early versions of this patchset)?
> 
> Yes, That is correct. Please see the discussion thread related to this[1].
> 
> Geert pointed out that UART register sets between RZ/V2M and
> EMMA mobile SoC are almost identical.
> 
> "According to R19UH0040EJ0400 Rev.4.00 it is available on EMEV2, and the
> layout looks identical to RZ/V2M."
> 
> Niklas tested previous patch series on EMEV2 board and 
> It detected port type as 16750 and done read/write test
> with 64-bytes fifo enabled.
> 
> EMMA mobile SoC is very old and hardware manual
> is not updated for long time related.
> 
> So we guess it is safe to apply this restriction for this SoC
> aswell, since there is no regression.

Okay, fair enough.


-- 
 i.

Re: [PATCH v4 1/6] serial: 8250_em: Use dev_err_probe()

[Andy Shevchenko]

On Fri, Feb 17, 2023 at 11:42:50AM +0000, Biju Das wrote:
> This patch simplifies probe() by using dev_err() instead
> of dev_err_probe()

The patch does the opposite AFAICS.

> and added a local variable 'dev' to
> replace '&pdev->dev'.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/6] serial: 8250_em: Drop slab.h

[Andy Shevchenko]

On Fri, Feb 17, 2023 at 11:42:51AM +0000, Biju Das wrote:
> This patch removes the unused header file slab.h.

Please, read Submitting Patches on how to make your commit messages better.

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/6] serial: 8250_em: Update port type as PORT_16750

[Andy Shevchenko]

On Fri, Feb 17, 2023 at 11:42:53AM +0000, Biju Das wrote:
> The UART IP found on {RZ/V2M, EMMA mobile} SoC is Register-compatible
> with the general-purpose 16750 UART chip. This patch updates port type
> from 16550A->16750 and also enables 64-bytes fifo support.

...

> +	up.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP | UPF_FIXED_TYPE;

Do you still need UPF_BOOT_AUTOCONF?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 6/6] serial: 8250_em: Add serial8250_em_{reg_update(),out_helper()}

[Andy Shevchenko]

On Fri, Feb 17, 2023 at 11:42:55AM +0000, Biju Das wrote:
> As per RZ/V2M hardware manual(Rev.1.30 Jun, 2022), UART IP has a
> restriction as mentioned below.
> 
> 40.6.1 Point for Caution when Changing the Register Settings:
> 
> When changing the settings of the following registers, a PRESETn master
> reset or FIFO reset + SW reset (FCR[2],FCR[1], HCR0[7]) must be input to
> re-initialize them.
> 
> Target Registers: FCR, LCR, MCR, DLL, DLM, HCR0.
> 
> This patch adds serial8250_em_reg_update() and serial8250_em_serial_
> out_helper to handle it.
> 
> DLL/DLM register can be updated only by setting LCR[7]. So the
> updation of LCR[7] will perform reset for DLL/DLM register changes.
> 
> EMMA mobile has the same register set as RZ/V2M and this patch is tested on
> EMEV2 board. So, there is no harm in applying the same restriction here as
> well as the HW manual for EMMA mobile is not updated for a long time.

...

> +	serial8250_em_serial_out_helper(p, UART_FCR_EM, fcr | UART_FCR_CLEAR_RCVR |
> +				 UART_FCR_CLEAR_XMIT);


I would put it like

	serial8250_em_serial_out_helper(p, UART_FCR_EM, fcr |
							UART_FCR_CLEAR_RCVR |
							UART_FCR_CLEAR_XMIT);

...

> +	switch (off) {
> +	case UART_FCR_EM:
> +		fcr = value;
> +		break;
> +	case UART_LCR:
> +		lcr = value;
> +		break;
> +	case UART_MCR:
> +		mcr = value;

Missing break; statement.

> +	}

...

> +	switch (offset) {
> +	case UART_TX:
> +	case UART_SCR:
> +	case UART_IER:
> +	case UART_DLL_EM:
> +	case UART_DLM_EM:
> +		serial8250_em_serial_out_helper(p, offset, value);
> +		break;
> +	case UART_FCR:
> +		serial8250_em_reg_update(p, UART_FCR_EM, value);
> +		break;
> +	case UART_LCR:
> +	case UART_MCR:
> +		serial8250_em_reg_update(p, offset, value);

Missing break; statement.

> +	}

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 1/6] serial: 8250_em: Use dev_err_probe()

[Biju Das]

Hi Andy,

Thanks for the feedback.

> Subject: Re: [PATCH v4 1/6] serial: 8250_em: Use dev_err_probe()
> 
> On Fri, Feb 17, 2023 at 11:42:50AM +0000, Biju Das wrote:
> > This patch simplifies probe() by using dev_err() instead of
> > dev_err_probe()
> 
> The patch does the opposite AFAICS.

OK, will fix it in next version.

Cheers,
Biju

> 
> > and added a local variable 'dev' to
> > replace '&pdev->dev'.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

RE: [PATCH v4 2/6] serial: 8250_em: Drop slab.h

[Biju Das]

Hi Andy,

> Subject: Re: [PATCH v4 2/6] serial: 8250_em: Drop slab.h
> 
> On Fri, Feb 17, 2023 at 11:42:51AM +0000, Biju Das wrote:
> > This patch removes the unused header file slab.h.
> 
> Please, read Submitting Patches on how to make your commit messages better.
> 

Thanks for the link.

I rechecked slab.h [1] and 8250_em driver is not using
any of api's or macros provided by that header file.

Please correct me if that is not the case.

serial: 8250_em:Drop unused header file

Drop unused header file slab.h from the driver.

Is it ok?

[1]
https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h

Cheers,
Biju

RE: [PATCH v4 6/6] serial: 8250_em: Add serial8250_em_{reg_update(),out_helper()}

[Biju Das]

Hi Andy,

Thanks for the feedback.

> Subject: Re: [PATCH v4 6/6] serial: 8250_em: Add
> serial8250_em_{reg_update(),out_helper()}
> 
> On Fri, Feb 17, 2023 at 11:42:55AM +0000, Biju Das wrote:
> > As per RZ/V2M hardware manual(Rev.1.30 Jun, 2022), UART IP has a
> > restriction as mentioned below.
> >
> > 40.6.1 Point for Caution when Changing the Register Settings:
> >
> > When changing the settings of the following registers, a PRESETn
> > master reset or FIFO reset + SW reset (FCR[2],FCR[1], HCR0[7]) must be
> > input to re-initialize them.
> >
> > Target Registers: FCR, LCR, MCR, DLL, DLM, HCR0.
> >
> > This patch adds serial8250_em_reg_update() and serial8250_em_serial_
> > out_helper to handle it.
> >
> > DLL/DLM register can be updated only by setting LCR[7]. So the
> > updation of LCR[7] will perform reset for DLL/DLM register changes.
> >
> > EMMA mobile has the same register set as RZ/V2M and this patch is
> > tested on
> > EMEV2 board. So, there is no harm in applying the same restriction
> > here as well as the HW manual for EMMA mobile is not updated for a long
> time.
> 
> ...
> 
> > +	serial8250_em_serial_out_helper(p, UART_FCR_EM, fcr |
> UART_FCR_CLEAR_RCVR |
> > +				 UART_FCR_CLEAR_XMIT);
> 
> 
> I would put it like
> 
> 	serial8250_em_serial_out_helper(p, UART_FCR_EM, fcr |
> 							UART_FCR_CLEAR_RCVR |
> 							UART_FCR_CLEAR_XMIT);
> 
> ...

OK.

> 
> > +	switch (off) {
> > +	case UART_FCR_EM:
> > +		fcr = value;
> > +		break;
> > +	case UART_LCR:
> > +		lcr = value;
> > +		break;
> > +	case UART_MCR:
> > +		mcr = value;
> 
> Missing break; statement.

OK, will fix this in next version.

The original driver has some missing breaks[1].
So for consistency I dropped this. same for below.

[1] https://elixir.bootlin.com/linux/v6.2-rc8/source/drivers/tty/serial/8250/8250_em.c#L45

Cheers,
Biju

> 
> > +	}
> 
> ...
> 
> > +	switch (offset) {
> > +	case UART_TX:
> > +	case UART_SCR:
> > +	case UART_IER:
> > +	case UART_DLL_EM:
> > +	case UART_DLM_EM:
> > +		serial8250_em_serial_out_helper(p, offset, value);
> > +		break;
> > +	case UART_FCR:
> > +		serial8250_em_reg_update(p, UART_FCR_EM, value);
> > +		break;
> > +	case UART_LCR:
> > +	case UART_MCR:
> > +		serial8250_em_reg_update(p, offset, value);
> 
> Missing break; statement.
> 
> > +	}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

RE: [PATCH v4 4/6] serial: 8250_em: Update port type as PORT_16750

[Biju Das]

Hi Andy,

Thanks for the feedback.

> Subject: Re: [PATCH v4 4/6] serial: 8250_em: Update port type as PORT_16750
> 
> On Fri, Feb 17, 2023 at 11:42:53AM +0000, Biju Das wrote:
> > The UART IP found on {RZ/V2M, EMMA mobile} SoC is Register-compatible
> > with the general-purpose 16750 UART chip. This patch updates port type
> > from 16550A->16750 and also enables 64-bytes fifo support.
> 
> ...
> 
> > +	up.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP |
> > +UPF_FIXED_TYPE;
> 
> Do you still need UPF_BOOT_AUTOCONF?

No, it is not needed. I will fix this in next version.

Cheers,
Biju

Re: [PATCH v4 2/6] serial: 8250_em: Drop slab.h

[Andy Shevchenko]

On Fri, Feb 17, 2023 at 02:30:34PM +0000, Biju Das wrote:
> > On Fri, Feb 17, 2023 at 11:42:51AM +0000, Biju Das wrote:
> > > This patch removes the unused header file slab.h.
> > 
> > Please, read Submitting Patches on how to make your commit messages better.
> 
> Thanks for the link.
> 
> I rechecked slab.h [1] and 8250_em driver is not using
> any of api's or macros provided by that header file.

Yes, that's not what I'm talking about.

> Please correct me if that is not the case.

The following text is okay

---8<----8<----

serial: 8250_em: Drop unused header file

Drop unused header file slab.h from the driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 6/6] serial: 8250_em: Add serial8250_em_{reg_update(),out_helper()}

[Andy Shevchenko]

On Fri, Feb 17, 2023 at 02:35:16PM +0000, Biju Das wrote:
> Hi Andy,
> > Subject: Re: [PATCH v4 6/6] serial: 8250_em: Add
> > serial8250_em_{reg_update(),out_helper()}
> > On Fri, Feb 17, 2023 at 11:42:55AM +0000, Biju Das wrote:

...

> > > +	switch (off) {
> > > +	case UART_FCR_EM:
> > > +		fcr = value;
> > > +		break;
> > > +	case UART_LCR:
> > > +		lcr = value;
> > > +		break;
> > > +	case UART_MCR:
> > > +		mcr = value;
> > 
> > Missing break; statement.
> 
> OK, will fix this in next version.
> 
> The original driver has some missing breaks[1].
> So for consistency I dropped this. same for below.

I see. Perhaps you can add another patch that adds them there first?

> [1] https://elixir.bootlin.com/linux/v6.2-rc8/source/drivers/tty/serial/8250/8250_em.c#L45

> > > +	}

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 2/6] serial: 8250_em: Drop slab.h

[Biju Das]

Hi Andy,

> Subject: Re: [PATCH v4 2/6] serial: 8250_em: Drop slab.h
> 
> On Fri, Feb 17, 2023 at 02:30:34PM +0000, Biju Das wrote:
> > > On Fri, Feb 17, 2023 at 11:42:51AM +0000, Biju Das wrote:
> > > > This patch removes the unused header file slab.h.
> > >
> > > Please, read Submitting Patches on how to make your commit messages
> better.
> >
> > Thanks for the link.
> >
> > I rechecked slab.h [1] and 8250_em driver is not using any of api's or
> > macros provided by that header file.
> 
> Yes, that's not what I'm talking about.
> 
> > Please correct me if that is not the case.
> 
> The following text is okay
> 
> ---8<----8<----
> 
> serial: 8250_em: Drop unused header file
> 
> Drop unused header file slab.h from the driver.

Thanks. I got the point "Describe your changes in imperative mood", 

I should not use "This patch" in commit messages which looks like
giving orders to the codebase to change its behaviour.

Cheers,
Biju

> 

RE: [PATCH v4 6/6] serial: 8250_em: Add serial8250_em_{reg_update(),out_helper()}

[Biju Das]

Hi Andy,

Thanks for the feedback.

> Subject: Re: [PATCH v4 6/6] serial: 8250_em: Add
> serial8250_em_{reg_update(),out_helper()}
> 
> On Fri, Feb 17, 2023 at 02:35:16PM +0000, Biju Das wrote:
> > Hi Andy,
> > > Subject: Re: [PATCH v4 6/6] serial: 8250_em: Add
> > > serial8250_em_{reg_update(),out_helper()}
> > > On Fri, Feb 17, 2023 at 11:42:55AM +0000, Biju Das wrote:
> 
> ...
> 
> > > > +	switch (off) {
> > > > +	case UART_FCR_EM:
> > > > +		fcr = value;
> > > > +		break;
> > > > +	case UART_LCR:
> > > > +		lcr = value;
> > > > +		break;
> > > > +	case UART_MCR:
> > > > +		mcr = value;
> > >
> > > Missing break; statement.
> >
> > OK, will fix this in next version.
> >
> > The original driver has some missing breaks[1].
> > So for consistency I dropped this. same for below.
> 
> I see. Perhaps you can add another patch that adds them there first?

OK, will add separate patch for adding them first.

Cheers,
Biju