Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1518234 > unrolled thread

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

Started byMark Brown <broonie@kernel.org>
First post2016-11-09 16:00 +0100
Last post2016-11-11 04:20 +0100
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple  regulators per device Mark Brown <broonie@kernel.org> - 2016-11-09 16:00 +0100
    Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple  regulators per device Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-10 05:10 +0100
      Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple  regulators per device Mark Brown <broonie@kernel.org> - 2016-11-10 17:40 +0100
        Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple  regulators per device Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-10 19:10 +0100
          Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple  regulators per device Stephen Boyd <sboyd@codeaurora.org> - 2016-11-11 00:00 +0100
            Re: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple  regulators per device Viresh Kumar <viresh.kumar@linaro.org> - 2016-11-11 04:20 +0100

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

FromMark Brown <broonie@kernel.org>
Date2016-11-09 16:00 +0100
SubjectRe: [PATCH V3 1/9] PM / OPP: Reword binding supporting multiple regulators per device
Message-ID<sBCw2-el-23@gated-at.bofh.it>

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [next] | [standalone]

#1518681

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-11-10 05:10 +0100
Message-ID<sBOQx-om-3@gated-at.bofh.it>
In reply to#1518234
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

[toc] | [prev] | [next] | [standalone]

#1519120

FromMark Brown <broonie@kernel.org>
Date2016-11-10 17:40 +0100
Message-ID<sC0yl-dI-1@gated-at.bofh.it>
In reply to#1518681

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1519259

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-11-10 19:10 +0100
Message-ID<sC1Xx-1gl-31@gated-at.bofh.it>
In reply to#1519120
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

[toc] | [prev] | [next] | [standalone]

#1519426

FromStephen Boyd <sboyd@codeaurora.org>
Date2016-11-11 00:00 +0100
Message-ID<sC6u6-4fw-11@gated-at.bofh.it>
In reply to#1519259
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

[toc] | [prev] | [next] | [standalone]

#1519534

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-11-11 04:20 +0100
Message-ID<sCaxH-7dZ-5@gated-at.bofh.it>
In reply to#1519426
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web