Re: [PATCH v3] hw/char/pl011: Enable TxFIFO and async transmission

[Peter Maydell]

On Wed, 11 Mar 2020 at 04:09, Gavin Shan <address@hidden> wrote:
>
> The depth of TxFIFO can be 1 or 16 depending on LCR[4]. The TxFIFO is
> disabled when its depth is 1. It's nice to have TxFIFO enabled if
> possible because more characters can be piled and transmitted at once,
> which would have less overhead. Besides, we can be blocked because of
> qemu_chr_fe_write_all(), which isn't nice.
>
> This enables TxFIFO if possible. On ther other hand, the asynchronous
> transmission is enabled if needed, as we did in hw/char/cadence_uart.c
>
> Signed-off-by: Gavin Shan <address@hidden>
> ---
> v3: Use PL011() to do data type conversion
>     Return G_SOURCE_REMOVE when the backend is disconnected in pl011_xmit()
>     Drop parenthesis in the condition validating @size in pl011_write_fifo()
> ---
>  hw/char/pl011.c         | 105 +++++++++++++++++++++++++++++++++++++---
>  include/hw/char/pl011.h |   3 ++
>  2 files changed, 102 insertions(+), 6 deletions(-)

Thanks for this patch. I have some comments on some bits of the
code below.

> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 13e784f9d9..dccb8c42b0 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -169,6 +169,73 @@ static void pl011_set_read_trigger(PL011State *s)
>          s->read_trigger = 1;
>  }
>
> +static gboolean pl011_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> +{
> +    PL011State *s = PL011(opaque);
> +    int ret;
> +
> +    /* Drain FIFO if there is no backend */
> +    if (!qemu_chr_fe_backend_connected(&s->chr)) {
> +        s->write_count = 0;
> +        s->flags &= ~PL011_FLAG_TXFF;
> +        s->flags |= PL011_FLAG_TXFE;
> +        return G_SOURCE_REMOVE;
> +    }

This "handle no backend" code isn't necessary. There was a
period of time when it was, because some of the qemu_chr_fe_*
functions did the wrong thing if called on a CharBackend with
a NULL Chardev, which is why some code in the tree checks it,
but we fixed that. If there's no backend then both
qemu_chr_fe_write() and qemu_chr_fe_add_watch() will return 0
without doing anything, which will make us drain the FIFO
via the "!s->watch_tag" code path below.

> +
> +    /* Nothing to do */
> +    if (!s->write_count) {
> +        return FALSE;
> +    }
> +
> +    ret = qemu_chr_fe_write(&s->chr, s->write_fifo, s->write_count);
> +    if (ret > 0) {
> +        s->write_count -= ret;
> +        memmove(s->write_fifo, s->write_fifo + ret, s->write_count);
> +        s->flags &= ~PL011_FLAG_TXFF;
> +        if (!s->write_count) {
> +            s->flags |= PL011_FLAG_TXFE;
> +        }
> +    }
> +
> +    if (s->write_count) {
> +        s->watch_tag = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> +                                             pl011_xmit, s);
> +        if (!s->watch_tag) {
> +            s->write_count = 0;
> +            s->flags &= ~PL011_FLAG_TXFF;
> +            s->flags |= PL011_FLAG_TXFE;
> +            return FALSE;
> +        }
> +    }
> +
> +    s->int_level |= PL011_INT_TX;

Handling of INT_TX is more complicated when the FIFO is
enabled: the UARTIFLS.TXIFLSEL bits define at what point
we should raise the TX interrupt as the FIFO drains
(eg you can make it interrupt as the FIFO passes through
the "half full" point, or when it gets <= 1/8th full, etc).
Watch out that the definition is that the interrupt is raised
as the FIFO fill level progresses through the trigger
level, which is not the same as "is the FIFO fill level
less than or equal to the trigger level now?".

> +    pl011_update(s);
> +    return FALSE;
> +}
> +
> +static void pl011_write_fifo(void *opaque, const unsigned char *buf, int 
> size)
> +{
> +    PL011State *s = PL011(opaque);
> +    int depth = (s->lcr & 0x10) ? 16 : 1;
> +
> +    if (size >= depth - s->write_count) {
> +        size = depth - s->write_count;
> +    }
> +
> +    if (size > 0) {
> +        memcpy(s->write_fifo + s->write_count, buf, size);
> +        s->write_count += size;
> +        if (s->write_count >= depth) {
> +            s->flags |= PL011_FLAG_TXFF;
> +        }
> +        s->flags &= ~PL011_FLAG_TXFE;
> +    }
> +
> +    if (!s->watch_tag) {
> +        pl011_xmit(NULL, G_IO_OUT, s);
> +    }
> +}

It looks like we only ever call pl011_write_fifo() with
a size of 1 -- should we just make it directly take
a single 'uint8_t ch' to write to the FIFO? It would
simplify some of this code I think.

The UARTFR.BUSY bit should be set to 1 as soon as the UART
gets data into the tx FIFO and then cleared only when the
data has all been transmitted. We didn't need to worry about
that when we blocked until the data was sent (the guest could
not execute at a point where it would see BUSY=1), but now
we model the tx FIFO we need to update the BUSY bit (both
in this function to set it and then in anywhere that
empties the FIFO to clear it).

> +
>  static void pl011_write(void *opaque, hwaddr offset,
>                          uint64_t value, unsigned size)
>  {
> @@ -179,13 +246,8 @@ static void pl011_write(void *opaque, hwaddr offset,
>
>      switch (offset >> 2) {
>      case 0: /* UARTDR */
> -        /* ??? Check if transmitter is enabled.  */

This ??? comment is about the fact that we don't check
UARTCR.TXE (the transmit enable bit). Your patch doesn't
add support for that (which is fine, it's entirely separate
from the FIFO stuff), so it shouldn't delete the comment.

>          ch = value;
> -        /* XXX this blocks entire thread. Rewrite to use
> -         * qemu_chr_fe_write and background I/O callbacks */
> -        qemu_chr_fe_write_all(&s->chr, &ch, 1);
> -        s->int_level |= PL011_INT_TX;
> -        pl011_update(s);
> +        pl011_write_fifo(opaque, &ch, 1);
>          break;
>      case 1: /* UARTRSR/UARTECR */
>          s->rsr = 0;
> @@ -207,7 +269,16 @@ static void pl011_write(void *opaque, hwaddr offset,
>          if ((s->lcr ^ value) & 0x10) {
>              s->read_count = 0;
>              s->read_pos = 0;
> +
> +            if (s->watch_tag) {
> +                g_source_remove(s->watch_tag);
> +                s->watch_tag = 0;
> +            }
> +            s->write_count = 0;
> +            s->flags &= ~PL011_FLAG_TXFF;
> +            s->flags |= PL011_FLAG_TXFE;
>          }
> +
>          s->lcr = value;
>          pl011_set_read_trigger(s);
>          break;
> @@ -292,6 +363,24 @@ static const MemoryRegionOps pl011_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };

thanks
-- PMM