| Message ID | 6de4fcb5bb943a131d0cdf0a858bd35af02a2f88.1697710527.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | OPP: Simplify required-opp handling | expand |
On Thu, Oct 19, 2023 at 03:52:01PM +0530, Viresh Kumar wrote: > Configuring the required OPP was never properly implemented, we just > took an exception for genpds and configured them directly, while leaving > out all other required OPP types. > > Now that a standard call to dev_pm_opp_set_opp() takes care of > configuring the opp->level too, the special handling for genpds can be > avoided by simply calling dev_pm_opp_set_opp() for the required OPPs, > which shall eventually configure the corresponding level for genpds. > > This also makes it possible for us to configure other type of required > OPPs (no concrete users yet though), via the same path. This is how > other frameworks take care of parent nodes, like clock, regulators, etc, > where we recursively call the same helper. > > In order to call dev_pm_opp_set_opp() for the virtual genpd devices, > they must share the OPP table of the genpd. Call _add_opp_dev() for them > to get that done. > > This commit also extends the struct dev_pm_opp_config to pass required > devices, for non-genpd cases, which can be used to call > dev_pm_opp_set_opp() for the non-genpd required devices. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/opp/core.c | 144 ++++++++++++++++++----------------------- > drivers/opp/of.c | 12 ++-- > drivers/opp/opp.h | 8 +-- > include/linux/pm_opp.h | 7 +- > 4 files changed, 76 insertions(+), 95 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index aab8c8e79146..056b51abc501 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1046,42 +1046,19 @@ static int _set_opp_bw(const struct opp_table *opp_table, > return 0; > } > > -static int _set_performance_state(struct device *dev, struct device *pd_dev, > - struct dev_pm_opp *opp, int i) > -{ > - unsigned int pstate = likely(opp) ? opp->required_opps[i]->level: 0; Unfortunately this patch breaks scaling the performance state of the virtual genpd devices completely. As far as I can tell it just keeps setting level = 0 for every OPP switch (this is on MSM8909 with [1], a single "perf" power domain attached to the CPU): [ 457.252255] cpu cpu0: _set_opp: switching OPP: Freq 998400000 -> 799999804 Hz, Level 0 -> 0, Bw 3200000 -> 1600000 [ 457.253739] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 998400000 -> 799999804 Hz, Level 0 -> 0, Bw 3200000 -> 1600000 [ 457.304581] cpu cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000 [ 457.306091] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000 [ 457.323489] cpu cpu0: _set_opp: switching OPP: Freq 533333202 -> 399999902 Hz, Level 0 -> 0, Bw 1600000 -> 800000 [ 457.352792] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 533333202 -> 399999902 Hz, Level 0 -> 0, Bw 1600000 -> 800000 [ 457.353056] cpu cpu0: _set_opp: switching OPP: Freq 399999902 -> 199999951 Hz, Level 0 -> 0, Bw 800000 -> 800000 [ 457.355285] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 399999902 -> 199999951 Hz, Level 0 -> 0, Bw 800000 -> 800000 Where do you handle setting the pstate specified in the "required-opps" of the OPP table with this patch? I've looked at your changes for some time but must admit I haven't really understood how it is supposed to work. :-) Thanks, Stephan [1]: https://lore.kernel.org/linux-arm-msm/20231018-msm8909-cpufreq-v2-0-0962df95f654@kernkonzept.com/ > - int ret; > - > - if (!pd_dev) > - return 0; > - > - ret = dev_pm_domain_set_performance_state(pd_dev, pstate); > - if (ret) { > - dev_err(dev, "Failed to set performance state of %s: %d (%d)\n", > - dev_name(pd_dev), pstate, ret); > - } > - > - return ret; > -} > - > -static int _opp_set_required_opps_generic(struct device *dev, > - struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down) > -{ > - dev_err(dev, "setting required-opps isn't supported for non-genpd devices\n"); > - return -ENOENT; > -} > - > -static int _opp_set_required_opps_genpd(struct device *dev, > - struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down) > +/* This is only called for PM domain for now */ > +static int _set_required_opps(struct device *dev, struct opp_table *opp_table, > + struct dev_pm_opp *opp, bool up) > { > - struct device **genpd_virt_devs = opp_table->genpd_virt_devs; > + struct device **devs = opp_table->required_devs; > int index, target, delta, ret; > > - if (!genpd_virt_devs) > - return 0; > + /* required-opps not fully initialized yet */ > + if (lazy_linking_pending(opp_table)) > + return -EBUSY; > > /* Scaling up? Set required OPPs in normal order, else reverse */ > - if (!scaling_down) { > + if (up) { > index = 0; > target = opp_table->required_opp_count; > delta = 1; > @@ -1092,9 +1069,11 @@ static int _opp_set_required_opps_genpd(struct device *dev, > } > > while (index != target) { > - ret = _set_performance_state(dev, genpd_virt_devs[index], opp, index); > - if (ret) > - return ret; > + if (devs[index]) { > + ret = dev_pm_opp_set_opp(devs[index], opp); > + if (ret) > + return ret; > + } > > index += delta; > } > @@ -1102,34 +1081,6 @@ static int _opp_set_required_opps_genpd(struct device *dev, > return 0; > } > > -/* This is only called for PM domain for now */ > -static int _set_required_opps(struct device *dev, struct opp_table *opp_table, > - struct dev_pm_opp *opp, bool up) > -{ > - /* required-opps not fully initialized yet */ > - if (lazy_linking_pending(opp_table)) > - return -EBUSY; > - > - if (opp_table->set_required_opps) > - return opp_table->set_required_opps(dev, opp_table, opp, up); > - > - return 0; > -} > - > -/* Update set_required_opps handler */ > -void _update_set_required_opps(struct opp_table *opp_table) > -{ > - /* Already set */ > - if (opp_table->set_required_opps) > - return; > - > - /* All required OPPs will belong to genpd or none */ > - if (opp_table->required_opp_tables[0]->is_genpd) > - opp_table->set_required_opps = _opp_set_required_opps_genpd; > - else > - opp_table->set_required_opps = _opp_set_required_opps_generic; > -} > - > static int _set_opp_level(struct device *dev, struct opp_table *opp_table, > struct dev_pm_opp *opp) > { > @@ -2390,19 +2341,13 @@ static void _opp_detach_genpd(struct opp_table *opp_table) > { > int index; > > - if (!opp_table->genpd_virt_devs) > - return; > - > for (index = 0; index < opp_table->required_opp_count; index++) { > - if (!opp_table->genpd_virt_devs[index]) > + if (!opp_table->required_devs[index]) > continue; > > - dev_pm_domain_detach(opp_table->genpd_virt_devs[index], false); > - opp_table->genpd_virt_devs[index] = NULL; > + dev_pm_domain_detach(opp_table->required_devs[index], false); > + opp_table->required_devs[index] = NULL; > } > - > - kfree(opp_table->genpd_virt_devs); > - opp_table->genpd_virt_devs = NULL; > } > > /* > @@ -2429,15 +2374,10 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, > int index = 0, ret = -EINVAL; > const char * const *name = names; > > - if (opp_table->genpd_virt_devs) > + /* Checking only the first one is enough ? */ > + if (opp_table->required_devs[0]) > return 0; > > - opp_table->genpd_virt_devs = kcalloc(opp_table->required_opp_count, > - sizeof(*opp_table->genpd_virt_devs), > - GFP_KERNEL); > - if (!opp_table->genpd_virt_devs) > - return -ENOMEM; > - > while (*name) { > if (index >= opp_table->required_opp_count) { > dev_err(dev, "Index can't be greater than required-opp-count - 1, %s (%d : %d)\n", > @@ -2452,13 +2392,25 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, > goto err; > } > > - opp_table->genpd_virt_devs[index] = virt_dev; > + /* > + * Add the virtual genpd device as a user of the OPP table, so > + * we can call dev_pm_opp_set_opp() on it directly. > + * > + * This will be automatically removed when the OPP table is > + * removed, don't need to handle that here. > + */ > + if (!_add_opp_dev(virt_dev, opp_table->required_opp_tables[index])) { > + ret = -ENOMEM; > + goto err; > + } > + > + opp_table->required_devs[index] = virt_dev; > index++; > name++; > } > > if (virt_devs) > - *virt_devs = opp_table->genpd_virt_devs; > + *virt_devs = opp_table->required_devs; > > return 0; > > @@ -2468,10 +2420,34 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, > > } > > +static void _opp_set_required_devs(struct opp_table *opp_table, > + struct device **required_devs) > +{ > + int i; > + > + /* Another CPU that shares the OPP table has set the required devs ? */ > + if (opp_table->required_devs[0]) > + return; > + > + for (i = 0; i < opp_table->required_opp_count; i++) > + opp_table->required_devs[i] = required_devs[i]; > +} > + > +static void _opp_put_required_devs(struct opp_table *opp_table) > +{ > + int i; > + > + for (i = 0; i < opp_table->required_opp_count; i++) > + opp_table->required_devs[i] = NULL; > +} > + > static void _opp_clear_config(struct opp_config_data *data) > { > - if (data->flags & OPP_CONFIG_GENPD) > + if (data->flags & OPP_CONFIG_REQUIRED_DEVS) > + _opp_put_required_devs(data->opp_table); > + else if (data->flags & OPP_CONFIG_GENPD) > _opp_detach_genpd(data->opp_table); > + > if (data->flags & OPP_CONFIG_REGULATOR) > _opp_put_regulators(data->opp_table); > if (data->flags & OPP_CONFIG_SUPPORTED_HW) > @@ -2585,12 +2561,18 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config) > > /* Attach genpds */ > if (config->genpd_names) { > + if (config->required_devs) > + goto err; > + > ret = _opp_attach_genpd(opp_table, dev, config->genpd_names, > config->virt_devs); > if (ret) > goto err; > > data->flags |= OPP_CONFIG_GENPD; > + } else if (config->required_devs) { > + _opp_set_required_devs(opp_table, config->required_devs); > + data->flags |= OPP_CONFIG_REQUIRED_DEVS; > } > > ret = xa_alloc(&opp_configs, &id, data, XA_LIMIT(1, INT_MAX), > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index e056f31a48b5..27659d23d54d 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -165,7 +165,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, > struct opp_table **required_opp_tables; > struct device_node *required_np, *np; > bool lazy = false; > - int count, i; > + int count, i, size; > > /* Traversing the first OPP node is all we need */ > np = of_get_next_available_child(opp_np, NULL); > @@ -179,12 +179,13 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, > if (count <= 0) > goto put_np; > > - required_opp_tables = kcalloc(count, sizeof(*required_opp_tables), > - GFP_KERNEL); > + size = sizeof(*required_opp_tables) + sizeof(*opp_table->required_devs); > + required_opp_tables = kcalloc(count, size, GFP_KERNEL); > if (!required_opp_tables) > goto put_np; > > opp_table->required_opp_tables = required_opp_tables; > + opp_table->required_devs = (void *)(required_opp_tables + count); > opp_table->required_opp_count = count; > > for (i = 0; i < count; i++) { > @@ -208,8 +209,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, > mutex_lock(&opp_table_lock); > list_add(&opp_table->lazy, &lazy_opp_tables); > mutex_unlock(&opp_table_lock); > - } else { > - _update_set_required_opps(opp_table); > } > > goto put_np; > @@ -328,7 +327,7 @@ static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_tab > * dev_pm_opp_set_opp() will take care of in the normal path itself. > */ > if (required_table->is_genpd && opp_table->required_opp_count == 1 && > - !opp_table->genpd_virt_devs) { > + !opp_table->required_devs[0]) { > if (!WARN_ON(opp->level)) > opp->level = opp->required_opps[0]->level; > } > @@ -441,7 +440,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table) > > /* All required opp-tables found, remove from lazy list */ > if (!lazy) { > - _update_set_required_opps(opp_table); > list_del_init(&opp_table->lazy); > > list_for_each_entry(opp, &opp_table->opp_list, node) > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h > index 08366f90f16b..23dcb2fbf8c3 100644 > --- a/drivers/opp/opp.h > +++ b/drivers/opp/opp.h > @@ -35,6 +35,7 @@ extern struct list_head opp_tables; > #define OPP_CONFIG_PROP_NAME BIT(3) > #define OPP_CONFIG_SUPPORTED_HW BIT(4) > #define OPP_CONFIG_GENPD BIT(5) > +#define OPP_CONFIG_REQUIRED_DEVS BIT(6) > > /** > * struct opp_config_data - data for set config operations > @@ -160,9 +161,9 @@ enum opp_table_access { > * @rate_clk_single: Currently configured frequency for single clk. > * @current_opp: Currently configured OPP for the table. > * @suspend_opp: Pointer to OPP to be used during device suspend. > - * @genpd_virt_devs: List of virtual devices for multiple genpd support. > * @required_opp_tables: List of device OPP tables that are required by OPPs in > * this table. > + * @required_devs: List of devices for required OPP tables. > * @required_opp_count: Number of required devices. > * @supported_hw: Array of version number to support. > * @supported_hw_count: Number of elements in supported_hw array. > @@ -180,7 +181,6 @@ enum opp_table_access { > * @path_count: Number of interconnect paths > * @enabled: Set to true if the device's resources are enabled/configured. > * @is_genpd: Marks if the OPP table belongs to a genpd. > - * @set_required_opps: Helper responsible to set required OPPs. > * @dentry: debugfs dentry pointer of the real device directory (not links). > * @dentry_name: Name of the real dentry. > * > @@ -211,8 +211,8 @@ struct opp_table { > struct dev_pm_opp *current_opp; > struct dev_pm_opp *suspend_opp; > > - struct device **genpd_virt_devs; > struct opp_table **required_opp_tables; > + struct device **required_devs; > unsigned int required_opp_count; > > unsigned int *supported_hw; > @@ -229,8 +229,6 @@ struct opp_table { > unsigned int path_count; > bool enabled; > bool is_genpd; > - int (*set_required_opps)(struct device *dev, > - struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down); > > #ifdef CONFIG_DEBUG_FS > struct dentry *dentry; > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index ccd97bcef269..25f7bcbea7ab 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -74,8 +74,10 @@ typedef int (*config_clks_t)(struct device *dev, struct opp_table *opp_table, > * @supported_hw_count: Number of elements in the array. > * @regulator_names: Array of pointers to the names of the regulator, NULL terminated. > * @genpd_names: Null terminated array of pointers containing names of genpd to > - * attach. > - * @virt_devs: Pointer to return the array of virtual devices. > + * attach. Mutually exclusive with required_devs. > + * @virt_devs: Pointer to return the array of genpd virtual devices. Mutually > + * exclusive with required_devs. > + * @required_devs: Required OPP devices. Mutually exclusive with genpd_names/virt_devs. > * > * This structure contains platform specific OPP configurations for the device. > */ > @@ -90,6 +92,7 @@ struct dev_pm_opp_config { > const char * const *regulator_names; > const char * const *genpd_names; > struct device ***virt_devs; > + struct device **required_devs; > }; > > /** > -- > 2.31.1.272.g89b43f80a514 >
Thanks a lot for taking this up, really appreciate it Stephan. On 24-10-23, 13:18, Stephan Gerhold wrote: > Unfortunately this patch breaks scaling the performance state of the > virtual genpd devices completely. As far as I can tell it just keeps > setting level = 0 for every OPP switch (this is on MSM8909 with [1], > a single "perf" power domain attached to the CPU): > > [ 457.252255] cpu cpu0: _set_opp: switching OPP: Freq 998400000 -> 799999804 Hz, Level 0 -> 0, Bw 3200000 -> 1600000 > [ 457.253739] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 998400000 -> 799999804 Hz, Level 0 -> 0, Bw 3200000 -> 1600000 The thing I was more worried about worked fine it seems (recursively calling dev_pm_opp_set_opp() i.e.). > [ 457.304581] cpu cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000 > [ 457.306091] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000 > [ 457.323489] cpu cpu0: _set_opp: switching OPP: Freq 533333202 -> 399999902 Hz, Level 0 -> 0, Bw 1600000 -> 800000 > [ 457.352792] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 533333202 -> 399999902 Hz, Level 0 -> 0, Bw 1600000 -> 800000 > [ 457.353056] cpu cpu0: _set_opp: switching OPP: Freq 399999902 -> 199999951 Hz, Level 0 -> 0, Bw 800000 -> 800000 > [ 457.355285] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 399999902 -> 199999951 Hz, Level 0 -> 0, Bw 800000 -> 800000 > > Where do you handle setting the pstate specified in the "required-opps" > of the OPP table with this patch? I've looked at your changes for some > time but must admit I haven't really understood how it is supposed to > work. :-) Thanks for the debug print, they helped me find the issue. > [ 457.304581] cpu cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000 > [ 457.306091] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000 The second line shouldn't have had freq/bw/etc, but just level. Hopefully this will fix it. Pushed to my branch too. Thanks. Please try again. diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 056b51abc501..cb2b353129c6 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1070,7 +1070,7 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table, while (index != target) { if (devs[index]) { - ret = dev_pm_opp_set_opp(devs[index], opp); + ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]); if (ret) return ret; }
On Wed, Oct 25, 2023 at 01:06:34PM +0530, Viresh Kumar wrote: > Thanks a lot for taking this up, really appreciate it Stephan. > > On 24-10-23, 13:18, Stephan Gerhold wrote: > > Unfortunately this patch breaks scaling the performance state of the > > virtual genpd devices completely. As far as I can tell it just keeps > > setting level = 0 for every OPP switch (this is on MSM8909 with [1], > > a single "perf" power domain attached to the CPU): > > > > [ 457.252255] cpu cpu0: _set_opp: switching OPP: Freq 998400000 -> 799999804 Hz, Level 0 -> 0, Bw 3200000 -> 1600000 > > [ 457.253739] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 998400000 -> 799999804 Hz, Level 0 -> 0, Bw 3200000 -> 1600000 > > The thing I was more worried about worked fine it seems (recursively calling > dev_pm_opp_set_opp() i.e.). > > > [ 457.304581] cpu cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000 > > [ 457.306091] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000 > > [ 457.323489] cpu cpu0: _set_opp: switching OPP: Freq 533333202 -> 399999902 Hz, Level 0 -> 0, Bw 1600000 -> 800000 > > [ 457.352792] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 533333202 -> 399999902 Hz, Level 0 -> 0, Bw 1600000 -> 800000 > > [ 457.353056] cpu cpu0: _set_opp: switching OPP: Freq 399999902 -> 199999951 Hz, Level 0 -> 0, Bw 800000 -> 800000 > > [ 457.355285] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 399999902 -> 199999951 Hz, Level 0 -> 0, Bw 800000 -> 800000 > > > > Where do you handle setting the pstate specified in the "required-opps" > > of the OPP table with this patch? I've looked at your changes for some > > time but must admit I haven't really understood how it is supposed to > > work. :-) > > Thanks for the debug print, they helped me find the issue. > > > [ 457.304581] cpu cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000 > > [ 457.306091] genpd genpd:1:cpu0: _set_opp: switching OPP: Freq 799999804 -> 533333202 Hz, Level 0 -> 0, Bw 1600000 -> 1600000 > > The second line shouldn't have had freq/bw/etc, but just level. Hopefully this > will fix it. Pushed to my branch too. Thanks. Please try again. > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 056b51abc501..cb2b353129c6 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1070,7 +1070,7 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table, > > while (index != target) { > if (devs[index]) { > - ret = dev_pm_opp_set_opp(devs[index], opp); > + ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]); > if (ret) > return ret; > } > Thanks, this seems to work fine. I found another small problem: In my OPP setup for MSM8916, some of the required-opps point to an OPP with "opp-level = <0>" (see [1], the <&rpmpd_opp_none> in the cpu-opp-table). This is because the vote for the "CX" domain is for the CPU PLL clock source, which is only used for the higher CPU frequencies (>= 998.4 MHz). With the previous code you made it possible for me to vote for opp-level = <0> in commit a5663c9b1e31 ("opp: Allow opp-level to be set to 0", discussion in [2]). I think now it's broken because the _set_opp_level() added by Uffe checks for if (!opp->level) as a sign that there is no opp-level defined at all. Based on my longer discussion with Uffe recently [3] it's not entirely clear yet if I will still have the reference to &rpmpd_opp_none in the future. Alternatively, we discussed describing this differently, e.g. as a parent power domain (which would bypass most of the OPP code), or moving it directly to an OPP table of CPU PLL device node (which would only describe the actual "active" required-opps). I'm not sure if anyone else has a reasonable use case for pointing to a required-opp with opp-level = <0>, so we could potentially also postpone solving this to later. Thanks, Stephan [1]: https://github.com/msm8916-mainline/linux/commit/8880f39108206d7a60a0a8351c0373bddf58657c [2]: https://lore.kernel.org/linux-pm/20200911092538.jexqm6joww67d4yv@vireshk-i7/ [3]: https://lore.kernel.org/linux-arm-msm/ZQGqfMigCFZP_HLA@gerhold.net/
On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > Configuring the required OPP was never properly implemented, we just > took an exception for genpds and configured them directly, while leaving > out all other required OPP types. > > Now that a standard call to dev_pm_opp_set_opp() takes care of > configuring the opp->level too, the special handling for genpds can be > avoided by simply calling dev_pm_opp_set_opp() for the required OPPs, > which shall eventually configure the corresponding level for genpds. > > This also makes it possible for us to configure other type of required > OPPs (no concrete users yet though), via the same path. This is how > other frameworks take care of parent nodes, like clock, regulators, etc, > where we recursively call the same helper. > > In order to call dev_pm_opp_set_opp() for the virtual genpd devices, > they must share the OPP table of the genpd. Call _add_opp_dev() for them > to get that done. > > This commit also extends the struct dev_pm_opp_config to pass required > devices, for non-genpd cases, which can be used to call > dev_pm_opp_set_opp() for the non-genpd required devices. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/opp/core.c | 144 ++++++++++++++++++----------------------- > drivers/opp/of.c | 12 ++-- > drivers/opp/opp.h | 8 +-- > include/linux/pm_opp.h | 7 +- > 4 files changed, 76 insertions(+), 95 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index aab8c8e79146..056b51abc501 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c [...] > -static int _opp_set_required_opps_genpd(struct device *dev, > - struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down) > +/* This is only called for PM domain for now */ > +static int _set_required_opps(struct device *dev, struct opp_table *opp_table, > + struct dev_pm_opp *opp, bool up) > { > - struct device **genpd_virt_devs = opp_table->genpd_virt_devs; > + struct device **devs = opp_table->required_devs; > int index, target, delta, ret; > > - if (!genpd_virt_devs) > - return 0; Rather than continue the path below, wouldn't it be better to return 0 "if (!devs)" here? If I understand correctly, the code below does manage this condition, so it's not strictly needed though. > + /* required-opps not fully initialized yet */ > + if (lazy_linking_pending(opp_table)) > + return -EBUSY; > > /* Scaling up? Set required OPPs in normal order, else reverse */ > - if (!scaling_down) { > + if (up) { > index = 0; > target = opp_table->required_opp_count; > delta = 1; > @@ -1092,9 +1069,11 @@ static int _opp_set_required_opps_genpd(struct device *dev, > } > > while (index != target) { > - ret = _set_performance_state(dev, genpd_virt_devs[index], opp, index); > - if (ret) > - return ret; > + if (devs[index]) { > + ret = dev_pm_opp_set_opp(devs[index], opp); > + if (ret) > + return ret; > + } > > index += delta; > } [...] > > /* > @@ -2429,15 +2374,10 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, > int index = 0, ret = -EINVAL; > const char * const *name = names; > > - if (opp_table->genpd_virt_devs) > + /* Checking only the first one is enough ? */ > + if (opp_table->required_devs[0]) The allocation of opp_table->required_devs is being done from _opp_table_alloc_required_tables(), which doesn't necessarily allocate/assign the data for it. Maybe check "opp_table->required_devs" instead, to make that clear? > return 0; > > - opp_table->genpd_virt_devs = kcalloc(opp_table->required_opp_count, > - sizeof(*opp_table->genpd_virt_devs), > - GFP_KERNEL); > - if (!opp_table->genpd_virt_devs) > - return -ENOMEM; > - > while (*name) { > if (index >= opp_table->required_opp_count) { > dev_err(dev, "Index can't be greater than required-opp-count - 1, %s (%d : %d)\n", > @@ -2452,13 +2392,25 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, > goto err; > } > > - opp_table->genpd_virt_devs[index] = virt_dev; > + /* > + * Add the virtual genpd device as a user of the OPP table, so > + * we can call dev_pm_opp_set_opp() on it directly. > + * > + * This will be automatically removed when the OPP table is > + * removed, don't need to handle that here. > + */ > + if (!_add_opp_dev(virt_dev, opp_table->required_opp_tables[index])) { > + ret = -ENOMEM; > + goto err; > + } > + > + opp_table->required_devs[index] = virt_dev; > index++; > name++; > } > > if (virt_devs) > - *virt_devs = opp_table->genpd_virt_devs; > + *virt_devs = opp_table->required_devs; > > return 0; > > @@ -2468,10 +2420,34 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, > > } > > +static void _opp_set_required_devs(struct opp_table *opp_table, > + struct device **required_devs) > +{ > + int i; > + > + /* Another CPU that shares the OPP table has set the required devs ? */ Not sure I fully understand the above comment. Is this the only relevant use-case or could there be others too? > + if (opp_table->required_devs[0]) Maybe check opp_table->required_devs instead? > + return; > + > + for (i = 0; i < opp_table->required_opp_count; i++) > + opp_table->required_devs[i] = required_devs[i]; To be safe, don't we need to check the in-parameter required_devs? Or we should simply rely on the callers of dev_pm_opp_set_config() to do the right thing? [...] Besides the minor things above, this looks really great to me! Feel free to add: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe
On 25-10-23, 15:51, Ulf Hansson wrote: > On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > -static int _opp_set_required_opps_genpd(struct device *dev, > > - struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down) > > +/* This is only called for PM domain for now */ > > +static int _set_required_opps(struct device *dev, struct opp_table *opp_table, > > + struct dev_pm_opp *opp, bool up) > > { > > - struct device **genpd_virt_devs = opp_table->genpd_virt_devs; > > + struct device **devs = opp_table->required_devs; > > int index, target, delta, ret; > > > > - if (!genpd_virt_devs) > > - return 0; > > Rather than continue the path below, wouldn't it be better to return 0 > "if (!devs)" here? I can add that optimization, moreover it would make the code simpler to read. > > @@ -2429,15 +2374,10 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, > > int index = 0, ret = -EINVAL; > > const char * const *name = names; > > > > - if (opp_table->genpd_virt_devs) > > + /* Checking only the first one is enough ? */ > > + if (opp_table->required_devs[0]) > > The allocation of opp_table->required_devs is being done from > _opp_table_alloc_required_tables(), which doesn't necessarily > allocate/assign the data for it. > > Maybe check "opp_table->required_devs" instead, to make that clear? Hmm, the expectation here is that if someone has called the config routine with genpd option, then required OPPs must be present and so required_devs won't be NULL. What instead we are looking to check here, and later in _opp_set_required_devs(), is if this operation is already done for a group of devices. The OPP table is shared, for example, between CPUs of the same cluster and the OPP core supports the config routine getting called for all of them, in a loop. In that case, we just increase the ref count and return without redoing stuff. That's why we were checking for genpd_virt_devs earlier. Though interfaces and things have changed several times, I may need to check it again to make sure it all works as expected. > > +static void _opp_set_required_devs(struct opp_table *opp_table, > > + struct device **required_devs) > > +{ > > + int i; > > + > > + /* Another CPU that shares the OPP table has set the required devs ? */ > > Not sure I fully understand the above comment. Is this the only > relevant use-case or could there be others too? Answered above, but I shouldn't write CPU here anymore. We support all device types now. > > + if (opp_table->required_devs[0]) > > Maybe check opp_table->required_devs instead? > > > + return; > > + > > + for (i = 0; i < opp_table->required_opp_count; i++) > > + opp_table->required_devs[i] = required_devs[i]; > > To be safe, don't we need to check the in-parameter required_devs? I left it like that intentionally, in case someone wants to skip the required dev for some required OPP, while make all others change. But maybe I will keep it simpler and check it all the time. > Or we should simply rely on the callers of dev_pm_opp_set_config() to > do the right thing? > > [...] > > Besides the minor things above, this looks really great to me! Feel free to add: Thanks. > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
On 25-10-23, 14:17, Stephan Gerhold wrote: > Thanks, this seems to work fine. Thanks a lot. > I found another small problem: In my OPP setup for MSM8916, some of the > required-opps point to an OPP with "opp-level = <0>" (see [1], the > <&rpmpd_opp_none> in the cpu-opp-table). This is because the vote for > the "CX" domain is for the CPU PLL clock source, which is only used for > the higher CPU frequencies (>= 998.4 MHz). With the previous code you > made it possible for me to vote for opp-level = <0> in commit > a5663c9b1e31 ("opp: Allow opp-level to be set to 0", discussion in [2]). > I think now it's broken because the _set_opp_level() added by Uffe > checks for if (!opp->level) as a sign that there is no opp-level defined > at all. Yes, we broke that. I think a simple fix is to initialize the level with an impossible value, like -1 and then 0 becomes valid. > Based on my longer discussion with Uffe recently [3] it's not entirely > clear yet if I will still have the reference to &rpmpd_opp_none in the > future. Alternatively, we discussed describing this differently, e.g. as > a parent power domain (which would bypass most of the OPP code), or > moving it directly to an OPP table of CPU PLL device node (which would > only describe the actual "active" required-opps). > > I'm not sure if anyone else has a reasonable use case for pointing to a > required-opp with opp-level = <0>, so we could potentially also postpone > solving this to later. I would like to fix this respectively. Thanks for bringing this up.
On Wed, 25 Oct 2023 at 17:20, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 25-10-23, 14:17, Stephan Gerhold wrote: > > Thanks, this seems to work fine. > > Thanks a lot. > > > I found another small problem: In my OPP setup for MSM8916, some of the > > required-opps point to an OPP with "opp-level = <0>" (see [1], the > > <&rpmpd_opp_none> in the cpu-opp-table). This is because the vote for > > the "CX" domain is for the CPU PLL clock source, which is only used for > > the higher CPU frequencies (>= 998.4 MHz). With the previous code you > > made it possible for me to vote for opp-level = <0> in commit > > a5663c9b1e31 ("opp: Allow opp-level to be set to 0", discussion in [2]). > > I think now it's broken because the _set_opp_level() added by Uffe > > checks for if (!opp->level) as a sign that there is no opp-level defined > > at all. I don't think my patch broke it, I just followed the similar semantics of how we treated the OPPs for the required opps. As far as I understood it, the only way we could request the performance-level to be zero, was if the consumer driver would call dev_pm_opp_set_opp(dev, NULL); So, unless I am overlooking something, things can have been screwed up earlier too? > > Yes, we broke that. I think a simple fix is to initialize the level > with an impossible value, like -1 and then 0 becomes valid. Yep, make sense, let's fix it! Are you sending a patch? [...] Kind regards Uffe
On 25-10-23, 18:03, Ulf Hansson wrote: > I don't think my patch broke it, I just followed the similar semantics > of how we treated the OPPs for the required opps. Yes, your commit didn't break it as it didn't touch the required-opps part. So it is my commit that moved over to the generic solution that breaks it. > Are you sending a patch? Yes.
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index aab8c8e79146..056b51abc501 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1046,42 +1046,19 @@ static int _set_opp_bw(const struct opp_table *opp_table, return 0; } -static int _set_performance_state(struct device *dev, struct device *pd_dev, - struct dev_pm_opp *opp, int i) -{ - unsigned int pstate = likely(opp) ? opp->required_opps[i]->level: 0; - int ret; - - if (!pd_dev) - return 0; - - ret = dev_pm_domain_set_performance_state(pd_dev, pstate); - if (ret) { - dev_err(dev, "Failed to set performance state of %s: %d (%d)\n", - dev_name(pd_dev), pstate, ret); - } - - return ret; -} - -static int _opp_set_required_opps_generic(struct device *dev, - struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down) -{ - dev_err(dev, "setting required-opps isn't supported for non-genpd devices\n"); - return -ENOENT; -} - -static int _opp_set_required_opps_genpd(struct device *dev, - struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down) +/* This is only called for PM domain for now */ +static int _set_required_opps(struct device *dev, struct opp_table *opp_table, + struct dev_pm_opp *opp, bool up) { - struct device **genpd_virt_devs = opp_table->genpd_virt_devs; + struct device **devs = opp_table->required_devs; int index, target, delta, ret; - if (!genpd_virt_devs) - return 0; + /* required-opps not fully initialized yet */ + if (lazy_linking_pending(opp_table)) + return -EBUSY; /* Scaling up? Set required OPPs in normal order, else reverse */ - if (!scaling_down) { + if (up) { index = 0; target = opp_table->required_opp_count; delta = 1; @@ -1092,9 +1069,11 @@ static int _opp_set_required_opps_genpd(struct device *dev, } while (index != target) { - ret = _set_performance_state(dev, genpd_virt_devs[index], opp, index); - if (ret) - return ret; + if (devs[index]) { + ret = dev_pm_opp_set_opp(devs[index], opp); + if (ret) + return ret; + } index += delta; } @@ -1102,34 +1081,6 @@ static int _opp_set_required_opps_genpd(struct device *dev, return 0; } -/* This is only called for PM domain for now */ -static int _set_required_opps(struct device *dev, struct opp_table *opp_table, - struct dev_pm_opp *opp, bool up) -{ - /* required-opps not fully initialized yet */ - if (lazy_linking_pending(opp_table)) - return -EBUSY; - - if (opp_table->set_required_opps) - return opp_table->set_required_opps(dev, opp_table, opp, up); - - return 0; -} - -/* Update set_required_opps handler */ -void _update_set_required_opps(struct opp_table *opp_table) -{ - /* Already set */ - if (opp_table->set_required_opps) - return; - - /* All required OPPs will belong to genpd or none */ - if (opp_table->required_opp_tables[0]->is_genpd) - opp_table->set_required_opps = _opp_set_required_opps_genpd; - else - opp_table->set_required_opps = _opp_set_required_opps_generic; -} - static int _set_opp_level(struct device *dev, struct opp_table *opp_table, struct dev_pm_opp *opp) { @@ -2390,19 +2341,13 @@ static void _opp_detach_genpd(struct opp_table *opp_table) { int index; - if (!opp_table->genpd_virt_devs) - return; - for (index = 0; index < opp_table->required_opp_count; index++) { - if (!opp_table->genpd_virt_devs[index]) + if (!opp_table->required_devs[index]) continue; - dev_pm_domain_detach(opp_table->genpd_virt_devs[index], false); - opp_table->genpd_virt_devs[index] = NULL; + dev_pm_domain_detach(opp_table->required_devs[index], false); + opp_table->required_devs[index] = NULL; } - - kfree(opp_table->genpd_virt_devs); - opp_table->genpd_virt_devs = NULL; } /* @@ -2429,15 +2374,10 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, int index = 0, ret = -EINVAL; const char * const *name = names; - if (opp_table->genpd_virt_devs) + /* Checking only the first one is enough ? */ + if (opp_table->required_devs[0]) return 0; - opp_table->genpd_virt_devs = kcalloc(opp_table->required_opp_count, - sizeof(*opp_table->genpd_virt_devs), - GFP_KERNEL); - if (!opp_table->genpd_virt_devs) - return -ENOMEM; - while (*name) { if (index >= opp_table->required_opp_count) { dev_err(dev, "Index can't be greater than required-opp-count - 1, %s (%d : %d)\n", @@ -2452,13 +2392,25 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, goto err; } - opp_table->genpd_virt_devs[index] = virt_dev; + /* + * Add the virtual genpd device as a user of the OPP table, so + * we can call dev_pm_opp_set_opp() on it directly. + * + * This will be automatically removed when the OPP table is + * removed, don't need to handle that here. + */ + if (!_add_opp_dev(virt_dev, opp_table->required_opp_tables[index])) { + ret = -ENOMEM; + goto err; + } + + opp_table->required_devs[index] = virt_dev; index++; name++; } if (virt_devs) - *virt_devs = opp_table->genpd_virt_devs; + *virt_devs = opp_table->required_devs; return 0; @@ -2468,10 +2420,34 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, } +static void _opp_set_required_devs(struct opp_table *opp_table, + struct device **required_devs) +{ + int i; + + /* Another CPU that shares the OPP table has set the required devs ? */ + if (opp_table->required_devs[0]) + return; + + for (i = 0; i < opp_table->required_opp_count; i++) + opp_table->required_devs[i] = required_devs[i]; +} + +static void _opp_put_required_devs(struct opp_table *opp_table) +{ + int i; + + for (i = 0; i < opp_table->required_opp_count; i++) + opp_table->required_devs[i] = NULL; +} + static void _opp_clear_config(struct opp_config_data *data) { - if (data->flags & OPP_CONFIG_GENPD) + if (data->flags & OPP_CONFIG_REQUIRED_DEVS) + _opp_put_required_devs(data->opp_table); + else if (data->flags & OPP_CONFIG_GENPD) _opp_detach_genpd(data->opp_table); + if (data->flags & OPP_CONFIG_REGULATOR) _opp_put_regulators(data->opp_table); if (data->flags & OPP_CONFIG_SUPPORTED_HW) @@ -2585,12 +2561,18 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config) /* Attach genpds */ if (config->genpd_names) { + if (config->required_devs) + goto err; + ret = _opp_attach_genpd(opp_table, dev, config->genpd_names, config->virt_devs); if (ret) goto err; data->flags |= OPP_CONFIG_GENPD; + } else if (config->required_devs) { + _opp_set_required_devs(opp_table, config->required_devs); + data->flags |= OPP_CONFIG_REQUIRED_DEVS; } ret = xa_alloc(&opp_configs, &id, data, XA_LIMIT(1, INT_MAX), diff --git a/drivers/opp/of.c b/drivers/opp/of.c index e056f31a48b5..27659d23d54d 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -165,7 +165,7 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, struct opp_table **required_opp_tables; struct device_node *required_np, *np; bool lazy = false; - int count, i; + int count, i, size; /* Traversing the first OPP node is all we need */ np = of_get_next_available_child(opp_np, NULL); @@ -179,12 +179,13 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, if (count <= 0) goto put_np; - required_opp_tables = kcalloc(count, sizeof(*required_opp_tables), - GFP_KERNEL); + size = sizeof(*required_opp_tables) + sizeof(*opp_table->required_devs); + required_opp_tables = kcalloc(count, size, GFP_KERNEL); if (!required_opp_tables) goto put_np; opp_table->required_opp_tables = required_opp_tables; + opp_table->required_devs = (void *)(required_opp_tables + count); opp_table->required_opp_count = count; for (i = 0; i < count; i++) { @@ -208,8 +209,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, mutex_lock(&opp_table_lock); list_add(&opp_table->lazy, &lazy_opp_tables); mutex_unlock(&opp_table_lock); - } else { - _update_set_required_opps(opp_table); } goto put_np; @@ -328,7 +327,7 @@ static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_tab * dev_pm_opp_set_opp() will take care of in the normal path itself. */ if (required_table->is_genpd && opp_table->required_opp_count == 1 && - !opp_table->genpd_virt_devs) { + !opp_table->required_devs[0]) { if (!WARN_ON(opp->level)) opp->level = opp->required_opps[0]->level; } @@ -441,7 +440,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table) /* All required opp-tables found, remove from lazy list */ if (!lazy) { - _update_set_required_opps(opp_table); list_del_init(&opp_table->lazy); list_for_each_entry(opp, &opp_table->opp_list, node) diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h index 08366f90f16b..23dcb2fbf8c3 100644 --- a/drivers/opp/opp.h +++ b/drivers/opp/opp.h @@ -35,6 +35,7 @@ extern struct list_head opp_tables; #define OPP_CONFIG_PROP_NAME BIT(3) #define OPP_CONFIG_SUPPORTED_HW BIT(4) #define OPP_CONFIG_GENPD BIT(5) +#define OPP_CONFIG_REQUIRED_DEVS BIT(6) /** * struct opp_config_data - data for set config operations @@ -160,9 +161,9 @@ enum opp_table_access { * @rate_clk_single: Currently configured frequency for single clk. * @current_opp: Currently configured OPP for the table. * @suspend_opp: Pointer to OPP to be used during device suspend. - * @genpd_virt_devs: List of virtual devices for multiple genpd support. * @required_opp_tables: List of device OPP tables that are required by OPPs in * this table. + * @required_devs: List of devices for required OPP tables. * @required_opp_count: Number of required devices. * @supported_hw: Array of version number to support. * @supported_hw_count: Number of elements in supported_hw array. @@ -180,7 +181,6 @@ enum opp_table_access { * @path_count: Number of interconnect paths * @enabled: Set to true if the device's resources are enabled/configured. * @is_genpd: Marks if the OPP table belongs to a genpd. - * @set_required_opps: Helper responsible to set required OPPs. * @dentry: debugfs dentry pointer of the real device directory (not links). * @dentry_name: Name of the real dentry. * @@ -211,8 +211,8 @@ struct opp_table { struct dev_pm_opp *current_opp; struct dev_pm_opp *suspend_opp; - struct device **genpd_virt_devs; struct opp_table **required_opp_tables; + struct device **required_devs; unsigned int required_opp_count; unsigned int *supported_hw; @@ -229,8 +229,6 @@ struct opp_table { unsigned int path_count; bool enabled; bool is_genpd; - int (*set_required_opps)(struct device *dev, - struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down); #ifdef CONFIG_DEBUG_FS struct dentry *dentry; diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index ccd97bcef269..25f7bcbea7ab 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -74,8 +74,10 @@ typedef int (*config_clks_t)(struct device *dev, struct opp_table *opp_table, * @supported_hw_count: Number of elements in the array. * @regulator_names: Array of pointers to the names of the regulator, NULL terminated. * @genpd_names: Null terminated array of pointers containing names of genpd to - * attach. - * @virt_devs: Pointer to return the array of virtual devices. + * attach. Mutually exclusive with required_devs. + * @virt_devs: Pointer to return the array of genpd virtual devices. Mutually + * exclusive with required_devs. + * @required_devs: Required OPP devices. Mutually exclusive with genpd_names/virt_devs. * * This structure contains platform specific OPP configurations for the device. */ @@ -90,6 +92,7 @@ struct dev_pm_opp_config { const char * const *regulator_names; const char * const *genpd_names; struct device ***virt_devs; + struct device **required_devs; }; /**
Configuring the required OPP was never properly implemented, we just took an exception for genpds and configured them directly, while leaving out all other required OPP types. Now that a standard call to dev_pm_opp_set_opp() takes care of configuring the opp->level too, the special handling for genpds can be avoided by simply calling dev_pm_opp_set_opp() for the required OPPs, which shall eventually configure the corresponding level for genpds. This also makes it possible for us to configure other type of required OPPs (no concrete users yet though), via the same path. This is how other frameworks take care of parent nodes, like clock, regulators, etc, where we recursively call the same helper. In order to call dev_pm_opp_set_opp() for the virtual genpd devices, they must share the OPP table of the genpd. Call _add_opp_dev() for them to get that done. This commit also extends the struct dev_pm_opp_config to pass required devices, for non-genpd cases, which can be used to call dev_pm_opp_set_opp() for the non-genpd required devices. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/opp/core.c | 144 ++++++++++++++++++----------------------- drivers/opp/of.c | 12 ++-- drivers/opp/opp.h | 8 +-- include/linux/pm_opp.h | 7 +- 4 files changed, 76 insertions(+), 95 deletions(-)