On Wed, May 27, 2026 at 6:54 PM Lex Bailey <[email protected]> wrote: > > > On 26/05/2026 04:58, Alistair Francis wrote: > > On Fri, May 1, 2026 at 2:05 AM ~lexbaileylowrisc > <[email protected]> wrote: > > From: Lex Bailey <[email protected]> > > The register definitions for the UART device need to be updated to match > the documentation at https://opentitan.org/book/hw/ip/uart/doc/registers.html > > This commit does that, and also switches to using Fifo8 for managing the > transmit and receive buffers. > > Signed-off-by: Lex Bailey <[email protected]> > --- > hw/char/ot_uart.c | 338 ++++++++++++++++++++++---------------- > include/hw/char/ot_uart.h | 18 +- > 2 files changed, 201 insertions(+), 155 deletions(-) > > diff --git a/hw/char/ot_uart.c b/hw/char/ot_uart.c > index 9478af40f9..3fe7c5efe5 100644 > --- a/hw/char/ot_uart.c > +++ b/hw/char/ot_uart.c > @@ -27,6 +27,7 @@ > > #include "qemu/osdep.h" > #include "hw/char/ot_uart.h" > +#include "qemu/fifo8.h" > #include "hw/core/irq.h" > #include "hw/core/qdev-clock.h" > #include "hw/core/qdev-properties.h" > @@ -37,16 +38,22 @@ > #include "qemu/module.h" > > REG32(INTR_STATE, 0x00) > - FIELD(INTR_STATE, TX_WATERMARK, 0, 1) > - FIELD(INTR_STATE, RX_WATERMARK, 1, 1) > - FIELD(INTR_STATE, TX_EMPTY, 2, 1) > - FIELD(INTR_STATE, RX_OVERFLOW, 3, 1) > + SHARED_FIELD(INTR_TX_WATERMARK, 0, 1) > + SHARED_FIELD(INTR_RX_WATERMARK, 1, 1) > + SHARED_FIELD(INTR_TX_DONE, 2, 1) > + SHARED_FIELD(INTR_RX_OVERFLOW, 3, 1) > + SHARED_FIELD(INTR_RX_FRAME_ERR, 4, 1) > + SHARED_FIELD(INTR_RX_BREAK_ERR, 5, 1) > + SHARED_FIELD(INTR_RX_TIMEOUT, 6, 1) > + SHARED_FIELD(INTR_RX_PARITY_ERR, 7, 1) > + SHARED_FIELD(INTR_TX_EMPTY, 8, 1) > REG32(INTR_ENABLE, 0x04) > REG32(INTR_TEST, 0x08) > REG32(ALERT_TEST, 0x0C) > + FIELD(ALERT_TEST, FATAL_FAULT, 0, 1) > REG32(CTRL, 0x10) > - FIELD(CTRL, TX_ENABLE, 0, 1) > - FIELD(CTRL, RX_ENABLE, 1, 1) > + FIELD(CTRL, TX, 0, 1) > + FIELD(CTRL, RX, 1, 1) > FIELD(CTRL, NF, 2, 1) > FIELD(CTRL, SLPBK, 4, 1) > FIELD(CTRL, LLPBK, 5, 1) > @@ -58,43 +65,74 @@ REG32(STATUS, 0x14) > FIELD(STATUS, TXFULL, 0, 1) > FIELD(STATUS, RXFULL, 1, 1) > FIELD(STATUS, TXEMPTY, 2, 1) > + FIELD(STATUS, TXIDLE, 3, 1) > FIELD(STATUS, RXIDLE, 4, 1) > FIELD(STATUS, RXEMPTY, 5, 1) > REG32(RDATA, 0x18) > + FIELD(RDATA, RDATA, 0, 8) > REG32(WDATA, 0x1C) > + FIELD(WDATA, WDATA, 0, 8) > REG32(FIFO_CTRL, 0x20) > FIELD(FIFO_CTRL, RXRST, 0, 1) > FIELD(FIFO_CTRL, TXRST, 1, 1) > FIELD(FIFO_CTRL, RXILVL, 2, 3) > - FIELD(FIFO_CTRL, TXILVL, 5, 2) > + FIELD(FIFO_CTRL, TXILVL, 5, 3) > REG32(FIFO_STATUS, 0x24) > - FIELD(FIFO_STATUS, TXLVL, 0, 5) > - FIELD(FIFO_STATUS, RXLVL, 16, 5) > + FIELD(FIFO_STATUS, TXLVL, 0, 8) > + FIELD(FIFO_STATUS, RXLVL, 16, 8) > REG32(OVRD, 0x28) > + FIELD(OVRD, TXEN, 0, 1) > + FIELD(OVRD, TXVAL, 1, 1) > REG32(VAL, 0x2C) > + FIELD(VAL, RX, 0, 16) > REG32(TIMEOUT_CTRL, 0x30) > + FIELD(TIMEOUT_CTRL, VAL, 0, 24) > + FIELD(TIMEOUT_CTRL, EN, 31, 1) > + > +#define INTR_MASK \ > + (INTR_TX_WATERMARK_MASK | INTR_RX_WATERMARK_MASK | INTR_TX_DONE_MASK | \ > + INTR_RX_OVERFLOW_MASK | INTR_RX_FRAME_ERR_MASK | INTR_RX_BREAK_ERR_MASK > | \ > + INTR_RX_TIMEOUT_MASK | INTR_RX_PARITY_ERR_MASK | INTR_TX_EMPTY_MASK) > + > +#define CTRL_MASK \ > + (R_CTRL_TX_MASK | R_CTRL_RX_MASK | R_CTRL_NF_MASK | R_CTRL_SLPBK_MASK | \ > + R_CTRL_LLPBK_MASK | R_CTRL_PARITY_EN_MASK | R_CTRL_PARITY_ODD_MASK | \ > + R_CTRL_RXBLVL_MASK | R_CTRL_NCO_MASK) > + > +#define OT_UART_NCO_BITS 16 > +#define OT_UART_TX_FIFO_SIZE 128 > +#define OT_UART_RX_FIFO_SIZE 128 > + > +#define R32_OFF(_r_) ((_r_) / sizeof(uint32_t)) > + > +#define R_LAST_REG (R_TIMEOUT_CTRL) > +#define REGS_COUNT (R_LAST_REG + 1u) > +#define REGS_SIZE (REGS_COUNT * sizeof(uint32_t)) > > static void ot_uart_update_irqs(OtUARTState *s) > { > - if (s->uart_intr_state & s->uart_intr_enable & > R_INTR_STATE_TX_WATERMARK_MASK) { > + if (s->regs[R_INTR_STATE] & s->regs[R_INTR_ENABLE] > + & INTR_TX_WATERMARK_MASK) { > qemu_set_irq(s->tx_watermark, 1); > } else { > qemu_set_irq(s->tx_watermark, 0); > } > > - if (s->uart_intr_state & s->uart_intr_enable & > R_INTR_STATE_RX_WATERMARK_MASK) { > + if (s->regs[R_INTR_STATE] & s->regs[R_INTR_ENABLE] > + & INTR_RX_WATERMARK_MASK) { > qemu_set_irq(s->rx_watermark, 1); > } else { > qemu_set_irq(s->rx_watermark, 0); > } > > - if (s->uart_intr_state & s->uart_intr_enable & > R_INTR_STATE_TX_EMPTY_MASK) { > + if (s->regs[R_INTR_STATE] & s->regs[R_INTR_ENABLE] & INTR_TX_EMPTY_MASK) > { > qemu_set_irq(s->tx_empty, 1); > } else { > qemu_set_irq(s->tx_empty, 0); > } > > - if (s->uart_intr_state & s->uart_intr_enable & > R_INTR_STATE_RX_OVERFLOW_MASK) { > + if (s->regs[R_INTR_STATE] & s->regs[R_INTR_ENABLE] > + & INTR_RX_OVERFLOW_MASK) { > qemu_set_irq(s->rx_overflow, 1); > } else { > qemu_set_irq(s->rx_overflow, 0); > @@ -105,122 +143,133 @@ static int ot_uart_can_receive(void *opaque) > { > OtUARTState *s = opaque; > > - if ((s->uart_ctrl & R_CTRL_RX_ENABLE_MASK) > - && !(s->uart_status & R_STATUS_RXFULL_MASK)) { > - return 1; > + if (s->regs[R_CTRL] & R_CTRL_RX_MASK) { > + return (int)fifo8_num_free(&s->rx_fifo); > } > > return 0; > } > > +static uint32_t ot_uart_get_rx_watermark_level(const OtUARTState *s) > +{ > + uint32_t rx_ilvl = FIELD_EX32(s->regs[R_FIFO_CTRL], FIFO_CTRL, RXILVL); > + > + return rx_ilvl < 7 ? (1 << rx_ilvl) : 126; > +} > + > static void ot_uart_receive(void *opaque, const uint8_t *buf, int size) > { > OtUARTState *s = opaque; > - uint8_t rx_fifo_level = (s->uart_fifo_ctrl & R_FIFO_CTRL_RXILVL_MASK) > - >> R_FIFO_CTRL_RXILVL_SHIFT; > - > - s->uart_rdata = *buf; > + uint32_t rx_watermark_level; > + size_t count = MIN(fifo8_num_free(&s->rx_fifo), (size_t)size); > > - s->uart_status &= ~R_STATUS_RXIDLE_MASK; > - s->uart_status &= ~R_STATUS_RXEMPTY_MASK; > - /* The RXFULL is set after receiving a single byte > - * as the FIFO buffers are not yet implemented. > - */ > - s->uart_status |= R_STATUS_RXFULL_MASK; > - s->rx_level += 1; > + for (int index = 0; index < size; index++) { > + fifo8_push(&s->rx_fifo, buf[index]); > + } > > - if (size > rx_fifo_level) { > - s->uart_intr_state |= R_INTR_STATE_RX_WATERMARK_MASK; > + /* update INTR_STATE */ > + if (count != size) { > + s->regs[R_INTR_STATE] |= INTR_RX_OVERFLOW_MASK; > + } > + rx_watermark_level = ot_uart_get_rx_watermark_level(s); > + if (rx_watermark_level && size >= rx_watermark_level) { > + s->regs[R_INTR_STATE] |= INTR_RX_WATERMARK_MASK; > } > > ot_uart_update_irqs(s); > } > > -static gboolean ot_uart_xmit(void *do_not_use, GIOCondition cond, > - void *opaque) > +static void ot_uart_reset_tx_fifo(OtUARTState *s) > { > - OtUARTState *s = opaque; > - uint8_t tx_fifo_level = (s->uart_fifo_ctrl & R_FIFO_CTRL_TXILVL_MASK) > - >> R_FIFO_CTRL_TXILVL_SHIFT; > - int ret; > - > - /* instant drain the fifo when there's no back-end */ > - if (!qemu_chr_fe_backend_connected(&s->chr)) { > - s->tx_level = 0; > - return G_SOURCE_REMOVE; > + fifo8_reset(&s->tx_fifo); > + s->regs[R_INTR_STATE] |= INTR_TX_EMPTY_MASK; > + s->regs[R_INTR_STATE] |= INTR_TX_DONE_MASK; > + if (s->tx_watermark_level) { > + s->regs[R_INTR_STATE] |= INTR_TX_WATERMARK_MASK; > + s->tx_watermark_level = 0; > } > +} > > - if (!s->tx_level) { > - s->uart_status &= ~R_STATUS_TXFULL_MASK; > - s->uart_status |= R_STATUS_TXEMPTY_MASK; > - s->uart_intr_state |= R_INTR_STATE_TX_EMPTY_MASK; > - s->uart_intr_state &= ~R_INTR_STATE_TX_WATERMARK_MASK; > - ot_uart_update_irqs(s); > - return G_SOURCE_REMOVE; > +static void ot_uart_reset_rx_fifo(OtUARTState *s) > +{ > + fifo8_reset(&s->rx_fifo); > + s->regs[R_INTR_STATE] &= ~INTR_RX_WATERMARK_MASK; > + s->regs[R_INTR_STATE] &= ~INTR_RX_OVERFLOW_MASK; > + if (FIELD_EX32(s->regs[R_CTRL], CTRL, RX)) { > + qemu_chr_fe_accept_input(&s->chr); > } > +} > > - ret = qemu_chr_fe_write(&s->chr, s->tx_fifo, s->tx_level); > +static uint32_t ot_uart_get_tx_watermark_level(const OtUARTState *s) > +{ > + uint32_t tx_ilvl = FIELD_EX32(s->regs[R_FIFO_CTRL], FIFO_CTRL, TXILVL); > > - if (ret >= 0) { > - s->tx_level -= ret; > - memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_level); > - } > + return tx_ilvl < 7 ? (1 << tx_ilvl) : 64; > +} > > - if (s->tx_level) { > - guint r = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP, > - ot_uart_xmit, s); > - if (!r) { > - s->tx_level = 0; > - return G_SOURCE_REMOVE; > - } > +static void ot_uart_xmit(OtUARTState *s) > +{ > + const uint8_t *buf; > + uint32_t size; > + int ret; > + > + if (fifo8_is_empty(&s->tx_fifo)) { > + return; > } > > - /* Clear the TX Full bit */ > - if (s->tx_level != OT_UART_TX_FIFO_SIZE) { > - s->uart_status &= ~R_STATUS_TXFULL_MASK; > + /* instant drain the fifo when there's no back-end */ > + if (!qemu_chr_fe_backend_connected(&s->chr)) { > + ot_uart_reset_tx_fifo(s); > + ot_uart_update_irqs(s); > + return; > } > > - /* Disable the TX_WATERMARK IRQ */ > - if (s->tx_level < tx_fifo_level) { > - s->uart_intr_state &= ~R_INTR_STATE_TX_WATERMARK_MASK; > + /* get a continuous buffer from the FIFO */ > + buf = > + fifo8_peek_bufptr(&s->tx_fifo, fifo8_num_used(&s->tx_fifo), &size); > + /* send as much as possible */ > + ret = qemu_chr_fe_write(&s->chr, buf, (int)size); > + /* if some characters where sent, remove them from the FIFO */ > + if (ret >= 0) { > + fifo8_drop(&s->tx_fifo, ret); > } > > - /* Set TX empty */ > - if (s->tx_level == 0) { > - s->uart_status |= R_STATUS_TXEMPTY_MASK; > - s->uart_intr_state |= R_INTR_STATE_TX_EMPTY_MASK; > + /* update INTR_STATE */ > + if (fifo8_is_empty(&s->tx_fifo)) { > + s->regs[R_INTR_STATE] |= INTR_TX_EMPTY_MASK; > + s->regs[R_INTR_STATE] |= INTR_TX_DONE_MASK; > + } > + if (s->tx_watermark_level && > + fifo8_num_used(&s->tx_fifo) < s->tx_watermark_level) { > + s->regs[R_INTR_STATE] |= INTR_TX_WATERMARK_MASK; > + s->tx_watermark_level = 0; > } > > ot_uart_update_irqs(s); > - return G_SOURCE_REMOVE; > } > > -static void uart_write_tx_fifo(OtUARTState *s, const uint8_t *buf, > - int size) > +static void uart_write_tx_fifo(OtUARTState *s, uint8_t val) > { > uint64_t current_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > - uint8_t tx_fifo_level = (s->uart_fifo_ctrl & R_FIFO_CTRL_TXILVL_MASK) > - >> R_FIFO_CTRL_TXILVL_SHIFT; > - > - if (size > OT_UART_TX_FIFO_SIZE - s->tx_level) { > - size = OT_UART_TX_FIFO_SIZE - s->tx_level; > + if (fifo8_is_full(&s->tx_fifo)) { > qemu_log_mask(LOG_GUEST_ERROR, "ot_uart: TX FIFO overflow"); > + return; > } > > - memcpy(s->tx_fifo + s->tx_level, buf, size); > - s->tx_level += size; > + fifo8_push(&s->tx_fifo, val); > > - if (s->tx_level > 0) { > - s->uart_status &= ~R_STATUS_TXEMPTY_MASK; > + s->tx_watermark_level = ot_uart_get_tx_watermark_level(s); > + if (fifo8_num_used(&s->tx_fifo) < s->tx_watermark_level) { > + /* > + * TX watermark interrupt is raised when FIFO depth goes from above > + * watermark to below. If we haven't reached watermark, reset cached > + * watermark level > + */ > + s->tx_watermark_level = 0; > } > > Don't we need to update the interrupts at this point? > > Can you clarify what you mean by this? Are you wanting me to squash the next > commit in to this one so that the IRQ change happens simultaneously with this > one?
Basically you have updated a bunch of state that could cause an interrupt, but aren't checking if you should generate an interrupt. It looks like you will miss an interrupt here. I'm not sure about squashing, but each commit should not break existing code, and it looks like this will break interrupts. Alistair
