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

[Philippe Mathieu-Daudé]

Hi Tadej,

On 11/16/20 4:11 PM, Tadej Pecar wrote:
> Previously, the RX interrupt got missed if:
> - the character backend provided the next character before the RX IRQ
> Handler
>   managed to clear the currently served interrupt.
> - the character backend provided the next character while the RX interrupt
>   was disabled. Enabling the interrupt did not trigger the interrupt
>   even if the RXFULL status bit was set.
> 
> Signed-off-by: Tadej P <tpecar95@gmail.com <mailto:tpecar95@gmail.com>>

Thanks for your patch!

Minor comment, your git seems mis-configured (full name is expected
here: Tadej Pecar).

You might fix by running in a shell:

$ git config user.name "Tadej Pecar"

See:
https://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line

> ---
>  hw/char/cmsdk-apb-uart.c | 54 ++++++++++++++++++++++++++++------------
>  hw/char/trace-events     |  1 +
>  2 files changed, 39 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/char/cmsdk-apb-uart.c b/hw/char/cmsdk-apb-uart.c
> index 626b68f2ec..1b361bc4d6 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);
>  }
>  
> @@ -278,7 +295,12 @@ static void uart_write(void *opaque, hwaddr offset,
> uint64_t value,
>           * is then reflected into the intstatus value by the update
> function).
>           */
>          s->state &= ~(value & (R_INTSTATUS_TXO_MASK |
> R_INTSTATUS_RXO_MASK));
> -        s->intstatus &= ~value;
> +
> +        /*
> +         * Clear rx interrupt status only if no pending character
> +         * (no buffer full asserted).
> +         */
> +        s->intstatus &= ~value | (s->state & R_STATE_RXFULL_MASK);
>          cmsdk_apb_uart_update(s);
>          break;
>      case A_BAUDDIV:
> 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
>