Re: [Qemu-devel] [PATCH 11/14] timer: generalize ds1338

[David Gibson]

On Thu, Jul 05, 2018 at 11:19:58AM -0700, Michael Davidsaver wrote:
> Add class level handling for address space size
> and control register.
> 
> Signed-off-by: Michael Davidsaver 

Reviewed-by: David Gibson 

Although I don't love the name "addr_size" - just "nvram_size" seems
clearer.

> ---
>  hw/timer/ds-rtc.c | 62 
> +++
>  1 file changed, 49 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/timer/ds-rtc.c b/hw/timer/ds-rtc.c
> index 126566ce1f..6d3a8b5586 100644
> --- a/hw/timer/ds-rtc.c
> +++ b/hw/timer/ds-rtc.c
> @@ -21,8 +21,10 @@
>   */
>  #define NVRAM_SIZE 64
>  
> -#define TYPE_DSRTC "ds1338"
> +#define TYPE_DSRTC "dsrtc"
>  #define DSRTC(obj) OBJECT_CHECK(DSRTCState, (obj), TYPE_DSRTC)
> +#define DSRTC_CLASS(klass) OBJECT_CLASS_CHECK(DSRTCClass, (klass), 
> TYPE_DSRTC)
> +#define DSRTC_GET_CLASS(obj) OBJECT_GET_CLASS(DSRTCClass, (obj), TYPE_DSRTC)
>  
>  /* values stored in BCD */
>  /* 00-59 */
> @@ -38,7 +40,7 @@
>  /* 0-99 */
>  #define R_YEAR  (0x6)
>  
> -#define R_CTRL  (0x7)
> +#define R_DS1338_CTRL (0x7)
>  
>  /* use 12 hour mode when set */
>  FIELD(HOUR, SET12, 6, 1)
> @@ -65,6 +67,15 @@ typedef struct DSRTCState {
>  bool addr_byte;
>  } DSRTCState;
>  
> +typedef struct DSRTCClass {
> +I2CSlaveClass parent_obj;
> +
> +/* actual address space size must be <= NVRAM_SIZE */
> +unsigned addr_size;
> +unsigned ctrl_offset;
> +void (*ctrl_write)(DSRTCState *s, uint8_t);
> +} DSRTCClass;
> +
>  static const VMStateDescription vmstate_dsrtc = {
>  .name = "ds1338",
>  .version_id = 2,
> @@ -119,11 +130,12 @@ static void capture_current_time(DSRTCState *s)
>  
>  static void inc_regptr(DSRTCState *s)
>  {
> -/* The register pointer wraps around after 0x3F; wraparound
> +DSRTCClass *k = DSRTC_GET_CLASS(s);
> +/* The register pointer wraps around after k->addr_size-1; wraparound
>   * causes the current time/date to be retransferred into
>   * the secondary registers.
>   */
> -s->ptr = (s->ptr + 1) & (NVRAM_SIZE - 1);
> +s->ptr = (s->ptr + 1) % k->addr_size;
>  if (!s->ptr) {
>  capture_current_time(s);
>  }
> @@ -205,20 +217,15 @@ static void dsrtc_update(DSRTCState *s)
>  static int dsrtc_send(I2CSlave *i2c, uint8_t data)
>  {
>  DSRTCState *s = DSRTC(i2c);
> +DSRTCClass *k = DSRTC_GET_CLASS(s);
>  
>  if (s->addr_byte) {
> -s->ptr = data & (NVRAM_SIZE - 1);
> +s->ptr = data % k->addr_size;
>  s->addr_byte = false;
>  return 0;
>  }
> -if (s->ptr == R_CTRL) {
> -/* Control register. */
> -
> -/* Allow guest to set no-op controls for clock out pin and
> - * rate select.  Ignore write 1 to clear OSF.  We don't model
> - * oscillator stop, so it is never set.
> - */
> -data = data & 0x93;
> +if (s->ptr == k->ctrl_offset) {
> +(k->ctrl_write)(s, data);
>  }
>  s->nvram[s->ptr] = data;
>  if (s->ptr <= R_YEAR) {
> @@ -253,15 +260,44 @@ static void dsrtc_class_init(ObjectClass *klass, void 
> *data)
>  }
>  
>  static const TypeInfo dsrtc_info = {
> +.abstract  = true,
>  .name  = TYPE_DSRTC,
>  .parent= TYPE_I2C_SLAVE,
>  .instance_size = sizeof(DSRTCState),
>  .class_init= dsrtc_class_init,
>  };
>  
> +static void ds1338_control_write(DSRTCState *s, uint8_t data)
> +{
> +/* Control register. */
> +
> +/* Allow guest to set no-op controls for clock out pin and
> + * rate select.  Ignore write 1 to clear OSF.  We don't model
> + * oscillator stop, so it is never set.
> + */
> +s->nvram[R_DS1338_CTRL] = data & 0x93;
> +}
> +
> +static void ds1338_class_init(ObjectClass *klass, void *data)
> +{
> +DSRTCClass *k = DSRTC_CLASS(klass);
> +
> +k->addr_size = 0x40;
> +k->ctrl_offset = R_DS1338_CTRL;
> +k->ctrl_write = ds1338_control_write;
> +}
> +
> +static const TypeInfo ds1338_info = {
> +.name  = "ds1338",
> +.parent= TYPE_DSRTC,
> +.class_size= sizeof(DSRTCClass),
> +.class_init= ds1338_class_init,
> +};
> +
>  static void dsrtc_register_types(void)
>  {
>  type_register_static(_info);
> +type_register_static(_info);
>  }
>  
>  type_init(dsrtc_register_types)

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 11/14] timer: generalize ds1338

[Peter Maydell]

On 24 March 2018 at 19:24, Michael Davidsaver  wrote:
> Add class level handling for address space size
> and control register.
>
> Signed-off-by: Michael Davidsaver 
> ---

> @@ -206,22 +216,15 @@ static void dsrtc_update(DSRTCState *s)
>  static int dsrtc_send(I2CSlave *i2c, uint8_t data)
>  {
>  DSRTCState *s = DSRTC(i2c);
> +DSRTCClass *k = DSRTC_GET_CLASS(s);
>
>  if (s->addr_byte) {
> -s->ptr = data & (NVRAM_SIZE - 1);
> +s->ptr = data % k->addr_size;
>  s->addr_byte = false;
>  return 0;
>  }
> -if (s->ptr == R_CTRL) {
> -/* 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);
> -
> +if (s->ptr == k->ctrl_offset) {
> +(k->ctrl_write)(s, data);
>  }
>  s->nvram[s->ptr] = data;
>  if (s->ptr <= R_YEAR) {
> @@ -256,15 +259,41 @@ static void dsrtc_class_init(ObjectClass *klass, void 
> *data)
>  }
>
>  static const TypeInfo dsrtc_info = {
> +.abstract  = true,
>  .name  = TYPE_DSRTC,
>  .parent= TYPE_I2C_SLAVE,
>  .instance_size = sizeof(DSRTCState),
>  .class_init= dsrtc_class_init,
>  };
>
> +static void ds1338_control_write(DSRTCState *s, uint8_t data)
> +{
> +/* Control register. */
> +
> +/* allow guest to set no-op controls for clock out pin */
> +s->nvram[R_DS1338_CTRL] = data & 0x93;

This is mixing a behavioural change with an otherwise
pure refactoring -- can you split them out, please?

Otherwise the patch looks good.

thanks
-- PMM