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

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

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[] = {
>