[PATCH 0/3] Add Identification and 64 bit fifo support to Renesas RZ/V2M 16750 UART

[PATCH 0/3] Add Identification and 64 bit fifo support to Renesas RZ/V2M 16750 UART

[Biju Das]

Add identification and 64 bit fifo support to Renesas RZ/V2M 16750 UART.
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.

Biju Das (3):
  serial: 8250: Identify Renesas RZ/V2M 16750 UART
  serial: 8250_em: Use dev_err_probe()
  serial: 8250_em: Add serial8250_rzv2m_reg_update()

 drivers/tty/serial/8250/8250_em.c   | 70 +++++++++++++++++++++++------
 drivers/tty/serial/8250/8250_port.c | 27 +++++++++++
 2 files changed, 84 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART

[Biju Das]

Add identification support for RZ/V2M 16750 UART.

Currently, RZ/V2M UART is detected as 16550A instead of 16750.
"a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 14, base_baud = 3000000)
is a 16550A"

After adding identification support, it is detected as
"a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud = 3000000)
is a Renesas RZ/V2M 16750".

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/tty/serial/8250/8250_port.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index e61753c295d5..e4b205e3756b 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -111,6 +111,15 @@ static const struct serial8250_config uart_config[] = {
 		.rxtrig_bytes	= {1, 16, 32, 56},
 		.flags		= UART_CAP_FIFO | UART_CAP_SLEEP | UART_CAP_AFE,
 	},
+	[PORT_16750] = {
+		.name		= "Renesas RZ/V2M 16750",
+		.fifo_size	= 64,
+		.tx_loadsz	= 64,
+		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
+				  UART_FCR7_64BYTE,
+		.rxtrig_bytes	= {1, 16, 32, 56},
+		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
+	},
 	[PORT_STARTECH] = {
 		.name		= "Startech",
 		.fifo_size	= 1,
@@ -1142,6 +1151,24 @@ static void autoconfig_16550a(struct uart_8250_port *up)
 		return;
 	}
 
