lkml.org 
[lkml]   [2016]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V2 5/8] PM / OPP: Add infrastructure to manage multiple regulators
    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

    \
     
     \ /
      Last update: 2016-10-25 18:50    [W:4.531 / U:0.368 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site