On 30-11-18, 11:18, Ulf Hansson wrote: > On Fri, 30 Nov 2018 at 10:59, Viresh Kumar <viresh.ku...@linaro.org> wrote: > > Sure, but the ordering of locks is always subdomain first and then master. > > Considering the case of Qcom, we have two domains Cx (sub-domain) and Mx > > (master). > > > > On first genpd_power_on(Cx) call, we will first call genpd_power_on(Mx) > > which > > will just power it on as none of its master will have perf-state support. We > > then call _genpd_power_on(Cx) which will also not do anything with Mx as > > its own > > (Cx's) pstate would be 0 at that time. But even if it had a valid value, it > > will > > propagate just fine with all proper locking in place. > > Can you explain that, it's not super easy to follow the flow.
Sorry, I somehow assumed you would know it already :) > So what will happen if Cx has a value that needs to be propagated? > What locks will be taken, and in what order? > > Following, what if we had a Bx domain, being the subdomain of Cx, and > it too had a value that needs to be propagated. Lets take the worst example, we have Bx (sub-domain of Cx), Cx (sub-domain of Mx) and Dx (master). Normal power-on/off will always have the values 0, so lets consider resume sequence where all the domains will have a value pstate value. And please forgive me for any bugs I have introduced in the following super-complex sequence :) genpd_runtime_resume(dev) //domain Bx -> genpd_lock(Bx) -> genpd_power_on(Bx) -> genpd_lock(Cx) -> genpd_power_on(Cx) -> genpd_lock(Dx) -> genpd_power_on(Dx) -> _genpd_power_on(Dx) -> _genpd_set_performance_state(Dx, Dxstate) { //Doesn't have any masters -> genpd->set_performance_state(Dx, Dxstate); } -> genpd_unlock(Dx) -> _genpd_power_on(Cx) -> _genpd_set_performance_state(Cx, Cxstate) { //have one master, Dx -> genpd_lock(Dx) -> _genpd_set_performance_state(Dx, Dxstate) { //Doesn't have any masters -> genpd->set_performance_state(Dx, Dxstate); } -> genpd_unlock(Dx) // Change Cx state -> genpd->set_performance_state(Cx, Cxstate); } -> genpd_unlock(Cx) -> _genpd_power_on(Bx) -> _genpd_set_performance_state(Bx, Bxstate) { //have one master, Cx -> genpd_lock(Cx) -> _genpd_set_performance_state(Cx, Cxstate) { //have one master, Dx -> genpd_lock(Dx) -> _genpd_set_performance_state(Dx, Dxstate) { //Doesn't have any masters -> genpd->set_performance_state(Dx, Dxstate); } -> genpd_unlock(Dx) // Change Cx state -> genpd->set_performance_state(Cx, Cxstate); } -> genpd_unlock(Cx) -> genpd->set_performance_state(Bx, Bxstate); } -> genpd_unlock(Bx) > It sounds like we will > do the propagation one time per level. Is that really necessary, > couldn't we just do it once, after the power on sequence have been > completed? It will be a BIG hack somewhere, isn't it ? How will we know when has the time come to shoot the final sequence of set_performance_state() ? And where will we do it? genpd_runtime_resume() ? And then we will have more problems, for example Rajendra earlier compared this stuff to clk framework where it is possible to do clk_set_rate() first and then only call clk_enable() and the same should be possible with genpd as well, i.e. set performance state first and then only enable the device/domain. And so we need this right within genpd_power_on(). I know things are repetitive here, but that's the right way of doing it IMHO. What do you say ? -- viresh