Generalize Dallas/Maxim I2C RTC devices v3

The code change here looks like a reasonable no-behaviour-change simplification of the code, but it doesn't match up with the description in the commit message ?

Test current time and set+get round trip.

Separate current time test from set/get tests to avoid test needing to impose order, or to have a magic handshaketo reset RTC to current time.

Signed-off-by Michael Davidsaver <[email protected]>

---
 tests/Makefile.include      |   4 ++
 tests/ds-rtc-common.h       |  67 +++++++++++++++++++++++++
 tests/ds-rtc-current-test.c |  59 ++++++++++++++++++++++
 tests/ds-rtc-set-test.c     | 117 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 247 insertions(+)
 create mode 100644 tests/ds-rtc-common.h
 create mode 100644 tests/ds-rtc-current-test.c
 create mode 100644 tests/ds-rtc-set-test.c

tests/Makefile.include +4 -0

index 8859e88ffb..6ce5a9ff4d 100644
@@ -378,6 +378,8 @@ check-qtest-sparc64-y += tests/boot-serial-test$(EXESUF)
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 check-qtest-arm-y += tests/pca9552-test$(EXESUF)
 check-qtest-arm-y += tests/ds1338-test$(EXESUF)
+check-qtest-arm-y += tests/ds-rtc-current-test$(EXESUF)
+check-qtest-arm-y += tests/ds-rtc-set-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)

@@ -787,6 +789,8 @@ 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/pca9552-test$(EXESUF): tests/pca9552-test.o $(libqos-omap-obj-y)
 tests/ds1338-test$(EXESUF): tests/ds1338-test.o $(libqos-imx-obj-y)
+tests/ds-rtc-current-test$(EXESUF): tests/ds-rtc-current-test.o $(libqos-imx-obj-y)
+tests/ds-rtc-set-test$(EXESUF): tests/ds-rtc-set-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)

tests/ds-rtc-common.h new file +1 -0

new file mode 100644
index 0000000000..c8e6c2bc5b
@@ -0,0 +1,67 @@
+/* Common code for testing of Dallas/Maxim I2C bus RTC devices

tests/ds-rtc-common.h new file +56 -0

+#include "qemu/cutils.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;
+
+/* input buffer must have at least 7 elements */
+static inline time_t rtc_parse(const uint8_t *buf)
+{
+    struct tm parts;
+
+    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);
+}
+
+static time_t rtc_gettime(void)
+{
+    uint8_t buf[7];
+
+    /* zero address pointer */
+    buf[0] = 0;
+    i2c_send(i2c, addr, buf, 1);
+    /* read back current time registers */
+    i2c_recv(i2c, addr, buf, 7);
+
+    return rtc_parse(buf);
+}
+
+#endif /* DSRTCCOMMON_H */

tests/ds-rtc-current-test.c new file +0 -0

new file mode 100644
index 0000000000..6acbbed9a6

tests/ds-rtc-current-test.c new file +26 -0

+
+#include "qemu/osdep.h"
+#include "qemu/bcd.h"
+#include "qemu/cutils.h"
+#include "libqtest.h"
+#include "libqos/libqos.h"
+#include "libqos/i2c.h"
+
+#include "ds-rtc-common.h"
+
+/* read back and compare with current system time */
+static
+void test_rtc_current(void)
+{
+    time_t expected, actual;
+    /* relax test to limit false positives when host may be overloaded.
+     * Allow larger delta when running "-m quick"
+     */
+    time_t max_delta = g_test_slow() ? 1 : 30;
+
+    actual = time(NULL);
+    /* new second may start here */
+    expected = rtc_gettime();
+    g_assert_cmpuint(expected, <=, actual + max_delta);
+    g_assert_cmpuint(expected, >=, actual);
+}

Use names for registers and bits except
for R_CTRL which will be dealt with later,
and isn't modeled anyway.

Signed-off-by Michael Davidsaver <[email protected]>

Reviewed-by Peter Maydell <[email protected]>

---
 hw/timer/ds1338.c | 80 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 26 deletions(-)

hw/timer/ds1338.c +54 -26

index 3849b74a68..7298c5af43 100644
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "hw/i2c/i2c.h"
+#include "hw/registerfields.h"
 #include "qemu/bcd.h"
 
 /* Size of NVRAM including both the user-accessible area and the

@@ -20,15 +21,42 @@
  */
 #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)
 
