Re: [PATCH v2 5/7] hw/char/exynos4210_uart: Implement Rx FIFO level triggers and timeouts

[Guenter Roeck <linux@roeck-us.net>]

On 1/20/20 5:58 AM, Peter Maydell wrote:
> On Sat, 18 Jan 2020 at 16:42, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> The driver already implements a receive FIFO, but it does not
>> handle receive FIFO trigger levels and timeout. Implement the
>> missing functionality.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Call exynos4210_uart_rx_timeout_set() from new post_load function
>>      to set the receive timeout timer.
>>      Add timer to vmstate_exynos4210_uart.
>>
>>   hw/char/exynos4210_uart.c | 122 ++++++++++++++++++++++++++++++--------
>>   hw/char/trace-events      |   3 +-
>>   2 files changed, 99 insertions(+), 26 deletions(-)
> 
> Since the timeout value depends on s->wordtime, and
> exynos4210_uart_update_parameters() can change s->wordtime,
> do you need to recalculate the timeout at that point?
> This would correspond to if the guest wrote to the
> UBRDIV/UFRACVAL/ULCON registers, I think. Maybe this comes
> under the heading of "undefined behaviour if the guest does
> this odd thing" ? (The exact behaviour of the h/w is probably
> undocumented and mildly painful to emulate exactly, so it's
> hard to see why QEMU should care about getting it exactly right.)
> 
> I did also wonder whether writing the same value to the UCON
> timeout-interval field repeatedly really does restart the timer
> counting down from 8*(N+1) frames again, but again maybe that's
> just weird for a guest to do.
> 

The datasheet only talks about the number of word times that is set
with the UCON register. It doesn't say what the hardware does
if the word time (baud rate) is changed while data is in the receive
queue. But then data in the receive queue suggests that the remote
end is actively transmitting, and changing the baud rate in that
situation would result in a mess. With that in mind, I don't think
we need to be concerned about an inaccurate word time for a few
milliseconds after a baud-rate change.

I agree that changing the UCON value might not have an impact
on data already in the queue. I'll drop that call - I would guess
that HW doesn't bother recalculating anything when UCON is set
(changed or not), and it doesn't really matter, so why bother
with the extra code.

>> @@ -553,6 +620,7 @@ static const VMStateDescription vmstate_exynos4210_uart = {
>>                          vmstate_exynos4210_uart_fifo, Exynos4210UartFIFO),
>>           VMSTATE_UINT32_ARRAY(reg, Exynos4210UartState,
>>                                EXYNOS4210_UART_REGS_MEM_SIZE / sizeof(uint32_t)),
>> +        VMSTATE_TIMER_PTR(fifo_timeout_timer, Exynos4210UartState),
>>           VMSTATE_END_OF_LIST()
>>       }
>>   };
> 
> Unfortunately you can't simply add entries to a VMStateDescription:
> it breaks migration compatibility.
> 
> The choices here are:
>   * the nicest approach if it works is that in the post_load
> function you just recalculate the timer timeout. Then there's
> no need to migrate the current state of the timer. (In fact
> it looks like your code does do this in post_load.)


Correct, and that was the idea. The rest is just a lack of
understanding, so I'll drop VMSTATE_TIMER_PTR.

Thanks,
Guenter