Re: [PATCH v4 3/3] soc: imx: gpcv2: fix suspend/resume by setting GENPD_FLAG_IRQ_ON

[Lucas Stach <l.stach@pengutronix.de>]

Am Mittwoch, dem 20.07.2022 um 10:05 +0200 schrieb Martin Kepplinger:
> Am Mittwoch, dem 20.07.2022 um 09:53 +0200 schrieb Lucas Stach:
> > Am Mittwoch, dem 20.07.2022 um 06:34 +0200 schrieb Martin Kepplinger:
> > > For boards that use power-domains' power-supplies that need
> > > interrupts
> > > to work (like regulator over i2c), set GENPD_FLAG_IRQ_ON.
> > > This will tell genpd to adjust accordingly. Currently it "only"
> > > sets the
> > > correct suspend/resume callbacks.
> > > 
> > > This fixes suspend/resume on imx8mq-librem5 boards (tested) and
> > > imx8mq-evk (by looking at dts) and possibly more.
> > > 
> > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > > ---
> > >  drivers/soc/imx/gpcv2.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > > index 85aa86e1338a..46d2ead2352b 100644
> > > --- a/drivers/soc/imx/gpcv2.c
> > > +++ b/drivers/soc/imx/gpcv2.c
> > > @@ -1303,6 +1303,7 @@ static const struct imx_pgc_domain_data
> > > imx8mn_pgc_domain_data = {
> > >  static int imx_pgc_domain_probe(struct platform_device *pdev)
> > >  {
> > >         struct imx_pgc_domain *domain = pdev->dev.platform_data;
> > > +       struct device_node *dn;
> > >         int ret;
> > >  
> > >         domain->dev = &pdev->dev;
> > > @@ -1333,6 +1334,14 @@ static int imx_pgc_domain_probe(struct
> > > platform_device *pdev)
> > >                 regmap_update_bits(domain->regmap, domain->regs-
> > > > map,
> > >                                    domain->bits.map, domain-
> > > > bits.map);
> > >  
> > > +       dn = of_parse_phandle(domain->dev->of_node, "power-supply",
> > > 0);
> > > +       if (dn) {
> > > +               while ((dn = of_get_next_parent(dn))) {
> > > +                       if (of_get_property(dn, "interrupts",
> > > NULL))
> > > +                               domain->genpd.flags |=
> > > GENPD_FLAG_IRQ_ON;
> > > +               }
> > > +       }
> > > +
> > While I understand the intention, I think the DT walking is overkill.
> > I
> > believe that there are no cases where we have a external regulator
> > attached to the PD and the devices in the domain needing noirq
> > support.
> > I think it's sufficient to simply set the IRQ_ON flag based on
> > presence
> > of the power-supply property on the domain DT node.
> 
> Are you sure? Can't boards just *describe* a power-supply that doesn't
> really do much, where noirq would work? looking for "interrupts" in any
> parent feels very stable and makes sure we only change behaviour when
> really needed. But for the boards I'm looking at, I have to admit it
> wouldn't change anything afaik. So if you insist, I'll happily remove
> that.
> 
I'm pretty sure that this holds for all boards. Yes, it might introduce
some more runtime changes than your option, but it will be more
consistent.

One could possibly have a simple GPIO regulator, which could work in
noirq if the GPIO is internal MMIO, but it already breaks when the GPIO
is from an i2c attached GPIO expander. This might even be a good
example where your DT parsing breaks: a GPIO regulator is not
necessarily a child device of the i2c GPIO expander, so the DT walking
will miss that IRQs need to be functional in order to toggle the GPIO.

Just keying the IRQ_ON flag on the presence of the power-supply
property will have less surprises, I think.

> 
> 
> Also, I forgot to say earlier: We could even add "if not regulator-
> always-on" to the DT parsing above, because in that case noirq is fine
> even for external regulators. Should I add that? I'd like as little
> runtime change as possible so I would add that (and keep the
> "interrupts" search above for the same reason). 
> 
Yea, one could make this even more complex to preserve the current
behavior as much as possible, but I just don't think that the current
behavior is relevant enough to warrant the complexity and possible
inconsistent behavior on different systems.

Thanks for working on this!

Regards,
Lucas

> thanks for looking at this,
> 
>                              martin
> 
> 
> > 
> > Regards,
> > Lucas
> > 
> > >         ret = pm_genpd_init(&domain->genpd, NULL, true);
> > >         if (ret) {
> > >                 dev_err(domain->dev, "Failed to init power
> > > domain\n");
> > 
> > 
> 
>