| Message ID | ec154fa205967507c7352a2d0088346aba562ff2.1539341929.git.viresh.kumar@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | OPP: Support multiple power-domains per device | expand |
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.
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)
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(+) -- 2.18.0.rc1.242.g61856ae69a2c