[PATCH 0/3] serial: 8250-fsl: Two fixes

[PATCH 0/3] serial: 8250-fsl: Two fixes

[Uwe Kleine-König]

Hello,

after better describing the problem in FSL's 8250 variant, here come two
fixes needed to reliably use RS485 on this type of UART.

The patches were originally developed and tested on top of 5.15.y.

Best regards
Uwe

Uwe Kleine-König (3):
  serial: 8250-fsl: Expand description of the MPC83xx UART's
    misbehaviour
  serial: 8250: Clear IIR.RDI in serial8250_clear_fifos()
  serial: 8250-fsl: Only do the break workaround if IIR signals RLSI

 drivers/tty/serial/8250/8250_fsl.c  | 17 +++++++++++++++--
 drivers/tty/serial/8250/8250_port.c | 12 ++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

base-commit: ac9a78681b921877518763ba0e89202254349d1b
-- 
2.39.2

[PATCH 1/3] serial: 8250-fsl: Expand description of the MPC83xx UART's misbehaviour

[Uwe Kleine-König]

After working quite a bit on erratic behaviour of the MPC83xx UART I
(think I) understood the problem. Expand the description accoringly to
conserve the knowledge for the future.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/serial/8250/8250_fsl.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
index 8adfaa183f77..00f46b9a8b09 100644
--- a/drivers/tty/serial/8250/8250_fsl.c
+++ b/drivers/tty/serial/8250/8250_fsl.c
@@ -38,7 +38,19 @@ int fsl8250_handle_irq(struct uart_port *port)
 		return 0;
 	}
 
-	/* This is the WAR; if last event was BRK, then read and return */
+	/*
+	 * For a single break the hardware reports LSR.BI for each character
+	 * time. This is described in the MPC8313E chip errata as "General17".
+	 * A typical break has a duration of 0.3s, with a 115200n8 configuration
+	 * that (theoretically) corresponds to ~3500 interrupts in these 0.3s.
+	 * In practise it's less (around 500) because of hardware
+	 * and software latencies. The workaround recommended by the vendor is
+	 * to read the RX register (to clear LSR.DR and thus prevent a FIFO
+	 * aging interrupt). To prevent the irq from retriggering LSR must not be
+	 * read. (This would clear LSR.BI, hardware would reassert the BI event
+	 * immediately and interrupt the CPU again. The hardware clears LSR.BI
+	 * when the next valid char is read.)
+	 */
 	if (unlikely(up->lsr_saved_flags & UART_LSR_BI)) {
 		up->lsr_saved_flags &= ~UART_LSR_BI;
 		port->serial_in(port, UART_RX);
-- 
2.39.2

[PATCH 2/3] serial: 8250: Clear IIR.RDI in serial8250_clear_fifos()

[Uwe Kleine-König]

At least on MPC83xx (but I suspect also on other implementations) it can
happen that after irqs are disabled but before the FIFOs are cleared a
character is received. Resetting the FIFO throws away that character,
but doesn't clear the IIR.RDI event that signals that there is read data
available.

Read from RX to clear IIR.RDI and throw away the result.

This fixes a infinite loop after the above described race happened: The
irq handler (here: fsl8250_handle_irq()) triggers, but LSR.DR isn't set,
so RX isn't read and the irq handler returns just to be called again.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/serial/8250/8250_port.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index fe8d79c4ae95..8b47ec000922 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -556,6 +556,18 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
 		serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO |
 			       UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
 		serial_out(p, UART_FCR, 0);
+
+		/*
+		 * When doing rs485 on MPC8313 it can happen that in the short
+		 * window when switching from RX to TX there is a character in
+		 * the RX FIFO which isn't processed any more and then discarded
+		 * in here by clearing the FIFO. In that case IIR signals an RX
+		 * timeout once irqs are reenabled (i.e. in
+		 * serial8250_em485_stop_tx()) but LSR shows no pending event.
+		 * The RX timeout event can only be cleared by reading RX. Do
+		 * this here before reenabling the FIFO and irqs.
+		 */
+		serial_port_in(&p->port, UART_RX);
 	}
 }
 
