diff mbox series

[Xen-devel,v3,2/2] arm/xen: vpl011: Fix SBSA UART interrupt assertion

Message ID 20171024170922.17207-3-andre.przywara@linaro.org
State New
Headers show
Series Fix SBSA UART emulation | expand

Commit Message

Andre Przywara Oct. 24, 2017, 5:09 p.m. UTC 
From: Bhupinder Thakur <bhupinder.thakur@linaro.org>

With the current SBSA UART emulation, streaming larger amounts of data
(as caused by "find /", for instance) can lead to character loses.
This is due to the OUT ring buffer getting full, because we change the
TX interrupt bit only when the FIFO is actually full, and not already
when it's half-way filled, as the Linux driver expects.
The SBSA spec does not explicitly state this, but we assume that an
SBSA compliant UART uses the PL011 default "interrupt FIFO level select
register" value of "1/2 way". The Linux driver certainly makes this
assumption, so it expect to be able to write a number of characters
after the TX interrupt line has been asserted.
On a similar issue we have the same wrong behaviour on the receive side.
However changing the RX interrupt to trigger on reaching half of the FIFO
level will lead to lag, because the guest would not be notified of incoming
characters until the FIFO is half way filled. This leads to inacceptible
lags when typing on a terminal.
Real hardware solves this issue by using the "receive timeout
interrupt" (RTI), which is triggered when character reception stops for
32 baud cycles. As we cannot and do not want to emulate any timing here,
we slightly abuse the timeout interrupt to notify the guest of new
characters: when a new character comes in, the RTI is asserted, when
the FIFO is cleared, the interrupt gets cleared.

So this patch changes the emulated interrupt trigger behaviour to come
as close to real hardware as possible: the RX and TX interrupt trigger
when the FIFO gets half full / half empty, and the RTI interrupt signals
new incoming characters.

[Andre: reword commit message, introduce receive timeout interrupt, add
        comments]

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
Reviewed-by: Andre Przywara <andre.przywara@linaro.org>
Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/vpl011.c        | 131 ++++++++++++++++++++++++++++++-------------
 xen/include/asm-arm/vpl011.h |   2 +
 2 files changed, 94 insertions(+), 39 deletions(-)

Comments

Stefano Stabellini Oct. 25, 2017, 11:50 p.m. UTC | #1 
On Tue, 24 Oct 2017, Andre Przywara wrote:
> From: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> 
> With the current SBSA UART emulation, streaming larger amounts of data
> (as caused by "find /", for instance) can lead to character loses.
                                                                ^ losses


> This is due to the OUT ring buffer getting full, because we change the
> TX interrupt bit only when the FIFO is actually full, and not already
> when it's half-way filled, as the Linux driver expects.
> The SBSA spec does not explicitly state this, but we assume that an
> SBSA compliant UART uses the PL011 default "interrupt FIFO level select
> register" value of "1/2 way". The Linux driver certainly makes this
> assumption, so it expect to be able to write a number of characters
> after the TX interrupt line has been asserted.
> On a similar issue we have the same wrong behaviour on the receive side.
> However changing the RX interrupt to trigger on reaching half of the FIFO
> level will lead to lag, because the guest would not be notified of incoming
> characters until the FIFO is half way filled. This leads to inacceptible
> lags when typing on a terminal.
> Real hardware solves this issue by using the "receive timeout
> interrupt" (RTI), which is triggered when character reception stops for
> 32 baud cycles. As we cannot and do not want to emulate any timing here,
> we slightly abuse the timeout interrupt to notify the guest of new
> characters: when a new character comes in, the RTI is asserted, when
> the FIFO is cleared, the interrupt gets cleared.
> 
> So this patch changes the emulated interrupt trigger behaviour to come
> as close to real hardware as possible: the RX and TX interrupt trigger
> when the FIFO gets half full / half empty, and the RTI interrupt signals
> new incoming characters.
> 
> [Andre: reword commit message, introduce receive timeout interrupt, add
>         comments]
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> Reviewed-by: Andre Przywara <andre.przywara@linaro.org>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