+/* 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)
+
+#define R_CTRL  (0x7)
+
+/* use 12 hour mode when set */
+FIELD(HOUR, SET12, 6, 1)
+/* 00-23 */
+FIELD(HOUR, HOUR24, 0, 6)
+/* PM when set */
+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)
+
+FIELD(CTRL, OSF, 5, 1)
+
 typedef struct DS1338State {
     I2CSlave parent_obj;
 

@@ -61,25 +89,25 @@ static void capture_current_time(DS1338State *s)
      */
     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) {
+    s->nvram[R_SEC] = to_bcd(now.tm_sec);
+    s->nvram[R_MIN] = to_bcd(now.tm_min);
+    if (ARRAY_FIELD_EX32(s->nvram, HOUR, SET12)) {
         int tmp = now.tm_hour;
         if (tmp % 12 == 0) {
             tmp += 12;
         }
         if (tmp <= 12) {
-            s->nvram[2] = HOURS_12 | to_bcd(tmp);
+            s->nvram[R_HOUR] = R_HOUR_SET12_MASK | to_bcd(tmp);
         } else {
-            s->nvram[2] = HOURS_12 | HOURS_PM | to_bcd(tmp - 12);
+            s->nvram[R_HOUR] = R_HOUR_SET12_MASK | R_HOUR_AMPM_MASK | to_bcd(tmp - 12);
         }
     } else {
-        s->nvram[2] = to_bcd(now.tm_hour);
+        s->nvram[R_HOUR] = 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);
+    s->nvram[R_WDAY] = (now.tm_wday + s->wday_offset) % 7 + 1;
+    s->nvram[R_DATE] = to_bcd(now.tm_mday);
+    s->nvram[R_MONTH] = to_bcd(now.tm_mon + 1);
+    s->nvram[R_YEAR] = to_bcd(now.tm_year - 100);
 }
 
 static void inc_regptr(DS1338State *s)

@@ -141,17 +169,17 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
         struct tm now;
         qemu_get_timedate(&now, s->offset);
         switch(s->ptr) {
-        case 0:
+        case R_SEC:
             /* TODO: Implement CH (stop) bit.  */
             now.tm_sec = from_bcd(data & 0x7f);
             break;
-        case 1:
+        case R_MIN:
             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) {
+        case R_HOUR:
+            if (FIELD_EX32(data, HOUR, SET12)) {
+                int tmp = from_bcd(FIELD_EX32(data, HOUR, HOUR12));
+                if (FIELD_EX32(data, HOUR, AMPM)) {
                     tmp += 12;
                 }
                 if (tmp % 12 == 0) {

@@ -159,10 +187,10 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
                 }
                 now.tm_hour = tmp;
             } else {
-                now.tm_hour = from_bcd(data & (HOURS_12 - 1));
+                now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR24));
             }
             break;
-        case 3:
+        case R_WDAY:
             {
                 /* The day field is supposed to contain a value in
                    the range 1-7. Otherwise behavior is undefined.

@@ -171,18 +199,18 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
                 s->wday_offset = (user_wday - now.tm_wday + 7) % 7;
             }
             break;
-        case 4:
+        case R_DATE:
             now.tm_mday = from_bcd(data & 0x3f);
             break;
-        case 5:
+        case R_MONTH:
             now.tm_mon = from_bcd(data & 0x1f) - 1;
             break;
-        case 6:
+        case R_YEAR:
             now.tm_year = from_bcd(data) + 100;
             break;
         }
         s->offset = qemu_timedate_diff(&now);
-    } else if (s->ptr == 7) {
+    } else if (s->ptr == R_CTRL) {
         /* Control register. */
 
         /* Ensure bits 2, 3 and 6 will read back as zero. */

track of guest selection of 12-hour mode. Write through current time registers to achieve this. Will be overwritten by the next read/latch.

This was only being done in two of three arms of this conditional block.

Signed-off-by Michael Davidsaver <[email protected]>

---
 hw/timer/ds1338.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

hw/timer/ds1338.c +1 -3

index 7298c5af43..b56db5852e 100644
@@ -220,10 +220,8 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
            value unchanged. */
         data = (data & ~CTRL_OSF) | (data & s->nvram[s->ptr] & CTRL_OSF);
 
-        s->nvram[s->ptr] = data;
-    } else {
-        s->nvram[s->ptr] = data;
     }
