diff mbox series

[PATCH-for-8.2,v4,04/10] hw/char/pl011: Extract pl011_write_txdata() from pl011_write()

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

Commit Message

Philippe Mathieu-Daudé Nov. 9, 2023, 7:28 p.m. UTC 
When implementing FIFO, this code will become more complex.
Start by factoring it out to a new pl011_write_txdata() function.
No functional change intended.

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

Comments

Richard Henderson Nov. 9, 2023, 11:17 p.m. UTC | #1 
On 11/9/23 11:28, Philippe Mathieu-Daudé wrote:
> When implementing FIFO, this code will become more complex.
> Start by factoring it out to a new pl011_write_txdata() function.
> No functional change intended.

...
> @@ -262,19 +273,13 @@ 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);
> +        s->readbuff = value;

Why the write to readbuff?


r~
Philippe Mathieu-Daudé Dec. 13, 2023, 9:13 a.m. UTC | #2 
On 10/11/23 00:17, Richard Henderson wrote:
> On 11/9/23 11:28, Philippe Mathieu-Daudé wrote:
>> When implementing FIFO, this code will become more complex.
>> Start by factoring it out to a new pl011_write_txdata() function.
>> No functional change intended.
> 
> ...
>> @@ -262,19 +273,13 @@ 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);
>> +        s->readbuff = value;
> 
> Why the write to readbuff?

I think I wanted to use it when FIFO is disabled due to:

https://developer.arm.com/documentation/ddi0183/g/programmers-model/register-descriptions/line-control-register--uartlcr-h?lang=en

UARTLCR_H.FEN:

   Enable FIFOs:

   0 = FIFOs are disabled (character mode) that is, the FIFOs become
       1-byte-deep holding registers

   1 = transmit and receive FIFO buffers are enabled (FIFO mode).

and we don't have a fifo8_change_capacity() method.

Otherwise, not sure what for is this field... I'll see if we can
just remove it.
diff mbox series

Patch

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 1f07c7b021..1cb9015ea2 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, uint8_t data)
+{
+    /* ??? 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, &data, 1);
+    s->int_level |= INT_TX;
+    pl011_update(s);
+}
+
 static uint64_t pl011_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
@@ -262,19 +273,13 @@  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);
+        s->readbuff = value;
+        pl011_write_txdata(s, value);
         break;
     case 1: /* UARTRSR/UARTECR */
         s->rsr = 0;