Re: [PATCH 3/7] [BCM2835 AUX 3/7] Asynchronous transmit

[Peter Maydell]

On Sun, 17 Nov 2024 at 23:01, Ioan-Cristian CÎRSTEA
<jean.christian.cirstea@gmail.com> wrote:
>
> This commit changes data transmission: instead of using the blocking
> function `qemu_chr_fe_write_all()`, the transmit logic using the
> asynchronous counterpart `qemu_chr_fe_write()`.

This is a nice cleanup -- it's nice to reduce the number
of old serial devices we have that are still using the
blocking write.

> Signed-off-by: Ioan-Cristian CÎRSTEA <ioan-cristian.cirstea@tutanota.com>
> ---
>  hw/char/bcm2835_aux.c         | 110 ++++++++++++++++++++++++++++++----
>  include/hw/char/bcm2835_aux.h |   5 +-
>  2 files changed, 101 insertions(+), 14 deletions(-)
>
> diff --git a/hw/char/bcm2835_aux.c b/hw/char/bcm2835_aux.c
> index ebc7f5ade5..2ef3459566 100644
> --- a/hw/char/bcm2835_aux.c
> +++ b/hw/char/bcm2835_aux.c
> @@ -55,17 +55,29 @@
>  #define RX_ENABLE 0x1
>  #define TX_ENABLE 0x2
>
> +/* bits in STAT register */
> +#define STAT_TRANSMITTER_DONE 0x200
> +
>  /* FIFOs length */
>  #define BCM2835_AUX_RX_FIFO_LEN 8
> -#define BCM2835_AUX_TX_FIOF_LEN 8
> +#define BCM2835_AUX_TX_FIFO_LEN 8

Oh, here's the typo fix -- it should be in patch 1.

> +
> +#define log_guest_error(fmt, ...) \
> +    qemu_log_mask(LOG_GUEST_ERROR, \
> +                  "bcm2835_aux:%s:%d: " fmt, \
> +                  __func__, \
> +                  __LINE__, \
> +                  ##__VA_ARGS__ \
> +                  )

Please don't wrap qemu_log_mask() in a macro like this;
we only use it once anyway. Also, LOG_GUEST_ERROR messages
are going to be seen by users and they're for guest bugs,
not QEMU bugs -- so it's better to write them in a way
that's clear about the problem and doesn't include QEMU
line numbers that imply that the reader ought to go and
look up the QEMU source code. (Function names are a
borderline case -- we often print those but we hope they're
named clearly enough to imply what they mean...)

