OPP: Allow power/current values without voltage

PATCH New Viresh Kumar 2 participants 13 messages Nov 3, 2022 – Nov 4, 2022

Subsystems (2)

Device tree bindings Operating performance points (OPP)

lore

Share & tools

lore

Viresh Kumar

author

2022-11-03 11:01 UTC #0 lore raw

cover OPP: Allow power/current values without voltage

Hello,

Some platforms, such as Apple Silicon, do not describe their device's voltage regulators in the DT as they cannot be controlled by the kernel and/or rely on opaque firmware algorithms to control their voltage and current characteristics at runtime. They can, however, experimentally determine the power consumption of a given device at a given OPP, taking advantage of opp-microwatt to provide EAS on such devices as was initially intended.

But the OPP core currently doesn't parse the opp-microwatt property if opp-microvolt isn't present. This patch series targets to change this approach.

This first fixes few mistakes in the DT bindings, followed by code reorganization. And the last commit, from James, fixes the problem at hand.

I have tested all combinations on my Hikey board, hope it doesn't break anything.

James Calligeros (1):
OPP: decouple dt properties in opp_parse_supplies()

Viresh Kumar (4):
dt-bindings: opp: Fix usage of current in microwatt property
dt-bindings: opp: Fix named microwatt property
OPP: Parse named opp-microwatt property too
OPP: Simplify opp_parse_supplies() by restructuring it

 .../devicetree/bindings/opp/opp-v2-base.yaml  |   6 +-
 drivers/opp/of.c                              | 228 ++++++++----------
 2 files changed, 102 insertions(+), 132 deletions(-)

Signature · 1 line

2.31.1.272.g89b43f80a514

Viresh Kumar

author

2022-11-03 11:01 UTC #1 lore raw

PATCH 1/5 dt-bindings: opp: Fix usage of current in microwatt property

The bindings mentions "current" instead of "power". Fix it.

Signed-off-by Viresh Kumar <[email protected]>

---
 Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Documentation/devicetree/bindings/opp/opp-v2-base.yaml +1 -1

index 66d0ec763f0b..cb025b0a346d 100644
@@ -108,7 +108,7 @@ select: false
           The power for the OPP in micro-Watts.
 
           Entries for multiple regulators shall be provided in the same field
-          separated by angular brackets <>. If current values aren't required
+          separated by angular brackets <>. If power values aren't required
           for a regulator, then it shall be filled with 0. If power values
           aren't required for any of the regulators, then this field is not
           required. The OPP binding doesn't provide any provisions to relate the

Signature · 1 line

2.31.1.272.g89b43f80a514

Viresh Kumar

author

2022-11-03 11:01 UTC #2 lore raw

PATCH 2/5 dt-bindings: opp: Fix named microwatt property

In reply to Viresh Kumar

The named microwatt-<name> property should look exactly like microvolt and microamp properties. There were some differences, fix them.

Signed-off-by Viresh Kumar <[email protected]>

---
 Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Documentation/devicetree/bindings/opp/opp-v2-base.yaml +2 -2

index cb025b0a346d..cf9c2f7bddc2 100644
@@ -230,9 +230,9 @@ select: false
         minItems: 1
         maxItems: 8   # Should be enough regulators
 
-      '^opp-microwatt':
+      '^opp-microwatt-':
         description:
-          Named opp-microwatt property. Similar to opp-microamp property,
+          Named opp-microwatt property. Similar to opp-microamp-<name> property,
           but for microwatt instead.
         $ref: /schemas/types.yaml#/definitions/uint32-array
         minItems: 1

Signature · 1 line

2.31.1.272.g89b43f80a514

Viresh Kumar

author

2022-11-03 11:01 UTC #3 lore raw

PATCH 3/5 OPP: Parse named opp-microwatt property too

In reply to Viresh Kumar

We missed parsing the named opp-microwatt-<name> property, fix that.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/opp/of.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

drivers/opp/of.c +13 -3

index 605d68673f92..e010e119c42b 100644
@@ -695,9 +695,19 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 		}
 	}
 
