[PATCH 01/16] PM / OPP: Add 'supply-names' binding

[PATCH 01/16] PM / OPP: Add 'supply-names' binding

[Viresh Kumar]

Regulators already have stable DT bindings, wherein the consumer (of
supplies) will have following for each regulator/supply.

<name>-supply: <phandle to the regulator node>;

Current OPP bindings extend above, by transforming it into a list of
phandles. But we missed the <name> string, which is used to identify the
regulator.

And looking from regulators perspective, having two different ways of
specifying regulators doesn't seem like a step forward, it also means we
have to update every single device binding. And things will become
complex.

Another way to support multiple regulators per device (in OPP V2
bindings) is to leave regulator consumer bindings as is, and create a
'supply-names' property in the opp-table node, which will contain a list
of strings. The names in this list shall match 'name' from the
'<name>-supply' strings present in the device node.

The strings in this list also specify the order in which values must be
present in 'opp-microvolt' and 'opp-microamp' properties.

Cc: Mark Brown <broonie@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/devicetree/bindings/opp/opp.txt | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index 0cb44dc21f97..8759bc4783ed 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -69,6 +69,13 @@ This describes the OPPs belonging to a device. This node can have following
 - compatible: Allow OPPs to express their compatibility. It should be:
   "operating-points-v2".
 
+- supply-names: This is a required property, only if multiple supplies are
+  available for the device. Otherwise it is optional.
+
+  This list is used to pass names of all the device supplies. The order of names
+  present here is important, as that should match the order in which values are
+  present in 'opp-microvolt' and 'opp-microamp' properties.
+
 - OPP nodes: One or more OPP nodes describing voltage-current-frequency
   combinations. Their name isn't significant but their phandle can be used to
   reference an OPP.
