| Message ID | 28119b44689921f3c3cc00be49bff2bb99b32162.1477463128.git.viresh.kumar@linaro.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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.
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>; }; };