Hi,
Some platforms (like TI) have complex DVFS configuration for CPU devices, where multiple regulators are required to be configured to change DVFS state of the device. This was explained well by Nishanth earlier [1].
One of the major complaints around multiple regulators case was that the DT isn't responsible in any way to represent the order in which multiple supplies need to be programmed, before or after a frequency change. It was considered in this patch and such information is left for the platform specific OPP driver now, which can register its own opp_set_rate() callback with the OPP core and the OPP core will then call it during DVFS.
The patches are tested on Exynos5250 (Dual A15). I have hacked around DT and code to pass values for multiple regulators and verified that they are all properly read by the kernel (using debugfs interface).
Dave Gerlach has already tested [2] it on the real TI platforms and it works well for him.
This is rebased over: linux-next branch in the PM tree.
V5->V6: - Rebased over a recent fix (and resolved rebase conflicts) that will get applied before this series:
https://marc.info/?l=linux-kernel&m=148047843010418&w=2
V4->V5: - Stephen boyd had some minor review comments and gave his Reviewed-by tag for the rest. Only 2 patches don't have his RBY tag. - Individual patches contain the version history from V4 to V5.
V3->V4: - Separate out cpu-supply fix in the binding in a separate patch (Mark). - Add more documentation to the binding to explain that the relation to the supplies and the order of programming them is left for the platform specific bindings and that every platform using multiple regulators for their devices needs to provide a separate binding document explaining their implementation (Mark). - @Rob and Stephen: I have kept your Acks for the bindings as the bindings only got a bit reworded (improved) since the time you guys Acked them. Please let me know if you want more improvement in the bindings now. - V4 for 10/10 was sent earlier, which added a missing rcu_read_unlock(). Nothing else changed in it. - Added some missing Kernel documentation comments
V2->V3: - The last patch is new - Removed a debug leftover pr_info() message - Renamed few names as s/set_rate/set_opp - Removed a TODO comment (as it is done now with this series) - created struct for min_uV and max_uV - kerneldoc comments for structures in pm_opp.h - s/const char */const char * const - use kasprintf() - Some more minor reformatting - More Ack/RBY tags added
V1->V2: - Ack from Rob for 1st patch - Moved the supplies structure to pm_opp.h (Dave) - Fixed an compilation warning.
-- viresh
[1] https://marc.info/?l=linux-pm&m=145684495832764&w=2 [2] https://marc.info/?l=linux-kernel&m=147924789305276&w=2
Viresh Kumar (10): PM / OPP: Fix incorrect cpu-supply property in binding 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_set_opp() PM / OPP: Allow platform specific custom set_opp() callbacks PM / OPP: Don't WARN on multiple calls to dev_pm_opp_set_regulators() PM / OPP: Don't assume platform doesn't have regulators
Documentation/devicetree/bindings/opp/opp.txt | 27 +- drivers/base/power/opp/core.c | 535 ++++++++++++++++++++------ drivers/base/power/opp/debugfs.c | 52 ++- drivers/base/power/opp/of.c | 105 +++-- drivers/base/power/opp/opp.h | 22 +- drivers/cpufreq/cpufreq-dt.c | 9 +- include/linux/pm_opp.h | 69 +++- 7 files changed, 635 insertions(+), 184 deletions(-)
The regulator bindings allow the "<name>-supply" property to define a single parent supply and not a list of parents.
Fix the wrong example code present in OPP bindings.
While at it also change the compatible string as Rob pointed out earlier that none of A7 implementation have multiple supplies for the CPU core.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Acked-by: Rob Herring robh@kernel.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org --- Documentation/devicetree/bindings/opp/opp.txt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt index ee91cbdd95ee..f0239f68d186 100644 --- a/Documentation/devicetree/bindings/opp/opp.txt +++ b/Documentation/devicetree/bindings/opp/opp.txt @@ -386,10 +386,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>; }; };
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.
- There is no way for the code parsing these bindings to know which voltage values belong to which power supply.
- It is not possible to know the order in which the supplies need to be configured while switching OPPs.
This patch clarifies on those details by mentioning that such information is left for the implementation specific bindings to explain. They may want to hardcode such details or implement their own properties to get such information. All implementations using multiple regulators for their devices must provide a binding document explaining their implementation.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Acked-by: Rob Herring robh@kernel.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org --- Documentation/devicetree/bindings/opp/opp.txt | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt index f0239f68d186..9f5ca4457b5f 100644 --- a/Documentation/devicetree/bindings/opp/opp.txt +++ b/Documentation/devicetree/bindings/opp/opp.txt @@ -86,8 +86,14 @@ 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 and that is left for the implementation specific + binding. + + 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 +110,13 @@ 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 and that is left for the + implementation specific binding.
- opp-microamp-<name>: Named opp-microamp property. Similar to opp-microvolt-<name> property, but for microamp instead.
The OPP structure must not be used out of the rcu protected section. Cache the values to be used in separate variables instead.
Cc: # v4.6+ stable@vger.kernel.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org Tested-by: Dave Gerlach d-gerlach@ti.com --- drivers/base/power/opp/core.c | 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 60a4210ec6d1..705137da3954 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; }
This is a preparatory step for multiple regulator per device support. Move the voltage/current variables to a new structure.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com Reviewed-by: Stephen Boyd sboyd@codeaurora.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 | 11 +++------- include/linux/pm_opp.h | 16 +++++++++++++++ 5 files changed, 55 insertions(+), 42 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 705137da3954..f706a961815d 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 5b3755e49731..bdf409d42126 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 96cd30ac6c1d..8a02516542c2 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -61,10 +61,7 @@ extern struct list_head opp_tables; * @turbo: true if turbo (boost) OPP * @suspend: true if suspend OPP * @rate: Frequency in hertz - * @u_volt: Target voltage in microvolts corresponding to this OPP - * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP - * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP - * @u_amp: Maximum current drawn by the device in microamperes + * @supply: Power supply voltage/current values * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's * frequency from any other OPP's frequency. * @opp_table: points back to the opp_table struct this opp belongs to @@ -83,10 +80,8 @@ struct dev_pm_opp { bool suspend; unsigned long rate;
- unsigned long u_volt; - unsigned long u_volt_min; - unsigned long u_volt_max; - unsigned long u_amp; + struct dev_pm_opp_supply supply; + unsigned long clock_latency_ns;
struct opp_table *opp_table; diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index f6bc76501912..824f7268f687 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -25,6 +25,22 @@ enum dev_pm_opp_event { OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE, };
+/** + * struct dev_pm_opp_supply - Power supply voltage/current values + * @u_volt: Target voltage in microvolts corresponding to this OPP + * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP + * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP + * @u_amp: Maximum current drawn by the device in microamperes + * + * This structure stores the voltage/current values for a single power supply. + */ +struct dev_pm_opp_supply { + unsigned long u_volt; + unsigned long u_volt_min; + unsigned long u_volt_max; + unsigned long u_amp; +}; + #if defined(CONFIG_PM_OPP)
unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
Pass the entire supply structure instead of all of its fields.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com Reviewed-by: Stephen Boyd sboyd@codeaurora.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 f706a961815d..8328ff06cc08 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; }
This patch adds infrastructure to manage multiple regulators and updates the only user (cpufreq-dt) of dev_pm_opp_set{put}_regulator().
This is preparatory work for adding full support for devices with multiple regulators.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com
--- V4->V5: - Don't allocate from within rcu locked section. - s/+// --- drivers/base/power/opp/core.c | 245 +++++++++++++++++++++++++++------------ drivers/base/power/opp/debugfs.c | 52 +++++++-- drivers/base/power/opp/of.c | 103 +++++++++++----- drivers/base/power/opp/opp.h | 10 +- drivers/cpufreq/cpufreq-dt.c | 9 +- include/linux/pm_opp.h | 8 +- 6 files changed, 302 insertions(+), 125 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 8328ff06cc08..f0dd0bd29ce8 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; } @@ -210,6 +212,24 @@ unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev) } EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_clock_latency);
+static int _get_regulator_count(struct device *dev) +{ + struct opp_table *opp_table; + int count; + + rcu_read_lock(); + + opp_table = _find_opp_table(dev); + if (!IS_ERR(opp_table)) + count = opp_table->regulator_count; + else + count = 0; + + rcu_read_unlock(); + + return count; +} + /** * dev_pm_opp_get_max_volt_latency() - Get max voltage latency in nanoseconds * @dev: device for which we do this operation @@ -222,34 +242,51 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) { struct opp_table *opp_table; struct dev_pm_opp *opp; - struct regulator *reg; + struct regulator *reg, **regulators; unsigned long latency_ns = 0; - unsigned long min_uV = ~0, max_uV = 0; - int ret; + int ret, i, count; + struct { + unsigned long min; + unsigned long max; + } *uV; + + count = _get_regulator_count(dev); + + /* Regulator may not be required for the device */ + if (!count) + return 0; + + regulators = kmalloc_array(count, sizeof(*regulators), GFP_KERNEL); + if (!regulators) + return 0; + + uV = kmalloc_array(count, sizeof(*uV), GFP_KERNEL); + if (!uV) + goto free_regulators;
rcu_read_lock();
opp_table = _find_opp_table(dev); if (IS_ERR(opp_table)) { rcu_read_unlock(); - return 0; + goto free_uV; }
- reg = opp_table->regulator; - if (IS_ERR(reg)) { - /* Regulator may not be required for device */ - rcu_read_unlock(); - return 0; - } + memcpy(regulators, opp_table->regulators, count * sizeof(*regulators));
- list_for_each_entry_rcu(opp, &opp_table->opp_list, node) { - if (!opp->available) - continue; + for (i = 0; i < count; i++) { + uV[i].min = ~0; + uV[i].max = 0; + + list_for_each_entry_rcu(opp, &opp_table->opp_list, node) { + if (!opp->available) + continue;
- if (opp->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 < uV[i].min) + uV[i].min = opp->supplies[i].u_volt_min; + if (opp->supplies[i].u_volt_max > uV[i].max) + uV[i].max = opp->supplies[i].u_volt_max; + } }
rcu_read_unlock(); @@ -258,9 +295,16 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) * The caller needs to ensure that opp_table (and hence the regulator) * isn't freed, while we are executing this routine. */ - ret = regulator_set_voltage_time(reg, min_uV, max_uV); - if (ret > 0) - latency_ns = ret * 1000; + for (i = 0; reg = regulators[i], i < count; i++) { + ret = regulator_set_voltage_time(reg, uV[i].min, uV[i].max); + if (ret > 0) + latency_ns += ret * 1000; + } + +free_uV: + kfree(uV); +free_regulators: + kfree(regulators);
return latency_ns; } @@ -580,7 +624,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 +677,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 +817,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 +865,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 +974,50 @@ struct dev_pm_opp *_allocate_opp(struct device *dev, struct opp_table **opp_table) { struct dev_pm_opp *opp; + int count, supply_size; + struct opp_table *table;
- /* allocate new OPP node */ - opp = kzalloc(sizeof(*opp), GFP_KERNEL); - if (!opp) + table = _add_opp_table(dev); + if (!table) return NULL;
- INIT_LIST_HEAD(&opp->node); + /* Allocate space for at least one supply */ + count = table->regulator_count ? table->regulator_count : 1; + supply_size = sizeof(*opp->supplies) * count;
- *opp_table = _add_opp_table(dev); - if (!*opp_table) { - kfree(opp); + /* allocate new OPP node and supplies structures */ + opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL); + if (!opp) { + kfree(table); return NULL; }
+ /* Put the supplies at the end of the OPP structure as an empty array */ + opp->supplies = (struct dev_pm_opp_supply *)(opp + 1); + INIT_LIST_HEAD(&opp->node); + + *opp_table = table; + return opp; }
static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, struct opp_table *opp_table) { - struct regulator *reg = opp_table->regulator; - - if (!IS_ERR(reg) && - !regulator_is_supported_voltage(reg, opp->supply.u_volt_min, - opp->supply.u_volt_max)) { - pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n", - __func__, opp->supply.u_volt_min, - opp->supply.u_volt_max); - return false; + struct regulator *reg; + int i; + + for (i = 0; i < opp_table->regulator_count; i++) { + reg = opp_table->regulators[i]; + + if (!regulator_is_supported_voltage(reg, + opp->supplies[i].u_volt_min, + opp->supplies[i].u_volt_max)) { + pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n", + __func__, opp->supplies[i].u_volt_min, + opp->supplies[i].u_volt_max); + return false; + } }
return true; @@ -984,12 +1049,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 +1122,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 +1369,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,10 +1386,13 @@ 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. */ -struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name) +struct opp_table *dev_pm_opp_set_regulators(struct device *dev, + const char * const names[], + unsigned int count) { struct opp_table *opp_table; struct regulator *reg; + int ret, i;
mutex_lock(&opp_table_lock);
@@ -1337,26 +1408,44 @@ struct opp_table *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)) { opp_table = ERR_PTR(-EBUSY); goto err; } - /* Allocate the regulator */ - reg = regulator_get_optional(dev, name); - if (IS_ERR(reg)) { - opp_table = (struct opp_table *)reg; - if (PTR_ERR(reg) != -EPROBE_DEFER) - dev_err(dev, "%s: no regulator (%s) found: %ld\n", - __func__, name, PTR_ERR(reg)); + + opp_table->regulators = kmalloc_array(count, + sizeof(*opp_table->regulators), + GFP_KERNEL); + if (!opp_table->regulators) { + opp_table = ERR_PTR(-ENOMEM); goto err; }
- opp_table->regulator = reg; + for (i = 0; i < count; i++) { + reg = regulator_get_optional(dev, names[i]); + if (IS_ERR(reg)) { + opp_table = (struct opp_table *)reg; + if (PTR_ERR(reg) != -EPROBE_DEFER) + dev_err(dev, "%s: no regulator (%s) found: %ld\n", + __func__, names[i], PTR_ERR(reg)); + goto free_regulators; + } + + opp_table->regulators[i] = reg; + } + + opp_table->regulator_count = count;
mutex_unlock(&opp_table_lock); return opp_table;
+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: @@ -1364,10 +1453,10 @@ struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name)
return opp_table; } -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_pm_opp_put_regulators() - Releases resources blocked for regulators * @opp_table: opp_table returned from dev_pm_opp_set_regulator * * Locking: The internal opp_table and opp structures are RCU protected. @@ -1376,20 +1465,26 @@ 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 opp_table *opp_table) +void dev_pm_opp_put_regulators(struct opp_table *opp_table) { + int i; + mutex_lock(&opp_table_lock);
- if (IS_ERR(opp_table->regulator)) { - pr_err("%s: Doesn't have regulator set\n", __func__); + if (!opp_table->regulators) { + pr_err("%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); @@ -1397,7 +1492,7 @@ void dev_pm_opp_put_regulator(struct opp_table *opp_table) unlock: mutex_unlock(&opp_table_lock); } -EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulator); +EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
/** * dev_pm_opp_add() - Add an OPP table from a table definitions diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c index c897676ca35f..95f433db4ac7 100644 --- a/drivers/base/power/opp/debugfs.c +++ b/drivers/base/power/opp/debugfs.c @@ -15,6 +15,7 @@ #include <linux/err.h> #include <linux/init.h> #include <linux/limits.h> +#include <linux/slab.h>
#include "opp.h"
@@ -34,6 +35,46 @@ void opp_debug_remove_one(struct dev_pm_opp *opp) debugfs_remove_recursive(opp->dentry); }
+static bool opp_debug_create_supplies(struct dev_pm_opp *opp, + struct opp_table *opp_table, + struct dentry *pdentry) +{ + struct dentry *d; + int i = 0; + char *name; + + /* Always create at least supply-0 directory */ + do { + name = kasprintf(GFP_KERNEL, "supply-%d", i); + + /* Create per-opp directory */ + d = debugfs_create_dir(name, pdentry); + + kfree(name); + + if (!d) + return false; + + if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, + &opp->supplies[i].u_volt)) + return false; + + if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, + &opp->supplies[i].u_volt_min)) + return false; + + if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, + &opp->supplies[i].u_volt_max)) + return false; + + if (!debugfs_create_ulong("u_amp", S_IRUGO, d, + &opp->supplies[i].u_amp)) + return false; + } while (++i < opp_table->regulator_count); + + return true; +} + int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) { struct dentry *pdentry = opp_table->dentry; @@ -63,16 +104,7 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate)) return -ENOMEM;
- if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt)) - return -ENOMEM; - - if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min)) - return -ENOMEM; - - if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max)) - return -ENOMEM; - - if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp)) + if (!opp_debug_create_supplies(opp, opp_table, d)) return -ENOMEM;
if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d, diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c index bdf409d42126..3f7d2591b173 100644 --- a/drivers/base/power/opp/of.c +++ b/drivers/base/power/opp/of.c @@ -17,6 +17,7 @@ #include <linux/errno.h> #include <linux/device.h> #include <linux/of.h> +#include <linux/slab.h> #include <linux/export.h>
#include "opp.h" @@ -101,16 +102,16 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table, return true; }
-/* TODO: Support multiple regulators */ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, struct opp_table *opp_table) { - u32 microvolt[3] = {0}; - u32 val; - int count, ret; + u32 *microvolt, *microamp = NULL; + int supplies, vcount, icount, ret, i, j; struct property *prop = NULL; char name[NAME_MAX];
+ supplies = opp_table->regulator_count ? opp_table->regulator_count : 1; + /* Search for "opp-microvolt-<name>" */ if (opp_table->prop_name) { snprintf(name, sizeof(name), "opp-microvolt-%s", @@ -128,34 +129,29 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, return 0; }
- count = of_property_count_u32_elems(opp->np, name); - if (count < 0) { + vcount = of_property_count_u32_elems(opp->np, name); + if (vcount < 0) { dev_err(dev, "%s: Invalid %s property (%d)\n", - __func__, name, count); - return count; + __func__, name, vcount); + return vcount; }
- /* There can be one or three elements here */ - if (count != 1 && count != 3) { - dev_err(dev, "%s: Invalid number of elements in %s property (%d)\n", - __func__, name, count); + /* There can be one or three elements per supply */ + if (vcount != supplies && vcount != supplies * 3) { + dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n", + __func__, name, vcount, supplies); return -EINVAL; }
- ret = of_property_read_u32_array(opp->np, name, microvolt, count); + microvolt = kmalloc_array(vcount, sizeof(*microvolt), GFP_KERNEL); + if (!microvolt) + return -ENOMEM; + + ret = of_property_read_u32_array(opp->np, name, microvolt, vcount); if (ret) { dev_err(dev, "%s: error parsing %s: %d\n", __func__, name, ret); - return -EINVAL; - } - - opp->supply.u_volt = microvolt[0]; - - if (count == 1) { - opp->supply.u_volt_min = opp->supply.u_volt; - opp->supply.u_volt_max = opp->supply.u_volt; - } else { - opp->supply.u_volt_min = microvolt[1]; - opp->supply.u_volt_max = microvolt[2]; + ret = -EINVAL; + goto free_microvolt; }
/* Search for "opp-microamp-<name>" */ @@ -172,10 +168,59 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, prop = of_find_property(opp->np, name, NULL); }
- if (prop && !of_property_read_u32(opp->np, name, &val)) - opp->supply.u_amp = val; + if (prop) { + icount = of_property_count_u32_elems(opp->np, name); + if (icount < 0) { + dev_err(dev, "%s: Invalid %s property (%d)\n", __func__, + name, icount); + ret = icount; + goto free_microvolt; + }
- return 0; + if (icount != supplies) { + dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n", + __func__, name, icount, supplies); + ret = -EINVAL; + goto free_microvolt; + } + + microamp = kmalloc_array(icount, sizeof(*microamp), GFP_KERNEL); + if (!microamp) { + ret = -EINVAL; + goto free_microvolt; + } + + ret = of_property_read_u32_array(opp->np, name, microamp, + icount); + if (ret) { + dev_err(dev, "%s: error parsing %s: %d\n", __func__, + name, ret); + ret = -EINVAL; + goto free_microamp; + } + } + + for (i = 0, j = 0; i < supplies; i++) { + opp->supplies[i].u_volt = microvolt[j++]; + + if (vcount == supplies) { + opp->supplies[i].u_volt_min = opp->supplies[i].u_volt; + opp->supplies[i].u_volt_max = opp->supplies[i].u_volt; + } else { + opp->supplies[i].u_volt_min = microvolt[j++]; + opp->supplies[i].u_volt_max = microvolt[j++]; + } + + if (microamp) + opp->supplies[i].u_amp = microamp[i]; + } + +free_microamp: + kfree(microamp); +free_microvolt: + kfree(microvolt); + + return ret; }
/** @@ -304,8 +349,8 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n", __func__, new_opp->turbo, new_opp->rate, - new_opp->supply.u_volt, new_opp->supply.u_volt_min, - new_opp->supply.u_volt_max, new_opp->clock_latency_ns); + new_opp->supplies[0].u_volt, new_opp->supplies[0].u_volt_min, + new_opp->supplies[0].u_volt_max, new_opp->clock_latency_ns);
/* * Notify the changes in the availability of the operable diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index 8a02516542c2..5b0f7e53bede 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -61,7 +61,7 @@ extern struct list_head opp_tables; * @turbo: true if turbo (boost) OPP * @suspend: true if suspend OPP * @rate: Frequency in hertz - * @supply: Power supply voltage/current values + * @supplies: Power supplies voltage/current values * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's * frequency from any other OPP's frequency. * @opp_table: points back to the opp_table struct this opp belongs to @@ -80,7 +80,7 @@ struct dev_pm_opp { bool suspend; unsigned long rate;
- struct dev_pm_opp_supply supply; + struct dev_pm_opp_supply *supplies;
unsigned long clock_latency_ns;
@@ -139,7 +139,8 @@ enum opp_table_access { * @supported_hw_count: Number of elements in supported_hw array. * @prop_name: A name to postfix to many DT properties, while parsing them. * @clk: Device's clock handle - * @regulator: Supply regulator + * @regulators: Supply regulators + * @regulator_count: Number of power supply regulators * @dentry: debugfs dentry pointer of the real device directory (not links). * @dentry_name: Name of the real dentry. * @@ -174,7 +175,8 @@ struct opp_table { unsigned int supported_hw_count; const char *prop_name; struct clk *clk; - struct regulator *regulator; + struct regulator **regulators; + unsigned int regulator_count;
#ifdef CONFIG_DEBUG_FS struct dentry *dentry; diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 4d3ec92cbabf..1cd1fcfe835a 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -188,7 +188,10 @@ static int cpufreq_init(struct cpufreq_policy *policy) */ name = find_supply_name(cpu_dev); if (name) { - opp_table = dev_pm_opp_set_regulator(cpu_dev, name); + const char *names[] = {name}; + + opp_table = dev_pm_opp_set_regulators(cpu_dev, names, + ARRAY_SIZE(names)); if (IS_ERR(opp_table)) { ret = PTR_ERR(opp_table); dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n", @@ -289,7 +292,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(opp_table); + dev_pm_opp_put_regulators(opp_table); out_put_clk: clk_put(cpu_clk);
@@ -304,7 +307,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->opp_table); + dev_pm_opp_put_regulators(priv->opp_table);
clk_put(policy->clk); kfree(priv); diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 824f7268f687..9a825ae78653 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -79,8 +79,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); -struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name); -void dev_pm_opp_put_regulator(struct opp_table *opp_table); +struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count); +void dev_pm_opp_put_regulators(struct opp_table *opp_table); 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); @@ -187,12 +187,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 struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name) +static inline struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count) { return ERR_PTR(-ENOTSUPP); }
-static inline void dev_pm_opp_put_regulator(struct opp_table *opp_table) {} +static inline void dev_pm_opp_put_regulators(struct opp_table *opp_table) {}
static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) {
On 11/30, Viresh Kumar wrote:
@@ -1337,26 +1408,44 @@ struct opp_table *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)) { opp_table = ERR_PTR(-EBUSY); goto err; }
- /* Allocate the regulator */
- reg = regulator_get_optional(dev, name);
- if (IS_ERR(reg)) {
opp_table = (struct opp_table *)reg;
if (PTR_ERR(reg) != -EPROBE_DEFER)
dev_err(dev, "%s: no regulator (%s) found: %ld\n",
__func__, name, PTR_ERR(reg));
- opp_table->regulators = kmalloc_array(count,
sizeof(*opp_table->regulators),
GFP_KERNEL);
- if (!opp_table->regulators) {
goto err; }opp_table = ERR_PTR(-ENOMEM);
- opp_table->regulator = reg;
- for (i = 0; i < count; i++) {
reg = regulator_get_optional(dev, names[i]);
if (IS_ERR(reg)) {
opp_table = (struct opp_table *)reg;
This should be an ERR_CAST() instead. But this is not based on the correct patch anyway so this should be rebased.
if (PTR_ERR(reg) != -EPROBE_DEFER)
dev_err(dev, "%s: no regulator (%s) found: %ld\n",
__func__, names[i], PTR_ERR(reg));
goto free_regulators;
}
opp_table->regulators[i] = reg;
- }
- opp_table->regulator_count = count;
mutex_unlock(&opp_table_lock); return opp_table; +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: @@ -1364,10 +1453,10 @@ struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name) return opp_table; } -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_pm_opp_put_regulators() - Releases resources blocked for regulators
- @opp_table: opp_table returned from dev_pm_opp_set_regulator
This needs an update too ^
- Locking: The internal opp_table and opp structures are RCU protected.
diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c index c897676ca35f..95f433db4ac7 100644 --- a/drivers/base/power/opp/debugfs.c +++ b/drivers/base/power/opp/debugfs.c @@ -34,6 +35,46 @@ void opp_debug_remove_one(struct dev_pm_opp *opp) debugfs_remove_recursive(opp->dentry); } +static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
struct opp_table *opp_table,
struct dentry *pdentry)
+{
- struct dentry *d;
- int i = 0;
- char *name;
- /* Always create at least supply-0 directory */
- do {
name = kasprintf(GFP_KERNEL, "supply-%d", i);
Sorry I haven't noticed this before. Wouldn't we want to put the supply name here instead of a number?
/* Create per-opp directory */
d = debugfs_create_dir(name, pdentry);
kfree(name);
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 4d3ec92cbabf..1cd1fcfe835a 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -188,7 +188,10 @@ static int cpufreq_init(struct cpufreq_policy *policy) */ name = find_supply_name(cpu_dev); if (name) {
opp_table = dev_pm_opp_set_regulator(cpu_dev, name);
const char *names[] = {name};
opp_table = dev_pm_opp_set_regulators(cpu_dev, names,
ARRAY_SIZE(names));
This can be simplified to:
opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1);
if (IS_ERR(opp_table)) { ret = PTR_ERR(opp_table); dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
On 30-11-16, 14:37, Stephen Boyd wrote:
+static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
struct opp_table *opp_table,
struct dentry *pdentry)
+{
- struct dentry *d;
- int i = 0;
- char *name;
- /* Always create at least supply-0 directory */
- do {
name = kasprintf(GFP_KERNEL, "supply-%d", i);
Sorry I haven't noticed this before. Wouldn't we want to put the supply name here instead of a number?
The OPP core provides a way for platforms to use the set-OPP functionality, but that is not compulsory for its user. Which makes it very much possible that the OPP core wouldn't know the names of the supplies at all. And so I used the numbers here. Anyway, the numbers matches the order in which they are passed from DT, so that should be readable.
Here is the new rebased version of the patch. I will send the complete series again later on. Please see if it looks fine.
-------------------------8<-------------------------
From: Viresh Kumar viresh.kumar@linaro.org Date: Tue, 4 Oct 2016 17:07:28 +0530 Subject: [PATCH] PM / OPP: Add infrastructure to manage multiple regulators
This patch adds infrastructure to manage multiple regulators and updates the only user (cpufreq-dt) of dev_pm_opp_set{put}_regulator().
This is preparatory work for adding full support for devices with multiple regulators.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com --- drivers/base/power/opp/core.c | 248 +++++++++++++++++++++++++++------------ drivers/base/power/opp/debugfs.c | 52 ++++++-- drivers/base/power/opp/of.c | 103 +++++++++++----- drivers/base/power/opp/opp.h | 10 +- drivers/cpufreq/cpufreq-dt.c | 9 +- include/linux/pm_opp.h | 8 +- 6 files changed, 303 insertions(+), 127 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 46ad8470c438..b4da31c5a5eb 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; } @@ -210,6 +212,24 @@ unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev) } EXPORT_SYMBOL_GPL(dev_pm_opp_get_max_clock_latency);
+static int _get_regulator_count(struct device *dev) +{ + struct opp_table *opp_table; + int count; + + rcu_read_lock(); + + opp_table = _find_opp_table(dev); + if (!IS_ERR(opp_table)) + count = opp_table->regulator_count; + else + count = 0; + + rcu_read_unlock(); + + return count; +} + /** * dev_pm_opp_get_max_volt_latency() - Get max voltage latency in nanoseconds * @dev: device for which we do this operation @@ -222,34 +242,51 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) { struct opp_table *opp_table; struct dev_pm_opp *opp; - struct regulator *reg; + struct regulator *reg, **regulators; unsigned long latency_ns = 0; - unsigned long min_uV = ~0, max_uV = 0; - int ret; + int ret, i, count; + struct { + unsigned long min; + unsigned long max; + } *uV; + + count = _get_regulator_count(dev); + + /* Regulator may not be required for the device */ + if (!count) + return 0; + + regulators = kmalloc_array(count, sizeof(*regulators), GFP_KERNEL); + if (!regulators) + return 0; + + uV = kmalloc_array(count, sizeof(*uV), GFP_KERNEL); + if (!uV) + goto free_regulators;
rcu_read_lock();
opp_table = _find_opp_table(dev); if (IS_ERR(opp_table)) { rcu_read_unlock(); - return 0; + goto free_uV; }
- reg = opp_table->regulator; - if (IS_ERR(reg)) { - /* Regulator may not be required for device */ - rcu_read_unlock(); - return 0; - } + memcpy(regulators, opp_table->regulators, count * sizeof(*regulators));
- list_for_each_entry_rcu(opp, &opp_table->opp_list, node) { - if (!opp->available) - continue; + for (i = 0; i < count; i++) { + uV[i].min = ~0; + uV[i].max = 0;
- if (opp->supply.u_volt_min < min_uV) - min_uV = opp->supply.u_volt_min; - if (opp->supply.u_volt_max > max_uV) - max_uV = opp->supply.u_volt_max; + list_for_each_entry_rcu(opp, &opp_table->opp_list, node) { + if (!opp->available) + continue; + + if (opp->supplies[i].u_volt_min < uV[i].min) + uV[i].min = opp->supplies[i].u_volt_min; + if (opp->supplies[i].u_volt_max > uV[i].max) + uV[i].max = opp->supplies[i].u_volt_max; + } }
rcu_read_unlock(); @@ -258,9 +295,16 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev) * The caller needs to ensure that opp_table (and hence the regulator) * isn't freed, while we are executing this routine. */ - ret = regulator_set_voltage_time(reg, min_uV, max_uV); - if (ret > 0) - latency_ns = ret * 1000; + for (i = 0; reg = regulators[i], i < count; i++) { + ret = regulator_set_voltage_time(reg, uV[i].min, uV[i].max); + if (ret > 0) + latency_ns += ret * 1000; + } + +free_uV: + kfree(uV); +free_regulators: + kfree(regulators);
return latency_ns; } @@ -580,7 +624,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 +677,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(&new_supply, &opp->supply, sizeof(new_supply)); + memcpy(&old_supply, old_opp->supplies, sizeof(old_supply));
- reg = opp_table->regulator; + memcpy(&new_supply, opp->supplies, sizeof(new_supply));
rcu_read_unlock();
@@ -764,9 +817,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 +865,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 +974,50 @@ struct dev_pm_opp *_allocate_opp(struct device *dev, struct opp_table **opp_table) { struct dev_pm_opp *opp; + int count, supply_size; + struct opp_table *table;
- /* allocate new OPP node */ - opp = kzalloc(sizeof(*opp), GFP_KERNEL); - if (!opp) + table = _add_opp_table(dev); + if (!table) return NULL;
- INIT_LIST_HEAD(&opp->node); + /* Allocate space for at least one supply */ + count = table->regulator_count ? table->regulator_count : 1; + supply_size = sizeof(*opp->supplies) * count;
- *opp_table = _add_opp_table(dev); - if (!*opp_table) { - kfree(opp); + /* allocate new OPP node and supplies structures */ + opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL); + if (!opp) { + kfree(table); return NULL; }
+ /* Put the supplies at the end of the OPP structure as an empty array */ + opp->supplies = (struct dev_pm_opp_supply *)(opp + 1); + INIT_LIST_HEAD(&opp->node); + + *opp_table = table; + return opp; }
static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, struct opp_table *opp_table) { - struct regulator *reg = opp_table->regulator; - - if (!IS_ERR(reg) && - !regulator_is_supported_voltage(reg, opp->supply.u_volt_min, - opp->supply.u_volt_max)) { - pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n", - __func__, opp->supply.u_volt_min, - opp->supply.u_volt_max); - return false; + struct regulator *reg; + int i; + + for (i = 0; i < opp_table->regulator_count; i++) { + reg = opp_table->regulators[i]; + + if (!regulator_is_supported_voltage(reg, + opp->supplies[i].u_volt_min, + opp->supplies[i].u_volt_max)) { + pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n", + __func__, opp->supplies[i].u_volt_min, + opp->supplies[i].u_volt_max); + return false; + } }
return true; @@ -984,12 +1049,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 +1122,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 +1369,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 +1386,13 @@ 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. */ -struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name) +struct opp_table *dev_pm_opp_set_regulators(struct device *dev, + const char * const names[], + unsigned int count) { struct opp_table *opp_table; struct regulator *reg; - int ret; + int ret, i;
mutex_lock(&opp_table_lock);
@@ -1338,26 +1408,44 @@ struct opp_table *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) { + ret = -ENOMEM; goto err; }
- opp_table->regulator = reg; + for (i = 0; i < count; i++) { + reg = regulator_get_optional(dev, names[i]); + if (IS_ERR(reg)) { + ret = PTR_ERR(reg); + if (ret != -EPROBE_DEFER) + dev_err(dev, "%s: no regulator (%s) found: %d\n", + __func__, names[i], ret); + goto free_regulators; + } + + opp_table->regulators[i] = reg; + } + + opp_table->regulator_count = count;
mutex_unlock(&opp_table_lock); return opp_table;
+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 +1453,11 @@ struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name)
return ERR_PTR(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 - * @opp_table: OPP table returned from dev_pm_opp_set_regulator(). + * dev_pm_opp_put_regulators() - Releases resources blocked for regulator + * @opp_table: OPP table returned from dev_pm_opp_set_regulators(). * * Locking: The internal opp_table and opp structures are RCU protected. * Hence this function internally uses RCU updater strategy with mutex locks @@ -1377,20 +1465,26 @@ 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 opp_table *opp_table) +void dev_pm_opp_put_regulators(struct opp_table *opp_table) { + int i; + mutex_lock(&opp_table_lock);
- if (IS_ERR(opp_table->regulator)) { - pr_err("%s: Doesn't have regulator set\n", __func__); + if (!opp_table->regulators) { + pr_err("%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); @@ -1398,7 +1492,7 @@ void dev_pm_opp_put_regulator(struct opp_table *opp_table) unlock: mutex_unlock(&opp_table_lock); } -EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulator); +EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
/** * dev_pm_opp_add() - Add an OPP table from a table definitions diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c index c897676ca35f..95f433db4ac7 100644 --- a/drivers/base/power/opp/debugfs.c +++ b/drivers/base/power/opp/debugfs.c @@ -15,6 +15,7 @@ #include <linux/err.h> #include <linux/init.h> #include <linux/limits.h> +#include <linux/slab.h>
#include "opp.h"
@@ -34,6 +35,46 @@ void opp_debug_remove_one(struct dev_pm_opp *opp) debugfs_remove_recursive(opp->dentry); }
+static bool opp_debug_create_supplies(struct dev_pm_opp *opp, + struct opp_table *opp_table, + struct dentry *pdentry) +{ + struct dentry *d; + int i = 0; + char *name; + + /* Always create at least supply-0 directory */ + do { + name = kasprintf(GFP_KERNEL, "supply-%d", i); + + /* Create per-opp directory */ + d = debugfs_create_dir(name, pdentry); + + kfree(name); + + if (!d) + return false; + + if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, + &opp->supplies[i].u_volt)) + return false; + + if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, + &opp->supplies[i].u_volt_min)) + return false; + + if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, + &opp->supplies[i].u_volt_max)) + return false; + + if (!debugfs_create_ulong("u_amp", S_IRUGO, d, + &opp->supplies[i].u_amp)) + return false; + } while (++i < opp_table->regulator_count); + + return true; +} + int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) { struct dentry *pdentry = opp_table->dentry; @@ -63,16 +104,7 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate)) return -ENOMEM;
- if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt)) - return -ENOMEM; - - if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min)) - return -ENOMEM; - - if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max)) - return -ENOMEM; - - if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp)) + if (!opp_debug_create_supplies(opp, opp_table, d)) return -ENOMEM;
if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d, diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c index bdf409d42126..3f7d2591b173 100644 --- a/drivers/base/power/opp/of.c +++ b/drivers/base/power/opp/of.c @@ -17,6 +17,7 @@ #include <linux/errno.h> #include <linux/device.h> #include <linux/of.h> +#include <linux/slab.h> #include <linux/export.h>
#include "opp.h" @@ -101,16 +102,16 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table, return true; }
-/* TODO: Support multiple regulators */ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, struct opp_table *opp_table) { - u32 microvolt[3] = {0}; - u32 val; - int count, ret; + u32 *microvolt, *microamp = NULL; + int supplies, vcount, icount, ret, i, j; struct property *prop = NULL; char name[NAME_MAX];
+ supplies = opp_table->regulator_count ? opp_table->regulator_count : 1; + /* Search for "opp-microvolt-<name>" */ if (opp_table->prop_name) { snprintf(name, sizeof(name), "opp-microvolt-%s", @@ -128,34 +129,29 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, return 0; }
- count = of_property_count_u32_elems(opp->np, name); - if (count < 0) { + vcount = of_property_count_u32_elems(opp->np, name); + if (vcount < 0) { dev_err(dev, "%s: Invalid %s property (%d)\n", - __func__, name, count); - return count; + __func__, name, vcount); + return vcount; }
- /* There can be one or three elements here */ - if (count != 1 && count != 3) { - dev_err(dev, "%s: Invalid number of elements in %s property (%d)\n", - __func__, name, count); + /* There can be one or three elements per supply */ + if (vcount != supplies && vcount != supplies * 3) { + dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n", + __func__, name, vcount, supplies); return -EINVAL; }
- ret = of_property_read_u32_array(opp->np, name, microvolt, count); + microvolt = kmalloc_array(vcount, sizeof(*microvolt), GFP_KERNEL); + if (!microvolt) + return -ENOMEM; + + ret = of_property_read_u32_array(opp->np, name, microvolt, vcount); if (ret) { dev_err(dev, "%s: error parsing %s: %d\n", __func__, name, ret); - return -EINVAL; - } - - opp->supply.u_volt = microvolt[0]; - - if (count == 1) { - opp->supply.u_volt_min = opp->supply.u_volt; - opp->supply.u_volt_max = opp->supply.u_volt; - } else { - opp->supply.u_volt_min = microvolt[1]; - opp->supply.u_volt_max = microvolt[2]; + ret = -EINVAL; + goto free_microvolt; }
/* Search for "opp-microamp-<name>" */ @@ -172,10 +168,59 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, prop = of_find_property(opp->np, name, NULL); }
- if (prop && !of_property_read_u32(opp->np, name, &val)) - opp->supply.u_amp = val; + if (prop) { + icount = of_property_count_u32_elems(opp->np, name); + if (icount < 0) { + dev_err(dev, "%s: Invalid %s property (%d)\n", __func__, + name, icount); + ret = icount; + goto free_microvolt; + }
- return 0; + if (icount != supplies) { + dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n", + __func__, name, icount, supplies); + ret = -EINVAL; + goto free_microvolt; + } + + microamp = kmalloc_array(icount, sizeof(*microamp), GFP_KERNEL); + if (!microamp) { + ret = -EINVAL; + goto free_microvolt; + } + + ret = of_property_read_u32_array(opp->np, name, microamp, + icount); + if (ret) { + dev_err(dev, "%s: error parsing %s: %d\n", __func__, + name, ret); + ret = -EINVAL; + goto free_microamp; + } + } + + for (i = 0, j = 0; i < supplies; i++) { + opp->supplies[i].u_volt = microvolt[j++]; + + if (vcount == supplies) { + opp->supplies[i].u_volt_min = opp->supplies[i].u_volt; + opp->supplies[i].u_volt_max = opp->supplies[i].u_volt; + } else { + opp->supplies[i].u_volt_min = microvolt[j++]; + opp->supplies[i].u_volt_max = microvolt[j++]; + } + + if (microamp) + opp->supplies[i].u_amp = microamp[i]; + } + +free_microamp: + kfree(microamp); +free_microvolt: + kfree(microvolt); + + return ret; }
/** @@ -304,8 +349,8 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n", __func__, new_opp->turbo, new_opp->rate, - new_opp->supply.u_volt, new_opp->supply.u_volt_min, - new_opp->supply.u_volt_max, new_opp->clock_latency_ns); + new_opp->supplies[0].u_volt, new_opp->supplies[0].u_volt_min, + new_opp->supplies[0].u_volt_max, new_opp->clock_latency_ns);
/* * Notify the changes in the availability of the operable diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index 8a02516542c2..5b0f7e53bede 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -61,7 +61,7 @@ extern struct list_head opp_tables; * @turbo: true if turbo (boost) OPP * @suspend: true if suspend OPP * @rate: Frequency in hertz - * @supply: Power supply voltage/current values + * @supplies: Power supplies voltage/current values * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's * frequency from any other OPP's frequency. * @opp_table: points back to the opp_table struct this opp belongs to @@ -80,7 +80,7 @@ struct dev_pm_opp { bool suspend; unsigned long rate;
- struct dev_pm_opp_supply supply; + struct dev_pm_opp_supply *supplies;
unsigned long clock_latency_ns;
@@ -139,7 +139,8 @@ enum opp_table_access { * @supported_hw_count: Number of elements in supported_hw array. * @prop_name: A name to postfix to many DT properties, while parsing them. * @clk: Device's clock handle - * @regulator: Supply regulator + * @regulators: Supply regulators + * @regulator_count: Number of power supply regulators * @dentry: debugfs dentry pointer of the real device directory (not links). * @dentry_name: Name of the real dentry. * @@ -174,7 +175,8 @@ struct opp_table { unsigned int supported_hw_count; const char *prop_name; struct clk *clk; - struct regulator *regulator; + struct regulator **regulators; + unsigned int regulator_count;
#ifdef CONFIG_DEBUG_FS struct dentry *dentry; diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 4d3ec92cbabf..50669e0466c8 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -188,7 +188,10 @@ static int cpufreq_init(struct cpufreq_policy *policy) */ name = find_supply_name(cpu_dev); if (name) { - opp_table = dev_pm_opp_set_regulator(cpu_dev, name); + const char *names[] = {name}; + + opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, + ARRAY_SIZE(names)); if (IS_ERR(opp_table)) { ret = PTR_ERR(opp_table); dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n", @@ -289,7 +292,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(opp_table); + dev_pm_opp_put_regulators(opp_table); out_put_clk: clk_put(cpu_clk);
@@ -304,7 +307,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->opp_table); + dev_pm_opp_put_regulators(priv->opp_table);
clk_put(policy->clk); kfree(priv); diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 824f7268f687..9a825ae78653 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -79,8 +79,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); -struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name); -void dev_pm_opp_put_regulator(struct opp_table *opp_table); +struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count); +void dev_pm_opp_put_regulators(struct opp_table *opp_table); 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); @@ -187,12 +187,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 struct opp_table *dev_pm_opp_set_regulator(struct device *dev, const char *name) +static inline struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count) { return ERR_PTR(-ENOTSUPP); }
-static inline void dev_pm_opp_put_regulator(struct opp_table *opp_table) {} +static inline void dev_pm_opp_put_regulators(struct opp_table *opp_table) {}
static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) {
On 01-12-16, 06:21, Viresh Kumar wrote:
On 30-11-16, 14:37, Stephen Boyd wrote:
+static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
struct opp_table *opp_table,
struct dentry *pdentry)
+{
- struct dentry *d;
- int i = 0;
- char *name;
- /* Always create at least supply-0 directory */
- do {
name = kasprintf(GFP_KERNEL, "supply-%d", i);
Sorry I haven't noticed this before. Wouldn't we want to put the supply name here instead of a number?
The OPP core provides a way for platforms to use the set-OPP functionality, but that is not compulsory for its user. Which makes it very much possible that the OPP core wouldn't know the names of the supplies at all. And so I used the numbers here. Anyway, the numbers matches the order in which they are passed from DT, so that should be readable.
Here is the new rebased version of the patch. I will send the complete series again later on. Please see if it looks fine.
-------------------------8<-------------------------
From: Viresh Kumar viresh.kumar@linaro.org Date: Tue, 4 Oct 2016 17:07:28 +0530 Subject: [PATCH] PM / OPP: Add infrastructure to manage multiple regulators
This patch adds infrastructure to manage multiple regulators and updates the only user (cpufreq-dt) of dev_pm_opp_set{put}_regulator().
This is preparatory work for adding full support for devices with multiple regulators.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com
Ignore this one now. I have resent the series again after resolving all the pending issues. Please review that.
Thanks.
Later patches would add support for custom set_opp() callbacks. This patch separates out the code for _generic_set_opp() handler in order to prepare for that.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com Reviewed-by: Stephen Boyd sboyd@codeaurora.org
--- V4->V5: - Make 'dev' part of struct dev_pm_set_opp_data - Fix commit log: s/opp_set_rate/set_opp - Same in a comment as well --- drivers/base/power/opp/core.c | 181 +++++++++++++++++++++++++++++------------- drivers/base/power/opp/opp.h | 3 + include/linux/pm_opp.h | 35 ++++++++ 3 files changed, 166 insertions(+), 53 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index f0dd0bd29ce8..0a53ae132d7f 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -610,6 +610,69 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg, return ret; }
+static inline int +_generic_set_opp_clk_only(struct device *dev, struct clk *clk, + unsigned long old_freq, unsigned long freq) +{ + int ret; + + ret = clk_set_rate(clk, freq); + if (ret) { + dev_err(dev, "%s: failed to set clock rate: %d\n", __func__, + ret); + } + + return ret; +} + +static int _generic_set_opp(struct dev_pm_set_opp_data *data) +{ + struct dev_pm_opp_supply *old_supply = data->old_opp.supplies; + struct dev_pm_opp_supply *new_supply = data->new_opp.supplies; + unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate; + struct regulator *reg = data->regulators[0]; + struct device *dev= data->dev; + int ret; + + /* This function only supports single regulator per device */ + if (WARN_ON(data->regulator_count > 1)) { + dev_err(dev, "multiple regulators are not supported\n"); + return -EINVAL; + } + + /* Scaling up? Scale voltage before frequency */ + if (freq > old_freq) { + ret = _set_opp_voltage(dev, reg, new_supply); + if (ret) + goto restore_voltage; + } + + /* Change frequency */ + ret = _generic_set_opp_clk_only(dev, data->clk, old_freq, freq); + if (ret) + goto restore_voltage; + + /* Scaling down? Scale voltage after frequency */ + if (freq < old_freq) { + ret = _set_opp_voltage(dev, reg, new_supply); + if (ret) + goto restore_freq; + } + + return 0; + +restore_freq: + if (_generic_set_opp_clk_only(dev, data->clk, freq, old_freq)) + dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n", + __func__, old_freq); +restore_voltage: + /* This shouldn't harm even if the voltages weren't updated earlier */ + if (old_supply->u_volt) + _set_opp_voltage(dev, reg, old_supply); + + return ret; +} + /** * dev_pm_opp_set_rate() - Configure new OPP based on frequency * @dev: device for which we do this operation @@ -623,12 +686,12 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg, int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) { struct opp_table *opp_table; + unsigned long freq, old_freq; struct dev_pm_opp *old_opp, *opp; - struct regulator *reg = ERR_PTR(-ENXIO); + struct regulator **regulators; + struct dev_pm_set_opp_data *data; struct clk *clk; - unsigned long freq, old_freq; - struct dev_pm_opp_supply old_supply, new_supply; - int ret; + int ret, size;
if (unlikely(!target_freq)) { dev_err(dev, "%s: Invalid target frequency %lu\n", __func__, @@ -677,64 +740,36 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) return ret; }
- if (opp_table->regulators) { - /* This function only supports single regulator per device */ - if (WARN_ON(opp_table->regulator_count > 1)) { - dev_err(dev, "multiple regulators not supported\n"); - rcu_read_unlock(); - return -EINVAL; - } + dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__, + old_freq, freq);
- reg = opp_table->regulators[0]; + regulators = opp_table->regulators; + + /* Only frequency scaling */ + if (!regulators) { + rcu_read_unlock(); + return _generic_set_opp_clk_only(dev, clk, old_freq, freq); }
+ data = opp_table->set_opp_data; + data->regulators = regulators; + data->regulator_count = opp_table->regulator_count; + data->clk = clk; + data->dev = dev; + + data->old_opp.rate = old_freq; + size = sizeof(*opp->supplies) * opp_table->regulator_count; if (IS_ERR(old_opp)) - old_supply.u_volt = 0; + memset(data->old_opp.supplies, 0, size); else - memcpy(&old_supply, old_opp->supplies, sizeof(old_supply)); + memcpy(data->old_opp.supplies, old_opp->supplies, size);
- memcpy(&new_supply, opp->supplies, sizeof(new_supply)); + data->new_opp.rate = freq; + memcpy(data->new_opp.supplies, opp->supplies, size);
rcu_read_unlock();
- /* Scaling up? Scale voltage before frequency */ - if (freq > old_freq) { - ret = _set_opp_voltage(dev, reg, &new_supply); - if (ret) - goto restore_voltage; - } - - /* Change frequency */ - - dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", - __func__, old_freq, freq); - - ret = clk_set_rate(clk, freq); - if (ret) { - dev_err(dev, "%s: failed to set clock rate: %d\n", __func__, - ret); - goto restore_voltage; - } - - /* Scaling down? Scale voltage after frequency */ - if (freq < old_freq) { - ret = _set_opp_voltage(dev, reg, &new_supply); - if (ret) - goto restore_freq; - } - - return 0; - -restore_freq: - if (clk_set_rate(clk, old_freq)) - dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n", - __func__, old_freq); -restore_voltage: - /* This shouldn't harm even if the voltages weren't updated earlier */ - if (old_supply.u_volt) - _set_opp_voltage(dev, reg, &old_supply); - - return ret; + return _generic_set_opp(data); } EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
@@ -1368,6 +1403,38 @@ void dev_pm_opp_put_prop_name(struct device *dev) } EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
+static int _allocate_set_opp_data(struct opp_table *opp_table) +{ + struct dev_pm_set_opp_data *data; + int len, count = opp_table->regulator_count; + + if (WARN_ON(!count)) + return -EINVAL; + + /* space for set_opp_data */ + len = sizeof(*data); + + /* space for old_opp.supplies and new_opp.supplies */ + len += 2 * sizeof(struct dev_pm_opp_supply) * count; + + data = kzalloc(len, GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->old_opp.supplies = (void *)(data + 1); + data->new_opp.supplies = data->old_opp.supplies + count; + + opp_table->set_opp_data = data; + + return 0; +} + +static void _free_set_opp_data(struct opp_table *opp_table) +{ + kfree(opp_table->set_opp_data); + opp_table->set_opp_data = NULL; +} + /** * dev_pm_opp_set_regulators() - Set regulator names for the device * @dev: Device for which regulator name is being set. @@ -1437,6 +1504,11 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
opp_table->regulator_count = count;
+ /* Allocate block only once to pass to set_opp() routines */ + ret = _allocate_set_opp_data(opp_table); + if (ret) + goto free_regulators; + mutex_unlock(&opp_table_lock); return opp_table;
@@ -1446,6 +1518,7 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
kfree(opp_table->regulators); opp_table->regulators = NULL; + opp_table->regulator_count = 0; err: _remove_opp_table(opp_table); unlock: @@ -1482,6 +1555,8 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table) for (i = opp_table->regulator_count - 1; i >= 0; i--) regulator_put(opp_table->regulators[i]);
+ _free_set_opp_data(opp_table); + kfree(opp_table->regulators); opp_table->regulators = NULL; opp_table->regulator_count = 0; diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index 5b0f7e53bede..a05e43912c6b 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -141,6 +141,7 @@ enum opp_table_access { * @clk: Device's clock handle * @regulators: Supply regulators * @regulator_count: Number of power supply regulators + * @set_opp_data: Data to be passed to set_opp callback * @dentry: debugfs dentry pointer of the real device directory (not links). * @dentry_name: Name of the real dentry. * @@ -178,6 +179,8 @@ struct opp_table { struct regulator **regulators; unsigned int regulator_count;
+ struct dev_pm_set_opp_data *set_opp_data; + #ifdef CONFIG_DEBUG_FS struct dentry *dentry; char dentry_name[NAME_MAX]; diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 9a825ae78653..779b40a9287d 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -17,6 +17,8 @@ #include <linux/err.h> #include <linux/notifier.h>
+struct clk; +struct regulator; struct dev_pm_opp; struct device; struct opp_table; @@ -41,6 +43,39 @@ struct dev_pm_opp_supply { unsigned long u_amp; };
+/** + * struct dev_pm_opp_info - OPP freq/voltage/current values + * @rate: Target clk rate in hz + * @supplies: Array of voltage/current values for all power supplies + * + * This structure stores the freq/voltage/current values for a single OPP. + */ +struct dev_pm_opp_info { + unsigned long rate; + struct dev_pm_opp_supply *supplies; +}; + +/** + * struct dev_pm_set_opp_data - Set OPP data + * @old_opp: Old OPP info + * @new_opp: New OPP info + * @regulators: Array of regulator pointers + * @regulator_count: Number of regulators + * @clk: Pointer to clk + * @dev: Pointer to the struct device + * + * This structure contains all information required for setting an OPP. + */ +struct dev_pm_set_opp_data { + struct dev_pm_opp_info old_opp; + struct dev_pm_opp_info new_opp; + + struct regulator **regulators; + unsigned int regulator_count; + struct clk *clk; + struct device *dev; +}; + #if defined(CONFIG_PM_OPP)
unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
The generic set_opp() handler isn't sufficient for platforms with complex DVFS. For example, some TI platforms have multiple regulators for a CPU device. The order in which various supplies need to be programmed is only known to the platform code and its best to leave it to it.
This patch implements APIs to register platform specific set_opp() callback.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com
--- V4->V5: - s/custom OPP set rate/custom set OPP/ - set_opp() doesn't have a separate 'dev' argument now. --- drivers/base/power/opp/core.c | 114 +++++++++++++++++++++++++++++++++++++++++- drivers/base/power/opp/opp.h | 2 + include/linux/pm_opp.h | 10 ++++ 3 files changed, 125 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 0a53ae132d7f..1fb188b424c7 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -687,6 +687,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) { struct opp_table *opp_table; unsigned long freq, old_freq; + int (*set_opp)(struct dev_pm_set_opp_data *data); struct dev_pm_opp *old_opp, *opp; struct regulator **regulators; struct dev_pm_set_opp_data *data; @@ -751,6 +752,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq) return _generic_set_opp_clk_only(dev, clk, old_freq, freq); }
+ if (opp_table->set_opp) + set_opp = opp_table->set_opp; + else + set_opp = _generic_set_opp; + data = opp_table->set_opp_data; data->regulators = regulators; data->regulator_count = opp_table->regulator_count; @@ -769,7 +775,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
rcu_read_unlock();
- return _generic_set_opp(data); + return set_opp(data); } EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
@@ -903,6 +909,9 @@ static void _remove_opp_table(struct opp_table *opp_table) if (opp_table->regulators) return;
+ if (opp_table->set_opp) + return; + /* Release clk */ if (!IS_ERR(opp_table->clk)) clk_put(opp_table->clk); @@ -1570,6 +1579,109 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table) EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
/** + * dev_pm_opp_register_set_opp_helper() - Register custom set OPP helper + * @dev: Device for which the helper is getting registered. + * @set_opp: Custom set OPP helper. + * + * This is useful to support complex platforms (like platforms with multiple + * regulators per device), instead of the generic OPP set rate helper. + * + * This must be called before any OPPs are initialized for the device. + * + * Locking: The internal opp_table and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + */ +int dev_pm_opp_register_set_opp_helper(struct device *dev, + int (*set_opp)(struct dev_pm_set_opp_data *data)) +{ + struct opp_table *opp_table; + int ret; + + if (!set_opp) + return -EINVAL; + + mutex_lock(&opp_table_lock); + + opp_table = _add_opp_table(dev); + if (!opp_table) { + ret = -ENOMEM; + goto unlock; + } + + /* This should be called before OPPs are initialized */ + if (WARN_ON(!list_empty(&opp_table->opp_list))) { + ret = -EBUSY; + goto err; + } + + /* Already have custom set_opp helper */ + if (WARN_ON(opp_table->set_opp)) { + ret = -EBUSY; + goto err; + } + + opp_table->set_opp = set_opp; + + mutex_unlock(&opp_table_lock); + return 0; + +err: + _remove_opp_table(opp_table); +unlock: + mutex_unlock(&opp_table_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(dev_pm_opp_register_set_opp_helper); + +/** + * dev_pm_opp_register_put_opp_helper() - Releases resources blocked for + * set_opp helper + * @dev: Device for which custom set_opp helper has to be cleared. + * + * Locking: The internal opp_table and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + */ +void dev_pm_opp_register_put_opp_helper(struct device *dev) +{ + struct opp_table *opp_table; + + mutex_lock(&opp_table_lock); + + /* Check for existing table for 'dev' first */ + opp_table = _find_opp_table(dev); + if (IS_ERR(opp_table)) { + dev_err(dev, "Failed to find opp_table: %ld\n", + PTR_ERR(opp_table)); + goto unlock; + } + + if (!opp_table->set_opp) { + dev_err(dev, "%s: Doesn't have custom set_opp helper set\n", + __func__); + goto unlock; + } + + /* Make sure there are no concurrent readers while updating opp_table */ + WARN_ON(!list_empty(&opp_table->opp_list)); + + opp_table->set_opp = NULL; + + /* Try freeing opp_table if this was the last blocking resource */ + _remove_opp_table(opp_table); + +unlock: + mutex_unlock(&opp_table_lock); +} +EXPORT_SYMBOL_GPL(dev_pm_opp_register_put_opp_helper); + +/** * dev_pm_opp_add() - Add an OPP table from a table definitions * @dev: device for which we do this operation * @freq: Frequency in Hz for this OPP diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h index a05e43912c6b..af9f2b849a66 100644 --- a/drivers/base/power/opp/opp.h +++ b/drivers/base/power/opp/opp.h @@ -141,6 +141,7 @@ enum opp_table_access { * @clk: Device's clock handle * @regulators: Supply regulators * @regulator_count: Number of power supply regulators + * @set_opp: Platform specific set_opp callback * @set_opp_data: Data to be passed to set_opp callback * @dentry: debugfs dentry pointer of the real device directory (not links). * @dentry_name: Name of the real dentry. @@ -179,6 +180,7 @@ struct opp_table { struct regulator **regulators; unsigned int regulator_count;
+ int (*set_opp)(struct dev_pm_set_opp_data *data); struct dev_pm_set_opp_data *set_opp_data;
#ifdef CONFIG_DEBUG_FS diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 779b40a9287d..0edd88f93904 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -116,6 +116,8 @@ int dev_pm_opp_set_prop_name(struct device *dev, const char *name); void dev_pm_opp_put_prop_name(struct device *dev); struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count); void dev_pm_opp_put_regulators(struct opp_table *opp_table); +int dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data)); +void dev_pm_opp_register_put_opp_helper(struct device *dev); int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq); int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask); int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask); @@ -215,6 +217,14 @@ static inline int dev_pm_opp_set_supported_hw(struct device *dev,
static inline void dev_pm_opp_put_supported_hw(struct device *dev) {}
+static inline int dev_pm_opp_register_set_opp_helper(struct device *dev, + int (*set_opp)(struct dev_pm_set_opp_data *data)) +{ + return -ENOTSUPP; +} + +static inline void dev_pm_opp_register_put_opp_helper(struct device *dev) {} + static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name) { return -ENOTSUPP;
On 11/30, Viresh Kumar wrote:
The generic set_opp() handler isn't sufficient for platforms with complex DVFS. For example, some TI platforms have multiple regulators for a CPU device. The order in which various supplies need to be programmed is only known to the platform code and its best to leave it to it.
This patch implements APIs to register platform specific set_opp() callback.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
So this one has the same set/put problem the other APIs has? Presumably we're going to need to fix and change the API that is introduced here. Wouldn't it be better to do that first though?
On 30-11-16, 14:04, Stephen Boyd wrote:
On 11/30, Viresh Kumar wrote:
The generic set_opp() handler isn't sufficient for platforms with complex DVFS. For example, some TI platforms have multiple regulators for a CPU device. The order in which various supplies need to be programmed is only known to the platform code and its best to leave it to it.
This patch implements APIs to register platform specific set_opp() callback.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Tested-by: Dave Gerlach d-gerlach@ti.com
Reviewed-by: Stephen Boyd sboyd@codeaurora.org
Thanks.
So this one has the same set/put problem the other APIs has?
Yes.
Presumably we're going to need to fix and change the API that is introduced here. Wouldn't it be better to do that first though?
Returning opp_table is one of the solutions and I am not sure if it is the best one. I am working on the cleanup series and will modify all the routine the same way very soon.
If a platform specific OPP driver has called this routine first and set the regulators, then the second call from cpufreq-dt driver will hit the WARN_ON(). Remove the WARN_ON(), but continue to return error in such cases.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org Tested-by: Dave Gerlach d-gerlach@ti.com --- drivers/base/power/opp/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 1fb188b424c7..f60d7f943b64 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -1485,7 +1485,7 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, }
/* Already have regulators set */ - if (WARN_ON(opp_table->regulators)) { + if (opp_table->regulators) { opp_table = ERR_PTR(-EBUSY); goto err; }
If the regulators aren't set explicitly by the platform, the OPP core assumes that the platform doesn't have any regulator and uses the clk-only callback.
If the platform failed to register a regulator with the core, then this can turn out to be a dangerous assumption as the OPP core will try to change clk without changing regulators.
Handle that properly by making sure that the DT didn't have any entries for supply voltages as well.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Reviewed-by: Stephen Boyd sboyd@codeaurora.org --- drivers/base/power/opp/core.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index f60d7f943b64..979403448cd6 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -748,7 +748,20 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
/* Only frequency scaling */ if (!regulators) { + unsigned long u_volt = opp->supplies[0].u_volt; + rcu_read_unlock(); + + /* + * DT contained supply ratings? Consider platform failed to set + * regulators. + */ + if (unlikely(u_volt)) { + dev_err(dev, "%s: Regulator not registered with OPP core\n", + __func__); + return -EINVAL; + } + return _generic_set_opp_clk_only(dev, clk, old_freq, freq); }
linaro-kernel@lists.linaro.org