@@ -97,8 +104,8 @@ 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 must be present in the same order as their
+  names are present in 'supply-names' property of the opp-table.
 
 - opp-microamp: The maximum current drawn by the device in microamperes
   considering system specific parameters (such as transients, process, aging,
@@ -107,10 +114,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 must be present in the same order as their
+  names are present in 'supply-names' property of the opp-table.
+
+  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.
 
 - clock-latency-ns: Specifies the maximum possible transition latency (in
   nanoseconds) for switching to this OPP from any other OPP.
@@ -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;
 
 		opp00 {
-- 
2.4.0

Re: [PATCH 01/16] PM / OPP: Add 'supply-names' binding

[Rob Herring]

On 09/11/2015 07:01 AM, Viresh Kumar wrote:
> Regulators already have stable DT bindings, wherein the consumer (of
> supplies) will have following for each regulator/supply.
> 
> <name>-supply: <phandle to the regulator node>;
> 
> Current OPP bindings extend above, by transforming it into a list of
> phandles. But we missed the <name> string, which is used to identify the
> regulator.
> 
> And looking from regulators perspective, having two different ways of
> specifying regulators doesn't seem like a step forward, it also means we
> have to update every single device binding. And things will become
> complex.
> 
> Another way to support multiple regulators per device (in OPP V2
> bindings) is to leave regulator consumer bindings as is, and create a
> 'supply-names' property in the opp-table node, which will contain a list
> of strings. The names in this list shall match 'name' from the
> '<name>-supply' strings present in the device node.
> 
> The strings in this list also specify the order in which values must be
> present in 'opp-microvolt' and 'opp-microamp' properties.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  Documentation/devicetree/bindings/opp/opp.txt | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index 0cb44dc21f97..8759bc4783ed 100644
> --- a/Documentation/devicetree/bindings/opp/opp.txt
> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> @@ -69,6 +69,13 @@ This describes the OPPs belonging to a device. This node can have following
>  - compatible: Allow OPPs to express their compatibility. It should be:
>    "operating-points-v2".
>  
> +- supply-names: This is a required property, only if multiple supplies are
> +  available for the device. Otherwise it is optional.
> +
> +  This list is used to pass names of all the device supplies. The order of names
> +  present here is important, as that should match the order in which values are
> +  present in 'opp-microvolt' and 'opp-microamp' properties.
> +

What if we have a 2nd device and supply rail? For example, what if the
L2$ has a separate rail from the cores but is linked to the OPPs.

>  - OPP nodes: One or more OPP nodes describing voltage-current-frequency
>    combinations. Their name isn't significant but their phandle can be used to
>    reference an OPP.
> @@ -97,8 +104,8 @@ 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 must be present in the same order as their
> +  names are present in 'supply-names' property of the opp-table.
>  
>  - opp-microamp: The maximum current drawn by the device in microamperes
>    considering system specific parameters (such as transients, process, aging,
> @@ -107,10 +114,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 must be present in the same order as their
> +  names are present in 'supply-names' property of the opp-table.
> +
> +  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.

Remind me of when do we have multiple regulators for a cpu? The number
of supplies should be defined by the cpu binding as function of how many
supply rails a cpu has.

>  
>  - clock-latency-ns: Specifies the maximum possible transition latency (in
>    nanoseconds) for switching to this OPP from any other OPP.
> @@ -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>;

This may just be an example, but a CA7 doesn't have 3 supply rails.

Rob

Re: [PATCH 01/16] PM / OPP: Add 'supply-names' binding

[Viresh Kumar]

On 14-09-15, 15:22, Rob Herring wrote:
> What if we have a 2nd device and supply rail? For example, what if the
> L2$ has a separate rail from the cores but is linked to the OPPs.

Right, so that is the case with the Mediatek SoC as well, AFAIR. How
do we plan to treat L2 devices? For example, in the mediatek cpufreq
driver, they change L2's supplies together with CPUs.

One way to get that done, with such very closely bound devices is to
add two regulators:

cpu-supply = ...;
l2-supply = ...;

And then a property in OPP table:

supply-name = "cpu", "l2";

And maybe fix the order in which the supplies which be updated, based
on the order in which entries are present in the above property.

Any other way you suggest for doing that ?

> Remind me of when do we have multiple regulators for a cpu?

I haven't seen that yet, but its more like what I explained above.
i.e. one for the CPU and other one for the L2 cache.

But, these bindings do apply for other devices as well, where it
should be very much possible.

-- 
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 01/16] PM / OPP: Add 'supply-names' binding

[Viresh Kumar]

Rob,

On 15-09-15, 08:17, Viresh Kumar wrote:
> On 14-09-15, 15:22, Rob Herring wrote:
> > What if we have a 2nd device and supply rail? For example, what if the
> > L2$ has a separate rail from the cores but is linked to the OPPs.
> 
> Right, so that is the case with the Mediatek SoC as well, AFAIR. How
> do we plan to treat L2 devices? For example, in the mediatek cpufreq
> driver, they change L2's supplies together with CPUs.
> 
> One way to get that done, with such very closely bound devices is to
> add two regulators:
> 
> cpu-supply = ...;
> l2-supply = ...;
> 
> And then a property in OPP table:
> 
> supply-name = "cpu", "l2";
> 
> And maybe fix the order in which the supplies which be updated, based
> on the order in which entries are present in the above property.
> 
> Any other way you suggest for doing that ?
> 
> > Remind me of when do we have multiple regulators for a cpu?
> 
> I haven't seen that yet, but its more like what I explained above.
> i.e. one for the CPU and other one for the L2 cache.
> 
> But, these bindings do apply for other devices as well, where it
> should be very much possible.

Not sure if you still have any objections to this patch?

-- 
viresh

Re: [PATCH 01/16] PM / OPP: Add 'supply-names' binding

[Stephen Boyd]

On 09/14, Rob Herring wrote:
> On 09/11/2015 07:01 AM, Viresh Kumar wrote:
> > Regulators already have stable DT bindings, wherein the consumer (of
> > supplies) will have following for each regulator/supply.
> > 
> > <name>-supply: <phandle to the regulator node>;
> > 
> > Current OPP bindings extend above, by transforming it into a list of
> > phandles. But we missed the <name> string, which is used to identify the
> > regulator.
> > 
> > And looking from regulators perspective, having two different ways of
> > specifying regulators doesn't seem like a step forward, it also means we
> > have to update every single device binding. And things will become
> > complex.
> > 
> > Another way to support multiple regulators per device (in OPP V2
> > bindings) is to leave regulator consumer bindings as is, and create a
> > 'supply-names' property in the opp-table node, which will contain a list
> > of strings. The names in this list shall match 'name' from the
> > '<name>-supply' strings present in the device node.
> > 
> > The strings in this list also specify the order in which values must be
> > present in 'opp-microvolt' and 'opp-microamp' properties.
> > 
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/opp/opp.txt | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> > index 0cb44dc21f97..8759bc4783ed 100644
> > --- a/Documentation/devicetree/bindings/opp/opp.txt
> > +++ b/Documentation/devicetree/bindings/opp/opp.txt
> > @@ -69,6 +69,13 @@ This describes the OPPs belonging to a device. This node can have following
> >  - compatible: Allow OPPs to express their compatibility. It should be:
> >    "operating-points-v2".
> >  
> > +- supply-names: This is a required property, only if multiple supplies are
> > +  available for the device. Otherwise it is optional.
> > +
> > +  This list is used to pass names of all the device supplies. The order of names
> > +  present here is important, as that should match the order in which values are
> > +  present in 'opp-microvolt' and 'opp-microamp' properties.
> > +
> 
> What if we have a 2nd device and supply rail? For example, what if the
> L2$ has a separate rail from the cores but is linked to the OPPs.

I'm lost why we need this property at all. What happened to using

 opp-microvolt-0 = <1 2 3>;
 opp-microvolt-1 = <1>;
 opp-microvolt-2 = <3 4 5>;
 etc.

That seems to avoid any problem with 3 vs. 1 element properties
combined into one large array. Having supply-names seems too
brittle and would tie us to a particular OPP user's decision to
call supplies by some name.

Also, I've seen devices that are split across two power domains.
These devices aren't CPUs, but they are other devices including
L2 caches. So we're going to need either multiple regulator
support or multiple "power domain at a particular performance
levels" support somehow.

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

Re: [PATCH 01/16] PM / OPP: Add 'supply-names' binding

[Viresh Kumar]

On 15-10-15, 17:22, Stephen Boyd wrote:
> I'm lost why we need this property at all. What happened to using
> 
>  opp-microvolt-0 = <1 2 3>;
>  opp-microvolt-1 = <1>;
>  opp-microvolt-2 = <3 4 5>;
>  etc.

Perhaps you are confusing this with the bindings we came up for
picking right voltage levels based on the cuts/version of the hardware
we are running on. The problem that Lee Jones mentioned and that can
be used in your case as well.

> That seems to avoid any problem with 3 vs. 1 element properties
> combined into one large array.

That's not the problem I was trying to solve here.

> Having supply-names seems too
> brittle and would tie us to a particular OPP user's decision to
> call supplies by some name.

No. The name has to match the <name>-supply property present in the
device's node, that's why we need this property :)

> Also, I've seen devices that are split across two power domains.
> These devices aren't CPUs, but they are other devices including
> L2 caches. So we're going to need either multiple regulator
> support or multiple "power domain at a particular performance
> levels" support somehow.

Right, that's a good example of why we need multi-regulator support :)

-- 
viresh

Re: [PATCH 01/16] PM / OPP: Add 'supply-names' binding

[Stephen Boyd]

On 10/16, Viresh Kumar wrote:
> On 15-10-15, 17:22, Stephen Boyd wrote:
> > I'm lost why we need this property at all. What happened to using
> > 
> >  opp-microvolt-0 = <1 2 3>;
> >  opp-microvolt-1 = <1>;
> >  opp-microvolt-2 = <3 4 5>;
> >  etc.
> 
> Perhaps you are confusing this with the bindings we came up for
> picking right voltage levels based on the cuts/version of the hardware
> we are running on. The problem that Lee Jones mentioned and that can
> be used in your case as well.

Isn't that what this patch series is for?

> 
> > That seems to avoid any problem with 3 vs. 1 element properties
> > combined into one large array.
> 
> That's not the problem I was trying to solve here.

What problem are you trying to solve then?

> 
> > Having supply-names seems too
> > brittle and would tie us to a particular OPP user's decision to
> > call supplies by some name.
> 
> No. The name has to match the <name>-supply property present in the
> device's node, that's why we need this property :)

