[PATCH V3 0/9] PM / OPP: Multiple regulator support

[PATCH V3 0/9] PM / OPP: Multiple regulator support

[Viresh Kumar]

Hi,

Some platforms (like TI) have complex DVFS configuration for CPU
devices, where multiple regulators are required to be configured to
change DVFS state of the device. This was explained well by Nishanth
earlier [1].

One of the major complaints around multiple regulators case was that the
DT isn't responsible in any way to represent the ordering in which
multiple supplies need to be programmed, before or after frequency
change. It was considered in this patch and such information is left to
the platform specific OPP driver now, which can register its own
opp_set_rate() callback with the OPP core and the OPP core will then
call it during DVFS.

The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
and code to pass values for multiple regulators and verified that they
are all properly read by the kernel (using debugfs interface).

Dave Gerlach has already tested it on the real TI platforms and it works
well for him.

This is rebased over: linux-next branch in the PM tree.

V2->V3:
- The last patch is new
- Removed a debug leftover pr_info() message
- Renamed few names as s/set_rate/set_opp
- Removed a TODO comment (as it is done now with this series)
- created struct for min_uV and max_uV
- kerneldoc comments for structures in pm_opp.h
- s/const char */const char * const
- use kasprintf()
- Some more minor reformatting
- More Ack/RBY tags added

V1->V2:
- Ack from Rob for 1st patch
- Moved the supplies structure to pm_opp.h (Dave)
- Fixed an compilation warning.

--
viresh

[1] https://marc.info/?l=linux-pm&m=145684495832764&w=2

Viresh Kumar (9):
  PM / OPP: Reword binding supporting multiple regulators per device
  PM / OPP: Don't use OPP structure outside of rcu protected section
  PM / OPP: Manage supply's voltage/current in a separate structure
  PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()
  PM / OPP: Add infrastructure to manage multiple regulators
  PM / OPP: Separate out _generic_opp_set_rate()
  PM / OPP: Allow platform specific custom set_opp() callbacks
  PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators()
  PM / OPP: Don't assume platform doesn't have regulators

 Documentation/devicetree/bindings/opp/opp.txt |  25 +-
 drivers/base/power/opp/core.c                 | 510 ++++++++++++++++++++------
 drivers/base/power/opp/debugfs.c              |  52 ++-
 drivers/base/power/opp/of.c                   | 105 ++++--
 drivers/base/power/opp/opp.h                  |  20 +-
 drivers/cpufreq/cpufreq-dt.c                  |   9 +-
 include/linux/pm_opp.h                        |  67 +++-
 7 files changed, 605 insertions(+), 183 deletions(-)

-- 
2.7.1.410.g6faf27b

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

[Viresh Kumar]

On certain platforms (like TI), DVFS for a single device (CPU) requires
configuring multiple power supplies.

The OPP bindings already contains binding and example to explain this
case, but it isn't sufficient. For example, there is no way for the code
parsing these bindings to know which voltage values belong to which
power supply. Also its not possible to know the order in which the
supplies need to be configured while switching OPPs.

This patch tries to clarify on those details and does some minor changes
as well.

Note that the bindings do not specify the order in which the regulators
need to be programmed and the order in which the entries are added for
the supplies.

The user of the bindings (like the kernel) shall know these details
already and the DT is responsible to supply only the readings for the
regulators.

Cc: Mark Brown <broonie@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
---
 Documentation/devicetree/bindings/opp/opp.txt | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index ee91cbdd95ee..af476df510f1 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -86,8 +86,13 @@ 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 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.
+
+  Entries for all regulators shall be of the same size, i.e. either all use a
+  single value or triplets.
 
 - 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 +109,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 shall be provided in the same field separated
+  by angular brackets <>. If current values aren't required for a regulator,
+  then it shall be filled with 0. If current 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 values to their power supplies or the
+  order in which the supplies need to be configured.
 
 - opp-microamp-<name>: Named opp-microamp property. Similar to
   opp-microvolt-<name> property, but for microamp instead.
@@ -386,10 +393,12 @@ Example 4: Handling multiple regulators
 / {
 	cpus {
 		cpu@0 {
-			compatible = "arm,cortex-a7";
+			compatible = "vendor,cpu-type";
 			...
 
-			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>;
 		};
 	};
-- 
2.7.1.410.g6faf27b

[PATCH V3 2/9] PM / OPP: Don't use OPP structure outside of rcu protected section

[Viresh Kumar]

The OPP structure must not be used out of the rcu protected section.
Cache the values to be used in separate variables instead.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/base/power/opp/core.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 4c7c6da7a989..056527a3fb4e 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -584,6 +584,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	struct clk *clk;
 	unsigned long freq, old_freq;
 	unsigned long u_volt, u_volt_min, u_volt_max;
+	unsigned long old_u_volt, old_u_volt_min, old_u_volt_max;
 	int ret;
 
 	if (unlikely(!target_freq)) {
@@ -633,6 +634,14 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		return ret;
 	}
 
+	if (IS_ERR(old_opp)) {
+		old_u_volt = 0;
+	} else {
+		old_u_volt = old_opp->u_volt;
+		old_u_volt_min = old_opp->u_volt_min;
+		old_u_volt_max = old_opp->u_volt_max;
+	}
+
 	u_volt = opp->u_volt;
 	u_volt_min = opp->u_volt_min;
 	u_volt_max = opp->u_volt_max;
@@ -677,9 +686,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 			__func__, old_freq);
 restore_voltage:
 	/* This shouldn't harm even if the voltages weren't updated earlier */
-	if (!IS_ERR(old_opp))
-		_set_opp_voltage(dev, reg, old_opp->u_volt,
-				 old_opp->u_volt_min, old_opp->u_volt_max);
+	if (old_u_volt) {
+		_set_opp_voltage(dev, reg, old_u_volt, old_u_volt_min,
+				 old_u_volt_max);
+	}
 
 	return ret;
 }
-- 
2.7.1.410.g6faf27b

[PATCH V3 3/9] PM / OPP: Manage supply's voltage/current in a separate structure

[Viresh Kumar]

This is a preparatory step for multiple regulator per device support.
Move the voltage/current variables to a new structure.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/base/power/opp/core.c    | 44 +++++++++++++++++++++-------------------
 drivers/base/power/opp/debugfs.c |  8 ++++----
 drivers/base/power/opp/of.c      | 18 ++++++++--------
 drivers/base/power/opp/opp.h     | 11 +++-------
 include/linux/pm_opp.h           | 16 +++++++++++++++
 5 files changed, 55 insertions(+), 42 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 056527a3fb4e..8d6006151c9a 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -112,7 +112,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 	if (IS_ERR_OR_NULL(tmp_opp))
 		pr_err("%s: Invalid parameters\n", __func__);
 	else
-		v = tmp_opp->u_volt;
+		v = tmp_opp->supply.u_volt;
 
 	return v;
 }
@@ -246,10 +246,10 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
 		if (!opp->available)
 			continue;
 
-		if (opp->u_volt_min < min_uV)
-			min_uV = opp->u_volt_min;
-		if (opp->u_volt_max > max_uV)
-			max_uV = opp->u_volt_max;
+		if (opp->supply.u_volt_min < min_uV)
+			min_uV = opp->supply.u_volt_min;
+		if (opp->supply.u_volt_max > max_uV)
+			max_uV = opp->supply.u_volt_max;
 	}
 
 	rcu_read_unlock();
@@ -637,14 +637,14 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	if (IS_ERR(old_opp)) {
 		old_u_volt = 0;
 	} else {
-		old_u_volt = old_opp->u_volt;
-		old_u_volt_min = old_opp->u_volt_min;
-		old_u_volt_max = old_opp->u_volt_max;
+		old_u_volt = old_opp->supply.u_volt;
+		old_u_volt_min = old_opp->supply.u_volt_min;
+		old_u_volt_max = old_opp->supply.u_volt_max;
 	}
 
-	u_volt = opp->u_volt;
-	u_volt_min = opp->u_volt_min;
-	u_volt_max = opp->u_volt_max;
+	u_volt = opp->supply.u_volt;
+	u_volt_min = opp->supply.u_volt_min;
+	u_volt_max = opp->supply.u_volt_max;
 
 	reg = opp_table->regulator;
 
@@ -957,10 +957,11 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 	struct regulator *reg = opp_table->regulator;
 
 	if (!IS_ERR(reg) &&
-	    !regulator_is_supported_voltage(reg, opp->u_volt_min,
-					    opp->u_volt_max)) {
+	    !regulator_is_supported_voltage(reg, opp->supply.u_volt_min,
+					    opp->supply.u_volt_max)) {
 		pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
-			__func__, opp->u_volt_min, opp->u_volt_max);
+			__func__, opp->supply.u_volt_min,
+			opp->supply.u_volt_max);
 		return false;
 	}
 
@@ -993,11 +994,12 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 
 		/* Duplicate OPPs */
 		dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
-			 __func__, opp->rate, opp->u_volt, opp->available,
-			 new_opp->rate, new_opp->u_volt, new_opp->available);
+			 __func__, opp->rate, opp->supply.u_volt,
+			 opp->available, new_opp->rate, new_opp->supply.u_volt,
+			 new_opp->available);
 
-		return opp->available && new_opp->u_volt == opp->u_volt ?
-			0 : -EEXIST;
+		return opp->available &&
+		       new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST;
 	}
 
 	new_opp->opp_table = opp_table;
@@ -1064,9 +1066,9 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
 	/* populate the opp table */
 	new_opp->rate = freq;
 	tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
-	new_opp->u_volt = u_volt;
-	new_opp->u_volt_min = u_volt - tol;
-	new_opp->u_volt_max = u_volt + tol;
+	new_opp->supply.u_volt = u_volt;
+	new_opp->supply.u_volt_min = u_volt - tol;
+	new_opp->supply.u_volt_max = u_volt + tol;
 	new_opp->available = true;
 	new_opp->dynamic = dynamic;
 
diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
index ef1ae6b52042..c897676ca35f 100644
--- a/drivers/base/power/opp/debugfs.c
+++ b/drivers/base/power/opp/debugfs.c
@@ -63,16 +63,16 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
 	if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate))
 		return -ENOMEM;
 
-	if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->u_volt))
+	if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt))
 		return -ENOMEM;
 
-	if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->u_volt_min))
+	if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min))
 		return -ENOMEM;
 
-	if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->u_volt_max))
+	if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max))
 		return -ENOMEM;
 
-	if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->u_amp))
+	if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp))
 		return -ENOMEM;
 
 	if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d,
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index 5552211e6fcd..b7fcd0a1b58b 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -148,14 +148,14 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 		return -EINVAL;
 	}
 
-	opp->u_volt = microvolt[0];
+	opp->supply.u_volt = microvolt[0];
 
 	if (count == 1) {
-		opp->u_volt_min = opp->u_volt;
-		opp->u_volt_max = opp->u_volt;
+		opp->supply.u_volt_min = opp->supply.u_volt;
+		opp->supply.u_volt_max = opp->supply.u_volt;
 	} else {
-		opp->u_volt_min = microvolt[1];
-		opp->u_volt_max = microvolt[2];
+		opp->supply.u_volt_min = microvolt[1];
+		opp->supply.u_volt_max = microvolt[2];
 	}
 
 	/* Search for "opp-microamp-<name>" */
@@ -173,7 +173,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 	}
 
 	if (prop && !of_property_read_u32(opp->np, name, &val))
-		opp->u_amp = val;
+		opp->supply.u_amp = val;
 
 	return 0;
 }
@@ -303,9 +303,9 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	mutex_unlock(&opp_table_lock);
 
 	pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n",
-		 __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt,
-		 new_opp->u_volt_min, new_opp->u_volt_max,
-		 new_opp->clock_latency_ns);
+		 __func__, new_opp->turbo, new_opp->rate,
+		 new_opp->supply.u_volt, new_opp->supply.u_volt_min,
+		 new_opp->supply.u_volt_max, new_opp->clock_latency_ns);
 
 	/*
 	 * Notify the changes in the availability of the operable
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index fabd5ca1a083..791213175683 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -61,10 +61,7 @@ extern struct list_head opp_tables;
  * @turbo:	true if turbo (boost) OPP
  * @suspend:	true if suspend OPP
  * @rate:	Frequency in hertz
- * @u_volt:	Target voltage in microvolts corresponding to this OPP
- * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
- * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
- * @u_amp:	Maximum current drawn by the device in microamperes
+ * @supply:	Power supply voltage/current values
  * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
  *		frequency from any other OPP's frequency.
  * @opp_table:	points back to the opp_table struct this opp belongs to
@@ -83,10 +80,8 @@ struct dev_pm_opp {
 	bool suspend;
 	unsigned long rate;
 
-	unsigned long u_volt;
-	unsigned long u_volt_min;
-	unsigned long u_volt_max;
-	unsigned long u_amp;
+	struct dev_pm_opp_supply supply;
+
 	unsigned long clock_latency_ns;
 
 	struct opp_table *opp_table;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index bca26157f5b6..f69126e2bb59 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -24,6 +24,22 @@ enum dev_pm_opp_event {
 	OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
 };
 
+/**
+ * struct dev_pm_opp_supply - Power supply voltage/current values
+ * @u_volt:	Target voltage in microvolts corresponding to this OPP
+ * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
+ * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
+ * @u_amp:	Maximum current drawn by the device in microamperes
+ *
+ * This structure stores the voltage/current values for a single power supply.
+ */
+struct dev_pm_opp_supply {
+	unsigned long u_volt;
+	unsigned long u_volt_min;
+	unsigned long u_volt_max;
+	unsigned long u_amp;
+};
+
 #if defined(CONFIG_PM_OPP)
 
 unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
-- 
2.7.1.410.g6faf27b

[PATCH V3 4/9] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()

[Viresh Kumar]

Pass the entire supply structure instead of all of its fields.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/base/power/opp/core.c | 44 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 8d6006151c9a..37fad2eb0f47 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -542,8 +542,7 @@ static struct clk *_get_opp_clk(struct device *dev)
 }
 
 static int _set_opp_voltage(struct device *dev, struct regulator *reg,
-			    unsigned long u_volt, unsigned long u_volt_min,
-			    unsigned long u_volt_max)
+			    struct dev_pm_opp_supply *supply)
 {
 	int ret;
 
@@ -554,14 +553,15 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 		return 0;
 	}
 
-	dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__, u_volt_min,
-		u_volt, u_volt_max);
+	dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
+		supply->u_volt_min, supply->u_volt, supply->u_volt_max);
 
-	ret = regulator_set_voltage_triplet(reg, u_volt_min, u_volt,
-					    u_volt_max);
+	ret = regulator_set_voltage_triplet(reg, supply->u_volt_min,
+					    supply->u_volt, supply->u_volt_max);
 	if (ret)
 		dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n",
-			__func__, u_volt_min, u_volt, u_volt_max, ret);
+			__func__, supply->u_volt_min, supply->u_volt,
+			supply->u_volt_max, ret);
 
 	return ret;
 }
@@ -583,8 +583,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	struct regulator *reg;
 	struct clk *clk;
 	unsigned long freq, old_freq;
-	unsigned long u_volt, u_volt_min, u_volt_max;
-	unsigned long old_u_volt, old_u_volt_min, old_u_volt_max;
+	struct dev_pm_opp_supply old_supply, new_supply;
 	int ret;
 
 	if (unlikely(!target_freq)) {
@@ -634,17 +633,12 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		return ret;
 	}
 
-	if (IS_ERR(old_opp)) {
-		old_u_volt = 0;
-	} else {
-		old_u_volt = old_opp->supply.u_volt;
-		old_u_volt_min = old_opp->supply.u_volt_min;
-		old_u_volt_max = old_opp->supply.u_volt_max;
-	}
+	if (IS_ERR(old_opp))
+		old_supply.u_volt = 0;
+	else
+		memcpy(&old_supply, &old_opp->supply, sizeof(old_supply));
 
-	u_volt = opp->supply.u_volt;
-	u_volt_min = opp->supply.u_volt_min;
-	u_volt_max = opp->supply.u_volt_max;
+	memcpy(&new_supply, &opp->supply, sizeof(new_supply));
 
 	reg = opp_table->regulator;
 
@@ -652,8 +646,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	/* Scaling up? Scale voltage before frequency */
 	if (freq > old_freq) {
-		ret = _set_opp_voltage(dev, reg, u_volt, u_volt_min,
-				       u_volt_max);
+		ret = _set_opp_voltage(dev, reg, &new_supply);
 		if (ret)
 			goto restore_voltage;
 	}
