On 11/21/2018 11:01 AM, Rajendra Nayak wrote:


On 11/21/2018 10:46 AM, Viresh Kumar wrote:
On 21-11-18, 10:33, Rajendra Nayak wrote:
Hi Viresh,

On 11/5/2018 12:06 PM, Viresh Kumar wrote:
This commit updates genpd core to start propagating performance state
updates to master domains that have their set_performance_state()
callback set.

A genpd handles two type of performance states now. The first one is the
performance state requirement put on the genpd by the devices and
sub-domains under it, which is already represented by
genpd->performance_state. The second type, introduced in this commit, is
the performance state requirement(s) put by the genpd on its master
domain(s). There is a separate value required for each master that the
genpd has and so a new field is added to the struct gpd_link
(link->performance_state), which represents the link between a genpd and
its master. The struct gpd_link also got another field
prev_performance_state, which is used by genpd core as a temporary
variable during transitions.

We need to propagate setting performance state while powering-on a genpd
as well, as we ignore performance state requirements from sub-domains
which are powered-off. For this reason _genpd_power_on() also received
the additional parameter, depth, which is used for hierarchical locking
within genpd.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
   drivers/base/power/domain.c | 107 +++++++++++++++++++++++++++++-------
   include/linux/pm_domain.h   |   4 ++
   2 files changed, 92 insertions(+), 19 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 6d2e9b3406f1..81e02c5f753f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -239,28 +239,86 @@ static void genpd_update_accounting(struct 
generic_pm_domain *genpd)
   static inline void genpd_update_accounting(struct generic_pm_domain *genpd) 
{}
   #endif
+static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
+                       unsigned int state, int depth);
+
   static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
-                    unsigned int state)
+                    unsigned int state, int depth)
   {
+    struct generic_pm_domain *master;
+    struct gpd_link *link;
+    unsigned int mstate;
       int ret;
       if (!genpd_status_on(genpd))
           goto out;

This check here would mean we only propogate performance state
to masters if the genpd is ON?

Yeah, isn't that what should we do anyway? It is similar to clk-enable
etc. Why increase the reference count if the device isn't enabled and
isn't using the clock ?

I would think this is analogous to a driver calling clk_set_rate() first and
then a clk_enable(), which is certainly valid.
So my question is, if calling a dev_pm_genpd_set_performance_state()
and then runtime enabling the device would work (and take care of propagating 
the performance
state). In my testing I found it does not.

And the reason for that seems to be that we update the genpd status to 
GPD_STATE_ACTIVE
*after* we try to set the performance state, so we always hit this check which 
bails out
thinking the genpd is not ON.



Also you can see that I have updated the genpd power-on code to start
propagation to make sure we don't miss anything.

Reply via email to