+    s->nvram[s->ptr] = data;
     inc_regptr(s);
     return 0;
 }

On Thu, Jul 05, 2018 at 11:19:50AM -0700, Michael Davidsaver wrote:
11;rgb:ffff/ffff/ffff> Need to save HOUR[HOUR12] bit to keep

Quoted 8 lines

track of guest selection of 12-hour mode. Write through current time registers to achieve this. Will be overwritten by the next read/latch.

This was only being done in two of three arms of this conditional block.

Quoted 1 line

Signed-off-by Michael Davidsaver <[email protected]>

This looks dubious to me, or at least the explanation of it does. The other branch of the conditional is covering different registers in the device, which are part of the RTC component, rather than the NVRAM area. I wouldn't necessarily expect them to persist data as a rule the way the rest of the block does, even if this specific bit does need to be preserved.

    if (s->nvram[2] & HOURS_12) {

Quoted 20 lines

---
 hw/timer/ds1338.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

hw/timer/ds1338.c +1 -3

index 7298c5af43..b56db5852e 100644
@@ -220,10 +220,8 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
            value unchanged. */
         data = (data & ~CTRL_OSF) | (data & s->nvram[s->ptr] & CTRL_OSF);
 
-        s->nvram[s->ptr] = data;
-    } else {
-        s->nvram[s->ptr] = data;
     }
+    s->nvram[s->ptr] = data;
     inc_regptr(s);
     return 0;
 }

Also, I though I had test coverage of this bug. That's actually how I noticed it to begin with. But it seems my later change to allow for a slow test runner also stopped testing readback of the 12/24 hour mode bit. It just silently uses whichever it reads. I'll be re-issuing an updated version which restores this check. Then you will be able to easily see the effect of reverting 'timer: ds1338 persist 12-hour mode selection'.

test_rtc_set: assertion failed (mode_expect == mode_actual): (1 == 0)

Quoted 10 lines · David Gibson

On Thu, Jul 05, 2018 at 11:19:50AM -0700, Michael Davidsaver wrote:
11;rgb:ffff/ffff/ffff> Need to save HOUR[HOUR12] bit to keep

Quoted 8 lines

track of guest selection of 12-hour mode. Write through current time registers to achieve this. Will be overwritten by the next read/latch.

This was only being done in two of three arms of this conditional block.

Quoted 8 lines

Quoted 1 line

Signed-off-by Michael Davidsaver <[email protected]>

This looks dubious to me, or at least the explanation of it does. The other branch of the conditional is covering different registers in the device, which are part of the RTC component, rather than the NVRAM area. I wouldn't necessarily expect them to persist data as a rule the way the rest of the block does, even if this specific bit does need to be preserved.

The fact that the above capture_current_time() included the line

Quoted 1 line

    if (s->nvram[2] & HOURS_12) {

was enough to convince me that the original author intended to persist the 12/24 hour mode in this way. There are certainly other ways to accomplish this, but they would involved adding to the vmstate, which I've tried to avoid in this iteration.

Also, I though I had test coverage of this bug. That's actually how I noticed it to begin with. But it seems my later change to allow for a slow test runner also stopped testing readback of the 12/24 hour mode bit. It just silently uses whichever it reads. I'll be re-issuing an updated version which restores this check. Then you will be able to easily see the effect of reverting 'timer: ds1338 persist 12-hour mode selection'.

Quoted 21 lines

Quoted 20 lines

---
 hw/timer/ds1338.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

hw/timer/ds1338.c +1 -3

index 7298c5af43..b56db5852e 100644
@@ -220,10 +220,8 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
            value unchanged. */
         data = (data & ~CTRL_OSF) | (data & s->nvram[s->ptr] & CTRL_OSF);
 
-        s->nvram[s->ptr] = data;
-    } else {
-        s->nvram[s->ptr] = data;
     }
+    s->nvram[s->ptr] = data;
     inc_regptr(s);
     return 0;
 }

Simplify and comment the translation between
registers and struct tm.

Signed-off-by Michael Davidsaver <[email protected]>

hw/timer/ds1338.c +6 -6

+        /* 24 hour mode. */
         s->nvram[R_HOUR] = to_bcd(now.tm_hour);
     }
     s->nvram[R_WDAY] = (now.tm_wday + s->wday_offset) % 7 + 1;

@@ -178,14 +184,13 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
             break;
         case R_HOUR:
             if (FIELD_EX32(data, HOUR, SET12)) {
-                int tmp = from_bcd(FIELD_EX32(data, HOUR, HOUR12));
+                /* 12 hour (1-12) */
+                /* read and wrap 1-12 -> 0-11 */
+                now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR12)) % 12u;
                 if (FIELD_EX32(data, HOUR, AMPM)) {
-                    tmp += 12;
+                    now.tm_hour += 12;
                 }
-                if (tmp % 12 == 0) {
-                    tmp -= 12;
-                }
-                now.tm_hour = tmp;
+
             } else {
                 now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR24));
             }

instead of a read-modify-write, do direct translation of device registers to struct tm members.

This new ds1338_update() is the reverse of the existing capture_current_time().

Simplifies later handling of CENTURY bit in similar Dallas RTC chips.

Correctly handle different real weekday in guest and host timezones.
Allow guest to use any day as start of week (day 1). eg. Monday instead of Sunday.

Signed-off-by Michael Davidsaver <[email protected]>

---
 hw/timer/ds1338.c | 95 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 48 insertions(+), 47 deletions(-)

hw/timer/ds1338.c +0 -0

index 637a0f4246..0134ffd72a 100644

hw/timer/ds1338.c +13 -0

     s->nvram[R_DATE] = to_bcd(now.tm_mday);
     s->nvram[R_MONTH] = to_bcd(now.tm_mon + 1);
     s->nvram[R_YEAR] = to_bcd(now.tm_year - 100);

@@ -161,6 +164,46 @@ static int ds1338_recv(I2CSlave *i2c)
     return res;
 }
 
+/* call after guest writes to current time registers
+ * to re-compute our offset from host time.
+ */
+static void ds1338_update(DS1338State *s)
+{
+
+    struct tm now = {};
+
+    /* TODO: Implement CH (stop) bit?  */
+    now.tm_sec = from_bcd(s->nvram[R_SEC] & 0x7f);
+    now.tm_min = from_bcd(s->nvram[R_MIN] & 0x7f);
+    if (ARRAY_FIELD_EX32(s->nvram, HOUR, SET12)) {
+        /* 12 hour (1-12) */

hw/timer/ds1338.c +21 -46

+    now.tm_mday = from_bcd(s->nvram[R_DATE] & 0x3f);
+    now.tm_mon = from_bcd(s->nvram[R_MONTH] & 0x1f) - 1;
+    now.tm_year = from_bcd(s->nvram[R_YEAR]) + 100;
+    s->offset = qemu_timedate_diff(&now);
+
+    {
+        /* Round trip to get real wday_offset based on time delta and
+         * ref. timezone.
+         * Race if midnight (in ref. timezone) happens here.
+         */
+        int user_wday = now.tm_wday;
+        qemu_get_timedate(&now, s->offset);
+
+        s->wday_offset = (user_wday - now.tm_wday) % 7 + 1;
+    }
+}
+
 static int ds1338_send(I2CSlave *i2c, uint8_t data)
 {
     DS1338State *s = DS1338(i2c);

@@ -170,52 +213,7 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
         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 R_SEC:
-            /* TODO: Implement CH (stop) bit.  */
-            now.tm_sec = from_bcd(data & 0x7f);
-            break;
-        case R_MIN:
-            now.tm_min = from_bcd(data & 0x7f);
-            break;
-        case R_HOUR:
-            if (FIELD_EX32(data, HOUR, SET12)) {
-                /* 12 hour (1-12) */
-                /* read and wrap 1-12 -> 0-11 */
-                now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR12)) % 12u;
-                if (FIELD_EX32(data, HOUR, AMPM)) {
-                    now.tm_hour += 12;
-                }
-
-            } else {
-                now.tm_hour = from_bcd(FIELD_EX32(data, HOUR, HOUR24));
-            }
-            break;
-        case R_WDAY:
-            {
-                /* 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 R_DATE:
-            now.tm_mday = from_bcd(data & 0x3f);
-            break;
-        case R_MONTH:
-            now.tm_mon = from_bcd(data & 0x1f) - 1;
-            break;
-        case R_YEAR:
-            now.tm_year = from_bcd(data) + 100;
-            break;
-        }
-        s->offset = qemu_timedate_diff(&now);
-    } else if (s->ptr == R_CTRL) {
+    if (s->ptr == R_CTRL) {
         /* Control register. */
 
         /* Ensure bits 2, 3 and 6 will read back as zero. */

@@ -227,6 +225,9 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
 
     }
     s->nvram[s->ptr] = data;
