[PATCH] serial: PL011: clear pending interrupts

[PATCH] serial: PL011: clear pending interrupts

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

Chanho Min reported that when the boot loader transfers
control to the kernel, there may be pending interrupts
causing the UART to lock up in an eternal loop trying to
pick tokens from the FIFO (since the RX interrupt flag
indicates there are tokens) while in practice there are
no tokens - in fact there is only a pending IRQ flag.

This patch address the issue with a combination of a patch
from Russell King that clears and mask all interrupts at
probe() and clears any pending error and RX interrupts at
port startup time, and a patch from Jong-Sung Kim that
clears any RX interrupts (including timeouts) even if if
there are zero tokens in the FIFO.

This way these pending interrupts should be addressed in
two ways and solidify the driver in both probe() and
IRQ paths.

Cc: stable at kernel.org
Cc: Shreshtha Kumar Sahu <shreshthakumar.sahu@stericsson.com>
Reported-by: Chanho Min <chanho0207@gmail.com>
Suggested-by: Russell King <linux@arm.linux.org.uk>
Suggested-by: Jong-Sung Kim <neidhard.kim@lge.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
OK Greg requested that we send out this combined approach, can
addressees (Chanho especially) please confirm that the patch
solves the problem? Tested on U300 and U8500.
---
 drivers/tty/serial/amba-pl011.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 6800f5f..ff3fed0 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -224,6 +224,11 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap)
 		uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag);
 	}
 
+	/* RXIS but RXFE? Just clear the interrupt */
+	if(unlikely(fifotaken == 0))
+		writew(UART011_RTIS | UART011_RXIS,
+		       uap->port.membase + UART011_ICR);
+
 	return fifotaken;
 }
 
@@ -1381,6 +1386,10 @@ static int pl011_startup(struct uart_port *port)
 
 	uap->port.uartclk = clk_get_rate(uap->clk);
 
+	/* Clear pending error and receive interrupts */
+	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS |
+	       UART011_RTIS | UART011_RXIS, uap->port.membase + UART011_ICR);
+
 	/*
 	 * Allocate the IRQ
 	 */
@@ -1417,10 +1426,6 @@ static int pl011_startup(struct uart_port *port)
 	cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
 	writew(cr, uap->port.membase + UART011_CR);
 
-	/* Clear pending error interrupts */
-	writew(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS,
-	       uap->port.membase + UART011_ICR);
-
 	/*
 	 * initialise the old status of the modem signals
 	 */
@@ -1927,6 +1932,10 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 		goto unmap;
 	}
 
+	/* Ensure interrupts from this UART are masked and cleared */
+	writew(0, uap->port.membase + UART011_IMSC);
+	writew(0xffff, uap->port.membase + UART011_ICR);
+
 	uap->vendor = vendor;
 	uap->lcrh_rx = vendor->lcrh_rx;
 	uap->lcrh_tx = vendor->lcrh_tx;
-- 
1.7.8

[PATCH] serial: PL011: clear pending interrupts

[Russell King - ARM Linux]

On Mon, Mar 12, 2012 at 09:25:50AM +0100, Linus Walleij wrote:
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 6800f5f..ff3fed0 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -224,6 +224,11 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap)
>  		uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag);
>  	}
>  

What if we really do end up receiving some characters here?

> +	/* RXIS but RXFE? Just clear the interrupt */
> +	if(unlikely(fifotaken == 0))
> +		writew(UART011_RTIS | UART011_RXIS,
> +		       uap->port.membase + UART011_ICR);

[PATCH] serial: PL011: clear pending interrupts

[James Courtier-Dutton]

On 12 March 2012 08:25, Linus Walleij <linus.walleij@stericsson.com> wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
>
> Chanho Min reported that when the boot loader transfers
> control to the kernel, there may be pending interrupts
> causing the UART to lock up in an eternal loop trying to
> pick tokens from the FIFO (since the RX interrupt flag
> indicates there are tokens) while in practice there are
> no tokens - in fact there is only a pending IRQ flag.
>

