Re: [Qemu-devel] [PATCH v5 02/15] ssi: Added VMSD stub

[Peter Crosthwaite <peter.crosthwaite@petalogix.com>]

On Mon, 2012-08-06 at 10:13 +0100, Peter Maydell wrote:
> On 6 August 2012 03:16, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
> > Added VMSD stub for SSI slaves. Fields may be added to this VMSD for generic
> > SSI slave state (e.g. the CS line state).
> 
> This is more me being confused about how this should work than a
> review comment, but it seems a bit odd that we have a hierarchy
> Device->SSI->ADS7846[etc], where the VMState for the ADS7846
> includes a field for the SSI VMState, but the SSI VMState doesn't
> include a field for the Device VMState.
> 
> What you've done here matches how i2c works currently, but I've
> just cc'd Anthony and Juan to check whether there's a better way
> of doing it.
> 

So VMSD is handled by casting to TYPE_DEVICE and populating dc->vmsd.
This is only going to work for one layer of heirachy. But, I notice a
few device models here and there just call vmstate_register() directly.
Question is, can we call vmstate_register() from the object init
function for SSI_SLAVE, seperately? This would mean that a single
TYPE_DEVICE object is going to get two vmsds instead of one. The
original, and one for SSI_SLAVE (which has the cs state). Sound legit?
Any implementation details and pitfalls to be aware of?

Would be nice to get rid of this change pattern applied to all the
existing SSI devices.

Regards,
Peter

