| Message ID | 20250219210841.94797-10-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/char: Improve RX FIFO depth uses | expand |
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 > --
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 {
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(-)