[Qemu-devel] [PATCH 0/5] Generalize Dallas/Maxim I2C RTC devices

[Qemu-devel] [PATCH 0/5] Generalize Dallas/Maxim I2C RTC devices

[Michael Davidsaver]

These changes previously appeared as part of a series "Add MVME3100 PPC SBC v2"
back in November.  David Gibson, who looked that that series, suggested getting
this reviewed separately.  There doesn't appear to be a listed maintainer
for this code, so I'm addressing this to the 3 people who have made more than
cosmetic changes to it.  The most recent of these was in 2012.

This series replaces the ds1338 RTC with a model covering a number of these
similar chips: ds1307, ds1337, ds1338, ds1339, ds1340, ds1375, ds1388,
and ds3231.

The limits of the new model are the same as the old.  Only the time of day
registers, and NVRAM are modeled.  The alarm and control registers are not.

I've added a more thorough test of the time of day function, covering
reading and setting in both 12 and 24 hour mode.  In the process
I found two minor issues with the ds1338 model.  These are described in the
commit message for #3.  So this series first adds those tests which pass with
both old and new model.  Then later adds some additional tests which only
pass with the new model.


Michael Davidsaver (5):
  timer: ds1338 add magic reset for test code
  tests: more thorough test of ds1338
  timer: generalize Dallas/Maxim RTC i2c devices
  tests: ds-rtc-i2c-test test 12 hour mode and DoW
  tests: drop ds1338-test

 default-configs/arm-softmmu.mak |   2 +-
 hw/timer/Makefile.objs          |   2 +-
 hw/timer/ds-rtc-i2c.c           | 466 ++++++++++++++++++++++++++++++++++++++++
 hw/timer/ds1338.c               | 239 ---------------------
 tests/Makefile.include          |   4 +-
 tests/ds-rtc-i2c-test.c         | 245 +++++++++++++++++++++
 tests/ds1338-test.c             |  75 -------
 7 files changed, 715 insertions(+), 318 deletions(-)
 create mode 100644 hw/timer/ds-rtc-i2c.c
 delete mode 100644 hw/timer/ds1338.c
 create mode 100644 tests/ds-rtc-i2c-test.c
 delete mode 100644 tests/ds1338-test.c

-- 
2.11.0

[Qemu-devel] [PATCH 1/5] timer: ds1338 add magic reset for test code

[Michael Davidsaver]

When running w/ QTest, allow the tester
to reliably zero time offsets.
Allows tests to read the current time,
and set time, independent of test order.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/timer/ds1338.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
index 3849b74a68..41c2d7dac6 100644
--- a/hw/timer/ds1338.c
+++ b/hw/timer/ds1338.c
@@ -14,6 +14,7 @@
 #include "qemu-common.h"
 #include "hw/i2c/i2c.h"
 #include "qemu/bcd.h"
+#include "sysemu/qtest.h"
 
 /* Size of NVRAM including both the user-accessible area and the
  * secondary register area.
@@ -132,6 +133,14 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
     DS1338State *s = DS1338(i2c);
 
     if (s->addr_byte) {
+        if (data == 0xff && qtest_enabled()) {
+            /* magic, out of bounds, address to allow test code
+             * to reset offset
+             */
+            s->offset = 0;
+            s->wday_offset = 0;
+            return 0;
+        }
         s->ptr = data & (NVRAM_SIZE - 1);
         s->addr_byte = false;
         return 0;
-- 
2.11.0

[Qemu-devel] [PATCH 2/5] tests: more thorough test of ds1338

[Michael Davidsaver]

Test current time and set+get round trip.

The set+get test is repeated 4 times.  These cases are
spread across a single day in an attempt to trigger some potential
issues regardless of the timezone of the machine running the tests.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 tests/Makefile.include  |   2 +
 tests/ds-rtc-i2c-test.c | 193 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 195 insertions(+)
 create mode 100644 tests/ds-rtc-i2c-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index a1bcbffe12..f5dcd274e0 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -360,6 +360,7 @@ check-qtest-sparc64-y += tests/boot-serial-test$(EXESUF)
 
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 check-qtest-arm-y += tests/ds1338-test$(EXESUF)
+check-qtest-arm-y += tests/ds-rtc-i2c-test$(EXESUF)
 check-qtest-arm-y += tests/m25p80-test$(EXESUF)
 gcov-files-arm-y += hw/misc/tmp105.c
 check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
@@ -764,6 +765,7 @@ tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
 tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
 tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
 tests/ds1338-test$(EXESUF): tests/ds1338-test.o $(libqos-imx-obj-y)
+tests/ds-rtc-i2c-test$(EXESUF): tests/ds-rtc-i2c-test.o $(libqos-imx-obj-y)
 tests/m25p80-test$(EXESUF): tests/m25p80-test.o
 tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
 tests/q35-test$(EXESUF): tests/q35-test.o $(libqos-pc-obj-y)
diff --git a/tests/ds-rtc-i2c-test.c b/tests/ds-rtc-i2c-test.c
new file mode 100644
index 0000000000..464eb08558
--- /dev/null
+++ b/tests/ds-rtc-i2c-test.c
@@ -0,0 +1,193 @@
+/* Testing of Dallas/Maxim I2C bus RTC devices
+ *
+ * Copyright (c) 2017 Michael Davidsaver
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the LICENSE file in the top-level directory.
+ */
+#include <stdio.h>
+
+#include "qemu/osdep.h"
+#include "qemu/bcd.h"
+#include "qemu/cutils.h"
+#include "qemu/timer.h"
+#include "libqtest.h"
+#include "libqos/libqos.h"
+#include "libqos/i2c.h"
+
+#define IMX25_I2C_0_BASE 0x43F80000
+#define DS1338_ADDR 0x68
+
+static I2CAdapter *i2c;
+static uint8_t addr;
+static bool use_century;
+
+static
+time_t rtc_gettime(void)
+{
+    struct tm parts;
+    uint8_t buf[7];
+
+    buf[0] = 0;
+    i2c_send(i2c, addr, buf, 1);
+    i2c_recv(i2c, addr, buf, 7);
+
+    parts.tm_sec = from_bcd(buf[0]);
+    parts.tm_min = from_bcd(buf[1]);
+    if (buf[2] & 0x40) {
+        /* 12 hour */
+        /* HOUR register is 1-12. */
+        parts.tm_hour = from_bcd(buf[2] & 0x1f);
+        g_assert_cmpuint(parts.tm_hour, >=, 1);
+        g_assert_cmpuint(parts.tm_hour, <=, 12);
+        parts.tm_hour %= 12u; /* wrap 12 -> 0 */
+        if (buf[2] & 0x20) {
+            parts.tm_hour += 12u;
+        }
+    } else {
+        /* 24 hour */
+        parts.tm_hour = from_bcd(buf[2] & 0x3f);
+    }
+    parts.tm_wday = from_bcd(buf[3]);
+    parts.tm_mday = from_bcd(buf[4]);
+    parts.tm_mon =  from_bcd((buf[5] & 0x1f) - 1u);
+    parts.tm_year = from_bcd(buf[6]);
+    if (!use_century || (buf[5] & 0x80)) {
+        parts.tm_year += 100u;
+    }
+
+    return mktimegm(&parts);
+}
+
+/* read back and compare with current system time */
+static
+void test_rtc_current(void)
+{
+    uint8_t buf;
+    time_t expected, actual;
+
+    /* magic address to zero RTC time offset
+     * as tests may be run in any order
+     */
+    buf = 0xff;
+    i2c_send(i2c, addr, &buf, 1);
+
+    actual = time(NULL);
+    /* new second may start here */
+    expected = rtc_gettime();
+    g_assert_cmpuint(expected, <=, actual + 1);
+    g_assert_cmpuint(expected, >=, actual);
+}
+
+
+static uint8_t test_time_24_12am[8] = {
+    0, /* address */
+    /* Wed, 22 Nov 2017 00:30:53 +0000 */
+    0x53,
+    0x30,
+    0x00, /* 12 AM in 24 hour mode */
+    0x03, /* monday is our day 1 */
+    0x22,
+    0x11 | 0x80,
+    0x17,
+};
+
+static uint8_t test_time_24_6am[8] = {
+    0, /* address */
+    /* Wed, 22 Nov 2017 06:30:53 +0000 */
+    0x53,
+    0x30,
+    0x06, /* 6 AM in 24 hour mode */
+    0x03, /* monday is our day 1 */
+    0x22,
+    0x11 | 0x80,
+    0x17,
+};
+
+static uint8_t test_time_24_12pm[8] = {
+    0, /* address */
+    /* Wed, 22 Nov 2017 12:30:53 +0000 */
+    0x53,
+    0x30,
+    0x12, /* 12 PM in 24 hour mode */
+    0x03, /* monday is our day 1 */
+    0x22,
+    0x11 | 0x80,
+    0x17,
+};
+
+static uint8_t test_time_24_6pm[8] = {
+    0, /* address */
+    /* Wed, 22 Nov 2017 18:30:53 +0000 */
+    0x53,
+    0x30,
+    0x18, /* 6 PM in 24 hour mode */
+    0x03, /* monday is our day 1 */
+    0x22,
+    0x11 | 0x80,
+    0x17,
+};
+
+/* write in and read back known time */
+static
+void test_rtc_set(const void *raw)
+{
+    const uint8_t *testtime = raw;
+    uint8_t buf[7];
+    unsigned retry = 2;
+
+    for (; retry; retry--) {
+        i2c_send(i2c, addr, testtime, 8);
+        /* new second may start here */
+        i2c_send(i2c, addr, testtime, 1);
+        i2c_recv(i2c, addr, buf, 7);
+
+        if (testtime[1] == buf[0]) {
+            break;
+        }
+        /* we raced start of second, retry */
+    };
+
+    g_assert_cmpuint(testtime[1], ==, buf[0]); /* SEC */
+    g_assert_cmpuint(testtime[2], ==, buf[1]); /* MIN */
+    g_assert_cmpuint(testtime[3], ==, buf[2]); /* HOUR */
+    /* skip comparing Day of Week.  Not handled correctly */
+    g_assert_cmpuint(testtime[5], ==, buf[4]); /* DoM */
+    if (use_century) {
+        g_assert_cmpuint(testtime[6], ==, buf[5]); /* MON+century */
+    } else {
+        g_assert_cmpuint(testtime[6] & 0x7f, ==, buf[5]); /* MON */
+    }
+    g_assert_cmpuint(testtime[7], ==, buf[6]); /* YEAR */
+
+    g_assert_cmpuint(retry, >, 0);
+}
+
+int main(int argc, char *argv[])
+{
+    int ret;
+    const char *arch = qtest_get_arch();
+    QTestState *s = NULL;
+
+    g_test_init(&argc, &argv, NULL);
+
+    if (strcmp(arch, "arm") == 0) {
+        s = qtest_start("-display none -machine imx25-pdk");
+        i2c = imx_i2c_create(s, IMX25_I2C_0_BASE);
+        addr = DS1338_ADDR;
+        use_century = false;
+
+    }
+
+    qtest_add_data_func("/ds-rtc-i2c/set24_12am", test_time_24_12am, test_rtc_set);
+    qtest_add_data_func("/ds-rtc-i2c/set24_6am", test_time_24_6am, test_rtc_set);
+    qtest_add_data_func("/ds-rtc-i2c/set24_12pm", test_time_24_12pm, test_rtc_set);
+    qtest_add_data_func("/ds-rtc-i2c/set24_6pm", test_time_24_6pm, test_rtc_set);
+    qtest_add_func("/ds-rtc-i2c/current", test_rtc_current);
+
+    ret = g_test_run();
+
+    qtest_end();
+
+    return ret;
+}
-- 
2.11.0

