diff mbox series

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

Message ID 20200118164229.22539-6-linux@roeck-us.net (mailing list archive)
State New, archived
Headers show
Series Fix Exynos4210 DMA support | expand

Commit Message

Guenter Roeck Jan. 18, 2020, 4:42 p.m. UTC 
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(-)

Comments

Peter Maydell Jan. 20, 2020, 1:58 p.m. UTC | #1 
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.

> @@ -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.)
 * if something really does need adding to the migration state,
then the version_id and minimum_version_id need to be bumped
(so migration fails cleanly rather than confusingly).
 * if you want migration to continue to work across versions
(which we don't care about for the exynos boards but does
apply for boards like 'virt'), this can be done by adding
a 'subsection' to the vmstate.

I think the answer in this case is just "you don't need to
add this line to the vmstate at all". (This does mean that
a vmsave/vmload will slightly extend the rx-timeout and
delay the interrupt because we re-calculate the timer,
but I guess that's OK. If you don't like that and would
prefer the timer to retain the exact timeout value across
migration, then keep the new vmstate array entry, bump the
version fields, and don't call exynos4210_uart_rx_timeout_set()
in post-load.)

thanks
-- PMM
Guenter Roeck Jan. 20, 2020, 3:04 p.m. UTC | #2 
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
diff mbox series

Patch

diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index 5d48701b6d..63ea9663f2 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -24,6 +24,7 @@ 
 #include "migration/vmstate.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
+#include "qemu/timer.h"
 #include "chardev/char-fe.h"
 #include "chardev/char-serial.h"
 
@@ -118,6 +119,7 @@  static const Exynos4210UartReg exynos4210_uart_regs[] = {
 #define ULCON_STOP_BIT_SHIFT  1
 
 /* UART Tx/Rx Status */
+#define UTRSTAT_Rx_TIMEOUT              0x8
 #define UTRSTAT_TRANSMITTER_EMPTY       0x4
 #define UTRSTAT_Tx_BUFFER_EMPTY         0x2
 #define UTRSTAT_Rx_BUFFER_DATA_READY    0x1
@@ -147,6 +149,9 @@  typedef struct Exynos4210UartState {
     Exynos4210UartFIFO   rx;
     Exynos4210UartFIFO   tx;
 
+    QEMUTimer *fifo_timeout_timer;
+    uint64_t wordtime;        /* word time in ns */
+
     CharBackend       chr;
     qemu_irq          irq;
 
@@ -209,15 +214,12 @@  static void fifo_reset(Exynos4210UartFIFO *q)
     q->rp = 0;
 }
 
-static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState *s)
+static uint32_t exynos4210_uart_FIFO_trigger_level(uint32_t channel,
+                                                   uint32_t reg)
 {
-    uint32_t level = 0;
-    uint32_t reg;
+    uint32_t level;
 
-    reg = (s->reg[I_(UFCON)] & UFCON_Tx_FIFO_TRIGGER_LEVEL) >>
-            UFCON_Tx_FIFO_TRIGGER_LEVEL_SHIFT;
-
-    switch (s->channel) {
+    switch (channel) {
     case 0:
         level = reg * 32;
         break;
@@ -231,12 +233,34 @@  static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState
         break;
     default:
         level = 0;
-        trace_exynos_uart_channel_error(s->channel);
+        trace_exynos_uart_channel_error(channel);
+        break;
     }
-
     return level;
 }
 
+static uint32_t
+exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState *s)
+{
+    uint32_t reg;
+
+    reg = (s->reg[I_(UFCON)] & UFCON_Tx_FIFO_TRIGGER_LEVEL) >>
+            UFCON_Tx_FIFO_TRIGGER_LEVEL_SHIFT;
+
+    return exynos4210_uart_FIFO_trigger_level(s->channel, reg);
+}
+
+static uint32_t
+exynos4210_uart_Rx_FIFO_trigger_level(const Exynos4210UartState *s)
+{
+    uint32_t reg;
+
+    reg = ((s->reg[I_(UFCON)] & UFCON_Rx_FIFO_TRIGGER_LEVEL) >>
+            UFCON_Rx_FIFO_TRIGGER_LEVEL_SHIFT) + 1;
+
+    return exynos4210_uart_FIFO_trigger_level(s->channel, reg);
+}
+
 static void exynos4210_uart_update_irq(Exynos4210UartState *s)
 {
     /*
@@ -244,13 +268,25 @@  static void exynos4210_uart_update_irq(Exynos4210UartState *s)
      * transmit FIFO is smaller than the trigger level.
      */
     if (s->reg[I_(UFCON)] & UFCON_FIFO_ENABLE) {
-
         uint32_t count = (s->reg[I_(UFSTAT)] & UFSTAT_Tx_FIFO_COUNT) >>
                 UFSTAT_Tx_FIFO_COUNT_SHIFT;
 
         if (count <= exynos4210_uart_Tx_FIFO_trigger_level(s)) {
             s->reg[I_(UINTSP)] |= UINTSP_TXD;
         }
+
+        /*
+         * Rx interrupt if trigger level is reached or if rx timeout
+         * interrupt is disabled and there is data in the receive buffer
+         */
+        count = fifo_elements_number(&s->rx);
+        if ((count && !(s->reg[I_(UCON)] & 0x80)) ||
+            count >= exynos4210_uart_Rx_FIFO_trigger_level(s)) {
+            s->reg[I_(UINTSP)] |= UINTSP_RXD;
+            timer_del(s->fifo_timeout_timer);
+        }
+    } else if (s->reg[I_(UTRSTAT)] & UTRSTAT_Rx_BUFFER_DATA_READY) {
+        s->reg[I_(UINTSP)] |= UINTSP_RXD;
     }
 
     s->reg[I_(UINTP)] = s->reg[I_(UINTSP)] & ~s->reg[I_(UINTM)];
@@ -264,6 +300,21 @@  static void exynos4210_uart_update_irq(Exynos4210UartState *s)
     }
 }
 
