PM / Domains: Add support for 'required-opps' to set default perf state
Subsystems (9)
v6 cover PM / Domains: Add support for 'required-opps' to set default perf state
v6: Fixed up some more error handling in __genpd_dev_pm_attach()
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, 51 insertions(+), 2 deletions(-)
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v6 1/2 PM / Domains: Add support for 'required-opps' to set default perf state
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.
--- drivers/base/power/domain.c | 28 ++++++++++++++++++++++++++-- include/linux/pm_domain.h | 1 + 2 files changed, 27 insertions(+), 2 deletions(-)
drivers/base/power/domain.c +26 -2
@@ -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 )
@@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, {struct of_phandle_args pd_args;struct generic_pm_domain *pd; +struct device_node *np; +int pstate;int ret; ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
@@ -2675,10 +2683,26 @@ 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 = 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); + }else { +dev_pm_genpd_set_performance_state (dev, pstate); +dev_gpd_data (dev)->default_pstate = pstate; + } + } -return ret ? -EPROBE_DEFER :1 ; +return ret ? ret :1 ; }/**
include/linux/pm_domain.h +1 -0
@@ -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;
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Quoted 49 lines · 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 <[email protected]>--- drivers/base/power/domain.c | 28 ++++++++++++++++++++++++++-- include/linux/pm_domain.h | 1 + 2 files changed, 27 insertions(+), 2 deletions(-)drivers/base/power/domain.c +8
-0@@ -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 )@@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, {struct of_phandle_args pd_args;struct generic_pm_domain *pd; +struct device_node *np; +int pstate;int ret; ret = of_parse_phandle_with_args(dev->of_node, "power-domains",@@ -2675,10 +2683,26 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,genpd_unlock (pd); }
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2693
Quoted 10 lines
drivers/base/power/domain.c +7
-1-if (ret) +if (ret) {genpd_remove_device (pd, dev); +return -EPROBE_DEFER ; + } + +/* Set the default performance state */ + np = dev->of_node; +if (of_parse_phandle (np,"required-opps" , index)) {
Looks like Viresh thinks it's a good idea to drop the error print in of_get_required_opp_performance_state() when there is no "required-opps" specifier.
Would you mind folding in a patch for that in the series, so this code can be simplified according to our earlier discussions?
Quoted 21 lines
drivers/base/power/domain.c +11
-1+ 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); + }else { +dev_pm_genpd_set_performance_state (dev, pstate); +dev_gpd_data (dev)->default_pstate = pstate; + } + } -return ret ? -EPROBE_DEFER :1 ; +return ret ? ret :1 ; }/** include/linux/pm_domain.h +0
-0
include/linux/pm_domain.h struct generic_pm_domain_data { · line 204
Quoted 7 lines
include/linux/pm_domain.h +1
-0@@ -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;
Other than the above, this looks good to me!
Kind regards
Uffe
Quoted 65 lines · Ulf Hansson
Quoted 49 lines · 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 <[email protected]>--- drivers/base/power/domain.c | 28 ++++++++++++++++++++++++++-- include/linux/pm_domain.h | 1 + 2 files changed, 27 insertions(+), 2 deletions(-)drivers/base/power/domain.c +8
-0@@ -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 )@@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, {struct of_phandle_args pd_args;struct generic_pm_domain *pd; +struct device_node *np; +int pstate;int ret; ret = of_parse_phandle_with_args(dev->of_node, "power-domains",@@ -2675,10 +2683,26 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,genpd_unlock (pd); }drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2693
Quoted 10 lines
drivers/base/power/domain.c +7
-1-if (ret) +if (ret) {genpd_remove_device (pd, dev); +return -EPROBE_DEFER ; + } + +/* Set the default performance state */ + np = dev->of_node; +if (of_parse_phandle (np,"required-opps" , index)) {Looks like Viresh thinks it's a good idea to drop the error print in
of_get_required_opp_performance_state()when there is no "required-opps" specifier.
Quoted 2 lines
Would you mind folding in a patch for that in the series, so this code can be simplified according to our earlier discussions?
Sure, I can do that, apart from the error print, the function currently also returns a -EINVAL in case of the missing 'required-opps', are we suggesting we change that to not return an error also?
Since this is completely optional in the device node, we would want the function to ideally not return error and only do so in case 'required-opps' exists and the translation to performance state fails.
I am not sure that's the behavior we expect in case of 'required-opps' in the OPP tables also, Viresh?
Quoted 35 lines
Quoted 22 lines
drivers/base/power/domain.c +11
-1+ 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); + }else { +dev_pm_genpd_set_performance_state (dev, pstate); +dev_gpd_data (dev)->default_pstate = pstate; + } + } -return ret ? -EPROBE_DEFER :1 ; +return ret ? ret :1 ; }/** include/linux/pm_domain.h +0
-0include/linux/pm_domain.h struct generic_pm_domain_data { · line 204
Quoted 7 lines
include/linux/pm_domain.h +1
-0@@ -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;Other than the above, this looks good to me!
Kind regards
Uffe
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Quoted 3 lines · Rajendra Nayak
Sure, I can do that, apart from the error print, the function currently also returns a
-EINVALin case of the missing 'required-opps', are we suggesting we change that to not return an error also?
No.
Quoted 3 lines
Since this is completely optional in the device node, we would want the function to ideally not return error and only do so in case 'required-opps' exists and the translation to performance state fails.
Not really. The function should return failure if the property isn't there, but it shouldn't be EINVAL but ENODEV.
Signature · 1 line
viresh
Quoted 5 lines · Viresh Kumar
Quoted 3 lines · Rajendra Nayak
Sure, I can do that, apart from the error print, the function currently also returns a
-EINVALin case of the missing 'required-opps', are we suggesting we change that to not return an error also?
Quoted 8 lines
No.
Quoted 3 lines
Since this is completely optional in the device node, we would want the function to ideally not return error and only do so in case 'required-opps' exists and the translation to performance state fails.
Not really. The function should return failure if the property isn't there, but it shouldn't be EINVAL but ENODEV.
In my case I don't want to error out if the property is missing, I want to error out only when the property exists but can't be translated into a performance state.
So currently I check if the property exists and *only then* try to translate it, Ulf asked me to skip the check. If I do that and I call of_get_required_opp_performance_state() unconditionally, and if it errors out I will need to put in additional logic (check for return value of ENODEV) to distinguish between the property-does-not-exist vs property-exists-but-cannot-be-translated case.
It just seems more straight-forward to call this only when the property exists, Ulf?
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Quoted 9 lines · Rajendra Nayak
In my case I don't want to error out if the property is missing, I want to error out only when the property exists but can't be translated into a performance state.
So currently I check if the property exists and *only then* try to translate it, Ulf asked me to skip the check. If I do that and I call
of_get_required_opp_performance_state()unconditionally, and if it errors out I will need to put in additional logic (check for return value of ENODEV) to distinguish between the property-does-not-exist vs property-exists-but-cannot-be-translated case.
It just seems more straight-forward to call this only when the property exists, Ulf?
The same check will be done by OPP core as well, so it is better to optimize for the success case here. I will say, don't error out on ENODEV, rest you know well.
Signature · 1 line
viresh
Quoted 12 lines · Viresh Kumar
Quoted 9 lines · Rajendra Nayak
In my case I don't want to error out if the property is missing, I want to error out only when the property exists but can't be translated into a performance state.
So currently I check if the property exists and *only then* try to translate it, Ulf asked me to skip the check. If I do that and I call
of_get_required_opp_performance_state()unconditionally, and if it errors out I will need to put in additional logic (check for return value of ENODEV) to distinguish between the property-does-not-exist vs property-exists-but-cannot-be-translated case.
It just seems more straight-forward to call this only when the property exists, Ulf?
Quoted 3 lines
The same check will be done by OPP core as well, so it is better to optimize for the success case here. I will say, don't error out on ENODEV, rest you know well.
This should work, while I generally favor not having to parse for specific return codes.
Another option is to add a new OPP OF helperfunction that just informs the caller whether the required-opps property exists (instead of open-coding that part), and if so, the caller can continue with of_get_required_opp_performance_state() and then expect it to succeed.
I have no strong opinion though! Whatever works for me.
Kind regards
Uffe
Quoted 20 lines · Ulf Hansson
Quoted 12 lines · Viresh Kumar
Quoted 9 lines · Rajendra Nayak
In my case I don't want to error out if the property is missing, I want to error out only when the property exists but can't be translated into a performance state.
So currently I check if the property exists and *only then* try to translate it, Ulf asked me to skip the check. If I do that and I call
of_get_required_opp_performance_state()unconditionally, and if it errors out I will need to put in additional logic (check for return value of ENODEV) to distinguish between the property-does-not-exist vs property-exists-but-cannot-be-translated case.
It just seems more straight-forward to call this only when the property exists, Ulf?Quoted 3 lines
The same check will be done by OPP core as well, so it is better to optimize for the success case here. I will say, don't error out on ENODEV, rest you know well.
This should work, while I generally favor not having to parse for specific return codes.
Quoted 6 lines
Another option is to add a new OPP OF helperfunction that just informs the caller whether the required-opps property exists (instead of open-coding that part), and if so, the caller can continue with
of_get_required_opp_performance_state()and then expect it to succeed.I have no strong opinion though! Whatever works for me.
I would like to work with the existing set of APIs, as the OPP core is going to do that check anyways, again.
Signature · 1 line
viresh
04.08.2021 13:58, Rajendra Nayak пишет:
Quoted 22 lines
drivers/base/power/domain.c +9
-1@@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, {struct of_phandle_args pd_args;struct generic_pm_domain *pd; +struct device_node *np; +int pstate;int ret; ret = of_parse_phandle_with_args(dev->of_node, "power-domains",@@ -2675,10 +2683,26 @@ 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 = dev->of_node; +if (of_parse_phandle (np,"required-opps" , index)) {
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2703
Quoted 10 lines
drivers/base/power/domain.c +10
-0+ 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); + }else { +dev_pm_genpd_set_performance_state (dev, pstate); +dev_gpd_data (dev)->default_pstate = pstate; + } + }
Why performance state is set after genpd_power_on()?
10.08.2021 01:26, Dmitry Osipenko пишет:
Quoted 14 lines
04.08.2021 13:58, Rajendra Nayak пишет:
Quoted 13 lines
drivers/base/power/domain.c +2
-0@@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, {struct of_phandle_args pd_args;struct generic_pm_domain *pd; +struct device_node *np; +int pstate;int ret; ret = of_parse_phandle_with_args(dev->of_node, "power-domains",@@ -2675,10 +2683,26 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,genpd_unlock (pd); }
Quoted 16 lines
Quoted 6 lines
drivers/base/power/domain.c +4
-1-if (ret) +if (ret) {genpd_remove_device (pd, dev); +return -EPROBE_DEFER ; + } +drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2700
Quoted 10 lines
drivers/base/power/domain.c +10
-0+/* Set the default performance state */ + np = 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); + }else { +dev_pm_genpd_set_performance_state (dev, pstate);
Where is error handling?
Quoted 6 lines
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2703
Quoted 3 lines
drivers/base/power/domain.c +3
-0+dev_gpd_data (dev)->default_pstate = pstate; + } + }Why performance state is set after
genpd_power_on()?
10.08.2021 01:44, Dmitry Osipenko пишет:
Quoted 15 lines
10.08.2021 01:26, Dmitry Osipenko пишет:
Quoted 14 lines
04.08.2021 13:58, Rajendra Nayak пишет:
Quoted 13 lines
drivers/base/power/domain.c +2
-0@@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, {struct of_phandle_args pd_args;struct generic_pm_domain *pd; +struct device_node *np; +int pstate;int ret; ret = of_parse_phandle_with_args(dev->of_node, "power-domains",@@ -2675,10 +2683,26 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,genpd_unlock (pd); }
Quoted 9 lines
Quoted 9 lines
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2693
Quoted 9 lines
drivers/base/power/domain.c +7
-1-if (ret) +if (ret) {genpd_remove_device (pd, dev); +return -EPROBE_DEFER ; + } + +/* Set the default performance state */ + np = dev->of_node; +if (of_parse_phandle (np,"required-opps" , index)) {
The OF node returned by of_parse_phandle() must be put.
Quoted 1 line
Quoted 1 line
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2694
Quoted 1 line
drivers/base/power/domain.c +1
-0+ pstate =of_get_required_opp_performance_state (np, index);
If you have more than one power domain, then this will override the pstate which was set for a previous domain. This code doesn't feel solid to me, at least a clarifying comment is needed about how it's supposed to work.
Quoted 14 lines
Quoted 6 lines
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2700
Quoted 6 lines
drivers/base/power/domain.c +6
-0+if (pstate <0 ) { + ret = pstate; +dev_err (dev,"failed to set required performance state for power-domain %s: %d\n" , + pd->name, ret); + }else { +dev_pm_genpd_set_performance_state (dev, pstate);Where is error handling?
Quoted 5 lines
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2703
Quoted 3 lines
drivers/base/power/domain.c +3
-0+dev_gpd_data (dev)->default_pstate = pstate; + } + }Why performance state is set after
genpd_power_on()?
Should set_performance_state() be invoked when power_on=false?
I assume it should be:
if (power_on) {
dev_pm_genpd_set_performance_state(dev, pstate);
dev_gpd_data(dev)->default_pstate = pstate;
} else {
dev_gpd_data(dev)->rpm_pstate = pstate;
}10.08.2021 02:46, Dmitry Osipenko пишет:
Quoted 52 lines
10.08.2021 01:44, Dmitry Osipenko пишет:
Quoted 15 lines
10.08.2021 01:26, Dmitry Osipenko пишет:
Quoted 14 lines
04.08.2021 13:58, Rajendra Nayak пишет:
Quoted 13 lines
drivers/base/power/domain.c +2
-0@@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, {struct of_phandle_args pd_args;struct generic_pm_domain *pd; +struct device_node *np; +int pstate;int ret; ret = of_parse_phandle_with_args(dev->of_node, "power-domains",@@ -2675,10 +2683,26 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,genpd_unlock (pd); }Quoted 9 lines
Quoted 9 lines
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2693
Quoted 9 lines
drivers/base/power/domain.c +7
-1-if (ret) +if (ret) {genpd_remove_device (pd, dev); +return -EPROBE_DEFER ; + } + +/* Set the default performance state */ + np = dev->of_node; +if (of_parse_phandle (np,"required-opps" , index)) {The OF node returned by
of_parse_phandle()must be put.Quoted 1 line
Quoted 1 line
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2694
Quoted 1 line
drivers/base/power/domain.c +1
-0+ pstate =of_get_required_opp_performance_state (np, index);If you have more than one power domain, then this will override the pstate which was set for a previous domain. This code doesn't feel solid to me, at least a clarifying comment is needed about how it's supposed to work.
Quoted 14 lines
Quoted 6 lines
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2700
Quoted 6 lines
drivers/base/power/domain.c +6
-0+if (pstate <0 ) { + ret = pstate; +dev_err (dev,"failed to set required performance state for power-domain %s: %d\n" , + pd->name, ret); + }else { +dev_pm_genpd_set_performance_state (dev, pstate);Where is error handling?
Quoted 5 lines
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2703
Quoted 3 lines
drivers/base/power/domain.c +3
-0+dev_gpd_data (dev)->default_pstate = pstate; + } + }Why performance state is set after
genpd_power_on()?Should
set_performance_state()be invoked when power_on=false?
Quoted 9 lines
I assume it should be:
if (power_on) { dev_pm_genpd_set_performance_state(dev, pstate); dev_gpd_data(dev)->default_pstate = pstate; } else { dev_gpd_data(dev)->rpm_pstate = pstate; }
Although, thinking a bit more about this, it should be better to skip setting perf state in a case of power_on=false and let drivers to handle it, IMO. Too much trouble with it for the core.
Quoted 16 lines · Dmitry Osipenko
10.08.2021 01:44, Dmitry Osipenko пишет:
Quoted 15 lines
10.08.2021 01:26, Dmitry Osipenko пишет:
Quoted 14 lines
04.08.2021 13:58, Rajendra Nayak пишет:
Quoted 13 lines
drivers/base/power/domain.c +2
-0@@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, {struct of_phandle_args pd_args;struct generic_pm_domain *pd; +struct device_node *np; +int pstate;int ret; ret = of_parse_phandle_with_args(dev->of_node, "power-domains",@@ -2675,10 +2683,26 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,genpd_unlock (pd); }
Quoted 11 lines
Quoted 9 lines
Quoted 9 lines
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2693
Quoted 9 lines
drivers/base/power/domain.c +7
-1-if (ret) +if (ret) {genpd_remove_device (pd, dev); +return -EPROBE_DEFER ; + } + +/* Set the default performance state */ + np = dev->of_node; +if (of_parse_phandle (np,"required-opps" , index)) {The OF node returned by
of_parse_phandle()must be put.
Thanks for spotting that, I will fix it, though I might just drop the use of of_parse_phandle() and call of_get_required_opp_performance_state() unconditionally as suggested by Ulf.
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2694
Quoted 2 lines
drivers/base/power/domain.c +1
-0+ pstate =of_get_required_opp_performance_state (np, index);If you have more than one power domain, then this will override the pstate which was set for a previous domain. This code doesn't feel solid to me, at least a clarifying comment is needed about how it's supposed to work.
I don't quite understand the concern here, this should work with devices having multiple power-domains as well. __genpd_dev_pm_attach gets called once per power-domain, and we use the index above to identify the power-domain.
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2700
Quoted 6 lines
drivers/base/power/domain.c +6
-0+if (pstate <0 ) { + ret = pstate; +dev_err (dev,"failed to set required performance state for power-domain %s: %d\n" , + pd->name, ret); + }else { +dev_pm_genpd_set_performance_state (dev, pstate);Where is error handling?
Sure, I'll fix that.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/base/power/domain.c static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, · line 2703
Quoted 4 lines
drivers/base/power/domain.c +3
-0+dev_gpd_data (dev)->default_pstate = pstate; + } + }Why performance state is set after
genpd_power_on()?
The requirement is that the domain is operating at a given performance state before the device becomes operational. The driver should ideally wait for the attach to complete before expecting the device to be functional. So the order here should really not matter, or rather the small amount of time while the power-domain is on but not at the right performance state should not matter if that's the concern.
Quoted 12 lines
Should
set_performance_state()be invoked when power_on=false?I assume it should be:
if (power_on) { dev_pm_genpd_set_performance_state(dev, pstate); dev_gpd_data(dev)->default_pstate = pstate; } else { dev_gpd_data(dev)->rpm_pstate = pstate; }
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
11.08.2021 12:45, Rajendra Nayak пишет:
Quoted 10 lines
Quoted 5 lines
If you have more than one power domain, then this will override the pstate which was set for a previous domain. This code doesn't feel solid to me, at least a clarifying comment is needed about how it's supposed to work.
I don't quite understand the concern here, this should work with devices having multiple power-domains as well.
__genpd_dev_pm_attachgets called once per power-domain, and we use the index above to identify the power-domain.
The domain core code supports only one domain per device, see what genpd_set_performance_state() does. This means that the second domain will set the state of the *first* domain, which doesn't make sense. The genpd_set_performance_state() will actually fail with -ENODEV for all domains if you will try to do that.
11.08.2021 22:48, Dmitry Osipenko пишет:
Quoted 12 lines
11.08.2021 12:45, Rajendra Nayak пишет:
Quoted 10 lines
Quoted 5 lines
If you have more than one power domain, then this will override the pstate which was set for a previous domain. This code doesn't feel solid to me, at least a clarifying comment is needed about how it's supposed to work.
I don't quite understand the concern here, this should work with devices having multiple power-domains as well.
__genpd_dev_pm_attachgets called once per power-domain, and we use the index above to identify the power-domain.
Quoted 6 lines
The domain core code supports only one domain per device, see what
genpd_set_performance_state()does. This means that the second domain will set the state of the *first* domain, which doesn't make sense. Thegenpd_set_performance_state()will actually fail with-ENODEVfor all domains if you will try to do that.
I confused the base device with the virtual device there, looks like it should be okay then.
PATCH v6 2/2 arm64: dts: sc7180: Add required-opps for i2c
In reply to Rajendra Nayakqup-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.
--- arch/arm64/boot/dts/qcom/sc7180.dtsi | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
arch/arm64/boot/dts/qcom/sc7180.dtsi +24 -0
@@ -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 {
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation