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

[Peter Maydell]

On 3 December 2012 20:10, Antoine Mathys <address@hidden> wrote:
> Per the datasheet, the DATE (wday) register is user defined. Implement this.
>
> Signed-off-by: Antoine Mathys <address@hidden>
> ---
>  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