* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.