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

[Gavin Shan <gshan@redhat.com>]

On 3/11/20 3:09 PM, Gavin Shan 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 <gshan@redhat.com>
> ---
> 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(-)
> 

Marc-André, ping. Could you please review when you get a chance? Thanks in
advance :)

> 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;
> +    }
> +
> +    /* 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;
> +    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);
> +    }
> +}
> +
>   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.  */
>           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,
>   };
>   
> +static bool pl011_write_fifo_needed(void *opaque)
> +{
> +    PL011State *s = PL011(opaque);
> +    return s->write_count > 0;
> +}
> +
> +static const VMStateDescription vmstate_pl011_write_fifo = {
> +    .name = "pl011/write_fifo",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = pl011_write_fifo_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT32(write_count, PL011State),
> +        VMSTATE_UINT8_ARRAY(write_fifo, PL011State, 16),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   static const VMStateDescription vmstate_pl011 = {
>       .name = "pl011",
>       .version_id = 2,
> @@ -314,6 +403,10 @@ static const VMStateDescription vmstate_pl011 = {
>           VMSTATE_INT32(read_count, PL011State),
>           VMSTATE_INT32(read_trigger, PL011State),
>           VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription * []) {
> +        &vmstate_pl011_write_fifo,
> +        NULL
>       }
>   };
>   
> diff --git a/include/hw/char/pl011.h b/include/hw/char/pl011.h
> index 14187165c6..9d1c24db48 100644
> --- a/include/hw/char/pl011.h
> +++ b/include/hw/char/pl011.h
> @@ -38,6 +38,7 @@ typedef struct PL011State {
>       uint32_t int_enabled;
>       uint32_t int_level;
>       uint32_t read_fifo[16];
> +    uint8_t  write_fifo[16];
>       uint32_t ilpr;
>       uint32_t ibrd;
>       uint32_t fbrd;
> @@ -45,6 +46,8 @@ typedef struct PL011State {
>       int read_pos;
>       int read_count;
>       int read_trigger;
> +    int write_count;
> +    guint watch_tag;
>       CharBackend chr;
>       qemu_irq irq[6];
>       const unsigned char *id;
>