Hi
On Mon, Feb 24, 2020 at 4:13 AM Gavin Shan <gs...@redhat.com> 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 <gs...@redhat.com>
> ---
> v2: Put write_{count,fifo} into migration subsection
> Don't start async IO handle if it has been started, to avoid race
> Update with PL011_FLAG_{TXFF,TXFE} on changing write_count
> ---
> hw/char/pl011.c | 105 +++++++++++++++++++++++++++++++++++++---
> include/hw/char/pl011.h | 3 ++
> 2 files changed, 102 insertions(+), 6 deletions(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 13e784f9d9..de5c4254fe 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 = (PL011State *)opaque;
Useless casts, perhaps use PL011() instead?
> + 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 FALSE;
Perhaps use 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 = (PL011State *)opaque;
> + int depth = (s->lcr & 0x10) ? 16 : 1;
> +
> + if (size >= (depth - s->write_count)) {
parenthesis could be dropped
> + size = depth - s->write_count;
> + }
> +
Why make a function that accepts size != 1 (and may silently drop
data), when the only caller is pl011_write_fifo(opaque, &ch, 1); ?
> + if (size > 0) {
I don't think other cases are supposed to happen.
> + 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 = (PL011State *)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;
> --
> 2.23.0
>
>
Looks ok otherwise
--
Marc-André Lureau