+    if (s->ptr <= R_YEAR) {
+        ds1338_update(s);
+    }
     inc_regptr(s);
     return 0;
 }

Signed-off-by Michael Davidsaver <[email protected]>

---
 tests/ds-rtc-common.h       |  9 ++++---
 tests/ds-rtc-current-test.c |  2 +-
 tests/ds-rtc-set-test.c     | 58 +++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 63 insertions(+), 6 deletions(-)

tests/ds-rtc-common.h +6 -3

index c8e6c2bc5b..5bc7ab32a6 100644
@@ -20,12 +20,15 @@ static uint8_t addr;
 static bool use_century;
 
 /* input buffer must have at least 7 elements */
-static inline time_t rtc_parse(const uint8_t *buf)
+static inline time_t rtc_parse(const uint8_t *buf, int *mmode)
 {
     struct tm parts;
 
     parts.tm_sec = from_bcd(buf[0]);
     parts.tm_min = from_bcd(buf[1]);
+    if (mmode) {
+        *mmode = !!(buf[2] & 0x40);
+    }
     if (buf[2] & 0x40) {
         /* 12 hour */
         /* HOUR register is 1-12. */

@@ -51,7 +54,7 @@ static inline time_t rtc_parse(const uint8_t *buf)
     return mktimegm(&parts);
 }
 
-static time_t rtc_gettime(void)
+static time_t rtc_gettime(int *mmode)
 {
     uint8_t buf[7];
 

@@ -61,7 +64,7 @@ static time_t rtc_gettime(void)
     /* read back current time registers */
     i2c_recv(i2c, addr, buf, 7);
 
-    return rtc_parse(buf);
+    return rtc_parse(buf, mmode);
 }
 
 #endif /* DSRTCCOMMON_H */

tests/ds-rtc-current-test.c +1 -1

index 6acbbed9a6..3c15482a9d 100644
@@ -28,7 +28,7 @@ void test_rtc_current(void)
 
     actual = time(NULL);
     /* new second may start here */
-    expected = rtc_gettime();
+    expected = rtc_gettime(NULL);
     g_assert_cmpuint(expected, <=, actual + max_delta);
     g_assert_cmpuint(expected, >=, actual);
 }

tests/ds-rtc-set-test.c +56 -2

index 35e1a36281..3a742e897f 100644
@@ -29,6 +29,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 */

@@ -41,6 +53,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 */

@@ -53,6 +77,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 */

@@ -65,6 +101,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)

@@ -76,16 +124,18 @@ void test_rtc_set(const void *raw)
 
     const uint8_t *testtime = raw;
     time_t expected, actual;
+    int mode_expect, mode_actual;
 
     /* skip address pointer and parse remainder */
-    expected = rtc_parse(&testtime[1]);
+    expected = rtc_parse(&testtime[1], &mode_expect);
 
     i2c_send(i2c, addr, testtime, 8);
     /* host may start new second here */
-    actual = rtc_gettime();
+    actual = rtc_gettime(&mode_actual);
 
     g_assert_cmpuint(expected, <=, actual);
     g_assert_cmpuint(expected + max_delta, >=, actual);
+    g_assert_cmpint(mode_expect, ==, mode_actual);
 }
 
 int main(int argc, char *argv[])

@@ -108,6 +158,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);
 
     ret = g_test_run();
 

Signed-off-by Michael Davidsaver <[email protected]>

---
 tests/ds-rtc-common.h       | 10 +++++++---
 tests/ds-rtc-current-test.c |  9 ++++++++-
 tests/ds-rtc-set-test.c     |  6 ++++--
 3 files changed, 19 insertions(+), 6 deletions(-)

tests/ds-rtc-common.h +7 -3

index 5bc7ab32a6..782ea60453 100644
@@ -20,7 +20,7 @@ static uint8_t addr;
 static bool use_century;
 
 /* input buffer must have at least 7 elements */
-static inline time_t rtc_parse(const uint8_t *buf, int *mmode)
+static inline time_t rtc_parse(const uint8_t *buf, unsigned *wday, int *mmode)
 {
     struct tm parts;
 

@@ -51,10 +51,14 @@ static inline time_t rtc_parse(const uint8_t *buf, int *mmode)
         parts.tm_year += 100u;
     }
 
