| Message ID | 1344931787-27056-1-git-send-email-peter.crosthwaite@petalogix.com |
|---|---|
| State | New |
| Headers | show |
"Peter A. G. Crosthwaite" <peter.crosthwaite@petalogix.com> wrote: > Hi All. PMM raised a query on a recent series of mine (the SSI series) about > handling VMSD for devices which define state at multiple levels of the QOM > heirachy. Rather than complicate the discussion over in my series im trying to > start the discussion with an existing subsystem - i2c. This patch is a first > attempt at trying to get the VMSD for generic I2C state factored out of the > individual devices and handled transparently by the super class (I2C_SLAVE). > > I have applied the change to only the one I2C device (max7310). If we were going > to run with this, the change pattern would be applied to all I2C devices. > > This patch is not a merge proposal it is RFC only. > > Please review and let us know if this is flawed or not. What needs to be done to > get this multi-level VMSD going? > > I will use whatever review I get to fix my SSI series as well as fix I2C. > > Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com> This series move data from one part to another (obvious), now the questions: - how do you know that one part of the data relates to the same device on the other side? I don't know about i2c, I am hoping for an answer O;-) This will make that the state of the device will be sent in two chunks, so we should make sure that the two chunks ends in the same device. - This makes the change completely uncompatible. What boards use i2c/ssi? My understanding is that it is only used outside of x86(_64), if so, we can live without backward compatibility. We should increase the version numbers. Something like that on addition. static const VMStateDescription vmstate_max7310 = { .name = "max7310", - .version_id = 0, - .minimum_version_id = 0, - .minimum_version_id_old = 0, + .version_id = 1, + .minimum_version_id = 1, + .minimum_version_id_old = 1, And that should make it. - If you ask me, I would very much preffer something like PCI devices, where the 1st field of any specific device is the i2c part. This would achieve two things: * all i2c devices would have the common fields at the beggining * we sent the data for one device in one go, so we will never had trouble making sure that both devices arrive at the same time, in the right order, etc. - I guess there is same reasy why you want to split the device state, it could be on the other series where I haven't read it though. Later, Juan.
On 14 August 2012 09:27, Juan Quintela <quintela@redhat.com> wrote: > "Peter A. G. Crosthwaite" <peter.crosthwaite@petalogix.com> wrote: >> Hi All. PMM raised a query on a recent series of mine (the SSI series) about >> handling VMSD for devices which define state at multiple levels of the QOM >> heirachy. > - If you ask me, I would very much preffer something like PCI devices, > where the 1st field of any specific device is the i2c part. This > would achieve two things: > * all i2c devices would have the common fields at the beggining > * we sent the data for one device in one go, so we will never had > trouble making sure that both devices arrive at the same time, in > the right order, etc. > > - I guess there is same reasy why you want to split the device state, > it could be on the other series where I haven't read it though. Really I'm just trying to get clarification on how class hierarchies should handle vmstate. At the moment any device which is a subclass of i2c has a VMSTATE_I2C_SLAVE field corresponding to the element in its struct which is its parent object. This seems a bit odd because surely the parent class should be responsible for its own save/load? I'm also not sure we do this consistently through the whole QOM hierarchy. So what I'm after is not necessarily a patch so much as a decision about which way this should be handled. This would probably be good to put in a section of Anthony's QOM style guide... -- PMM
On Tue, Aug 14, 2012 at 6:46 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 14 August 2012 09:27, Juan Quintela <quintela@redhat.com> wrote: >> "Peter A. G. Crosthwaite" <peter.crosthwaite@petalogix.com> wrote: >>> Hi All. PMM raised a query on a recent series of mine (the SSI series) about >>> handling VMSD for devices which define state at multiple levels of the QOM >>> heirachy. > >> - If you ask me, I would very much preffer something like PCI devices, >> where the 1st field of any specific device is the i2c part. This >> would achieve two things: >> * all i2c devices would have the common fields at the beggining >> * we sent the data for one device in one go, so we will never had >> trouble making sure that both devices arrive at the same time, in >> the right order, etc. >> >> - I guess there is same reasy why you want to split the device state, >> it could be on the other series where I haven't read it though. So this is exactly what I have done in the SSI. Correct me if I am wrong but it is the same setup as PCI where the VMSTATE_PCI_DEVICE (VMSTATE_SSI_SLAVE in my case) is the first field. All I need to do is bump version numbers? Regards, Peter > > Really I'm just trying to get clarification on how class hierarchies should > handle vmstate. At the moment any device which is a subclass of i2c > has a VMSTATE_I2C_SLAVE field corresponding to the element in > its struct which is its parent object. This seems a bit odd because > surely the parent class should be responsible for its own save/load? Yes I agree. Means you have to define the super class in two places rather than one (one in the class definition and one in the VMSD). The problem is if people stick to the guns over backwards compatibility then this proposed cleanup will never happen. So unless we can fix it globally and bite the bullet on a pritty much a global version number bump, then we might as well stick with the current system for the sake of consistency rather than PCI and I2C working one way and SSI using something completely different. > I'm also not sure we do this consistently through the whole QOM > hierarchy. > Ditto on getting consistency going, someone is going to have to sign off on a backwards incompatibility. Regards, Peter > So what I'm after is not necessarily a patch so much as a decision about > which way this should be handled. This would probably be good to put > in a section of Anthony's QOM style guide... > > -- PMM
Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote: > On Tue, Aug 14, 2012 at 6:46 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 14 August 2012 09:27, Juan Quintela <quintela@redhat.com> wrote: >>> "Peter A. G. Crosthwaite" <peter.crosthwaite@petalogix.com> wrote: >>>> Hi All. PMM raised a query on a recent series of mine (the SSI series) about >>>> handling VMSD for devices which define state at multiple levels of the QOM >>>> heirachy. >> >>> - If you ask me, I would very much preffer something like PCI devices, >>> where the 1st field of any specific device is the i2c part. This >>> would achieve two things: >>> * all i2c devices would have the common fields at the beggining >>> * we sent the data for one device in one go, so we will never had >>> trouble making sure that both devices arrive at the same time, in >>> the right order, etc. >>> >>> - I guess there is same reasy why you want to split the device state, >>> it could be on the other series where I haven't read it though. > > So this is exactly what I have done in the SSI. Correct me if I am > wrong but it is the same setup as PCI where the VMSTATE_PCI_DEVICE > (VMSTATE_SSI_SLAVE in my case) is the first field. All I need to do is > bump version numbers? I think so. What boards normally use SSI? Later, Juan.
>>>> >>>> - I guess there is same reasy why you want to split the device state, >>>> it could be on the other series where I haven't read it though. >> >> So this is exactly what I have done in the SSI. Correct me if I am >> wrong but it is the same setup as PCI where the VMSTATE_PCI_DEVICE >> (VMSTATE_SSI_SLAVE in my case) is the first field. All I need to do is >> bump version numbers? > > I think so. What boards normally use SSI? > Tracing all the calls to ssi_create_bus() at the moment we have stellaris, spitz, highbank, gumstix, mainstone, z2, tosa & collie. Regards, Peter > Later, Juan.
Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote: >>>>> >>>>> - I guess there is same reasy why you want to split the device state, >>>>> it could be on the other series where I haven't read it though. >>> >>> So this is exactly what I have done in the SSI. Correct me if I am >>> wrong but it is the same setup as PCI where the VMSTATE_PCI_DEVICE >>> (VMSTATE_SSI_SLAVE in my case) is the first field. All I need to do is >>> bump version numbers? >> >> I think so. What boards normally use SSI? >> > > Tracing all the calls to ssi_create_bus() at the moment we have > stellaris, spitz, highbank, gumstix, mainstone, z2, tosa & collie. Then we can change it without regards to backwards compatibility (migration is still not working correctly there), so backwards compatibility is the less of the troubles. Later, Juan.
diff --git a/hw/i2c.c b/hw/i2c.c index 296bece..17e1633 100644 --- a/hw/i2c.c +++ b/hw/i2c.c @@ -207,6 +207,8 @@ static int i2c_slave_qdev_init(DeviceState *dev) I2CSlave *s = I2C_SLAVE_FROM_QDEV(dev); I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(s); + vmstate_register(NULL, 0, &vmstate_i2c_slave, s); + return sc->init(s); } diff --git a/hw/i2c.h b/hw/i2c.h index 0f5682b..5b75ecc 100644 --- a/hw/i2c.h +++ b/hw/i2c.h @@ -81,12 +81,4 @@ void lm832x_key_event(DeviceState *dev, int key, int state); extern const VMStateDescription vmstate_i2c_slave; -#define VMSTATE_I2C_SLAVE(_field, _state) { \ - .name = (stringify(_field)), \ - .size = sizeof(I2CSlave), \ - .vmsd = &vmstate_i2c_slave, \ - .flags = VMS_STRUCT, \ - .offset = vmstate_offset_value(_state, _field, I2CSlave), \ -} - #endif diff --git a/hw/max7310.c b/hw/max7310.c index 1ed18ba..9375691 100644 --- a/hw/max7310.c +++ b/hw/max7310.c @@ -156,7 +156,6 @@ static const VMStateDescription vmstate_max7310 = { VMSTATE_UINT8(polarity, MAX7310State), VMSTATE_UINT8(status, MAX7310State), VMSTATE_UINT8(command, MAX7310State), - VMSTATE_I2C_SLAVE(i2c, MAX7310State), VMSTATE_END_OF_LIST() } };
Hi All. PMM raised a query on a recent series of mine (the SSI series) about handling VMSD for devices which define state at multiple levels of the QOM heirachy. Rather than complicate the discussion over in my series im trying to start the discussion with an existing subsystem - i2c. This patch is a first attempt at trying to get the VMSD for generic I2C state factored out of the individual devices and handled transparently by the super class (I2C_SLAVE). I have applied the change to only the one I2C device (max7310). If we were going to run with this, the change pattern would be applied to all I2C devices. This patch is not a merge proposal it is RFC only. Please review and let us know if this is flawed or not. What needs to be done to get this multi-level VMSD going? I will use whatever review I get to fix my SSI series as well as fix I2C. Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com> --- hw/i2c.c | 2 ++ hw/i2c.h | 8 -------- hw/max7310.c | 1 - 3 files changed, 2 insertions(+), 9 deletions(-)