* [PATCH V2 0/8] PM / OPP: Multiple regulator support
@ 2016-10-20 8:44 Viresh Kumar
2016-10-20 8:44 ` [PATCH V2 1/8] PM / OPP: Reword binding supporting multiple regulators per device Viresh Kumar
` (8 more replies)
0 siblings, 9 replies; 32+ messages in thread
From: Viresh Kumar @ 2016-10-20 8:44 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: linux-next branch in the PM tree.
V1->V2:
- Ack from Rob for 1st patch
- Moved the supplies structure to pm_opp.h (Dave)
- Fixed an compilation warning.
--
viresh
[1] https://marc.info/?l=linux-pm&m=145684495832764&w=2
Viresh Kumar (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 | 20 +-
drivers/cpufreq/cpufreq-dt.c | 9 +-
include/linux/pm_opp.h | 49 ++-
7 files changed, 574 insertions(+), 182 deletions(-)
--
2.7.1.410.g6faf27b
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH V2 1/8] PM / OPP: Reword binding supporting multiple regulators per device
2016-10-20 8:44 [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar
@ 2016-10-20 8:44 ` Viresh Kumar
2016-10-24 22:48 ` Stephen Boyd
2016-10-20 8:44 ` [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section Viresh Kumar
` (7 subsequent siblings)
8 siblings, 1 reply; 32+ messages in thread
From: Viresh Kumar @ 2016-10-20 8:44 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>
Acked-by: Rob Herring <robh@kernel.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] 32+ messages in thread
* [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section
2016-10-20 8:44 [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar
2016-10-20 8:44 ` [PATCH V2 1/8] PM / OPP: Reword binding supporting multiple regulators per device Viresh Kumar
@ 2016-10-20 8:44 ` Viresh Kumar
2016-10-24 22:52 ` Stephen Boyd
2016-10-20 8:44 ` [PATCH V2 3/8] PM / OPP: Manage supply's voltage/current in a separate structure Viresh Kumar
` (6 subsequent siblings)
8 siblings, 1 reply; 32+ messages in thread
From: Viresh Kumar @ 2016-10-20 8:44 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] 32+ messages in thread
* [PATCH V2 3/8] PM / OPP: Manage supply's voltage/current in a separate structure
2016-10-20 8:44 [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar
2016-10-20 8:44 ` [PATCH V2 1/8] PM / OPP: Reword binding supporting multiple regulators per device Viresh Kumar
2016-10-20 8:44 ` [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section Viresh Kumar
@ 2016-10-20 8:44 ` Viresh Kumar
2016-10-20 8:44 ` [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage() Viresh Kumar
` (5 subsequent siblings)
8 siblings, 0 replies; 32+ messages in thread
From: Viresh Kumar @ 2016-10-20 8:44 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] 32+ messages in thread
* [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()
2016-10-20 8:44 [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar
` (2 preceding siblings ...)
2016-10-20 8:44 ` [PATCH V2 3/8] PM / OPP: Manage supply's voltage/current in a separate structure Viresh Kumar
@ 2016-10-20 8:44 ` Viresh Kumar
2016-10-24 23:14 ` Stephen Boyd
2016-10-20 8:44 ` [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators Viresh Kumar
` (4 subsequent siblings)
8 siblings, 1 reply; 32+ messages in thread
From: Viresh Kumar @ 2016-10-20 8:44 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] 32+ messages in thread
* [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators
2016-10-20 8:44 [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar
` (3 preceding siblings ...)
2016-10-20 8:44 ` [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage() Viresh Kumar
@ 2016-10-20 8:44 ` Viresh Kumar
2016-10-21 22:32 ` Dave Gerlach
2016-10-25 16:49 ` Stephen Boyd
2016-10-20 8:45 ` [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate() Viresh Kumar
` (3 subsequent siblings)
8 siblings, 2 replies; 32+ messages in thread
From: Viresh Kumar @ 2016-10-20 8:44 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] 32+ messages in thread
* [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate()
2016-10-20 8:44 [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar
` (4 preceding siblings ...)
2016-10-20 8:44 ` [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators Viresh Kumar
@ 2016-10-20 8:45 ` Viresh Kumar
2016-10-25 18:59 ` Stephen Boyd
2016-10-20 8:45 ` [PATCH V2 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks Viresh Kumar
` (2 subsequent siblings)
8 siblings, 1 reply; 32+ messages in thread
From: Viresh Kumar @ 2016-10-20 8:45 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 | 18 +----
include/linux/pm_opp.h | 31 ++++++++
3 files changed, 161 insertions(+), 69 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..6629c53c0aa1 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -47,22 +47,6 @@ 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
@@ -194,6 +178,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..73713a8424b1 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,36 @@ enum dev_pm_opp_event {
OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
};
+/**
+ * struct dev_pm_opp_supply - Power supply voltage/current values
+ * @u_volt: Target voltage in microvolts corresponding to this OPP
+ * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP
+ * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP
+ * @u_amp: Maximum current drawn by the device in microamperes
+ *
+ * This structure stores the voltage/current values for a single power supply.
+ */
+struct dev_pm_opp_supply {
+ unsigned long u_volt;
+ unsigned long u_volt_min;
+ unsigned long u_volt_max;
+ unsigned long u_amp;
+};
+
+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] 32+ messages in thread
* [PATCH V2 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks
2016-10-20 8:44 [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar
` (5 preceding siblings ...)
2016-10-20 8:45 ` [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate() Viresh Kumar
@ 2016-10-20 8:45 ` Viresh Kumar
2016-10-25 19:01 ` Stephen Boyd
2016-10-20 8:45 ` [PATCH V2 8/8] PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators() Viresh Kumar
2016-10-21 13:39 ` [PATCH V2 0/8] PM / OPP: Multiple regulator support Rafael J. Wysocki
8 siblings, 1 reply; 32+ messages in thread
From: Viresh Kumar @ 2016-10-20 8:45 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 6629c53c0aa1..86e31f06ca64 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -178,6 +178,7 @@ struct opp_table {
struct regulator **regulators;
unsigned int regulator_count;
+ int (*set_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 73713a8424b1..ce070257d6eb 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -95,6 +95,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);
@@ -194,6 +196,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] 32+ messages in thread
* [PATCH V2 8/8] PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators()
2016-10-20 8:44 [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar
` (6 preceding siblings ...)
2016-10-20 8:45 ` [PATCH V2 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks Viresh Kumar
@ 2016-10-20 8:45 ` Viresh Kumar
2016-10-25 19:01 ` Stephen Boyd
2016-10-21 13:39 ` [PATCH V2 0/8] PM / OPP: Multiple regulator support Rafael J. Wysocki
8 siblings, 1 reply; 32+ messages in thread
From: Viresh Kumar @ 2016-10-20 8:45 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] 32+ messages in thread
* Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support
2016-10-20 8:44 [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar
` (7 preceding siblings ...)
2016-10-20 8:45 ` [PATCH V2 8/8] PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators() Viresh Kumar
@ 2016-10-21 13:39 ` Rafael J. Wysocki
2016-10-21 15:40 ` Viresh Kumar
8 siblings, 1 reply; 32+ messages in thread
From: Rafael J. Wysocki @ 2016-10-21 13:39 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Nishanth Menon, Stephen Boyd,
Lists linaro-kernel, Linux PM, Linux Kernel Mailing List,
Vincent Guittot, Rob Herring, Dave Gerlach, Mark Brown
On Thu, Oct 20, 2016 at 10:44 AM, Viresh Kumar <viresh.kumar@linaro.org> 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.
>
> This is rebased over: linux-next branch in the PM tree.
>
> V1->V2:
> - Ack from Rob for 1st patch
> - Moved the supplies structure to pm_opp.h (Dave)
> - Fixed an compilation warning.
I need somebody from the OPP camp to review patches [2-8/8] for me.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support
2016-10-21 13:39 ` [PATCH V2 0/8] PM / OPP: Multiple regulator support Rafael J. Wysocki
@ 2016-10-21 15:40 ` Viresh Kumar
2016-10-24 1:08 ` Dave Gerlach
0 siblings, 1 reply; 32+ messages in thread
From: Viresh Kumar @ 2016-10-21 15:40 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael Wysocki, Nishanth Menon, Stephen Boyd,
Lists linaro-kernel, Linux PM, Linux Kernel Mailing List,
Vincent Guittot, Rob Herring, Dave Gerlach, Mark Brown
On 21 October 2016 at 19:09, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Oct 20, 2016 at 10:44 AM, Viresh Kumar <viresh.kumar@linaro.org> 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.
>>
>> This is rebased over: linux-next branch in the PM tree.
>>
>> V1->V2:
>> - Ack from Rob for 1st patch
>> - Moved the supplies structure to pm_opp.h (Dave)
>> - Fixed an compilation warning.
>
> I need somebody from the OPP camp to review patches [2-8/8] for me.
Sure, I have already asked Stephen yesterday to do that.
--
viresh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators
2016-10-20 8:44 ` [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators Viresh Kumar
@ 2016-10-21 22:32 ` Dave Gerlach
2016-10-24 3:35 ` Viresh Kumar
2016-10-25 16:49 ` Stephen Boyd
1 sibling, 1 reply; 32+ messages in thread
From: Dave Gerlach @ 2016-10-21 22:32 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/20/2016 03:44 AM, Viresh Kumar wrote:
> 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]);
Think this is leftover debug msg?
> + 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,
Though not in the patch there's a comment to
/* TODO: Support multiple regulators */
in the file right above the below opp_parse_supplies function that can probably
be removed as part of this patch.
Otherwise this patch looks good to me.
Regards,
Dave
> 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)
> {
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support
2016-10-21 15:40 ` Viresh Kumar
@ 2016-10-24 1:08 ` Dave Gerlach
2016-10-24 4:26 ` Viresh Kumar
0 siblings, 1 reply; 32+ messages in thread
From: Dave Gerlach @ 2016-10-24 1:08 UTC (permalink / raw)
To: Viresh Kumar, Rafael J. Wysocki
Cc: Rafael Wysocki, Nishanth Menon, Stephen Boyd,
Lists linaro-kernel, Linux PM, Linux Kernel Mailing List,
Vincent Guittot, Rob Herring, Mark Brown
Hi,
On 10/21/2016 10:40 AM, Viresh Kumar wrote:
> On 21 October 2016 at 19:09, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Thu, Oct 20, 2016 at 10:44 AM, Viresh Kumar <viresh.kumar@linaro.org> 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.
>>>
>>> This is rebased over: linux-next branch in the PM tree.
>>>
>>> V1->V2:
>>> - Ack from Rob for 1st patch
>>> - Moved the supplies structure to pm_opp.h (Dave)
>>> - Fixed an compilation warning.
>>
>> I need somebody from the OPP camp to review patches [2-8/8] for me.
>
> Sure, I have already asked Stephen yesterday to do that.
Overall this series looks good to me apart from a few small things. Most
importantly I was able to get a working implementation using two
regulators on ti dra7xx platform with proper sequencing built on top of
this series. We have cpu regulator and Adaptive body bias (abb)
regulator that must be scaled in a certain order before or after clock
scaling and I was able to implement a rough custom set_rate to perform
this and ran some dvfs stress tests that all worked fine.
First comment, I think the platform specific set_rate is a good place to
hook in for adaptive voltage scaling as well. I was able to implement TI
Class0 AVS in the same code by using the requested transition voltage as
a reference and programming AVS voltage using that, along with scaling
the additional regulators in sequence (the original multi regulator
functionality). I would think some people would want to use this even
with single regulator platforms, no? cpufreq-dt works as is for that, we
just swap out the regulators.
This raises some concerns about dependencies/probe sequencing. Right now
we just need to make sure the cpufreq-dt driver probes after we have
called _set_regulators, but if our platform code fails cpufreq-dt
currently will treat this as no regulator needed for the platform and
operate without one, which will likely hang the system. Is there a good
way to to guarantee this doesn't happen? My main concern is that if we
plan to provide a platform specific set-rate function, we should have a
way to indicate this and prevent things from progressing if it isn't yet
ready.
Again, overall I think it solves the multi regulator problem, and it
works well for AVS as well. For the series:
Tested-by: Dave Gerlach <d-gerlach@ti.com>
Regards,
Dave
>
> --
> viresh
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators
2016-10-21 22:32 ` Dave Gerlach
@ 2016-10-24 3:35 ` Viresh Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Viresh Kumar @ 2016-10-24 3:35 UTC (permalink / raw)
To: Dave Gerlach
Cc: Rafael Wysocki, nm, sboyd, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, broonie
On 21-10-16, 17:32, Dave Gerlach wrote:
> Hi,
> On 10/20/2016 03:44 AM, Viresh Kumar wrote:
> > 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]);
>
> Think this is leftover debug msg?
Yes.
> > + 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,
>
> Though not in the patch there's a comment to
>
> /* TODO: Support multiple regulators */
>
> in the file right above the below opp_parse_supplies function that can probably
> be removed as part of this patch.
Sure.
--
viresh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support
2016-10-24 1:08 ` Dave Gerlach
@ 2016-10-24 4:26 ` Viresh Kumar
2016-10-25 21:13 ` Dave Gerlach
0 siblings, 1 reply; 32+ messages in thread
From: Viresh Kumar @ 2016-10-24 4:26 UTC (permalink / raw)
To: Dave Gerlach
Cc: Rafael J. Wysocki, Rafael Wysocki, Nishanth Menon, Stephen Boyd,
Lists linaro-kernel, Linux PM, Linux Kernel Mailing List,
Vincent Guittot, Rob Herring, Mark Brown
On 23-10-16, 20:08, Dave Gerlach wrote:
> Overall this series looks good to me apart from a few small things. Most
> importantly I was able to get a working implementation using two regulators
> on ti dra7xx platform with proper sequencing built on top of this series. We
> have cpu regulator and Adaptive body bias (abb) regulator that must be
> scaled in a certain order before or after clock scaling and I was able to
> implement a rough custom set_rate to perform this and ran some dvfs stress
> tests that all worked fine.
Thanks for testing it buddy.
> First comment, I think the platform specific set_rate is a good place to
> hook in for adaptive voltage scaling as well. I was able to implement TI
> Class0 AVS in the same code by using the requested transition voltage as a
> reference and programming AVS voltage using that, along with scaling the
> additional regulators in sequence (the original multi regulator
> functionality).
Hmm, interesting..
> I would think some people would want to use this even with
> single regulator platforms, no?
Maybe, but I would like to see such user code first. It may be possible to
handle much of AVS stuff in core so that everyone isn't required to do it.
> This raises some concerns about dependencies/probe sequencing. Right now we
> just need to make sure the cpufreq-dt driver probes after we have called
> _set_regulators, but if our platform code fails cpufreq-dt currently will
> treat this as no regulator needed for the platform and operate without one,
> which will likely hang the system. Is there a good way to to guarantee this
> doesn't happen? My main concern is that if we plan to provide a platform
> specific set-rate function, we should have a way to indicate this and
> prevent things from progressing if it isn't yet ready.
>
> Again, overall I think it solves the multi regulator problem, and it works
> well for AVS as well. For the series:
>
> Tested-by: Dave Gerlach <d-gerlach@ti.com>
Thanks.
For the concern you shared about, does the below patch fix it ? I will include
that in V3 then.
-------------------------8<-------------------------
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Mon, 24 Oct 2016 09:45:30 +0530
Subject: [PATCH] PM / OPP: Don't assume platform doesn't have regulators
If the regulators aren't set explicitly by the platform, the OPP core
assumes that the platform doesn't have any regulator and uses the
clk-only callback.
If the platform failed to register a regulator with the core, then this
can turn out to be a dangerous assumption as the OPP core will try to
change clk without changing regulators.
Handle that properly by making sure that the DT didn't had any entries
for supply voltages as well.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/base/power/opp/core.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index b69908b74ed6..fb4250532180 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -737,7 +737,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
/* Only frequency scaling */
if (!regulators) {
- rcu_read_unlock();
+ /*
+ * DT contained supply ratings? Consider platform failed to set
+ * regulators.
+ */
+ if (unlikely(opp->supplies[0].u_volt)) {
+ rcu_read_unlock();
+ dev_err(dev, "%s: Regulator not registered with OPP core\n",
+ __func__);
+ return -EINVAL;
+ }
+
return _generic_opp_set_rate_clk_only(dev, clk, old_freq, freq);
}
--
viresh
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH V2 1/8] PM / OPP: Reword binding supporting multiple regulators per device
2016-10-20 8:44 ` [PATCH V2 1/8] PM / OPP: Reword binding supporting multiple regulators per device Viresh Kumar
@ 2016-10-24 22:48 ` Stephen Boyd
0 siblings, 0 replies; 32+ messages in thread
From: Stephen Boyd @ 2016-10-24 22:48 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie,
devicetree
On 10/20, 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>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section
2016-10-20 8:44 ` [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section Viresh Kumar
@ 2016-10-24 22:52 ` Stephen Boyd
2016-10-25 3:37 ` Viresh Kumar
0 siblings, 1 reply; 32+ messages in thread
From: Stephen Boyd @ 2016-10-24 22:52 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 10/20, Viresh Kumar wrote:
> 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>
Was this found by visual inspection or through some static
checker? Just curious.
> @@ -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) {
What if old_u_volt == 0 is valid? We could have another variable
like 'valid' or something that we use to figure out if we should
set values instead. Then this isn't a potential pitfall.
> + _set_opp_voltage(dev, reg, old_u_volt, old_u_volt_min,
> + old_u_volt_max);
> + }
>
> return ret;
> }
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()
2016-10-20 8:44 ` [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage() Viresh Kumar
@ 2016-10-24 23:14 ` Stephen Boyd
2016-10-25 3:45 ` Viresh Kumar
0 siblings, 1 reply; 32+ messages in thread
From: Stephen Boyd @ 2016-10-24 23:14 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 10/20, Viresh Kumar wrote:
> Pass the entire supply structure instead of all of its fields.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
This patch should be combined with the previous one. I'm still
not sure if it even makes sense to do this though. Do we really
have to make duplicate "OPP snapshot" structures just because of
how OPPs use RCU?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section
2016-10-24 22:52 ` Stephen Boyd
@ 2016-10-25 3:37 ` Viresh Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Viresh Kumar @ 2016-10-25 3:37 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 24-10-16, 15:52, Stephen Boyd wrote:
> On 10/20, Viresh Kumar wrote:
> > 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>
>
> Was this found by visual inspection or through some static
> checker? Just curious.
Visual inspection :)
> > @@ -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) {
>
> What if old_u_volt == 0 is valid?
How can that be valid ?
> We could have another variable
> like 'valid' or something that we use to figure out if we should
> set values instead. Then this isn't a potential pitfall.
I can do that but just wanted to know if we need that or not.
--
viresh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()
2016-10-24 23:14 ` Stephen Boyd
@ 2016-10-25 3:45 ` Viresh Kumar
2016-10-25 20:26 ` Stephen Boyd
0 siblings, 1 reply; 32+ messages in thread
From: Viresh Kumar @ 2016-10-25 3:45 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 24-10-16, 16:14, Stephen Boyd wrote:
> On 10/20, Viresh Kumar wrote:
> > Pass the entire supply structure instead of all of its fields.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
>
> This patch should be combined with the previous one.
I think it is a fair to do this separately as this is a completely different
logical change.
> I'm still
> not sure if it even makes sense to do this though.
:)
> Do we really
> have to make duplicate "OPP snapshot" structures just because of
> how OPPs use RCU?
I agree. With RCU, yes this change is probably required. But I am not sure if
RCU fits that well to OPP core anymore. A rw-lock may be much easier to help.
--
viresh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators
2016-10-20 8:44 ` [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators Viresh Kumar
2016-10-21 22:32 ` Dave Gerlach
@ 2016-10-25 16:49 ` Stephen Boyd
2016-10-26 6:05 ` Viresh Kumar
1 sibling, 1 reply; 32+ messages in thread
From: Stephen Boyd @ 2016-10-25 16:49 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 10/20, Viresh Kumar wrote:
> 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
> @@ -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);
How do we allocate under RCU? Doesn't that spit out big warnings?
> + if (!regulators) {
> + rcu_read_unlock();
> + return 0;
> + }
> +
> + min_uV = kmalloc(count * (sizeof(*min_uV) + sizeof(*max_uV)),
Do we imagine min_uV is going to be a different size from max_uV?
It may be better to have a struct for min/max pair and then
stride them. Then the kmalloc() can become a kmalloc_array().
> + 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();
> @@ -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);
So put the supplies at the end of the OPP structure as an empty
array?
> + 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;
> @@ -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[],
Make names a const char * const *? I seem to recall arrays as
function arguments has some problem but my C mastery is failing
right now.
> + 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]);
Debug noise?
> + 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:
> 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 */
But we don't verify that's the case? Why not use kasprintf() and
free() and then there isn't any limit?
> +
> + /* 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;
> 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
> @@ -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;
We can't have regulator_count == 1 by default?
> +
> /* Search for "opp-microvolt-<name>" */
> if (opp_table->prop_name) {
> snprintf(name, sizeof(name), "opp-microvolt-%s",
> @@ -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
Lowercase Power Supply please.
> * @dentry: debugfs dentry pointer of the real device directory (not links).
> * @dentry_name: Name of the real 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,
We can't just do &names instead here? Hmm...
> + ARRAY_SIZE(names));
> if (ret) {
> dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
> policy->cpu, ret);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate()
2016-10-20 8:45 ` [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate() Viresh Kumar
@ 2016-10-25 18:59 ` Stephen Boyd
2016-10-26 6:07 ` Viresh Kumar
0 siblings, 1 reply; 32+ messages in thread
From: Stephen Boyd @ 2016-10-25 18:59 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 10/20, Viresh Kumar wrote:
> Later patches would add support for custom opp_set_rate callbacks. This
I know the OPP consumer function has "rate" in the name, but it
makes more sense to call the callback set_opp instead because we
could be doing a lot more than setting the opp rate.
> 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>
> ---
> 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);
Perhaps this should stay at the beginning of OPP transitions?
Otherwise it can get confusing when multiple switching OPP
messages appear on OPP transition failures.
> +
> + ret = clk_set_rate(clk, freq);
> + if (ret) {
> + dev_err(dev, "%s: failed to set clock rate: %d\n", __func__,
> + ret);
> + }
> +
> + return ret;
> +}
> +
> diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
> index d3f0861f9bff..6629c53c0aa1 100644
> --- a/drivers/base/power/opp/opp.h
> +++ b/drivers/base/power/opp/opp.h
> @@ -47,22 +47,6 @@ 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
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 0606b70a8b97..73713a8424b1 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;
Is struct regulator also forward declared?
> struct dev_pm_opp;
> struct device;
>
> @@ -24,6 +25,36 @@ enum dev_pm_opp_event {
> OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
> };
>
> +/**
> + * struct dev_pm_opp_supply - Power supply voltage/current values
> + * @u_volt: Target voltage in microvolts corresponding to this OPP
> + * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP
> + * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP
> + * @u_amp: Maximum current drawn by the device in microamperes
> + *
> + * This structure stores the voltage/current values for a single power supply.
> + */
> +struct dev_pm_opp_supply {
> + unsigned long u_volt;
> + unsigned long u_volt_min;
> + unsigned long u_volt_max;
> + unsigned long u_amp;
> +};
This structure moved during this series. Can we avoid that and
already have it in the right place to begin with?
> +
> +struct dev_pm_opp_info {
> + unsigned long rate;
> + struct dev_pm_opp_supply *supplies;
> +};
> +
> +struct dev_pm_set_rate_data {
dev_pm_set_opp_data?
> + struct dev_pm_opp_info old_opp;
> + struct dev_pm_opp_info new_opp;
> +
> + struct regulator **regulators;
> + unsigned int regulator_count;
> + struct clk *clk;
> +};
The above two structures don't get kernel doc?
> +
> #if defined(CONFIG_PM_OPP)
>
> unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
> --
> 2.7.1.410.g6faf27b
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks
2016-10-20 8:45 ` [PATCH V2 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks Viresh Kumar
@ 2016-10-25 19:01 ` Stephen Boyd
2016-10-26 6:07 ` Viresh Kumar
0 siblings, 1 reply; 32+ messages in thread
From: Stephen Boyd @ 2016-10-25 19:01 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 10/20, Viresh Kumar wrote:
> 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>
> ---
Overall it looks ok, but I'd prefer set_opp instead of set_rate.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 8/8] PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators()
2016-10-20 8:45 ` [PATCH V2 8/8] PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators() Viresh Kumar
@ 2016-10-25 19:01 ` Stephen Boyd
0 siblings, 0 replies; 32+ messages in thread
From: Stephen Boyd @ 2016-10-25 19:01 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 10/20, Viresh Kumar wrote:
> If a platform specific OPP driver has called this routine first and set
> the regulators, then the second call from cpufreq-dt driver will hit the
> WARN_ON(). Remove the WARN_ON(), but continue to return error in such
> cases.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()
2016-10-25 3:45 ` Viresh Kumar
@ 2016-10-25 20:26 ` Stephen Boyd
2016-10-26 3:34 ` Viresh Kumar
0 siblings, 1 reply; 32+ messages in thread
From: Stephen Boyd @ 2016-10-25 20:26 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 10/25, Viresh Kumar wrote:
> On 24-10-16, 16:14, Stephen Boyd wrote:
> > On 10/20, Viresh Kumar wrote:
> > > Pass the entire supply structure instead of all of its fields.
> > >
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> >
> > This patch should be combined with the previous one.
>
> I think it is a fair to do this separately as this is a completely different
> logical change.
Let's agree to disagree.
>
> > I'm still
> > not sure if it even makes sense to do this though.
>
> :)
>
> > Do we really
> > have to make duplicate "OPP snapshot" structures just because of
> > how OPPs use RCU?
>
> I agree. With RCU, yes this change is probably required. But I am not sure if
> RCU fits that well to OPP core anymore. A rw-lock may be much easier to help.
>
For things like AVS we'll probably want to do that, although it's
sort of funny because replacing RCU with rw-locks is the opposite
direction most people go. With AVS we would be updating the
voltage(s) in use for the current OPP, and we would want that
update to block any OPP transition until the voltage is adjusted.
I don't know how we would do that with RCU very well. Plus, RCU
is for reader heavy things, but we mostly have one or two
readers.
I guess it's ok for now to do all this copying, but it feels like
we'll need to undo a large portion of it later with things like
AVS. Or at least we'll be doing copies for almost no reason
because we'll want to hold the read lock across the whole OPP
transition. I was going to suggest we pass around information
about what we want to grab from the RCU protected data
structures, think index of regulator, etc. and then have small
RCU read-side critical sections to grab that info during the OPP
transition but I'm not sure that's any better. It might be worse
because the OPP could change during the OPP transition and we
could be using half of the old and half of the new data.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support
2016-10-24 4:26 ` Viresh Kumar
@ 2016-10-25 21:13 ` Dave Gerlach
2016-10-26 3:21 ` Viresh Kumar
0 siblings, 1 reply; 32+ messages in thread
From: Dave Gerlach @ 2016-10-25 21:13 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Rafael Wysocki, Nishanth Menon, Stephen Boyd,
Lists linaro-kernel, Linux PM, Linux Kernel Mailing List,
Vincent Guittot, Rob Herring, Mark Brown
Hi,
On 10/23/2016 11:26 PM, Viresh Kumar wrote:
> On 23-10-16, 20:08, Dave Gerlach wrote:
>> Overall this series looks good to me apart from a few small things. Most
>> importantly I was able to get a working implementation using two regulators
>> on ti dra7xx platform with proper sequencing built on top of this series. We
>> have cpu regulator and Adaptive body bias (abb) regulator that must be
>> scaled in a certain order before or after clock scaling and I was able to
>> implement a rough custom set_rate to perform this and ran some dvfs stress
>> tests that all worked fine.
>
> Thanks for testing it buddy.
>
>> First comment, I think the platform specific set_rate is a good place to
>> hook in for adaptive voltage scaling as well. I was able to implement TI
>> Class0 AVS in the same code by using the requested transition voltage as a
>> reference and programming AVS voltage using that, along with scaling the
>> additional regulators in sequence (the original multi regulator
>> functionality).
>
> Hmm, interesting..
>
>> I would think some people would want to use this even with
>> single regulator platforms, no?
>
> Maybe, but I would like to see such user code first. It may be possible to
> handle much of AVS stuff in core so that everyone isn't required to do it.
Ok, I think it would be a logical next step to look at once this series
gets accepted. For now, the particular implementation I did just looks
up an optimized value for the requested voltage from a register and
programs the optimal value instead of the requested voltage.
>
>> This raises some concerns about dependencies/probe sequencing. Right now we
>> just need to make sure the cpufreq-dt driver probes after we have called
>> _set_regulators, but if our platform code fails cpufreq-dt currently will
>> treat this as no regulator needed for the platform and operate without one,
>> which will likely hang the system. Is there a good way to to guarantee this
>> doesn't happen? My main concern is that if we plan to provide a platform
>> specific set-rate function, we should have a way to indicate this and
>> prevent things from progressing if it isn't yet ready.
>>
>> Again, overall I think it solves the multi regulator problem, and it works
>> well for AVS as well. For the series:
>>
>> Tested-by: Dave Gerlach <d-gerlach@ti.com>
>
> Thanks.
>
> For the concern you shared about, does the below patch fix it ? I will include
> that in V3 then.
I think what you have shared below is a good safety check but if I
rename the regulator properties in the DT for the cpu (to vdd and vbb,
meaning cpufreq detects no regulator) and do *not* call
dev_pm_opp_set_regulators before cpufreq-dt probes we fail before we
even get to that point:
[16.946] cpu cpu0: opp_parse_supplies: Invalid number of elements in
opp-microvolt property (6) with supplies (1)
[16.967] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22
[16.982] cpu cpu0: dev_pm_opp_get_opp_count: OPP table not found (-19)
[16.982] cpu cpu0: OPP table is not ready, deferring probe
This failure is because opp_parse_supplies assumes a count of 1
regulator if no regulators at all are present and then hard fails if too
many voltages have been passed for each OPP. It seems we need a check
much earlier similar to what you suggested below to allow us to defer if
an OPP has supplied voltages but no regulator has been registered with
the system. I think this is reasonable even for the 1 regulator case,
no? If we have passed voltages then we presumably are hoping to use them
with a regulator, and if no regulators are present, OPP framework should
defer.
cpufreq-dt won't handle this properly as is, but now that the opp core
is evolving perhaps it makes sense to modify the resources_available
check slightly to rely on the OPP core rather than just a dummy
regulator_get_optional to see if the regulator is ready.
Regards,
Dave
>
> -------------------------8<-------------------------
>
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Mon, 24 Oct 2016 09:45:30 +0530
> Subject: [PATCH] PM / OPP: Don't assume platform doesn't have regulators
>
> If the regulators aren't set explicitly by the platform, the OPP core
> assumes that the platform doesn't have any regulator and uses the
> clk-only callback.
>
> If the platform failed to register a regulator with the core, then this
> can turn out to be a dangerous assumption as the OPP core will try to
> change clk without changing regulators.
>
> Handle that properly by making sure that the DT didn't had any entries
> for supply voltages as well.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/base/power/opp/core.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index b69908b74ed6..fb4250532180 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -737,7 +737,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>
> /* Only frequency scaling */
> if (!regulators) {
> - rcu_read_unlock();
> + /*
> + * DT contained supply ratings? Consider platform failed to set
> + * regulators.
> + */
> + if (unlikely(opp->supplies[0].u_volt)) {
> + rcu_read_unlock();
> + dev_err(dev, "%s: Regulator not registered with OPP core\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> return _generic_opp_set_rate_clk_only(dev, clk, old_freq, freq);
> }
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 0/8] PM / OPP: Multiple regulator support
2016-10-25 21:13 ` Dave Gerlach
@ 2016-10-26 3:21 ` Viresh Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Viresh Kumar @ 2016-10-26 3:21 UTC (permalink / raw)
To: Dave Gerlach
Cc: Rafael J. Wysocki, Rafael Wysocki, Nishanth Menon, Stephen Boyd,
Lists linaro-kernel, Linux PM, Linux Kernel Mailing List,
Vincent Guittot, Rob Herring, Mark Brown
On 25-10-16, 16:13, Dave Gerlach wrote:
> I think what you have shared below is a good safety check but if I rename
> the regulator properties in the DT for the cpu (to vdd and vbb, meaning
> cpufreq detects no regulator) and do *not* call dev_pm_opp_set_regulators
> before cpufreq-dt probes we fail before we even get to that point:
>
> [16.946] cpu cpu0: opp_parse_supplies: Invalid number of elements in
> opp-microvolt property (6) with supplies (1)
> [16.967] cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22
> [16.982] cpu cpu0: dev_pm_opp_get_opp_count: OPP table not found (-19)
> [16.982] cpu cpu0: OPP table is not ready, deferring probe
>
> This failure is because opp_parse_supplies assumes a count of 1 regulator if
> no regulators at all are present and then hard fails if too many voltages
> have been passed for each OPP.
Exactly. And yes this is intentional.
> It seems we need a check much earlier similar
> to what you suggested below to allow us to defer if an OPP has supplied
> voltages but no regulator has been registered with the system. I think this
> is reasonable even for the 1 regulator case, no?
No.
OPP core needs to know about regulators only if the user drivers want it to
manage DVFS. It is still possible for cpufreq drivers to use OPP framework for
managing the tables, but do the real DVFS stuff themselves. That's why it is not
compulsory in the code to set regulator names.
And its only wrong if dev_pm_opp_set_rate() is called without first setting the
regulators..
> cpufreq-dt won't handle this properly as is, but now that the opp core is
> evolving perhaps it makes sense to modify the resources_available check
> slightly to rely on the OPP core rather than just a dummy
> regulator_get_optional to see if the regulator is ready.
I am not sure yet on what to change there. You mean regarding multiple
regulators?
--
viresh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()
2016-10-25 20:26 ` Stephen Boyd
@ 2016-10-26 3:34 ` Viresh Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Viresh Kumar @ 2016-10-26 3:34 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 25-10-16, 13:26, Stephen Boyd wrote:
> For things like AVS we'll probably want to do that, although it's
> sort of funny because replacing RCU with rw-locks is the opposite
> direction most people go.
Yes, that would be very funny :)
> With AVS we would be updating the
> voltage(s) in use for the current OPP, and we would want that
> update to block any OPP transition until the voltage is adjusted.
> I don't know how we would do that with RCU very well. Plus, RCU
> is for reader heavy things, but we mostly have one or two
> readers.
Not just that, think of opp_disable() function. What guarantees currently that
an OPP being disabled isn't already used right now? Or is on the way of getting
used?
I strongly feel RCU is not the best fit for OPP core at least.
> I guess it's ok for now to do all this copying, but it feels like
> we'll need to undo a large portion of it later with things like
> AVS.
Yes.
> Or at least we'll be doing copies for almost no reason
> because we'll want to hold the read lock across the whole OPP
> transition. I was going to suggest we pass around information
> about what we want to grab from the RCU protected data
> structures, think index of regulator, etc. and then have small
> RCU read-side critical sections to grab that info during the OPP
> transition but I'm not sure that's any better. It might be worse
> because the OPP could change during the OPP transition and we
> could be using half of the old and half of the new data.
The problem is that this code is getting harder to read for everybody. If we are
finding it difficult to understand, what about newbies..
--
viresh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators
2016-10-25 16:49 ` Stephen Boyd
@ 2016-10-26 6:05 ` Viresh Kumar
2016-11-10 1:37 ` Stephen Boyd
0 siblings, 1 reply; 32+ messages in thread
From: Viresh Kumar @ 2016-10-26 6:05 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 25-10-16, 09:49, Stephen Boyd wrote:
> On 10/20, Viresh Kumar wrote:
> > 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
> > @@ -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);
>
> How do we allocate under RCU? Doesn't that spit out big warnings?
That doesn't. I even tried enabling several locking debug config options.
> > + if (!regulators) {
> > + rcu_read_unlock();
> > + return 0;
> > + }
> > +
> > + min_uV = kmalloc(count * (sizeof(*min_uV) + sizeof(*max_uV)),
>
> Do we imagine min_uV is going to be a different size from max_uV?
> It may be better to have a struct for min/max pair and then
> stride them. Then the kmalloc() can become a kmalloc_array().
done.
> > - *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);
>
> So put the supplies at the end of the OPP structure as an empty
> array?
Yes. Added a comment to clarify as well.
> > -int dev_pm_opp_set_regulator(struct device *dev, const char *name)
> > +int dev_pm_opp_set_regulators(struct device *dev, const char *names[],
>
> Make names a const char * const *?
Done.
> I seem to recall arrays as
> function arguments has some problem but my C mastery is failing
> right now.
I am not sure why it would be a problem, and of course what gets passed is the
address and not the array.
> > + 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]);
>
> Debug noise?
Yes.
> > +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 */
>
> But we don't verify that's the case? Why not use kasprintf() and
> free() and then there isn't any limit?
Done.
> > 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
> > @@ -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;
>
> We can't have regulator_count == 1 by default?
It is used at various places to distinguish if regulators are set by platform
code or not. The OPP core can still be used just for DT data, i.e. no opp-set.
And so it is important to support cases where the regulators aren't set.
> > @@ -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
>
> Lowercase Power Supply please.
Done.
> > * @dentry: debugfs dentry pointer of the real device directory (not links).
> > * @dentry_name: Name of the real 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,
>
> We can't just do &names instead here? Hmm...
I don't think so. You sure we can do it ?
--
viresh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate()
2016-10-25 18:59 ` Stephen Boyd
@ 2016-10-26 6:07 ` Viresh Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Viresh Kumar @ 2016-10-26 6:07 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 25-10-16, 11:59, Stephen Boyd wrote:
> On 10/20, Viresh Kumar wrote:
> > Later patches would add support for custom opp_set_rate callbacks. This
>
> I know the OPP consumer function has "rate" in the name, but it
> makes more sense to call the callback set_opp instead because we
> could be doing a lot more than setting the opp rate.
Done.
> > 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>
> > ---
> > 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);
>
> Perhaps this should stay at the beginning of OPP transitions?
> Otherwise it can get confusing when multiple switching OPP
> messages appear on OPP transition failures.
Done.
> > +struct clk;
>
> Is struct regulator also forward declared?
Done now.
> > struct dev_pm_opp;
> > struct device;
> >
> > @@ -24,6 +25,36 @@ enum dev_pm_opp_event {
> > OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
> > };
> >
> > +/**
> > + * struct dev_pm_opp_supply - Power supply voltage/current values
> > + * @u_volt: Target voltage in microvolts corresponding to this OPP
> > + * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP
> > + * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP
> > + * @u_amp: Maximum current drawn by the device in microamperes
> > + *
> > + * This structure stores the voltage/current values for a single power supply.
> > + */
> > +struct dev_pm_opp_supply {
> > + unsigned long u_volt;
> > + unsigned long u_volt_min;
> > + unsigned long u_volt_max;
> > + unsigned long u_amp;
> > +};
>
> This structure moved during this series. Can we avoid that and
> already have it in the right place to begin with?
Done.
> > +
> > +struct dev_pm_opp_info {
> > + unsigned long rate;
> > + struct dev_pm_opp_supply *supplies;
> > +};
> > +
> > +struct dev_pm_set_rate_data {
>
> dev_pm_set_opp_data?
Done.
> > + struct dev_pm_opp_info old_opp;
> > + struct dev_pm_opp_info new_opp;
> > +
> > + struct regulator **regulators;
> > + unsigned int regulator_count;
> > + struct clk *clk;
> > +};
>
> The above two structures don't get kernel doc?
Done.
--
viresh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks
2016-10-25 19:01 ` Stephen Boyd
@ 2016-10-26 6:07 ` Viresh Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Viresh Kumar @ 2016-10-26 6:07 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 25-10-16, 12:01, Stephen Boyd wrote:
> On 10/20, Viresh Kumar wrote:
> > 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>
> > ---
>
> Overall it looks ok, but I'd prefer set_opp instead of set_rate.
Done.
--
viresh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators
2016-10-26 6:05 ` Viresh Kumar
@ 2016-11-10 1:37 ` Stephen Boyd
0 siblings, 0 replies; 32+ messages in thread
From: Stephen Boyd @ 2016-11-10 1:37 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie
On 10/26, Viresh Kumar wrote:
> On 25-10-16, 09:49, Stephen Boyd wrote:
> > On 10/20, Viresh Kumar wrote:
> > > 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
> > > @@ -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);
> >
> > How do we allocate under RCU? Doesn't that spit out big warnings?
>
> That doesn't. I even tried enabling several locking debug config options.
Please read RCU documentation. From rcu_read_lock() function
documentation:
In non-preemptible RCU implementations (TREE_RCU and TINY_RCU),
it is illegal to block while in an RCU read-side critical section.
In preemptible RCU implementations (TREE_PREEMPT_RCU) in CONFIG_PREEMPT
kernel builds, RCU read-side critical sections may be preempted,
but explicit blocking is illegal. Finally, in preemptible RCU
implementations in real-time (with -rt patchset) kernel builds, RCU
read-side critical sections may be preempted and they may also block, but
only when acquiring spinlocks that are subject to priority inheritance.
I suppose that in certain configurations it will warn and in
others it won't. I thought lockdep would always complain though,
so that's sad it doesn't. At least in some implementations of RCU
rcu_read_lock() is the same as preempt_disable(), which basically
means no sleeping calls like GFP_KERNEL allocations.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2016-11-10 1:37 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-20 8:44 [PATCH V2 0/8] PM / OPP: Multiple regulator support Viresh Kumar
2016-10-20 8:44 ` [PATCH V2 1/8] PM / OPP: Reword binding supporting multiple regulators per device Viresh Kumar
2016-10-24 22:48 ` Stephen Boyd
2016-10-20 8:44 ` [PATCH V2 2/8] PM / OPP: Don't use OPP structure outside of rcu protected section Viresh Kumar
2016-10-24 22:52 ` Stephen Boyd
2016-10-25 3:37 ` Viresh Kumar
2016-10-20 8:44 ` [PATCH V2 3/8] PM / OPP: Manage supply's voltage/current in a separate structure Viresh Kumar
2016-10-20 8:44 ` [PATCH V2 4/8] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage() Viresh Kumar
2016-10-24 23:14 ` Stephen Boyd
2016-10-25 3:45 ` Viresh Kumar
2016-10-25 20:26 ` Stephen Boyd
2016-10-26 3:34 ` Viresh Kumar
2016-10-20 8:44 ` [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators Viresh Kumar
2016-10-21 22:32 ` Dave Gerlach
2016-10-24 3:35 ` Viresh Kumar
2016-10-25 16:49 ` Stephen Boyd
2016-10-26 6:05 ` Viresh Kumar
2016-11-10 1:37 ` Stephen Boyd
2016-10-20 8:45 ` [PATCH V2 6/8] PM / OPP: Separate out _generic_opp_set_rate() Viresh Kumar
2016-10-25 18:59 ` Stephen Boyd
2016-10-26 6:07 ` Viresh Kumar
2016-10-20 8:45 ` [PATCH V2 7/8] PM / OPP: Allow platform specific custom opp_set_rate() callbacks Viresh Kumar
2016-10-25 19:01 ` Stephen Boyd
2016-10-26 6:07 ` Viresh Kumar
2016-10-20 8:45 ` [PATCH V2 8/8] PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators() Viresh Kumar
2016-10-25 19:01 ` Stephen Boyd
2016-10-21 13:39 ` [PATCH V2 0/8] PM / OPP: Multiple regulator support Rafael J. Wysocki
2016-10-21 15:40 ` Viresh Kumar
2016-10-24 1:08 ` Dave Gerlach
2016-10-24 4:26 ` Viresh Kumar
2016-10-25 21:13 ` Dave Gerlach
2016-10-26 3:21 ` 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).