+    if (wday) {
+        *wday = parts.tm_wday;
+    }
+
     return mktimegm(&parts);
 }
 
-static time_t rtc_gettime(int *mmode)
+static time_t rtc_gettime(unsigned *wday, int *mmode)
 {
     uint8_t buf[7];
 

@@ -64,7 +68,7 @@ static time_t rtc_gettime(int *mmode)
     /* read back current time registers */
     i2c_recv(i2c, addr, buf, 7);
 
-    return rtc_parse(buf, mmode);
+    return rtc_parse(buf, wday, mmode);
 }
 
 #endif /* DSRTCCOMMON_H */

tests/ds-rtc-current-test.c +8 -1

index 3c15482a9d..08d8411671 100644
@@ -20,17 +20,24 @@
 static
 void test_rtc_current(void)
 {
+    struct tm tm_actual;
     time_t expected, actual;
     /* relax test to limit false positives when host may be overloaded.
      * Allow larger delta when running "-m quick"
      */
     time_t max_delta = g_test_slow() ? 1 : 30;
 
+    unsigned wday_expect;
+
     actual = time(NULL);
     /* new second may start here */
-    expected = rtc_gettime(NULL);
+    expected = rtc_gettime(&wday_expect, NULL);
+
+    gmtime_r(&actual, &tm_actual);
+
     g_assert_cmpuint(expected, <=, actual + max_delta);
     g_assert_cmpuint(expected, >=, actual);
+    g_assert_cmpuint(wday_expect, ==, tm_actual.tm_wday);
 }
 
 int main(int argc, char *argv[])

tests/ds-rtc-set-test.c +4 -2

index 3a742e897f..1004470931 100644
@@ -124,17 +124,19 @@ void test_rtc_set(const void *raw)
 
     const uint8_t *testtime = raw;
     time_t expected, actual;
+    unsigned wday_expect, wday_actual;
     int mode_expect, mode_actual;
 
     /* skip address pointer and parse remainder */
-    expected = rtc_parse(&testtime[1], &mode_expect);
+    expected = rtc_parse(&testtime[1], &wday_expect, &mode_expect);
 
     i2c_send(i2c, addr, testtime, 8);
     /* host may start new second here */
-    actual = rtc_gettime(&mode_actual);
+    actual = rtc_gettime(&wday_actual, &mode_actual);
 
     g_assert_cmpuint(expected, <=, actual);
     g_assert_cmpuint(expected + max_delta, >=, actual);
+    g_assert_cmpuint(wday_expect, ==, wday_actual);
     g_assert_cmpint(mode_expect, ==, mode_actual);
 }
 

Oscillator stop has never been modeled, so the
Oscillator Stop Flag can never be set.

Signed-off-by Michael Davidsaver <[email protected]>

---
 hw/timer/ds-rtc.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

hw/timer/ds-rtc.c +5 -9

index 3c5781d53c..126566ce1f 100644
@@ -21,8 +21,6 @@
  */
 #define NVRAM_SIZE 64
 
-#define CTRL_OSF   0x20
-
 #define TYPE_DSRTC "ds1338"
 #define DSRTC(obj) OBJECT_CHECK(DSRTCState, (obj), TYPE_DSRTC)
 