@@ -672,8 +665,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	/* Scaling down? Scale voltage after frequency */
 	if (freq < old_freq) {
-		ret = _set_opp_voltage(dev, reg, u_volt, u_volt_min,
-				       u_volt_max);
+		ret = _set_opp_voltage(dev, reg, &new_supply);
 		if (ret)
 			goto restore_freq;
 	}
@@ -686,10 +678,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 			__func__, old_freq);
 restore_voltage:
 	/* This shouldn't harm even if the voltages weren't updated earlier */
-	if (old_u_volt) {
-		_set_opp_voltage(dev, reg, old_u_volt, old_u_volt_min,
-				 old_u_volt_max);
-	}
+	if (old_supply.u_volt)
+		_set_opp_voltage(dev, reg, &old_supply);
 
 	return ret;
 }
-- 
2.7.1.410.g6faf27b

[PATCH V3 5/9] PM / OPP: Add infrastructure to manage multiple regulators

[Viresh Kumar]

This patch adds infrastructure to manage multiple regulators and updates
the only user (cpufreq-dt) of dev_pm_opp_set{put}_regulator().

This is preparatory work for adding full support for devices with
multiple regulators.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/base/power/opp/core.c    | 220 ++++++++++++++++++++++++++-------------
 drivers/base/power/opp/debugfs.c |  52 +++++++--
 drivers/base/power/opp/of.c      | 103 ++++++++++++------
 drivers/base/power/opp/opp.h     |  10 +-
 drivers/cpufreq/cpufreq-dt.c     |   9 +-
 include/linux/pm_opp.h           |   8 +-
 6 files changed, 280 insertions(+), 122 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 37fad2eb0f47..5a35fdd4b61b 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -93,6 +93,8 @@ struct opp_table *_find_opp_table(struct device *dev)
  * Return: voltage in micro volt corresponding to the opp, else
  * return 0
  *
+ * This is useful only for devices with single power supply.
+ *
  * Locking: This function must be called under rcu_read_lock(). opp is a rcu
  * protected pointer. This means that opp which could have been fetched by
  * opp_find_freq_{exact,ceil,floor} functions is valid as long as we are
@@ -112,7 +114,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 	if (IS_ERR_OR_NULL(tmp_opp))
 		pr_err("%s: Invalid parameters\n", __func__);
 	else
-		v = tmp_opp->supply.u_volt;
+		v = tmp_opp->supplies[0].u_volt;
 
 	return v;
 }
@@ -222,10 +224,13 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
 {
 	struct opp_table *opp_table;
 	struct dev_pm_opp *opp;
-	struct regulator *reg;
+	struct regulator *reg, **regulators;
 	unsigned long latency_ns = 0;
-	unsigned long min_uV = ~0, max_uV = 0;
-	int ret;
+	int ret, size, i, count;
+	struct {
+		unsigned long min;
+		unsigned long max;
+	} *uV;
 
 	rcu_read_lock();
 
@@ -235,21 +240,41 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
 		return 0;
 	}
 
-	reg = opp_table->regulator;
-	if (IS_ERR(reg)) {
+	count = opp_table->regulator_count;
+
+	if (!count) {
 		/* Regulator may not be required for device */
 		rcu_read_unlock();
 		return 0;
 	}
 
-	list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
-		if (!opp->available)
-			continue;
+	size = count * sizeof(*regulators);
+	regulators = kmemdup(opp_table->regulators, size, GFP_KERNEL);
+	if (!regulators) {
+		rcu_read_unlock();
+		return 0;
+	}
 
-		if (opp->supply.u_volt_min < min_uV)
-			min_uV = opp->supply.u_volt_min;
-		if (opp->supply.u_volt_max > max_uV)
-			max_uV = opp->supply.u_volt_max;
+	uV = kmalloc_array(count, sizeof(*uV), GFP_KERNEL);
+	if (!uV) {
+		kfree(regulators);
+		rcu_read_unlock();
+		return 0;
+	}
+
+	for (i = 0; i < count; i++) {
+		uV[i].min = ~0;
+		uV[i].max = 0;
+
+		list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
+			if (!opp->available)
+				continue;
+
+			if (opp->supplies[i].u_volt_min < uV[i].min)
+				uV[i].min = opp->supplies[i].u_volt_min;
+			if (opp->supplies[i].u_volt_max > uV[i].max)
+				uV[i].max = opp->supplies[i].u_volt_max;
+		}
 	}
 
 	rcu_read_unlock();
@@ -258,9 +283,14 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
 	 * The caller needs to ensure that opp_table (and hence the regulator)
 	 * isn't freed, while we are executing this routine.
 	 */
-	ret = regulator_set_voltage_time(reg, min_uV, max_uV);
-	if (ret > 0)
-		latency_ns = ret * 1000;
+	for (i = 0; reg = regulators[i], i < count; i++) {
+		ret = regulator_set_voltage_time(reg, uV[i].min, uV[i].max);
+		if (ret > 0)
+			latency_ns += ret * 1000;
+	}
+
+	kfree(uV);
+	kfree(regulators);
 
 	return latency_ns;
 }
@@ -580,7 +610,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
 	struct opp_table *opp_table;
 	struct dev_pm_opp *old_opp, *opp;
-	struct regulator *reg;
+	struct regulator *reg = ERR_PTR(-ENXIO);
 	struct clk *clk;
 	unsigned long freq, old_freq;
 	struct dev_pm_opp_supply old_supply, new_supply;
@@ -633,14 +663,23 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		return ret;
 	}
 
+	if (opp_table->regulators) {
+		/* This function only supports single regulator per device */
+		if (WARN_ON(opp_table->regulator_count > 1)) {
+			dev_err(dev, "multiple regulators not supported\n");
+			rcu_read_unlock();
+			return -EINVAL;
+		}
+
+		reg = opp_table->regulators[0];
+	}
+
 	if (IS_ERR(old_opp))
 		old_supply.u_volt = 0;
 	else
-		memcpy(&old_supply, &old_opp->supply, sizeof(old_supply));
+		memcpy(&old_supply, old_opp->supplies, sizeof(old_supply));
 
-	memcpy(&new_supply, &opp->supply, sizeof(new_supply));
-
-	reg = opp_table->regulator;
+	memcpy(&new_supply, opp->supplies, sizeof(new_supply));
 
 	rcu_read_unlock();
 
@@ -764,9 +803,6 @@ static struct opp_table *_add_opp_table(struct device *dev)
 
 	_of_init_opp_table(opp_table, dev);
 
-	/* Set regulator to a non-NULL error value */
-	opp_table->regulator = ERR_PTR(-ENXIO);
-
 	/* Find clk for the device */
 	opp_table->clk = clk_get(dev, NULL);
 	if (IS_ERR(opp_table->clk)) {
@@ -815,7 +851,7 @@ static void _remove_opp_table(struct opp_table *opp_table)
 	if (opp_table->prop_name)
 		return;
 
-	if (!IS_ERR(opp_table->regulator))
+	if (opp_table->regulators)
 		return;
 
 	/* Release clk */
@@ -924,35 +960,50 @@ struct dev_pm_opp *_allocate_opp(struct device *dev,
 				 struct opp_table **opp_table)
 {
 	struct dev_pm_opp *opp;
+	int count, supply_size;
+	struct opp_table *table;
 
-	/* allocate new OPP node */
-	opp = kzalloc(sizeof(*opp), GFP_KERNEL);
-	if (!opp)
+	table = _add_opp_table(dev);
+	if (!table)
 		return NULL;
 
-	INIT_LIST_HEAD(&opp->node);
+	/* Allocate space for at least one supply */
+	count = table->regulator_count ? table->regulator_count : 1;
+	supply_size = sizeof(*opp->supplies) * count;
 
-	*opp_table = _add_opp_table(dev);
-	if (!*opp_table) {
-		kfree(opp);
+	/* allocate new OPP node + and supplies structures */
+	opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL);
+	if (!opp) {
+		kfree(table);
 		return NULL;
 	}
 
+	/* Put the supplies at the end of the OPP structure as an empty array */
+	opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
+	INIT_LIST_HEAD(&opp->node);
+
+	*opp_table = table;
+
 	return opp;
 }
 
 static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 					 struct opp_table *opp_table)
 {
-	struct regulator *reg = opp_table->regulator;
-
-	if (!IS_ERR(reg) &&
-	    !regulator_is_supported_voltage(reg, opp->supply.u_volt_min,
-					    opp->supply.u_volt_max)) {
-		pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
-			__func__, opp->supply.u_volt_min,
-			opp->supply.u_volt_max);
-		return false;
+	struct regulator *reg;
+	int i;
+
+	for (i = 0; i < opp_table->regulator_count; i++) {
+		reg = opp_table->regulators[i];
+
+		if (!regulator_is_supported_voltage(reg,
+					opp->supplies[i].u_volt_min,
+					opp->supplies[i].u_volt_max)) {
+			pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
+				__func__, opp->supplies[i].u_volt_min,
+				opp->supplies[i].u_volt_max);
+			return false;
+		}
 	}
 
 	return true;
@@ -984,12 +1035,13 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 
 		/* Duplicate OPPs */
 		dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
-			 __func__, opp->rate, opp->supply.u_volt,
-			 opp->available, new_opp->rate, new_opp->supply.u_volt,
-			 new_opp->available);
+			 __func__, opp->rate, opp->supplies[0].u_volt,
+			 opp->available, new_opp->rate,
+			 new_opp->supplies[0].u_volt, new_opp->available);
 
+		/* Should we compare voltages for all regulators here ? */
 		return opp->available &&
-		       new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST;
+		       new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? 0 : -EEXIST;
 	}
 
 	new_opp->opp_table = opp_table;
@@ -1056,9 +1108,9 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
 	/* populate the opp table */
 	new_opp->rate = freq;
 	tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
-	new_opp->supply.u_volt = u_volt;
-	new_opp->supply.u_volt_min = u_volt - tol;
-	new_opp->supply.u_volt_max = u_volt + tol;
+	new_opp->supplies[0].u_volt = u_volt;
+	new_opp->supplies[0].u_volt_min = u_volt - tol;
+	new_opp->supplies[0].u_volt_max = u_volt + tol;
 	new_opp->available = true;
 	new_opp->dynamic = dynamic;
 
@@ -1303,12 +1355,14 @@ void dev_pm_opp_put_prop_name(struct device *dev)
 EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
 
 /**
- * dev_pm_opp_set_regulator() - Set regulator name for the device
+ * dev_pm_opp_set_regulators() - Set regulator names for the device
  * @dev: Device for which regulator name is being set.
- * @name: Name of the regulator.
+ * @names: Array of pointers to the names of the regulator.
+ * @count: Number of regulators.
  *
  * In order to support OPP switching, OPP layer needs to know the name of the
- * device's regulator, as the core would be required to switch voltages as well.
+ * device's regulators, as the core would be required to switch voltages as
+ * well.
  *
  * This must be called before any OPPs are initialized for the device.
  *
@@ -1318,11 +1372,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
  * that this function is *NOT* called under RCU protection or in contexts where
  * mutex cannot be locked.
  */
-int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
+			      unsigned int count)
 {
 	struct opp_table *opp_table;
 	struct regulator *reg;
-	int ret;
+	int ret, i;
 
 	mutex_lock(&opp_table_lock);
 
@@ -1338,26 +1393,42 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
 		goto err;
 	}
 
-	/* Already have a regulator set */
-	if (WARN_ON(!IS_ERR(opp_table->regulator))) {
+	/* Already have regulators set */
+	if (WARN_ON(opp_table->regulators)) {
 		ret = -EBUSY;
 		goto err;
 	}
-	/* Allocate the regulator */
-	reg = regulator_get_optional(dev, name);
-	if (IS_ERR(reg)) {
-		ret = PTR_ERR(reg);
-		if (ret != -EPROBE_DEFER)
-			dev_err(dev, "%s: no regulator (%s) found: %d\n",
-				__func__, name, ret);
+
+	opp_table->regulators = kmalloc_array(count,
+					      sizeof(*opp_table->regulators),
+					      GFP_KERNEL);
+	if (!opp_table->regulators)
 		goto err;
+
+	for (i = 0; i < count; i++) {
+		reg = regulator_get_optional(dev, names[i]);
+		if (IS_ERR(reg)) {
+			ret = PTR_ERR(reg);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "%s: regulator (%s) not found: %d\n",
+					__func__, names[i], ret);
+			goto free_regulators;
+		}
+
+		opp_table->regulators[i] = reg;
 	}
 
-	opp_table->regulator = reg;
+	opp_table->regulator_count = count;
 
 	mutex_unlock(&opp_table_lock);
 	return 0;
 
+free_regulators:
+	while (i != 0)
+		regulator_put(opp_table->regulators[--i]);
+
+	kfree(opp_table->regulators);
+	opp_table->regulators = NULL;
 err:
 	_remove_opp_table(opp_table);
 unlock:
@@ -1365,11 +1436,11 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulators);
 
 /**
- * dev_pm_opp_put_regulator() - Releases resources blocked for regulator
- * @dev: Device for which regulator was set.
+ * dev_pm_opp_put_regulators() - Releases resources blocked for regulators
+ * @dev: Device for which regulators were set.
  *
  * Locking: The internal opp_table and opp structures are RCU protected.
  * Hence this function internally uses RCU updater strategy with mutex locks
@@ -1377,9 +1448,10 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
  * that this function is *NOT* called under RCU protection or in contexts where
  * mutex cannot be locked.
  */
-void dev_pm_opp_put_regulator(struct device *dev)
+void dev_pm_opp_put_regulators(struct device *dev)
 {
 	struct opp_table *opp_table;
+	int i;
 
 	mutex_lock(&opp_table_lock);
 
@@ -1391,16 +1463,20 @@ void dev_pm_opp_put_regulator(struct device *dev)
 		goto unlock;
 	}
 
-	if (IS_ERR(opp_table->regulator)) {
-		dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
+	if (!opp_table->regulators) {
+		dev_err(dev, "%s: Doesn't have regulators set\n", __func__);
 		goto unlock;
 	}
 
 	/* Make sure there are no concurrent readers while updating opp_table */
 	WARN_ON(!list_empty(&opp_table->opp_list));
 
-	regulator_put(opp_table->regulator);
-	opp_table->regulator = ERR_PTR(-ENXIO);
+	for (i = opp_table->regulator_count - 1; i >= 0; i--)
+		regulator_put(opp_table->regulators[i]);
+
+	kfree(opp_table->regulators);
+	opp_table->regulators = NULL;
+	opp_table->regulator_count = 0;
 
 	/* Try freeing opp_table if this was the last blocking resource */
 	_remove_opp_table(opp_table);
@@ -1408,7 +1484,7 @@ void dev_pm_opp_put_regulator(struct device *dev)
 unlock:
 	mutex_unlock(&opp_table_lock);
 }
-EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulator);
+EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
 
 /**
  * dev_pm_opp_add()  - Add an OPP table from a table definitions
diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
index c897676ca35f..95f433db4ac7 100644
--- a/drivers/base/power/opp/debugfs.c
+++ b/drivers/base/power/opp/debugfs.c
@@ -15,6 +15,7 @@
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/limits.h>
+#include <linux/slab.h>
 
 #include "opp.h"
 
@@ -34,6 +35,46 @@ void opp_debug_remove_one(struct dev_pm_opp *opp)
 	debugfs_remove_recursive(opp->dentry);
 }
 
+static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
+				      struct opp_table *opp_table,
+				      struct dentry *pdentry)
+{
+	struct dentry *d;
+	int i = 0;
+	char *name;
+
+	/* Always create at least supply-0 directory */
+	do {
+		name = kasprintf(GFP_KERNEL, "supply-%d", i);
+
+		/* Create per-opp directory */
+		d = debugfs_create_dir(name, pdentry);
+
+		kfree(name);
+
+		if (!d)
+			return false;
+
+		if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d,
+					  &opp->supplies[i].u_volt))
+			return false;
+
+		if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d,
+					  &opp->supplies[i].u_volt_min))
+			return false;
+
+		if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d,
+					  &opp->supplies[i].u_volt_max))
+			return false;
+
+		if (!debugfs_create_ulong("u_amp", S_IRUGO, d,
+					  &opp->supplies[i].u_amp))
+			return false;
+	} while (++i < opp_table->regulator_count);
+
+	return true;
+}
+
 int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
 {
 	struct dentry *pdentry = opp_table->dentry;
@@ -63,16 +104,7 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
 	if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate))
 		return -ENOMEM;
 
-	if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt))
-		return -ENOMEM;
-
-	if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min))
-		return -ENOMEM;
-
-	if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max))
-		return -ENOMEM;
-
-	if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp))
+	if (!opp_debug_create_supplies(opp, opp_table, d))
 		return -ENOMEM;
 
 	if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d,
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index b7fcd0a1b58b..477bedab28cf 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -17,6 +17,7 @@
 #include <linux/errno.h>
 #include <linux/device.h>
 #include <linux/of.h>
