Re: [PATCH v6 5/7] hw/char/pl011: Consider TX FIFO overrun error

[Philippe Mathieu-Daudé]

On 17/2/25 15:52, Peter Maydell wrote:

On Mon, 17 Feb 2025 at 14:29, Peter Maydell <peter.mayd...@linaro.org> wrote:

On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <phi...@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é <phi...@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.mayd...@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