[Qemu-devel] [PATCH 4/4] hw/ds1338.c: Handle stuck bits and preserve CH

[Qemu-devel] [PATCH 4/4] hw/ds1338.c: Handle stuck bits and preserve CH

[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).

Signed-off-by: Antoine Mathys <barsamin@gmail.com>
---
  hw/ds1338.c |   50 ++++++++++++++++++++++++++++++++++----------------
  1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/hw/ds1338.c b/hw/ds1338.c
index f3c6bc5..8f85635 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -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;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 4/4] hw/ds1338.c: Handle stuck bits and preserve CH

[Peter Maydell]

On 2 December 2012 17:22, Antoine Mathys <barsamin@gmail.com> wrote:
> 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