Skip to contentSkip to conversation

hw/ds1338.c

PATCH Superseded Antoine Mathys 2 participants 17 messages Dec 11, 2012 – Dec 12, 2012

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Antoine Mathys

author
#0 lore raw

v2 cover hw/ds1338.c

This is the second version of my series of patch to hw/ds1338.c
It should address the various points that were made. The main difference with the previous version is that I dropped some stuff that will only be useful once we implement clock enable/disable.

author
#1 lore raw

PATCH v2 1/6 hw/ds1338.c: Correct bug in conversion to BCD.

Signed-off-by Antoine Mathys <[email protected]>
---
 hw/ds1338.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
hw/ds1338.c +2 -2
index b576d56..faaa4a0 100644
@@ -57,9 +57,9 @@ static void capture_current_time(DS1338State *s)
     } else {
         s->nvram[2] = to_bcd(now.tm_hour);
     }
-    s->nvram[3] = to_bcd(now.tm_wday) + 1;
+    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[5] = to_bcd(now.tm_mon + 1);
     s->nvram[6] = to_bcd(now.tm_year - 100);
 }
Signature · 1 line
1.7.10.4
author
#3 lore raw

PATCH v2 2/6 hw/ds1338.c: Add definitions for various flags in the RTC registers.

In reply to Antoine Mathys
Signed-off-by Antoine Mathys <[email protected]>
---
 hw/ds1338.c |    6 ++++++
 1 file changed, 6 insertions(+)
hw/ds1338.c +6 -0
index faaa4a0..69018bc 100644
@@ -17,6 +17,12 @@
  */
 #define NVRAM_SIZE 64
 
+/* Flags definitions */
+#define SECONDS_CH 0x80
+#define HOURS_12   0x40
+#define HOURS_PM   0x20
+#define CTRL_OSF   0x20
+
 typedef struct {
     I2CSlave i2c;
     int64_t offset;
Signature · 1 line
1.7.10.4
author
#5 lore raw

PATCH v2 3/6 hw/ds1338.c: Fix handling of HOURS register.

In reply to Antoine Mathys

Per the datasheet, the mapping between 12 and 24 hours modes is:

     0      <->  12   PM
     1-12   <->  1-12 AM
     13-23  <->  1-11 PM
Signed-off-by Antoine Mathys <[email protected]>
---
 hw/ds1338.c |   29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)
hw/ds1338.c +18 -11
index 69018bc..0f88720 100644
@@ -55,10 +55,15 @@ static void capture_current_time(DS1338State *s)
     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] & 0x40) {
-        s->nvram[2] = (to_bcd((now.tm_hour % 12)) + 1) | 0x40;
-        if (now.tm_hour >= 12) {
-            s->nvram[2] |= 0x20;
+    if (s->nvram[2] & HOURS_12) {
+        int tmp = now.tm_hour;
+        if (tmp == 0) {
+            tmp = 24;
+        }
+        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);

@@ -132,16 +137,18 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
             now.tm_min = from_bcd(data & 0x7f);
             break;
         case 2:
-            if (data & 0x40) {
-                if (data & 0x20) {
-                    data = from_bcd(data & 0x4f) + 11;
-                } else {
-                    data = from_bcd(data & 0x1f) - 1;
+            if (data & HOURS_12) {
+                int tmp = from_bcd(data & (HOURS_PM - 1));
+                if (data & HOURS_PM) {
+                    tmp += 12;
+                }
+                if (tmp == 24) {
+                    tmp = 0;
                 }
+                now.tm_hour = tmp;
             } else {
-                data = from_bcd(data);
+                now.tm_hour = from_bcd(data & (HOURS_12 - 1));
             }
-            now.tm_hour = data;
             break;
         case 3:
             now.tm_wday = from_bcd(data & 7) - 1;
Signature · 1 line
1.7.10.4
author
#7 lore raw

PATCH v2 4/6 hw/ds1338.c: Ensure state is properly initialized.

In reply to Antoine Mathys
Signed-off-by Antoine Mathys <[email protected]>
---
 hw/ds1338.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
hw/ds1338.c +10 -0
index 0f88720..b4d5b74 100644
@@ -179,6 +179,15 @@ static int ds1338_init(I2CSlave *i2c)
     return 0;
 }
 
+static void ds1338_reset(DeviceState *dev)
+{
+    DS1338State *s = FROM_I2C_SLAVE(DS1338State, I2C_SLAVE_FROM_QDEV(dev));
+
+    /* The clock is running and synchronized with the host */
+    s->offset = 0;
+    memset(s->nvram, 0, NVRAM_SIZE);
+}
+
 static void ds1338_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);

@@ -188,6 +197,7 @@ static void ds1338_class_init(ObjectClass *klass, void *data)
     k->event = ds1338_event;
     k->recv = ds1338_recv;
     k->send = ds1338_send;
+    dc->reset = ds1338_reset;
     dc->vmsd = &vmstate_ds1338;
 }
Signature · 1 line
1.7.10.4
author
#13 lore raw

PATCH v2 5/6 hw/ds1338.c: Implement support for the control register.

In reply to Antoine Mathys

The previous patch has the side effect of clearing the control register. That's already its proper power-on-reset value.

Signed-off-by Antoine Mathys <[email protected]>
---
 hw/ds1338.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
hw/ds1338.c +13 -4
index 5a6234b..319c341 100644
@@ -125,7 +125,8 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
         s->addr_byte = false;
         return 0;
     }
