| Message ID | 20230320134217.1685781-1-abel.vesa@linaro.org |
|---|---|
| Headers | show |
| Series | Allow genpd providers to power off domains on sync state | expand |
On Mon, 20 Mar 2023 at 14:42, Abel Vesa <abel.vesa@linaro.org> wrote: > > In some cases, the providers might choose to refuse powering off some > domains until all of the consumer have had a chance to probe, that is, /s/consumer/consumers > until sync state callback has been called. Such providers might choose > to disable such domains on their on, from the sync state callback. So, /s/their on/their own > in order to do that, they need a way to queue up a power off request. > Since the generic genpd already has such API, make that available to > those providers. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/base/power/domain.c | 3 ++- > include/linux/pm_domain.h | 6 ++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 32084e38b73d..97d4e2f2da91 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -649,10 +649,11 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed) > * Queue up the execution of genpd_power_off() unless it's already been done > * before. > */ > -static void genpd_queue_power_off_work(struct generic_pm_domain *genpd) Please add a function description - and make sure to state that its external use is explicitly intended for being called from genpd providers ->sync_state callbacks. > +void genpd_queue_power_off_work(struct generic_pm_domain *genpd) As this becomes an exported function, we should also conform to genpd's function naming rules, which is to use the "pm_genpd_*" prefix. While renaming it, perhaps it's sufficient with "pm_genpd_queue_power_off" or maybe even better "pm_genpd_sync_state", what do you think? > { > queue_work(pm_wq, &genpd->power_off_work); > } > +EXPORT_SYMBOL_GPL(genpd_queue_power_off_work); > > /** > * genpd_power_off - Remove power from a given PM domain. > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index f776fb93eaa0..f9729640f87e 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -231,6 +231,7 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd, > int pm_genpd_init(struct generic_pm_domain *genpd, > struct dev_power_governor *gov, bool is_off); > int pm_genpd_remove(struct generic_pm_domain *genpd); > +void genpd_queue_power_off_work(struct generic_pm_domain *genpd); > int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state); > int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb); > int dev_pm_genpd_remove_notifier(struct device *dev); > @@ -278,6 +279,11 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd) > return -EOPNOTSUPP; > } > > +void genpd_queue_power_off_work(struct generic_pm_domain *genpd) > +{ > + return -EOPNOTSUPP; > +} > + > static inline int dev_pm_genpd_set_performance_state(struct device *dev, > unsigned int state) > { > -- > 2.34.1 > Other than the minor things above, the approach looks reasonable to me! Kind regards Uffe
On Mon, 20 Mar 2023 at 14:42, Abel Vesa <abel.vesa@linaro.org> wrote: > > There have been already a couple of tries to make the genpd "disable > unused" late initcall skip the powering off of domains that might be > needed until later on (i.e. until some consumer probes). The conclusion > was that the provider could return -EBUSY from the power_off callback > until the provider's sync state has been reached. This patch series tries > to provide a proof-of-concept that is working on Qualcomm platforms. > > I've been doing extensive testing on SM8450, but I've also spinned this > on my X13s (SC8280XP). Both patches that add the sync state callback to > the SC8280XP and SM8450 are here to provide context. Once we agree on > the form, I intend to add the sync state callback to all gdsc providers. > > Currently, some of the gdsc providers might not reach sync state due to > list of consumers not probing yet (or at all). The sync state can be > enforced by writing 1 to the state_synced sysfs attribute of the > provider, thanks to Saravana's commit [1] which has been already merged. > > [1] https://lore.kernel.org/r/20230304005355.746421-3-saravanak@google.com > > V1 of this patchset was here: > https://lore.kernel.org/all/20230315132330.450877-1-abel.vesa@linaro.org/ > > Changes since v1: > * Added the qcom_cc sync state callback which calls in turn the gdsc one > * dropped extra semicolon from pm_domain.h > > Abel Vesa (5): > PM: domains: Allow power off queuing from providers > soc: qcom: rpmhpd: Do proper power off when state synced > clk: qcom: gdsc: Avoid actual power off until sync state > clk: qcom: Add sync state callback to all SC8280XP providers > clk: qcom: Add sync state callback to all SM8450 providers > > drivers/base/power/domain.c | 3 ++- > drivers/clk/qcom/camcc-sm8450.c | 1 + > drivers/clk/qcom/common.c | 19 +++++++++++++++++++ > drivers/clk/qcom/common.h | 2 ++ > drivers/clk/qcom/dispcc-sc8280xp.c | 1 + > drivers/clk/qcom/dispcc-sm8450.c | 1 + > drivers/clk/qcom/gcc-sc8280xp.c | 1 + > drivers/clk/qcom/gcc-sm8450.c | 1 + > drivers/clk/qcom/gdsc.c | 26 ++++++++++++++++++++++++++ > drivers/clk/qcom/gdsc.h | 6 ++++++ > drivers/clk/qcom/gpucc-sc8280xp.c | 1 + > drivers/soc/qcom/rpmhpd.c | 19 +++++++------------ > include/linux/pm_domain.h | 6 ++++++ > 13 files changed, 74 insertions(+), 13 deletions(-) > Besides the minor comments on patch1, this looks good to me! So, feel free to add: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe