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

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

[Philippe Mathieu-Daudé]

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       | 28 ++++++++++++++++++++--------
 hw/char/sh_serial.c   | 30 ++++++++++++++----------------
 hw/char/trace-events  |  7 ++++---
 6 files changed, 59 insertions(+), 36 deletions(-)

-- 
2.47.1

[PATCH 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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 06ce851044d..60cea1d9a16 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,12 @@ 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 reading data on disabled UART\n");
+    }
+    if (!(s->cr & CR_RXE)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "PL011 reading data on disabled TX 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 1/9] hw/char/pl011: Warn when using disabled receiver

[Luc Michel]

Hi Phil,

On 22:08 Wed 19 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>
> ---
>  hw/char/pl011.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 06ce851044d..60cea1d9a16 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,12 @@ 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 reading data on disabled UART\n");
I find "reading" a bit misleading here. The guest is not reading the
data at this time. The chardev backend is trying to push data it
received to the device model. I'd use "receiving" instead.

> +    }
> +    if (!(s->cr & CR_RXE)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "PL011 reading data on disabled TX UART\n");
s/TX/RX

Thanks,
Luc

> +    }
>      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 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>
---
 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 60cea1d9a16..bcd516d682d 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, "PL011 reading data on disabled UART\n");
@@ -494,7 +496,6 @@ static int pl011_can_receive(void *opaque)
     if (!(s->cr & CR_RXE)) {
         qemu_log_mask(LOG_GUEST_ERROR, "PL011 reading data on disabled TX 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 2/9] hw/char/pl011: Simplify a bit pl011_can_receive()

[Luc Michel]

On 22:08 Wed 19 Feb     , 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(-)
> 
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 60cea1d9a16..bcd516d682d 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, "PL011 reading data on disabled UART\n");
> @@ -494,7 +496,6 @@ static int pl011_can_receive(void *opaque)
>      if (!(s->cr & CR_RXE)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "PL011 reading data on disabled TX 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 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>
---
 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 bcd516d682d..148a7d0dc60 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);
@@ -496,12 +497,13 @@ static int pl011_can_receive(void *opaque)
     if (!(s->cr & CR_RXE)) {
         qemu_log_mask(LOG_GUEST_ERROR, "PL011 reading data on disabled TX 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

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

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Log FIFO use (availability and depth).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Luc Michel]

On 22:08 Wed 19 Feb     , Philippe Mathieu-Daudé wrote:
> Log FIFO use (availability and depth).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@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 bcd516d682d..148a7d0dc60 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);
> @@ -496,12 +497,13 @@ static int pl011_can_receive(void *opaque)
>      if (!(s->cr & CR_RXE)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "PL011 reading data on disabled TX 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 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>
---
 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 148a7d0dc60..57d293d1e3a 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, "PL011 reading data on disabled UART\n");
@@ -498,7 +497,8 @@ static int pl011_can_receive(void *opaque)
         qemu_log_mask(LOG_GUEST_ERROR, "PL011 reading data on disabled TX 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)
@@ -513,7 +513,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

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

[Luc Michel]

On 22:08 Wed 19 Feb     , 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 148a7d0dc60..57d293d1e3a 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, "PL011 reading data on disabled UART\n");
> @@ -498,7 +497,8 @@ static int pl011_can_receive(void *opaque)
>          qemu_log_mask(LOG_GUEST_ERROR, "PL011 reading data on disabled TX 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)
> @@ -513,7 +513,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 5/9] hw/char/bcm2835_aux: Really use RX FIFO depth

[Philippe Mathieu-Daudé]

While we model a 8-elements RX FIFO since the PL011 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>
---
 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

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

[Luc Michel]

On 22:08 Wed 19 Feb     , Philippe Mathieu-Daudé wrote:
> While we model a 8-elements RX FIFO since the PL011 model was
> introduced in commit 97398d900ca ("bcm2835_aux: add emulation
> of BCM2835 AUX block") we only read 1 char at a time!

I'm not sure I get why in this patch and the subsequent ones you keep
mentioning the PL011 model while you modify other UARTs. I guess you
mean "the BCM2835 AUX model" here?

In any case:

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

> 
> 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>
> ---
>  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
> 

-- 

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

[Philippe Mathieu-Daudé]

On 20/2/25 09:21, Luc Michel wrote:
> On 22:08 Wed 19 Feb     , Philippe Mathieu-Daudé wrote:
>> While we model a 8-elements RX FIFO since the PL011 model was
>> introduced in commit 97398d900ca ("bcm2835_aux: add emulation
>> of BCM2835 AUX block") we only read 1 char at a time!
> 
> I'm not sure I get why in this patch and the subsequent ones you keep
> mentioning the PL011 model while you modify other UARTs. I guess you
> mean "the BCM2835 AUX model" here?

Oops too much copy/pasting...

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

Thanks!

> 
>>
>> 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>
>> ---
>>   hw/char/bcm2835_aux.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)

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

[Philippe Mathieu-Daudé]

While we model a 32-elements RX FIFO since the PL011 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>
---
 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

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

[Luc Michel]

On 22:08 Wed 19 Feb     , Philippe Mathieu-Daudé wrote:
> While we model a 32-elements RX FIFO since the PL011 model was
> introduced in commit 988f2442971 ("hw/char/imx_serial: Implement
> receive FIFO and ageing timer") we only read 1 char at a time!

"the IMX serial model"?

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

> 
> 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>
> ---
>  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 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>
---
 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

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

[Luc Michel]

On 22:08 Wed 19 Feb     , 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(-)
> 
> 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 8/9] hw/char/mcf_uart: Really use RX FIFO depth

[Philippe Mathieu-Daudé]

While we model a 4-elements RX FIFO since the PL011 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>
---
 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

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

[Luc Michel]

On 22:08 Wed 19 Feb     , Philippe Mathieu-Daudé wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> While we model a 4-elements RX FIFO since the PL011 model
> was introduced in commit 20dcee94833 ("MCF5208 emulation"),
> we only read 1 char at a time!

"the MCF UART model"

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

> 
> 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>
> ---
>  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 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
returns 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>
---
 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 9/9] hw/char/sh_serial: Return correct number of empty RX FIFO elements

[Luc Michel]

On 22:08 Wed 19 Feb     , 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
> returns a size of (1 << 4) which happens to be equal to 16,
return

> 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>

nice one :)
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
> 

--