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

Ulf Hansson
2023-10-19T11:16:56Z

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