[PATCH] ARM: OMAP2+: Only write the sysconfig on idle when necessary

[PATCH] ARM: OMAP2+: Only write the sysconfig on idle when necessary

[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.

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(+)

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);
 }
 
-- 
1.7.9.5

Re: [PATCH] ARM: OMAP2+: Only write the sysconfig on idle when necessary

[Paul Walmsley]

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

Re: [PATCH] ARM: OMAP2+: Only write the sysconfig on idle when necessary

[Jon Hunter]

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

Re: [PATCH] ARM: OMAP2+: Only write the sysconfig on idle when necessary

[Paul Walmsley]

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

Re: [PATCH] ARM: OMAP2+: Only write the sysconfig on idle when necessary

[Jon Hunter]

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

Re: [PATCH] ARM: OMAP2+: Only write the sysconfig on idle when necessary

[Rajendra Nayak]

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.