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

On Mon, 16 Nov 2020 at 19:58, Tadej Pečar <tpecar95@gmail.com> wrote:
>
> 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).
>
>
> 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);

I don't think this logic is correct. It makes the values we
send out on the output interrupt lines different from the
values visible in the INTSTATUS register bits, and I don't
think that's how the hardware behaves.

thanks
-- PMM

On 17. 11. 20 17:38, Peter Maydell wrote:
> On Mon, 16 Nov 2020 at 19:58, Tadej Pečar <tpecar95@gmail.com> wrote:
>>
>> 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).
>>
>>
>> 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);
> 
> I don't think this logic is correct. It makes the values we
> send out on the output interrupt lines different from the
> values visible in the INTSTATUS register bits, and I don't
> think that's how the hardware behaves.
> 
> thanks
> -- PMM
> 

Yep, it seems that I stand corrected. More so, it seems that I was completely wrong.

I've had a closer look at the cmsdk_apb_uart.v HDL file, available in the 
Cortex-M0/M3 DesignStart, and the interrupt lines are directly assigned to 
INTSTATUS.

Additionally, it seems that the actual hardware suffers from the same issues 
described as "fixed" by this patch:

 - If you happen to be in the interrupt handler for long enough to receive
   the next character, STATE will report RX buffer full / overrun, depending
   on whether you've already read the current character from DATA.
   
   Which is fine and dandy - but if you happen to clear INTSTATUS _after_ you
   have received the next character, you've essentially cleared the next interrupt,
   so the next character won't get handled.

   This is because INTSTATUS register logic is independent from the STATE register.

 - RX / TX interrupt lines are masked by interrupt enable _before_ they are 
   registered, and the rx'd / tx'd status from the state machine
   is present only for one clock cycle.
   
   So the interrupts are ignored when they are disabled by CTRL, and they don't
   get fired at interrupt enable, regardless of the current STATE.
   
   Interestingly enough, RX / TX overrun interrupts are masked _after_ they
   are registered, so enabling the interrupt for those should trigger the 
   interrupt (as correctly modeled by the current cmsdk-apb-uart).
   
Guess I wanted my hardware emulation to be better than the actual hardware ;)

Jokes aside, I guess these issues weren't apparent in hardware because serial 
communication is usually so much slower that
 - the mcu could always clear the interrupt before the next character was 
   received.
 - the time when the rx interrupt was disabled
   was always shorter than the time required to receive the next character.

The uart emulation could be made a little more forgiving by postponing the
next character until the current interrupt is finished, but that's probably for
some other discussion.


In the end, the patch is unnecessary, as the current cmsdk-apb-uart
provides a more faithful emulation of the actual hardware, along with its warts.

It was educational, at least.

Regards, TP