* [PATCH v5 0/2] PM / Domains: Add support for 'required-opps' to set default perf state @ 2021-07-20 7:07 Rajendra Nayak 2021-07-20 7:07 ` [PATCH v5 1/2] " Rajendra Nayak 2021-07-20 7:07 ` [PATCH v5 2/2] arm64: dts: sc7180: Add required-opps for i2c Rajendra Nayak 0 siblings, 2 replies; 9+ messages in thread From: Rajendra Nayak @ 2021-07-20 7:07 UTC (permalink / raw) To: ulf.hansson, bjorn.andersson, viresh.kumar Cc: linux-pm, devicetree, linux-kernel, linux-arm-msm, swboyd, rojay, stephan, Rajendra Nayak v5: Dropped all default_pstate handling in runtime suspend/resume v4: Fixed error handling in __genpd_dev_pm_attach() This is a re-spin of the series [1] which was adding support for a new DT binding (assigned-performance-state) and based on the discussions on that thread [2] it was concluded that we could achieve the same with the existing 'required-opps' binding instead. So this series, just drops the new binding and uses required-opps to achieve the default perf state setting thats needed by some devices. --- Some devics within power-domains with performance states do not support DVFS, but still need to vote on a default/static state while they are active. Add support for this using the 'required-opps' property in device tree. [1] https://lore.kernel.org/patchwork/project/lkml/list/?series=501336&state=%2A&archive=both [2] https://lore.kernel.org/patchwork/patch/1436886/ Rajendra Nayak (2): PM / Domains: Add support for 'required-opps' to set default perf state arm64: dts: sc7180: Add required-opps for i2c arch/arm64/boot/dts/qcom/sc7180.dtsi | 24 ++++++++++++++++++++++++ drivers/base/power/domain.c | 28 +++++++++++++++++++++++++--- include/linux/pm_domain.h | 1 + 3 files changed, 50 insertions(+), 3 deletions(-) -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state 2021-07-20 7:07 [PATCH v5 0/2] PM / Domains: Add support for 'required-opps' to set default perf state Rajendra Nayak @ 2021-07-20 7:07 ` Rajendra Nayak 2021-07-20 7:18 ` Felipe Balbi 2021-08-02 12:59 ` Ulf Hansson 2021-07-20 7:07 ` [PATCH v5 2/2] arm64: dts: sc7180: Add required-opps for i2c Rajendra Nayak 1 sibling, 2 replies; 9+ messages in thread From: Rajendra Nayak @ 2021-07-20 7:07 UTC (permalink / raw) To: ulf.hansson, bjorn.andersson, viresh.kumar Cc: linux-pm, devicetree, linux-kernel, linux-arm-msm, swboyd, rojay, stephan, Rajendra Nayak 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; + 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; -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state 2021-07-20 7:07 ` [PATCH v5 1/2] " Rajendra Nayak @ 2021-07-20 7:18 ` Felipe Balbi 2021-08-02 12:59 ` Ulf Hansson 1 sibling, 0 replies; 9+ messages in thread From: Felipe Balbi @ 2021-07-20 7:18 UTC (permalink / raw) To: Rajendra Nayak, ulf.hansson, bjorn.andersson, viresh.kumar Cc: linux-pm, devicetree, linux-kernel, linux-arm-msm, swboyd, rojay, stephan, Rajendra Nayak [-- Attachment #1: Type: text/plain, Size: 483 bytes --] 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. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 511 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state 2021-07-20 7:07 ` [PATCH v5 1/2] " Rajendra Nayak 2021-07-20 7:18 ` Felipe Balbi @ 2021-08-02 12:59 ` Ulf Hansson 2021-08-03 4:38 ` Rajendra Nayak 1 sibling, 1 reply; 9+ messages in thread From: Ulf Hansson @ 2021-08-02 12:59 UTC (permalink / raw) To: Rajendra Nayak Cc: Bjorn Andersson, Viresh Kumar, Linux PM, DTML, Linux Kernel Mailing List, linux-arm-msm, Stephen Boyd, Roja Rani Yarubandi, Stephan Gerhold 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state 2021-08-02 12:59 ` Ulf Hansson @ 2021-08-03 4:38 ` Rajendra Nayak 2021-08-04 11:08 ` Rajendra Nayak 0 siblings, 1 reply; 9+ messages in thread From: Rajendra Nayak @ 2021-08-03 4:38 UTC (permalink / raw) To: Ulf Hansson Cc: Bjorn Andersson, Viresh Kumar, Linux PM, DTML, Linux Kernel Mailing List, linux-arm-msm, Stephen Boyd, Roja Rani Yarubandi, Stephan Gerhold 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state 2021-08-03 4:38 ` Rajendra Nayak @ 2021-08-04 11:08 ` Rajendra Nayak 2021-08-04 11:39 ` Ulf Hansson 0 siblings, 1 reply; 9+ messages in thread From: Rajendra Nayak @ 2021-08-04 11:08 UTC (permalink / raw) To: Ulf Hansson Cc: Bjorn Andersson, Viresh Kumar, Linux PM, DTML, Linux Kernel Mailing List, linux-arm-msm, Stephen Boyd, Roja Rani Yarubandi, Stephan Gerhold 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 -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state 2021-08-04 11:08 ` Rajendra Nayak @ 2021-08-04 11:39 ` Ulf Hansson 2021-08-05 4:05 ` Viresh Kumar 0 siblings, 1 reply; 9+ messages in thread From: Ulf Hansson @ 2021-08-04 11:39 UTC (permalink / raw) To: Rajendra Nayak, Viresh Kumar Cc: Bjorn Andersson, Linux PM, DTML, Linux Kernel Mailing List, linux-arm-msm, Stephen Boyd, Roja Rani Yarubandi, Stephan Gerhold 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state 2021-08-04 11:39 ` Ulf Hansson @ 2021-08-05 4:05 ` Viresh Kumar 0 siblings, 0 replies; 9+ messages in thread From: Viresh Kumar @ 2021-08-05 4:05 UTC (permalink / raw) To: Ulf Hansson Cc: Rajendra Nayak, Bjorn Andersson, Linux PM, DTML, Linux Kernel Mailing List, linux-arm-msm, Stephen Boyd, Roja Rani Yarubandi, Stephan Gerhold 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. -- viresh ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-20 7:07 [PATCH v5 0/2] PM / Domains: Add support for 'required-opps' to set default perf state Rajendra Nayak 2021-07-20 7:07 ` [PATCH v5 1/2] " Rajendra Nayak @ 2021-07-20 7:07 ` Rajendra Nayak 1 sibling, 0 replies; 9+ messages in thread From: Rajendra Nayak @ 2021-07-20 7:07 UTC (permalink / raw) To: ulf.hansson, bjorn.andersson, viresh.kumar Cc: linux-pm, devicetree, linux-kernel, linux-arm-msm, swboyd, rojay, stephan, Rajendra Nayak qup-i2c devices on sc7180 are clocked with a fixed clock (19.2 MHz) Though qup-i2c does not support DVFS, it still needs to vote for a performance state on 'CX' to satisfy the 19.2 Mhz clock frequency requirement. Use 'required-opps' to pass this information from device tree, and also add the power-domains property to specify the CX power-domain. Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> Reviewed-by: Stephen Boyd <swboyd@chromium.org> --- arch/arm64/boot/dts/qcom/sc7180.dtsi | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi index a5d58eb..cd30185 100644 --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi @@ -785,8 +785,10 @@ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>, <&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + required-opps = <&rpmhpd_opp_low_svs>; status = "disabled"; }; spi0: spi@880000 { @@ -837,8 +839,10 @@ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>, <&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + required-opps = <&rpmhpd_opp_low_svs>; status = "disabled"; }; spi1: spi@884000 { @@ -889,8 +893,10 @@ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>, <&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + required-opps = <&rpmhpd_opp_low_svs>; status = "disabled"; }; uart2: serial@888000 { @@ -923,8 +929,10 @@ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>, <&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + required-opps = <&rpmhpd_opp_low_svs>; status = "disabled"; }; spi3: spi@88c000 { @@ -975,8 +983,10 @@ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>, <&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + required-opps = <&rpmhpd_opp_low_svs>; status = "disabled"; }; uart4: serial@890000 { @@ -1009,8 +1019,10 @@ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>, <&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + required-opps = <&rpmhpd_opp_low_svs>; status = "disabled"; }; spi5: spi@894000 { @@ -1074,8 +1086,10 @@ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_1 0>, <&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + required-opps = <&rpmhpd_opp_low_svs>; status = "disabled"; }; spi6: spi@a80000 { @@ -1126,8 +1140,10 @@ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_1 0>, <&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + required-opps = <&rpmhpd_opp_low_svs>; status = "disabled"; }; uart7: serial@a84000 { @@ -1160,8 +1176,10 @@ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_1 0>, <&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + required-opps = <&rpmhpd_opp_low_svs>; status = "disabled"; }; spi8: spi@a88000 { @@ -1212,8 +1230,10 @@ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_1 0>, <&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + required-opps = <&rpmhpd_opp_low_svs>; status = "disabled"; }; uart9: serial@a8c000 { @@ -1246,8 +1266,10 @@ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_1 0>, <&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + required-opps = <&rpmhpd_opp_low_svs>; status = "disabled"; }; spi10: spi@a90000 { @@ -1298,8 +1320,10 @@ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_1 0>, <&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + required-opps = <&rpmhpd_opp_low_svs>; status = "disabled"; }; spi11: spi@a94000 { -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-08-05 4:05 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-20 7:07 [PATCH v5 0/2] PM / Domains: Add support for 'required-opps' to set default perf state Rajendra Nayak 2021-07-20 7:07 ` [PATCH v5 1/2] " Rajendra Nayak 2021-07-20 7:18 ` Felipe Balbi 2021-08-02 12:59 ` Ulf Hansson 2021-08-03 4:38 ` Rajendra Nayak 2021-08-04 11:08 ` Rajendra Nayak 2021-08-04 11:39 ` Ulf Hansson 2021-08-05 4:05 ` Viresh Kumar 2021-07-20 7:07 ` [PATCH v5 2/2] arm64: dts: sc7180: Add required-opps for i2c Rajendra Nayak
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).