@@ -216,13 +214,11 @@ static int dsrtc_send(I2CSlave *i2c, uint8_t data)
     if (s->ptr == R_CTRL) {
         /* 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);
-
+        /* Allow guest to set no-op controls for clock out pin and
+         * rate select.  Ignore write 1 to clear OSF.  We don't model
+         * oscillator stop, so it is never set.
+         */
+        data = data & 0x93;
     }
     s->nvram[s->ptr] = data;
     if (s->ptr <= R_YEAR) {

Signed-off-by Michael Davidsaver <[email protected]>

Reviewed-by Peter Maydell <[email protected]>

---
 hw/timer/ds-rtc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

hw/timer/ds-rtc.c +7 -1

index 6d3a8b5586..9abac38628 100644
@@ -70,6 +70,7 @@ typedef struct DSRTCState {
 typedef struct DSRTCClass {
     I2CSlaveClass parent_obj;
 
+    bool has_century;
     /* actual address space size must be <= NVRAM_SIZE */
     unsigned addr_size;
     unsigned ctrl_offset;

@@ -93,6 +94,7 @@ static const VMStateDescription vmstate_dsrtc = {
 
 static void capture_current_time(DSRTCState *s)
 {
+    DSRTCClass *k = DSRTC_GET_CLASS(s);
     /* Capture the current time into the secondary registers
      * which will be actually read by the data transfer operation.
      */

@@ -125,7 +127,10 @@ static void capture_current_time(DSRTCState *s)
     }
     s->nvram[R_DATE] = to_bcd(now.tm_mday);
     s->nvram[R_MONTH] = to_bcd(now.tm_mon + 1);
-    s->nvram[R_YEAR] = to_bcd(now.tm_year - 100);
+    s->nvram[R_YEAR] = to_bcd(now.tm_year % 100u);
+
+    ARRAY_FIELD_DP32(s->nvram, MONTH, CENTURY,
+                     k->has_century && now.tm_year >= 100)
 }
 
 static void inc_regptr(DSRTCState *s)

@@ -282,6 +287,7 @@ static void ds1338_class_init(ObjectClass *klass, void *data)
 {
     DSRTCClass *k = DSRTC_CLASS(klass);
 
+    k->has_century = false;
     k->addr_size = 0x40;
     k->ctrl_offset = R_DS1338_CTRL;
     k->ctrl_write = ds1338_control_write;

differences from ds1338

* Has alarms (not modeled)
* different control register (not modeled)
* smaller address space (0x20 vs. 0x40)

Signed-off-by Michael Davidsaver <[email protected]>

Reviewed-by Peter Maydell <[email protected]>

---
 hw/timer/ds-rtc.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

hw/timer/ds-rtc.c +28 -2

index 9abac38628..3a8ad1a00f 100644
@@ -1,8 +1,9 @@
 /*
- * MAXIM DS1338 I2C RTC+NVRAM
+ * MAXIM/Dallas DS1338 and DS1375 I2C RTC+NVRAM
  *
+ * Copyright (c) 2018 Michael Davidsaver
  * Copyright (c) 2009 CodeSourcery.
- * Written by Paul Brook
+ * Written by Paul Brook, Michael Davidsaver
  *
  * This code is licensed under the GNU GPL v2.
  *

@@ -41,6 +42,7 @@
 #define R_YEAR  (0x6)
 
 #define R_DS1338_CTRL (0x7)
+#define R_DS1375_CTRL (0xe)
 
 /* use 12 hour mode when set */
 FIELD(HOUR, SET12, 6, 1)

@@ -300,10 +302,34 @@ static const TypeInfo ds1338_info = {
     .class_init    = ds1338_class_init,
 };
 
+static void ds1375_control_write(DSRTCState *s, uint8_t data)
+{
+    /* just store it, we don't model any features */
+    s->nvram[R_DS1375_CTRL] = data;
+}
+
+static void ds1375_class_init(ObjectClass *klass, void *data)
+{
+    DSRTCClass *k = DSRTC_CLASS(klass);
+
+    k->has_century = true;
+    k->addr_size = 0x20;
+    k->ctrl_offset = R_DS1375_CTRL;
+    k->ctrl_write = ds1375_control_write;
+}
+
+static const TypeInfo ds1375_info = {
+    .name          = "ds1375",
+    .parent        = TYPE_DSRTC,
+    .class_size    = sizeof(DSRTCClass),
+    .class_init    = ds1375_class_init,
+};
+
 static void dsrtc_register_types(void)
 {
     type_register_static(&dsrtc_info);
     type_register_static(&ds1338_info);
+    type_register_static(&ds1375_info);
 }
 
 type_init(dsrtc_register_types)

redundant to ds-rtc-*-test.c

Signed-off-by Michael Davidsaver <[email protected]>

---
 tests/Makefile.include |  2 --
 tests/ds1338-test.c    | 75 --------------------------------------------------
 2 files changed, 77 deletions(-)
 delete mode 100644 tests/ds1338-test.c

Add class level handling for address space size
and control register.

Signed-off-by Michael Davidsaver <[email protected]>

---
 hw/timer/ds-rtc.c | 62 +++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 13 deletions(-)

hw/timer/ds-rtc.c +49 -13

index 126566ce1f..6d3a8b5586 100644
@@ -21,8 +21,10 @@
  */
 #define NVRAM_SIZE 64
 
-#define TYPE_DSRTC "ds1338"
+#define TYPE_DSRTC "dsrtc"
 #define DSRTC(obj) OBJECT_CHECK(DSRTCState, (obj), TYPE_DSRTC)
+#define DSRTC_CLASS(klass) OBJECT_CLASS_CHECK(DSRTCClass, (klass), TYPE_DSRTC)
+#define DSRTC_GET_CLASS(obj) OBJECT_GET_CLASS(DSRTCClass, (obj), TYPE_DSRTC)
 
 /* values stored in BCD */
 /* 00-59 */

@@ -38,7 +40,7 @@
 /* 0-99 */
 #define R_YEAR  (0x6)
 
-#define R_CTRL  (0x7)
+#define R_DS1338_CTRL (0x7)
 
 /* use 12 hour mode when set */
 FIELD(HOUR, SET12, 6, 1)

@@ -65,6 +67,15 @@ typedef struct DSRTCState {
     bool addr_byte;
 } DSRTCState;
 
+typedef struct DSRTCClass {
+    I2CSlaveClass parent_obj;
+
+    /* actual address space size must be <= NVRAM_SIZE */
+    unsigned addr_size;
+    unsigned ctrl_offset;
+    void (*ctrl_write)(DSRTCState *s, uint8_t);
+} DSRTCClass;
+
 static const VMStateDescription vmstate_dsrtc = {
     .name = "ds1338",
     .version_id = 2,

@@ -119,11 +130,12 @@ static void capture_current_time(DSRTCState *s)
 
 static void inc_regptr(DSRTCState *s)
 {
-    /* The register pointer wraps around after 0x3F; wraparound
+    DSRTCClass *k = DSRTC_GET_CLASS(s);
+    /* The register pointer wraps around after k->addr_size-1; wraparound
      * causes the current time/date to be retransferred into
      * the secondary registers.
      */
-    s->ptr = (s->ptr + 1) & (NVRAM_SIZE - 1);
+    s->ptr = (s->ptr + 1) % k->addr_size;
     if (!s->ptr) {
         capture_current_time(s);
     }

@@ -205,20 +217,15 @@ static void dsrtc_update(DSRTCState *s)
 static int dsrtc_send(I2CSlave *i2c, uint8_t data)
 {
     DSRTCState *s = DSRTC(i2c);
+    DSRTCClass *k = DSRTC_GET_CLASS(s);
 
     if (s->addr_byte) {
-        s->ptr = data & (NVRAM_SIZE - 1);
+        s->ptr = data % k->addr_size;
         s->addr_byte = false;
         return 0;
     }
-    if (s->ptr == R_CTRL) {
-        /* Control register. */
-
-        /* Allow guest to set no-op controls for clock out pin and
-         * rate select.  Ignore write 1 to clear OSF.  We don't model
-         * oscillator stop, so it is never set.
-         */
-        data = data & 0x93;
+    if (s->ptr == k->ctrl_offset) {
+        (k->ctrl_write)(s, data);
     }
     s->nvram[s->ptr] = data;
     if (s->ptr <= R_YEAR) {

@@ -253,15 +260,44 @@ static void dsrtc_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo dsrtc_info = {
+    .abstract      = true,
     .name          = TYPE_DSRTC,
     .parent        = TYPE_I2C_SLAVE,
     .instance_size = sizeof(DSRTCState),
     .class_init    = dsrtc_class_init,
 };
 
+static void ds1338_control_write(DSRTCState *s, uint8_t data)
+{
+    /* Control register. */
+
+    /* Allow guest to set no-op controls for clock out pin and
+     * rate select.  Ignore write 1 to clear OSF.  We don't model
+     * oscillator stop, so it is never set.
+     */
+    s->nvram[R_DS1338_CTRL] = data & 0x93;
+}
+
+static void ds1338_class_init(ObjectClass *klass, void *data)
+{
+    DSRTCClass *k = DSRTC_CLASS(klass);
+
+    k->addr_size = 0x40;
+    k->ctrl_offset = R_DS1338_CTRL;
+    k->ctrl_write = ds1338_control_write;
+}
+
+static const TypeInfo ds1338_info = {
+    .name          = "ds1338",
+    .parent        = TYPE_DSRTC,
+    .class_size    = sizeof(DSRTCClass),
+    .class_init    = ds1338_class_init,
+};
+
 static void dsrtc_register_types(void)
 {
     type_register_static(&dsrtc_info);
+    type_register_static(&ds1338_info);
 }
 
 type_init(dsrtc_register_types)