On 11/15, Viresh Kumar wrote:

On 14-11-16, 18:13, Stephen Boyd wrote:

quoted

On 11/14, Rob Herring wrote:

quoted

On Fri, Nov 11, 2016 at 08:41:20AM +0530, Viresh Kumar wrote:

quoted

On 10-11-16, 14:51, Stephen Boyd wrote:

quoted

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.

quoted

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.

quoted

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