Messages in this thread | | | From | Ulf Hansson <> | Date | Thu, 19 Oct 2023 13:16:14 +0200 | Subject | Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case |
| |
On Thu, 19 Oct 2023 at 12:22, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > There are two genpd (as required-opp) cases that we need to handle, > devices with a single genpd and ones with multiple genpds. > > The multiple genpds case is clear, where the OPP core calls > dev_pm_domain_attach_by_name() for them and uses the virtual devices > returned by this helper to call dev_pm_domain_set_performance_state() > later to change the performance state. > > The single genpd case however requires special handling as we need to > use the same `dev` structure (instead of a virtual one provided by genpd > core) for setting the performance state via > dev_pm_domain_set_performance_state(). > > As we move towards more generic code to take care of the required OPPs, > where we will recursively call dev_pm_opp_set_opp() for all the required > OPPs, the above special case becomes a problem. > > Eventually we want to handle all performance state changes via > _set_opp_level(), so lets move the single genpd case to that right away. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/opp/core.c | 6 ++++-- > drivers/opp/of.c | 25 ++++++++++++++++++++++--- > 2 files changed, 26 insertions(+), 5 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 84f345c69ea5..aab8c8e79146 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1074,10 +1074,12 @@ static int _opp_set_required_opps_generic(struct device *dev, > static int _opp_set_required_opps_genpd(struct device *dev, > struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down) > { > - struct device **genpd_virt_devs = > - opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev; > + struct device **genpd_virt_devs = opp_table->genpd_virt_devs; > int index, target, delta, ret; > > + if (!genpd_virt_devs) > + return 0; > + > /* Scaling up? Set required OPPs in normal order, else reverse */ > if (!scaling_down) { > index = 0; > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index 81fa27599d58..e056f31a48b5 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -296,7 +296,7 @@ void _of_clear_opp(struct opp_table *opp_table, struct dev_pm_opp *opp) > of_node_put(opp->np); > } > > -static int _link_required_opps(struct dev_pm_opp *opp, > +static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_table, > struct opp_table *required_table, int index) > { > struct device_node *np; > @@ -314,6 +314,25 @@ static int _link_required_opps(struct dev_pm_opp *opp, > return -ENODEV; > } > > + /* > + * There are two genpd (as required-opp) cases that we need to handle, > + * devices with a single genpd and ones with multiple genpds. > + * > + * The single genpd case requires special handling as we need to use the > + * same `dev` structure (instead of a virtual one provided by genpd > + * core) for setting the performance state. Lets treat this as a case > + * where the OPP's level is directly available without required genpd > + * link in the DT. > + * > + * Just update the `level` with the right value, which > + * 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) { > + if (!WARN_ON(opp->level))
Hmm. Doesn't this introduce an unnecessary limitation?
An opp node that has a required-opps phande, may have "opp-hz", "opp-microvolt", etc. Why would we not allow the "opp-level" to be used too?
> + opp->level = opp->required_opps[0]->level; > + } > + > return 0; > } > > @@ -338,7 +357,7 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table, > if (IS_ERR_OR_NULL(required_table)) > continue; > > - ret = _link_required_opps(opp, required_table, i); > + ret = _link_required_opps(opp, opp_table, required_table, i); > if (ret) > goto free_required_opps; > } > @@ -359,7 +378,7 @@ static int lazy_link_required_opps(struct opp_table *opp_table, > int ret; > > list_for_each_entry(opp, &opp_table->opp_list, node) { > - ret = _link_required_opps(opp, new_table, index); > + ret = _link_required_opps(opp, opp_table, new_table, index); > if (ret) > return ret; > }
Kind regards Uffe
| |