[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Guenter Roeck
Subject: Re: [PATCH v2 5/7] hw/char/exynos4210_uart: Implement Rx FIFO level triggers and timeouts
Date: Mon, 20 Jan 2020 07:04:22 -0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2

On 1/20/20 5:58 AM, Peter Maydell wrote:
On Sat, 18 Jan 2020 at 16:42, Guenter Roeck <address@hidden> 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 <address@hidden>
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 / 
+        VMSTATE_TIMER_PTR(fifo_timeout_timer, Exynos4210UartState),

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]