I might be off base here, because I don't know this particular UART,
but if an IRQ routine is called because the RX interrupt flag is set,
shouldn't the IRQ routine be written in such a way that it can never
lock up.
If the IRQ routing has a loop that is used to read each byte from the
port, the loop should have a counter limit in it, so in case of some
unexpected situation, it can always make an exit, and not lock up.
So, in your case, if the loop counter limit is hit, it would mean that
although we had an RX interrupt flag set, there are no tokens, so act
according with a suitable error path.

I had to implement this in one Linux kernel driver due to an error
condition that happened on PCMCIA card insertion.
On the first insertion, there were unwanted IRQ calls, but after the
second false IRQ call, it calmed down. I used a counter limit in the
IRQ driver in order to handle the false IRQ calls situation.

Kind Regards

James

[PATCH] serial: PL011: clear pending interrupts

[Russell King - ARM Linux]

On Mon, Mar 12, 2012 at 09:27:28AM +0000, James Courtier-Dutton wrote:
> On 12 March 2012 08:25, Linus Walleij <linus.walleij@stericsson.com> wrote:
> > From: Linus Walleij <linus.walleij@linaro.org>
> >
> > Chanho Min reported that when the boot loader transfers
> > control to the kernel, there may be pending interrupts
> > causing the UART to lock up in an eternal loop trying to
> > pick tokens from the FIFO (since the RX interrupt flag
> > indicates there are tokens) while in practice there are
> > no tokens - in fact there is only a pending IRQ flag.
> >
> 
> I might be off base here, because I don't know this particular UART,
> but if an IRQ routine is called because the RX interrupt flag is set,
> shouldn't the IRQ routine be written in such a way that it can never
> lock up.

The problem is that there are two flags:

1. The receiver interrupt status flags
2. The fifo status flags

It appears that there's a condition where the receiver interrupt flags can
indicate that characters are pending, but the status flags say that there
are no characters pending in the receive buffer.

This occurs because it seems there's a hardware bug when switching from
FIFO mode to non-FIFO mode - this causes previously received characters
to be hidden from the FIFO status flags.

[PATCH] serial: PL011: clear pending interrupts

[Russell King - ARM Linux]

On Mon, Mar 12, 2012 at 08:32:21AM +0000, Russell King - ARM Linux wrote:
> On Mon, Mar 12, 2012 at 09:25:50AM +0100, Linus Walleij wrote:
> > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > index 6800f5f..ff3fed0 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -224,6 +224,11 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap)
> >  		uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag);
> >  	}
> >  
> 
> What if we really do end up receiving some characters here?
> 
> > +	/* RXIS but RXFE? Just clear the interrupt */
> > +	if(unlikely(fifotaken == 0))
> > +		writew(UART011_RTIS | UART011_RXIS,
> > +		       uap->port.membase + UART011_ICR);

BTW, I don't see why we need any of this stuff.

The problem as described is that the interrupt handler is called with one
of the receive flags set (UART011_RTIS or UART011_RXIS) but with
UART01x_FR_RXFE clear.

The problem report indicates that this is caused when we switch from FIFO
mode to non-FIFO mode, and the FIFO contained some unread characters.

That can only happen once we've enabled interrupts in the IMSC register,
which doesn't happen until later in the startup function.

So:

        spin_lock_irq(&uap->port.lock);
        uap->im = UART011_RTIM;
        if (!pl011_dma_rx_running(uap))
                uap->im |= UART011_RXIM;
        writew(uap->im, uap->port.membase + UART011_IMSC);
        spin_unlock_irq(&uap->port.lock);

clearing the transmit/receive interrupts here _before_ we unmask them
should be sufficient.

As we never turn the FIFO off after that point, we can't then cause the
condition.  So I don't see any point in adding stuff to the interrupt
handling for this.

[PATCH] serial: PL011: clear pending interrupts

[Linus Walleij]

On Mon, Mar 12, 2012 at 11:11 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