This is good, only minor cosmetic comments.

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


Julien, can we have the release-ack?



> ---
>  xen/arch/arm/vpl011.c        | 131 ++++++++++++++++++++++++++++++-------------
>  xen/include/asm-arm/vpl011.h |   2 +
>  2 files changed, 94 insertions(+), 39 deletions(-)
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 0b0743679f..6d02406acf 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -18,6 +18,9 @@
>  
>  #define XEN_WANT_FLEX_CONSOLE_RING 1
>  
> +/* We assume the PL011 default of "1/2 way" for the FIFO trigger level. */
> +#define SBSA_UART_FIFO_LEVEL (SBSA_UART_FIFO_SIZE / 2)
> +
>  #include <xen/errno.h>
>  #include <xen/event.h>
>  #include <xen/guest_access.h>
> @@ -93,24 +96,37 @@ static uint8_t vpl011_read_data(struct domain *d)
>       */
>      if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
>      {
> +        unsigned int fifo_level;
> +
>          data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
>          in_cons += 1;
>          smp_mb();
>          intf->in_cons = in_cons;
> +
> +        fifo_level = xencons_queued(in_prod, in_cons, sizeof(intf->in));

This is only cosmetics but can we call xencons_queued only once? See the `if' just above.


> +        /* If the FIFO is now empty, we clear the receive timeout interrupt. */
> +        if ( fifo_level == 0 )
> +        {
> +            vpl011->uartfr |= RXFE;
> +            vpl011->uartris &= ~RTI;
> +        }
> +
> +        /* If the FIFO is more than half empty, we clear the RX interrupt. */
> +        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_LEVEL )
> +            vpl011->uartris &= ~RXI;
> +
> +        vpl011_update_interrupt_status(d);
>      }
>      else
>          gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
>  
> -    if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
> -    {
> -        vpl011->uartfr |= RXFE;
> -        vpl011->uartris &= ~RXI;
> -    }
> -
> +    /*
> +     * We have consumed a character or the FIFO was empty, so clear the
> +     * "FIFO full" bit.
> +     */
>      vpl011->uartfr &= ~RXFF;
>  
> -    vpl011_update_interrupt_status(d);
> -
>      VPL011_UNLOCK(d, flags);
>  
>      /*
> @@ -122,6 +138,24 @@ static uint8_t vpl011_read_data(struct domain *d)
>      return data;
>  }
>  
> +static void vpl011_update_tx_fifo_status(struct vpl011 *vpl011,
> +                                         unsigned int fifo_level)
> +{
> +    struct xencons_interface *intf = vpl011->ring_buf;
> +    unsigned int fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_LEVEL;
> +
> +    BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE);
> +
> +    /*
> +     * Set the TXI bit only when there is space for fifo_size/2 bytes which
> +     * is the trigger level for asserting/de-assterting the TX interrupt.
> +     */
> +    if ( fifo_level <= fifo_threshold )
> +        vpl011->uartris |= TXI;
> +    else
> +        vpl011->uartris &= ~TXI;
> +}
> +
>  static void vpl011_write_data(struct domain *d, uint8_t data)
>  {
>      unsigned long flags;
> @@ -146,33 +180,37 @@ static void vpl011_write_data(struct domain *d, uint8_t data)
>      if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
>           sizeof (intf->out) )
>      {
> +        unsigned int fifo_level;
> +
>          intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data;
>          out_prod += 1;
>          smp_wmb();
>          intf->out_prod = out_prod;
> -    }
> -    else
> -        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>  
> -    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
> -         sizeof (intf->out) )
> -    {
> -        vpl011->uartfr |= TXFF;
> -        vpl011->uartris &= ~TXI;
> +        fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out));

Same here


> -        /*
> -         * This bit is set only when FIFO becomes full. This ensures that
> -         * the SBSA UART driver can write the early console data as fast as
> -         * possible, without waiting for the BUSY bit to get cleared before
> -         * writing each byte.
> -         */
> -        vpl011->uartfr |= BUSY;
> +        if ( fifo_level == sizeof (intf->out) )

code style


> +        {
> +            vpl011->uartfr |= TXFF;
> +
> +            /*
> +             * This bit is set only when FIFO becomes full. This ensures that
> +             * the SBSA UART driver can write the early console data as fast as
> +             * possible, without waiting for the BUSY bit to get cleared before
> +             * writing each byte.
> +             */
> +            vpl011->uartfr |= BUSY;
> +        }
> +
> +        vpl011_update_tx_fifo_status(vpl011, fifo_level);
> +
> +        vpl011_update_interrupt_status(d);
>      }
> +    else
> +        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>  
>      vpl011->uartfr &= ~TXFE;
>  
> -    vpl011_update_interrupt_status(d);
> -
>      VPL011_UNLOCK(d, flags);
>  
>      /*
> @@ -344,7 +382,7 @@ static void vpl011_data_avail(struct domain *d)
>      struct vpl011 *vpl011 = &d->arch.vpl011;
>      struct xencons_interface *intf = vpl011->ring_buf;
>      XENCONS_RING_IDX in_cons, in_prod, out_cons, out_prod;
> -    XENCONS_RING_IDX in_ring_qsize, out_ring_qsize;
> +    XENCONS_RING_IDX in_fifo_level, out_fifo_level;
>  
>      VPL011_LOCK(d, flags);
>  
> @@ -355,28 +393,41 @@ static void vpl011_data_avail(struct domain *d)
>  
>      smp_rmb();
>  
> -    in_ring_qsize = xencons_queued(in_prod,
> +    in_fifo_level = xencons_queued(in_prod,
>                                     in_cons,
>                                     sizeof(intf->in));
>  
> -    out_ring_qsize = xencons_queued(out_prod,
> +    out_fifo_level = xencons_queued(out_prod,
>                                      out_cons,
>                                      sizeof(intf->out));
>  
> -    /* Update the uart rx state if the buffer is not empty. */
> -    if ( in_ring_qsize != 0 )
> -    {
> +    /**** Update the UART RX state ****/
> +
> +    /* Clear the FIFO_EMPTY bit if the FIFO holds at least one character. */
> +    if ( in_fifo_level > 0 )
>          vpl011->uartfr &= ~RXFE;
> -        if ( in_ring_qsize == sizeof(intf->in) )
> -            vpl011->uartfr |= RXFF;
> +
> +    /* Set the FIFO_FULL bit if the Xen buffer is full. */
> +    if ( in_fifo_level == sizeof(intf->in) )
> +        vpl011->uartfr |= RXFF;
> +
> +    /* Assert the RX interrupt if the FIFO is more than half way filled. */
> +    if ( in_fifo_level >= sizeof(intf->in) - SBSA_UART_FIFO_LEVEL )
>          vpl011->uartris |= RXI;
> -    }
>  
> -    /* Update the uart tx state if the buffer is not full. */
> -    if ( out_ring_qsize != sizeof(intf->out) )
> +    /*
> +     * If the input queue is not empty, we assert the receive timeout interrupt.
> +     * As we don't emulate any timing here, so we ignore the actual timeout
> +     * of 32 baud cycles.
> +     */
> +    if ( in_fifo_level > 0 )
> +        vpl011->uartris |= RTI;
> +
> +    /**** Update the UART TX state ****/
> +
> +    if ( out_fifo_level != sizeof(intf->out) )
>      {
>          vpl011->uartfr &= ~TXFF;
> -        vpl011->uartris |= TXI;
>  
>          /*
>           * Clear the BUSY bit as soon as space becomes available
> @@ -385,12 +436,14 @@ static void vpl011_data_avail(struct domain *d)
>           */
>          vpl011->uartfr &= ~BUSY;
>  
> -        if ( out_ring_qsize == 0 )
> -            vpl011->uartfr |= TXFE;
> +        vpl011_update_tx_fifo_status(vpl011, out_fifo_level);
>      }
>  
>      vpl011_update_interrupt_status(d);
>  
> +    if ( out_fifo_level == 0 )
> +        vpl011->uartfr |= TXFE;
> +
>      VPL011_UNLOCK(d, flags);
>  }
>  
> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> index 1b583dac3c..db95ff822f 100644
> --- a/xen/include/asm-arm/vpl011.h
> +++ b/xen/include/asm-arm/vpl011.h
> @@ -28,6 +28,8 @@
>  #define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags)
>  #define VPL011_UNLOCK(d,flags) spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
>  
> +#define SBSA_UART_FIFO_SIZE 32
> +
>  struct vpl011 {
>      void *ring_buf;
>      struct page_info *ring_page;
> -- 
> 2.14.1
>
Julien Grall Oct. 27, 2017, 1:28 p.m. UTC | #2 
Hi Stefano,

On 26/10/17 00:50, Stefano Stabellini wrote:
> On Tue, 24 Oct 2017, Andre Przywara wrote:
>> From: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>>
>> With the current SBSA UART emulation, streaming larger amounts of data
>> (as caused by "find /", for instance) can lead to character loses.
>                                                                  ^ losses
> 
> 
>> This is due to the OUT ring buffer getting full, because we change the
>> TX interrupt bit only when the FIFO is actually full, and not already
>> when it's half-way filled, as the Linux driver expects.
>> The SBSA spec does not explicitly state this, but we assume that an
>> SBSA compliant UART uses the PL011 default "interrupt FIFO level select
>> register" value of "1/2 way". The Linux driver certainly makes this
>> assumption, so it expect to be able to write a number of characters
>> after the TX interrupt line has been asserted.
>> On a similar issue we have the same wrong behaviour on the receive side.
>> However changing the RX interrupt to trigger on reaching half of the FIFO
>> level will lead to lag, because the guest would not be notified of incoming
>> characters until the FIFO is half way filled. This leads to inacceptible
>> lags when typing on a terminal.
>> Real hardware solves this issue by using the "receive timeout
>> interrupt" (RTI), which is triggered when character reception stops for
>> 32 baud cycles. As we cannot and do not want to emulate any timing here,
>> we slightly abuse the timeout interrupt to notify the guest of new
>> characters: when a new character comes in, the RTI is asserted, when
>> the FIFO is cleared, the interrupt gets cleared.
>>
>> So this patch changes the emulated interrupt trigger behaviour to come
>> as close to real hardware as possible: the RX and TX interrupt trigger
>> when the FIFO gets half full / half empty, and the RTI interrupt signals
>> new incoming characters.
>>
>> [Andre: reword commit message, introduce receive timeout interrupt, add
>>          comments]
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> Reviewed-by: Andre Przywara <andre.przywara@linaro.org>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> 
> This is good, only minor cosmetic comments.
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
> Julien, can we have the release-ack?

Sure. For the 2 patches:

Release-acked-by: Julien Grall <julien.gral@linaro.org>

Cheers,

> 
> 
> 
>> ---
>>   xen/arch/arm/vpl011.c        | 131 ++++++++++++++++++++++++++++++-------------
>>   xen/include/asm-arm/vpl011.h |   2 +
>>   2 files changed, 94 insertions(+), 39 deletions(-)
>>
>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>> index 0b0743679f..6d02406acf 100644
>> --- a/xen/arch/arm/vpl011.c
>> +++ b/xen/arch/arm/vpl011.c
>> @@ -18,6 +18,9 @@
>>   
>>   #define XEN_WANT_FLEX_CONSOLE_RING 1
>>   
>> +/* We assume the PL011 default of "1/2 way" for the FIFO trigger level. */
>> +#define SBSA_UART_FIFO_LEVEL (SBSA_UART_FIFO_SIZE / 2)
>> +
>>   #include <xen/errno.h>
>>   #include <xen/event.h>
>>   #include <xen/guest_access.h>
>> @@ -93,24 +96,37 @@ static uint8_t vpl011_read_data(struct domain *d)
>>        */
>>       if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
>>       {
>> +        unsigned int fifo_level;
>> +
>>           data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
>>           in_cons += 1;
>>           smp_mb();
>>           intf->in_cons = in_cons;
>> +
>> +        fifo_level = xencons_queued(in_prod, in_cons, sizeof(intf->in));
> 
> This is only cosmetics but can we call xencons_queued only once? See the `if' just above.
> 
> 
>> +        /* If the FIFO is now empty, we clear the receive timeout interrupt. */
>> +        if ( fifo_level == 0 )
>> +        {
>> +            vpl011->uartfr |= RXFE;
>> +            vpl011->uartris &= ~RTI;
>> +        }
>> +
>> +        /* If the FIFO is more than half empty, we clear the RX interrupt. */
>> +        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_LEVEL )
>> +            vpl011->uartris &= ~RXI;
>> +
>> +        vpl011_update_interrupt_status(d);
>>       }
>>       else
>>           gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
>>   
>> -    if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
>> -    {
>> -        vpl011->uartfr |= RXFE;
>> -        vpl011->uartris &= ~RXI;
>> -    }
>> -
>> +    /*
>> +     * We have consumed a character or the FIFO was empty, so clear the
>> +     * "FIFO full" bit.
>> +     */
>>       vpl011->uartfr &= ~RXFF;
>>   
>> -    vpl011_update_interrupt_status(d);
>> -
>>       VPL011_UNLOCK(d, flags);
>>   
>>       /*
>> @@ -122,6 +138,24 @@ static uint8_t vpl011_read_data(struct domain *d)
>>       return data;
>>   }
>>   
>> +static void vpl011_update_tx_fifo_status(struct vpl011 *vpl011,
>> +                                         unsigned int fifo_level)
>> +{
>> +    struct xencons_interface *intf = vpl011->ring_buf;
>> +    unsigned int fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_LEVEL;
>> +
>> +    BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE);
>> +
>> +    /*
>> +     * Set the TXI bit only when there is space for fifo_size/2 bytes which
>> +     * is the trigger level for asserting/de-assterting the TX interrupt.
>> +     */
>> +    if ( fifo_level <= fifo_threshold )
>> +        vpl011->uartris |= TXI;
>> +    else
>> +        vpl011->uartris &= ~TXI;
>> +}
>> +
>>   static void vpl011_write_data(struct domain *d, uint8_t data)
>>   {
>>       unsigned long flags;
>> @@ -146,33 +180,37 @@ static void vpl011_write_data(struct domain *d, uint8_t data)
>>       if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
>>            sizeof (intf->out) )
>>       {
>> +        unsigned int fifo_level;
>> +
>>           intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data;
>>           out_prod += 1;
>>           smp_wmb();
>>           intf->out_prod = out_prod;
>> -    }
>> -    else
>> -        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>>   
>> -    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
>> -         sizeof (intf->out) )
>> -    {
>> -        vpl011->uartfr |= TXFF;
>> -        vpl011->uartris &= ~TXI;
>> +        fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out));
> 
> Same here
> 
> 
>> -        /*
>> -         * This bit is set only when FIFO becomes full. This ensures that
>> -         * the SBSA UART driver can write the early console data as fast as
>> -         * possible, without waiting for the BUSY bit to get cleared before
>> -         * writing each byte.
>> -         */
>> -        vpl011->uartfr |= BUSY;
>> +        if ( fifo_level == sizeof (intf->out) )
> 
> code style
> 
> 
>> +        {
>> +            vpl011->uartfr |= TXFF;
>> +
>> +            /*
>> +             * This bit is set only when FIFO becomes full. This ensures that
>> +             * the SBSA UART driver can write the early console data as fast as
>> +             * possible, without waiting for the BUSY bit to get cleared before
>> +             * writing each byte.
>> +             */
>> +            vpl011->uartfr |= BUSY;
>> +        }
>> +
>> +        vpl011_update_tx_fifo_status(vpl011, fifo_level);
>> +
>> +        vpl011_update_interrupt_status(d);
>>       }
>> +    else
>> +        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>>   
>>       vpl011->uartfr &= ~TXFE;
>>   
>> -    vpl011_update_interrupt_status(d);
>> -
>>       VPL011_UNLOCK(d, flags);
>>   
>>       /*
>> @@ -344,7 +382,7 @@ static void vpl011_data_avail(struct domain *d)
>>       struct vpl011 *vpl011 = &d->arch.vpl011;
>>       struct xencons_interface *intf = vpl011->ring_buf;
>>       XENCONS_RING_IDX in_cons, in_prod, out_cons, out_prod;
>> -    XENCONS_RING_IDX in_ring_qsize, out_ring_qsize;
>> +    XENCONS_RING_IDX in_fifo_level, out_fifo_level;
>>   
>>       VPL011_LOCK(d, flags);
>>   
>> @@ -355,28 +393,41 @@ static void vpl011_data_avail(struct domain *d)
>>   
>>       smp_rmb();
>>   
>> -    in_ring_qsize = xencons_queued(in_prod,
>> +    in_fifo_level = xencons_queued(in_prod,
>>                                      in_cons,
>>                                      sizeof(intf->in));
>>   
>> -    out_ring_qsize = xencons_queued(out_prod,
>> +    out_fifo_level = xencons_queued(out_prod,
>>                                       out_cons,
>>                                       sizeof(intf->out));
>>   
>> -    /* Update the uart rx state if the buffer is not empty. */
>> -    if ( in_ring_qsize != 0 )
>> -    {
>> +    /**** Update the UART RX state ****/
>> +
>> +    /* Clear the FIFO_EMPTY bit if the FIFO holds at least one character. */
>> +    if ( in_fifo_level > 0 )
>>           vpl011->uartfr &= ~RXFE;
>> -        if ( in_ring_qsize == sizeof(intf->in) )
>> -            vpl011->uartfr |= RXFF;
>> +
>> +    /* Set the FIFO_FULL bit if the Xen buffer is full. */
>> +    if ( in_fifo_level == sizeof(intf->in) )
>> +        vpl011->uartfr |= RXFF;
>> +
>> +    /* Assert the RX interrupt if the FIFO is more than half way filled. */
>> +    if ( in_fifo_level >= sizeof(intf->in) - SBSA_UART_FIFO_LEVEL )
>>           vpl011->uartris |= RXI;
>> -    }
>>   
>> -    /* Update the uart tx state if the buffer is not full. */
>> -    if ( out_ring_qsize != sizeof(intf->out) )
>> +    /*
>> +     * If the input queue is not empty, we assert the receive timeout interrupt.
>> +     * As we don't emulate any timing here, so we ignore the actual timeout
>> +     * of 32 baud cycles.
>> +     */
>> +    if ( in_fifo_level > 0 )
>> +        vpl011->uartris |= RTI;
>> +
>> +    /**** Update the UART TX state ****/
>> +
>> +    if ( out_fifo_level != sizeof(intf->out) )
>>       {
>>           vpl011->uartfr &= ~TXFF;
>> -        vpl011->uartris |= TXI;
>>   
>>           /*
>>            * Clear the BUSY bit as soon as space becomes available
>> @@ -385,12 +436,14 @@ static void vpl011_data_avail(struct domain *d)
>>            */
>>           vpl011->uartfr &= ~BUSY;
>>   
>> -        if ( out_ring_qsize == 0 )
>> -            vpl011->uartfr |= TXFE;
>> +        vpl011_update_tx_fifo_status(vpl011, out_fifo_level);
>>       }
>>   
>>       vpl011_update_interrupt_status(d);
>>   
>> +    if ( out_fifo_level == 0 )
>> +        vpl011->uartfr |= TXFE;
>> +
>>       VPL011_UNLOCK(d, flags);
>>   }
>>   
>> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
>> index 1b583dac3c..db95ff822f 100644
>> --- a/xen/include/asm-arm/vpl011.h
>> +++ b/xen/include/asm-arm/vpl011.h
>> @@ -28,6 +28,8 @@
>>   #define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags)
>>   #define VPL011_UNLOCK(d,flags) spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
>>   
>> +#define SBSA_UART_FIFO_SIZE 32
>> +
>>   struct vpl011 {
>>       void *ring_buf;
>>       struct page_info *ring_page;
>> -- 
>> 2.14.1
>>
diff mbox series

Patch

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 0b0743679f..6d02406acf 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -18,6 +18,9 @@ 
 
 #define XEN_WANT_FLEX_CONSOLE_RING 1
 
+/* We assume the PL011 default of "1/2 way" for the FIFO trigger level. */
+#define SBSA_UART_FIFO_LEVEL (SBSA_UART_FIFO_SIZE / 2)
+
 #include <xen/errno.h>
 #include <xen/event.h>
 #include <xen/guest_access.h>
@@ -93,24 +96,37 @@  static uint8_t vpl011_read_data(struct domain *d)
      */
     if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
     {
+        unsigned int fifo_level;
+
         data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
         in_cons += 1;
         smp_mb();
         intf->in_cons = in_cons;
+
+        fifo_level = xencons_queued(in_prod, in_cons, sizeof(intf->in));
+
+        /* If the FIFO is now empty, we clear the receive timeout interrupt. */
+        if ( fifo_level == 0 )
+        {
+            vpl011->uartfr |= RXFE;
+            vpl011->uartris &= ~RTI;
+        }
+
+        /* If the FIFO is more than half empty, we clear the RX interrupt. */
+        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_LEVEL )
+            vpl011->uartris &= ~RXI;
+
+        vpl011_update_interrupt_status(d);
     }
     else
         gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
 
