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

Groups > linux.kernel > #1487947 > unrolled thread

[PATCH 1/2] PM / OPP: compatible is an optional property

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] PM / OPP: compatible is an optional property Viresh Kumar <viresh.kumar@linaro.org> - 2016-09-21 11:40 +0200
    [PATCH 2/2] PM / OPP: Multiple regulators aren't supported yet Viresh Kumar <viresh.kumar@linaro.org> - 2016-09-21 11:40 +0200
      Re: [PATCH 2/2] PM / OPP: Multiple regulators aren't supported yet Rob Herring <robh@kernel.org> - 2016-09-23 22:00 +0200
        Re: [PATCH 2/2] PM / OPP: Multiple regulators aren't supported yet Viresh Kumar <viresh.kumar@linaro.org> - 2016-09-26 07:10 +0200
    Re: [PATCH 1/2] PM / OPP: compatible is an optional property Stephen Boyd <sboyd@codeaurora.org> - 2016-09-22 21:30 +0200
      Re: [PATCH 1/2] PM / OPP: compatible is an optional property Viresh Kumar <viresh.kumar@linaro.org> - 2016-09-23 07:20 +0200
        Re: [PATCH 1/2] PM / OPP: compatible is an optional property Rob Herring <robh@kernel.org> - 2016-09-23 22:00 +0200
          Re: [PATCH 1/2] PM / OPP: compatible is an optional property Viresh Kumar <viresh.kumar@linaro.org> - 2016-09-26 07:00 +0200
      [PATCH V2 1/2] PM / OPP: compatible string is not optional Viresh Kumar <viresh.kumar@linaro.org> - 2016-09-23 08:50 +0200

#1487947 — [PATCH 1/2] PM / OPP: compatible is an optional property

It was never compulsory to have a compatible string in the OPP table.
Fix the documentation to mark it optional.

Also update its description a bit.

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

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index ee91cbdd95ee..5eab6f0215d1 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -55,14 +55,14 @@ This describes the OPPs belonging to a device. This node can have following
 properties:
 
 Required properties:
-- compatible: Allow OPPs to express their compatibility. It should be:
-  "operating-points-v2".
-
 - 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.
 
 Optional properties:
+- compatible: Allow OPPs to express their compatibility. It should be
+  "operating-points-v2" or a vendor specific string.
+
 - opp-shared: Indicates that device nodes using this OPP Table Node's phandle
   switch their DVFS state together, i.e. they share clock/voltage/current lines.
   Missing property means devices have independent clock/voltage/current lines,
-- 
2.7.1.410.g6faf27b

[toc] | [next] | [standalone]

#1487948 — [PATCH 2/2] PM / OPP: Multiple regulators aren't supported yet

Multiple regulators per device aren't supported yet by the kernel code
and the bindings provided in documentation aren't sufficient to handle
that case (as there is no way for kernel code to link multiple
voltage/current values to a power supply).

Remove them. These can be added later if required with appropriate
changes.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/devicetree/bindings/opp/opp.txt | 68 ++-------------------------
 1 file changed, 4 insertions(+), 64 deletions(-)

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index 5eab6f0215d1..5d0595f0a10c 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -86,8 +86,7 @@ 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 aren't supported.
 
 - 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 +103,7 @@ 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 aren't supported.
 
 - opp-microamp-<name>: Named opp-microamp property. Similar to
   opp-microvolt-<name> property, but for microamp instead.
@@ -381,63 +377,7 @@ DVFS state together.
 	};
 };
 
