Messages in this thread Patch in this message | | | From | Ulf Hansson <> | Date | Wed, 25 Oct 2023 15:51:44 +0200 | Subject | Re: [PATCH 2/2] OPP: Call dev_pm_opp_set_opp() for required OPPs |
| |
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
| |