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

[Tadej Pecar]

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