Why does it need to match? Sorry I'm totally lost now.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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 01/16] PM / OPP: Add 'supply-names' binding

[Viresh Kumar]

On 16-10-15, 12:16, Stephen Boyd wrote:
> On 10/16, Viresh Kumar wrote:
> > On 15-10-15, 17:22, Stephen Boyd wrote:
> > > I'm lost why we need this property at all. What happened to using
> > > 
> > >  opp-microvolt-0 = <1 2 3>;
> > >  opp-microvolt-1 = <1>;
> > >  opp-microvolt-2 = <3 4 5>;
> > >  etc.
> > 
> > Perhaps you are confusing this with the bindings we came up for
> > picking right voltage levels based on the cuts/version of the hardware
> > we are running on. The problem that Lee Jones mentioned and that can
> > be used in your case as well.
> 
> Isn't that what this patch series is for?

Hehe, no.

Okay here is the problem statement:

We have two supplies for a device and the device node will have
something like:

name1-supply = <&supply1>;
name2-supply = <&supply2>;

And the OPP node needs to have voltages for both of them:

opp-microvolt = <X1 Y1 Z1>, <X2 Y2 Z2>;

Where XYZ(1) are for supply1 and XYZ(2) are for supply2.

Now we need to identify the supplies for which the values are present
here and their order as well. How do we do that?

