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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 15 Nov 2016 09:01:51 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Stephen Boyd <sboyd@...eaurora.org>
Cc:     Rob Herring <robh@...nel.org>, Mark Brown <broonie@...nel.org>,
        Rafael Wysocki <rjw@...ysocki.net>, nm@...com,
        Viresh Kumar <vireshk@...nel.org>,
        linaro-kernel@...ts.linaro.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Vincent Guittot <vincent.guittot@...aro.org>, d-gerlach@...com,
        devicetree@...r.kernel.org
Subject: Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple
 regulators per device

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@...0000000 {
			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