hi Ulf, Lucas and all interested,
This (after a cleanup patch) makes available a new genpd flag
GENPD_FLAG_IRQ_ON in a relatively generic way: genpd providers can set
it when irqs are needed to manage power on/off. Since the main goal
here has been to fix systemd suspend/resume, adjusting these callbacks
is all that's being done when this flag gets set.
And since I'm working on imx8mq, the 3rd patch makes gpcv2 set this new
flag when any power-supply's parent DT node has "interrupts" described.
For i.MX8M* platforms, this should be ok. For other platforms this might
be useful too but needs to be tested.
revision history
----------------
v4: (thank you Ulf and Lucas)
* split up genpd core and gpcv2 changes
* set callbacks inside of pm_genpd_init()
* make flag name and description a bit more generic
* print an error in __genpd_dev_pm_attach() if there a "mismatch"
v3: (thank you Ulf)
* move DT parsing to gpcv2 and create a genpd flag that gets set
https://lore.kernel.org/linux-arm-kernel/[email protected]/
v2: (thank you Krzysztof)
* rewrite: find possible regulators' interrupts property in parents
instead of inventing a new property.
https://lore.kernel.org/linux-arm-kernel/[email protected]/
v1: (initial idea)
https://lore.kernel.org/linux-arm-kernel/[email protected]/T/#t
Martin Kepplinger (3):
PM: domain: fix indentation and use BIT macro for flags
power: domain: handle genpd correctly when needing interrupts
soc: imx: gpcv2: fix suspend/resume by setting GENPD_FLAG_IRQ_ON
drivers/base/power/domain.c | 13 +++++++++++++
drivers/soc/imx/gpcv2.c | 9 +++++++++
include/linux/pm_domain.h | 20 +++++++++++++-------
3 files changed, 35 insertions(+), 7 deletions(-)
--
2.30.2
If for example 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 many other
boards too.
This is designed so that genpd providers just say "this genpd needs
interrupts" (by setting the flag) - without implying an implementation.
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]>
---
drivers/base/power/domain.c | 13 +++++++++++++
include/linux/pm_domain.h | 5 +++++
2 files changed, 18 insertions(+)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 739e52cd4aba..76ea39e960f3 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -130,6 +130,7 @@ static const struct genpd_lock_ops genpd_spin_ops = {
#define genpd_is_active_wakeup(genpd) (genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP)
#define genpd_is_cpu_domain(genpd) (genpd->flags & GENPD_FLAG_CPU_DOMAIN)
#define genpd_is_rpm_always_on(genpd) (genpd->flags & GENPD_FLAG_RPM_ALWAYS_ON)
+#define genpd_irq_on(genpd) (genpd->flags & GENPD_FLAG_IRQ_ON)
static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
const struct generic_pm_domain *genpd)
@@ -2076,6 +2077,13 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
genpd->dev_ops.start = pm_clk_resume;
}
+ if (genpd_irq_on(genpd)) {
+ genpd->domain.ops.suspend = genpd_suspend_noirq;
+ genpd->domain.ops.resume = genpd_resume_noirq;
+ genpd->domain.ops.suspend_noirq = NULL;
+ genpd->domain.ops.resume_noirq = NULL;
+ }
+
/* The always-on governor works better with the corresponding flag. */
if (gov == &pm_domain_always_on_gov)
genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
@@ -2766,6 +2774,11 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
goto err;
dev_gpd_data(dev)->default_pstate = pstate;
}
+
+ if (pd->domain.ops.suspend_noirq && (pd->flags & GENPD_FLAG_IRQ_ON))
+ dev_err(dev, "PM domain %s needs irqs but uses noirq suspend\n",
+ pd->name);
+
return 1;
err:
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 76bc9e3ef5ff..03bb86e43550 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -61,6 +61,10 @@
* GENPD_FLAG_MIN_RESIDENCY: Enable the genpd governor to consider its
* components' next wakeup when determining the
* optimal idle state.
+ *
+ * GENPD_FLAG_IRQ_ON: genpd needs irqs to be able to manage power
+ * on/off. Use the outer suspend/resume callbacks
+ * instead of noirq for example.
*/
#define GENPD_FLAG_PM_CLK BIT(0)
#define GENPD_FLAG_IRQ_SAFE BIT(1)
@@ -69,6 +73,7 @@
#define GENPD_FLAG_CPU_DOMAIN BIT(4)
#define GENPD_FLAG_RPM_ALWAYS_ON BIT(5)
#define GENPD_FLAG_MIN_RESIDENCY BIT(6)
+#define GENPD_FLAG_IRQ_ON BIT(7)
enum gpd_status {
GENPD_STATE_ON = 0, /* PM domain is on */
--
2.30.2
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 <[email protected]>
---
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;
+ }
+ }
+
ret = pm_genpd_init(&domain->genpd, NULL, true);
if (ret) {
dev_err(domain->dev, "Failed to init power domain\n");
--
2.30.2
Use the BIT macro for flags and simply do 2 tags indentation.
Signed-off-by: Martin Kepplinger <[email protected]>
---
include/linux/pm_domain.h | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index ebc351698090..76bc9e3ef5ff 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -8,6 +8,7 @@
#ifndef _LINUX_PM_DOMAIN_H
#define _LINUX_PM_DOMAIN_H
+#include <linux/bits.h>
#include <linux/device.h>
#include <linux/ktime.h>
#include <linux/mutex.h>
@@ -61,13 +62,13 @@
* components' next wakeup when determining the
* optimal idle state.
*/
-#define GENPD_FLAG_PM_CLK (1U << 0)
-#define GENPD_FLAG_IRQ_SAFE (1U << 1)
-#define GENPD_FLAG_ALWAYS_ON (1U << 2)
-#define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3)
-#define GENPD_FLAG_CPU_DOMAIN (1U << 4)
-#define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
-#define GENPD_FLAG_MIN_RESIDENCY (1U << 6)
+#define GENPD_FLAG_PM_CLK BIT(0)
+#define GENPD_FLAG_IRQ_SAFE BIT(1)
+#define GENPD_FLAG_ALWAYS_ON BIT(2)
+#define GENPD_FLAG_ACTIVE_WAKEUP BIT(3)
+#define GENPD_FLAG_CPU_DOMAIN BIT(4)
+#define GENPD_FLAG_RPM_ALWAYS_ON BIT(5)
+#define GENPD_FLAG_MIN_RESIDENCY BIT(6)
enum gpd_status {
GENPD_STATE_ON = 0, /* PM domain is on */
--
2.30.2
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 <[email protected]>
> ---
> 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.
Regards,
Lucas
> ret = pm_genpd_init(&domain->genpd, NULL, true);
> if (ret) {
> dev_err(domain->dev, "Failed to init power domain\n");
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 <[email protected]>
> > ---
> > 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.
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).
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");
>
>
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 <[email protected]>
> > > ---
> > > 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");
> >
> >
>
>