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

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2014-12-09 at 10:09 +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> In pl011.c, when TX interrupt is received
> serial_tx_interrupt() is called to push next
> characters. If TX buffer is empty, serial_tx_interrupt()
> does not disable TX interrupt and hence pl011 UART
> irq handler pl011_interrupt() always sees TX interrupt
> status set in MIS register and cpu does not come out of
> UART irq handler.
> 
> With this patch, mask TX interrupt by writing 0 to
> IMSC register when TX buffer is empty and unmask by
> writing 1 to IMSC register before sending characters.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/drivers/char/pl011.c  |   16 ++++++++++++++++
>  xen/drivers/char/serial.c |   34 ++++++++++++++++++++++++++++++++++
>  xen/include/xen/serial.h  |    4 ++++

These last two changes require that you cc the common serial maintainer,
not just the ARM maintainers. In this case that means Keir, who I have
CCd.

./scripts/get_maintainers.pl can help automate this.

>  3 files changed, 54 insertions(+)
> 
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index dd19ce8..57274d9 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -197,6 +197,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 +221,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..c583a48 100644
> --- a/xen/drivers/char/serial.c
> +++ b/xen/drivers/char/serial.c
> @@ -31,6 +31,18 @@ static struct serial_port com[SERHND_IDX + 1] = {
>  
>  static bool_t __read_mostly post_irq;
>  
> +static inline void serial_start_tx(struct serial_port *port)
> +{
> +    if ( port->driver->start_tx != NULL )
> +        port->driver->start_tx(port);
> +}
> +
> +static inline void serial_stop_tx(struct serial_port *port)
> +{
> +    if ( port->driver->stop_tx != NULL )
> +        port->driver->stop_tx(port);
> +}
> +
>  void serial_rx_interrupt(struct serial_port *port, struct cpu_user_regs *regs)
>  {
>      char c;
> @@ -76,6 +88,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 */
> +        serial_stop_tx(port);
> +        spin_unlock(&port->tx_lock);
> +        goto out;
> +    }
> +    else
> +    {
> +        if ( port->driver->tx_ready(port) )
> +            serial_start_tx(port);
> +    }
>      for ( i = 0, n = port->driver->tx_ready(port); i < n; i++ )
>      {
>          if ( port->txbufc == port->txbufp )
> @@ -117,6 +141,8 @@ static void __serial_putc(struct serial_port *port, char c)
>                      cpu_relax();
>                  if ( n > 0 )
>                  {
> +                    /* Enable TX before sending chars */
> +                    serial_start_tx(port);
>                      while ( n-- )
>                          port->driver->putc(
>                              port,
> @@ -135,6 +161,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 */
> +            serial_start_tx(port);
>              /* Buffer and UART FIFO are both empty, and port is available. */
>              port->driver->putc(port, c);
>          }
> @@ -152,11 +180,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 */
> +            serial_start_tx(port);
>              port->driver->putc(port, c);
> +        }
>      }
>      else
>      {
>          /* Simple synchronous transmitter. */
> +        serial_start_tx(port);
>          port->driver->putc(port, c);
>      }
>  }
> @@ -404,6 +437,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;
> +            serial_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 *);
>  };