Re: [Xen-devel] xen/arm: uart interrupts handling

[Tim Deegan <tim@xxxxxxx>]

At 13:51 +0000 on 04 Dec (1417697518), Julien Grall wrote:
> On 04/12/14 03:50, Vijay Kilari wrote:
> > Hi Tim,
> 
> Hi Vijay,
> 
> > I see that on uart interrupt, ICR is written to clear the all
> > interrupts except TX, RX and RX timeout. With this, cpu always finds
> > TX/RX is active and never
> > comes out of the loop.
> 
> FWIW, the PL011 serial code has been copied from the Linux drivers.
> 
> Linux interrupt handler also clear all interrupts except TX, RX, RX
> timeout. So do you see the issue on Linux?
> 
> > 
> > With the below changes, TX, RX & RTI are cleared before handling this
> > interrupts.
> > 
> > Is my observation is correct?. If so I wonder how it is working on
> > platforms that
> > are using pl011. Without this for my cpu just keeps looping here.
> > 
> >   index fba0a55..d21bce3 100644
> > --- a/xen/drivers/char/pl011.c
> > +++ b/xen/drivers/char/pl011.c
> > @@ -63,7 +63,7 @@ static void pl011_interrupt(int irq, void *data,
> > struct cpu_user_regs *regs)
> >      {
> >          do
> >          {
> > -            pl011_write(uart, ICR, status & ~(TXI|RTI|RXI));
> > +            pl011_write(uart, ICR, status & (TXI|RTI|RXI));
> 
> 
> This changes looks wrong to me. We want to clear the bit in status we
> don't handle. Otherwise the interrupt will be fired in loop.

Yes - even if we do want to explicitly clear the TXI|RTI|RXI bits, we
must clear the others too!

> If I'm not mistaken, TXI/RTI/RXI will be cleared when data is read or
> write into the fifo. So we should not clear automatically.

I've been looking at that and I think it's OK for the RX path -- we
ought to keep calling serial_rx_interrupt() until we've drained the
FIFO, at which point both RTI and RXI will be cleared.  Certainly we
shouldn't unconditionally clear RXI/RTI without somehow making sure
that we're not leaving bytes in the FIFO.

The TX path looks more suspect -- if the uart raises TXI and we have
nothing buffered to send, then TXI won't get cleared.
Still, I wonder why you're seeing this and other people haven't.

And again, we ought not to just clear the interrupt:
serial_tx_interrupt might not send any bytes (and indeed by my reading
it won't do anything unless the FIFO is actually empty), in which case
we need to keep something around to tell us to try again.

Actually, looking at the current behaviour, I wonder whether we ought
to push the interrupt trigger back from firing at half-empty.  Like so:

diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index dd19ce8..2e97234 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -109,6 +109,8 @@ static void __init pl011_init_preirq(struct
serial_port *port)
             panic("pl011: No Baud rate configured\n");
         uart->baud = (uart->clock_hz << 2) / divisor;
     }
+    /* Trigger RX interrupt at 1/2 full, TX interrupt at 7/8 empty */
+    pl011_write(uart, IFLS, (2<<3 | 0));
     /* This write must follow FBRD and IBRD writes. */
     pl011_write(uart, LCR_H, (uart->data_bits - 5) << 5
                             | FEN


Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel