[PATCH v2] hw/char/cmsdk-apb-uart: Fix rx interrupt handling

[Tadej Pečar]

Previously, the RX interrupt got missed if:
- the character backend provided next character before
  the RX IRQ Handler managed to clear the currently served interrupt.
- the character backend provided next character while the RX interrupt
  was disabled. Enabling the interrupt did not trigger the interrupt
  even if the RXFULL status bit was set.

These bugs become apparent when the terminal emulator buffers the line
before sending it to qemu stdin (Eclipse IDE console does this).

---
Patch was tested on the mps2-an500 machine with
 - a baremetal application using a USART_V2M-MPS2.c driver,
   sourced from Keil.V2M-MPS2_CMx_BSP.1.7.0.pack
   (available at https://www.keil.com/dd2/Pack/),
   which invoked the aforementioned bugs.

   The following command line was used
     qemu-system-arm -M mps2-an500 -serial stdio -display none -device 
loader,file=baremetal-app.elf

 - uClinux system, built with the following instructions
   
https://community.arm.com/developer/tools-software/oss-platforms/w/docs/578/running-uclinux-on-the-arm-mps2-platform

   The linux "mps2-uart" driver works and seems unaffected by this patch.

   The following command line was used
     qemu-system-arm -M mps2-an500 -serial stdio -display none -kernel boot.axf 
-device loader,file=linux.axf

---
Changes:
- original patch -> v2:
    Removed unnecessary check in uart_write, since this is sufficiently
    handled in cmsdk_apb_uart_update

    Better formatting, documentation.


Signed-off-by: Tadej Pecar <tpecar95@gmail.com>
---
 hw/char/cmsdk-apb-uart.c | 47 +++++++++++++++++++++++++++-------------
 hw/char/trace-events     |  1 +
 2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/hw/char/cmsdk-apb-uart.c b/hw/char/cmsdk-apb-uart.c
index 626b68f2ec..d76ca76e01 100644
--- a/hw/char/cmsdk-apb-uart.c
+++ b/hw/char/cmsdk-apb-uart.c
@@ -96,19 +96,34 @@ static void uart_update_parameters(CMSDKAPBUART *s)
 
 static void cmsdk_apb_uart_update(CMSDKAPBUART *s)
 {
-    /* update outbound irqs, including handling the way the rxo and txo
-     * interrupt status bits are just logical AND of the overrun bit in
-     * STATE and the overrun interrupt enable bit in CTRL.
+    /*
+     * update outbound irqs
+     * (
+     *     state     [rxo,  txo,  rxbf, txbf ] at bit [3, 2, 1, 0]
+     *   | intstatus [rxo,  txo,  rx,   tx   ] at bit [3, 2, 1, 0]
+     * )
+     * & ctrl        [rxoe, txoe, rxe,  txe  ] at bit [5, 4, 3, 2]
+     * = masked_intstatus
+     *
+     * state: status register
+     * intstatus: pending interrupts and is sticky (has to be cleared by sw)
+     * masked_intstatus: masked (by ctrl) pending interrupts
+     *
+     * intstatus [rxo, txo, rx] bits are set here
+     * intstatus [tx] is managed in uart_transmit
      */
-    uint32_t omask = (R_INTSTATUS_RXO_MASK | R_INTSTATUS_TXO_MASK);
-    s->intstatus &= ~omask;
-    s->intstatus |= (s->state & (s->ctrl >> 2) & omask);
-
-    qemu_set_irq(s->txint, !!(s->intstatus & R_INTSTATUS_TX_MASK));
-    qemu_set_irq(s->rxint, !!(s->intstatus & R_INTSTATUS_RX_MASK));
-    qemu_set_irq(s->txovrint, !!(s->intstatus & R_INTSTATUS_TXO_MASK));
-    qemu_set_irq(s->rxovrint, !!(s->intstatus & R_INTSTATUS_RXO_MASK));
-    qemu_set_irq(s->uartint, !!(s->intstatus));
+    s->intstatus |= s->state &
+        (R_INTSTATUS_RXO_MASK | R_INTSTATUS_TXO_MASK | R_INTSTATUS_RX_MASK);
+
+    uint32_t masked_intstatus = s->intstatus & (s->ctrl >> 2);
+
+    trace_cmsdk_apb_uart_update(s->state, s->intstatus, masked_intstatus);
+
+    qemu_set_irq(s->txint,    !!(masked_intstatus & R_INTSTATUS_TX_MASK));
+    qemu_set_irq(s->rxint,    !!(masked_intstatus & R_INTSTATUS_RX_MASK));
+    qemu_set_irq(s->txovrint, !!(masked_intstatus & R_INTSTATUS_TXO_MASK));
+    qemu_set_irq(s->rxovrint, !!(masked_intstatus & R_INTSTATUS_RXO_MASK));
+    qemu_set_irq(s->uartint,  !!(masked_intstatus));
 }
 
 static int uart_can_receive(void *opaque)
@@ -144,9 +159,11 @@ static void uart_receive(void *opaque, const uint8_t *buf, 
int size)
 
     s->rxbuf = *buf;
     s->state |= R_STATE_RXFULL_MASK;
-    if (s->ctrl & R_CTRL_RX_INTEN_MASK) {
-        s->intstatus |= R_INTSTATUS_RX_MASK;
-    }
+
+    /*
+     * Handled in cmsdk_apb_uart_update, in order to properly handle
+     * pending rx interrupt when rxen gets enabled
+     */
     cmsdk_apb_uart_update(s);
 }
 
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 81026f6612..0821c8eb3a 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -68,6 +68,7 @@ pl011_put_fifo_full(void) "FIFO now full, RXFF set"
 pl011_baudrate_change(unsigned int baudrate, uint64_t clock, uint32_t ibrd, uint32_t fbrd) "new baudrate %u (clk: 
%" PRIu64 "hz, ibrd: %" PRIu32 ", fbrd: %" PRIu32 ")"
 
 # cmsdk-apb-uart.c
+cmsdk_apb_uart_update(uint32_t state, uint32_t intstatus, uint32_t masked_intstatus) 
"CMSDK APB UART update: state 0x%x intstatus 0x%x masked_intstatus 0x%x"
 cmsdk_apb_uart_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB UART read: offset 
0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 cmsdk_apb_uart_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB UART write: offset 
0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 cmsdk_apb_uart_reset(void) "CMSDK APB UART: reset"
--
2.29.2