>  /* TODO: Add separate functions for RX and TX interrupts */
> -static void bcm2835_aux_update(BCM2835AuxState *s)
> +static void bcm2835_aux_update_irq(BCM2835AuxState *s)
>  {
>      /* TODO: this should be a pointer to const data. However, the fifo8 API
>       * requires a pointer to non-const data.
>       */
>      Fifo8 *rx_fifo = &s->rx_fifo;
> +    Fifo8 *tx_fifo = &s->tx_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)
> @@ -74,13 +86,19 @@ static void bcm2835_aux_update(BCM2835AuxState *s)
>      if ((s->ier & RX_INT) && fifo8_is_empty(rx_fifo) == false) {
>          s->iir |= RX_INT;
>      }
> -    if (s->ier & TX_INT) {
> +    if (s->ier & TX_INT && fifo8_is_empty(tx_fifo)) {
>          s->iir |= TX_INT;
>      }
>      qemu_set_irq(s->irq, s->iir != 0);
>  }
>
> -static bool bcm2835_aux_is_tx_enabled(BCM2835AuxState *s)
> +static void bcm2835_aux_update(BCM2835AuxState *s)
> +{
> +    /* Currently, only IRQ registers are updated */
> +    bcm2835_aux_update_irq(s);

What other kind of update were you envisaging? This
kind of foo_update() function in a QEMU device model
is usually "update the interrupt state".

Please don't add this wrapper function until/unless we
actually need it.

> +}
> +
> +static bool bcm2835_aux_is_tx_enabled(const BCM2835AuxState *s)
>  {
>      return (s->cntl & TX_ENABLE) != 0;
>  }
> @@ -90,6 +108,70 @@ static bool bcm2835_aux_is_rx_enabled(BCM2835AuxState *s)
>      return (s->cntl & RX_ENABLE) != 0;
>  }
>
> +static bool bcm2835_aux_put_tx_fifo(BCM2835AuxState *s, char ch)
> +{
> +    Fifo8 *tx_fifo = &s->tx_fifo;
> +
> +    if (fifo8_is_full(tx_fifo)) {
> +        log_guest_error("TX buffer overflow");
> +
> +        return false;
> +    }
> +
> +    fifo8_push(tx_fifo, ch);
> +
> +    return true;
> +}
> +
> +static gboolean bcm2835_aux_xmit_handler(void *do_not_use, GIOCondition cond,
> +                                         void *opaque)
> +{
> +    BCM2835AuxState *s = opaque;
> +    Fifo8 *tx_fifo = &s->tx_fifo;
> +
> +    if (!fifo8_is_empty(tx_fifo)) {
> +        const uint8_t ch = fifo8_pop(&s->tx_fifo);
> +        qemu_chr_fe_write(&s->chr, &ch, 1);

You can write more than one character at a time here.
See hw/char/cadence_uart.c as an example of how to
do a non-blocking write with a UART with a FIFO,
or sifive_uart.c for one that uses a fifo8.

> +
> +        return G_SOURCE_CONTINUE;

No other code in hw/char uses G_SOURCE_CONTINUE, so chances
are high this device should not either.

> +    } else {
> +        bcm2835_aux_update(s);
> +
> +        return G_SOURCE_REMOVE;
> +    }
> +}
> +
> +static bool bcm2835_aux_is_tx_busy(const BCM2835AuxState *s)
> +{
> +    return !(s->stat & STAT_TRANSMITTER_DONE);
> +}
> +
> +static bool bcm2835_aux_can_send(const BCM2835AuxState *s)
> +{
> +    return bcm2835_aux_is_tx_enabled(s) && !bcm2835_aux_is_tx_busy(s);
> +}
> +
> +static void bcm2835_aux_send(BCM2835AuxState *s)
> +{
> +    if (bcm2835_aux_can_send(s)) {
> +        const uint8_t ch = fifo8_pop(&s->tx_fifo);
> +        qemu_chr_fe_write(&s->chr, &ch, 1);
> +        qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> +                              bcm2835_aux_xmit_handler, s);

There's a nice way to structure the code so that both the
initial "try to send the data to the chardev frontend" and the
later "glib told us we should try to send again" are the same
function -- again, see the cadence UART or some other existing
device for examples. If you use the same pattern as existing
code it's easier to review.

You're also missing the "instant drain the fifo when there's
no back-end" check which those other devices have.

> +    }
> +}
> +
> +static void bcm2835_aux_transmit(BCM2835AuxState *s, uint8_t ch)
> +{
> +    const bool result = bcm2835_aux_put_tx_fifo(s, ch);
> +
> +    if (result) {
> +        bcm2835_aux_send(s);
> +    }
> +
> +    bcm2835_aux_update(s);
> +}
> +
>  static uint64_t bcm2835_aux_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      BCM2835AuxState *s = opaque;
> @@ -205,9 +287,7 @@ static void bcm2835_aux_write(void *opaque, hwaddr 
> offset, uint64_t value,
>          ch = value;
>          /* XXX this blocks entire thread. Rewrite to use
>           * qemu_chr_fe_write and background I/O callbacks */

We've fixed this issue, so we can remove the XXX comment now.

> -        if (bcm2835_aux_is_tx_enabled(s)) {
> -            qemu_chr_fe_write_all(&s->chr, &ch, 1);
> -        }
> +        bcm2835_aux_transmit(s, ch);
>          break;
>
>      case AUX_MU_IER_REG:
> @@ -264,7 +344,6 @@ static int bcm2835_aux_can_receive(void *opaque)
>
>  static void bcm2835_aux_put_fifo(BCM2835AuxState *s, uint8_t value)
>  {
> -    BCM2835AuxState *s = opaque;

This looks like a change that should be in some other patch ?

>      Fifo8 *rx_fifo = &s->rx_fifo;
>
>      if (fifo8_is_full(rx_fifo) == false) {
> @@ -298,10 +377,11 @@ static const VMStateDescription vmstate_bcm2835_aux = {
>      .minimum_version_id = 1,
>      .fields = (const VMStateField[]) {
>          VMSTATE_FIFO8(rx_fifo, BCM2835AuxState),
> -        VMSTATE_FIFO8(_tx_fifo, BCM2835AuxState),
> +        VMSTATE_FIFO8(tx_fifo, BCM2835AuxState),
>          VMSTATE_UINT32(ier, BCM2835AuxState),
>          VMSTATE_UINT32(iir, BCM2835AuxState),
>          VMSTATE_UINT32(cntl, BCM2835AuxState),
> +        VMSTATE_UINT32(stat, BCM2835AuxState),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> @@ -311,8 +391,6 @@ 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);
> @@ -323,6 +401,16 @@ static void bcm2835_aux_realize(DeviceState *dev, Error 
> **errp)
>  {
>      BCM2835AuxState *s = BCM2835_AUX(dev);
>
> +    fifo8_create(&s->rx_fifo, BCM2835_AUX_RX_FIFO_LEN);
> +    fifo8_create(&s->tx_fifo, BCM2835_AUX_TX_FIFO_LEN);
> +    s->ier = 0x0;
> +    /* FIFOs enabled and interrupt pending */
> +    s->iir = 0xC1;
> +    /* Both transmitter and receiver are initially enabled */
> +    s->cntl = 0x3;
> +    /* Transmitter done and FIFO empty */
> +    s->stat = 0x300;
>
>      qemu_chr_fe_set_handlers(&s->chr, bcm2835_aux_can_receive,
>                               bcm2835_aux_receive, NULL, NULL, s, NULL, true);
>  }
> diff --git a/include/hw/char/bcm2835_aux.h b/include/hw/char/bcm2835_aux.h
> index feaedc9e2f..f024277169 100644
> --- a/include/hw/char/bcm2835_aux.h
> +++ b/include/hw/char/bcm2835_aux.h
> @@ -27,10 +27,9 @@ struct BCM2835AuxState {
>      qemu_irq irq;
>
>      Fifo8 rx_fifo;
> -    /* Unused for now */
> -    Fifo8 _tx_fifo;
> +    Fifo8 tx_fifo;
>      /* Registers */
> -    uint32_t ier, iir, cntl;
> +    uint32_t ier, iir, cntl, stat;
>  };

thanks
-- PMM