-	/* Search for "opp-microwatt" */
-	sprintf(name, "opp-microwatt");
-	prop = of_find_property(opp->np, name, NULL);
+	/* Search for "opp-microwatt-<name>" */
+	prop = NULL;
+	if (opp_table->prop_name) {
+		snprintf(name, sizeof(name), "opp-microwatt-%s",
+			 opp_table->prop_name);
+		prop = of_find_property(opp->np, name, NULL);
+	}
+
+	if (!prop) {
+		/* Search for "opp-microwatt" */
+		sprintf(name, "opp-microwatt");
+		prop = of_find_property(opp->np, name, NULL);
+	}
 
 	if (prop) {
 		pcount = of_property_count_u32_elems(opp->np, name);

Signature · 1 line

2.31.1.272.g89b43f80a514

Viresh Kumar

author

2022-11-03 11:01 UTC #4 lore raw

PATCH 4/5 OPP: Simplify opp_parse_supplies() by restructuring it

In reply to Viresh Kumar

opp_parse_supplies() has grown into too big of a routine (~190 lines) and it is not straight-forward to understand it anymore.

Break it into smaller routines and reduce code redundancy a bit by using the same code to parse properties.

This shouldn't result in any logical changes.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/opp/of.c | 216 ++++++++++++++++++-----------------------------
 1 file changed, 81 insertions(+), 135 deletions(-)

drivers/opp/of.c +81 ~~-135~~

index e010e119c42b..e51c43495e21 100644
@@ -578,179 +578,126 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
 	return false;
 }
 
