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

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

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.