-    if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
-    {
-        vpl011->uartfr |= RXFE;
-        vpl011->uartris &= ~RXI;
-    }
-
+    /*
+     * We have consumed a character or the FIFO was empty, so clear the
+     * "FIFO full" bit.
+     */
     vpl011->uartfr &= ~RXFF;
 
-    vpl011_update_interrupt_status(d);
-
     VPL011_UNLOCK(d, flags);
 
     /*
@@ -122,6 +138,24 @@  static uint8_t vpl011_read_data(struct domain *d)
     return data;
 }
 
+static void vpl011_update_tx_fifo_status(struct vpl011 *vpl011,
+                                         unsigned int fifo_level)
+{
+    struct xencons_interface *intf = vpl011->ring_buf;
+    unsigned int fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_LEVEL;
+
+    BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE);
+
+    /*
+     * Set the TXI bit only when there is space for fifo_size/2 bytes which
+     * is the trigger level for asserting/de-assterting the TX interrupt.
+     */
+    if ( fifo_level <= fifo_threshold )
+        vpl011->uartris |= TXI;
+    else
+        vpl011->uartris &= ~TXI;
+}
+
 static void vpl011_write_data(struct domain *d, uint8_t data)
 {
     unsigned long flags;
@@ -146,33 +180,37 @@  static void vpl011_write_data(struct domain *d, uint8_t data)
     if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
          sizeof (intf->out) )
     {
+        unsigned int fifo_level;
+
         intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data;
         out_prod += 1;
         smp_wmb();
         intf->out_prod = out_prod;
-    }
-    else
-        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
 
