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

[Peter Maydell <peter.maydell@linaro.org>]

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