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

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

Posted by Glenn Miles 6 months, 3 weeks ago

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;
-- 
2.31.1

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

Posted by Mark Cave-Ayland 6 months, 3 weeks ago

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.

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

Posted by Miles Glenn 6 months, 3 weeks ago

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.
>

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

Posted by Cédric Le Goater 6 months, 3 weeks ago

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.

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

Posted by Miles Glenn 6 months, 2 weeks ago

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

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

Posted by Cédric Le Goater 6 months, 3 weeks ago

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;

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

Posted by Miles Glenn 6 months, 3 weeks ago

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;