[PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Viresh Kumar]

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>;
 		};
 	};
-- 
2.7.1.410.g6faf27b

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Viresh Kumar]

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;

-- 
viresh

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Viresh Kumar]

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>;
                };
        };


-- 
viresh

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Rob Herring]

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Stephen Boyd]

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?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Viresh Kumar]

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
?

-- 
viresh

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Dave Gerlach]

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.
>

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Viresh Kumar]

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.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device

[Viresh Kumar]

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 ?

-- 
viresh