On 17. 11. 20 17:38, Peter Maydell wrote: > On Mon, 16 Nov 2020 at 19:58, Tadej Pečar <[email protected]> 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