-static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
-			      struct opp_table *opp_table)
+static u32 *_parse_named_prop(struct dev_pm_opp *opp, struct device *dev,
+			      struct opp_table *opp_table,
+			      const char *prop_type, bool *triplet)
 {
-	u32 *microvolt, *microamp = NULL, *microwatt = NULL;
-	int supplies = opp_table->regulator_count;
-	int vcount, icount, pcount, ret, i, j;
 	struct property *prop = NULL;
 	char name[NAME_MAX];
+	int count, ret;
+	u32 *out;
 
-	/* Search for "opp-microvolt-<name>" */
+	/* Search for "opp-<prop_type>-<name>" */
 	if (opp_table->prop_name) {
-		snprintf(name, sizeof(name), "opp-microvolt-%s",
+		snprintf(name, sizeof(name), "opp-%s-%s", prop_type,
 			 opp_table->prop_name);
 		prop = of_find_property(opp->np, name, NULL);
 	}
 
 	if (!prop) {
-		/* Search for "opp-microvolt" */
-		sprintf(name, "opp-microvolt");
+		/* Search for "opp-<prop_type>" */
+		snprintf(name, sizeof(name), "opp-%s", prop_type);
 		prop = of_find_property(opp->np, name, NULL);
-
-		/* Missing property isn't a problem, but an invalid entry is */
-		if (!prop) {
-			if (unlikely(supplies == -1)) {
-				/* Initialize regulator_count */
-				opp_table->regulator_count = 0;
-				return 0;
-			}
-
-			if (!supplies)
-				return 0;
-
-			dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n",
-				__func__);
-			return -EINVAL;
-		}
-	}
-
-	if (unlikely(supplies == -1)) {
-		/* Initialize regulator_count */
-		supplies = opp_table->regulator_count = 1;
-	} else if (unlikely(!supplies)) {
-		dev_err(dev, "%s: opp-microvolt wasn't expected\n", __func__);
-		return -EINVAL;
+		if (!prop)
+			return NULL;
 	}
 
-	vcount = of_property_count_u32_elems(opp->np, name);
-	if (vcount < 0) {
-		dev_err(dev, "%s: Invalid %s property (%d)\n",
-			__func__, name, vcount);
-		return vcount;
+	count = of_property_count_u32_elems(opp->np, name);
+	if (count < 0) {
+		dev_err(dev, "%s: Invalid %s property (%d)\n", __func__, name,
+			count);
+		return ERR_PTR(count);
 	}
 
-	/* There can be one or three elements per supply */
-	if (vcount != supplies && vcount != supplies * 3) {
-		dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n",
-			__func__, name, vcount, supplies);
-		return -EINVAL;
+	/*
+	 * Initialize regulator_count, if regulator information isn't provided
+	 * by the platform. Now that one of the properties is available, fix the
+	 * regulator_count to 1.
+	 */
+	if (unlikely(opp_table->regulator_count == -1))
+		opp_table->regulator_count = 1;
+
+	if (count != opp_table->regulator_count &&
+	    (!triplet || count != opp_table->regulator_count * 3)) {
+		dev_err(dev, "%s: Invalid number of elements in %s property (%u) with supplies (%d)\n",
+			__func__, prop_type, count, opp_table->regulator_count);
+		return ERR_PTR(-EINVAL);
 	}
 
-	microvolt = kmalloc_array(vcount, sizeof(*microvolt), GFP_KERNEL);
-	if (!microvolt)
-		return -ENOMEM;
+	out = kmalloc_array(count, sizeof(*out), GFP_KERNEL);
+	if (!out)
+		return ERR_PTR(-EINVAL);
 
-	ret = of_property_read_u32_array(opp->np, name, microvolt, vcount);
+	ret = of_property_read_u32_array(opp->np, name, out, count);
 	if (ret) {
 		dev_err(dev, "%s: error parsing %s: %d\n", __func__, name, ret);
-		ret = -EINVAL;
-		goto free_microvolt;
+		kfree(out);
+		return ERR_PTR(-EINVAL);
 	}
 
-	/* Search for "opp-microamp-<name>" */
-	prop = NULL;
-	if (opp_table->prop_name) {
-		snprintf(name, sizeof(name), "opp-microamp-%s",
-			 opp_table->prop_name);
-		prop = of_find_property(opp->np, name, NULL);
-	}
+	if (triplet)
+		*triplet = count != opp_table->regulator_count;
 
-	if (!prop) {
-		/* Search for "opp-microamp" */
-		sprintf(name, "opp-microamp");
-		prop = of_find_property(opp->np, name, NULL);
-	}
-
-	if (prop) {
-		icount = of_property_count_u32_elems(opp->np, name);
-		if (icount < 0) {
-			dev_err(dev, "%s: Invalid %s property (%d)\n", __func__,
-				name, icount);
-			ret = icount;
-			goto free_microvolt;
-		}
+	return out;
+}
 
-		if (icount != supplies) {
-			dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n",
-				__func__, name, icount, supplies);
-			ret = -EINVAL;
-			goto free_microvolt;
-		}
+static u32 *opp_parse_microvolt(struct dev_pm_opp *opp, struct device *dev,
+				struct opp_table *opp_table, bool *triplet)
+{
+	u32 *microvolt;
 
-		microamp = kmalloc_array(icount, sizeof(*microamp), GFP_KERNEL);
-		if (!microamp) {
-			ret = -EINVAL;
-			goto free_microvolt;
-		}
+	microvolt = _parse_named_prop(opp, dev, opp_table, "microvolt", triplet);
+	if (IS_ERR(microvolt))
+		return microvolt;
 
-		ret = of_property_read_u32_array(opp->np, name, microamp,
-						 icount);
-		if (ret) {
-			dev_err(dev, "%s: error parsing %s: %d\n", __func__,
-				name, ret);
-			ret = -EINVAL;
-			goto free_microamp;
+	if (!microvolt) {
+		/*
+		 * Missing property isn't a problem, but an invalid
+		 * entry is. This property isn't optional if regulator
+		 * information is provided.
+		 */
+		if (opp_table->regulator_count > 0) {
+			dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n",
+				__func__);
+			return ERR_PTR(-EINVAL);
 		}
 	}
 
-	/* Search for "opp-microwatt-<name>" */
-	prop = NULL;
-	if (opp_table->prop_name) {
-		snprintf(name, sizeof(name), "opp-microwatt-%s",
-			 opp_table->prop_name);
-		prop = of_find_property(opp->np, name, NULL);
-	}
-
-	if (!prop) {
-		/* Search for "opp-microwatt" */
-		sprintf(name, "opp-microwatt");
-		prop = of_find_property(opp->np, name, NULL);
-	}
+	return microvolt;
+}
 
-	if (prop) {
-		pcount = of_property_count_u32_elems(opp->np, name);
-		if (pcount < 0) {
-			dev_err(dev, "%s: Invalid %s property (%d)\n", __func__,
-				name, pcount);
-			ret = pcount;
-			goto free_microamp;
-		}
+static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
+			      struct opp_table *opp_table)
+{
+	u32 *microvolt, *microamp, *microwatt;
+	int ret, i, j;
+	bool triplet;
 
-		if (pcount != supplies) {
-			dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n",
-				__func__, name, pcount, supplies);
-			ret = -EINVAL;
-			goto free_microamp;
-		}
+	microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet);
+	if (IS_ERR_OR_NULL(microvolt))
+		return PTR_ERR(microvolt);
 