+	/*
+	 * No EFR.  Try to detect a Renesas RZ/V2M 16750, which only sets bit 5
+	 * of the IIR when 64 byte FIFO mode is enabled.
+	 * Try setting/clear bit5 of FCR.
+	 */
+	serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
+	status1 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED);
+
+	serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR7_64BYTE);
+	status2 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED);
+
+	if (status1 == UART_IIR_FIFO_ENABLED_16550A &&
+	    status2 == (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED_16550A)) {
+		up->port.type = PORT_16750;
+		up->capabilities |= UART_CAP_AFE;
+		return;
+	}
+
 	/*
 	 * Try writing and reading the UART_IER_UUE bit (b6).
 	 * If it works, this is probably one of the Xscale platform's
-- 
2.25.1

Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART

[Ilpo Järvinen]

On Thu, 9 Feb 2023, Biju Das wrote:

> Add identification support for RZ/V2M 16750 UART.
> 
> Currently, RZ/V2M UART is detected as 16550A instead of 16750.
> "a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 14, base_baud = 3000000)
> is a 16550A"
> 
> After adding identification support, it is detected as
> "a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud = 3000000)
> is a Renesas RZ/V2M 16750".
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index e61753c295d5..e4b205e3756b 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -111,6 +111,15 @@ static const struct serial8250_config uart_config[] = {
>  		.rxtrig_bytes	= {1, 16, 32, 56},
>  		.flags		= UART_CAP_FIFO | UART_CAP_SLEEP | UART_CAP_AFE,
>  	},
> +	[PORT_16750] = {
> +		.name		= "Renesas RZ/V2M 16750",
> +		.fifo_size	= 64,
> +		.tx_loadsz	= 64,
> +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
> +				  UART_FCR7_64BYTE,
> +		.rxtrig_bytes	= {1, 16, 32, 56},
> +		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
> +	},

Eh, how can you reuse [PORT_16750] again in the initializer like that?

-- 
 i.

>  	[PORT_STARTECH] = {
>  		.name		= "Startech",
>  		.fifo_size	= 1,
> @@ -1142,6 +1151,24 @@ static void autoconfig_16550a(struct uart_8250_port *up)
>  		return;
>  	}
>  
> +	/*
> +	 * No EFR.  Try to detect a Renesas RZ/V2M 16750, which only sets bit 5
> +	 * of the IIR when 64 byte FIFO mode is enabled.
> +	 * Try setting/clear bit5 of FCR.
> +	 */
> +	serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
> +	status1 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED);
> +
> +	serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR7_64BYTE);
> +	status2 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED);
> +
> +	if (status1 == UART_IIR_FIFO_ENABLED_16550A &&
> +	    status2 == (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED_16550A)) {
> +		up->port.type = PORT_16750;
> +		up->capabilities |= UART_CAP_AFE;
> +		return;
> +	}
> +
>  	/*
>  	 * Try writing and reading the UART_IER_UUE bit (b6).
>  	 * If it works, this is probably one of the Xscale platform's
> 

RE: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART

[Biju Das]

Hi Ilpo Järvinen,

Thanks for the feedback.

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: Thursday, February 9, 2023 2:09 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>;
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; Maciej W. Rozycki
> <macro@orcam.me.uk>; Eric Tremblay <etremblay@distech-controls.com>; Wander
> Lairson Costa <wander@redhat.com>; linux-serial <linux-
> serial@vger.kernel.org>; Geert Uytterhoeven <geert+renesas@glider.be>;
> Fabrizio Castro <fabrizio.castro.jz@renesas.com>; linux-renesas-
> soc@vger.kernel.org
> Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART
> 
> On Thu, 9 Feb 2023, Biju Das wrote:
> 
> > Add identification support for RZ/V2M 16750 UART.
> >
> > Currently, RZ/V2M UART is detected as 16550A instead of 16750.
> > "a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 14, base_baud =
> > 3000000) is a 16550A"
> >
> > After adding identification support, it is detected as
> > "a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud =
> > 3000000) is a Renesas RZ/V2M 16750".
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/tty/serial/8250/8250_port.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_port.c
> > b/drivers/tty/serial/8250/8250_port.c
> > index e61753c295d5..e4b205e3756b 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -111,6 +111,15 @@ static const struct serial8250_config uart_config[] =
> {
> >  		.rxtrig_bytes	= {1, 16, 32, 56},
> >  		.flags		= UART_CAP_FIFO | UART_CAP_SLEEP | UART_CAP_AFE,
> >  	},
> > +	[PORT_16750] = {
> > +		.name		= "Renesas RZ/V2M 16750",
> > +		.fifo_size	= 64,
> > +		.tx_loadsz	= 64,
> > +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
> > +				  UART_FCR7_64BYTE,
> > +		.rxtrig_bytes	= {1, 16, 32, 56},
> > +		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
> > +	},
> 
> Eh, how can you reuse [PORT_16750] again in the initializer like that?

Oops. Missed it. Is it ok to introduce PORT_RENESAS_16750_F64 instead
as PORT_16750 is used by TI16750?

Cheers,
Biju

> 
> --
>  i.
> 
> >  	[PORT_STARTECH] = {
> >  		.name		= "Startech",
> >  		.fifo_size	= 1,
> > @@ -1142,6 +1151,24 @@ static void autoconfig_16550a(struct uart_8250_port
> *up)
> >  		return;
> >  	}
> >
> > +	/*
> > +	 * No EFR.  Try to detect a Renesas RZ/V2M 16750, which only sets bit
> 5
> > +	 * of the IIR when 64 byte FIFO mode is enabled.
> > +	 * Try setting/clear bit5 of FCR.
> > +	 */
> > +	serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
> > +	status1 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO |
> > +UART_IIR_FIFO_ENABLED);
> > +
> > +	serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR7_64BYTE);
> > +	status2 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO |
> > +UART_IIR_FIFO_ENABLED);
> > +
> > +	if (status1 == UART_IIR_FIFO_ENABLED_16550A &&
> > +	    status2 == (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED_16550A))
> {
> > +		up->port.type = PORT_16750;
> > +		up->capabilities |= UART_CAP_AFE;
> > +		return;
> > +	}
> > +
> >  	/*
> >  	 * Try writing and reading the UART_IER_UUE bit (b6).
> >  	 * If it works, this is probably one of the Xscale platform's
> >

Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART

[Andy Shevchenko]

On Thu, Feb 09, 2023 at 02:28:55PM +0000, Biju Das wrote:
> > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Sent: Thursday, February 9, 2023 2:09 PM
> > On Thu, 9 Feb 2023, Biju Das wrote:

> > > +	[PORT_16750] = {
> > > +		.name		= "Renesas RZ/V2M 16750",
> > > +		.fifo_size	= 64,
> > > +		.tx_loadsz	= 64,
> > > +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
> > > +				  UART_FCR7_64BYTE,
> > > +		.rxtrig_bytes	= {1, 16, 32, 56},
> > > +		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
> > > +	},
> > 
> > Eh, how can you reuse [PORT_16750] again in the initializer like that?
> 
> Oops. Missed it. Is it ok to introduce PORT_RENESAS_16750_F64 instead
> as PORT_16750 is used by TI16750?

What the difference to the 16750 from TI that prevents you from using it?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART
> 
> On Thu, Feb 09, 2023 at 02:28:55PM +0000, Biju Das wrote:
> > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Sent: Thursday, February 9, 2023 2:09 PM On Thu, 9 Feb 2023, Biju
> > > Das wrote:
> 
> > > > +	[PORT_16750] = {
> > > > +		.name		= "Renesas RZ/V2M 16750",
> > > > +		.fifo_size	= 64,
> > > > +		.tx_loadsz	= 64,
> > > > +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
> > > > +				  UART_FCR7_64BYTE,
> > > > +		.rxtrig_bytes	= {1, 16, 32, 56},
> > > > +		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
> > > > +	},
> > >
> > > Eh, how can you reuse [PORT_16750] again in the initializer like that?
> >
> > Oops. Missed it. Is it ok to introduce PORT_RENESAS_16750_F64 instead
> > as PORT_16750 is used by TI16750?
> 
> What the difference to the 16750 from TI that prevents you from using it?

Mostly it is identical.

The main difference is detection method, and we don't have UART_IERX_SLEEP bit in IER.

On TI, it sets bit 5 of the IIR when 64-byte FIFO mode is enabled when DLAB is set.

Whereas in our case DLAB does n't have any role for Identification, 

It set bit 5 of the IIR when 64 byte FIFO mode is enabled.
and it clears bit 5 of the IIR when 64 byte FIFO mode is disabled.

Other than that, when I use PORT_16750 type and capabilities in 8250_em driver and 
add identification method for Renesas UART in 8250_port driver,

It detected as PORT_16750 UART, but I get below prints during autoconf which is confusing for the end user

[    0.214926] serial8250-em a4040000.serial: detected caps 00000900 should be 00000d00
[    0.214975] a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud = 3000000) is a TI16750


Modification in 8250_em driver

+		    up.port.type = PORT_16750;
+               up.port.name            = "Renesas RZ/V2M 16750";
+               up.port.fifosize        = 64;
+               up.tx_loadsz = 64;
+               up.capabilities = UART_CAP_FIFO | UART_CAP_AFE;

Identification method in 8250_port.c driver

+       /*
+        * No EFR.  Try to detect a Renesas RZ/V2M 16750, which only sets bit 5
+        * of the IIR when 64 byte FIFO mode is enabled.
+        * Try setting/clear bit5 of FCR.
+        */
+       serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
+       status1 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED);
+
+       serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR7_64BYTE);
+       status2 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED);
+
+       if (status1 == UART_IIR_FIFO_ENABLED_16550A &&
+           status2 == (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED_16550A)) {
+               up->port.type = PORT_16750;
+               up->capabilities |= UART_CAP_AFE;
+               return;
+       }
+

