[PATCH v2 0/3] pdate Renesas RZ/V2M UART Port type

[PATCH v2 0/3] pdate Renesas RZ/V2M UART Port type

[Biju Das]

The Renesas RZ/V2M UART compatible with the general-purpose 16750 UART chip.
This patch updates Renesas 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
RZ/V2M specific register handling for the below restriction mentioned in
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.

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 (3):
  serial: 8250_em: Use dev_err_probe()
  serial: 8250_em: Update RZ/V2M port type as PORT_16750
  serial: 8250_em: Add serial_out() to struct serial8250_em_hw_info

 drivers/tty/serial/8250/8250_em.c | 114 ++++++++++++++++++++++++------
 1 file changed, 91 insertions(+), 23 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] serial: 8250_em: Use dev_err_probe()

[Biju Das]

This patch simplifies probe() function by using dev_err_probe()
instead of dev_err in probe().

While at it, remove the unused header file slab.h and added a
local variable 'dev' to replace '&pdev->dev' in probe().

Also replace devm_clk_get->devm_clk_get_enabled and updated the
clk handling in probe().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
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 | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
index f8e99995eee9..b78c74755735 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"
 
@@ -79,6 +78,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,30 +88,25 @@ 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_enabled(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);
 	up.port.uartclk = clk_get_rate(priv->sclk);
 
 	up.port.iotype = UPIO_MEM32;
@@ -121,11 +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) {
-		dev_err(&pdev->dev, "unable to register 8250 port\n");
-		clk_disable_unprepare(priv->sclk);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "unable to register 8250 port\n");
 
 	priv->line = ret;
 	platform_set_drvdata(pdev, priv);
-- 
2.25.1

Re: [PATCH v2 1/3] serial: 8250_em: Use dev_err_probe()

[Andy Shevchenko]

On Fri, Feb 10, 2023 at 03:41:38PM +0000, Biju Das wrote:
> This patch simplifies probe() function by using dev_err_probe()
> instead of dev_err in probe().
> 
> While at it, remove the unused header file slab.h and added a
> local variable 'dev' to replace '&pdev->dev' in probe().
> 
> Also replace devm_clk_get->devm_clk_get_enabled and updated the
> clk handling in probe().

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> 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 | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
> index f8e99995eee9..b78c74755735 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"
>  
> @@ -79,6 +78,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,30 +88,25 @@ 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_enabled(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);
>  	up.port.uartclk = clk_get_rate(priv->sclk);
>  
>  	up.port.iotype = UPIO_MEM32;
> @@ -121,11 +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) {
> -		dev_err(&pdev->dev, "unable to register 8250 port\n");
> -		clk_disable_unprepare(priv->sclk);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "unable to register 8250 port\n");
>  
>  	priv->line = ret;
>  	platform_set_drvdata(pdev, priv);
> -- 
> 2.25.1
> 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] serial: 8250_em: Use dev_err_probe()

[Geert Uytterhoeven]

Hi Biju,

On Fri, Feb 10, 2023 at 4:41 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> This patch simplifies probe() function by using dev_err_probe()
> instead of dev_err in probe().
>
> While at it, remove the unused header file slab.h and added a
> local variable 'dev' to replace '&pdev->dev' in probe().
>
> Also replace devm_clk_get->devm_clk_get_enabled and updated the
> clk handling in probe().
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v1->v2:
>  * replaced devm_clk_get->devm_clk_get_enabled() and updated clk
>    handling in probe().
>  * Added Rb tag from Geert.

Thanks for the update!

> --- a/drivers/tty/serial/8250/8250_em.c
> +++ b/drivers/tty/serial/8250/8250_em.c