-		microwatt = kmalloc_array(pcount, sizeof(*microwatt),
-					  GFP_KERNEL);
-		if (!microwatt) {
-			ret = -EINVAL;
-			goto free_microamp;
-		}
+	microamp = _parse_named_prop(opp, dev, opp_table, "microamp", NULL);
+	if (IS_ERR(microamp)) {
+		ret = PTR_ERR(microamp);
+		goto free_microvolt;
+	}
 
-		ret = of_property_read_u32_array(opp->np, name, microwatt,
-						 pcount);
-		if (ret) {
-			dev_err(dev, "%s: error parsing %s: %d\n", __func__,
-				name, ret);
-			ret = -EINVAL;
-			goto free_microwatt;
-		}
+	microwatt = _parse_named_prop(opp, dev, opp_table, "microwatt", NULL);
+	if (IS_ERR(microwatt)) {
+		ret = PTR_ERR(microwatt);
+		goto free_microamp;
 	}
 
-	for (i = 0, j = 0; i < supplies; i++) {
+	for (i = 0, j = 0; i < opp_table->regulator_count; i++) {
 		opp->supplies[i].u_volt = microvolt[j++];
 
-		if (vcount == supplies) {
-			opp->supplies[i].u_volt_min = opp->supplies[i].u_volt;
-			opp->supplies[i].u_volt_max = opp->supplies[i].u_volt;
-		} else {
+		if (triplet) {
 			opp->supplies[i].u_volt_min = microvolt[j++];
 			opp->supplies[i].u_volt_max = microvolt[j++];
+		} else {
+			opp->supplies[i].u_volt_min = opp->supplies[i].u_volt;
+			opp->supplies[i].u_volt_max = opp->supplies[i].u_volt;
 		}
 
 		if (microamp)

@@ -760,7 +707,6 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 			opp->supplies[i].u_watt = microwatt[i];
 	}
 
-free_microwatt:
 	kfree(microwatt);
 free_microamp:
 	kfree(microamp);

Signature · 1 line

2.31.1.272.g89b43f80a514

Viresh Kumar

author

2022-11-03 11:02 UTC #5 lore raw

PATCH v2 5/5 OPP: decouple dt properties in opp_parse_supplies()

In reply to Viresh Kumar

From: James Calligeros <[email protected]>

The opp-microwatt property was added with the intention of providing platforms a way to specify a precise value for the power consumption of a device at a given OPP to enable better energy-aware scheduling decisions by informing the kernel of the total static and dynamic power of a device at a given OPP, removing the reliance on the EM subsystem's often flawed estimations. This property is parsed by opp_parse_supplies(), which creates a hard dependency on the opp-microvolt property.

Some platforms, such as Apple Silicon, do not describe their device's voltage regulators in the DT as they cannot be controlled by the kernel and/or rely on opaque firmware algorithms to control their voltage and current characteristics at runtime. We can, however, experimentally determine the power consumption of a given device at a given OPP, taking advantage of opp-microwatt to provide EAS on such devices as was initially intended.

Allow platforms to specify and consume any subset of opp-microvolt, opp-microamp, or opp-microwatt without a hard dependency on opp-microvolt to enable this functionality on such platforms.

Signed-off-by James Calligeros <[email protected]>

Co-developed-by Viresh Kumar <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
V2: Rewritten by Viresh on top of his changes.

 drivers/opp/of.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

drivers/opp/of.c +25 ~~-11~~

index e51c43495e21..273fa9c0e1c0 100644
@@ -654,9 +654,12 @@ static u32 *opp_parse_microvolt(struct dev_pm_opp *opp, struct device *dev,
 		/*
 		 * Missing property isn't a problem, but an invalid
 		 * entry is. This property isn't optional if regulator
-		 * information is provided.
+		 * information is provided. Check only for the first OPP, as
+		 * regulator_count may get initialized after that to a valid
+		 * value.
 		 */
-		if (opp_table->regulator_count > 0) {
+		if (list_empty(&opp_table->opp_list) &&
+		    opp_table->regulator_count > 0) {
 			dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n",
 				__func__);
 			return ERR_PTR(-EINVAL);

@@ -674,7 +677,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 	bool triplet;
 
 	microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet);
-	if (IS_ERR_OR_NULL(microvolt))
+	if (IS_ERR(microvolt))
 		return PTR_ERR(microvolt);
 
 	microamp = _parse_named_prop(opp, dev, opp_table, "microamp", NULL);

@@ -689,15 +692,26 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 		goto free_microamp;
 	}
 
-	for (i = 0, j = 0; i < opp_table->regulator_count; i++) {
-		opp->supplies[i].u_volt = microvolt[j++];
+	/*
+	 * Initialize regulator_count if it is uninitialized and no properties
+	 * are found.
+	 */
+	if (unlikely(opp_table->regulator_count == -1)) {
+		opp_table->regulator_count = 0;
+		return 0;
+	}
 
-		if (triplet) {
-			opp->supplies[i].u_volt_min = microvolt[j++];
-			opp->supplies[i].u_volt_max = microvolt[j++];
-		} else {
-			opp->supplies[i].u_volt_min = opp->supplies[i].u_volt;
-			opp->supplies[i].u_volt_max = opp->supplies[i].u_volt;
+	for (i = 0, j = 0; i < opp_table->regulator_count; i++) {
+		if (microvolt) {
+			opp->supplies[i].u_volt = microvolt[j++];
+
+			if (triplet) {
+				opp->supplies[i].u_volt_min = microvolt[j++];
+				opp->supplies[i].u_volt_max = microvolt[j++];
+			} else {
+				opp->supplies[i].u_volt_min = opp->supplies[i].u_volt;
+				opp->supplies[i].u_volt_max = opp->supplies[i].u_volt;
+			}
 		}
 
 		if (microamp)

Signature · 1 line

2.31.1.272.g89b43f80a514

James Calligeros

Tested-by on v2 5/5

2022-11-03 12:25 UTC #6 lore raw

Re: [PATCH V2 5/5] OPP: decouple dt properties in opp_parse_supplies()

drivers/opp/of.c static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, · line 681

Quoted 7 lines · AEST Viresh Kumar

drivers/opp/of.c +1 -1

@@ -674,7 +677,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 	bool triplet;
 
 	microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet);
-	if (IS_ERR_OR_NULL(microvolt))
+	if (IS_ERR(microvolt))
 		return PTR_ERR(microvolt);

Erroring out here will still block EM registration on platforms without the opp-microvolt prop so we're back to square one, which means the patch does not do what the description says it does. It behaves almost identically to the current code.

I assume this is an intentional choice given the comments in opp_parse_microvolt(), so the commit message should drop references to fixing such platforms.

If this is a hard sticking point and opp_parse_supplies() must return a regulator with opp-microvolt, then I am of the opinion that the parsing of opp-microwatt should be detangled entirely from the regulator infrastructure.

Otherwise for the whole series,

Tested-by James Calligeros <[email protected]>

- James

Viresh Kumar

author

2022-11-03 13:11 UTC #7 lore raw

Re: [PATCH V2 5/5] OPP: decouple dt properties in opp_parse_supplies()

Quoted 5 lines · James Calligeros

Quoted 3 lines · AEST Viresh Kumar

drivers/opp/of.c +0 -0

@@ -674,7 +677,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 	bool triplet;

Quoted 9 lines

drivers/opp/of.c static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, · line 681
Quoted 4 lines
drivers/opp/of.c +1 -1
 	microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet);
-	if (IS_ERR_OR_NULL(microvolt))
+	if (IS_ERR(microvolt))
 		return PTR_ERR(microvolt);
Erroring out here will still block EM registration on platforms without the opp-microvolt prop so we're back to square one, which means the patch does not do what the description says it does. It behaves almost identically to the current code.

I am confused.

With the current code, the following will work:
- all three available
- microvolt available and nothing else.
- only microamp available
- only microwatt available
- both microamp and microwatt available but no microvolt
- and other combinations

Isn't this all we want ?

We error out here when there is a problem with DT entries, i.e. incorrect entries, etc. Else we will get NULL here and we continue as we don't check for IS_ERR_OR_NULL() anymore after this patch.

Quoted 4 lines

I assume this is an intentional choice given the comments in opp_parse_microvolt(), so the commit message should drop references to fixing such platforms.

Quoted 8 lines

If this is a hard sticking point and opp_parse_supplies() must return a regulator with opp-microvolt, then I am of the opinion that the parsing of opp-microwatt should be detangled entirely from the regulator infrastructure.
Otherwise for the whole series,
Tested-by James Calligeros <[email protected]>

What did you test exactly ? As I thought you will be testing the only microwatt case here and you say it won't work.

Sorry, I just got a little bit confused :(

Signature · 1 line

viresh

James Calligeros

2022-11-03 15:23 UTC #8 lore raw

Re: [PATCH V2 5/5] OPP: decouple dt properties in opp_parse_supplies()

Quoted 26 lines · AEST Viresh Kumar

Quoted 5 lines · James Calligeros
Quoted 3 lines · AEST Viresh Kumar
drivers/opp/of.c +0 -0
@@ -674,7 +677,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 	bool triplet;
 
Quoted 9 lines
drivers/opp/of.c static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, · line 681
Quoted 4 lines
drivers/opp/of.c +1 -1
 	microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet);
-	if (IS_ERR_OR_NULL(microvolt))
+	if (IS_ERR(microvolt))
 		return PTR_ERR(microvolt);
Erroring out here will still block EM registration on platforms without the opp-microvolt prop so we're back to square one, which means the patch does not do what the description says it does. It behaves almost identically to the current code.
I am confused.
With the current code, the following will work:
- all three available
- microvolt available and nothing else.
- only microamp available
- only microwatt available
- both microamp and microwatt available but no microvolt
- and other combinations

Quoted 1 line

Isn't this all we want ?

You're right, I misinterpreted an error. Details as below.

Quoted 5 lines

What did you test exactly ? As I thought you will be testing the only microwatt case here and you say it won't work.
Sorry, I just got a little bit confused :(

I did test on the Apple machine with only opp-microwatt, but I misinterpreted the error. We end up here upon parsing the second OPP:

Quoted 6 lines

if (list_empty(&opp_table->opp_list) &&

   opp_table->regulator_count > 0) {
	dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n",
		__func__);
	return ERR_PTR(-EINVAL);
}

When this path is removed, things work as expected. Could it be that opp_list has not been updated by the time we're parsing the next OPP? Seems unlikely, but at the same time if we're ending up taking this branch then there's not much else that could have gone wrong.

- James

Viresh Kumar

author

2022-11-04 05:08 UTC #9 lore raw

Re: [PATCH V2 5/5] OPP: decouple dt properties in opp_parse_supplies()

Quoted 2 lines · James Calligeros

Quoted 2 lines
if (list_empty(&opp_table->opp_list) &&
opp_table->regulator_count > 0) {

I did test this and it should have worked for you as well.

Quoted 9 lines

Quoted 4 lines
	dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n",
		__func__);
	return ERR_PTR(-EINVAL);
}
When this path is removed, things work as expected. Could it be that opp_list has not been updated by the time we're parsing the next OPP? Seems unlikely, but at the same time if we're ending up taking this branch then there's not much else that could have gone wrong.

It should happen sequentially.

Ahh, I looked code for sometime before I wrote this line. I know what's going on. Its a bug in the patchset that I fixed yesterday and pushed.

Initialize "ret = 0" in opp_parse_supplies().

Or pick patches from:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

Sorry for the trouble.

Signature · 1 line

viresh

James Calligeros

2022-11-04 05:25 UTC #10 lore raw

Re: [PATCH V2 5/5] OPP: decouple dt properties in opp_parse_supplies()

Quoted 1 line · AEST Viresh Kumar

It should happen sequentially.

I noticed the mutexes after I got some sleep :)

Quoted 6 lines

Initialize "ret = 0" in opp_parse_supplies().
Or pick patches from:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

It didn't even cross my mind that opp_parse_supplies() returning ret uninitialised was causing the failure further up the chain. Applied and tested, everything's working as expected now.

Quoted 1 line

Sorry for the trouble.

This is on me, I should have heeded the compiler warning.

- James

Viresh Kumar

author

2022-11-04 05:28 UTC #11 lore raw

Re: [PATCH V2 5/5] OPP: decouple dt properties in opp_parse_supplies()

Quoted 12 lines · James Calligeros

Quoted 1 line · AEST Viresh Kumar
It should happen sequentially.
I noticed the mutexes after I got some sleep :)
Quoted 6 lines
Initialize "ret = 0" in opp_parse_supplies().
Or pick patches from:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