+#include <linux/slab.h>
 #include <linux/export.h>
 
 #include "opp.h"
@@ -101,16 +102,16 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
 	return true;
 }
 
-/* TODO: Support multiple regulators */
 static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 			      struct opp_table *opp_table)
 {
-	u32 microvolt[3] = {0};
-	u32 val;
-	int count, ret;
+	u32 *microvolt, *microamp = NULL;
+	int supplies, vcount, icount, ret, i, j;
 	struct property *prop = NULL;
 	char name[NAME_MAX];
 
+	supplies = opp_table->regulator_count ? opp_table->regulator_count : 1;
+
 	/* Search for "opp-microvolt-<name>" */
 	if (opp_table->prop_name) {
 		snprintf(name, sizeof(name), "opp-microvolt-%s",
@@ -128,34 +129,29 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 			return 0;
 	}
 
-	count = of_property_count_u32_elems(opp->np, name);
-	if (count < 0) {
+	vcount = of_property_count_u32_elems(opp->np, name);
+	if (vcount < 0) {
 		dev_err(dev, "%s: Invalid %s property (%d)\n",
-			__func__, name, count);
-		return count;
+			__func__, name, vcount);
+		return vcount;
 	}
 
-	/* There can be one or three elements here */
-	if (count != 1 && count != 3) {
-		dev_err(dev, "%s: Invalid number of elements in %s property (%d)\n",
-			__func__, name, 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;
 	}
 
-	ret = of_property_read_u32_array(opp->np, name, microvolt, count);
+	microvolt = kmalloc_array(vcount, sizeof(*microvolt), GFP_KERNEL);
+	if (!microvolt)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(opp->np, name, microvolt, vcount);
 	if (ret) {
 		dev_err(dev, "%s: error parsing %s: %d\n", __func__, name, ret);
-		return -EINVAL;
-	}
-
-	opp->supply.u_volt = microvolt[0];
-
-	if (count == 1) {
-		opp->supply.u_volt_min = opp->supply.u_volt;
-		opp->supply.u_volt_max = opp->supply.u_volt;
-	} else {
-		opp->supply.u_volt_min = microvolt[1];
-		opp->supply.u_volt_max = microvolt[2];
+		ret = -EINVAL;
+		goto free_microvolt;
 	}
 
 	/* Search for "opp-microamp-<name>" */
@@ -172,10 +168,59 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
 		prop = of_find_property(opp->np, name, NULL);
 	}
 
-	if (prop && !of_property_read_u32(opp->np, name, &val))
-		opp->supply.u_amp = val;
+	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 0;
+		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;
+		}
+
+		microamp = kmalloc_array(icount, sizeof(*microamp), GFP_KERNEL);
+		if (!microamp) {
+			ret = -EINVAL;
+			goto free_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;
+		}
+	}
+
+	for (i = 0, j = 0; i < supplies; 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 {
+			opp->supplies[i].u_volt_min = microvolt[j++];
+			opp->supplies[i].u_volt_max = microvolt[j++];
+		}
+
+		if (microamp)
+			opp->supplies[i].u_amp = microamp[i];
+	}
+
+free_microamp:
+	kfree(microamp);
+free_microvolt:
+	kfree(microvolt);
+
+	return ret;
 }
 
 /**
@@ -304,8 +349,8 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 
 	pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n",
 		 __func__, new_opp->turbo, new_opp->rate,
-		 new_opp->supply.u_volt, new_opp->supply.u_volt_min,
-		 new_opp->supply.u_volt_max, new_opp->clock_latency_ns);
+		 new_opp->supplies[0].u_volt, new_opp->supplies[0].u_volt_min,
+		 new_opp->supplies[0].u_volt_max, new_opp->clock_latency_ns);
 
 	/*
 	 * Notify the changes in the availability of the operable
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 791213175683..d5f09ee90347 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -61,7 +61,7 @@ extern struct list_head opp_tables;
  * @turbo:	true if turbo (boost) OPP
  * @suspend:	true if suspend OPP
  * @rate:	Frequency in hertz
- * @supply:	Power supply voltage/current values
+ * @supplies:	Power supplies voltage/current values
  * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
  *		frequency from any other OPP's frequency.
  * @opp_table:	points back to the opp_table struct this opp belongs to
@@ -80,7 +80,7 @@ struct dev_pm_opp {
 	bool suspend;
 	unsigned long rate;
 
-	struct dev_pm_opp_supply supply;
+	struct dev_pm_opp_supply *supplies;
 
 	unsigned long clock_latency_ns;
 
@@ -139,7 +139,8 @@ enum opp_table_access {
  * @supported_hw_count: Number of elements in supported_hw array.
  * @prop_name: A name to postfix to many DT properties, while parsing them.
  * @clk: Device's clock handle
- * @regulator: Supply regulator
+ * @regulators: Supply regulators
+ * @regulator_count: Number of power supply regulators
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
  * @dentry_name: Name of the real dentry.
  *
@@ -174,7 +175,8 @@ struct opp_table {
 	unsigned int supported_hw_count;
 	const char *prop_name;
 	struct clk *clk;
-	struct regulator *regulator;
+	struct regulator **regulators;
+	unsigned int regulator_count;
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 5c07ae05d69a..15cb26118dc7 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -186,7 +186,10 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	 */
 	name = find_supply_name(cpu_dev);
 	if (name) {
-		ret = dev_pm_opp_set_regulator(cpu_dev, name);
+		const char *names[] = {name};
+
+		ret = dev_pm_opp_set_regulators(cpu_dev, names,
+						ARRAY_SIZE(names));
 		if (ret) {
 			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
 				policy->cpu, ret);
@@ -285,7 +288,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 out_free_opp:
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
 	if (name)
-		dev_pm_opp_put_regulator(cpu_dev);
+		dev_pm_opp_put_regulators(cpu_dev);
 out_put_clk:
 	clk_put(cpu_clk);
 
@@ -300,7 +303,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
 	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
 	if (priv->reg_name)
-		dev_pm_opp_put_regulator(priv->cpu_dev);
+		dev_pm_opp_put_regulators(priv->cpu_dev);
 
 	clk_put(policy->clk);
 	kfree(priv);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index f69126e2bb59..27eea9bfc5ed 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -78,8 +78,8 @@ int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
 void dev_pm_opp_put_supported_hw(struct device *dev);
 int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
 void dev_pm_opp_put_prop_name(struct device *dev);
-int dev_pm_opp_set_regulator(struct device *dev, const char *name);
-void dev_pm_opp_put_regulator(struct device *dev);
+int dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count);
+void dev_pm_opp_put_regulators(struct device *dev);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -186,12 +186,12 @@ static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
 
 static inline void dev_pm_opp_put_prop_name(struct device *dev) {}
 
-static inline int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+static inline int dev_pm_opp_set_regulators(struct device *dev, const char *names[], unsigned int count)
 {
 	return -ENOTSUPP;
 }
 
-static inline void dev_pm_opp_put_regulator(struct device *dev) {}
+static inline void dev_pm_opp_put_regulators(struct device *dev) {}
 
 static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
-- 
2.7.1.410.g6faf27b

[PATCH V3 6/9] PM / OPP: Separate out _generic_opp_set_rate()

[Viresh Kumar]

Later patches would add support for custom opp_set_rate callbacks. This
patch separates out the code for generic opp_set_rate handler in order
to prepare for that.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/base/power/opp/core.c | 180 +++++++++++++++++++++++++++++-------------
 drivers/base/power/opp/opp.h  |   2 +
 include/linux/pm_opp.h        |  33 ++++++++
 3 files changed, 162 insertions(+), 53 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 5a35fdd4b61b..dedb08a66e99 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -596,6 +596,69 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 	return ret;
 }
 
+static inline int
+_generic_set_opp_clk_only(struct device *dev, struct clk *clk,
+			  unsigned long old_freq, unsigned long freq)
+{
+	int ret;
+
+	ret = clk_set_rate(clk, freq);
+	if (ret) {
+		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
+			ret);
+	}
+
+	return ret;
+}
+
+static int _generic_set_opp(struct device *dev,
+			    struct dev_pm_set_opp_data *data)
+{
+	struct dev_pm_opp_supply *old_supply = data->old_opp.supplies;
+	struct dev_pm_opp_supply *new_supply = data->new_opp.supplies;
+	unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate;
+	struct regulator *reg = data->regulators[0];
+	int ret;
+
+	/* This function only supports single regulator per device */
+	if (WARN_ON(data->regulator_count > 1)) {
+		dev_err(dev, "multiple regulators are not supported\n");
+		return -EINVAL;
+	}
+
+	/* Scaling up? Scale voltage before frequency */
+	if (freq > old_freq) {
+		ret = _set_opp_voltage(dev, reg, new_supply);
+		if (ret)
+			goto restore_voltage;
+	}
+
+	/* Change frequency */
+	ret = _generic_set_opp_clk_only(dev, data->clk, old_freq, freq);
+	if (ret)
+		goto restore_voltage;
+
+	/* Scaling down? Scale voltage after frequency */
+	if (freq < old_freq) {
+		ret = _set_opp_voltage(dev, reg, new_supply);
+		if (ret)
+			goto restore_freq;
+	}
+
+	return 0;
+
+restore_freq:
+	if (_generic_set_opp_clk_only(dev, data->clk, freq, old_freq))
+		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
+			__func__, old_freq);
+restore_voltage:
+	/* This shouldn't harm even if the voltages weren't updated earlier */
+	if (old_supply->u_volt)
+		_set_opp_voltage(dev, reg, old_supply);
+
+	return ret;
+}
+
 /**
  * dev_pm_opp_set_rate() - Configure new OPP based on frequency
  * @dev:	 device for which we do this operation
@@ -609,12 +672,12 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
 	struct opp_table *opp_table;
+	unsigned long freq, old_freq;
 	struct dev_pm_opp *old_opp, *opp;
-	struct regulator *reg = ERR_PTR(-ENXIO);
+	struct regulator **regulators;
+	struct dev_pm_set_opp_data *data;
 	struct clk *clk;
-	unsigned long freq, old_freq;
-	struct dev_pm_opp_supply old_supply, new_supply;
-	int ret;
+	int ret, size;
 
 	if (unlikely(!target_freq)) {
 		dev_err(dev, "%s: Invalid target frequency %lu\n", __func__,
@@ -663,64 +726,35 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		return ret;
 	}
 
-	if (opp_table->regulators) {
-		/* This function only supports single regulator per device */
-		if (WARN_ON(opp_table->regulator_count > 1)) {
-			dev_err(dev, "multiple regulators not supported\n");
-			rcu_read_unlock();
-			return -EINVAL;
-		}
+	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
+		old_freq, freq);
 
-		reg = opp_table->regulators[0];
+	regulators = opp_table->regulators;
+
+	/* Only frequency scaling */
+	if (!regulators) {
+		rcu_read_unlock();
+		return _generic_set_opp_clk_only(dev, clk, old_freq, freq);
 	}
 
+	data = opp_table->set_opp_data;
+	data->regulators = regulators;
+	data->regulator_count = opp_table->regulator_count;
+	data->clk = clk;
+
+	data->old_opp.rate = old_freq;
+	size = sizeof(*opp->supplies) * opp_table->regulator_count;
 	if (IS_ERR(old_opp))
-		old_supply.u_volt = 0;
+		memset(data->old_opp.supplies, 0, size);
 	else
-		memcpy(&old_supply, old_opp->supplies, sizeof(old_supply));
+		memcpy(data->old_opp.supplies, old_opp->supplies, size);
 
-	memcpy(&new_supply, opp->supplies, sizeof(new_supply));
+	data->new_opp.rate = freq;
+	memcpy(data->new_opp.supplies, opp->supplies, size);
 
 	rcu_read_unlock();
 
-	/* Scaling up? Scale voltage before frequency */
-	if (freq > old_freq) {
-		ret = _set_opp_voltage(dev, reg, &new_supply);
-		if (ret)
-			goto restore_voltage;
-	}
-
-	/* Change frequency */
-
-	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",
-		__func__, old_freq, freq);
-
-	ret = clk_set_rate(clk, freq);
-	if (ret) {
-		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
-			ret);
-		goto restore_voltage;
-	}
-
-	/* Scaling down? Scale voltage after frequency */
-	if (freq < old_freq) {
-		ret = _set_opp_voltage(dev, reg, &new_supply);
-		if (ret)
-			goto restore_freq;
-	}
-
-	return 0;
-
-restore_freq:
-	if (clk_set_rate(clk, old_freq))
-		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
-			__func__, old_freq);
-restore_voltage:
-	/* This shouldn't harm even if the voltages weren't updated earlier */
-	if (old_supply.u_volt)
-		_set_opp_voltage(dev, reg, &old_supply);
-
-	return ret;
+	return _generic_set_opp(dev, data);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
 
@@ -1354,6 +1388,38 @@ void dev_pm_opp_put_prop_name(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
 
+static int _allocate_set_opp_data(struct opp_table *opp_table)
+{
+	struct dev_pm_set_opp_data *data;
+	int len, count = opp_table->regulator_count;
+
+	if (WARN_ON(!count))
+		return -EINVAL;
+
+	/* space for set_opp_data */
+	len = sizeof(*data);
+
+	/* space for old_opp.supplies and new_opp.supplies */
+	len += 2 * sizeof(struct dev_pm_opp_supply) * count;
+
+	data = kzalloc(len, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->old_opp.supplies = (void *)(data + 1);
+	data->new_opp.supplies = data->old_opp.supplies + count;
+
+	opp_table->set_opp_data = data;
+
+	return 0;
+}
+
+static void _free_set_opp_data(struct opp_table *opp_table)
+{
+	kfree(opp_table->set_opp_data);
+	opp_table->set_opp_data = NULL;
+}
+
 /**
  * dev_pm_opp_set_regulators() - Set regulator names for the device
  * @dev: Device for which regulator name is being set.
@@ -1420,6 +1486,11 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
 
 	opp_table->regulator_count = count;
 
+	/* Allocate block only once to pass to ->set_rate() */
+	ret = _allocate_set_opp_data(opp_table);
+	if (ret)
+		goto free_regulators;
+
 	mutex_unlock(&opp_table_lock);
 	return 0;
 
@@ -1429,6 +1500,7 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
 
 	kfree(opp_table->regulators);
 	opp_table->regulators = NULL;
+	opp_table->regulator_count = 0;
 err:
 	_remove_opp_table(opp_table);
 unlock:
@@ -1474,6 +1546,8 @@ void dev_pm_opp_put_regulators(struct device *dev)
 	for (i = opp_table->regulator_count - 1; i >= 0; i--)
 		regulator_put(opp_table->regulators[i]);
 
+	_free_set_opp_data(opp_table);
+
 	kfree(opp_table->regulators);
 	opp_table->regulators = NULL;
 	opp_table->regulator_count = 0;
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index d5f09ee90347..26bc6c1b8c60 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -178,6 +178,8 @@ struct opp_table {
 	struct regulator **regulators;
 	unsigned int regulator_count;
 
+	struct dev_pm_set_opp_data *set_opp_data;
+
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
 	char dentry_name[NAME_MAX];
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 27eea9bfc5ed..2969519bf5f7 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -17,6 +17,8 @@
 #include <linux/err.h>
 #include <linux/notifier.h>
 
+struct clk;
+struct regulator;
 struct dev_pm_opp;
 struct device;
 
@@ -40,6 +42,37 @@ struct dev_pm_opp_supply {
 	unsigned long u_amp;
 };
 
+/**
+ * struct dev_pm_opp_info - OPP freq/voltage/current values
+ * @rate:	Target clk rate in hz
+ * @supplies:	Array of voltage/current values for all power supplies
+ *
+ * This structure stores the freq/voltage/current values for a single OPP.
+ */
+struct dev_pm_opp_info {
+	unsigned long rate;
+	struct dev_pm_opp_supply *supplies;
+};
+
+/**
+ * struct dev_pm_set_opp_data - Set OPP data
+ * @old_opp:	Old OPP info
+ * @new_opp:	New OPP info
+ * @regulators:	Array of regulator pointers
+ * @regulator_count: Number of regulators
+ * @clk:	Pointer to clk
+ *
+ * This structure contains all information required for setting an OPP.
+ */
+struct dev_pm_set_opp_data {
+	struct dev_pm_opp_info old_opp;
+	struct dev_pm_opp_info new_opp;
+
+	struct regulator **regulators;
+	unsigned int regulator_count;
+	struct clk *clk;
+};
+
 #if defined(CONFIG_PM_OPP)
 
 unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
-- 
2.7.1.410.g6faf27b

[PATCH V3 7/9] PM / OPP: Allow platform specific custom set_opp() callbacks

[Viresh Kumar]

The generic set_opp() handler isn't sufficient for platforms with
complex DVFS.  For example, some TI platforms have multiple regulators
for a CPU device. The order in which various supplies need to be
programmed is only known to the platform code and its best to leave it
to it.

This patch implements APIs to register platform specific set_opp()
callback.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/base/power/opp/core.c | 116 +++++++++++++++++++++++++++++++++++++++++-
 drivers/base/power/opp/opp.h  |   1 +
 include/linux/pm_opp.h        |  10 ++++
 3 files changed, 125 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index dedb08a66e99..f4f6b1fdbe06 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -673,6 +673,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
 	struct opp_table *opp_table;
 	unsigned long freq, old_freq;
+	int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data);
 	struct dev_pm_opp *old_opp, *opp;
 	struct regulator **regulators;
 	struct dev_pm_set_opp_data *data;
@@ -737,6 +738,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		return _generic_set_opp_clk_only(dev, clk, old_freq, freq);
 	}
 
+	if (opp_table->set_opp)
+		set_opp = opp_table->set_opp;
+	else
+		set_opp = _generic_set_opp;
+
 	data = opp_table->set_opp_data;
 	data->regulators = regulators;
 	data->regulator_count = opp_table->regulator_count;
@@ -754,7 +760,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	rcu_read_unlock();
 
-	return _generic_set_opp(dev, data);
+	return set_opp(dev, data);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
 
@@ -888,6 +894,9 @@ static void _remove_opp_table(struct opp_table *opp_table)
 	if (opp_table->regulators)
 		return;
 
+	if (opp_table->set_opp)
+		return;
+
 	/* Release clk */
 	if (!IS_ERR(opp_table->clk))
 		clk_put(opp_table->clk);
@@ -1486,7 +1495,7 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
 
 	opp_table->regulator_count = count;
 
-	/* Allocate block only once to pass to ->set_rate() */
+	/* Allocate block only once to pass to ->set_opp() */
 	ret = _allocate_set_opp_data(opp_table);
 	if (ret)
 		goto free_regulators;
@@ -1561,6 +1570,109 @@ void dev_pm_opp_put_regulators(struct device *dev)
 EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
 
 /**
+ * dev_pm_opp_register_set_opp_helper() - Register custom OPP set rate helper
+ * @dev: Device for which the helper is getting registered.
+ * @set_opp: Custom set OPP helper.
+ *
+ * This is useful to support complex platforms (like platforms with multiple
+ * regulators per device), instead of the generic OPP set rate helper.
+ *
+ * This must be called before any OPPs are initialized for the device.
+ *
+ * Locking: The internal opp_table and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ */
+int dev_pm_opp_register_set_opp_helper(struct device *dev,
+	int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data))
+{
+	struct opp_table *opp_table;
+	int ret;
+
+	if (!set_opp)
+		return -EINVAL;
+
+	mutex_lock(&opp_table_lock);
+
+	opp_table = _add_opp_table(dev);
+	if (!opp_table) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	/* This should be called before OPPs are initialized */
+	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
+		ret = -EBUSY;
+		goto err;
+	}
+
+	/* Already have custom set_opp helper */
+	if (WARN_ON(opp_table->set_opp)) {
+		ret = -EBUSY;
+		goto err;
+	}
+
+	opp_table->set_opp = set_opp;
+
+	mutex_unlock(&opp_table_lock);
+	return 0;
+
+err:
+	_remove_opp_table(opp_table);
+unlock:
+	mutex_unlock(&opp_table_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_register_set_opp_helper);
+
+/**
+ * dev_pm_opp_register_put_opp_helper() - Releases resources blocked for
+ *					   set_opp helper
+ * @dev: Device for which custom set_opp helper has to be cleared.
+ *
+ * Locking: The internal opp_table and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks
+ * to keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex cannot be locked.
+ */
+void dev_pm_opp_register_put_opp_helper(struct device *dev)
+{
+	struct opp_table *opp_table;
+
+	mutex_lock(&opp_table_lock);
+
+	/* Check for existing table for 'dev' first */
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table)) {
+		dev_err(dev, "Failed to find opp_table: %ld\n",
+			PTR_ERR(opp_table));
+		goto unlock;
+	}
+
+	if (!opp_table->set_opp) {
+		dev_err(dev, "%s: Doesn't have custom set_opp helper set\n",
+			__func__);
+		goto unlock;
+	}
+
+	/* Make sure there are no concurrent readers while updating opp_table */
+	WARN_ON(!list_empty(&opp_table->opp_list));
+
+	opp_table->set_opp = NULL;
+
+	/* Try freeing opp_table if this was the last blocking resource */
+	_remove_opp_table(opp_table);
+
+unlock:
+	mutex_unlock(&opp_table_lock);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_register_put_opp_helper);
+
+/**
  * dev_pm_opp_add()  - Add an OPP table from a table definitions
  * @dev:	device for which we do this operation
  * @freq:	Frequency in Hz for this OPP
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 26bc6c1b8c60..62a6020b2c3d 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -178,6 +178,7 @@ struct opp_table {
 	struct regulator **regulators;
 	unsigned int regulator_count;
 
+	int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data);
 	struct dev_pm_set_opp_data *set_opp_data;
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 2969519bf5f7..cb5bc4747502 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -113,6 +113,8 @@ int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
 void dev_pm_opp_put_prop_name(struct device *dev);
 int dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count);
 void dev_pm_opp_put_regulators(struct device *dev);
+int dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data));
+void dev_pm_opp_register_put_opp_helper(struct device *dev);
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
 int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -212,6 +214,14 @@ static inline int dev_pm_opp_set_supported_hw(struct device *dev,
 
 static inline void dev_pm_opp_put_supported_hw(struct device *dev) {}
 
+static inline int dev_pm_opp_register_set_opp_helper(struct device *dev,
+	int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data))
+{
+	return -ENOTSUPP;
+}
+
+static inline void dev_pm_opp_register_put_opp_helper(struct device *dev) {}
+
 static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
 {
 	return -ENOTSUPP;
-- 
2.7.1.410.g6faf27b

[PATCH V3 8/9] PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators()

[Viresh Kumar]

If a platform specific OPP driver has called this routine first and set
the regulators, then the second call from cpufreq-dt driver will hit the
WARN_ON(). Remove the WARN_ON(), but continue to return error in such
cases.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
Tested-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/base/power/opp/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index f4f6b1fdbe06..3298fac01bb0 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -1469,7 +1469,7 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
 	}
 
 	/* Already have regulators set */
-	if (WARN_ON(opp_table->regulators)) {
+	if (opp_table->regulators) {
 		ret = -EBUSY;
 		goto err;
 	}
-- 
2.7.1.410.g6faf27b

[PATCH V3 9/9] PM / OPP: Don't assume platform doesn't have regulators

[Viresh Kumar]

If the regulators aren't set explicitly by the platform, the OPP core
assumes that the platform doesn't have any regulator and uses the
clk-only callback.

If the platform failed to register a regulator with the core, then this
can turn out to be a dangerous assumption as the OPP core will try to
change clk without changing regulators.

Handle that properly by making sure that the DT didn't had any entries
for supply voltages as well.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 3298fac01bb0..34cd48dfe89e 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -734,7 +734,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	/* Only frequency scaling */
 	if (!regulators) {
-		rcu_read_unlock();
+		/*
+		 * DT contained supply ratings? Consider platform failed to set
+		 * regulators.
+		 */
+		if (unlikely(opp->supplies[0].u_volt)) {
+			rcu_read_unlock();
+			dev_err(dev, "%s: Regulator not registered with OPP core\n",
+				__func__);
+			return -EINVAL;
+		}
+
 		return _generic_set_opp_clk_only(dev, clk, old_freq, freq);
 	}
 
-- 
2.7.1.410.g6faf27b

Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

[Viresh Kumar]

On 26-10-16, 12:02, Viresh Kumar wrote:
> Hi,
> 
> Some platforms (like TI) have complex DVFS configuration for CPU
> devices, where multiple regulators are required to be configured to
> change DVFS state of the device. This was explained well by Nishanth
> earlier [1].
> 
> One of the major complaints around multiple regulators case was that the
> DT isn't responsible in any way to represent the ordering in which
> multiple supplies need to be programmed, before or after frequency
> change. It was considered in this patch and such information is left to
> the platform specific OPP driver now, which can register its own
> opp_set_rate() callback with the OPP core and the OPP core will then
> call it during DVFS.
> 
> The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
> and code to pass values for multiple regulators and verified that they
> are all properly read by the kernel (using debugfs interface).
> 
> Dave Gerlach has already tested it on the real TI platforms and it works
> well for him.
> 
> This is rebased over: linux-next branch in the PM tree.
> 
> V2->V3:
> - The last patch is new
> - Removed a debug leftover pr_info() message
> - Renamed few names as s/set_rate/set_opp
> - Removed a TODO comment (as it is done now with this series)
> - created struct for min_uV and max_uV
> - kerneldoc comments for structures in pm_opp.h
> - s/const char */const char * const
> - use kasprintf()
> - Some more minor reformatting
> - More Ack/RBY tags added

@Stephen: Can you please provide further comments or Acks ?

-- 
viresh

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

[Mark Brown]

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

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.

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

Re: [PATCH V3 9/9] PM / OPP: Don't assume platform doesn't have regulators

[Stephen Boyd]

On 10/26, Viresh Kumar wrote:
> If the regulators aren't set explicitly by the platform, the OPP core
> assumes that the platform doesn't have any regulator and uses the
> clk-only callback.
> 
> If the platform failed to register a regulator with the core, then this
> can turn out to be a dangerous assumption as the OPP core will try to
> change clk without changing regulators.
> 
> Handle that properly by making sure that the DT didn't had any entries

s/had/have/

> for supply voltages as well.
> 
> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index 3298fac01bb0..34cd48dfe89e 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -734,7 +734,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  
>  	/* Only frequency scaling */
>  	if (!regulators) {
> -		rcu_read_unlock();
> +		/*
> +		 * DT contained supply ratings? Consider platform failed to set
> +		 * regulators.
> +		 */
> +		if (unlikely(opp->supplies[0].u_volt)) {
> +			rcu_read_unlock();
> +			dev_err(dev, "%s: Regulator not registered with OPP core\n",
> +				__func__);
> +			return -EINVAL;
> +		}
> +

Don't we need an rcu_read_unlock() here as well?

>  		return _generic_set_opp_clk_only(dev, clk, old_freq, freq);
>  	}
>  

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

Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

[Stephen Boyd]

On 11/02, Viresh Kumar wrote:
> On 26-10-16, 12:02, Viresh Kumar wrote:
> > Hi,
> > 
> > Some platforms (like TI) have complex DVFS configuration for CPU
> > devices, where multiple regulators are required to be configured to
> > change DVFS state of the device. This was explained well by Nishanth
> > earlier [1].
> > 
> > One of the major complaints around multiple regulators case was that the
> > DT isn't responsible in any way to represent the ordering in which
> > multiple supplies need to be programmed, before or after frequency
> > change. It was considered in this patch and such information is left to
> > the platform specific OPP driver now, which can register its own
> > opp_set_rate() callback with the OPP core and the OPP core will then
> > call it during DVFS.
> > 
> > The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
> > and code to pass values for multiple regulators and verified that they
> > are all properly read by the kernel (using debugfs interface).
> > 
> > Dave Gerlach has already tested it on the real TI platforms and it works
> > well for him.
> > 
> > This is rebased over: linux-next branch in the PM tree.
> > 
> > V2->V3:
> > - The last patch is new
> > - Removed a debug leftover pr_info() message
> > - Renamed few names as s/set_rate/set_opp
> > - Removed a TODO comment (as it is done now with this series)
> > - created struct for min_uV and max_uV
> > - kerneldoc comments for structures in pm_opp.h
> > - s/const char */const char * const
> > - use kasprintf()
> > - Some more minor reformatting
> > - More Ack/RBY tags added
> 
> @Stephen: Can you please provide further comments or Acks ?
> 

Where did we end up on the topic of RCU usage in OPP? I'd rather
we figure that out before investing more time in reviewing code
that we may end up completely rewriting in the future.

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

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

[Viresh Kumar]

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

Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

[Viresh Kumar]

On 09-11-16, 17:19, Stephen Boyd wrote:
> On 11/02, Viresh Kumar wrote:
> > On 26-10-16, 12:02, Viresh Kumar wrote:
> > > Hi,
> > > 
> > > Some platforms (like TI) have complex DVFS configuration for CPU
> > > devices, where multiple regulators are required to be configured to
> > > change DVFS state of the device. This was explained well by Nishanth
> > > earlier [1].
> > > 
> > > One of the major complaints around multiple regulators case was that the
> > > DT isn't responsible in any way to represent the ordering in which
> > > multiple supplies need to be programmed, before or after frequency
> > > change. It was considered in this patch and such information is left to
> > > the platform specific OPP driver now, which can register its own
> > > opp_set_rate() callback with the OPP core and the OPP core will then
> > > call it during DVFS.
> > > 
> > > The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
> > > and code to pass values for multiple regulators and verified that they
> > > are all properly read by the kernel (using debugfs interface).
> > > 
> > > Dave Gerlach has already tested it on the real TI platforms and it works
> > > well for him.
> > > 
> > > This is rebased over: linux-next branch in the PM tree.
> > > 
> > > V2->V3:
> > > - The last patch is new
> > > - Removed a debug leftover pr_info() message
> > > - Renamed few names as s/set_rate/set_opp
> > > - Removed a TODO comment (as it is done now with this series)
> > > - created struct for min_uV and max_uV
> > > - kerneldoc comments for structures in pm_opp.h
> > > - s/const char */const char * const
> > > - use kasprintf()
> > > - Some more minor reformatting
> > > - More Ack/RBY tags added
> > 
> > @Stephen: Can you please provide further comments or Acks ?
> > 
> 
> Where did we end up on the topic of RCU usage in OPP? I'd rather
> we figure that out before investing more time in reviewing code
> that we may end up completely rewriting in the future.

We haven't decided anything on that yet and I don't think there is any reason to
block this series for that. There is lots of existing code that needs to be
updated if we decide to walk that path and I wouldn't mind a small addition to
that from this series.

Lots of effort Coding/testing/reviewing has already gone into this work and we
better get it merged now.

-- 
viresh

[PATCH V4 9/9] PM / OPP: Don't assume platform doesn't have regulators

[Viresh Kumar]

If the regulators aren't set explicitly by the platform, the OPP core
assumes that the platform doesn't have any regulator and uses the
clk-only callback.

If the platform failed to register a regulator with the core, then this
can turn out to be a dangerous assumption as the OPP core will try to
change clk without changing regulators.

Handle that properly by making sure that the DT didn't have any entries
for supply voltages as well.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V4:
- s/had/have
- fix missing rcu_read_unlock()

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

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 3298fac01bb0..cb33f8b2b56d 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -734,7 +734,20 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	/* Only frequency scaling */
 	if (!regulators) {
+		unsigned long u_volt = opp->supplies[0].u_volt;
+
 		rcu_read_unlock();
+
+		/*
+		 * DT contained supply ratings? Consider platform failed to set
+		 * regulators.
+		 */
+		if (unlikely(u_volt)) {
+			dev_err(dev, "%s: Regulator not registered with OPP core\n",
+				__func__);
+			return -EINVAL;
+		}
+
 		return _generic_set_opp_clk_only(dev, clk, old_freq, freq);
 	}
 
-- 
2.7.1.410.g6faf27b

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

[Mark Brown]

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

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.

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

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

[Viresh Kumar]

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

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

[Stephen Boyd]

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

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

[Viresh Kumar]

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

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

[Rob Herring]

On Fri, Nov 11, 2016 at 08:41:20AM +0530, Viresh Kumar wrote:
> 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";

Uh, no. You already have the names in the *-supply properties. Yes, they 
are a PIA to retrieve compared to a *-names property, but that is the 
nature of this style of binding.

Rob

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

[Stephen Boyd]

On 11/14, Rob Herring wrote:
> On Fri, Nov 11, 2016 at 08:41:20AM +0530, Viresh Kumar wrote:
> > On 10-11-16, 14:51, Stephen Boyd wrote:
> > > 
> > > 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";
> 
> Uh, no. You already have the names in the *-supply properties. Yes, they 
> are a PIA to retrieve compared to a *-names property, but that is the 
> nature of this style of binding.
> 

I think the problem is that Viresh wants the binding to be "self
describing" so that the OPP can be used without a driver knowing
that a supply corresponds to a particular column in the voltage
table. I don't understand that though. Can't we set the supply
names from C code somewhere based on the consumer of the OPPs?
Similar to how we pick the different tables based on fuses?

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

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

[Viresh Kumar]

First of all, thanks to all of you for commenting here. Please
continue doing so as I want to finish this stuff quickly, it has
already killed a lot of time :)

On 14-11-16, 18:13, Stephen Boyd wrote:
> On 11/14, Rob Herring wrote:
> > On Fri, Nov 11, 2016 at 08:41:20AM +0530, Viresh Kumar wrote:
> > > On 10-11-16, 14:51, Stephen Boyd wrote:
> > > > 
> > > > 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";
> > 
> > Uh, no. You already have the names in the *-supply properties. Yes, they 
> > are a PIA to retrieve compared to a *-names property, but that is the 
> > nature of this style of binding.

Its not just PIA, but impossible AFAICT.

There are two important pieces of information we need for multiple
regulator support:
- Which regulator in the consumer node corresponds to which entry in
  the OPP table. As Mark mentioned earlier, DT should be able to get
  us this.
- The order in which the supplies need to be programmed. We have all
  agreed to do this in code instead of inferring it from DT and this
  patch series already does that.

I want to solve the first problem here and I don't see how it can be
solved using such entries:

	cpus {
		cpu@0 {
			compatible = "arm,cortex-a7";
			...

                        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";
		opp-shared;

		opp@1000000000 {
			opp-hz = /bits/ 64 <1000000000>;
			opp-microvolt = <970000>, /* Supply 0 */
					<960000>, /* Supply 1 */
					<960000>; /* Supply 2 */
		};
        };

The code can't figure out which of vcc0, vcc1, vcc2 is added first in
the CPU node and so we need to get the order somehow. A separate
binding as I mentioned earlier is a probably (ugly) solution.

> I think the problem is that Viresh wants the binding to be "self
> describing" so that the OPP can be used without a driver knowing
> that a supply corresponds to a particular column in the voltage
> table.

Right, and that's what Mark suggested as well.

> I don't understand that though. Can't we set the supply
> names from C code somewhere based on the consumer of the OPPs?

That's what this patch series is doing right now.

So, are you saying that the way this patchset does it is fine with you
?

-- 
viresh

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

[Stephen Boyd]

On 11/15, Viresh Kumar wrote:
> On 14-11-16, 18:13, Stephen Boyd wrote:
> > On 11/14, Rob Herring wrote:
> > > On Fri, Nov 11, 2016 at 08:41:20AM +0530, Viresh Kumar wrote:
> > > > On 10-11-16, 14:51, Stephen Boyd wrote:
> > > > > 
> > > > > 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";
> > > 
> > > Uh, no. You already have the names in the *-supply properties. Yes, they 
> > > are a PIA to retrieve compared to a *-names property, but that is the 
> > > nature of this style of binding.
> 
> Its not just PIA, but impossible AFAICT.
> 
> There are two important pieces of information we need for multiple
> regulator support:
> - Which regulator in the consumer node corresponds to which entry in
>   the OPP table. As Mark mentioned earlier, DT should be able to get
>   us this.

This is also possible from C code though. Or is there some case
where it isn't possible if we're sharing the same table with two
devices? I'm lost on when this would ever happen.

It feels like trying to keep the OPP table agnostic of the
consuming device and the device's binding is more trouble than
it's worth. Especially considering we have opp-shared and *-name
now.

> - The order in which the supplies need to be programmed. We have all
>   agreed to do this in code instead of inferring it from DT and this
>   patch series already does that.

Agreed. Encoding a sequence into DT doesn't sound very feasible.
How is this going to be handled though? I don't see any users of
the code we're reviewing here, so it's hard to grasp how things
will work. It would be really useful if we had some user of the
code included in the patch series to get the big picture.

> 
> I want to solve the first problem here and I don't see how it can be
> solved using such entries:
> 
> 	cpus {
> 		cpu@0 {
> 			compatible = "arm,cortex-a7";
> 			...
> 
>                         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";
> 		opp-shared;
> 
> 		opp@1000000000 {
> 			opp-hz = /bits/ 64 <1000000000>;
> 			opp-microvolt = <970000>, /* Supply 0 */
> 					<960000>, /* Supply 1 */
> 					<960000>; /* Supply 2 */
> 		};
>         };
> 
> The code can't figure out which of vcc0, vcc1, vcc2 is added first in
> the CPU node and so we need to get the order somehow. A separate
> binding as I mentioned earlier is a probably (ugly) solution.
> 
> > I think the problem is that Viresh wants the binding to be "self
> > describing" so that the OPP can be used without a driver knowing
> > that a supply corresponds to a particular column in the voltage
> > table.
> 
> Right, and that's what Mark suggested as well.
> 
> > I don't understand that though. Can't we set the supply
> > names from C code somewhere based on the consumer of the OPPs?
> 
> That's what this patch series is doing right now.
> 
> So, are you saying that the way this patchset does it is fine with you
> ?

That's just to handle the ordering of operations? I need to take
a minute and understand what's changing. You may have spent
plenty of time developing/updating, but I haven't spent near
enough time understanding what's going on in these patches to
give a thorough review.

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

[TEST PATCH] WIP: Test OPP multi regulator support with ti-opp-domain driver

[Dave Gerlach]

NOT FOR MERGE!

Introduce a test version of a 'ti-opp-domain' driver that will use new
multiple regulator support introduced to the OPP core by Viresh [1].
Tested on v4.9-rc1 with that series applied.  This is needed on TI
platforms like DRA7/AM57 in order to control both CPU regulator and
Adaptive Body Bias (ABB) regulator as described by Nishanth Menon here
[2]. These regulators must be scaled in sequence during an OPP
transition depending on whether or not the frequency is being scaled up
or down. Based on the new functionality provided by Viresh this driver
does the following:

* Call dev_pm_opp_set_regulators with the names of the two regulators
  that feed the CPU:
	* vdd is the 'cpu-supply' commonly used for cpufreq-dt but
	  renamed so the cpufreq-dt driver doesn't use it directly.
	  Note that this is supplied in board dts as it's external to
	  SoC.
	* vbb for the ABB regulator. This is provided in SoC dtsi as it
	  is internal to the SoC.
* Provide a platform set_opp function using
  dev_pm_opp_register_set_opp_helper that is called when an OPP
  transition is requested.
* Allow cpufreq-dt to probe which will work because no cpu-supply
  regulator is found so the driver proceeds and calls
  dev_pm_opp_set_rate which through the OPP core invokes the platform
  set_opp call we provided
* Platform set_opp call provided by this driver checks to see if we are
  scaling frequency up or down and based on this, scales vbb before vdd
  for up or the other way around for down.

In addition to that, this driver implements AVS Class 0 as described in
section 18.4.6.12 of AM572x TRM [3] using the same platform set_rate
hook added to the OPP core. There are registers that define the optimal
voltage for that specific piece of silicon for an OPP so this driver
simply looks up this optimal value and programs that for an OPP instead
of the nominal value.

Missing from this is a good way to ensure that cpufreq-dt does not just
proceed if no cpu-supply regulator is found but we were intending to
rely on a platform set_opp and multiple regulators.

[1] https://marc.info/?l=linux-pm&m=147746362402994&w=2
[2] https://marc.info/?l=linux-pm&m=145684495832764&w=2
[3] http://www.ti.com/lit/ug/spruhz6g/spruhz6g.pdf

Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
---
 arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi |   2 +-
 arch/arm/boot/dts/dra7.dtsi                     |  46 ++-
 drivers/soc/ti/Makefile                         |   2 +
 drivers/soc/ti/ti-opp-domain.c                  | 427 ++++++++++++++++++++++++
 4 files changed, 471 insertions(+), 6 deletions(-)
 create mode 100644 drivers/soc/ti/ti-opp-domain.c

diff --git a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
index 6df7829a2c15..d92551c15780 100644
--- a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
+++ b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
@@ -410,7 +410,7 @@
 };
 
 &cpu0 {
-	cpu0-supply = <&smps12_reg>;
+	vdd-supply = <&smps12_reg>;
 	voltage-tolerance = <1>;
 };
 
diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index d4fcd68f6349..311166b9e8c4 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -80,11 +80,7 @@
 			compatible = "arm,cortex-a15";
 			reg = <0>;
 
-			operating-points = <
-				/* kHz    uV */
-				1000000	1060000
-				1176000	1160000
-				>;
+			operating-points-v2 = <&cpu0_opp_table>;
 
 			clocks = <&dpll_mpu_ck>;
 			clock-names = "cpu";
@@ -95,6 +91,32 @@
 			cooling-min-level = <0>;
 			cooling-max-level = <2>;
 			#cooling-cells = <2>; /* min followed by max */
+
+			vbb-supply = <&abb_mpu>;
+		};
+	};
+
+	cpu0_opp_table: opp_table0 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		opp_nom@1000000000 {
+			opp-hz = /bits/ 64 <1000000000>;
+			opp-microvolt = <1060000 850000 1150000>,
+					<1060000 850000 1150000>;
+			opp-suspend;
+		};
+
+		opp_od@1176000000 {
+			opp-hz = /bits/ 64 <1176000000>;
+			opp-microvolt = <1160000 885000 1160000>,
+					<1160000 885000 1160000>;
+		};
+
+		opp_high@1500000000 {
+			opp-hz = /bits/ 64 <1500000000>;
+			opp-microvolt = <1210000 950000 1250000>,
+					< 1210000 950000 1250000>;
 		};
 	};
 
@@ -1966,6 +1988,20 @@
 			clocks = <&l3_iclk_div>;
 			clock-names = "fck";
 		};
+
+		oppdm_mpu: oppdm@4a003b20 {
+			compatible = "ti,omap5-oppdm";
+			#oppdm-cells = <0>;
+			reg = <0x4a003b20 0xc>;
+			ti,efuse-settings = <
+			/* uV   offset */
+			1060000 0x0
+			1160000 0x4
+			1210000 0x8
+			>;
+			ti,absolute-max-voltage-uv = <1500000>;
+		};
+
 	};
 
 	thermal_zones: thermal-zones {
diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
index 48ff3a79634f..e3595e3f1d6a 100644
--- a/drivers/soc/ti/Makefile
+++ b/drivers/soc/ti/Makefile
@@ -5,3 +5,5 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)	+= knav_qmss.o
 knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
 obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
 obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
