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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 18 Jul 2022 at 14:08, Martin Kepplinger
<martin.kepplinger@xxxxxxx> wrote:
>
> 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@xxxxxxx> 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@xxxxxxxxxxxxxx/
> > > 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@xxxxxxx/T/
> > >
> > > Signed-off-by: Martin Kepplinger <martin.kepplinger@xxxxxxx>
> > > ---
> > >
> > > 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@xxxxxxx/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?

Correct.

> Conceptually I know what you
> mean and will try to make it work later. thanks a lot!

Great! Feel free to ping me if you need some help to put it together.

Kind regards
Uffe



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux