linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/8] PM / OPP: Multiple regulator support
@ 2016-10-04 11:56 Viresh Kumar
  2016-10-04 11:56 ` [PATCH 1/8] PM / OPP: Reword binding supporting multiple regulators per device Viresh Kumar
                   ` (8 more replies)
  0 siblings, 9 replies; 17+ messages in thread
From: Viresh Kumar @ 2016-10-04 11:56 UTC (permalink / raw)
  To: Rafael Wysocki, nm, sboyd
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
	d-gerlach, broonie, 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].

Some thoughts went into it few months back but then it all got lost. I
am trying to get that back on track with this thread.

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

Though more testing on real (TI) platforms would be useful.

This is rebased over: pm-4.9-rc1 tag in the PM tree.

--
viresh

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

Viresh Kumar (8):
  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 opp_set_rate() callbacks
  PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators()

 Documentation/devicetree/bindings/opp/opp.txt |  25 +-
 drivers/base/power/opp/core.c                 | 501 ++++++++++++++++++++------
 drivers/base/power/opp/debugfs.c              |  48 ++-
 drivers/base/power/opp/of.c                   | 104 ++++--
 drivers/base/power/opp/opp.h                  |  36 +-
 drivers/cpufreq/cpufreq-dt.c                  |   9 +-
 include/linux/pm_opp.h                        |  33 +-
 7 files changed, 574 insertions(+), 182 deletions(-)

-- 
2.7.1.410.g6faf27b

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [PATCH 1/8] PM / OPP: Reword binding supporting multiple regulators per device
  2016-10-04 11:56 [PATCH 0/8] PM / OPP: Multiple regulator support Viresh Kumar
@ 2016-10-04 11:56 ` Viresh Kumar
  2016-10-09  1:29   ` Rob Herring
  2016-10-04 11:56 ` [PATCH 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section Viresh Kumar
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Viresh Kumar @ 2016-10-04 11:56 UTC (permalink / raw)
  To: Rafael Wysocki, nm, sboyd, Viresh Kumar
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
	d-gerlach, broonie, Viresh Kumar, devicetree

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section
  2016-10-04 11:56 [PATCH 0/8] PM / OPP: Multiple regulator support Viresh Kumar
  2016-10-04 11:56 ` [PATCH 1/8] PM / OPP: Reword binding supporting multiple regulators per device Viresh Kumar
@ 2016-10-04 11:56 ` Viresh Kumar
  2016-10-04 11:56 ` [PATCH 3/8] PM / OPP: Manage supply's voltage/current in a separate structure Viresh Kumar
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2016-10-04 11:56 UTC (permalink / raw)
  To: Rafael Wysocki, nm, sboyd, Viresh Kumar
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
	d-gerlach, broonie, 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>
