Re: [PATCH V2 5/9] PM / Domains: Add genpd_opp_to_performance_state()

[Ulf Hansson]

On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> The OPP core currently stores the performance state in the consumer
> device's OPP table, but that is going to change going forward and
> performance state will rather be set directly in the genpd's OPP table.
>
> For that we need to get the performance state for genpd's device
> structure instead of the consumer device's structure. Add a new helper
> to do that.

I guess what puzzles me a bit here is that we are using a struct
device, while we actually should be talking about an OPP cookie
instead, right?

So the "genpd's device structure" here is not the same as the virtual
devices created by genpd to support multiple PM domains, right? Or is
it?

>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>  drivers/base/power/domain.c | 39 +++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  8 ++++++++
>  2 files changed, 47 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 4b5714199490..2c82194d2a30 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2508,6 +2508,45 @@ int of_genpd_parse_idle_states(struct device_node *dn,
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>
> +/**
> + * genpd_opp_to_performance_state- Gets performance state of the genpd from its OPP node.

Please rename to:

pm_genpd_opp_to_perfromance_state().

> + *
> + * @genpd_dev: Genpd's device for which the performance-state needs to be found.

Maybe "genpd_dev" is the correct name to use here, as I understand
it's actually the device representing the genpd. However, in other
patches in this series you are also using "genpd_dev", while those
instead corresponds to the virtual created devices by genpd.

I would appreciate if we could make that more clear in the code.

Maybe distinguish them as:

genpd_dev
genpd_virt_dev

or just:

dev
virt_dev

> + * @opp: struct dev_pm_opp of the OPP for which we need to find performance
> + *     state.
> + *
> + * Returns performance state encoded in the OPP of the genpd. This calls
> + * platform specific genpd->opp_to_performance_state() callback to translate
> + * power domain OPP to performance state.
> + *
> + * Returns performance state on success and 0 on failure.
> + */
> +unsigned int genpd_opp_to_performance_state(struct device *genpd_dev,
> +                                           struct dev_pm_opp *opp)
> +{
> +       struct generic_pm_domain *genpd = NULL, *temp;
> +       int state;
> +
> +       lockdep_assert_held(&gpd_list_lock);

What's this?

> +
> +       list_for_each_entry(temp, &gpd_list, gpd_list_node) {
> +               if (&temp->dev == genpd_dev) {
> +                       genpd = temp;
> +                       break;
> +               }
> +       }

I think we can do better than this.

We really don't want to walk the list of genpds while doing this. The
caller should already know (if not now, we should fix it) that the
struct device is used to represent a genpd.

In other words, I am thinking using a container_of() or a finding a
function pointer through the struct device, in any case, it would be
better.

If it all sounds fuzzy, let me think a bit about it and come back with
a more clear suggestion.

> +
> +       if (unlikely(!genpd || !genpd->opp_to_performance_state))
> +               return 0;
> +
> +       genpd_lock(genpd);

.... so in the end, this is the only lock that should be needed.

> +       state = genpd->opp_to_performance_state(genpd, opp);
> +       genpd_unlock(genpd);
> +
> +       return state;
> +}
> +EXPORT_SYMBOL_GPL(genpd_opp_to_performance_state);
> +
>  /**
>   * of_genpd_opp_to_performance_state- Gets performance state of device's
>   * power domain corresponding to a DT node's "required-opps" property.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 776c546d581a..e03f300f7468 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -233,6 +233,8 @@ int of_genpd_add_subdomain(struct of_phandle_args *parent,
>  struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
>  int of_genpd_parse_idle_states(struct device_node *dn,
>                                struct genpd_power_state **states, int *n);
> +unsigned int genpd_opp_to_performance_state(struct device *genpd_dev,
> +                                           struct dev_pm_opp *opp);
>  unsigned int of_genpd_opp_to_performance_state(struct device *dev,
>                                 struct device_node *np);
>
> @@ -274,6 +276,12 @@ static inline int of_genpd_parse_idle_states(struct device_node *dn,
>         return -ENODEV;
>  }
>
> +static inline unsigned int
> +genpd_opp_to_performance_state(struct device *genpd_dev, struct dev_pm_opp *opp)
> +{
> +       return 0;
> +}
> +
>  static inline unsigned int
>  of_genpd_opp_to_performance_state(struct device *dev,
>                                   struct device_node *np)
> --
> 2.18.0.rc1.242.g61856ae69a2c
>

Besides the comments above, I am all in for the approach as such.

Kind regards
Uffe