diff mbox series

[Xen-devel,27/27,v11] xen/arm: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt

Message ID 1506492816-25954-28-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series SBSA UART emulation support in Xen | expand

Commit Message

Bhupinder Thakur Sept. 27, 2017, 6:13 a.m. UTC
This patch fixes the issue observed when pl011 patches were tested on
the junos hardware by Andre/Julien. It was observed that when large output is
generated such as on running 'find /', output was getting truncated intermittently
due to OUT ring buffer getting full.

This issue was due to the fact that the SBSA UART driver expects that when
a TX interrupt is asserted then the FIFO queue should be atleast half empty and
that it can write N bytes in the FIFO, where N is half the FIFO queue size, without
the bytes getting dropped due to FIFO getting full.

The SBSA UART emulation logic was asserting the TX interrupt as soon as
any space became available in the FIFO and the SBSA UART driver tried to write
more data (upto 16 bytes) in the FIFO expecting that there is enough space
available leading to dropped bytes.

The SBSA spec [1] does not specify when the TX interrupt should be asserted
or de-asserted. Due to lack of clarity on the expected behavior, it is
assumed for now that TX interrupt should be asserted only when the FIFO goes
half empty.

TBD: Once the SBSA spec is updated with the expected behavior, the implementation
will be modified to align with the spec requirement.

[1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
CC: Julien Grall <julien.grall@arm.com>
CC: Andre Przywara <andre.przywara@arm.com>
CC: Stefano Stabellini <sstabellini@kernel.org>

Changes since v8:
- Used variables fifo_level/fifo_threshold for more clarity
- Added a new macro SBSA_UART_FIFO_SIZE instead of using a magic number
- Renamed ring_qsize variables to fifo_level for consistency 

 xen/arch/arm/vpl011.c        | 87 ++++++++++++++++++++++++++++++--------------
 xen/include/asm-arm/vpl011.h |  2 +
 2 files changed, 61 insertions(+), 28 deletions(-)

Comments

Andre Przywara Oct. 9, 2017, 5:18 p.m. UTC | #1
Hi,

can you please re-break the commit message to fit into 72 characters?
git show looks rather ugly as it is now.

On 27/09/17 07:13, Bhupinder Thakur wrote:
> This patch fixes the issue observed when pl011 patches were tested on
> the junos hardware by Andre/Julien. It was observed that when large output is
> generated such as on running 'find /', output was getting truncated intermittently
> due to OUT ring buffer getting full.
> 
> This issue was due to the fact that the SBSA UART driver expects that when
> a TX interrupt is asserted then the FIFO queue should be atleast half empty and
> that it can write N bytes in the FIFO, where N is half the FIFO queue size, without
> the bytes getting dropped due to FIFO getting full.
> 
> The SBSA UART emulation logic was asserting the TX interrupt as soon as
> any space became available in the FIFO and the SBSA UART driver tried to write
> more data (upto 16 bytes) in the FIFO expecting that there is enough space
> available leading to dropped bytes.
> 
> The SBSA spec [1] does not specify when the TX interrupt should be asserted
> or de-asserted. Due to lack of clarity on the expected behavior, it is
> assumed for now that TX interrupt should be asserted only when the FIFO goes
> half empty.
> 
> TBD: Once the SBSA spec is updated with the expected behavior, the implementation
> will be modified to align with the spec requirement.
> 
> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>

Only some minor things left below, but in general this looks much better
to me now.

> ---
> CC: Julien Grall <julien.grall@arm.com>
> CC: Andre Przywara <andre.przywara@arm.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> 
> Changes since v8:
> - Used variables fifo_level/fifo_threshold for more clarity
> - Added a new macro SBSA_UART_FIFO_SIZE instead of using a magic number
> - Renamed ring_qsize variables to fifo_level for consistency 
> 
>  xen/arch/arm/vpl011.c        | 87 ++++++++++++++++++++++++++++++--------------
>  xen/include/asm-arm/vpl011.h |  2 +
>  2 files changed, 61 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 36794d8..1f97261 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -91,20 +91,24 @@ 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 ( fifo_level == 0 )
> +        {
> +            vpl011->uartfr |= RXFE;
> +            vpl011->uartris &= ~RXI;
> +        }
>      }
>      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;
> -    }
> -
>      vpl011->uartfr &= ~RXFF;
>  
>      vpl011_update_interrupt_status(d);
> @@ -144,28 +148,41 @@ 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, fifo_threshold;
> +
>          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));
> +
> +        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;
> +        }
>  
>          /*
> -         * 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.
> +         * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark which
> +         * is the trigger level for asserting/de-assterting the TX interrupt.
>           */
> -        vpl011->uartfr |= BUSY;
> +        fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2;
> +
> +        if ( fifo_level <= fifo_threshold )
> +            vpl011->uartris |= TXI;
> +        else
> +            vpl011->uartris &= ~TXI;

