Re: [PATCH 2/3] hw/char: cadence_uart: Disable transmit when input clock is disabled

[Philippe Mathieu-Daudé]

On 8/23/21 11:57 AM, Bin Meng wrote:
> On Mon, Aug 23, 2021 at 4:14 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> wrote:
>>
>> On 8/23/21 4:08 AM, Bin Meng wrote:
>>> At present when input clock is disabled, any character transmitted
>>> to tx fifo can still show on the serial line, which is wrong.
>>>
>>> Fixes: b636db306e06 ("hw/char/cadence_uart: add clock support")
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>>
>>>  hw/char/cadence_uart.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
>>> index b4b5e8a3ee..154be34992 100644
>>> --- a/hw/char/cadence_uart.c
>>> +++ b/hw/char/cadence_uart.c
>>> @@ -327,6 +327,11 @@ static gboolean cadence_uart_xmit(void *do_not_use, 
>>> GIOCondition cond,
>>>  static void uart_write_tx_fifo(CadenceUARTState *s, const uint8_t *buf,
>>>                                 int size)
>>>  {
>>> +    /* ignore characters when unclocked or in reset */
>>> +    if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
>>> +        return;
>>> +    }
>>
>> Incorrect handler?
>>
> 
> Sorry I don't get it. This patch is for the Tx path, while patch #3 is
> for the Rx path.

Sorry, I was not totally awake o_O

-- >8 --
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index b4b5e8a3ee0..78990ea79dc 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -403,15 +403,20 @@ static void uart_read_rx_fifo(CadenceUARTState *s,
uint32_t *c)
     uart_update_status(s);
 }

-static void uart_write(void *opaque, hwaddr offset,
-                          uint64_t value, unsigned size)
+static MemTxResult uart_write(void *opaque, hwaddr offset, uint64_t value,
+                              unsigned size, MemTxAttrs attrs)
 {
     CadenceUARTState *s = opaque;

+    /* ignore characters when unclocked or in reset */
+    if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
+        return MEMTX_DECODE_ERROR;
+    }
+
     DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value);
     offset >>= 2;
     if (offset >= CADENCE_UART_R_MAX) {
-        return;
+        return MEMTX_OK;
     }
     switch (offset) {
     case R_IER: /* ier (wts imr) */
@@ -458,14 +463,21 @@ static void uart_write(void *opaque, hwaddr offset,
         break;
     }
     uart_update_status(s);
+
+    return MEMTX_OK;
 }

-static uint64_t uart_read(void *opaque, hwaddr offset,
-        unsigned size)
+static MemTxResult uart_read(void *opaque, hwaddr offset, uint64_t *data,
+                             unsigned size, MemTxAttrs attrs)
 {
     CadenceUARTState *s = opaque;
     uint32_t c = 0;

+    /* ignore characters when unclocked or in reset */
+    if (!clock_is_enabled(s->refclk) || device_is_in_reset(DEVICE(s))) {
+        return MEMTX_DECODE_ERROR;
+    }
+
     offset >>= 2;
     if (offset >= CADENCE_UART_R_MAX) {
         c = 0;
@@ -476,12 +488,14 @@ static uint64_t uart_read(void *opaque, hwaddr offset,
     }

     DB_PRINT(" offset:%x data:%08x\n", (unsigned)(offset << 2),
(unsigned)c);
-    return c;
+    *data = c;
+
+    return MEMTX_OK;
 }

 static const MemoryRegionOps uart_ops = {
-    .read = uart_read,
-    .write = uart_write,
+    .read_with_attrs = uart_read,
+    .write_with_attrs = uart_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
---