-    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
-         sizeof (intf->out) )
-    {
-        vpl011->uartfr |= TXFF;
-        vpl011->uartris &= ~TXI;
+        fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out));
 
-        /*
-         * This bit is set only when FIFO becomes full. This ensures that
-         * the SBSA UART driver can write the early console data as fast as
-         * possible, without waiting for the BUSY bit to get cleared before
-         * writing each byte.
-         */
-        vpl011->uartfr |= BUSY;
+        if ( fifo_level == sizeof (intf->out) )
+        {
+            vpl011->uartfr |= TXFF;
+
+            /*
+             * This bit is set only when FIFO becomes full. This ensures that
+             * the SBSA UART driver can write the early console data as fast as
+             * possible, without waiting for the BUSY bit to get cleared before
+             * writing each byte.
+             */
+            vpl011->uartfr |= BUSY;
+        }
+
+        vpl011_update_tx_fifo_status(vpl011, fifo_level);
+
+        vpl011_update_interrupt_status(d);
     }
+    else
+        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
 
     vpl011->uartfr &= ~TXFE;
 
-    vpl011_update_interrupt_status(d);
-
     VPL011_UNLOCK(d, flags);
 
     /*
@@ -344,7 +382,7 @@  static void vpl011_data_avail(struct domain *d)
     struct vpl011 *vpl011 = &d->arch.vpl011;
     struct xencons_interface *intf = vpl011->ring_buf;
     XENCONS_RING_IDX in_cons, in_prod, out_cons, out_prod;
-    XENCONS_RING_IDX in_ring_qsize, out_ring_qsize;
+    XENCONS_RING_IDX in_fifo_level, out_fifo_level;
 
     VPL011_LOCK(d, flags);
 
@@ -355,28 +393,41 @@  static void vpl011_data_avail(struct domain *d)
 
     smp_rmb();
 
-    in_ring_qsize = xencons_queued(in_prod,
+    in_fifo_level = xencons_queued(in_prod,
                                    in_cons,
                                    sizeof(intf->in));
 
-    out_ring_qsize = xencons_queued(out_prod,
+    out_fifo_level = xencons_queued(out_prod,
                                     out_cons,
                                     sizeof(intf->out));
 
-    /* Update the uart rx state if the buffer is not empty. */
-    if ( in_ring_qsize != 0 )
-    {
+    /**** Update the UART RX state ****/
+
+    /* Clear the FIFO_EMPTY bit if the FIFO holds at least one character. */
+    if ( in_fifo_level > 0 )
         vpl011->uartfr &= ~RXFE;
-        if ( in_ring_qsize == sizeof(intf->in) )
-            vpl011->uartfr |= RXFF;
+
+    /* Set the FIFO_FULL bit if the Xen buffer is full. */
+    if ( in_fifo_level == sizeof(intf->in) )
+        vpl011->uartfr |= RXFF;
+
+    /* Assert the RX interrupt if the FIFO is more than half way filled. */
+    if ( in_fifo_level >= sizeof(intf->in) - SBSA_UART_FIFO_LEVEL )
         vpl011->uartris |= RXI;
-    }
 
