Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Oct 2022 11:12:41 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Tony Lindgren <tony@atomide.com>,
        Peter Hurley <peter@hurleysoftware.com>,
        linux-serial <linux-serial@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] serial: 8250_omap: remove wait loop from Errata i202
 workaround
In-Reply-To: <20221013112339.2540767-1-matthias.schiffer@ew.tq-group.com>
Message-ID: <ea90b0ba-61bf-e56e-5120-9771122838cf@linux.intel.com>
References: <20221013112339.2540767-1-matthias.schiffer@ew.tq-group.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-1487723672-1665994365=:5493"
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-1487723672-1665994365=:5493
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 8BIT

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@linux.intel.com>

Thanks.

-- 
 i.


> Fixes: 61929cf0169d ("tty: serial: Add 8250-core based omap driver")
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  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,
> 

--8323329-1487723672-1665994365=:5493--