Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

From: Ulf Hansson <ulf.hansson@linaro.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: 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>,
	Stephan Gerhold <stephan.gerhold@kernkonzept.com>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case
Date: Thu, 19 Oct 2023 13:16:14 +0200	[thread overview]
Message-ID: <CAPDyKFqbnsdT0nqKwQhai875CwwpW_vepr816fL+i8yLh=YQhw@mail.gmail.com> (raw)
In-Reply-To: <f709e9e273004be43efe3a2854a7e7b51a777f99.1697710527.git.viresh.kumar@linaro.org>

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