| Message ID | 1626764876-10229-2-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / Domains: Add support for 'required-opps' to set default perf state | expand |
Hi, Rajendra Nayak <rnayak@codeaurora.org> writes: > @@ -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; nit: might want to keep one variable declaration per line here.
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. 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? > + } > > - 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
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 >
On 8/3/2021 10:08 AM, Rajendra Nayak wrote: > > 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? Looks like calling of_get_required_opp_performance_state() unconditionally makes it spit out a pr_err() in case the node is missing "required-opps" property, so I posted a v6 [1] with the check in place and adding the missing else condition. [1] https://lore.kernel.org/patchwork/project/lkml/list/?series=510727
On Wed, 4 Aug 2021 at 13:08, Rajendra Nayak <rnayak@codeaurora.org> wrote: > > > On 8/3/2021 10:08 AM, Rajendra Nayak wrote: > > > > 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? > > Looks like calling of_get_required_opp_performance_state() unconditionally > makes it spit out a pr_err() in case the node is missing "required-opps" property, > so I posted a v6 [1] with the check in place and adding the missing else > condition. I see. Viresh, would it make sense to remove that print? I mean, the required-opps property could be considered as optional and it seems a bit silly that a pre-parsing of the property is needed to figure that out. > > [1] https://lore.kernel.org/patchwork/project/lkml/list/?series=510727 Kind regards Uffe
On 04-08-21, 13:39, Ulf Hansson wrote: > Viresh, would it make sense to remove that print? I mean, the > required-opps property could be considered as optional and it seems a > bit silly that a pre-parsing of the property is needed to figure that > out. Sure, np.
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; + 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; + } - 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;
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(-)