Quoted 7 lines

It didn't even cross my mind that opp_parse_supplies() returning ret uninitialised was causing the failure further up the chain. Applied and tested, everything's working as expected now.
Quoted 1 line
Sorry for the trouble.
This is on me, I should have heeded the compiler warning.

Thanks James for giving this a try. Really appreciate it. The changes are applied and pushed for linux-next along with your Tested-by.

Signature · 1 line

viresh

Viresh Kumar

author

2022-11-04 05:08 UTC #12 lore raw

Re: [PATCH V2 5/5] OPP: decouple dt properties in opp_parse_supplies()

In reply to Viresh Kumar

Quoted 23 lines · Viresh Kumar

From: James Calligeros <[email protected]>
The opp-microwatt property was added with the intention of providing platforms a way to specify a precise value for the power consumption of a device at a given OPP to enable better energy-aware scheduling decisions by informing the kernel of the total static and dynamic power of a device at a given OPP, removing the reliance on the EM subsystem's often flawed estimations. This property is parsed by opp_parse_supplies(), which creates a hard dependency on the opp-microvolt property.
Some platforms, such as Apple Silicon, do not describe their device's voltage regulators in the DT as they cannot be controlled by the kernel and/or rely on opaque firmware algorithms to control their voltage and current characteristics at runtime. We can, however, experimentally determine the power consumption of a given device at a given OPP, taking advantage of opp-microwatt to provide EAS on such devices as was initially intended.
Allow platforms to specify and consume any subset of opp-microvolt, opp-microamp, or opp-microwatt without a hard dependency on opp-microvolt to enable this functionality on such platforms.

Quoted 8 lines

Signed-off-by James Calligeros <[email protected]>
Co-developed-by Viresh Kumar <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
V2: Rewritten by Viresh on top of his changes.
 drivers/opp/of.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

Plus this fix:

drivers/opp/of.c +1 -1

index 273fa9c0e1c0..e55c6095adf0 100644
@@ -673,7 +673,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
                              struct opp_table *opp_table)
 {
        u32 *microvolt, *microamp, *microwatt;
-       int ret, i, j;
+       int ret = 0, i, j;
        bool triplet;
 
        microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet);

Signature · 1 line

viresh