-- 
2.39.2

[PATCH 3/3] serial: 8250-fsl: Only do the break workaround if IIR signals RLSI

[Uwe Kleine-König]

It can happen that while a break is received the transmitter gets empty
and IIR signals a Transmitter holding register empty (THRI) event. In
this case it's too early for the break workaround. Still doing it then
results in the THRI event not being rereported which makes the driver
miss that and e.g. for RS485 half-duplex communication it fails to
switch back to RX mode.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/serial/8250/8250_fsl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
index 00f46b9a8b09..c12a4b4bd0f4 100644
--- a/drivers/tty/serial/8250/8250_fsl.c
+++ b/drivers/tty/serial/8250/8250_fsl.c
@@ -51,7 +51,8 @@ int fsl8250_handle_irq(struct uart_port *port)
 	 * immediately and interrupt the CPU again. The hardware clears LSR.BI
 	 * when the next valid char is read.)
 	 */
-	if (unlikely(up->lsr_saved_flags & UART_LSR_BI)) {
+	if (unlikely((iir & UART_IIR_ID) == UART_IIR_RLSI &&
+		     (up->lsr_saved_flags & UART_LSR_BI))) {
 		up->lsr_saved_flags &= ~UART_LSR_BI;
 		port->serial_in(port, UART_RX);
 		spin_unlock_irqrestore(&up->port.lock, flags);
-- 
2.39.2

Re: [PATCH 2/3] serial: 8250: Clear IIR.RDI in serial8250_clear_fifos()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3004 bytes --]

On Wed, 24 May 2023, Uwe Kleine-König wrote:

> At least on MPC83xx (but I suspect also on other implementations) it can
> happen that after irqs are disabled but before the FIFOs are cleared a
> character is received. Resetting the FIFO throws away that character,
> but doesn't clear the IIR.RDI event that signals that there is read data
> available.
> 
> Read from RX to clear IIR.RDI and throw away the result.
> 
> This fixes a infinite loop after the above described race happened: The
> irq handler (here: fsl8250_handle_irq()) triggers, but LSR.DR isn't set,
> so RX isn't read and the irq handler returns just to be called again.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/tty/serial/8250/8250_port.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index fe8d79c4ae95..8b47ec000922 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -556,6 +556,18 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>  		serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO |
>  			       UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
>  		serial_out(p, UART_FCR, 0);
> +
> +		/*
> +		 * When doing rs485 on MPC8313 it can happen that in the short
> +		 * window when switching from RX to TX there is a character in
> +		 * the RX FIFO which isn't processed any more and then discarded
> +		 * in here by clearing the FIFO. In that case IIR signals an RX
> +		 * timeout once irqs are reenabled (i.e. in
> +		 * serial8250_em485_stop_tx()) but LSR shows no pending event.
> +		 * The RX timeout event can only be cleared by reading RX. Do
> +		 * this here before reenabling the FIFO and irqs.
> +		 */
> +		serial_port_in(&p->port, UART_RX);
>  	}
>  }

This solution has too wide impact, I think. It should be made driver 
specific. Can't you read IIR to see if the event indication is there 
before doing this UART_RX read? Maybe add an UART specific function for 
fifo clearing/reset.

I've long wondered this related thing:

Does anyone have idea why serial8250_clear_and_reinit_fifos() and 
serial8250_clear_fifos() are separate, what is the benefit of not setting 
FCR back to up->fcr? That is that intermediate FCR <= 0 really required 
for the FIFO reset sequence or is it just an artifact of how the code is 
structured into those two functions.

It might make sense to drop serial8250_clear_and_reinit_fifos() and 
change serial8250_clear_fifos() into something like this (depending on 
the answers):

static void serial8250_clear_fifos(struct uart_8250_port *p, bool disable)
{
        if (p->capabilities & UART_CAP_FIFO) {
                serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO);
                serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO |
                               UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
	        serial_out(p, UART_FCR, disable ? 0 : p->fcr);
        }
}


