Messages in this thread | | | Date | Thu, 2 Feb 2023 21:53:41 +0200 | Subject | Re: [RFC PATCH v2 1/2] PM: domains: Skip disabling unused domains if provider has sync_state | From | Dmitry Baryshkov <> |
| |
On 02/02/2023 20:24, Matthias Kaehlcke wrote: > Hi Abel, > > On Fri, Jan 27, 2023 at 12:40:53PM +0200, Abel Vesa wrote: >> Currently, there are cases when a domain needs to remain enabled until >> the consumer driver probes. Sometimes such consumer drivers may be built >> as modules. Since the genpd_power_off_unused is called too early for >> such consumer driver modules to get a chance to probe, the domain, since >> it is unused, will get disabled. On the other hand, the best time for >> an unused domain to be disabled is on the provider's sync_state >> callback. So, if the provider has registered a sync_state callback, >> assume the unused domains for that provider will be disabled on its >> sync_state callback. Also provide a generic sync_state callback which >> disables all the domains unused for the provider that registers it. >> >> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >> --- >> >> This approach has been applied for unused clocks as well. >> With this patch merged in, all the providers that have sync_state >> callback registered will leave the domains enabled unless the provider's >> sync_state callback explicitly disables them. So those providers will >> need to add the disabling part to their sync_state callback. On the >> other hand, the platforms that have cases where domains need to remain >> enabled (even if unused) until the consumer driver probes, will be able, >> with this patch in, to run without the pd_ignore_unused kernel argument, >> which seems to be the case for most Qualcomm platforms, at this moment. > > I recently encountered a related issue on a Qualcomm platform with a > v6.2-rc kernel, which includes 3a39049f88e4 ("soc: qcom: rpmhpd: Use > highest corner until sync_state"). The issue involves a DT node with a > rpmhpd, the DT node is enabled, however the corresponding device driver > is not enabled in the kernel. In such a scenario the sync_state callback > is never called, because the genpd consumer never probes. As a result > the Always-on subsystem (AOSS) of the SoC doesn't enter sleep mode during > system suspend, which results in a substantially higher power consumption > in S3. > > I wonder if genpd (and some other frameworks) needs something like > regulator_init_complete(), which turns off unused regulators 30s after > system boot. That's conceptually similar to the current > genpd_power_off_unused(), but would provide time for modules being loaded.
I think the overall goal is to move away from ad-hoc implementations like clk_disable_unused/genpd_power_off_unused/regulator_init_complete towards the sync_state.
So inherently one either has to provide drivers for all devices in question or disable unused devices in DT.
> >> The v1 is here: >> https://lore.kernel.org/all/20230126234013.3638425-1-abel.vesa@linaro.org/ >> >> Changes since v1: >> * added a generic sync state callback to be registered by providers in >> order to disable the unused domains on their sync state. Also >> mentioned this in the commit message. >> >> drivers/base/power/domain.c | 17 ++++++++++++++++- >> include/linux/pm_domain.h | 3 +++ >> 2 files changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index 84662d338188..c2a5f77c01f3 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -1099,7 +1099,8 @@ static int __init genpd_power_off_unused(void) >> mutex_lock(&gpd_list_lock); >> >> list_for_each_entry(genpd, &gpd_list, gpd_list_node) >> - genpd_queue_power_off_work(genpd); >> + if (!dev_has_sync_state(genpd->provider->dev)) >> + genpd_queue_power_off_work(genpd); >> >> mutex_unlock(&gpd_list_lock); >> >> @@ -1107,6 +1108,20 @@ static int __init genpd_power_off_unused(void) >> } >> late_initcall(genpd_power_off_unused); >> >> +void genpd_power_off_unused_sync_state(struct device *dev) >> +{ >> + struct generic_pm_domain *genpd; >> + >> + mutex_lock(&gpd_list_lock); >> + >> + list_for_each_entry(genpd, &gpd_list, gpd_list_node) >> + if (genpd->provider->dev == dev) >> + genpd_queue_power_off_work(genpd); >> + >> + mutex_unlock(&gpd_list_lock); >> +} >> +EXPORT_SYMBOL_GPL(genpd_power_off_unused_sync_state); >> + >> #ifdef CONFIG_PM_SLEEP >> >> /** >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >> index f776fb93eaa0..1fd5aa500c81 100644 >> --- a/include/linux/pm_domain.h >> +++ b/include/linux/pm_domain.h >> @@ -351,6 +351,7 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev, >> unsigned int index); >> struct device *genpd_dev_pm_attach_by_name(struct device *dev, >> const char *name); >> +void genpd_power_off_unused_sync_state(struct device *dev); >> #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */ >> static inline int of_genpd_add_provider_simple(struct device_node *np, >> struct generic_pm_domain *genpd) >> @@ -419,6 +420,8 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np) >> { >> return ERR_PTR(-EOPNOTSUPP); >> } >> + >> +static inline genpd_power_off_unused_sync_state(struct device *dev) {} >> #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */ >> >> #ifdef CONFIG_PM >> -- >> 2.34.1 >>
-- With best wishes Dmitry
| |