[RFC PATCH] xen/arm: Manage uart TX interrupt correctly

[RFC PATCH] xen/arm: Manage uart TX interrupt correctly

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

On pl011.c when TX interrupt is received and
TX buffer is empty, TX interrupt is not disabled and
hence UART interrupt routine see TX interrupt always
in MIS register and cpu loops infinitly.

With this patch, mask and umask TX interrupt
when required

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 xen/drivers/char/pl011.c  |   18 ++++++++++++++++++
 xen/drivers/char/serial.c |   30 +++++++++++++++++++++++++++++-
 xen/include/xen/serial.h  |    4 ++++
 3 files changed, 51 insertions(+), 1 deletion(-)

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..d2ce8a8 100644
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -76,6 +76,19 @@ 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 */
+        if ( port->driver->stop_tx != NULL )
+            port->driver->stop_tx(port);
+        spin_unlock(&port->tx_lock);
+        goto out;
+    }
+    else
+    {
+        if ( port->driver->tx_ready(port) && (port->driver->start_tx != NULL) )
+            port->driver->start_tx(port);
+    }
     for ( i = 0, n = port->driver->tx_ready(port); i < n; i++ )
     {
         if ( port->txbufc == port->txbufp )
@@ -117,6 +130,9 @@ static void __serial_putc(struct serial_port *port, char c)
                     cpu_relax();
                 if ( n > 0 )
                 {
+                    /* Enable TX before sending chars */
+                    if ( port->driver->start_tx != NULL )
+                        port->driver->start_tx(port);
                     while ( n-- )
                         port->driver->putc(
                             port,
@@ -135,6 +151,9 @@ 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 */
+            if ( port->driver->start_tx != NULL )
+                port->driver->start_tx(port);
             /* Buffer and UART FIFO are both empty, and port is available. */
             port->driver->putc(port, c);
         }
@@ -152,11 +171,18 @@ 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 */
+            if ( port->driver->start_tx != NULL )
+                port->driver->start_tx(port);
             port->driver->putc(port, c);
+        }
     }
     else
     {
         /* Simple synchronous transmitter. */
+        if ( port->driver->start_tx != NULL )
+            port->driver->start_tx(port);
         port->driver->putc(port, c);
     }
 }
@@ -403,7 +429,9 @@ void serial_start_sync(int handle)
             if ( n < 0 )
                 /* port is unavailable and might not come up until reenabled by
                    dom0, we can't really do proper sync */
-                break;
+                break; 
+            if ( port->driver->start_tx != NULL )
+                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 *);
 };
-- 
1.7.9.5

Re: [RFC PATCH] xen/arm: Manage uart TX interrupt correctly

[Julien Grall]

Hi Vijay,

You are fixing the pl011 driver, not all the UART. So the commit title
should at least contain the word "pl011".

On 06/12/14 01:42, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> On pl011.c when TX interrupt is received and

Why do you give the filename rather than the UART?

> TX buffer is empty, TX interrupt is not disabled and
> hence UART interrupt routine see TX interrupt always
> in MIS register and cpu loops infinitly.

I'm sorry to say that, but this sentence is difficult to understand.

> With this patch, mask and umask TX interrupt

s/umask/unmask

> when required

You need to explain when it's required...

> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/drivers/char/pl011.c  |   18 ++++++++++++++++++
>  xen/drivers/char/serial.c |   30 +++++++++++++++++++++++++++++-
>  xen/include/xen/serial.h  |    4 ++++
>  3 files changed, 51 insertions(+), 1 deletion(-)
> 
> 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));

The RX change seems to come from nowhere. You have to explain why you
need it in the commit message.

As you add start_tx/stop_tx, I don't think this has to be kept.

[..]

> diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
> index 44026b1..d2ce8a8 100644
> --- a/xen/drivers/char/serial.c
> +++ b/xen/drivers/char/serial.c
> @@ -76,6 +76,19 @@ 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 */
> +        if ( port->driver->stop_tx != NULL )
> +            port->driver->stop_tx(port);

Can you introduce helpers for both stop_tx and start_tx? It would avoid
to add if ( ... ) port->driver->...( ) every time.

Regards,

-- 
Julien Grall