[Xen-devel] [PATCH RFC] ARM: vPL011: use receive timeout interrupt

[Andre Przywara <andre.przywara@xxxxxxx>]

Instead of asserting the receive interrupt (RXI) on the first character
in the FIFO, lets (ab)use the receive timeout interrupt (RTI) for that
purpose. That seems to be closer to the spec and what hardware does.
Improve the readability of vpl011_data_avail() on the way.

Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
Hi,

this one is the approach I mentioned in the email earlier today.
It goes on top of Bhupinders v12 27/27, but should eventually be merged
into this one once we agreed on the subject. I just carved it out here
for clarity to make it clearer what has been changed.
Would be good if someone could test it.

Cheers,
Andre.
 xen/arch/arm/vpl011.c | 61 ++++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index adf1711571..ae18bddd81 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -105,9 +105,13 @@ static uint8_t vpl011_read_data(struct domain *d)
         if ( fifo_level == 0 )
         {
             vpl011->uartfr |= RXFE;
-            vpl011->uartris &= ~RXI;
-            vpl011_update_interrupt_status(d);
+            vpl011->uartris &= ~RTI;
         }
+
+        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
+            vpl011->uartris &= ~RXI;
+
+        vpl011_update_interrupt_status(d);
     }
     else
         gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
@@ -129,7 +133,7 @@ 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;
+    unsigned int fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
 
     BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE);
 
@@ -137,8 +141,6 @@ static void vpl011_update_tx_fifo_status(struct vpl011 
*vpl011,
      * 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.
      */
-    fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
-
     if ( fifo_level <= fifo_threshold )
         vpl011->uartris |= TXI;
     else
@@ -390,35 +392,30 @@ static void vpl011_data_avail(struct domain *d)
                                     out_cons,
                                     sizeof(intf->out));
 
-    /* Update the uart rx state if the buffer is not empty. */
-    if ( in_fifo_level != 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_fifo_level == sizeof(intf->in) )
-            vpl011->uartfr |= RXFF;
+    /* Set the FIFO_FULL bit if the ring buffer is full. */
+    if ( in_fifo_level == sizeof(intf->in) )
+        vpl011->uartfr |= RXFF;
 
-        /*
-         * Currently, the RXI bit is getting set even if there is a single
-         * byte of data in the rx fifo. Ideally, the RXI bit should be set
-         * only if the rx fifo level reaches the threshold.
-         *
-         * However, since currently RX timeout interrupt is not
-         * implemented as there is not enough clarity in the SBSA spec,
-         * the guest may keep waiting for an interrupt to read more
-         * data. To ensure that guest reads all the data without
-         * any delay, the RXI interrupt is raised if there is RX data
-         * available without checking whether fifo level has reached
-         * the threshold.
-         *
-         * TBD: Once there is more clarity in the SBSA spec on whether RX
-         * timeout interrupt needs to be implemented, the RXI interrupt
-         * will be raised only when rx fifo level reaches the threshold.
-         */
+    /* The FIFO trigger level is fixed to half of the FIFO. */
+    if ( in_fifo_level >= sizeof(intf->in) - SBSA_UART_FIFO_SIZE / 2 )
         vpl011->uartris |= RXI;
-    }
 
-    /* Update the uart tx state if the buffer is not full. */
+    /*
+     * If the input queue is not empty, we assert the receive timeout 
interrupt.
+     * As we don't emulate any timing here, we ignore the actual timeout
+     * of 32 bit periods.
+     */
+    if ( in_fifo_level > 0 )
+        vpl011->uartris |= RTI;
+
+    /**** Update the UART TX state ****/
+
     if ( out_fifo_level != sizeof(intf->out) )
     {
         vpl011->uartfr &= ~TXFF;
@@ -431,13 +428,13 @@ static void vpl011_data_avail(struct domain *d)
         vpl011->uartfr &= ~BUSY;
 
         vpl011_update_tx_fifo_status(vpl011, out_fifo_level);
-
-        if ( out_fifo_level == 0 )
-            vpl011->uartfr |= TXFE;
     }
 
     vpl011_update_interrupt_status(d);
 
+    if ( out_fifo_level == 0 )
+        vpl011->uartfr |= TXFE;
+
     VPL011_UNLOCK(d, flags);
 }
 
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel