[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH v2 11/11] hw/char/pl011: Implement TX FIFO
From: |
Richard Henderson |
Subject: |
Re: [RFC PATCH v2 11/11] hw/char/pl011: Implement TX FIFO |
Date: |
Fri, 14 Jul 2023 08:27:48 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 |
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~
- Re: [PATCH v2 05/11] hw/char/pl011: Split RX/TX path of pl011_reset_fifo(), (continued)
- [PATCH v2 08/11] hw/char/pl011: Warn when using disabled transmitter, Philippe Mathieu-Daudé, 2023/07/10
- [PATCH v2 07/11] hw/char/pl011: Extract pl011_read_rxdata() from pl011_read(), Philippe Mathieu-Daudé, 2023/07/10
- [PATCH v2 09/11] hw/char/pl011: Check if receiver is enabled, Philippe Mathieu-Daudé, 2023/07/10
- [PATCH v2 10/11] hw/char/pl011: Rename RX FIFO methods, Philippe Mathieu-Daudé, 2023/07/10
- [RFC PATCH v2 11/11] hw/char/pl011: Implement TX FIFO, Philippe Mathieu-Daudé, 2023/07/10
- Re: [RFC PATCH v2 11/11] hw/char/pl011: Implement TX FIFO,
Richard Henderson <=