RE: [PATCH 2/5] ARM: OMAP2+: PM: use the power domains registers cache for the power states
On Tue, May 01, 2012 at 21:07:39, Hunter, Jon wrote: [...] int pwrdm_read_pwrst(struct powerdomain *pwrdm) { - int ret = -EINVAL; + int pwrst, ret = -EINVAL; if (!pwrdm) return -EINVAL; - if (arch_pwrdm arch_pwrdm-pwrdm_read_pwrst) + if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_PWRST, pwrst)) + return pwrst; + + if (arch_pwrdm arch_pwrdm-pwrdm_read_pwrst) { ret = arch_pwrdm-pwrdm_read_pwrst(pwrdm); + if (ret = 0) + pwrdm_cache_write(pwrdm, PWRDM_CACHE_PWRST, ret); + } Do we really want to use the cache for the current state? This is updated by hardware. In the above it appears that once we have read it once we will just return this value until the cache is invalidated. Makes me a little nervous. I echo the concern here. If a powerdomain transition occurs when h/w conditions are met, the cached values will go out of sync and this may lead to unexpected behavior wherever this API is being used. As Jon pointed out in another patch, the registers which can be updated by h/w should be handled differently. Just a wild thought, can PRCM_MPU_IRQ ([4]TRANSITION_EN) be used as a trigger to update the cache if the transition happened under s/w control? Regards, Vaibhav -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] ARM: OMAP2+: PM: use the power domains registers cache for the power states
Hi Jon, Vaibhav, On Thu, May 3, 2012 at 8:38 AM, Bedia, Vaibhav vaibhav.be...@ti.com wrote: On Tue, May 01, 2012 at 21:07:39, Hunter, Jon wrote: [...] int pwrdm_read_pwrst(struct powerdomain *pwrdm) { - int ret = -EINVAL; + int pwrst, ret = -EINVAL; if (!pwrdm) return -EINVAL; - if (arch_pwrdm arch_pwrdm-pwrdm_read_pwrst) + if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_PWRST, pwrst)) + return pwrst; + + if (arch_pwrdm arch_pwrdm-pwrdm_read_pwrst) { ret = arch_pwrdm-pwrdm_read_pwrst(pwrdm); + if (ret = 0) + pwrdm_cache_write(pwrdm, PWRDM_CACHE_PWRST, ret); + } Do we really want to use the cache for the current state? This is updated by hardware. In the above it appears that once we have read it once we will just return this value until the cache is invalidated. Makes me a little nervous. I agree with your concerns. The HW can update things in our back and so there is a risk of having the wrong state reported by the API. Ideally the registers accesses shall be optimized in the HW itself (hint for next gen chipsets ;-). IMO a generic approach on caching is preferred on a hack on the low power code. The cache implementation is based on the following principles: 1. the registers value are read from the cache if it is 'hot', the values are read from the registers if the cache has been invalidated 2. the coherency of the cache fields relies on the invalidate of the fields at the right time during the low power transitions 3. the previous states fields of the cache are invalidated when clearing the previous power states 4. the current states fields of the cache are invalidated after coming back from the low power mode, i.e. after the return from the low level WFI instruction 5. the pwrdm_state_switch function detects the states changes, updates the last internal state (pwrdm-state) and the states statistics 6. in PM debug any discrepancy between pwrdm-state and the current state from the API is reported. More checking could be added in PM debug So there must be a compromise on which fields are invalidated and when. I first tried to invalidate the whole cache after every low power transition, which works fine but does not gain much in performance. The current invalidate scheme (points 3, 4) is optimized for performance and has been tested succesfully on OMAP3 (using cpuidle in RET and OFF). In any case if the HW asynchronously changes the registers states it will not always be detected, with or without the cache implementation. For example if a power domain transitions more than once, only the last transition -to a different state than pwrdm-state- will be detected the next time pwrdm_state_switch runs. I echo the concern here. If a powerdomain transition occurs when h/w conditions are met, the cached values will go out of sync and this may lead to unexpected behavior wherever this API is being used. As Jon pointed out in another patch, the registers which can be updated by h/w should be handled differently. Agree but if you do not cache the previous and current registers there is no point in having the cache in place. Just a wild thought, can PRCM_MPU_IRQ ([4]TRANSITION_EN) be used as a trigger to update the cache if the transition happened under s/w control? That is a nice suggestion. We need to explore the ways to detect transitions. Regards, Vaibhav Thanks for your comments! Regards, Jean -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] ARM: OMAP2+: PM: use the power domains registers cache for the power states
Hi Jean, On 05/01/2012 08:07 AM, jean.pi...@newoldbits.com wrote: From: Jean Pihet j-pi...@ti.com Use the caching API for the previous, current and next power domains states. Signed-off-by: Jean Pihet j-pi...@ti.com --- arch/arm/mach-omap2/powerdomain.c | 32 ++-- 1 files changed, 26 insertions(+), 6 deletions(-) diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c index 18e1ffc..2058e27 100644 --- a/arch/arm/mach-omap2/powerdomain.c +++ b/arch/arm/mach-omap2/powerdomain.c @@ -854,6 +854,8 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst) smp_processor_id()); /* Program the pwrdm desired target state */ ret = arch_pwrdm-pwrdm_set_next_pwrst(pwrdm, pwrst); + if (!ret) + pwrdm_cache_write(pwrdm, PWRDM_CACHE_NEXT_PWRST, pwrst); } return ret; @@ -869,13 +871,19 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst) */ int pwrdm_read_next_pwrst(struct powerdomain *pwrdm) { - int ret = -EINVAL; + int pwrst, ret = -EINVAL; if (!pwrdm) return -EINVAL; - if (arch_pwrdm arch_pwrdm-pwrdm_read_next_pwrst) + if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_NEXT_PWRST, pwrst)) + return pwrst; + + if (arch_pwrdm arch_pwrdm-pwrdm_read_next_pwrst) { ret = arch_pwrdm-pwrdm_read_next_pwrst(pwrdm); + if (ret = 0) + pwrdm_cache_write(pwrdm, PWRDM_CACHE_NEXT_PWRST, ret); + } return ret; } @@ -906,13 +914,19 @@ int pwrdm_read_next_func_pwrst(struct powerdomain *pwrdm) */ int pwrdm_read_pwrst(struct powerdomain *pwrdm) { - int ret = -EINVAL; + int pwrst, ret = -EINVAL; if (!pwrdm) return -EINVAL; - if (arch_pwrdm arch_pwrdm-pwrdm_read_pwrst) + if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_PWRST, pwrst)) + return pwrst; + + if (arch_pwrdm arch_pwrdm-pwrdm_read_pwrst) { ret = arch_pwrdm-pwrdm_read_pwrst(pwrdm); + if (ret = 0) + pwrdm_cache_write(pwrdm, PWRDM_CACHE_PWRST, ret); + } Do we really want to use the cache for the current state? This is updated by hardware. In the above it appears that once we have read it once we will just return this value until the cache is invalidated. Makes me a little nervous. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html