-Example 4: Handling multiple regulators
-
-/ {
-	cpus {
-		cpu@0 {
-			compatible = "arm,cortex-a7";
-			...
-
-			cpu-supply = <&cpu_supply0>, <&cpu_supply1>, <&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 */
-			opp-microamp =  <70000>,  /* Supply 0 */
-					<70000>,  /* Supply 1 */
-					<70000>;  /* Supply 2 */
-			clock-latency-ns = <300000>;
-		};
-
-		/* OR */
-
-		opp@1000000000 {
-			opp-hz = /bits/ 64 <1000000000>;
-			opp-microvolt = <970000 975000 985000>, /* Supply 0 */
-					<960000 965000 975000>, /* Supply 1 */
-					<960000 965000 975000>; /* Supply 2 */
-			opp-microamp =  <70000>,		/* Supply 0 */
-					<70000>,		/* Supply 1 */
-					<70000>;		/* Supply 2 */
-			clock-latency-ns = <300000>;
-		};
-
-		/* OR */
-
-		opp@1000000000 {
-			opp-hz = /bits/ 64 <1000000000>;
-			opp-microvolt = <970000 975000 985000>, /* Supply 0 */
-					<960000 965000 975000>, /* Supply 1 */
-					<960000 965000 975000>; /* Supply 2 */
-			opp-microamp =  <70000>,		/* Supply 0 */
-					<0>,			/* Supply 1 doesn't need this */
-					<70000>;		/* Supply 2 */
-			clock-latency-ns = <300000>;
-		};
-	};
-};
-
-Example 5: opp-supported-hw
+Example 4: opp-supported-hw
 (example: three level hierarchy of versions: cuts, substrate and process)
 
 / {
@@ -482,7 +422,7 @@ Example 5: opp-supported-hw
 	};
 };
 