The way I am suggesting is to add a property in opp node which will
keep "name1" and "name2" in it.

-- 
viresh

Re: [PATCH 01/16] PM / OPP: Add 'supply-names' binding

[Viresh Kumar]

On 17-10-15, 09:40, Viresh Kumar wrote:
> Hehe, no.
> 
> Okay here is the problem statement:
> 
> We have two supplies for a device and the device node will have
> something like:
> 
> name1-supply = <&supply1>;
> name2-supply = <&supply2>;
> 
> And the OPP node needs to have voltages for both of them:
> 
> opp-microvolt = <X1 Y1 Z1>, <X2 Y2 Z2>;
> 
> Where XYZ(1) are for supply1 and XYZ(2) are for supply2.
> 
> Now we need to identify the supplies for which the values are present
> here and their order as well. How do we do that?
> 
> The way I am suggesting is to add a property in opp node which will
> keep "name1" and "name2" in it.

Any more comments? Acks ? I want to close this series :)

-- 
viresh

Re: [PATCH 01/16] PM / OPP: Add 'supply-names' binding

[Mark Brown]

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

On Sat, Oct 17, 2015 at 09:40:55AM +0530, Viresh Kumar wrote:

> Okay here is the problem statement:

> We have two supplies for a device and the device node will have
> something like:

> name1-supply = <&supply1>;
> name2-supply = <&supply2>;

> And the OPP node needs to have voltages for both of them:

> opp-microvolt = <X1 Y1 Z1>, <X2 Y2 Z2>;

> Where XYZ(1) are for supply1 and XYZ(2) are for supply2.

> Now we need to identify the supplies for which the values are present
> here and their order as well. How do we do that?

> The way I am suggesting is to add a property in opp node which will
> keep "name1" and "name2" in it.

When we start doing this we also start having to worry about things like
the sequencing of the updates between the various supplies and end up in
full on power sequencing (or at least baking some sequencing into the DT
which will doubtless need extending at some point).  I'm not sure that's
a place we want to end up just yet, I think it's safer to just have a
little bit of code in the kernel that glues things together in the cases
where this is needed.  

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

Re: [PATCH 01/16] PM / OPP: Add 'supply-names' binding

[Viresh Kumar]

On 23-10-15, 01:39, Mark Brown wrote:
> When we start doing this we also start having to worry about things like
> the sequencing of the updates between the various supplies and end up in
> full on power sequencing (or at least baking some sequencing into the DT
> which will doubtless need extending at some point).