-    if (s->ptr < 8) {
+    if (s->ptr < 7) {
+        /* Time register. */
         struct tm now;
         qemu_get_timedate(&now, s->offset);
         switch(s->ptr) {

@@ -162,11 +163,19 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
         case 6:
             now.tm_year = from_bcd(data) + 100;
             break;
-        case 7:
-            /* Control register. Currently ignored.  */
-            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;
     }
Signature · 1 line
1.7.10.4
author
#15 lore raw

PATCH v2 6/6 hw/ds1338.c: Fix handling of DAY (wday) register.

In reply to Antoine Mathys

Per the datasheet, the DAY (wday) register is user defined. Implement this.

Signed-off-by Antoine Mathys <[email protected]>
---
 hw/ds1338.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
hw/ds1338.c +12 -3
index 319c341..c5cee99 100644
@@ -26,6 +26,7 @@
 typedef struct {
     I2CSlave i2c;
     int64_t offset;
+    uint8_t wday_offset;
     uint8_t nvram[NVRAM_SIZE];
     int32_t ptr;
     bool addr_byte;

@@ -33,12 +34,13 @@ typedef struct {
 
 static const VMStateDescription vmstate_ds1338 = {
     .name = "ds1338",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields = (VMStateField[]) {
         VMSTATE_I2C_SLAVE(i2c, 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),

@@ -68,7 +70,7 @@ static void capture_current_time(DS1338State *s)
     } else {
         s->nvram[2] = to_bcd(now.tm_hour);
     }
-    s->nvram[3] = to_bcd(now.tm_wday + 1);
+    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);

@@ -152,7 +154,13 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
             }
             break;
         case 3:
-            now.tm_wday = from_bcd(data & 7) - 1;
+            {
+                /* 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);

@@ -194,6 +202,7 @@ static void ds1338_reset(DeviceState *dev)
 
     /* The clock is running and synchronized with the host */
     s->offset = 0;
+    s->wday_offset = 0;
     memset(s->nvram, 0, NVRAM_SIZE);
 }
Signature · 1 line
1.7.10.4
#8 lore raw

Re: [Qemu-devel] [PATCH v2 4/6] hw/ds1338.c: Ensure state is properly initialized.

In reply to Antoine Mathys
Quoted 10 lines · Antoine Mathys
Signed-off-by Antoine Mathys <[email protected]>
---
 hw/ds1338.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
hw/ds1338.c +0 -0
index 0f88720..b4d5b74 100644

hw/ds1338.c static int ds1338_init(I2CSlave *i2c) · line 187

Quoted 9 lines
hw/ds1338.c +6 -0
@@ -179,6 +179,15 @@ static int ds1338_init(I2CSlave *i2c)
     return 0;
 }
 
+static void ds1338_reset(DeviceState *dev)
+{
+    DS1338State *s = FROM_I2C_SLAVE(DS1338State, I2C_SLAVE_FROM_QDEV(dev));
+
+    /* The clock is running and synchronized with the host */
+    s->offset = 0;

I think we need to reset ptr and addr_byte too...

hw/ds1338.c static int ds1338_init(I2CSlave *i2c) · line 189

Quoted 2 lines
hw/ds1338.c +2 -0
+    memset(s->nvram, 0, NVRAM_SIZE);
+}

-- PMM

Reviewed-by on v2 5/6
#14 lore raw

Re: [Qemu-devel] [PATCH v2 5/6] hw/ds1338.c: Implement support for the control register.

In reply to Antoine Mathys
Quoted 2 lines · Antoine Mathys

The previous patch has the side effect of clearing the control register. That's already its proper power-on-reset value.

This is true but it reads a bit oddly as a standalone git commit message...

Quoted 1 line
Signed-off-by Antoine Mathys <[email protected]>
Reviewed-by Peter Maydell <[email protected]>

-- PMM

author
#9 lore raw

Re: [Qemu-devel] [PATCH v2 4/6] hw/ds1338.c: Ensure state is properly initialized.

In reply to Peter Maydell
Quoted 1 line · Peter Maydell

I think we need to reset ptr and addr_byte too...

The initial value of the register pointer is unspecified.

addr_byte is only used when receiving user data. It is written in ds1338_event() then read in ds1338_send(). Setting it in ds1338_reset() is unnecessary and would break encapsulation.

author
#11 lore raw

Re: [Qemu-devel] [PATCH v2 4/6] hw/ds1338.c: Ensure state is properly initialized.

In reply to Peter Maydell
Quoted 9 lines

It's true that it probably doesn't make much difference, but both the addr_byte and ptr are part of the migration state listed in the vmstate struct. I think it is cleaner and more straightforward to reason about if resetting the device returns it to an exact known state. The state may be undefined according to the specification but that doesn't mean that our implementation has to leave it undefined.

Quoted 4 lines

I don't understand your point about encapsulation -- these are both fields of the device state, and ds1338_reset() is a method of the device and has every right to access them.

You are right. While it is not strictly necessary it is cleaner (and safer) to set every field to a reasonable value. In addition some guest code probably incorrectly assumes that the pointer is initially at zero so that wouldn't hurt.

Would it be ok to reply with a new version of this patch only, instead of resending the whole series ?

#12 lore raw

Re: [Qemu-devel] [PATCH v2 4/6] hw/ds1338.c: Ensure state is properly initialized.

Quoted 2 lines · Antoine Mathys

Would it be ok to reply with a new version of this patch only, instead of resending the whole series ?

We generally prefer the whole series to be resent, it avoids confusion about which versions of which patches are most current. You might like to also expand the 5/6 commit message a little since you're resending anyway.

thanks
-- PMM

#10 lore raw

Re: [Qemu-devel] [PATCH v2 4/6] hw/ds1338.c: Ensure state is properly initialized.

In reply to Antoine Mathys
Quoted 8 lines · Antoine Mathys
Quoted 1 line · Peter Maydell

I think we need to reset ptr and addr_byte too...

The initial value of the register pointer is unspecified.

addr_byte is only used when receiving user data. It is written in ds1338_event() then read in ds1338_send(). Setting it in ds1338_reset() is unnecessary and would break encapsulation.

It's true that it probably doesn't make much difference, but both the addr_byte and ptr are part of the migration state listed in the vmstate struct. I think it is cleaner and more straightforward to reason about if resetting the device returns it to an exact known state. The state may be undefined according to the specification but that doesn't mean that our implementation has to leave it undefined.

I don't understand your point about encapsulation -- these are both fields of the device state, and ds1338_reset() is a method of the device and has every right to access them.

-- PMM