Re: [PATCH v3] misc/pca9552: Fix for pca9552 not getting reset

[Miles Glenn <milesg@linux.vnet.ibm.com>]

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