>> > + ? /* RXIS but RXFE? Just clear the interrupt */
>> > + ? if(unlikely(fifotaken == 0))
>> > + ? ? ? ? ? writew(UART011_RTIS | UART011_RXIS,
>> > + ? ? ? ? ? ? ? ? ?uap->port.membase + UART011_ICR);
>
> BTW, I don't see why we need any of this stuff.
>
> The problem as described is that the interrupt handler is called with one
> of the receive flags set (UART011_RTIS or UART011_RXIS) but with
> UART01x_FR_RXFE clear.
>
> The problem report indicates that this is caused when we switch from FIFO
> mode to non-FIFO mode, and the FIFO contained some unread characters.
>
> That can only happen once we've enabled interrupts in the IMSC register,
> which doesn't happen until later in the startup function.
>
> So:
>
> ? ? ? ?spin_lock_irq(&uap->port.lock);
> ? ? ? ?uap->im = UART011_RTIM;
> ? ? ? ?if (!pl011_dma_rx_running(uap))
> ? ? ? ? ? ? ? ?uap->im |= UART011_RXIM;
> ? ? ? ?writew(uap->im, uap->port.membase + UART011_IMSC);
> ? ? ? ?spin_unlock_irq(&uap->port.lock);
>
> clearing the transmit/receive interrupts here _before_ we unmask them
> should be sufficient.

Right! OK I'll cook a v2 patch according to this suggestion and
request Chanho to test it.

Yours,
Linus Walleij

[PATCH] serial: PL011: clear pending interrupts

[Kim, Jong-Sung]

> > diff --git a/drivers/tty/serial/amba-pl011.c
> > b/drivers/tty/serial/amba-pl011.c index 6800f5f..ff3fed0 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -224,6 +224,11 @@ static int pl011_fifo_to_tty(struct uart_amba_port
> *uap)
> >  		uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag);
> >  	}
> >
> 
> What if we really do end up receiving some characters here?

Maybe the character is handled on next upcoming RTIS or RXIS in normal.
However, I agree with you there is possibility of already asserted RXIS at
this point. (with debugger?) In such a case, we may lose Rx interrupts
forever because it's not level-triggered, and the ISR lacks error interrupt
handling. Correct me if something wrong.
Thank you for your comment, Russell.

> 
> > +	/* RXIS but RXFE? Just clear the interrupt */
> > +	if(unlikely(fifotaken == 0))
> > +		writew(UART011_RTIS | UART011_RXIS,
> > +		       uap->port.membase + UART011_ICR);

[PATCH] serial: PL011: clear pending interrupts

[Russell King - ARM Linux]

On Mon, Mar 12, 2012 at 08:22:07PM +0900, Kim, Jong-Sung wrote:
> Maybe the character is handled on next upcoming RTIS or RXIS in normal.
> However, I agree with you there is possibility of already asserted RXIS at
> this point. (with debugger?) In such a case, we may lose Rx interrupts
> forever because it's not level-triggered, and the ISR lacks error interrupt
> handling. Correct me if something wrong.

The handler does not lack error handling - error handling is associated
with a character, and we deal with errors when we read the associated
character from the receive register.

So, we have no need for the error interrupts.

The PL011, as with all serial drivers I write, has full error handling
incorporated.

[PATCH] serial: PL011: clear pending interrupts

[Kim, Jong-Sung]

> On Mon, Mar 12, 2012 at 08:32:21AM +0000, Russell King - ARM Linux wrote:
> > On Mon, Mar 12, 2012 at 09:25:50AM +0100, Linus Walleij wrote:
> > > diff --git a/drivers/tty/serial/amba-pl011.c
> > > b/drivers/tty/serial/amba-pl011.c index 6800f5f..ff3fed0 100644
> > > --- a/drivers/tty/serial/amba-pl011.c
> > > +++ b/drivers/tty/serial/amba-pl011.c
> > > @@ -224,6 +224,11 @@ static int pl011_fifo_to_tty(struct
uart_amba_port
> *uap)
> > >  		uart_insert_char(&uap->port, ch, UART011_DR_OE, ch, flag);
> > >  	}
> > >
> >
> > What if we really do end up receiving some characters here?
> >
> > > +	/* RXIS but RXFE? Just clear the interrupt */
> > > +	if(unlikely(fifotaken == 0))
> > > +		writew(UART011_RTIS | UART011_RXIS,
> > > +		       uap->port.membase + UART011_ICR);
> 
> BTW, I don't see why we need any of this stuff.

Actually, I meant my patch applied alone, not with your patch. My thought
was the problem we're trying to handle is undocumented and is not guaranteed
not to happen again.