| Message ID | 20231010195209.264757-1-milesg@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] misc/pca9552: Fix for pca9552 not getting reset | expand |
On 10/10/23 21:52, Glenn Miles wrote: > Testing of the pca9552 device on the powernv platform > showed that the reset method was not being called when > an instance of the device was realized. This was causing > the INPUT0/INPUT1 POR values to be incorrect. > > Fixed by overriding the parent pca955x_realize method with a > new pca9552_realize method which first calls > the parent pca955x_realize method followed by the > pca9552_reset function. > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > --- It is good practice to include a changelog after '---' > hw/misc/pca9552.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > index fff19e369a..4e183cc554 100644 > --- a/hw/misc/pca9552.c > +++ b/hw/misc/pca9552.c > @@ -384,6 +384,12 @@ static void pca955x_realize(DeviceState *dev, Error **errp) > qdev_init_gpio_out(dev, s->gpio, k->pin_count); > } > > +static void pca9552_realize(DeviceState *dev, Error **errp) > +{ > + pca955x_realize(dev, errp); > + pca9552_reset(dev); > +} I don't see any change from v2. Thanks, C. > + > static Property pca955x_properties[] = { > DEFINE_PROP_STRING("description", PCA955xState, description), > DEFINE_PROP_END_OF_LIST(), > @@ -417,6 +423,7 @@ static void pca9552_class_init(ObjectClass *oc, void *data) > PCA955xClass *pc = PCA955X_CLASS(oc); > > dc->reset = pca9552_reset; > + dc->realize = pca9552_realize; > dc->vmsd = &pca9552_vmstate; > pc->max_reg = PCA9552_LS3; > pc->pin_count = 16;
On Tue, 2023-10-10 at 21:58 +0200, Cédric Le Goater wrote: > On 10/10/23 21:52, Glenn Miles wrote: > > Testing of the pca9552 device on the powernv platform > > showed that the reset method was not being called when > > an instance of the device was realized. This was causing > > the INPUT0/INPUT1 POR values to be incorrect. > > > > Fixed by overriding the parent pca955x_realize method with a > > new pca9552_realize method which first calls > > the parent pca955x_realize method followed by the > > pca9552_reset function. > > > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > > --- > > It is good practice to include a changelog after '---' Ok, thanks. I'll remember that for next time! > > > > hw/misc/pca9552.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > > index fff19e369a..4e183cc554 100644 > > --- a/hw/misc/pca9552.c > > +++ b/hw/misc/pca9552.c > > @@ -384,6 +384,12 @@ static void pca955x_realize(DeviceState *dev, > > Error **errp) > > qdev_init_gpio_out(dev, s->gpio, k->pin_count); > > } > > > > +static void pca9552_realize(DeviceState *dev, Error **errp) > > +{ > > + pca955x_realize(dev, errp); > > + pca9552_reset(dev); > > +} > > I don't see any change from v2. The change from v2 can be seen below here where I added back the line for setting the dc->reset method, which was removed in v2. Perhaps I misunderstood what you meant by "You need both handlers, a realize and a reset"? You can see below that both the reset and the realize methods are being initialized. Are you taking issue with the realize function calling the reset function? I did this because in my testing I noticed that reset was not getting called at any point. Is the reset function supposed to get called automatically during device realization? It does not seem to be happening. Thanks, Glenn > > Thanks, > > C. > > > + > > static Property pca955x_properties[] = { > > DEFINE_PROP_STRING("description", PCA955xState, description), > > DEFINE_PROP_END_OF_LIST(), > > @@ -417,6 +423,7 @@ static void pca9552_class_init(ObjectClass *oc, > > void *data) > > PCA955xClass *pc = PCA955X_CLASS(oc); > > > > dc->reset = pca9552_reset; > > + dc->realize = pca9552_realize; > > dc->vmsd = &pca9552_vmstate; > > pc->max_reg = PCA9552_LS3; > > pc->pin_count = 16;
On 10/10/2023 20:52, Glenn Miles wrote: > Testing of the pca9552 device on the powernv platform > showed that the reset method was not being called when > an instance of the device was realized. This was causing > the INPUT0/INPUT1 POR values to be incorrect. > > Fixed by overriding the parent pca955x_realize method with a > new pca9552_realize method which first calls > the parent pca955x_realize method followed by the > pca9552_reset function. > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > --- > hw/misc/pca9552.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > index fff19e369a..4e183cc554 100644 > --- a/hw/misc/pca9552.c > +++ b/hw/misc/pca9552.c > @@ -384,6 +384,12 @@ static void pca955x_realize(DeviceState *dev, Error **errp) > qdev_init_gpio_out(dev, s->gpio, k->pin_count); > } > > +static void pca9552_realize(DeviceState *dev, Error **errp) > +{ > + pca955x_realize(dev, errp); > + pca9552_reset(dev); > +} > + > static Property pca955x_properties[] = { > DEFINE_PROP_STRING("description", PCA955xState, description), > DEFINE_PROP_END_OF_LIST(), > @@ -417,6 +423,7 @@ static void pca9552_class_init(ObjectClass *oc, void *data) > PCA955xClass *pc = PCA955X_CLASS(oc); > > dc->reset = pca9552_reset; > + dc->realize = pca9552_realize; > dc->vmsd = &pca9552_vmstate; > pc->max_reg = PCA9552_LS3; > pc->pin_count = 16; The reason that the reset function isn't being called here is because TYPE_I2C_SLAVE is derived from TYPE_DEVICE, and for various historical reasons the DeviceClass reset function is only called for devices that inherit from TYPE_SYS_BUS_DEVICE. Probably the best way to make this work instead of mixing up the reset and realize parts of the object lifecycle is to convert pca9552_reset() to use the new Resettable interface for TYPE_PCA9552: take a look at commit d43e967f69 ("q800-glue.c: convert to Resettable interface") as an example, along with the documentation at https://www.qemu.org/docs/master/devel/reset.html. ATB, Mark.
On Tue, 2023-10-10 at 21:31 +0100, Mark Cave-Ayland wrote: > On 10/10/2023 20:52, Glenn Miles wrote: > > > Testing of the pca9552 device on the powernv platform > > showed that the reset method was not being called when > > an instance of the device was realized. This was causing > > the INPUT0/INPUT1 POR values to be incorrect. > > > > Fixed by overriding the parent pca955x_realize method with a > > new pca9552_realize method which first calls > > the parent pca955x_realize method followed by the > > pca9552_reset function. > > > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > > --- > > hw/misc/pca9552.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > > index fff19e369a..4e183cc554 100644 > > --- a/hw/misc/pca9552.c > > +++ b/hw/misc/pca9552.c > > @@ -384,6 +384,12 @@ static void pca955x_realize(DeviceState *dev, > > Error **errp) > > qdev_init_gpio_out(dev, s->gpio, k->pin_count); > > } > > > > +static void pca9552_realize(DeviceState *dev, Error **errp) > > +{ > > + pca955x_realize(dev, errp); > > + pca9552_reset(dev); > > +} > > + > > static Property pca955x_properties[] = { > > DEFINE_PROP_STRING("description", PCA955xState, description), > > DEFINE_PROP_END_OF_LIST(), > > @@ -417,6 +423,7 @@ static void pca9552_class_init(ObjectClass *oc, > > void *data) > > PCA955xClass *pc = PCA955X_CLASS(oc); > > > > dc->reset = pca9552_reset; > > + dc->realize = pca9552_realize; > > dc->vmsd = &pca9552_vmstate; > > pc->max_reg = PCA9552_LS3; > > pc->pin_count = 16; > > The reason that the reset function isn't being called here is because > TYPE_I2C_SLAVE > is derived from TYPE_DEVICE, and for various historical reasons the > DeviceClass reset > function is only called for devices that inherit from > TYPE_SYS_BUS_DEVICE. > > Probably the best way to make this work instead of mixing up the > reset and realize > parts of the object lifecycle is to convert pca9552_reset() to use > the new Resettable > interface for TYPE_PCA9552: take a look at commit d43e967f69 ("q800- > glue.c: convert > to Resettable interface") as an example, along with the documentation > at > https://www.qemu.org/docs/master/devel/reset.html. > Ahh, that's very helpful. Thanks, Mark! -Glenn > > ATB, > > Mark. >
On 10/10/23 22:35, Miles Glenn wrote: > On Tue, 2023-10-10 at 21:31 +0100, Mark Cave-Ayland wrote: >> On 10/10/2023 20:52, Glenn Miles wrote: >> >>> Testing of the pca9552 device on the powernv platform >>> showed that the reset method was not being called when >>> an instance of the device was realized. This was causing >>> the INPUT0/INPUT1 POR values to be incorrect. >>> >>> Fixed by overriding the parent pca955x_realize method with a >>> new pca9552_realize method which first calls >>> the parent pca955x_realize method followed by the >>> pca9552_reset function. >>> >>> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> >>> --- >>> hw/misc/pca9552.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c >>> index fff19e369a..4e183cc554 100644 >>> --- a/hw/misc/pca9552.c >>> +++ b/hw/misc/pca9552.c >>> @@ -384,6 +384,12 @@ static void pca955x_realize(DeviceState *dev, >>> Error **errp) >>> qdev_init_gpio_out(dev, s->gpio, k->pin_count); >>> } >>> >>> +static void pca9552_realize(DeviceState *dev, Error **errp) >>> +{ >>> + pca955x_realize(dev, errp); >>> + pca9552_reset(dev); >>> +} >>> + >>> static Property pca955x_properties[] = { >>> DEFINE_PROP_STRING("description", PCA955xState, description), >>> DEFINE_PROP_END_OF_LIST(), >>> @@ -417,6 +423,7 @@ static void pca9552_class_init(ObjectClass *oc, >>> void *data) >>> PCA955xClass *pc = PCA955X_CLASS(oc); >>> >>> dc->reset = pca9552_reset; >>> + dc->realize = pca9552_realize; >>> dc->vmsd = &pca9552_vmstate; >>> pc->max_reg = PCA9552_LS3; >>> pc->pin_count = 16; >> >> The reason that the reset function isn't being called here is because >> TYPE_I2C_SLAVE >> is derived from TYPE_DEVICE, and for various historical reasons the >> DeviceClass reset >> function is only called for devices that inherit from >> TYPE_SYS_BUS_DEVICE. >> >> Probably the best way to make this work instead of mixing up the >> reset and realize >> parts of the object lifecycle is to convert pca9552_reset() to use >> the new Resettable >> interface for TYPE_PCA9552: take a look at commit d43e967f69 ("q800- >> glue.c: convert >> to Resettable interface") as an example, along with the documentation >> at >> https://www.qemu.org/docs/master/devel/reset.html. >> > > Ahh, that's very helpful. Thanks, Mark! yes. My bad, I didn't look close enough. Thanks Mark C.
On Tue, 2023-10-10 at 22:41 +0200, Cédric Le Goater wrote: > On 10/10/23 22:35, Miles Glenn wrote: > > On Tue, 2023-10-10 at 21:31 +0100, Mark Cave-Ayland wrote: > > > On 10/10/2023 20:52, Glenn Miles wrote: > > > > > > > Testing of the pca9552 device on the powernv platform > > > > showed that the reset method was not being called when > > > > an instance of the device was realized. This was causing > > > > the INPUT0/INPUT1 POR values to be incorrect. > > > > > > > > Fixed by overriding the parent pca955x_realize method with a > > > > new pca9552_realize method which first calls > > > > the parent pca955x_realize method followed by the > > > > pca9552_reset function. > > > > > > > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> > > > > --- > > > > hw/misc/pca9552.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c > > > > index fff19e369a..4e183cc554 100644 > > > > --- a/hw/misc/pca9552.c > > > > +++ b/hw/misc/pca9552.c > > > > @@ -384,6 +384,12 @@ static void pca955x_realize(DeviceState > > > > *dev, > > > > Error **errp) > > > > qdev_init_gpio_out(dev, s->gpio, k->pin_count); > > > > } > > > > > > > > +static void pca9552_realize(DeviceState *dev, Error **errp) > > > > +{ > > > > + pca955x_realize(dev, errp); > > > > + pca9552_reset(dev); > > > > +} > > > > + > > > > static Property pca955x_properties[] = { > > > > DEFINE_PROP_STRING("description", PCA955xState, > > > > description), > > > > DEFINE_PROP_END_OF_LIST(), > > > > @@ -417,6 +423,7 @@ static void pca9552_class_init(ObjectClass > > > > *oc, > > > > void *data) > > > > PCA955xClass *pc = PCA955X_CLASS(oc); > > > > > > > > dc->reset = pca9552_reset; > > > > + dc->realize = pca9552_realize; > > > > dc->vmsd = &pca9552_vmstate; > > > > pc->max_reg = PCA9552_LS3; > > > > pc->pin_count = 16; > > > > > > The reason that the reset function isn't being called here is > > > because > > > TYPE_I2C_SLAVE > > > is derived from TYPE_DEVICE, and for various historical reasons > > > the > > > DeviceClass reset > > > function is only called for devices that inherit from > > > TYPE_SYS_BUS_DEVICE. > > > > > > Probably the best way to make this work instead of mixing up the > > > reset and realize > > > parts of the object lifecycle is to convert pca9552_reset() to > > > use > > > the new Resettable > > > interface for TYPE_PCA9552: take a look at commit d43e967f69 > > > ("q800- > > > glue.c: convert > > > to Resettable interface") as an example, along with the > > > documentation > > > at > > > https://www.qemu.org/docs/master/devel/reset.html. > > > > > > > Ahh, that's very helpful. Thanks, Mark! > > yes. My bad, I didn't look close enough. Thanks Mark > > C. > Sorry, for the delay... It's now looking like the problem was not in the pca9552 code, but in some new pnv_i2c controller code. I made the changes suggested above and the pca9552 reset function still was not being called when connected to a bus off of the pnv_i2c controller. Howerver, I found that when I start up an arm machine that uses the pca9552, the pca9552_reset function (without any changes) is getting called. If I start up a ppc64 machine that uses the pca9552, through the pnv_i2c controller, then the pca9552_reset function does not get called. Also, the buses connected to the pnv_i2c controller are not getting reset either. So, I'm thinking we're probably missing something in the pnv_i2c code or one of the parent objects. I'll drop this change and start looking there. Thanks, Glenn
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c index fff19e369a..4e183cc554 100644 --- a/hw/misc/pca9552.c +++ b/hw/misc/pca9552.c @@ -384,6 +384,12 @@ static void pca955x_realize(DeviceState *dev, Error **errp) qdev_init_gpio_out(dev, s->gpio, k->pin_count); } +static void pca9552_realize(DeviceState *dev, Error **errp) +{ + pca955x_realize(dev, errp); + pca9552_reset(dev); +} + static Property pca955x_properties[] = { DEFINE_PROP_STRING("description", PCA955xState, description), DEFINE_PROP_END_OF_LIST(), @@ -417,6 +423,7 @@ static void pca9552_class_init(ObjectClass *oc, void *data) PCA955xClass *pc = PCA955X_CLASS(oc); dc->reset = pca9552_reset; + dc->realize = pca9552_realize; dc->vmsd = &pca9552_vmstate; pc->max_reg = PCA9552_LS3; pc->pin_count = 16;
Testing of the pca9552 device on the powernv platform showed that the reset method was not being called when an instance of the device was realized. This was causing the INPUT0/INPUT1 POR values to be incorrect. Fixed by overriding the parent pca955x_realize method with a new pca9552_realize method which first calls the parent pca955x_realize method followed by the pca9552_reset function. Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com> --- hw/misc/pca9552.c | 7 +++++++ 1 file changed, 7 insertions(+)