+static void exynos4210_uart_timeout_int(void *opaque)
+{
+    Exynos4210UartState *s = opaque;
+
+    trace_exynos_uart_rx_timeout(s->channel, s->reg[I_(UTRSTAT)],
+                                 s->reg[I_(UINTSP)]);
+
+    if ((s->reg[I_(UTRSTAT)] & UTRSTAT_Rx_BUFFER_DATA_READY) ||
+        (s->reg[I_(UCON)] & (1 << 11))) {
+        s->reg[I_(UINTSP)] |= UINTSP_RXD;
+        s->reg[I_(UTRSTAT)] |= UTRSTAT_Rx_TIMEOUT;
+        exynos4210_uart_update_irq(s);
+    }
+}
+
 static void exynos4210_uart_update_parameters(Exynos4210UartState *s)
 {
     int speed, parity, data_bits, stop_bits;
@@ -302,10 +353,24 @@  static void exynos4210_uart_update_parameters(Exynos4210UartState *s)
     ssp.data_bits = data_bits;
     ssp.stop_bits = stop_bits;
 
+    s->wordtime = NANOSECONDS_PER_SECOND * (data_bits + stop_bits + 1) / speed;
+
     qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
 
     trace_exynos_uart_update_params(
-                s->channel, speed, parity, data_bits, stop_bits);
+                s->channel, speed, parity, data_bits, stop_bits, s->wordtime);
+}
+
+static void exynos4210_uart_rx_timeout_set(Exynos4210UartState *s)
+{
+    if (s->reg[I_(UCON)] & 0x80) {
+        uint32_t timeout = ((s->reg[I_(UCON)] >> 12) & 0x0f) * s->wordtime;
+
+        timer_mod(s->fifo_timeout_timer,
+                  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + timeout);
+    } else {
+        timer_del(s->fifo_timeout_timer);
+    }
 }
 
 static void exynos4210_uart_write(void *opaque, hwaddr offset,
@@ -361,6 +426,10 @@  static void exynos4210_uart_write(void *opaque, hwaddr offset,
         exynos4210_uart_update_irq(s);
         break;
     case UTRSTAT:
+        if (val & UTRSTAT_Rx_TIMEOUT) {
+            s->reg[I_(UTRSTAT)] &= ~UTRSTAT_Rx_TIMEOUT;
+        }
+        break;
     case UERSTAT:
     case UFSTAT:
     case UMSTAT:
@@ -376,12 +445,17 @@  static void exynos4210_uart_write(void *opaque, hwaddr offset,
         exynos4210_uart_update_irq(s);
         break;
     case UCON:
+        s->reg[I_(UCON)] = val;
+        exynos4210_uart_rx_timeout_set(s);
+        exynos4210_uart_update_irq(s);
+        break;
     case UMCON:
     default:
         s->reg[I_(offset)] = val;
         break;
     }
 }
+
 static uint64_t exynos4210_uart_read(void *opaque, hwaddr offset,
                                   unsigned size)
 {
@@ -461,7 +535,6 @@  static int exynos4210_uart_can_receive(void *opaque)
     return fifo_empty_elements_number(&s->rx);
 }
 
-
 static void exynos4210_uart_receive(void *opaque, const uint8_t *buf, int size)
 {
     Exynos4210UartState *s = (Exynos4210UartState *)opaque;
@@ -469,24 +542,17 @@  static void exynos4210_uart_receive(void *opaque, const uint8_t *buf, int size)
 
     if (s->reg[I_(UFCON)] & UFCON_FIFO_ENABLE) {
         if (fifo_empty_elements_number(&s->rx) < size) {
-            for (i = 0; i < fifo_empty_elements_number(&s->rx); i++) {
-                fifo_store(&s->rx, buf[i]);
-            }
+            size = fifo_empty_elements_number(&s->rx);
             s->reg[I_(UINTSP)] |= UINTSP_ERROR;
-            s->reg[I_(UTRSTAT)] |= UTRSTAT_Rx_BUFFER_DATA_READY;
-        } else {
-            for (i = 0; i < size; i++) {
-                fifo_store(&s->rx, buf[i]);
-            }
-            s->reg[I_(UTRSTAT)] |= UTRSTAT_Rx_BUFFER_DATA_READY;
         }
-        /* XXX: Around here we maybe should check Rx trigger level */
-        s->reg[I_(UINTSP)] |= UINTSP_RXD;
+        for (i = 0; i < size; i++) {
+            fifo_store(&s->rx, buf[i]);
+        }
+        exynos4210_uart_rx_timeout_set(s);
     } else {
         s->reg[I_(URXH)] = buf[0];
-        s->reg[I_(UINTSP)] |= UINTSP_RXD;
-        s->reg[I_(UTRSTAT)] |= UTRSTAT_Rx_BUFFER_DATA_READY;
     }
+    s->reg[I_(UTRSTAT)] |= UTRSTAT_Rx_BUFFER_DATA_READY;
 
     exynos4210_uart_update_irq(s);
 }
@@ -527,6 +593,7 @@  static int exynos4210_uart_post_load(void *opaque, int version_id)
     Exynos4210UartState *s = (Exynos4210UartState *)opaque;
 
     exynos4210_uart_update_parameters(s);
+    exynos4210_uart_rx_timeout_set(s);
 
     return 0;
 }
