hw/char/cmsdk-apb-uart.c | 54 ++++++++++++++++++++++++++++------------ hw/char/trace-events | 1 + 2 files changed, 39 insertions(+), 16 deletions(-)
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>
---
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
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 >
© 2016 - 2024 Red Hat, Inc.