Cheers,
Biju

Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART

[Andy Shevchenko]

On Fri, Feb 10, 2023 at 07:14:54AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART
> > On Thu, Feb 09, 2023 at 02:28:55PM +0000, Biju Das wrote:
> > > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > Sent: Thursday, February 9, 2023 2:09 PM On Thu, 9 Feb 2023, Biju
> > > > Das wrote:

...

> > > > > +	[PORT_16750] = {
> > > > > +		.name		= "Renesas RZ/V2M 16750",
> > > > > +		.fifo_size	= 64,
> > > > > +		.tx_loadsz	= 64,
> > > > > +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
> > > > > +				  UART_FCR7_64BYTE,
> > > > > +		.rxtrig_bytes	= {1, 16, 32, 56},
> > > > > +		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
> > > > > +	},
> > > >
> > > > Eh, how can you reuse [PORT_16750] again in the initializer like that?
> > >
> > > Oops. Missed it. Is it ok to introduce PORT_RENESAS_16750_F64 instead
> > > as PORT_16750 is used by TI16750?
> > 
> > What the difference to the 16750 from TI that prevents you from using it?
> 
> Mostly it is identical.
> 
> The main difference is detection method, and we don't have UART_IERX_SLEEP bit in IER.
> 
> On TI, it sets bit 5 of the IIR when 64-byte FIFO mode is enabled when DLAB is set.
> 
> Whereas in our case DLAB does n't have any role for Identification, 
> 
> It set bit 5 of the IIR when 64 byte FIFO mode is enabled.
> and it clears bit 5 of the IIR when 64 byte FIFO mode is disabled.

So the question here is do these minor deviations affect the actual functionality?