[Qemu-devel] [PATCH 3/5] timer: generalize Dallas/Maxim RTC i2c devices

[Michael Davidsaver]

Support for: ds1307, ds1337, ds1338, ds1339,
ds1340, ds1375, ds1388, and ds3231.

Tested with ds1338 and ds1375.

The existing ds1338 model has two bugs
with almost no practical impact.

1. Trying to set time in 12-hour mode works,
but the 12 hour mode bit isn't stored.
So time always reads in 24 hour mode.

2. wday_offset is always stored for the
local time zone.  When the RTC is set
and rtc_utc=1 and the local timezone
has a different day than UTC, then
wday_offset will be off by one.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 default-configs/arm-softmmu.mak |   2 +-
 hw/timer/Makefile.objs          |   2 +-
 hw/timer/ds-rtc-i2c.c           | 466 ++++++++++++++++++++++++++++++++++++++++
 hw/timer/ds1338.c               | 248 ---------------------
 4 files changed, 468 insertions(+), 250 deletions(-)
 create mode 100644 hw/timer/ds-rtc-i2c.c
 delete mode 100644 hw/timer/ds1338.c

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index ca34cf4462..510a92c9a8 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -31,7 +31,7 @@ CONFIG_SMC91C111=y
 CONFIG_ALLWINNER_EMAC=y
 CONFIG_IMX_FEC=y
 CONFIG_FTGMAC100=y
-CONFIG_DS1338=y
+CONFIG_DSRTCI2C=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_PFLASH_CFI02=y
 CONFIG_MICRODRIVE=y
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 8c19eac3b6..290015ebec 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -3,7 +3,7 @@ common-obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o
 common-obj-$(CONFIG_ARM_V7M) += armv7m_systick.o
 common-obj-$(CONFIG_A9_GTIMER) += a9gtimer.o
 common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
-common-obj-$(CONFIG_DS1338) += ds1338.o
+common-obj-$(CONFIG_DSRTCI2C) += ds-rtc-i2c.o
 common-obj-$(CONFIG_HPET) += hpet.o
 common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
 common-obj-$(CONFIG_M48T59) += m48t59.o
