| Message ID | 56b9926836652e9dbf56ca6b7628e836cddca351.1677063656.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | OPP: Simplify set_required_opp handling | expand |
On Wed, 22 Feb 2023 at 12:07, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > The required-opps configuration is closely tied to genpd and performance > states at the moment and it is not very obvious that required-opps can > live without genpds. Though we don't support configuring required-opps > for non-genpd cases currently. > > This commit aims at separating these parts, where configuring genpds > would be a special case of configuring the required-opps. > > Add a specialized callback, set_required_opps(), to the opp table and > set it to different callbacks accordingly. > > This shouldn't result in any functional changes for now. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> This looks reasonable to me, but I guess it also depends on whether you will land patch3 or not? Nevertheless, feel free to add: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/opp/core.c | 69 ++++++++++++++++++++++++++++------------------ > drivers/opp/of.c | 3 ++ > drivers/opp/opp.h | 4 +++ > 3 files changed, 49 insertions(+), 27 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 6d7016ce8c53..954c94865cf5 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -935,8 +935,8 @@ static int _set_opp_bw(const struct opp_table *opp_table, > return 0; > } > > -static int _set_required_opp(struct device *dev, struct device *pd_dev, > - struct dev_pm_opp *opp, int i) > +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]->pstate : 0; > int ret; > @@ -953,33 +953,20 @@ static int _set_required_opp(struct device *dev, struct device *pd_dev, > return ret; > } > > -/* 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) > +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) > { > - struct opp_table **required_opp_tables = opp_table->required_opp_tables; > struct device **genpd_virt_devs = > opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev; > int i, ret = 0; > > - if (!required_opp_tables) > - return 0; > - > - /* required-opps not fully initialized yet */ > - if (lazy_linking_pending(opp_table)) > - return -EBUSY; > - > - /* > - * We only support genpd's OPPs in the "required-opps" for now, as we > - * don't know much about other use cases. Error out if the required OPP > - * doesn't belong to a genpd. > - */ > - if (unlikely(!required_opp_tables[0]->is_genpd)) { > - dev_err(dev, "required-opps don't belong to a genpd\n"); > - return -ENOENT; > - } > - > /* > * Acquire genpd_virt_dev_lock to make sure we don't use a genpd_dev > * after it is freed from another thread. > @@ -987,15 +974,15 @@ static int _set_required_opps(struct device *dev, > mutex_lock(&opp_table->genpd_virt_dev_lock); > > /* Scaling up? Set required OPPs in normal order, else reverse */ > - if (up) { > + if (!scaling_down) { > for (i = 0; i < opp_table->required_opp_count; i++) { > - ret = _set_required_opp(dev, genpd_virt_devs[i], opp, i); > + ret = _set_performance_state(dev, genpd_virt_devs[i], opp, i); > if (ret) > break; > } > } else { > for (i = opp_table->required_opp_count - 1; i >= 0; i--) { > - ret = _set_required_opp(dev, genpd_virt_devs[i], opp, i); > + ret = _set_performance_state(dev, genpd_virt_devs[i], opp, i); > if (ret) > break; > } > @@ -1006,6 +993,34 @@ static int _set_required_opps(struct device *dev, > return ret; > } > > +/* 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 void _find_current_opp(struct device *dev, struct opp_table *opp_table) > { > struct dev_pm_opp *opp = ERR_PTR(-ENODEV); > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index e55c6095adf0..93da3c797afc 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -196,6 +196,8 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, > /* Let's do the linking later on */ > if (lazy) > list_add(&opp_table->lazy, &lazy_opp_tables); > + else > + _update_set_required_opps(opp_table); > > goto put_np; > > @@ -411,6 +413,7 @@ 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 3a6e077df386..2a057c42ddf4 100644 > --- a/drivers/opp/opp.h > +++ b/drivers/opp/opp.h > @@ -184,6 +184,7 @@ enum opp_table_access { > * @enabled: Set to true if the device's resources are enabled/configured. > * @genpd_performance_state: Device's power domain support performance state. > * @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. > * > @@ -234,6 +235,8 @@ struct opp_table { > bool enabled; > bool genpd_performance_state; > 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; > @@ -257,6 +260,7 @@ void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cp > struct opp_table *_add_opp_table_indexed(struct device *dev, int index, bool getclk); > void _put_opp_list_kref(struct opp_table *opp_table); > void _required_opps_available(struct dev_pm_opp *opp, int count); > +void _update_set_required_opps(struct opp_table *opp_table); > > static inline bool lazy_linking_pending(struct opp_table *opp_table) > { > -- > 2.31.1.272.g89b43f80a514 >
On 01-03-23, 14:06, Ulf Hansson wrote: > On Wed, 22 Feb 2023 at 12:07, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > The required-opps configuration is closely tied to genpd and performance > > states at the moment and it is not very obvious that required-opps can > > live without genpds. Though we don't support configuring required-opps > > for non-genpd cases currently. > > > > This commit aims at separating these parts, where configuring genpds > > would be a special case of configuring the required-opps. > > > > Add a specialized callback, set_required_opps(), to the opp table and > > set it to different callbacks accordingly. > > > > This shouldn't result in any functional changes for now. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > This looks reasonable to me, but I guess it also depends on whether > you will land patch3 or not? Not necessarily, its better to have a clear error path to avoid unnecessary confusion. I have applied first two patches now. Thanks.
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 6d7016ce8c53..954c94865cf5 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -935,8 +935,8 @@ static int _set_opp_bw(const struct opp_table *opp_table, return 0; } -static int _set_required_opp(struct device *dev, struct device *pd_dev, - struct dev_pm_opp *opp, int i) +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]->pstate : 0; int ret; @@ -953,33 +953,20 @@ static int _set_required_opp(struct device *dev, struct device *pd_dev, return ret; } -/* 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) +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) { - struct opp_table **required_opp_tables = opp_table->required_opp_tables; struct device **genpd_virt_devs = opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev; int i, ret = 0; - if (!required_opp_tables) - return 0; - - /* required-opps not fully initialized yet */ - if (lazy_linking_pending(opp_table)) - return -EBUSY; - - /* - * We only support genpd's OPPs in the "required-opps" for now, as we - * don't know much about other use cases. Error out if the required OPP - * doesn't belong to a genpd. - */ - if (unlikely(!required_opp_tables[0]->is_genpd)) { - dev_err(dev, "required-opps don't belong to a genpd\n"); - return -ENOENT; - } - /* * Acquire genpd_virt_dev_lock to make sure we don't use a genpd_dev * after it is freed from another thread. @@ -987,15 +974,15 @@ static int _set_required_opps(struct device *dev, mutex_lock(&opp_table->genpd_virt_dev_lock); /* Scaling up? Set required OPPs in normal order, else reverse */ - if (up) { + if (!scaling_down) { for (i = 0; i < opp_table->required_opp_count; i++) { - ret = _set_required_opp(dev, genpd_virt_devs[i], opp, i); + ret = _set_performance_state(dev, genpd_virt_devs[i], opp, i); if (ret) break; } } else { for (i = opp_table->required_opp_count - 1; i >= 0; i--) { - ret = _set_required_opp(dev, genpd_virt_devs[i], opp, i); + ret = _set_performance_state(dev, genpd_virt_devs[i], opp, i); if (ret) break; } @@ -1006,6 +993,34 @@ static int _set_required_opps(struct device *dev, return ret; } +/* 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 void _find_current_opp(struct device *dev, struct opp_table *opp_table) { struct dev_pm_opp *opp = ERR_PTR(-ENODEV); diff --git a/drivers/opp/of.c b/drivers/opp/of.c index e55c6095adf0..93da3c797afc 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -196,6 +196,8 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, /* Let's do the linking later on */ if (lazy) list_add(&opp_table->lazy, &lazy_opp_tables); + else + _update_set_required_opps(opp_table); goto put_np; @@ -411,6 +413,7 @@ 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 3a6e077df386..2a057c42ddf4 100644 --- a/drivers/opp/opp.h +++ b/drivers/opp/opp.h @@ -184,6 +184,7 @@ enum opp_table_access { * @enabled: Set to true if the device's resources are enabled/configured. * @genpd_performance_state: Device's power domain support performance state. * @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. * @@ -234,6 +235,8 @@ struct opp_table { bool enabled; bool genpd_performance_state; 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; @@ -257,6 +260,7 @@ void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cp struct opp_table *_add_opp_table_indexed(struct device *dev, int index, bool getclk); void _put_opp_list_kref(struct opp_table *opp_table); void _required_opps_available(struct dev_pm_opp *opp, int count); +void _update_set_required_opps(struct opp_table *opp_table); static inline bool lazy_linking_pending(struct opp_table *opp_table) {
The required-opps configuration is closely tied to genpd and performance states at the moment and it is not very obvious that required-opps can live without genpds. Though we don't support configuring required-opps for non-genpd cases currently. This commit aims at separating these parts, where configuring genpds would be a special case of configuring the required-opps. Add a specialized callback, set_required_opps(), to the opp table and set it to different callbacks accordingly. This shouldn't result in any functional changes for now. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/opp/core.c | 69 ++++++++++++++++++++++++++++------------------ drivers/opp/of.c | 3 ++ drivers/opp/opp.h | 4 +++ 3 files changed, 49 insertions(+), 27 deletions(-)