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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 13 Feb 2023, Biju Das wrote:

> Hi Jiri Slaby,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v3 3/3] serial: 8250_em: Add serial_out() to struct
> > serial8250_em_hw_info
> > 
> > On 13. 02. 23, 10:31, Biju Das wrote:
> > > So looks like similar to other macros, UART_FCR_EM (0x3) is sensible one.
> > >
> > > UART_FCR_RO_OFFSET (9)
> > > UART_FCR_RO_EM (UART_FCR_EM + UART_FCR_RO_OFFSET)
> > >
> > >
> > > static unsigned int serial8250_em_serial_in(struct uart_port *p, int
> > > offset) case UART_FCR_RO_EM:
> > > 	return readl(p->membase + (offset - UART_FCR_RO_OFFSET << 2));
> > 
> > 
> > Please send a complete patch as a reply. I am completely lost now.
> 
> Please find the complete patc.
> 
> 
> From e597ae60eb170c1f1b650e1e533bf4e12c09f822 Mon Sep 17 00:00:00 2001
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Date: Tue, 7 Feb 2023 15:07:13 +0000
> Subject: [PATCH] serial: 8250_em: Add serial_out() to struct
>  serial8250_em_hw_info
> 
> As per RZ/V2M hardware manual(Rev.1.30 Jun, 2022), UART IP has a
> restriction as mentioned below.
> 
> 40.6.1 Point for Caution when Changing the Register Settings:
> 
> When changing the settings of the following registers, a PRESETn master
> reset or FIFO reset + SW reset (FCR[2],FCR[1], HCR0[7]) must be input to
> re-initialize them.
> 
> Target Registers: FCR, LCR, MCR, DLL, DLM, HCR0.
> 
> This patch adds 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>
> ---
>  drivers/tty/serial/8250/8250_em.c | 70 ++++++++++++++++++++++++++++++-
>  1 file changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c
> index 69cd3b611501..c1c64f48ee7e 100644
> --- a/drivers/tty/serial/8250/8250_em.c
> +++ b/drivers/tty/serial/8250/8250_em.c
> @@ -17,12 +17,23 @@
>  
>  #include "8250.h"
>  
> +#define UART_FCR_EM 3
>  #define UART_DLL_EM 9
>  #define UART_DLM_EM 10
> +#define UART_HCR0_EM 11
> +
> +#define UART_FCR_R_EM	(UART_FCR_EM + UART_HCR0_EM)

It's easy to lose track of all this, IMHO this would be simple:

/*
 * A high value for UART_FCR_EM avoids overlapping with existing UART_* 
 * register defines. UART_FCR_EM_HW is the real HW register offset.
 */
#define UART_FCR_EM 12
#define UART_FCR_EM_HW 3

Then use UART_FCR_EM consistently within the function you add for RZ so 
you'd need provide both _in/_out with UART_FCR_EM. (You'll have both 
UART_FCR and UART_FCR_EM cases in _out but that seems fine, IMHO).

To me that would look the cleanest workaround to the overlapping defines.

-- 
 i.


> +#define UART_HCR0_EM_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 {
> @@ -46,6 +57,7 @@ static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
>  		fallthrough;
>  	case UART_DLL_EM: /* DLL @ 0x24 (+9) */
>  	case UART_DLM_EM: /* DLM @ 0x28 (+9) */
> +	case UART_HCR0_EM: /* HCR0 @ 0x2c */
>  		writel(value, p->membase + (offset << 2));
>  	}
>  }
> @@ -55,20 +67,74 @@ static unsigned int serial8250_em_serial_in(struct uart_port *p, int offset)
>  	switch (offset) {
>  	case UART_RX: /* RX @ 0x00 */
>  		return readb(p->membase);
> +	case UART_LCR: /* LCR @ 0x10 (+1) */
>  	case UART_MCR: /* MCR @ 0x14 (+1) */
>  	case UART_LSR: /* LSR @ 0x18 (+1) */
>  	case UART_MSR: /* MSR @ 0x1c (+1) */
>  	case UART_SCR: /* SCR @ 0x20 (+1) */
>  		return readl(p->membase + ((offset + 1) << 2));
> +	case UART_FCR_R_EM:
> +		return readl(p->membase + (UART_FCR_EM << 2));
>  	case UART_IER: /* IER @ 0x04 */
>  	case UART_IIR: /* IIR @ 0x08 */
>  	case UART_DLL_EM: /* DLL @ 0x24 (+9) */
>  	case UART_DLM_EM: /* DLM @ 0x28 (+9) */
> +	case UART_HCR0_EM: /* HCR0 @ 0x2c */
>  		return readl(p->membase + (offset << 2));
>  	}
>  	return 0;
>  }
>  
> +static void serial8250_rzv2m_reg_update(struct uart_port *p, int off, int value)
> +{
> +	unsigned int ier, fcr, lcr, mcr, hcr0;
> +
> +	ier = serial8250_em_serial_in(p, UART_IER);
> +	fcr = serial8250_em_serial_in(p, UART_FCR_R_EM);
> +	lcr = serial8250_em_serial_in(p, UART_LCR);
> +	mcr = serial8250_em_serial_in(p, UART_MCR);
> +	hcr0 = serial8250_em_serial_in(p, UART_HCR0_EM);
> +
> +	serial8250_em_serial_out(p, UART_FCR, fcr | UART_FCR_CLEAR_RCVR |
> +				 UART_FCR_CLEAR_XMIT);
> +	serial8250_em_serial_out(p, UART_HCR0_EM, hcr0 | UART_HCR0_EM_SW_RESET);
> +	serial8250_em_serial_out(p, UART_HCR0_EM, hcr0 & ~UART_HCR0_EM_SW_RESET);
> +
> +	switch (off) {
> +	case UART_FCR:
> +		fcr = value;
> +		break;
> +	case UART_LCR:
> +		lcr = value;
> +		break;
> +	case UART_MCR:
> +		mcr = value;
> +	}
> +
> +	serial8250_em_serial_out(p, UART_IER, ier);
> +	serial8250_em_serial_out(p, UART_FCR, fcr);
> +	serial8250_em_serial_out(p, UART_MCR, mcr);
> +	serial8250_em_serial_out(p, UART_LCR, lcr);
> +	serial8250_em_serial_out(p, UART_HCR0_EM, hcr0);
> +}
> +
> +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 int serial8250_em_serial_dl_read(struct uart_8250_port *up)
>  {
>  	return serial_in(up, UART_DLL_EM) | serial_in(up, UART_DLM_EM) << 8;
> @@ -120,7 +186,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 = info->serial_out;
>  	up.dl_read = serial8250_em_serial_dl_read;
>  	up.dl_write = serial8250_em_serial_dl_write;
>  
> @@ -144,11 +210,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[] = {
>