xen/arm: uart interrupts handling

xen/arm: uart interrupts handling

[Vijay Kilari]

Hi Tim,

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.

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));

             if ( status & (RTI|RXI) )
                 serial_rx_interrupt(port, regs);
@@ -157,7 +157,7 @@ static void pl011_resume(struct serial_port *port)
 {
     BUG(); // XXX
 }


Regards
Vijay

Re: xen/arm: uart interrupts handling

[Julien Grall]

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.

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.

Regards,

-- 
Julien Grall

Re: xen/arm: uart interrupts handling

[Tim Deegan]

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.

Re: xen/arm: uart interrupts handling

[Vijay Kilari]

Hi Tim,

On Thu, Dec 4, 2014 at 9:10 PM, Tim Deegan <tim@xen.org> wrote:
> 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.

Yes, this is the behaviour that Iam seeing. In Linux, uart driver
masks TXI interrupt
in IMSC if buffer is empty. However in xen, this scenario is not
handled. This is the reason why cpu does not come out of uart irq
routine if TX interrupt is raised but buffer is empty.

I have added below changes to fix this on top of your suggested change

diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index dd19ce8..ad48df3 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
@@ -197,6 +199,20 @@ static const struct vuart_info
*pl011_vuart(struct serial_port *port)
     return &uart->vuart;
 }

+static void pl011_tx_stop(struct serial_port *port)
+{
+    struct pl011 *uart = port->uart;
+
+    pl011_write(uart, IMSC, pl011_read(uart, IMSC) & ~(TXI));
+}
+
+static void pl011_tx_start(struct serial_port *port)
+{
+    struct pl011 *uart = port->uart;
+
+    pl011_write(uart, IMSC, pl011_read(uart, IMSC) | (TXI));
+}
+
 static struct uart_driver __read_mostly pl011_driver = {
     .init_preirq  = pl011_init_preirq,
     .init_postirq = pl011_init_postirq,
@@ -207,6 +223,8 @@ static struct uart_driver __read_mostly pl011_driver = {
     .putc         = pl011_putc,
     .getc         = pl011_getc,
     .irq          = pl011_irq,
+    .start_tx     = pl011_tx_start,
+    .stop_tx      = pl011_tx_stop,
     .vuart_info   = pl011_vuart,
 };

diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
index 44026b1..0f26d40 100644
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -76,6 +76,18 @@ void serial_tx_interrupt(struct serial_port *port,
struct cpu_user_regs *regs)
         cpu_relax();
     }

+    if ( port->txbufc == port->txbufp )
+    {
+        /* Disable TX. nothing to send */
+        port->driver->stop_tx(port);
+        spin_unlock(&port->tx_lock);
+        goto out;
+    }
+    else
+    {
+        if ( port->driver->tx_ready(port) )
+            port->driver->start_tx(port);
+    }
     for ( i = 0, n = port->driver->tx_ready(port); i < n; i++ )
     {
         if ( port->txbufc == port->txbufp )
@@ -117,6 +129,8 @@ static void __serial_putc(struct serial_port *port, char c)
                     cpu_relax();
                 if ( n > 0 )
                 {
+                    /* Enable TX before sending chars */
+                    port->driver->start_tx(port);
                     while ( n-- )
                         port->driver->putc(
                             port,
@@ -135,6 +149,8 @@ static void __serial_putc(struct serial_port *port, char c)
         if ( ((port->txbufp - port->txbufc) == 0) &&
              port->driver->tx_ready(port) > 0 )
         {
+            /* Enable TX before sending chars */
+            port->driver->start_tx(port);
             /* Buffer and UART FIFO are both empty, and port is available. */
             port->driver->putc(port, c);
         }
@@ -152,11 +168,16 @@ static void __serial_putc(struct serial_port
*port, char c)
         while ( !(n = port->driver->tx_ready(port)) )
             cpu_relax();
         if ( n > 0 )
+        {
+            /* Enable TX before sending chars */
+            port->driver->start_tx(port);
             port->driver->putc(port, c);
+        }
     }
     else
     {
         /* Simple synchronous transmitter. */
+        port->driver->start_tx(port);
         port->driver->putc(port, c);
     }
 }
@@ -404,6 +425,7 @@ void serial_start_sync(int handle)
                 /* port is unavailable and might not come up until reenabled by
                    dom0, we can't really do proper sync */
                 break;
+            port->driver->start_tx(port);
             port->driver->putc(
                 port, port->txbuf[mask_serial_txbuf_idx(port->txbufc++)]);
         }
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 9f4451b..71e6ade 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -81,6 +81,10 @@ struct uart_driver {
     int  (*getc)(struct serial_port *, char *);
     /* Get IRQ number for this port's serial line: returns -1 if none. */
     int  (*irq)(struct serial_port *);
+    /* Unmask TX interrupt */
+    void  (*start_tx)(struct serial_port *);
+    /* Mask TX interrupt */
+    void  (*stop_tx)(struct serial_port *);
     /* Get serial information */
     const struct vuart_info *(*vuart_info)(struct serial_port *);
 };

-Vijay

Re: xen/arm: uart interrupts handling

[Julien Grall]

Hi Vijay,

On 05/12/2014 00:46, Vijay Kilari wrote:
  > Yes, this is the behaviour that Iam seeing. In Linux, uart driver
> masks TXI interrupt
> in IMSC if buffer is empty. However in xen, this scenario is not
> handled. This is the reason why cpu does not come out of uart irq
> routine if TX interrupt is raised but buffer is empty.
>
> I have added below changes to fix this on top of your suggested change

Can you send a formal patch (commit message + signed-off-by)?

Also, you will have to make sure you don't break the other serial drivers.

Regards,

-- 
Julien Grall