-- 
 i.

Re: [PATCH 2/3] serial: 8250: Clear IIR.RDI in serial8250_clear_fifos()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4162 bytes --]

On Wed, May 24, 2023 at 04:02:58PM +0300, Ilpo Järvinen wrote:
> On Wed, 24 May 2023, Uwe Kleine-König wrote:
> 
> > At least on MPC83xx (but I suspect also on other implementations) it can
> > happen that after irqs are disabled but before the FIFOs are cleared a
> > character is received. Resetting the FIFO throws away that character,
> > but doesn't clear the IIR.RDI event that signals that there is read data
> > available.
> > 
> > Read from RX to clear IIR.RDI and throw away the result.
> > 
> > This fixes a infinite loop after the above described race happened: The
> > irq handler (here: fsl8250_handle_irq()) triggers, but LSR.DR isn't set,
> > so RX isn't read and the irq handler returns just to be called again.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/tty/serial/8250/8250_port.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index fe8d79c4ae95..8b47ec000922 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -556,6 +556,18 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
> >  		serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO |
> >  			       UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
> >  		serial_out(p, UART_FCR, 0);
> > +
> > +		/*
> > +		 * When doing rs485 on MPC8313 it can happen that in the short
> > +		 * window when switching from RX to TX there is a character in
> > +		 * the RX FIFO which isn't processed any more and then discarded
> > +		 * in here by clearing the FIFO. In that case IIR signals an RX
> > +		 * timeout once irqs are reenabled (i.e. in
> > +		 * serial8250_em485_stop_tx()) but LSR shows no pending event.
> > +		 * The RX timeout event can only be cleared by reading RX. Do
> > +		 * this here before reenabling the FIFO and irqs.
> > +		 */
> > +		serial_port_in(&p->port, UART_RX);
> >  	}
> >  }
> 
> This solution has too wide impact, I think.

What is the impact? After the FIFO is reset reading the RX register
shouldn't matter?

> It should be made driver specific.

I'm not a big fan, the 8250 driver is already very fragmented.

> Can't you read IIR to see if the event indication is there before
> doing this UART_RX read?

Assuming reading IIR and reading RX take approx the same amount of
time, I don't see an upside of checking IIR first?!

> Maybe add an UART specific function for fifo clearing/reset.

See above. And with the theory that reading RX doesn't hurt after the
FIFO was just cleared, adding this to generic code has the upside that
other variants that might have the same issue get fixed, too.

> I've long wondered this related thing:
> 
> Does anyone have idea why serial8250_clear_and_reinit_fifos() and 
> serial8250_clear_fifos() are separate, what is the benefit of not setting 
> FCR back to up->fcr? That is that intermediate FCR <= 0 really required 
> for the FIFO reset sequence or is it just an artifact of how the code is 
> structured into those two functions.
> 
> It might make sense to drop serial8250_clear_and_reinit_fifos() and 
> change serial8250_clear_fifos() into something like this (depending on 
> the answers):
> 
> static void serial8250_clear_fifos(struct uart_8250_port *p, bool disable)
> {
>         if (p->capabilities & UART_CAP_FIFO) {
>                 serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO);
>                 serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO |
>                                UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
> 	        serial_out(p, UART_FCR, disable ? 0 : p->fcr);
>         }
> }

I'd say this should work. Apart from skipping

	serial_out(p, UART_FCR, 0);

it has the side effect of skipping

	serial_out(p, UART_FCR, p->fcr);

if !(p->capabilities & UART_CAP_FIFO). That shouldn't matter though.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] serial: 8250: Clear IIR.RDI in serial8250_clear_fifos()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4729 bytes --]

On Wed, 24 May 2023, Uwe Kleine-König wrote:

