[Qemu-devel] [PATCH v2 0/6] hw/ds1338.c

[Qemu-devel] [PATCH v2 0/6] hw/ds1338.c

[Antoine Mathys]

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.

[Qemu-devel] [PATCH v2 1/6] hw/ds1338.c: Correct bug in conversion to BCD.

[Antoine Mathys]

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

diff --git a/hw/ds1338.c b/hw/ds1338.c
index b576d56..faaa4a0 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -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);
  }

-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v2 1/6] hw/ds1338.c: Correct bug in conversion to BCD.

[Peter Maydell]

On 11 December 2012 22:49, Antoine Mathys <barsamin@gmail.com> wrote:
> Signed-off-by: Antoine Mathys <barsamin@gmail.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [PATCH v2 2/6] hw/ds1338.c: Add definitions for various flags in the RTC registers.

[Antoine Mathys]

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

diff --git a/hw/ds1338.c b/hw/ds1338.c
index faaa4a0..69018bc 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -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;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v2 2/6] hw/ds1338.c: Add definitions for various flags in the RTC registers.

[Peter Maydell]

On 11 December 2012 22:52, Antoine Mathys <barsamin@gmail.com> wrote:
> Signed-off-by: Antoine Mathys <barsamin@gmail.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [PATCH v2 3/6] hw/ds1338.c: Fix handling of HOURS register.

[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 <barsamin@gmail.com>
---
  hw/ds1338.c |   29 ++++++++++++++++++-----------
  1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/hw/ds1338.c b/hw/ds1338.c
index 69018bc..0f88720 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -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;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v2 3/6] hw/ds1338.c: Fix handling of HOURS register.

[Peter Maydell]

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

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

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

[Antoine Mathys]

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

diff --git a/hw/ds1338.c b/hw/ds1338.c
index 0f88720..b4d5b74 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -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;
  }

-- 
1.7.10.4

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

[Peter Maydell]

On 11 December 2012 22:58, Antoine Mathys <barsamin@gmail.com> wrote:
> Signed-off-by: Antoine Mathys <barsamin@gmail.com>
> ---
>  hw/ds1338.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/ds1338.c b/hw/ds1338.c
> index 0f88720..b4d5b74 100644
> --- a/hw/ds1338.c
> +++ b/hw/ds1338.c
> @@ -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...

> +    memset(s->nvram, 0, NVRAM_SIZE);
> +}

-- PMM

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

[Antoine Mathys]

On 12/12/2012 01:05 PM, Peter Maydell wrote:
> 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.

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

[Peter Maydell]

On 12 December 2012 12:46, Antoine Mathys <barsamin@gmail.com> wrote:
> On 12/12/2012 01:05 PM, Peter Maydell wrote:
>> 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

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

[Antoine Mathys]

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

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 ?

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

[Peter Maydell]

On 12 December 2012 13:11, Antoine Mathys <barsamin@gmail.com> wrote:
> 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

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

[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 <barsamin@gmail.com>
---
  hw/ds1338.c |   17 +++++++++++++----
  1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/ds1338.c b/hw/ds1338.c
index 5a6234b..319c341 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -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;
      }
-- 
1.7.10.4

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

[Peter Maydell]

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

> Signed-off-by: Antoine Mathys <barsamin@gmail.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [PATCH v2 6/6] hw/ds1338.c: Fix handling of DAY (wday) register.

[Antoine Mathys]

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

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

diff --git a/hw/ds1338.c b/hw/ds1338.c
index 319c341..c5cee99 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -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);
  }

-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v2 6/6] hw/ds1338.c: Fix handling of DAY (wday) register.

[Peter Maydell]

On 11 December 2012 22:59, Antoine Mathys <barsamin@gmail.com> wrote:
> Per the datasheet, the DAY (wday) register is user defined. Implement this.
>
> Signed-off-by: Antoine Mathys <barsamin@gmail.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM