| Message ID | 20250208163911.54522-6-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop | expand |
On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > When transmission is disabled, characters are still queued > to the FIFO which eventually overruns. Report that error > condition in the status register. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/char/pl011.c | 20 ++++++++++++++++++++ > hw/char/trace-events | 2 ++ > 2 files changed, 22 insertions(+) > > diff --git a/hw/char/pl011.c b/hw/char/pl011.c > index 447f185e2d5..ef39ab666a2 100644 > --- a/hw/char/pl011.c > +++ b/hw/char/pl011.c > @@ -61,6 +61,9 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr) > /* Data Register, UARTDR */ > #define DR_BE (1 << 10) > > +/* Receive Status Register/Error Clear Register, UARTRSR/UARTECR */ > +#define RSR_OE (1 << 3) > + > /* Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC */ > #define INT_OE (1 << 10) > #define INT_BE (1 << 9) > @@ -158,6 +161,16 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s) > return pl011_is_fifo_enabled(s) ? PL011_FIFO_DEPTH : 1; > } > > +static bool pl011_is_tx_fifo_full(PL011State *s) > +{ > + if (pl011_is_fifo_enabled(s)) { > + trace_pl011_fifo_tx_is_full("FIFO", fifo8_is_full(&s->xmit_fifo)); > + return fifo8_is_full(&s->xmit_fifo); > + } > + trace_pl011_fifo_tx_is_full("CHAR", !fifo8_is_empty(&s->xmit_fifo)); > + return !fifo8_is_empty(&s->xmit_fifo); More repetition of expressions, but anyway Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Mon, 17 Feb 2025 at 14:29, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > > When transmission is disabled, characters are still queued > > to the FIFO which eventually overruns. Report that error > > condition in the status register. > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > --- > > hw/char/pl011.c | 20 ++++++++++++++++++++ > > hw/char/trace-events | 2 ++ > > 2 files changed, 22 insertions(+) > > > > diff --git a/hw/char/pl011.c b/hw/char/pl011.c > > index 447f185e2d5..ef39ab666a2 100644 > > --- a/hw/char/pl011.c > > +++ b/hw/char/pl011.c > > @@ -61,6 +61,9 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr) > > /* Data Register, UARTDR */ > > #define DR_BE (1 << 10) > > > > +/* Receive Status Register/Error Clear Register, UARTRSR/UARTECR */ > > +#define RSR_OE (1 << 3) > > + > > /* Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC */ > > #define INT_OE (1 << 10) > > #define INT_BE (1 << 9) > > @@ -158,6 +161,16 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s) > > return pl011_is_fifo_enabled(s) ? PL011_FIFO_DEPTH : 1; > > } > > > > +static bool pl011_is_tx_fifo_full(PL011State *s) > > +{ > > + if (pl011_is_fifo_enabled(s)) { > > + trace_pl011_fifo_tx_is_full("FIFO", fifo8_is_full(&s->xmit_fifo)); > > + return fifo8_is_full(&s->xmit_fifo); > > + } > > + trace_pl011_fifo_tx_is_full("CHAR", !fifo8_is_empty(&s->xmit_fifo)); > > + return !fifo8_is_empty(&s->xmit_fifo); > > More repetition of expressions, but anyway > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Here I propose to squash in this tweak: --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -165,12 +165,13 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s) static bool pl011_is_tx_fifo_full(PL011State *s) { - if (pl011_is_fifo_enabled(s)) { - trace_pl011_fifo_tx_is_full("FIFO", fifo8_is_full(&s->xmit_fifo)); - return fifo8_is_full(&s->xmit_fifo); - } - trace_pl011_fifo_tx_is_full("CHAR", !fifo8_is_empty(&s->xmit_fifo)); - return !fifo8_is_empty(&s->xmit_fifo); + bool fifo_enabled = pl011_is_fifo_enabled(s); + bool tx_fifo_full = fifo_enabled ? + fifo8_is_full(&s->xmit_fifo) : !fifo8_is_empty(&s->xmit_fifo); + + trace_pl011_fifo_tx_is_full(fifo_enabled ? "FIFO" : "CHAR", + tx_fifo_full); + return tx_fifo_full; } thanks -- PMM
On 17/2/25 15:52, Peter Maydell wrote: > On Mon, 17 Feb 2025 at 14:29, Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >>> >>> When transmission is disabled, characters are still queued >>> to the FIFO which eventually overruns. Report that error >>> condition in the status register. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/char/pl011.c | 20 ++++++++++++++++++++ >>> hw/char/trace-events | 2 ++ >>> 2 files changed, 22 insertions(+) >>> >>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c >>> index 447f185e2d5..ef39ab666a2 100644 >>> --- a/hw/char/pl011.c >>> +++ b/hw/char/pl011.c >>> @@ -61,6 +61,9 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr) >>> /* Data Register, UARTDR */ >>> #define DR_BE (1 << 10) >>> >>> +/* Receive Status Register/Error Clear Register, UARTRSR/UARTECR */ >>> +#define RSR_OE (1 << 3) >>> + >>> /* Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC */ >>> #define INT_OE (1 << 10) >>> #define INT_BE (1 << 9) >>> @@ -158,6 +161,16 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s) >>> return pl011_is_fifo_enabled(s) ? PL011_FIFO_DEPTH : 1; >>> } >>> >>> +static bool pl011_is_tx_fifo_full(PL011State *s) >>> +{ >>> + if (pl011_is_fifo_enabled(s)) { >>> + trace_pl011_fifo_tx_is_full("FIFO", fifo8_is_full(&s->xmit_fifo)); >>> + return fifo8_is_full(&s->xmit_fifo); >>> + } >>> + trace_pl011_fifo_tx_is_full("CHAR", !fifo8_is_empty(&s->xmit_fifo)); >>> + return !fifo8_is_empty(&s->xmit_fifo); >> >> More repetition of expressions, but anyway >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > Here I propose to squash in this tweak: > > --- a/hw/char/pl011.c > +++ b/hw/char/pl011.c > @@ -165,12 +165,13 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s) > > static bool pl011_is_tx_fifo_full(PL011State *s) > { > - if (pl011_is_fifo_enabled(s)) { > - trace_pl011_fifo_tx_is_full("FIFO", fifo8_is_full(&s->xmit_fifo)); > - return fifo8_is_full(&s->xmit_fifo); > - } > - trace_pl011_fifo_tx_is_full("CHAR", !fifo8_is_empty(&s->xmit_fifo)); > - return !fifo8_is_empty(&s->xmit_fifo); > + bool fifo_enabled = pl011_is_fifo_enabled(s); > + bool tx_fifo_full = fifo_enabled ? > + fifo8_is_full(&s->xmit_fifo) : !fifo8_is_empty(&s->xmit_fifo); > + > + trace_pl011_fifo_tx_is_full(fifo_enabled ? "FIFO" : "CHAR", > + tx_fifo_full); > + return tx_fifo_full; > } Thank you, appreciated! > > thanks > -- PMM
diff --git a/hw/char/pl011.c b/hw/char/pl011.c index 447f185e2d5..ef39ab666a2 100644 --- a/hw/char/pl011.c +++ b/hw/char/pl011.c @@ -61,6 +61,9 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr) /* Data Register, UARTDR */ #define DR_BE (1 << 10) +/* Receive Status Register/Error Clear Register, UARTRSR/UARTECR */ +#define RSR_OE (1 << 3) + /* Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC */ #define INT_OE (1 << 10) #define INT_BE (1 << 9) @@ -158,6 +161,16 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s) return pl011_is_fifo_enabled(s) ? PL011_FIFO_DEPTH : 1; } +static bool pl011_is_tx_fifo_full(PL011State *s) +{ + if (pl011_is_fifo_enabled(s)) { + trace_pl011_fifo_tx_is_full("FIFO", fifo8_is_full(&s->xmit_fifo)); + return fifo8_is_full(&s->xmit_fifo); + } + trace_pl011_fifo_tx_is_full("CHAR", !fifo8_is_empty(&s->xmit_fifo)); + return !fifo8_is_empty(&s->xmit_fifo); +} + static inline void pl011_reset_rx_fifo(PL011State *s) { s->read_count = 0; @@ -264,6 +277,13 @@ static void pl011_write_txdata(PL011State *s, uint8_t data) "PL011 data written to disabled TX UART\n"); } + if (pl011_is_tx_fifo_full(s)) { + /* The FIFO is already full. Content remains valid. */ + trace_pl011_fifo_tx_overrun(); + s->rsr |= RSR_OE; + return; + } + trace_pl011_fifo_tx_put(data); pl011_loopback_tx(s, data); fifo8_push(&s->xmit_fifo, data); diff --git a/hw/char/trace-events b/hw/char/trace-events index dd635ac6012..8234f3afa13 100644 --- a/hw/char/trace-events +++ b/hw/char/trace-events @@ -67,9 +67,11 @@ pl011_fifo_enable(bool enable) "enable:%u" pl011_fifo_is_enabled(bool enabled) "enabled:%u" pl011_fifo_rx_put(uint32_t c, int read_count) "new char 0x%02x read_count now %d" pl011_fifo_rx_full(void) "RX FIFO now full, RXFF set" +pl011_fifo_tx_is_full(const char *desc, bool full) "mode:%s full:%u" pl011_fifo_tx_put(uint8_t byte) "TX FIFO push char [0x%02x]" pl011_fifo_tx_xmit_used(unsigned sent) "TX FIFO used %u chars" pl011_fifo_tx_xmit_consumed(unsigned sent) "TX FIFO consumed %u chars" +pl011_fifo_tx_overrun(void) "TX FIFO overrun" 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
When transmission is disabled, characters are still queued to the FIFO which eventually overruns. Report that error condition in the status register. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/char/pl011.c | 20 ++++++++++++++++++++ hw/char/trace-events | 2 ++ 2 files changed, 22 insertions(+)