Messages in this thread | | | Subject | Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state | From | Rajendra Nayak <> | Date | Tue, 3 Aug 2021 10:08:26 +0530 |
| |
On 8/2/2021 6:29 PM, Ulf Hansson wrote: > On Tue, 20 Jul 2021 at 09:12, Rajendra Nayak <rnayak@codeaurora.org> wrote: >> >> Some devices within power domains with performance states do not >> support DVFS, but still need to vote on a default/static state >> while they are active. They can express this using the 'required-opps' >> property in device tree, which points to the phandle of the OPP >> supported by the corresponding power-domains. >> >> Add support to parse this information from DT and then set the >> specified performance state during attach and drop it on detach. >> runtime suspend/resume callbacks already have logic to drop/set >> the vote as needed and should take care of dropping the default >> perf state vote on runtime suspend and restore it back on runtime >> resume. >> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> --- >> drivers/base/power/domain.c | 28 +++++++++++++++++++++++++--- >> include/linux/pm_domain.h | 1 + >> 2 files changed, 26 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index a934c67..f454031 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -2598,6 +2598,12 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off) >> >> dev_dbg(dev, "removing from PM domain %s\n", pd->name); >> >> + /* Drop the default performance state */ >> + if (dev_gpd_data(dev)->default_pstate) { >> + dev_pm_genpd_set_performance_state(dev, 0); >> + dev_gpd_data(dev)->default_pstate = 0; >> + } >> + >> for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) { >> ret = genpd_remove_device(pd, dev); >> if (ret != -EAGAIN) >> @@ -2635,9 +2641,10 @@ static void genpd_dev_pm_sync(struct device *dev) >> static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, >> unsigned int index, bool power_on) >> { >> + struct device_node *np; >> struct of_phandle_args pd_args; >> struct generic_pm_domain *pd; >> - int ret; >> + int ret, pstate; >> >> ret = of_parse_phandle_with_args(dev->of_node, "power-domains", >> "#power-domain-cells", index, &pd_args); >> @@ -2675,10 +2682,25 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, >> genpd_unlock(pd); >> } >> >> - if (ret) >> + if (ret) { >> genpd_remove_device(pd, dev); >> + return -EPROBE_DEFER; >> + } >> + >> + /* Set the default performance state */ >> + np = base_dev->of_node; > > Please use dev->of_node instead (it is set to the same of_node as > base_dev by the callers of __genpd_dev_pm_attach) as it's more > consistent with existing code. > >> + if (of_parse_phandle(np, "required-opps", index)) { >> + pstate = of_get_required_opp_performance_state(np, index); >> + if (pstate < 0) { >> + ret = pstate; >> + dev_err(dev, "failed to set required performance state for power-domain %s: %d\n", >> + pd->name, ret); >> + } >> + dev_pm_genpd_set_performance_state(dev, pstate); >> + dev_gpd_data(dev)->default_pstate = pstate; > > This doesn't look entirely correct to me. If we fail to translate a > required opp to a performance state, we shouldn't try to set it.
yeah, that does not seem right at all :(
> Perhaps it's also easier to call > of_get_required_opp_performance_state() unconditionally of whether a > "required-opps" specifier exists. If it fails with the translation, > then we just skip setting a default state and continue with returning > 1. > > Would that work?
I think it should, I'll redo the error handling, hopefully right this time, and re-post. Thanks for the review.
> >> + } >> >> - return ret ? -EPROBE_DEFER : 1; >> + return ret ? ret : 1; >> } >> >> /** >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >> index 21a0577..67017c9 100644 >> --- a/include/linux/pm_domain.h >> +++ b/include/linux/pm_domain.h >> @@ -198,6 +198,7 @@ struct generic_pm_domain_data { >> struct notifier_block *power_nb; >> int cpu; >> unsigned int performance_state; >> + unsigned int default_pstate; >> unsigned int rpm_pstate; >> ktime_t next_wakeup; >> void *data; >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation >> > > Kind regards > Uffe >
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
| |