[PATCH v2 0/9] hw/char: Improve RX FIFO depth uses

[PATCH v2 0/9] hw/char: Improve RX FIFO depth uses

[Philippe Mathieu-Daudé]

Since v1:
- Fixed typos (Luc)

Some UART devices implement a RX FIFO but their code
(via IOCanReadHandler) only return a size of 1 element,
while we can receive more chars.

This series takes advantage of the full depth.

Inspired by pm215 chat comment on yesterday's community
meeting on the PL011 Rust implementation description by
Paolo :)

Philippe Mathieu-Daudé (9):
  hw/char/pl011: Warn when using disabled receiver
  hw/char/pl011: Simplify a bit pl011_can_receive()
  hw/char/pl011: Improve RX flow tracing events
  hw/char/pl011: Really use RX FIFO depth
  hw/char/bcm2835_aux: Really use RX FIFO depth
  hw/char/imx_serial: Really use RX FIFO depth
  hw/char/mcf_uart: Use FIFO_DEPTH definition instead of magic values
  hw/char/mcf_uart: Really use RX FIFO depth
  hw/char/sh_serial: Return correct number of empty RX FIFO elements

 hw/char/bcm2835_aux.c |  6 ++++--
 hw/char/imx_serial.c  |  8 ++++++--
 hw/char/mcf_uart.c    | 16 +++++++++++-----
 hw/char/pl011.c       | 30 ++++++++++++++++++++++--------
 hw/char/sh_serial.c   | 30 ++++++++++++++----------------
 hw/char/trace-events  |  7 ++++---
 6 files changed, 61 insertions(+), 36 deletions(-)

-- 
2.47.1

[PATCH v2 1/9] hw/char/pl011: Warn when using disabled receiver

[Philippe Mathieu-Daudé]

We shouldn't receive characters when the full UART or its
receiver is disabled. However we don't want to break the
possibly incomplete "my first bare metal assembly program"s,
so we choose to simply display a warning when this occurs.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/pl011.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 06ce851044d..12a2d4bc7bd 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -85,6 +85,7 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
 #define CR_OUT1     (1 << 12)
 #define CR_RTS      (1 << 11)
 #define CR_DTR      (1 << 10)
+#define CR_RXE      (1 << 9)
 #define CR_TXE      (1 << 8)
 #define CR_LBE      (1 << 7)
 #define CR_UARTEN   (1 << 0)
@@ -487,6 +488,14 @@ static int pl011_can_receive(void *opaque)
     PL011State *s = (PL011State *)opaque;
     int r;
 
+    if (!(s->cr & CR_UARTEN)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "PL011 receiving data on disabled UART\n");
+    }
+    if (!(s->cr & CR_RXE)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "PL011 receiving data on disabled RX UART\n");
+    }
     r = s->read_count < pl011_get_fifo_depth(s);
     trace_pl011_can_receive(s->lcr, s->read_count, r);
     return r;
-- 
2.47.1

[PATCH v2 2/9] hw/char/pl011: Simplify a bit pl011_can_receive()

[Philippe Mathieu-Daudé]

Introduce 'fifo_depth' and 'fifo_available' local variables
to better express the 'r' variable use.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Luc Michel <luc.michel@amd.com>
---
 hw/char/pl011.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 12a2d4bc7bd..5bb83c54216 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -486,7 +486,9 @@ static void pl011_write(void *opaque, hwaddr offset,
 static int pl011_can_receive(void *opaque)
 {
     PL011State *s = (PL011State *)opaque;
-    int r;
+    unsigned fifo_depth = pl011_get_fifo_depth(s);
+    unsigned fifo_available = fifo_depth - s->read_count;
+    int r = fifo_available ? 1 : 0;
 
     if (!(s->cr & CR_UARTEN)) {
         qemu_log_mask(LOG_GUEST_ERROR,
@@ -496,7 +498,6 @@ static int pl011_can_receive(void *opaque)
         qemu_log_mask(LOG_GUEST_ERROR,
                       "PL011 receiving data on disabled RX UART\n");
     }
-    r = s->read_count < pl011_get_fifo_depth(s);
     trace_pl011_can_receive(s->lcr, s->read_count, r);
     return r;
 }
-- 
2.47.1

[PATCH v2 3/9] hw/char/pl011: Improve RX flow tracing events

[Philippe Mathieu-Daudé]

Log FIFO use (availability and depth).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Luc Michel <luc.michel@amd.com>
---
 hw/char/pl011.c      | 10 ++++++----
 hw/char/trace-events |  7 ++++---
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 5bb83c54216..f7485e7c541 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -185,7 +185,7 @@ static void pl011_fifo_rx_put(void *opaque, uint32_t value)
     s->read_fifo[slot] = value;
     s->read_count++;
     s->flags &= ~PL011_FLAG_RXFE;
-    trace_pl011_fifo_rx_put(value, s->read_count);
+    trace_pl011_fifo_rx_put(value, s->read_count, pipe_depth);
     if (s->read_count == pipe_depth) {
         trace_pl011_fifo_rx_full();
         s->flags |= PL011_FLAG_RXFF;
@@ -248,12 +248,13 @@ static void pl011_write_txdata(PL011State *s, uint8_t data)
 static uint32_t pl011_read_rxdata(PL011State *s)
 {
     uint32_t c;
+    unsigned fifo_depth = pl011_get_fifo_depth(s);
 
     s->flags &= ~PL011_FLAG_RXFF;
     c = s->read_fifo[s->read_pos];
     if (s->read_count > 0) {
         s->read_count--;
-        s->read_pos = (s->read_pos + 1) & (pl011_get_fifo_depth(s) - 1);
+        s->read_pos = (s->read_pos + 1) & (fifo_depth - 1);
     }
     if (s->read_count == 0) {
         s->flags |= PL011_FLAG_RXFE;
@@ -261,7 +262,7 @@ static uint32_t pl011_read_rxdata(PL011State *s)
     if (s->read_count == s->read_trigger - 1) {
         s->int_level &= ~INT_RX;
     }
-    trace_pl011_read_fifo(s->read_count);
+    trace_pl011_read_fifo(s->read_count, fifo_depth);
     s->rsr = c >> 8;
     pl011_update(s);
     qemu_chr_fe_accept_input(&s->chr);
@@ -498,12 +499,13 @@ static int pl011_can_receive(void *opaque)
         qemu_log_mask(LOG_GUEST_ERROR,
                       "PL011 receiving data on disabled RX UART\n");
     }
-    trace_pl011_can_receive(s->lcr, s->read_count, r);
+    trace_pl011_can_receive(s->lcr, s->read_count, fifo_depth, fifo_available);
     return r;
 }
 
 static void pl011_receive(void *opaque, const uint8_t *buf, int size)
 {
+    trace_pl011_receive(size);
     /*
      * In loopback mode, the RX input signal is internally disconnected
      * from the entire receiving logics; thus, all inputs are ignored,
diff --git a/hw/char/trace-events b/hw/char/trace-events
index b2e3d25ae34..05a33036c12 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -60,12 +60,13 @@ imx_serial_put_data(const char *chrname, uint32_t value) "%s: 0x%" PRIx32
 # pl011.c
 pl011_irq_state(int level) "irq state %d"
 pl011_read(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x value 0x%08x reg %s"
-pl011_read_fifo(int read_count) "FIFO read, read_count now %d"
+pl011_read_fifo(unsigned rx_fifo_used, size_t rx_fifo_depth) "RX FIFO read, used %u/%zu"
 pl011_write(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x value 0x%08x reg %s"
-pl011_can_receive(uint32_t lcr, int read_count, int r) "LCR 0x%08x read_count %d returning %d"
-pl011_fifo_rx_put(uint32_t c, int read_count) "new char 0x%02x read_count now %d"
+pl011_can_receive(uint32_t lcr, unsigned rx_fifo_used, size_t rx_fifo_depth, unsigned rx_fifo_available) "LCR 0x%02x, RX FIFO used %u/%zu, can_receive %u chars"
+pl011_fifo_rx_put(uint32_t c, unsigned read_count, size_t rx_fifo_depth) "RX FIFO push char [0x%02x] %d/%zu depth used"
 pl011_fifo_rx_full(void) "RX FIFO now full, RXFF set"
 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 ")"
+pl011_receive(int size) "recv %d chars"
 
 # cmsdk-apb-uart.c
 cmsdk_apb_uart_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB UART read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
-- 
2.47.1

[PATCH v2 4/9] hw/char/pl011: Really use RX FIFO depth

[Philippe Mathieu-Daudé]

While we model a 16-elements RX FIFO since the PL011 model was
introduced in commit cdbdb648b7c ("ARM Versatile Platform Baseboard
emulation"), we only read 1 char at a time!

Have the IOCanReadHandler handler return how many elements are
available, and use that in the IOReadHandler handler.

Example of FIFO better used by enabling the pl011 tracing events
and running the tests/functional/test_aarch64_virt.py tests:

  pl011_can_receive LCR 0x70, RX FIFO used 0/16, can_receive 16 chars
  pl011_receive recv 5 chars
  pl011_fifo_rx_put RX FIFO push char [0x72] 1/16 depth used
  pl011_irq_state irq state 1
  pl011_fifo_rx_put RX FIFO push char [0x6f] 2/16 depth used
  pl011_fifo_rx_put RX FIFO push char [0x6f] 3/16 depth used
  pl011_fifo_rx_put RX FIFO push char [0x74] 4/16 depth used
  pl011_fifo_rx_put RX FIFO push char [0x0d] 5/16 depth used
  pl011_can_receive LCR 0x70, RX FIFO used 5/16, can_receive 11 chars
  pl011_can_receive LCR 0x70, RX FIFO used 5/16, can_receive 11 chars
  pl011_write addr 0x038 value 0x00000050 reg IMSC
  pl011_irq_state irq state 1
  pl011_can_receive LCR 0x70, RX FIFO used 5/16, can_receive 11 chars
  pl011_read addr 0x03c value 0x00000030 reg RIS
  pl011_write addr 0x044 value 0x00000000 reg ICR
  pl011_irq_state irq state 1
  pl011_read addr 0x018 value 0x00000080 reg FR
  pl011_read_fifo RX FIFO read, used 4/16
  pl011_irq_state irq state 1
  pl011_read addr 0x000 value 0x00000072 reg DR
  pl011_can_receive LCR 0x70, RX FIFO used 4/16, can_receive 12 chars
  pl011_read addr 0x018 value 0x00000080 reg FR
  pl011_read_fifo RX FIFO read, used 3/16
  pl011_irq_state irq state 1
  pl011_read addr 0x000 value 0x0000006f reg DR
  pl011_can_receive LCR 0x70, RX FIFO used 3/16, can_receive 13 chars
  pl011_read addr 0x018 value 0x00000080 reg FR
  pl011_read_fifo RX FIFO read, used 2/16
  pl011_irq_state irq state 1
  pl011_read addr 0x000 value 0x0000006f reg DR
  pl011_can_receive LCR 0x70, RX FIFO used 2/16, can_receive 14 chars
  pl011_read addr 0x018 value 0x00000080 reg FR
  pl011_read_fifo RX FIFO read, used 1/16
  pl011_irq_state irq state 1
  pl011_read addr 0x000 value 0x00000074 reg DR
  pl011_can_receive LCR 0x70, RX FIFO used 1/16, can_receive 15 chars
  pl011_read addr 0x018 value 0x00000080 reg FR
  pl011_read_fifo RX FIFO read, used 0/16
  pl011_irq_state irq state 0
  pl011_read addr 0x000 value 0x0000000d reg DR
  pl011_can_receive LCR 0x70, RX FIFO used 0/16, can_receive 16 chars
  pl011_read addr 0x018 value 0x00000090 reg FR
  pl011_read addr 0x03c value 0x00000020 reg RIS
  pl011_write addr 0x038 value 0x00000050 reg IMSC
  pl011_irq_state irq state 0
  pl011_can_receive LCR 0x70, RX FIFO used 0/16, can_receive 16 chars
  pl011_can_receive LCR 0x70, RX FIFO used 0/16, can_receive 16 chars
  pl011_read addr 0x018 value 0x00000090 reg FR
  pl011_write addr 0x000 value 0x00000072 reg DR

Inspired-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Luc Michel <luc.michel@amd.com>
---
 hw/char/pl011.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index f7485e7c541..23a9db8c57c 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -489,7 +489,6 @@ static int pl011_can_receive(void *opaque)
     PL011State *s = (PL011State *)opaque;
     unsigned fifo_depth = pl011_get_fifo_depth(s);
     unsigned fifo_available = fifo_depth - s->read_count;
-    int r = fifo_available ? 1 : 0;
 
     if (!(s->cr & CR_UARTEN)) {
         qemu_log_mask(LOG_GUEST_ERROR,
@@ -500,7 +499,8 @@ static int pl011_can_receive(void *opaque)
                       "PL011 receiving data on disabled RX UART\n");
     }
     trace_pl011_can_receive(s->lcr, s->read_count, fifo_depth, fifo_available);
-    return r;
+
+    return fifo_available;
 }
 
 static void pl011_receive(void *opaque, const uint8_t *buf, int size)
@@ -515,7 +515,9 @@ static void pl011_receive(void *opaque, const uint8_t *buf, int size)
         return;
     }
 
-    pl011_fifo_rx_put(opaque, *buf);
+    for (int i = 0; i < size; i++) {
+        pl011_fifo_rx_put(opaque, buf[i]);
+    }
 }
 
 static void pl011_event(void *opaque, QEMUChrEvent event)
-- 
2.47.1

[PATCH v2 5/9] hw/char/bcm2835_aux: Really use RX FIFO depth

[Philippe Mathieu-Daudé]

While we model a 8-elements RX FIFO since the BCM2835 AUX model
was introduced in commit 97398d900ca ("bcm2835_aux: add emulation
of BCM2835 AUX block") we only read 1 char at a time!

Have the IOCanReadHandler handler return how many elements are
available, and use that in the IOReadHandler handler.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Luc Michel <luc.michel@amd.com>
---
 hw/char/bcm2835_aux.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/char/bcm2835_aux.c b/hw/char/bcm2835_aux.c
index 73ad5934067..c6e7eccf7dd 100644
--- a/hw/char/bcm2835_aux.c
+++ b/hw/char/bcm2835_aux.c
@@ -221,7 +221,7 @@ static int bcm2835_aux_can_receive(void *opaque)
 {
     BCM2835AuxState *s = opaque;
 
-    return s->read_count < BCM2835_AUX_RX_FIFO_LEN;
+    return BCM2835_AUX_RX_FIFO_LEN - s->read_count;
 }
 
 static void bcm2835_aux_put_fifo(void *opaque, uint8_t value)
@@ -243,7 +243,9 @@ static void bcm2835_aux_put_fifo(void *opaque, uint8_t value)
 
 static void bcm2835_aux_receive(void *opaque, const uint8_t *buf, int size)
 {
-    bcm2835_aux_put_fifo(opaque, *buf);
+    for (int i = 0; i < size; i++) {
+        bcm2835_aux_put_fifo(opaque, buf[i]);
+    }
 }
 
 static const MemoryRegionOps bcm2835_aux_ops = {
-- 
2.47.1

[PATCH v2 6/9] hw/char/imx_serial: Really use RX FIFO depth

[Philippe Mathieu-Daudé]

While we model a 32-elements RX FIFO since the IMX serial
model was introduced in commit 988f2442971 ("hw/char/imx_serial:
Implement receive FIFO and ageing timer") we only read 1 char
at a time!

Have the IOCanReadHandler handler return how many elements are
available, and use that in the IOReadHandler handler.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Luc Michel <luc.michel@amd.com>
---
 hw/char/imx_serial.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
index 38b4865157e..6f14f8403a9 100644
--- a/hw/char/imx_serial.c
+++ b/hw/char/imx_serial.c
@@ -386,7 +386,8 @@ static void imx_serial_write(void *opaque, hwaddr offset,
 static int imx_can_receive(void *opaque)
 {
     IMXSerialState *s = (IMXSerialState *)opaque;
-    return s->ucr2 & UCR2_RXEN && fifo32_num_used(&s->rx_fifo) < FIFO_SIZE;
+
+    return s->ucr2 & UCR2_RXEN ? fifo32_num_free(&s->rx_fifo) : 0;
 }
 
 static void imx_put_data(void *opaque, uint32_t value)
@@ -417,7 +418,10 @@ static void imx_receive(void *opaque, const uint8_t *buf, int size)
     IMXSerialState *s = (IMXSerialState *)opaque;
 
     s->usr2 |= USR2_WAKE;
-    imx_put_data(opaque, *buf);
+
+    for (int i = 0; i < size; i++) {
+        imx_put_data(opaque, buf[i]);
+    }
 }
 
 static void imx_event(void *opaque, QEMUChrEvent event)
-- 
2.47.1

[PATCH v2 7/9] hw/char/mcf_uart: Use FIFO_DEPTH definition instead of magic values

[Philippe Mathieu-Daudé]

Defines FIFO_DEPTH and use it, fixing coding style.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Luc Michel <luc.michel@amd.com>
---
 hw/char/mcf_uart.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/char/mcf_uart.c b/hw/char/mcf_uart.c
index 980a12fcb7d..95f269ee9b7 100644
--- a/hw/char/mcf_uart.c
+++ b/hw/char/mcf_uart.c
@@ -17,6 +17,8 @@
 #include "chardev/char-fe.h"
 #include "qom/object.h"
 
+#define FIFO_DEPTH 4
+
 struct mcf_uart_state {
     SysBusDevice parent_obj;
 
@@ -27,7 +29,7 @@ struct mcf_uart_state {
     uint8_t imr;
     uint8_t bg1;
     uint8_t bg2;
-    uint8_t fifo[4];
+    uint8_t fifo[FIFO_DEPTH];
     uint8_t tb;
     int current_mr;
     int fifo_len;
@@ -247,14 +249,16 @@ static void mcf_uart_reset(DeviceState *dev)
 static void mcf_uart_push_byte(mcf_uart_state *s, uint8_t data)
 {
     /* Break events overwrite the last byte if the fifo is full.  */
-    if (s->fifo_len == 4)
+    if (s->fifo_len == FIFO_DEPTH) {
         s->fifo_len--;
+    }
 
     s->fifo[s->fifo_len] = data;
     s->fifo_len++;
     s->sr |= MCF_UART_RxRDY;
-    if (s->fifo_len == 4)
+    if (s->fifo_len == FIFO_DEPTH) {
         s->sr |= MCF_UART_FFULL;
+    }
 
     mcf_uart_update(s);
 }
-- 
2.47.1

[PATCH v2 8/9] hw/char/mcf_uart: Really use RX FIFO depth

[Philippe Mathieu-Daudé]

While we model a 4-elements RX FIFO since the MCF UART model
was introduced in commit 20dcee94833 ("MCF5208 emulation"),
we only read 1 char at a time!

Have the IOCanReadHandler handler return how many elements are
available, and use that in the IOReadHandler handler.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Luc Michel <luc.michel@amd.com>
---
 hw/char/mcf_uart.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/char/mcf_uart.c b/hw/char/mcf_uart.c
index 95f269ee9b7..529c26be93a 100644
--- a/hw/char/mcf_uart.c
+++ b/hw/char/mcf_uart.c
@@ -281,14 +281,16 @@ static int mcf_uart_can_receive(void *opaque)
 {
     mcf_uart_state *s = (mcf_uart_state *)opaque;
 
-    return s->rx_enabled && (s->sr & MCF_UART_FFULL) == 0;
+    return s->rx_enabled ? FIFO_DEPTH - s->fifo_len : 0;
 }
 
 static void mcf_uart_receive(void *opaque, const uint8_t *buf, int size)
 {
     mcf_uart_state *s = (mcf_uart_state *)opaque;
 
-    mcf_uart_push_byte(s, buf[0]);
+    for (int i = 0; i < size; i++) {
+        mcf_uart_push_byte(s, buf[i]);
+    }
 }
 
 static const MemoryRegionOps mcf_uart_ops = {
-- 
2.47.1

[PATCH v2 9/9] hw/char/sh_serial: Return correct number of empty RX FIFO elements

[Philippe Mathieu-Daudé]

In the IOCanReadHandler sh_serial_can_receive(), if the Serial
Control Register 'Receive Enable' bit is set (bit 4), then we
return a size of (1 << 4) which happens to be equal to 16, so
effectively SH_RX_FIFO_LENGTH.

The IOReadHandler, sh_serial_receive1() takes care to receive
multiple chars, but if the FIFO is partly filled, we only process
the number of free slots in the FIFO, discarding the other chars!

Fix by returning how many elements the FIFO can queue in the
IOCanReadHandler, so we don't have to process more than that in
the IOReadHandler, thus not discarding anything.

Remove the now unnecessary check on 's->rx_cnt < SH_RX_FIFO_LENGTH'
in IOReadHandler, reducing the block indentation.

Fixes: 63242a007a1 ("SH4: Serial controller improvement")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Luc Michel <luc.michel@amd.com>
---
 hw/char/sh_serial.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index 247aeb071ac..41c8175a638 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -320,7 +320,7 @@ static uint64_t sh_serial_read(void *opaque, hwaddr offs,
 
 static int sh_serial_can_receive(SHSerialState *s)
 {
-    return s->scr & (1 << 4);
+    return s->scr & (1 << 4) ? SH_RX_FIFO_LENGTH - s->rx_head : 0;
 }
 
 static void sh_serial_receive_break(SHSerialState *s)
@@ -353,22 +353,20 @@ static void sh_serial_receive1(void *opaque, const uint8_t *buf, int size)
     if (s->feat & SH_SERIAL_FEAT_SCIF) {
         int i;
         for (i = 0; i < size; i++) {
-            if (s->rx_cnt < SH_RX_FIFO_LENGTH) {
-                s->rx_fifo[s->rx_head++] = buf[i];
-                if (s->rx_head == SH_RX_FIFO_LENGTH) {
-                    s->rx_head = 0;
-                }
-                s->rx_cnt++;
-                if (s->rx_cnt >= s->rtrg) {
-                    s->flags |= SH_SERIAL_FLAG_RDF;
-                    if (s->scr & (1 << 6) && s->rxi) {
-                        timer_del(&s->fifo_timeout_timer);
-                        qemu_set_irq(s->rxi, 1);
-                    }
-                } else {
-                    timer_mod(&s->fifo_timeout_timer,
-                        qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 15 * s->etu);
+            s->rx_fifo[s->rx_head++] = buf[i];
+            if (s->rx_head == SH_RX_FIFO_LENGTH) {
+                s->rx_head = 0;
+            }
+            s->rx_cnt++;
+            if (s->rx_cnt >= s->rtrg) {
+                s->flags |= SH_SERIAL_FLAG_RDF;
+                if (s->scr & (1 << 6) && s->rxi) {
+                    timer_del(&s->fifo_timeout_timer);
+                    qemu_set_irq(s->rxi, 1);
                 }
+            } else {
+                timer_mod(&s->fifo_timeout_timer,
+                    qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 15 * s->etu);
             }
         }
     } else {
-- 
2.47.1

Re: [PATCH v2 1/9] hw/char/pl011: Warn when using disabled receiver

[Luc Michel]

On 10:28 Thu 20 Feb     , Philippe Mathieu-Daudé wrote:
> We shouldn't receive characters when the full UART or its
> receiver is disabled. However we don't want to break the
> possibly incomplete "my first bare metal assembly program"s,
> so we choose to simply display a warning when this occurs.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Luc Michel <luc.michel@amd.com>

> ---
>  hw/char/pl011.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 06ce851044d..12a2d4bc7bd 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -85,6 +85,7 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
>  #define CR_OUT1     (1 << 12)
>  #define CR_RTS      (1 << 11)
>  #define CR_DTR      (1 << 10)
> +#define CR_RXE      (1 << 9)
>  #define CR_TXE      (1 << 8)
>  #define CR_LBE      (1 << 7)
>  #define CR_UARTEN   (1 << 0)
> @@ -487,6 +488,14 @@ static int pl011_can_receive(void *opaque)
>      PL011State *s = (PL011State *)opaque;
>      int r;
> 
> +    if (!(s->cr & CR_UARTEN)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "PL011 receiving data on disabled UART\n");
> +    }
> +    if (!(s->cr & CR_RXE)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "PL011 receiving data on disabled RX UART\n");
> +    }
>      r = s->read_count < pl011_get_fifo_depth(s);
>      trace_pl011_can_receive(s->lcr, s->read_count, r);
>      return r;
> --
> 2.47.1
> 

-- 

Re: [PATCH v2 7/9] hw/char/mcf_uart: Use FIFO_DEPTH definition instead of magic values

[Thomas Huth]

Am Thu, 20 Feb 2025 10:29:00 +0100
schrieb Philippe Mathieu-Daudé <philmd@linaro.org>:

> Defines FIFO_DEPTH and use it, fixing coding style.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> ---
>  hw/char/mcf_uart.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH v2 8/9] hw/char/mcf_uart: Really use RX FIFO depth

[Thomas Huth]

Am Thu, 20 Feb 2025 10:29:01 +0100
schrieb Philippe Mathieu-Daudé <philmd@linaro.org>:

> While we model a 4-elements RX FIFO since the MCF UART model
> was introduced in commit 20dcee94833 ("MCF5208 emulation"),
> we only read 1 char at a time!
> 
> Have the IOCanReadHandler handler return how many elements are
> available, and use that in the IOReadHandler handler.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> ---
>  hw/char/mcf_uart.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/char/mcf_uart.c b/hw/char/mcf_uart.c
> index 95f269ee9b7..529c26be93a 100644
> --- a/hw/char/mcf_uart.c
> +++ b/hw/char/mcf_uart.c
> @@ -281,14 +281,16 @@ static int mcf_uart_can_receive(void *opaque)
>  {
>      mcf_uart_state *s = (mcf_uart_state *)opaque;
>  
> -    return s->rx_enabled && (s->sr & MCF_UART_FFULL) == 0;
> +    return s->rx_enabled ? FIFO_DEPTH - s->fifo_len : 0;
>  }
>  
>  static void mcf_uart_receive(void *opaque, const uint8_t *buf, int size)
>  {
>      mcf_uart_state *s = (mcf_uart_state *)opaque;
>  
> -    mcf_uart_push_byte(s, buf[0]);
> +    for (int i = 0; i < size; i++) {
> +        mcf_uart_push_byte(s, buf[i]);
> +    }
>  }

Tested-by: Thomas Huth <huth@tuxfamily.org>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH v2 2/9] hw/char/pl011: Simplify a bit pl011_can_receive()

[Richard Henderson]

On 2/20/25 01:28, Philippe Mathieu-Daudé wrote:
> Introduce 'fifo_depth' and 'fifo_available' local variables
> to better express the 'r' variable use.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> ---
>   hw/char/pl011.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


> 
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 12a2d4bc7bd..5bb83c54216 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -486,7 +486,9 @@ static void pl011_write(void *opaque, hwaddr offset,
>   static int pl011_can_receive(void *opaque)
>   {
>       PL011State *s = (PL011State *)opaque;
> -    int r;
> +    unsigned fifo_depth = pl011_get_fifo_depth(s);
> +    unsigned fifo_available = fifo_depth - s->read_count;
> +    int r = fifo_available ? 1 : 0;

This is begging for a bool, though.  Both in the local and the return type.


r~

Re: [PATCH v2 3/9] hw/char/pl011: Improve RX flow tracing events

[Richard Henderson]

On 2/20/25 01:28, Philippe Mathieu-Daudé wrote:
> Log FIFO use (availability and depth).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> ---
>   hw/char/pl011.c      | 10 ++++++----
>   hw/char/trace-events |  7 ++++---
>   2 files changed, 10 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 4/9] hw/char/pl011: Really use RX FIFO depth

[Richard Henderson]

On 2/20/25 01:28, Philippe Mathieu-Daudé wrote:
> While we model a 16-elements RX FIFO since the PL011 model was
> introduced in commit cdbdb648b7c ("ARM Versatile Platform Baseboard
> emulation"), we only read 1 char at a time!
> 
> Have the IOCanReadHandler handler return how many elements are
> available, and use that in the IOReadHandler handler.
> 
> Example of FIFO better used by enabling the pl011 tracing events
> and running the tests/functional/test_aarch64_virt.py tests:
> 
>    pl011_can_receive LCR 0x70, RX FIFO used 0/16, can_receive 16 chars
>    pl011_receive recv 5 chars
>    pl011_fifo_rx_put RX FIFO push char [0x72] 1/16 depth used
>    pl011_irq_state irq state 1
>    pl011_fifo_rx_put RX FIFO push char [0x6f] 2/16 depth used
>    pl011_fifo_rx_put RX FIFO push char [0x6f] 3/16 depth used
>    pl011_fifo_rx_put RX FIFO push char [0x74] 4/16 depth used
>    pl011_fifo_rx_put RX FIFO push char [0x0d] 5/16 depth used
>    pl011_can_receive LCR 0x70, RX FIFO used 5/16, can_receive 11 chars
>    pl011_can_receive LCR 0x70, RX FIFO used 5/16, can_receive 11 chars
>    pl011_write addr 0x038 value 0x00000050 reg IMSC
>    pl011_irq_state irq state 1
>    pl011_can_receive LCR 0x70, RX FIFO used 5/16, can_receive 11 chars
>    pl011_read addr 0x03c value 0x00000030 reg RIS
>    pl011_write addr 0x044 value 0x00000000 reg ICR
>    pl011_irq_state irq state 1
>    pl011_read addr 0x018 value 0x00000080 reg FR
>    pl011_read_fifo RX FIFO read, used 4/16
>    pl011_irq_state irq state 1
>    pl011_read addr 0x000 value 0x00000072 reg DR
>    pl011_can_receive LCR 0x70, RX FIFO used 4/16, can_receive 12 chars
>    pl011_read addr 0x018 value 0x00000080 reg FR
>    pl011_read_fifo RX FIFO read, used 3/16
>    pl011_irq_state irq state 1
>    pl011_read addr 0x000 value 0x0000006f reg DR
>    pl011_can_receive LCR 0x70, RX FIFO used 3/16, can_receive 13 chars
>    pl011_read addr 0x018 value 0x00000080 reg FR
>    pl011_read_fifo RX FIFO read, used 2/16
>    pl011_irq_state irq state 1
>    pl011_read addr 0x000 value 0x0000006f reg DR
>    pl011_can_receive LCR 0x70, RX FIFO used 2/16, can_receive 14 chars
>    pl011_read addr 0x018 value 0x00000080 reg FR
>    pl011_read_fifo RX FIFO read, used 1/16
>    pl011_irq_state irq state 1
>    pl011_read addr 0x000 value 0x00000074 reg DR
>    pl011_can_receive LCR 0x70, RX FIFO used 1/16, can_receive 15 chars
>    pl011_read addr 0x018 value 0x00000080 reg FR
>    pl011_read_fifo RX FIFO read, used 0/16
>    pl011_irq_state irq state 0
>    pl011_read addr 0x000 value 0x0000000d reg DR
>    pl011_can_receive LCR 0x70, RX FIFO used 0/16, can_receive 16 chars
>    pl011_read addr 0x018 value 0x00000090 reg FR
>    pl011_read addr 0x03c value 0x00000020 reg RIS
>    pl011_write addr 0x038 value 0x00000050 reg IMSC
>    pl011_irq_state irq state 0
>    pl011_can_receive LCR 0x70, RX FIFO used 0/16, can_receive 16 chars
>    pl011_can_receive LCR 0x70, RX FIFO used 0/16, can_receive 16 chars
>    pl011_read addr 0x018 value 0x00000090 reg FR
>    pl011_write addr 0x000 value 0x00000072 reg DR
> 
> Inspired-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> ---
>   hw/char/pl011.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index f7485e7c541..23a9db8c57c 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -489,7 +489,6 @@ static int pl011_can_receive(void *opaque)
>       PL011State *s = (PL011State *)opaque;
>       unsigned fifo_depth = pl011_get_fifo_depth(s);
>       unsigned fifo_available = fifo_depth - s->read_count;
> -    int r = fifo_available ? 1 : 0;
>   
>       if (!(s->cr & CR_UARTEN)) {
>           qemu_log_mask(LOG_GUEST_ERROR,
> @@ -500,7 +499,8 @@ static int pl011_can_receive(void *opaque)
>                         "PL011 receiving data on disabled RX UART\n");
>       }
>       trace_pl011_can_receive(s->lcr, s->read_count, fifo_depth, fifo_available);
> -    return r;
> +
> +    return fifo_available;
>   }

Ah, I get it; my bool comment was ill-informed.


>   static void pl011_receive(void *opaque, const uint8_t *buf, int size)
> @@ -515,7 +515,9 @@ static void pl011_receive(void *opaque, const uint8_t *buf, int size)
>           return;
>       }
>   
> -    pl011_fifo_rx_put(opaque, *buf);
> +    for (int i = 0; i < size; i++) {
> +        pl011_fifo_rx_put(opaque, buf[i]);
> +    }
>   }

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 5/9] hw/char/bcm2835_aux: Really use RX FIFO depth

[Richard Henderson]

On 2/20/25 01:28, Philippe Mathieu-Daudé wrote:
> While we model a 8-elements RX FIFO since the BCM2835 AUX model
> was introduced in commit 97398d900ca ("bcm2835_aux: add emulation
> of BCM2835 AUX block") we only read 1 char at a time!
> 
> Have the IOCanReadHandler handler return how many elements are
> available, and use that in the IOReadHandler handler.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> ---
>   hw/char/bcm2835_aux.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 6/9] hw/char/imx_serial: Really use RX FIFO depth

[Richard Henderson]

On 2/20/25 01:28, Philippe Mathieu-Daudé wrote:
> While we model a 32-elements RX FIFO since the IMX serial
> model was introduced in commit 988f2442971 ("hw/char/imx_serial:
> Implement receive FIFO and ageing timer") we only read 1 char
> at a time!
> 
> Have the IOCanReadHandler handler return how many elements are
> available, and use that in the IOReadHandler handler.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> ---
>   hw/char/imx_serial.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 7/9] hw/char/mcf_uart: Use FIFO_DEPTH definition instead of magic values

[Richard Henderson]

On 2/20/25 01:29, Philippe Mathieu-Daudé wrote:
> Defines FIFO_DEPTH and use it, fixing coding style.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> ---
>   hw/char/mcf_uart.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 8/9] hw/char/mcf_uart: Really use RX FIFO depth

[Richard Henderson]

On 2/20/25 01:29, Philippe Mathieu-Daudé wrote:
> While we model a 4-elements RX FIFO since the MCF UART model
> was introduced in commit 20dcee94833 ("MCF5208 emulation"),
> we only read 1 char at a time!
> 
> Have the IOCanReadHandler handler return how many elements are
> available, and use that in the IOReadHandler handler.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> ---
>   hw/char/mcf_uart.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 9/9] hw/char/sh_serial: Return correct number of empty RX FIFO elements