I think we can move the call to vpl011_update_interrupt_status() here.
Currently it's called below, but here is the only place in this function
where the interrupt status can change.
This isn't a showstopper, but makes more sense and might come in handy
once we get level trigger vIRQ support.

>      }
> +    else
> +        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>  
>      vpl011->uartfr &= ~TXFE;
>  
> @@ -342,7 +359,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);
>  
> @@ -353,37 +370,51 @@ 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 )
> +    if ( in_fifo_level != 0 )
>      {
>          vpl011->uartfr &= ~RXFE;
> -        if ( in_ring_qsize == sizeof(intf->in) )
> +
> +        if ( in_fifo_level == sizeof(intf->in) )
>              vpl011->uartfr |= RXFF;
> +
>          vpl011->uartris |= RXI;
>      }
>  
>      /* Update the uart tx state if the buffer is not full. */
> -    if ( out_ring_qsize != sizeof(intf->out) )
> +    if ( out_fifo_level != sizeof(intf->out) )
>      {
> +        unsigned int out_fifo_threshold;
> +
>          vpl011->uartfr &= ~TXFF;
> -        vpl011->uartris |= TXI;
>  
>          /*
> -         * Clear the BUSY bit as soon as space becomes available
> +         * Clear the BUSY bit as soon as space becomes avaliable

Looks like a newly introduced typo to me.

>           * so that the SBSA UART driver can start writing more data
>           * without any further delay.
>           */
>          vpl011->uartfr &= ~BUSY;
>  
> -        if ( out_ring_qsize == 0 )
> +        /*
> +         * 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.
> +         */
> +        out_fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
> +
> +        if ( out_fifo_level <= out_fifo_threshold )
> +            vpl011->uartris |= TXI;
> +        else
> +            vpl011->uartris &= ~TXI;

Same here with moving the call to vpl011_update_interrupt_status().
Eventually this could become a direct call to the function setting the
line level of the vIRQ.

Cheers,
Andre.

> +
> +        if ( out_fifo_level == 0 )
>              vpl011->uartfr |= TXFE;
>      }
>  
> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> index 1b583da..db95ff8 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;
>
Bhupinder Thakur Oct. 12, 2017, 9:33 a.m. UTC | #2
On 9 October 2017 at 22:48, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi,
>
> can you please re-break the commit message to fit into 72 characters?
> git show looks rather ugly as it is now.
ok.
>
> On 27/09/17 07:13, Bhupinder Thakur wrote:
>> This patch fixes the issue observed when pl011 patches were tested on
>> the junos hardware by Andre/Julien. It was observed that when large output is
>> generated such as on running 'find /', output was getting truncated intermittently
>> due to OUT ring buffer getting full.
>>
>> This issue was due to the fact that the SBSA UART driver expects that when
>> a TX interrupt is asserted then the FIFO queue should be atleast half empty and
>> that it can write N bytes in the FIFO, where N is half the FIFO queue size, without
>> the bytes getting dropped due to FIFO getting full.
>>
>> The SBSA UART emulation logic was asserting the TX interrupt as soon as
>> any space became available in the FIFO and the SBSA UART driver tried to write
>> more data (upto 16 bytes) in the FIFO expecting that there is enough space
>> available leading to dropped bytes.
>>
>> The SBSA spec [1] does not specify when the TX interrupt should be asserted
>> or de-asserted. Due to lack of clarity on the expected behavior, it is
>> assumed for now that TX interrupt should be asserted only when the FIFO goes
>> half empty.
>>
>> TBD: Once the SBSA spec is updated with the expected behavior, the implementation
>> will be modified to align with the spec requirement.
>>
>> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>
> Only some minor things left below, but in general this looks much better
> to me now.
>
>> ---
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Andre Przywara <andre.przywara@arm.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>
>> Changes since v8:
>> - Used variables fifo_level/fifo_threshold for more clarity
>> - Added a new macro SBSA_UART_FIFO_SIZE instead of using a magic number
>> - Renamed ring_qsize variables to fifo_level for consistency
>>
>>  xen/arch/arm/vpl011.c        | 87 ++++++++++++++++++++++++++++++--------------
>>  xen/include/asm-arm/vpl011.h |  2 +
>>  2 files changed, 61 insertions(+), 28 deletions(-)
>>
>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>> index 36794d8..1f97261 100644
>> --- a/xen/arch/arm/vpl011.c
>> +++ b/xen/arch/arm/vpl011.c
>> @@ -91,20 +91,24 @@ 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 ( fifo_level == 0 )
>> +        {
>> +            vpl011->uartfr |= RXFE;
>> +            vpl011->uartris &= ~RXI;
>> +        }
>>      }
>>      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;
>> -    }
>> -
>>      vpl011->uartfr &= ~RXFF;
>>
>>      vpl011_update_interrupt_status(d);
>> @@ -144,28 +148,41 @@ 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, fifo_threshold;
>> +
>>          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));
>> +
>> +        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;
>> +        }
>>
>>          /*
>> -         * 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.
>> +         * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark which
>> +         * is the trigger level for asserting/de-assterting the TX interrupt.
>>           */
>> -        vpl011->uartfr |= BUSY;
>> +        fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2;
>> +
>> +        if ( fifo_level <= fifo_threshold )
>> +            vpl011->uartris |= TXI;
>> +        else
>> +            vpl011->uartris &= ~TXI;
>
> I think we can move the call to vpl011_update_interrupt_status() here.
> Currently it's called below, but here is the only place in this function
> where the interrupt status can change.
> This isn't a showstopper, but makes more sense and might come in handy
> once we get level trigger vIRQ support.
>
ok.

