Re: [PATCH 1/7] [BCM2835 AUX 1/7] Replace hard-coded FIFO

[Peter Maydell]

On Sun, 17 Nov 2024 at 23:01, Ioan-Cristian CÎRSTEA
<jean.christian.cirstea@gmail.com> wrote:
>
> The previous BCM2835 mini UART implementation used a hard-coded FIFO.
> This commit changes the implementation by making use of the provided
> Fifo8.
>
> Signed-off-by: Ioan-Cristian CÎRSTEA <ioan-cristian.cirstea@tutanota.com>

Hi; thanks for this patchset.

For future multi-patch submissions, could you include a cover
letter email, please? Some of our automated tooling relies on
the cover letter. (No cover letter needed for single standalone
patches.) It also gives you a place to explain the purpose/aim
of the patchset.

> ---
>  hw/char/bcm2835_aux.c         | 61 ++++++++++++++++++-----------------
>  include/hw/char/bcm2835_aux.h | 10 +++---
>  2 files changed, 37 insertions(+), 34 deletions(-)
>
> diff --git a/hw/char/bcm2835_aux.c b/hw/char/bcm2835_aux.c
> index fca2f27a55..540cb30872 100644
> --- a/hw/char/bcm2835_aux.c
> +++ b/hw/char/bcm2835_aux.c
> @@ -47,14 +47,23 @@
>  #define RX_INT  0x1
>  #define TX_INT  0x2
>
> +/* FIFOs length */
> +#define BCM2835_AUX_RX_FIFO_LEN 8
> +#define BCM2835_AUX_TX_FIOF_LEN 8

Typo: "FIFO".

> +
> +/* TODO: Add separate functions for RX and TX interrupts */

Why? Usually we just have one function for this kind of
"update the outgoing interrupt state".

