diff mbox series

[v2,06/11] hw/char/pl011: Extract pl011_write_txdata() from pl011_write()

Message ID 20230710175102.32429-7-philmd@linaro.org
State Superseded
Headers show
Series hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop | expand

Commit Message

Philippe Mathieu-Daudé July 10, 2023, 5:50 p.m. UTC 
When implementing FIFO, this code will become more complex.
Start by factoring it out to a new pl011_write_txdata() function.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/char/pl011.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Comments

Richard Henderson July 14, 2023, 6:58 a.m. UTC | #1 
On 7/10/23 18:50, Philippe Mathieu-Daudé wrote:
> +static void pl011_write_txdata(PL011State *s, const uint8_t *buf, int length)
> +{
> +    /* ??? Check if transmitter is enabled.  */
> +
> +    /* XXX this blocks entire thread. Rewrite to use
> +     * qemu_chr_fe_write and background I/O callbacks */
> +    qemu_chr_fe_write_all(&s->chr, buf, 1);

Not using length?


> +        pl011_write_txdata(s, (uint8_t *) &value, 1);

Host endianness error.  Copy to local uint8_t first.


r~
Philippe Mathieu-Daudé Oct. 12, 2023, 1:07 p.m. UTC | #2 
On 14/7/23 08:58, Richard Henderson wrote:
> On 7/10/23 18:50, Philippe Mathieu-Daudé wrote:
>> +static void pl011_write_txdata(PL011State *s, const uint8_t *buf, int 
>> length)
>> +{
>> +    /* ??? Check if transmitter is enabled.  */
>> +
>> +    /* XXX this blocks entire thread. Rewrite to use
>> +     * qemu_chr_fe_write and background I/O callbacks */
>> +    qemu_chr_fe_write_all(&s->chr, buf, 1);
> 
> Not using length?

This is a simple "code extract" patch. Length is used when
we switch to FIFO in the last patch.

>> +        pl011_write_txdata(s, (uint8_t *) &value, 1);
> 
> Host endianness error.  Copy to local uint8_t first.

Oops, good catch, thanks.
Philippe Mathieu-Daudé Oct. 12, 2023, 2:25 p.m. UTC | #3 
On 12/10/23 15:07, Philippe Mathieu-Daudé wrote:
> On 14/7/23 08:58, Richard Henderson wrote:
>> On 7/10/23 18:50, Philippe Mathieu-Daudé wrote:
>>> +static void pl011_write_txdata(PL011State *s, const uint8_t *buf, 
>>> int length)
>>> +{
>>> +    /* ??? Check if transmitter is enabled.  */
>>> +
>>> +    /* XXX this blocks entire thread. Rewrite to use
>>> +     * qemu_chr_fe_write and background I/O callbacks */
>>> +    qemu_chr_fe_write_all(&s->chr, buf, 1);
>>
>> Not using length?
> 
> This is a simple "code extract" patch. Length is used when
> we switch to FIFO in the last patch.

Hmm you are right it isn't used...

The UARTDR register is 16-bit wide. Only 8 bits are used for data.
No need for a 'length' param here.
diff mbox series

Patch

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 1f07c7b021..7bc7819d8b 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -149,6 +149,17 @@  static inline void pl011_reset_tx_fifo(PL011State *s)
     s->flags |= PL011_FLAG_TXFE;
 }
 
+static void pl011_write_txdata(PL011State *s, const uint8_t *buf, int length)
+{
+    /* ??? Check if transmitter is enabled.  */
+
+    /* XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks */
+    qemu_chr_fe_write_all(&s->chr, buf, 1);
+    s->int_level |= INT_TX;
+    pl011_update(s);
+}
+
 static uint64_t pl011_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
@@ -262,19 +273,12 @@  static void pl011_write(void *opaque, hwaddr offset,
                         uint64_t value, unsigned size)
 {
     PL011State *s = (PL011State *)opaque;
-    unsigned char ch;
 
     trace_pl011_write(offset, value, pl011_regname(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 |= INT_TX;
-        pl011_update(s);
+        pl011_write_txdata(s, (uint8_t *) &value, 1);
         break;
     case 1: /* UARTRSR/UARTECR */
         s->rsr = 0;