* [PATCH 0/2] PM / Domains: Support enter deepest state during suspend and probe failure @ 2019-03-06 13:35 ` Aisheng Dong 0 siblings, 0 replies; 18+ messages in thread From: Aisheng Dong @ 2019-03-06 13:35 UTC (permalink / raw) To: linux-pm Cc: Aisheng Dong, ulf.hansson, dongas86, khilman, rjw, dl-linux-imx, linux-arm-kernel Currently the generic power domain will power off the domain if all devices in it have been stopped during system suspend. It is done by checking if the domain is active in genpd_sync_power_off, then disable it. However, for power domains supporting multiple low power states, it may have already entered an intermediate low power state by runtime PM before system suspend and the status is already GPD_STATE_POWER_OFF which results in then the power domain stay at an intermediate low power state during system suspend. Then genpd_sync_power_off will keep it at the low power state instead of completely gate off it. And for the probe failture case, if there's no devices using it, we can fully gate off the domain instead of enter an intermediate state. Dong Aisheng (2): PM / Domains: Support enter deepest state for multiple states domains PM / Domains: Choose the deepest state to enter if no devices using it drivers/base/power/domain.c | 22 +++++++++++++++++++++- include/linux/pm_domain.h | 1 + 2 files changed, 22 insertions(+), 1 deletion(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 0/2] PM / Domains: Support enter deepest state during suspend and probe failure @ 2019-03-06 13:35 ` Aisheng Dong 0 siblings, 0 replies; 18+ messages in thread From: Aisheng Dong @ 2019-03-06 13:35 UTC (permalink / raw) To: linux-pm Cc: Aisheng Dong, ulf.hansson, dongas86, khilman, rjw, dl-linux-imx, linux-arm-kernel Currently the generic power domain will power off the domain if all devices in it have been stopped during system suspend. It is done by checking if the domain is active in genpd_sync_power_off, then disable it. However, for power domains supporting multiple low power states, it may have already entered an intermediate low power state by runtime PM before system suspend and the status is already GPD_STATE_POWER_OFF which results in then the power domain stay at an intermediate low power state during system suspend. Then genpd_sync_power_off will keep it at the low power state instead of completely gate off it. And for the probe failture case, if there's no devices using it, we can fully gate off the domain instead of enter an intermediate state. Dong Aisheng (2): PM / Domains: Support enter deepest state for multiple states domains PM / Domains: Choose the deepest state to enter if no devices using it drivers/base/power/domain.c | 22 +++++++++++++++++++++- include/linux/pm_domain.h | 1 + 2 files changed, 22 insertions(+), 1 deletion(-) -- 2.7.4 _______________________________________________ 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] 18+ messages in thread
* [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains 2019-03-06 13:35 ` Aisheng Dong @ 2019-03-06 13:35 ` Aisheng Dong -1 siblings, 0 replies; 18+ messages in thread From: Aisheng Dong @ 2019-03-06 13:35 UTC (permalink / raw) To: linux-pm Cc: Aisheng Dong, ulf.hansson, dongas86, khilman, rjw, dl-linux-imx, linux-arm-kernel Currently the generic power domain will power off the domain if all devices in it have been stopped during system suspend. It is done by checking if the domain is active in genpd_sync_power_off, then disable it. However, for power domains supporting multiple low power states, it may have already entered an intermediate low power state by runtime PM before system suspend and the status is already GPD_STATE_POWER_OFF which results in then the power domain stay at an intermediate low power state during system suspend. Then genpd_sync_power_off will keep it at the low power state instead of completely gate off it. Let's give the power domain a chance to switch to the deepest state in case it's already off but in an intermediate low power state. Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- drivers/base/power/domain.c | 18 +++++++++++++++++- include/linux/pm_domain.h | 1 + 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 61cd500..847a69e 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -959,7 +959,17 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock, { struct gpd_link *link; - if (!genpd_status_on(genpd) || genpd_is_always_on(genpd)) + /* + * Give the power domain a chance to switch to the deepest state in + * case it's already off but in an intermediate low power state. + */ + genpd->state_idx_saved = genpd->state_idx; + + if (genpd_is_always_on(genpd)) + return; + + if (!genpd_status_on(genpd) && + genpd->state_idx == (genpd->state_count - 1)) return; if (genpd->suspended_count != genpd->device_count) @@ -970,6 +980,9 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock, if (_genpd_power_off(genpd, false)) return; + if (genpd->status == GPD_STATE_POWER_OFF) + return; + genpd->status = GPD_STATE_POWER_OFF; list_for_each_entry(link, &genpd->slave_links, slave_node) { @@ -1017,6 +1030,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock, _genpd_power_on(genpd, false); + /* restore save power domain state after resume */ + genpd->state_idx = genpd->state_idx_saved; + genpd->status = GPD_STATE_ACTIVE; } diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 1ed5874..95db21f 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -124,6 +124,7 @@ struct generic_pm_domain { }; }; + unsigned int state_idx_saved; /* saved power state for recovery after system suspend/resume */ }; static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd) -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains @ 2019-03-06 13:35 ` Aisheng Dong 0 siblings, 0 replies; 18+ messages in thread From: Aisheng Dong @ 2019-03-06 13:35 UTC (permalink / raw) To: linux-pm Cc: Aisheng Dong, ulf.hansson, dongas86, khilman, rjw, dl-linux-imx, linux-arm-kernel Currently the generic power domain will power off the domain if all devices in it have been stopped during system suspend. It is done by checking if the domain is active in genpd_sync_power_off, then disable it. However, for power domains supporting multiple low power states, it may have already entered an intermediate low power state by runtime PM before system suspend and the status is already GPD_STATE_POWER_OFF which results in then the power domain stay at an intermediate low power state during system suspend. Then genpd_sync_power_off will keep it at the low power state instead of completely gate off it. Let's give the power domain a chance to switch to the deepest state in case it's already off but in an intermediate low power state. Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- drivers/base/power/domain.c | 18 +++++++++++++++++- include/linux/pm_domain.h | 1 + 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 61cd500..847a69e 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -959,7 +959,17 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock, { struct gpd_link *link; - if (!genpd_status_on(genpd) || genpd_is_always_on(genpd)) + /* + * Give the power domain a chance to switch to the deepest state in + * case it's already off but in an intermediate low power state. + */ + genpd->state_idx_saved = genpd->state_idx; + + if (genpd_is_always_on(genpd)) + return; + + if (!genpd_status_on(genpd) && + genpd->state_idx == (genpd->state_count - 1)) return; if (genpd->suspended_count != genpd->device_count) @@ -970,6 +980,9 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock, if (_genpd_power_off(genpd, false)) return; + if (genpd->status == GPD_STATE_POWER_OFF) + return; + genpd->status = GPD_STATE_POWER_OFF; list_for_each_entry(link, &genpd->slave_links, slave_node) { @@ -1017,6 +1030,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock, _genpd_power_on(genpd, false); + /* restore save power domain state after resume */ + genpd->state_idx = genpd->state_idx_saved; + genpd->status = GPD_STATE_ACTIVE; } diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 1ed5874..95db21f 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -124,6 +124,7 @@ struct generic_pm_domain { }; }; + unsigned int state_idx_saved; /* saved power state for recovery after system suspend/resume */ }; static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd) -- 2.7.4 _______________________________________________ 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] 18+ messages in thread
* Re: [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains 2019-03-06 13:35 ` Aisheng Dong @ 2019-03-06 15:17 ` Ulf Hansson -1 siblings, 0 replies; 18+ messages in thread From: Ulf Hansson @ 2019-03-06 15:17 UTC (permalink / raw) To: Aisheng Dong Cc: dongas86, khilman, linux-pm, rjw, dl-linux-imx, linux-arm-kernel On Wed, 6 Mar 2019 at 14:35, Aisheng Dong <aisheng.dong@nxp.com> wrote: > > Currently the generic power domain will power off the domain if all > devices in it have been stopped during system suspend. > > It is done by checking if the domain is active in genpd_sync_power_off, > then disable it. However, for power domains supporting multiple low power > states, it may have already entered an intermediate low power state by > runtime PM before system suspend and the status is already > GPD_STATE_POWER_OFF which results in then the power domain stay at an > intermediate low power state during system suspend. > Then genpd_sync_power_off will keep it at the low power state instead > of completely gate off it. I agree that this could be a concern. However, before we look for a solution, do you have practical use case where you observes this? > > Let's give the power domain a chance to switch to the deepest state in > case it's already off but in an intermediate low power state. > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > --- > drivers/base/power/domain.c | 18 +++++++++++++++++- > include/linux/pm_domain.h | 1 + > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 61cd500..847a69e 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -959,7 +959,17 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock, > { > struct gpd_link *link; > > - if (!genpd_status_on(genpd) || genpd_is_always_on(genpd)) > + /* > + * Give the power domain a chance to switch to the deepest state in > + * case it's already off but in an intermediate low power state. > + */ > + genpd->state_idx_saved = genpd->state_idx; > + > + if (genpd_is_always_on(genpd)) > + return; > + > + if (!genpd_status_on(genpd) && > + genpd->state_idx == (genpd->state_count - 1)) > return; This means that the ->power_off() callback may be called twice in a raw, for the genpd in question and in cases of multiple idle states. It could be a problem. Well, currently it may not be a real issue, as I doubt there is rather few genpd backend drivers supporting multiple idle states, but still. > > if (genpd->suspended_count != genpd->device_count) > @@ -970,6 +980,9 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock, > if (_genpd_power_off(genpd, false)) > return; > > + if (genpd->status == GPD_STATE_POWER_OFF) > + return; > + > genpd->status = GPD_STATE_POWER_OFF; > > list_for_each_entry(link, &genpd->slave_links, slave_node) { > @@ -1017,6 +1030,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock, > > _genpd_power_on(genpd, false); > > + /* restore save power domain state after resume */ > + genpd->state_idx = genpd->state_idx_saved; > + > genpd->status = GPD_STATE_ACTIVE; > } > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 1ed5874..95db21f 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -124,6 +124,7 @@ struct generic_pm_domain { > }; > }; > > + unsigned int state_idx_saved; /* saved power state for recovery after system suspend/resume */ > }; > > static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd) > -- > 2.7.4 > I need to think about this a bit more, let me get back to you. Kind regards Uffe ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains @ 2019-03-06 15:17 ` Ulf Hansson 0 siblings, 0 replies; 18+ messages in thread From: Ulf Hansson @ 2019-03-06 15:17 UTC (permalink / raw) To: Aisheng Dong Cc: dongas86, khilman, linux-pm, rjw, dl-linux-imx, linux-arm-kernel On Wed, 6 Mar 2019 at 14:35, Aisheng Dong <aisheng.dong@nxp.com> wrote: > > Currently the generic power domain will power off the domain if all > devices in it have been stopped during system suspend. > > It is done by checking if the domain is active in genpd_sync_power_off, > then disable it. However, for power domains supporting multiple low power > states, it may have already entered an intermediate low power state by > runtime PM before system suspend and the status is already > GPD_STATE_POWER_OFF which results in then the power domain stay at an > intermediate low power state during system suspend. > Then genpd_sync_power_off will keep it at the low power state instead > of completely gate off it. I agree that this could be a concern. However, before we look for a solution, do you have practical use case where you observes this? > > Let's give the power domain a chance to switch to the deepest state in > case it's already off but in an intermediate low power state. > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > --- > drivers/base/power/domain.c | 18 +++++++++++++++++- > include/linux/pm_domain.h | 1 + > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 61cd500..847a69e 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -959,7 +959,17 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock, > { > struct gpd_link *link; > > - if (!genpd_status_on(genpd) || genpd_is_always_on(genpd)) > + /* > + * Give the power domain a chance to switch to the deepest state in > + * case it's already off but in an intermediate low power state. > + */ > + genpd->state_idx_saved = genpd->state_idx; > + > + if (genpd_is_always_on(genpd)) > + return; > + > + if (!genpd_status_on(genpd) && > + genpd->state_idx == (genpd->state_count - 1)) > return; This means that the ->power_off() callback may be called twice in a raw, for the genpd in question and in cases of multiple idle states. It could be a problem. Well, currently it may not be a real issue, as I doubt there is rather few genpd backend drivers supporting multiple idle states, but still. > > if (genpd->suspended_count != genpd->device_count) > @@ -970,6 +980,9 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock, > if (_genpd_power_off(genpd, false)) > return; > > + if (genpd->status == GPD_STATE_POWER_OFF) > + return; > + > genpd->status = GPD_STATE_POWER_OFF; > > list_for_each_entry(link, &genpd->slave_links, slave_node) { > @@ -1017,6 +1030,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock, > > _genpd_power_on(genpd, false); > > + /* restore save power domain state after resume */ > + genpd->state_idx = genpd->state_idx_saved; > + > genpd->status = GPD_STATE_ACTIVE; > } > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 1ed5874..95db21f 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -124,6 +124,7 @@ struct generic_pm_domain { > }; > }; > > + unsigned int state_idx_saved; /* saved power state for recovery after system suspend/resume */ > }; > > static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd) > -- > 2.7.4 > I need to think about this a bit more, let me get back to you. 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] 18+ messages in thread
* RE: [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains 2019-03-06 15:17 ` Ulf Hansson @ 2019-03-06 15:36 ` Aisheng Dong -1 siblings, 0 replies; 18+ messages in thread From: Aisheng Dong @ 2019-03-06 15:36 UTC (permalink / raw) To: Ulf Hansson Cc: dongas86, khilman, linux-pm, rjw, dl-linux-imx, linux-arm-kernel > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Wednesday, March 6, 2019 11:18 PM> > On Wed, 6 Mar 2019 at 14:35, Aisheng Dong <aisheng.dong@nxp.com> > wrote: > > > > Currently the generic power domain will power off the domain if all > > devices in it have been stopped during system suspend. > > > > It is done by checking if the domain is active in > > genpd_sync_power_off, then disable it. However, for power domains > > supporting multiple low power states, it may have already entered an > > intermediate low power state by runtime PM before system suspend and > > the status is already GPD_STATE_POWER_OFF which results in then the > > power domain stay at an intermediate low power state during system > suspend. > > Then genpd_sync_power_off will keep it at the low power state instead > > of completely gate off it. > > I agree that this could be a concern. However, before we look for a solution, do > you have practical use case where you observes this? > Yes, this solution[1] is used by NXP internally for former releases that we have two Level states for power Domains[2]. We use state 0 (Low Power Mode with state retention) for device runtime pm support and state 1 (power off with no state retention) for system suspend resume case. Without [1], we will meet the issue that all domains runtime suspended will stay at low power mode instead of power off during system suspend which consumes more power. 1. https://source.codeaurora.org/external/imx/linux-imx/log/drivers/base/power?h=imx_4.14.78_1.0.0_ga 2. https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/soc/imx/pm-domains.c?h=imx_4.14.78_1.0.0_ga#n325 > > > > Let's give the power domain a chance to switch to the deepest state in > > case it's already off but in an intermediate low power state. > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > --- > > drivers/base/power/domain.c | 18 +++++++++++++++++- > > include/linux/pm_domain.h | 1 + > > 2 files changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 61cd500..847a69e 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -959,7 +959,17 @@ static void genpd_sync_power_off(struct > > generic_pm_domain *genpd, bool use_lock, { > > struct gpd_link *link; > > > > - if (!genpd_status_on(genpd) || genpd_is_always_on(genpd)) > > + /* > > + * Give the power domain a chance to switch to the deepest state > in > > + * case it's already off but in an intermediate low power state. > > + */ > > + genpd->state_idx_saved = genpd->state_idx; > > + > > + if (genpd_is_always_on(genpd)) > > + return; > > + > > + if (!genpd_status_on(genpd) && > > + genpd->state_idx == (genpd->state_count - 1)) > > return; > > This means that the ->power_off() callback may be called twice in a raw, for > the genpd in question and in cases of multiple idle states. > It could be a problem. > > Well, currently it may not be a real issue, as I doubt there is rather few genpd > backend drivers supporting multiple idle states, but still. > So far, I've no idea what the problem could be as there's no multi states power domain drivers in tree. For IMX, there's no problem as the driver could handle it well. The core code just provides an interface for driver to handle such requirement. Do you know who else using such power domain with multi states? I guess TI, but did not see their code in tree. > > > > if (genpd->suspended_count != genpd->device_count) @@ -970,6 > > +980,9 @@ static void genpd_sync_power_off(struct generic_pm_domain > *genpd, bool use_lock, > > if (_genpd_power_off(genpd, false)) > > return; > > > > + if (genpd->status == GPD_STATE_POWER_OFF) > > + return; > > + > > genpd->status = GPD_STATE_POWER_OFF; > > > > list_for_each_entry(link, &genpd->slave_links, slave_node) { > > @@ -1017,6 +1030,9 @@ static void genpd_sync_power_on(struct > > generic_pm_domain *genpd, bool use_lock, > > > > _genpd_power_on(genpd, false); > > > > + /* restore save power domain state after resume */ > > + genpd->state_idx = genpd->state_idx_saved; > > + > > genpd->status = GPD_STATE_ACTIVE; } > > > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > > index 1ed5874..95db21f 100644 > > --- a/include/linux/pm_domain.h > > +++ b/include/linux/pm_domain.h > > @@ -124,6 +124,7 @@ struct generic_pm_domain { > > }; > > }; > > > > + unsigned int state_idx_saved; /* saved power state for > > + recovery after system suspend/resume */ > > }; > > > > static inline struct generic_pm_domain *pd_to_genpd(struct > > dev_pm_domain *pd) > > -- > > 2.7.4 > > > > I need to think about this a bit more, let me get back to you. > Thanks for the help. Regards Dong Aisheng > Kind regards > Uffe ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains @ 2019-03-06 15:36 ` Aisheng Dong 0 siblings, 0 replies; 18+ messages in thread From: Aisheng Dong @ 2019-03-06 15:36 UTC (permalink / raw) To: Ulf Hansson Cc: dongas86, khilman, linux-pm, rjw, dl-linux-imx, linux-arm-kernel > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Wednesday, March 6, 2019 11:18 PM> > On Wed, 6 Mar 2019 at 14:35, Aisheng Dong <aisheng.dong@nxp.com> > wrote: > > > > Currently the generic power domain will power off the domain if all > > devices in it have been stopped during system suspend. > > > > It is done by checking if the domain is active in > > genpd_sync_power_off, then disable it. However, for power domains > > supporting multiple low power states, it may have already entered an > > intermediate low power state by runtime PM before system suspend and > > the status is already GPD_STATE_POWER_OFF which results in then the > > power domain stay at an intermediate low power state during system > suspend. > > Then genpd_sync_power_off will keep it at the low power state instead > > of completely gate off it. > > I agree that this could be a concern. However, before we look for a solution, do > you have practical use case where you observes this? > Yes, this solution[1] is used by NXP internally for former releases that we have two Level states for power Domains[2]. We use state 0 (Low Power Mode with state retention) for device runtime pm support and state 1 (power off with no state retention) for system suspend resume case. Without [1], we will meet the issue that all domains runtime suspended will stay at low power mode instead of power off during system suspend which consumes more power. 1. https://source.codeaurora.org/external/imx/linux-imx/log/drivers/base/power?h=imx_4.14.78_1.0.0_ga 2. https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/soc/imx/pm-domains.c?h=imx_4.14.78_1.0.0_ga#n325 > > > > Let's give the power domain a chance to switch to the deepest state in > > case it's already off but in an intermediate low power state. > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > --- > > drivers/base/power/domain.c | 18 +++++++++++++++++- > > include/linux/pm_domain.h | 1 + > > 2 files changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 61cd500..847a69e 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -959,7 +959,17 @@ static void genpd_sync_power_off(struct > > generic_pm_domain *genpd, bool use_lock, { > > struct gpd_link *link; > > > > - if (!genpd_status_on(genpd) || genpd_is_always_on(genpd)) > > + /* > > + * Give the power domain a chance to switch to the deepest state > in > > + * case it's already off but in an intermediate low power state. > > + */ > > + genpd->state_idx_saved = genpd->state_idx; > > + > > + if (genpd_is_always_on(genpd)) > > + return; > > + > > + if (!genpd_status_on(genpd) && > > + genpd->state_idx == (genpd->state_count - 1)) > > return; > > This means that the ->power_off() callback may be called twice in a raw, for > the genpd in question and in cases of multiple idle states. > It could be a problem. > > Well, currently it may not be a real issue, as I doubt there is rather few genpd > backend drivers supporting multiple idle states, but still. > So far, I've no idea what the problem could be as there's no multi states power domain drivers in tree. For IMX, there's no problem as the driver could handle it well. The core code just provides an interface for driver to handle such requirement. Do you know who else using such power domain with multi states? I guess TI, but did not see their code in tree. > > > > if (genpd->suspended_count != genpd->device_count) @@ -970,6 > > +980,9 @@ static void genpd_sync_power_off(struct generic_pm_domain > *genpd, bool use_lock, > > if (_genpd_power_off(genpd, false)) > > return; > > > > + if (genpd->status == GPD_STATE_POWER_OFF) > > + return; > > + > > genpd->status = GPD_STATE_POWER_OFF; > > > > list_for_each_entry(link, &genpd->slave_links, slave_node) { > > @@ -1017,6 +1030,9 @@ static void genpd_sync_power_on(struct > > generic_pm_domain *genpd, bool use_lock, > > > > _genpd_power_on(genpd, false); > > > > + /* restore save power domain state after resume */ > > + genpd->state_idx = genpd->state_idx_saved; > > + > > genpd->status = GPD_STATE_ACTIVE; } > > > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > > index 1ed5874..95db21f 100644 > > --- a/include/linux/pm_domain.h > > +++ b/include/linux/pm_domain.h > > @@ -124,6 +124,7 @@ struct generic_pm_domain { > > }; > > }; > > > > + unsigned int state_idx_saved; /* saved power state for > > + recovery after system suspend/resume */ > > }; > > > > static inline struct generic_pm_domain *pd_to_genpd(struct > > dev_pm_domain *pd) > > -- > > 2.7.4 > > > > I need to think about this a bit more, let me get back to you. > Thanks for the help. Regards Dong Aisheng > 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] 18+ messages in thread
* RE: [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains 2019-03-06 15:36 ` Aisheng Dong @ 2019-03-07 16:01 ` Kevin Hilman -1 siblings, 0 replies; 18+ messages in thread From: Kevin Hilman @ 2019-03-07 16:01 UTC (permalink / raw) To: Aisheng Dong, Ulf Hansson Cc: dongas86, khilman, linux-pm, rjw, dl-linux-imx, linux-arm-kernel Aisheng Dong <aisheng.dong@nxp.com> writes: >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >> Sent: Wednesday, March 6, 2019 11:18 PM> >> On Wed, 6 Mar 2019 at 14:35, Aisheng Dong <aisheng.dong@nxp.com> >> wrote: >> > >> > Currently the generic power domain will power off the domain if all >> > devices in it have been stopped during system suspend. >> > >> > It is done by checking if the domain is active in >> > genpd_sync_power_off, then disable it. However, for power domains >> > supporting multiple low power states, it may have already entered an >> > intermediate low power state by runtime PM before system suspend and >> > the status is already GPD_STATE_POWER_OFF which results in then the >> > power domain stay at an intermediate low power state during system >> suspend. >> > Then genpd_sync_power_off will keep it at the low power state instead >> > of completely gate off it. >> >> I agree that this could be a concern. However, before we look for a solution, do >> you have practical use case where you observes this? >> > > Yes, this solution[1] is used by NXP internally for former releases that we have two > Level states for power Domains[2]. > We use state 0 (Low Power Mode with state retention) for device runtime pm support > and state 1 (power off with no state retention) for system suspend resume case. > > Without [1], we will meet the issue that all domains runtime suspended will stay at > low power mode instead of power off during system suspend which consumes more power. > > 1. https://source.codeaurora.org/external/imx/linux-imx/log/drivers/base/power?h=imx_4.14.78_1.0.0_ga > 2. https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/soc/imx/pm-domains.c?h=imx_4.14.78_1.0.0_ga#n325 > >> > >> > Let's give the power domain a chance to switch to the deepest state in >> > case it's already off but in an intermediate low power state. >> > >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> >> > --- >> > drivers/base/power/domain.c | 18 +++++++++++++++++- >> > include/linux/pm_domain.h | 1 + >> > 2 files changed, 18 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> > index 61cd500..847a69e 100644 >> > --- a/drivers/base/power/domain.c >> > +++ b/drivers/base/power/domain.c >> > @@ -959,7 +959,17 @@ static void genpd_sync_power_off(struct >> > generic_pm_domain *genpd, bool use_lock, { >> > struct gpd_link *link; >> > >> > - if (!genpd_status_on(genpd) || genpd_is_always_on(genpd)) >> > + /* >> > + * Give the power domain a chance to switch to the deepest state >> in >> > + * case it's already off but in an intermediate low power state. >> > + */ >> > + genpd->state_idx_saved = genpd->state_idx; >> > + >> > + if (genpd_is_always_on(genpd)) >> > + return; >> > + >> > + if (!genpd_status_on(genpd) && >> > + genpd->state_idx == (genpd->state_count - 1)) >> > return; >> >> This means that the ->power_off() callback may be called twice in a raw, for >> the genpd in question and in cases of multiple idle states. >> It could be a problem. >> >> Well, currently it may not be a real issue, as I doubt there is rather few genpd >> backend drivers supporting multiple idle states, but still. >> > > So far, I've no idea what the problem could be as there's no multi states power domain > drivers in tree. A problem could arise if the drivers need to do something before they can switch from retention to OFF (e.g. save context). AFAICT without seeing the affected drivers in your solution, I can't tell if the drivers have an opportunity to save context to prepare for off-mode. > For IMX, there's no problem as the driver could handle it well. In that case, I guess you're assuming that system suspsend will always hit off-mode, so all your drivers always save context in their suspend callbacks? > The core code just provides an interface for driver to handle such requirement. I think we're up against a terminology problem/confusion since we added multiple states around what does "off" really mean. For example, should "off" mean in the deepest, possible state, or does it mean idle in the last selected state? Before we had multiple states, it always meant the former, and the code was built around the idea of really, truly off (zero volts, no retention.) With multiple states, it's now a bit ambigious. In this series, you're using it to mean both, which is part of the confusion. Stated differently, while we technically have support for mulitple states, in the core we still just have 2: enum gpd_status { GPD_STATE_ACTIVE = 0, /* PM domain is active */ GPD_STATE_POWER_OFF, /* PM domain is off */ }; So bascially we have to make a decsion whether retention is _ACTIVE or _OFF. This series is assuming it should be _OFF, but when we were adding multiple states, I had always assumed retention states would be considered _ACTIVE. Yes, I know tha neither "active" nor "off" accurately defines retention, but since there is still voltage applied, it may not be active, but it's definitely not off. When we were adding multiple states, I had always imagined domains with multiple states would handle this kind of thing with a governor. Retention states whould leave the domain in GPD_STATE_ACTIVE, and the governor would then determine when to switch to the real off state. Note that this gets you more flexiblity than just switching to off in suspend. You could also use the governor to let the domain switch to off based on other criteria such as long idle time, or QoS criteria. It seems you may be the first upstream solution to start really exercising the multiple states, so we may still have some kinks to work out, but thanks for pushing your changes upstream so we can work all this out together. Thanks, Kevin ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains @ 2019-03-07 16:01 ` Kevin Hilman 0 siblings, 0 replies; 18+ messages in thread From: Kevin Hilman @ 2019-03-07 16:01 UTC (permalink / raw) To: Aisheng Dong, Ulf Hansson Cc: dongas86, khilman, linux-pm, rjw, dl-linux-imx, linux-arm-kernel Aisheng Dong <aisheng.dong@nxp.com> writes: >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >> Sent: Wednesday, March 6, 2019 11:18 PM> >> On Wed, 6 Mar 2019 at 14:35, Aisheng Dong <aisheng.dong@nxp.com> >> wrote: >> > >> > Currently the generic power domain will power off the domain if all >> > devices in it have been stopped during system suspend. >> > >> > It is done by checking if the domain is active in >> > genpd_sync_power_off, then disable it. However, for power domains >> > supporting multiple low power states, it may have already entered an >> > intermediate low power state by runtime PM before system suspend and >> > the status is already GPD_STATE_POWER_OFF which results in then the >> > power domain stay at an intermediate low power state during system >> suspend. >> > Then genpd_sync_power_off will keep it at the low power state instead >> > of completely gate off it. >> >> I agree that this could be a concern. However, before we look for a solution, do >> you have practical use case where you observes this? >> > > Yes, this solution[1] is used by NXP internally for former releases that we have two > Level states for power Domains[2]. > We use state 0 (Low Power Mode with state retention) for device runtime pm support > and state 1 (power off with no state retention) for system suspend resume case. > > Without [1], we will meet the issue that all domains runtime suspended will stay at > low power mode instead of power off during system suspend which consumes more power. > > 1. https://source.codeaurora.org/external/imx/linux-imx/log/drivers/base/power?h=imx_4.14.78_1.0.0_ga > 2. https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/soc/imx/pm-domains.c?h=imx_4.14.78_1.0.0_ga#n325 > >> > >> > Let's give the power domain a chance to switch to the deepest state in >> > case it's already off but in an intermediate low power state. >> > >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> >> > --- >> > drivers/base/power/domain.c | 18 +++++++++++++++++- >> > include/linux/pm_domain.h | 1 + >> > 2 files changed, 18 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> > index 61cd500..847a69e 100644 >> > --- a/drivers/base/power/domain.c >> > +++ b/drivers/base/power/domain.c >> > @@ -959,7 +959,17 @@ static void genpd_sync_power_off(struct >> > generic_pm_domain *genpd, bool use_lock, { >> > struct gpd_link *link; >> > >> > - if (!genpd_status_on(genpd) || genpd_is_always_on(genpd)) >> > + /* >> > + * Give the power domain a chance to switch to the deepest state >> in >> > + * case it's already off but in an intermediate low power state. >> > + */ >> > + genpd->state_idx_saved = genpd->state_idx; >> > + >> > + if (genpd_is_always_on(genpd)) >> > + return; >> > + >> > + if (!genpd_status_on(genpd) && >> > + genpd->state_idx == (genpd->state_count - 1)) >> > return; >> >> This means that the ->power_off() callback may be called twice in a raw, for >> the genpd in question and in cases of multiple idle states. >> It could be a problem. >> >> Well, currently it may not be a real issue, as I doubt there is rather few genpd >> backend drivers supporting multiple idle states, but still. >> > > So far, I've no idea what the problem could be as there's no multi states power domain > drivers in tree. A problem could arise if the drivers need to do something before they can switch from retention to OFF (e.g. save context). AFAICT without seeing the affected drivers in your solution, I can't tell if the drivers have an opportunity to save context to prepare for off-mode. > For IMX, there's no problem as the driver could handle it well. In that case, I guess you're assuming that system suspsend will always hit off-mode, so all your drivers always save context in their suspend callbacks? > The core code just provides an interface for driver to handle such requirement. I think we're up against a terminology problem/confusion since we added multiple states around what does "off" really mean. For example, should "off" mean in the deepest, possible state, or does it mean idle in the last selected state? Before we had multiple states, it always meant the former, and the code was built around the idea of really, truly off (zero volts, no retention.) With multiple states, it's now a bit ambigious. In this series, you're using it to mean both, which is part of the confusion. Stated differently, while we technically have support for mulitple states, in the core we still just have 2: enum gpd_status { GPD_STATE_ACTIVE = 0, /* PM domain is active */ GPD_STATE_POWER_OFF, /* PM domain is off */ }; So bascially we have to make a decsion whether retention is _ACTIVE or _OFF. This series is assuming it should be _OFF, but when we were adding multiple states, I had always assumed retention states would be considered _ACTIVE. Yes, I know tha neither "active" nor "off" accurately defines retention, but since there is still voltage applied, it may not be active, but it's definitely not off. When we were adding multiple states, I had always imagined domains with multiple states would handle this kind of thing with a governor. Retention states whould leave the domain in GPD_STATE_ACTIVE, and the governor would then determine when to switch to the real off state. Note that this gets you more flexiblity than just switching to off in suspend. You could also use the governor to let the domain switch to off based on other criteria such as long idle time, or QoS criteria. It seems you may be the first upstream solution to start really exercising the multiple states, so we may still have some kinks to work out, but thanks for pushing your changes upstream so we can work all this out together. Thanks, Kevin _______________________________________________ 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] 18+ messages in thread
* RE: [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains 2019-03-07 16:01 ` Kevin Hilman @ 2019-03-15 2:08 ` Aisheng Dong -1 siblings, 0 replies; 18+ messages in thread From: Aisheng Dong @ 2019-03-15 2:08 UTC (permalink / raw) To: Kevin Hilman, Ulf Hansson Cc: dongas86, khilman, linux-pm, rjw, dl-linux-imx, linux-arm-kernel Hi Kevin, Thanks for the detailed review. > From: Kevin Hilman [mailto:khilman@baylibre.com] >> Aisheng Dong <aisheng.dong@nxp.com> writes: > >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > >> Sent: Wednesday, March 6, 2019 11:18 PM> On Wed, 6 Mar 2019 at 14:35, > >> Aisheng Dong <aisheng.dong@nxp.com> > >> wrote: > >> > > >> > Currently the generic power domain will power off the domain if all > >> > devices in it have been stopped during system suspend. > >> > > >> > It is done by checking if the domain is active in > >> > genpd_sync_power_off, then disable it. However, for power domains > >> > supporting multiple low power states, it may have already entered > >> > an intermediate low power state by runtime PM before system suspend > >> > and the status is already GPD_STATE_POWER_OFF which results in then > >> > the power domain stay at an intermediate low power state during > >> > system > >> suspend. > >> > Then genpd_sync_power_off will keep it at the low power state > >> > instead of completely gate off it. > >> > >> I agree that this could be a concern. However, before we look for a > >> solution, do you have practical use case where you observes this? > >> > > > > Yes, this solution[1] is used by NXP internally for former releases > > that we have two Level states for power Domains[2]. > > We use state 0 (Low Power Mode with state retention) for device > > runtime pm support and state 1 (power off with no state retention) for > system suspend resume case. > > > > Without [1], we will meet the issue that all domains runtime suspended > > will stay at low power mode instead of power off during system suspend > which consumes more power. > > > >> > > >> > Let's give the power domain a chance to switch to the deepest state > >> > in case it's already off but in an intermediate low power state. > >> > > >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > >> > --- > >> > drivers/base/power/domain.c | 18 +++++++++++++++++- > >> > include/linux/pm_domain.h | 1 + > >> > 2 files changed, 18 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/base/power/domain.c > >> > b/drivers/base/power/domain.c index 61cd500..847a69e 100644 > >> > --- a/drivers/base/power/domain.c > >> > +++ b/drivers/base/power/domain.c > >> > @@ -959,7 +959,17 @@ static void genpd_sync_power_off(struct > >> > generic_pm_domain *genpd, bool use_lock, { > >> > struct gpd_link *link; > >> > > >> > - if (!genpd_status_on(genpd) || genpd_is_always_on(genpd)) > >> > + /* > >> > + * Give the power domain a chance to switch to the deepest > >> > + state > >> in > >> > + * case it's already off but in an intermediate low power state. > >> > + */ > >> > + genpd->state_idx_saved = genpd->state_idx; > >> > + > >> > + if (genpd_is_always_on(genpd)) > >> > + return; > >> > + > >> > + if (!genpd_status_on(genpd) && > >> > + genpd->state_idx == (genpd->state_count - 1)) > >> > return; > >> > >> This means that the ->power_off() callback may be called twice in a > >> raw, for the genpd in question and in cases of multiple idle states. > >> It could be a problem. > >> > >> Well, currently it may not be a real issue, as I doubt there is > >> rather few genpd backend drivers supporting multiple idle states, but still. > >> > > > > So far, I've no idea what the problem could be as there's no multi > > states power domain drivers in tree. > > A problem could arise if the drivers need to do something before they can > switch from retention to OFF (e.g. save context). > > AFAICT without seeing the affected drivers in your solution, I can't tell if the > drivers have an opportunity to save context to prepare for off-mode. > > > For IMX, there's no problem as the driver could handle it well. > > In that case, I guess you're assuming that system suspsend will always hit > off-mode, so all your drivers always save context in their suspend callbacks? > Yes, we did like that so far as other drivers doing in kernel right now Because there's still no notify mechanism in PM core to tell the driver whether the state may be retained during system sleep transition. We probably could add that support in PM core in the future. > > The core code just provides an interface for driver to handle such > requirement. > > I think we're up against a terminology problem/confusion since we added > multiple states around what does "off" really mean. For example, should > "off" mean in the deepest, possible state, or does it mean idle in the last > selected state? > Yes, i agree. Currently "off" seems one of available low power states opposed to "on". > Before we had multiple states, it always meant the former, and the code was > built around the idea of really, truly off (zero volts, no > retention.) With multiple states, it's now a bit ambigious. In this series, > you're using it to mean both, which is part of the confusion. > > Stated differently, while we technically have support for mulitple states, in the > core we still just have 2: > > enum gpd_status { > GPD_STATE_ACTIVE = 0, /* PM domain is active */ > GPD_STATE_POWER_OFF, /* PM domain is off */ > }; > > So bascially we have to make a decsion whether retention is _ACTIVE or _OFF. > This series is assuming it should be _OFF, but when we were adding multiple > states, I had always assumed retention states would be considered _ACTIVE. > Yes, I know tha neither "active" nor "off" > accurately defines retention, but since there is still voltage applied, it may not > be active, but it's definitely not off. > > When we were adding multiple states, I had always imagined domains with > multiple states would handle this kind of thing with a governor. > Retention states whould leave the domain in GPD_STATE_ACTIVE, and the > governor would then determine when to switch to the real off state. > The current implementation in kernel seems not very like that. Governor is responsible for selecting a suitable low power state starting from deepest to enter and PM core will use it to power down the domain and regards it as GPD_STATE_POWER_OFF. That means from PM core point of view, all low power states decided by governor to enter are "off". And this is for runtime pm management. When system sleep, PM core will bypass the governor and directly choose the deepest state of power off to save the maximum power. I can't say this is not good as it's exactly what we're using internally and can meet our requirements. However, I do believe we could improve the PM core to be aware of RETENTION state (e.g. add a flag for state definition?) and notify the driver whether the state will be lost and saving&restore required. static bool default_power_down_ok(struct dev_pm_domain *pd) { ... /* Find a state to power down to, starting from the deepest. */ while (!__default_power_down_ok(pd, genpd->state_idx)) { if (genpd->state_idx == 0) { genpd->cached_power_down_ok = false; break; } genpd->state_idx--; } ... } static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on, unsigned int depth) { ... if (genpd->gov && genpd->gov->power_down_ok) { if (!genpd->gov->power_down_ok(&genpd->domain)) return -EAGAIN; } ret = _genpd_power_off(genpd, true); genpd->status = GPD_STATE_POWER_OFF; ... } > Note that this gets you more flexiblity than just switching to off in suspend. > You could also use the governor to let the domain switch to off based on other > criteria such as long idle time, or QoS criteria. > > It seems you may be the first upstream solution to start really exercising the > multiple states, so we may still have some kinks to work out, but thanks for > pushing your changes upstream so we can work all this out together. > It's great to have you professional guys' suggestion and helping review. And I'm exciting to co-work with you for this solution. Regards Dong Aisheng > Thanks, > > Kevin > > > > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains @ 2019-03-15 2:08 ` Aisheng Dong 0 siblings, 0 replies; 18+ messages in thread From: Aisheng Dong @ 2019-03-15 2:08 UTC (permalink / raw) To: Kevin Hilman, Ulf Hansson Cc: dongas86, khilman, linux-pm, rjw, dl-linux-imx, linux-arm-kernel Hi Kevin, Thanks for the detailed review. > From: Kevin Hilman [mailto:khilman@baylibre.com] >> Aisheng Dong <aisheng.dong@nxp.com> writes: > >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > >> Sent: Wednesday, March 6, 2019 11:18 PM> On Wed, 6 Mar 2019 at 14:35, > >> Aisheng Dong <aisheng.dong@nxp.com> > >> wrote: > >> > > >> > Currently the generic power domain will power off the domain if all > >> > devices in it have been stopped during system suspend. > >> > > >> > It is done by checking if the domain is active in > >> > genpd_sync_power_off, then disable it. However, for power domains > >> > supporting multiple low power states, it may have already entered > >> > an intermediate low power state by runtime PM before system suspend > >> > and the status is already GPD_STATE_POWER_OFF which results in then > >> > the power domain stay at an intermediate low power state during > >> > system > >> suspend. > >> > Then genpd_sync_power_off will keep it at the low power state > >> > instead of completely gate off it. > >> > >> I agree that this could be a concern. However, before we look for a > >> solution, do you have practical use case where you observes this? > >> > > > > Yes, this solution[1] is used by NXP internally for former releases > > that we have two Level states for power Domains[2]. > > We use state 0 (Low Power Mode with state retention) for device > > runtime pm support and state 1 (power off with no state retention) for > system suspend resume case. > > > > Without [1], we will meet the issue that all domains runtime suspended > > will stay at low power mode instead of power off during system suspend > which consumes more power. > > > >> > > >> > Let's give the power domain a chance to switch to the deepest state > >> > in case it's already off but in an intermediate low power state. > >> > > >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > >> > --- > >> > drivers/base/power/domain.c | 18 +++++++++++++++++- > >> > include/linux/pm_domain.h | 1 + > >> > 2 files changed, 18 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/base/power/domain.c > >> > b/drivers/base/power/domain.c index 61cd500..847a69e 100644 > >> > --- a/drivers/base/power/domain.c > >> > +++ b/drivers/base/power/domain.c > >> > @@ -959,7 +959,17 @@ static void genpd_sync_power_off(struct > >> > generic_pm_domain *genpd, bool use_lock, { > >> > struct gpd_link *link; > >> > > >> > - if (!genpd_status_on(genpd) || genpd_is_always_on(genpd)) > >> > + /* > >> > + * Give the power domain a chance to switch to the deepest > >> > + state > >> in > >> > + * case it's already off but in an intermediate low power state. > >> > + */ > >> > + genpd->state_idx_saved = genpd->state_idx; > >> > + > >> > + if (genpd_is_always_on(genpd)) > >> > + return; > >> > + > >> > + if (!genpd_status_on(genpd) && > >> > + genpd->state_idx == (genpd->state_count - 1)) > >> > return; > >> > >> This means that the ->power_off() callback may be called twice in a > >> raw, for the genpd in question and in cases of multiple idle states. > >> It could be a problem. > >> > >> Well, currently it may not be a real issue, as I doubt there is > >> rather few genpd backend drivers supporting multiple idle states, but still. > >> > > > > So far, I've no idea what the problem could be as there's no multi > > states power domain drivers in tree. > > A problem could arise if the drivers need to do something before they can > switch from retention to OFF (e.g. save context). > > AFAICT without seeing the affected drivers in your solution, I can't tell if the > drivers have an opportunity to save context to prepare for off-mode. > > > For IMX, there's no problem as the driver could handle it well. > > In that case, I guess you're assuming that system suspsend will always hit > off-mode, so all your drivers always save context in their suspend callbacks? > Yes, we did like that so far as other drivers doing in kernel right now Because there's still no notify mechanism in PM core to tell the driver whether the state may be retained during system sleep transition. We probably could add that support in PM core in the future. > > The core code just provides an interface for driver to handle such > requirement. > > I think we're up against a terminology problem/confusion since we added > multiple states around what does "off" really mean. For example, should > "off" mean in the deepest, possible state, or does it mean idle in the last > selected state? > Yes, i agree. Currently "off" seems one of available low power states opposed to "on". > Before we had multiple states, it always meant the former, and the code was > built around the idea of really, truly off (zero volts, no > retention.) With multiple states, it's now a bit ambigious. In this series, > you're using it to mean both, which is part of the confusion. > > Stated differently, while we technically have support for mulitple states, in the > core we still just have 2: > > enum gpd_status { > GPD_STATE_ACTIVE = 0, /* PM domain is active */ > GPD_STATE_POWER_OFF, /* PM domain is off */ > }; > > So bascially we have to make a decsion whether retention is _ACTIVE or _OFF. > This series is assuming it should be _OFF, but when we were adding multiple > states, I had always assumed retention states would be considered _ACTIVE. > Yes, I know tha neither "active" nor "off" > accurately defines retention, but since there is still voltage applied, it may not > be active, but it's definitely not off. > > When we were adding multiple states, I had always imagined domains with > multiple states would handle this kind of thing with a governor. > Retention states whould leave the domain in GPD_STATE_ACTIVE, and the > governor would then determine when to switch to the real off state. > The current implementation in kernel seems not very like that. Governor is responsible for selecting a suitable low power state starting from deepest to enter and PM core will use it to power down the domain and regards it as GPD_STATE_POWER_OFF. That means from PM core point of view, all low power states decided by governor to enter are "off". And this is for runtime pm management. When system sleep, PM core will bypass the governor and directly choose the deepest state of power off to save the maximum power. I can't say this is not good as it's exactly what we're using internally and can meet our requirements. However, I do believe we could improve the PM core to be aware of RETENTION state (e.g. add a flag for state definition?) and notify the driver whether the state will be lost and saving&restore required. static bool default_power_down_ok(struct dev_pm_domain *pd) { ... /* Find a state to power down to, starting from the deepest. */ while (!__default_power_down_ok(pd, genpd->state_idx)) { if (genpd->state_idx == 0) { genpd->cached_power_down_ok = false; break; } genpd->state_idx--; } ... } static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on, unsigned int depth) { ... if (genpd->gov && genpd->gov->power_down_ok) { if (!genpd->gov->power_down_ok(&genpd->domain)) return -EAGAIN; } ret = _genpd_power_off(genpd, true); genpd->status = GPD_STATE_POWER_OFF; ... } > Note that this gets you more flexiblity than just switching to off in suspend. > You could also use the governor to let the domain switch to off based on other > criteria such as long idle time, or QoS criteria. > > It seems you may be the first upstream solution to start really exercising the > multiple states, so we may still have some kinks to work out, but thanks for > pushing your changes upstream so we can work all this out together. > It's great to have you professional guys' suggestion and helping review. And I'm exciting to co-work with you for this solution. Regards Dong Aisheng > Thanks, > > Kevin > > > > > > > _______________________________________________ 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] 18+ messages in thread
* [PATCH 2/2] PM / Domains: Choose the deepest state to enter if no devices using it 2019-03-06 13:35 ` Aisheng Dong @ 2019-03-06 13:35 ` Aisheng Dong -1 siblings, 0 replies; 18+ messages in thread From: Aisheng Dong @ 2019-03-06 13:35 UTC (permalink / raw) To: linux-pm Cc: Aisheng Dong, ulf.hansson, dongas86, khilman, rjw, dl-linux-imx, linux-arm-kernel For a domain has no working devices anymore, let's choose the deepest state to enter to save power. e.g. driver probe failure. Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- drivers/base/power/domain.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 847a69e..d3f57c2 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -550,6 +550,10 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on, if (!genpd->gov) genpd->state_idx = 0; + /* Choose the deepest state if no devices using this domain */ + if (!genpd->device_count) + genpd->state_idx = genpd->state_count - 1; + /* * If sd_count > 0 at this point, one of the subdomains hasn't * managed to call genpd_power_on() for the master yet after -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] PM / Domains: Choose the deepest state to enter if no devices using it @ 2019-03-06 13:35 ` Aisheng Dong 0 siblings, 0 replies; 18+ messages in thread From: Aisheng Dong @ 2019-03-06 13:35 UTC (permalink / raw) To: linux-pm Cc: Aisheng Dong, ulf.hansson, dongas86, khilman, rjw, dl-linux-imx, linux-arm-kernel For a domain has no working devices anymore, let's choose the deepest state to enter to save power. e.g. driver probe failure. Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- drivers/base/power/domain.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 847a69e..d3f57c2 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -550,6 +550,10 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on, if (!genpd->gov) genpd->state_idx = 0; + /* Choose the deepest state if no devices using this domain */ + if (!genpd->device_count) + genpd->state_idx = genpd->state_count - 1; + /* * If sd_count > 0 at this point, one of the subdomains hasn't * managed to call genpd_power_on() for the master yet after -- 2.7.4 _______________________________________________ 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] 18+ messages in thread
* Re: [PATCH 2/2] PM / Domains: Choose the deepest state to enter if no devices using it 2019-03-06 13:35 ` Aisheng Dong @ 2019-03-06 15:10 ` Ulf Hansson -1 siblings, 0 replies; 18+ messages in thread From: Ulf Hansson @ 2019-03-06 15:10 UTC (permalink / raw) To: Aisheng Dong Cc: dongas86, khilman, linux-pm, rjw, dl-linux-imx, linux-arm-kernel On Wed, 6 Mar 2019 at 14:35, Aisheng Dong <aisheng.dong@nxp.com> wrote: > > For a domain has no working devices anymore, let's choose the deepest state > to enter to save power. e.g. driver probe failure. > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > --- > drivers/base/power/domain.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 847a69e..d3f57c2 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -550,6 +550,10 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on, > if (!genpd->gov) > genpd->state_idx = 0; > > + /* Choose the deepest state if no devices using this domain */ > + if (!genpd->device_count) > + genpd->state_idx = genpd->state_count - 1; > + This doesn't work, sorry. For example, there may be a sub-domain with devices in it and which have constraints applicable also for the master (current) domain. > /* > * If sd_count > 0 at this point, one of the subdomains hasn't > * managed to call genpd_power_on() for the master yet after > -- > 2.7.4 > Kind regards Uffe ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] PM / Domains: Choose the deepest state to enter if no devices using it @ 2019-03-06 15:10 ` Ulf Hansson 0 siblings, 0 replies; 18+ messages in thread From: Ulf Hansson @ 2019-03-06 15:10 UTC (permalink / raw) To: Aisheng Dong Cc: dongas86, khilman, linux-pm, rjw, dl-linux-imx, linux-arm-kernel On Wed, 6 Mar 2019 at 14:35, Aisheng Dong <aisheng.dong@nxp.com> wrote: > > For a domain has no working devices anymore, let's choose the deepest state > to enter to save power. e.g. driver probe failure. > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > --- > drivers/base/power/domain.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 847a69e..d3f57c2 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -550,6 +550,10 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on, > if (!genpd->gov) > genpd->state_idx = 0; > > + /* Choose the deepest state if no devices using this domain */ > + if (!genpd->device_count) > + genpd->state_idx = genpd->state_count - 1; > + This doesn't work, sorry. For example, there may be a sub-domain with devices in it and which have constraints applicable also for the master (current) domain. > /* > * If sd_count > 0 at this point, one of the subdomains hasn't > * managed to call genpd_power_on() for the master yet after > -- > 2.7.4 > 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] 18+ messages in thread
* RE: [PATCH 2/2] PM / Domains: Choose the deepest state to enter if no devices using it 2019-03-06 15:10 ` Ulf Hansson @ 2019-03-06 15:46 ` Aisheng Dong -1 siblings, 0 replies; 18+ messages in thread From: Aisheng Dong @ 2019-03-06 15:46 UTC (permalink / raw) To: Ulf Hansson Cc: dongas86, khilman, linux-pm, rjw, dl-linux-imx, linux-arm-kernel > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Wednesday, March 6, 2019 11:11 PM > On Wed, 6 Mar 2019 at 14:35, Aisheng Dong <aisheng.dong@nxp.com> > wrote: > > > > For a domain has no working devices anymore, let's choose the deepest > > state to enter to save power. e.g. driver probe failure. > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > --- > > drivers/base/power/domain.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 847a69e..d3f57c2 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -550,6 +550,10 @@ static int genpd_power_off(struct > generic_pm_domain *genpd, bool one_dev_on, > > if (!genpd->gov) > > genpd->state_idx = 0; > > > > + /* Choose the deepest state if no devices using this domain */ > > + if (!genpd->device_count) > > + genpd->state_idx = genpd->state_count - 1; > > + > > This doesn't work, sorry. > > For example, there may be a sub-domain with devices in it and which have > constraints applicable also for the master (current) domain. Okay, got your point. Then how about make it for no governor case only? Regards Dong Aisheng > > > /* > > * If sd_count > 0 at this point, one of the subdomains hasn't > > * managed to call genpd_power_on() for the master yet after > > -- > > 2.7.4 > > > > Kind regards > Uffe ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH 2/2] PM / Domains: Choose the deepest state to enter if no devices using it @ 2019-03-06 15:46 ` Aisheng Dong 0 siblings, 0 replies; 18+ messages in thread From: Aisheng Dong @ 2019-03-06 15:46 UTC (permalink / raw) To: Ulf Hansson Cc: dongas86, khilman, linux-pm, rjw, dl-linux-imx, linux-arm-kernel > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Wednesday, March 6, 2019 11:11 PM > On Wed, 6 Mar 2019 at 14:35, Aisheng Dong <aisheng.dong@nxp.com> > wrote: > > > > For a domain has no working devices anymore, let's choose the deepest > > state to enter to save power. e.g. driver probe failure. > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > --- > > drivers/base/power/domain.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 847a69e..d3f57c2 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -550,6 +550,10 @@ static int genpd_power_off(struct > generic_pm_domain *genpd, bool one_dev_on, > > if (!genpd->gov) > > genpd->state_idx = 0; > > > > + /* Choose the deepest state if no devices using this domain */ > > + if (!genpd->device_count) > > + genpd->state_idx = genpd->state_count - 1; > > + > > This doesn't work, sorry. > > For example, there may be a sub-domain with devices in it and which have > constraints applicable also for the master (current) domain. Okay, got your point. Then how about make it for no governor case only? Regards Dong Aisheng > > > /* > > * If sd_count > 0 at this point, one of the subdomains hasn't > > * managed to call genpd_power_on() for the master yet after > > -- > > 2.7.4 > > > > 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] 18+ messages in thread
end of thread, other threads:[~2019-03-15 2:08 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-06 13:35 [PATCH 0/2] PM / Domains: Support enter deepest state during suspend and probe failure Aisheng Dong 2019-03-06 13:35 ` Aisheng Dong 2019-03-06 13:35 ` [PATCH 1/2] PM / Domains: Support enter deepest state for multiple states domains Aisheng Dong 2019-03-06 13:35 ` Aisheng Dong 2019-03-06 15:17 ` Ulf Hansson 2019-03-06 15:17 ` Ulf Hansson 2019-03-06 15:36 ` Aisheng Dong 2019-03-06 15:36 ` Aisheng Dong 2019-03-07 16:01 ` Kevin Hilman 2019-03-07 16:01 ` Kevin Hilman 2019-03-15 2:08 ` Aisheng Dong 2019-03-15 2:08 ` Aisheng Dong 2019-03-06 13:35 ` [PATCH 2/2] PM / Domains: Choose the deepest state to enter if no devices using it Aisheng Dong 2019-03-06 13:35 ` Aisheng Dong 2019-03-06 15:10 ` Ulf Hansson 2019-03-06 15:10 ` Ulf Hansson 2019-03-06 15:46 ` Aisheng Dong 2019-03-06 15:46 ` Aisheng Dong
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.