>  static void bcm2835_aux_update(BCM2835AuxState *s)
>  {
> +    /* TODO: this should be a pointer to const data. However, the fifo8 API
> +     * requires a pointer to non-const data.
> +     */

QEMU coding style for multi-line comments has the initial
"/*" on a line of its own. (There's some existing code
which is old enough that it doesn't follow the current
style guidelines; generally we leave that alone unless
it needs changing anyway, but ask for newly added code
to follow style. A rough rule of thumb is "fix stuff
that checkpatch.pl complains about in your patch", but
bear in mind that checkpatch sometimes complains about
stuff that isn't a problem.)

> +    Fifo8 *rx_fifo = &s->rx_fifo;
>      /* signal an interrupt if either:
>       * 1. rx interrupt is enabled and we have a non-empty rx fifo, or
>       * 2. the tx interrupt is enabled (since we instantly drain the tx fifo)
>       */
>      s->iir = 0;
> -    if ((s->ier & RX_INT) && s->read_count != 0) {
> +    if ((s->ier & RX_INT) && fifo8_is_empty(rx_fifo) == false) {

Why the local variable when you could write
 fifo8_is_empty(&s->rx_fifo)
?

>          s->iir |= RX_INT;
>      }
>      if (s->ier & TX_INT) {
> @@ -66,7 +75,10 @@ static void bcm2835_aux_update(BCM2835AuxState *s)
>  static uint64_t bcm2835_aux_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      BCM2835AuxState *s = opaque;
> -    uint32_t c, res;
> +    Fifo8 *rx_fifo = &s->rx_fifo;
> +    const bool is_rx_fifo_not_empty = !fifo8_is_empty(rx_fifo);
> +    const uint32_t rx_fifo_fill_level = fifo8_num_used(rx_fifo);

Do we need these?

> +    uint32_t c = 0, res;
>
>      switch (offset) {
>      case AUX_IRQ:
> @@ -77,12 +89,8 @@ static uint64_t bcm2835_aux_read(void *opaque, hwaddr 
> offset, unsigned size)
>
>      case AUX_MU_IO_REG:
>          /* "DLAB bit set means access baudrate register" is NYI */
> -        c = s->read_fifo[s->read_pos];
> -        if (s->read_count > 0) {
> -            s->read_count--;
> -            if (++s->read_pos == BCM2835_AUX_RX_FIFO_LEN) {
> -                s->read_pos = 0;
> -            }
> +        if (is_rx_fifo_not_empty) {

For instance here can we directly write

       if (!fifo8_is_empty(&s->rx_fifo) {
           c = fifo8_pop(&s->rx_fifo);
       }

That is the way other QEMU code generally does this.

> +            c = fifo8_pop(rx_fifo);
>          }
>          qemu_chr_fe_accept_input(&s->chr);
>          bcm2835_aux_update(s);
> @@ -98,7 +106,7 @@ static uint64_t bcm2835_aux_read(void *opaque, hwaddr 
> offset, unsigned size)
>           * interrupts are active, besides that this cannot occur. At
>           * present, we choose to prioritise the rx interrupt, since
>           * the tx fifo is always empty. */
> -        if (s->read_count != 0) {
> +        if (is_rx_fifo_not_empty) {
>              res |= 0x4;
>          } else {
>              res |= 0x2;
> @@ -118,7 +126,7 @@ static uint64_t bcm2835_aux_read(void *opaque, hwaddr 
> offset, unsigned size)
>
>      case AUX_MU_LSR_REG:
>          res = 0x60; /* tx idle, empty */
> -        if (s->read_count != 0) {
> +        if (is_rx_fifo_not_empty) {
>              res |= 0x1;
>          }
>          return res;
> @@ -136,10 +144,10 @@ static uint64_t bcm2835_aux_read(void *opaque, hwaddr 
> offset, unsigned size)
>
>      case AUX_MU_STAT_REG:
>          res = 0x30e; /* space in the output buffer, empty tx fifo, idle 
> tx/rx */
> -        if (s->read_count > 0) {
> +        if (is_rx_fifo_not_empty) {
>              res |= 0x1; /* data in input buffer */
> -            assert(s->read_count <= BCM2835_AUX_RX_FIFO_LEN);
> -            res |= ((uint32_t)s->read_count) << 16; /* rx fifo fill level */
> +            assert(rx_fifo_fill_level <= BCM2835_AUX_RX_FIFO_LEN);
> +            res |= ((uint32_t)rx_fifo_fill_level) << 16; /* rx fifo fill 
> level */
>          }
>          return res;
>
> @@ -158,6 +166,7 @@ static void bcm2835_aux_write(void *opaque, hwaddr 
> offset, uint64_t value,
>                                unsigned size)
>  {
>      BCM2835AuxState *s = opaque;
> +    Fifo8 *rx_fifo = &s->rx_fifo;
>      unsigned char ch;
>
>      switch (offset) {
> @@ -185,7 +194,7 @@ static void bcm2835_aux_write(void *opaque, hwaddr 
> offset, uint64_t value,
>
>      case AUX_MU_IIR_REG:
>          if (value & 0x2) {
> -            s->read_count = 0;
> +            fifo8_reset(rx_fifo);
>          }
>          break;
>
> @@ -221,24 +230,18 @@ static int bcm2835_aux_can_receive(void *opaque)
>  {
>      BCM2835AuxState *s = opaque;
>
> -    return s->read_count < BCM2835_AUX_RX_FIFO_LEN;
> +    return !fifo8_is_full(&s->rx_fifo);
>  }
>
>  static void bcm2835_aux_put_fifo(void *opaque, uint8_t value)
>  {
>      BCM2835AuxState *s = opaque;
> -    int slot;
> +    Fifo8 *rx_fifo = &s->rx_fifo;
>
> -    slot = s->read_pos + s->read_count;
> -    if (slot >= BCM2835_AUX_RX_FIFO_LEN) {
> -        slot -= BCM2835_AUX_RX_FIFO_LEN;
> -    }
> -    s->read_fifo[slot] = value;
> -    s->read_count++;
> -    if (s->read_count == BCM2835_AUX_RX_FIFO_LEN) {
> -        /* buffer full */
> +    if (fifo8_is_full(rx_fifo) == false) {
> +        fifo8_push(rx_fifo, value);
> +        bcm2835_aux_update(s);
>      }
> -    bcm2835_aux_update(s);
>  }
>
>  static void bcm2835_aux_receive(void *opaque, const uint8_t *buf, int size)
> @@ -261,10 +264,8 @@ static const VMStateDescription vmstate_bcm2835_aux = {
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .fields = (const VMStateField[]) {
> -        VMSTATE_UINT8_ARRAY(read_fifo, BCM2835AuxState,
> -                            BCM2835_AUX_RX_FIFO_LEN),
> -        VMSTATE_UINT8(read_pos, BCM2835AuxState),
> -        VMSTATE_UINT8(read_count, BCM2835AuxState),
> +        VMSTATE_FIFO8(rx_fifo, BCM2835AuxState),
> +        VMSTATE_FIFO8(_tx_fifo, BCM2835AuxState),
>          VMSTATE_UINT8(ier, BCM2835AuxState),
>          VMSTATE_UINT8(iir, BCM2835AuxState),
>          VMSTATE_END_OF_LIST()

If you change the vmstate you need to also bump the
version_id and minimum_version_id fields, and note in the
commit message that this is a migration-compatibility break
for the affected machine types.

> @@ -276,6 +277,8 @@ static void bcm2835_aux_init(Object *obj)
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>      BCM2835AuxState *s = BCM2835_AUX(obj);
>
> +    fifo8_create(&s->rx_fifo, BCM2835_AUX_RX_FIFO_LEN);
> +
>      memory_region_init_io(&s->iomem, OBJECT(s), &bcm2835_aux_ops, s,
>                            TYPE_BCM2835_AUX, 0x100);
>      sysbus_init_mmio(sbd, &s->iomem);
> diff --git a/include/hw/char/bcm2835_aux.h b/include/hw/char/bcm2835_aux.h
> index 9e081793a0..cb1a824994 100644
> --- a/include/hw/char/bcm2835_aux.h
> +++ b/include/hw/char/bcm2835_aux.h
> @@ -9,15 +9,14 @@
>  #ifndef BCM2835_AUX_H
>  #define BCM2835_AUX_H
>
> -#include "hw/sysbus.h"
>  #include "chardev/char-fe.h"
> +#include "hw/sysbus.h"
> +#include "qemu/fifo8.h"
>  #include "qom/object.h"
>
>  #define TYPE_BCM2835_AUX "bcm2835-aux"
>  OBJECT_DECLARE_SIMPLE_TYPE(BCM2835AuxState, BCM2835_AUX)
>
> -#define BCM2835_AUX_RX_FIFO_LEN 8
> -
>  struct BCM2835AuxState {
>      /*< private >*/
>      SysBusDevice parent_obj;
> @@ -27,8 +26,9 @@ struct BCM2835AuxState {
>      CharBackend chr;
>      qemu_irq irq;
>
> -    uint8_t read_fifo[BCM2835_AUX_RX_FIFO_LEN];
> -    uint8_t read_pos, read_count;
> +    Fifo8 rx_fifo;
> +    /* Unused for now */
> +    Fifo8 _tx_fifo;

Please don't use underscores at the start of identifiers,
and don't add fields that aren't used yet.

>      uint8_t ier, iir;
>  };

thanks
-- PMM