-    /* Update the uart tx state if the buffer is not full. */
-    if ( out_ring_qsize != sizeof(intf->out) )
+    /*
+     * If the input queue is not empty, we assert the receive timeout interrupt.
+     * As we don't emulate any timing here, so we ignore the actual timeout
+     * of 32 baud cycles.
+     */
+    if ( in_fifo_level > 0 )
+        vpl011->uartris |= RTI;
+
+    /**** Update the UART TX state ****/
+
+    if ( out_fifo_level != sizeof(intf->out) )
     {
         vpl011->uartfr &= ~TXFF;
-        vpl011->uartris |= TXI;
 
         /*
          * Clear the BUSY bit as soon as space becomes available
@@ -385,12 +436,14 @@  static void vpl011_data_avail(struct domain *d)
          */
         vpl011->uartfr &= ~BUSY;
 
-        if ( out_ring_qsize == 0 )
-            vpl011->uartfr |= TXFE;
+        vpl011_update_tx_fifo_status(vpl011, out_fifo_level);
     }
 
     vpl011_update_interrupt_status(d);
 
+    if ( out_fifo_level == 0 )
+        vpl011->uartfr |= TXFE;
+
     VPL011_UNLOCK(d, flags);
 }
 
diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
index 1b583dac3c..db95ff822f 100644
--- a/xen/include/asm-arm/vpl011.h
+++ b/xen/include/asm-arm/vpl011.h
@@ -28,6 +28,8 @@ 
 #define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags)
 #define VPL011_UNLOCK(d,flags) spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
 
+#define SBSA_UART_FIFO_SIZE 32
+
 struct vpl011 {
     void *ring_buf;
     struct page_info *ring_page;