Absolutely.

> I'm not sure that's
> a place we want to end up just yet, I think it's safer to just have a
> little bit of code in the kernel that glues things together in the cases
> where this is needed.  

So you are effectively saying that we shouldn't go ahead with multi
regulator support in OPP library, right?

I went ahead with it as it came as a requirement (specially from
Qcom).

To the problem of sequencing, maybe we can just support that for the
simple case, where supplies will be programmed in the order in which
they are present in the property I added in this patch. And not try to
solve problem for the complex cases, if we feel it is getting ugly.

@Stephen ?

-- 
viresh

Re: [PATCH 01/16] PM / OPP: Add 'supply-names' binding

[Mark Brown]

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

On Tue, Oct 27, 2015 at 01:49:17PM +0530, Viresh Kumar wrote:
> On 23-10-15, 01:39, Mark Brown wrote:

> > I'm not sure that's
> > a place we want to end up just yet, I think it's safer to just have a
> > little bit of code in the kernel that glues things together in the cases
> > where this is needed.  

> So you are effectively saying that we shouldn't go ahead with multi
> regulator support in OPP library, right?

Well, I think things like libraries for getting the data tables out of
DT are fine but I'm not convinced that trying to avoid having any device
specific code at all is sufficiently clear yet - as far as I know we're
mostly looking at a fairly small subset of devices still and with things
like sequencing in the mix it's a bit worrying to me to be putting it
all into an ABI intended to be used with no knowledge of the platform.

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

Re: [PATCH 01/16] PM / OPP: Add 'supply-names' binding

[Stephen Boyd]

On 10/28, Mark Brown wrote:
> On Tue, Oct 27, 2015 at 01:49:17PM +0530, Viresh Kumar wrote:
> > On 23-10-15, 01:39, Mark Brown wrote:
> 
> > > I'm not sure that's
> > > a place we want to end up just yet, I think it's safer to just have a
> > > little bit of code in the kernel that glues things together in the cases
> > > where this is needed.  
> 
> > So you are effectively saying that we shouldn't go ahead with multi
> > regulator support in OPP library, right?
> 
> Well, I think things like libraries for getting the data tables out of
> DT are fine but I'm not convinced that trying to avoid having any device
> specific code at all is sufficiently clear yet - as far as I know we're
> mostly looking at a fairly small subset of devices still and with things
> like sequencing in the mix it's a bit worrying to me to be putting it
> all into an ABI intended to be used with no knowledge of the platform.

Agreed. Looking at the current oppv2 binding I'm confused how it
even works. It shows cpu-supply = <&phandle1>, <&phandle2>, etc.
which doesn't work with the regulator bindings design. It seems
that Viresh figured that out when implementing support for
multiple regulators, so the binding was changed to put names in
the opp tables and then use that to get regulators in the opp
core. Urgh.

I think I understand the goal here though. The goal is to move
all the clk_get()/regulator_get() and clock and regulator API
interactions into the OPP framework so that we can say "go to OPP
at index 4" from the cpufreq core and the OPP framework takes
care of actually doing the transition. Given that doing such a
transition could be very machine specific, we probably ought to
make the OPP framework flexible enough to let us decide how to do
that.

Perhaps we need to expand on the compatible string in the opp
node to have compatible = "vendor,cool-transition",
"operating-points-v2"? Then we can plug in vendor specific
drivers that handle the frequency/voltage transition and they'll
know that the fourth column in the voltage entry corresponds to
this particular device's foo-supply and in what order to change
voltages. A generic driver could exist for the simple case of one
regulator and one clock that matches on "operating-points-v2".

Or we can avoid doing any clk_get()/regulator_get() stuff inside
the OPP framework, and let OPP consumers tell the OPP framework
about each clock and regulator it wants the framework to manage
on the consumer's behalf. It could even tell the framework which
regulator corresponds to which voltage column and in what order
to change the voltages so that the OPP framework doesn't need to
know these device specific details. Then the consumer can still
say go to OPP level 4 and that all works without introducing some
new DT ABI.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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 01/16] PM / OPP: Add 'supply-names' binding

