Hi,
Some platforms (like TI) have complex DVFS configuration for CPU devices, where multiple regulators are required to be configured to change DVFS state of the device. This was explained well by Nishanth earlier [1].
One of the major complaints around multiple regulators case was that the DT isn't responsible in any way to represent the ordering in which multiple supplies need to be programmed, before or after frequency change. It was considered in this patch and such information is left to the platform specific OPP driver now, which can register its own opp_set_rate() callback with the OPP core and the OPP core will then call it during DVFS.
The patches are tested on Exynos5250 (Dual A15). I have hacked around DT and code to pass values for multiple regulators and verified that they are all properly read by the kernel (using debugfs interface).
Dave Gerlach has already tested it on the real TI platforms and it works well for him.
This is rebased over: linux-next branch in the PM tree.
V2->V3: - The last patch is new - Removed a debug leftover pr_info() message - Renamed few names as s/set_rate/set_opp - Removed a TODO comment (as it is done now with this series) - created struct for min_uV and max_uV - kerneldoc comments for structures in pm_opp.h - s/const char */const char * const - use kasprintf() - Some more minor reformatting - More Ack/RBY tags added
V1->V2: - Ack from Rob for 1st patch - Moved the supplies structure to pm_opp.h (Dave) - Fixed an compilation warning.
-- viresh
[1] https://marc.info/?l=linux-pm&m=145684495832764&w=2
Viresh Kumar (9): PM / OPP: Reword binding supporting multiple regulators per device PM / OPP: Don't use OPP structure outside of rcu protected section PM / OPP: Manage supply's voltage/current in a separate structure PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage() PM / OPP: Add infrastructure to manage multiple regulators PM / OPP: Separate out _generic_opp_set_rate() PM / OPP: Allow platform specific custom set_opp() callbacks PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators() PM / OPP: Don't assume platform doesn't have regulators
Documentation/devicetree/bindings/opp/opp.txt | 25 +- drivers/base/power/opp/core.c | 510 ++++++++++++++++++++------ drivers/base/power/opp/debugfs.c | 52 ++- drivers/base/power/opp/of.c | 105 ++++-- drivers/base/power/opp/opp.h | 20 +- drivers/cpufreq/cpufreq-dt.c | 9 +- include/linux/pm_opp.h | 67 +++- 7 files changed, 605 insertions(+), 183 deletions(-)
On certain platforms (like TI), DVFS for a single device (CPU) requires configuring multiple power supplies.
The OPP bindings already contains binding and example to explain this case, but it isn't sufficient. For example, there is no way for the code parsing these bindings to know which voltage values belong to which power supply. Also its not possible to know the order in which the supplies need to be configured while switching OPPs.
This patch tries to clarify on those details and does some minor changes as well.
Note that the bindings do not specify the order in which the regulators need to be programmed and the order in which the entries are added for the supplies.
The user of the bindings (like the kernel) shall know these details already and the DT is responsible to supply only the readings for the regulators.
Cc: Mark Brown broonie@kernel.org Cc: devicetree@vger.kernel.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Acked-by: Rob Herring robh@kernel.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org --- Documentation/devicetree/bindings/opp/opp.txt | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt index ee91cbdd95ee..af476df510f1 100644 --- a/Documentation/devicetree/bindings/opp/opp.txt +++ b/Documentation/devicetree/bindings/opp/opp.txt @@ -86,8 +86,13 @@ properties. Single entry is for target voltage and three entries are for <target min max> voltages.
- Entries for multiple regulators must be present in the same order as - regulators are specified in device's DT node. + Entries for multiple regulators shall be provided in the same field separated + by angular brackets <>. The OPP binding doesn't provide any provisions to + relate the values to their power supplies or the order in which the supplies + need to be configured. + + Entries for all regulators shall be of the same size, i.e. either all use a + single value or triplets.
- opp-microvolt-<name>: Named opp-microvolt property. This is exactly similar to the above opp-microvolt property, but allows multiple voltage ranges to be @@ -104,10 +109,12 @@ properties.
Should only be set if opp-microvolt is set for the OPP.
- Entries for multiple regulators must be present in the same order as - regulators are specified in device's DT node. If this property isn't required - for few regulators, then this should be marked as zero for them. If it isn't - required for any regulator, then this property need not be present. + Entries for multiple regulators shall be provided in the same field separated + by angular brackets <>. If current values aren't required for a regulator, + then it shall be filled with 0. If current values aren't required for any of + the regulators, then this field is not required. The OPP binding doesn't + provide any provisions to relate the values to their power supplies or the + order in which the supplies need to be configured.
- opp-microamp-<name>: Named opp-microamp property. Similar to opp-microvolt-<name> property, but for microamp instead. @@ -386,10 +393,12 @@ Example 4: Handling multiple regulators / { cpus { cpu@0 { - compatible = "arm,cortex-a7"; + compatible = "vendor,cpu-type"; ...
- cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>; + vcc0-supply = <&cpu_supply0>; + vcc1-supply = <&cpu_supply1>; + vcc2-supply = <&cpu_supply2>; operating-points-v2 = <&cpu0_opp_table>; }; };
On Wed, Oct 26, 2016 at 12:02:56PM +0530, Viresh Kumar wrote:
- Entries for multiple regulators shall be provided in the same field separated
- by angular brackets <>. The OPP binding doesn't provide any provisions to
- relate the values to their power supplies or the order in which the supplies
- need to be configured.
I don't understand how this works. If we have an unordered list of values to set for regulators how will we make sense of them?
cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
vcc0-supply = <&cpu_supply0>;vcc1-supply = <&cpu_supply1>;vcc2-supply = <&cpu_supply2>;
This change doesn't seem to correspond to the documentation change.
On 09-11-16, 14:58, Mark Brown wrote:
On Wed, Oct 26, 2016 at 12:02:56PM +0530, Viresh Kumar wrote:
- Entries for multiple regulators shall be provided in the same field separated
- by angular brackets <>. The OPP binding doesn't provide any provisions to
- relate the values to their power supplies or the order in which the supplies
- need to be configured.
I don't understand how this works. If we have an unordered list of values to set for regulators how will we make sense of them?
The platform driver is responsible to identify the order and pass it on to the OPP core. And the platform driver needs to have that hard coded.
If we want to identify the entries for regulators just by parsing the DT then we would need another field in the OPP table which I added earlier.
Something like this:
cpu0_opp_table: opp_table0 { compatible = "operating-points-v2"; + supply-names = "vcc0", "vcc1", "vcc2"; opp-shared;
opp00 {
Will that be acceptable ?
cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
vcc0-supply = <&cpu_supply0>;vcc1-supply = <&cpu_supply1>;vcc2-supply = <&cpu_supply2>;This change doesn't seem to correspond to the documentation change.
This rectifies the incorrect binding previously added to the example, which I realized to be incorrect only while attempting to code for it. And so it brings the example on the same state as the documentation now.
On Thu, Nov 10, 2016 at 09:34:40AM +0530, Viresh Kumar wrote:
On 09-11-16, 14:58, Mark Brown wrote:
On Wed, Oct 26, 2016 at 12:02:56PM +0530, Viresh Kumar wrote:
- Entries for multiple regulators shall be provided in the same field separated
- by angular brackets <>. The OPP binding doesn't provide any provisions to
- relate the values to their power supplies or the order in which the supplies
- need to be configured.
I don't understand how this works. If we have an unordered list of values to set for regulators how will we make sense of them?
The platform driver is responsible to identify the order and pass it on to the OPP core. And the platform driver needs to have that hard coded.
That *really* should be in the binding. Honestly if the binding is this vague I'm not even clear that it's worth documenting these properties at this level, might be better to just put the documentation in the platform driver bindings.
cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
vcc0-supply = <&cpu_supply0>;vcc1-supply = <&cpu_supply1>;vcc2-supply = <&cpu_supply2>;
This change doesn't seem to correspond to the documentation change.
This rectifies the incorrect binding previously added to the example, which I realized to be incorrect only while attempting to code for it. And so it brings the example on the same state as the documentation now.
Then that should be in a separate patch with a changelog explaining what the change is doing.
On 10-11-16, 16:36, Mark Brown wrote:
On Thu, Nov 10, 2016 at 09:34:40AM +0530, Viresh Kumar wrote:
On 09-11-16, 14:58, Mark Brown wrote:
On Wed, Oct 26, 2016 at 12:02:56PM +0530, Viresh Kumar wrote:
- Entries for multiple regulators shall be provided in the same field separated
- by angular brackets <>. The OPP binding doesn't provide any provisions to
- relate the values to their power supplies or the order in which the supplies
- need to be configured.
I don't understand how this works. If we have an unordered list of values to set for regulators how will we make sense of them?
The platform driver is responsible to identify the order and pass it on to the OPP core. And the platform driver needs to have that hard coded.
That *really* should be in the binding.
Okay, how do you suggest doing that? Will a property like supply-names in the OPP table be fine? Like this:
@@ -369,13 +378,16 @@ Example 4: Handling multiple regulators compatible = "arm,cortex-a7"; ...
- cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>; + vcc0-supply = <&cpu_supply0>; + vcc1-supply = <&cpu_supply1>; + vcc2-supply = <&cpu_supply2>; operating-points-v2 = <&cpu0_opp_table>; }; };
cpu0_opp_table: opp_table0 { compatible = "operating-points-v2"; + supply-names = "vcc0", "vcc1", "vcc2"; opp-shared;
On 11/10, Viresh Kumar wrote:
On 10-11-16, 16:36, Mark Brown wrote:
On Thu, Nov 10, 2016 at 09:34:40AM +0530, Viresh Kumar wrote:
On 09-11-16, 14:58, Mark Brown wrote:
On Wed, Oct 26, 2016 at 12:02:56PM +0530, Viresh Kumar wrote:
- Entries for multiple regulators shall be provided in the same field separated
- by angular brackets <>. The OPP binding doesn't provide any provisions to
- relate the values to their power supplies or the order in which the supplies
- need to be configured.
I don't understand how this works. If we have an unordered list of values to set for regulators how will we make sense of them?
The platform driver is responsible to identify the order and pass it on to the OPP core. And the platform driver needs to have that hard coded.
That *really* should be in the binding.
Okay, how do you suggest doing that? Will a property like supply-names in the OPP table be fine? Like this:
@@ -369,13 +378,16 @@ Example 4: Handling multiple regulators compatible = "arm,cortex-a7"; ...
cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
vcc0-supply = <&cpu_supply0>;vcc1-supply = <&cpu_supply1>;vcc2-supply = <&cpu_supply2>; operating-points-v2 = <&cpu0_opp_table>; }; };cpu0_opp_table: opp_table0 { compatible = "operating-points-v2";
supply-names = "vcc0", "vcc1", "vcc2"; opp-shared;
No. The supply names (and also clock names/index) should be left up to the consumer of the OPP table. We don't want to encode any sort of details like this between the OPP table and the consumer of it in DT because then it seriously couples the OPP table to the consumer device. "The binding" in this case that needs to be updated is the consumer binding, to indicate that it correlated foo-supply and bar-supply to index 0 and 1 of the OPP table voltages.
On 10-11-16, 14:51, Stephen Boyd wrote:
On 11/10, Viresh Kumar wrote:
On 10-11-16, 16:36, Mark Brown wrote:
On Thu, Nov 10, 2016 at 09:34:40AM +0530, Viresh Kumar wrote:
On 09-11-16, 14:58, Mark Brown wrote:
On Wed, Oct 26, 2016 at 12:02:56PM +0530, Viresh Kumar wrote:
- Entries for multiple regulators shall be provided in the same field separated
- by angular brackets <>. The OPP binding doesn't provide any provisions to
- relate the values to their power supplies or the order in which the supplies
- need to be configured.
I don't understand how this works. If we have an unordered list of values to set for regulators how will we make sense of them?
The platform driver is responsible to identify the order and pass it on to the OPP core. And the platform driver needs to have that hard coded.
That *really* should be in the binding.
Okay, how do you suggest doing that? Will a property like supply-names in the OPP table be fine? Like this:
@@ -369,13 +378,16 @@ Example 4: Handling multiple regulators compatible = "arm,cortex-a7"; ...
cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
vcc0-supply = <&cpu_supply0>;vcc1-supply = <&cpu_supply1>;vcc2-supply = <&cpu_supply2>; operating-points-v2 = <&cpu0_opp_table>; }; };cpu0_opp_table: opp_table0 { compatible = "operating-points-v2";
supply-names = "vcc0", "vcc1", "vcc2"; opp-shared;No. The supply names (and also clock names/index) should be left up to the consumer of the OPP table. We don't want to encode any sort of details like this between the OPP table and the consumer of it in DT because then it seriously couples the OPP table to the consumer device. "The binding" in this case that needs to be updated is the consumer binding, to indicate that it correlated foo-supply and bar-supply to index 0 and 1 of the OPP table voltages.
Are you saying that we shall have a property like this then?
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt index ee91cbdd95ee..733946df2fb8 100644 --- a/Documentation/devicetree/bindings/opp/opp.txt +++ b/Documentation/devicetree/bindings/opp/opp.txt @@ -389,7 +389,10 @@ Example 4: Handling multiple regulators compatible = "arm,cortex-a7"; ...
- cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>; + vcc0-supply = <&cpu_supply0>; + vcc1-supply = <&cpu_supply1>; + vcc2-supply = <&cpu_supply2>; + opp-supply-names = "vcc0", "vcc1", "vcc2"; operating-points-v2 = <&cpu0_opp_table>; }; };
On Fri, Nov 11, 2016 at 08:41:20AM +0530, Viresh Kumar wrote:
On 10-11-16, 14:51, Stephen Boyd wrote:
On 11/10, Viresh Kumar wrote:
On 10-11-16, 16:36, Mark Brown wrote:
On Thu, Nov 10, 2016 at 09:34:40AM +0530, Viresh Kumar wrote:
On 09-11-16, 14:58, Mark Brown wrote:
On Wed, Oct 26, 2016 at 12:02:56PM +0530, Viresh Kumar wrote:
> + Entries for multiple regulators shall be provided in the same field separated > + by angular brackets <>. The OPP binding doesn't provide any provisions to > + relate the values to their power supplies or the order in which the supplies > + need to be configured.
I don't understand how this works. If we have an unordered list of values to set for regulators how will we make sense of them?
The platform driver is responsible to identify the order and pass it on to the OPP core. And the platform driver needs to have that hard coded.
That *really* should be in the binding.
Okay, how do you suggest doing that? Will a property like supply-names in the OPP table be fine? Like this:
@@ -369,13 +378,16 @@ Example 4: Handling multiple regulators compatible = "arm,cortex-a7"; ...
cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
vcc0-supply = <&cpu_supply0>;vcc1-supply = <&cpu_supply1>;vcc2-supply = <&cpu_supply2>; operating-points-v2 = <&cpu0_opp_table>; }; };cpu0_opp_table: opp_table0 { compatible = "operating-points-v2";
supply-names = "vcc0", "vcc1", "vcc2"; opp-shared;No. The supply names (and also clock names/index) should be left up to the consumer of the OPP table. We don't want to encode any sort of details like this between the OPP table and the consumer of it in DT because then it seriously couples the OPP table to the consumer device. "The binding" in this case that needs to be updated is the consumer binding, to indicate that it correlated foo-supply and bar-supply to index 0 and 1 of the OPP table voltages.
Are you saying that we shall have a property like this then?
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt index ee91cbdd95ee..733946df2fb8 100644 --- a/Documentation/devicetree/bindings/opp/opp.txt +++ b/Documentation/devicetree/bindings/opp/opp.txt @@ -389,7 +389,10 @@ Example 4: Handling multiple regulators compatible = "arm,cortex-a7"; ...
cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
vcc0-supply = <&cpu_supply0>;vcc1-supply = <&cpu_supply1>;vcc2-supply = <&cpu_supply2>;opp-supply-names = "vcc0", "vcc1", "vcc2";
Uh, no. You already have the names in the *-supply properties. Yes, they are a PIA to retrieve compared to a *-names property, but that is the nature of this style of binding.
Rob
On 11/14, Rob Herring wrote:
On Fri, Nov 11, 2016 at 08:41:20AM +0530, Viresh Kumar wrote:
On 10-11-16, 14:51, Stephen Boyd wrote:
No. The supply names (and also clock names/index) should be left up to the consumer of the OPP table. We don't want to encode any sort of details like this between the OPP table and the consumer of it in DT because then it seriously couples the OPP table to the consumer device. "The binding" in this case that needs to be updated is the consumer binding, to indicate that it correlated foo-supply and bar-supply to index 0 and 1 of the OPP table voltages.
Are you saying that we shall have a property like this then?
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt index ee91cbdd95ee..733946df2fb8 100644 --- a/Documentation/devicetree/bindings/opp/opp.txt +++ b/Documentation/devicetree/bindings/opp/opp.txt @@ -389,7 +389,10 @@ Example 4: Handling multiple regulators compatible = "arm,cortex-a7"; ...
cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
vcc0-supply = <&cpu_supply0>;vcc1-supply = <&cpu_supply1>;vcc2-supply = <&cpu_supply2>;opp-supply-names = "vcc0", "vcc1", "vcc2";Uh, no. You already have the names in the *-supply properties. Yes, they are a PIA to retrieve compared to a *-names property, but that is the nature of this style of binding.
I think the problem is that Viresh wants the binding to be "self describing" so that the OPP can be used without a driver knowing that a supply corresponds to a particular column in the voltage table. I don't understand that though. Can't we set the supply names from C code somewhere based on the consumer of the OPPs? Similar to how we pick the different tables based on fuses?
First of all, thanks to all of you for commenting here. Please continue doing so as I want to finish this stuff quickly, it has already killed a lot of time :)
On 14-11-16, 18:13, Stephen Boyd wrote:
On 11/14, Rob Herring wrote:
On Fri, Nov 11, 2016 at 08:41:20AM +0530, Viresh Kumar wrote:
On 10-11-16, 14:51, Stephen Boyd wrote:
No. The supply names (and also clock names/index) should be left up to the consumer of the OPP table. We don't want to encode any sort of details like this between the OPP table and the consumer of it in DT because then it seriously couples the OPP table to the consumer device. "The binding" in this case that needs to be updated is the consumer binding, to indicate that it correlated foo-supply and bar-supply to index 0 and 1 of the OPP table voltages.
Are you saying that we shall have a property like this then?
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt index ee91cbdd95ee..733946df2fb8 100644 --- a/Documentation/devicetree/bindings/opp/opp.txt +++ b/Documentation/devicetree/bindings/opp/opp.txt @@ -389,7 +389,10 @@ Example 4: Handling multiple regulators compatible = "arm,cortex-a7"; ...
cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
vcc0-supply = <&cpu_supply0>;vcc1-supply = <&cpu_supply1>;vcc2-supply = <&cpu_supply2>;opp-supply-names = "vcc0", "vcc1", "vcc2";Uh, no. You already have the names in the *-supply properties. Yes, they are a PIA to retrieve compared to a *-names property, but that is the nature of this style of binding.
Its not just PIA, but impossible AFAICT.
There are two important pieces of information we need for multiple regulator support: - Which regulator in the consumer node corresponds to which entry in the OPP table. As Mark mentioned earlier, DT should be able to get us this. - The order in which the supplies need to be programmed. We have all agreed to do this in code instead of inferring it from DT and this patch series already does that.
I want to solve the first problem here and I don't see how it can be solved using such entries:
cpus { cpu@0 { compatible = "arm,cortex-a7"; ...
vcc0-supply = <&cpu_supply0>; vcc1-supply = <&cpu_supply1>; vcc2-supply = <&cpu_supply2>; operating-points-v2 = <&cpu0_opp_table>; }; };
cpu0_opp_table: opp_table0 { compatible = "operating-points-v2"; opp-shared;
opp@1000000000 { opp-hz = /bits/ 64 <1000000000>; opp-microvolt = <970000>, /* Supply 0 */ <960000>, /* Supply 1 */ <960000>; /* Supply 2 */ }; };
The code can't figure out which of vcc0, vcc1, vcc2 is added first in the CPU node and so we need to get the order somehow. A separate binding as I mentioned earlier is a probably (ugly) solution.
I think the problem is that Viresh wants the binding to be "self describing" so that the OPP can be used without a driver knowing that a supply corresponds to a particular column in the voltage table.
Right, and that's what Mark suggested as well.
I don't understand that though. Can't we set the supply names from C code somewhere based on the consumer of the OPPs?
That's what this patch series is doing right now.
So, are you saying that the way this patchset does it is fine with you ?
On 11/15, Viresh Kumar wrote:
On 14-11-16, 18:13, Stephen Boyd wrote:
On 11/14, Rob Herring wrote:
On Fri, Nov 11, 2016 at 08:41:20AM +0530, Viresh Kumar wrote:
On 10-11-16, 14:51, Stephen Boyd wrote:
No. The supply names (and also clock names/index) should be left up to the consumer of the OPP table. We don't want to encode any sort of details like this between the OPP table and the consumer of it in DT because then it seriously couples the OPP table to the consumer device. "The binding" in this case that needs to be updated is the consumer binding, to indicate that it correlated foo-supply and bar-supply to index 0 and 1 of the OPP table voltages.
Are you saying that we shall have a property like this then?
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt index ee91cbdd95ee..733946df2fb8 100644 --- a/Documentation/devicetree/bindings/opp/opp.txt +++ b/Documentation/devicetree/bindings/opp/opp.txt @@ -389,7 +389,10 @@ Example 4: Handling multiple regulators compatible = "arm,cortex-a7"; ...
cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
vcc0-supply = <&cpu_supply0>;vcc1-supply = <&cpu_supply1>;vcc2-supply = <&cpu_supply2>;opp-supply-names = "vcc0", "vcc1", "vcc2";Uh, no. You already have the names in the *-supply properties. Yes, they are a PIA to retrieve compared to a *-names property, but that is the nature of this style of binding.
Its not just PIA, but impossible AFAICT.
There are two important pieces of information we need for multiple regulator support:
- Which regulator in the consumer node corresponds to which entry in the OPP table. As Mark mentioned earlier, DT should be able to get us this.
This is also possible from C code though. Or is there some case where it isn't possible if we're sharing the same table with two devices? I'm lost on when this would ever happen.
It feels like trying to keep the OPP table agnostic of the consuming device and the device's binding is more trouble than it's worth. Especially considering we have opp-shared and *-name now.
- The order in which the supplies need to be programmed. We have all agreed to do this in code instead of inferring it from DT and this patch series already does that.
Agreed. Encoding a sequence into DT doesn't sound very feasible. How is this going to be handled though? I don't see any users of the code we're reviewing here, so it's hard to grasp how things will work. It would be really useful if we had some user of the code included in the patch series to get the big picture.
I want to solve the first problem here and I don't see how it can be solved using such entries:
cpus { cpu@0 { compatible = "arm,cortex-a7"; ...
vcc0-supply = <&cpu_supply0>; vcc1-supply = <&cpu_supply1>; vcc2-supply = <&cpu_supply2>; operating-points-v2 = <&cpu0_opp_table>; }; };cpu0_opp_table: opp_table0 { compatible = "operating-points-v2"; opp-shared;
opp@1000000000 { opp-hz = /bits/ 64 <1000000000>; opp-microvolt = <970000>, /* Supply 0 */ <960000>, /* Supply 1 */ <960000>; /* Supply 2 */ }; };The code can't figure out which of vcc0, vcc1, vcc2 is added first in the CPU node and so we need to get the order somehow. A separate binding as I mentioned earlier is a probably (ugly) solution.
I think the problem is that Viresh wants the binding to be "self describing" so that the OPP can be used without a driver knowing that a supply corresponds to a particular column in the voltage table.
Right, and that's what Mark suggested as well.
I don't understand that though. Can't we set the supply names from C code somewhere based on the consumer of the OPPs?
That's what this patch series is doing right now.
So, are you saying that the way this patchset does it is fine with you ?
That's just to handle the ordering of operations? I need to take a minute and understand what's changing. You may have spent plenty of time developing/updating, but I haven't spent near enough time understanding what's going on in these patches to give a thorough review.
Hi, On 11/15/2016 12:56 PM, Stephen Boyd wrote:
On 11/15, Viresh Kumar wrote:
On 14-11-16, 18:13, Stephen Boyd wrote:
On 11/14, Rob Herring wrote:
On Fri, Nov 11, 2016 at 08:41:20AM +0530, Viresh Kumar wrote:
On 10-11-16, 14:51, Stephen Boyd wrote:
No. The supply names (and also clock names/index) should be left up to the consumer of the OPP table. We don't want to encode any sort of details like this between the OPP table and the consumer of it in DT because then it seriously couples the OPP table to the consumer device. "The binding" in this case that needs to be updated is the consumer binding, to indicate that it correlated foo-supply and bar-supply to index 0 and 1 of the OPP table voltages.
Are you saying that we shall have a property like this then?
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt index ee91cbdd95ee..733946df2fb8 100644 --- a/Documentation/devicetree/bindings/opp/opp.txt +++ b/Documentation/devicetree/bindings/opp/opp.txt @@ -389,7 +389,10 @@ Example 4: Handling multiple regulators compatible = "arm,cortex-a7"; ...
cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&cpu_supply2>;
vcc0-supply = <&cpu_supply0>;vcc1-supply = <&cpu_supply1>;vcc2-supply = <&cpu_supply2>;opp-supply-names = "vcc0", "vcc1", "vcc2";Uh, no. You already have the names in the *-supply properties. Yes, they are a PIA to retrieve compared to a *-names property, but that is the nature of this style of binding.
Its not just PIA, but impossible AFAICT.
There are two important pieces of information we need for multiple regulator support:
- Which regulator in the consumer node corresponds to which entry in the OPP table. As Mark mentioned earlier, DT should be able to get us this.
This is also possible from C code though. Or is there some case where it isn't possible if we're sharing the same table with two devices? I'm lost on when this would ever happen.
It feels like trying to keep the OPP table agnostic of the consuming device and the device's binding is more trouble than it's worth. Especially considering we have opp-shared and *-name now.
I agree with this, I do not like having to pass a list of regulator names to the opp core that I *hope* the device I am controlling has provided. The intent seems to be to use the cpufreq-dt driver as is and not pass any cpu-supply anymore so the cpufreq-dt driver has no knowledge of what regulators are present (it operates as it would today on a system with no regulator required). But as is it will move forward regardless of whether or not we actually intended to provide a multi regulator set up or platform set_opp helper, and this probably isn't ideal. I would think cpufreq-dt/opp core should be have knowledge of what regulators are needed to achieve these opp transitions and make sure everything is in place before moving ahead.
- The order in which the supplies need to be programmed. We have all agreed to do this in code instead of inferring it from DT and this patch series already does that.
Agreed. Encoding a sequence into DT doesn't sound very feasible. How is this going to be handled though? I don't see any users of the code we're reviewing here, so it's hard to grasp how things will work. It would be really useful if we had some user of the code included in the patch series to get the big picture.
I have sent a patch in reply to the cover letter of this series showing the driver that I used to test multi regulator on TI am57x platform and wrote as much detail as I could on how I used what Viresh has provided. Perhaps that will show how this can be used and help to see what's missing from the core implementation here.
Previous discussions drove me to pass regulators and necessary values in the DT but do all sequencing from the driver from fixed code without inferring anything from the device tree.
Regards, Dave
I want to solve the first problem here and I don't see how it can be solved using such entries:
cpus { cpu@0 { compatible = "arm,cortex-a7"; ...
vcc0-supply = <&cpu_supply0>; vcc1-supply = <&cpu_supply1>; vcc2-supply = <&cpu_supply2>; operating-points-v2 = <&cpu0_opp_table>; }; };cpu0_opp_table: opp_table0 { compatible = "operating-points-v2"; opp-shared;
opp@1000000000 { opp-hz = /bits/ 64 <1000000000>; opp-microvolt = <970000>, /* Supply 0 */ <960000>, /* Supply 1 */ <960000>; /* Supply 2 */ }; };The code can't figure out which of vcc0, vcc1, vcc2 is added first in the CPU node and so we need to get the order somehow. A separate binding as I mentioned earlier is a probably (ugly) solution.
I think the problem is that Viresh wants the binding to be "self describing" so that the OPP can be used without a driver knowing that a supply corresponds to a particular column in the voltage table.
Right, and that's what Mark suggested as well.
I don't understand that though. Can't we set the supply names from C code somewhere based on the consumer of the OPPs?
That's what this patch series is doing right now.
So, are you saying that the way this patchset does it is fine with you ?
That's just to handle the ordering of operations? I need to take a minute and understand what's changing. You may have spent plenty of time developing/updating, but I haven't spent near enough time understanding what's going on in these patches to give a thorough review.
On 15-11-16, 16:11, Dave Gerlach wrote:
On 11/15/2016 12:56 PM, Stephen Boyd wrote:
On 11/15, Viresh Kumar wrote:
There are two important pieces of information we need for multiple regulator support:
- Which regulator in the consumer node corresponds to which entry in
the OPP table. As Mark mentioned earlier, DT should be able to get us this.
This is also possible from C code though. Or is there some case where it isn't possible if we're sharing the same table with two devices? I'm lost on when this would ever happen.
It feels like trying to keep the OPP table agnostic of the consuming device and the device's binding is more trouble than it's worth. Especially considering we have opp-shared and *-name now.
I agree with this, I do not like having to pass a list of regulator names to the opp core that I *hope* the device I am controlling has provided.
What do you mean by that? Are you saying this from DT's point of view or of the code? i.e. Are you saying that you don't like the dev_pm_opp_set_regulators() API ?
The intent seems to be to use the cpufreq-dt driver as is and not pass any
I would like to kill all regulators code from cpufreq-dt sometime soon. All that is left there is making sure we have a regulator in place, but I strongly feel OPP core is the right place for doing that now.
cpu-supply anymore so the cpufreq-dt driver has no knowledge of what regulators are present (it operates as it would today on a system with no regulator required). But as is it will move forward regardless of whether or not we actually intended to provide a multi regulator set up or platform set_opp helper, and this probably isn't ideal.
Yes and that's why I am more inclined towards my above comment. We shall make it consistent.
I would think cpufreq-dt/opp core should be have knowledge of what regulators are needed to achieve these opp transitions and make sure everything is in place before moving ahead.
The last patch in my series does what you are looking for:
PM / OPP: Don't assume platform doesn't have regulators
Isn't it ?
On 15-11-16, 10:56, Stephen Boyd wrote:
This is also possible from C code though.
Right and this is what this patchset is doing right now. To make it clear, the order of regulator names in the call dev_pm_opp_set_regulators() is used now to communicate the order in which entries are present in the OPP table.
Or is there some case where it isn't possible if we're sharing the same table with two devices?
Even in that case it will be possible to set regulators separately, so that's not a problem.
I'm lost on when this would ever happen.
It would happen in case of Krait for example, where CPUs manage DVFS separately but their tables may all be same.
It feels like trying to keep the OPP table agnostic of the consuming device and the device's binding is more trouble than it's worth. Especially considering we have opp-shared and *-name now.
Right.
- The order in which the supplies need to be programmed. We have all agreed to do this in code instead of inferring it from DT and this patch series already does that.
Agreed. Encoding a sequence into DT doesn't sound very feasible. How is this going to be handled though? I don't see any users of the code we're reviewing here, so it's hard to grasp how things will work. It would be really useful if we had some user of the code included in the patch series to get the big picture.
The TI guys would be doing it soon. The sequence will be handled by platform specific set_opp() callbacks now. So, there is nothing in the core for that.
So, are you saying that the way this patchset does it is fine with you ?
That's just to handle the ordering of operations?
Not just that. The blocking question here is that "Do we want to know the sequence in which the entries for multiple regulators are present in the OPP nodes from the DT? Or is it fine to handle that in code".
And AFAIU, you are saying that we better handle that in code as handling that in DT is going to be nightmare without a new ugly property.
The OPP structure must not be used out of the rcu protected section. Cache the values to be used in separate variables instead.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com --- drivers/base/power/opp/core.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 4c7c6da7a989..056527a3fb4e 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -584,6 +584,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) struct clk *clk; unsigned long freq, old_freq; unsigned long u_volt, u_volt_min, u_volt_max; + unsigned long old_u_volt, old_u_volt_min, old_u_volt_max; int ret;
if (unlikely(!target_freq)) { @@ -633,6 +634,14 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) return ret; }
+ if (IS_ERR(old_opp)) { + old_u_volt = 0; + } else { + old_u_volt = old_opp->u_volt; + old_u_volt_min = old_opp->u_volt_min; + old_u_volt_max = old_opp->u_volt_max; + } + u_volt = opp->u_volt; u_volt_min = opp->u_volt_min; u_volt_max = opp->u_volt_max; @@ -677,9 +686,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) __func__, old_freq); restore_voltage: /* This shouldn't harm even if the voltages weren't updated earlier */ - if (!IS_ERR(old_opp)) - _set_opp_voltage(dev, reg, old_opp->u_volt, - old_opp->u_volt_min, old_opp->u_volt_max); + if (old_u_volt) { + _set_opp_voltage(dev, reg, old_u_volt, old_u_volt_min, + old_u_volt_max); + }
return ret; }
This is a preparatory step for multiple regulator per device support. Move the voltage/current variables to a new structure.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com --- drivers/base/power/opp/core.c | 44 +++++++++++++++++++++------------------- drivers/base/power/opp/debugfs.c | 8 ++++---- drivers/base/power/opp/of.c | 18 ++++++++-------- drivers/base/power/opp/opp.h | 11 +++------- include/linux/pm_opp.h | 16 +++++++++++++++ 5 files changed, 55 insertions(+), 42 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 056527a3fb4e..8d6006151c9a 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -112,7 +112,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp) if (IS_ERR_OR_NULL(tmp_opp)) pr_err("%s: Invalid parameters\n", __func__); else - v = tmp_opp->u_volt; + v = tmp_opp->supply.u_volt;
return v; } @@ -246,10 +246,10 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) if (!opp->available) continue;
- if (opp->u_volt_min < min_uV) - min_uV = opp->u_volt_min; - if (opp->u_volt_max > max_uV) - max_uV = opp->u_volt_max; + if (opp->supply.u_volt_min < min_uV) + min_uV = opp->supply.u_volt_min; + if (opp->supply.u_volt_max > max_uV) + max_uV = opp->supply.u_volt_max; }
rcu_read_unlock(); @@ -637,14 +637,14 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) if (IS_ERR(old_opp)) { old_u_volt = 0; } else { - old_u_volt = old_opp->u_volt; - old_u_volt_min = old_opp->u_volt_min; - old_u_volt_max = old_opp->u_volt_max; + old_u_volt = old_opp->supply.u_volt; + old_u_volt_min = old_opp->supply.u_volt_min; + old_u_volt_max = old_opp->supply.u_volt_max; }
- u_volt = opp->u_volt; - u_volt_min = opp->u_volt_min; - u_volt_max = opp->u_volt_max; + u_volt = opp->supply.u_volt; + u_volt_min = opp->supply.u_volt_min; + u_volt_max = opp->supply.u_volt_max;
reg = opp_table->regulator;
@@ -957,10 +957,11 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, struct regulator *reg = opp_table->regulator;
if (!IS_ERR(reg) && - !regulator_is_supported_voltage(reg, opp->u_volt_min, - opp->u_volt_max)) { + !regulator_is_supported_voltage(reg, opp->supply.u_volt_min, + opp->supply.u_volt_max)) { pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n", - __func__, opp->u_volt_min, opp->u_volt_max); + __func__, opp->supply.u_volt_min, + opp->supply.u_volt_max); return false; }
@@ -993,11 +994,12 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
/* Duplicate OPPs */ dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", - __func__, opp->rate, opp->u_volt, opp->available, - new_opp->rate, new_opp->u_volt, new_opp->available); + __func__, opp->rate, opp->supply.u_volt, + opp->available, new_opp->rate, new_opp->supply.u_volt, + new_opp->available);
- return opp->available && new_opp->u_volt == opp->u_volt ? - 0 : -EEXIST; + return opp->available && + new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST; }
new_opp->opp_table = opp_table; @@ -1064,9 +1066,9 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, /* populate the opp table */ new_opp->rate = freq; tol = u_volt * opp_table->voltage_tolerance_v1 / 100; - new_opp->u_volt = u_volt; - new_opp->u_volt_min = u_volt - tol; - new_opp->u_volt_max = u_volt + tol; + new_opp->supply.u_volt = u_volt; + new_opp->supply.u_volt_min = u_volt - tol; + new_opp->supply.u_volt_max = u_volt + tol; new_opp->available = true; new_opp->dynamic = dynamic;
diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c index ef1ae6b52042..c897676ca35f 100644 --- a/drivers/base/power/opp/debugfs.c +++ b/drivers/base/power/opp/debugfs.c @@ -63,16 +63,16 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate)) return -ENOMEM;
- if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->u_volt)) + if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt)) return -ENOMEM;
- if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->u_volt_min)) + if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min)) return -ENOMEM;
- if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->u_volt_max)) + if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max)) return -ENOMEM;
- if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->u_amp)) + if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp)) return -ENOMEM;
if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d, diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c index 5552211e6fcd..b7fcd0a1b58b 100644 --- a/drivers/base/power/opp/of.c +++ b/drivers/base/power/opp/of.c @@ -148,14 +148,14 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, return -EINVAL; }
- opp->u_volt = microvolt[0]; + opp->supply.u_volt = microvolt[0];
if (count == 1) { - opp->u_volt_min = opp->u_volt; - opp->u_volt_max = opp->u_volt; + opp->supply.u_volt_min = opp->supply.u_volt; + opp->supply.u_volt_max = opp->supply.u_volt; } else { - opp->u_volt_min = microvolt[1]; - opp->u_volt_max = microvolt[2]; + opp->supply.u_volt_min = microvolt[1]; + opp->supply.u_volt_max = microvolt[2]; }
/* Search for "opp-microamp-<name>" */ @@ -173,7 +173,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, }
if (prop && !of_property_read_u32(opp->np, name, &val)) - opp->u_amp = val; + opp->supply.u_amp = val;
return 0; } @@ -303,9 +303,9 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np) mutex_unlock(&opp_table_lock);
pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n", - __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt, - new_opp->u_volt_min, new_opp->u_volt_max, - new_opp->clock_latency_ns); + __func__, new_opp->turbo, new_opp->rate, + new_opp->supply.u_volt, new_opp->supply.u_volt_min, + new_opp->supply.u_volt_max, new_opp->clock_latency_ns);
/* * Notify the changes in the availability of the operable diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index fabd5ca1a083..791213175683 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -61,10 +61,7 @@ extern struct list_head opp_tables; * @turbo: true if turbo (boost) OPP * @suspend: true if suspend OPP * @rate: Frequency in hertz - * @u_volt: Target voltage in microvolts corresponding to this OPP - * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP - * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP - * @u_amp: Maximum current drawn by the device in microamperes + * @supply: Power supply voltage/current values * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's * frequency from any other OPP's frequency. * @opp_table: points back to the opp_table struct this opp belongs to @@ -83,10 +80,8 @@ struct dev_pm_opp { bool suspend; unsigned long rate;
- unsigned long u_volt; - unsigned long u_volt_min; - unsigned long u_volt_max; - unsigned long u_amp; + struct dev_pm_opp_supply supply; + unsigned long clock_latency_ns;
struct opp_table *opp_table; diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index bca26157f5b6..f69126e2bb59 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -24,6 +24,22 @@ enum dev_pm_opp_event { OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE, };
+/** + * struct dev_pm_opp_supply - Power supply voltage/current values + * @u_volt: Target voltage in microvolts corresponding to this OPP + * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP + * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP + * @u_amp: Maximum current drawn by the device in microamperes + * + * This structure stores the voltage/current values for a single power supply. + */ +struct dev_pm_opp_supply { + unsigned long u_volt; + unsigned long u_volt_min; + unsigned long u_volt_max; + unsigned long u_amp; +}; + #if defined(CONFIG_PM_OPP)
unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
Pass the entire supply structure instead of all of its fields.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com --- drivers/base/power/opp/core.c | 44 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 27 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 8d6006151c9a..37fad2eb0f47 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -542,8 +542,7 @@ static struct clk *_get_opp_clk(struct device *dev) }
static int _set_opp_voltage(struct device *dev, struct regulator *reg, - unsigned long u_volt, unsigned long u_volt_min, - unsigned long u_volt_max) + struct dev_pm_opp_supply *supply) { int ret;
@@ -554,14 +553,15 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg, return 0; }
- dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, u_volt_min, - u_volt, u_volt_max); + dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, + supply->u_volt_min, supply->u_volt, supply->u_volt_max);
- ret = regulator_set_voltage_triplet(reg, u_volt_min, u_volt, - u_volt_max); + ret = regulator_set_voltage_triplet(reg, supply->u_volt_min, + supply->u_volt, supply->u_volt_max); if (ret) dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n", - __func__, u_volt_min, u_volt, u_volt_max, ret); + __func__, supply->u_volt_min, supply->u_volt, + supply->u_volt_max, ret);
return ret; } @@ -583,8 +583,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) struct regulator *reg; struct clk *clk; unsigned long freq, old_freq; - unsigned long u_volt, u_volt_min, u_volt_max; - unsigned long old_u_volt, old_u_volt_min, old_u_volt_max; + struct dev_pm_opp_supply old_supply, new_supply; int ret;
if (unlikely(!target_freq)) { @@ -634,17 +633,12 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) return ret; }
- if (IS_ERR(old_opp)) { - old_u_volt = 0; - } else { - old_u_volt = old_opp->supply.u_volt; - old_u_volt_min = old_opp->supply.u_volt_min; - old_u_volt_max = old_opp->supply.u_volt_max; - } + if (IS_ERR(old_opp)) + old_supply.u_volt = 0; + else + memcpy(&old_supply, &old_opp->supply, sizeof(old_supply));
- u_volt = opp->supply.u_volt; - u_volt_min = opp->supply.u_volt_min; - u_volt_max = opp->supply.u_volt_max; + memcpy(&new_supply, &opp->supply, sizeof(new_supply));
reg = opp_table->regulator;
@@ -652,8 +646,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
/* Scaling up? Scale voltage before frequency */ if (freq > old_freq) { - ret = _set_opp_voltage(dev, reg, u_volt, u_volt_min, - u_volt_max); + ret = _set_opp_voltage(dev, reg, &new_supply); if (ret) goto restore_voltage; } @@ -672,8 +665,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
/* Scaling down? Scale voltage after frequency */ if (freq < old_freq) { - ret = _set_opp_voltage(dev, reg, u_volt, u_volt_min, - u_volt_max); + ret = _set_opp_voltage(dev, reg, &new_supply); if (ret) goto restore_freq; } @@ -686,10 +678,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) __func__, old_freq); restore_voltage: /* This shouldn't harm even if the voltages weren't updated earlier */ - if (old_u_volt) { - _set_opp_voltage(dev, reg, old_u_volt, old_u_volt_min, - old_u_volt_max); - } + if (old_supply.u_volt) + _set_opp_voltage(dev, reg, &old_supply);
return ret; }
This patch adds infrastructure to manage multiple regulators and updates the only user (cpufreq-dt) of dev_pm_opp_set{put}_regulator().
This is preparatory work for adding full support for devices with multiple regulators.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com --- drivers/base/power/opp/core.c | 220 ++++++++++++++++++++++++++------------- drivers/base/power/opp/debugfs.c | 52 +++++++-- drivers/base/power/opp/of.c | 103 ++++++++++++------ drivers/base/power/opp/opp.h | 10 +- drivers/cpufreq/cpufreq-dt.c | 9 +- include/linux/pm_opp.h | 8 +- 6 files changed, 280 insertions(+), 122 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 37fad2eb0f47..5a35fdd4b61b 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -93,6 +93,8 @@ struct opp_table *_find_opp_table(struct device *dev) * Return: voltage in micro volt corresponding to the opp, else * return 0 * + * This is useful only for devices with single power supply. + * * Locking: This function must be called under rcu_read_lock(). opp is a rcu * protected pointer. This means that opp which could have been fetched by * opp_find_freq_{exact,ceil,floor} functions is valid as long as we are @@ -112,7 +114,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp) if (IS_ERR_OR_NULL(tmp_opp)) pr_err("%s: Invalid parameters\n", __func__); else - v = tmp_opp->supply.u_volt; + v = tmp_opp->supplies[0].u_volt;
return v; } @@ -222,10 +224,13 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) { struct opp_table *opp_table; struct dev_pm_opp *opp; - struct regulator *reg; + struct regulator *reg, **regulators; unsigned long latency_ns = 0; - unsigned long min_uV = ~0, max_uV = 0; - int ret; + int ret, size, i, count; + struct { + unsigned long min; + unsigned long max; + } *uV;
rcu_read_lock();
@@ -235,21 +240,41 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) return 0; }
- reg = opp_table->regulator; - if (IS_ERR(reg)) { + count = opp_table->regulator_count; + + if (!count) { /* Regulator may not be required for device */ rcu_read_unlock(); return 0; }
- list_for_each_entry_rcu(opp, &opp_table->opp_list, node) { - if (!opp->available) - continue; + size = count * sizeof(*regulators); + regulators = kmemdup(opp_table->regulators, size, GFP_KERNEL); + if (!regulators) { + rcu_read_unlock(); + return 0; + }
- if (opp->supply.u_volt_min < min_uV) - min_uV = opp->supply.u_volt_min; - if (opp->supply.u_volt_max > max_uV) - max_uV = opp->supply.u_volt_max; + uV = kmalloc_array(count, sizeof(*uV), GFP_KERNEL); + if (!uV) { + kfree(regulators); + rcu_read_unlock(); + return 0; + } + + for (i = 0; i < count; i++) { + uV[i].min = ~0; + uV[i].max = 0; + + list_for_each_entry_rcu(opp, &opp_table->opp_list, node) { + if (!opp->available) + continue; + + if (opp->supplies[i].u_volt_min < uV[i].min) + uV[i].min = opp->supplies[i].u_volt_min; + if (opp->supplies[i].u_volt_max > uV[i].max) + uV[i].max = opp->supplies[i].u_volt_max; + } }
rcu_read_unlock(); @@ -258,9 +283,14 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) * The caller needs to ensure that opp_table (and hence the regulator) * isn't freed, while we are executing this routine. */ - ret = regulator_set_voltage_time(reg, min_uV, max_uV); - if (ret > 0) - latency_ns = ret * 1000; + for (i = 0; reg = regulators[i], i < count; i++) { + ret = regulator_set_voltage_time(reg, uV[i].min, uV[i].max); + if (ret > 0) + latency_ns += ret * 1000; + } + + kfree(uV); + kfree(regulators);
return latency_ns; } @@ -580,7 +610,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) { struct opp_table *opp_table; struct dev_pm_opp *old_opp, *opp; - struct regulator *reg; + struct regulator *reg = ERR_PTR(-ENXIO); struct clk *clk; unsigned long freq, old_freq; struct dev_pm_opp_supply old_supply, new_supply; @@ -633,14 +663,23 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) return ret; }
+ if (opp_table->regulators) { + /* This function only supports single regulator per device */ + if (WARN_ON(opp_table->regulator_count > 1)) { + dev_err(dev, "multiple regulators not supported\n"); + rcu_read_unlock(); + return -EINVAL; + } + + reg = opp_table->regulators[0]; + } + if (IS_ERR(old_opp)) old_supply.u_volt = 0; else - memcpy(&old_supply, &old_opp->supply, sizeof(old_supply)); + memcpy(&old_supply, old_opp->supplies, sizeof(old_supply));
- memcpy(&new_supply, &opp->supply, sizeof(new_supply)); - - reg = opp_table->regulator; + memcpy(&new_supply, opp->supplies, sizeof(new_supply));
rcu_read_unlock();
@@ -764,9 +803,6 @@ static struct opp_table *_add_opp_table(struct device *dev)
_of_init_opp_table(opp_table, dev);
- /* Set regulator to a non-NULL error value */ - opp_table->regulator = ERR_PTR(-ENXIO); - /* Find clk for the device */ opp_table->clk = clk_get(dev, NULL); if (IS_ERR(opp_table->clk)) { @@ -815,7 +851,7 @@ static void _remove_opp_table(struct opp_table *opp_table) if (opp_table->prop_name) return;
- if (!IS_ERR(opp_table->regulator)) + if (opp_table->regulators) return;
/* Release clk */ @@ -924,35 +960,50 @@ struct dev_pm_opp *_allocate_opp(struct device *dev, struct opp_table **opp_table) { struct dev_pm_opp *opp; + int count, supply_size; + struct opp_table *table;
- /* allocate new OPP node */ - opp = kzalloc(sizeof(*opp), GFP_KERNEL); - if (!opp) + table = _add_opp_table(dev); + if (!table) return NULL;
- INIT_LIST_HEAD(&opp->node); + /* Allocate space for at least one supply */ + count = table->regulator_count ? table->regulator_count : 1; + supply_size = sizeof(*opp->supplies) * count;
- *opp_table = _add_opp_table(dev); - if (!*opp_table) { - kfree(opp); + /* allocate new OPP node + and supplies structures */ + opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL); + if (!opp) { + kfree(table); return NULL; }
+ /* Put the supplies at the end of the OPP structure as an empty array */ + opp->supplies = (struct dev_pm_opp_supply *)(opp + 1); + INIT_LIST_HEAD(&opp->node); + + *opp_table = table; + return opp; }
static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, struct opp_table *opp_table) { - struct regulator *reg = opp_table->regulator; - - if (!IS_ERR(reg) && - !regulator_is_supported_voltage(reg, opp->supply.u_volt_min, - opp->supply.u_volt_max)) { - pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n", - __func__, opp->supply.u_volt_min, - opp->supply.u_volt_max); - return false; + struct regulator *reg; + int i; + + for (i = 0; i < opp_table->regulator_count; i++) { + reg = opp_table->regulators[i]; + + if (!regulator_is_supported_voltage(reg, + opp->supplies[i].u_volt_min, + opp->supplies[i].u_volt_max)) { + pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n", + __func__, opp->supplies[i].u_volt_min, + opp->supplies[i].u_volt_max); + return false; + } }
return true; @@ -984,12 +1035,13 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
/* Duplicate OPPs */ dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", - __func__, opp->rate, opp->supply.u_volt, - opp->available, new_opp->rate, new_opp->supply.u_volt, - new_opp->available); + __func__, opp->rate, opp->supplies[0].u_volt, + opp->available, new_opp->rate, + new_opp->supplies[0].u_volt, new_opp->available);
+ /* Should we compare voltages for all regulators here ? */ return opp->available && - new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST; + new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? 0 : -EEXIST; }
new_opp->opp_table = opp_table; @@ -1056,9 +1108,9 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt, /* populate the opp table */ new_opp->rate = freq; tol = u_volt * opp_table->voltage_tolerance_v1 / 100; - new_opp->supply.u_volt = u_volt; - new_opp->supply.u_volt_min = u_volt - tol; - new_opp->supply.u_volt_max = u_volt + tol; + new_opp->supplies[0].u_volt = u_volt; + new_opp->supplies[0].u_volt_min = u_volt - tol; + new_opp->supplies[0].u_volt_max = u_volt + tol; new_opp->available = true; new_opp->dynamic = dynamic;
@@ -1303,12 +1355,14 @@ void dev_pm_opp_put_prop_name(struct device *dev) EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
/** - * dev_pm_opp_set_regulator() - Set regulator name for the device + * dev_pm_opp_set_regulators() - Set regulator names for the device * @dev: Device for which regulator name is being set. - * @name: Name of the regulator. + * @names: Array of pointers to the names of the regulator. + * @count: Number of regulators. * * In order to support OPP switching, OPP layer needs to know the name of the - * device's regulator, as the core would be required to switch voltages as well. + * device's regulators, as the core would be required to switch voltages as + * well. * * This must be called before any OPPs are initialized for the device. * @@ -1318,11 +1372,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name); * that this function is *NOT* called under RCU protection or in contexts where * mutex cannot be locked. */ -int dev_pm_opp_set_regulator(struct device *dev, const char *name) +int dev_pm_opp_set_regulators(struct device *dev, const char * const names[], + unsigned int count) { struct opp_table *opp_table; struct regulator *reg; - int ret; + int ret, i;
mutex_lock(&opp_table_lock);
@@ -1338,26 +1393,42 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name) goto err; }
- /* Already have a regulator set */ - if (WARN_ON(!IS_ERR(opp_table->regulator))) { + /* Already have regulators set */ + if (WARN_ON(opp_table->regulators)) { ret = -EBUSY; goto err; } - /* Allocate the regulator */ - reg = regulator_get_optional(dev, name); - if (IS_ERR(reg)) { - ret = PTR_ERR(reg); - if (ret != -EPROBE_DEFER) - dev_err(dev, "%s: no regulator (%s) found: %d\n", - __func__, name, ret); + + opp_table->regulators = kmalloc_array(count, + sizeof(*opp_table->regulators), + GFP_KERNEL); + if (!opp_table->regulators) goto err; + + for (i = 0; i < count; i++) { + reg = regulator_get_optional(dev, names[i]); + if (IS_ERR(reg)) { + ret = PTR_ERR(reg); + if (ret != -EPROBE_DEFER) + dev_err(dev, "%s: regulator (%s) not found: %d\n", + __func__, names[i], ret); + goto free_regulators; + } + + opp_table->regulators[i] = reg; }
- opp_table->regulator = reg; + opp_table->regulator_count = count;
mutex_unlock(&opp_table_lock); return 0;
+free_regulators: + while (i != 0) + regulator_put(opp_table->regulators[--i]); + + kfree(opp_table->regulators); + opp_table->regulators = NULL; err: _remove_opp_table(opp_table); unlock: @@ -1365,11 +1436,11 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
return ret; } -EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator); +EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulators);
/** - * dev_pm_opp_put_regulator() - Releases resources blocked for regulator - * @dev: Device for which regulator was set. + * dev_pm_opp_put_regulators() - Releases resources blocked for regulators + * @dev: Device for which regulators were set. * * Locking: The internal opp_table and opp structures are RCU protected. * Hence this function internally uses RCU updater strategy with mutex locks @@ -1377,9 +1448,10 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator); * that this function is *NOT* called under RCU protection or in contexts where * mutex cannot be locked. */ -void dev_pm_opp_put_regulator(struct device *dev) +void dev_pm_opp_put_regulators(struct device *dev) { struct opp_table *opp_table; + int i;
mutex_lock(&opp_table_lock);
@@ -1391,16 +1463,20 @@ void dev_pm_opp_put_regulator(struct device *dev) goto unlock; }
- if (IS_ERR(opp_table->regulator)) { - dev_err(dev, "%s: Doesn't have regulator set\n", __func__); + if (!opp_table->regulators) { + dev_err(dev, "%s: Doesn't have regulators set\n", __func__); goto unlock; }
/* Make sure there are no concurrent readers while updating opp_table */ WARN_ON(!list_empty(&opp_table->opp_list));
- regulator_put(opp_table->regulator); - opp_table->regulator = ERR_PTR(-ENXIO); + for (i = opp_table->regulator_count - 1; i >= 0; i--) + regulator_put(opp_table->regulators[i]); + + kfree(opp_table->regulators); + opp_table->regulators = NULL; + opp_table->regulator_count = 0;
/* Try freeing opp_table if this was the last blocking resource */ _remove_opp_table(opp_table); @@ -1408,7 +1484,7 @@ void dev_pm_opp_put_regulator(struct device *dev) unlock: mutex_unlock(&opp_table_lock); } -EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulator); +EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
/** * dev_pm_opp_add() - Add an OPP table from a table definitions diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c index c897676ca35f..95f433db4ac7 100644 --- a/drivers/base/power/opp/debugfs.c +++ b/drivers/base/power/opp/debugfs.c @@ -15,6 +15,7 @@ #include <linux/err.h> #include <linux/init.h> #include <linux/limits.h> +#include <linux/slab.h>
#include "opp.h"
@@ -34,6 +35,46 @@ void opp_debug_remove_one(struct dev_pm_opp *opp) debugfs_remove_recursive(opp->dentry); }
+static bool opp_debug_create_supplies(struct dev_pm_opp *opp, + struct opp_table *opp_table, + struct dentry *pdentry) +{ + struct dentry *d; + int i = 0; + char *name; + + /* Always create at least supply-0 directory */ + do { + name = kasprintf(GFP_KERNEL, "supply-%d", i); + + /* Create per-opp directory */ + d = debugfs_create_dir(name, pdentry); + + kfree(name); + + if (!d) + return false; + + if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, + &opp->supplies[i].u_volt)) + return false; + + if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, + &opp->supplies[i].u_volt_min)) + return false; + + if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, + &opp->supplies[i].u_volt_max)) + return false; + + if (!debugfs_create_ulong("u_amp", S_IRUGO, d, + &opp->supplies[i].u_amp)) + return false; + } while (++i < opp_table->regulator_count); + + return true; +} + int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) { struct dentry *pdentry = opp_table->dentry; @@ -63,16 +104,7 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate)) return -ENOMEM;
- if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt)) - return -ENOMEM; - - if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min)) - return -ENOMEM; - - if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max)) - return -ENOMEM; - - if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp)) + if (!opp_debug_create_supplies(opp, opp_table, d)) return -ENOMEM;
if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d, diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c index b7fcd0a1b58b..477bedab28cf 100644 --- a/drivers/base/power/opp/of.c +++ b/drivers/base/power/opp/of.c @@ -17,6 +17,7 @@ #include <linux/errno.h> #include <linux/device.h> #include <linux/of.h> +#include <linux/slab.h> #include <linux/export.h>
#include "opp.h" @@ -101,16 +102,16 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table, return true; }
-/* TODO: Support multiple regulators */ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, struct opp_table *opp_table) { - u32 microvolt[3] = {0}; - u32 val; - int count, ret; + u32 *microvolt, *microamp = NULL; + int supplies, vcount, icount, ret, i, j; struct property *prop = NULL; char name[NAME_MAX];
+ supplies = opp_table->regulator_count ? opp_table->regulator_count : 1; + /* Search for "opp-microvolt-<name>" */ if (opp_table->prop_name) { snprintf(name, sizeof(name), "opp-microvolt-%s", @@ -128,34 +129,29 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, return 0; }
- count = of_property_count_u32_elems(opp->np, name); - if (count < 0) { + vcount = of_property_count_u32_elems(opp->np, name); + if (vcount < 0) { dev_err(dev, "%s: Invalid %s property (%d)\n", - __func__, name, count); - return count; + __func__, name, vcount); + return vcount; }
- /* There can be one or three elements here */ - if (count != 1 && count != 3) { - dev_err(dev, "%s: Invalid number of elements in %s property (%d)\n", - __func__, name, count); + /* There can be one or three elements per supply */ + if (vcount != supplies && vcount != supplies * 3) { + dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n", + __func__, name, vcount, supplies); return -EINVAL; }
- ret = of_property_read_u32_array(opp->np, name, microvolt, count); + microvolt = kmalloc_array(vcount, sizeof(*microvolt), GFP_KERNEL); + if (!microvolt) + return -ENOMEM; + + ret = of_property_read_u32_array(opp->np, name, microvolt, vcount); if (ret) { dev_err(dev, "%s: error parsing %s: %d\n", __func__, name, ret); - return -EINVAL; - } - - opp->supply.u_volt = microvolt[0]; - - if (count == 1) { - opp->supply.u_volt_min = opp->supply.u_volt; - opp->supply.u_volt_max = opp->supply.u_volt; - } else { - opp->supply.u_volt_min = microvolt[1]; - opp->supply.u_volt_max = microvolt[2]; + ret = -EINVAL; + goto free_microvolt; }
/* Search for "opp-microamp-<name>" */ @@ -172,10 +168,59 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, prop = of_find_property(opp->np, name, NULL); }
- if (prop && !of_property_read_u32(opp->np, name, &val)) - opp->supply.u_amp = val; + if (prop) { + icount = of_property_count_u32_elems(opp->np, name); + if (icount < 0) { + dev_err(dev, "%s: Invalid %s property (%d)\n", __func__, + name, icount); + ret = icount; + goto free_microvolt; + }
- return 0; + if (icount != supplies) { + dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n", + __func__, name, icount, supplies); + ret = -EINVAL; + goto free_microvolt; + } + + microamp = kmalloc_array(icount, sizeof(*microamp), GFP_KERNEL); + if (!microamp) { + ret = -EINVAL; + goto free_microvolt; + } + + ret = of_property_read_u32_array(opp->np, name, microamp, + icount); + if (ret) { + dev_err(dev, "%s: error parsing %s: %d\n", __func__, + name, ret); + ret = -EINVAL; + goto free_microamp; + } + } + + for (i = 0, j = 0; i < supplies; i++) { + opp->supplies[i].u_volt = microvolt[j++]; + + if (vcount == supplies) { + opp->supplies[i].u_volt_min = opp->supplies[i].u_volt; + opp->supplies[i].u_volt_max = opp->supplies[i].u_volt; + } else { + opp->supplies[i].u_volt_min = microvolt[j++]; + opp->supplies[i].u_volt_max = microvolt[j++]; + } + + if (microamp) + opp->supplies[i].u_amp = microamp[i]; + } + +free_microamp: + kfree(microamp); +free_microvolt: + kfree(microvolt); + + return ret; }
/** @@ -304,8 +349,8 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n", __func__, new_opp->turbo, new_opp->rate, - new_opp->supply.u_volt, new_opp->supply.u_volt_min, - new_opp->supply.u_volt_max, new_opp->clock_latency_ns); + new_opp->supplies[0].u_volt, new_opp->supplies[0].u_volt_min, + new_opp->supplies[0].u_volt_max, new_opp->clock_latency_ns);
/* * Notify the changes in the availability of the operable diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index 791213175683..d5f09ee90347 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -61,7 +61,7 @@ extern struct list_head opp_tables; * @turbo: true if turbo (boost) OPP * @suspend: true if suspend OPP * @rate: Frequency in hertz - * @supply: Power supply voltage/current values + * @supplies: Power supplies voltage/current values * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's * frequency from any other OPP's frequency. * @opp_table: points back to the opp_table struct this opp belongs to @@ -80,7 +80,7 @@ struct dev_pm_opp { bool suspend; unsigned long rate;
- struct dev_pm_opp_supply supply; + struct dev_pm_opp_supply *supplies;
unsigned long clock_latency_ns;
@@ -139,7 +139,8 @@ enum opp_table_access { * @supported_hw_count: Number of elements in supported_hw array. * @prop_name: A name to postfix to many DT properties, while parsing them. * @clk: Device's clock handle - * @regulator: Supply regulator + * @regulators: Supply regulators + * @regulator_count: Number of power supply regulators * @dentry: debugfs dentry pointer of the real device directory (not links). * @dentry_name: Name of the real dentry. * @@ -174,7 +175,8 @@ struct opp_table { unsigned int supported_hw_count; const char *prop_name; struct clk *clk; - struct regulator *regulator; + struct regulator **regulators; + unsigned int regulator_count;
#ifdef CONFIG_DEBUG_FS struct dentry *dentry; diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 5c07ae05d69a..15cb26118dc7 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -186,7 +186,10 @@ static int cpufreq_init(struct cpufreq_policy *policy) */ name = find_supply_name(cpu_dev); if (name) { - ret = dev_pm_opp_set_regulator(cpu_dev, name); + const char *names[] = {name}; + + ret = dev_pm_opp_set_regulators(cpu_dev, names, + ARRAY_SIZE(names)); if (ret) { dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n", policy->cpu, ret); @@ -285,7 +288,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) out_free_opp: dev_pm_opp_of_cpumask_remove_table(policy->cpus); if (name) - dev_pm_opp_put_regulator(cpu_dev); + dev_pm_opp_put_regulators(cpu_dev); out_put_clk: clk_put(cpu_clk);
@@ -300,7 +303,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy) dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); if (priv->reg_name) - dev_pm_opp_put_regulator(priv->cpu_dev); + dev_pm_opp_put_regulators(priv->cpu_dev);
clk_put(policy->clk); kfree(priv); diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index f69126e2bb59..27eea9bfc5ed 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -78,8 +78,8 @@ int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, void dev_pm_opp_put_supported_hw(struct device *dev); int dev_pm_opp_set_prop_name(struct device *dev, const char *name); void dev_pm_opp_put_prop_name(struct device *dev); -int dev_pm_opp_set_regulator(struct device *dev, const char *name); -void dev_pm_opp_put_regulator(struct device *dev); +int dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count); +void dev_pm_opp_put_regulators(struct device *dev); int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq); int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask); int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask); @@ -186,12 +186,12 @@ static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
static inline void dev_pm_opp_put_prop_name(struct device *dev) {}
-static inline int dev_pm_opp_set_regulator(struct device *dev, const char *name) +static inline int dev_pm_opp_set_regulators(struct device *dev, const char *names[], unsigned int count) { return -ENOTSUPP; }
-static inline void dev_pm_opp_put_regulator(struct device *dev) {} +static inline void dev_pm_opp_put_regulators(struct device *dev) {}
static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) {
Later patches would add support for custom opp_set_rate callbacks. This patch separates out the code for generic opp_set_rate handler in order to prepare for that.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com --- drivers/base/power/opp/core.c | 180 +++++++++++++++++++++++++++++------------- drivers/base/power/opp/opp.h | 2 + include/linux/pm_opp.h | 33 ++++++++ 3 files changed, 162 insertions(+), 53 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 5a35fdd4b61b..dedb08a66e99 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -596,6 +596,69 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg, return ret; }
+static inline int +_generic_set_opp_clk_only(struct device *dev, struct clk *clk, + unsigned long old_freq, unsigned long freq) +{ + int ret; + + ret = clk_set_rate(clk, freq); + if (ret) { + dev_err(dev, "%s: failed to set clock rate: %d\n", __func__, + ret); + } + + return ret; +} + +static int _generic_set_opp(struct device *dev, + struct dev_pm_set_opp_data *data) +{ + struct dev_pm_opp_supply *old_supply = data->old_opp.supplies; + struct dev_pm_opp_supply *new_supply = data->new_opp.supplies; + unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate; + struct regulator *reg = data->regulators[0]; + int ret; + + /* This function only supports single regulator per device */ + if (WARN_ON(data->regulator_count > 1)) { + dev_err(dev, "multiple regulators are not supported\n"); + return -EINVAL; + } + + /* Scaling up? Scale voltage before frequency */ + if (freq > old_freq) { + ret = _set_opp_voltage(dev, reg, new_supply); + if (ret) + goto restore_voltage; + } + + /* Change frequency */ + ret = _generic_set_opp_clk_only(dev, data->clk, old_freq, freq); + if (ret) + goto restore_voltage; + + /* Scaling down? Scale voltage after frequency */ + if (freq < old_freq) { + ret = _set_opp_voltage(dev, reg, new_supply); + if (ret) + goto restore_freq; + } + + return 0; + +restore_freq: + if (_generic_set_opp_clk_only(dev, data->clk, freq, old_freq)) + dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n", + __func__, old_freq); +restore_voltage: + /* This shouldn't harm even if the voltages weren't updated earlier */ + if (old_supply->u_volt) + _set_opp_voltage(dev, reg, old_supply); + + return ret; +} + /** * dev_pm_opp_set_rate() - Configure new OPP based on frequency * @dev: device for which we do this operation @@ -609,12 +672,12 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg, int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) { struct opp_table *opp_table; + unsigned long freq, old_freq; struct dev_pm_opp *old_opp, *opp; - struct regulator *reg = ERR_PTR(-ENXIO); + struct regulator **regulators; + struct dev_pm_set_opp_data *data; struct clk *clk; - unsigned long freq, old_freq; - struct dev_pm_opp_supply old_supply, new_supply; - int ret; + int ret, size;
if (unlikely(!target_freq)) { dev_err(dev, "%s: Invalid target frequency %lu\n", __func__, @@ -663,64 +726,35 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) return ret; }
- if (opp_table->regulators) { - /* This function only supports single regulator per device */ - if (WARN_ON(opp_table->regulator_count > 1)) { - dev_err(dev, "multiple regulators not supported\n"); - rcu_read_unlock(); - return -EINVAL; - } + dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__, + old_freq, freq);
- reg = opp_table->regulators[0]; + regulators = opp_table->regulators; + + /* Only frequency scaling */ + if (!regulators) { + rcu_read_unlock(); + return _generic_set_opp_clk_only(dev, clk, old_freq, freq); }
+ data = opp_table->set_opp_data; + data->regulators = regulators; + data->regulator_count = opp_table->regulator_count; + data->clk = clk; + + data->old_opp.rate = old_freq; + size = sizeof(*opp->supplies) * opp_table->regulator_count; if (IS_ERR(old_opp)) - old_supply.u_volt = 0; + memset(data->old_opp.supplies, 0, size); else - memcpy(&old_supply, old_opp->supplies, sizeof(old_supply)); + memcpy(data->old_opp.supplies, old_opp->supplies, size);
- memcpy(&new_supply, opp->supplies, sizeof(new_supply)); + data->new_opp.rate = freq; + memcpy(data->new_opp.supplies, opp->supplies, size);
rcu_read_unlock();
- /* Scaling up? Scale voltage before frequency */ - if (freq > old_freq) { - ret = _set_opp_voltage(dev, reg, &new_supply); - if (ret) - goto restore_voltage; - } - - /* Change frequency */ - - dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", - __func__, old_freq, freq); - - ret = clk_set_rate(clk, freq); - if (ret) { - dev_err(dev, "%s: failed to set clock rate: %d\n", __func__, - ret); - goto restore_voltage; - } - - /* Scaling down? Scale voltage after frequency */ - if (freq < old_freq) { - ret = _set_opp_voltage(dev, reg, &new_supply); - if (ret) - goto restore_freq; - } - - return 0; - -restore_freq: - if (clk_set_rate(clk, old_freq)) - dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n", - __func__, old_freq); -restore_voltage: - /* This shouldn't harm even if the voltages weren't updated earlier */ - if (old_supply.u_volt) - _set_opp_voltage(dev, reg, &old_supply); - - return ret; + return _generic_set_opp(dev, data); } EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
@@ -1354,6 +1388,38 @@ void dev_pm_opp_put_prop_name(struct device *dev) } EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
+static int _allocate_set_opp_data(struct opp_table *opp_table) +{ + struct dev_pm_set_opp_data *data; + int len, count = opp_table->regulator_count; + + if (WARN_ON(!count)) + return -EINVAL; + + /* space for set_opp_data */ + len = sizeof(*data); + + /* space for old_opp.supplies and new_opp.supplies */ + len += 2 * sizeof(struct dev_pm_opp_supply) * count; + + data = kzalloc(len, GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->old_opp.supplies = (void *)(data + 1); + data->new_opp.supplies = data->old_opp.supplies + count; + + opp_table->set_opp_data = data; + + return 0; +} + +static void _free_set_opp_data(struct opp_table *opp_table) +{ + kfree(opp_table->set_opp_data); + opp_table->set_opp_data = NULL; +} + /** * dev_pm_opp_set_regulators() - Set regulator names for the device * @dev: Device for which regulator name is being set. @@ -1420,6 +1486,11 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
opp_table->regulator_count = count;
+ /* Allocate block only once to pass to ->set_rate() */ + ret = _allocate_set_opp_data(opp_table); + if (ret) + goto free_regulators; + mutex_unlock(&opp_table_lock); return 0;
@@ -1429,6 +1500,7 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
kfree(opp_table->regulators); opp_table->regulators = NULL; + opp_table->regulator_count = 0; err: _remove_opp_table(opp_table); unlock: @@ -1474,6 +1546,8 @@ void dev_pm_opp_put_regulators(struct device *dev) for (i = opp_table->regulator_count - 1; i >= 0; i--) regulator_put(opp_table->regulators[i]);
+ _free_set_opp_data(opp_table); + kfree(opp_table->regulators); opp_table->regulators = NULL; opp_table->regulator_count = 0; diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index d5f09ee90347..26bc6c1b8c60 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -178,6 +178,8 @@ struct opp_table { struct regulator **regulators; unsigned int regulator_count;
+ struct dev_pm_set_opp_data *set_opp_data; + #ifdef CONFIG_DEBUG_FS struct dentry *dentry; char dentry_name[NAME_MAX]; diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 27eea9bfc5ed..2969519bf5f7 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -17,6 +17,8 @@ #include <linux/err.h> #include <linux/notifier.h>
+struct clk; +struct regulator; struct dev_pm_opp; struct device;
@@ -40,6 +42,37 @@ struct dev_pm_opp_supply { unsigned long u_amp; };
+/** + * struct dev_pm_opp_info - OPP freq/voltage/current values + * @rate: Target clk rate in hz + * @supplies: Array of voltage/current values for all power supplies + * + * This structure stores the freq/voltage/current values for a single OPP. + */ +struct dev_pm_opp_info { + unsigned long rate; + struct dev_pm_opp_supply *supplies; +}; + +/** + * struct dev_pm_set_opp_data - Set OPP data + * @old_opp: Old OPP info + * @new_opp: New OPP info + * @regulators: Array of regulator pointers + * @regulator_count: Number of regulators + * @clk: Pointer to clk + * + * This structure contains all information required for setting an OPP. + */ +struct dev_pm_set_opp_data { + struct dev_pm_opp_info old_opp; + struct dev_pm_opp_info new_opp; + + struct regulator **regulators; + unsigned int regulator_count; + struct clk *clk; +}; + #if defined(CONFIG_PM_OPP)
unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
The generic set_opp() handler isn't sufficient for platforms with complex DVFS. For example, some TI platforms have multiple regulators for a CPU device. The order in which various supplies need to be programmed is only known to the platform code and its best to leave it to it.
This patch implements APIs to register platform specific set_opp() callback.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com --- drivers/base/power/opp/core.c | 116 +++++++++++++++++++++++++++++++++++++++++- drivers/base/power/opp/opp.h | 1 + include/linux/pm_opp.h | 10 ++++ 3 files changed, 125 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index dedb08a66e99..f4f6b1fdbe06 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -673,6 +673,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) { struct opp_table *opp_table; unsigned long freq, old_freq; + int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data); struct dev_pm_opp *old_opp, *opp; struct regulator **regulators; struct dev_pm_set_opp_data *data; @@ -737,6 +738,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) return _generic_set_opp_clk_only(dev, clk, old_freq, freq); }
+ if (opp_table->set_opp) + set_opp = opp_table->set_opp; + else + set_opp = _generic_set_opp; + data = opp_table->set_opp_data; data->regulators = regulators; data->regulator_count = opp_table->regulator_count; @@ -754,7 +760,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
rcu_read_unlock();
- return _generic_set_opp(dev, data); + return set_opp(dev, data); } EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
@@ -888,6 +894,9 @@ static void _remove_opp_table(struct opp_table *opp_table) if (opp_table->regulators) return;
+ if (opp_table->set_opp) + return; + /* Release clk */ if (!IS_ERR(opp_table->clk)) clk_put(opp_table->clk); @@ -1486,7 +1495,7 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
opp_table->regulator_count = count;
- /* Allocate block only once to pass to ->set_rate() */ + /* Allocate block only once to pass to ->set_opp() */ ret = _allocate_set_opp_data(opp_table); if (ret) goto free_regulators; @@ -1561,6 +1570,109 @@ void dev_pm_opp_put_regulators(struct device *dev) EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
/** + * dev_pm_opp_register_set_opp_helper() - Register custom OPP set rate helper + * @dev: Device for which the helper is getting registered. + * @set_opp: Custom set OPP helper. + * + * This is useful to support complex platforms (like platforms with multiple + * regulators per device), instead of the generic OPP set rate helper. + * + * This must be called before any OPPs are initialized for the device. + * + * Locking: The internal opp_table and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + */ +int dev_pm_opp_register_set_opp_helper(struct device *dev, + int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data)) +{ + struct opp_table *opp_table; + int ret; + + if (!set_opp) + return -EINVAL; + + mutex_lock(&opp_table_lock); + + opp_table = _add_opp_table(dev); + if (!opp_table) { + ret = -ENOMEM; + goto unlock; + } + + /* This should be called before OPPs are initialized */ + if (WARN_ON(!list_empty(&opp_table->opp_list))) { + ret = -EBUSY; + goto err; + } + + /* Already have custom set_opp helper */ + if (WARN_ON(opp_table->set_opp)) { + ret = -EBUSY; + goto err; + } + + opp_table->set_opp = set_opp; + + mutex_unlock(&opp_table_lock); + return 0; + +err: + _remove_opp_table(opp_table); +unlock: + mutex_unlock(&opp_table_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(dev_pm_opp_register_set_opp_helper); + +/** + * dev_pm_opp_register_put_opp_helper() - Releases resources blocked for + * set_opp helper + * @dev: Device for which custom set_opp helper has to be cleared. + * + * Locking: The internal opp_table and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + */ +void dev_pm_opp_register_put_opp_helper(struct device *dev) +{ + struct opp_table *opp_table; + + mutex_lock(&opp_table_lock); + + /* Check for existing table for 'dev' first */ + opp_table = _find_opp_table(dev); + if (IS_ERR(opp_table)) { + dev_err(dev, "Failed to find opp_table: %ld\n", + PTR_ERR(opp_table)); + goto unlock; + } + + if (!opp_table->set_opp) { + dev_err(dev, "%s: Doesn't have custom set_opp helper set\n", + __func__); + goto unlock; + } + + /* Make sure there are no concurrent readers while updating opp_table */ + WARN_ON(!list_empty(&opp_table->opp_list)); + + opp_table->set_opp = NULL; + + /* Try freeing opp_table if this was the last blocking resource */ + _remove_opp_table(opp_table); + +unlock: + mutex_unlock(&opp_table_lock); +} +EXPORT_SYMBOL_GPL(dev_pm_opp_register_put_opp_helper); + +/** * dev_pm_opp_add() - Add an OPP table from a table definitions * @dev: device for which we do this operation * @freq: Frequency in Hz for this OPP diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index 26bc6c1b8c60..62a6020b2c3d 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -178,6 +178,7 @@ struct opp_table { struct regulator **regulators; unsigned int regulator_count;
+ int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data); struct dev_pm_set_opp_data *set_opp_data;
#ifdef CONFIG_DEBUG_FS diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 2969519bf5f7..cb5bc4747502 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -113,6 +113,8 @@ int dev_pm_opp_set_prop_name(struct device *dev, const char *name); void dev_pm_opp_put_prop_name(struct device *dev); int dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count); void dev_pm_opp_put_regulators(struct device *dev); +int dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data)); +void dev_pm_opp_register_put_opp_helper(struct device *dev); int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq); int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask); int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask); @@ -212,6 +214,14 @@ static inline int dev_pm_opp_set_supported_hw(struct device *dev,
static inline void dev_pm_opp_put_supported_hw(struct device *dev) {}
+static inline int dev_pm_opp_register_set_opp_helper(struct device *dev, + int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data)) +{ + return -ENOTSUPP; +} + +static inline void dev_pm_opp_register_put_opp_helper(struct device *dev) {} + static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name) { return -ENOTSUPP;
If a platform specific OPP driver has called this routine first and set the regulators, then the second call from cpufreq-dt driver will hit the WARN_ON(). Remove the WARN_ON(), but continue to return error in such cases.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org Tested-by: Dave Gerlach d-gerlach@ti.com --- drivers/base/power/opp/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index f4f6b1fdbe06..3298fac01bb0 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1469,7 +1469,7 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[], }
/* Already have regulators set */ - if (WARN_ON(opp_table->regulators)) { + if (opp_table->regulators) { ret = -EBUSY; goto err; }
If the regulators aren't set explicitly by the platform, the OPP core assumes that the platform doesn't have any regulator and uses the clk-only callback.
If the platform failed to register a regulator with the core, then this can turn out to be a dangerous assumption as the OPP core will try to change clk without changing regulators.
Handle that properly by making sure that the DT didn't had any entries for supply voltages as well.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp/core.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 3298fac01bb0..34cd48dfe89e 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -734,7 +734,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
/* Only frequency scaling */ if (!regulators) { - rcu_read_unlock(); + /* + * DT contained supply ratings? Consider platform failed to set + * regulators. + */ + if (unlikely(opp->supplies[0].u_volt)) { + rcu_read_unlock(); + dev_err(dev, "%s: Regulator not registered with OPP core\n", + __func__); + return -EINVAL; + } + return _generic_set_opp_clk_only(dev, clk, old_freq, freq); }
On 10/26, Viresh Kumar wrote:
If the regulators aren't set explicitly by the platform, the OPP core assumes that the platform doesn't have any regulator and uses the clk-only callback.
If the platform failed to register a regulator with the core, then this can turn out to be a dangerous assumption as the OPP core will try to change clk without changing regulators.
Handle that properly by making sure that the DT didn't had any entries
s/had/have/
for supply voltages as well.
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 3298fac01bb0..34cd48dfe89e 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -734,7 +734,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) /* Only frequency scaling */ if (!regulators) {
rcu_read_unlock();
/** DT contained supply ratings? Consider platform failed to set* regulators.*/if (unlikely(opp->supplies[0].u_volt)) {rcu_read_unlock();dev_err(dev, "%s: Regulator not registered with OPP core\n",__func__);return -EINVAL;}
Don't we need an rcu_read_unlock() here as well?
return _generic_set_opp_clk_only(dev, clk, old_freq, freq);}
If the regulators aren't set explicitly by the platform, the OPP core assumes that the platform doesn't have any regulator and uses the clk-only callback.
If the platform failed to register a regulator with the core, then this can turn out to be a dangerous assumption as the OPP core will try to change clk without changing regulators.
Handle that properly by making sure that the DT didn't have any entries for supply voltages as well.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- V4: - s/had/have - fix missing rcu_read_unlock()
drivers/base/power/opp/core.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 3298fac01bb0..cb33f8b2b56d 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -734,7 +734,20 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
/* Only frequency scaling */ if (!regulators) { + unsigned long u_volt = opp->supplies[0].u_volt; + rcu_read_unlock(); + + /* + * DT contained supply ratings? Consider platform failed to set + * regulators. + */ + if (unlikely(u_volt)) { + dev_err(dev, "%s: Regulator not registered with OPP core\n", + __func__); + return -EINVAL; + } + return _generic_set_opp_clk_only(dev, clk, old_freq, freq); }
On 26-10-16, 12:02, Viresh Kumar wrote:
Hi,
Some platforms (like TI) have complex DVFS configuration for CPU devices, where multiple regulators are required to be configured to change DVFS state of the device. This was explained well by Nishanth earlier [1].
One of the major complaints around multiple regulators case was that the DT isn't responsible in any way to represent the ordering in which multiple supplies need to be programmed, before or after frequency change. It was considered in this patch and such information is left to the platform specific OPP driver now, which can register its own opp_set_rate() callback with the OPP core and the OPP core will then call it during DVFS.
The patches are tested on Exynos5250 (Dual A15). I have hacked around DT and code to pass values for multiple regulators and verified that they are all properly read by the kernel (using debugfs interface).
Dave Gerlach has already tested it on the real TI platforms and it works well for him.
This is rebased over: linux-next branch in the PM tree.
V2->V3:
- The last patch is new
- Removed a debug leftover pr_info() message
- Renamed few names as s/set_rate/set_opp
- Removed a TODO comment (as it is done now with this series)
- created struct for min_uV and max_uV
- kerneldoc comments for structures in pm_opp.h
- s/const char */const char * const
- use kasprintf()
- Some more minor reformatting
- More Ack/RBY tags added
@Stephen: Can you please provide further comments or Acks ?
On 11/02, Viresh Kumar wrote:
On 26-10-16, 12:02, Viresh Kumar wrote:
Hi,
Some platforms (like TI) have complex DVFS configuration for CPU devices, where multiple regulators are required to be configured to change DVFS state of the device. This was explained well by Nishanth earlier [1].
One of the major complaints around multiple regulators case was that the DT isn't responsible in any way to represent the ordering in which multiple supplies need to be programmed, before or after frequency change. It was considered in this patch and such information is left to the platform specific OPP driver now, which can register its own opp_set_rate() callback with the OPP core and the OPP core will then call it during DVFS.
The patches are tested on Exynos5250 (Dual A15). I have hacked around DT and code to pass values for multiple regulators and verified that they are all properly read by the kernel (using debugfs interface).
Dave Gerlach has already tested it on the real TI platforms and it works well for him.
This is rebased over: linux-next branch in the PM tree.
V2->V3:
- The last patch is new
- Removed a debug leftover pr_info() message
- Renamed few names as s/set_rate/set_opp
- Removed a TODO comment (as it is done now with this series)
- created struct for min_uV and max_uV
- kerneldoc comments for structures in pm_opp.h
- s/const char */const char * const
- use kasprintf()
- Some more minor reformatting
- More Ack/RBY tags added
@Stephen: Can you please provide further comments or Acks ?
Where did we end up on the topic of RCU usage in OPP? I'd rather we figure that out before investing more time in reviewing code that we may end up completely rewriting in the future.
On 09-11-16, 17:19, Stephen Boyd wrote:
On 11/02, Viresh Kumar wrote:
On 26-10-16, 12:02, Viresh Kumar wrote:
Hi,
Some platforms (like TI) have complex DVFS configuration for CPU devices, where multiple regulators are required to be configured to change DVFS state of the device. This was explained well by Nishanth earlier [1].
One of the major complaints around multiple regulators case was that the DT isn't responsible in any way to represent the ordering in which multiple supplies need to be programmed, before or after frequency change. It was considered in this patch and such information is left to the platform specific OPP driver now, which can register its own opp_set_rate() callback with the OPP core and the OPP core will then call it during DVFS.
The patches are tested on Exynos5250 (Dual A15). I have hacked around DT and code to pass values for multiple regulators and verified that they are all properly read by the kernel (using debugfs interface).
Dave Gerlach has already tested it on the real TI platforms and it works well for him.
This is rebased over: linux-next branch in the PM tree.
V2->V3:
- The last patch is new
- Removed a debug leftover pr_info() message
- Renamed few names as s/set_rate/set_opp
- Removed a TODO comment (as it is done now with this series)
- created struct for min_uV and max_uV
- kerneldoc comments for structures in pm_opp.h
- s/const char */const char * const
- use kasprintf()
- Some more minor reformatting
- More Ack/RBY tags added
@Stephen: Can you please provide further comments or Acks ?
Where did we end up on the topic of RCU usage in OPP? I'd rather we figure that out before investing more time in reviewing code that we may end up completely rewriting in the future.
We haven't decided anything on that yet and I don't think there is any reason to block this series for that. There is lots of existing code that needs to be updated if we decide to walk that path and I wouldn't mind a small addition to that from this series.
Lots of effort Coding/testing/reviewing has already gone into this work and we better get it merged now.
NOT FOR MERGE!
Introduce a test version of a 'ti-opp-domain' driver that will use new multiple regulator support introduced to the OPP core by Viresh [1]. Tested on v4.9-rc1 with that series applied. This is needed on TI platforms like DRA7/AM57 in order to control both CPU regulator and Adaptive Body Bias (ABB) regulator as described by Nishanth Menon here [2]. These regulators must be scaled in sequence during an OPP transition depending on whether or not the frequency is being scaled up or down. Based on the new functionality provided by Viresh this driver does the following:
* Call dev_pm_opp_set_regulators with the names of the two regulators that feed the CPU: * vdd is the 'cpu-supply' commonly used for cpufreq-dt but renamed so the cpufreq-dt driver doesn't use it directly. Note that this is supplied in board dts as it's external to SoC. * vbb for the ABB regulator. This is provided in SoC dtsi as it is internal to the SoC. * Provide a platform set_opp function using dev_pm_opp_register_set_opp_helper that is called when an OPP transition is requested. * Allow cpufreq-dt to probe which will work because no cpu-supply regulator is found so the driver proceeds and calls dev_pm_opp_set_rate which through the OPP core invokes the platform set_opp call we provided * Platform set_opp call provided by this driver checks to see if we are scaling frequency up or down and based on this, scales vbb before vdd for up or the other way around for down.
In addition to that, this driver implements AVS Class 0 as described in section 18.4.6.12 of AM572x TRM [3] using the same platform set_rate hook added to the OPP core. There are registers that define the optimal voltage for that specific piece of silicon for an OPP so this driver simply looks up this optimal value and programs that for an OPP instead of the nominal value.
Missing from this is a good way to ensure that cpufreq-dt does not just proceed if no cpu-supply regulator is found but we were intending to rely on a platform set_opp and multiple regulators.
[1] https://marc.info/?l=linux-pm&m=147746362402994&w=2 [2] https://marc.info/?l=linux-pm&m=145684495832764&w=2 [3] http://www.ti.com/lit/ug/spruhz6g/spruhz6g.pdf
Signed-off-by: Dave Gerlach d-gerlach@ti.com --- arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi | 2 +- arch/arm/boot/dts/dra7.dtsi | 46 ++- drivers/soc/ti/Makefile | 2 + drivers/soc/ti/ti-opp-domain.c | 427 ++++++++++++++++++++++++ 4 files changed, 471 insertions(+), 6 deletions(-) create mode 100644 drivers/soc/ti/ti-opp-domain.c
diff --git a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi index 6df7829a2c15..d92551c15780 100644 --- a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi +++ b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi @@ -410,7 +410,7 @@ };
&cpu0 { - cpu0-supply = <&smps12_reg>; + vdd-supply = <&smps12_reg>; voltage-tolerance = <1>; };
diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi index d4fcd68f6349..311166b9e8c4 100644 --- a/arch/arm/boot/dts/dra7.dtsi +++ b/arch/arm/boot/dts/dra7.dtsi @@ -80,11 +80,7 @@ compatible = "arm,cortex-a15"; reg = <0>;
- operating-points = < - /* kHz uV */ - 1000000 1060000 - 1176000 1160000 - >; + operating-points-v2 = <&cpu0_opp_table>;
clocks = <&dpll_mpu_ck>; clock-names = "cpu"; @@ -95,6 +91,32 @@ cooling-min-level = <0>; cooling-max-level = <2>; #cooling-cells = <2>; /* min followed by max */ + + vbb-supply = <&abb_mpu>; + }; + }; + + cpu0_opp_table: opp_table0 { + compatible = "operating-points-v2"; + opp-shared; + + opp_nom@1000000000 { + opp-hz = /bits/ 64 <1000000000>; + opp-microvolt = <1060000 850000 1150000>, + <1060000 850000 1150000>; + opp-suspend; + }; + + opp_od@1176000000 { + opp-hz = /bits/ 64 <1176000000>; + opp-microvolt = <1160000 885000 1160000>, + <1160000 885000 1160000>; + }; + + opp_high@1500000000 { + opp-hz = /bits/ 64 <1500000000>; + opp-microvolt = <1210000 950000 1250000>, + < 1210000 950000 1250000>; }; };
@@ -1966,6 +1988,20 @@ clocks = <&l3_iclk_div>; clock-names = "fck"; }; + + oppdm_mpu: oppdm@4a003b20 { + compatible = "ti,omap5-oppdm"; + #oppdm-cells = <0>; + reg = <0x4a003b20 0xc>; + ti,efuse-settings = < + /* uV offset */ + 1060000 0x0 + 1160000 0x4 + 1210000 0x8 + >; + ti,absolute-max-voltage-uv = <1500000>; + }; + };
thermal_zones: thermal-zones { diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile index 48ff3a79634f..e3595e3f1d6a 100644 --- a/drivers/soc/ti/Makefile +++ b/drivers/soc/ti/Makefile @@ -5,3 +5,5 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS) += knav_qmss.o knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA) += knav_dma.o obj-$(CONFIG_WKUP_M3_IPC) += wkup_m3_ipc.o +obj-y += ti-opp-domain.o +#obj-$(CONFIG_OPP_DOMAIN_TI) += ti-opp-domain.o diff --git a/drivers/soc/ti/ti-opp-domain.c b/drivers/soc/ti/ti-opp-domain.c new file mode 100644 index 000000000000..33683548b326 --- /dev/null +++ b/drivers/soc/ti/ti-opp-domain.c @@ -0,0 +1,427 @@ +/* + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/ + * Nishanth Menon nm@ti.com + * Dave Gerlach d-gerlach@ti.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * TI OPP Domain driver that provides overrides into the regulator control + * for generic opp domains to handle devices with ABB regulator and/or + * SmartReflex Class0. + */ +#include <linux/clk.h> +#include <linux/cpufreq.h> +#include <linux/device.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/notifier.h> +#include <linux/of_device.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_opp.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> + +/** + * struct ti_oppdm_optimum_voltage_table - optimized voltage table + * @reference_uv: reference voltage (usually Nominal voltage) + * @optimized_uv: Optimized voltage from efuse + */ +struct ti_oppdm_optimum_voltage_table { + unsigned int reference_uv; + unsigned int optimized_uv; +}; + +/** + * struct ti_oppdm_data - OMAP specific opp domain data + * @vdd_reg: VDD regulator + * @vbb_reg: Body Bias regulator + * @vdd_table: Optimized voltage mapping table + * @num_vdd_table: number of entries in vdd_table + * @vdd_absolute_max_voltage_uv: absolute maximum voltage in UV for the domain + */ +struct ti_oppdm_data { + struct regulator *vdd_reg; + struct regulator *vbb_reg; + struct ti_oppdm_optimum_voltage_table *vdd_table; + u32 num_vdd_table; + u32 vdd_absolute_max_voltage_uv; +}; + +static struct ti_oppdm_data opp_data; +/** + * struct ti_oppdm_of_data - device tree match data + * @desc: opp domain descriptor for opp domain core + * @flags: specific type of opp domain + * @efuse_voltage_mask: mask required for efuse register representing voltage + * @efuse_voltage_uv: Are the efuse entries in micro-volts? if not, assume + * milli-volts. + */ +struct ti_oppdm_of_data { +#define OPPDM_EFUSE_CLASS0_OPTIMIZED_VOLTAGE BIT(1) +#define OPPDM_HAS_NO_ABB BIT(2) + const u8 flags; + const u32 efuse_voltage_mask; + const bool efuse_voltage_uv; +}; + +/** + * oppdm_store_optimized_voltages() - store optimized voltages + * @dev: opp domain device for which we need to store info + * @data: data specific to the device + * + * Picks up efuse based optimized voltages for VDD unique per device and + * stores it in internal data structure for use during transition requests. + * + * Return: If successful, 0, else appropriate error value. + */ +static int oppdm_store_optimized_voltages(struct device *dev, + struct ti_oppdm_data *data) +{ + void __iomem *base; + struct property *prop; + struct resource *res; + const __be32 *val; + int proplen, i; + int ret = 0; + struct ti_oppdm_optimum_voltage_table *table; + const struct ti_oppdm_of_data *of_data = dev_get_drvdata(dev); + + /* pick up Efuse based voltages */ + res = platform_get_resource(to_platform_device(dev), IORESOURCE_MEM, 0); + if (!res) { + dev_err(dev, "Unable to get IO resource\n"); + ret = -ENODEV; + goto out_map; + } + + base = ioremap_nocache(res->start, resource_size(res)); + if (!base) { + dev_err(dev, "Unable to map Efuse registers\n"); + ret = -ENOMEM; + goto out_map; + } + + /* Fetch efuse-settings. */ + prop = of_find_property(dev->of_node, "ti,efuse-settings", NULL); + if (!prop) { + dev_err(dev, "No 'ti,efuse-settings' property found\n"); + ret = -EINVAL; + goto out; + } + + proplen = prop->length / sizeof(int); + data->num_vdd_table = proplen / 2; + /* Verify for corrupted OPP entries in dt */ + if (data->num_vdd_table * 2 * sizeof(int) != prop->length) { + dev_err(dev, "Invalid 'ti,efuse-settings'\n"); + ret = -EINVAL; + goto out; + } + + ret = of_property_read_u32(dev->of_node, "ti,absolute-max-voltage-uv", + &data->vdd_absolute_max_voltage_uv); + if (ret) { + dev_err(dev, "ti,absolute-max-voltage-uv is missing\n"); + ret = -EINVAL; + goto out; + } + + table = kzalloc(sizeof(*data->vdd_table) * + data->num_vdd_table, GFP_KERNEL); + if (!table) { + ret = -ENOMEM; + goto out; + } + data->vdd_table = table; + + val = prop->value; + for (i = 0; i < data->num_vdd_table; i++, table++) { + u32 efuse_offset; + u32 tmp; + + table->reference_uv = be32_to_cpup(val++); + efuse_offset = be32_to_cpup(val++); + + tmp = readl(base + efuse_offset); + tmp &= of_data->efuse_voltage_mask; + tmp >>= __ffs(of_data->efuse_voltage_mask); + + table->optimized_uv = of_data->efuse_voltage_uv ? tmp : + tmp * 1000; + + dev_dbg(dev, "[%d] efuse=0x%08x volt_table=%d vset=%d\n", + i, efuse_offset, table->reference_uv, + table->optimized_uv); + + /* + * Some older samples might not have optimized efuse + * Use reference voltage for those - just add debug message + * for them. + */ + if (!table->optimized_uv) { + dev_dbg(dev, "[%d] efuse=0x%08x volt_table=%d:vset0\n", + i, efuse_offset, table->reference_uv); + table->optimized_uv = table->reference_uv; + } + } +out: + iounmap(base); +out_map: + return ret; +} + +/** + * oppdm_free_optimized_voltages() - free resources for optimized voltages + * @dev: opp domain device for which we need to free info + * @data: data specific to the device + */ +static void oppdm_free_optimized_voltages(struct device *dev, + struct ti_oppdm_data *data) +{ + kfree(data->vdd_table); + data->vdd_table = NULL; + data->num_vdd_table = 0; +} + +/** + * oppdm_get_optimal_vdd_voltage() - Finds optimal voltage for the domain + * @dev: opp domain device for which we need to find info + * @data: data specific to the device + * @reference_uv: reference voltage (OPP voltage) for which we need value + * + * Return: if a match is found, return optimized voltage, else return + * reference_uv, also return reference_uv if no optimization is needed. + */ +static int oppdm_get_optimal_vdd_voltage(struct device *dev, + struct ti_oppdm_data *data, + int reference_uv) +{ + int i; + struct ti_oppdm_optimum_voltage_table *table; + + if (!data->num_vdd_table) + return reference_uv; + + table = data->vdd_table; + if (!table) + return -EINVAL; + + /* Find a exact match - this list is usually very small */ + for (i = 0; i < data->num_vdd_table; i++, table++) + if (table->reference_uv == reference_uv) + return table->optimized_uv; + + /* IF things are screwed up, we'd make a mess on console.. ratelimit */ + dev_err_ratelimited(dev, "%s: Failed optimized voltage match for %d\n", + __func__, reference_uv); + return reference_uv; +} + +/** + * ti_oppdm_set_opp() - do the opp domain transition + * @dev: opp domain device for which we are doing the transition + * @data: information on regulators and new and old opps provided by + * opp core to use in transition + * + * Return: If successful, 0, else appropriate error value. + */ +int ti_oppdm_set_opp(struct device *dev, struct dev_pm_set_opp_data *data) +{ + struct dev_pm_opp_supply *old_supply_vdd = &data->old_opp.supplies[0]; + struct dev_pm_opp_supply *old_supply_vbb = &data->old_opp.supplies[1]; + struct dev_pm_opp_supply *new_supply_vdd = &data->new_opp.supplies[0]; + struct dev_pm_opp_supply *new_supply_vbb = &data->new_opp.supplies[1]; + unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate; + struct clk *clk = data->clk; + struct regulator *vdd_reg = data->regulators[0]; + struct regulator *vbb_reg = data->regulators[1]; + int vdd_uv; + int ret; + + vdd_uv = oppdm_get_optimal_vdd_voltage(dev, &opp_data, new_supply_vbb->u_volt); + + /* Scaling up? Scale voltage before frequency */ + if (freq > old_freq) { + /* Regulator not available for device */ + + dev_dbg(dev, "vbb pre %luuV[min %luuV max %luuV]\n", + new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, + new_supply_vbb->u_volt_max); + + ret = regulator_set_voltage_triplet(vbb_reg, + new_supply_vbb->u_volt_min, + new_supply_vbb->u_volt, + new_supply_vbb->u_volt_max); + if (ret) { + dev_err(dev, "vbb failed for %luuV[min %luuV max %luuV]\n", + new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, + new_supply_vbb->u_volt_max); + return ret; + } + + dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, + new_supply_vdd->u_volt_min, new_supply_vdd->u_volt, + new_supply_vdd->u_volt_max); + + ret = regulator_set_voltage_triplet(vdd_reg, + new_supply_vdd->u_volt_min, + new_supply_vdd->u_volt, + new_supply_vdd->u_volt_max); + if (ret) + dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n", + __func__, new_supply_vdd->u_volt_min, + new_supply_vdd->u_volt, + new_supply_vdd->u_volt_max, ret); + } + + /* Change frequency */ + dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", + __func__, old_freq, freq); + + ret = clk_set_rate(clk, freq); + if (ret) { + dev_err(dev, "%s: failed to set clock rate: %d\n", __func__, + ret); + } + + /* Scaling down? Scale voltage after frequency */ + if (freq < old_freq) { + dev_dbg(dev, "vbb post %luuV[min %luuV max %luuV]\n", + new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, + new_supply_vbb->u_volt_max); + + ret = regulator_set_voltage_triplet(vbb_reg, + new_supply_vbb->u_volt_min, + new_supply_vbb->u_volt, + new_supply_vbb->u_volt_max); + if (ret) { + dev_err(dev, "vbb failed for %luuV[min %luuV max %luuV]\n", + new_supply_vbb->u_volt, + new_supply_vbb->u_volt_min, + new_supply_vbb->u_volt_max); + return ret; + } + + dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, + new_supply_vdd->u_volt_min, new_supply_vdd->u_volt, + new_supply_vdd->u_volt_max); + + ret = regulator_set_voltage_triplet(vdd_reg, + new_supply_vdd->u_volt_min, + new_supply_vdd->u_volt, + new_supply_vdd->u_volt_max); + if (ret) + dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n", + __func__, new_supply_vdd->u_volt_min, + new_supply_vdd->u_volt, + new_supply_vdd->u_volt_max, ret); + } + + return 0; + +restore_freq: + ret = clk_set_rate(clk, old_freq); + if (ret) + dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n", + __func__, old_freq); +restore_voltage: + /* This shouldn't harm even if the voltages weren't updated earlier */ + if (old_supply_vdd->u_volt) { + dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, + old_supply_vdd->u_volt_min, old_supply_vdd->u_volt, + old_supply_vdd->u_volt_max); + + ret = regulator_set_voltage_triplet(vdd_reg, + old_supply_vdd->u_volt_min, + old_supply_vdd->u_volt, + old_supply_vdd->u_volt_max); + if (ret) + dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n", + __func__, old_supply_vdd->u_volt_min, + old_supply_vdd->u_volt, + old_supply_vdd->u_volt_max, ret); + } + + return ret; +} + +static const struct ti_oppdm_of_data omap_generic_of_data = { +}; + +static const struct ti_oppdm_of_data omap_omap5_of_data = { + .flags = OPPDM_EFUSE_CLASS0_OPTIMIZED_VOLTAGE, + .efuse_voltage_mask = 0xFFF, + .efuse_voltage_uv = false, +}; + +static const struct ti_oppdm_of_data omap_omap5core_of_data = { + .flags = OPPDM_EFUSE_CLASS0_OPTIMIZED_VOLTAGE | OPPDM_HAS_NO_ABB, + .efuse_voltage_mask = 0xFFF, + .efuse_voltage_uv = false, +}; + +static const struct of_device_id ti_oppdm_of_match[] = { + {.compatible = "ti,omap-oppdm", .data = &omap_generic_of_data}, + {.compatible = "ti,omap5-oppdm", .data = &omap_omap5_of_data}, + {.compatible = "ti,omap5-core-oppdm", .data = &omap_omap5core_of_data}, + {}, +}; + +static int ti_oppdm_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device *cpu_dev = get_cpu_device(0); /* Gross hack */ + const struct of_device_id *match; + struct pm_opp_domain_dev *oppdm_dev; + int ret = 0; + const struct ti_oppdm_of_data *of_data; + const char *names[] = {"vdd", "vbb"}; + + ret = dev_pm_opp_set_regulators(cpu_dev, names, + ARRAY_SIZE(names)); + + if (ret) + return ret; + + match = of_match_device(ti_oppdm_of_match, dev); + if (!match) { + /* We do not expect this to happen */ + dev_err(dev, "%s: Unable to match device\n", __func__); + return -ENODEV; + } + if (!match->data) { + /* Again, unlikely.. but mistakes do happen */ + dev_err(dev, "%s: Bad data in match\n", __func__); + return -EINVAL; + } + of_data = match->data; + + dev_set_drvdata(dev, (void *)of_data); + /* If we need optimized voltage */ + if (of_data->flags & OPPDM_EFUSE_CLASS0_OPTIMIZED_VOLTAGE) { + ret = oppdm_store_optimized_voltages(dev, &opp_data); + } + + dev_pm_opp_register_set_opp_helper(cpu_dev, ti_oppdm_set_opp); + + return ret; +} + +MODULE_DEVICE_TABLE(of, ti_oppdm_of_match); + +static struct platform_driver ti_oppdm_driver = { + .probe = ti_oppdm_probe, + .driver = { + .name = "ti_oppdm", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(ti_oppdm_of_match), + }, +}; +module_platform_driver(ti_oppdm_driver); + +MODULE_DESCRIPTION("Texas Instruments OMAP OPP Domain driver"); +MODULE_AUTHOR("Texas Instruments Inc."); +MODULE_LICENSE("GPL v2");
Hi Dave,
[auto build test ERROR on robh/for-next] [also build test ERROR on v4.9-rc5] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dave-Gerlach/WIP-Test-OPP-multi-reg... base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next config: ia64-allmodconfig (attached as .config) compiler: ia64-linux-gcc (GCC) 6.2.0 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/ma... -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=ia64
All error/warnings (new ones prefixed by >>):
drivers/soc/ti/ti-opp-domain.c:231:49: warning: 'struct dev_pm_set_opp_data' declared inside parameter list will not be visible outside of this definition or declaration
int ti_oppdm_set_opp(struct device *dev, struct dev_pm_set_opp_data *data) ^~~~~~~~~~~~~~~~~~~ drivers/soc/ti/ti-opp-domain.c: In function 'ti_oppdm_set_opp':
drivers/soc/ti/ti-opp-domain.c:233:50: error: dereferencing pointer to incomplete type 'struct dev_pm_set_opp_data'
struct dev_pm_opp_supply *old_supply_vdd = &data->old_opp.supplies[0]; ^~
drivers/soc/ti/ti-opp-domain.c:244:71: error: dereferencing pointer to incomplete type 'struct dev_pm_opp_supply'
vdd_uv = oppdm_get_optimal_vdd_voltage(dev, &opp_data, new_supply_vbb->u_volt); ^~
drivers/soc/ti/ti-opp-domain.c:330:1: warning: label 'restore_voltage' defined but not used [-Wunused-label]
restore_voltage: ^~~~~~~~~~~~~~~
drivers/soc/ti/ti-opp-domain.c:325:1: warning: label 'restore_freq' defined but not used [-Wunused-label]
restore_freq: ^~~~~~~~~~~~
drivers/soc/ti/ti-opp-domain.c:234:28: warning: unused variable 'old_supply_vbb' [-Wunused-variable]
struct dev_pm_opp_supply *old_supply_vbb = &data->old_opp.supplies[1]; ^~~~~~~~~~~~~~ drivers/soc/ti/ti-opp-domain.c: In function 'ti_oppdm_probe': drivers/soc/ti/ti-opp-domain.c:383:8: error: implicit declaration of function 'dev_pm_opp_set_regulators' [-Werror=implicit-function-declaration] ret = dev_pm_opp_set_regulators(cpu_dev, names, ^~~~~~~~~~~~~~~~~~~~~~~~~ drivers/soc/ti/ti-opp-domain.c:408:2: error: implicit declaration of function 'dev_pm_opp_register_set_opp_helper' [-Werror=implicit-function-declaration] dev_pm_opp_register_set_opp_helper(cpu_dev, ti_oppdm_set_opp); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/soc/ti/ti-opp-domain.c:378:28: warning: unused variable 'oppdm_dev' [-Wunused-variable]
struct pm_opp_domain_dev *oppdm_dev; ^~~~~~~~~ At top level: drivers/soc/ti/ti-opp-domain.c:181:13: warning: 'oppdm_free_optimized_voltages' defined but not used [-Wunused-function] static void oppdm_free_optimized_voltages(struct device *dev, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors
vim +233 drivers/soc/ti/ti-opp-domain.c
225 * @dev: opp domain device for which we are doing the transition 226 * @data: information on regulators and new and old opps provided by 227 * opp core to use in transition 228 * 229 * Return: If successful, 0, else appropriate error value. 230 */
231 int ti_oppdm_set_opp(struct device *dev, struct dev_pm_set_opp_data *data)
232 {
233 struct dev_pm_opp_supply *old_supply_vdd = &data->old_opp.supplies[0]; 234 struct dev_pm_opp_supply *old_supply_vbb = &data->old_opp.supplies[1];
235 struct dev_pm_opp_supply *new_supply_vdd = &data->new_opp.supplies[0]; 236 struct dev_pm_opp_supply *new_supply_vbb = &data->new_opp.supplies[1]; 237 unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate; 238 struct clk *clk = data->clk; 239 struct regulator *vdd_reg = data->regulators[0]; 240 struct regulator *vbb_reg = data->regulators[1]; 241 int vdd_uv; 242 int ret; 243
244 vdd_uv = oppdm_get_optimal_vdd_voltage(dev, &opp_data, new_supply_vbb->u_volt);
245 246 /* Scaling up? Scale voltage before frequency */ 247 if (freq > old_freq) { 248 /* Regulator not available for device */ 249 250 dev_dbg(dev, "vbb pre %luuV[min %luuV max %luuV]\n", 251 new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, 252 new_supply_vbb->u_volt_max); 253 254 ret = regulator_set_voltage_triplet(vbb_reg, 255 new_supply_vbb->u_volt_min, 256 new_supply_vbb->u_volt, 257 new_supply_vbb->u_volt_max); 258 if (ret) { 259 dev_err(dev, "vbb failed for %luuV[min %luuV max %luuV]\n", 260 new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, 261 new_supply_vbb->u_volt_max); 262 return ret; 263 } 264 265 dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, 266 new_supply_vdd->u_volt_min, new_supply_vdd->u_volt, 267 new_supply_vdd->u_volt_max); 268 269 ret = regulator_set_voltage_triplet(vdd_reg, 270 new_supply_vdd->u_volt_min, 271 new_supply_vdd->u_volt, 272 new_supply_vdd->u_volt_max); 273 if (ret) 274 dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n", 275 __func__, new_supply_vdd->u_volt_min, 276 new_supply_vdd->u_volt, 277 new_supply_vdd->u_volt_max, ret); 278 } 279 280 /* Change frequency */ 281 dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", 282 __func__, old_freq, freq); 283 284 ret = clk_set_rate(clk, freq); 285 if (ret) { 286 dev_err(dev, "%s: failed to set clock rate: %d\n", __func__, 287 ret); 288 } 289 290 /* Scaling down? Scale voltage after frequency */ 291 if (freq < old_freq) { 292 dev_dbg(dev, "vbb post %luuV[min %luuV max %luuV]\n", 293 new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, 294 new_supply_vbb->u_volt_max); 295 296 ret = regulator_set_voltage_triplet(vbb_reg, 297 new_supply_vbb->u_volt_min, 298 new_supply_vbb->u_volt, 299 new_supply_vbb->u_volt_max); 300 if (ret) { 301 dev_err(dev, "vbb failed for %luuV[min %luuV max %luuV]\n", 302 new_supply_vbb->u_volt, 303 new_supply_vbb->u_volt_min, 304 new_supply_vbb->u_volt_max); 305 return ret; 306 } 307 308 dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, 309 new_supply_vdd->u_volt_min, new_supply_vdd->u_volt, 310 new_supply_vdd->u_volt_max); 311 312 ret = regulator_set_voltage_triplet(vdd_reg, 313 new_supply_vdd->u_volt_min, 314 new_supply_vdd->u_volt, 315 new_supply_vdd->u_volt_max); 316 if (ret) 317 dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n", 318 __func__, new_supply_vdd->u_volt_min, 319 new_supply_vdd->u_volt, 320 new_supply_vdd->u_volt_max, ret); 321 } 322 323 return 0; 324
325 restore_freq:
326 ret = clk_set_rate(clk, old_freq); 327 if (ret) 328 dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n", 329 __func__, old_freq);
330 restore_voltage:
331 /* This shouldn't harm even if the voltages weren't updated earlier */ 332 if (old_supply_vdd->u_volt) { 333 dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, 334 old_supply_vdd->u_volt_min, old_supply_vdd->u_volt, 335 old_supply_vdd->u_volt_max); 336 337 ret = regulator_set_voltage_triplet(vdd_reg, 338 old_supply_vdd->u_volt_min, 339 old_supply_vdd->u_volt, 340 old_supply_vdd->u_volt_max); 341 if (ret) 342 dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n", 343 __func__, old_supply_vdd->u_volt_min, 344 old_supply_vdd->u_volt, 345 old_supply_vdd->u_volt_max, ret); 346 } 347 348 return ret; 349 } 350 351 static const struct ti_oppdm_of_data omap_generic_of_data = { 352 }; 353 354 static const struct ti_oppdm_of_data omap_omap5_of_data = { 355 .flags = OPPDM_EFUSE_CLASS0_OPTIMIZED_VOLTAGE, 356 .efuse_voltage_mask = 0xFFF, 357 .efuse_voltage_uv = false, 358 }; 359 360 static const struct ti_oppdm_of_data omap_omap5core_of_data = { 361 .flags = OPPDM_EFUSE_CLASS0_OPTIMIZED_VOLTAGE | OPPDM_HAS_NO_ABB, 362 .efuse_voltage_mask = 0xFFF, 363 .efuse_voltage_uv = false, 364 }; 365 366 static const struct of_device_id ti_oppdm_of_match[] = { 367 {.compatible = "ti,omap-oppdm", .data = &omap_generic_of_data}, 368 {.compatible = "ti,omap5-oppdm", .data = &omap_omap5_of_data}, 369 {.compatible = "ti,omap5-core-oppdm", .data = &omap_omap5core_of_data}, 370 {}, 371 }; 372 373 static int ti_oppdm_probe(struct platform_device *pdev) 374 { 375 struct device *dev = &pdev->dev; 376 struct device *cpu_dev = get_cpu_device(0); /* Gross hack */ 377 const struct of_device_id *match;
378 struct pm_opp_domain_dev *oppdm_dev;
379 int ret = 0; 380 const struct ti_oppdm_of_data *of_data; 381 const char *names[] = {"vdd", "vbb"}; 382
383 ret = dev_pm_opp_set_regulators(cpu_dev, names,
384 ARRAY_SIZE(names)); 385 386 if (ret)
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Dave,
[auto build test WARNING on robh/for-next] [also build test WARNING on v4.9-rc5 next-20161115] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dave-Gerlach/WIP-Test-OPP-multi-reg... base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next config: m68k-allmodconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 4.9.0 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/ma... -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=m68k
All warnings (new ones prefixed by >>):
drivers/soc/ti/ti-opp-domain.c:231:49: warning: 'struct dev_pm_set_opp_data' declared inside parameter list int ti_oppdm_set_opp(struct device *dev, struct dev_pm_set_opp_data *data) ^ drivers/soc/ti/ti-opp-domain.c:231:49: warning: its scope is only this definition or declaration, which is probably not what you want drivers/soc/ti/ti-opp-domain.c: In function 'ti_oppdm_set_opp': drivers/soc/ti/ti-opp-domain.c:233:50: error: dereferencing pointer to incomplete type struct dev_pm_opp_supply *old_supply_vdd = &data->old_opp.supplies[0]; ^ drivers/soc/ti/ti-opp-domain.c:234:50: error: dereferencing pointer to incomplete type struct dev_pm_opp_supply *old_supply_vbb = &data->old_opp.supplies[1]; ^ drivers/soc/ti/ti-opp-domain.c:235:50: error: dereferencing pointer to incomplete type struct dev_pm_opp_supply *new_supply_vdd = &data->new_opp.supplies[0]; ^ drivers/soc/ti/ti-opp-domain.c:236:50: error: dereferencing pointer to incomplete type struct dev_pm_opp_supply *new_supply_vbb = &data->new_opp.supplies[1]; ^ drivers/soc/ti/ti-opp-domain.c:237:31: error: dereferencing pointer to incomplete type unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate; ^ drivers/soc/ti/ti-opp-domain.c:237:58: error: dereferencing pointer to incomplete type unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate; ^ drivers/soc/ti/ti-opp-domain.c:238:24: error: dereferencing pointer to incomplete type struct clk *clk = data->clk; ^ drivers/soc/ti/ti-opp-domain.c:239:34: error: dereferencing pointer to incomplete type struct regulator *vdd_reg = data->regulators[0]; ^ drivers/soc/ti/ti-opp-domain.c:240:34: error: dereferencing pointer to incomplete type struct regulator *vbb_reg = data->regulators[1]; ^ drivers/soc/ti/ti-opp-domain.c:244:71: error: dereferencing pointer to incomplete type vdd_uv = oppdm_get_optimal_vdd_voltage(dev, &opp_data, new_supply_vbb->u_volt); ^ In file included from include/linux/printk.h:305:0, from include/linux/kernel.h:13, from include/linux/clk.h:16, from drivers/soc/ti/ti-opp-domain.c:14: drivers/soc/ti/ti-opp-domain.c:251:18: error: dereferencing pointer to incomplete type new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, ^ include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg' ##__VA_ARGS__); \ ^
drivers/soc/ti/ti-opp-domain.c:250:3: note: in expansion of macro 'dev_dbg'
dev_dbg(dev, "vbb pre %luuV[min %luuV max %luuV]\n", ^ drivers/soc/ti/ti-opp-domain.c:251:42: error: dereferencing pointer to incomplete type new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, ^ include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg' ##__VA_ARGS__); \ ^
drivers/soc/ti/ti-opp-domain.c:250:3: note: in expansion of macro 'dev_dbg'
dev_dbg(dev, "vbb pre %luuV[min %luuV max %luuV]\n", ^ drivers/soc/ti/ti-opp-domain.c:252:18: error: dereferencing pointer to incomplete type new_supply_vbb->u_volt_max); ^ include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg' ##__VA_ARGS__); \ ^
drivers/soc/ti/ti-opp-domain.c:250:3: note: in expansion of macro 'dev_dbg'
dev_dbg(dev, "vbb pre %luuV[min %luuV max %luuV]\n", ^ drivers/soc/ti/ti-opp-domain.c:255:25: error: dereferencing pointer to incomplete type new_supply_vbb->u_volt_min, ^ drivers/soc/ti/ti-opp-domain.c:256:25: error: dereferencing pointer to incomplete type new_supply_vbb->u_volt, ^ drivers/soc/ti/ti-opp-domain.c:257:25: error: dereferencing pointer to incomplete type new_supply_vbb->u_volt_max); ^ drivers/soc/ti/ti-opp-domain.c:260:19: error: dereferencing pointer to incomplete type new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, ^ drivers/soc/ti/ti-opp-domain.c:260:43: error: dereferencing pointer to incomplete type new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, ^ drivers/soc/ti/ti-opp-domain.c:261:19: error: dereferencing pointer to incomplete type new_supply_vbb->u_volt_max); ^ In file included from include/linux/printk.h:305:0, from include/linux/kernel.h:13, from include/linux/clk.h:16, from drivers/soc/ti/ti-opp-domain.c:14: drivers/soc/ti/ti-opp-domain.c:266:18: error: dereferencing pointer to incomplete type new_supply_vdd->u_volt_min, new_supply_vdd->u_volt, ^ include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg' ##__VA_ARGS__); \ ^ drivers/soc/ti/ti-opp-domain.c:265:3: note: in expansion of macro 'dev_dbg' dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, ^ drivers/soc/ti/ti-opp-domain.c:266:46: error: dereferencing pointer to incomplete type new_supply_vdd->u_volt_min, new_supply_vdd->u_volt, ^ include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg' ##__VA_ARGS__); \ ^ drivers/soc/ti/ti-opp-domain.c:265:3: note: in expansion of macro 'dev_dbg' dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, ^ drivers/soc/ti/ti-opp-domain.c:267:18: error: dereferencing pointer to incomplete type new_supply_vdd->u_volt_max); ^ include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg' ##__VA_ARGS__); \ ^ drivers/soc/ti/ti-opp-domain.c:265:3: note: in expansion of macro 'dev_dbg' dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, ^ drivers/soc/ti/ti-opp-domain.c:270:25: error: dereferencing pointer to incomplete type new_supply_vdd->u_volt_min, ^ drivers/soc/ti/ti-opp-domain.c:271:25: error: dereferencing pointer to incomplete type new_supply_vdd->u_volt, ^ drivers/soc/ti/ti-opp-domain.c:272:25: error: dereferencing pointer to incomplete type new_supply_vdd->u_volt_max); ^ drivers/soc/ti/ti-opp-domain.c:275:29: error: dereferencing pointer to incomplete type __func__, new_supply_vdd->u_volt_min, ^ drivers/soc/ti/ti-opp-domain.c:276:19: error: dereferencing pointer to incomplete type new_supply_vdd->u_volt, ^ drivers/soc/ti/ti-opp-domain.c:277:19: error: dereferencing pointer to incomplete type new_supply_vdd->u_volt_max, ret); ^ In file included from include/linux/printk.h:305:0, from include/linux/kernel.h:13, from include/linux/clk.h:16, from drivers/soc/ti/ti-opp-domain.c:14: drivers/soc/ti/ti-opp-domain.c:293:19: error: dereferencing pointer to incomplete type new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, ^ include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg' ##__VA_ARGS__); \ ^ drivers/soc/ti/ti-opp-domain.c:292:3: note: in expansion of macro 'dev_dbg' dev_dbg(dev, "vbb post %luuV[min %luuV max %luuV]\n", ^ drivers/soc/ti/ti-opp-domain.c:293:43: error: dereferencing pointer to incomplete type new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, ^ include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg' ##__VA_ARGS__); \ ^ drivers/soc/ti/ti-opp-domain.c:292:3: note: in expansion of macro 'dev_dbg' dev_dbg(dev, "vbb post %luuV[min %luuV max %luuV]\n", ^ drivers/soc/ti/ti-opp-domain.c:294:19: error: dereferencing pointer to incomplete type new_supply_vbb->u_volt_max); ^ include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg' ##__VA_ARGS__); \ ^ drivers/soc/ti/ti-opp-domain.c:292:3: note: in expansion of macro 'dev_dbg' dev_dbg(dev, "vbb post %luuV[min %luuV max %luuV]\n", ^
vim +/dev_dbg +250 drivers/soc/ti/ti-opp-domain.c
234 struct dev_pm_opp_supply *old_supply_vbb = &data->old_opp.supplies[1]; 235 struct dev_pm_opp_supply *new_supply_vdd = &data->new_opp.supplies[0]; 236 struct dev_pm_opp_supply *new_supply_vbb = &data->new_opp.supplies[1]; 237 unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate; 238 struct clk *clk = data->clk; 239 struct regulator *vdd_reg = data->regulators[0]; 240 struct regulator *vbb_reg = data->regulators[1]; 241 int vdd_uv; 242 int ret; 243 244 vdd_uv = oppdm_get_optimal_vdd_voltage(dev, &opp_data, new_supply_vbb->u_volt); 245 246 /* Scaling up? Scale voltage before frequency */ 247 if (freq > old_freq) { 248 /* Regulator not available for device */ 249
250 dev_dbg(dev, "vbb pre %luuV[min %luuV max %luuV]\n",
251 new_supply_vbb->u_volt, new_supply_vbb->u_volt_min, 252 new_supply_vbb->u_volt_max); 253 254 ret = regulator_set_voltage_triplet(vbb_reg, 255 new_supply_vbb->u_volt_min, 256 new_supply_vbb->u_volt, 257 new_supply_vbb->u_volt_max); 258 if (ret) {
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Thanks for this Dave :)
On 15-11-16, 16:10, Dave Gerlach wrote:
NOT FOR MERGE!
Introduce a test version of a 'ti-opp-domain' driver that will use new multiple regulator support introduced to the OPP core by Viresh [1]. Tested on v4.9-rc1 with that series applied. This is needed on TI platforms like DRA7/AM57 in order to control both CPU regulator and Adaptive Body Bias (ABB) regulator as described by Nishanth Menon here [2]. These regulators must be scaled in sequence during an OPP transition depending on whether or not the frequency is being scaled up or down. Based on the new functionality provided by Viresh this driver does the following:
- Call dev_pm_opp_set_regulators with the names of the two regulators that feed the CPU:
- vdd is the 'cpu-supply' commonly used for cpufreq-dt but renamed so the cpufreq-dt driver doesn't use it directly. Note that this is supplied in board dts as it's external to SoC.
I think I can fix this somehow.. Lemme check.
- vbb for the ABB regulator. This is provided in SoC dtsi as it is internal to the SoC.
- Provide a platform set_opp function using dev_pm_opp_register_set_opp_helper that is called when an OPP transition is requested.
- Allow cpufreq-dt to probe which will work because no cpu-supply regulator is found so the driver proceeds and calls dev_pm_opp_set_rate which through the OPP core invokes the platform set_opp call we provided
- Platform set_opp call provided by this driver checks to see if we are scaling frequency up or down and based on this, scales vbb before vdd for up or the other way around for down.
In addition to that, this driver implements AVS Class 0 as described in section 18.4.6.12 of AM572x TRM [3] using the same platform set_rate hook added to the OPP core. There are registers that define the optimal voltage for that specific piece of silicon for an OPP so this driver simply looks up this optimal value and programs that for an OPP instead of the nominal value.
Missing from this is a good way to ensure that cpufreq-dt does not just proceed if no cpu-supply regulator is found but we were intending to rely on a platform set_opp and multiple regulators.
[1] https://marc.info/?l=linux-pm&m=147746362402994&w=2 [2] https://marc.info/?l=linux-pm&m=145684495832764&w=2 [3] http://www.ti.com/lit/ug/spruhz6g/spruhz6g.pdf
Signed-off-by: Dave Gerlach d-gerlach@ti.com
arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi | 2 +- arch/arm/boot/dts/dra7.dtsi | 46 ++- drivers/soc/ti/Makefile | 2 + drivers/soc/ti/ti-opp-domain.c | 427 ++++++++++++++++++++++++
I would rather ask you to move this to drivers/base/power/opp/
On 26-10-16, 12:02, Viresh Kumar wrote:
Hi,
Some platforms (like TI) have complex DVFS configuration for CPU devices, where multiple regulators are required to be configured to change DVFS state of the device. This was explained well by Nishanth earlier [1].
One of the major complaints around multiple regulators case was that the DT isn't responsible in any way to represent the ordering in which multiple supplies need to be programmed, before or after frequency change. It was considered in this patch and such information is left to the platform specific OPP driver now, which can register its own opp_set_rate() callback with the OPP core and the OPP core will then call it during DVFS.
The patches are tested on Exynos5250 (Dual A15). I have hacked around DT and code to pass values for multiple regulators and verified that they are all properly read by the kernel (using debugfs interface).
Dave Gerlach has already tested it on the real TI platforms and it works well for him.
This is rebased over: linux-next branch in the PM tree.
V2->V3:
- The last patch is new
- Removed a debug leftover pr_info() message
- Renamed few names as s/set_rate/set_opp
- Removed a TODO comment (as it is done now with this series)
- created struct for min_uV and max_uV
- kerneldoc comments for structures in pm_opp.h
- s/const char */const char * const
- use kasprintf()
- Some more minor reformatting
- More Ack/RBY tags added
Hi guys,
Can we please get this series reviewed quickly and come to a conclusion? It has already taken a lot of time getting this merged and the present code seems to be the best possible shot we have, AFAIU.
On Fri, Nov 18, 2016 at 08:36:36AM +0530, Viresh Kumar wrote:
Can we please get this series reviewed quickly and come to a conclusion? It has already taken a lot of time getting this merged and the present code seems to be the best possible shot we have, AFAIU.
There already seems to be extensive, ongoing discusion about this...
On 18-11-16, 10:43, Mark Brown wrote:
On Fri, Nov 18, 2016 at 08:36:36AM +0530, Viresh Kumar wrote:
Can we please get this series reviewed quickly and come to a conclusion? It has already taken a lot of time getting this merged and the present code seems to be the best possible shot we have, AFAIU.
There already seems to be extensive, ongoing discusion about this...
And I am quite sure we are stuck again :)
I just wanted to say that we should get it to some sort of conclusion. And yes I want to say thanks to all who invested their time on this thread :)
So the LAST remaining question is:
"How do we know (from the DT) the order in which entries for multiple regulators are present in the OPP table?"
And I am not sure if we can do that without having a property like:
+ supply-names = "vcc0", "vcc1", "vcc2";
in the OPP table or the consumer device. And surely it isn't a clean enough solution and that's why this series relied on the code to get such details.
Does someone have an alternative? If NO, can we go ahead with this series as is?
On Tue, Nov 22, 2016 at 09:19:22AM +0530, Viresh Kumar wrote:
So the LAST remaining question is:
"How do we know (from the DT) the order in which entries for multiple regulators are present in the OPP table?"
And I am not sure if we can do that without having a property like:
supply-names = "vcc0", "vcc1", "vcc2";in the OPP table or the consumer device. And surely it isn't a clean enough solution and that's why this series relied on the code to get such details.
Does someone have an alternative? If NO, can we go ahead with this series as is?
I'm really not at all clear why this has to be in DT. My understanding was that this is basically a helper library for more specific bindings which already have to hard code things like sequencing so surely they'd be specifying the ordering to be used when supplying data?
On 22-11-16, 18:41, Mark Brown wrote:
On Tue, Nov 22, 2016 at 09:19:22AM +0530, Viresh Kumar wrote:
"How do we know (from the DT) the order in which entries for multiple regulators are present in the OPP table?"
And I am not sure if we can do that without having a property like:
supply-names = "vcc0", "vcc1", "vcc2";in the OPP table or the consumer device. And surely it isn't a clean enough solution and that's why this series relied on the code to get such details.
Does someone have an alternative? If NO, can we go ahead with this series as is?
I'm really not at all clear why this has to be in DT. My understanding was that this is basically a helper library for more specific bindings which already have to hard code things like sequencing so surely they'd be specifying the ordering to be used when supplying data?
I am a bit confused and perhaps I am misreading your feedback.
Are you saying that:
"we don't need to identify which microVolts value in the OPP table corresponds to which supply from the DT itself and we can do that with some hard coded stuff" ?
If yes, then below is from an earlier email from you, which I feel is opposite of what you are suggesting now.
On 09-11-16, 14:58, Mark Brown wrote:
On Wed, Oct 26, 2016 at 12:02:56PM +0530, Viresh Kumar wrote:
The platform driver is responsible to identify the order and pass it on to the OPP core. And the platform driver needs to have that hard coded.
That *really* should be in the binding. Honestly if the binding is this vague I'm not even clear that it's worth documenting these properties at this level, might be better to just put the documentation in the platform driver bindings.
On Wed, Nov 23, 2016 at 09:16:57AM +0530, Viresh Kumar wrote:
On 22-11-16, 18:41, Mark Brown wrote:
I'm really not at all clear why this has to be in DT. My understanding was that this is basically a helper library for more specific bindings which already have to hard code things like sequencing so surely they'd be specifying the ordering to be used when supplying data?
I am a bit confused and perhaps I am misreading your feedback.
Are you saying that:
"we don't need to identify which microVolts value in the OPP table corresponds to which supply from the DT itself and we can do that with some hard coded stuff" ?
No, of course not. That would be completely incoherent, there would be no way for anything to use the data if the values can just be in any random order.
If yes, then below is from an earlier email from you, which I feel is opposite of what you are suggesting now.
That *really* should be in the binding. Honestly if the binding is this vague I'm not even clear that it's worth documenting these properties at this level, might be better to just put the documentation in the platform driver bindings.
The "platform driver bindings" bit of this is very important here. This is a generic binding that is going to be used by platform specific drivers (as I understand it). I would therefore expect that these things can be described in the platform specific bindings.
Please, take a step back and think about what what the binding means and how it's going to be used. Not only is this a DT binding and therefore an ABI it's also a generic binding that's going to affect a lot of systems probably for a long time. This means it is really important to think things through and make sure we understand what they're doing. When working on kernel internal code it's relatively easy to fix things if we realize later that they don't work well so it's easier to just work quickly but when we're making ABIs that's not possible.
Hi Mark,
On 23-11-16, 12:29, Mark Brown wrote:
On Wed, Nov 23, 2016 at 09:16:57AM +0530, Viresh Kumar wrote:
Are you saying that:
"we don't need to identify which microVolts value in the OPP table corresponds to which supply from the DT itself and we can do that with some hard coded stuff" ?
No, of course not. That would be completely incoherent, there would be no way for anything to use the data if the values can just be in any random order.
With the current implementation in this patchset, the order in which entries are present in the OPP node is _assumed_ to be known to the platform specific code, which will pass it on to the OPP core with some callbacks. So the order will not be completely random.
If yes, then below is from an earlier email from you, which I feel is opposite of what you are suggesting now.
That *really* should be in the binding. Honestly if the binding is this vague I'm not even clear that it's worth documenting these properties at this level, might be better to just put the documentation in the platform driver bindings.
The "platform driver bindings" bit of this is very important here. This is a generic binding that is going to be used by platform specific drivers (as I understand it).
There is no platform specific binding here.
For example in case of a single regulator for the device (CPU), the platform specific DT file contains the CPU nodes (using generic bindings) and an OPP table node (again generic bindings only). The OPP core reads both these nodes for the device and constructs the OPP table.
Now in case of multiple regulators for the device, as you already know, the only unanswered detail (apart from the order in which the regulators need to be programmed) is to link which entries in the OPP table are for which regulator.
We can either get this information from DT (somehow) or hardcode it in platform specific code. This patch provided infrastructure for the later one.
If we indeed want to get this information from the DT then there are two options:
- Create platform specific binding:
foo-platform,supply-names = "vcc0", "vcc1", "vcc2";
- Create common binding that can be used by all platforms:
supply-names = "vcc0", "vcc1", "vcc2";
Such bindings will end up either in the consumer device node (like CPU0 node) or in the OPP table itself. I am personally inclined towards the common supply-names bindings, otherwise every user platform will end up creating very similar bindings.
I would therefore expect that these things can be described in the platform specific bindings.
Please, take a step back and think about what what the binding means and how it's going to be used. Not only is this a DT binding and therefore an ABI it's also a generic binding that's going to affect a lot of systems probably for a long time. This means it is really important to think things through and make sure we understand what they're doing. When working on kernel internal code it's relatively easy to fix things if we realize later that they don't work well so it's easier to just work quickly but when we're making ABIs that's not possible.
I agree and I completely understand your concerns here and it is surely very important to get the bindings right as they will last for very long.
But I am still unsure about what's the best way of doing this. The new bindings are rejected by almost everyone as they contain some of the information already contained in the consumer node while the regulators are defined.
On Thu, Nov 24, 2016 at 10:37:24AM +0530, Viresh Kumar wrote:
On 23-11-16, 12:29, Mark Brown wrote:
No, of course not. That would be completely incoherent, there would be no way for anything to use the data if the values can just be in any random order.
With the current implementation in this patchset, the order in which entries are present in the OPP node is _assumed_ to be known to the platform specific code, which will pass it on to the OPP core with some callbacks. So the order will not be completely random.
What we're reviewing here is the DT binding and the DT binding explicitly said the order doesn't matter. The DT binding is OS neutral so it needs to make sense without the code.
The "platform driver bindings" bit of this is very important here. This is a generic binding that is going to be used by platform specific drivers (as I understand it).
There is no platform specific binding here.
It seems like we're going to need one for this to be a comprehensible binding.
We can either get this information from DT (somehow) or hardcode it in platform specific code. This patch provided infrastructure for the later one.
If we indeed want to get this information from the DT then there are two options:
Why would we want to get it from DT when we can't get half the other information we need to make the data useful from DT?
linaro-kernel@lists.linaro.org
-
Dave Gerlach -
kbuild test robot -
Mark Brown -
Rob Herring -
Stephen Boyd -
Viresh Kumar