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 <tpeca...@gmail.com <mailto:tpeca...@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 >