-Example 6: opp-microvolt-<name>, opp-microamp-<name>:
+Example 5: opp-microvolt-<name>, opp-microamp-<name>:
 (example: device with two possible microvolt ranges: slow and fast)
 
 / {
-- 
2.7.1.410.g6faf27b

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

#1490422 — Re: [PATCH 2/2] PM / OPP: Multiple regulators aren't supported yet

On Wed, Sep 21, 2016 at 03:02:50PM +0530, Viresh Kumar wrote:
> Multiple regulators per device aren't supported yet by the kernel code
> and the bindings provided in documentation aren't sufficient to handle
> that case (as there is no way for kernel code to link multiple
> voltage/current values to a power supply).

What do you mean? Because the supplies are in the cpu node?

Rob

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

#1491070 — Re: [PATCH 2/2] PM / OPP: Multiple regulators aren't supported yet

On 23-09-16, 14:59, Rob Herring wrote:
> On Wed, Sep 21, 2016 at 03:02:50PM +0530, Viresh Kumar wrote:
> > Multiple regulators per device aren't supported yet by the kernel code
> > and the bindings provided in documentation aren't sufficient to handle
> > that case (as there is no way for kernel code to link multiple
> > voltage/current values to a power supply).
> 
> What do you mean? Because the supplies are in the cpu node?

Not just that. For example if two supplies are present in CPU node like:
ABC-supply and XYZ-supply. And the values present in OPP node are like:

               opp00 {
                       opp-hz = /bits/ 64 <1700000000>;
                        /*                     ABC supply                XYZ        supply */
                       opp-microvolt = <1300000 1300000 1300000>, <1400000 1400000 1400000>;
                       opp-microamp = <70000>;
                       clock-latency-ns = <30>;
                       opp-suspend;
               };


Then the code can get regulators with those names (ABC and XYZ), but it can't
figure out which triplet in the "opp-microvolt" property belongs to which
supply.

I am working on some solution to that, but wanted to keep the entire series self
sufficient and so removing the leftovers first in this patch.

-- 
viresh

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

#1489491

On 09/21/2016 02:32 AM, Viresh Kumar wrote:
> It was never compulsory to have a compatible string in the OPP table.
> Fix the documentation to mark it optional.
>
> Also update its description a bit.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Why? I'd prefer the compatible string to be required so we know what
sort of node it is.

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

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

#1489754

On 22-09-16, 12:24, Stephen Boyd wrote:
> On 09/21/2016 02:32 AM, Viresh Kumar wrote:
> > It was never compulsory to have a compatible string in the OPP table.
> > Fix the documentation to mark it optional.
> >
> > Also update its description a bit.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> 
> Why? I'd prefer the compatible string to be required so we know what
> sort of node it is.

Okay, the code doesn't have any checks for it then and that needs to be fixed.

Just for my clarity, for platforms with special OPP bindings and so a different
compatible string like: "operating-points-v2-XYZ", should the compatible string
contain both "operating-points-v2" and the above one? It would be easier to
check for "operating-points-v2" in that case from core code.

-- 
viresh

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

#1490419

On Fri, Sep 23, 2016 at 10:45:26AM +0530, Viresh Kumar wrote:
> On 22-09-16, 12:24, Stephen Boyd wrote:
> > On 09/21/2016 02:32 AM, Viresh Kumar wrote:
> > > It was never compulsory to have a compatible string in the OPP table.
> > > Fix the documentation to mark it optional.
> > >

NAK.

> > > Also update its description a bit.
> > >
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > 
> > Why? I'd prefer the compatible string to be required so we know what
> > sort of node it is.

Agreed.

> Okay, the code doesn't have any checks for it then and that needs to be fixed.

Why? The kernel is not a DT validator.
 
> Just for my clarity, for platforms with special OPP bindings and so a different
> compatible string like: "operating-points-v2-XYZ", should the compatible string
> contain both "operating-points-v2" and the above one? It would be easier to
> check for "operating-points-v2" in that case from core code.

That would imply operating-points-v2-XYZ has extra properties or is 
different in some way. If an OS only understanding operating-points-v2 
will work, then yes it should have both. If not, then no.

Rob

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

#1491068

On 23-09-16, 14:55, Rob Herring wrote:
> On Fri, Sep 23, 2016 at 10:45:26AM +0530, Viresh Kumar wrote:
> > On 22-09-16, 12:24, Stephen Boyd wrote:
> > > On 09/21/2016 02:32 AM, Viresh Kumar wrote:
> > > > It was never compulsory to have a compatible string in the OPP table.
> > > > Fix the documentation to mark it optional.
> > > >
> 
> NAK.
> 
> > > > Also update its description a bit.
> > > >
> > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > > ---
> > > 
> > > Why? I'd prefer the compatible string to be required so we know what
> > > sort of node it is.
> 
> Agreed.
> 
> > Okay, the code doesn't have any checks for it then and that needs to be fixed.
> 
> Why? The kernel is not a DT validator.

Hmm.. I thought it should be checking if it can parse those bindings or not.
What if someone adds compatible property as "foo" for OPP node? Should the OPP
code even try to parse it?

> > Just for my clarity, for platforms with special OPP bindings and so a different
> > compatible string like: "operating-points-v2-XYZ", should the compatible string
> > contain both "operating-points-v2" and the above one? It would be easier to
> > check for "operating-points-v2" in that case from core code.
> 
> That would imply operating-points-v2-XYZ has extra properties or is 
> different in some way. If an OS only understanding operating-points-v2 
> will work, then yes it should have both. If not, then no.

Well, in this case that can't be done fully, so we should have only the -xyz
one. Got it, thanks.

-- 
viresh

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

#1489788 — [PATCH V2 1/2] PM / OPP: compatible string is not optional

The OPP core code doesn't check for the compatible string today and any
faulty binding will go unnoticed.

Add a check in OPP core to take care of that.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
This patch replaces: "[PATCH 1/2] PM / OPP: compatible is an optional
property".

FWIW, I have checked that none of the current users of
operating-points-v2 had this problem and this patch can be merged
safely.

 drivers/base/power/opp/of.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index 1dfd3dd92624..111a5cad9f8a 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -318,6 +318,11 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 	struct opp_table *opp_table;
 	int ret = 0, count = 0;
 
+	if (!of_device_is_compatible(opp_np, "operating-points-v2")) {
+		dev_err(dev, "Not compatible with operating-points-v2\n");
+		return -EINVAL;
+	}
+
 	mutex_lock(&opp_table_lock);
 
 	opp_table = _managed_opp(opp_np);
-- 
2.7.1.410.g6faf27b

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web