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

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

[Martin Kepplinger]

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);
-- 
2.30.2

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

[Rafael J. Wysocki]

On Tue, Jul 12, 2022 at 2:19 PM 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/

I need Ulf to look at this.  Ulf?

> 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);
> --
> 2.30.2
>

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

[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?

Kind regards
Uffe

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

[Martin Kepplinger]

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

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

[Ulf Hansson]

On Mon, 18 Jul 2022 at 14:08, Martin Kepplinger
<martin.kepplinger@puri.sm> 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@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?

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