[xen master] arm/xen: vpl011: Fix SBSA UART interrupt assertion

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[xen master] arm/xen: vpl011: Fix SBSA UART interrupt assertion

patchbot
commit bb2c1a1cc98a22e2d4c14b18421aa7be6c2adf0d
Author:     Bhupinder Thakur <[hidden email]>
AuthorDate: Tue Oct 24 18:09:22 2017 +0100
Commit:     Stefano Stabellini <[hidden email]>
CommitDate: Fri Oct 27 10:11:05 2017 -0700

    arm/xen: vpl011: Fix SBSA UART interrupt assertion
   
    With the current SBSA UART emulation, streaming larger amounts of data
    (as caused by "find /", for instance) can lead to character 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 <[hidden email]>
    Reviewed-by: Andre Przywara <[hidden email]>
    Signed-off-by: Andre Przywara <[hidden email]>
    Signed-off-by: Stefano Stabellini <[hidden email]>
    Reviewed-by: Stefano Stabellini <[hidden email]>
    Release-acked-by: Julien Grall <[hidden email]>
---
 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 0b07436..725b2e0 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 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;
--
generated by git-patchbot for /home/xen/git/xen.git#master

_______________________________________________
Xen-changelog mailing list
[hidden email]
https://lists.xenproject.org/xen-changelog