+obj-y             += ti-opp-domain.o
+#obj-$(CONFIG_OPP_DOMAIN_TI)             += ti-opp-domain.o
diff --git a/drivers/soc/ti/ti-opp-domain.c b/drivers/soc/ti/ti-opp-domain.c
new file mode 100644
index 000000000000..33683548b326
--- /dev/null
+++ b/drivers/soc/ti/ti-opp-domain.c
@@ -0,0 +1,427 @@
+/*
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
+ *	Nishanth Menon <nm@ti.com>
+ *	Dave Gerlach <d-gerlach@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * TI OPP Domain driver that provides overrides into the regulator control
+ * for generic opp domains to handle devices with ABB regulator and/or
+ * SmartReflex Class0.
+ */
+#include <linux/clk.h>
+#include <linux/cpufreq.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+/**
+ * struct ti_oppdm_optimum_voltage_table - optimized voltage table
+ * @reference_uv:	reference voltage (usually Nominal voltage)
+ * @optimized_uv:	Optimized voltage from efuse
+ */
+struct ti_oppdm_optimum_voltage_table {
+	unsigned int reference_uv;
+	unsigned int optimized_uv;
+};
+
+/**
+ * struct ti_oppdm_data - OMAP specific opp domain data
+ * @vdd_reg:	VDD regulator
+ * @vbb_reg:	Body Bias regulator
+ * @vdd_table:	Optimized voltage mapping table
+ * @num_vdd_table: number of entries in vdd_table
+ * @vdd_absolute_max_voltage_uv: absolute maximum voltage in UV for the domain
+ */
+struct ti_oppdm_data {
+	struct regulator *vdd_reg;
+	struct regulator *vbb_reg;
+	struct ti_oppdm_optimum_voltage_table *vdd_table;
+	u32 num_vdd_table;
+	u32 vdd_absolute_max_voltage_uv;
+};
+
+static struct ti_oppdm_data opp_data;
+/**
+ * struct ti_oppdm_of_data - device tree match data
+ * @desc:	opp domain descriptor for opp domain core
+ * @flags:	specific type of opp domain
+ * @efuse_voltage_mask: mask required for efuse register representing voltage
+ * @efuse_voltage_uv: Are the efuse entries in micro-volts? if not, assume
+ *		milli-volts.
+ */
+struct ti_oppdm_of_data {
+#define OPPDM_EFUSE_CLASS0_OPTIMIZED_VOLTAGE	BIT(1)
+#define OPPDM_HAS_NO_ABB			BIT(2)
+	const u8 flags;
+	const u32 efuse_voltage_mask;
+	const bool efuse_voltage_uv;
+};
+
+/**
+ * oppdm_store_optimized_voltages() - store optimized voltages
+ * @dev:	opp domain device for which we need to store info
+ * @data:	data specific to the device
+ *
+ * Picks up efuse based optimized voltages for VDD unique per device and
+ * stores it in internal data structure for use during transition requests.
+ *
+ * Return: If successful, 0, else appropriate error value.
+ */
+static int oppdm_store_optimized_voltages(struct device *dev,
+					  struct ti_oppdm_data *data)
+{
+	void __iomem *base;
+	struct property *prop;
+	struct resource *res;
+	const __be32 *val;
+	int proplen, i;
+	int ret = 0;
+	struct ti_oppdm_optimum_voltage_table *table;
+	const struct ti_oppdm_of_data *of_data = dev_get_drvdata(dev);
+
+	/* pick up Efuse based voltages */
+	res = platform_get_resource(to_platform_device(dev), IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "Unable to get IO resource\n");
+		ret = -ENODEV;
+		goto out_map;
+	}
+
+	base = ioremap_nocache(res->start, resource_size(res));
+	if (!base) {
+		dev_err(dev, "Unable to map Efuse registers\n");
+		ret = -ENOMEM;
+		goto out_map;
+	}
+
+	/* Fetch efuse-settings. */
+	prop = of_find_property(dev->of_node, "ti,efuse-settings", NULL);
+	if (!prop) {
+		dev_err(dev, "No 'ti,efuse-settings' property found\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	proplen = prop->length / sizeof(int);
+	data->num_vdd_table = proplen / 2;
+	/* Verify for corrupted OPP entries in dt */
+	if (data->num_vdd_table * 2 * sizeof(int) != prop->length) {
+		dev_err(dev, "Invalid 'ti,efuse-settings'\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = of_property_read_u32(dev->of_node, "ti,absolute-max-voltage-uv",
+				   &data->vdd_absolute_max_voltage_uv);
+	if (ret) {
+		dev_err(dev, "ti,absolute-max-voltage-uv is missing\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	table = kzalloc(sizeof(*data->vdd_table) *
+				  data->num_vdd_table, GFP_KERNEL);
+	if (!table) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	data->vdd_table = table;
+
+	val = prop->value;
+	for (i = 0; i < data->num_vdd_table; i++, table++) {
+		u32 efuse_offset;
+		u32 tmp;
+
+		table->reference_uv = be32_to_cpup(val++);
+		efuse_offset = be32_to_cpup(val++);
+
+		tmp = readl(base + efuse_offset);
+		tmp &= of_data->efuse_voltage_mask;
+		tmp >>= __ffs(of_data->efuse_voltage_mask);
+
+		table->optimized_uv = of_data->efuse_voltage_uv ? tmp :
+					tmp * 1000;
+
+		dev_dbg(dev, "[%d] efuse=0x%08x volt_table=%d vset=%d\n",
+			i, efuse_offset, table->reference_uv,
+			table->optimized_uv);
+
+		/*
+		 * Some older samples might not have optimized efuse
+		 * Use reference voltage for those - just add debug message
+		 * for them.
+		 */
+		if (!table->optimized_uv) {
+			dev_dbg(dev, "[%d] efuse=0x%08x volt_table=%d:vset0\n",
+				i, efuse_offset, table->reference_uv);
+			table->optimized_uv = table->reference_uv;
+		}
+	}
+out:
+	iounmap(base);
+out_map:
+	return ret;
+}
+
+/**
+ * oppdm_free_optimized_voltages() - free resources for optimized voltages
+ * @dev:	opp domain device for which we need to free info
+ * @data:	data specific to the device
+ */
+static void oppdm_free_optimized_voltages(struct device *dev,
+					  struct ti_oppdm_data *data)
+{
+	kfree(data->vdd_table);
+	data->vdd_table = NULL;
+	data->num_vdd_table = 0;
+}
+
+/**
+ * oppdm_get_optimal_vdd_voltage() - Finds optimal voltage for the domain
+ * @dev:	opp domain device for which we need to find info
+ * @data:	data specific to the device
+ * @reference_uv:	reference voltage (OPP voltage) for which we need value
+ *
+ * Return: if a match is found, return optimized voltage, else return
+ * reference_uv, also return reference_uv if no optimization is needed.
+ */
+static int oppdm_get_optimal_vdd_voltage(struct device *dev,
+					 struct ti_oppdm_data *data,
+					 int reference_uv)
+{
+	int i;
+	struct ti_oppdm_optimum_voltage_table *table;
+
+	if (!data->num_vdd_table)
+		return reference_uv;
+
+	table = data->vdd_table;
+	if (!table)
+		return -EINVAL;
+
+	/* Find a exact match - this list is usually very small */
+	for (i = 0; i < data->num_vdd_table; i++, table++)
+		if (table->reference_uv == reference_uv)
+			return table->optimized_uv;
+
+	/* IF things are screwed up, we'd make a mess on console.. ratelimit */
+	dev_err_ratelimited(dev, "%s: Failed optimized voltage match for %d\n",
+			    __func__, reference_uv);
+	return reference_uv;
+}
+
+/**
+ * ti_oppdm_set_opp() - do the opp domain transition
+ * @dev:	opp domain device for which we are doing the transition
+ * @data:	information on regulators and new and old opps provided by
+ *		opp core to use in transition
+ *
+ * Return: If successful, 0, else appropriate error value.
+ */
+int ti_oppdm_set_opp(struct device *dev, struct dev_pm_set_opp_data *data)
+{
+	struct dev_pm_opp_supply *old_supply_vdd = &data->old_opp.supplies[0];
+	struct dev_pm_opp_supply *old_supply_vbb = &data->old_opp.supplies[1];
+	struct dev_pm_opp_supply *new_supply_vdd = &data->new_opp.supplies[0];
+	struct dev_pm_opp_supply *new_supply_vbb = &data->new_opp.supplies[1];
+	unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate;
+	struct clk *clk = data->clk;
+	struct regulator *vdd_reg = data->regulators[0];
+	struct regulator *vbb_reg = data->regulators[1];
+	int vdd_uv;
+	int ret;
+
+	vdd_uv = oppdm_get_optimal_vdd_voltage(dev, &opp_data, new_supply_vbb->u_volt);
+
+	/* Scaling up? Scale voltage before frequency */
+	if (freq > old_freq) {
+		/* Regulator not available for device */
+
+		dev_dbg(dev, "vbb pre %luuV[min %luuV max %luuV]\n",
+			new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
+			new_supply_vbb->u_volt_max);
+
+		ret = regulator_set_voltage_triplet(vbb_reg,
+						    new_supply_vbb->u_volt_min,
+						    new_supply_vbb->u_volt,
+						    new_supply_vbb->u_volt_max);
+		if (ret) {
+			dev_err(dev, "vbb failed for %luuV[min %luuV max %luuV]\n",
+				new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
+				new_supply_vbb->u_volt_max);
+			return ret;
+		}
+
+		dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
+			new_supply_vdd->u_volt_min, new_supply_vdd->u_volt,
+			new_supply_vdd->u_volt_max);
+
+		ret = regulator_set_voltage_triplet(vdd_reg,
+						    new_supply_vdd->u_volt_min,
+						    new_supply_vdd->u_volt,
+						    new_supply_vdd->u_volt_max);
+		if (ret)
+			dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n",
+				__func__, new_supply_vdd->u_volt_min,
+				new_supply_vdd->u_volt,
+				new_supply_vdd->u_volt_max, ret);
+	}
+
+	/* Change frequency */
+	dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",
+		__func__, old_freq, freq);
+
+	ret = clk_set_rate(clk, freq);
+	if (ret) {
+		dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
+			ret);
+	}
+
+	/* Scaling down? Scale voltage after frequency */
+	if (freq < old_freq) {
+		dev_dbg(dev, "vbb post %luuV[min %luuV max %luuV]\n",
+			 new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
+			 new_supply_vbb->u_volt_max);
+
+		ret = regulator_set_voltage_triplet(vbb_reg,
+						    new_supply_vbb->u_volt_min,
+						    new_supply_vbb->u_volt,
+						    new_supply_vbb->u_volt_max);
+		if (ret) {
+			dev_err(dev, "vbb failed for %luuV[min %luuV max %luuV]\n",
+				new_supply_vbb->u_volt,
+				new_supply_vbb->u_volt_min,
+				new_supply_vbb->u_volt_max);
+			return ret;
+		}
+
+		dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
+			new_supply_vdd->u_volt_min, new_supply_vdd->u_volt,
+			new_supply_vdd->u_volt_max);
+
+		ret = regulator_set_voltage_triplet(vdd_reg,
+						    new_supply_vdd->u_volt_min,
+						    new_supply_vdd->u_volt,
+						    new_supply_vdd->u_volt_max);
+		if (ret)
+			dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n",
+				__func__, new_supply_vdd->u_volt_min,
+				new_supply_vdd->u_volt,
+				new_supply_vdd->u_volt_max, ret);
+	}
+
+	return 0;
+
+restore_freq:
+	ret = clk_set_rate(clk, old_freq);
+	if (ret)
+		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
+			__func__, old_freq);
+restore_voltage:
+	/* This shouldn't harm even if the voltages weren't updated earlier */
+	if (old_supply_vdd->u_volt) {
+		dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
+			old_supply_vdd->u_volt_min, old_supply_vdd->u_volt,
+			old_supply_vdd->u_volt_max);
+
+		ret = regulator_set_voltage_triplet(vdd_reg,
+						    old_supply_vdd->u_volt_min,
+						    old_supply_vdd->u_volt,
+						    old_supply_vdd->u_volt_max);
+		if (ret)
+			dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n",
+				__func__, old_supply_vdd->u_volt_min,
+				old_supply_vdd->u_volt,
+				old_supply_vdd->u_volt_max, ret);
+	}
+
+	return ret;
+}
+
+static const struct ti_oppdm_of_data omap_generic_of_data = {
+};
+
+static const struct ti_oppdm_of_data omap_omap5_of_data = {
+	.flags = OPPDM_EFUSE_CLASS0_OPTIMIZED_VOLTAGE,
+	.efuse_voltage_mask = 0xFFF,
+	.efuse_voltage_uv = false,
+};
+
+static const struct ti_oppdm_of_data omap_omap5core_of_data = {
+	.flags = OPPDM_EFUSE_CLASS0_OPTIMIZED_VOLTAGE | OPPDM_HAS_NO_ABB,
+	.efuse_voltage_mask = 0xFFF,
+	.efuse_voltage_uv = false,
+};
+
+static const struct of_device_id ti_oppdm_of_match[] = {
+	{.compatible = "ti,omap-oppdm", .data = &omap_generic_of_data},
+	{.compatible = "ti,omap5-oppdm", .data = &omap_omap5_of_data},
+	{.compatible = "ti,omap5-core-oppdm", .data = &omap_omap5core_of_data},
+	{},
+};
+
+static int ti_oppdm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device *cpu_dev = get_cpu_device(0); /* Gross hack */
+	const struct of_device_id *match;
+	struct pm_opp_domain_dev *oppdm_dev;
+	int ret = 0;
+	const struct ti_oppdm_of_data *of_data;
+	const char *names[] = {"vdd", "vbb"};
+
+	ret = dev_pm_opp_set_regulators(cpu_dev, names,
+					ARRAY_SIZE(names));
+
+	if (ret)
+		return ret;
+
+	match = of_match_device(ti_oppdm_of_match, dev);
+	if (!match) {
+		/* We do not expect this to happen */
+		dev_err(dev, "%s: Unable to match device\n", __func__);
+		return -ENODEV;
+	}
+	if (!match->data) {
+		/* Again, unlikely.. but mistakes do happen */
+		dev_err(dev, "%s: Bad data in match\n", __func__);
+		return -EINVAL;
+	}
+	of_data = match->data;
+
+	dev_set_drvdata(dev, (void *)of_data);
+	/* If we need optimized voltage */
+	if (of_data->flags & OPPDM_EFUSE_CLASS0_OPTIMIZED_VOLTAGE) {
+		ret = oppdm_store_optimized_voltages(dev, &opp_data);
+	}
+
+	dev_pm_opp_register_set_opp_helper(cpu_dev, ti_oppdm_set_opp);
+
+	return ret;
+}
+
+MODULE_DEVICE_TABLE(of, ti_oppdm_of_match);
+
+static struct platform_driver ti_oppdm_driver = {
+	.probe = ti_oppdm_probe,
+	.driver = {
+		   .name = "ti_oppdm",
+		   .owner = THIS_MODULE,
+		   .of_match_table = of_match_ptr(ti_oppdm_of_match),
+		   },
+};
+module_platform_driver(ti_oppdm_driver);
+
+MODULE_DESCRIPTION("Texas Instruments OMAP OPP Domain driver");
+MODULE_AUTHOR("Texas Instruments Inc.");
+MODULE_LICENSE("GPL v2");
-- 
2.9.3

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

[Dave Gerlach]

Hi,
On 11/15/2016 12:56 PM, Stephen Boyd wrote:
> On 11/15, Viresh Kumar wrote:
>> On 14-11-16, 18:13, Stephen Boyd wrote:
>>> On 11/14, Rob Herring wrote:
>>>> On Fri, Nov 11, 2016 at 08:41:20AM +0530, Viresh Kumar wrote:
>>>>> On 10-11-16, 14:51, Stephen Boyd wrote:
>>>>>>
>>>>>> 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";
>>>>
>>>> Uh, no. You already have the names in the *-supply properties. Yes, they
>>>> are a PIA to retrieve compared to a *-names property, but that is the
>>>> nature of this style of binding.
>>
>> Its not just PIA, but impossible AFAICT.
>>
>> There are two important pieces of information we need for multiple
>> regulator support:
>> - Which regulator in the consumer node corresponds to which entry in
>>   the OPP table. As Mark mentioned earlier, DT should be able to get
>>   us this.
>
> This is also possible from C code though. Or is there some case
> where it isn't possible if we're sharing the same table with two
> devices? I'm lost on when this would ever happen.
>
> It feels like trying to keep the OPP table agnostic of the
> consuming device and the device's binding is more trouble than
> it's worth. Especially considering we have opp-shared and *-name
> now.

I agree with this, I do not like having to pass a list of regulator 
names to the opp core that I *hope* the device I am controlling has 
provided. The intent seems to be to use the cpufreq-dt driver as is and 
not pass any cpu-supply anymore so the cpufreq-dt driver has no 
knowledge of what regulators are present (it operates as it would today 
on a system with no regulator required). But as is it will move forward 
regardless of whether or not we actually intended to provide a multi 
regulator set up or platform set_opp helper, and this probably isn't 
ideal. I would think cpufreq-dt/opp core should be have knowledge of 
what regulators are needed to achieve these opp transitions and make 
sure everything is in place before moving ahead.

>
>> - The order in which the supplies need to be programmed. We have all
>>   agreed to do this in code instead of inferring it from DT and this
>>   patch series already does that.
>
> Agreed. Encoding a sequence into DT doesn't sound very feasible.
> How is this going to be handled though? I don't see any users of
> the code we're reviewing here, so it's hard to grasp how things
> will work. It would be really useful if we had some user of the
> code included in the patch series to get the big picture.

I have sent a patch in reply to the cover letter of this series showing 
the driver that I used to test multi regulator on TI am57x platform and 
wrote as much detail as I could on how I used what Viresh has provided. 
Perhaps that will show how this can be used and help to see what's 
missing from the core implementation here.

Previous discussions drove me to pass regulators and necessary values in 
the DT but do all sequencing from the driver from fixed code without 
inferring anything from the device tree.

Regards,
Dave