> 
> > Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> > ---
> >  hw/ads7846.c   |    1 +
> >  hw/max111x.c   |    1 +
> >  hw/spitz.c     |    2 ++
> >  hw/ssi.c       |   10 ++++++++++
> >  hw/ssi.h       |   10 ++++++++++
> >  hw/stellaris.c |    1 +
> >  hw/z2.c        |    1 +
> >  7 files changed, 26 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/ads7846.c b/hw/ads7846.c
> > index 41c7f10..6a523f6 100644
> > --- a/hw/ads7846.c
> > +++ b/hw/ads7846.c
> > @@ -124,6 +124,7 @@ static const VMStateDescription vmstate_ads7846 = {
> >      .minimum_version_id_old = 0,
> >      .post_load = ads7856_post_load,
> >      .fields      = (VMStateField[]) {
> > +        VMSTATE_SSI_SLAVE(ssidev, ADS7846State),
> >          VMSTATE_INT32_ARRAY(input, ADS7846State, 8),
> >          VMSTATE_INT32(noise, ADS7846State),
> >          VMSTATE_INT32(cycle, ADS7846State),
> > diff --git a/hw/max111x.c b/hw/max111x.c
> > index 706d89f..948fd97 100644
> > --- a/hw/max111x.c
> > +++ b/hw/max111x.c
> > @@ -103,6 +103,7 @@ static const VMStateDescription vmstate_max111x = {
> >      .minimum_version_id = 0,
> >      .minimum_version_id_old = 0,
> >      .fields      = (VMStateField[]) {
> > +        VMSTATE_SSI_SLAVE(ssidev, MAX111xState),
> >          VMSTATE_UINT8(tb1, MAX111xState),
> >          VMSTATE_UINT8(rb2, MAX111xState),
> >          VMSTATE_UINT8(rb3, MAX111xState),
> > diff --git a/hw/spitz.c b/hw/spitz.c
> > index 20e7835..f5d502d 100644
> > --- a/hw/spitz.c
> > +++ b/hw/spitz.c
> > @@ -1087,6 +1087,7 @@ static const VMStateDescription vmstate_corgi_ssp_regs = {
> >      .minimum_version_id = 1,
> >      .minimum_version_id_old = 1,
> >      .fields = (VMStateField []) {
> > +        VMSTATE_SSI_SLAVE(ssidev, CorgiSSPState),
> >          VMSTATE_UINT32_ARRAY(enable, CorgiSSPState, 3),
> >          VMSTATE_END_OF_LIST(),
> >      }
> > @@ -1115,6 +1116,7 @@ static const VMStateDescription vmstate_spitz_lcdtg_regs = {
> >      .minimum_version_id = 1,
> >      .minimum_version_id_old = 1,
> >      .fields = (VMStateField []) {
> > +        VMSTATE_SSI_SLAVE(ssidev, SpitzLCDTG),
> >          VMSTATE_UINT32(bl_intensity, SpitzLCDTG),
> >          VMSTATE_UINT32(bl_power, SpitzLCDTG),
> >          VMSTATE_END_OF_LIST(),
> > diff --git a/hw/ssi.c b/hw/ssi.c
> > index 35d0a04..2db88fc 100644
> > --- a/hw/ssi.c
> > +++ b/hw/ssi.c
> > @@ -80,6 +80,16 @@ uint32_t ssi_transfer(SSIBus *bus, uint32_t val)
> >      return r;
> >  }
> >
> > +const VMStateDescription vmstate_ssi_slave = {
> > +    .name = "SSISlave",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .minimum_version_id_old = 1,
> > +    .fields      = (VMStateField[]) {
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  static void ssi_slave_register_types(void)
> >  {
> >      type_register_static(&ssi_bus_info);
> > diff --git a/hw/ssi.h b/hw/ssi.h
> > index 06657d7..975f9fb 100644
> > --- a/hw/ssi.h
> > +++ b/hw/ssi.h
> > @@ -38,6 +38,16 @@ struct SSISlave {
> >  #define SSI_SLAVE_FROM_QDEV(dev) DO_UPCAST(SSISlave, qdev, dev)
> >  #define FROM_SSI_SLAVE(type, dev) DO_UPCAST(type, ssidev, dev)
> >
> > +extern const VMStateDescription vmstate_ssi_slave;
> > +
> > +#define VMSTATE_SSI_SLAVE(_field, _state) {                          \
> > +    .name       = (stringify(_field)),                               \
> > +    .size       = sizeof(SSISlave),                                  \
> > +    .vmsd       = &vmstate_ssi_slave,                                \
> > +    .flags      = VMS_STRUCT,                                        \
> > +    .offset     = vmstate_offset_value(_state, _field, SSISlave),    \
> > +}
> > +
> >  DeviceState *ssi_create_slave(SSIBus *bus, const char *name);
> >
> >  /* Master interface.  */
> > diff --git a/hw/stellaris.c b/hw/stellaris.c
> > index 562fbbf..4d26857 100644
> > --- a/hw/stellaris.c
> > +++ b/hw/stellaris.c
> > @@ -1188,6 +1188,7 @@ static const VMStateDescription vmstate_stellaris_ssi_bus = {
> >      .minimum_version_id = 1,
> >      .minimum_version_id_old = 1,
> >      .fields      = (VMStateField[]) {
> > +        VMSTATE_SSI_SLAVE(ssidev, stellaris_ssi_bus_state),
> >          VMSTATE_INT32(current_dev, stellaris_ssi_bus_state),
> >          VMSTATE_END_OF_LIST()
> >      }
> > diff --git a/hw/z2.c b/hw/z2.c
> > index 289cee9..721aaf1 100644
> > --- a/hw/z2.c
> > +++ b/hw/z2.c
> > @@ -165,6 +165,7 @@ static VMStateDescription vmstate_zipit_lcd_state = {
> >      .minimum_version_id = 1,
> >      .minimum_version_id_old = 1,
> >      .fields = (VMStateField[]) {
> > +        VMSTATE_SSI_SLAVE(ssidev, ZipitLCD),
> >          VMSTATE_INT32(selected, ZipitLCD),
> >          VMSTATE_INT32(enabled, ZipitLCD),
> >          VMSTATE_BUFFER(buf, ZipitLCD),
> > --
> > 1.7.0.4
> >
> 
> 
> -- PMM