MIME-Version: 1.0
References: <cover.1677063656.git.viresh.kumar@linaro.org> <56b9926836652e9dbf56ca6b7628e836cddca351.1677063656.git.viresh.kumar@linaro.org>
In-Reply-To: <56b9926836652e9dbf56ca6b7628e836cddca351.1677063656.git.viresh.kumar@linaro.org>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Wed, 1 Mar 2023 14:06:22 +0100
Message-ID: <CAPDyKFoQu1=XVCueOaUDgke3xdDy3bt9VRHPo2xdqkLbSbP8CA@mail.gmail.com>
Subject: Re: [PATCH 2/3] OPP: Move required opps configuration to specialized callback
To:     Viresh Kumar <viresh.kumar@linaro.org>
Cc:     Jun Nie <jun.nie@linaro.org>, Viresh Kumar <vireshk@kernel.org>,
        Nishanth Menon <nm@ti.com>, Stephen Boyd <sboyd@kernel.org>,
        linux-pm@vger.kernel.org,
        Vincent Guittot <vincent.guittot@linaro.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>