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

[Philippe Mathieu-Daudé]

On 8/23/21 3:14 PM, Bin Meng wrote:
> On Mon, Aug 23, 2021 at 6:43 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> wrote:
>>
>> 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,
>>  };
> 
> Thanks, but I think the above change should be a totally separate patch.

Yes, this was not a patch snippet, but the result.

First patch would be convert to memop_with_attrs() and
second return MEMTX_DECODE_ERROR when unclocked.