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/[email protected]/
which led to discussing the pmic that contains the regulators which
serve as power-domain power-supplies:
https://lore.kernel.org/linux-pm/[email protected]/T/
Signed-off-by: Martin Kepplinger <[email protected]>
---
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/[email protected]/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
On Tue, Jul 12, 2022 at 2:19 PM Martin Kepplinger
<[email protected]> 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/[email protected]/
> which led to discussing the pmic that contains the regulators which
> serve as power-domain power-supplies:
> https://lore.kernel.org/linux-pm/[email protected]/T/
I need Ulf to look at this. Ulf?
> Signed-off-by: Martin Kepplinger <[email protected]>
> ---
>
> 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/[email protected]/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
>
On Tue, 12 Jul 2022 at 14:19, Martin Kepplinger
<[email protected]> 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/[email protected]/
> which led to discussing the pmic that contains the regulators which
> serve as power-domain power-supplies:
> https://lore.kernel.org/linux-pm/[email protected]/T/
>
> Signed-off-by: Martin Kepplinger <[email protected]>
> ---
>
> 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/[email protected]/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
Am Montag, dem 18.07.2022 um 12:54 +0200 schrieb Ulf Hansson:
> On Tue, 12 Jul 2022 at 14:19, Martin Kepplinger
> <[email protected]> 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/[email protected]/
> > which led to discussing the pmic that contains the regulators which
> > serve as power-domain power-supplies:
> > https://lore.kernel.org/linux-pm/[email protected]/T/
> >
> > Signed-off-by: Martin Kepplinger <[email protected]>
> > ---
> >
> > 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/[email protected]/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
On Mon, 18 Jul 2022 at 14:08, Martin Kepplinger
<[email protected]> wrote:
>
> Am Montag, dem 18.07.2022 um 12:54 +0200 schrieb Ulf Hansson:
> > On Tue, 12 Jul 2022 at 14:19, Martin Kepplinger
> > <[email protected]> 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/[email protected]/
> > > which led to discussing the pmic that contains the regulators which
> > > serve as power-domain power-supplies:
> > > https://lore.kernel.org/linux-pm/[email protected]/T/
> > >
> > > Signed-off-by: Martin Kepplinger <[email protected]>
> > > ---
> > >
> > > 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/[email protected]/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