[V2,5/9] PM / Domains: Add genpd_opp_to_performance_state()
diff mbox series

Message ID ec154fa205967507c7352a2d0088346aba562ff2.1539341929.git.viresh.kumar@linaro.org
State New
Headers show
Series
  • OPP: Support multiple power-domains per device
Related show

Commit Message

Viresh Kumar Oct. 12, 2018, 11:11 a.m. UTC
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.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/domain.c | 39 +++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  8 ++++++++
 2 files changed, 47 insertions(+)

Comments

Ulf Hansson Oct. 12, 2018, 3:07 p.m. UTC | #1
On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@linaro.org> 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@linaro.org>
> ---
>  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
Viresh Kumar Oct. 12, 2018, 3:40 p.m. UTC | #2
On 12 October 2018 at 20:37, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 12 October 2018 at 13:11, Viresh Kumar <viresh.kumar@linaro.org> 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?

The OPP cookie wouldn't get us to the platform specific conversion
handler, for that we need something from the genpd itself and so its
structure.

> 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?

You already found that I believe, it is genpd->dev.

>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>  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().

Ok.

>> + *
>> + * @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.

Naming is a mess because I tried to follow the names you followed in
your multiple domain support. You used genpd_dev for the virtual device :)

> 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

Maybe, perhaps we should change domain.c with same naming for the internal
coding handling multiple domains as well ? I will send the patch for
that if you agree.

>> + * @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?

Don't we need to protect with a lock while traversing the below list?
Above is just a check to make sure lock is taken.

>> +
>> +       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.

I really want to :)

> 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.

Caller knows that genpd_dev here is genpd->dev really. But it doesn't
have pointer of the genpd itself.

> 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.

I am stupid. Container-of will work just fine I belive.

Patch
diff mbox series

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.
+ *
+ * @genpd_dev: Genpd's device for which the performance-state needs to be found.
+ * @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);
+
+	list_for_each_entry(temp, &gpd_list, gpd_list_node) {
+		if (&temp->dev == genpd_dev) {
+			genpd = temp;
+			break;
+		}
+	}
+
+	if (unlikely(!genpd || !genpd->opp_to_performance_state))
+		return 0;
+
+	genpd_lock(genpd);
+	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)