>
>>
>> I want to solve the first problem here and I don't see how it can be
>> solved using such entries:
>>
>> 	cpus {
>> 		cpu@0 {
>> 			compatible = "arm,cortex-a7";
>> 			...
>>
>>                         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";
>> 		opp-shared;
>>
>> 		opp@1000000000 {
>> 			opp-hz = /bits/ 64 <1000000000>;
>> 			opp-microvolt = <970000>, /* Supply 0 */
>> 					<960000>, /* Supply 1 */
>> 					<960000>; /* Supply 2 */
>> 		};
>>         };
>>
>> The code can't figure out which of vcc0, vcc1, vcc2 is added first in
>> the CPU node and so we need to get the order somehow. A separate
>> binding as I mentioned earlier is a probably (ugly) solution.
>>
>>> I think the problem is that Viresh wants the binding to be "self
>>> describing" so that the OPP can be used without a driver knowing
>>> that a supply corresponds to a particular column in the voltage
>>> table.
>>
>> Right, and that's what Mark suggested as well.
>>
>>> I don't understand that though. Can't we set the supply
>>> names from C code somewhere based on the consumer of the OPPs?
>>
>> That's what this patch series is doing right now.
>>
>> So, are you saying that the way this patchset does it is fine with you
>> ?
>
> That's just to handle the ordering of operations? I need to take
> a minute and understand what's changing. You may have spent
> plenty of time developing/updating, but I haven't spent near
> enough time understanding what's going on in these patches to
> give a thorough review.
>

Re: [TEST PATCH] WIP: Test OPP multi regulator support with ti-opp-domain driver

[kbuild test robot]

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

Hi Dave,

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.9-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dave-Gerlach/WIP-Test-OPP-multi-regulator-support-with-ti-opp-domain-driver/20161116-084525
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All error/warnings (new ones prefixed by >>):

>> drivers/soc/ti/ti-opp-domain.c:231:49: warning: 'struct dev_pm_set_opp_data' declared inside parameter list will not be visible outside of this definition or declaration
    int ti_oppdm_set_opp(struct device *dev, struct dev_pm_set_opp_data *data)
                                                    ^~~~~~~~~~~~~~~~~~~
   drivers/soc/ti/ti-opp-domain.c: In function 'ti_oppdm_set_opp':
>> drivers/soc/ti/ti-opp-domain.c:233:50: error: dereferencing pointer to incomplete type 'struct dev_pm_set_opp_data'
     struct dev_pm_opp_supply *old_supply_vdd = &data->old_opp.supplies[0];
                                                     ^~
>> drivers/soc/ti/ti-opp-domain.c:244:71: error: dereferencing pointer to incomplete type 'struct dev_pm_opp_supply'
     vdd_uv = oppdm_get_optimal_vdd_voltage(dev, &opp_data, new_supply_vbb->u_volt);
                                                                          ^~
>> drivers/soc/ti/ti-opp-domain.c:330:1: warning: label 'restore_voltage' defined but not used [-Wunused-label]
    restore_voltage:
    ^~~~~~~~~~~~~~~
>> drivers/soc/ti/ti-opp-domain.c:325:1: warning: label 'restore_freq' defined but not used [-Wunused-label]
    restore_freq:
    ^~~~~~~~~~~~
>> drivers/soc/ti/ti-opp-domain.c:234:28: warning: unused variable 'old_supply_vbb' [-Wunused-variable]
     struct dev_pm_opp_supply *old_supply_vbb = &data->old_opp.supplies[1];
                               ^~~~~~~~~~~~~~
   drivers/soc/ti/ti-opp-domain.c: In function 'ti_oppdm_probe':
   drivers/soc/ti/ti-opp-domain.c:383:8: error: implicit declaration of function 'dev_pm_opp_set_regulators' [-Werror=implicit-function-declaration]
     ret = dev_pm_opp_set_regulators(cpu_dev, names,
           ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/soc/ti/ti-opp-domain.c:408:2: error: implicit declaration of function 'dev_pm_opp_register_set_opp_helper' [-Werror=implicit-function-declaration]
     dev_pm_opp_register_set_opp_helper(cpu_dev, ti_oppdm_set_opp);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/soc/ti/ti-opp-domain.c:378:28: warning: unused variable 'oppdm_dev' [-Wunused-variable]
     struct pm_opp_domain_dev *oppdm_dev;
                               ^~~~~~~~~
   At top level:
   drivers/soc/ti/ti-opp-domain.c:181:13: warning: 'oppdm_free_optimized_voltages' defined but not used [-Wunused-function]
    static void oppdm_free_optimized_voltages(struct device *dev,
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +233 drivers/soc/ti/ti-opp-domain.c

   225	 * @dev:	opp domain device for which we are doing the transition
   226	 * @data:	information on regulators and new and old opps provided by
   227	 *		opp core to use in transition
   228	 *
   229	 * Return: If successful, 0, else appropriate error value.
   230	 */
 > 231	int ti_oppdm_set_opp(struct device *dev, struct dev_pm_set_opp_data *data)
   232	{
 > 233		struct dev_pm_opp_supply *old_supply_vdd = &data->old_opp.supplies[0];
 > 234		struct dev_pm_opp_supply *old_supply_vbb = &data->old_opp.supplies[1];
   235		struct dev_pm_opp_supply *new_supply_vdd = &data->new_opp.supplies[0];
   236		struct dev_pm_opp_supply *new_supply_vbb = &data->new_opp.supplies[1];
   237		unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate;
   238		struct clk *clk = data->clk;
   239		struct regulator *vdd_reg = data->regulators[0];
   240		struct regulator *vbb_reg = data->regulators[1];
   241		int vdd_uv;
   242		int ret;
   243	
 > 244		vdd_uv = oppdm_get_optimal_vdd_voltage(dev, &opp_data, new_supply_vbb->u_volt);
   245	
   246		/* Scaling up? Scale voltage before frequency */
   247		if (freq > old_freq) {
   248			/* Regulator not available for device */
   249	
   250			dev_dbg(dev, "vbb pre %luuV[min %luuV max %luuV]\n",
   251				new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
   252				new_supply_vbb->u_volt_max);
   253	
   254			ret = regulator_set_voltage_triplet(vbb_reg,
   255							    new_supply_vbb->u_volt_min,
   256							    new_supply_vbb->u_volt,
   257							    new_supply_vbb->u_volt_max);
   258			if (ret) {
   259				dev_err(dev, "vbb failed for %luuV[min %luuV max %luuV]\n",
   260					new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
   261					new_supply_vbb->u_volt_max);
   262				return ret;
   263			}
   264	
   265			dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
   266				new_supply_vdd->u_volt_min, new_supply_vdd->u_volt,
   267				new_supply_vdd->u_volt_max);
   268	
   269			ret = regulator_set_voltage_triplet(vdd_reg,
   270							    new_supply_vdd->u_volt_min,
   271							    new_supply_vdd->u_volt,
   272							    new_supply_vdd->u_volt_max);
   273			if (ret)
   274				dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n",
   275					__func__, new_supply_vdd->u_volt_min,
   276					new_supply_vdd->u_volt,
   277					new_supply_vdd->u_volt_max, ret);
   278		}
   279	
   280		/* Change frequency */
   281		dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n",
   282			__func__, old_freq, freq);
   283	
   284		ret = clk_set_rate(clk, freq);
   285		if (ret) {
   286			dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
   287				ret);
   288		}
   289	
   290		/* Scaling down? Scale voltage after frequency */
   291		if (freq < old_freq) {
   292			dev_dbg(dev, "vbb post %luuV[min %luuV max %luuV]\n",
   293				 new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
   294				 new_supply_vbb->u_volt_max);
   295	
   296			ret = regulator_set_voltage_triplet(vbb_reg,
   297							    new_supply_vbb->u_volt_min,
   298							    new_supply_vbb->u_volt,
   299							    new_supply_vbb->u_volt_max);
   300			if (ret) {
   301				dev_err(dev, "vbb failed for %luuV[min %luuV max %luuV]\n",
   302					new_supply_vbb->u_volt,
   303					new_supply_vbb->u_volt_min,
   304					new_supply_vbb->u_volt_max);
   305				return ret;
   306			}
   307	
   308			dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
   309				new_supply_vdd->u_volt_min, new_supply_vdd->u_volt,
   310				new_supply_vdd->u_volt_max);
   311	
   312			ret = regulator_set_voltage_triplet(vdd_reg,
   313							    new_supply_vdd->u_volt_min,
   314							    new_supply_vdd->u_volt,
   315							    new_supply_vdd->u_volt_max);
   316			if (ret)
   317				dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n",
   318					__func__, new_supply_vdd->u_volt_min,
   319					new_supply_vdd->u_volt,
   320					new_supply_vdd->u_volt_max, ret);
   321		}
   322	
   323		return 0;
   324	
 > 325	restore_freq:
   326		ret = clk_set_rate(clk, old_freq);
   327		if (ret)
   328			dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
   329				__func__, old_freq);
 > 330	restore_voltage:
   331		/* This shouldn't harm even if the voltages weren't updated earlier */
   332		if (old_supply_vdd->u_volt) {
   333			dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
   334				old_supply_vdd->u_volt_min, old_supply_vdd->u_volt,
   335				old_supply_vdd->u_volt_max);
   336	
   337			ret = regulator_set_voltage_triplet(vdd_reg,
   338							    old_supply_vdd->u_volt_min,
   339							    old_supply_vdd->u_volt,
   340							    old_supply_vdd->u_volt_max);
   341			if (ret)
   342				dev_err(dev, "%s: failed to set voltage (%lu %lu %lu mV): %d\n",
   343					__func__, old_supply_vdd->u_volt_min,
   344					old_supply_vdd->u_volt,
   345					old_supply_vdd->u_volt_max, ret);
   346		}
   347	
   348		return ret;
   349	}
   350	
   351	static const struct ti_oppdm_of_data omap_generic_of_data = {
   352	};
   353	
   354	static const struct ti_oppdm_of_data omap_omap5_of_data = {
   355		.flags = OPPDM_EFUSE_CLASS0_OPTIMIZED_VOLTAGE,
   356		.efuse_voltage_mask = 0xFFF,
   357		.efuse_voltage_uv = false,
   358	};
   359	
   360	static const struct ti_oppdm_of_data omap_omap5core_of_data = {
   361		.flags = OPPDM_EFUSE_CLASS0_OPTIMIZED_VOLTAGE | OPPDM_HAS_NO_ABB,
   362		.efuse_voltage_mask = 0xFFF,
   363		.efuse_voltage_uv = false,
   364	};
   365	
   366	static const struct of_device_id ti_oppdm_of_match[] = {
   367		{.compatible = "ti,omap-oppdm", .data = &omap_generic_of_data},
   368		{.compatible = "ti,omap5-oppdm", .data = &omap_omap5_of_data},
   369		{.compatible = "ti,omap5-core-oppdm", .data = &omap_omap5core_of_data},
   370		{},
   371	};
   372	
   373	static int ti_oppdm_probe(struct platform_device *pdev)
   374	{
   375		struct device *dev = &pdev->dev;
   376		struct device *cpu_dev = get_cpu_device(0); /* Gross hack */
   377		const struct of_device_id *match;
 > 378		struct pm_opp_domain_dev *oppdm_dev;
   379		int ret = 0;
   380		const struct ti_oppdm_of_data *of_data;
   381		const char *names[] = {"vdd", "vbb"};
   382	
 > 383		ret = dev_pm_opp_set_regulators(cpu_dev, names,
   384						ARRAY_SIZE(names));
   385	
   386		if (ret)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45092 bytes --]

Re: [TEST PATCH] WIP: Test OPP multi regulator support with ti-opp-domain driver

[kbuild test robot]

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

Hi Dave,

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.9-rc5 next-20161115]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Dave-Gerlach/WIP-Test-OPP-multi-regulator-support-with-ti-opp-domain-driver/20161116-084525
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m68k 

All warnings (new ones prefixed by >>):

   drivers/soc/ti/ti-opp-domain.c:231:49: warning: 'struct dev_pm_set_opp_data' declared inside parameter list
    int ti_oppdm_set_opp(struct device *dev, struct dev_pm_set_opp_data *data)
                                                    ^
   drivers/soc/ti/ti-opp-domain.c:231:49: warning: its scope is only this definition or declaration, which is probably not what you want
   drivers/soc/ti/ti-opp-domain.c: In function 'ti_oppdm_set_opp':
   drivers/soc/ti/ti-opp-domain.c:233:50: error: dereferencing pointer to incomplete type
     struct dev_pm_opp_supply *old_supply_vdd = &data->old_opp.supplies[0];
                                                     ^
   drivers/soc/ti/ti-opp-domain.c:234:50: error: dereferencing pointer to incomplete type
     struct dev_pm_opp_supply *old_supply_vbb = &data->old_opp.supplies[1];
                                                     ^
   drivers/soc/ti/ti-opp-domain.c:235:50: error: dereferencing pointer to incomplete type
     struct dev_pm_opp_supply *new_supply_vdd = &data->new_opp.supplies[0];
                                                     ^
   drivers/soc/ti/ti-opp-domain.c:236:50: error: dereferencing pointer to incomplete type
     struct dev_pm_opp_supply *new_supply_vbb = &data->new_opp.supplies[1];
                                                     ^
   drivers/soc/ti/ti-opp-domain.c:237:31: error: dereferencing pointer to incomplete type
     unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate;
                                  ^
   drivers/soc/ti/ti-opp-domain.c:237:58: error: dereferencing pointer to incomplete type
     unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate;
                                                             ^
   drivers/soc/ti/ti-opp-domain.c:238:24: error: dereferencing pointer to incomplete type
     struct clk *clk = data->clk;
                           ^
   drivers/soc/ti/ti-opp-domain.c:239:34: error: dereferencing pointer to incomplete type
     struct regulator *vdd_reg = data->regulators[0];
                                     ^
   drivers/soc/ti/ti-opp-domain.c:240:34: error: dereferencing pointer to incomplete type
     struct regulator *vbb_reg = data->regulators[1];
                                     ^
   drivers/soc/ti/ti-opp-domain.c:244:71: error: dereferencing pointer to incomplete type
     vdd_uv = oppdm_get_optimal_vdd_voltage(dev, &opp_data, new_supply_vbb->u_volt);
                                                                          ^
   In file included from include/linux/printk.h:305:0,
                    from include/linux/kernel.h:13,
                    from include/linux/clk.h:16,
                    from drivers/soc/ti/ti-opp-domain.c:14:
   drivers/soc/ti/ti-opp-domain.c:251:18: error: dereferencing pointer to incomplete type
       new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
                     ^
   include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg'
          ##__VA_ARGS__);  \
            ^