> On Wed, May 24, 2023 at 04:02:58PM +0300, Ilpo Järvinen wrote:
> > On Wed, 24 May 2023, Uwe Kleine-König wrote:
> > 
> > > At least on MPC83xx (but I suspect also on other implementations) it can
> > > happen that after irqs are disabled but before the FIFOs are cleared a
> > > character is received. Resetting the FIFO throws away that character,
> > > but doesn't clear the IIR.RDI event that signals that there is read data
> > > available.
> > > 
> > > Read from RX to clear IIR.RDI and throw away the result.
> > > 
> > > This fixes a infinite loop after the above described race happened: The
> > > irq handler (here: fsl8250_handle_irq()) triggers, but LSR.DR isn't set,
> > > so RX isn't read and the irq handler returns just to be called again.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/tty/serial/8250/8250_port.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > > index fe8d79c4ae95..8b47ec000922 100644
> > > --- a/drivers/tty/serial/8250/8250_port.c
> > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > @@ -556,6 +556,18 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
> > >  		serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO |
> > >  			       UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
> > >  		serial_out(p, UART_FCR, 0);
> > > +
> > > +		/*
> > > +		 * When doing rs485 on MPC8313 it can happen that in the short
> > > +		 * window when switching from RX to TX there is a character in
> > > +		 * the RX FIFO which isn't processed any more and then discarded
> > > +		 * in here by clearing the FIFO. In that case IIR signals an RX
> > > +		 * timeout once irqs are reenabled (i.e. in
> > > +		 * serial8250_em485_stop_tx()) but LSR shows no pending event.
> > > +		 * The RX timeout event can only be cleared by reading RX. Do
> > > +		 * this here before reenabling the FIFO and irqs.
> > > +		 */
> > > +		serial_port_in(&p->port, UART_RX);
> > >  	}
> > >  }
> > 
> > This solution has too wide impact, I think.
> 
> What is the impact? After the FIFO is reset reading the RX register
> shouldn't matter?
> 
> > It should be made driver specific.
> 
> I'm not a big fan, the 8250 driver is already very fragmented.
> 
> > Can't you read IIR to see if the event indication is there before
> > doing this UART_RX read?
> 
> Assuming reading IIR and reading RX take approx the same amount of
> time, I don't see an upside of checking IIR first?!
> 
> > Maybe add an UART specific function for fifo clearing/reset.
> 
> See above. And with the theory that reading RX doesn't hurt after the
> FIFO was just cleared, adding this to generic code has the upside that
> other variants that might have the same issue get fixed, too.

See e.g. 4f4e670342b14f302e17c93bd22fc943bbaaf1de. I guess you could argue 
since FIFO is not just cleared at that point but also disabled it won't 
trigger that particular problem but I'd prefer to not depend on that given 
I'd want moving into the direction where the unnecessary looking FIFO 
disable does not occur (as was discussed below).

> > I've long wondered this related thing:
> > 
> > Does anyone have idea why serial8250_clear_and_reinit_fifos() and 
> > serial8250_clear_fifos() are separate, what is the benefit of not setting 
> > FCR back to up->fcr? That is that intermediate FCR <= 0 really required 
> > for the FIFO reset sequence or is it just an artifact of how the code is 
> > structured into those two functions.
> > 
> > It might make sense to drop serial8250_clear_and_reinit_fifos() and 
> > change serial8250_clear_fifos() into something like this (depending on 
> > the answers):
> > 
> > static void serial8250_clear_fifos(struct uart_8250_port *p, bool disable)
> > {
> >         if (p->capabilities & UART_CAP_FIFO) {
> >                 serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO);
> >                 serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO |
> >                                UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
> > 	        serial_out(p, UART_FCR, disable ? 0 : p->fcr);
> >         }
> > }
> 
> I'd say this should work. Apart from skipping
> 
> 	serial_out(p, UART_FCR, 0);
> 
> it has the side effect of skipping
> 
> 	serial_out(p, UART_FCR, p->fcr);
> 
> if !(p->capabilities & UART_CAP_FIFO). That shouldn't matter though.

Yes, but I'd expect clear fifos to be no-op when FIFO is not enabled so 
it's better IMHO.

Thanks foy your thoughts on this. Maybe I'll spend the time to dig through 
all the related history about it to see if there's something interesting 
to find.


-- 
 i.