On 8/23/21 3:14 PM, Bin Meng wrote:
> On Mon, Aug 23, 2021 at 6:43 PM Philippe Mathieu-Daudé <f4...@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é <f4...@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...@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.
