Re: [RFC PATCH v2 1/2] PM: domains: Skip disabling unused domains if provider has sync_state

[Matthias Kaehlcke <mka@chromium.org>]

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.

> 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
>