Note, on Intel hardware we use directly TI16750 while we have no sleep
functionality available IIRC. Ilpo may correct me if I'm wrong.

> Other than that, when I use PORT_16750 type and capabilities in 8250_em driver and 
> add identification method for Renesas UART in 8250_port driver,
> 
> It detected as PORT_16750 UART, but I get below prints during autoconf which is confusing for the end user
> 
> [    0.214926] serial8250-em a4040000.serial: detected caps 00000900 should be 00000d00
> [    0.214975] a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud = 3000000) is a TI16750
> 
> 
> Modification in 8250_em driver
> 
> +		    up.port.type = PORT_16750;
> +               up.port.name            = "Renesas RZ/V2M 16750";
> +               up.port.fifosize        = 64;
> +               up.tx_loadsz = 64;
> +               up.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
> 
> Identification method in 8250_port.c driver
> 
> +       /*
> +        * No EFR.  Try to detect a Renesas RZ/V2M 16750, which only sets bit 5
> +        * of the IIR when 64 byte FIFO mode is enabled.
> +        * Try setting/clear bit5 of FCR.
> +        */
> +       serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
> +       status1 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED);
> +
> +       serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR7_64BYTE);
> +       status2 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED);
> +
> +       if (status1 == UART_IIR_FIFO_ENABLED_16550A &&
> +           status2 == (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED_16550A)) {
> +               up->port.type = PORT_16750;
> +               up->capabilities |= UART_CAP_AFE;
> +               return;
> +       }

What I don't like is increasing quirks in the 8250_port. Can't you simply use FIXED_PORT facility?
Again, look how 8250_mid is written.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART
> 
> On Fri, Feb 10, 2023 at 07:14:54AM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750
> > > UART On Thu, Feb 09, 2023 at 02:28:55PM +0000, Biju Das wrote:
> > > > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > > Sent: Thursday, February 9, 2023 2:09 PM On Thu, 9 Feb 2023,
> > > > > Biju Das wrote:
> 
> ...
> 
> > > > > > +	[PORT_16750] = {
> > > > > > +		.name		= "Renesas RZ/V2M 16750",
> > > > > > +		.fifo_size	= 64,
> > > > > > +		.tx_loadsz	= 64,
> > > > > > +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
> > > > > > +				  UART_FCR7_64BYTE,
> > > > > > +		.rxtrig_bytes	= {1, 16, 32, 56},
> > > > > > +		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
> > > > > > +	},
> > > > >
> > > > > Eh, how can you reuse [PORT_16750] again in the initializer like
> that?
> > > >
> > > > Oops. Missed it. Is it ok to introduce PORT_RENESAS_16750_F64
> > > > instead as PORT_16750 is used by TI16750?
> > >
> > > What the difference to the 16750 from TI that prevents you from using
> it?
> >
> > Mostly it is identical.
> >
> > The main difference is detection method, and we don't have UART_IERX_SLEEP
> bit in IER.
> >
> > On TI, it sets bit 5 of the IIR when 64-byte FIFO mode is enabled when
> DLAB is set.
> >
> > Whereas in our case DLAB does n't have any role for Identification,
> >
> > It set bit 5 of the IIR when 64 byte FIFO mode is enabled.
> > and it clears bit 5 of the IIR when 64 byte FIFO mode is disabled.
> 
> So the question here is do these minor deviations affect the actual
> functionality?

OK.

> 
> Note, on Intel hardware we use directly TI16750 while we have no sleep
> functionality available IIRC. Ilpo may correct me if I'm wrong.
> 
> > Other than that, when I use PORT_16750 type and capabilities in
> > 8250_em driver and add identification method for Renesas UART in
> > 8250_port driver,
> >
> > It detected as PORT_16750 UART, but I get below prints during autoconf
> > which is confusing for the end user
> >
> > [    0.214926] serial8250-em a4040000.serial: detected caps 00000900
> should be 00000d00
> > [    0.214975] a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24,
> base_baud = 3000000) is a TI16750
> >
> >
> > Modification in 8250_em driver
> >
> > +		    up.port.type = PORT_16750;
> > +               up.port.name            = "Renesas RZ/V2M 16750";
> > +               up.port.fifosize        = 64;
> > +               up.tx_loadsz = 64;
> > +               up.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
> >
> > Identification method in 8250_port.c driver
> >
> > +       /*
> > +        * No EFR.  Try to detect a Renesas RZ/V2M 16750, which only sets
> bit 5
> > +        * of the IIR when 64 byte FIFO mode is enabled.
> > +        * Try setting/clear bit5 of FCR.
> > +        */
> > +       serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO);
> > +       status1 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO |
> > + UART_IIR_FIFO_ENABLED);
> > +
> > +       serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR7_64BYTE);
> > +       status2 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO |
> > + UART_IIR_FIFO_ENABLED);
> > +
> > +       if (status1 == UART_IIR_FIFO_ENABLED_16550A &&
> > +           status2 == (UART_IIR_64BYTE_FIFO |
> UART_IIR_FIFO_ENABLED_16550A)) {
> > +               up->port.type = PORT_16750;
> > +               up->capabilities |= UART_CAP_AFE;
> > +               return;
> > +       }
> 
> What I don't like is increasing quirks in the 8250_port. Can't you simply
> use FIXED_PORT facility?
> Again, look how 8250_mid is written.