>>      }
>> +    else
>> +        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
>>
>>      vpl011->uartfr &= ~TXFE;
>>
>> @@ -342,7 +359,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);
>>
>> @@ -353,37 +370,51 @@ 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 )
>> +    if ( in_fifo_level != 0 )
>>      {
>>          vpl011->uartfr &= ~RXFE;
>> -        if ( in_ring_qsize == sizeof(intf->in) )
>> +
>> +        if ( in_fifo_level == sizeof(intf->in) )
>>              vpl011->uartfr |= RXFF;
>> +
>>          vpl011->uartris |= RXI;
>>      }
>>
>>      /* Update the uart tx state if the buffer is not full. */
>> -    if ( out_ring_qsize != sizeof(intf->out) )
>> +    if ( out_fifo_level != sizeof(intf->out) )
>>      {
>> +        unsigned int out_fifo_threshold;
>> +
>>          vpl011->uartfr &= ~TXFF;
>> -        vpl011->uartris |= TXI;
>>
>>          /*
>> -         * Clear the BUSY bit as soon as space becomes available
>> +         * Clear the BUSY bit as soon as space becomes avaliable
>
> Looks like a newly introduced typo to me.
Thanks. I will fix it.

>
>>           * so that the SBSA UART driver can start writing more data
>>           * without any further delay.
>>           */
>>          vpl011->uartfr &= ~BUSY;
>>
>> -        if ( out_ring_qsize == 0 )
>> +        /*
>> +         * 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.
>> +         */
>> +        out_fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
>> +
>> +        if ( out_fifo_level <= out_fifo_threshold )
>> +            vpl011->uartris |= TXI;
>> +        else
>> +            vpl011->uartris &= ~TXI;
>
> Same here with moving the call to vpl011_update_interrupt_status().
> Eventually this could become a direct call to the function setting the
> line level of the vIRQ.
In the above if block, it sets the RXI bit in uartris. In that case
also we need to trigger the interrupt. But I will fix the same thing
in the other function vpl011_read_data().

Regards,
Bhupinder
diff mbox series

Patch

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 36794d8..1f97261 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -91,20 +91,24 @@  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 ( fifo_level == 0 )
+        {
+            vpl011->uartfr |= RXFE;
+            vpl011->uartris &= ~RXI;
+        }
     }
     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;
-    }
-
     vpl011->uartfr &= ~RXFF;
 
     vpl011_update_interrupt_status(d);
@@ -144,28 +148,41 @@  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, fifo_threshold;
+
         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));
+
+        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;
+        }
 
         /*
-         * 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.
+         * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark which
+         * is the trigger level for asserting/de-assterting the TX interrupt.
          */
-        vpl011->uartfr |= BUSY;
+        fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2;
+
+        if ( fifo_level <= fifo_threshold )
+            vpl011->uartris |= TXI;
+        else
+            vpl011->uartris &= ~TXI;
     }
+    else
+        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
 
     vpl011->uartfr &= ~TXFE;
 
@@ -342,7 +359,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);
 
@@ -353,37 +370,51 @@  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 )
+    if ( in_fifo_level != 0 )
     {
         vpl011->uartfr &= ~RXFE;
-        if ( in_ring_qsize == sizeof(intf->in) )
+
+        if ( in_fifo_level == sizeof(intf->in) )
             vpl011->uartfr |= RXFF;
+
         vpl011->uartris |= RXI;
     }
 
     /* Update the uart tx state if the buffer is not full. */
-    if ( out_ring_qsize != sizeof(intf->out) )
+    if ( out_fifo_level != sizeof(intf->out) )
     {
+        unsigned int out_fifo_threshold;
+
         vpl011->uartfr &= ~TXFF;
-        vpl011->uartris |= TXI;
 
         /*
-         * Clear the BUSY bit as soon as space becomes available
+         * Clear the BUSY bit as soon as space becomes avaliable
          * so that the SBSA UART driver can start writing more data
          * without any further delay.
          */
         vpl011->uartfr &= ~BUSY;
 
-        if ( out_ring_qsize == 0 )
+        /*
+         * 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.
+         */
+        out_fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
+
+        if ( out_fifo_level <= out_fifo_threshold )
+            vpl011->uartris |= TXI;
+        else
+            vpl011->uartris &= ~TXI;
+
+        if ( out_fifo_level == 0 )
             vpl011->uartfr |= TXFE;
     }
 
diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
index 1b583da..db95ff8 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;