Re: [Qemu-devel] [PATCH 5/?] hw/ds1338.c: Fix handling of DATE (wday) register

2012-12-04 Thread Peter Maydell
On 3 December 2012 20:10, Antoine Mathys barsa...@gmail.com wrote:
 Per the datasheet, the DATE (wday) register is user defined. Implement this.

 Signed-off-by: Antoine Mathys barsa...@gmail.com
 ---
  hw/ds1338.c |   14 --
  1 file changed, 12 insertions(+), 2 deletions(-)

 diff --git a/hw/ds1338.c b/hw/ds1338.c
 index 8f85635..c502934 100644
 --- a/hw/ds1338.c
 +++ b/hw/ds1338.c
 @@ -20,6 +20,7 @@
  typedef struct {
  I2CSlave i2c;
  int64_t offset;
 +uint8_t wday_offset;
  uint8_t nvram[NVRAM_SIZE];
  int32_t ptr;
  bool addr_byte;
 @@ -33,6 +34,7 @@ static const VMStateDescription vmstate_ds1338 = {
  .fields = (VMStateField[]) {
  VMSTATE_I2C_SLAVE(i2c, DS1338State),
  VMSTATE_INT64(offset, DS1338State),
 +VMSTATE_UINT8(wday_offset, DS1338State),
  VMSTATE_UINT8_ARRAY(nvram, DS1338State, NVRAM_SIZE),
  VMSTATE_INT32(ptr, DS1338State),
  VMSTATE_BOOL(addr_byte, DS1338State),

This breaks migration -- you need to bump the version_id to 2 and mark
the new field as only present in the new version, like this:
 VMSTATE_UINT8_V(wday_offset, DS1338State, 2),

 @@ -62,7 +64,7 @@ static void write_time(DS1338State *s, const struct tm
 *tm)
  } else {
  s-nvram[2] = to_bcd(tm-tm_hour);
  }
 -s-nvram[3] = to_bcd(tm-tm_wday + 1);
 +s-nvram[3] = to_bcd((tm-tm_wday + s-wday_offset) % 7 + 1);

There's not much point doing a to_bcd() on a value guaranteed to
be in [0..9].

  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);
 @@ -164,7 +166,12 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
  }
  break;
  case 3:
 -now.tm_wday = from_bcd(data  0x07) - 1;
 +{

This brace should line up with the 'c' in 'case' (ditto the closing brace).

 +int user_wday = from_bcd(data  0x07) - 1;

...again, from_bcd() is pointless here.

 +if ((user_wday = 0)  (user_wday = 6)) {

This condition is an obscure way to guard against the undefined case
of the guest writing zero to a register that's supposed to contain
values between 1 and 7. It would also be good to have a comment saying
explicitly that the datasheet says you get undefined operation here.

(for curiosity, do you happen to have real hardware to see what it
does in this case?)

 +s-wday_offset = (user_wday - now.tm_wday + 7) % 7;
 +}
 +}
  break;
  case 4:
  now.tm_mday = from_bcd(data  0x3f);
 @@ -194,6 +201,9 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)

  static int ds1338_init(I2CSlave *i2c)
  {
 +DS1338State *s = FROM_I2C_SLAVE(DS1338State, i2c);
 +s-wday_offset = 0;
 +

This doesn't belong in the device init function, you need to
create a reset function and reset it there. (ie set dc-reset
in the class init function, see eg hw/pl190.c for an example).

  return 0;
  }

 --
 1.7.10.4


thanks
-- PMM



[Qemu-devel] [PATCH 5/?] hw/ds1338.c: Fix handling of DATE (wday) register

2012-12-03 Thread Antoine Mathys

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

Signed-off-by: Antoine Mathys barsa...@gmail.com
---
 hw/ds1338.c |   14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/ds1338.c b/hw/ds1338.c
index 8f85635..c502934 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -20,6 +20,7 @@
 typedef struct {
 I2CSlave i2c;
 int64_t offset;
+uint8_t wday_offset;
 uint8_t nvram[NVRAM_SIZE];
 int32_t ptr;
 bool addr_byte;
@@ -33,6 +34,7 @@ static const VMStateDescription vmstate_ds1338 = {
 .fields = (VMStateField[]) {
 VMSTATE_I2C_SLAVE(i2c, DS1338State),
 VMSTATE_INT64(offset, DS1338State),
+VMSTATE_UINT8(wday_offset, DS1338State),
 VMSTATE_UINT8_ARRAY(nvram, DS1338State, NVRAM_SIZE),
 VMSTATE_INT32(ptr, DS1338State),
 VMSTATE_BOOL(addr_byte, DS1338State),
@@ -62,7 +64,7 @@ static void write_time(DS1338State *s, const struct tm 
*tm)

 } else {
 s-nvram[2] = to_bcd(tm-tm_hour);
 }
-s-nvram[3] = to_bcd(tm-tm_wday + 1);
+s-nvram[3] = to_bcd((tm-tm_wday + s-wday_offset) % 7 + 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);
@@ -164,7 +166,12 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
 }
 break;
 case 3:
-now.tm_wday = from_bcd(data  0x07) - 1;
+{
+int user_wday = from_bcd(data  0x07) - 1;
+if ((user_wday = 0)  (user_wday = 6)) {
+s-wday_offset = (user_wday - now.tm_wday + 7) % 7;
+}
+}
 break;
 case 4:
 now.tm_mday = from_bcd(data  0x3f);
@@ -194,6 +201,9 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)

 static int ds1338_init(I2CSlave *i2c)
 {
+DS1338State *s = FROM_I2C_SLAVE(DS1338State, i2c);
+s-wday_offset = 0;
+
 return 0;
 }

--
1.7.10.4