| Message ID | 1350504742-19995-1-git-send-email-jon-hunter@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
cc Rajendra Hi Jon On Wed, 17 Oct 2012, Jon Hunter wrote: > 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. 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
On 10/17/2012 03:25 PM, Paul Walmsley wrote: > cc Rajendra > > Hi Jon > > On Wed, 17 Oct 2012, Jon Hunter wrote: > >> 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. > > 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. 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
On Wed, 17 Oct 2012, Jon Hunter wrote: > 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
On 10/17/2012 03:58 PM, Paul Walmsley wrote: > On Wed, 17 Oct 2012, Jon Hunter wrote: > >> 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. Ah, so you really want the cache to behave like a cache. That would be nice! > 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... 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
On Thursday 18 October 2012 02:07 AM, Jon Hunter wrote: > 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. 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.
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index b969ab1..962773b 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -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); }
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. Signed-off-by: Jon Hunter <jon-hunter@ti.com> --- arch/arm/mach-omap2/omap_hwmod.c | 4 ++++ 1 file changed, 4 insertions(+)