| Message ID | 1565398727-23090-2-git-send-email-thara.gopinath@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | qcom: Model RPMH power domains as thermal cooling devices | expand |
On Sat, 10 Aug 2019 at 02:58, Thara Gopinath <thara.gopinath@linaro.org> wrote: > > Add two new APIs in the genpd framework, > dev_pm_genpd_get_performance_state to return the current performance > state of a power domain and dev_pm_genpd_performance_state_count to > return the total number of performance states supported by a > power domain. Since the genpd framework does not maintain > a count of number of performance states supported by a power domain, > introduce a new callback(.get_performance_state_count) that can be used > to retrieve this information from power domain drivers. I think some brief background to *why* this is useful needs to be squeezed into the changelog. Or at least state that following changes makes use of it, somehow. > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > --- > drivers/base/power/domain.c | 38 ++++++++++++++++++++++++++++++++++++++ > include/linux/pm_domain.h | 18 ++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index b063bc4..17e0375 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -413,6 +413,44 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) > } > EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state); > > +int dev_pm_genpd_get_performance_state(struct device *dev, > + unsigned int *state) > +{ > + struct generic_pm_domain *genpd; > + > + genpd = dev_to_genpd(dev); We need to verify that the there is a genpd attached before doing this cast. Let me post a patch in a day or so, it will give you a helper function that covers this. > + if (IS_ERR(genpd)) > + return -ENODEV; > + > + genpd_lock(genpd); > + *state = genpd->performance_state; Why not return the state, rather than assigning an out-parameter? > + genpd_unlock(genpd); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_performance_state); > + > +int dev_pm_genpd_performance_state_count(struct device *dev, > + unsigned int *count) > +{ > + struct generic_pm_domain *genpd; > + int ret; > + > + genpd = dev_to_genpd(dev); > + if (IS_ERR(genpd)) > + return -ENODEV; > + > + if (unlikely(!genpd->get_performance_state_count)) > + return -EINVAL; > + > + genpd_lock(genpd); > + ret = genpd->get_performance_state_count(genpd, count); Why not having the callback to return the state, rather than using an out-parameter? > + genpd_unlock(genpd); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(dev_pm_genpd_performance_state_count); > + > static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) > { > unsigned int state_idx = genpd->state_idx; > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 91d9bf4..0e5f502 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -117,6 +117,8 @@ struct generic_pm_domain { > struct dev_pm_opp *opp); > int (*set_performance_state)(struct generic_pm_domain *genpd, > unsigned int state); > + int (*get_performance_state_count)(struct generic_pm_domain *genpd, > + unsigned int *count); > struct gpd_dev_ops dev_ops; > s64 max_off_time_ns; /* Maximum allowed "suspended" time. */ > bool max_off_time_changed; > @@ -204,6 +206,10 @@ int pm_genpd_init(struct generic_pm_domain *genpd, > struct dev_power_governor *gov, bool is_off); > int pm_genpd_remove(struct generic_pm_domain *genpd); > int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state); > +int dev_pm_genpd_get_performance_state(struct device *dev, > + unsigned int *state); > +int dev_pm_genpd_performance_state_count(struct device *dev, > + unsigned int *count); > > extern struct dev_power_governor simple_qos_governor; > extern struct dev_power_governor pm_domain_always_on_gov; > @@ -251,6 +257,18 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev, > return -ENOTSUPP; > } > > +static inline int dev_pm_genpd_get_performance_state(struct device *dev, > + unsigned int *state); > +{ > + return -ENOTSUPP; > +} > + > +static inline int dev_pm_genpd_performance_state_count(struct device *dev, > + unsigned int *count); > +{ > + return -ENOTSUPP; > +} > + > #define simple_qos_governor (*(struct dev_power_governor *)(NULL)) > #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL)) > #endif > -- > 2.1.4 > Kind regards Uffe
Hi Ulf, Thanks for the review. On 08/22/2019 11:03 AM, Ulf Hansson wrote: > On Sat, 10 Aug 2019 at 02:58, Thara Gopinath <thara.gopinath@linaro.org> wrote: >> >> Add two new APIs in the genpd framework, >> dev_pm_genpd_get_performance_state to return the current performance >> state of a power domain and dev_pm_genpd_performance_state_count to >> return the total number of performance states supported by a >> power domain. Since the genpd framework does not maintain >> a count of number of performance states supported by a power domain, >> introduce a new callback(.get_performance_state_count) that can be used >> to retrieve this information from power domain drivers. > > I think some brief background to *why* this is useful needs to be > squeezed into the changelog. Or at least state that following changes > makes use of it, somehow. Will do. > >> >> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> >> --- >> drivers/base/power/domain.c | 38 ++++++++++++++++++++++++++++++++++++++ >> include/linux/pm_domain.h | 18 ++++++++++++++++++ >> 2 files changed, 56 insertions(+) >> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index b063bc4..17e0375 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -413,6 +413,44 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) >> } >> EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state); >> >> +int dev_pm_genpd_get_performance_state(struct device *dev, >> + unsigned int *state) >> +{ >> + struct generic_pm_domain *genpd; >> + >> + genpd = dev_to_genpd(dev); > > We need to verify that the there is a genpd attached before doing this > cast. Let me post a patch in a day or so, it will give you a helper > function that covers this. Sounds good.. Thanks.. I will wait for it. > >> + if (IS_ERR(genpd)) >> + return -ENODEV; >> + >> + genpd_lock(genpd); >> + *state = genpd->performance_state; > > Why not return the state, rather than assigning an out-parameter? We can. I will change it for this and dev_pm_genpd_performance_state_count. Regards Thara > >> + genpd_unlock(genpd); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_performance_state); >> + >> +int dev_pm_genpd_performance_state_count(struct device *dev, >> + unsigned int *count) >> +{ >> + struct generic_pm_domain *genpd; >> + int ret; >> + >> + genpd = dev_to_genpd(dev); >> + if (IS_ERR(genpd)) >> + return -ENODEV; >> + >> + if (unlikely(!genpd->get_performance_state_count)) >> + return -EINVAL; >> + >> + genpd_lock(genpd); >> + ret = genpd->get_performance_state_count(genpd, count); > > Why not having the callback to return the state, rather than using an > out-parameter? > >> + genpd_unlock(genpd); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(dev_pm_genpd_performance_state_count); >> + >> static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) >> { >> unsigned int state_idx = genpd->state_idx; >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >> index 91d9bf4..0e5f502 100644 >> --- a/include/linux/pm_domain.h >> +++ b/include/linux/pm_domain.h >> @@ -117,6 +117,8 @@ struct generic_pm_domain { >> struct dev_pm_opp *opp); >> int (*set_performance_state)(struct generic_pm_domain *genpd, >> unsigned int state); >> + int (*get_performance_state_count)(struct generic_pm_domain *genpd, >> + unsigned int *count); >> struct gpd_dev_ops dev_ops; >> s64 max_off_time_ns; /* Maximum allowed "suspended" time. */ >> bool max_off_time_changed; >> @@ -204,6 +206,10 @@ int pm_genpd_init(struct generic_pm_domain *genpd, >> struct dev_power_governor *gov, bool is_off); >> int pm_genpd_remove(struct generic_pm_domain *genpd); >> int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state); >> +int dev_pm_genpd_get_performance_state(struct device *dev, >> + unsigned int *state); >> +int dev_pm_genpd_performance_state_count(struct device *dev, >> + unsigned int *count); >> >> extern struct dev_power_governor simple_qos_governor; >> extern struct dev_power_governor pm_domain_always_on_gov; >> @@ -251,6 +257,18 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev, >> return -ENOTSUPP; >> } >> >> +static inline int dev_pm_genpd_get_performance_state(struct device *dev, >> + unsigned int *state); >> +{ >> + return -ENOTSUPP; >> +} >> + >> +static inline int dev_pm_genpd_performance_state_count(struct device *dev, >> + unsigned int *count); >> +{ >> + return -ENOTSUPP; >> +} >> + >> #define simple_qos_governor (*(struct dev_power_governor *)(NULL)) >> #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL)) >> #endif >> -- >> 2.1.4 >> > > Kind regards > Uffe >
On 08/22/2019 11:03 AM, Ulf Hansson wrote: > On Sat, 10 Aug 2019 at 02:58, Thara Gopinath <thara.gopinath@linaro.org> wrote: >> >> Add two new APIs in the genpd framework, >> dev_pm_genpd_get_performance_state to return the current performance >> state of a power domain and dev_pm_genpd_performance_state_count to >> return the total number of performance states supported by a >> power domain. Since the genpd framework does not maintain >> a count of number of performance states supported by a power domain, >> introduce a new callback(.get_performance_state_count) that can be used >> to retrieve this information from power domain drivers. > > I think some brief background to *why* this is useful needs to be > squeezed into the changelog. Or at least state that following changes > makes use of it, somehow. > >> >> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> >> --- >> drivers/base/power/domain.c | 38 ++++++++++++++++++++++++++++++++++++++ >> include/linux/pm_domain.h | 18 ++++++++++++++++++ >> 2 files changed, 56 insertions(+) >> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index b063bc4..17e0375 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -413,6 +413,44 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) >> } >> EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state); >> >> +int dev_pm_genpd_get_performance_state(struct device *dev, >> + unsigned int *state) >> +{ >> + struct generic_pm_domain *genpd; >> + >> + genpd = dev_to_genpd(dev); > > We need to verify that the there is a genpd attached before doing this > cast. Let me post a patch in a day or so, it will give you a helper > function that covers this. > >> + if (IS_ERR(genpd)) >> + return -ENODEV; >> + >> + genpd_lock(genpd); >> + *state = genpd->performance_state; > > Why not return the state, rather than assigning an out-parameter? > >> + genpd_unlock(genpd); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_performance_state); >> + >> +int dev_pm_genpd_performance_state_count(struct device *dev, >> + unsigned int *count) >> +{ >> + struct generic_pm_domain *genpd; >> + int ret; >> + >> + genpd = dev_to_genpd(dev); >> + if (IS_ERR(genpd)) >> + return -ENODEV; >> + >> + if (unlikely(!genpd->get_performance_state_count)) >> + return -EINVAL; >> + >> + genpd_lock(genpd); >> + ret = genpd->get_performance_state_count(genpd, count); > > Why not having the callback to return the state, rather than using an > out-parameter? Hi Ulf, I just realized that returning the state instead of using a parameter will prevent me from access under lock. Is that okay ? Regards Thara > >> + genpd_unlock(genpd); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(dev_pm_genpd_performance_state_count); >> + >> static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) >> { >> unsigned int state_idx = genpd->state_idx; >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >> index 91d9bf4..0e5f502 100644 >> --- a/include/linux/pm_domain.h >> +++ b/include/linux/pm_domain.h >> @@ -117,6 +117,8 @@ struct generic_pm_domain { >> struct dev_pm_opp *opp); >> int (*set_performance_state)(struct generic_pm_domain *genpd, >> unsigned int state); >> + int (*get_performance_state_count)(struct generic_pm_domain *genpd, >> + unsigned int *count); >> struct gpd_dev_ops dev_ops; >> s64 max_off_time_ns; /* Maximum allowed "suspended" time. */ >> bool max_off_time_changed; >> @@ -204,6 +206,10 @@ int pm_genpd_init(struct generic_pm_domain *genpd, >> struct dev_power_governor *gov, bool is_off); >> int pm_genpd_remove(struct generic_pm_domain *genpd); >> int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state); >> +int dev_pm_genpd_get_performance_state(struct device *dev, >> + unsigned int *state); >> +int dev_pm_genpd_performance_state_count(struct device *dev, >> + unsigned int *count); >> >> extern struct dev_power_governor simple_qos_governor; >> extern struct dev_power_governor pm_domain_always_on_gov; >> @@ -251,6 +257,18 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev, >> return -ENOTSUPP; >> } >> >> +static inline int dev_pm_genpd_get_performance_state(struct device *dev, >> + unsigned int *state); >> +{ >> + return -ENOTSUPP; >> +} >> + >> +static inline int dev_pm_genpd_performance_state_count(struct device *dev, >> + unsigned int *count); >> +{ >> + return -ENOTSUPP; >> +} >> + >> #define simple_qos_governor (*(struct dev_power_governor *)(NULL)) >> #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL)) >> #endif >> -- >> 2.1.4 >> > > Kind regards > Uffe >
On Sat, 7 Sep 2019 at 00:24, Thara Gopinath <thara.gopinath@linaro.org> wrote: > > On 08/22/2019 11:03 AM, Ulf Hansson wrote: > > On Sat, 10 Aug 2019 at 02:58, Thara Gopinath <thara.gopinath@linaro.org> wrote: > >> > >> Add two new APIs in the genpd framework, > >> dev_pm_genpd_get_performance_state to return the current performance > >> state of a power domain and dev_pm_genpd_performance_state_count to > >> return the total number of performance states supported by a > >> power domain. Since the genpd framework does not maintain > >> a count of number of performance states supported by a power domain, > >> introduce a new callback(.get_performance_state_count) that can be used > >> to retrieve this information from power domain drivers. > > > > I think some brief background to *why* this is useful needs to be > > squeezed into the changelog. Or at least state that following changes > > makes use of it, somehow. > > > >> > >> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > >> --- > >> drivers/base/power/domain.c | 38 ++++++++++++++++++++++++++++++++++++++ > >> include/linux/pm_domain.h | 18 ++++++++++++++++++ > >> 2 files changed, 56 insertions(+) > >> > >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > >> index b063bc4..17e0375 100644 > >> --- a/drivers/base/power/domain.c > >> +++ b/drivers/base/power/domain.c > >> @@ -413,6 +413,44 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) > >> } > >> EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state); > >> > >> +int dev_pm_genpd_get_performance_state(struct device *dev, > >> + unsigned int *state) > >> +{ > >> + struct generic_pm_domain *genpd; > >> + > >> + genpd = dev_to_genpd(dev); > > > > We need to verify that the there is a genpd attached before doing this > > cast. Let me post a patch in a day or so, it will give you a helper > > function that covers this. > > > >> + if (IS_ERR(genpd)) > >> + return -ENODEV; > >> + > >> + genpd_lock(genpd); > >> + *state = genpd->performance_state; > > > > Why not return the state, rather than assigning an out-parameter? > > > >> + genpd_unlock(genpd); > >> + > >> + return 0; > >> +} > >> +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_performance_state); > >> + > >> +int dev_pm_genpd_performance_state_count(struct device *dev, > >> + unsigned int *count) > >> +{ > >> + struct generic_pm_domain *genpd; > >> + int ret; > >> + > >> + genpd = dev_to_genpd(dev); > >> + if (IS_ERR(genpd)) > >> + return -ENODEV; > >> + > >> + if (unlikely(!genpd->get_performance_state_count)) > >> + return -EINVAL; > >> + > >> + genpd_lock(genpd); > >> + ret = genpd->get_performance_state_count(genpd, count); > > > > Why not having the callback to return the state, rather than using an > > out-parameter? > Hi Ulf, > I just realized that returning the state instead of using a parameter > will prevent me from access under lock. Is that okay ? Not sure I understand. Why can't you just assign a local variable and return that? Like this: genpd_lock(); count = genpd->get_performance_state_count(); genpd_unlock(); return count; [...] Kind regards Uffe
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index b063bc4..17e0375 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -413,6 +413,44 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) } EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state); +int dev_pm_genpd_get_performance_state(struct device *dev, + unsigned int *state) +{ + struct generic_pm_domain *genpd; + + genpd = dev_to_genpd(dev); + if (IS_ERR(genpd)) + return -ENODEV; + + genpd_lock(genpd); + *state = genpd->performance_state; + genpd_unlock(genpd); + + return 0; +} +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_performance_state); + +int dev_pm_genpd_performance_state_count(struct device *dev, + unsigned int *count) +{ + struct generic_pm_domain *genpd; + int ret; + + genpd = dev_to_genpd(dev); + if (IS_ERR(genpd)) + return -ENODEV; + + if (unlikely(!genpd->get_performance_state_count)) + return -EINVAL; + + genpd_lock(genpd); + ret = genpd->get_performance_state_count(genpd, count); + genpd_unlock(genpd); + + return ret; +} +EXPORT_SYMBOL_GPL(dev_pm_genpd_performance_state_count); + static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) { unsigned int state_idx = genpd->state_idx; diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 91d9bf4..0e5f502 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -117,6 +117,8 @@ struct generic_pm_domain { struct dev_pm_opp *opp); int (*set_performance_state)(struct generic_pm_domain *genpd, unsigned int state); + int (*get_performance_state_count)(struct generic_pm_domain *genpd, + unsigned int *count); struct gpd_dev_ops dev_ops; s64 max_off_time_ns; /* Maximum allowed "suspended" time. */ bool max_off_time_changed; @@ -204,6 +206,10 @@ int pm_genpd_init(struct generic_pm_domain *genpd, struct dev_power_governor *gov, bool is_off); int pm_genpd_remove(struct generic_pm_domain *genpd); int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state); +int dev_pm_genpd_get_performance_state(struct device *dev, + unsigned int *state); +int dev_pm_genpd_performance_state_count(struct device *dev, + unsigned int *count); extern struct dev_power_governor simple_qos_governor; extern struct dev_power_governor pm_domain_always_on_gov; @@ -251,6 +257,18 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev, return -ENOTSUPP; } +static inline int dev_pm_genpd_get_performance_state(struct device *dev, + unsigned int *state); +{ + return -ENOTSUPP; +} + +static inline int dev_pm_genpd_performance_state_count(struct device *dev, + unsigned int *count); +{ + return -ENOTSUPP; +} + #define simple_qos_governor (*(struct dev_power_governor *)(NULL)) #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL)) #endif
Add two new APIs in the genpd framework, dev_pm_genpd_get_performance_state to return the current performance state of a power domain and dev_pm_genpd_performance_state_count to return the total number of performance states supported by a power domain. Since the genpd framework does not maintain a count of number of performance states supported by a power domain, introduce a new callback(.get_performance_state_count) that can be used to retrieve this information from power domain drivers. Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> --- drivers/base/power/domain.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/pm_domain.h | 18 ++++++++++++++++++ 2 files changed, 56 insertions(+)