diff --git a/hw/timer/ds-rtc-i2c.c b/hw/timer/ds-rtc-i2c.c
new file mode 100644
index 0000000000..ebe53bbec7
--- /dev/null
+++ b/hw/timer/ds-rtc-i2c.c
@@ -0,0 +1,466 @@
+/* Emulation of various Dallas/Maxim RTCs accessed via I2C bus
+ *
+ * Copyright (c) 2017 Michael Davidsaver
+ * Copyright (c) 2009 CodeSourcery
+ *
+ * Authors: Michael Davidsaver
+ *          Paul Brook
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the LICENSE file in the top-level directory.
+ *
+ * Models real time read/set and NVRAM.
+ * Does not model alarms, or control/status registers.
+ *
+ * Generalized register map is:
+ *   [Current time]
+ *   [Alarm settings] (optional)
+ *   [Control/Status] (optional)
+ *   [Non-volatile memory] (optional)
+ *
+ * The current time registers are almost always the same,
+ * with the exception being that some have a CENTURY bit
+ * in the month register.
+ */
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+#include "qemu/bcd.h"
+#include "hw/hw.h"
+#include "hw/registerfields.h"
+#include "hw/i2c/i2c.h"
+#include "sysemu/qtest.h"
+#include "qemu/error-report.h"
+
+/* #define DEBUG_DSRTC */
+
+#ifdef DEBUG_DSRTC
+#define DPRINTK(FMT, ...) info_report(TYPE_DSRTC " : " FMT, ## __VA_ARGS__)
+#else
+#define DPRINTK(FMT, ...) do {} while (0)
+#endif
+
+#define LOG(MSK, FMT, ...) qemu_log_mask(MSK, TYPE_DSRTC " : " FMT "\n", \
+                            ## __VA_ARGS__)
+
+#define DSRTC_REGSIZE (0x40)
+
+/* values stored in BCD */
+/* 00-59 */
+#define R_SEC   (0x0)
+/* 00-59 */
+#define R_MIN   (0x1)
+#define R_HOUR  (0x2)
+/* 1-7 */
+#define R_WDAY  (0x3)
+/* 0-31 */
+#define R_DATE  (0x4)
+#define R_MONTH (0x5)
+/* 0-99 */
+#define R_YEAR  (0x6)
+
+/* use 12 hour mode when set */
+FIELD(HOUR, SET12, 6, 1)
+/* 00-23 */
+FIELD(HOUR, HOUR24, 0, 6)
+FIELD(HOUR, AMPM, 5, 1)
+/* 1-12 (not 0-11!) */
+FIELD(HOUR, HOUR12, 0, 5)
+
+/* 1-12 */
+FIELD(MONTH, MONTH, 0, 5)
+FIELD(MONTH, CENTURY, 7, 1)
+
+typedef struct DSRTCInfo {
+    /* if bit 7 of the Month register is set after Y2K */
+    bool has_century;
+    /* address of first non-volatile memory cell.
+     * nv_start >= reg_end means no NV memory.
+     */
+    uint8_t nv_start;
+    /* total size of register range.  When address counter rolls over. */
+    uint8_t reg_size;
+} DSRTCInfo;
+
+typedef struct DSRTCState {
+    I2CSlave parent_obj;
+
+    const DSRTCInfo *info;
+
+    qemu_irq alarm_irq;
+
+    /* register address counter */
+    uint8_t addr;
+    /* when writing, whether the address has been sent */
+    bool addrd;
+
+    int64_t time_offset;
+    int8_t wday_offset;
+
+    uint8_t regs[DSRTC_REGSIZE];
+} DSRTCState;
+
+typedef struct DSRTCClass {
+    I2CSlaveClass parent_class;
+
+    const DSRTCInfo *info;
+} DSRTCClass;
+
+#define TYPE_DSRTC "ds-rtc-i2c"
+#define DSRTC(obj) OBJECT_CHECK(DSRTCState, (obj), TYPE_DSRTC)
+#define DSRTC_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(DSRTCClass, obj, TYPE_DSRTC)
+#define DSRTC_CLASS(klass) \
+    OBJECT_CLASS_CHECK(DSRTCClass, klass, TYPE_DSRTC)
+
+static const VMStateDescription vmstate_dsrtc = {
+    .name = TYPE_DSRTC,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_I2C_SLAVE(parent_obj, DSRTCState),
+        VMSTATE_INT64(time_offset, DSRTCState),
+        VMSTATE_INT8_V(wday_offset, DSRTCState, 2),
+        VMSTATE_UINT8_ARRAY(regs, DSRTCState, DSRTC_REGSIZE),
+        VMSTATE_UINT8(addr, DSRTCState),
+        VMSTATE_BOOL(addrd, DSRTCState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void dsrtc_reset(DeviceState *device);
+
+/* update current time registers */
+static
+void dsrtc_latch(DSRTCState *ds)
+{
+    struct tm now;
+    bool use12;
+
+    qemu_get_timedate(&now, ds->time_offset);
+
+    DPRINTK("Current Time %3u/%02u/%02u %02u:%02u:%02u (wday %u)",
+            1900u + now.tm_year, now.tm_mon, now.tm_mday,
+            now.tm_hour, now.tm_min, now.tm_sec,
+            now.tm_wday);
+
+    use12 = ARRAY_FIELD_EX32(ds->regs, HOUR, SET12);
+
+    /* ensure unused bits are zero */
+    memset(ds->regs, 0, R_YEAR + 1);
+
+    ds->regs[R_SEC] = to_bcd(now.tm_sec);
+    ds->regs[R_MIN] = to_bcd(now.tm_min);
+
+    if (!use12) {
+        /* 24 hour (0-23) */
+        ARRAY_FIELD_DP32(ds->regs, HOUR, HOUR24, to_bcd(now.tm_hour));
+    } else {
+        /* 12 hour am/pm (1-12) */
+        ARRAY_FIELD_DP32(ds->regs, HOUR, SET12, 1);
+        ARRAY_FIELD_DP32(ds->regs, HOUR, AMPM, now.tm_hour >= 12u);
+        now.tm_hour %= 12u; /* wrap 0-23 to 0-11 */
+        if (now.tm_hour == 0u) {
+            /* midnight/noon stored as 12 */
+            now.tm_hour = 12u;
+        }
+        ARRAY_FIELD_DP32(ds->regs, HOUR, HOUR12, to_bcd(now.tm_hour));
+    }
+
+    ds->regs[R_WDAY] = (now.tm_wday + ds->wday_offset) % 7u + 1u;
+    ds->regs[R_DATE] = to_bcd(now.tm_mday);
+
+    ARRAY_FIELD_DP32(ds->regs, MONTH, MONTH, to_bcd(now.tm_mon + 1));
+    if (ds->info->has_century) {
+        ARRAY_FIELD_DP32(ds->regs, MONTH, CENTURY, now.tm_year >= 100u);
+    }
+
+    ds->regs[R_YEAR] = to_bcd(now.tm_year % 100u);
+
+    DPRINTK("Latched time");
+}
+
+/* call after guest writes to current time registers
+ * to re-compute our offset from host time.
+ */
+static
+void dsrtc_update(DSRTCState *ds)
+{
+    int user_wday;
+    struct tm now;
+
+    now.tm_sec = from_bcd(ds->regs[R_SEC]);
+    now.tm_min = from_bcd(ds->regs[R_MIN]);
+
+    if (ARRAY_FIELD_EX32(ds->regs, HOUR, SET12)) {
+        /* 12 hour (1-12) */
+        /* read and wrap 1-12 -> 0-11 */
+        now.tm_hour = from_bcd(ARRAY_FIELD_EX32(ds->regs, HOUR, HOUR12)) % 12u;
+        if (ARRAY_FIELD_EX32(ds->regs, HOUR, AMPM)) {
+            now.tm_hour += 12;
+        }
+
+    } else {
+        /* 23 hour (0-23) */
+        now.tm_hour = from_bcd(ARRAY_FIELD_EX32(ds->regs, HOUR, HOUR24));
+    }
+
+    now.tm_wday = from_bcd(ds->regs[R_WDAY]) - 1u;
+    now.tm_mday = from_bcd(ds->regs[R_DATE]);
+    now.tm_mon = from_bcd(ARRAY_FIELD_EX32(ds->regs, MONTH, MONTH)) - 1;
+
+    now.tm_year = from_bcd(ds->regs[R_YEAR]);
+    if (ARRAY_FIELD_EX32(ds->regs, MONTH, CENTURY) || !ds->info->has_century) {
+        now.tm_year += 100;
+    }
+
+    DPRINTK("New Time %3u/%02u/%02u %02u:%02u:%02u (wday %u)",
+            1900u + now.tm_year, now.tm_mon, now.tm_mday,
+            now.tm_hour, now.tm_min, now.tm_sec,
+            now.tm_wday);
+
+    /* round trip to get real wday_offset based on time delta */
+    user_wday = now.tm_wday;
+    ds->time_offset = qemu_timedate_diff(&now);
+    /* race possible if we run at midnight
+     * TODO: make qemu_timedate_diff() calculate wday offset as well?
+     */
+    qemu_get_timedate(&now, ds->time_offset);
+    /* calculate wday_offset to achieve guest requested wday */
+    ds->wday_offset = user_wday - now.tm_wday;
+
+    DPRINTK("Update offset = %" PRId64 ", wday_offset = %d",
+            ds->time_offset, ds->wday_offset);
+}
+
+static
+void dsrtc_advance(DSRTCState *ds)
+{
+    ds->addr = (ds->addr + 1) % ds->info->reg_size;
+    if (ds->addr == 0) {
+        /* latch time on roll over */
+        dsrtc_latch(ds);
+    }
+}
+
+static
+int dsrtc_event(I2CSlave *s, enum i2c_event event)
+{
+    DSRTCState *ds = DSRTC(s);
+
+    switch (event) {
+    case I2C_START_SEND:
+        ds->addrd = false;
+        /* fall through */
+    case I2C_START_RECV:
+        dsrtc_latch(ds);
+        /* fall through */
+    case I2C_FINISH:
+        DPRINTK("Event %d", (int)event);
+        /* fall through */
+    case I2C_NACK:
+        break;
+    }
+    return 0;
+}
+
+static
+int dsrtc_recv(I2CSlave *s)
+{
+    DSRTCState *ds = DSRTC(s);
+    int ret = 0;
+
+    ret = ds->regs[ds->addr];
+
+    if (ds->addr > R_YEAR && ds->addr < ds->info->nv_start) {
+        LOG(LOG_UNIMP, "Read from unimplemented (%02x) %02x", ds->addr, ret);
+    }
+
+    DPRINTK("Recv (%02x) %02x", ds->addr, ret);
+
+    dsrtc_advance(ds);
+
+    return ret;
+}
+
+static
+int dsrtc_send(I2CSlave *s, uint8_t data)
+{
+    DSRTCState *ds = DSRTC(s);
+
+    if (!ds->addrd) {
+        if (data == 0xff && qtest_enabled()) {
+            /* allow test runner to zero offsets */
+            DPRINTK("Testing reset");
+            dsrtc_reset(DEVICE(s));
+            return 0;
+        }
+        ds->addr = data % DSRTC_REGSIZE;
+        ds->addrd = true;
+        DPRINTK("Set address pointer %02x", data);
+        return 0;
+    }
+
+    DPRINTK("Send (%02x) %02x", ds->addr, data);
+
+    if (ds->addr <= R_YEAR) {
+        ds->regs[ds->addr] = data;
+        dsrtc_update(ds);
+
+    } else if (ds->addr >= ds->info->nv_start) {
+        ds->regs[ds->addr] = data;
+
+    } else {
+        LOG(LOG_UNIMP, "Register not modeled");
+    }
+
+    dsrtc_advance(ds);
+
+    return 0;
+}
+
+static
+void dsrtc_reset(DeviceState *device)
+{
+    DSRTCState *ds = DSRTC(device);
+
+    memset(ds->regs, 0, sizeof(ds->regs));
+
+    ds->addr = 0;
+    ds->addrd = false;
+    ds->time_offset = 0;
+    ds->wday_offset = 0;
+
+    DPRINTK("Reset");
+}
+
+static
+void dsrtc_realize(DeviceState *device, Error **errp)
+{
+    DSRTCState *ds = DSRTC(device);
+    DSRTCClass *r = DSRTC_GET_CLASS(device);
+
+    ds->info = r->info;
+
+    /* Alarms not yet implemented, but allow
+     * board code to wire up the alarm interrupt
+     * output anyway.
+     */
+    qdev_init_gpio_out(device, &ds->alarm_irq, 1);
+}
+
+static
+void dsrtc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+    DSRTCClass *r = DSRTC_CLASS(klass);
+
+    r->info = data;
+
+    k->event = &dsrtc_event;
+    k->recv = &dsrtc_recv;
+    k->send = &dsrtc_send;
+
+    dc->vmsd = &vmstate_dsrtc;
+    dc->realize = dsrtc_realize;
+    dc->reset = dsrtc_reset;
+    dc->user_creatable = true;
+}
+
+static
+const TypeInfo ds_rtc_base_type = {
+    .abstract = true,
+    .name = TYPE_DSRTC,
+    .parent = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(DSRTCState),
+};
+
+#define DSRTC_CONFIG(NAME) \
+static const TypeInfo NAME##_type = { \
+    .name = #NAME, \
+    .parent = TYPE_DSRTC, \
+    .class_size = sizeof(I2CSlaveClass), \
+    .class_init = dsrtc_class_init, \
+    .class_data = (void *)&NAME##_info, \
+};
+
+/* ds3231 - alarms, no eeprom */
+static const DSRTCInfo ds3231_info = {
+    .has_century = true,
+    .nv_start    = 0x13, /* no nv memory */
+    .reg_size    = 0x13,
+};
+DSRTC_CONFIG(ds3231)
+
+/* only model block 0 (RTC), blocks 1,2 (eeprom) not modeled.
+ * blocks have different i2c addresses
+ */
+static const DSRTCInfo ds1388_info = {
+    .has_century = false,
+    .nv_start    = 0x0d,
+    .reg_size    = 0x0d,
+};
+DSRTC_CONFIG(ds1388)
+
+/* alarms, eeprom */
+static const DSRTCInfo ds1375_info = {
+    .has_century = true,
+    .nv_start    = 0x10,
+    .reg_size    = 0x20,
+};
+DSRTC_CONFIG(ds1375)
+
+/* no alarms, no eeprom */
+static const DSRTCInfo ds1340_info = {
+    .has_century = false,
+    .nv_start    = 0x10,
+    .reg_size    = 0x10,
+};
+DSRTC_CONFIG(ds1340)
+
+/* alarms, no eeprom */
+static const DSRTCInfo ds1339_info = {
+    .has_century = false,
+    .nv_start    = 0x11,
+    .reg_size    = 0x11,
+};
+DSRTC_CONFIG(ds1339)
+
+/* no alarms, eeprom */
+static const DSRTCInfo ds1338_info = {
+    .has_century = false,
+    .nv_start    = 0x08,
+    .reg_size    = 0x40,
+};
+DSRTC_CONFIG(ds1338)
+
+/* alarms, no eeprom */
+static const DSRTCInfo ds1337_info = {
+    .has_century = true,
+    .nv_start    = 0x10,
+    .reg_size    = 0x10,
+};
+DSRTC_CONFIG(ds1337)
+
+/* ds1307 registers are identical to ds1338 */
+static
+const TypeInfo ds1307_type = {
+    .name = "ds1307",
+    .parent = "ds1338",
+};
+
+static void ds_rtc_i2c_register(void)
+{
+    type_register_static(&ds_rtc_base_type);
+    type_register_static(&ds3231_type);
+    type_register_static(&ds1388_type);
+    type_register_static(&ds1375_type);
+    type_register_static(&ds1340_type);
+    type_register_static(&ds1339_type);
+    type_register_static(&ds1338_type);
+    type_register_static(&ds1337_type);
+    type_register_static(&ds1307_type);
+}
+
+type_init(ds_rtc_i2c_register)
diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
deleted file mode 100644
index 41c2d7dac6..0000000000
--- a/hw/timer/ds1338.c
+++ /dev/null
@@ -1,248 +0,0 @@
-/*
- * MAXIM DS1338 I2C RTC+NVRAM
- *
- * Copyright (c) 2009 CodeSourcery.
- * Written by Paul Brook
- *
- * This code is licensed under the GNU GPL v2.
- *
- * Contributions after 2012-01-13 are licensed under the terms of the
- * GNU GPL, version 2 or (at your option) any later version.
- */
-
-#include "qemu/osdep.h"
-#include "qemu-common.h"
-#include "hw/i2c/i2c.h"
-#include "qemu/bcd.h"
-#include "sysemu/qtest.h"
-
-/* Size of NVRAM including both the user-accessible area and the
- * secondary register area.
- */
-#define NVRAM_SIZE 64
-
-/* Flags definitions */
-#define SECONDS_CH 0x80
-#define HOURS_12   0x40
-#define HOURS_PM   0x20
-#define CTRL_OSF   0x20
-
-#define TYPE_DS1338 "ds1338"
-#define DS1338(obj) OBJECT_CHECK(DS1338State, (obj), TYPE_DS1338)
-
-typedef struct DS1338State {
-    I2CSlave parent_obj;
-
-    int64_t offset;
-    uint8_t wday_offset;
-    uint8_t nvram[NVRAM_SIZE];
-    int32_t ptr;
-    bool addr_byte;
-} DS1338State;
-
-static const VMStateDescription vmstate_ds1338 = {
-    .name = "ds1338",
-    .version_id = 2,
-    .minimum_version_id = 1,
-    .fields = (VMStateField[]) {
-        VMSTATE_I2C_SLAVE(parent_obj, DS1338State),
-        VMSTATE_INT64(offset, DS1338State),
-        VMSTATE_UINT8_V(wday_offset, DS1338State, 2),
-        VMSTATE_UINT8_ARRAY(nvram, DS1338State, NVRAM_SIZE),
-        VMSTATE_INT32(ptr, DS1338State),
-        VMSTATE_BOOL(addr_byte, DS1338State),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
-static void capture_current_time(DS1338State *s)
-{
-    /* Capture the current time into the secondary registers
-     * which will be actually read by the data transfer operation.
-     */
-    struct tm now;
-    qemu_get_timedate(&now, s->offset);
-    s->nvram[0] = to_bcd(now.tm_sec);
-    s->nvram[1] = to_bcd(now.tm_min);
-    if (s->nvram[2] & HOURS_12) {
-        int tmp = now.tm_hour;
-        if (tmp % 12 == 0) {
-            tmp += 12;
-        }
-        if (tmp <= 12) {
-            s->nvram[2] = HOURS_12 | to_bcd(tmp);
-        } else {
-            s->nvram[2] = HOURS_12 | HOURS_PM | to_bcd(tmp - 12);
-        }
-    } else {
-        s->nvram[2] = to_bcd(now.tm_hour);
-    }
-    s->nvram[3] = (now.tm_wday + s->wday_offset) % 7 + 1;
-    s->nvram[4] = to_bcd(now.tm_mday);
-    s->nvram[5] = to_bcd(now.tm_mon + 1);
-    s->nvram[6] = to_bcd(now.tm_year - 100);
-}
-
-static void inc_regptr(DS1338State *s)
-{
-    /* The register pointer wraps around after 0x3F; wraparound
-     * causes the current time/date to be retransferred into
-     * the secondary registers.
-     */
-    s->ptr = (s->ptr + 1) & (NVRAM_SIZE - 1);
-    if (!s->ptr) {
-        capture_current_time(s);
-    }
-}
-
-static int ds1338_event(I2CSlave *i2c, enum i2c_event event)
-{
-    DS1338State *s = DS1338(i2c);
-
-    switch (event) {
-    case I2C_START_RECV:
-        /* In h/w, capture happens on any START condition, not just a
-         * START_RECV, but there is no need to actually capture on
-         * START_SEND, because the guest can't get at that data
-         * without going through a START_RECV which would overwrite it.
-         */
-        capture_current_time(s);
-        break;
-    case I2C_START_SEND:
-        s->addr_byte = true;
-        break;
-    default:
-        break;
-    }
-
-    return 0;
-}
-
-static int ds1338_recv(I2CSlave *i2c)
-{
-    DS1338State *s = DS1338(i2c);
-    uint8_t res;
-
-    res  = s->nvram[s->ptr];
-    inc_regptr(s);
-    return res;
-}
-
-static int ds1338_send(I2CSlave *i2c, uint8_t data)
-{
-    DS1338State *s = DS1338(i2c);
-
-    if (s->addr_byte) {
-        if (data == 0xff && qtest_enabled()) {
-            /* magic, out of bounds, address to allow test code
-             * to reset offset
-             */
-            s->offset = 0;
-            s->wday_offset = 0;
-            return 0;
-        }
-        s->ptr = data & (NVRAM_SIZE - 1);
-        s->addr_byte = false;
-        return 0;
-    }
-    if (s->ptr < 7) {
-        /* Time register. */
-        struct tm now;
-        qemu_get_timedate(&now, s->offset);
-        switch(s->ptr) {
-        case 0:
-            /* TODO: Implement CH (stop) bit.  */
-            now.tm_sec = from_bcd(data & 0x7f);
-            break;
-        case 1:
-            now.tm_min = from_bcd(data & 0x7f);
-            break;
-        case 2:
-            if (data & HOURS_12) {
-                int tmp = from_bcd(data & (HOURS_PM - 1));
-                if (data & HOURS_PM) {
-                    tmp += 12;
-                }
-                if (tmp % 12 == 0) {
-                    tmp -= 12;
-                }
-                now.tm_hour = tmp;
-            } else {
-                now.tm_hour = from_bcd(data & (HOURS_12 - 1));
-            }
-            break;
-        case 3:
-            {
-                /* The day field is supposed to contain a value in
-                   the range 1-7. Otherwise behavior is undefined.
-                 */
-                int user_wday = (data & 7) - 1;
-                s->wday_offset = (user_wday - now.tm_wday + 7) % 7;
-            }
-            break;
-        case 4:
-            now.tm_mday = from_bcd(data & 0x3f);
-            break;
-        case 5:
-            now.tm_mon = from_bcd(data & 0x1f) - 1;
-            break;
-        case 6:
-            now.tm_year = from_bcd(data) + 100;
-            break;
-        }
-        s->offset = qemu_timedate_diff(&now);
-    } else if (s->ptr == 7) {
-        /* Control register. */
-
-        /* Ensure bits 2, 3 and 6 will read back as zero. */
-        data &= 0xB3;
-
-        /* Attempting to write the OSF flag to logic 1 leaves the
-           value unchanged. */
-        data = (data & ~CTRL_OSF) | (data & s->nvram[s->ptr] & CTRL_OSF);
-
-        s->nvram[s->ptr] = data;
-    } else {
-        s->nvram[s->ptr] = data;
-    }
-    inc_regptr(s);
-    return 0;
-}
-
-static void ds1338_reset(DeviceState *dev)
-{
-    DS1338State *s = DS1338(dev);
-
-    /* The clock is running and synchronized with the host */
-    s->offset = 0;
-    s->wday_offset = 0;
-    memset(s->nvram, 0, NVRAM_SIZE);
-    s->ptr = 0;
-    s->addr_byte = false;
-}
-
-static void ds1338_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
-
-    k->event = ds1338_event;
-    k->recv = ds1338_recv;
-    k->send = ds1338_send;
-    dc->reset = ds1338_reset;
-    dc->vmsd = &vmstate_ds1338;
-}
-
-static const TypeInfo ds1338_info = {
-    .name          = TYPE_DS1338,
-    .parent        = TYPE_I2C_SLAVE,
-    .instance_size = sizeof(DS1338State),
-    .class_init    = ds1338_class_init,
-};
-
-static void ds1338_register_types(void)
-{
-    type_register_static(&ds1338_info);
-}
-
-type_init(ds1338_register_types)
-- 
2.11.0

[Qemu-devel] [PATCH 4/5] tests: ds-rtc-i2c-test test 12 hour mode and DoW

[Michael Davidsaver]

Test time set+get in 12 hour mode.
Also test handling of day of week
offset.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 tests/ds-rtc-i2c-test.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/tests/ds-rtc-i2c-test.c b/tests/ds-rtc-i2c-test.c
index 464eb08558..226ac1399e 100644
--- a/tests/ds-rtc-i2c-test.c
+++ b/tests/ds-rtc-i2c-test.c
@@ -92,6 +92,18 @@ static uint8_t test_time_24_12am[8] = {
     0x17,
 };
 
+static uint8_t test_time_12_12am[8] = {
+    0, /* address */
+    /* Wed, 22 Nov 2017 00:30:53 +0000 */
+    0x53,
+    0x30,
+    0x52, /* 12 AM in 12 hour mode */
+    0x03, /* monday is our day 1 */
+    0x22,
+    0x11 | 0x80,
+    0x17,
+};
+
 static uint8_t test_time_24_6am[8] = {
     0, /* address */
     /* Wed, 22 Nov 2017 06:30:53 +0000 */
@@ -104,6 +116,18 @@ static uint8_t test_time_24_6am[8] = {
     0x17,
 };
 
+static uint8_t test_time_12_6am[8] = {
+    0, /* address */
+    /* Wed, 22 Nov 2017 06:30:53 +0000 */
+    0x53,
+    0x30,
+    0x46, /* 6 AM in 12 hour mode */
+    0x03, /* monday is our day 1 */
+    0x22,
+    0x11 | 0x80,
+    0x17,
+};
+
 static uint8_t test_time_24_12pm[8] = {
     0, /* address */
     /* Wed, 22 Nov 2017 12:30:53 +0000 */
@@ -116,6 +140,18 @@ static uint8_t test_time_24_12pm[8] = {
     0x17,
 };
 
+static uint8_t test_time_12_12pm[8] = {
+    0, /* address */
+    /* Wed, 22 Nov 2017 12:30:53 +0000 */
+    0x53,
+    0x30,
+    0x72, /* 12 PM in 24 hour mode */
+    0x03, /* monday is our day 1 */
+    0x22,
+    0x11 | 0x80,
+    0x17,
+};
+
 static uint8_t test_time_24_6pm[8] = {
     0, /* address */
     /* Wed, 22 Nov 2017 18:30:53 +0000 */
@@ -128,6 +164,18 @@ static uint8_t test_time_24_6pm[8] = {
     0x17,
 };
 
+static uint8_t test_time_12_6pm[8] = {
+    0, /* address */
+    /* Wed, 22 Nov 2017 18:30:53 +0000 */
+    0x53,
+    0x30,
+    0x66, /* 6 PM in 12 hour mode */
+    0x03, /* monday is our day 1 */
+    0x22,
+    0x11 | 0x80,
+    0x17,
+};
+
 /* write in and read back known time */
 static
 void test_rtc_set(const void *raw)
@@ -151,7 +199,7 @@ void test_rtc_set(const void *raw)
     g_assert_cmpuint(testtime[1], ==, buf[0]); /* SEC */
     g_assert_cmpuint(testtime[2], ==, buf[1]); /* MIN */
     g_assert_cmpuint(testtime[3], ==, buf[2]); /* HOUR */
-    /* skip comparing Day of Week.  Not handled correctly */
+    g_assert_cmpuint(testtime[4], ==, buf[3]); /* DoW */
     g_assert_cmpuint(testtime[5], ==, buf[4]); /* DoM */
     if (use_century) {
         g_assert_cmpuint(testtime[6], ==, buf[5]); /* MON+century */
@@ -183,6 +231,10 @@ int main(int argc, char *argv[])
     qtest_add_data_func("/ds-rtc-i2c/set24_6am", test_time_24_6am, test_rtc_set);
     qtest_add_data_func("/ds-rtc-i2c/set24_12pm", test_time_24_12pm, test_rtc_set);
     qtest_add_data_func("/ds-rtc-i2c/set24_6pm", test_time_24_6pm, test_rtc_set);
+    qtest_add_data_func("/ds-rtc-i2c/set12_12am", test_time_12_12am, test_rtc_set);
+    qtest_add_data_func("/ds-rtc-i2c/set12_6am", test_time_12_6am, test_rtc_set);
+    qtest_add_data_func("/ds-rtc-i2c/set12_12pm", test_time_12_12pm, test_rtc_set);
+    qtest_add_data_func("/ds-rtc-i2c/set12_6pm", test_time_12_6pm, test_rtc_set);
     qtest_add_func("/ds-rtc-i2c/current", test_rtc_current);
 
     ret = g_test_run();
-- 
2.11.0

[Qemu-devel] [PATCH 5/5] tests: drop ds1338-test

[Michael Davidsaver]

Now redundant to ds-rtc-i2c-test.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 tests/Makefile.include |  2 --
 tests/ds1338-test.c    | 75 --------------------------------------------------
 2 files changed, 77 deletions(-)
 delete mode 100644 tests/ds1338-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index f5dcd274e0..8b1e486e32 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -359,7 +359,6 @@ check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
 check-qtest-sparc64-y += tests/boot-serial-test$(EXESUF)
 
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
-check-qtest-arm-y += tests/ds1338-test$(EXESUF)
 check-qtest-arm-y += tests/ds-rtc-i2c-test$(EXESUF)
 check-qtest-arm-y += tests/m25p80-test$(EXESUF)
 gcov-files-arm-y += hw/misc/tmp105.c
@@ -764,7 +763,6 @@ tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
 	tests/boot-sector.o tests/acpi-utils.o $(libqos-obj-y)
 tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
 tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
-tests/ds1338-test$(EXESUF): tests/ds1338-test.o $(libqos-imx-obj-y)
 tests/ds-rtc-i2c-test$(EXESUF): tests/ds-rtc-i2c-test.o $(libqos-imx-obj-y)
 tests/m25p80-test$(EXESUF): tests/m25p80-test.o
 tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
diff --git a/tests/ds1338-test.c b/tests/ds1338-test.c
deleted file mode 100644
index 742dad9113..0000000000
--- a/tests/ds1338-test.c
+++ /dev/null
@@ -1,75 +0,0 @@
-/*
- * QTest testcase for the DS1338 RTC
- *
- * Copyright (c) 2013 Jean-Christophe Dubois
- *
- *  This program is free software; you can redistribute it and/or modify it
- *  under the terms of the GNU General Public License as published by the
- *  Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful, but WITHOUT
- *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- *  FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
- *  for more details.
- *
- *  You should have received a copy of the GNU General Public License along
- *  with this program; if not, see <http://www.gnu.org/licenses/>.
- */
-
-#include "qemu/osdep.h"
-#include "libqtest.h"
-#include "libqos/i2c.h"
-
-#define IMX25_I2C_0_BASE 0x43F80000
-
-#define DS1338_ADDR 0x68
-
-static I2CAdapter *i2c;
-static uint8_t addr;
-
-static inline uint8_t bcd2bin(uint8_t x)
-{
-    return ((x) & 0x0f) + ((x) >> 4) * 10;
-}
-
-static void send_and_receive(void)
-{
-    uint8_t cmd[1];
-    uint8_t resp[7];
-    time_t now = time(NULL);
-    struct tm *tm_ptr = gmtime(&now);
-
-    /* reset the index in the RTC memory */
-    cmd[0] = 0;
-    i2c_send(i2c, addr, cmd, 1);
-
-    /* retrieve the date */
-    i2c_recv(i2c, addr, resp, 7);
-
-    /* check retrieved time againt local time */
-    g_assert_cmpuint(bcd2bin(resp[4]), == , tm_ptr->tm_mday);
-    g_assert_cmpuint(bcd2bin(resp[5]), == , 1 + tm_ptr->tm_mon);
-    g_assert_cmpuint(2000 + bcd2bin(resp[6]), == , 1900 + tm_ptr->tm_year);
-}
-
-int main(int argc, char **argv)
-{
-    QTestState *s = NULL;
-    int ret;
-
-    g_test_init(&argc, &argv, NULL);
-
-    s = qtest_start("-display none -machine imx25-pdk");
-    i2c = imx_i2c_create(s, IMX25_I2C_0_BASE);
-    addr = DS1338_ADDR;
-
-    qtest_add_func("/ds1338/tx-rx", send_and_receive);
-
-    ret = g_test_run();
-
-    qtest_quit(s);
-    g_free(i2c);
-
-    return ret;
-}
-- 
2.11.0

Re: [Qemu-devel] [PATCH 2/5] tests: more thorough test of ds1338

[Thomas Huth]

On 19.02.2018 05:03, Michael Davidsaver wrote:
> Test current time and set+get round trip.
> 
> The set+get test is repeated 4 times.  These cases are
> spread across a single day in an attempt to trigger some potential
> issues regardless of the timezone of the machine running the tests.
> 
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  tests/Makefile.include  |   2 +
>  tests/ds-rtc-i2c-test.c | 193 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 195 insertions(+)
>  create mode 100644 tests/ds-rtc-i2c-test.c
[...]
>  tests/q35-test$(EXESUF): tests/q35-test.o $(libqos-pc-obj-y)
> diff --git a/tests/ds-rtc-i2c-test.c b/tests/ds-rtc-i2c-test.c
> new file mode 100644
> index 0000000000..464eb08558
> --- /dev/null
> +++ b/tests/ds-rtc-i2c-test.c
> @@ -0,0 +1,193 @@
> +/* Testing of Dallas/Maxim I2C bus RTC devices
> + *
> + * Copyright (c) 2017 Michael Davidsaver
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the LICENSE file in the top-level directory.
> + */
> +#include <stdio.h>
> +
> +#include "qemu/osdep.h"
> +#include "qemu/bcd.h"
> +#include "qemu/cutils.h"
> +#include "qemu/timer.h"
> +#include "libqtest.h"
> +#include "libqos/libqos.h"
> +#include "libqos/i2c.h"
> +
> +#define IMX25_I2C_0_BASE 0x43F80000
> +#define DS1338_ADDR 0x68
> +
> +static I2CAdapter *i2c;
> +static uint8_t addr;
> +static bool use_century;
> +
> +static
> +time_t rtc_gettime(void)
> +{
> +    struct tm parts;
> +    uint8_t buf[7];
> +
> +    buf[0] = 0;
> +    i2c_send(i2c, addr, buf, 1);
> +    i2c_recv(i2c, addr, buf, 7);
> +
> +    parts.tm_sec = from_bcd(buf[0]);
> +    parts.tm_min = from_bcd(buf[1]);
> +    if (buf[2] & 0x40) {
> +        /* 12 hour */
> +        /* HOUR register is 1-12. */
> +        parts.tm_hour = from_bcd(buf[2] & 0x1f);
> +        g_assert_cmpuint(parts.tm_hour, >=, 1);
> +        g_assert_cmpuint(parts.tm_hour, <=, 12);
> +        parts.tm_hour %= 12u; /* wrap 12 -> 0 */
> +        if (buf[2] & 0x20) {
> +            parts.tm_hour += 12u;
> +        }
> +    } else {
> +        /* 24 hour */
> +        parts.tm_hour = from_bcd(buf[2] & 0x3f);
> +    }
> +    parts.tm_wday = from_bcd(buf[3]);
> +    parts.tm_mday = from_bcd(buf[4]);
> +    parts.tm_mon =  from_bcd((buf[5] & 0x1f) - 1u);
> +    parts.tm_year = from_bcd(buf[6]);
> +    if (!use_century || (buf[5] & 0x80)) {
> +        parts.tm_year += 100u;
> +    }
> +
> +    return mktimegm(&parts);
> +}
> +
> +/* read back and compare with current system time */
> +static
> +void test_rtc_current(void)
> +{
> +    uint8_t buf;
> +    time_t expected, actual;
> +
> +    /* magic address to zero RTC time offset
> +     * as tests may be run in any order
> +     */
> +    buf = 0xff;
> +    i2c_send(i2c, addr, &buf, 1);

That magic (together with patch 1/5) is IMHO a little bit ugly. I've hit
the same problem with the m48t59 test recently, and I solved it by
moving the qtest_start() and qtest_end() calls from the main() function
into the single tests instead, so that each test starts with a clean state:

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9c29830c90d82f27f

Could you maybe try whether that approach works for your test cases
here, too? Then you could do this without the "0xff" hack here...

> +
> +    actual = time(NULL);
> +    /* new second may start here */
> +    expected = rtc_gettime();
> +    g_assert_cmpuint(expected, <=, actual + 1);
> +    g_assert_cmpuint(expected, >=, actual);
> +}
> +
> +
> +static uint8_t test_time_24_12am[8] = {
> +    0, /* address */
> +    /* Wed, 22 Nov 2017 00:30:53 +0000 */
> +    0x53,
> +    0x30,
> +    0x00, /* 12 AM in 24 hour mode */
> +    0x03, /* monday is our day 1 */
> +    0x22,
> +    0x11 | 0x80,
> +    0x17,
> +};
> +
> +static uint8_t test_time_24_6am[8] = {
> +    0, /* address */
> +    /* Wed, 22 Nov 2017 06:30:53 +0000 */
> +    0x53,
> +    0x30,
> +    0x06, /* 6 AM in 24 hour mode */
> +    0x03, /* monday is our day 1 */
> +    0x22,
> +    0x11 | 0x80,
> +    0x17,
> +};
> +
> +static uint8_t test_time_24_12pm[8] = {
> +    0, /* address */
> +    /* Wed, 22 Nov 2017 12:30:53 +0000 */
> +    0x53,
> +    0x30,
> +    0x12, /* 12 PM in 24 hour mode */
> +    0x03, /* monday is our day 1 */
> +    0x22,
> +    0x11 | 0x80,
> +    0x17,
> +};
> +
> +static uint8_t test_time_24_6pm[8] = {
> +    0, /* address */
> +    /* Wed, 22 Nov 2017 18:30:53 +0000 */
> +    0x53,
> +    0x30,
> +    0x18, /* 6 PM in 24 hour mode */
> +    0x03, /* monday is our day 1 */
> +    0x22,
> +    0x11 | 0x80,
> +    0x17,
> +};
> +
> +/* write in and read back known time */
> +static
> +void test_rtc_set(const void *raw)
> +{
> +    const uint8_t *testtime = raw;
> +    uint8_t buf[7];
> +    unsigned retry = 2;
> +
> +    for (; retry; retry--) {
> +        i2c_send(i2c, addr, testtime, 8);
> +        /* new second may start here */
> +        i2c_send(i2c, addr, testtime, 1);
> +        i2c_recv(i2c, addr, buf, 7);
> +
> +        if (testtime[1] == buf[0]) {

Please also check the minutes here (reason: see below).

> +            break;
> +        }
> +        /* we raced start of second, retry */
> +    };
> +
> +    g_assert_cmpuint(testtime[1], ==, buf[0]); /* SEC */
> +    g_assert_cmpuint(testtime[2], ==, buf[1]); /* MIN */

Could you please wrap the SEC and MIN lines in a "if (!g_test_slow()) {
... }" statement? The problem is: The "make check" tests are run as CI
on a system that is sometimes *very* overloaded. It might happen that
the test is sometimes interrupted for dozens of seconds, so it might
fail to properly read the time on a granularity of seconds. With
!g_test_slow() you can make sure that the check is not done on such
overloaded CI systems.

> +    g_assert_cmpuint(testtime[3], ==, buf[2]); /* HOUR */
> +    /* skip comparing Day of Week.  Not handled correctly */
> +    g_assert_cmpuint(testtime[5], ==, buf[4]); /* DoM */
> +    if (use_century) {
> +        g_assert_cmpuint(testtime[6], ==, buf[5]); /* MON+century */
> +    } else {
> +        g_assert_cmpuint(testtime[6] & 0x7f, ==, buf[5]); /* MON */
> +    }
> +    g_assert_cmpuint(testtime[7], ==, buf[6]); /* YEAR */
> +
> +    g_assert_cmpuint(retry, >, 0);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +    int ret;
> +    const char *arch = qtest_get_arch();
> +    QTestState *s = NULL;
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    if (strcmp(arch, "arm") == 0) {
> +        s = qtest_start("-display none -machine imx25-pdk");

Do you really need the "-display none" parameter here? ... I thought
that was the default for qtests anyway?

> +        i2c = imx_i2c_create(s, IMX25_I2C_0_BASE);
> +        addr = DS1338_ADDR;
> +        use_century = false;
> +
> +    }
> +
> +    qtest_add_data_func("/ds-rtc-i2c/set24_12am", test_time_24_12am, test_rtc_set);
> +    qtest_add_data_func("/ds-rtc-i2c/set24_6am", test_time_24_6am, test_rtc_set);
> +    qtest_add_data_func("/ds-rtc-i2c/set24_12pm", test_time_24_12pm, test_rtc_set);
> +    qtest_add_data_func("/ds-rtc-i2c/set24_6pm", test_time_24_6pm, test_rtc_set);
> +    qtest_add_func("/ds-rtc-i2c/current", test_rtc_current);
> +
> +    ret = g_test_run();
> +
> +    qtest_end();
> +
> +    return ret;
> +}
> 

 Thomas

Re: [Qemu-devel] [PATCH 2/5] tests: more thorough test of ds1338

[Michael Davidsaver]

On 02/18/2018 11:39 PM, Thomas Huth wrote:
> On 19.02.2018 05:03, Michael Davidsaver wrote:
>> Test current time and set+get round trip.
>>
>> The set+get test is repeated 4 times.  These cases are
>> spread across a single day in an attempt to trigger some potential
>> issues regardless of the timezone of the machine running the tests.
>>
>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>> ---
>>  tests/Makefile.include  |   2 +
>>  tests/ds-rtc-i2c-test.c | 193 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 195 insertions(+)
>>  create mode 100644 tests/ds-rtc-i2c-test.c
> [...]
>>  tests/q35-test$(EXESUF): tests/q35-test.o $(libqos-pc-obj-y)
>> diff --git a/tests/ds-rtc-i2c-test.c b/tests/ds-rtc-i2c-test.c
>> new file mode 100644
>> index 0000000000..464eb08558
>> --- /dev/null
>> +++ b/tests/ds-rtc-i2c-test.c
>> @@ -0,0 +1,193 @@
>> +/* Testing of Dallas/Maxim I2C bus RTC devices
>> + *
>> + * Copyright (c) 2017 Michael Davidsaver
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the LICENSE file in the top-level directory.
>> + */
>> +#include <stdio.h>
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/bcd.h"
>> +#include "qemu/cutils.h"
>> +#include "qemu/timer.h"
>> +#include "libqtest.h"
>> +#include "libqos/libqos.h"
>> +#include "libqos/i2c.h"
>> +
>> +#define IMX25_I2C_0_BASE 0x43F80000
>> +#define DS1338_ADDR 0x68
>> +
>> +static I2CAdapter *i2c;
>> +static uint8_t addr;
>> +static bool use_century;
>> +
>> +static
>> +time_t rtc_gettime(void)
>> +{
>> +    struct tm parts;
>> +    uint8_t buf[7];
>> +
>> +    buf[0] = 0;
>> +    i2c_send(i2c, addr, buf, 1);
>> +    i2c_recv(i2c, addr, buf, 7);
>> +
>> +    parts.tm_sec = from_bcd(buf[0]);
>> +    parts.tm_min = from_bcd(buf[1]);
>> +    if (buf[2] & 0x40) {
>> +        /* 12 hour */
>> +        /* HOUR register is 1-12. */
>> +        parts.tm_hour = from_bcd(buf[2] & 0x1f);
>> +        g_assert_cmpuint(parts.tm_hour, >=, 1);
>> +        g_assert_cmpuint(parts.tm_hour, <=, 12);
>> +        parts.tm_hour %= 12u; /* wrap 12 -> 0 */
>> +        if (buf[2] & 0x20) {
>> +            parts.tm_hour += 12u;
>> +        }
>> +    } else {
>> +        /* 24 hour */
>> +        parts.tm_hour = from_bcd(buf[2] & 0x3f);
>> +    }
>> +    parts.tm_wday = from_bcd(buf[3]);
>> +    parts.tm_mday = from_bcd(buf[4]);
>> +    parts.tm_mon =  from_bcd((buf[5] & 0x1f) - 1u);
>> +    parts.tm_year = from_bcd(buf[6]);
>> +    if (!use_century || (buf[5] & 0x80)) {
>> +        parts.tm_year += 100u;
>> +    }
>> +
>> +    return mktimegm(&parts);
>> +}
>> +
>> +/* read back and compare with current system time */
>> +static
>> +void test_rtc_current(void)
>> +{
>> +    uint8_t buf;
>> +    time_t expected, actual;
>> +
>> +    /* magic address to zero RTC time offset
>> +     * as tests may be run in any order
>> +     */
>> +    buf = 0xff;
>> +    i2c_send(i2c, addr, &buf, 1);
> 
> That magic (together with patch 1/5) is IMHO a little bit ugly. I've hit
> the same problem with the m48t59 test recently, and I solved it by
> moving the qtest_start() and qtest_end() calls from the main() function
> into the single tests instead, so that each test starts with a clean state:
> 
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9c29830c90d82f27f
> 
> Could you maybe try whether that approach works for your test cases
> here, too? Then you could do this without the "0xff" hack here...

Your right, this looks clearer.  I'll try this approach.

>> +
>> +    actual = time(NULL);
>> +    /* new second may start here */
>> +    expected = rtc_gettime();
>> +    g_assert_cmpuint(expected, <=, actual + 1);
>> +    g_assert_cmpuint(expected, >=, actual);
>> +}
>> +
>> +
>> +static uint8_t test_time_24_12am[8] = {
>> +    0, /* address */
>> +    /* Wed, 22 Nov 2017 00:30:53 +0000 */
>> +    0x53,
>> +    0x30,
>> +    0x00, /* 12 AM in 24 hour mode */
>> +    0x03, /* monday is our day 1 */
>> +    0x22,
>> +    0x11 | 0x80,
>> +    0x17,
>> +};
>> +
>> +static uint8_t test_time_24_6am[8] = {
>> +    0, /* address */
>> +    /* Wed, 22 Nov 2017 06:30:53 +0000 */
>> +    0x53,
>> +    0x30,
>> +    0x06, /* 6 AM in 24 hour mode */
>> +    0x03, /* monday is our day 1 */
>> +    0x22,
>> +    0x11 | 0x80,
>> +    0x17,
>> +};
>> +
>> +static uint8_t test_time_24_12pm[8] = {
>> +    0, /* address */
>> +    /* Wed, 22 Nov 2017 12:30:53 +0000 */
>> +    0x53,
>> +    0x30,
>> +    0x12, /* 12 PM in 24 hour mode */
>> +    0x03, /* monday is our day 1 */
>> +    0x22,
>> +    0x11 | 0x80,
>> +    0x17,
>> +};
>> +
>> +static uint8_t test_time_24_6pm[8] = {
>> +    0, /* address */
>> +    /* Wed, 22 Nov 2017 18:30:53 +0000 */
>> +    0x53,
>> +    0x30,
>> +    0x18, /* 6 PM in 24 hour mode */
>> +    0x03, /* monday is our day 1 */
>> +    0x22,
>> +    0x11 | 0x80,
>> +    0x17,
>> +};
>> +
>> +/* write in and read back known time */
>> +static
>> +void test_rtc_set(const void *raw)
>> +{
>> +    const uint8_t *testtime = raw;
>> +    uint8_t buf[7];
>> +    unsigned retry = 2;
>> +
>> +    for (; retry; retry--) {
>> +        i2c_send(i2c, addr, testtime, 8);
>> +        /* new second may start here */
>> +        i2c_send(i2c, addr, testtime, 1);
>> +        i2c_recv(i2c, addr, buf, 7);
>> +
>> +        if (testtime[1] == buf[0]) {
> 
> Please also check the minutes here (reason: see below).
> 
>> +            break;
>> +        }
>> +        /* we raced start of second, retry */
>> +    };
>> +
>> +    g_assert_cmpuint(testtime[1], ==, buf[0]); /* SEC */
>> +    g_assert_cmpuint(testtime[2], ==, buf[1]); /* MIN */
> 
> Could you please wrap the SEC and MIN lines in a "if (!g_test_slow()) {
> ... }" statement? The problem is: The "make check" tests are run as CI
> on a system that is sometimes *very* overloaded. It might happen that
> the test is sometimes interrupted for dozens of seconds, so it might
> fail to properly read the time on a granularity of seconds. With
> !g_test_slow() you can make sure that the check is not done on such
> overloaded CI systems.

Ok I guess.  I certainly don't want to add more false positive test failures.

>> +    g_assert_cmpuint(testtime[3], ==, buf[2]); /* HOUR */
>> +    /* skip comparing Day of Week.  Not handled correctly */
>> +    g_assert_cmpuint(testtime[5], ==, buf[4]); /* DoM */
>> +    if (use_century) {
>> +        g_assert_cmpuint(testtime[6], ==, buf[5]); /* MON+century */
>> +    } else {
>> +        g_assert_cmpuint(testtime[6] & 0x7f, ==, buf[5]); /* MON */
>> +    }
>> +    g_assert_cmpuint(testtime[7], ==, buf[6]); /* YEAR */
>> +
>> +    g_assert_cmpuint(retry, >, 0);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +    int ret;
>> +    const char *arch = qtest_get_arch();
>> +    QTestState *s = NULL;
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    if (strcmp(arch, "arm") == 0) {
>> +        s = qtest_start("-display none -machine imx25-pdk");
> 
> Do you really need the "-display none" parameter here? ... I thought
> that was the default for qtests anyway?

This is a straight copy+paste from the ds1338-test I'm replacing.
I'll remove it.

>> +        i2c = imx_i2c_create(s, IMX25_I2C_0_BASE);
>> +        addr = DS1338_ADDR;
>> +        use_century = false;
>> +
>> +    }
>> +
>> +    qtest_add_data_func("/ds-rtc-i2c/set24_12am", test_time_24_12am, test_rtc_set);
>> +    qtest_add_data_func("/ds-rtc-i2c/set24_6am", test_time_24_6am, test_rtc_set);
>> +    qtest_add_data_func("/ds-rtc-i2c/set24_12pm", test_time_24_12pm, test_rtc_set);
>> +    qtest_add_data_func("/ds-rtc-i2c/set24_6pm", test_time_24_6pm, test_rtc_set);
>> +    qtest_add_func("/ds-rtc-i2c/current", test_rtc_current);
>> +
>> +    ret = g_test_run();
>> +
>> +    qtest_end();
>> +
>> +    return ret;
>> +}
>>
> 
>  Thomas
> 

Re: [Qemu-devel] [PATCH 3/5] timer: generalize Dallas/Maxim RTC i2c devices

[Peter Maydell]

On 19 February 2018 at 04:03, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Support for: ds1307, ds1337, ds1338, ds1339,
> ds1340, ds1375, ds1388, and ds3231.
>
> Tested with ds1338 and ds1375.
>
> The existing ds1338 model has two bugs
> with almost no practical impact.
>
> 1. Trying to set time in 12-hour mode works,
> but the 12 hour mode bit isn't stored.
> So time always reads in 24 hour mode.
>
> 2. wday_offset is always stored for the
> local time zone.  When the RTC is set
> and rtc_utc=1 and the local timezone
> has a different day than UTC, then
> wday_offset will be off by one.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  default-configs/arm-softmmu.mak |   2 +-
>  hw/timer/Makefile.objs          |   2 +-
>  hw/timer/ds-rtc-i2c.c           | 466 ++++++++++++++++++++++++++++++++++++++++
>  hw/timer/ds1338.c               | 248 ---------------------

This patch is really hard for me to read because it's
basically "throw away the old file and implement a new one".
Can you split it up a bit please? I'd recommend starting with
"just a plain file rename" and then piecemeal changes of
behaviour. Then it's easy to see what has been changed.


> --- /dev/null
> +++ b/hw/timer/ds-rtc-i2c.c
> @@ -0,0 +1,466 @@
> +/* Emulation of various Dallas/Maxim RTCs accessed via I2C bus
> + *
> + * Copyright (c) 2017 Michael Davidsaver
> + * Copyright (c) 2009 CodeSourcery
> + *
> + * Authors: Michael Davidsaver
> + *          Paul Brook
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the LICENSE file in the top-level directory.

This has lost the "contributions after 2012 are GPL-2-or-later"
part of the licensing information.

> +#ifdef DEBUG_DSRTC
> +#define DPRINTK(FMT, ...) info_report(TYPE_DSRTC " : " FMT, ## __VA_ARGS__)
> +#else
> +#define DPRINTK(FMT, ...) do {} while (0)
> +#endif

If we're overhauling the device it might be nice to convert it
to tracepoints, but I don't insist on that.

> +#define TYPE_DSRTC "ds-rtc-i2c"
> +#define DSRTC(obj) OBJECT_CHECK(DSRTCState, (obj), TYPE_DSRTC)
> +#define DSRTC_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(DSRTCClass, obj, TYPE_DSRTC)
> +#define DSRTC_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(DSRTCClass, klass, TYPE_DSRTC)
> +
> +static const VMStateDescription vmstate_dsrtc = {
> +    .name = TYPE_DSRTC,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_I2C_SLAVE(parent_obj, DSRTCState),
> +        VMSTATE_INT64(time_offset, DSRTCState),
> +        VMSTATE_INT8_V(wday_offset, DSRTCState, 2),
> +        VMSTATE_UINT8_ARRAY(regs, DSRTCState, DSRTC_REGSIZE),
> +        VMSTATE_UINT8(addr, DSRTCState),
> +        VMSTATE_BOOL(addrd, DSRTCState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};

Changing the VMState name and fields will break cross-version
migration from old QEMU versions of all the boards which use a
DS1338. I'd strongly prefer to avoid that. There are ways to
do that, like subsections and pre/post-load functions, but a
big part of it is not changing fields if you don't really need to.

> +
> +static void dsrtc_reset(DeviceState *device);
> +
> +/* update current time registers */
> +static

Can you not put newlines after the 'static' keyword, please?

> +void dsrtc_latch(DSRTCState *ds)
> +{


thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/5] tests: more thorough test of ds1338

[Michael Davidsaver]

> On 02/20/2018 09:44 AM, Michael Davidsaver wrote:
>> On 02/18/2018 11:39 PM, Thomas Huth wrote:
...
>> That magic (together with patch 1/5) is IMHO a little bit ugly. I've hit
>> the same problem with the m48t59 test recently, and I solved it by
>> moving the qtest_start() and qtest_end() calls from the main() function
>> into the single tests instead, so that each test starts with a clean state:
>>
>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9c29830c90d82f27f
>>
>> Could you maybe try whether that approach works for your test cases
>> here, too? Then you could do this without the "0xff" hack here...
> 
> Your right, this looks clearer.  I'll try this approach.

I ultimately decided not to go with this approach as test failures
didn't call qtest_quit(), and the process under test is left running
after gtester exits.

Instead I split the current time test off into a second executable.
This avoids all of the magic.


FYI.  I also looked at using g_test_add(), keeping the QTestState* in a
Fixture struct, and using setup and teardown functions to call
qtest_start()/qtest_quit().  This works, but seemed to me like too much
effort in this case.

Re: [Qemu-devel] [PATCH 2/5] tests: more thorough test of ds1338

[Thomas Huth]

On 24.03.2018 20:39, Michael Davidsaver wrote:
>> On 02/20/2018 09:44 AM, Michael Davidsaver wrote:
>>> On 02/18/2018 11:39 PM, Thomas Huth wrote:
> ...
>>> That magic (together with patch 1/5) is IMHO a little bit ugly. I've hit
>>> the same problem with the m48t59 test recently, and I solved it by
>>> moving the qtest_start() and qtest_end() calls from the main() function
>>> into the single tests instead, so that each test starts with a clean state:
>>>
>>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9c29830c90d82f27f
>>>
>>> Could you maybe try whether that approach works for your test cases
>>> here, too? Then you could do this without the "0xff" hack here...
>>
>> Your right, this looks clearer.  I'll try this approach.
> 
> I ultimately decided not to go with this approach as test failures
> didn't call qtest_quit(), and the process under test is left running
> after gtester exits.
> 
> Instead I split the current time test off into a second executable.
> This avoids all of the magic.

Honestly, I don't like the idea that we put tests into different files
just because of this. We're not doing this for any other tests so far,
so just using this pattern for the ds1338 / ds1375 tests sounds wrong.

If there is an issue with test processes not being killed correctly, I
think we should rather fix that issue instead, maybe by adding a special
function with atexit() or something similar ... I haven't looked closely
into that yet.

Just my 0.02 €.

 Thomas