RE: [PATCH] tty: serial: fsl_lpuart: don't break the on-going transfer when global reset

[Sherry Sun]

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Sent: 2022年10月19日 17:20
> To: Sherry Sun <sherry.sun@xxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Jiri Slaby
> <jirislaby@xxxxxxxxxx>; Lukas Wunner <lukas@xxxxxxxxx>; linux-serial
> <linux-serial@xxxxxxxxxxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>; dl-
> linux-imx <linux-imx@xxxxxxx>
> Subject: Re: [PATCH] tty: serial: fsl_lpuart: don't break the on-going transfer
> when global reset
> 
> On Wed, 19 Oct 2022, Sherry Sun wrote:
> 
> > lpuart_global_reset() shouldn't break the on-going transmit engin,
> > need to recover the on-going data transfer after reset.
> >
> > This can help earlycon here, since commit 60f361722ad2 ("serial:
> > fsl_lpuart: Reset prior to registration") moved lpuart_global_reset()
> > before uart_add_one_port(), earlycon is writing during global reset,
> > as global reset will disable the TX and clear the baud rate register,
> > which caused the earlycon cannot work any more after reset, needs to
> > restore the baud rate and re-enable the transmitter to recover the
> > earlycon write.
> >
> > Fixes: bd5305dcabbc ("tty: serial: fsl_lpuart: do software reset for
> > imx7ulp and imx8qxp")
> > Signed-off-by: Sherry Sun <sherry.sun@xxxxxxx>
> > ---
> >  drivers/tty/serial/fsl_lpuart.c | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > b/drivers/tty/serial/fsl_lpuart.c index 67fa113f77d4..5064fdba1b61
> > 100644
> > --- a/drivers/tty/serial/fsl_lpuart.c
> > +++ b/drivers/tty/serial/fsl_lpuart.c
> > @@ -408,11 +408,9 @@ static int lpuart_global_reset(struct lpuart_port
> > *sport)  {
> >  	struct uart_port *port = &sport->port;
> >  	void __iomem *global_addr;
> > +	unsigned long tx_enable, bd, stat, sfifo;
> >  	int ret;
> >
> > -	if (uart_console(port))
> > -		return 0;
> > -
> >  	ret = clk_prepare_enable(sport->ipg_clk);
> >  	if (ret) {
> >  		dev_err(sport->port.dev, "failed to enable uart ipg clk: %d\n",
> > ret); @@ -420,11 +418,30 @@ static int lpuart_global_reset(struct
> lpuart_port *sport)
> >  	}
> >
> >  	if (is_imx7ulp_lpuart(sport) || is_imx8qxp_lpuart(sport)) {
> > +		/*
> > +		 * If the transmitter is used by earlycon, wait transmit engin
> complete
> > +		 * and then reset
> > +		 */
> > +		tx_enable = lpuart32_read(port, UARTCTRL) & UARTCTRL_TE;
> > +		if (tx_enable) {
> > +			bd = lpuart32_read(&sport->port, UARTBAUD);
> > +			stat = lpuart32_read(port, UARTSTAT);
> > +			sfifo = lpuart32_read(port, UARTFIFO);
> > +			while (!(stat & UARTSTAT_TC && sfifo &
> UARTFIFO_TXEMPT))
> > +				cpu_relax();
> 
> This loop, if ever taken once, will loop forever as neither stat nor sfifo are
> reread inside the loop.
> 
Hi Ilpo,

Good catch, you are right, will fix it in V2.

Best Regards
Sherry

> --
>  i.
> 
> > +		}
> > +
> >  		global_addr = port->membase + UART_GLOBAL -
> IMX_REG_OFF;
> >  		writel(UART_GLOBAL_RST, global_addr);
> >  		usleep_range(GLOBAL_RST_MIN_US, GLOBAL_RST_MAX_US);
> >  		writel(0, global_addr);
> >  		usleep_range(GLOBAL_RST_MIN_US, GLOBAL_RST_MAX_US);
> > +
> > +		/* Recover the transmitter for earlycon */
> > +		if (tx_enable) {
> > +			lpuart32_write(port, bd, UARTBAUD);
> > +			lpuart32_write(port, UARTCTRL_TE, UARTCTRL);
> > +		}
> >  	}
> >
> >  	clk_disable_unprepare(sport->ipg_clk);
> >