* [PATCH v6 0/2] power: domain: handle power supplies that need interrupts @ 2022-07-26 8:32 ` Martin Kepplinger 0 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-07-26 8:32 UTC (permalink / raw) To: rafael, khilman, ulf.hansson, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel Cc: kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel, Martin Kepplinger hi Ulf, Lucas and all interested, This 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 2nd patch makes gpcv2 set this new flag when a power domain has a power-supply described in DT. For i.MX8M* platforms, this should be ok. For other platforms this might be useful too but needs to be tested. revision history ---------------- v6: (thank you Ulf and Lucas) * drop the cleanup patch to use BIT() * use of_property_read_bool() to find the power-supply * clean up the callback assignment * remove the wrong error printing (flag-check can be added later to *drivers* that have noirq callbacks) v5: (thank you Lucas) * simplify gpcv2 code: just set GENPD_FLAG_IRQ_ON when a power-supply is present https://lore.kernel.org/linux-arm-kernel/20220721043608.1527686-1-martin.kepplinger@puri.sm/ 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" https://lore.kernel.org/linux-arm-kernel/20220720043444.1289952-1-martin.kepplinger@puri.sm/T/#t v3: (thank you Ulf) * move DT parsing to gpcv2 and create a genpd flag that gets set https://lore.kernel.org/linux-arm-kernel/20220718210302.674897-1-martin.kepplinger@puri.sm/ 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/20220712121832.3659769-1-martin.kepplinger@puri.sm/ v1: (initial idea) https://lore.kernel.org/linux-arm-kernel/20220711094549.3445566-1-martin.kepplinger@puri.sm/T/#t Martin Kepplinger (2): 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 | 3 +++ include/linux/pm_domain.h | 5 +++++ 3 files changed, 19 insertions(+), 2 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v6 0/2] power: domain: handle power supplies that need interrupts @ 2022-07-26 8:32 ` Martin Kepplinger 0 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-07-26 8:32 UTC (permalink / raw) To: rafael, khilman, ulf.hansson, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel Cc: kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel, Martin Kepplinger hi Ulf, Lucas and all interested, This 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 2nd patch makes gpcv2 set this new flag when a power domain has a power-supply described in DT. For i.MX8M* platforms, this should be ok. For other platforms this might be useful too but needs to be tested. revision history ---------------- v6: (thank you Ulf and Lucas) * drop the cleanup patch to use BIT() * use of_property_read_bool() to find the power-supply * clean up the callback assignment * remove the wrong error printing (flag-check can be added later to *drivers* that have noirq callbacks) v5: (thank you Lucas) * simplify gpcv2 code: just set GENPD_FLAG_IRQ_ON when a power-supply is present https://lore.kernel.org/linux-arm-kernel/20220721043608.1527686-1-martin.kepplinger@puri.sm/ 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" https://lore.kernel.org/linux-arm-kernel/20220720043444.1289952-1-martin.kepplinger@puri.sm/T/#t v3: (thank you Ulf) * move DT parsing to gpcv2 and create a genpd flag that gets set https://lore.kernel.org/linux-arm-kernel/20220718210302.674897-1-martin.kepplinger@puri.sm/ 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/20220712121832.3659769-1-martin.kepplinger@puri.sm/ v1: (initial idea) https://lore.kernel.org/linux-arm-kernel/20220711094549.3445566-1-martin.kepplinger@puri.sm/T/#t Martin Kepplinger (2): 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 | 3 +++ include/linux/pm_domain.h | 5 +++++ 3 files changed, 19 insertions(+), 2 deletions(-) -- 2.30.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2022-07-26 8:32 ` Martin Kepplinger @ 2022-07-26 8:32 ` Martin Kepplinger -1 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-07-26 8:32 UTC (permalink / raw) To: rafael, khilman, ulf.hansson, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel Cc: kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel, Martin Kepplinger 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/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> --- drivers/base/power/domain.c | 13 +++++++++++-- include/linux/pm_domain.h | 5 +++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 5a2e0232862e..58376752a4de 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) @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct generic_pm_domain *genpd, genpd->domain.ops.runtime_suspend = genpd_runtime_suspend; genpd->domain.ops.runtime_resume = genpd_runtime_resume; genpd->domain.ops.prepare = genpd_prepare; - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; - genpd->domain.ops.resume_noirq = genpd_resume_noirq; + + if (genpd_irq_on(genpd)) { + genpd->domain.ops.suspend = genpd_suspend_noirq; + genpd->domain.ops.resume = genpd_resume_noirq; + } else { + genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; + genpd->domain.ops.resume_noirq = genpd_resume_noirq; + } + genpd->domain.ops.freeze_noirq = genpd_freeze_noirq; genpd->domain.ops.thaw_noirq = genpd_thaw_noirq; genpd->domain.ops.poweroff_noirq = genpd_poweroff_noirq; @@ -2769,6 +2777,7 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, goto err; dev_gpd_data(dev)->default_pstate = pstate; } + return 1; err: diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index ebc351698090..9cdedbc21158 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -60,6 +60,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 (1U << 0) #define GENPD_FLAG_IRQ_SAFE (1U << 1) @@ -68,6 +72,7 @@ #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_IRQ_ON (1U << 7) enum gpd_status { GENPD_STATE_ON = 0, /* PM domain is on */ -- 2.30.2 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2022-07-26 8:32 ` Martin Kepplinger 0 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-07-26 8:32 UTC (permalink / raw) To: rafael, khilman, ulf.hansson, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel Cc: kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel, Martin Kepplinger 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/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> --- drivers/base/power/domain.c | 13 +++++++++++-- include/linux/pm_domain.h | 5 +++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 5a2e0232862e..58376752a4de 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) @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct generic_pm_domain *genpd, genpd->domain.ops.runtime_suspend = genpd_runtime_suspend; genpd->domain.ops.runtime_resume = genpd_runtime_resume; genpd->domain.ops.prepare = genpd_prepare; - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; - genpd->domain.ops.resume_noirq = genpd_resume_noirq; + + if (genpd_irq_on(genpd)) { + genpd->domain.ops.suspend = genpd_suspend_noirq; + genpd->domain.ops.resume = genpd_resume_noirq; + } else { + genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; + genpd->domain.ops.resume_noirq = genpd_resume_noirq; + } + genpd->domain.ops.freeze_noirq = genpd_freeze_noirq; genpd->domain.ops.thaw_noirq = genpd_thaw_noirq; genpd->domain.ops.poweroff_noirq = genpd_poweroff_noirq; @@ -2769,6 +2777,7 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, goto err; dev_gpd_data(dev)->default_pstate = pstate; } + return 1; err: diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index ebc351698090..9cdedbc21158 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -60,6 +60,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 (1U << 0) #define GENPD_FLAG_IRQ_SAFE (1U << 1) @@ -68,6 +72,7 @@ #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_IRQ_ON (1U << 7) enum gpd_status { GENPD_STATE_ON = 0, /* PM domain is on */ -- 2.30.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2022-07-26 8:32 ` Martin Kepplinger @ 2022-07-26 15:07 ` Ulf Hansson -1 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2022-07-26 15:07 UTC (permalink / raw) To: Martin Kepplinger Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > 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/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> > --- > drivers/base/power/domain.c | 13 +++++++++++-- > include/linux/pm_domain.h | 5 +++++ > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 5a2e0232862e..58376752a4de 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) > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct generic_pm_domain *genpd, > genpd->domain.ops.runtime_suspend = genpd_runtime_suspend; > genpd->domain.ops.runtime_resume = genpd_runtime_resume; > genpd->domain.ops.prepare = genpd_prepare; > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > + > + if (genpd_irq_on(genpd)) { > + genpd->domain.ops.suspend = genpd_suspend_noirq; > + genpd->domain.ops.resume = genpd_resume_noirq; > + } else { > + genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > + genpd->domain.ops.resume_noirq = genpd_resume_noirq; As we discussed previously, I am thinking that it may be better to move to using genpd->domain.ops.suspend_late and genpd->domain.ops.resume_early instead. Beside this, I think the $subject patch looks good to me. [...] Kind regards Uffe ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2022-07-26 15:07 ` Ulf Hansson 0 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2022-07-26 15:07 UTC (permalink / raw) To: Martin Kepplinger Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > 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/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> > --- > drivers/base/power/domain.c | 13 +++++++++++-- > include/linux/pm_domain.h | 5 +++++ > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 5a2e0232862e..58376752a4de 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) > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct generic_pm_domain *genpd, > genpd->domain.ops.runtime_suspend = genpd_runtime_suspend; > genpd->domain.ops.runtime_resume = genpd_runtime_resume; > genpd->domain.ops.prepare = genpd_prepare; > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > + > + if (genpd_irq_on(genpd)) { > + genpd->domain.ops.suspend = genpd_suspend_noirq; > + genpd->domain.ops.resume = genpd_resume_noirq; > + } else { > + genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > + genpd->domain.ops.resume_noirq = genpd_resume_noirq; As we discussed previously, I am thinking that it may be better to move to using genpd->domain.ops.suspend_late and genpd->domain.ops.resume_early instead. Beside this, I think the $subject patch looks good to me. [...] Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2022-07-26 15:07 ` Ulf Hansson @ 2022-07-26 18:33 ` Rafael J. Wysocki -1 siblings, 0 replies; 32+ messages in thread From: Rafael J. Wysocki @ 2022-07-26 18:33 UTC (permalink / raw) To: Ulf Hansson Cc: Martin Kepplinger, Rafael J. Wysocki, Kevin Hilman, Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam, Pavel Machek, kernel, dl-linux-imx, Mark Brown, Lucas Stach, aford173, Linux PM, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux ARM On Tue, Jul 26, 2022 at 5:07 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > 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/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> > > --- > > drivers/base/power/domain.c | 13 +++++++++++-- > > include/linux/pm_domain.h | 5 +++++ > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 5a2e0232862e..58376752a4de 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) > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct generic_pm_domain *genpd, > > genpd->domain.ops.runtime_suspend = genpd_runtime_suspend; > > genpd->domain.ops.runtime_resume = genpd_runtime_resume; > > genpd->domain.ops.prepare = genpd_prepare; > > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > + > > + if (genpd_irq_on(genpd)) { > > + genpd->domain.ops.suspend = genpd_suspend_noirq; > > + genpd->domain.ops.resume = genpd_resume_noirq; > > + } else { > > + genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > + genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > As we discussed previously, I am thinking that it may be better to > move to using genpd->domain.ops.suspend_late and > genpd->domain.ops.resume_early instead. Should I take this as a change request? > Beside this, I think the $subject patch looks good to me. Or not? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2022-07-26 18:33 ` Rafael J. Wysocki 0 siblings, 0 replies; 32+ messages in thread From: Rafael J. Wysocki @ 2022-07-26 18:33 UTC (permalink / raw) To: Ulf Hansson Cc: Martin Kepplinger, Rafael J. Wysocki, Kevin Hilman, Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam, Pavel Machek, kernel, dl-linux-imx, Mark Brown, Lucas Stach, aford173, Linux PM, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux ARM On Tue, Jul 26, 2022 at 5:07 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > 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/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> > > --- > > drivers/base/power/domain.c | 13 +++++++++++-- > > include/linux/pm_domain.h | 5 +++++ > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 5a2e0232862e..58376752a4de 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) > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct generic_pm_domain *genpd, > > genpd->domain.ops.runtime_suspend = genpd_runtime_suspend; > > genpd->domain.ops.runtime_resume = genpd_runtime_resume; > > genpd->domain.ops.prepare = genpd_prepare; > > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > + > > + if (genpd_irq_on(genpd)) { > > + genpd->domain.ops.suspend = genpd_suspend_noirq; > > + genpd->domain.ops.resume = genpd_resume_noirq; > > + } else { > > + genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > + genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > As we discussed previously, I am thinking that it may be better to > move to using genpd->domain.ops.suspend_late and > genpd->domain.ops.resume_early instead. Should I take this as a change request? > Beside this, I think the $subject patch looks good to me. Or not? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2022-07-26 18:33 ` Rafael J. Wysocki @ 2022-07-28 9:19 ` Ulf Hansson -1 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2022-07-28 9:19 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Martin Kepplinger, Kevin Hilman, Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam, Pavel Machek, kernel, dl-linux-imx, Mark Brown, Lucas Stach, aford173, Linux PM, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux ARM On Tue, 26 Jul 2022 at 20:33, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Jul 26, 2022 at 5:07 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > <martin.kepplinger@puri.sm> wrote: > > > > > > 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/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> > > > --- > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > include/linux/pm_domain.h | 5 +++++ > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > > index 5a2e0232862e..58376752a4de 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) > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct generic_pm_domain *genpd, > > > genpd->domain.ops.runtime_suspend = genpd_runtime_suspend; > > > genpd->domain.ops.runtime_resume = genpd_runtime_resume; > > > genpd->domain.ops.prepare = genpd_prepare; > > > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > > + > > > + if (genpd_irq_on(genpd)) { > > > + genpd->domain.ops.suspend = genpd_suspend_noirq; > > > + genpd->domain.ops.resume = genpd_resume_noirq; > > > + } else { > > > + genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > > + genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > > > As we discussed previously, I am thinking that it may be better to > > move to using genpd->domain.ops.suspend_late and > > genpd->domain.ops.resume_early instead. > > Should I take this as a change request? Yes. > > > Beside this, I think the $subject patch looks good to me. > > Or not? The patch needs to be updated before I give it my blessing, but the only changes that are needed are the minor things I pointed out above. Sorry if it was not clear enough. Kind regards Uffe ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2022-07-28 9:19 ` Ulf Hansson 0 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2022-07-28 9:19 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Martin Kepplinger, Kevin Hilman, Rob Herring, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Fabio Estevam, Pavel Machek, kernel, dl-linux-imx, Mark Brown, Lucas Stach, aford173, Linux PM, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux ARM On Tue, 26 Jul 2022 at 20:33, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Jul 26, 2022 at 5:07 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > <martin.kepplinger@puri.sm> wrote: > > > > > > 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/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> > > > --- > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > include/linux/pm_domain.h | 5 +++++ > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > > index 5a2e0232862e..58376752a4de 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) > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct generic_pm_domain *genpd, > > > genpd->domain.ops.runtime_suspend = genpd_runtime_suspend; > > > genpd->domain.ops.runtime_resume = genpd_runtime_resume; > > > genpd->domain.ops.prepare = genpd_prepare; > > > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > > + > > > + if (genpd_irq_on(genpd)) { > > > + genpd->domain.ops.suspend = genpd_suspend_noirq; > > > + genpd->domain.ops.resume = genpd_resume_noirq; > > > + } else { > > > + genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > > + genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > > > As we discussed previously, I am thinking that it may be better to > > move to using genpd->domain.ops.suspend_late and > > genpd->domain.ops.resume_early instead. > > Should I take this as a change request? Yes. > > > Beside this, I think the $subject patch looks good to me. > > Or not? The patch needs to be updated before I give it my blessing, but the only changes that are needed are the minor things I pointed out above. Sorry if it was not clear enough. Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2022-07-26 15:07 ` Ulf Hansson @ 2022-08-19 9:17 ` Martin Kepplinger -1 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-08-19 9:17 UTC (permalink / raw) To: Ulf Hansson Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > 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/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> > > --- > > drivers/base/power/domain.c | 13 +++++++++++-- > > include/linux/pm_domain.h | 5 +++++ > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/base/power/domain.c > > b/drivers/base/power/domain.c > > index 5a2e0232862e..58376752a4de 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) > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct generic_pm_domain > > *genpd, > > genpd->domain.ops.runtime_suspend = genpd_runtime_suspend; > > genpd->domain.ops.runtime_resume = genpd_runtime_resume; > > genpd->domain.ops.prepare = genpd_prepare; > > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > + > > + if (genpd_irq_on(genpd)) { > > + genpd->domain.ops.suspend = genpd_suspend_noirq; > > + genpd->domain.ops.resume = genpd_resume_noirq; > > + } else { > > + genpd->domain.ops.suspend_noirq = > > genpd_suspend_noirq; > > + genpd->domain.ops.resume_noirq = > > genpd_resume_noirq; > > As we discussed previously, I am thinking that it may be better to > move to using genpd->domain.ops.suspend_late and > genpd->domain.ops.resume_early instead. Wouldn't that better be a separate patch (on top)? Do you really want me to change the current behaviour (default case) to from noirq to late? Then I'll resend this series with such a patch added. thanks, martin > > Beside this, I think the $subject patch looks good to me. > > [...] > > Kind regards > Uffe ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2022-08-19 9:17 ` Martin Kepplinger 0 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-08-19 9:17 UTC (permalink / raw) To: Ulf Hansson Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > 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/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> > > --- > > drivers/base/power/domain.c | 13 +++++++++++-- > > include/linux/pm_domain.h | 5 +++++ > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/base/power/domain.c > > b/drivers/base/power/domain.c > > index 5a2e0232862e..58376752a4de 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) > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct generic_pm_domain > > *genpd, > > genpd->domain.ops.runtime_suspend = genpd_runtime_suspend; > > genpd->domain.ops.runtime_resume = genpd_runtime_resume; > > genpd->domain.ops.prepare = genpd_prepare; > > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > + > > + if (genpd_irq_on(genpd)) { > > + genpd->domain.ops.suspend = genpd_suspend_noirq; > > + genpd->domain.ops.resume = genpd_resume_noirq; > > + } else { > > + genpd->domain.ops.suspend_noirq = > > genpd_suspend_noirq; > > + genpd->domain.ops.resume_noirq = > > genpd_resume_noirq; > > As we discussed previously, I am thinking that it may be better to > move to using genpd->domain.ops.suspend_late and > genpd->domain.ops.resume_early instead. Wouldn't that better be a separate patch (on top)? Do you really want me to change the current behaviour (default case) to from noirq to late? Then I'll resend this series with such a patch added. thanks, martin > > Beside this, I think the $subject patch looks good to me. > > [...] > > Kind regards > Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2022-08-19 9:17 ` Martin Kepplinger @ 2022-08-19 14:53 ` Ulf Hansson -1 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2022-08-19 14:53 UTC (permalink / raw) To: Martin Kepplinger Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > <martin.kepplinger@puri.sm> wrote: > > > > > > 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/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> > > > --- > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > include/linux/pm_domain.h | 5 +++++ > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/base/power/domain.c > > > b/drivers/base/power/domain.c > > > index 5a2e0232862e..58376752a4de 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) > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct generic_pm_domain > > > *genpd, > > > genpd->domain.ops.runtime_suspend = genpd_runtime_suspend; > > > genpd->domain.ops.runtime_resume = genpd_runtime_resume; > > > genpd->domain.ops.prepare = genpd_prepare; > > > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > > + > > > + if (genpd_irq_on(genpd)) { > > > + genpd->domain.ops.suspend = genpd_suspend_noirq; > > > + genpd->domain.ops.resume = genpd_resume_noirq; > > > + } else { > > > + genpd->domain.ops.suspend_noirq = > > > genpd_suspend_noirq; > > > + genpd->domain.ops.resume_noirq = > > > genpd_resume_noirq; > > > > As we discussed previously, I am thinking that it may be better to > > move to using genpd->domain.ops.suspend_late and > > genpd->domain.ops.resume_early instead. > > Wouldn't that better be a separate patch (on top)? Do you really want > me to change the current behaviour (default case) to from noirq to > late? Then I'll resend this series with such a patch added. Sorry, I wasn't clear enough, the default behaviour should remain as is. What I meant was, when genpd_irq_on() is true, we should use the genpd->domain.ops.suspend_late and genpd->domain.ops.resume_early. Kind regards Uffe ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2022-08-19 14:53 ` Ulf Hansson 0 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2022-08-19 14:53 UTC (permalink / raw) To: Martin Kepplinger Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > <martin.kepplinger@puri.sm> wrote: > > > > > > 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/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> > > > --- > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > include/linux/pm_domain.h | 5 +++++ > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/base/power/domain.c > > > b/drivers/base/power/domain.c > > > index 5a2e0232862e..58376752a4de 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) > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct generic_pm_domain > > > *genpd, > > > genpd->domain.ops.runtime_suspend = genpd_runtime_suspend; > > > genpd->domain.ops.runtime_resume = genpd_runtime_resume; > > > genpd->domain.ops.prepare = genpd_prepare; > > > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > > + > > > + if (genpd_irq_on(genpd)) { > > > + genpd->domain.ops.suspend = genpd_suspend_noirq; > > > + genpd->domain.ops.resume = genpd_resume_noirq; > > > + } else { > > > + genpd->domain.ops.suspend_noirq = > > > genpd_suspend_noirq; > > > + genpd->domain.ops.resume_noirq = > > > genpd_resume_noirq; > > > > As we discussed previously, I am thinking that it may be better to > > move to using genpd->domain.ops.suspend_late and > > genpd->domain.ops.resume_early instead. > > Wouldn't that better be a separate patch (on top)? Do you really want > me to change the current behaviour (default case) to from noirq to > late? Then I'll resend this series with such a patch added. Sorry, I wasn't clear enough, the default behaviour should remain as is. What I meant was, when genpd_irq_on() is true, we should use the genpd->domain.ops.suspend_late and genpd->domain.ops.resume_early. Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2022-08-19 14:53 ` Ulf Hansson @ 2022-08-22 8:38 ` Martin Kepplinger -1 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-08-22 8:38 UTC (permalink / raw) To: Ulf Hansson Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > 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/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> > > > > --- > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > include/linux/pm_domain.h | 5 +++++ > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > b/drivers/base/power/domain.c > > > > index 5a2e0232862e..58376752a4de 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) > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > generic_pm_domain > > > > *genpd, > > > > genpd->domain.ops.runtime_suspend = > > > > genpd_runtime_suspend; > > > > genpd->domain.ops.runtime_resume = > > > > genpd_runtime_resume; > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > > > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > > > + > > > > + if (genpd_irq_on(genpd)) { > > > > + genpd->domain.ops.suspend = > > > > genpd_suspend_noirq; > > > > + genpd->domain.ops.resume = genpd_resume_noirq; > > > > + } else { > > > > + genpd->domain.ops.suspend_noirq = > > > > genpd_suspend_noirq; > > > > + genpd->domain.ops.resume_noirq = > > > > genpd_resume_noirq; > > > > > > As we discussed previously, I am thinking that it may be better > > > to > > > move to using genpd->domain.ops.suspend_late and > > > genpd->domain.ops.resume_early instead. > > > > Wouldn't that better be a separate patch (on top)? Do you really > > want > > me to change the current behaviour (default case) to from noirq to > > late? Then I'll resend this series with such a patch added. > > Sorry, I wasn't clear enough, the default behaviour should remain as > is. > > What I meant was, when genpd_irq_on() is true, we should use the > genpd->domain.ops.suspend_late and genpd->domain.ops.resume_early. Testing that shows that this isn't working. I can provide the logs later, but suspend fails and I think it makes sense: "suspend_late" is simply already too late when i2c (or any needed driver) uses "suspend". > > Kind regards > Uffe ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2022-08-22 8:38 ` Martin Kepplinger 0 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-08-22 8:38 UTC (permalink / raw) To: Ulf Hansson Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > 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/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> > > > > --- > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > include/linux/pm_domain.h | 5 +++++ > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > b/drivers/base/power/domain.c > > > > index 5a2e0232862e..58376752a4de 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) > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > generic_pm_domain > > > > *genpd, > > > > genpd->domain.ops.runtime_suspend = > > > > genpd_runtime_suspend; > > > > genpd->domain.ops.runtime_resume = > > > > genpd_runtime_resume; > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > > > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > > > + > > > > + if (genpd_irq_on(genpd)) { > > > > + genpd->domain.ops.suspend = > > > > genpd_suspend_noirq; > > > > + genpd->domain.ops.resume = genpd_resume_noirq; > > > > + } else { > > > > + genpd->domain.ops.suspend_noirq = > > > > genpd_suspend_noirq; > > > > + genpd->domain.ops.resume_noirq = > > > > genpd_resume_noirq; > > > > > > As we discussed previously, I am thinking that it may be better > > > to > > > move to using genpd->domain.ops.suspend_late and > > > genpd->domain.ops.resume_early instead. > > > > Wouldn't that better be a separate patch (on top)? Do you really > > want > > me to change the current behaviour (default case) to from noirq to > > late? Then I'll resend this series with such a patch added. > > Sorry, I wasn't clear enough, the default behaviour should remain as > is. > > What I meant was, when genpd_irq_on() is true, we should use the > genpd->domain.ops.suspend_late and genpd->domain.ops.resume_early. Testing that shows that this isn't working. I can provide the logs later, but suspend fails and I think it makes sense: "suspend_late" is simply already too late when i2c (or any needed driver) uses "suspend". > > Kind regards > Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2022-08-22 8:38 ` Martin Kepplinger @ 2022-08-24 13:30 ` Ulf Hansson -1 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2022-08-24 13:30 UTC (permalink / raw) To: Martin Kepplinger Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > <martin.kepplinger@puri.sm> wrote: > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > 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/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> > > > > > --- > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > b/drivers/base/power/domain.c > > > > > index 5a2e0232862e..58376752a4de 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) > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > generic_pm_domain > > > > > *genpd, > > > > > genpd->domain.ops.runtime_suspend = > > > > > genpd_runtime_suspend; > > > > > genpd->domain.ops.runtime_resume = > > > > > genpd_runtime_resume; > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > > > > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > > > > + > > > > > + if (genpd_irq_on(genpd)) { > > > > > + genpd->domain.ops.suspend = > > > > > genpd_suspend_noirq; > > > > > + genpd->domain.ops.resume = genpd_resume_noirq; > > > > > + } else { > > > > > + genpd->domain.ops.suspend_noirq = > > > > > genpd_suspend_noirq; > > > > > + genpd->domain.ops.resume_noirq = > > > > > genpd_resume_noirq; > > > > > > > > As we discussed previously, I am thinking that it may be better > > > > to > > > > move to using genpd->domain.ops.suspend_late and > > > > genpd->domain.ops.resume_early instead. > > > > > > Wouldn't that better be a separate patch (on top)? Do you really > > > want > > > me to change the current behaviour (default case) to from noirq to > > > late? Then I'll resend this series with such a patch added. > > > > Sorry, I wasn't clear enough, the default behaviour should remain as > > is. > > > > What I meant was, when genpd_irq_on() is true, we should use the > > genpd->domain.ops.suspend_late and genpd->domain.ops.resume_early. > > Testing that shows that this isn't working. I can provide the logs > later, but suspend fails and I think it makes sense: "suspend_late" is > simply already too late when i2c (or any needed driver) uses "suspend". Okay, I see. The reason why I suggested moving the callbacks to "suspend_late", was that I was worried that some of the attached devices to genpd could use "suspend_late" themselves. This is the case for some drivers for DMA/clock/gpio/pinctrl-controllers, for example. That said, I am curious to look at the DT files for the platform you are running, would you mind giving me a pointer? So, this made me think about this a bit more. In the end, just using different levels (suspend, suspend_late, suspend_noirq) of callbacks are just papering over the real *dependency* problem. What we need for the genpd provider driver, is to be asked to be suspended under the following conditions: 1. All consumer devices (and child-domains) for its corresponding PM domain have been suspended. 2. All its supplier devices supplies must remain resumed, until the genpd provider has been suspended. Please allow me a few more days to think in more detail about this. In some way, it looks like we should be able to combine the information genpd has about its devices and child-domains, use PM callbacks for the genpd provider driver - so we can rely on the depency-path the fw_devlinks would give us for its supplier devices. Kind regards Uffe ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2022-08-24 13:30 ` Ulf Hansson 0 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2022-08-24 13:30 UTC (permalink / raw) To: Martin Kepplinger Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > <martin.kepplinger@puri.sm> wrote: > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > 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/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> > > > > > --- > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > b/drivers/base/power/domain.c > > > > > index 5a2e0232862e..58376752a4de 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) > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > generic_pm_domain > > > > > *genpd, > > > > > genpd->domain.ops.runtime_suspend = > > > > > genpd_runtime_suspend; > > > > > genpd->domain.ops.runtime_resume = > > > > > genpd_runtime_resume; > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > - genpd->domain.ops.suspend_noirq = genpd_suspend_noirq; > > > > > - genpd->domain.ops.resume_noirq = genpd_resume_noirq; > > > > > + > > > > > + if (genpd_irq_on(genpd)) { > > > > > + genpd->domain.ops.suspend = > > > > > genpd_suspend_noirq; > > > > > + genpd->domain.ops.resume = genpd_resume_noirq; > > > > > + } else { > > > > > + genpd->domain.ops.suspend_noirq = > > > > > genpd_suspend_noirq; > > > > > + genpd->domain.ops.resume_noirq = > > > > > genpd_resume_noirq; > > > > > > > > As we discussed previously, I am thinking that it may be better > > > > to > > > > move to using genpd->domain.ops.suspend_late and > > > > genpd->domain.ops.resume_early instead. > > > > > > Wouldn't that better be a separate patch (on top)? Do you really > > > want > > > me to change the current behaviour (default case) to from noirq to > > > late? Then I'll resend this series with such a patch added. > > > > Sorry, I wasn't clear enough, the default behaviour should remain as > > is. > > > > What I meant was, when genpd_irq_on() is true, we should use the > > genpd->domain.ops.suspend_late and genpd->domain.ops.resume_early. > > Testing that shows that this isn't working. I can provide the logs > later, but suspend fails and I think it makes sense: "suspend_late" is > simply already too late when i2c (or any needed driver) uses "suspend". Okay, I see. The reason why I suggested moving the callbacks to "suspend_late", was that I was worried that some of the attached devices to genpd could use "suspend_late" themselves. This is the case for some drivers for DMA/clock/gpio/pinctrl-controllers, for example. That said, I am curious to look at the DT files for the platform you are running, would you mind giving me a pointer? So, this made me think about this a bit more. In the end, just using different levels (suspend, suspend_late, suspend_noirq) of callbacks are just papering over the real *dependency* problem. What we need for the genpd provider driver, is to be asked to be suspended under the following conditions: 1. All consumer devices (and child-domains) for its corresponding PM domain have been suspended. 2. All its supplier devices supplies must remain resumed, until the genpd provider has been suspended. Please allow me a few more days to think in more detail about this. In some way, it looks like we should be able to combine the information genpd has about its devices and child-domains, use PM callbacks for the genpd provider driver - so we can rely on the depency-path the fw_devlinks would give us for its supplier devices. Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2022-08-24 13:30 ` Ulf Hansson @ 2022-08-25 7:06 ` Martin Kepplinger -1 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-08-25 7:06 UTC (permalink / raw) To: Ulf Hansson Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel Am Mittwoch, dem 24.08.2022 um 15:30 +0200 schrieb Ulf Hansson: > On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > 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/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> > > > > > > --- > > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > > b/drivers/base/power/domain.c > > > > > > index 5a2e0232862e..58376752a4de 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) > > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > > generic_pm_domain > > > > > > *genpd, > > > > > > genpd->domain.ops.runtime_suspend = > > > > > > genpd_runtime_suspend; > > > > > > genpd->domain.ops.runtime_resume = > > > > > > genpd_runtime_resume; > > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > > - genpd->domain.ops.suspend_noirq = > > > > > > genpd_suspend_noirq; > > > > > > - genpd->domain.ops.resume_noirq = > > > > > > genpd_resume_noirq; > > > > > > + > > > > > > + if (genpd_irq_on(genpd)) { > > > > > > + genpd->domain.ops.suspend = > > > > > > genpd_suspend_noirq; > > > > > > + genpd->domain.ops.resume = > > > > > > genpd_resume_noirq; > > > > > > + } else { > > > > > > + genpd->domain.ops.suspend_noirq = > > > > > > genpd_suspend_noirq; > > > > > > + genpd->domain.ops.resume_noirq = > > > > > > genpd_resume_noirq; > > > > > > > > > > As we discussed previously, I am thinking that it may be > > > > > better > > > > > to > > > > > move to using genpd->domain.ops.suspend_late and > > > > > genpd->domain.ops.resume_early instead. > > > > > > > > Wouldn't that better be a separate patch (on top)? Do you > > > > really > > > > want > > > > me to change the current behaviour (default case) to from noirq > > > > to > > > > late? Then I'll resend this series with such a patch added. > > > > > > Sorry, I wasn't clear enough, the default behaviour should remain > > > as > > > is. > > > > > > What I meant was, when genpd_irq_on() is true, we should use the > > > genpd->domain.ops.suspend_late and genpd- > > > >domain.ops.resume_early. > > > > Testing that shows that this isn't working. I can provide the logs > > later, but suspend fails and I think it makes sense: "suspend_late" > > is > > simply already too late when i2c (or any needed driver) uses > > "suspend". > > Okay, I see. > > The reason why I suggested moving the callbacks to "suspend_late", > was > that I was worried that some of the attached devices to genpd could > use "suspend_late" themselves. This is the case for some drivers for > DMA/clock/gpio/pinctrl-controllers, for example. That said, I am > curious to look at the DT files for the platform you are running, > would you mind giving me a pointer? I'm running https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi with these (small) patches on top: https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.19.3/librem5 > > So, this made me think about this a bit more. In the end, just using > different levels (suspend, suspend_late, suspend_noirq) of callbacks > are just papering over the real *dependency* problem. true, it doesn't feel like a stable solution. > > What we need for the genpd provider driver, is to be asked to be > suspended under the following conditions: > 1. All consumer devices (and child-domains) for its corresponding PM > domain have been suspended. > 2. All its supplier devices supplies must remain resumed, until the > genpd provider has been suspended. > > Please allow me a few more days to think in more detail about this. Thanks a lot for thinking about this! > > In some way, it looks like we should be able to combine the > information genpd has about its devices and child-domains, use PM > callbacks for the genpd provider driver - so we can rely on the > depency-path the fw_devlinks would give us for its supplier devices. > > Kind regards > Uffe martin ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2022-08-25 7:06 ` Martin Kepplinger 0 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-08-25 7:06 UTC (permalink / raw) To: Ulf Hansson Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel Am Mittwoch, dem 24.08.2022 um 15:30 +0200 schrieb Ulf Hansson: > On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > 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/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> > > > > > > --- > > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > > b/drivers/base/power/domain.c > > > > > > index 5a2e0232862e..58376752a4de 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) > > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > > generic_pm_domain > > > > > > *genpd, > > > > > > genpd->domain.ops.runtime_suspend = > > > > > > genpd_runtime_suspend; > > > > > > genpd->domain.ops.runtime_resume = > > > > > > genpd_runtime_resume; > > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > > - genpd->domain.ops.suspend_noirq = > > > > > > genpd_suspend_noirq; > > > > > > - genpd->domain.ops.resume_noirq = > > > > > > genpd_resume_noirq; > > > > > > + > > > > > > + if (genpd_irq_on(genpd)) { > > > > > > + genpd->domain.ops.suspend = > > > > > > genpd_suspend_noirq; > > > > > > + genpd->domain.ops.resume = > > > > > > genpd_resume_noirq; > > > > > > + } else { > > > > > > + genpd->domain.ops.suspend_noirq = > > > > > > genpd_suspend_noirq; > > > > > > + genpd->domain.ops.resume_noirq = > > > > > > genpd_resume_noirq; > > > > > > > > > > As we discussed previously, I am thinking that it may be > > > > > better > > > > > to > > > > > move to using genpd->domain.ops.suspend_late and > > > > > genpd->domain.ops.resume_early instead. > > > > > > > > Wouldn't that better be a separate patch (on top)? Do you > > > > really > > > > want > > > > me to change the current behaviour (default case) to from noirq > > > > to > > > > late? Then I'll resend this series with such a patch added. > > > > > > Sorry, I wasn't clear enough, the default behaviour should remain > > > as > > > is. > > > > > > What I meant was, when genpd_irq_on() is true, we should use the > > > genpd->domain.ops.suspend_late and genpd- > > > >domain.ops.resume_early. > > > > Testing that shows that this isn't working. I can provide the logs > > later, but suspend fails and I think it makes sense: "suspend_late" > > is > > simply already too late when i2c (or any needed driver) uses > > "suspend". > > Okay, I see. > > The reason why I suggested moving the callbacks to "suspend_late", > was > that I was worried that some of the attached devices to genpd could > use "suspend_late" themselves. This is the case for some drivers for > DMA/clock/gpio/pinctrl-controllers, for example. That said, I am > curious to look at the DT files for the platform you are running, > would you mind giving me a pointer? I'm running https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi with these (small) patches on top: https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.19.3/librem5 > > So, this made me think about this a bit more. In the end, just using > different levels (suspend, suspend_late, suspend_noirq) of callbacks > are just papering over the real *dependency* problem. true, it doesn't feel like a stable solution. > > What we need for the genpd provider driver, is to be asked to be > suspended under the following conditions: > 1. All consumer devices (and child-domains) for its corresponding PM > domain have been suspended. > 2. All its supplier devices supplies must remain resumed, until the > genpd provider has been suspended. > > Please allow me a few more days to think in more detail about this. Thanks a lot for thinking about this! > > In some way, it looks like we should be able to combine the > information genpd has about its devices and child-domains, use PM > callbacks for the genpd provider driver - so we can rely on the > depency-path the fw_devlinks would give us for its supplier devices. > > Kind regards > Uffe martin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2022-08-25 7:06 ` Martin Kepplinger @ 2022-09-23 13:55 ` Ulf Hansson -1 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2022-09-23 13:55 UTC (permalink / raw) To: Martin Kepplinger Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel On Thu, 25 Aug 2022 at 09:06, Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > Am Mittwoch, dem 24.08.2022 um 15:30 +0200 schrieb Ulf Hansson: > > On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger > > <martin.kepplinger@puri.sm> wrote: > > > > > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > > > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > > > 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/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> > > > > > > > --- > > > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > > > b/drivers/base/power/domain.c > > > > > > > index 5a2e0232862e..58376752a4de 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) > > > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > > > generic_pm_domain > > > > > > > *genpd, > > > > > > > genpd->domain.ops.runtime_suspend = > > > > > > > genpd_runtime_suspend; > > > > > > > genpd->domain.ops.runtime_resume = > > > > > > > genpd_runtime_resume; > > > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > > > - genpd->domain.ops.suspend_noirq = > > > > > > > genpd_suspend_noirq; > > > > > > > - genpd->domain.ops.resume_noirq = > > > > > > > genpd_resume_noirq; > > > > > > > + > > > > > > > + if (genpd_irq_on(genpd)) { > > > > > > > + genpd->domain.ops.suspend = > > > > > > > genpd_suspend_noirq; > > > > > > > + genpd->domain.ops.resume = > > > > > > > genpd_resume_noirq; > > > > > > > + } else { > > > > > > > + genpd->domain.ops.suspend_noirq = > > > > > > > genpd_suspend_noirq; > > > > > > > + genpd->domain.ops.resume_noirq = > > > > > > > genpd_resume_noirq; > > > > > > > > > > > > As we discussed previously, I am thinking that it may be > > > > > > better > > > > > > to > > > > > > move to using genpd->domain.ops.suspend_late and > > > > > > genpd->domain.ops.resume_early instead. > > > > > > > > > > Wouldn't that better be a separate patch (on top)? Do you > > > > > really > > > > > want > > > > > me to change the current behaviour (default case) to from noirq > > > > > to > > > > > late? Then I'll resend this series with such a patch added. > > > > > > > > Sorry, I wasn't clear enough, the default behaviour should remain > > > > as > > > > is. > > > > > > > > What I meant was, when genpd_irq_on() is true, we should use the > > > > genpd->domain.ops.suspend_late and genpd- > > > > >domain.ops.resume_early. > > > > > > Testing that shows that this isn't working. I can provide the logs > > > later, but suspend fails and I think it makes sense: "suspend_late" > > > is > > > simply already too late when i2c (or any needed driver) uses > > > "suspend". > > > > Okay, I see. > > > > The reason why I suggested moving the callbacks to "suspend_late", > > was > > that I was worried that some of the attached devices to genpd could > > use "suspend_late" themselves. This is the case for some drivers for > > DMA/clock/gpio/pinctrl-controllers, for example. That said, I am > > curious to look at the DT files for the platform you are running, > > would you mind giving me a pointer? > > I'm running > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi > with these (small) patches on top: > https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.19.3/librem5 Thanks for sharing the information! > > > > > So, this made me think about this a bit more. In the end, just using > > different levels (suspend, suspend_late, suspend_noirq) of callbacks > > are just papering over the real *dependency* problem. > > true, it doesn't feel like a stable solution. > > > > > What we need for the genpd provider driver, is to be asked to be > > suspended under the following conditions: > > 1. All consumer devices (and child-domains) for its corresponding PM > > domain have been suspended. > > 2. All its supplier devices supplies must remain resumed, until the > > genpd provider has been suspended. > > > > Please allow me a few more days to think in more detail about this. > > Thanks a lot for thinking about this! I have made some more thinking, but it's been a busy period for me, so unfortunately I need some additional time (another week). It seems like I also need to do some prototyping, to convince myself about the approach. So, my apologies for the delay! Kind regards Uffe ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2022-09-23 13:55 ` Ulf Hansson 0 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2022-09-23 13:55 UTC (permalink / raw) To: Martin Kepplinger Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel On Thu, 25 Aug 2022 at 09:06, Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > Am Mittwoch, dem 24.08.2022 um 15:30 +0200 schrieb Ulf Hansson: > > On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger > > <martin.kepplinger@puri.sm> wrote: > > > > > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf Hansson: > > > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > > > 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/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> > > > > > > > --- > > > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > > > b/drivers/base/power/domain.c > > > > > > > index 5a2e0232862e..58376752a4de 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) > > > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > > > generic_pm_domain > > > > > > > *genpd, > > > > > > > genpd->domain.ops.runtime_suspend = > > > > > > > genpd_runtime_suspend; > > > > > > > genpd->domain.ops.runtime_resume = > > > > > > > genpd_runtime_resume; > > > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > > > - genpd->domain.ops.suspend_noirq = > > > > > > > genpd_suspend_noirq; > > > > > > > - genpd->domain.ops.resume_noirq = > > > > > > > genpd_resume_noirq; > > > > > > > + > > > > > > > + if (genpd_irq_on(genpd)) { > > > > > > > + genpd->domain.ops.suspend = > > > > > > > genpd_suspend_noirq; > > > > > > > + genpd->domain.ops.resume = > > > > > > > genpd_resume_noirq; > > > > > > > + } else { > > > > > > > + genpd->domain.ops.suspend_noirq = > > > > > > > genpd_suspend_noirq; > > > > > > > + genpd->domain.ops.resume_noirq = > > > > > > > genpd_resume_noirq; > > > > > > > > > > > > As we discussed previously, I am thinking that it may be > > > > > > better > > > > > > to > > > > > > move to using genpd->domain.ops.suspend_late and > > > > > > genpd->domain.ops.resume_early instead. > > > > > > > > > > Wouldn't that better be a separate patch (on top)? Do you > > > > > really > > > > > want > > > > > me to change the current behaviour (default case) to from noirq > > > > > to > > > > > late? Then I'll resend this series with such a patch added. > > > > > > > > Sorry, I wasn't clear enough, the default behaviour should remain > > > > as > > > > is. > > > > > > > > What I meant was, when genpd_irq_on() is true, we should use the > > > > genpd->domain.ops.suspend_late and genpd- > > > > >domain.ops.resume_early. > > > > > > Testing that shows that this isn't working. I can provide the logs > > > later, but suspend fails and I think it makes sense: "suspend_late" > > > is > > > simply already too late when i2c (or any needed driver) uses > > > "suspend". > > > > Okay, I see. > > > > The reason why I suggested moving the callbacks to "suspend_late", > > was > > that I was worried that some of the attached devices to genpd could > > use "suspend_late" themselves. This is the case for some drivers for > > DMA/clock/gpio/pinctrl-controllers, for example. That said, I am > > curious to look at the DT files for the platform you are running, > > would you mind giving me a pointer? > > I'm running > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi > with these (small) patches on top: > https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.19.3/librem5 Thanks for sharing the information! > > > > > So, this made me think about this a bit more. In the end, just using > > different levels (suspend, suspend_late, suspend_noirq) of callbacks > > are just papering over the real *dependency* problem. > > true, it doesn't feel like a stable solution. > > > > > What we need for the genpd provider driver, is to be asked to be > > suspended under the following conditions: > > 1. All consumer devices (and child-domains) for its corresponding PM > > domain have been suspended. > > 2. All its supplier devices supplies must remain resumed, until the > > genpd provider has been suspended. > > > > Please allow me a few more days to think in more detail about this. > > Thanks a lot for thinking about this! I have made some more thinking, but it's been a busy period for me, so unfortunately I need some additional time (another week). It seems like I also need to do some prototyping, to convince myself about the approach. So, my apologies for the delay! Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2022-09-23 13:55 ` Ulf Hansson @ 2022-09-26 9:52 ` Martin Kepplinger -1 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-09-26 9:52 UTC (permalink / raw) To: Ulf Hansson Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel Am Freitag, dem 23.09.2022 um 15:55 +0200 schrieb Ulf Hansson: > On Thu, 25 Aug 2022 at 09:06, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > Am Mittwoch, dem 24.08.2022 um 15:30 +0200 schrieb Ulf Hansson: > > > On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > > > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf > > > > > > Hansson: > > > > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > > > > > 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/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> > > > > > > > > --- > > > > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > > > > b/drivers/base/power/domain.c > > > > > > > > index 5a2e0232862e..58376752a4de 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) > > > > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > > > > generic_pm_domain > > > > > > > > *genpd, > > > > > > > > genpd->domain.ops.runtime_suspend = > > > > > > > > genpd_runtime_suspend; > > > > > > > > genpd->domain.ops.runtime_resume = > > > > > > > > genpd_runtime_resume; > > > > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > > > > - genpd->domain.ops.suspend_noirq = > > > > > > > > genpd_suspend_noirq; > > > > > > > > - genpd->domain.ops.resume_noirq = > > > > > > > > genpd_resume_noirq; > > > > > > > > + > > > > > > > > + if (genpd_irq_on(genpd)) { > > > > > > > > + genpd->domain.ops.suspend = > > > > > > > > genpd_suspend_noirq; > > > > > > > > + genpd->domain.ops.resume = > > > > > > > > genpd_resume_noirq; > > > > > > > > + } else { > > > > > > > > + genpd->domain.ops.suspend_noirq = > > > > > > > > genpd_suspend_noirq; > > > > > > > > + genpd->domain.ops.resume_noirq = > > > > > > > > genpd_resume_noirq; > > > > > > > > > > > > > > As we discussed previously, I am thinking that it may be > > > > > > > better > > > > > > > to > > > > > > > move to using genpd->domain.ops.suspend_late and > > > > > > > genpd->domain.ops.resume_early instead. > > > > > > > > > > > > Wouldn't that better be a separate patch (on top)? Do you > > > > > > really > > > > > > want > > > > > > me to change the current behaviour (default case) to from > > > > > > noirq > > > > > > to > > > > > > late? Then I'll resend this series with such a patch added. > > > > > > > > > > Sorry, I wasn't clear enough, the default behaviour should > > > > > remain > > > > > as > > > > > is. > > > > > > > > > > What I meant was, when genpd_irq_on() is true, we should use > > > > > the > > > > > genpd->domain.ops.suspend_late and genpd- > > > > > > domain.ops.resume_early. > > > > > > > > Testing that shows that this isn't working. I can provide the > > > > logs > > > > later, but suspend fails and I think it makes sense: > > > > "suspend_late" > > > > is > > > > simply already too late when i2c (or any needed driver) uses > > > > "suspend". > > > > > > Okay, I see. > > > > > > The reason why I suggested moving the callbacks to > > > "suspend_late", > > > was > > > that I was worried that some of the attached devices to genpd > > > could > > > use "suspend_late" themselves. This is the case for some drivers > > > for > > > DMA/clock/gpio/pinctrl-controllers, for example. That said, I am > > > curious to look at the DT files for the platform you are running, > > > would you mind giving me a pointer? > > > > I'm running > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi > > with these (small) patches on top: > > https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.19.3/librem5 > > Thanks for sharing the information! > > > > > > > > > So, this made me think about this a bit more. In the end, just > > > using > > > different levels (suspend, suspend_late, suspend_noirq) of > > > callbacks > > > are just papering over the real *dependency* problem. > > > > true, it doesn't feel like a stable solution. > > > > > > > > What we need for the genpd provider driver, is to be asked to be > > > suspended under the following conditions: > > > 1. All consumer devices (and child-domains) for its corresponding > > > PM > > > domain have been suspended. > > > 2. All its supplier devices supplies must remain resumed, until > > > the > > > genpd provider has been suspended. > > > > > > Please allow me a few more days to think in more detail about > > > this. > > > > Thanks a lot for thinking about this! > > I have made some more thinking, but it's been a busy period for me, > so > unfortunately I need some additional time (another week). It seems > like I also need to do some prototyping, to convince myself about the > approach. > > So, my apologies for the delay! to be honest, I'm happy as long as you don't forget about the bug. The workaround I got (these patches) is solid enough for me to be able to wait. And I'm happy to always answer specific questions or test a patch of course. thanks for the update! martin ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2022-09-26 9:52 ` Martin Kepplinger 0 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-09-26 9:52 UTC (permalink / raw) To: Ulf Hansson Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel Am Freitag, dem 23.09.2022 um 15:55 +0200 schrieb Ulf Hansson: > On Thu, 25 Aug 2022 at 09:06, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > Am Mittwoch, dem 24.08.2022 um 15:30 +0200 schrieb Ulf Hansson: > > > On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > > > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf > > > > > > Hansson: > > > > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > > > > > 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/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> > > > > > > > > --- > > > > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > > > > b/drivers/base/power/domain.c > > > > > > > > index 5a2e0232862e..58376752a4de 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) > > > > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > > > > generic_pm_domain > > > > > > > > *genpd, > > > > > > > > genpd->domain.ops.runtime_suspend = > > > > > > > > genpd_runtime_suspend; > > > > > > > > genpd->domain.ops.runtime_resume = > > > > > > > > genpd_runtime_resume; > > > > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > > > > - genpd->domain.ops.suspend_noirq = > > > > > > > > genpd_suspend_noirq; > > > > > > > > - genpd->domain.ops.resume_noirq = > > > > > > > > genpd_resume_noirq; > > > > > > > > + > > > > > > > > + if (genpd_irq_on(genpd)) { > > > > > > > > + genpd->domain.ops.suspend = > > > > > > > > genpd_suspend_noirq; > > > > > > > > + genpd->domain.ops.resume = > > > > > > > > genpd_resume_noirq; > > > > > > > > + } else { > > > > > > > > + genpd->domain.ops.suspend_noirq = > > > > > > > > genpd_suspend_noirq; > > > > > > > > + genpd->domain.ops.resume_noirq = > > > > > > > > genpd_resume_noirq; > > > > > > > > > > > > > > As we discussed previously, I am thinking that it may be > > > > > > > better > > > > > > > to > > > > > > > move to using genpd->domain.ops.suspend_late and > > > > > > > genpd->domain.ops.resume_early instead. > > > > > > > > > > > > Wouldn't that better be a separate patch (on top)? Do you > > > > > > really > > > > > > want > > > > > > me to change the current behaviour (default case) to from > > > > > > noirq > > > > > > to > > > > > > late? Then I'll resend this series with such a patch added. > > > > > > > > > > Sorry, I wasn't clear enough, the default behaviour should > > > > > remain > > > > > as > > > > > is. > > > > > > > > > > What I meant was, when genpd_irq_on() is true, we should use > > > > > the > > > > > genpd->domain.ops.suspend_late and genpd- > > > > > > domain.ops.resume_early. > > > > > > > > Testing that shows that this isn't working. I can provide the > > > > logs > > > > later, but suspend fails and I think it makes sense: > > > > "suspend_late" > > > > is > > > > simply already too late when i2c (or any needed driver) uses > > > > "suspend". > > > > > > Okay, I see. > > > > > > The reason why I suggested moving the callbacks to > > > "suspend_late", > > > was > > > that I was worried that some of the attached devices to genpd > > > could > > > use "suspend_late" themselves. This is the case for some drivers > > > for > > > DMA/clock/gpio/pinctrl-controllers, for example. That said, I am > > > curious to look at the DT files for the platform you are running, > > > would you mind giving me a pointer? > > > > I'm running > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi > > with these (small) patches on top: > > https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.19.3/librem5 > > Thanks for sharing the information! > > > > > > > > > So, this made me think about this a bit more. In the end, just > > > using > > > different levels (suspend, suspend_late, suspend_noirq) of > > > callbacks > > > are just papering over the real *dependency* problem. > > > > true, it doesn't feel like a stable solution. > > > > > > > > What we need for the genpd provider driver, is to be asked to be > > > suspended under the following conditions: > > > 1. All consumer devices (and child-domains) for its corresponding > > > PM > > > domain have been suspended. > > > 2. All its supplier devices supplies must remain resumed, until > > > the > > > genpd provider has been suspended. > > > > > > Please allow me a few more days to think in more detail about > > > this. > > > > Thanks a lot for thinking about this! > > I have made some more thinking, but it's been a busy period for me, > so > unfortunately I need some additional time (another week). It seems > like I also need to do some prototyping, to convince myself about the > approach. > > So, my apologies for the delay! to be honest, I'm happy as long as you don't forget about the bug. The workaround I got (these patches) is solid enough for me to be able to wait. And I'm happy to always answer specific questions or test a patch of course. thanks for the update! martin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2022-09-23 13:55 ` Ulf Hansson @ 2023-06-21 18:20 ` Martin Kepplinger -1 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2023-06-21 18:20 UTC (permalink / raw) To: Ulf Hansson Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel Am Freitag, dem 23.09.2022 um 15:55 +0200 schrieb Ulf Hansson: > On Thu, 25 Aug 2022 at 09:06, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > Am Mittwoch, dem 24.08.2022 um 15:30 +0200 schrieb Ulf Hansson: > > > On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > > > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf > > > > > > Hansson: > > > > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > > > > > 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/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> > > > > > > > > --- > > > > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > > > > b/drivers/base/power/domain.c > > > > > > > > index 5a2e0232862e..58376752a4de 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) > > > > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > > > > generic_pm_domain > > > > > > > > *genpd, > > > > > > > > genpd->domain.ops.runtime_suspend = > > > > > > > > genpd_runtime_suspend; > > > > > > > > genpd->domain.ops.runtime_resume = > > > > > > > > genpd_runtime_resume; > > > > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > > > > - genpd->domain.ops.suspend_noirq = > > > > > > > > genpd_suspend_noirq; > > > > > > > > - genpd->domain.ops.resume_noirq = > > > > > > > > genpd_resume_noirq; > > > > > > > > + > > > > > > > > + if (genpd_irq_on(genpd)) { > > > > > > > > + genpd->domain.ops.suspend = > > > > > > > > genpd_suspend_noirq; > > > > > > > > + genpd->domain.ops.resume = > > > > > > > > genpd_resume_noirq; > > > > > > > > + } else { > > > > > > > > + genpd->domain.ops.suspend_noirq = > > > > > > > > genpd_suspend_noirq; > > > > > > > > + genpd->domain.ops.resume_noirq = > > > > > > > > genpd_resume_noirq; > > > > > > > > > > > > > > As we discussed previously, I am thinking that it may be > > > > > > > better > > > > > > > to > > > > > > > move to using genpd->domain.ops.suspend_late and > > > > > > > genpd->domain.ops.resume_early instead. > > > > > > > > > > > > Wouldn't that better be a separate patch (on top)? Do you > > > > > > really > > > > > > want > > > > > > me to change the current behaviour (default case) to from > > > > > > noirq > > > > > > to > > > > > > late? Then I'll resend this series with such a patch added. > > > > > > > > > > Sorry, I wasn't clear enough, the default behaviour should > > > > > remain > > > > > as > > > > > is. > > > > > > > > > > What I meant was, when genpd_irq_on() is true, we should use > > > > > the > > > > > genpd->domain.ops.suspend_late and genpd- > > > > > > domain.ops.resume_early. > > > > > > > > Testing that shows that this isn't working. I can provide the > > > > logs > > > > later, but suspend fails and I think it makes sense: > > > > "suspend_late" > > > > is > > > > simply already too late when i2c (or any needed driver) uses > > > > "suspend". > > > > > > Okay, I see. > > > > > > The reason why I suggested moving the callbacks to > > > "suspend_late", > > > was > > > that I was worried that some of the attached devices to genpd > > > could > > > use "suspend_late" themselves. This is the case for some drivers > > > for > > > DMA/clock/gpio/pinctrl-controllers, for example. That said, I am > > > curious to look at the DT files for the platform you are running, > > > would you mind giving me a pointer? > > > > I'm running > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi > > with these (small) patches on top: > > https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.19.3/librem5 > > Thanks for sharing the information! > > > > > > > > > So, this made me think about this a bit more. In the end, just > > > using > > > different levels (suspend, suspend_late, suspend_noirq) of > > > callbacks > > > are just papering over the real *dependency* problem. > > > > true, it doesn't feel like a stable solution. > > > > > > > > What we need for the genpd provider driver, is to be asked to be > > > suspended under the following conditions: > > > 1. All consumer devices (and child-domains) for its corresponding > > > PM > > > domain have been suspended. > > > 2. All its supplier devices supplies must remain resumed, until > > > the > > > genpd provider has been suspended. > > > > > > Please allow me a few more days to think in more detail about > > > this. > > > > Thanks a lot for thinking about this! > > I have made some more thinking, but it's been a busy period for me, > so > unfortunately I need some additional time (another week). It seems > like I also need to do some prototyping, to convince myself about the > approach. > > So, my apologies for the delay! > > Kind regards > Uffe Hi Ulf and all interested, Has there been any development regarding this bug? - genpd that needs interrupts for power-on/off being run in noirq phases - you remember it? it's been a while :) Anyway I still run these patches and while it's a reasonable workaround IMO, I wanted to check whether you are aware of anything that might solve this. (or maybe it *is* solved and I simply overlooked because my patches still apply?) thanks! martin ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2023-06-21 18:20 ` Martin Kepplinger 0 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2023-06-21 18:20 UTC (permalink / raw) To: Ulf Hansson Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel Am Freitag, dem 23.09.2022 um 15:55 +0200 schrieb Ulf Hansson: > On Thu, 25 Aug 2022 at 09:06, Martin Kepplinger > <martin.kepplinger@puri.sm> wrote: > > > > Am Mittwoch, dem 24.08.2022 um 15:30 +0200 schrieb Ulf Hansson: > > > On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > > > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf > > > > > > Hansson: > > > > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > > > > > 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/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> > > > > > > > > --- > > > > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > > > > b/drivers/base/power/domain.c > > > > > > > > index 5a2e0232862e..58376752a4de 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) > > > > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > > > > generic_pm_domain > > > > > > > > *genpd, > > > > > > > > genpd->domain.ops.runtime_suspend = > > > > > > > > genpd_runtime_suspend; > > > > > > > > genpd->domain.ops.runtime_resume = > > > > > > > > genpd_runtime_resume; > > > > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > > > > - genpd->domain.ops.suspend_noirq = > > > > > > > > genpd_suspend_noirq; > > > > > > > > - genpd->domain.ops.resume_noirq = > > > > > > > > genpd_resume_noirq; > > > > > > > > + > > > > > > > > + if (genpd_irq_on(genpd)) { > > > > > > > > + genpd->domain.ops.suspend = > > > > > > > > genpd_suspend_noirq; > > > > > > > > + genpd->domain.ops.resume = > > > > > > > > genpd_resume_noirq; > > > > > > > > + } else { > > > > > > > > + genpd->domain.ops.suspend_noirq = > > > > > > > > genpd_suspend_noirq; > > > > > > > > + genpd->domain.ops.resume_noirq = > > > > > > > > genpd_resume_noirq; > > > > > > > > > > > > > > As we discussed previously, I am thinking that it may be > > > > > > > better > > > > > > > to > > > > > > > move to using genpd->domain.ops.suspend_late and > > > > > > > genpd->domain.ops.resume_early instead. > > > > > > > > > > > > Wouldn't that better be a separate patch (on top)? Do you > > > > > > really > > > > > > want > > > > > > me to change the current behaviour (default case) to from > > > > > > noirq > > > > > > to > > > > > > late? Then I'll resend this series with such a patch added. > > > > > > > > > > Sorry, I wasn't clear enough, the default behaviour should > > > > > remain > > > > > as > > > > > is. > > > > > > > > > > What I meant was, when genpd_irq_on() is true, we should use > > > > > the > > > > > genpd->domain.ops.suspend_late and genpd- > > > > > > domain.ops.resume_early. > > > > > > > > Testing that shows that this isn't working. I can provide the > > > > logs > > > > later, but suspend fails and I think it makes sense: > > > > "suspend_late" > > > > is > > > > simply already too late when i2c (or any needed driver) uses > > > > "suspend". > > > > > > Okay, I see. > > > > > > The reason why I suggested moving the callbacks to > > > "suspend_late", > > > was > > > that I was worried that some of the attached devices to genpd > > > could > > > use "suspend_late" themselves. This is the case for some drivers > > > for > > > DMA/clock/gpio/pinctrl-controllers, for example. That said, I am > > > curious to look at the DT files for the platform you are running, > > > would you mind giving me a pointer? > > > > I'm running > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi > > with these (small) patches on top: > > https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.19.3/librem5 > > Thanks for sharing the information! > > > > > > > > > So, this made me think about this a bit more. In the end, just > > > using > > > different levels (suspend, suspend_late, suspend_noirq) of > > > callbacks > > > are just papering over the real *dependency* problem. > > > > true, it doesn't feel like a stable solution. > > > > > > > > What we need for the genpd provider driver, is to be asked to be > > > suspended under the following conditions: > > > 1. All consumer devices (and child-domains) for its corresponding > > > PM > > > domain have been suspended. > > > 2. All its supplier devices supplies must remain resumed, until > > > the > > > genpd provider has been suspended. > > > > > > Please allow me a few more days to think in more detail about > > > this. > > > > Thanks a lot for thinking about this! > > I have made some more thinking, but it's been a busy period for me, > so > unfortunately I need some additional time (another week). It seems > like I also need to do some prototyping, to convince myself about the > approach. > > So, my apologies for the delay! > > Kind regards > Uffe Hi Ulf and all interested, Has there been any development regarding this bug? - genpd that needs interrupts for power-on/off being run in noirq phases - you remember it? it's been a while :) Anyway I still run these patches and while it's a reasonable workaround IMO, I wanted to check whether you are aware of anything that might solve this. (or maybe it *is* solved and I simply overlooked because my patches still apply?) thanks! martin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts 2023-06-21 18:20 ` Martin Kepplinger @ 2023-06-26 9:50 ` Ulf Hansson -1 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2023-06-26 9:50 UTC (permalink / raw) To: Martin Kepplinger Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel On Wed, 21 Jun 2023 at 20:20, Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > Am Freitag, dem 23.09.2022 um 15:55 +0200 schrieb Ulf Hansson: > > On Thu, 25 Aug 2022 at 09:06, Martin Kepplinger > > <martin.kepplinger@puri.sm> wrote: > > > > > > Am Mittwoch, dem 24.08.2022 um 15:30 +0200 schrieb Ulf Hansson: > > > > On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > > > > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf > > > > > > > Hansson: > > > > > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > > > > > > > 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/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> > > > > > > > > > --- > > > > > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > > > > > b/drivers/base/power/domain.c > > > > > > > > > index 5a2e0232862e..58376752a4de 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) > > > > > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > > > > > generic_pm_domain > > > > > > > > > *genpd, > > > > > > > > > genpd->domain.ops.runtime_suspend = > > > > > > > > > genpd_runtime_suspend; > > > > > > > > > genpd->domain.ops.runtime_resume = > > > > > > > > > genpd_runtime_resume; > > > > > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > > > > > - genpd->domain.ops.suspend_noirq = > > > > > > > > > genpd_suspend_noirq; > > > > > > > > > - genpd->domain.ops.resume_noirq = > > > > > > > > > genpd_resume_noirq; > > > > > > > > > + > > > > > > > > > + if (genpd_irq_on(genpd)) { > > > > > > > > > + genpd->domain.ops.suspend = > > > > > > > > > genpd_suspend_noirq; > > > > > > > > > + genpd->domain.ops.resume = > > > > > > > > > genpd_resume_noirq; > > > > > > > > > + } else { > > > > > > > > > + genpd->domain.ops.suspend_noirq = > > > > > > > > > genpd_suspend_noirq; > > > > > > > > > + genpd->domain.ops.resume_noirq = > > > > > > > > > genpd_resume_noirq; > > > > > > > > > > > > > > > > As we discussed previously, I am thinking that it may be > > > > > > > > better > > > > > > > > to > > > > > > > > move to using genpd->domain.ops.suspend_late and > > > > > > > > genpd->domain.ops.resume_early instead. > > > > > > > > > > > > > > Wouldn't that better be a separate patch (on top)? Do you > > > > > > > really > > > > > > > want > > > > > > > me to change the current behaviour (default case) to from > > > > > > > noirq > > > > > > > to > > > > > > > late? Then I'll resend this series with such a patch added. > > > > > > > > > > > > Sorry, I wasn't clear enough, the default behaviour should > > > > > > remain > > > > > > as > > > > > > is. > > > > > > > > > > > > What I meant was, when genpd_irq_on() is true, we should use > > > > > > the > > > > > > genpd->domain.ops.suspend_late and genpd- > > > > > > > domain.ops.resume_early. > > > > > > > > > > Testing that shows that this isn't working. I can provide the > > > > > logs > > > > > later, but suspend fails and I think it makes sense: > > > > > "suspend_late" > > > > > is > > > > > simply already too late when i2c (or any needed driver) uses > > > > > "suspend". > > > > > > > > Okay, I see. > > > > > > > > The reason why I suggested moving the callbacks to > > > > "suspend_late", > > > > was > > > > that I was worried that some of the attached devices to genpd > > > > could > > > > use "suspend_late" themselves. This is the case for some drivers > > > > for > > > > DMA/clock/gpio/pinctrl-controllers, for example. That said, I am > > > > curious to look at the DT files for the platform you are running, > > > > would you mind giving me a pointer? > > > > > > I'm running > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi > > > with these (small) patches on top: > > > https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.19.3/librem5 > > > > Thanks for sharing the information! > > > > > > > > > > > > > So, this made me think about this a bit more. In the end, just > > > > using > > > > different levels (suspend, suspend_late, suspend_noirq) of > > > > callbacks > > > > are just papering over the real *dependency* problem. > > > > > > true, it doesn't feel like a stable solution. > > > > > > > > > > > What we need for the genpd provider driver, is to be asked to be > > > > suspended under the following conditions: > > > > 1. All consumer devices (and child-domains) for its corresponding > > > > PM > > > > domain have been suspended. > > > > 2. All its supplier devices supplies must remain resumed, until > > > > the > > > > genpd provider has been suspended. > > > > > > > > Please allow me a few more days to think in more detail about > > > > this. > > > > > > Thanks a lot for thinking about this! > > > > I have made some more thinking, but it's been a busy period for me, > > so > > unfortunately I need some additional time (another week). It seems > > like I also need to do some prototyping, to convince myself about the > > approach. > > > > So, my apologies for the delay! > > > > Kind regards > > Uffe > > Hi Ulf and all interested, > > Has there been any development regarding this bug? - genpd that needs > interrupts for power-on/off being run in noirq phases - you remember > it? it's been a while :) Yes, sorry for the no-progress on my side. Except for some thinking and drawing, I don't have an update. Although, to clarify, I have not forgotten about it. It's in my TODO list of prioritized things. I just need to complete a couple other things before I come to this and I will certainly keep you in the loop if I post something. > > Anyway I still run these patches and while it's a reasonable workaround > IMO, I wanted to check whether you are aware of anything that might > solve this. (or maybe it *is* solved and I simply overlooked because my > patches still apply?) The problem is still there, unfortunately. Kind regards Uffe ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts @ 2023-06-26 9:50 ` Ulf Hansson 0 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2023-06-26 9:50 UTC (permalink / raw) To: Martin Kepplinger Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel On Wed, 21 Jun 2023 at 20:20, Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > Am Freitag, dem 23.09.2022 um 15:55 +0200 schrieb Ulf Hansson: > > On Thu, 25 Aug 2022 at 09:06, Martin Kepplinger > > <martin.kepplinger@puri.sm> wrote: > > > > > > Am Mittwoch, dem 24.08.2022 um 15:30 +0200 schrieb Ulf Hansson: > > > > On Mon, 22 Aug 2022 at 10:38, Martin Kepplinger > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > Am Freitag, dem 19.08.2022 um 16:53 +0200 schrieb Ulf Hansson: > > > > > > On Fri, 19 Aug 2022 at 11:17, Martin Kepplinger > > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > > > Am Dienstag, dem 26.07.2022 um 17:07 +0200 schrieb Ulf > > > > > > > Hansson: > > > > > > > > On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger > > > > > > > > <martin.kepplinger@puri.sm> wrote: > > > > > > > > > > > > > > > > > > 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/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> > > > > > > > > > --- > > > > > > > > > drivers/base/power/domain.c | 13 +++++++++++-- > > > > > > > > > include/linux/pm_domain.h | 5 +++++ > > > > > > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/base/power/domain.c > > > > > > > > > b/drivers/base/power/domain.c > > > > > > > > > index 5a2e0232862e..58376752a4de 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) > > > > > > > > > @@ -2065,8 +2066,15 @@ int pm_genpd_init(struct > > > > > > > > > generic_pm_domain > > > > > > > > > *genpd, > > > > > > > > > genpd->domain.ops.runtime_suspend = > > > > > > > > > genpd_runtime_suspend; > > > > > > > > > genpd->domain.ops.runtime_resume = > > > > > > > > > genpd_runtime_resume; > > > > > > > > > genpd->domain.ops.prepare = genpd_prepare; > > > > > > > > > - genpd->domain.ops.suspend_noirq = > > > > > > > > > genpd_suspend_noirq; > > > > > > > > > - genpd->domain.ops.resume_noirq = > > > > > > > > > genpd_resume_noirq; > > > > > > > > > + > > > > > > > > > + if (genpd_irq_on(genpd)) { > > > > > > > > > + genpd->domain.ops.suspend = > > > > > > > > > genpd_suspend_noirq; > > > > > > > > > + genpd->domain.ops.resume = > > > > > > > > > genpd_resume_noirq; > > > > > > > > > + } else { > > > > > > > > > + genpd->domain.ops.suspend_noirq = > > > > > > > > > genpd_suspend_noirq; > > > > > > > > > + genpd->domain.ops.resume_noirq = > > > > > > > > > genpd_resume_noirq; > > > > > > > > > > > > > > > > As we discussed previously, I am thinking that it may be > > > > > > > > better > > > > > > > > to > > > > > > > > move to using genpd->domain.ops.suspend_late and > > > > > > > > genpd->domain.ops.resume_early instead. > > > > > > > > > > > > > > Wouldn't that better be a separate patch (on top)? Do you > > > > > > > really > > > > > > > want > > > > > > > me to change the current behaviour (default case) to from > > > > > > > noirq > > > > > > > to > > > > > > > late? Then I'll resend this series with such a patch added. > > > > > > > > > > > > Sorry, I wasn't clear enough, the default behaviour should > > > > > > remain > > > > > > as > > > > > > is. > > > > > > > > > > > > What I meant was, when genpd_irq_on() is true, we should use > > > > > > the > > > > > > genpd->domain.ops.suspend_late and genpd- > > > > > > > domain.ops.resume_early. > > > > > > > > > > Testing that shows that this isn't working. I can provide the > > > > > logs > > > > > later, but suspend fails and I think it makes sense: > > > > > "suspend_late" > > > > > is > > > > > simply already too late when i2c (or any needed driver) uses > > > > > "suspend". > > > > > > > > Okay, I see. > > > > > > > > The reason why I suggested moving the callbacks to > > > > "suspend_late", > > > > was > > > > that I was worried that some of the attached devices to genpd > > > > could > > > > use "suspend_late" themselves. This is the case for some drivers > > > > for > > > > DMA/clock/gpio/pinctrl-controllers, for example. That said, I am > > > > curious to look at the DT files for the platform you are running, > > > > would you mind giving me a pointer? > > > > > > I'm running > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi > > > with these (small) patches on top: > > > https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.19.3/librem5 > > > > Thanks for sharing the information! > > > > > > > > > > > > > So, this made me think about this a bit more. In the end, just > > > > using > > > > different levels (suspend, suspend_late, suspend_noirq) of > > > > callbacks > > > > are just papering over the real *dependency* problem. > > > > > > true, it doesn't feel like a stable solution. > > > > > > > > > > > What we need for the genpd provider driver, is to be asked to be > > > > suspended under the following conditions: > > > > 1. All consumer devices (and child-domains) for its corresponding > > > > PM > > > > domain have been suspended. > > > > 2. All its supplier devices supplies must remain resumed, until > > > > the > > > > genpd provider has been suspended. > > > > > > > > Please allow me a few more days to think in more detail about > > > > this. > > > > > > Thanks a lot for thinking about this! > > > > I have made some more thinking, but it's been a busy period for me, > > so > > unfortunately I need some additional time (another week). It seems > > like I also need to do some prototyping, to convince myself about the > > approach. > > > > So, my apologies for the delay! > > > > Kind regards > > Uffe > > Hi Ulf and all interested, > > Has there been any development regarding this bug? - genpd that needs > interrupts for power-on/off being run in noirq phases - you remember > it? it's been a while :) Yes, sorry for the no-progress on my side. Except for some thinking and drawing, I don't have an update. Although, to clarify, I have not forgotten about it. It's in my TODO list of prioritized things. I just need to complete a couple other things before I come to this and I will certainly keep you in the loop if I post something. > > Anyway I still run these patches and while it's a reasonable workaround > IMO, I wanted to check whether you are aware of anything that might > solve this. (or maybe it *is* solved and I simply overlooked because my > patches still apply?) The problem is still there, unfortunately. Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v6 2/2] soc: imx: gpcv2: fix suspend/resume by setting GENPD_FLAG_IRQ_ON 2022-07-26 8:32 ` Martin Kepplinger @ 2022-07-26 8:32 ` Martin Kepplinger -1 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-07-26 8:32 UTC (permalink / raw) To: rafael, khilman, ulf.hansson, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel Cc: kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel, 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 <martin.kepplinger@puri.sm> --- drivers/soc/imx/gpcv2.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c index 6383a4edc360..199a621d8186 100644 --- a/drivers/soc/imx/gpcv2.c +++ b/drivers/soc/imx/gpcv2.c @@ -1337,6 +1337,9 @@ static int imx_pgc_domain_probe(struct platform_device *pdev) regmap_update_bits(domain->regmap, domain->regs->map, domain->bits.map, domain->bits.map); + if (of_property_read_bool(domain->dev->of_node, "power-supply")) + 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v6 2/2] soc: imx: gpcv2: fix suspend/resume by setting GENPD_FLAG_IRQ_ON @ 2022-07-26 8:32 ` Martin Kepplinger 0 siblings, 0 replies; 32+ messages in thread From: Martin Kepplinger @ 2022-07-26 8:32 UTC (permalink / raw) To: rafael, khilman, ulf.hansson, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel Cc: kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel, 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 <martin.kepplinger@puri.sm> --- drivers/soc/imx/gpcv2.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c index 6383a4edc360..199a621d8186 100644 --- a/drivers/soc/imx/gpcv2.c +++ b/drivers/soc/imx/gpcv2.c @@ -1337,6 +1337,9 @@ static int imx_pgc_domain_probe(struct platform_device *pdev) regmap_update_bits(domain->regmap, domain->regs->map, domain->bits.map, domain->bits.map); + if (of_property_read_bool(domain->dev->of_node, "power-supply")) + 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v6 2/2] soc: imx: gpcv2: fix suspend/resume by setting GENPD_FLAG_IRQ_ON 2022-07-26 8:32 ` Martin Kepplinger @ 2022-07-26 15:08 ` Ulf Hansson -1 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2022-07-26 15:08 UTC (permalink / raw) To: Martin Kepplinger Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > 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 <martin.kepplinger@puri.sm> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/soc/imx/gpcv2.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c > index 6383a4edc360..199a621d8186 100644 > --- a/drivers/soc/imx/gpcv2.c > +++ b/drivers/soc/imx/gpcv2.c > @@ -1337,6 +1337,9 @@ static int imx_pgc_domain_probe(struct platform_device *pdev) > regmap_update_bits(domain->regmap, domain->regs->map, > domain->bits.map, domain->bits.map); > > + if (of_property_read_bool(domain->dev->of_node, "power-supply")) > + 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 > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 2/2] soc: imx: gpcv2: fix suspend/resume by setting GENPD_FLAG_IRQ_ON @ 2022-07-26 15:08 ` Ulf Hansson 0 siblings, 0 replies; 32+ messages in thread From: Ulf Hansson @ 2022-07-26 15:08 UTC (permalink / raw) To: Martin Kepplinger Cc: rafael, khilman, robh, krzysztof.kozlowski, shawnguo, s.hauer, festevam, pavel, kernel, linux-imx, broonie, l.stach, aford173, linux-pm, devicetree, linux-kernel, linux-arm-kernel On Tue, 26 Jul 2022 at 10:33, Martin Kepplinger <martin.kepplinger@puri.sm> wrote: > > 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 <martin.kepplinger@puri.sm> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/soc/imx/gpcv2.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c > index 6383a4edc360..199a621d8186 100644 > --- a/drivers/soc/imx/gpcv2.c > +++ b/drivers/soc/imx/gpcv2.c > @@ -1337,6 +1337,9 @@ static int imx_pgc_domain_probe(struct platform_device *pdev) > regmap_update_bits(domain->regmap, domain->regs->map, > domain->bits.map, domain->bits.map); > > + if (of_property_read_bool(domain->dev->of_node, "power-supply")) > + 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2023-06-26 9:52 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-26 8:32 [PATCH v6 0/2] power: domain: handle power supplies that need interrupts Martin Kepplinger 2022-07-26 8:32 ` Martin Kepplinger 2022-07-26 8:32 ` [PATCH v6 1/2] power: domain: handle genpd correctly when needing interrupts Martin Kepplinger 2022-07-26 8:32 ` Martin Kepplinger 2022-07-26 15:07 ` Ulf Hansson 2022-07-26 15:07 ` Ulf Hansson 2022-07-26 18:33 ` Rafael J. Wysocki 2022-07-26 18:33 ` Rafael J. Wysocki 2022-07-28 9:19 ` Ulf Hansson 2022-07-28 9:19 ` Ulf Hansson 2022-08-19 9:17 ` Martin Kepplinger 2022-08-19 9:17 ` Martin Kepplinger 2022-08-19 14:53 ` Ulf Hansson 2022-08-19 14:53 ` Ulf Hansson 2022-08-22 8:38 ` Martin Kepplinger 2022-08-22 8:38 ` Martin Kepplinger 2022-08-24 13:30 ` Ulf Hansson 2022-08-24 13:30 ` Ulf Hansson 2022-08-25 7:06 ` Martin Kepplinger 2022-08-25 7:06 ` Martin Kepplinger 2022-09-23 13:55 ` Ulf Hansson 2022-09-23 13:55 ` Ulf Hansson 2022-09-26 9:52 ` Martin Kepplinger 2022-09-26 9:52 ` Martin Kepplinger 2023-06-21 18:20 ` Martin Kepplinger 2023-06-21 18:20 ` Martin Kepplinger 2023-06-26 9:50 ` Ulf Hansson 2023-06-26 9:50 ` Ulf Hansson 2022-07-26 8:32 ` [PATCH v6 2/2] soc: imx: gpcv2: fix suspend/resume by setting GENPD_FLAG_IRQ_ON Martin Kepplinger 2022-07-26 8:32 ` Martin Kepplinger 2022-07-26 15:08 ` Ulf Hansson 2022-07-26 15:08 ` Ulf Hansson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.