Coll. I have referred 8250_mid and added below changes and it is working as expected.
I will sent next version after incorporating comments from Geert and ilpo.


+               up.port.type = PORT_16750;
+               up.port.flags |=  UPF_FIXED_TYPE;

a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud = 3000000) is a TI16750

root@rzv2m:~# /uart_dump.sh
Read at address  0xA4040000 (0xffff933ec000): 0x00000000
Read at address  0xA4040004 (0xffffb4994004): 0x00000005
Read at address  0xA4040008 (0xffff8041c008): 0x000000E1
Read at address  0xA404000C (0xffffbb62100c): 0x000000A1
Read at address  0xA4040010 (0xffff94158010): 0x00000013
Read at address  0xA4040014 (0xffff8018c014): 0x0000000B
Read at address  0xA4040018 (0xffff971af018): 0x00000060
Read at address  0xA404001C (0xffffb057d01c): 0x00000010
Read at address  0xA4040020 (0xffff8e8b6020): 0x00000000
Read at address  0xA4040024 (0xffff8094a024): 0x0000001A
Read at address  0xA4040028 (0xffffb6e81028): 0x00000000
Read at address  0xA404002C (0xffffa663902c): 0x00000000
root@rzv2m:~# stty -F /dev/ttyS0 115200 crtscts
root@rzv2m:~# /uart_dump.sh
Read at address  0xA4040000 (0xffff9672f000): 0x00000000
Read at address  0xA4040004 (0xffffb4d63004): 0x0000000D
Read at address  0xA4040008 (0xffffbdd07008): 0x000000E1
Read at address  0xA404000C (0xffff9573a00c): 0x000000A1
Read at address  0xA4040010 (0xffffbd5be010): 0x00000013
Read at address  0xA4040014 (0xffff9451c014): 0x0000002B
Read at address  0xA4040018 (0xffffb47cb018): 0x00000060
Read at address  0xA404001C (0xffffa680901c): 0x00000010
Read at address  0xA4040020 (0xffffb76d1020): 0x00000000
Read at address  0xA4040024 (0xffffb8667024): 0x0000001A
Read at address  0xA4040028 (0xffffad9c8028): 0x00000000
Read at address  0xA404002C (0xffff8f46702c): 0x00000000

Cheers,
Biju

Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART

[Andy Shevchenko]

On Fri, Feb 10, 2023 at 11:53:43AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART
> > On Fri, Feb 10, 2023 at 07:14:54AM +0000, Biju Das wrote:

...

> > What I don't like is increasing quirks in the 8250_port. Can't you simply
> > use FIXED_PORT facility?
> > Again, look how 8250_mid is written.
> 
> Coll. I have referred 8250_mid and added below changes and it is working as expected.
> I will sent next version after incorporating comments from Geert and ilpo.

Glad to hear that and thank you for cooperation!

-- 
With Best Regards,
Andy Shevchenko

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

[Biju Das]

This patch simplify 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().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/tty/serial/8250/8250_em.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
index f8e99995eee9..3a45aa066d3d 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,27 +88,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);
-	}
+	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 +118,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

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

[Geert Uytterhoeven]

Hi Biju,

On Thu, Feb 9, 2023 at 2:26 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> This patch simplify 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().
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- 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,27 +88,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");

One missed opportunity to use "dev".
And to use the new devm_clk_get_enabled() ;-)

> -       if (IS_ERR(priv->sclk)) {
> -               dev_err(&pdev->dev, "unable to get clock\n");
> -               return PTR_ERR(priv->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 +118,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;

As the patch is correct:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 2/3] serial: 8250_em: Use dev_err_probe()

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 2/3] serial: 8250_em: Use dev_err_probe()
> 
> Hi Biju,
> 
> On Thu, Feb 9, 2023 at 2:26 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > This patch simplify 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().
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- 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,27 +88,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");
> 
> One missed opportunity to use "dev".

