hw/ds1338.c: Handle stuck bits and preserve CH

PATCH In review Antoine Mathys 2 participants 2 messages Dec 2, 2012 – Dec 4, 2012

lore

Share & tools

lore

Antoine Mathys

author

2012-12-02 17:22 UTC #0 lore raw

PATCH 4/4 hw/ds1338.c: Handle stuck bits and preserve CH

Preserve the CH bit when updating the time.

Per the datasheet, some bits in the first eight registers must always read back as zero. These registers can be written in two ways:
1. By ourselves when we update the registers with the current time.
2. By user request
Even though (1) is not supposed to set these bits it is safer to check anyway. In order not to duplicate this logic I thus chose to enforce it at read time. Note that currently we don't preserve what the user sends in (2). We will have to once we support stopping the clock (CH bit).

Signed-off-by Antoine Mathys <[email protected]>

---
 hw/ds1338.c |   50 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 16 deletions(-)

hw/ds1338.c +34 ~~-16~~

index f3c6bc5..8f85635 100644
@@ -40,17 +40,17 @@ static const VMStateDescription vmstate_ds1338 = {
     }
 };
 
-static void capture_current_time(DS1338State *s)
+/* Write TM to the RTC registers. */
+static void write_time(DS1338State *s, const struct tm *tm)
 {
-    /* 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);
+    /* Preserve the CH flag. */
+    s->nvram[0] &= 0x80;
+    s->nvram[0] |= to_bcd(tm->tm_sec);
+
+    s->nvram[1] = to_bcd(tm->tm_min);
+
     if (s->nvram[2] & 0x40) {
-        int tmp = now.tm_hour;
+        int tmp = tm->tm_hour;
         if (tmp == 0) {
             tmp = 24;
         }

@@ -60,12 +60,30 @@ static void capture_current_time(DS1338State *s)
             s->nvram[2] = 0x60 | to_bcd(tmp - 12);
         }
     } else {
-        s->nvram[2] = to_bcd(now.tm_hour);
+        s->nvram[2] = to_bcd(tm->tm_hour);
     }
-    s->nvram[3] = to_bcd(now.tm_wday + 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[3] = to_bcd(tm->tm_wday + 1);
+    s->nvram[4] = to_bcd(tm->tm_mday);
+    s->nvram[5] = to_bcd(tm->tm_mon + 1);
+    s->nvram[6] = to_bcd(tm->tm_year - 100);
+}
+
+static void update_read_buffer(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);
+    write_time(s, &now);
+
+    /* Ensure specified bits read as zero. */
+    s->nvram[1] &= 0x7F;
+    s->nvram[2] &= 0x7F;
+    s->nvram[3] &= 0x07;
+    s->nvram[4] &= 0x3F;
+    s->nvram[5] &= 0x1F;
+    s->nvram[7] &= 0xB3;
 }
 
 static void inc_regptr(DS1338State *s)

@@ -76,7 +94,7 @@ static void inc_regptr(DS1338State *s)
      */
     s->ptr = (s->ptr + 1) & (NVRAM_SIZE - 1);
     if (!s->ptr) {
-        capture_current_time(s);
+        update_read_buffer(s);
     }
 }

@@ -91,7 +109,7 @@ static void ds1338_event(I2CSlave *i2c, enum i2c_event event)
          * 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);
+        update_read_buffer(s);
         break;
     case I2C_START_SEND:
         s->addr_byte = true;

Signature · 1 line

1.7.10.4

Peter Maydell

2012-12-04 18:02 UTC #1 lore raw

Quoted 11 lines · Antoine Mathys

Preserve the CH bit when updating the time.
Per the datasheet, some bits in the first eight registers must always read back as zero. These registers can be written in two ways:
1. By ourselves when we update the registers with the current time.
2. By user request
Even though (1) is not supposed to set these bits it is safer to check anyway. In order not to duplicate this logic I thus chose to enforce it at read time.
Note that currently we don't preserve what the user sends in (2). We will have to once we support stopping the clock (CH bit).

This patch is trying to do two things at once, which makes it very hard to review. Please split it.

I'm also not convinced it's worth doing bit masking after updating the time -- if that's wrong then we're returning wrong values anyway and non-zero RAZ fields is the least of our problems.

thanks
-- PMM