Messages in this thread | | | From | Ulf Hansson <> | Date | Wed, 22 Nov 2023 15:26:34 +0100 | Subject | Re: [PATCH V3 3/3] OPP: Don't set OPP recursively for a parent genpd |
| |
On Thu, 16 Nov 2023 at 11:43, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > Like other frameworks (clk, regulator, etc.) genpd core too takes care > of propagation to performance state to parent genpds. The OPP core > shouldn't attempt the same, or it may result in undefined behavior. > > Add checks at various places to take care of the same. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
I think this looks good to me, even if I think it's a bit tricky to consider the various scenarios. :-) Feel free to add:
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards Uffe
> --- > drivers/opp/core.c | 16 +++++++++++++++- > drivers/opp/of.c | 7 +++++-- > 2 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index e08375ed50aa..4f1ca84d9ed0 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -2392,6 +2392,12 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, > return -EINVAL; > } > > + /* Genpd core takes care of propagation to parent genpd */ > + if (opp_table->is_genpd) { > + dev_err(dev, "%s: Operation not supported for genpds\n", __func__); > + return -EOPNOTSUPP; > + } > + > /* Checking only the first one is enough ? */ > if (opp_table->required_devs[0]) > return 0; > @@ -2453,8 +2459,16 @@ static int _opp_set_required_devs(struct opp_table *opp_table, > if (opp_table->required_devs[0]) > return 0; > > - for (i = 0; i < opp_table->required_opp_count; i++) > + for (i = 0; i < opp_table->required_opp_count; i++) { > + /* Genpd core takes care of propagation to parent genpd */ > + if (required_devs[i] && opp_table->is_genpd && > + opp_table->required_opp_tables[i]->is_genpd) { > + dev_err(dev, "%s: Operation not supported for genpds\n", __func__); > + return -EOPNOTSUPP; > + } > + > opp_table->required_devs[i] = required_devs[i]; > + } > > return 0; > } > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index 5a7e294e56b7..f9f0b22bccbb 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -339,8 +339,11 @@ static int _link_required_opps(struct dev_pm_opp *opp, struct opp_table *opp_tab > */ > if (required_table->is_genpd && opp_table->required_opp_count == 1 && > !opp_table->required_devs[0]) { > - if (!WARN_ON(opp->level != OPP_LEVEL_UNSET)) > - opp->level = opp->required_opps[0]->level; > + /* Genpd core takes care of propagation to parent genpd */ > + if (!opp_table->is_genpd) { > + if (!WARN_ON(opp->level != OPP_LEVEL_UNSET)) > + opp->level = opp->required_opps[0]->level; > + } > } > > return 0; > -- > 2.31.1.272.g89b43f80a514 >
| |