Re: [PATCH] serial: 8250_omap: remove wait loop from Errata i202 workaround

[Ilpo Järvinen]

On Thu, 13 Oct 2022, Matthias Schiffer wrote:

> We were occasionally seeing the "Errata i202: timedout" on an AM335x
> board when repeatedly opening and closing a UART connected to an active
> sender. As new input may arrive at any time, it is possible to miss the
> "RX FIFO empty" condition, forcing the loop to wait until it times out.

I can see this problem could occur and why your patch fixes it.

> Nothing in the i202 Advisory states that such a wait is even necessary;
> other FIFO clear functions like serial8250_clear_fifos() do not wait
> either. For this reason, it seems safe to remove the wait, fixing the
> mentioned issue.

Checking the commit that added this driver and the loop along with it, 
there was no information why it would be needed there either.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>

Thanks.

-- 
 i.


> Fixes: 61929cf0169d ("tty: serial: Add 8250-core based omap driver")
> Signed-off-by: Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 17 -----------------
>  1 file changed, 17 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 41b8c6b27136..484f791617af 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -193,27 +193,10 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  static void omap_8250_mdr1_errataset(struct uart_8250_port *up,
>  				     struct omap8250_priv *priv)
>  {
> -	u8 timeout = 255;
> -
>  	serial_out(up, UART_OMAP_MDR1, priv->mdr1);
>  	udelay(2);
>  	serial_out(up, UART_FCR, up->fcr | UART_FCR_CLEAR_XMIT |
>  			UART_FCR_CLEAR_RCVR);
> -	/*
> -	 * Wait for FIFO to empty: when empty, RX_FIFO_E bit is 0 and
> -	 * TX_FIFO_E bit is 1.
> -	 */
> -	while (UART_LSR_THRE != (serial_in(up, UART_LSR) &
> -				(UART_LSR_THRE | UART_LSR_DR))) {
> -		timeout--;
> -		if (!timeout) {
> -			/* Should *never* happen. we warn and carry on */
> -			dev_crit(up->port.dev, "Errata i202: timedout %x\n",
> -				 serial_in(up, UART_LSR));
> -			break;
> -		}
> -		udelay(1);
> -	}
>  }
>  
>  static void omap_8250_get_divisor(struct uart_port *port, unsigned int baud,
>