Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1504636 > unrolled thread
| Started by | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| First post | 2016-10-20 10:50 +0200 |
| Last post | 2016-10-26 08:10 +0200 |
| Articles | 5 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-20 10:50 +0200
Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators Dave Gerlach <d-gerlach@ti.com> - 2016-10-22 00:40 +0200
Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-24 05:40 +0200
Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators Stephen Boyd <sboyd@codeaurora.org> - 2016-10-25 19:00 +0200
Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators Viresh Kumar <viresh.kumar@linaro.org> - 2016-10-26 08:10 +0200
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-10-20 10:50 +0200 |
| Subject | [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators |
| Message-ID | <suhcZ-5ak-1@gated-at.bofh.it> |
This patch adds infrastructure to manage multiple regulators and updates
the only user (cpufreq-dt) of dev_pm_opp_set{put}_regulator().
This is preparatory work for adding full support for devices with
multiple regulators.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/base/power/opp/core.c | 220 ++++++++++++++++++++++++++-------------
drivers/base/power/opp/debugfs.c | 48 +++++++--
drivers/base/power/opp/of.c | 102 +++++++++++++-----
drivers/base/power/opp/opp.h | 10 +-
drivers/cpufreq/cpufreq-dt.c | 9 +-
include/linux/pm_opp.h | 8 +-
6 files changed, 276 insertions(+), 121 deletions(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 37fad2eb0f47..45c70ce07864 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -93,6 +93,8 @@ struct opp_table *_find_opp_table(struct device *dev)
* Return: voltage in micro volt corresponding to the opp, else
* return 0
*
+ * This is useful only for devices with single power supply.
+ *
* Locking: This function must be called under rcu_read_lock(). opp is a rcu
* protected pointer. This means that opp which could have been fetched by
* opp_find_freq_{exact,ceil,floor} functions is valid as long as we are
@@ -112,7 +114,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
if (IS_ERR_OR_NULL(tmp_opp))
pr_err("%s: Invalid parameters\n", __func__);
else
- v = tmp_opp->supply.u_volt;
+ v = tmp_opp->supplies[0].u_volt;
return v;
}
@@ -222,10 +224,10 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
{
struct opp_table *opp_table;
struct dev_pm_opp *opp;
- struct regulator *reg;
+ struct regulator *reg, **regulators;
unsigned long latency_ns = 0;
- unsigned long min_uV = ~0, max_uV = 0;
- int ret;
+ unsigned long *min_uV, *max_uV;
+ int ret, size, i, count;
rcu_read_lock();
@@ -235,21 +237,44 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
return 0;
}
- reg = opp_table->regulator;
- if (IS_ERR(reg)) {
+ count = opp_table->regulator_count;
+
+ if (!count) {
/* Regulator may not be required for device */
rcu_read_unlock();
return 0;
}
- list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
- if (!opp->available)
- continue;
+ size = count * sizeof(*regulators);
+ regulators = kmemdup(opp_table->regulators, size, GFP_KERNEL);
+ if (!regulators) {
+ rcu_read_unlock();
+ return 0;
+ }
+
+ min_uV = kmalloc(count * (sizeof(*min_uV) + sizeof(*max_uV)),
+ GFP_KERNEL);
+ if (!min_uV) {
+ kfree(regulators);
+ rcu_read_unlock();
+ return 0;
+ }
+
+ max_uV = min_uV + count;
+
+ for (i = 0; i < count; i++) {
+ min_uV[i] = ~0;
+ max_uV[i] = 0;
+
+ list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
+ if (!opp->available)
+ continue;
- if (opp->supply.u_volt_min < min_uV)
- min_uV = opp->supply.u_volt_min;
- if (opp->supply.u_volt_max > max_uV)
- max_uV = opp->supply.u_volt_max;
+ if (opp->supplies[i].u_volt_min < min_uV[i])
+ min_uV[i] = opp->supplies[i].u_volt_min;
+ if (opp->supplies[i].u_volt_max > max_uV[i])
+ max_uV[i] = opp->supplies[i].u_volt_max;
+ }
}
rcu_read_unlock();
@@ -258,9 +283,14 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
* The caller needs to ensure that opp_table (and hence the regulator)
* isn't freed, while we are executing this routine.
*/
- ret = regulator_set_voltage_time(reg, min_uV, max_uV);
- if (ret > 0)
- latency_ns = ret * 1000;
+ for (i = 0; reg = regulators[i], i < count; i++) {
+ ret = regulator_set_voltage_time(reg, min_uV[i], max_uV[i]);
+ if (ret > 0)
+ latency_ns += ret * 1000;
+ }
+
+ kfree(min_uV);
+ kfree(regulators);
return latency_ns;
}
@@ -580,7 +610,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
{
struct opp_table *opp_table;
struct dev_pm_opp *old_opp, *opp;
- struct regulator *reg;
+ struct regulator *reg = ERR_PTR(-ENXIO);
struct clk *clk;
unsigned long freq, old_freq;
struct dev_pm_opp_supply old_supply, new_supply;
@@ -633,14 +663,23 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
return ret;
}
+ if (opp_table->regulators) {
+ /* This function only supports single regulator per device */
+ if (WARN_ON(opp_table->regulator_count > 1)) {
+ dev_err(dev, "multiple regulators not supported\n");
+ rcu_read_unlock();
+ return -EINVAL;
+ }
+
+ reg = opp_table->regulators[0];
+ }
+
if (IS_ERR(old_opp))
old_supply.u_volt = 0;
else
- memcpy(&old_supply, &old_opp->supply, sizeof(old_supply));
+ memcpy(&old_supply, old_opp->supplies, sizeof(old_supply));
- memcpy(&new_supply, &opp->supply, sizeof(new_supply));
-
- reg = opp_table->regulator;
+ memcpy(&new_supply, opp->supplies, sizeof(new_supply));
rcu_read_unlock();
@@ -764,9 +803,6 @@ static struct opp_table *_add_opp_table(struct device *dev)
_of_init_opp_table(opp_table, dev);
- /* Set regulator to a non-NULL error value */
- opp_table->regulator = ERR_PTR(-ENXIO);
-
/* Find clk for the device */
opp_table->clk = clk_get(dev, NULL);
if (IS_ERR(opp_table->clk)) {
@@ -815,7 +851,7 @@ static void _remove_opp_table(struct opp_table *opp_table)
if (opp_table->prop_name)
return;
- if (!IS_ERR(opp_table->regulator))
+ if (opp_table->regulators)
return;
/* Release clk */
@@ -924,35 +960,49 @@ struct dev_pm_opp *_allocate_opp(struct device *dev,
struct opp_table **opp_table)
{
struct dev_pm_opp *opp;
+ int count, supply_size;
+ struct opp_table *table;
- /* allocate new OPP node */
- opp = kzalloc(sizeof(*opp), GFP_KERNEL);
- if (!opp)
+ table = _add_opp_table(dev);
+ if (!table)
return NULL;
- INIT_LIST_HEAD(&opp->node);
+ /* Allocate space for at least one supply */
+ count = table->regulator_count ? table->regulator_count : 1;
+ supply_size = sizeof(*opp->supplies) * count;
- *opp_table = _add_opp_table(dev);
- if (!*opp_table) {
- kfree(opp);
+ /* allocate new OPP node + and supplies structures */
+ opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL);
+ if (!opp) {
+ kfree(table);
return NULL;
}
+ opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
+ INIT_LIST_HEAD(&opp->node);
+
+ *opp_table = table;
+
return opp;
}
static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
struct opp_table *opp_table)
{
- struct regulator *reg = opp_table->regulator;
-
- if (!IS_ERR(reg) &&
- !regulator_is_supported_voltage(reg, opp->supply.u_volt_min,
- opp->supply.u_volt_max)) {
- pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
- __func__, opp->supply.u_volt_min,
- opp->supply.u_volt_max);
- return false;
+ struct regulator *reg;
+ int i;
+
+ for (i = 0; i < opp_table->regulator_count; i++) {
+ reg = opp_table->regulators[i];
+
+ if (!regulator_is_supported_voltage(reg,
+ opp->supplies[i].u_volt_min,
+ opp->supplies[i].u_volt_max)) {
+ pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
+ __func__, opp->supplies[i].u_volt_min,
+ opp->supplies[i].u_volt_max);
+ return false;
+ }
}
return true;
@@ -984,12 +1034,13 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
/* Duplicate OPPs */
dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
- __func__, opp->rate, opp->supply.u_volt,
- opp->available, new_opp->rate, new_opp->supply.u_volt,
- new_opp->available);
+ __func__, opp->rate, opp->supplies[0].u_volt,
+ opp->available, new_opp->rate,
+ new_opp->supplies[0].u_volt, new_opp->available);
+ /* Should we compare voltages for all regulators here ? */
return opp->available &&
- new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST;
+ new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? 0 : -EEXIST;
}
new_opp->opp_table = opp_table;
@@ -1056,9 +1107,9 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
/* populate the opp table */
new_opp->rate = freq;
tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
- new_opp->supply.u_volt = u_volt;
- new_opp->supply.u_volt_min = u_volt - tol;
- new_opp->supply.u_volt_max = u_volt + tol;
+ new_opp->supplies[0].u_volt = u_volt;
+ new_opp->supplies[0].u_volt_min = u_volt - tol;
+ new_opp->supplies[0].u_volt_max = u_volt + tol;
new_opp->available = true;
new_opp->dynamic = dynamic;
@@ -1303,12 +1354,14 @@ void dev_pm_opp_put_prop_name(struct device *dev)
EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
/**
- * dev_pm_opp_set_regulator() - Set regulator name for the device
+ * dev_pm_opp_set_regulators() - Set regulator names for the device
* @dev: Device for which regulator name is being set.
- * @name: Name of the regulator.
+ * @names: Array of pointers to the names of the regulator.
+ * @count: Number of regulators.
*
* In order to support OPP switching, OPP layer needs to know the name of the
- * device's regulator, as the core would be required to switch voltages as well.
+ * device's regulators, as the core would be required to switch voltages as
+ * well.
*
* This must be called before any OPPs are initialized for the device.
*
@@ -1318,11 +1371,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
* that this function is *NOT* called under RCU protection or in contexts where
* mutex cannot be locked.
*/
-int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+int dev_pm_opp_set_regulators(struct device *dev, const char *names[],
+ unsigned int count)
{
struct opp_table *opp_table;
struct regulator *reg;
- int ret;
+ int ret, i;
mutex_lock(&opp_table_lock);
@@ -1338,26 +1392,43 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
goto err;
}
- /* Already have a regulator set */
- if (WARN_ON(!IS_ERR(opp_table->regulator))) {
+ /* Already have regulators set */
+ if (WARN_ON(opp_table->regulators)) {
ret = -EBUSY;
goto err;
}
- /* Allocate the regulator */
- reg = regulator_get_optional(dev, name);
- if (IS_ERR(reg)) {
- ret = PTR_ERR(reg);
- if (ret != -EPROBE_DEFER)
- dev_err(dev, "%s: no regulator (%s) found: %d\n",
- __func__, name, ret);
+
+ opp_table->regulators = kmalloc_array(count,
+ sizeof(*opp_table->regulators),
+ GFP_KERNEL);
+ if (!opp_table->regulators)
goto err;
+
+ for (i = 0; i < count; i++) {
+ reg = regulator_get_optional(dev, names[i]);
+ pr_info("%s: %d: %p: %s\n", __func__, __LINE__, reg, names[i]);
+ if (IS_ERR(reg)) {
+ ret = PTR_ERR(reg);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "%s: regulator (%s) not found: %d\n",
+ __func__, names[i], ret);
+ goto free_regulators;
+ }
+
+ opp_table->regulators[i] = reg;
}
- opp_table->regulator = reg;
+ opp_table->regulator_count = count;
mutex_unlock(&opp_table_lock);
return 0;
+free_regulators:
+ while (i != 0)
+ regulator_put(opp_table->regulators[--i]);
+
+ kfree(opp_table->regulators);
+ opp_table->regulators = NULL;
err:
_remove_opp_table(opp_table);
unlock:
@@ -1365,11 +1436,11 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
return ret;
}
-EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulators);
/**
- * dev_pm_opp_put_regulator() - Releases resources blocked for regulator
- * @dev: Device for which regulator was set.
+ * dev_pm_opp_put_regulators() - Releases resources blocked for regulators
+ * @dev: Device for which regulators were set.
*
* Locking: The internal opp_table and opp structures are RCU protected.
* Hence this function internally uses RCU updater strategy with mutex locks
@@ -1377,9 +1448,10 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
* that this function is *NOT* called under RCU protection or in contexts where
* mutex cannot be locked.
*/
-void dev_pm_opp_put_regulator(struct device *dev)
+void dev_pm_opp_put_regulators(struct device *dev)
{
struct opp_table *opp_table;
+ int i;
mutex_lock(&opp_table_lock);
@@ -1391,16 +1463,20 @@ void dev_pm_opp_put_regulator(struct device *dev)
goto unlock;
}
- if (IS_ERR(opp_table->regulator)) {
- dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
+ if (!opp_table->regulators) {
+ dev_err(dev, "%s: Doesn't have regulators set\n", __func__);
goto unlock;
}
/* Make sure there are no concurrent readers while updating opp_table */
WARN_ON(!list_empty(&opp_table->opp_list));
- regulator_put(opp_table->regulator);
- opp_table->regulator = ERR_PTR(-ENXIO);
+ for (i = opp_table->regulator_count - 1; i >= 0; i--)
+ regulator_put(opp_table->regulators[i]);
+
+ kfree(opp_table->regulators);
+ opp_table->regulators = NULL;
+ opp_table->regulator_count = 0;
/* Try freeing opp_table if this was the last blocking resource */
_remove_opp_table(opp_table);
@@ -1408,7 +1484,7 @@ void dev_pm_opp_put_regulator(struct device *dev)
unlock:
mutex_unlock(&opp_table_lock);
}
-EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulator);
+EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
/**
* dev_pm_opp_add() - Add an OPP table from a table definitions
diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
index c897676ca35f..cb5e5fde3d39 100644
--- a/drivers/base/power/opp/debugfs.c
+++ b/drivers/base/power/opp/debugfs.c
@@ -34,6 +34,43 @@ void opp_debug_remove_one(struct dev_pm_opp *opp)
debugfs_remove_recursive(opp->dentry);
}
+static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
+ struct opp_table *opp_table,
+ struct dentry *pdentry)
+{
+ struct dentry *d;
+ int i = 0;
+ char name[] = "supply-X"; /* support only 0-9 supplies */
+
+ /* Always create at least supply-0 directory */
+ do {
+ name[7] = i + '0';
+
+ /* Create per-opp directory */
+ d = debugfs_create_dir(name, pdentry);
+ if (!d)
+ return false;
+
+ if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d,
+ &opp->supplies[i].u_volt))
+ return false;
+
+ if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d,
+ &opp->supplies[i].u_volt_min))
+ return false;
+
+ if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d,
+ &opp->supplies[i].u_volt_max))
+ return false;
+
+ if (!debugfs_create_ulong("u_amp", S_IRUGO, d,
+ &opp->supplies[i].u_amp))
+ return false;
+ } while (++i < opp_table->regulator_count);
+
+ return true;
+}
+
int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
{
struct dentry *pdentry = opp_table->dentry;
@@ -63,16 +100,7 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate))
return -ENOMEM;
- if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt))
- return -ENOMEM;
-
- if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min))
- return -ENOMEM;
-
- if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max))
- return -ENOMEM;
-
- if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp))
+ if (!opp_debug_create_supplies(opp, opp_table, d))
return -ENOMEM;
if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d,
diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index b7fcd0a1b58b..c857fb07a5bc 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -17,6 +17,7 @@
#include <linux/errno.h>
#include <linux/device.h>
#include <linux/of.h>
+#include <linux/slab.h>
#include <linux/export.h>
#include "opp.h"
@@ -105,12 +106,13 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
struct opp_table *opp_table)
{
- u32 microvolt[3] = {0};
- u32 val;
- int count, ret;
+ u32 *microvolt, *microamp = NULL;
+ int supplies, vcount, icount, ret, i, j;
struct property *prop = NULL;
char name[NAME_MAX];
+ supplies = opp_table->regulator_count ? opp_table->regulator_count : 1;
+
/* Search for "opp-microvolt-<name>" */
if (opp_table->prop_name) {
snprintf(name, sizeof(name), "opp-microvolt-%s",
@@ -128,34 +130,29 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
return 0;
}
- count = of_property_count_u32_elems(opp->np, name);
- if (count < 0) {
+ vcount = of_property_count_u32_elems(opp->np, name);
+ if (vcount < 0) {
dev_err(dev, "%s: Invalid %s property (%d)\n",
- __func__, name, count);
- return count;
+ __func__, name, vcount);
+ return vcount;
}
- /* There can be one or three elements here */
- if (count != 1 && count != 3) {
- dev_err(dev, "%s: Invalid number of elements in %s property (%d)\n",
- __func__, name, count);
+ /* There can be one or three elements per supply */
+ if (vcount != supplies && vcount != supplies * 3) {
+ dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n",
+ __func__, name, vcount, supplies);
return -EINVAL;
}
- ret = of_property_read_u32_array(opp->np, name, microvolt, count);
+ microvolt = kmalloc_array(vcount, sizeof(*microvolt), GFP_KERNEL);
+ if (!microvolt)
+ return -ENOMEM;
+
+ ret = of_property_read_u32_array(opp->np, name, microvolt, vcount);
if (ret) {
dev_err(dev, "%s: error parsing %s: %d\n", __func__, name, ret);
- return -EINVAL;
- }
-
- opp->supply.u_volt = microvolt[0];
-
- if (count == 1) {
- opp->supply.u_volt_min = opp->supply.u_volt;
- opp->supply.u_volt_max = opp->supply.u_volt;
- } else {
- opp->supply.u_volt_min = microvolt[1];
- opp->supply.u_volt_max = microvolt[2];
+ ret = -EINVAL;
+ goto free_microvolt;
}
/* Search for "opp-microamp-<name>" */
@@ -172,10 +169,59 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
prop = of_find_property(opp->np, name, NULL);
}
- if (prop && !of_property_read_u32(opp->np, name, &val))
- opp->supply.u_amp = val;
+ if (prop) {
+ icount = of_property_count_u32_elems(opp->np, name);
+ if (icount < 0) {
+ dev_err(dev, "%s: Invalid %s property (%d)\n", __func__,
+ name, icount);
+ ret = icount;
+ goto free_microvolt;
+ }
- return 0;
+ if (icount != supplies) {
+ dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n",
+ __func__, name, icount, supplies);
+ ret = -EINVAL;
+ goto free_microvolt;
+ }
+
+ microamp = kmalloc_array(icount, sizeof(*microamp), GFP_KERNEL);
+ if (!microamp) {
+ ret = -EINVAL;
+ goto free_microvolt;
+ }
+
+ ret = of_property_read_u32_array(opp->np, name, microamp,
+ icount);
+ if (ret) {
+ dev_err(dev, "%s: error parsing %s: %d\n", __func__,
+ name, ret);
+ ret = -EINVAL;
+ goto free_microamp;
+ }
+ }
+
+ for (i = 0, j = 0; i < supplies; i++) {
+ opp->supplies[i].u_volt = microvolt[j++];
+
+ if (vcount == supplies) {
+ opp->supplies[i].u_volt_min = opp->supplies[i].u_volt;
+ opp->supplies[i].u_volt_max = opp->supplies[i].u_volt;
+ } else {
+ opp->supplies[i].u_volt_min = microvolt[j++];
+ opp->supplies[i].u_volt_max = microvolt[j++];
+ }
+
+ if (microamp)
+ opp->supplies[i].u_amp = microamp[i];
+ }
+
+free_microamp:
+ kfree(microamp);
+free_microvolt:
+ kfree(microvolt);
+
+ return ret;
}
/**
@@ -304,8 +350,8 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n",
__func__, new_opp->turbo, new_opp->rate,
- new_opp->supply.u_volt, new_opp->supply.u_volt_min,
- new_opp->supply.u_volt_max, new_opp->clock_latency_ns);
+ new_opp->supplies[0].u_volt, new_opp->supplies[0].u_volt_min,
+ new_opp->supplies[0].u_volt_max, new_opp->clock_latency_ns);
/*
* Notify the changes in the availability of the operable
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 1bda0d35c486..d3f0861f9bff 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -77,7 +77,7 @@ struct dev_pm_opp_supply {
* @turbo: true if turbo (boost) OPP
* @suspend: true if suspend OPP
* @rate: Frequency in hertz
- * @supply: Power supply voltage/current values
+ * @supplies: Power supplies voltage/current values
* @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
* frequency from any other OPP's frequency.
* @opp_table: points back to the opp_table struct this opp belongs to
@@ -96,7 +96,7 @@ struct dev_pm_opp {
bool suspend;
unsigned long rate;
- struct dev_pm_opp_supply supply;
+ struct dev_pm_opp_supply *supplies;
unsigned long clock_latency_ns;
@@ -155,7 +155,8 @@ enum opp_table_access {
* @supported_hw_count: Number of elements in supported_hw array.
* @prop_name: A name to postfix to many DT properties, while parsing them.
* @clk: Device's clock handle
- * @regulator: Supply regulator
+ * @regulators: Supply regulators
+ * @regulator_count: Number of Power Supply regulators
* @dentry: debugfs dentry pointer of the real device directory (not links).
* @dentry_name: Name of the real dentry.
*
@@ -190,7 +191,8 @@ struct opp_table {
unsigned int supported_hw_count;
const char *prop_name;
struct clk *clk;
- struct regulator *regulator;
+ struct regulator **regulators;
+ unsigned int regulator_count;
#ifdef CONFIG_DEBUG_FS
struct dentry *dentry;
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 5c07ae05d69a..15cb26118dc7 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -186,7 +186,10 @@ static int cpufreq_init(struct cpufreq_policy *policy)
*/
name = find_supply_name(cpu_dev);
if (name) {
- ret = dev_pm_opp_set_regulator(cpu_dev, name);
+ const char *names[] = {name};
+
+ ret = dev_pm_opp_set_regulators(cpu_dev, names,
+ ARRAY_SIZE(names));
if (ret) {
dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
policy->cpu, ret);
@@ -285,7 +288,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
out_free_opp:
dev_pm_opp_of_cpumask_remove_table(policy->cpus);
if (name)
- dev_pm_opp_put_regulator(cpu_dev);
+ dev_pm_opp_put_regulators(cpu_dev);
out_put_clk:
clk_put(cpu_clk);
@@ -300,7 +303,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
if (priv->reg_name)
- dev_pm_opp_put_regulator(priv->cpu_dev);
+ dev_pm_opp_put_regulators(priv->cpu_dev);
clk_put(policy->clk);
kfree(priv);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index bca26157f5b6..0606b70a8b97 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -62,8 +62,8 @@ int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
void dev_pm_opp_put_supported_hw(struct device *dev);
int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
void dev_pm_opp_put_prop_name(struct device *dev);
-int dev_pm_opp_set_regulator(struct device *dev, const char *name);
-void dev_pm_opp_put_regulator(struct device *dev);
+int dev_pm_opp_set_regulators(struct device *dev, const char *names[], unsigned int count);
+void dev_pm_opp_put_regulators(struct device *dev);
int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -170,12 +170,12 @@ static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
static inline void dev_pm_opp_put_prop_name(struct device *dev) {}
-static inline int dev_pm_opp_set_regulator(struct device *dev, const char *name)
+static inline int dev_pm_opp_set_regulators(struct device *dev, const char *names[], unsigned int count)
{
return -ENOTSUPP;
}
-static inline void dev_pm_opp_put_regulator(struct device *dev) {}
+static inline void dev_pm_opp_put_regulators(struct device *dev) {}
static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
{
--
2.7.1.410.g6faf27b
[toc] | [next] | [standalone]
| From | Dave Gerlach <d-gerlach@ti.com> |
|---|---|
| Date | 2016-10-22 00:40 +0200 |
| Subject | Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators |
| Message-ID | <suQDL-3qP-3@gated-at.bofh.it> |
| In reply to | #1504636 |
Hi,
On 10/20/2016 03:44 AM, Viresh Kumar wrote:
> This patch adds infrastructure to manage multiple regulators and updates
> the only user (cpufreq-dt) of dev_pm_opp_set{put}_regulator().
>
> This is preparatory work for adding full support for devices with
> multiple regulators.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/base/power/opp/core.c | 220 ++++++++++++++++++++++++++-------------
> drivers/base/power/opp/debugfs.c | 48 +++++++--
> drivers/base/power/opp/of.c | 102 +++++++++++++-----
> drivers/base/power/opp/opp.h | 10 +-
> drivers/cpufreq/cpufreq-dt.c | 9 +-
> include/linux/pm_opp.h | 8 +-
> 6 files changed, 276 insertions(+), 121 deletions(-)
>
> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index 37fad2eb0f47..45c70ce07864 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -93,6 +93,8 @@ struct opp_table *_find_opp_table(struct device *dev)
> * Return: voltage in micro volt corresponding to the opp, else
> * return 0
> *
> + * This is useful only for devices with single power supply.
> + *
> * Locking: This function must be called under rcu_read_lock(). opp is a rcu
> * protected pointer. This means that opp which could have been fetched by
> * opp_find_freq_{exact,ceil,floor} functions is valid as long as we are
> @@ -112,7 +114,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
> if (IS_ERR_OR_NULL(tmp_opp))
> pr_err("%s: Invalid parameters\n", __func__);
> else
> - v = tmp_opp->supply.u_volt;
> + v = tmp_opp->supplies[0].u_volt;
>
> return v;
> }
> @@ -222,10 +224,10 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
> {
> struct opp_table *opp_table;
> struct dev_pm_opp *opp;
> - struct regulator *reg;
> + struct regulator *reg, **regulators;
> unsigned long latency_ns = 0;
> - unsigned long min_uV = ~0, max_uV = 0;
> - int ret;
> + unsigned long *min_uV, *max_uV;
> + int ret, size, i, count;
>
> rcu_read_lock();
>
> @@ -235,21 +237,44 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
> return 0;
> }
>
> - reg = opp_table->regulator;
> - if (IS_ERR(reg)) {
> + count = opp_table->regulator_count;
> +
> + if (!count) {
> /* Regulator may not be required for device */
> rcu_read_unlock();
> return 0;
> }
>
> - list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
> - if (!opp->available)
> - continue;
> + size = count * sizeof(*regulators);
> + regulators = kmemdup(opp_table->regulators, size, GFP_KERNEL);
> + if (!regulators) {
> + rcu_read_unlock();
> + return 0;
> + }
> +
> + min_uV = kmalloc(count * (sizeof(*min_uV) + sizeof(*max_uV)),
> + GFP_KERNEL);
> + if (!min_uV) {
> + kfree(regulators);
> + rcu_read_unlock();
> + return 0;
> + }
> +
> + max_uV = min_uV + count;
> +
> + for (i = 0; i < count; i++) {
> + min_uV[i] = ~0;
> + max_uV[i] = 0;
> +
> + list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
> + if (!opp->available)
> + continue;
>
> - if (opp->supply.u_volt_min < min_uV)
> - min_uV = opp->supply.u_volt_min;
> - if (opp->supply.u_volt_max > max_uV)
> - max_uV = opp->supply.u_volt_max;
> + if (opp->supplies[i].u_volt_min < min_uV[i])
> + min_uV[i] = opp->supplies[i].u_volt_min;
> + if (opp->supplies[i].u_volt_max > max_uV[i])
> + max_uV[i] = opp->supplies[i].u_volt_max;
> + }
> }
>
> rcu_read_unlock();
> @@ -258,9 +283,14 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
> * The caller needs to ensure that opp_table (and hence the regulator)
> * isn't freed, while we are executing this routine.
> */
> - ret = regulator_set_voltage_time(reg, min_uV, max_uV);
> - if (ret > 0)
> - latency_ns = ret * 1000;
> + for (i = 0; reg = regulators[i], i < count; i++) {
> + ret = regulator_set_voltage_time(reg, min_uV[i], max_uV[i]);
> + if (ret > 0)
> + latency_ns += ret * 1000;
> + }
> +
> + kfree(min_uV);
> + kfree(regulators);
>
> return latency_ns;
> }
> @@ -580,7 +610,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> {
> struct opp_table *opp_table;
> struct dev_pm_opp *old_opp, *opp;
> - struct regulator *reg;
> + struct regulator *reg = ERR_PTR(-ENXIO);
> struct clk *clk;
> unsigned long freq, old_freq;
> struct dev_pm_opp_supply old_supply, new_supply;
> @@ -633,14 +663,23 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> return ret;
> }
>
> + if (opp_table->regulators) {
> + /* This function only supports single regulator per device */
> + if (WARN_ON(opp_table->regulator_count > 1)) {
> + dev_err(dev, "multiple regulators not supported\n");
> + rcu_read_unlock();
> + return -EINVAL;
> + }
> +
> + reg = opp_table->regulators[0];
> + }
> +
> if (IS_ERR(old_opp))
> old_supply.u_volt = 0;
> else
> - memcpy(&old_supply, &old_opp->supply, sizeof(old_supply));
> + memcpy(&old_supply, old_opp->supplies, sizeof(old_supply));
>
> - memcpy(&new_supply, &opp->supply, sizeof(new_supply));
> -
> - reg = opp_table->regulator;
> + memcpy(&new_supply, opp->supplies, sizeof(new_supply));
>
> rcu_read_unlock();
>
> @@ -764,9 +803,6 @@ static struct opp_table *_add_opp_table(struct device *dev)
>
> _of_init_opp_table(opp_table, dev);
>
> - /* Set regulator to a non-NULL error value */
> - opp_table->regulator = ERR_PTR(-ENXIO);
> -
> /* Find clk for the device */
> opp_table->clk = clk_get(dev, NULL);
> if (IS_ERR(opp_table->clk)) {
> @@ -815,7 +851,7 @@ static void _remove_opp_table(struct opp_table *opp_table)
> if (opp_table->prop_name)
> return;
>
> - if (!IS_ERR(opp_table->regulator))
> + if (opp_table->regulators)
> return;
>
> /* Release clk */
> @@ -924,35 +960,49 @@ struct dev_pm_opp *_allocate_opp(struct device *dev,
> struct opp_table **opp_table)
> {
> struct dev_pm_opp *opp;
> + int count, supply_size;
> + struct opp_table *table;
>
> - /* allocate new OPP node */
> - opp = kzalloc(sizeof(*opp), GFP_KERNEL);
> - if (!opp)
> + table = _add_opp_table(dev);
> + if (!table)
> return NULL;
>
> - INIT_LIST_HEAD(&opp->node);
> + /* Allocate space for at least one supply */
> + count = table->regulator_count ? table->regulator_count : 1;
> + supply_size = sizeof(*opp->supplies) * count;
>
> - *opp_table = _add_opp_table(dev);
> - if (!*opp_table) {
> - kfree(opp);
> + /* allocate new OPP node + and supplies structures */
> + opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL);
> + if (!opp) {
> + kfree(table);
> return NULL;
> }
>
> + opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
> + INIT_LIST_HEAD(&opp->node);
> +
> + *opp_table = table;
> +
> return opp;
> }
>
> static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
> struct opp_table *opp_table)
> {
> - struct regulator *reg = opp_table->regulator;
> -
> - if (!IS_ERR(reg) &&
> - !regulator_is_supported_voltage(reg, opp->supply.u_volt_min,
> - opp->supply.u_volt_max)) {
> - pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
> - __func__, opp->supply.u_volt_min,
> - opp->supply.u_volt_max);
> - return false;
> + struct regulator *reg;
> + int i;
> +
> + for (i = 0; i < opp_table->regulator_count; i++) {
> + reg = opp_table->regulators[i];
> +
> + if (!regulator_is_supported_voltage(reg,
> + opp->supplies[i].u_volt_min,
> + opp->supplies[i].u_volt_max)) {
> + pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
> + __func__, opp->supplies[i].u_volt_min,
> + opp->supplies[i].u_volt_max);
> + return false;
> + }
> }
>
> return true;
> @@ -984,12 +1034,13 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
>
> /* Duplicate OPPs */
> dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
> - __func__, opp->rate, opp->supply.u_volt,
> - opp->available, new_opp->rate, new_opp->supply.u_volt,
> - new_opp->available);
> + __func__, opp->rate, opp->supplies[0].u_volt,
> + opp->available, new_opp->rate,
> + new_opp->supplies[0].u_volt, new_opp->available);
>
> + /* Should we compare voltages for all regulators here ? */
> return opp->available &&
> - new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST;
> + new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? 0 : -EEXIST;
> }
>
> new_opp->opp_table = opp_table;
> @@ -1056,9 +1107,9 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
> /* populate the opp table */
> new_opp->rate = freq;
> tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
> - new_opp->supply.u_volt = u_volt;
> - new_opp->supply.u_volt_min = u_volt - tol;
> - new_opp->supply.u_volt_max = u_volt + tol;
> + new_opp->supplies[0].u_volt = u_volt;
> + new_opp->supplies[0].u_volt_min = u_volt - tol;
> + new_opp->supplies[0].u_volt_max = u_volt + tol;
> new_opp->available = true;
> new_opp->dynamic = dynamic;
>
> @@ -1303,12 +1354,14 @@ void dev_pm_opp_put_prop_name(struct device *dev)
> EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
>
> /**
> - * dev_pm_opp_set_regulator() - Set regulator name for the device
> + * dev_pm_opp_set_regulators() - Set regulator names for the device
> * @dev: Device for which regulator name is being set.
> - * @name: Name of the regulator.
> + * @names: Array of pointers to the names of the regulator.
> + * @count: Number of regulators.
> *
> * In order to support OPP switching, OPP layer needs to know the name of the
> - * device's regulator, as the core would be required to switch voltages as well.
> + * device's regulators, as the core would be required to switch voltages as
> + * well.
> *
> * This must be called before any OPPs are initialized for the device.
> *
> @@ -1318,11 +1371,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
> * that this function is *NOT* called under RCU protection or in contexts where
> * mutex cannot be locked.
> */
> -int dev_pm_opp_set_regulator(struct device *dev, const char *name)
> +int dev_pm_opp_set_regulators(struct device *dev, const char *names[],
> + unsigned int count)
> {
> struct opp_table *opp_table;
> struct regulator *reg;
> - int ret;
> + int ret, i;
>
> mutex_lock(&opp_table_lock);
>
> @@ -1338,26 +1392,43 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
> goto err;
> }
>
> - /* Already have a regulator set */
> - if (WARN_ON(!IS_ERR(opp_table->regulator))) {
> + /* Already have regulators set */
> + if (WARN_ON(opp_table->regulators)) {
> ret = -EBUSY;
> goto err;
> }
> - /* Allocate the regulator */
> - reg = regulator_get_optional(dev, name);
> - if (IS_ERR(reg)) {
> - ret = PTR_ERR(reg);
> - if (ret != -EPROBE_DEFER)
> - dev_err(dev, "%s: no regulator (%s) found: %d\n",
> - __func__, name, ret);
> +
> + opp_table->regulators = kmalloc_array(count,
> + sizeof(*opp_table->regulators),
> + GFP_KERNEL);
> + if (!opp_table->regulators)
> goto err;
> +
> + for (i = 0; i < count; i++) {
> + reg = regulator_get_optional(dev, names[i]);
> + pr_info("%s: %d: %p: %s\n", __func__, __LINE__, reg, names[i]);
Think this is leftover debug msg?
> + if (IS_ERR(reg)) {
> + ret = PTR_ERR(reg);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "%s: regulator (%s) not found: %d\n",
> + __func__, names[i], ret);
> + goto free_regulators;
> + }
> +
> + opp_table->regulators[i] = reg;
> }
>
> - opp_table->regulator = reg;
> + opp_table->regulator_count = count;
>
> mutex_unlock(&opp_table_lock);
> return 0;
>
> +free_regulators:
> + while (i != 0)
> + regulator_put(opp_table->regulators[--i]);
> +
> + kfree(opp_table->regulators);
> + opp_table->regulators = NULL;
> err:
> _remove_opp_table(opp_table);
> unlock:
> @@ -1365,11 +1436,11 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
>
> return ret;
> }
> -EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
> +EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulators);
>
> /**
> - * dev_pm_opp_put_regulator() - Releases resources blocked for regulator
> - * @dev: Device for which regulator was set.
> + * dev_pm_opp_put_regulators() - Releases resources blocked for regulators
> + * @dev: Device for which regulators were set.
> *
> * Locking: The internal opp_table and opp structures are RCU protected.
> * Hence this function internally uses RCU updater strategy with mutex locks
> @@ -1377,9 +1448,10 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
> * that this function is *NOT* called under RCU protection or in contexts where
> * mutex cannot be locked.
> */
> -void dev_pm_opp_put_regulator(struct device *dev)
> +void dev_pm_opp_put_regulators(struct device *dev)
> {
> struct opp_table *opp_table;
> + int i;
>
> mutex_lock(&opp_table_lock);
>
> @@ -1391,16 +1463,20 @@ void dev_pm_opp_put_regulator(struct device *dev)
> goto unlock;
> }
>
> - if (IS_ERR(opp_table->regulator)) {
> - dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
> + if (!opp_table->regulators) {
> + dev_err(dev, "%s: Doesn't have regulators set\n", __func__);
> goto unlock;
> }
>
> /* Make sure there are no concurrent readers while updating opp_table */
> WARN_ON(!list_empty(&opp_table->opp_list));
>
> - regulator_put(opp_table->regulator);
> - opp_table->regulator = ERR_PTR(-ENXIO);
> + for (i = opp_table->regulator_count - 1; i >= 0; i--)
> + regulator_put(opp_table->regulators[i]);
> +
> + kfree(opp_table->regulators);
> + opp_table->regulators = NULL;
> + opp_table->regulator_count = 0;
>
> /* Try freeing opp_table if this was the last blocking resource */
> _remove_opp_table(opp_table);
> @@ -1408,7 +1484,7 @@ void dev_pm_opp_put_regulator(struct device *dev)
> unlock:
> mutex_unlock(&opp_table_lock);
> }
> -EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulator);
> +EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
>
> /**
> * dev_pm_opp_add() - Add an OPP table from a table definitions
> diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
> index c897676ca35f..cb5e5fde3d39 100644
> --- a/drivers/base/power/opp/debugfs.c
> +++ b/drivers/base/power/opp/debugfs.c
> @@ -34,6 +34,43 @@ void opp_debug_remove_one(struct dev_pm_opp *opp)
> debugfs_remove_recursive(opp->dentry);
> }
>
> +static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
> + struct opp_table *opp_table,
> + struct dentry *pdentry)
> +{
> + struct dentry *d;
> + int i = 0;
> + char name[] = "supply-X"; /* support only 0-9 supplies */
> +
> + /* Always create at least supply-0 directory */
> + do {
> + name[7] = i + '0';
> +
> + /* Create per-opp directory */
> + d = debugfs_create_dir(name, pdentry);
> + if (!d)
> + return false;
> +
> + if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d,
> + &opp->supplies[i].u_volt))
> + return false;
> +
> + if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d,
> + &opp->supplies[i].u_volt_min))
> + return false;
> +
> + if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d,
> + &opp->supplies[i].u_volt_max))
> + return false;
> +
> + if (!debugfs_create_ulong("u_amp", S_IRUGO, d,
> + &opp->supplies[i].u_amp))
> + return false;
> + } while (++i < opp_table->regulator_count);
> +
> + return true;
> +}
> +
> int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
> {
> struct dentry *pdentry = opp_table->dentry;
> @@ -63,16 +100,7 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
> if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate))
> return -ENOMEM;
>
> - if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt))
> - return -ENOMEM;
> -
> - if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min))
> - return -ENOMEM;
> -
> - if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max))
> - return -ENOMEM;
> -
> - if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp))
> + if (!opp_debug_create_supplies(opp, opp_table, d))
> return -ENOMEM;
>
> if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d,
> diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
> index b7fcd0a1b58b..c857fb07a5bc 100644
> --- a/drivers/base/power/opp/of.c
> +++ b/drivers/base/power/opp/of.c
> @@ -17,6 +17,7 @@
> #include <linux/errno.h>
> #include <linux/device.h>
> #include <linux/of.h>
> +#include <linux/slab.h>
> #include <linux/export.h>
>
> #include "opp.h"
> @@ -105,12 +106,13 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
Though not in the patch there's a comment to
/* TODO: Support multiple regulators */
in the file right above the below opp_parse_supplies function that can probably
be removed as part of this patch.
Otherwise this patch looks good to me.
Regards,
Dave
> static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
> struct opp_table *opp_table)
> {
> - u32 microvolt[3] = {0};
> - u32 val;
> - int count, ret;
> + u32 *microvolt, *microamp = NULL;
> + int supplies, vcount, icount, ret, i, j;
> struct property *prop = NULL;
> char name[NAME_MAX];
>
> + supplies = opp_table->regulator_count ? opp_table->regulator_count : 1;
> +
> /* Search for "opp-microvolt-<name>" */
> if (opp_table->prop_name) {
> snprintf(name, sizeof(name), "opp-microvolt-%s",
> @@ -128,34 +130,29 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
> return 0;
> }
>
> - count = of_property_count_u32_elems(opp->np, name);
> - if (count < 0) {
> + vcount = of_property_count_u32_elems(opp->np, name);
> + if (vcount < 0) {
> dev_err(dev, "%s: Invalid %s property (%d)\n",
> - __func__, name, count);
> - return count;
> + __func__, name, vcount);
> + return vcount;
> }
>
> - /* There can be one or three elements here */
> - if (count != 1 && count != 3) {
> - dev_err(dev, "%s: Invalid number of elements in %s property (%d)\n",
> - __func__, name, count);
> + /* There can be one or three elements per supply */
> + if (vcount != supplies && vcount != supplies * 3) {
> + dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n",
> + __func__, name, vcount, supplies);
> return -EINVAL;
> }
>
> - ret = of_property_read_u32_array(opp->np, name, microvolt, count);
> + microvolt = kmalloc_array(vcount, sizeof(*microvolt), GFP_KERNEL);
> + if (!microvolt)
> + return -ENOMEM;
> +
> + ret = of_property_read_u32_array(opp->np, name, microvolt, vcount);
> if (ret) {
> dev_err(dev, "%s: error parsing %s: %d\n", __func__, name, ret);
> - return -EINVAL;
> - }
> -
> - opp->supply.u_volt = microvolt[0];
> -
> - if (count == 1) {
> - opp->supply.u_volt_min = opp->supply.u_volt;
> - opp->supply.u_volt_max = opp->supply.u_volt;
> - } else {
> - opp->supply.u_volt_min = microvolt[1];
> - opp->supply.u_volt_max = microvolt[2];
> + ret = -EINVAL;
> + goto free_microvolt;
> }
>
> /* Search for "opp-microamp-<name>" */
> @@ -172,10 +169,59 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
> prop = of_find_property(opp->np, name, NULL);
> }
>
> - if (prop && !of_property_read_u32(opp->np, name, &val))
> - opp->supply.u_amp = val;
> + if (prop) {
> + icount = of_property_count_u32_elems(opp->np, name);
> + if (icount < 0) {
> + dev_err(dev, "%s: Invalid %s property (%d)\n", __func__,
> + name, icount);
> + ret = icount;
> + goto free_microvolt;
> + }
>
> - return 0;
> + if (icount != supplies) {
> + dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n",
> + __func__, name, icount, supplies);
> + ret = -EINVAL;
> + goto free_microvolt;
> + }
> +
> + microamp = kmalloc_array(icount, sizeof(*microamp), GFP_KERNEL);
> + if (!microamp) {
> + ret = -EINVAL;
> + goto free_microvolt;
> + }
> +
> + ret = of_property_read_u32_array(opp->np, name, microamp,
> + icount);
> + if (ret) {
> + dev_err(dev, "%s: error parsing %s: %d\n", __func__,
> + name, ret);
> + ret = -EINVAL;
> + goto free_microamp;
> + }
> + }
> +
> + for (i = 0, j = 0; i < supplies; i++) {
> + opp->supplies[i].u_volt = microvolt[j++];
> +
> + if (vcount == supplies) {
> + opp->supplies[i].u_volt_min = opp->supplies[i].u_volt;
> + opp->supplies[i].u_volt_max = opp->supplies[i].u_volt;
> + } else {
> + opp->supplies[i].u_volt_min = microvolt[j++];
> + opp->supplies[i].u_volt_max = microvolt[j++];
> + }
> +
> + if (microamp)
> + opp->supplies[i].u_amp = microamp[i];
> + }
> +
> +free_microamp:
> + kfree(microamp);
> +free_microvolt:
> + kfree(microvolt);
> +
> + return ret;
> }
>
> /**
> @@ -304,8 +350,8 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
>
> pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n",
> __func__, new_opp->turbo, new_opp->rate,
> - new_opp->supply.u_volt, new_opp->supply.u_volt_min,
> - new_opp->supply.u_volt_max, new_opp->clock_latency_ns);
> + new_opp->supplies[0].u_volt, new_opp->supplies[0].u_volt_min,
> + new_opp->supplies[0].u_volt_max, new_opp->clock_latency_ns);
>
> /*
> * Notify the changes in the availability of the operable
> diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
> index 1bda0d35c486..d3f0861f9bff 100644
> --- a/drivers/base/power/opp/opp.h
> +++ b/drivers/base/power/opp/opp.h
> @@ -77,7 +77,7 @@ struct dev_pm_opp_supply {
> * @turbo: true if turbo (boost) OPP
> * @suspend: true if suspend OPP
> * @rate: Frequency in hertz
> - * @supply: Power supply voltage/current values
> + * @supplies: Power supplies voltage/current values
> * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
> * frequency from any other OPP's frequency.
> * @opp_table: points back to the opp_table struct this opp belongs to
> @@ -96,7 +96,7 @@ struct dev_pm_opp {
> bool suspend;
> unsigned long rate;
>
> - struct dev_pm_opp_supply supply;
> + struct dev_pm_opp_supply *supplies;
>
> unsigned long clock_latency_ns;
>
> @@ -155,7 +155,8 @@ enum opp_table_access {
> * @supported_hw_count: Number of elements in supported_hw array.
> * @prop_name: A name to postfix to many DT properties, while parsing them.
> * @clk: Device's clock handle
> - * @regulator: Supply regulator
> + * @regulators: Supply regulators
> + * @regulator_count: Number of Power Supply regulators
> * @dentry: debugfs dentry pointer of the real device directory (not links).
> * @dentry_name: Name of the real dentry.
> *
> @@ -190,7 +191,8 @@ struct opp_table {
> unsigned int supported_hw_count;
> const char *prop_name;
> struct clk *clk;
> - struct regulator *regulator;
> + struct regulator **regulators;
> + unsigned int regulator_count;
>
> #ifdef CONFIG_DEBUG_FS
> struct dentry *dentry;
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index 5c07ae05d69a..15cb26118dc7 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -186,7 +186,10 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> */
> name = find_supply_name(cpu_dev);
> if (name) {
> - ret = dev_pm_opp_set_regulator(cpu_dev, name);
> + const char *names[] = {name};
> +
> + ret = dev_pm_opp_set_regulators(cpu_dev, names,
> + ARRAY_SIZE(names));
> if (ret) {
> dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
> policy->cpu, ret);
> @@ -285,7 +288,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> out_free_opp:
> dev_pm_opp_of_cpumask_remove_table(policy->cpus);
> if (name)
> - dev_pm_opp_put_regulator(cpu_dev);
> + dev_pm_opp_put_regulators(cpu_dev);
> out_put_clk:
> clk_put(cpu_clk);
>
> @@ -300,7 +303,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
> dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
> dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
> if (priv->reg_name)
> - dev_pm_opp_put_regulator(priv->cpu_dev);
> + dev_pm_opp_put_regulators(priv->cpu_dev);
>
> clk_put(policy->clk);
> kfree(priv);
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index bca26157f5b6..0606b70a8b97 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -62,8 +62,8 @@ int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
> void dev_pm_opp_put_supported_hw(struct device *dev);
> int dev_pm_opp_set_prop_name(struct device *dev, const char *name);
> void dev_pm_opp_put_prop_name(struct device *dev);
> -int dev_pm_opp_set_regulator(struct device *dev, const char *name);
> -void dev_pm_opp_put_regulator(struct device *dev);
> +int dev_pm_opp_set_regulators(struct device *dev, const char *names[], unsigned int count);
> +void dev_pm_opp_put_regulators(struct device *dev);
> int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
> int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
> int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
> @@ -170,12 +170,12 @@ static inline int dev_pm_opp_set_prop_name(struct device *dev, const char *name)
>
> static inline void dev_pm_opp_put_prop_name(struct device *dev) {}
>
> -static inline int dev_pm_opp_set_regulator(struct device *dev, const char *name)
> +static inline int dev_pm_opp_set_regulators(struct device *dev, const char *names[], unsigned int count)
> {
> return -ENOTSUPP;
> }
>
> -static inline void dev_pm_opp_put_regulator(struct device *dev) {}
> +static inline void dev_pm_opp_put_regulators(struct device *dev) {}
>
> static inline int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> {
>
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-10-24 05:40 +0200 |
| Subject | Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators |
| Message-ID | <svEhb-20C-5@gated-at.bofh.it> |
| In reply to | #1506337 |
On 21-10-16, 17:32, Dave Gerlach wrote:
> Hi,
> On 10/20/2016 03:44 AM, Viresh Kumar wrote:
> > This patch adds infrastructure to manage multiple regulators and updates
> > the only user (cpufreq-dt) of dev_pm_opp_set{put}_regulator().
> >
> > This is preparatory work for adding full support for devices with
> > multiple regulators.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > drivers/base/power/opp/core.c | 220 ++++++++++++++++++++++++++-------------
> > drivers/base/power/opp/debugfs.c | 48 +++++++--
> > drivers/base/power/opp/of.c | 102 +++++++++++++-----
> > drivers/base/power/opp/opp.h | 10 +-
> > drivers/cpufreq/cpufreq-dt.c | 9 +-
> > include/linux/pm_opp.h | 8 +-
> > 6 files changed, 276 insertions(+), 121 deletions(-)
> >
> > diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> > index 37fad2eb0f47..45c70ce07864 100644
> > --- a/drivers/base/power/opp/core.c
> > +++ b/drivers/base/power/opp/core.c
> > @@ -93,6 +93,8 @@ struct opp_table *_find_opp_table(struct device *dev)
> > * Return: voltage in micro volt corresponding to the opp, else
> > * return 0
> > *
> > + * This is useful only for devices with single power supply.
> > + *
> > * Locking: This function must be called under rcu_read_lock(). opp is a rcu
> > * protected pointer. This means that opp which could have been fetched by
> > * opp_find_freq_{exact,ceil,floor} functions is valid as long as we are
> > @@ -112,7 +114,7 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
> > if (IS_ERR_OR_NULL(tmp_opp))
> > pr_err("%s: Invalid parameters\n", __func__);
> > else
> > - v = tmp_opp->supply.u_volt;
> > + v = tmp_opp->supplies[0].u_volt;
> >
> > return v;
> > }
> > @@ -222,10 +224,10 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
> > {
> > struct opp_table *opp_table;
> > struct dev_pm_opp *opp;
> > - struct regulator *reg;
> > + struct regulator *reg, **regulators;
> > unsigned long latency_ns = 0;
> > - unsigned long min_uV = ~0, max_uV = 0;
> > - int ret;
> > + unsigned long *min_uV, *max_uV;
> > + int ret, size, i, count;
> >
> > rcu_read_lock();
> >
> > @@ -235,21 +237,44 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
> > return 0;
> > }
> >
> > - reg = opp_table->regulator;
> > - if (IS_ERR(reg)) {
> > + count = opp_table->regulator_count;
> > +
> > + if (!count) {
> > /* Regulator may not be required for device */
> > rcu_read_unlock();
> > return 0;
> > }
> >
> > - list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
> > - if (!opp->available)
> > - continue;
> > + size = count * sizeof(*regulators);
> > + regulators = kmemdup(opp_table->regulators, size, GFP_KERNEL);
> > + if (!regulators) {
> > + rcu_read_unlock();
> > + return 0;
> > + }
> > +
> > + min_uV = kmalloc(count * (sizeof(*min_uV) + sizeof(*max_uV)),
> > + GFP_KERNEL);
> > + if (!min_uV) {
> > + kfree(regulators);
> > + rcu_read_unlock();
> > + return 0;
> > + }
> > +
> > + max_uV = min_uV + count;
> > +
> > + for (i = 0; i < count; i++) {
> > + min_uV[i] = ~0;
> > + max_uV[i] = 0;
> > +
> > + list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
> > + if (!opp->available)
> > + continue;
> >
> > - if (opp->supply.u_volt_min < min_uV)
> > - min_uV = opp->supply.u_volt_min;
> > - if (opp->supply.u_volt_max > max_uV)
> > - max_uV = opp->supply.u_volt_max;
> > + if (opp->supplies[i].u_volt_min < min_uV[i])
> > + min_uV[i] = opp->supplies[i].u_volt_min;
> > + if (opp->supplies[i].u_volt_max > max_uV[i])
> > + max_uV[i] = opp->supplies[i].u_volt_max;
> > + }
> > }
> >
> > rcu_read_unlock();
> > @@ -258,9 +283,14 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
> > * The caller needs to ensure that opp_table (and hence the regulator)
> > * isn't freed, while we are executing this routine.
> > */
> > - ret = regulator_set_voltage_time(reg, min_uV, max_uV);
> > - if (ret > 0)
> > - latency_ns = ret * 1000;
> > + for (i = 0; reg = regulators[i], i < count; i++) {
> > + ret = regulator_set_voltage_time(reg, min_uV[i], max_uV[i]);
> > + if (ret > 0)
> > + latency_ns += ret * 1000;
> > + }
> > +
> > + kfree(min_uV);
> > + kfree(regulators);
> >
> > return latency_ns;
> > }
> > @@ -580,7 +610,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> > {
> > struct opp_table *opp_table;
> > struct dev_pm_opp *old_opp, *opp;
> > - struct regulator *reg;
> > + struct regulator *reg = ERR_PTR(-ENXIO);
> > struct clk *clk;
> > unsigned long freq, old_freq;
> > struct dev_pm_opp_supply old_supply, new_supply;
> > @@ -633,14 +663,23 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> > return ret;
> > }
> >
> > + if (opp_table->regulators) {
> > + /* This function only supports single regulator per device */
> > + if (WARN_ON(opp_table->regulator_count > 1)) {
> > + dev_err(dev, "multiple regulators not supported\n");
> > + rcu_read_unlock();
> > + return -EINVAL;
> > + }
> > +
> > + reg = opp_table->regulators[0];
> > + }
> > +
> > if (IS_ERR(old_opp))
> > old_supply.u_volt = 0;
> > else
> > - memcpy(&old_supply, &old_opp->supply, sizeof(old_supply));
> > + memcpy(&old_supply, old_opp->supplies, sizeof(old_supply));
> >
> > - memcpy(&new_supply, &opp->supply, sizeof(new_supply));
> > -
> > - reg = opp_table->regulator;
> > + memcpy(&new_supply, opp->supplies, sizeof(new_supply));
> >
> > rcu_read_unlock();
> >
> > @@ -764,9 +803,6 @@ static struct opp_table *_add_opp_table(struct device *dev)
> >
> > _of_init_opp_table(opp_table, dev);
> >
> > - /* Set regulator to a non-NULL error value */
> > - opp_table->regulator = ERR_PTR(-ENXIO);
> > -
> > /* Find clk for the device */
> > opp_table->clk = clk_get(dev, NULL);
> > if (IS_ERR(opp_table->clk)) {
> > @@ -815,7 +851,7 @@ static void _remove_opp_table(struct opp_table *opp_table)
> > if (opp_table->prop_name)
> > return;
> >
> > - if (!IS_ERR(opp_table->regulator))
> > + if (opp_table->regulators)
> > return;
> >
> > /* Release clk */
> > @@ -924,35 +960,49 @@ struct dev_pm_opp *_allocate_opp(struct device *dev,
> > struct opp_table **opp_table)
> > {
> > struct dev_pm_opp *opp;
> > + int count, supply_size;
> > + struct opp_table *table;
> >
> > - /* allocate new OPP node */
> > - opp = kzalloc(sizeof(*opp), GFP_KERNEL);
> > - if (!opp)
> > + table = _add_opp_table(dev);
> > + if (!table)
> > return NULL;
> >
> > - INIT_LIST_HEAD(&opp->node);
> > + /* Allocate space for at least one supply */
> > + count = table->regulator_count ? table->regulator_count : 1;
> > + supply_size = sizeof(*opp->supplies) * count;
> >
> > - *opp_table = _add_opp_table(dev);
> > - if (!*opp_table) {
> > - kfree(opp);
> > + /* allocate new OPP node + and supplies structures */
> > + opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL);
> > + if (!opp) {
> > + kfree(table);
> > return NULL;
> > }
> >
> > + opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
> > + INIT_LIST_HEAD(&opp->node);
> > +
> > + *opp_table = table;
> > +
> > return opp;
> > }
> >
> > static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
> > struct opp_table *opp_table)
> > {
> > - struct regulator *reg = opp_table->regulator;
> > -
> > - if (!IS_ERR(reg) &&
> > - !regulator_is_supported_voltage(reg, opp->supply.u_volt_min,
> > - opp->supply.u_volt_max)) {
> > - pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
> > - __func__, opp->supply.u_volt_min,
> > - opp->supply.u_volt_max);
> > - return false;
> > + struct regulator *reg;
> > + int i;
> > +
> > + for (i = 0; i < opp_table->regulator_count; i++) {
> > + reg = opp_table->regulators[i];
> > +
> > + if (!regulator_is_supported_voltage(reg,
> > + opp->supplies[i].u_volt_min,
> > + opp->supplies[i].u_volt_max)) {
> > + pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
> > + __func__, opp->supplies[i].u_volt_min,
> > + opp->supplies[i].u_volt_max);
> > + return false;
> > + }
> > }
> >
> > return true;
> > @@ -984,12 +1034,13 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> >
> > /* Duplicate OPPs */
> > dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
> > - __func__, opp->rate, opp->supply.u_volt,
> > - opp->available, new_opp->rate, new_opp->supply.u_volt,
> > - new_opp->available);
> > + __func__, opp->rate, opp->supplies[0].u_volt,
> > + opp->available, new_opp->rate,
> > + new_opp->supplies[0].u_volt, new_opp->available);
> >
> > + /* Should we compare voltages for all regulators here ? */
> > return opp->available &&
> > - new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST;
> > + new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? 0 : -EEXIST;
> > }
> >
> > new_opp->opp_table = opp_table;
> > @@ -1056,9 +1107,9 @@ int _opp_add_v1(struct device *dev, unsigned long freq, long u_volt,
> > /* populate the opp table */
> > new_opp->rate = freq;
> > tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
> > - new_opp->supply.u_volt = u_volt;
> > - new_opp->supply.u_volt_min = u_volt - tol;
> > - new_opp->supply.u_volt_max = u_volt + tol;
> > + new_opp->supplies[0].u_volt = u_volt;
> > + new_opp->supplies[0].u_volt_min = u_volt - tol;
> > + new_opp->supplies[0].u_volt_max = u_volt + tol;
> > new_opp->available = true;
> > new_opp->dynamic = dynamic;
> >
> > @@ -1303,12 +1354,14 @@ void dev_pm_opp_put_prop_name(struct device *dev)
> > EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
> >
> > /**
> > - * dev_pm_opp_set_regulator() - Set regulator name for the device
> > + * dev_pm_opp_set_regulators() - Set regulator names for the device
> > * @dev: Device for which regulator name is being set.
> > - * @name: Name of the regulator.
> > + * @names: Array of pointers to the names of the regulator.
> > + * @count: Number of regulators.
> > *
> > * In order to support OPP switching, OPP layer needs to know the name of the
> > - * device's regulator, as the core would be required to switch voltages as well.
> > + * device's regulators, as the core would be required to switch voltages as
> > + * well.
> > *
> > * This must be called before any OPPs are initialized for the device.
> > *
> > @@ -1318,11 +1371,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
> > * that this function is *NOT* called under RCU protection or in contexts where
> > * mutex cannot be locked.
> > */
> > -int dev_pm_opp_set_regulator(struct device *dev, const char *name)
> > +int dev_pm_opp_set_regulators(struct device *dev, const char *names[],
> > + unsigned int count)
> > {
> > struct opp_table *opp_table;
> > struct regulator *reg;
> > - int ret;
> > + int ret, i;
> >
> > mutex_lock(&opp_table_lock);
> >
> > @@ -1338,26 +1392,43 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
> > goto err;
> > }
> >
> > - /* Already have a regulator set */
> > - if (WARN_ON(!IS_ERR(opp_table->regulator))) {
> > + /* Already have regulators set */
> > + if (WARN_ON(opp_table->regulators)) {
> > ret = -EBUSY;
> > goto err;
> > }
> > - /* Allocate the regulator */
> > - reg = regulator_get_optional(dev, name);
> > - if (IS_ERR(reg)) {
> > - ret = PTR_ERR(reg);
> > - if (ret != -EPROBE_DEFER)
> > - dev_err(dev, "%s: no regulator (%s) found: %d\n",
> > - __func__, name, ret);
> > +
> > + opp_table->regulators = kmalloc_array(count,
> > + sizeof(*opp_table->regulators),
> > + GFP_KERNEL);
> > + if (!opp_table->regulators)
> > goto err;
> > +
> > + for (i = 0; i < count; i++) {
> > + reg = regulator_get_optional(dev, names[i]);
> > + pr_info("%s: %d: %p: %s\n", __func__, __LINE__, reg, names[i]);
>
> Think this is leftover debug msg?
Yes.
> > + if (IS_ERR(reg)) {
> > + ret = PTR_ERR(reg);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(dev, "%s: regulator (%s) not found: %d\n",
> > + __func__, names[i], ret);
> > + goto free_regulators;
> > + }
> > +
> > + opp_table->regulators[i] = reg;
> > }
> >
> > - opp_table->regulator = reg;
> > + opp_table->regulator_count = count;
> >
> > mutex_unlock(&opp_table_lock);
> > return 0;
> >
> > +free_regulators:
> > + while (i != 0)
> > + regulator_put(opp_table->regulators[--i]);
> > +
> > + kfree(opp_table->regulators);
> > + opp_table->regulators = NULL;
> > err:
> > _remove_opp_table(opp_table);
> > unlock:
> > @@ -1365,11 +1436,11 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
> >
> > return ret;
> > }
> > -EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
> > +EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulators);
> >
> > /**
> > - * dev_pm_opp_put_regulator() - Releases resources blocked for regulator
> > - * @dev: Device for which regulator was set.
> > + * dev_pm_opp_put_regulators() - Releases resources blocked for regulators
> > + * @dev: Device for which regulators were set.
> > *
> > * Locking: The internal opp_table and opp structures are RCU protected.
> > * Hence this function internally uses RCU updater strategy with mutex locks
> > @@ -1377,9 +1448,10 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulator);
> > * that this function is *NOT* called under RCU protection or in contexts where
> > * mutex cannot be locked.
> > */
> > -void dev_pm_opp_put_regulator(struct device *dev)
> > +void dev_pm_opp_put_regulators(struct device *dev)
> > {
> > struct opp_table *opp_table;
> > + int i;
> >
> > mutex_lock(&opp_table_lock);
> >
> > @@ -1391,16 +1463,20 @@ void dev_pm_opp_put_regulator(struct device *dev)
> > goto unlock;
> > }
> >
> > - if (IS_ERR(opp_table->regulator)) {
> > - dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
> > + if (!opp_table->regulators) {
> > + dev_err(dev, "%s: Doesn't have regulators set\n", __func__);
> > goto unlock;
> > }
> >
> > /* Make sure there are no concurrent readers while updating opp_table */
> > WARN_ON(!list_empty(&opp_table->opp_list));
> >
> > - regulator_put(opp_table->regulator);
> > - opp_table->regulator = ERR_PTR(-ENXIO);
> > + for (i = opp_table->regulator_count - 1; i >= 0; i--)
> > + regulator_put(opp_table->regulators[i]);
> > +
> > + kfree(opp_table->regulators);
> > + opp_table->regulators = NULL;
> > + opp_table->regulator_count = 0;
> >
> > /* Try freeing opp_table if this was the last blocking resource */
> > _remove_opp_table(opp_table);
> > @@ -1408,7 +1484,7 @@ void dev_pm_opp_put_regulator(struct device *dev)
> > unlock:
> > mutex_unlock(&opp_table_lock);
> > }
> > -EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulator);
> > +EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
> >
> > /**
> > * dev_pm_opp_add() - Add an OPP table from a table definitions
> > diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
> > index c897676ca35f..cb5e5fde3d39 100644
> > --- a/drivers/base/power/opp/debugfs.c
> > +++ b/drivers/base/power/opp/debugfs.c
> > @@ -34,6 +34,43 @@ void opp_debug_remove_one(struct dev_pm_opp *opp)
> > debugfs_remove_recursive(opp->dentry);
> > }
> >
> > +static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
> > + struct opp_table *opp_table,
> > + struct dentry *pdentry)
> > +{
> > + struct dentry *d;
> > + int i = 0;
> > + char name[] = "supply-X"; /* support only 0-9 supplies */
> > +
> > + /* Always create at least supply-0 directory */
> > + do {
> > + name[7] = i + '0';
> > +
> > + /* Create per-opp directory */
> > + d = debugfs_create_dir(name, pdentry);
> > + if (!d)
> > + return false;
> > +
> > + if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d,
> > + &opp->supplies[i].u_volt))
> > + return false;
> > +
> > + if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d,
> > + &opp->supplies[i].u_volt_min))
> > + return false;
> > +
> > + if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d,
> > + &opp->supplies[i].u_volt_max))
> > + return false;
> > +
> > + if (!debugfs_create_ulong("u_amp", S_IRUGO, d,
> > + &opp->supplies[i].u_amp))
> > + return false;
> > + } while (++i < opp_table->regulator_count);
> > +
> > + return true;
> > +}
> > +
> > int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
> > {
> > struct dentry *pdentry = opp_table->dentry;
> > @@ -63,16 +100,7 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
> > if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate))
> > return -ENOMEM;
> >
> > - if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d, &opp->supply.u_volt))
> > - return -ENOMEM;
> > -
> > - if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d, &opp->supply.u_volt_min))
> > - return -ENOMEM;
> > -
> > - if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d, &opp->supply.u_volt_max))
> > - return -ENOMEM;
> > -
> > - if (!debugfs_create_ulong("u_amp", S_IRUGO, d, &opp->supply.u_amp))
> > + if (!opp_debug_create_supplies(opp, opp_table, d))
> > return -ENOMEM;
> >
> > if (!debugfs_create_ulong("clock_latency_ns", S_IRUGO, d,
> > diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
> > index b7fcd0a1b58b..c857fb07a5bc 100644
> > --- a/drivers/base/power/opp/of.c
> > +++ b/drivers/base/power/opp/of.c
> > @@ -17,6 +17,7 @@
> > #include <linux/errno.h>
> > #include <linux/device.h>
> > #include <linux/of.h>
> > +#include <linux/slab.h>
> > #include <linux/export.h>
> >
> > #include "opp.h"
> > @@ -105,12 +106,13 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
>
> Though not in the patch there's a comment to
>
> /* TODO: Support multiple regulators */
>
> in the file right above the below opp_parse_supplies function that can probably
> be removed as part of this patch.
Sure.
--
viresh
[toc] | [prev] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2016-10-25 19:00 +0200 |
| Subject | Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators |
| Message-ID | <swdeW-8ec-15@gated-at.bofh.it> |
| In reply to | #1504636 |
On 10/20, Viresh Kumar wrote:
> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index 37fad2eb0f47..45c70ce07864 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -235,21 +237,44 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
> return 0;
> }
>
> - reg = opp_table->regulator;
> - if (IS_ERR(reg)) {
> + count = opp_table->regulator_count;
> +
> + if (!count) {
> /* Regulator may not be required for device */
> rcu_read_unlock();
> return 0;
> }
>
> - list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
> - if (!opp->available)
> - continue;
> + size = count * sizeof(*regulators);
> + regulators = kmemdup(opp_table->regulators, size, GFP_KERNEL);
How do we allocate under RCU? Doesn't that spit out big warnings?
> + if (!regulators) {
> + rcu_read_unlock();
> + return 0;
> + }
> +
> + min_uV = kmalloc(count * (sizeof(*min_uV) + sizeof(*max_uV)),
Do we imagine min_uV is going to be a different size from max_uV?
It may be better to have a struct for min/max pair and then
stride them. Then the kmalloc() can become a kmalloc_array().
> + GFP_KERNEL);
> + if (!min_uV) {
> + kfree(regulators);
> + rcu_read_unlock();
> + return 0;
> + }
> +
> + max_uV = min_uV + count;
> +
> + for (i = 0; i < count; i++) {
> + min_uV[i] = ~0;
> + max_uV[i] = 0;
> +
> + list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
> + if (!opp->available)
> + continue;
>
> - if (opp->supply.u_volt_min < min_uV)
> - min_uV = opp->supply.u_volt_min;
> - if (opp->supply.u_volt_max > max_uV)
> - max_uV = opp->supply.u_volt_max;
> + if (opp->supplies[i].u_volt_min < min_uV[i])
> + min_uV[i] = opp->supplies[i].u_volt_min;
> + if (opp->supplies[i].u_volt_max > max_uV[i])
> + max_uV[i] = opp->supplies[i].u_volt_max;
> + }
> }
>
> rcu_read_unlock();
> @@ -924,35 +960,49 @@ struct dev_pm_opp *_allocate_opp(struct device *dev,
> struct opp_table **opp_table)
> {
> struct dev_pm_opp *opp;
> + int count, supply_size;
> + struct opp_table *table;
>
> - /* allocate new OPP node */
> - opp = kzalloc(sizeof(*opp), GFP_KERNEL);
> - if (!opp)
> + table = _add_opp_table(dev);
> + if (!table)
> return NULL;
>
> - INIT_LIST_HEAD(&opp->node);
> + /* Allocate space for at least one supply */
> + count = table->regulator_count ? table->regulator_count : 1;
> + supply_size = sizeof(*opp->supplies) * count;
>
> - *opp_table = _add_opp_table(dev);
> - if (!*opp_table) {
> - kfree(opp);
> + /* allocate new OPP node + and supplies structures */
> + opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL);
> + if (!opp) {
> + kfree(table);
> return NULL;
> }
>
> + opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
So put the supplies at the end of the OPP structure as an empty
array?
> + INIT_LIST_HEAD(&opp->node);
> +
> + *opp_table = table;
> +
> return opp;
> }
>
> static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
> struct opp_table *opp_table)
> {
> - struct regulator *reg = opp_table->regulator;
> -
> - if (!IS_ERR(reg) &&
> - !regulator_is_supported_voltage(reg, opp->supply.u_volt_min,
> - opp->supply.u_volt_max)) {
> - pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
> - __func__, opp->supply.u_volt_min,
> - opp->supply.u_volt_max);
> - return false;
> + struct regulator *reg;
> + int i;
> +
> + for (i = 0; i < opp_table->regulator_count; i++) {
> + reg = opp_table->regulators[i];
> +
> + if (!regulator_is_supported_voltage(reg,
> + opp->supplies[i].u_volt_min,
> + opp->supplies[i].u_volt_max)) {
> + pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
> + __func__, opp->supplies[i].u_volt_min,
> + opp->supplies[i].u_volt_max);
> + return false;
> + }
> }
>
> return true;
> @@ -984,12 +1034,13 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
>
> /* Duplicate OPPs */
> dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
> - __func__, opp->rate, opp->supply.u_volt,
> - opp->available, new_opp->rate, new_opp->supply.u_volt,
> - new_opp->available);
> + __func__, opp->rate, opp->supplies[0].u_volt,
> + opp->available, new_opp->rate,
> + new_opp->supplies[0].u_volt, new_opp->available);
>
> + /* Should we compare voltages for all regulators here ? */
> return opp->available &&
> - new_opp->supply.u_volt == opp->supply.u_volt ? 0 : -EEXIST;
> + new_opp->supplies[0].u_volt == opp->supplies[0].u_volt ? 0 : -EEXIST;
> }
>
> new_opp->opp_table = opp_table;
> @@ -1303,12 +1354,14 @@ void dev_pm_opp_put_prop_name(struct device *dev)
> EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
>
> /**
> - * dev_pm_opp_set_regulator() - Set regulator name for the device
> + * dev_pm_opp_set_regulators() - Set regulator names for the device
> * @dev: Device for which regulator name is being set.
> - * @name: Name of the regulator.
> + * @names: Array of pointers to the names of the regulator.
> + * @count: Number of regulators.
> *
> * In order to support OPP switching, OPP layer needs to know the name of the
> - * device's regulator, as the core would be required to switch voltages as well.
> + * device's regulators, as the core would be required to switch voltages as
> + * well.
> *
> * This must be called before any OPPs are initialized for the device.
> *
> @@ -1318,11 +1371,12 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
> * that this function is *NOT* called under RCU protection or in contexts where
> * mutex cannot be locked.
> */
> -int dev_pm_opp_set_regulator(struct device *dev, const char *name)
> +int dev_pm_opp_set_regulators(struct device *dev, const char *names[],
Make names a const char * const *? I seem to recall arrays as
function arguments has some problem but my C mastery is failing
right now.
> + unsigned int count)
> {
> struct opp_table *opp_table;
> struct regulator *reg;
> - int ret;
> + int ret, i;
>
> mutex_lock(&opp_table_lock);
>
> @@ -1338,26 +1392,43 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
> goto err;
> }
>
> - /* Already have a regulator set */
> - if (WARN_ON(!IS_ERR(opp_table->regulator))) {
> + /* Already have regulators set */
> + if (WARN_ON(opp_table->regulators)) {
> ret = -EBUSY;
> goto err;
> }
> - /* Allocate the regulator */
> - reg = regulator_get_optional(dev, name);
> - if (IS_ERR(reg)) {
> - ret = PTR_ERR(reg);
> - if (ret != -EPROBE_DEFER)
> - dev_err(dev, "%s: no regulator (%s) found: %d\n",
> - __func__, name, ret);
> +
> + opp_table->regulators = kmalloc_array(count,
> + sizeof(*opp_table->regulators),
> + GFP_KERNEL);
> + if (!opp_table->regulators)
> goto err;
> +
> + for (i = 0; i < count; i++) {
> + reg = regulator_get_optional(dev, names[i]);
> + pr_info("%s: %d: %p: %s\n", __func__, __LINE__, reg, names[i]);
Debug noise?
> + if (IS_ERR(reg)) {
> + ret = PTR_ERR(reg);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "%s: regulator (%s) not found: %d\n",
> + __func__, names[i], ret);
> + goto free_regulators;
> + }
> +
> + opp_table->regulators[i] = reg;
> }
>
> - opp_table->regulator = reg;
> + opp_table->regulator_count = count;
>
> mutex_unlock(&opp_table_lock);
> return 0;
>
> +free_regulators:
> + while (i != 0)
> + regulator_put(opp_table->regulators[--i]);
> +
> + kfree(opp_table->regulators);
> + opp_table->regulators = NULL;
> err:
> _remove_opp_table(opp_table);
> unlock:
> diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
> index c897676ca35f..cb5e5fde3d39 100644
> --- a/drivers/base/power/opp/debugfs.c
> +++ b/drivers/base/power/opp/debugfs.c
> @@ -34,6 +34,43 @@ void opp_debug_remove_one(struct dev_pm_opp *opp)
> debugfs_remove_recursive(opp->dentry);
> }
>
> +static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
> + struct opp_table *opp_table,
> + struct dentry *pdentry)
> +{
> + struct dentry *d;
> + int i = 0;
> + char name[] = "supply-X"; /* support only 0-9 supplies */
But we don't verify that's the case? Why not use kasprintf() and
free() and then there isn't any limit?
> +
> + /* Always create at least supply-0 directory */
> + do {
> + name[7] = i + '0';
> +
> + /* Create per-opp directory */
> + d = debugfs_create_dir(name, pdentry);
> + if (!d)
> + return false;
> +
> + if (!debugfs_create_ulong("u_volt_target", S_IRUGO, d,
> + &opp->supplies[i].u_volt))
> + return false;
> +
> + if (!debugfs_create_ulong("u_volt_min", S_IRUGO, d,
> + &opp->supplies[i].u_volt_min))
> + return false;
> +
> + if (!debugfs_create_ulong("u_volt_max", S_IRUGO, d,
> + &opp->supplies[i].u_volt_max))
> + return false;
> +
> + if (!debugfs_create_ulong("u_amp", S_IRUGO, d,
> + &opp->supplies[i].u_amp))
> + return false;
> + } while (++i < opp_table->regulator_count);
> +
> + return true;
> +}
> +
> int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
> {
> struct dentry *pdentry = opp_table->dentry;
> diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
> index b7fcd0a1b58b..c857fb07a5bc 100644
> --- a/drivers/base/power/opp/of.c
> +++ b/drivers/base/power/opp/of.c
> @@ -105,12 +106,13 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
> static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
> struct opp_table *opp_table)
> {
> - u32 microvolt[3] = {0};
> - u32 val;
> - int count, ret;
> + u32 *microvolt, *microamp = NULL;
> + int supplies, vcount, icount, ret, i, j;
> struct property *prop = NULL;
> char name[NAME_MAX];
>
> + supplies = opp_table->regulator_count ? opp_table->regulator_count : 1;
We can't have regulator_count == 1 by default?
> +
> /* Search for "opp-microvolt-<name>" */
> if (opp_table->prop_name) {
> snprintf(name, sizeof(name), "opp-microvolt-%s",
> @@ -155,7 +155,8 @@ enum opp_table_access {
> * @supported_hw_count: Number of elements in supported_hw array.
> * @prop_name: A name to postfix to many DT properties, while parsing them.
> * @clk: Device's clock handle
> - * @regulator: Supply regulator
> + * @regulators: Supply regulators
> + * @regulator_count: Number of Power Supply regulators
Lowercase Power Supply please.
> * @dentry: debugfs dentry pointer of the real device directory (not links).
> * @dentry_name: Name of the real dentry.
> *
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index 5c07ae05d69a..15cb26118dc7 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -186,7 +186,10 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> */
> name = find_supply_name(cpu_dev);
> if (name) {
> - ret = dev_pm_opp_set_regulator(cpu_dev, name);
> + const char *names[] = {name};
> +
> + ret = dev_pm_opp_set_regulators(cpu_dev, names,
We can't just do &names instead here? Hmm...
> + ARRAY_SIZE(names));
> if (ret) {
> dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
> policy->cpu, ret);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-10-26 08:10 +0200 |
| Subject | Re: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators |
| Message-ID | <swpzr-889-5@gated-at.bofh.it> |
| In reply to | #1508481 |
On 25-10-16, 09:49, Stephen Boyd wrote:
> On 10/20, Viresh Kumar wrote:
> > diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> > index 37fad2eb0f47..45c70ce07864 100644
> > --- a/drivers/base/power/opp/core.c
> > +++ b/drivers/base/power/opp/core.c
> > @@ -235,21 +237,44 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
> > return 0;
> > }
> >
> > - reg = opp_table->regulator;
> > - if (IS_ERR(reg)) {
> > + count = opp_table->regulator_count;
> > +
> > + if (!count) {
> > /* Regulator may not be required for device */
> > rcu_read_unlock();
> > return 0;
> > }
> >
> > - list_for_each_entry_rcu(opp, &opp_table->opp_list, node) {
> > - if (!opp->available)
> > - continue;
> > + size = count * sizeof(*regulators);
> > + regulators = kmemdup(opp_table->regulators, size, GFP_KERNEL);
>
> How do we allocate under RCU? Doesn't that spit out big warnings?
That doesn't. I even tried enabling several locking debug config options.
> > + if (!regulators) {
> > + rcu_read_unlock();
> > + return 0;
> > + }
> > +
> > + min_uV = kmalloc(count * (sizeof(*min_uV) + sizeof(*max_uV)),
>
> Do we imagine min_uV is going to be a different size from max_uV?
> It may be better to have a struct for min/max pair and then
> stride them. Then the kmalloc() can become a kmalloc_array().
done.
> > - *opp_table = _add_opp_table(dev);
> > - if (!*opp_table) {
> > - kfree(opp);
> > + /* allocate new OPP node + and supplies structures */
> > + opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL);
> > + if (!opp) {
> > + kfree(table);
> > return NULL;
> > }
> >
> > + opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
>
> So put the supplies at the end of the OPP structure as an empty
> array?
Yes. Added a comment to clarify as well.
> > -int dev_pm_opp_set_regulator(struct device *dev, const char *name)
> > +int dev_pm_opp_set_regulators(struct device *dev, const char *names[],
>
> Make names a const char * const *?
Done.
> I seem to recall arrays as
> function arguments has some problem but my C mastery is failing
> right now.
I am not sure why it would be a problem, and of course what gets passed is the
address and not the array.
> > + for (i = 0; i < count; i++) {
> > + reg = regulator_get_optional(dev, names[i]);
> > + pr_info("%s: %d: %p: %s\n", __func__, __LINE__, reg, names[i]);
>
> Debug noise?
Yes.
> > +static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
> > + struct opp_table *opp_table,
> > + struct dentry *pdentry)
> > +{
> > + struct dentry *d;
> > + int i = 0;
> > + char name[] = "supply-X"; /* support only 0-9 supplies */
>
> But we don't verify that's the case? Why not use kasprintf() and
> free() and then there isn't any limit?
Done.
> > diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
> > index b7fcd0a1b58b..c857fb07a5bc 100644
> > --- a/drivers/base/power/opp/of.c
> > +++ b/drivers/base/power/opp/of.c
> > @@ -105,12 +106,13 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
> > static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
> > struct opp_table *opp_table)
> > {
> > - u32 microvolt[3] = {0};
> > - u32 val;
> > - int count, ret;
> > + u32 *microvolt, *microamp = NULL;
> > + int supplies, vcount, icount, ret, i, j;
> > struct property *prop = NULL;
> > char name[NAME_MAX];
> >
> > + supplies = opp_table->regulator_count ? opp_table->regulator_count : 1;
>
> We can't have regulator_count == 1 by default?
It is used at various places to distinguish if regulators are set by platform
code or not. The OPP core can still be used just for DT data, i.e. no opp-set.
And so it is important to support cases where the regulators aren't set.
> > @@ -155,7 +155,8 @@ enum opp_table_access {
> > * @supported_hw_count: Number of elements in supported_hw array.
> > * @prop_name: A name to postfix to many DT properties, while parsing them.
> > * @clk: Device's clock handle
> > - * @regulator: Supply regulator
> > + * @regulators: Supply regulators
> > + * @regulator_count: Number of Power Supply regulators
>
> Lowercase Power Supply please.
Done.
> > * @dentry: debugfs dentry pointer of the real device directory (not links).
> > * @dentry_name: Name of the real dentry.
> > *
> > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> > index 5c07ae05d69a..15cb26118dc7 100644
> > --- a/drivers/cpufreq/cpufreq-dt.c
> > +++ b/drivers/cpufreq/cpufreq-dt.c
> > @@ -186,7 +186,10 @@ static int cpufreq_init(struct cpufreq_policy *policy)
> > */
> > name = find_supply_name(cpu_dev);
> > if (name) {
> > - ret = dev_pm_opp_set_regulator(cpu_dev, name);
> > + const char *names[] = {name};
> > +
> > + ret = dev_pm_opp_set_regulators(cpu_dev, names,
>
> We can't just do &names instead here? Hmm...
I don't think so. You sure we can do it ?
--
viresh
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web