On Thu, Mar 12, 2026 at 11:31:58AM +0800, [email protected] wrote: > From: Frank Chang <[email protected]> > > Implement txctrl.txen and rxctrl.rxen as follows: > > * txctrl.txen > The txen bit controls whether the Tx channel is active. When cleared, > transmission of Tx FIFO contents is suppressed, and the txd pin is > driven high. > > * rxctrl.rxen: > The rxen bit controls whether the Rx channel is active. When cleared, > the state of the rxd pin is ignored, and no characters will be > enqueued into the Rx FIFO. > > Therefore, the Tx FIFO should not be dequeued when txctrl.txen is > cleared, and the Rx FIFO should not be enqueued when rxctrl.rxen is > cleared. > > Signed-off-by: Frank Chang <[email protected]> > --- > hw/char/sifive_uart.c | 27 ++++++++++++++++++++------- > include/hw/char/sifive_uart.h | 2 ++ > 2 files changed, 22 insertions(+), 7 deletions(-) > > diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c > index af17cf9a6ce..3ce6a4ee76a 100644 > --- a/hw/char/sifive_uart.c > +++ b/hw/char/sifive_uart.c > @@ -78,6 +78,11 @@ static gboolean sifive_uart_xmit(void *do_not_use, > GIOCondition cond, > return G_SOURCE_REMOVE; > } > > + /* Don't pop the FIFO if transmit is disabled. */ > + if (!SIFIVE_UART_TXEN(s->txctrl)) { > + return G_SOURCE_REMOVE; > + } > + > /* Don't pop the FIFO in case the write fails */ > characters = fifo8_peek_bufptr(&s->tx_fifo, > fifo8_num_used(&s->tx_fifo), &numptr); > @@ -106,11 +111,19 @@ static gboolean sifive_uart_xmit(void *do_not_use, > GIOCondition cond, > return G_SOURCE_REMOVE; > } > > -static void sifive_uart_write_tx_fifo(SiFiveUARTState *s, const uint8_t *buf, > - int size) > +static void sifive_uart_trigger_tx_fifo(SiFiveUARTState *s) > { > uint64_t current_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > + if (!timer_pending(s->fifo_trigger_handle)) { > + timer_mod(s->fifo_trigger_handle, current_time + > + TX_INTERRUPT_TRIGGER_DELAY_NS); > + } > +} > + > +static void sifive_uart_write_tx_fifo(SiFiveUARTState *s, const uint8_t *buf, > + int size) > +{ > if (size > fifo8_num_free(&s->tx_fifo)) { > size = fifo8_num_free(&s->tx_fifo); > qemu_log_mask(LOG_GUEST_ERROR, "sifive_uart: TX FIFO overflow.\n"); > @@ -124,10 +137,7 @@ static void sifive_uart_write_tx_fifo(SiFiveUARTState > *s, const uint8_t *buf, > s->txfifo |= SIFIVE_UART_TXFIFO_FULL; > } > > - if (!timer_pending(s->fifo_trigger_handle)) { > - timer_mod(s->fifo_trigger_handle, current_time + > - TX_INTERRUPT_TRIGGER_DELAY_NS); > - } > + sifive_uart_trigger_tx_fifo(s); > } > > static uint64_t > @@ -184,6 +194,9 @@ sifive_uart_write(void *opaque, hwaddr addr, > return; > case SIFIVE_UART_TXCTRL: > s->txctrl = val64; A sifive_uart_update_irq() call is also needed here. When txctrl is written, the watermark (txcnt) may change, or txen may toggle(you can see SiFive FE310-G002 datasheet), both of which can alter the txwm pending state.
Patch 3/4 adds this call for RXCTRL with the rationale "the user may activate the Rx channel or change the Rx FIFO watermark level" -- the same reasoning applies to TXCTRL. Without this, there's a window where the IRQ line doesn't reflect the actual pending state after a txctrl write that changes the watermark. Everything else looks good to me. Reviewed-by: Chao Liu <[email protected]> Thanks, Chao > + if (SIFIVE_UART_TXEN(s->txctrl) && !fifo8_is_empty(&s->tx_fifo)) { > + sifive_uart_trigger_tx_fifo(s); > + } > return; > case SIFIVE_UART_RXCTRL: > s->rxctrl = val64; > @@ -231,7 +244,7 @@ static int sifive_uart_can_rx(void *opaque) > { > SiFiveUARTState *s = opaque; > > - return s->rx_fifo_len < sizeof(s->rx_fifo); > + return SIFIVE_UART_RXEN(s->rxctrl) && (s->rx_fifo_len < > sizeof(s->rx_fifo)); > } > > static void sifive_uart_event(void *opaque, QEMUChrEvent event) > diff --git a/include/hw/char/sifive_uart.h b/include/hw/char/sifive_uart.h > index 414564b0266..c78d9bd1fc6 100644 > --- a/include/hw/char/sifive_uart.h > +++ b/include/hw/char/sifive_uart.h > @@ -51,6 +51,8 @@ enum { > > #define SIFIVE_UART_TXFIFO_FULL 0x80000000 > > +#define SIFIVE_UART_TXEN(txctrl) (txctrl & 0x1) > +#define SIFIVE_UART_RXEN(rxctrl) (rxctrl & 0x1) macro arguments should be parenthesized to avoid precedence surprises. e.g. if the argument is ever an expression like (a | b), & binds tighter than |. I suggest: #define SIFIVE_UART_TXEN(txctrl) ((txctrl) & 0x1) #define SIFIVE_UART_RXEN(rxctrl) ((rxctrl) & 0x1) The existing GET_TXCNT / GET_RXCNT macros have the same issue, so this could be a follow-up cleanup. > #define SIFIVE_UART_GET_TXCNT(txctrl) ((txctrl >> 16) & 0x7) > #define SIFIVE_UART_GET_RXCNT(rxctrl) ((rxctrl >> 16) & 0x7) > > -- > 2.43.0 > >
