Re: [RFC PATCH v2 11/11] hw/char/pl011: Implement TX FIFO

[Richard Henderson]

On 7/10/23 18:51, Philippe Mathieu-Daudé wrote:
> +static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
> +{
> +    PL011State *s = opaque;
> +    int ret;
> +    const uint8_t *buf;
> +    uint32_t buflen;
> +    uint32_t count;
> +    bool tx_enabled;
> +
> +    if (!qemu_chr_fe_backend_connected(&s->chr)) {
> +        /* Instant drain the fifo when there's no back-end */
> +        return pl011_drain_tx(s);
> +    }
> +
> +    tx_enabled = s->cr & CR_UARTEN;

What happened to "Hello, World"?  We ought to be consistent.
For actual modeling, I think you need TXE too.

Where does UARTFR get updated after successfully transmitting data?

>   static void pl011_write_txdata(PL011State *s, const uint8_t *buf, int length)
> @@ -162,12 +218,32 @@ static void pl011_write_txdata(PL011State *s, const 
> uint8_t *buf, int length)
>       if (!(s->cr & CR_TXE)) {
>           qemu_log_mask(LOG_GUEST_ERROR, "PL011 write data but TX disabled\n");
>       }
> +    if (!fifo8_is_empty(&s->xmit_fifo)) {
> +        /*
> +         * If the UART is disabled in the middle of transmission
> +         * or reception, it completes the current character before
> +         * stopping.
> +         */
> +        pl011_xmit(NULL, G_IO_OUT, s);
> +        return;
> +    }

Why is this in write_txdata?  I would expect to find this with a write to 
UARTCR.
You appear to *not* be queuing data unless the fifo is empty.

> +    if (length > fifo8_num_free(&s->xmit_fifo)) {
> +        /*
> +         * The FIFO contents remain valid because no more data is
> +         * written when the FIFO is full, only the contents of the
> +         * shift register are overwritten. The CPU must now read
> +         * the data, to empty the FIFO.
> +         */
> +        trace_pl011_fifo_tx_overrun();
> +        s->rsr |= RSR_OE;
> +        return;
> +    }
> +
> +    trace_pl011_fifo_tx_put(length);
> +    fifo8_push_all(&s->xmit_fifo, buf, length);

Since length will always be 1, probably we should just remove it.

> +static bool pl011_xmit_fifo_state_needed(void *opaque, int version_id)
> +{
> +    PL011State* s = opaque;
> +
> +    return pl011_is_fifo_enabled(s) && !fifo8_is_empty(&s->xmit_fifo);
> +}

Ok.

>   static int pl011_post_load(void *opaque, int version_id)
>   {
>       PL011State* s = opaque;
> @@ -455,6 +538,11 @@ static int pl011_post_load(void *opaque, int version_id)
>           s->read_pos = 0;
>       }
>   
> +    if (pl011_xmit_fifo_state_needed(s, version_id)) {
> +        /* Reschedule another transmission */
> +        qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP, pl011_xmit, s);
> +    }

Ok.

> @@ -473,6 +561,7 @@ static const VMStateDescription vmstate_pl011 = {
>           VMSTATE_UINT32(int_enabled, PL011State),
>           VMSTATE_UINT32(int_level, PL011State),
>           VMSTATE_UINT32_ARRAY(read_fifo, PL011State, PL011_FIFO_DEPTH),
> +        VMSTATE_FIFO8_TEST(xmit_fifo, PL011State, 
> pl011_xmit_fifo_state_needed),

Not ok.

The new data should go in its own VMStateDescription, like vmstate_pl011_clock.


r~