>> drivers/soc/ti/ti-opp-domain.c:250:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(dev, "vbb pre %luuV[min %luuV max %luuV]\n",
      ^
   drivers/soc/ti/ti-opp-domain.c:251:42: error: dereferencing pointer to incomplete type
       new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
                                             ^
   include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg'
          ##__VA_ARGS__);  \
            ^
>> drivers/soc/ti/ti-opp-domain.c:250:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(dev, "vbb pre %luuV[min %luuV max %luuV]\n",
      ^
   drivers/soc/ti/ti-opp-domain.c:252:18: error: dereferencing pointer to incomplete type
       new_supply_vbb->u_volt_max);
                     ^
   include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg'
          ##__VA_ARGS__);  \
            ^
>> drivers/soc/ti/ti-opp-domain.c:250:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(dev, "vbb pre %luuV[min %luuV max %luuV]\n",
      ^
   drivers/soc/ti/ti-opp-domain.c:255:25: error: dereferencing pointer to incomplete type
              new_supply_vbb->u_volt_min,
                            ^
   drivers/soc/ti/ti-opp-domain.c:256:25: error: dereferencing pointer to incomplete type
              new_supply_vbb->u_volt,
                            ^
   drivers/soc/ti/ti-opp-domain.c:257:25: error: dereferencing pointer to incomplete type
              new_supply_vbb->u_volt_max);
                            ^
   drivers/soc/ti/ti-opp-domain.c:260:19: error: dereferencing pointer to incomplete type
        new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
                      ^
   drivers/soc/ti/ti-opp-domain.c:260:43: error: dereferencing pointer to incomplete type
        new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
                                              ^
   drivers/soc/ti/ti-opp-domain.c:261:19: error: dereferencing pointer to incomplete type
        new_supply_vbb->u_volt_max);
                      ^
   In file included from include/linux/printk.h:305:0,
                    from include/linux/kernel.h:13,
                    from include/linux/clk.h:16,
                    from drivers/soc/ti/ti-opp-domain.c:14:
   drivers/soc/ti/ti-opp-domain.c:266:18: error: dereferencing pointer to incomplete type
       new_supply_vdd->u_volt_min, new_supply_vdd->u_volt,
                     ^
   include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg'
          ##__VA_ARGS__);  \
            ^
   drivers/soc/ti/ti-opp-domain.c:265:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
      ^
   drivers/soc/ti/ti-opp-domain.c:266:46: error: dereferencing pointer to incomplete type
       new_supply_vdd->u_volt_min, new_supply_vdd->u_volt,
                                                 ^
   include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg'
          ##__VA_ARGS__);  \
            ^
   drivers/soc/ti/ti-opp-domain.c:265:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
      ^
   drivers/soc/ti/ti-opp-domain.c:267:18: error: dereferencing pointer to incomplete type
       new_supply_vdd->u_volt_max);
                     ^
   include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg'
          ##__VA_ARGS__);  \
            ^
   drivers/soc/ti/ti-opp-domain.c:265:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(dev, "%s: voltages (mV): %lu %lu %lu\n", __func__,
      ^
   drivers/soc/ti/ti-opp-domain.c:270:25: error: dereferencing pointer to incomplete type
              new_supply_vdd->u_volt_min,
                            ^
   drivers/soc/ti/ti-opp-domain.c:271:25: error: dereferencing pointer to incomplete type
              new_supply_vdd->u_volt,
                            ^
   drivers/soc/ti/ti-opp-domain.c:272:25: error: dereferencing pointer to incomplete type
              new_supply_vdd->u_volt_max);
                            ^
   drivers/soc/ti/ti-opp-domain.c:275:29: error: dereferencing pointer to incomplete type
        __func__, new_supply_vdd->u_volt_min,
                                ^
   drivers/soc/ti/ti-opp-domain.c:276:19: error: dereferencing pointer to incomplete type
        new_supply_vdd->u_volt,
                      ^
   drivers/soc/ti/ti-opp-domain.c:277:19: error: dereferencing pointer to incomplete type
        new_supply_vdd->u_volt_max, ret);
                      ^
   In file included from include/linux/printk.h:305:0,
                    from include/linux/kernel.h:13,
                    from include/linux/clk.h:16,
                    from drivers/soc/ti/ti-opp-domain.c:14:
   drivers/soc/ti/ti-opp-domain.c:293:19: error: dereferencing pointer to incomplete type
        new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
                      ^
   include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg'
          ##__VA_ARGS__);  \
            ^
   drivers/soc/ti/ti-opp-domain.c:292:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(dev, "vbb post %luuV[min %luuV max %luuV]\n",
      ^
   drivers/soc/ti/ti-opp-domain.c:293:43: error: dereferencing pointer to incomplete type
        new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
                                              ^
   include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg'
          ##__VA_ARGS__);  \
            ^
   drivers/soc/ti/ti-opp-domain.c:292:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(dev, "vbb post %luuV[min %luuV max %luuV]\n",
      ^
   drivers/soc/ti/ti-opp-domain.c:294:19: error: dereferencing pointer to incomplete type
        new_supply_vbb->u_volt_max);
                      ^
   include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg'
          ##__VA_ARGS__);  \
            ^
   drivers/soc/ti/ti-opp-domain.c:292:3: note: in expansion of macro 'dev_dbg'
      dev_dbg(dev, "vbb post %luuV[min %luuV max %luuV]\n",
      ^

vim +/dev_dbg +250 drivers/soc/ti/ti-opp-domain.c

   234		struct dev_pm_opp_supply *old_supply_vbb = &data->old_opp.supplies[1];
   235		struct dev_pm_opp_supply *new_supply_vdd = &data->new_opp.supplies[0];
   236		struct dev_pm_opp_supply *new_supply_vbb = &data->new_opp.supplies[1];
   237		unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate;
   238		struct clk *clk = data->clk;
   239		struct regulator *vdd_reg = data->regulators[0];
   240		struct regulator *vbb_reg = data->regulators[1];
   241		int vdd_uv;
   242		int ret;
   243	
   244		vdd_uv = oppdm_get_optimal_vdd_voltage(dev, &opp_data, new_supply_vbb->u_volt);
   245	
   246		/* Scaling up? Scale voltage before frequency */
   247		if (freq > old_freq) {
   248			/* Regulator not available for device */
   249	
 > 250			dev_dbg(dev, "vbb pre %luuV[min %luuV max %luuV]\n",
   251				new_supply_vbb->u_volt, new_supply_vbb->u_volt_min,
   252				new_supply_vbb->u_volt_max);
   253	
   254			ret = regulator_set_voltage_triplet(vbb_reg,
   255							    new_supply_vbb->u_volt_min,
   256							    new_supply_vbb->u_volt,
   257							    new_supply_vbb->u_volt_max);
   258			if (ret) {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38259 bytes --]

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

[Viresh Kumar]

On 15-11-16, 10:56, Stephen Boyd wrote:
> This is also possible from C code though.

Right and this is what this patchset is doing right now. To make it
clear, the order of regulator names in the call
dev_pm_opp_set_regulators() is used now to communicate the order in
which entries are present in the OPP table.

> Or is there some case
> where it isn't possible if we're sharing the same table with two
> devices?

Even in that case it will be possible to set regulators separately, so
that's not a problem.

> I'm lost on when this would ever happen.

It would happen in case of Krait for example, where CPUs manage DVFS
separately but their tables may all be same.

> It feels like trying to keep the OPP table agnostic of the
> consuming device and the device's binding is more trouble than
> it's worth. Especially considering we have opp-shared and *-name
> now.

Right.

> > - The order in which the supplies need to be programmed. We have all
> >   agreed to do this in code instead of inferring it from DT and this
> >   patch series already does that.
> 
> Agreed. Encoding a sequence into DT doesn't sound very feasible.
> How is this going to be handled though? I don't see any users of
> the code we're reviewing here, so it's hard to grasp how things
> will work. It would be really useful if we had some user of the
> code included in the patch series to get the big picture.

The TI guys would be doing it soon. The sequence will be handled by
platform specific set_opp() callbacks now. So, there is nothing in the
core for that.

> > So, are you saying that the way this patchset does it is fine with you
> > ?
> 
> That's just to handle the ordering of operations?

Not just that. The blocking question here is that "Do we want to know
the sequence in which the entries for multiple regulators are present
in the OPP nodes from the DT? Or is it fine to handle that in code".

And AFAIU, you are saying that we better handle that in code as
handling that in DT is going to be nightmare without a new ugly
property.

-- 
viresh

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

[Viresh Kumar]

On 15-11-16, 16:11, Dave Gerlach wrote:
> On 11/15/2016 12:56 PM, Stephen Boyd wrote:
> >On 11/15, Viresh Kumar wrote:
> >>There are two important pieces of information we need for multiple
> >>regulator support:
> >>- Which regulator in the consumer node corresponds to which entry in
> >>  the OPP table. As Mark mentioned earlier, DT should be able to get
> >>  us this.
> >
> >This is also possible from C code though. Or is there some case
> >where it isn't possible if we're sharing the same table with two
> >devices? I'm lost on when this would ever happen.
> >
> >It feels like trying to keep the OPP table agnostic of the
> >consuming device and the device's binding is more trouble than
> >it's worth. Especially considering we have opp-shared and *-name
> >now.
> 
> I agree with this, I do not like having to pass a list of regulator names to
> the opp core that I *hope* the device I am controlling has provided.

What do you mean by that? Are you saying this from DT's point of view
or of the code? i.e. Are you saying that you don't like the
dev_pm_opp_set_regulators() API ?

> The
> intent seems to be to use the cpufreq-dt driver as is and not pass any

I would like to kill all regulators code from cpufreq-dt sometime
soon. All that is left there is making sure we have a regulator in
place, but I strongly feel OPP core is the right place for doing that
now.

> cpu-supply anymore so the cpufreq-dt driver has no knowledge of what
> regulators are present (it operates as it would today on a system with no
> regulator required). But as is it will move forward regardless of whether or
> not we actually intended to provide a multi regulator set up or platform
> set_opp helper, and this probably isn't ideal.

Yes and that's why I am more inclined towards my above comment. We
shall make it consistent.

> I would think cpufreq-dt/opp
> core should be have knowledge of what regulators are needed to achieve these
> opp transitions and make sure everything is in place before moving ahead.

The last patch in my series does what you are looking for:

PM / OPP: Don't assume platform doesn't have regulators

Isn't it ?

-- 
viresh

Re: [TEST PATCH] WIP: Test OPP multi regulator support with ti-opp-domain driver

[Viresh Kumar]

Thanks for this Dave :)

On 15-11-16, 16:10, Dave Gerlach wrote:
> NOT FOR MERGE!
> 
> Introduce a test version of a 'ti-opp-domain' driver that will use new
> multiple regulator support introduced to the OPP core by Viresh [1].
> Tested on v4.9-rc1 with that series applied.  This is needed on TI
> platforms like DRA7/AM57 in order to control both CPU regulator and
> Adaptive Body Bias (ABB) regulator as described by Nishanth Menon here
> [2]. These regulators must be scaled in sequence during an OPP
> transition depending on whether or not the frequency is being scaled up
> or down. Based on the new functionality provided by Viresh this driver
> does the following:
> 
> * Call dev_pm_opp_set_regulators with the names of the two regulators
>   that feed the CPU:
> 	* vdd is the 'cpu-supply' commonly used for cpufreq-dt but
> 	  renamed so the cpufreq-dt driver doesn't use it directly.
> 	  Note that this is supplied in board dts as it's external to
> 	  SoC.

I think I can fix this somehow.. Lemme check.

> 	* vbb for the ABB regulator. This is provided in SoC dtsi as it
> 	  is internal to the SoC.
> * Provide a platform set_opp function using
>   dev_pm_opp_register_set_opp_helper that is called when an OPP
>   transition is requested.
> * Allow cpufreq-dt to probe which will work because no cpu-supply
>   regulator is found so the driver proceeds and calls
>   dev_pm_opp_set_rate which through the OPP core invokes the platform
>   set_opp call we provided
> * Platform set_opp call provided by this driver checks to see if we are
>   scaling frequency up or down and based on this, scales vbb before vdd
>   for up or the other way around for down.
> 
> In addition to that, this driver implements AVS Class 0 as described in
> section 18.4.6.12 of AM572x TRM [3] using the same platform set_rate
> hook added to the OPP core. There are registers that define the optimal
> voltage for that specific piece of silicon for an OPP so this driver
> simply looks up this optimal value and programs that for an OPP instead
> of the nominal value.
> 
> Missing from this is a good way to ensure that cpufreq-dt does not just
> proceed if no cpu-supply regulator is found but we were intending to
> rely on a platform set_opp and multiple regulators.
> 
> [1] https://marc.info/?l=linux-pm&m=147746362402994&w=2
> [2] https://marc.info/?l=linux-pm&m=145684495832764&w=2
> [3] http://www.ti.com/lit/ug/spruhz6g/spruhz6g.pdf
> 
> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> ---
>  arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi |   2 +-
>  arch/arm/boot/dts/dra7.dtsi                     |  46 ++-
>  drivers/soc/ti/Makefile                         |   2 +
>  drivers/soc/ti/ti-opp-domain.c                  | 427 ++++++++++++++++++++++++

I would rather ask you to move this to drivers/base/power/opp/ 

-- 
viresh

Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

[Viresh Kumar]

On 26-10-16, 12:02, Viresh Kumar wrote:
> Hi,
> 
> Some platforms (like TI) have complex DVFS configuration for CPU
> devices, where multiple regulators are required to be configured to
> change DVFS state of the device. This was explained well by Nishanth
> earlier [1].
> 
> One of the major complaints around multiple regulators case was that the
> DT isn't responsible in any way to represent the ordering in which
> multiple supplies need to be programmed, before or after frequency
> change. It was considered in this patch and such information is left to
> the platform specific OPP driver now, which can register its own
> opp_set_rate() callback with the OPP core and the OPP core will then
> call it during DVFS.
> 
> The patches are tested on Exynos5250 (Dual A15). I have hacked around DT
> and code to pass values for multiple regulators and verified that they
> are all properly read by the kernel (using debugfs interface).
> 
> Dave Gerlach has already tested it on the real TI platforms and it works
> well for him.
> 
> This is rebased over: linux-next branch in the PM tree.
> 
> V2->V3:
> - The last patch is new
> - Removed a debug leftover pr_info() message
> - Renamed few names as s/set_rate/set_opp
> - Removed a TODO comment (as it is done now with this series)
> - created struct for min_uV and max_uV
> - kerneldoc comments for structures in pm_opp.h
> - s/const char */const char * const
> - use kasprintf()
> - Some more minor reformatting
> - More Ack/RBY tags added

Hi guys,

Can we please get this series reviewed quickly and come to a conclusion? It has
already taken a lot of time getting this merged and the present code seems to be
the best possible shot we have, AFAIU.

-- 
viresh

Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

[Mark Brown]

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

On Fri, Nov 18, 2016 at 08:36:36AM +0530, Viresh Kumar wrote:

> Can we please get this series reviewed quickly and come to a conclusion? It has
> already taken a lot of time getting this merged and the present code seems to be
> the best possible shot we have, AFAIU.

There already seems to be extensive, ongoing discusion about this...

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

Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

[Viresh Kumar]

On 18-11-16, 10:43, Mark Brown wrote:
> On Fri, Nov 18, 2016 at 08:36:36AM +0530, Viresh Kumar wrote:
> 
> > Can we please get this series reviewed quickly and come to a conclusion? It has
> > already taken a lot of time getting this merged and the present code seems to be
> > the best possible shot we have, AFAIU.
> 
> There already seems to be extensive, ongoing discusion about this...

And I am quite sure we are stuck again :)

I just wanted to say that we should get it to some sort of conclusion. And yes I
want to say thanks to all who invested their time on this thread :)

So the LAST remaining question is:

"How do we know (from the DT) the order in which entries for multiple regulators
are present in the OPP table?"

And I am not sure if we can do that without having a property like:

+               supply-names = "vcc0", "vcc1", "vcc2";

in the OPP table or the consumer device. And surely it isn't a clean enough
solution and that's why this series relied on the code to get such details.

Does someone have an alternative? If NO, can we go ahead with this series as is?

-- 
viresh

Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

[Mark Brown]

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

On Tue, Nov 22, 2016 at 09:19:22AM +0530, Viresh Kumar wrote:

> So the LAST remaining question is:

> "How do we know (from the DT) the order in which entries for multiple regulators
> are present in the OPP table?"
> 
> And I am not sure if we can do that without having a property like:
> 
> +               supply-names = "vcc0", "vcc1", "vcc2";
> 
> in the OPP table or the consumer device. And surely it isn't a clean enough
> solution and that's why this series relied on the code to get such details.
> 
> Does someone have an alternative? If NO, can we go ahead with this series as is?

I'm really not at all clear why this has to be in DT.  My understanding
was that this is basically a helper library for more specific bindings
which already have to hard code things like sequencing so surely they'd
be specifying the ordering to be used when supplying data?

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

Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

[Viresh Kumar]

On 22-11-16, 18:41, Mark Brown wrote:
> On Tue, Nov 22, 2016 at 09:19:22AM +0530, Viresh Kumar wrote:
> > "How do we know (from the DT) the order in which entries for multiple regulators
> > are present in the OPP table?"
> > 
> > And I am not sure if we can do that without having a property like:
> > 
> > +               supply-names = "vcc0", "vcc1", "vcc2";
> > 
> > in the OPP table or the consumer device. And surely it isn't a clean enough
> > solution and that's why this series relied on the code to get such details.
> > 
> > Does someone have an alternative? If NO, can we go ahead with this series as is?
> 
> I'm really not at all clear why this has to be in DT.  My understanding
> was that this is basically a helper library for more specific bindings
> which already have to hard code things like sequencing so surely they'd
> be specifying the ordering to be used when supplying data?

I am a bit confused and perhaps I am misreading your feedback.

Are you saying that:

"we don't need to identify which microVolts value in the OPP table corresponds
to which supply from the DT itself and we can do that with some hard coded
stuff" ?

If yes, then below is from an earlier email from you, which I feel is opposite
of what you are suggesting now.

On 09-11-16, 14:58, Mark Brown wrote:
> On Wed, Oct 26, 2016 at 12:02:56PM +0530, Viresh Kumar wrote:

> > 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.

-- 
viresh

Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

[Mark Brown]

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

On Wed, Nov 23, 2016 at 09:16:57AM +0530, Viresh Kumar wrote:
> On 22-11-16, 18:41, Mark Brown wrote:

> > I'm really not at all clear why this has to be in DT.  My understanding
> > was that this is basically a helper library for more specific bindings
> > which already have to hard code things like sequencing so surely they'd
> > be specifying the ordering to be used when supplying data?

> I am a bit confused and perhaps I am misreading your feedback.

> Are you saying that:

> "we don't need to identify which microVolts value in the OPP table corresponds
> to which supply from the DT itself and we can do that with some hard coded
> stuff" ?

No, of course not.  That would be completely incoherent, there would be
no way for anything to use the data if the values can just be in any
random order.

> If yes, then below is from an earlier email from you, which I feel is opposite
> of what you are suggesting now.

> > 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.

The "platform driver bindings" bit of this is very important here.  This
is a generic binding that is going to be used by platform specific
drivers (as I understand it).  I would therefore expect that these
things can be described in the platform specific bindings.

Please, take a step back and think about what what the binding means and
how it's going to be used.  Not only is this a DT binding and therefore
an ABI it's also a generic binding that's going to affect a lot of
systems probably for a long time.  This means it is really important to
think things through and make sure we understand what they're doing.
When working on kernel internal code it's relatively easy to fix things
if we realize later that they don't work well so it's easier to just
work quickly but when we're making ABIs that's not possible.

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

Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

[Viresh Kumar]

Hi Mark,

On 23-11-16, 12:29, Mark Brown wrote:
> On Wed, Nov 23, 2016 at 09:16:57AM +0530, Viresh Kumar wrote:
> > Are you saying that:
> 
> > "we don't need to identify which microVolts value in the OPP table corresponds
> > to which supply from the DT itself and we can do that with some hard coded
> > stuff" ?
> 
> No, of course not.  That would be completely incoherent, there would be
> no way for anything to use the data if the values can just be in any
> random order.

With the current implementation in this patchset, the order in which entries are
present in the OPP node is _assumed_ to be known to the platform specific code,
which will pass it on to the OPP core with some callbacks. So the order will not
be completely random.

> > If yes, then below is from an earlier email from you, which I feel is opposite
> > of what you are suggesting now.
> 
> > > 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.
> 
> The "platform driver bindings" bit of this is very important here.  This
> is a generic binding that is going to be used by platform specific
> drivers (as I understand it).

There is no platform specific binding here.

For example in case of a single regulator for the device (CPU), the platform
specific DT file contains the CPU nodes (using generic bindings) and an OPP
table node (again generic bindings only). The OPP core reads both these nodes
for the device and constructs the OPP table.

Now in case of multiple regulators for the device, as you already know, the only
unanswered detail (apart from the order in which the regulators need to be
programmed) is to link which entries in the OPP table are for which regulator.

We can either get this information from DT (somehow) or hardcode it in platform
specific code. This patch provided infrastructure for the later one.

If we indeed want to get this information from the DT then there are two
options:

- Create platform specific binding:

  foo-platform,supply-names = "vcc0", "vcc1", "vcc2";

- Create common binding that can be used by all platforms:

  supply-names = "vcc0", "vcc1", "vcc2";


Such bindings will end up either in the consumer device node (like CPU0 node) or
in the OPP table itself. I am personally inclined towards the common
supply-names bindings, otherwise every user platform will end up creating very
similar bindings.

> I would therefore expect that these
> things can be described in the platform specific bindings.
> 
> Please, take a step back and think about what what the binding means and
> how it's going to be used.  Not only is this a DT binding and therefore
> an ABI it's also a generic binding that's going to affect a lot of
> systems probably for a long time.  This means it is really important to
> think things through and make sure we understand what they're doing.
> When working on kernel internal code it's relatively easy to fix things
> if we realize later that they don't work well so it's easier to just
> work quickly but when we're making ABIs that's not possible.

I agree and I completely understand your concerns here and it is surely very
important to get the bindings right as they will last for very long.

But I am still unsure about what's the best way of doing this. The new bindings
are rejected by almost everyone as they contain some of the information already
contained in the consumer node while the regulators are defined.

-- 
viresh

Re: [PATCH V3 0/9] PM / OPP: Multiple regulator support

[Mark Brown]

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

On Thu, Nov 24, 2016 at 10:37:24AM +0530, Viresh Kumar wrote:
> On 23-11-16, 12:29, Mark Brown wrote:

> > No, of course not.  That would be completely incoherent, there would be
> > no way for anything to use the data if the values can just be in any
> > random order.

> With the current implementation in this patchset, the order in which entries are
> present in the OPP node is _assumed_ to be known to the platform specific code,
> which will pass it on to the OPP core with some callbacks. So the order will not
> be completely random.

What we're reviewing here is the DT binding and the DT binding
explicitly said the order doesn't matter.  The DT binding is OS neutral
so it needs to make sense without the code.

> > The "platform driver bindings" bit of this is very important here.  This
> > is a generic binding that is going to be used by platform specific
> > drivers (as I understand it).

> There is no platform specific binding here.

It seems like we're going to need one for this to be a comprehensible
binding.

> We can either get this information from DT (somehow) or hardcode it in platform
> specific code. This patch provided infrastructure for the later one.

> If we indeed want to get this information from the DT then there are two
> options:

Why would we want to get it from DT when we can't get half the other
information we need to make the data useful from DT?

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