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

[Peter Maydell]

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