opp: Allow opp-supported-hw to contain multiple versions

The bindings allow multiple versions to be passed to "opp-supported-hw" property, either of which can result in enabling of the OPP.

Update code to allow that.

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

---
 drivers/opp/of.c | 47 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

drivers/opp/of.c +33 -14

index e39ddcc779af..5dac8bffd68c 100644
@@ -434,9 +434,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_find_icc_paths);
 static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
 			      struct device_node *np)
 {
-	unsigned int count = opp_table->supported_hw_count;
-	u32 version;
-	int ret;
+	unsigned int levels = opp_table->supported_hw_count;
+	int count, versions, ret, i, j;
+	u32 val;
 
 	if (!opp_table->supported_hw) {
 		/*

@@ -451,21 +451,40 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
 			return true;
 	}
 
-	while (count--) {
-		ret = of_property_read_u32_index(np, "opp-supported-hw", count,
-						 &version);
-		if (ret) {
-			dev_warn(dev, "%s: failed to read opp-supported-hw property at index %d: %d\n",
-				 __func__, count, ret);
-			return false;
+	count = of_property_count_u32_elems(np, "opp-supported-hw");
+	if (count <= 0 || count % levels) {
+		dev_err(dev, "%s: Invalid opp-supported-hw property (%d)\n",
+			__func__, count);
+		return false;
+	}
+
+	versions = count / levels;
+
+	/* All levels in at least one of the versions should match */
+	for (i = 0; i < versions; i++) {
+		bool supported = true;
+
+		for (j = 0; j < levels; j++) {
+			ret = of_property_read_u32_index(np, "opp-supported-hw",
+							 i * levels + j, &val);
+			if (ret) {
+				dev_warn(dev, "%s: failed to read opp-supported-hw property at index %d: %d\n",
+					 __func__, i * levels + j, ret);
+				return false;
+			}
+
+			/* Check if the level is supported */
+			if (!(val & opp_table->supported_hw[j])) {
+				supported = false;
+				break;
+			}
 		}
 
-		/* Both of these are bitwise masks of the versions */
-		if (!(version & opp_table->supported_hw[count]))
-			return false;
+		if (supported)
+			return true;
 	}
 
-	return true;
+	return false;
 }
 
 static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,

Signature · 1 line

2.25.0.rc1.19.g042ed3e048af

The bindings allow multiple versions to be passed to "opp-supported-hw" property, either of which can result in enabling of the OPP.

Update code to allow that.

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

---
 drivers/opp/of.c | 47 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

We can now pass multiple versions in "opp-supported-hw" property, lets do that and simplify the tables a bit.

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

---
Dmitry, I think there is further scope of simplifying stuff here by using the opp-microvolt-<name> property and corresponding dev_pm_opp_set_prop_name() call.

---
 .../boot/dts/tegra20-cpu-opp-microvolt.dtsi   |  36 -
 arch/arm/boot/dts/tegra20-cpu-opp.dtsi        |  67 +-
 .../boot/dts/tegra30-cpu-opp-microvolt.dtsi   | 512 ---------
 arch/arm/boot/dts/tegra30-cpu-opp.dtsi        | 986 +++---------------
 4 files changed, 149 insertions(+), 1452 deletions(-)

arch/arm/boot/dts/tegra30-cpu-opp.dtsi +0 -0

26.08.2020 14:50, Viresh Kumar пишет:

Quoted 9 lines

We can now pass multiple versions in "opp-supported-hw" property, lets do that and simplify the tables a bit.

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

---
Dmitry, I think there is further scope of simplifying stuff here by using the opp-microvolt-<name> property and corresponding dev_pm_opp_set_prop_name() call.

Quoted 1 line

arch/arm/boot/dts/tegra30-cpu-opp.dtsi +0 -0

...

arch/arm/boot/dts/tegra30-cpu-opp.dtsi opp@475000000,800 { · line 118

Quoted 11 lines

arch/arm/boot/dts/tegra30-cpu-opp.dtsi +6 -4

-		opp@1000000000,975,3,7 {
-			clock-latency-ns = <100000>;
-			opp-supported-hw = <0x08 0x0080>;
-			opp-hz = /bits/ 64 <1000000000>;
+			opp-supported-hw = <0x0F 0x0001>,
+				<0x01 0x0002>,
+				<0x01 0x0010>,
+				<0x01 0x0080>,
+				<0x01 0x0100>;
+			opp-hz = /bits/ 64 <475000000>;
 		};

The only very minor difference between my OPP-gen result and yours is that the above hunk has inconsistent single-column formatting, while all others are two-column.

Reviewed-by Dmitry Osipenko <[email protected]>

Tested-by Dmitry Osipenko <[email protected]>

Quoted 3 lines

Quoted 3 lines

Dmitry, I think there is further scope of simplifying stuff here by using the opp-microvolt-<name> property and corresponding dev_pm_opp_set_prop_name() call.

Any inputs on this Dmitry ?

31.08.2020 07:39, Viresh Kumar пишет:
...

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

Dmitry, I think there is further scope of simplifying stuff here by using the opp-microvolt-<name> property and corresponding dev_pm_opp_set_prop_name() call.

Any inputs on this Dmitry ?

Could you please give an example?

Quoted 9 lines

31.08.2020 07:39, Viresh Kumar пишет:
...

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

Dmitry, I think there is further scope of simplifying stuff here by using the opp-microvolt-<name> property and corresponding dev_pm_opp_set_prop_name() call.

Any inputs on this Dmitry ?

Could you please give an example?

There are many users of it in the kernel. grep for "opp-microvolt-" in the DT files and you will see.

The use of this property is to specific multiple microvolt properties to the same frequency without a need to create separate nodes for them all. The right microvolt property will be selected based on the call made to dev_pm_opp_set_prop_name(), search for that too in kernel.

It's not clear to me how it could be applicable to the Tegra CPU OPP because Tegra depends on a combination of SPEEDO + PROCESS versions.

It's not like all voltages are the same for all OPPs that have the same PROCESS ID, otherwise it indeed would be nice to have "opp-microvolt-process0", but unfortunately this variant is not suitable for Tegra because some freqs have different voltages using the same PROCESS ID and the same applies to the SPEEDO ID.

Quoted 8 lines

It's not clear to me how it could be applicable to the Tegra CPU OPP because Tegra depends on a combination of SPEEDO + PROCESS versions.

It's not like all voltages are the same for all OPPs that have the same PROCESS ID, otherwise it indeed would be nice to have "opp-microvolt-process0", but unfortunately this variant is not suitable for Tegra because some freqs have different voltages using the same PROCESS ID and the same applies to the SPEEDO ID.

How exactly do you know what voltage belongs to a particular OPP ?

		opp@216000000 {
			clock-latency-ns = <400000>;
			opp-supported-hw = <0x0F 0x0003>;
			opp-hz = /bits/ 64 <216000000>;
			opp-microvolt-fast-process0 = <750000 750000 1125000>;
			opp-microvolt-slow-process0 = <750000 850000 1125000>;

		};

		opp@312000000 {
			clock-latency-ns = <400000>;
			opp-supported-hw = <0x0F 0x0003>;
			opp-hz = /bits/ 64 <312000000>;
			opp-microvolt-fast-process0 = <750000 750000 1125000>;
			opp-microvolt-slow-process0 = <750000 850000 1125000>;
		};

You can make any combinations of such names that come from speedo, process, or something else. If you can get this done as a fixed formula then it is workable.

IIUC, there is no fixed formula for Tegra, at least I don't see it. For example, if you'll take a look at the 1300MHz OPP of Tegra30, then you could see that this freq has a lot of voltages each depending on specific combination of SPEEDO+PROCESS versions.