---
 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

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 3/8] PM / OPP: Manage supply's voltage/current in a separate structure
  2016-10-04 11:56 [PATCH 0/8] PM / OPP: Multiple regulator support Viresh Kumar
  2016-10-04 11:56 ` [PATCH 1/8] PM / OPP: Reword binding supporting multiple regulators per device Viresh Kumar
  2016-10-04 11:56 ` [PATCH 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section Viresh Kumar
@ 2016-10-04 11:56 ` Viresh Kumar
  2016-10-12 21:15   ` Dave Gerlach
  2016-10-04 11:56 ` [PATCH 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage() Viresh Kumar
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Viresh Kumar @ 2016-10-04 11:56 UTC (permalink / raw)
  To: Rafael Wysocki, nm, sboyd, Viresh Kumar
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
	d-gerlach, broonie, 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>
---
 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     | 27 ++++++++++++++++--------
 4 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..1bda0d35c486 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -47,6 +47,22 @@ extern struct list_head opp_tables;
  */
 
 /**
+ * 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;
+};
+
+/**
  * struct dev_pm_opp - Generic OPP description structure
  * @node:	opp table node. The nodes are maintained throughout the lifetime
  *		of boot. It is expected only an optimal set of OPPs are
@@ -61,10 +77,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 +96,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;
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()
  2016-10-04 11:56 [PATCH 0/8] PM / OPP: Multiple regulator support Viresh Kumar
                   ` (2 preceding siblings ...)
  2016-10-04 11:56 ` [PATCH 3/8] PM / OPP: Manage supply's voltage/current in a separate structure Viresh Kumar
@ 2016-10-04 11:56 ` Viresh Kumar
  2016-10-04 11:56 ` [PATCH 5/8] PM / OPP: Add infrastructure to manage multiple regulators Viresh Kumar
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2016-10-04 11:56 UTC (permalink / raw)
  To: Rafael Wysocki, nm, sboyd, Viresh Kumar
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
	d-gerlach, broonie, Viresh Kumar

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

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 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

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 5/8] PM / OPP: Add infrastructure to manage multiple regulators
  2016-10-04 11:56 [PATCH 0/8] PM / OPP: Multiple regulator support Viresh Kumar
                   ` (3 preceding siblings ...)
  2016-10-04 11:56 ` [PATCH 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage() Viresh Kumar
@ 2016-10-04 11:56 ` Viresh Kumar
  2016-10-04 11:56 ` [PATCH 6/8] PM / OPP: Separate out _generic_opp_set_rate() Viresh Kumar
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2016-10-04 11:56 UTC (permalink / raw)
  To: Rafael Wysocki, nm, sboyd, Viresh Kumar
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
	d-gerlach, broonie, 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>
---
 drivers/base/power/opp/core.c    | 220 ++++++++++++++++++++++++++-------------
 drivers/base/power/opp/debugfs.c |  48 +++++++--
 drivers/base/power/opp/of.c      | 102 +++++++++++++-----
 drivers/base/power/opp/opp.h     |  10 +-
 drivers/cpufreq/cpufreq-dt.c     |   9 +-
 include/linux/pm_opp.h           |   8 +-
 6 files changed, 276 insertions(+), 121 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 37fad2eb0f47..45c70ce07864 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,10 @@ 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;
+	unsigned long *min_uV, *max_uV;
+	int ret, size, i, count;
 
 	rcu_read_lock();
 
@@ -235,21 +237,44 @@ 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;
+	}
+
+	min_uV = kmalloc(count * (sizeof(*min_uV) + sizeof(*max_uV)),
+			 GFP_KERNEL);
+	if (!min_uV) {
+		kfree(regulators);
+		rcu_read_unlock();
+		return 0;
+	}
+
+	max_uV = min_uV + count;
+
+	for (i = 0; i < count; i++) {
+		min_uV[i] = ~0;
+		max_uV[i] = 0;
+
+		list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
+			if (!opp->available)
+				continue;
 
-		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;
+			if (opp->supplies[i].u_volt_min < min_uV[i])
+				min_uV[i] = opp->supplies[i].u_volt_min;
+			if (opp->supplies[i].u_volt_max > max_uV[i])
+				max_uV[i] = 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, min_uV[i], max_uV[i]);
+		if (ret > 0)
+			latency_ns += ret * 1000;
+	}
+
+	kfree(min_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,49 @@ 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;
 	}
 
+	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 +1034,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 +1107,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 +1354,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 +1371,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 *names[],
+			      unsigned int count)
 {
 	struct opp_table *opp_table;
 	struct regulator *reg;
-	int ret;
+	int ret, i;
 
 	mutex_lock(&opp_table_lock);
 
@@ -1338,26 +1392,43 @@ 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]);
+		pr_info("%s: %d: %p: %s\n", __func__, __LINE__, reg, 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..cb5e5fde3d39 100644
--- a/drivers/base/power/opp/debugfs.c
+++ b/drivers/base/power/opp/debugfs.c
@@ -34,6 +34,43 @@ 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[] = "supply-X"; /* support only 0-9 supplies */
+
+	/* Always create at least supply-0 directory */
+	do {
+		name[7] = i + '0';
+
+		/* Create per-opp directory */
+		d = debugfs_create_dir(name, pdentry);
+		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 +100,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..c857fb07a5bc 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"
@@ -105,12 +106,13 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
 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 +130,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 +169,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 +350,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 1bda0d35c486..d3f0861f9bff 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -77,7 +77,7 @@ struct dev_pm_opp_supply {
  * @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
@@ -96,7 +96,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;
 
@@ -155,7 +155,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.
  *
@@ -190,7 +191,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 bca26157f5b6..0606b70a8b97 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -62,8 +62,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 *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);
@@ -170,12 +170,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

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 6/8] PM / OPP: Separate out _generic_opp_set_rate()
  2016-10-04 11:56 [PATCH 0/8] PM / OPP: Multiple regulator support Viresh Kumar
                   ` (4 preceding siblings ...)
  2016-10-04 11:56 ` [PATCH 5/8] PM / OPP: Add infrastructure to manage multiple regulators Viresh Kumar
@ 2016-10-04 11:56 ` Viresh Kumar
  2016-10-04 11:56 ` [PATCH 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks Viresh Kumar
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2016-10-04 11:56 UTC (permalink / raw)
  To: Rafael Wysocki, nm, sboyd, Viresh Kumar
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
	d-gerlach, broonie, 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>
---
 drivers/base/power/opp/core.c | 181 +++++++++++++++++++++++++++++-------------
 drivers/base/power/opp/opp.h  |   2 +
 include/linux/pm_opp.h        |  15 ++++
 3 files changed, 145 insertions(+), 53 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 45c70ce07864..96f04392daef 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -596,6 +596,73 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 	return ret;
 }
 
+static inline int
+_generic_opp_set_rate_clk_only(struct device *dev, struct clk *clk,
+			       unsigned long old_freq, unsigned long freq)
+{
+	int 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);
+	}
+
+	return ret;
+}
+
+static int _generic_opp_set_rate(struct device *dev,
+				 struct dev_pm_set_rate_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_opp_set_rate_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_opp_set_rate_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 +676,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_rate_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 +730,32 @@ 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;
-		}
+	regulators = opp_table->regulators;
 
-		reg = opp_table->regulators[0];
+	/* Only frequency scaling */
+	if (!regulators) {
+		rcu_read_unlock();
+		return _generic_opp_set_rate_clk_only(dev, clk, old_freq, freq);
 	}
 
+	data = opp_table->set_rate_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_opp_set_rate(dev, data);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
 
@@ -1353,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_rate_data(struct opp_table *opp_table)
+{
+	struct dev_pm_set_rate_data *data;
+	int len, count = opp_table->regulator_count;
+
+	if (WARN_ON(!count))
+		return -EINVAL;
+
+	/* space for set_rate_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_rate_data = data;
+
+	return 0;
+}
+
+static void _free_set_rate_data(struct opp_table *opp_table)
+{
+	kfree(opp_table->set_rate_data);
+	opp_table->set_rate_data = NULL;
+}
+
 /**
  * dev_pm_opp_set_regulators() - Set regulator names for the device
  * @dev: Device for which regulator name is being set.
@@ -1420,6 +1487,11 @@ int dev_pm_opp_set_regulators(struct device *dev, const char *names[],
 
 	opp_table->regulator_count = count;
 
+	/* Allocate block only once to pass to ->set_rate() */
+	ret = _allocate_set_rate_data(opp_table);
+	if (ret)
+		goto free_regulators;
+
 	mutex_unlock(&opp_table_lock);
 	return 0;
 
@@ -1429,6 +1501,7 @@ int dev_pm_opp_set_regulators(struct device *dev, const char *names[],
 
 	kfree(opp_table->regulators);
 	opp_table->regulators = NULL;
+	opp_table->regulator_count = 0;
 err:
 	_remove_opp_table(opp_table);
 unlock:
@@ -1474,6 +1547,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_rate_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 d3f0861f9bff..28011d6da345 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -194,6 +194,8 @@ struct opp_table {
 	struct regulator **regulators;
 	unsigned int regulator_count;
 
+	struct dev_pm_set_rate_data *set_rate_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 0606b70a8b97..630f0fd6065c 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -17,6 +17,7 @@
 #include <linux/err.h>
 #include <linux/notifier.h>
 
+struct clk;
 struct dev_pm_opp;
 struct device;
 
@@ -24,6 +25,20 @@ enum dev_pm_opp_event {
 	OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
 };
 
+struct dev_pm_opp_info {
+	unsigned long rate;
+	struct dev_pm_opp_supply *supplies;
+};
+
+struct dev_pm_set_rate_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

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks
  2016-10-04 11:56 [PATCH 0/8] PM / OPP: Multiple regulator support Viresh Kumar
                   ` (5 preceding siblings ...)
  2016-10-04 11:56 ` [PATCH 6/8] PM / OPP: Separate out _generic_opp_set_rate() Viresh Kumar
@ 2016-10-04 11:56 ` Viresh Kumar
  2016-10-05  1:29   ` Viresh Kumar
  2016-10-12  2:10   ` [PATCH V2 " Viresh Kumar
  2016-10-04 11:56 ` [PATCH 8/8] PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators() Viresh Kumar
  2016-10-10 19:20 ` [PATCH 0/8] PM / OPP: Multiple regulator support Dave Gerlach
  8 siblings, 2 replies; 17+ messages in thread
From: Viresh Kumar @ 2016-10-04 11:56 UTC (permalink / raw)
  To: Rafael Wysocki, nm, sboyd, Viresh Kumar
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
	d-gerlach, broonie, Viresh Kumar

The generic opp_set_rate() 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 opp_set_rate()
callback.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 114 +++++++++++++++++++++++++++++++++++++++++-
 drivers/base/power/opp/opp.h  |   1 +
 include/linux/pm_opp.h        |  10 ++++
 3 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 96f04392daef..f7c4ef194aac 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -677,6 +677,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_rate)(struct device *dev, struct dev_pm_set_rate_data *data);
 	struct dev_pm_opp *old_opp, *opp;
 	struct regulator **regulators;
 	struct dev_pm_set_rate_data *data;
@@ -738,6 +739,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		return _generic_opp_set_rate_clk_only(dev, clk, old_freq, freq);
 	}
 
+	if (opp_table->set_rate)
+		set_rate = opp_table->set_rate;
+	else
+		set_rate = _generic_opp_set_rate;
+
 	data = opp_table->set_rate_data;
 	data->regulators = regulators;
 	data->regulator_count = opp_table->regulator_count;
@@ -755,7 +761,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	rcu_read_unlock();
 
-	return _generic_opp_set_rate(dev, data);
+	return set_rate(dev, data);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
 
@@ -889,6 +895,9 @@ static void _remove_opp_table(struct opp_table *opp_table)
 	if (opp_table->regulators)
 		return;
 
+	if (opp_table->set_rate)
+		return;
+
 	/* Release clk */
 	if (!IS_ERR(opp_table->clk))
 		clk_put(opp_table->clk);
@@ -1562,6 +1571,109 @@ void dev_pm_opp_put_regulators(struct device *dev)
 EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
 
 /**
+ * dev_pm_opp_register_set_rate_helper() - Register custom OPP set rate helper
+ * @dev: Device for which the helper is getting registered.
+ * @set_rate: Custom OPP set rate 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_rate_helper(struct device *dev,
+	int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data))
+{
+	struct opp_table *opp_table;
+	int ret;
+
+	if (!set_rate)
+		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_rate helper */
+	if (WARN_ON(opp_table->set_rate)) {
+		ret = -EBUSY;
+		goto err;
+	}
+
+	opp_table->set_rate = set_rate;
+
+	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_rate_helper);
+
+/**
+ * dev_pm_opp_register_put_rate_helper() - Releases resources blocked for
+ *					   set_rate helper
+ * @dev: Device for which custom set_rate 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_rate_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_rate) {
+		dev_err(dev, "%s: Doesn't have custom set_rate 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_rate = 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_rate_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 28011d6da345..38d97a59d762 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -194,6 +194,7 @@ struct opp_table {
 	struct regulator **regulators;
 	unsigned int regulator_count;
 
+	int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data);
 	struct dev_pm_set_rate_data *set_rate_data;
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 630f0fd6065c..ef882ed24b0e 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -79,6 +79,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 *names[], unsigned int count);
 void dev_pm_opp_put_regulators(struct device *dev);
+int dev_pm_opp_register_set_rate_helper(struct device *dev, int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data));
+void dev_pm_opp_register_put_rate_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);
@@ -178,6 +180,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) {}
 
+int dev_pm_opp_register_set_rate_helper(struct device *dev,
+	int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data))
+{
+	return -ENOTSUPP;
+}
+
+static inline void dev_pm_opp_register_put_rate_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

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 8/8] PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators()
  2016-10-04 11:56 [PATCH 0/8] PM / OPP: Multiple regulator support Viresh Kumar
                   ` (6 preceding siblings ...)
  2016-10-04 11:56 ` [PATCH 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks Viresh Kumar
@ 2016-10-04 11:56 ` Viresh Kumar
  2016-10-10 19:20 ` [PATCH 0/8] PM / OPP: Multiple regulator support Dave Gerlach
  8 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2016-10-04 11:56 UTC (permalink / raw)
  To: Rafael Wysocki, nm, sboyd, Viresh Kumar
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
	d-gerlach, broonie, 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>
---
 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 f7c4ef194aac..51dd71d16269 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 *names[],
 	}
 
 	/* Already have regulators set */