Agreed.

> And to use the new devm_clk_get_enabled() ;-)

OK, will do as it will simplify clk handling in probe()

Cheers,
Biju

> 
> > -       if (IS_ERR(priv->sclk)) {
> > -               dev_err(&pdev->dev, "unable to get clock\n");
> > -               return PTR_ERR(priv->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 +118,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;
> 
> As the patch is correct:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> 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 3/3] serial: 8250_em: Add serial8250_rzv2m_reg_update()

[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 serial8250_rzv2m_reg_update() 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.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/tty/serial/8250/8250_em.c | 49 +++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
index 3a45aa066d3d..a1e42b8ef99d 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.h>
 #include <linux/serial_8250.h>
 #include <linux/serial_reg.h>
 #include <linux/platform_device.h>
@@ -18,14 +19,53 @@
 
 #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_priv {
 	struct clk *sclk;
 	int line;
+	bool is_rzv2m;
 };
 
+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)
 {
+	struct serial8250_em_priv *priv = p->private_data;
+
 	switch (offset) {
 	case UART_TX: /* TX @ 0x00 */
 		writeb(value, p->membase);
@@ -33,6 +73,11 @@ static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
 	case UART_FCR: /* FCR @ 0x0c (+1) */
 	case UART_LCR: /* LCR @ 0x10 (+1) */
 	case UART_MCR: /* MCR @ 0x14 (+1) */
+		if (priv->is_rzv2m)
+			serial8250_rzv2m_reg_update(p, offset, value);
+		else
+			writel(value, p->membase + ((offset + 1) << 2));
+		break;
 	case UART_SCR: /* SCR @ 0x20 (+1) */
 		writel(value, p->membase + ((offset + 1) << 2));
 		break;
@@ -111,6 +156,10 @@ static int serial8250_em_probe(struct platform_device *pdev)
 	up.port.uartclk = clk_get_rate(priv->sclk);
 
 	up.port.iotype = UPIO_MEM32;
+
+	if (of_device_is_compatible(dev->of_node, "renesas,r9a09g011-uart"))
+		priv->is_rzv2m = true;
+
 	up.port.serial_in = serial8250_em_serial_in;
 	up.port.serial_out = serial8250_em_serial_out;
 	up.dl_read = serial8250_em_serial_dl_read;
-- 
2.25.1

Re: [PATCH 3/3] serial: 8250_em: Add serial8250_rzv2m_reg_update()

[Ilpo Järvinen]

On Thu, 9 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 serial8250_rzv2m_reg_update() 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.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/tty/serial/8250/8250_em.c | 49 +++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
> index 3a45aa066d3d..a1e42b8ef99d 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.h>
>  #include <linux/serial_8250.h>
>  #include <linux/serial_reg.h>
>  #include <linux/platform_device.h>
> @@ -18,14 +19,53 @@
>  
>  #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_priv {
>  	struct clk *sclk;
>  	int line;
> +	bool is_rzv2m;
>  };
>  
> +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)
>  {
> +	struct serial8250_em_priv *priv = p->private_data;
> +
>  	switch (offset) {
>  	case UART_TX: /* TX @ 0x00 */
>  		writeb(value, p->membase);
> @@ -33,6 +73,11 @@ static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
>  	case UART_FCR: /* FCR @ 0x0c (+1) */
>  	case UART_LCR: /* LCR @ 0x10 (+1) */
>  	case UART_MCR: /* MCR @ 0x14 (+1) */
> +		if (priv->is_rzv2m)
> +			serial8250_rzv2m_reg_update(p, offset, value);
> +		else
> +			writel(value, p->membase + ((offset + 1) << 2));
> +		break;

Create serial8250_em_rzv2m_serial_out() that does the necessary magic and 
calls serial8250_em_serial_out() in other cases.

I think you can use .data in of_device_id table to pick the correct 
.serial_out function so you don't need to add that bool at all.

>  	case UART_SCR: /* SCR @ 0x20 (+1) */
>  		writel(value, p->membase + ((offset + 1) << 2));
>  		break;
> @@ -111,6 +156,10 @@ static int serial8250_em_probe(struct platform_device *pdev)
>  	up.port.uartclk = clk_get_rate(priv->sclk);
>  
>  	up.port.iotype = UPIO_MEM32;
> +
> +	if (of_device_is_compatible(dev->of_node, "renesas,r9a09g011-uart"))
> +		priv->is_rzv2m = true;
> +
>  	up.port.serial_in = serial8250_em_serial_in;
>  	up.port.serial_out = serial8250_em_serial_out;
>  	up.dl_read = serial8250_em_serial_dl_read;
> 

I'm bit lost why you need patch 1/3 and cannot set the port type and 
capabilities here?

-- 
 i.

RE: [PATCH 3/3] serial: 8250_em: Add serial8250_rzv2m_reg_update()

[Biju Das]

Hi Ilpo,

Thanks for the feedback.

> Subject: Re: [PATCH 3/3] serial: 8250_em: Add serial8250_rzv2m_reg_update()
> 
> On Thu, 9 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 serial8250_rzv2m_reg_update() 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.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/tty/serial/8250/8250_em.c | 49
> > +++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_em.c
> > b/drivers/tty/serial/8250/8250_em.c
> > index 3a45aa066d3d..a1e42b8ef99d 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.h>
> >  #include <linux/serial_8250.h>
> >  #include <linux/serial_reg.h>
> >  #include <linux/platform_device.h>
> > @@ -18,14 +19,53 @@
> >
> >  #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_priv {
> >  	struct clk *sclk;
> >  	int line;
> > +	bool is_rzv2m;
> >  };
> >
> > +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)  {
> > +	struct serial8250_em_priv *priv = p->private_data;
> > +
> >  	switch (offset) {
> >  	case UART_TX: /* TX @ 0x00 */
> >  		writeb(value, p->membase);
> > @@ -33,6 +73,11 @@ static void serial8250_em_serial_out(struct uart_port
> *p, int offset, int value)
> >  	case UART_FCR: /* FCR @ 0x0c (+1) */
> >  	case UART_LCR: /* LCR @ 0x10 (+1) */
> >  	case UART_MCR: /* MCR @ 0x14 (+1) */
> > +		if (priv->is_rzv2m)
> > +			serial8250_rzv2m_reg_update(p, offset, value);
> > +		else
> > +			writel(value, p->membase + ((offset + 1) << 2));
> > +		break;
> 
> Create serial8250_em_rzv2m_serial_out() that does the necessary magic and
> calls serial8250_em_serial_out() in other cases.
> 
> I think you can use .data in of_device_id table to pick the correct
> .serial_out function so you don't need to add that bool at all.

OK, will add .data and will make use a feature flag to handle this
as suggested by Geert.

> 
> >  	case UART_SCR: /* SCR @ 0x20 (+1) */
> >  		writel(value, p->membase + ((offset + 1) << 2));
> >  		break;
> > @@ -111,6 +156,10 @@ static int serial8250_em_probe(struct platform_device
> *pdev)
> >  	up.port.uartclk = clk_get_rate(priv->sclk);
> >
> >  	up.port.iotype = UPIO_MEM32;
> > +
> > +	if (of_device_is_compatible(dev->of_node, "renesas,r9a09g011-uart"))
> > +		priv->is_rzv2m = true;
> > +
> >  	up.port.serial_in = serial8250_em_serial_in;
> >  	up.port.serial_out = serial8250_em_serial_out;
> >  	up.dl_read = serial8250_em_serial_dl_read;
> >
> 
> I'm bit lost why you need patch 1/3 and cannot set the port type and
> capabilities here?

As per Andy's suggestion I have tried this based on 8250_mid.c and
It works as expected.

So plan is create separate patch for changing port type from 16550A to
16750.

Cheers,
Biju

Re: [PATCH 3/3] serial: 8250_em: Add serial8250_rzv2m_reg_update()

[Geert Uytterhoeven]

Hi Biju,

On Thu, Feb 9, 2023 at 2:30 PM Biju Das <biju.das.jz@bp.renesas.com> 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 serial8250_rzv2m_reg_update() 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.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

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

> @@ -111,6 +156,10 @@ static int serial8250_em_probe(struct platform_device *pdev)
>         up.port.uartclk = clk_get_rate(priv->sclk);
>
>         up.port.iotype = UPIO_MEM32;
> +
> +       if (of_device_is_compatible(dev->of_node, "renesas,r9a09g011-uart"))
> +               priv->is_rzv2m = true;

Please add an entry to serial8250_em_dt_ids[] instead, providing
a feature flag in of_device_id.data.

> +
>         up.port.serial_in = serial8250_em_serial_in;
>         up.port.serial_out = serial8250_em_serial_out;
>         up.dl_read = serial8250_em_serial_dl_read;

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 3/3] serial: 8250_em: Add serial8250_rzv2m_reg_update()

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 3/3] serial: 8250_em: Add serial8250_rzv2m_reg_update()
> 
> Hi Biju,
> 
> On Thu, Feb 9, 2023 at 2:30 PM Biju Das <biju.das.jz@bp.renesas.com> 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 serial8250_rzv2m_reg_update() 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.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/tty/serial/8250/8250_em.c
> > +++ b/drivers/tty/serial/8250/8250_em.c
> 
> > @@ -111,6 +156,10 @@ static int serial8250_em_probe(struct platform_device
> *pdev)
> >         up.port.uartclk = clk_get_rate(priv->sclk);
> >
> >         up.port.iotype = UPIO_MEM32;
> > +
> > +       if (of_device_is_compatible(dev->of_node, "renesas,r9a09g011-
> uart"))
> > +               priv->is_rzv2m = true;
> 
> Please add an entry to serial8250_em_dt_ids[] instead, providing a feature
> flag in of_device_id.data.

OK, will add a feature flag in next version.

Cheers,
Biju

> 
> > +
> >         up.port.serial_in = serial8250_em_serial_in;
> >         up.port.serial_out = serial8250_em_serial_out;
> >         up.dl_read = serial8250_em_serial_dl_read;
> 
> 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 3/3] serial: 8250_em: Add serial8250_rzv2m_reg_update()

[Ilpo Järvinen]

On Fri, 10 Feb 2023, Biju Das wrote:

> Hi Geert,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH 3/3] serial: 8250_em: Add serial8250_rzv2m_reg_update()
> > 
> > Hi Biju,
> > 
> > On Thu, Feb 9, 2023 at 2:30 PM Biju Das <biju.das.jz@bp.renesas.com> 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 serial8250_rzv2m_reg_update() 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.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > 
> > Thanks for your patch!
> > 
> > > --- a/drivers/tty/serial/8250/8250_em.c
> > > +++ b/drivers/tty/serial/8250/8250_em.c
> > 
> > > @@ -111,6 +156,10 @@ static int serial8250_em_probe(struct platform_device
> > *pdev)
> > >         up.port.uartclk = clk_get_rate(priv->sclk);
> > >
> > >         up.port.iotype = UPIO_MEM32;
> > > +
> > > +       if (of_device_is_compatible(dev->of_node, "renesas,r9a09g011-
> > uart"))
> > > +               priv->is_rzv2m = true;
> > 
> > Please add an entry to serial8250_em_dt_ids[] instead, providing a feature
> > flag in of_device_id.data.
> 
> OK, will add a feature flag in next version.

> > > +
> > >         up.port.serial_in = serial8250_em_serial_in;
> > >         up.port.serial_out = serial8250_em_serial_out;

AFAICT, you don't need the feature flag at all. Just provide a different 
.serial_out function for this device that handles your special registers 
(and make that handler call serial8250_em_serial_out() when the write is 
into other regs than those special ones).


-- 
 i.

RE: [PATCH 3/3] serial: 8250_em: Add serial8250_rzv2m_reg_update()

[Biju Das]

Hi Ilpo,

Thanks for the feedback.

> Subject: RE: [PATCH 3/3] serial: 8250_em: Add serial8250_rzv2m_reg_update()
> 
> On Fri, 10 Feb 2023, Biju Das wrote:
> 
> > Hi Geert,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH 3/3] serial: 8250_em: Add
> > > serial8250_rzv2m_reg_update()
> > >
> > > Hi Biju,
> > >
> > > On Thu, Feb 9, 2023 at 2:30 PM Biju Das <biju.das.jz@bp.renesas.com>
> 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 serial8250_rzv2m_reg_update() 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.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/tty/serial/8250/8250_em.c
> > > > +++ b/drivers/tty/serial/8250/8250_em.c
> > >
> > > > @@ -111,6 +156,10 @@ static int serial8250_em_probe(struct
> > > > platform_device
> > > *pdev)
> > > >         up.port.uartclk = clk_get_rate(priv->sclk);
> > > >
> > > >         up.port.iotype = UPIO_MEM32;
> > > > +
> > > > +       if (of_device_is_compatible(dev->of_node,
> > > > + "renesas,r9a09g011-
> > > uart"))
> > > > +               priv->is_rzv2m = true;
> > >
> > > Please add an entry to serial8250_em_dt_ids[] instead, providing a
> > > feature flag in of_device_id.data.
> >
> > OK, will add a feature flag in next version.
> 
> > > > +
> > > >         up.port.serial_in = serial8250_em_serial_in;
> > > >         up.port.serial_out = serial8250_em_serial_out;
> 
> AFAICT, you don't need the feature flag at all. Just provide a different
> .serial_out function for this device that handles your special registers
> (and make that handler call serial8250_em_serial_out() when the write is
> into other regs than those special ones).

Agreed. Will send V2 with this change.

Cheers,
Biju