On Wed, Feb 15, 2023 at 02:21:53PM +0200, Abel Vesa wrote: > On 23-02-15 12:57:54, Ulf Hansson wrote: > > On Fri, 27 Jan 2023 at 11:40, Abel Vesa <abel.vesa@xxxxxxxxxx> 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@xxxxxxxxxx> > > > --- > > > > > > 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. > > > > My apologies for the somewhat late reply. Please see my comments below. > > > > > > > > The v1 is here: > > > https://lore.kernel.org/all/20230126234013.3638425-1-abel.vesa@xxxxxxxxxx/ > > > > > > 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)) > > > > Unfortunately, this doesn't really help, due to the fact that a > > genpd's ->power_off() callback may get called anyway. At power off, > > the genpd core only cares about those consumers that are currently > > attached, not those that might get attached at some point later in > > time. > > > > In other words, it's the responsibility for each specific genpd > > provider to cope with the condition that its ->sync_state() callback > > may *not* have been called, while its ->power_off() callback is being > > called. > > > > In these cases, the genpd provider should probably make the > > ->power_off() callback to return -EBUSY. This is what we do in > > psci_pd_power_off(), for example. > > > > Hmm, this might actually be a better idea. Bjorn, do you agree? > Yes, I agree. Regards, Bjorn