-	if (WARN_ON(opp_table->regulators)) {
+	if (opp_table->regulators) {
 		ret = -EBUSY;
 		goto err;
 	}
-- 
2.7.1.410.g6faf27b

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* Re: [PATCH 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks
  2016-10-04 11:56 ` [PATCH 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks Viresh Kumar
@ 2016-10-05  1:29   ` Viresh Kumar
  2016-10-11 22:26     ` Rafael J. Wysocki
  2016-10-12  2:10   ` [PATCH V2 " Viresh Kumar
  1 sibling, 1 reply; 17+ messages in thread
From: Viresh Kumar @ 2016-10-05  1:29 UTC (permalink / raw)
  To: Rafael Wysocki, nm, sboyd, Viresh Kumar
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
	d-gerlach, broonie

On 04-10-16, 17:26, Viresh Kumar wrote:
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> +int dev_pm_opp_register_set_rate_helper(struct device *dev,
> +	int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data))
> +{
> +	return -ENOTSUPP;
> +}

+ this to fix up a warning:

diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index ef882ed24b0e..483f33e70895 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -180,7 +180,7 @@ static inline int dev_pm_opp_set_supported_hw(struct device *dev,
 
 static inline void dev_pm_opp_put_supported_hw(struct device *dev) {}
 
-int dev_pm_opp_register_set_rate_helper(struct device *dev,
+static inline int dev_pm_opp_register_set_rate_helper(struct device *dev,
        int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data))
 {
        return -ENOTSUPP;

-- 
viresh

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* Re: [PATCH 1/8] PM / OPP: Reword binding supporting multiple regulators per device
  2016-10-04 11:56 ` [PATCH 1/8] PM / OPP: Reword binding supporting multiple regulators per device Viresh Kumar
@ 2016-10-09  1:29   ` Rob Herring
  0 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2016-10-09  1:29 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, nm, sboyd, Viresh Kumar, linaro-kernel, linux-pm,
	linux-kernel, Vincent Guittot, d-gerlach, broonie, devicetree

On Tue, Oct 04, 2016 at 05:26:43PM +0530, Viresh Kumar wrote:
> 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>
> ---
>  Documentation/devicetree/bindings/opp/opp.txt | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/8] PM / OPP: Multiple regulator support
  2016-10-04 11:56 [PATCH 0/8] PM / OPP: Multiple regulator support Viresh Kumar
                   ` (7 preceding siblings ...)
  2016-10-04 11:56 ` [PATCH 8/8] PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators() Viresh Kumar
@ 2016-10-10 19:20 ` Dave Gerlach
  2016-10-12  2:11   ` Viresh Kumar
  8 siblings, 1 reply; 17+ messages in thread
From: Dave Gerlach @ 2016-10-10 19:20 UTC (permalink / raw)
  To: Viresh Kumar, Rafael Wysocki, nm, sboyd
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh, broonie

Hi,
On 10/04/2016 06:56 AM, 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].
>
> Some thoughts went into it few months back but then it all got lost. I
> am trying to get that back on track with this thread.
>
> 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).
>
> Though more testing on real (TI) platforms would be useful.

Cool, I'm reviewing these patches still but I definitely plan to test 
them out on the TI platforms requiring multi-regulator support. At first 
glance it looks like it does give us the hooks we need. I believe we can 
use cpufreq-dt as is and just provide the regulators before it probes.

Regards,
Dave

>
> This is rebased over: pm-4.9-rc1 tag in the PM tree.
>
> --
> viresh
>
> [1] https://marc.info/?l=linux-pm&m=145684495832764&w=2
>
> Viresh Kumar (8):
>    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 opp_set_rate() callbacks
>    PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators()
>
>   Documentation/devicetree/bindings/opp/opp.txt |  25 +-
>   drivers/base/power/opp/core.c                 | 501 ++++++++++++++++++++------
>   drivers/base/power/opp/debugfs.c              |  48 ++-
>   drivers/base/power/opp/of.c                   | 104 ++++--
>   drivers/base/power/opp/opp.h                  |  36 +-
>   drivers/cpufreq/cpufreq-dt.c                  |   9 +-
>   include/linux/pm_opp.h                        |  33 +-
>   7 files changed, 574 insertions(+), 182 deletions(-)
>

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks
  2016-10-05  1:29   ` Viresh Kumar
@ 2016-10-11 22:26     ` Rafael J. Wysocki
  0 siblings, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2016-10-11 22:26 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: nm, sboyd, Viresh Kumar, linaro-kernel, linux-pm, linux-kernel,
	Vincent Guittot, robh, d-gerlach, broonie

On Wednesday, October 05, 2016 06:59:29 AM Viresh Kumar wrote:
> On 04-10-16, 17:26, Viresh Kumar wrote:
> > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> > +int dev_pm_opp_register_set_rate_helper(struct device *dev,
> > +	int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data))
> > +{
> > +	return -ENOTSUPP;
> > +}
> 
> + this to fix up a warning:
> 
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index ef882ed24b0e..483f33e70895 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -180,7 +180,7 @@ static inline int dev_pm_opp_set_supported_hw(struct device *dev,
>  
>  static inline void dev_pm_opp_put_supported_hw(struct device *dev) {}
>  
> -int dev_pm_opp_register_set_rate_helper(struct device *dev,
> +static inline int dev_pm_opp_register_set_rate_helper(struct device *dev,
>         int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data))
>  {
>         return -ENOTSUPP;
> 
> 

Can you resend the full patch, please?

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [PATCH V2 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks
  2016-10-04 11:56 ` [PATCH 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks Viresh Kumar
  2016-10-05  1:29   ` Viresh Kumar
@ 2016-10-12  2:10   ` Viresh Kumar
  1 sibling, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2016-10-12  2:10 UTC (permalink / raw)
  To: Rafael Wysocki, nm, sboyd, Viresh Kumar
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh,
	d-gerlach, broonie, Viresh Kumar

The generic opp_set_rate() 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 opp_set_rate()
callback.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V1->V2:
- Define the dummy helper in .h as static inline to fix a kernel
  warning.

 drivers/base/power/opp/core.c | 114 +++++++++++++++++++++++++++++++++++++++++-
 drivers/base/power/opp/opp.h  |   1 +
 include/linux/pm_opp.h        |  10 ++++
 3 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 96f04392daef..f7c4ef194aac 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -677,6 +677,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_rate)(struct device *dev, struct dev_pm_set_rate_data *data);
 	struct dev_pm_opp *old_opp, *opp;
 	struct regulator **regulators;
 	struct dev_pm_set_rate_data *data;
@@ -738,6 +739,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		return _generic_opp_set_rate_clk_only(dev, clk, old_freq, freq);
 	}
 
+	if (opp_table->set_rate)
+		set_rate = opp_table->set_rate;
+	else
+		set_rate = _generic_opp_set_rate;
+
 	data = opp_table->set_rate_data;
 	data->regulators = regulators;
 	data->regulator_count = opp_table->regulator_count;
@@ -755,7 +761,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	rcu_read_unlock();
 
-	return _generic_opp_set_rate(dev, data);
+	return set_rate(dev, data);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
 
@@ -889,6 +895,9 @@ static void _remove_opp_table(struct opp_table *opp_table)
 	if (opp_table->regulators)
 		return;
 
+	if (opp_table->set_rate)
+		return;
+
 	/* Release clk */
 	if (!IS_ERR(opp_table->clk))
 		clk_put(opp_table->clk);
@@ -1562,6 +1571,109 @@ void dev_pm_opp_put_regulators(struct device *dev)
 EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
 
 /**
+ * dev_pm_opp_register_set_rate_helper() - Register custom OPP set rate helper
+ * @dev: Device for which the helper is getting registered.
+ * @set_rate: Custom OPP set rate 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_rate_helper(struct device *dev,
+	int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data))
+{
+	struct opp_table *opp_table;
+	int ret;
+
+	if (!set_rate)
+		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_rate helper */
+	if (WARN_ON(opp_table->set_rate)) {
+		ret = -EBUSY;
+		goto err;
+	}
+
+	opp_table->set_rate = set_rate;
+
+	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_rate_helper);
+
+/**
+ * dev_pm_opp_register_put_rate_helper() - Releases resources blocked for
+ *					   set_rate helper
+ * @dev: Device for which custom set_rate 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_rate_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_rate) {
+		dev_err(dev, "%s: Doesn't have custom set_rate 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_rate = 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_rate_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 28011d6da345..38d97a59d762 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -194,6 +194,7 @@ struct opp_table {
 	struct regulator **regulators;
 	unsigned int regulator_count;
 
+	int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data);
 	struct dev_pm_set_rate_data *set_rate_data;
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 630f0fd6065c..483f33e70895 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -79,6 +79,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 *names[], unsigned int count);
 void dev_pm_opp_put_regulators(struct device *dev);
+int dev_pm_opp_register_set_rate_helper(struct device *dev, int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data));
+void dev_pm_opp_register_put_rate_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);
@@ -178,6 +180,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_rate_helper(struct device *dev,
+	int (*set_rate)(struct device *dev, struct dev_pm_set_rate_data *data))
+{
+	return -ENOTSUPP;
+}
+
+static inline void dev_pm_opp_register_put_rate_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

^ permalink raw reply related	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/8] PM / OPP: Multiple regulator support
  2016-10-10 19:20 ` [PATCH 0/8] PM / OPP: Multiple regulator support Dave Gerlach
@ 2016-10-12  2:11   ` Viresh Kumar
  0 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2016-10-12  2:11 UTC (permalink / raw)
  To: Dave Gerlach
  Cc: Rafael Wysocki, nm, sboyd, linaro-kernel, linux-pm, linux-kernel,
	Vincent Guittot, robh, broonie

On 10-10-16, 14:20, Dave Gerlach wrote:
> Cool, I'm reviewing these patches still but I definitely plan to test them
> out on the TI platforms requiring multi-regulator support. At first glance
> it looks like it does give us the hooks we need. I believe we can use
> cpufreq-dt as is and just provide the regulators before it probes.

Yes you can. That's how I tested it as well.

-- 
viresh

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 3/8] PM / OPP: Manage supply's voltage/current in a separate structure
  2016-10-04 11:56 ` [PATCH 3/8] PM / OPP: Manage supply's voltage/current in a separate structure Viresh Kumar
@ 2016-10-12 21:15   ` Dave Gerlach
  2016-10-13  5:42     ` Viresh Kumar
  0 siblings, 1 reply; 17+ messages in thread
From: Dave Gerlach @ 2016-10-12 21:15 UTC (permalink / raw)
  To: Viresh Kumar, Rafael Wysocki, nm, sboyd, Viresh Kumar
  Cc: linaro-kernel, linux-pm, linux-kernel, Vincent Guittot, robh, broonie

Hi,
On 10/04/2016 06:56 AM, Viresh Kumar wrote:
> 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>
> ---
>   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     | 27 ++++++++++++++++--------
>   4 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..1bda0d35c486 100644
> --- a/drivers/base/power/opp/opp.h
> +++ b/drivers/base/power/opp/opp.h
> @@ -47,6 +47,22 @@ extern struct list_head opp_tables;
>    */
>
>   /**
> + * 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 thisq 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;
> +};
> +
> +/**

This needs to move to include/linux/pm_opp.h, does it not? We need 
access to the actual voltage values from outside of the OPP core if we 
are going to be setting regulators from the platform provided 
opp_set_rate callback described in patch 7.

Regards,
Dave

>    * struct dev_pm_opp - Generic OPP description structure
>    * @node:	opp table node. The nodes are maintained throughout the lifetime
>    *		of boot. It is expected only an optimal set of OPPs are
> @@ -61,10 +77,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 +96,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;
>

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 3/8] PM / OPP: Manage supply's voltage/current in a separate structure
  2016-10-12 21:15   ` Dave Gerlach
@ 2016-10-13  5:42     ` Viresh Kumar
  0 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2016-10-13  5:42 UTC (permalink / raw)
  To: Dave Gerlach
  Cc: Rafael Wysocki, nm, sboyd, Viresh Kumar, linaro-kernel, linux-pm,
	linux-kernel, Vincent Guittot, robh, broonie

On 12-10-16, 16:15, Dave Gerlach wrote:
> >  /**
> >+ * 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 thisq 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;
> >+};
> >+
> >+/**
> 
> This needs to move to include/linux/pm_opp.h, does it not? We need access to
> the actual voltage values from outside of the OPP core if we are going to be
> setting regulators from the platform provided opp_set_rate callback
> described in patch 7.

Yes, will do in V2.

-- 
viresh

^ permalink raw reply	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-10-13  5:43 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-04 11:56 [PATCH 0/8] PM / OPP: Multiple regulator support Viresh Kumar
2016-10-04 11:56 ` [PATCH 1/8] PM / OPP: Reword binding supporting multiple regulators per device Viresh Kumar
2016-10-09  1:29   ` Rob Herring
2016-10-04 11:56 ` [PATCH 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section Viresh Kumar
2016-10-04 11:56 ` [PATCH 3/8] PM / OPP: Manage supply's voltage/current in a separate structure Viresh Kumar
2016-10-12 21:15   ` Dave Gerlach
2016-10-13  5:42     ` Viresh Kumar
2016-10-04 11:56 ` [PATCH 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage() Viresh Kumar
2016-10-04 11:56 ` [PATCH 5/8] PM / OPP: Add infrastructure to manage multiple regulators Viresh Kumar
2016-10-04 11:56 ` [PATCH 6/8] PM / OPP: Separate out _generic_opp_set_rate() Viresh Kumar
2016-10-04 11:56 ` [PATCH 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks Viresh Kumar
2016-10-05  1:29   ` Viresh Kumar
2016-10-11 22:26     ` Rafael J. Wysocki
2016-10-12  2:10   ` [PATCH V2 " Viresh Kumar
2016-10-04 11:56 ` [PATCH 8/8] PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators() Viresh Kumar
2016-10-10 19:20 ` [PATCH 0/8] PM / OPP: Multiple regulator support Dave Gerlach
2016-10-12  2:11   ` Viresh Kumar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).