Re: [PATCH v2] power: domain: handle power supplies that need interrupts

[Martin Kepplinger <martin.kepplinger@puri.sm>]

Am Montag, dem 18.07.2022 um 12:54 +0200 schrieb Ulf Hansson:
> On Tue, 12 Jul 2022 at 14:19, Martin Kepplinger
> <martin.kepplinger@puri.sm> wrote:
> > 
> > If the power-domains' power-supply node (regulator) needs
> > interrupts to work, the current setup with noirq callbacks cannot
> > work; for example a pmic regulator on i2c, when suspending, usually
> > already
> > times out during suspend_noirq:
> > 
> > [   41.024193] buck4: failed to disable: -ETIMEDOUT
> > 
> > So fix system suspend and resume for these power-domains by using
> > the
> > "outer" suspend/resume callbacks instead. Tested on the imx8mq-
> > librem5
> > board, but by looking at the dts, this will fix imx8mq-evk and
> > possibly
> > other boards too.
> > 
> > Possibly one can find more changes than suspend/resume for this
> > case. They
> > can be added later when testing them.
> > 
> > Initially system suspend problems had been discussed at
> > https://lore.kernel.org/linux-arm-kernel/20211002005954.1367653-8-l.stach@pengutronix.de/
> > which led to discussing the pmic that contains the regulators which
> > serve as power-domain power-supplies:
> > https://lore.kernel.org/linux-pm/573166b75e524517782471c2b7f96e03fd93d175.camel@puri.sm/T/
> > 
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > ---
> > 
> > revision history
> > ----------------
> > v2: (thank you Krzysztof)
> > * rewrite: find possible regulators' interrupts property in parents
> >   instead of inventing a new property.
> > 
> > v1: (initial idea)
> > https://lore.kernel.org/linux-arm-kernel/20220711094549.3445566-1-martin.kepplinger@puri.sm/T/#t
> > 
> > 
> >  drivers/base/power/domain.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/drivers/base/power/domain.c
> > b/drivers/base/power/domain.c
> > index 3e86772d5fac..ca3e3500939d 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -2298,6 +2298,28 @@ static bool genpd_present(const struct
> > generic_pm_domain *genpd)
> >         return ret;
> >  }
> > 
> > +/**
> > + * of_genpd_get_power_supply_irq() - Adjust if power-supply needs
> > interrupts
> > + * @genpd: Pointer to PM domain associated with the PM domain
> > provider.
> > + */
> > +static void of_genpd_get_power_supply_irq(struct generic_pm_domain
> > *pd)
> > +{
> > +       struct device_node *dn;
> > +
> > +       dn = of_parse_phandle(pd->dev.of_node, "power-supply", 0);
> > +       if (!dn)
> > +               return;
> > +
> > +       while ((dn = of_get_next_parent(dn))) {
> > +               if (of_get_property(dn, "interrupts", NULL)) {
> > +                       pd->domain.ops.suspend =
> > genpd_suspend_noirq;
> > +                       pd->domain.ops.resume = genpd_resume_noirq;
> > +                       pd->domain.ops.suspend_noirq = NULL;
> > +                       pd->domain.ops.resume_noirq = NULL;
> > +               }
> > +       }
> > +}
> > +
> >  /**
> >   * of_genpd_add_provider_simple() - Register a simple PM domain
> > provider
> >   * @np: Device node pointer associated with the PM domain
> > provider.
> > @@ -2343,6 +2365,8 @@ int of_genpd_add_provider_simple(struct
> > device_node *np,
> >         genpd->provider = &np->fwnode;
> >         genpd->has_provider = true;
> > 
> > +       of_genpd_get_power_supply_irq(genpd);
> > +
> >         return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
> > @@ -2394,6 +2418,8 @@ int of_genpd_add_provider_onecell(struct
> > device_node *np,
> > 
> >                 genpd->provider = &np->fwnode;
> >                 genpd->has_provider = true;
> > +
> > +               of_genpd_get_power_supply_irq(genpd);
> >         }
> > 
> >         ret = genpd_add_provider(np, data->xlate, data);
> 
> Overall I understand the need for this, but let me suggest a slightly
> different approach to solve this. See below.
> 
> I think the OF parsing looks quite platform specific. Rather than
> adding this in the generic layer of genpd, I suggest that we move the
> OF parsing into the genpd provider code.
> 
> Moreover, to inform genpd that it should use the other set of
> callbacks for system suspend/resume, let's add a new genpd
> configuration bit. The genpd provider should then set the genpd-
> >flag,
> prior to calling pm_genpd_init(), to let it know that it should pick
> the other callbacks.
> 
> Does it make sense?

the provider here would be gpcv2, right? Conceptually I know what you
mean and will try to make it work later. thanks a lot!

> 
> Kind regards
> Uffe