[Mark Brown]

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

On Fri, Sep 11, 2015 at 05:31:57PM +0530, Viresh Kumar wrote:

> +- supply-names: This is a required property, only if multiple supplies are
> +  available for the device. Otherwise it is optional.
> +
> +  This list is used to pass names of all the device supplies. The order of names
> +  present here is important, as that should match the order in which values are
> +  present in 'opp-microvolt' and 'opp-microamp' properties.

If we were going to add something like this (which I'm really not that
thrilled about due to their encouragement of bad practice as previously
discussed) it seems wrong to add it at the level of a specific binding
rather than as a general facility.  I think I'm not entirely sold on
this use case though, I'll follow up to a later message in this
thread...

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

[PATCH 02/16] PM / OPP: Add 'opp-microvolt-triplets' binding

[Viresh Kumar]

If 'opp-microvolt' is used to specify values for multiple regulators,
then we need this additional information to know if the values passed
should be treated as <target> or <target min max>.

This is because, DT doesn't differentiate between these two styles:

prop = <x>, <y>, <z>;
prop = <x y z>;

Cc: devicetree@vger.kernel.org
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/devicetree/bindings/opp/opp.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index 8759bc4783ed..719603b87353 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -76,6 +76,16 @@ This describes the OPPs belonging to a device. This node can have following
   present here is important, as that should match the order in which values are
   present in 'opp-microvolt' and 'opp-microamp' properties.
 
+- opp-microvolt-triplets: This is a required property, only if multiple supplies
+  are available for the device. Otherwise it is ignored.
+
+  'opp-microvolt' can be present in two forms: <target> or <target min max>, per
+  power-supply. 'opp-microvolt-triplets' property is used to find the form in
+  which 'opp-microvolt' is present.
+
+  If present, then 'opp-microvolt' must be present in <target min max> form,
+  else in <target> form.
+
 - OPP nodes: One or more OPP nodes describing voltage-current-frequency
   combinations. Their name isn't significant but their phandle can be used to
   reference an OPP.
@@ -403,6 +413,8 @@ Example 4: Handling multiple regulators
 
 		/* OR */
 
