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(-)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
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 <redacted>
---
drivers/base/power/domain.c | 28 ++++++++++++++++++++++++++--
include/linux/pm_domain.h | 1 +
2 files changed, 27 insertions(+), 2 deletions(-)
@@ -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;+}+}-returnret?-EPROBE_DEFER:1;+returnret?ret:1;}/**
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 <redacted>
Reviewed-by: Stephen Boyd <redacted>
---
arch/arm64/boot/dts/qcom/sc7180.dtsi | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
On Wed, 4 Aug 2021 at 12:58, Rajendra Nayak [off-list ref] wrote:
quoted hunk
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 <redacted>
---
drivers/base/power/domain.c | 28 ++++++++++++++++++++++++++--
include/linux/pm_domain.h | 1 +
2 files changed, 27 insertions(+), 2 deletions(-)
@@ -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)){
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 hunk
+ 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; } /**
On Wed, 4 Aug 2021 at 12:58, Rajendra Nayak [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/base/power/domain.c | 28 ++++++++++++++++++++++++++--
include/linux/pm_domain.h | 1 +
2 files changed, 27 insertions(+), 2 deletions(-)
@@ -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)){
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?
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
+ 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; } /**
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
quoted
+ 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);
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;
}
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
quoted
quoted
+ 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);
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;
}
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.
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?
No.
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.
--
viresh
@@ -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)) {
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.
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
quoted
quoted
+ 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);
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.
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;
}
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
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?
No.
quoted
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?
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
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.
--
viresh
On Wed, 11 Aug 2021 at 12:07, Viresh Kumar [off-list ref] wrote:
On 11-08-21, 15:30, Rajendra Nayak wrote:
quoted
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.
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
On Wed, 11 Aug 2021 at 12:07, Viresh Kumar [off-list ref] wrote:
quoted
On 11-08-21, 15:30, Rajendra Nayak wrote:
quoted
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.
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.
I would like to work with the existing set of APIs, as the OPP core is
going to do that check anyways, again.
--
viresh
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.
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.
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.
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.
I confused the base device with the virtual device there, looks like it
should be okay then.