@@ -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()
     }
 };
@@ -588,6 +656,10 @@  static void exynos4210_uart_init(Object *obj)
     SysBusDevice *dev = SYS_BUS_DEVICE(obj);
     Exynos4210UartState *s = EXYNOS4210_UART(dev);
 
+    s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                         exynos4210_uart_timeout_int, s);
+    s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
+
     /* memory mapping */
     memory_region_init_io(&s->iomem, obj, &exynos4210_uart_ops, s,
                           "exynos4210.uart", EXYNOS4210_UART_REGS_MEM_SIZE);
diff --git a/hw/char/trace-events b/hw/char/trace-events
index ba28b45b53..cb73fee6a9 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -81,7 +81,7 @@  nrf51_uart_write(uint64_t addr, uint64_t value, unsigned int size) "addr 0x%" PR
 # exynos4210_uart.c
 exynos_uart_irq_raised(uint32_t channel, uint32_t reg) "UART%d: IRQ raised: 0x%08"PRIx32
 exynos_uart_irq_lowered(uint32_t channel) "UART%d: IRQ lowered"
-exynos_uart_update_params(uint32_t channel, int speed, uint8_t parity, int data, int stop) "UART%d: speed: %d, parity: %c, data bits: %d, stop bits: %d"
+exynos_uart_update_params(uint32_t channel, int speed, uint8_t parity, int data, int stop, uint64_t wordtime) "UART%d: speed: %d, parity: %c, data bits: %d, stop bits: %d wordtime: %"PRId64"ns"
 exynos_uart_write(uint32_t channel, uint32_t offset, const char *name, uint64_t val) "UART%d: <0x%04x> %s <- 0x%" PRIx64
 exynos_uart_read(uint32_t channel, uint32_t offset, const char *name, uint64_t val) "UART%d: <0x%04x> %s -> 0x%" PRIx64
 exynos_uart_rx_fifo_reset(uint32_t channel) "UART%d: Rx FIFO Reset"
@@ -94,3 +94,4 @@  exynos_uart_rx_error(uint32_t channel) "UART%d: Rx error"
 exynos_uart_wo_read(uint32_t channel, const char *name, uint32_t reg) "UART%d: Trying to read from WO register: %s [0x%04"PRIx32"]"
 exynos_uart_rxsize(uint32_t channel, uint32_t size) "UART%d: Rx FIFO size: %d"
 exynos_uart_channel_error(uint32_t channel) "Wrong UART channel number: %d"
+exynos_uart_rx_timeout(uint32_t channel, uint32_t stat, uint32_t intsp) "UART%d: Rx timeout stat=0x%x intsp=0x%x"