ARM: OMAP2+: Only write the sysconfig on idle when necessary
Subsystems (4)
PATCH 1/1 ARM: OMAP2+: Only write the sysconfig on idle when necessary
Currently, whenever we idle a device _idle_sysc() is called and writes to the devices SYSCONFIG register to set the idle mode. A lot devices are using the smart-idle mode and so the write to the SYSCONFIG register is programming the same value that is already stored in the register.
Writes to the devices SYSCONFIG register can be slow, for example, writing to the DMTIMER SYSCONFIG register takes 3 interface clock cycles and 3 functional clock cycles. If the DMTIMER is using the slow 32kHz functional clock this can take ~100us.
Furthermore, during boot on an OMAP4430 panda board, I see that there are 100 calls to _idle_sysc(), however, only 3 out of the 100 calls actually write the SYSCONFIG register with a new value.
Therefore, to avoid unnecessary writes to device SYSCONFIG registers when idling the device, only write the value if the value has changed. It should be safe to do this on idle as the context of the register will never be lost while the device is active.
Verified that suspend, CORE off and retention states are working with this change on OMAP3430 Beagle board.
--- arch/arm/mach-omap2/omap_hwmod.c | 4 ++++ 1 file changed, 4 insertions(+)
arch/arm/mach-omap2/omap_hwmod.c +4 -0
@@ -1389,6 +1389,10 @@ static void _idle_sysc(struct omap_hwmod *oh)if ((sf &SYSC_HAS_SIDLEMODE ) && !(oh->flags &HWMOD_SWSUP_SIDLE ))_enable_wakeup (oh, &v); +/* If the cached value is the same as the new value, skip the write */ +if (oh->_sysc_cache == v) +return ; +_write_sysconfig (v, oh); }
Signature · 1 line
1.7.9.5
Paul Walmsley
maintainerPATCH 1/1 ARM: OMAP2+: Only write the sysconfig on idle when necessary
cc Rajendra
Hi Jon
Quoted 14 lines · Jon Hunter
Currently, whenever we idle a device
_idle_sysc()is called and writes to the devices SYSCONFIG register to set the idle mode. A lot devices are using the smart-idle mode and so the write to the SYSCONFIG register is programming the same value that is already stored in the register.Writes to the devices SYSCONFIG register can be slow, for example, writing to the DMTIMER SYSCONFIG register takes 3 interface clock cycles and 3 functional clock cycles. If the DMTIMER is using the slow 32kHz functional clock this can take ~100us.
Furthermore, during boot on an OMAP4430 panda board, I see that there are 100 calls to
_idle_sysc(), however, only 3 out of the 100 calls actually write the SYSCONFIG register with a new value.
Quoted 7 lines
Therefore, to avoid unnecessary writes to device SYSCONFIG registers when idling the device, only write the value if the value has changed. It should be safe to do this on idle as the context of the register will never be lost while the device is active.
Verified that suspend, CORE off and retention states are working with this change on OMAP3430 Beagle board.
The code used to do what you propose in _write_sysconfig(), which applied to all sysconfig writes, not just idle. But it was changed by commit 233cbe5b94096f95ba7bca2162d63275b0b90b5b ("OMAP2+: hwmod: Update the sysc_cache in case module context is lost"). Could you take a look at this and maybe discuss it with Rajendra? It seems to make more sense to me to cache it by default, and only invalidate it when we know that context is lost.
- Paul
PATCH 1/1 ARM: OMAP2+: Only write the sysconfig on idle when necessary
Quoted 28 lines · Paul Walmsley
cc Rajendra
Hi Jon
Quoted 14 lines · Jon Hunter
Currently, whenever we idle a device
_idle_sysc()is called and writes to the devices SYSCONFIG register to set the idle mode. A lot devices are using the smart-idle mode and so the write to the SYSCONFIG register is programming the same value that is already stored in the register.Writes to the devices SYSCONFIG register can be slow, for example, writing to the DMTIMER SYSCONFIG register takes 3 interface clock cycles and 3 functional clock cycles. If the DMTIMER is using the slow 32kHz functional clock this can take ~100us.
Furthermore, during boot on an OMAP4430 panda board, I see that there are 100 calls to
_idle_sysc(), however, only 3 out of the 100 calls actually write the SYSCONFIG register with a new value.Quoted 7 lines
Therefore, to avoid unnecessary writes to device SYSCONFIG registers when idling the device, only write the value if the value has changed. It should be safe to do this on idle as the context of the register will never be lost while the device is active.
Verified that suspend, CORE off and retention states are working with this change on OMAP3430 Beagle board.
Quoted 7 lines
The code used to do what you propose in
_write_sysconfig(), which applied to all sysconfig writes, not just idle. But it was changed by commit233cbe5b94096f95ba7bca2162d63275b0b90b5b("OMAP2+: hwmod: Update thesysc_cachein case module context is lost"). Could you take a look at this and maybe discuss it with Rajendra? It seems to make more sense to me to cache it by default, and only invalidate it when we know that context is lost.
Ok, thanks. Yes, only updating the register when the cache value changed would not work due to the possibility of context being lost. So Rajendra's change makes sense. However, I think there is room to optimise this.
With this change, on idle, the cache value and register value are only updated when needed. This should be safe.
Are you looking to go one step further and only update the sysconfig on enabling when the context has been lost? That would require more changes. This was a quick optimisation I saw when reviewing the code.
Rajendra, let me know if you have any comments.
Cheers
Jon
Paul Walmsley
maintainerPATCH 1/1 ARM: OMAP2+: Only write the sysconfig on idle when necessary
Quoted 3 lines · Jon Hunter
Are you looking to go one step further and only update the sysconfig on enabling when the context has been lost? That would require more changes.
Yes that's exactly it. That would avoid adding a special case for what should be the common case. From a quick glance it looks like the cache needs to be loaded in _reset(), omap_hwmod_softreset(), and _enable(). Other than that, seems like the cached value should work.
It should also be possible to avoid the reload in _enable() in most cases since the PM code should know whether the IP block's powerdomain was programmed to go off and indeed whether it did so. It shouldn't involve any extra register reads. But I wouldn't expect you to add that optimization; would just be nice to have a comment to that effect.
If the meta-theme of your message is that commit 233cbe5b94096f95ba7bca2162d63275b0b90b5b should have had closer scrutiny, I agree with you, but we're beyond that point now...
- Paul
PATCH 1/1 ARM: OMAP2+: Only write the sysconfig on idle when necessary
Quoted 11 lines · Paul Walmsley
Quoted 3 lines · Jon Hunter
Are you looking to go one step further and only update the sysconfig on enabling when the context has been lost? That would require more changes.
Yes that's exactly it. That would avoid adding a special case for what should be the common case. From a quick glance it looks like the cache needs to be loaded in
_reset(),omap_hwmod_softreset(), and_enable(). Other than that, seems like the cached value should work.
Quoted 5 lines
It should also be possible to avoid the reload in
_enable()in most cases since the PM code should know whether the IP block's powerdomain was programmed to go off and indeed whether it did so. It shouldn't involve any extra register reads. But I wouldn't expect you to add that optimization; would just be nice to have a comment to that effect.
Ah, so you really want the cache to behave like a cache. That would be nice!
Quoted 3 lines
If the meta-theme of your message is that commit
233cbe5b94096f95ba7bca2162d63275b0b90b5bshould have had closer scrutiny, I agree with you, but we're beyond that point now...
No under-lying theme here, just more of a "I stumbled across this while debugging something else" and I am a nut-case about saving cpu cycles where I can. Although not always possible and I am probably responsible for burning more cycles than I am saving these days!
Cheers
Jon
PATCH 1/1 ARM: OMAP2+: Only write the sysconfig on idle when necessary
In reply to Jon HunterQuoted 5 lines · Jon Hunter
Ok, thanks. Yes, only updating the register when the cache value changed would not work due to the possibility of context being lost. So Rajendra's change makes sense. However, I think there is room to optimise this.
Quoted 8 lines
With this change, on idle, the cache value and register value are only updated when needed. This should be safe.
Are you looking to go one step further and only update the sysconfig on enabling when the context has been lost? That would require more changes. This was a quick optimisation I saw when reviewing the code.
Rajendra, let me know if you have any comments.
Makes sense to me. To handle the more generic case of avoiding all reads and writes whenever possible, and making the cache really behave like a cache, as Paul suggested, is certainly more work and more importantly more testing, as it would rely heavily on the context lost counters to work correctly. I feel there is still some work needed around those counters to make them more robust before we start heading in that direction.