[Richard Henderson]

On 2/20/25 01:29, Philippe Mathieu-Daudé wrote:
> In the IOCanReadHandler sh_serial_can_receive(), if the Serial
> Control Register 'Receive Enable' bit is set (bit 4), then we
> return a size of (1 << 4) which happens to be equal to 16, so
> effectively SH_RX_FIFO_LENGTH.
> 
> The IOReadHandler, sh_serial_receive1() takes care to receive
> multiple chars, but if the FIFO is partly filled, we only process
> the number of free slots in the FIFO, discarding the other chars!
> 
> Fix by returning how many elements the FIFO can queue in the
> IOCanReadHandler, so we don't have to process more than that in
> the IOReadHandler, thus not discarding anything.
> 
> Remove the now unnecessary check on 's->rx_cnt < SH_RX_FIFO_LENGTH'
> in IOReadHandler, reducing the block indentation.
> 
> Fixes: 63242a007a1 ("SH4: Serial controller improvement")
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Luc Michel<luc.michel@amd.com>
> ---
>   hw/char/sh_serial.c | 30 ++++++++++++++----------------
>   1 file changed, 14 insertions(+), 16 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 6/9] hw/char/imx_serial: Really use RX FIFO depth

[Bernhard Beschow]

Am 20. Februar 2025 09:28:59 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>While we model a 32-elements RX FIFO since the IMX serial
>model was introduced in commit 988f2442971 ("hw/char/imx_serial:
>Implement receive FIFO and ageing timer") we only read 1 char
>at a time!
>
>Have the IOCanReadHandler handler return how many elements are
>available, and use that in the IOReadHandler handler.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>Reviewed-by: Luc Michel <luc.michel@amd.com>

Tested-by: Bernhard Beschow <shentey@gmail.com>

>---
> hw/char/imx_serial.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
>index 38b4865157e..6f14f8403a9 100644
>--- a/hw/char/imx_serial.c
>+++ b/hw/char/imx_serial.c
>@@ -386,7 +386,8 @@ static void imx_serial_write(void *opaque, hwaddr offset,
> static int imx_can_receive(void *opaque)
> {
>     IMXSerialState *s = (IMXSerialState *)opaque;
>-    return s->ucr2 & UCR2_RXEN && fifo32_num_used(&s->rx_fifo) < FIFO_SIZE;
>+
>+    return s->ucr2 & UCR2_RXEN ? fifo32_num_free(&s->rx_fifo) : 0;
> }
> 
> static void imx_put_data(void *opaque, uint32_t value)
>@@ -417,7 +418,10 @@ static void imx_receive(void *opaque, const uint8_t *buf, int size)
>     IMXSerialState *s = (IMXSerialState *)opaque;
> 
>     s->usr2 |= USR2_WAKE;
>-    imx_put_data(opaque, *buf);
>+
>+    for (int i = 0; i < size; i++) {
>+        imx_put_data(opaque, buf[i]);
>+    }
> }
> 
> static void imx_event(void *opaque, QEMUChrEvent event)

Re: [PATCH v2 0/9] hw/char: Improve RX FIFO depth uses

[Philippe Mathieu-Daudé]

On 20/2/25 10:28, Philippe Mathieu-Daudé wrote:

>    hw/char/pl011: Warn when using disabled receiver
>    hw/char/pl011: Simplify a bit pl011_can_receive()
>    hw/char/pl011: Improve RX flow tracing events
>    hw/char/pl011: Really use RX FIFO depth
>    hw/char/bcm2835_aux: Really use RX FIFO depth
>    hw/char/imx_serial: Really use RX FIFO depth
>    hw/char/mcf_uart: Use FIFO_DEPTH definition instead of magic values
>    hw/char/mcf_uart: Really use RX FIFO depth
>    hw/char/sh_serial: Return correct number of empty RX FIFO elements

Series queued, thanks!