> @@ -121,11 +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) {
> -               dev_err(&pdev->dev, "unable to register 8250 port\n");
> -               clk_disable_unprepare(priv->sclk);

You forgot to remove the call to clk_disable_unprepare()
in serial8250_em_remove().
After that, there is no longer a need to store the clock pointer
in priv->sclk.

> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "unable to register 8250 port\n");
>
>         priv->line = ret;
>         platform_set_drvdata(pdev, priv);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v2 1/3] serial: 8250_em: Use dev_err_probe()

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Friday, February 10, 2023 6:40 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; linux-serial@vger.kernel.org; Fabrizio Castro
> <fabrizio.castro.jz@renesas.com>; linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH v2 1/3] serial: 8250_em: Use dev_err_probe()
> 
> Hi Biju,
> 
> On Fri, Feb 10, 2023 at 4:41 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > This patch simplifies probe() function by using dev_err_probe()
> > instead of dev_err in probe().
> >
> > While at it, remove the unused header file slab.h and added a local
> > variable 'dev' to replace '&pdev->dev' in probe().
> >
> > Also replace devm_clk_get->devm_clk_get_enabled and updated the clk
> > handling in probe().
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > v1->v2:
> >  * replaced devm_clk_get->devm_clk_get_enabled() and updated clk
> >    handling in probe().
> >  * Added Rb tag from Geert.
> 
> Thanks for the update!
> 
> > --- a/drivers/tty/serial/8250/8250_em.c
> > +++ b/drivers/tty/serial/8250/8250_em.c
> 
> > @@ -121,11 +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) {
> > -               dev_err(&pdev->dev, "unable to register 8250 port\n");
> > -               clk_disable_unprepare(priv->sclk);
> 
> You forgot to remove the call to clk_disable_unprepare() in
> serial8250_em_remove().
> After that, there is no longer a need to store the clock pointer in priv-
> >sclk.

OK, will fix this in next version.

Cheers,
Biju

> 
> > -               return ret;
> > -       }
> > +       if (ret < 0)
> > +               return dev_err_probe(dev, ret, "unable to register
> > + 8250 port\n");
> >
> >         priv->line = ret;
> >         platform_set_drvdata(pdev, priv);
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
>                                 -- Linus Torvalds

[PATCH v2 2/3] serial: 8250_em: Update RZ/V2M port type as PORT_16750

[Biju Das]

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

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2:
 * New patch
---
 drivers/tty/serial/8250/8250_em.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
index b78c74755735..628a6846bfdc 100644
--- a/drivers/tty/serial/8250/8250_em.c
+++ b/drivers/tty/serial/8250/8250_em.c
@@ -9,6 +9,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
+#include <linux/of_device.h>
 #include <linux/serial_8250.h>
 #include <linux/serial_reg.h>
 #include <linux/platform_device.h>
@@ -19,9 +20,15 @@
 #define UART_DLL_EM 9
 #define UART_DLM_EM 10
 
+struct serial8250_em_hw_info {
+	unsigned int type;
+	upf_t flags;
+};
+
 struct serial8250_em_priv {
 	struct clk *sclk;
 	int line;
+	const struct serial8250_em_hw_info *info;
 };
 
 static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
@@ -95,6 +102,7 @@ static int serial8250_em_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->info = of_device_get_match_data(dev);
 	priv->sclk = devm_clk_get_enabled(dev, "sclk");
 	if (IS_ERR(priv->sclk))
 		return dev_err_probe(dev, PTR_ERR(priv->sclk), "unable to get clock\n");
@@ -102,8 +110,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 = priv->info->type;
+	up.port.flags = priv->info->flags;
 	up.port.dev = dev;
 	up.port.private_data = priv;
 
@@ -133,9 +141,20 @@ static int serial8250_em_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct serial8250_em_hw_info emma_mobile_uart_hw_info = {
+	.type = PORT_UNKNOWN,
+	.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP,
+};
+
+static const struct serial8250_em_hw_info rzv2m_uart_hw_info = {
+	.type = PORT_16750,
+	.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP | UPF_FIXED_TYPE,
+};
+
 static const struct of_device_id serial8250_em_dt_ids[] = {
-	{ .compatible = "renesas,em-uart", },
-	{},
+	{ .compatible = "renesas,r9a09g011-uart", .data = &rzv2m_uart_hw_info },
+	{ .compatible = "renesas,em-uart", .data = &emma_mobile_uart_hw_info },
+	{ /* Sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, serial8250_em_dt_ids);
 
-- 
2.25.1

Re: [PATCH v2 2/3] serial: 8250_em: Update RZ/V2M port type as PORT_16750

[Ilpo Järvinen]

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

On Fri, 10 Feb 2023, Biju Das wrote:

> The UART IP found on RZ/V2M SoC is Register-compatible with the
> general-purpose 16750 UART chip. This patch updates RZ/V2M
> port type from 16550A->16750 and also enables 64-bytes fifo support.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v2 2/3] serial: 8250_em: Update RZ/V2M port type as PORT_16750

[Ilpo Järvinen]

On Fri, 10 Feb 2023, Biju Das wrote:

> The UART IP found on RZ/V2M SoC is Register-compatible with the
> general-purpose 16750 UART chip. This patch updates RZ/V2M
> port type from 16550A->16750 and also enables 64-bytes fifo support.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2:
>  * New patch
> ---
>  drivers/tty/serial/8250/8250_em.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
> index b78c74755735..628a6846bfdc 100644
> --- a/drivers/tty/serial/8250/8250_em.c
> +++ b/drivers/tty/serial/8250/8250_em.c
> @@ -9,6 +9,7 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/of_device.h>
>  #include <linux/serial_8250.h>
>  #include <linux/serial_reg.h>
>  #include <linux/platform_device.h>
> @@ -19,9 +20,15 @@
>  #define UART_DLL_EM 9
>  #define UART_DLM_EM 10
>  
> +struct serial8250_em_hw_info {
> +	unsigned int type;
> +	upf_t flags;
> +};
> +
>  struct serial8250_em_priv {
>  	struct clk *sclk;
>  	int line;
> +	const struct serial8250_em_hw_info *info;
>  };
>  
>  static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
> @@ -95,6 +102,7 @@ static int serial8250_em_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	priv->info = of_device_get_match_data(dev);

On second thought, do you even need to store the info beyond .probe()?


-- 
 i.

RE: [PATCH v2 2/3] serial: 8250_em: Update RZ/V2M port type as PORT_16750

[Biju Das]

Hi Ilpo,

Thanks for the feedback.

> Subject: Re: [PATCH v2 2/3] serial: 8250_em: Update RZ/V2M port type as
> PORT_16750
> 
> On Fri, 10 Feb 2023, Biju Das wrote:
> 
> > The UART IP found on RZ/V2M SoC is Register-compatible with the
> > general-purpose 16750 UART chip. This patch updates RZ/V2M port type
> > from 16550A->16750 and also enables 64-bytes fifo support.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2:
> >  * New patch
> > ---
> >  drivers/tty/serial/8250/8250_em.c | 27 +++++++++++++++++++++++----
> >  1 file changed, 23 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_em.c
> > b/drivers/tty/serial/8250/8250_em.c
> > index b78c74755735..628a6846bfdc 100644
> > --- a/drivers/tty/serial/8250/8250_em.c
> > +++ b/drivers/tty/serial/8250/8250_em.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> >  #include <linux/mod_devicetable.h>
> > +#include <linux/of_device.h>
> >  #include <linux/serial_8250.h>
> >  #include <linux/serial_reg.h>
> >  #include <linux/platform_device.h>
> > @@ -19,9 +20,15 @@
> >  #define UART_DLL_EM 9
> >  #define UART_DLM_EM 10
> >
> > +struct serial8250_em_hw_info {
> > +	unsigned int type;
> > +	upf_t flags;
> > +};
> > +
> >  struct serial8250_em_priv {
> >  	struct clk *sclk;
> >  	int line;
> > +	const struct serial8250_em_hw_info *info;
> >  };
> >
> >  static void serial8250_em_serial_out(struct uart_port *p, int offset,
> > int value) @@ -95,6 +102,7 @@ static int serial8250_em_probe(struct
> platform_device *pdev)
> >  	if (!priv)
> >  		return -ENOMEM;
> >
> > +	priv->info = of_device_get_match_data(dev);
> 
> On second thought, do you even need to store the info beyond .probe()?

OK, will use local variable instead.

Cheers,
Biju

Re: [PATCH v2 2/3] serial: 8250_em: Update RZ/V2M port type as PORT_16750

[Andy Shevchenko]

On Fri, Feb 10, 2023 at 03:41:39PM +0000, Biju Das wrote:
> The UART IP found on RZ/V2M SoC is Register-compatible with the
> general-purpose 16750 UART chip. This patch updates RZ/V2M
> port type from 16550A->16750 and also enables 64-bytes fifo support.

...

> +#include <linux/of_device.h>

property.h

...

> +	priv->info = of_device_get_match_data(dev);

device_get_match_data()

...

With Ilpo's comment addressed this looks good!

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 2/3] serial: 8250_em: Update RZ/V2M port type as PORT_16750

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@intel.com>
> Sent: Friday, February 10, 2023 4:49 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; Geert Uytterhoeven <geert+renesas@glider.be>; linux-
> serial@vger.kernel.org; Fabrizio Castro <fabrizio.castro.jz@renesas.com>;
> linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH v2 2/3] serial: 8250_em: Update RZ/V2M port type as
> PORT_16750
> 
> On Fri, Feb 10, 2023 at 03:41:39PM +0000, Biju Das wrote:
> > The UART IP found on RZ/V2M SoC is Register-compatible with the
> > general-purpose 16750 UART chip. This patch updates RZ/V2M port type
> > from 16550A->16750 and also enables 64-bytes fifo support.
> 
> ...
> 
> > +#include <linux/of_device.h>
> 
> property.h
OK.
> 
> ...
> 
> > +	priv->info = of_device_get_match_data(dev);
> 
> device_get_match_data()

OK.

> 
> ...
> 
> With Ilpo's comment addressed this looks good!

Will send next version with above addressed.

Cheers,
Biju

[PATCH v2 3/3] serial: 8250_em: Add serial_out() to struct serial8250_em_hw_info

[Biju Das]

As per HW manual section 40.6.1, we need to perform FIFO reset + SW
reset before updating the below registers.

FCR[7:5], FCR[3:0], LCR[7][5:0], MCR[6:4], DLL[7:0], DLM[7:0] and
HCR0[6:5][3:2].

This patch adds serial_out() to struct serial8250_em_hw_info to
handle this difference between emma mobile and rz/v2m.

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.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Added serial_out to struct serial8250_em_hw_info
---
 drivers/tty/serial/8250/8250_em.c | 59 ++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
index 628a6846bfdc..1816362a3a3a 100644
--- a/drivers/tty/serial/8250/8250_em.c
+++ b/drivers/tty/serial/8250/8250_em.c
@@ -19,10 +19,14 @@
 
 #define UART_DLL_EM 9
 #define UART_DLM_EM 10
+#define UART_HCR0 11
+
+#define UART_HCR0_SW_RESET	BIT(7) /* SW Reset */
 
 struct serial8250_em_hw_info {
 	unsigned int type;
 	upf_t flags;
+	void (*serial_out)(struct uart_port *p, int off, int value);
 };
 
 struct serial8250_em_priv {
@@ -31,6 +35,40 @@ struct serial8250_em_priv {
 	const struct serial8250_em_hw_info *info;
 };
 
+static void serial8250_rzv2m_reg_update(struct uart_port *p, int off, int value)
+{
+	unsigned int ier, fcr, lcr, mcr, hcr0;
+
+	ier = readl(p->membase + (UART_IER << 2));
+	hcr0 = readl(p->membase + (UART_HCR0 << 2));
+	fcr = readl(p->membase + ((UART_FCR + 1) << 2));
+	lcr = readl(p->membase + ((UART_LCR + 1) << 2));
+	mcr = readl(p->membase + ((UART_MCR + 1) << 2));
+
+	writel(fcr | UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT,
+	       p->membase + ((UART_FCR + 1) << 2));
+	writel(hcr0 | UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
+	writel(hcr0 & ~UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
+
+	switch (off) {
+	case UART_FCR:
+		fcr = value;
+		break;
+	case UART_LCR:
+		lcr = value;
+		break;
+	case UART_MCR:
+		mcr = value;
+		break;
+	}
+
+	writel(ier, p->membase + (UART_IER << 2));
+	writel(fcr, p->membase + ((UART_FCR + 1) << 2));
+	writel(mcr, p->membase + ((UART_MCR + 1) << 2));
+	writel(lcr, p->membase + ((UART_LCR + 1) << 2));
+	writel(hcr0, p->membase + (UART_HCR0 << 2));
+}
+
 static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
 {
 	switch (offset) {
@@ -52,6 +90,23 @@ static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
 	}
 }
 
+static void serial8250_em_rzv2m_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(p, offset, value);
+		break;
+	case UART_FCR:
+	case UART_LCR:
+	case UART_MCR:
+		serial8250_rzv2m_reg_update(p, offset, value);
+	}
+}
+
 static unsigned int serial8250_em_serial_in(struct uart_port *p, int offset)
 {
 	switch (offset) {
@@ -119,7 +174,7 @@ static int serial8250_em_probe(struct platform_device *pdev)
 
 	up.port.iotype = UPIO_MEM32;
 	up.port.serial_in = serial8250_em_serial_in;
-	up.port.serial_out = serial8250_em_serial_out;
+	up.port.serial_out = priv->info->serial_out;
 	up.dl_read = serial8250_em_serial_dl_read;
 	up.dl_write = serial8250_em_serial_dl_write;
 
@@ -144,11 +199,13 @@ static int serial8250_em_remove(struct platform_device *pdev)
 static const struct serial8250_em_hw_info emma_mobile_uart_hw_info = {
 	.type = PORT_UNKNOWN,
 	.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP,
+	.serial_out = serial8250_em_serial_out,
 };
 
 static const struct serial8250_em_hw_info rzv2m_uart_hw_info = {
 	.type = PORT_16750,
 	.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP | UPF_FIXED_TYPE,
+	.serial_out = serial8250_em_rzv2m_serial_out,
 };
 
 static const struct of_device_id serial8250_em_dt_ids[] = {
-- 
2.25.1

Re: [PATCH v2 3/3] serial: 8250_em: Add serial_out() to struct serial8250_em_hw_info

[Ilpo Järvinen]

On Fri, 10 Feb 2023, Biju Das wrote:

> As per HW manual section 40.6.1, we need to perform FIFO reset + SW
> reset before updating the below registers.
> 
> FCR[7:5], FCR[3:0], LCR[7][5:0], MCR[6:4], DLL[7:0], DLM[7:0] and
> HCR0[6:5][3:2].
> 
> This patch adds serial_out() to struct serial8250_em_hw_info to
> handle this difference between emma mobile and rz/v2m.
> 
> 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.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v1->v2:
>  * Added serial_out to struct serial8250_em_hw_info

Thanks, this looks much better. One additional comment below.

> ---
>  drivers/tty/serial/8250/8250_em.c | 59 ++++++++++++++++++++++++++++++-
>  1 file changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
> index 628a6846bfdc..1816362a3a3a 100644
> --- a/drivers/tty/serial/8250/8250_em.c
> +++ b/drivers/tty/serial/8250/8250_em.c
> @@ -19,10 +19,14 @@
>  
>  #define UART_DLL_EM 9
>  #define UART_DLM_EM 10
> +#define UART_HCR0 11
> +
> +#define UART_HCR0_SW_RESET	BIT(7) /* SW Reset */
>  
>  struct serial8250_em_hw_info {
>  	unsigned int type;
>  	upf_t flags;
> +	void (*serial_out)(struct uart_port *p, int off, int value);
>  };
>  
>  struct serial8250_em_priv {
> @@ -31,6 +35,40 @@ struct serial8250_em_priv {
>  	const struct serial8250_em_hw_info *info;
>  };
>  
> +static void serial8250_rzv2m_reg_update(struct uart_port *p, int off, int value)
> +{
> +	unsigned int ier, fcr, lcr, mcr, hcr0;
> +
> +	ier = readl(p->membase + (UART_IER << 2));
> +	hcr0 = readl(p->membase + (UART_HCR0 << 2));
> +	fcr = readl(p->membase + ((UART_FCR + 1) << 2));
> +	lcr = readl(p->membase + ((UART_LCR + 1) << 2));
> +	mcr = readl(p->membase + ((UART_MCR + 1) << 2));
> +
> +	writel(fcr | UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT,
> +	       p->membase + ((UART_FCR + 1) << 2));
> +	writel(hcr0 | UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
> +	writel(hcr0 & ~UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
> +
> +	switch (off) {
> +	case UART_FCR:
> +		fcr = value;
> +		break;
> +	case UART_LCR:
> +		lcr = value;
> +		break;
> +	case UART_MCR:
> +		mcr = value;
> +		break;
> +	}
> +
> +	writel(ier, p->membase + (UART_IER << 2));
> +	writel(fcr, p->membase + ((UART_FCR + 1) << 2));
> +	writel(mcr, p->membase + ((UART_MCR + 1) << 2));
> +	writel(lcr, p->membase + ((UART_LCR + 1) << 2));
> +	writel(hcr0, p->membase + (UART_HCR0 << 2));

Perhaps it would make sense to instead of using readl/writel() directly to 
call serial8250_em_serial_in/out() so all the offset trickery wouldn't 
need to be duplicated inside this function?

-- 
 i.

> +}
> +
>  static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
>  {
>  	switch (offset) {
> @@ -52,6 +90,23 @@ static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
>  	}
>  }
>  
> +static void serial8250_em_rzv2m_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(p, offset, value);
> +		break;
> +	case UART_FCR:
> +	case UART_LCR:
> +	case UART_MCR:
> +		serial8250_rzv2m_reg_update(p, offset, value);
> +	}
> +}
> +
>  static unsigned int serial8250_em_serial_in(struct uart_port *p, int offset)
>  {
>  	switch (offset) {
> @@ -119,7 +174,7 @@ static int serial8250_em_probe(struct platform_device *pdev)
>  
>  	up.port.iotype = UPIO_MEM32;
>  	up.port.serial_in = serial8250_em_serial_in;
> -	up.port.serial_out = serial8250_em_serial_out;
> +	up.port.serial_out = priv->info->serial_out;
>  	up.dl_read = serial8250_em_serial_dl_read;
>  	up.dl_write = serial8250_em_serial_dl_write;
>  
> @@ -144,11 +199,13 @@ static int serial8250_em_remove(struct platform_device *pdev)
>  static const struct serial8250_em_hw_info emma_mobile_uart_hw_info = {
>  	.type = PORT_UNKNOWN,
>  	.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP,
> +	.serial_out = serial8250_em_serial_out,
>  };
>  
>  static const struct serial8250_em_hw_info rzv2m_uart_hw_info = {
>  	.type = PORT_16750,
>  	.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP | UPF_FIXED_TYPE,
> +	.serial_out = serial8250_em_rzv2m_serial_out,
>  };
>  
>  static const struct of_device_id serial8250_em_dt_ids[] = {
> 

RE: [PATCH v2 3/3] serial: 8250_em: Add serial_out() to struct serial8250_em_hw_info

[Biju Das]

Hi Ilpo,

Thanks for the update.

> Subject: Re: [PATCH v2 3/3] serial: 8250_em: Add serial_out() to struct
> serial8250_em_hw_info
> 
> On Fri, 10 Feb 2023, Biju Das wrote:
> 
> > As per HW manual section 40.6.1, we need to perform FIFO reset + SW
> > reset before updating the below registers.
> >
> > FCR[7:5], FCR[3:0], LCR[7][5:0], MCR[6:4], DLL[7:0], DLM[7:0] and
> > HCR0[6:5][3:2].
> >
> > This patch adds serial_out() to struct serial8250_em_hw_info to handle
> > this difference between emma mobile and rz/v2m.
> >
> > 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.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v1->v2:
> >  * Added serial_out to struct serial8250_em_hw_info
> 
> Thanks, this looks much better. One additional comment below.
> 
> > ---
> >  drivers/tty/serial/8250/8250_em.c | 59
> > ++++++++++++++++++++++++++++++-
> >  1 file changed, 58 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_em.c
> > b/drivers/tty/serial/8250/8250_em.c
> > index 628a6846bfdc..1816362a3a3a 100644
> > --- a/drivers/tty/serial/8250/8250_em.c
> > +++ b/drivers/tty/serial/8250/8250_em.c
> > @@ -19,10 +19,14 @@
> >
> >  #define UART_DLL_EM 9
> >  #define UART_DLM_EM 10
> > +#define UART_HCR0 11
> > +
> > +#define UART_HCR0_SW_RESET	BIT(7) /* SW Reset */
> >
> >  struct serial8250_em_hw_info {
> >  	unsigned int type;
> >  	upf_t flags;
> > +	void (*serial_out)(struct uart_port *p, int off, int value);
> >  };
> >
> >  struct serial8250_em_priv {
> > @@ -31,6 +35,40 @@ struct serial8250_em_priv {
> >  	const struct serial8250_em_hw_info *info;  };
> >
> > +static void serial8250_rzv2m_reg_update(struct uart_port *p, int off,
> > +int value) {
> > +	unsigned int ier, fcr, lcr, mcr, hcr0;
> > +
> > +	ier = readl(p->membase + (UART_IER << 2));
> > +	hcr0 = readl(p->membase + (UART_HCR0 << 2));
> > +	fcr = readl(p->membase + ((UART_FCR + 1) << 2));
> > +	lcr = readl(p->membase + ((UART_LCR + 1) << 2));
> > +	mcr = readl(p->membase + ((UART_MCR + 1) << 2));
> > +
> > +	writel(fcr | UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT,
> > +	       p->membase + ((UART_FCR + 1) << 2));
> > +	writel(hcr0 | UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
> > +	writel(hcr0 & ~UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
> > +
> > +	switch (off) {
> > +	case UART_FCR:
> > +		fcr = value;
> > +		break;
> > +	case UART_LCR:
> > +		lcr = value;
> > +		break;
> > +	case UART_MCR:
> > +		mcr = value;
> > +		break;
> > +	}
> > +
> > +	writel(ier, p->membase + (UART_IER << 2));
> > +	writel(fcr, p->membase + ((UART_FCR + 1) << 2));
> > +	writel(mcr, p->membase + ((UART_MCR + 1) << 2));
> > +	writel(lcr, p->membase + ((UART_LCR + 1) << 2));
> > +	writel(hcr0, p->membase + (UART_HCR0 << 2));
> 
> Perhaps it would make sense to instead of using readl/writel() directly to
> call serial8250_em_serial_in/out() so all the offset trickery wouldn't need
> to be duplicated inside this function?

HCR0 register is not available for emma mobile. Is it ok if I just do readl/writel for
that register and rest will use serial8250_em_serial_in/out()??

Cheers,
Biju

> 
> --
>  i.
> 
> > +}
> > +
> >  static void serial8250_em_serial_out(struct uart_port *p, int offset,
> > int value)  {
> >  	switch (offset) {
> > @@ -52,6 +90,23 @@ static void serial8250_em_serial_out(struct uart_port
> *p, int offset, int value)
> >  	}
> >  }
> >
> > +static void serial8250_em_rzv2m_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(p, offset, value);
> > +		break;
> > +	case UART_FCR:
> > +	case UART_LCR:
> > +	case UART_MCR:
> > +		serial8250_rzv2m_reg_update(p, offset, value);
> > +	}
> > +}
> > +
> >  static unsigned int serial8250_em_serial_in(struct uart_port *p, int
> > offset)  {
> >  	switch (offset) {
> > @@ -119,7 +174,7 @@ static int serial8250_em_probe(struct
> > platform_device *pdev)
> >
> >  	up.port.iotype = UPIO_MEM32;
> >  	up.port.serial_in = serial8250_em_serial_in;
> > -	up.port.serial_out = serial8250_em_serial_out;
> > +	up.port.serial_out = priv->info->serial_out;
> >  	up.dl_read = serial8250_em_serial_dl_read;
> >  	up.dl_write = serial8250_em_serial_dl_write;
> >
> > @@ -144,11 +199,13 @@ static int serial8250_em_remove(struct
> > platform_device *pdev)  static const struct serial8250_em_hw_info
> emma_mobile_uart_hw_info = {
> >  	.type = PORT_UNKNOWN,
> >  	.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP,
> > +	.serial_out = serial8250_em_serial_out,
> >  };
> >
> >  static const struct serial8250_em_hw_info rzv2m_uart_hw_info = {
> >  	.type = PORT_16750,
> >  	.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP |
> > UPF_FIXED_TYPE,
> > +	.serial_out = serial8250_em_rzv2m_serial_out,
> >  };
> >
> >  static const struct of_device_id serial8250_em_dt_ids[] = {
> >

RE: [PATCH v2 3/3] serial: 8250_em: Add serial_out() to struct serial8250_em_hw_info

[Ilpo Järvinen]

On Fri, 10 Feb 2023, Biju Das wrote:

> Hi Ilpo,
> 
> Thanks for the update.
> 
> > Subject: Re: [PATCH v2 3/3] serial: 8250_em: Add serial_out() to struct
> > serial8250_em_hw_info
> > 
> > On Fri, 10 Feb 2023, Biju Das wrote:
> > 
> > > As per HW manual section 40.6.1, we need to perform FIFO reset + SW
> > > reset before updating the below registers.
> > >
> > > FCR[7:5], FCR[3:0], LCR[7][5:0], MCR[6:4], DLL[7:0], DLM[7:0] and
> > > HCR0[6:5][3:2].
> > >
> > > This patch adds serial_out() to struct serial8250_em_hw_info to handle
> > > this difference between emma mobile and rz/v2m.
> > >
> > > 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.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > v1->v2:
> > >  * Added serial_out to struct serial8250_em_hw_info
> > 
> > Thanks, this looks much better. One additional comment below.
> > 
> > > ---
> > >  drivers/tty/serial/8250/8250_em.c | 59
> > > ++++++++++++++++++++++++++++++-
> > >  1 file changed, 58 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_em.c
> > > b/drivers/tty/serial/8250/8250_em.c
> > > index 628a6846bfdc..1816362a3a3a 100644
> > > --- a/drivers/tty/serial/8250/8250_em.c
> > > +++ b/drivers/tty/serial/8250/8250_em.c
> > > @@ -19,10 +19,14 @@
> > >
> > >  #define UART_DLL_EM 9
> > >  #define UART_DLM_EM 10
> > > +#define UART_HCR0 11
> > > +
> > > +#define UART_HCR0_SW_RESET	BIT(7) /* SW Reset */
> > >
> > >  struct serial8250_em_hw_info {
> > >  	unsigned int type;
> > >  	upf_t flags;
> > > +	void (*serial_out)(struct uart_port *p, int off, int value);
> > >  };
> > >
> > >  struct serial8250_em_priv {
> > > @@ -31,6 +35,40 @@ struct serial8250_em_priv {
> > >  	const struct serial8250_em_hw_info *info;  };
> > >
> > > +static void serial8250_rzv2m_reg_update(struct uart_port *p, int off,
> > > +int value) {
> > > +	unsigned int ier, fcr, lcr, mcr, hcr0;
> > > +
> > > +	ier = readl(p->membase + (UART_IER << 2));
> > > +	hcr0 = readl(p->membase + (UART_HCR0 << 2));
> > > +	fcr = readl(p->membase + ((UART_FCR + 1) << 2));
> > > +	lcr = readl(p->membase + ((UART_LCR + 1) << 2));
> > > +	mcr = readl(p->membase + ((UART_MCR + 1) << 2));
> > > +
> > > +	writel(fcr | UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT,
> > > +	       p->membase + ((UART_FCR + 1) << 2));
> > > +	writel(hcr0 | UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
> > > +	writel(hcr0 & ~UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
> > > +
> > > +	switch (off) {
> > > +	case UART_FCR:
> > > +		fcr = value;
> > > +		break;
> > > +	case UART_LCR:
> > > +		lcr = value;
> > > +		break;
> > > +	case UART_MCR:
> > > +		mcr = value;
> > > +		break;
> > > +	}
> > > +
> > > +	writel(ier, p->membase + (UART_IER << 2));
> > > +	writel(fcr, p->membase + ((UART_FCR + 1) << 2));
> > > +	writel(mcr, p->membase + ((UART_MCR + 1) << 2));
> > > +	writel(lcr, p->membase + ((UART_LCR + 1) << 2));
> > > +	writel(hcr0, p->membase + (UART_HCR0 << 2));
> > 
> > Perhaps it would make sense to instead of using readl/writel() directly to
> > call serial8250_em_serial_in/out() so all the offset trickery wouldn't need
> > to be duplicated inside this function?
> 
> HCR0 register is not available for emma mobile. Is it ok if I just do readl/writel for
> that register and rest will use serial8250_em_serial_in/out()??

Yes, it's fine.

-- 
 i.

Re: [PATCH v2 3/3] serial: 8250_em: Add serial_out() to struct serial8250_em_hw_info

[Geert Uytterhoeven]

Hi Biju,

On Fri, Feb 10, 2023 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > On Fri, 10 Feb 2023, Biju Das wrote:
> > > As per HW manual section 40.6.1, we need to perform FIFO reset + SW
> > > reset before updating the below registers.
> > >
> > > FCR[7:5], FCR[3:0], LCR[7][5:0], MCR[6:4], DLL[7:0], DLM[7:0] and
> > > HCR0[6:5][3:2].
> > >
> > > This patch adds serial_out() to struct serial8250_em_hw_info to handle
> > > this difference between emma mobile and rz/v2m.
> > >
> > > 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.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

> > > --- a/drivers/tty/serial/8250/8250_em.c
> > > +++ b/drivers/tty/serial/8250/8250_em.c
> > > @@ -31,6 +35,40 @@ struct serial8250_em_priv {
> > >     const struct serial8250_em_hw_info *info;  };
> > >
> > > +static void serial8250_rzv2m_reg_update(struct uart_port *p, int off,
> > > +int value) {
> > > +   unsigned int ier, fcr, lcr, mcr, hcr0;
> > > +
> > > +   ier = readl(p->membase + (UART_IER << 2));
> > > +   hcr0 = readl(p->membase + (UART_HCR0 << 2));
> > > +   fcr = readl(p->membase + ((UART_FCR + 1) << 2));
> > > +   lcr = readl(p->membase + ((UART_LCR + 1) << 2));
> > > +   mcr = readl(p->membase + ((UART_MCR + 1) << 2));
> > > +
> > > +   writel(fcr | UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT,
> > > +          p->membase + ((UART_FCR + 1) << 2));
> > > +   writel(hcr0 | UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
> > > +   writel(hcr0 & ~UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
> > > +
> > > +   switch (off) {
> > > +   case UART_FCR:
> > > +           fcr = value;
> > > +           break;
> > > +   case UART_LCR:
> > > +           lcr = value;
> > > +           break;
> > > +   case UART_MCR:
> > > +           mcr = value;
> > > +           break;
> > > +   }
> > > +
> > > +   writel(ier, p->membase + (UART_IER << 2));
> > > +   writel(fcr, p->membase + ((UART_FCR + 1) << 2));
> > > +   writel(mcr, p->membase + ((UART_MCR + 1) << 2));
> > > +   writel(lcr, p->membase + ((UART_LCR + 1) << 2));
> > > +   writel(hcr0, p->membase + (UART_HCR0 << 2));
> >
> > Perhaps it would make sense to instead of using readl/writel() directly to
> > call serial8250_em_serial_in/out() so all the offset trickery wouldn't need
> > to be duplicated inside this function?
>
> HCR0 register is not available for emma mobile. Is it ok if I just do readl/writel for
> that register and rest will use serial8250_em_serial_in/out()??

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

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v2 3/3] serial: 8250_em: Add serial_out() to struct serial8250_em_hw_info

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v2 3/3] serial: 8250_em: Add serial_out() to struct
> serial8250_em_hw_info
> 
> Hi Biju,
> 
> On Fri, Feb 10, 2023 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > On Fri, 10 Feb 2023, Biju Das wrote:
> > > > As per HW manual section 40.6.1, we need to perform FIFO reset +
> > > > SW reset before updating the below registers.
> > > >
> > > > FCR[7:5], FCR[3:0], LCR[7][5:0], MCR[6:4], DLL[7:0], DLM[7:0] and
> > > > HCR0[6:5][3:2].
> > > >
> > > > This patch adds serial_out() to struct serial8250_em_hw_info to
> > > > handle this difference between emma mobile and rz/v2m.
> > > >
> > > > 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.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> > > > --- a/drivers/tty/serial/8250/8250_em.c
> > > > +++ b/drivers/tty/serial/8250/8250_em.c
> > > > @@ -31,6 +35,40 @@ struct serial8250_em_priv {
> > > >     const struct serial8250_em_hw_info *info;  };
> > > >
> > > > +static void serial8250_rzv2m_reg_update(struct uart_port *p, int
> > > > +off, int value) {
> > > > +   unsigned int ier, fcr, lcr, mcr, hcr0;
> > > > +
> > > > +   ier = readl(p->membase + (UART_IER << 2));
> > > > +   hcr0 = readl(p->membase + (UART_HCR0 << 2));
> > > > +   fcr = readl(p->membase + ((UART_FCR + 1) << 2));
> > > > +   lcr = readl(p->membase + ((UART_LCR + 1) << 2));
> > > > +   mcr = readl(p->membase + ((UART_MCR + 1) << 2));
> > > > +
> > > > +   writel(fcr | UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT,
> > > > +          p->membase + ((UART_FCR + 1) << 2));
> > > > +   writel(hcr0 | UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
> > > > +   writel(hcr0 & ~UART_HCR0_SW_RESET, p->membase + (UART_HCR0 <<
> > > > + 2));
> > > > +
> > > > +   switch (off) {
> > > > +   case UART_FCR:
> > > > +           fcr = value;
> > > > +           break;
> > > > +   case UART_LCR:
> > > > +           lcr = value;
> > > > +           break;
> > > > +   case UART_MCR:
> > > > +           mcr = value;
> > > > +           break;
> > > > +   }
> > > > +
> > > > +   writel(ier, p->membase + (UART_IER << 2));
> > > > +   writel(fcr, p->membase + ((UART_FCR + 1) << 2));
> > > > +   writel(mcr, p->membase + ((UART_MCR + 1) << 2));
> > > > +   writel(lcr, p->membase + ((UART_LCR + 1) << 2));
> > > > +   writel(hcr0, p->membase + (UART_HCR0 << 2));
> > >
> > > Perhaps it would make sense to instead of using readl/writel()
> > > directly to call serial8250_em_serial_in/out() so all the offset
> > > trickery wouldn't need to be duplicated inside this function?
> >
> > HCR0 register is not available for emma mobile. Is it ok if I just do
> > readl/writel for that register and rest will use
> serial8250_em_serial_in/out()??
> 
> According to R19UH0040EJ0400 Rev.4.00 it is available on EMEV2, and the
> layout looks identical to RZ/V2M.

OK, will add HCR0 as well. Now only issue is related to fcr as
the value of UART_FCR and UART_IIR are 2. But their address offsets
are at 0x8 and 0xc. So I need to use readl(p->membase + ((UART_FCR + 1) << 2));
for reading FCR and I will add a comment above it.

/*
 * The value of UART_IIR and UART_FCR are 2, but the corresponding
 * RZ/V2M address offset are different(0x08 and 0x0c). So, we need to use
 * readl() here.
 */

Cheers,
Biju