+		opp-microvolt-triplets;
+
 		opp00 {
 			opp-hz = /bits/ 64 <1000000000>;
 			opp-microvolt = <970000 975000 985000>, /* Supply 0 */
@@ -416,6 +428,8 @@ Example 4: Handling multiple regulators
 
 		/* OR */
 
+		opp-microvolt-triplets;
+
 		opp00 {
 			opp-hz = /bits/ 64 <1000000000>;
 			opp-microvolt = <970000 975000 985000>, /* Supply 0 */
-- 
2.4.0

Re: [PATCH 02/16] PM / OPP: Add 'opp-microvolt-triplets' binding

[Rob Herring]

On 09/11/2015 07:01 AM, Viresh Kumar wrote:
> If 'opp-microvolt' is used to specify values for multiple regulators,
> then we need this additional information to know if the values passed
> should be treated as <target> or <target min max>.

Can't you determine this implicitly from # of cells / # of regulators
being either 1 or 3?

> 
> This is because, DT doesn't differentiate between these two styles:
> 
> prop = <x>, <y>, <z>;
> prop = <x y z>;
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  Documentation/devicetree/bindings/opp/opp.txt | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index 8759bc4783ed..719603b87353 100644
> --- a/Documentation/devicetree/bindings/opp/opp.txt
> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> @@ -76,6 +76,16 @@ This describes the OPPs belonging to a device. This node can have following
>    present here is important, as that should match the order in which values are
>    present in 'opp-microvolt' and 'opp-microamp' properties.
>  
> +- opp-microvolt-triplets: This is a required property, only if multiple supplies
> +  are available for the device. Otherwise it is ignored.
> +
> +  'opp-microvolt' can be present in two forms: <target> or <target min max>, per
> +  power-supply. 'opp-microvolt-triplets' property is used to find the form in
> +  which 'opp-microvolt' is present.
> +
> +  If present, then 'opp-microvolt' must be present in <target min max> form,
> +  else in <target> form.

This implies that bindings currently with triplets are wrong since they
will be missing opp-microvolt-triplets.

Rob

Re: [PATCH 02/16] PM / OPP: Add 'opp-microvolt-triplets' binding

[Viresh Kumar]

[+Cc Mark, I thought I cc'd him earlier, but no, I cc'd him only for
the first patch]

On 14-09-15, 15:30, Rob Herring wrote:
> On 09/11/2015 07:01 AM, Viresh Kumar wrote:
> > If 'opp-microvolt' is used to specify values for multiple regulators,
> > then we need this additional information to know if the values passed
> > should be treated as <target> or <target min max>.
> 
> Can't you determine this implicitly from # of cells / # of regulators
> being either 1 or 3?

I thought the #<name>-cells property is used to pass arguments along with the
phandle, so something like this:

supply0: regulator@f8000000 {
        regulator-cells = 1;
        ...
}

cpu@1 {
        cpu-supply = <&supply0 XYZ>
}

But if we can define something like:

supply0: regulator@f8000000 {
        regulator-cells or microvolt-cells = 1 or 3;
        ...
}

And then do:

cpu@1 {
        cpu-supply = <&supply0>
        operating-points-v2 = "&opp-table";
}

opp-table: table {

        ...

        opp0 {
                opp-hz = ...;
                ...
                opp-microvolt = <one or three values here>
        }
}

then it will be very simple. Also, this would mean that with multiple
regulators, we can have one regulator supporting single microvolts
value and other supporting tar/min/max values..

> > 
> > This is because, DT doesn't differentiate between these two styles:
> > 
> > prop = <x>, <y>, <z>;
> > prop = <x y z>;
> > 
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/opp/opp.txt | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> > index 8759bc4783ed..719603b87353 100644
> > --- a/Documentation/devicetree/bindings/opp/opp.txt
> > +++ b/Documentation/devicetree/bindings/opp/opp.txt
> > @@ -76,6 +76,16 @@ This describes the OPPs belonging to a device. This node can have following
> >    present here is important, as that should match the order in which values are
> >    present in 'opp-microvolt' and 'opp-microamp' properties.
> >  
> > +- opp-microvolt-triplets: This is a required property, only if multiple supplies
> > +  are available for the device. Otherwise it is ignored.
> > +
> > +  'opp-microvolt' can be present in two forms: <target> or <target min max>, per
> > +  power-supply. 'opp-microvolt-triplets' property is used to find the form in
> > +  which 'opp-microvolt' is present.
> > +
> > +  If present, then 'opp-microvolt' must be present in <target min max> form,
> > +  else in <target> form.
> 
> This implies that bindings currently with triplets are wrong since they
> will be missing opp-microvolt-triplets.

Yeah, the bindings are incomplete, but there are no users yet, which
need the triplet thing. So, its all working just fine.

-- 
viresh

Re: [PATCH 02/16] PM / OPP: Add 'opp-microvolt-triplets' binding

[Mark Brown]

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

On Tue, Sep 15, 2015 at 09:00:27AM +0530, Viresh Kumar wrote:

> [+Cc Mark, I thought I cc'd him earlier, but no, I cc'd him only for
> the first patch]

I'm reading this on a plane so have no other context and to be honest
I'm struggling to understand what is being discussed here.  It would be
really helpful if you were to describe in words what proposed bindings
are intended to do as well as presenting examples, the examples by
themselves require the reader to reverse engineer what the semantics are
intended to be.

> But if we can define something like:

> supply0: regulator@f8000000 {
>         regulator-cells or microvolt-cells = 1 or 3;
>         ...
> }

As far as I can tell this is proposing adding something to the regulator
binding specifying if users must present either a single value or a
min/target/max triplet.  This is obviously problematic since regulators
can be shared - the needs of one user may not match the needs of another
user, and of course most users should not be specifying voltages at all
in the device tree in the first place.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]