Patchwork [PATCHv2,02/12] ARM: OMAP2+: hwmod code/data: fix 32K sync timer

login
register
mail settings
Submitter Paul Walmsley
Date June 11, 2012, 12:45 a.m.
Message ID <20120611004555.20034.87035.stgit@dusk>
Download mbox | patch
Permalink /patch/164035/
State New
Headers show

Comments

Paul Walmsley - June 11, 2012, 12:45 a.m.
Kevin discovered that commit c8d82ff68fb6873691536cf33021977efbf5593c
("ARM: OMAP2/3: hwmod data: Add 32k-sync timer data to hwmod
database") broke CORE idle on OMAP3.  This prevents device low power
states.

The root cause is that the 32K sync timer IP block does not support
smart-idle mode[1], and so the hwmod code keeps the IP block in
no-idle mode while it is active.  This in turn prevents the WKUP
clockdomain from transitioning to idle.  There is a hardcoded sleep
dependency that prevents the CORE_L3 and CORE_CM clockdomains from
transitioning to idle when the WKUP clockdomain is active[2], so the
chip cannot enter any device low power states.

It turns out that there is no need to take the 32k sync timer out of
idle.  The IP block itself probably does not have any native idle
handling at all, due to its simplicity.  Furthermore, the PRCM will
never request target idle for this IP block while the kernel is
running, due to the sleep dependency that prevents the WKUP
clockdomain from idling while the CORE_L3 clockdomain is active.  So
we can safely leave the 32k sync timer in target-no-idle mode, even
while we continue to access it.

This workaround is implemented by defining a new hwmod flag,
HWMOD_ALWAYS_FORCE_SIDLE, that places the IP block into target
force-idle mode even when enabled.  The 32k sync timer hwmods are
marked with this flag for OMAP3 and OMAP4 SoCs.  (On OMAP2xxx, no OCP
header existed on the 32k sync timer.)

Another theoretically clean fix for this problem would be to implement
PM runtime-based control for 32k sync timer accesses.  These PM
runtime calls would need to located in a custom clocksource, since the
32k sync timer is currently used as an MMIO clocksource.  But in
practice, there would be little benefit to doing so; and there would
be some cost, due to the addition of unnecessary lines of code and the
additional CPU overhead of the PM runtime and hwmod code - unnecessary
in this case.

Another possible fix would have been to modify the pm34xx.c code to
force the IP block idle before entering WFI.  But this would not have
been an acceptable approach: we are trying to remove this type of
centralized IP block idle control from the PM code.

This patch is effectively a workaround for a hardware problem.  A
better hardware approach would have been to implement a smart-idle
target idle mode for this IP block.  The smart-idle mode in this case
would behave identically to the force-idle mode.  We consider the
force-idle and no-idle target idle mode settings to be intended for
debugging and automatic idle management bug workarounds only[4].

This patch is a collaboration between Kevin Hilman <khilman@ti.com>
and Paul Walmsley <paul@pwsan.com>.

Thanks to Vaibhav Hiremath <hvaibhav@ti.com> for providing comments on
an earlier version of this patch.  Thanks to Tero Kristo
<t-kristo@ti.com> for identifying a bug in an earlier version of this
patch.  Thanks to Benoît Cousson <b-cousson@ti.com> for identifying a
bug in an earlier version of this patch and for implementation comments.

References:

1. Table 16-96 "REG_32KSYNCNT_SYSCONFIG" of the OMAP34xx TRM Rev. ZU
   (SWPU223U), available from:
   http://www.ti.com/pdfs/wtbu/OMAP34x_ES3.1.x_PUBLIC_TRM_vzU.zip

2. Table 4-72 "Sleep Dependencies" of the OMAP34xx TRM Rev. ZU
   (SWPU223U)

3. ibid.

4. Section 3.1.1.1.2 "Module-Level Clock Management" of the OMAP4430
   TRM Rev. vAA (SWPU231AA).

Cc: Tony Lindgren <tony@atomide.com>
Cc: Vaibhav Hiremath <hvaibhav@ti.com>
Cc: Benoît Cousson <b-cousson@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
Tested-by: Kevin Hilman <khilman@ti.com>
Signed-off-by: Kevin Hilman <khilman@ti.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/omap_hwmod.c             |   17 +++++++++++------
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c   |    2 +-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c   |    2 +-
 arch/arm/plat-omap/include/plat/omap_hwmod.h |    9 +++++++++
 4 files changed, 22 insertions(+), 8 deletions(-)
Cousson, Benoit - June 14, 2012, 4:47 p.m.
Hi Paul,

On 6/11/2012 2:45 AM, Paul Walmsley wrote:
> Kuvin discovered that commit c8d82ff68fb6873691536cf33021977efbf5593c

I guess you meant Kevin?

> ("ARM: OMAP2/3: hwmod data: Add 32k-sync timer data to hwmod
> database") broke CORE idle on OMAP3.  This prevents device low power
> states.
>
> The root cause is that the 32K sync timer IP block does not support
> smart-idle mode[1], and so the hwmod code keeps the IP block in
> no-idle mode while it is active.  This in turn prevents the WKUP
> clockdomain from transitioning to idle.  There is a hardcoded sleep
> dependency that prevents the CORE_L3 and CORE_CM clockdomains from
> transitioning to idle when the WKUP clockdomain is active[2], so the
> chip cannot enter any device low power states.
>
> It turns out that there is no need to take the 32k sync timer out of
> idle.
> The IP block itself pbobably does not have any native idle

typo

> handling at all, due to its simplicity.

I don't think this description is really accurate, due to the usual 
confusing definition of IDLE for the PRCM standpoint :-)
The counter_32k will follow PRCM requirement toward SIdleReq/SIdleAck.
It has to be "idle" for PRCM standpoint to allow the transition of the 
L4_WKUP to inactive (SIdleAck=IDLE). And it will be functional as soon 
as the L4_WKUP domain is active.

The fact that the smartidle mode is missing does not change anything in 
the way the PRCM handle the protocol. It is just an issue at IP level.

In that case force-idle = smart-idle, just because this module does not 
have anything to do to delay the SIdleAck upon SIdleReq request. The IP 
is probably connecting the SIdleAck to the SIdleReq signal.
Please note that there are a bunch of IPs that are doing that even if 
they do support the smartidle mode.

> Furthermore, the PRCM will
> never request target idle for this IP block while the kernel is
> running, due to the sleep dependency that prevents the WKUP
> clockdomain from idling while the CORE_L3 clockdomain is active.  So
> we can safely leave the 32k sync timer in target-no-idle mode, even
> while we continue to access it.

Do you mean force-idle? Because accessing a module in no-idle is always 
possible.

> This workaround is implemented by defining a new hwmod flag,
> HWMOD_ALWAYS_FORCE_SIDLE, that places the IP block into target
> force-idle mode even when enabled.  The 32k sync timer hwmods are
> marked with this flag for OMAP3 and OMAP4 SoCs.  (On OMAP2xxx, no OCP
> header existed on the 32k sync timer.)

I still don't see the need for this flag. It looks to me that we are 
adding a redundant information and thus make things more complex than it 
should.

	.idlemodes	= (SIDLE_FORCE | SIDLE_NO),

It the real root cause of the problem. There is no need to re-encode 
that using an extra flag. Especially at hwmod level where the issue is 
at sysconfig level.

I did not really get the reason why you changed your mind on that point.

As soon as there is no SIDLE_SMART mode, what choice do we have but 
using the SIDLE_FORCE?


BTW, I even think the HWMOD_SWSUP_SIDLE hwmod flag is useless now. It 
was needed before probably because the idlemodes were wrongly populated.

In fact the hwmod flags is really there to *flag* some real HW bug we 
cannot figure out otherwise, but in that case, the sysc.idlemodes 
already contains all the information we need to set the proper mode 
during enable/disable.

Duplicating the information is always a source of confusion and might 
lead to nasty bugs due to the increase of complexity.

> Another theoretically clean fix for this problem would be to implement
> PM runtime-based control for 32k sync timer accesses.  These PM
> runtime calls would need to located in a custom clocksource, since the
> 32k sync timer is currently used as an MMIO clocksource.  But in
> practice, there would be little benefit to doing so; and there would
> be some cost, due to the addition of unnecessary lines of code and the
> additional CPU overhead of the PM runtime and hwmod code - unnecessary
> in this case.

I don't think that part is really relevant anymore.

> Another possible fix would have been to modify the pm34xx.c code to
> force the IP block idle before entering WFI.  But this would not have
> been an acceptable approach: we are trying to remove this type of
> centralized IP block idle control from the PM code.

Neither that one I guess. As soon as we consider force-idle to be 
equivalent to smart-idle, nothing more is needed.

> This patch is effectively a workaround for a hardware problem.  A
> better hardware approach would have been to implement a smart-idle
> target idle mode for this IP block.  The smart-idle mode in this case
> would behave identically to the force-idle mode.  We consider the
> force-idle and no-idle target idle mode settings to be intended for
> debugging and automatic idle management bug workarounds only[4].
>
> This patch is a collaboration between Kevin Hilman <khilman@ti.com>
> and Paul Walmsley <paul@pwsan.com>.
>
> Thanks to Vaibhav Hiremath <hvaibhav@ti.com> for providing comments on
> an earlier version of this patch.  Thanks to Tero Kristo
> <t-kristo@ti.com> for identifying a bug in an earlier version of this
> patch.  Thanks to Benoît Cousson <b-cousson@ti.com> for identifying a
> bug in an earlier version of this patch and for implementation comments.
>
> References:
>
> 1. Table 16-96 "REG_32KSYNCNT_SYSCONFIG" of the OMAP34xx TRM Rev. ZU
>     (SWPU223U), available from:
>     http://www.ti.com/pdfs/wtbu/OMAP34x_ES3.1.x_PUBLIC_TRM_vzU.zip
>
> 2. Table 4-72 "Sleep Dependencies" of the OMAP34xx TRM Rev. ZU
>     (SWPU223U)
>
> 3. ibid.
>
> 4. Section 3.1.1.1.2 "Module-Level Clock Management" of the OMAP4430
>     TRM Rev. vAA (SWPU231AA).
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Vaibhav Hiremath <hvaibhav@ti.com>
> Cc: Benoît Cousson <b-cousson@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Tested-by: Kevin Hilman <khilman@ti.com>
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod.c             |   17 +++++++++++------
>   arch/arm/mach-omap2/omap_hwmod_3xxx_data.c   |    2 +-
>   arch/arm/mach-omap2/omap_hwmod_44xx_data.c   |    2 +-
>   arch/arm/plat-omap/include/plat/omap_hwmod.h |    9 +++++++++
>   4 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index bf86f7e..096474c 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -1124,10 +1124,12 @@ static struct omap_hwmod_addr_space * __init _find_mpu_rt_addr_space(struct omap
>    * _enable_sysc - try to bring a module out of idle via OCP_SYSCONFIG
>    * @oh: struct omap_hwmod *
>    *
> - * If module is marked as SWSUP_SIDLE, force the module out of slave
> - * idle; otherwise, configure it for smart-idle.  If module is marked
> - * as SWSUP_MSUSPEND, force the module out of master standby;
> - * otherwise, configure it for smart-standby.  No return value.
> + * Ensure that the OCP_SYSCONFIG register for the IP block represented
> + * by @oh is set to indicate to the PRCM that the IP block is active.
> + * Usually this means placing the module into smart-idle mode and
> + * smart-standby, but if there is a bug in the automatic idle handling
> + * for the IP block, it may need to be placed into the force-idle or
> + * no-idle variants of these modes.  No return value.
>    */
>   static void _enable_sysc(struct omap_hwmod *oh)
>   {
> @@ -1141,8 +1143,11 @@ static void _enable_sysc(struct omap_hwmod *oh)
>   	sf = oh->class->sysc->sysc_flags;
>
>   	if (sf & SYSC_HAS_SIDLEMODE) {
> -		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
> -			HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
> +		if (oh->flags & HWMOD_ALWAYS_FORCE_SIDLE)
> +			idlemode = HWMOD_IDLEMODE_FORCE;
> +		else
> +			idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
> +				HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
>   		_set_slave_idlemode(oh, idlemode, &v);
>   	}
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> index b26d3c9..f8ac9e7 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> @@ -2018,7 +2018,7 @@ static struct omap_hwmod omap3xxx_counter_32k_hwmod = {
>   	.name		= "counter_32k",
>   	.class		= &omap3xxx_counter_hwmod_class,
>   	.clkdm_name	= "wkup_clkdm",
> -	.flags		= HWMOD_SWSUP_SIDLE,
> +	.flags		= HWMOD_ALWAYS_FORCE_SIDLE | HWMOD_SWSUP_SIDLE,

I guess we might be able to get rid of both in theory.

Regards,
Benoit
Paul Walmsley - June 14, 2012, 6:04 p.m.
Hi

On Thu, 14 Jun 2012, Cousson, Benoit wrote:

> On 6/11/2012 2:45 AM, Paul Walmsley wrote:
> > Kuvin discovered that commit c8d82ff68fb6873691536cf33021977efbf5593c
> 
> I guess you meant Kevin?

...

> > The IP block itself pbobably does not have any native idle
> 
> typo

Thanks for noticing these, but they are not in what was sent out by the 
list server:

http://www.spinics.net/lists/linux-omap/msg71503.html

Some software/hardware issue on your end, maybe?

> > handling at all, due to its simplicity.
> 
> I don't think this description is really accurate, due to the usual confusing
> definition of IDLE for the PRCM standpoint :-)
> The counter_32k will follow PRCM requirement toward SIdleReq/SIdleAck.
> It has to be "idle" for PRCM standpoint to allow the transition of the L4_WKUP
> to inactive (SIdleAck=IDLE). And it will be functional as soon as the L4_WKUP
> domain is active.
> 
> The fact that the smartidle mode is missing does not change anything in the
> way the PRCM handle the protocol. It is just an issue at IP level.
> 
> In that case force-idle = smart-idle, just because this module does not have
> anything to do to delay the SIdleAck upon SIdleReq request. The IP is probably
> connecting the SIdleAck to the SIdleReq signal.
> Please note that there are a bunch of IPs that are doing that even if they do
> support the smartidle mode.

Right, that's exactly my point -- perhaps made in an unclear way.

My point was that the 32k counter IP block doesn't do anything with the 
incoming IdleReq signal to determine whether or not to assert IdleAck back 
to the PRCM.  But rather than implementing smart-idle mode to handle this, 
like the other IP blocks do, the only two modes implemented were the 
debugging modes, force-idle and no-idle.

So depending on one's point of view, this patch is either:

1. a hardware bug workaround, because the hardware should have a 
smart-idle mode that acts the same way as the force-idle mode, just like 
the other IP blocks do; or

2. a software workaround, because we don't have a 32k counter device 
driver that implements runtime PM around counter reads.

Of course #2 would be rather pointless and the patch description tries to 
convey this too.

> > Furthermore, the PRCM will never request target idle for this IP block 
> > while the kernel is running, due to the sleep dependency that prevents 
> > the WKUP clockdomain from idling while the CORE_L3 clockdomain is 
> > active.  So we can safely leave the 32k sync timer in target-no-idle 
> > mode, even while we continue to access it.
> 
> Do you mean force-idle? Because accessing a module in no-idle is always 
> possible.

Thanks, that's indeed a description bug.

> > This workaround is implemented by defining a new hwmod flag,
> > HWMOD_ALWAYS_FORCE_SIDLE, that places the IP block into target
> > force-idle mode even when enabled.  The 32k sync timer hwmods are
> > marked with this flag for OMAP3 and OMAP4 SoCs.  (On OMAP2xxx, no OCP
> > header existed on the 32k sync timer.)
> 
> I still don't see the need for this flag. It looks to me that we are adding a
> redundant information and thus make things more complex than it should.
> 
> 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO),
> 
> It the real root cause of the problem. There is no need to re-encode that
> using an extra flag. Especially at hwmod level where the issue is at sysconfig
> level.
> 
> I did not really get the reason why you changed your mind on that point.
> 
> As soon as there is no SIDLE_SMART mode, what choice do we have but using the
> SIDLE_FORCE?

SIDLE_NO is the option that makes more sense to me.

Consider an IP block with SIDLE_NO and SIDLE_FORCE but without 
SIDLE_SMART.  When an initiator will access it, the default setting should 
be SIDLE_NO, for the reason that you identified above: "because accessing 
a module in no-idle is always possible."  On the other hand, we don't know 
when it's safe to access a module in SIDLE_FORCE unless we have additional 
information as to how the IP block handles the SIdleReq signal internally, 
and the characteristics of the clock domain in which it's integrated. For 
example, as mentioned earlier in the discussion on this patch, the AM335x 
documentation states "By definition, initiator may generate read/write 
transaction as long as it is out of IDLE state."

> BTW, I even think the HWMOD_SWSUP_SIDLE hwmod flag is useless now. It 
> was needed before probably because the idlemodes were wrongly populated.
> 
> In fact the hwmod flags is really there to *flag* some real HW bug we cannot
> figure out otherwise, but in that case, the sysc.idlemodes already contains
> all the information we need to set the proper mode during enable/disable.
> 
> Duplicating the information is always a source of confusion and might lead to
> nasty bugs due to the increase of complexity.

You may be misunderstanding the meaning of the HWMOD_SWSUP_SIDLE flag.  It 
was added to work around IP blocks that had broken smart-idle.  These 
modules definitely do exist; see for example the OMAP3 wdt2, usbhsotg, and 
usb_host_hs hwmods as some examples.

It might be reasonable to remove the HWMOD_SWSUP_SIDLE flag from the 32k 
counter and just test again for HWMOD_ALWAYS_FORCE_SIDLE in the module 
disable.  I'll take a look at this.

> > Another theoretically clean fix for this problem would be to implement
> > PM runtime-based control for 32k sync timer accesses.  These PM
> > runtime calls would need to located in a custom clocksource, since the
> > 32k sync timer is currently used as an MMIO clocksource.  But in
> > practice, there would be little benefit to doing so; and there would
> > be some cost, due to the addition of unnecessary lines of code and the
> > additional CPU overhead of the PM runtime and hwmod code - unnecessary
> > in this case.
> 
> I don't think that part is really relevant anymore.

Why?

> > Another possible fix would have been to modify the pm34xx.c code to
> > force the IP block idle before entering WFI.  But this would not have
> > been an acceptable approach: we are trying to remove this type of
> > centralized IP block idle control from the PM code.
> 
> Neither that one I guess. As soon as we consider force-idle to be equivalent
> to smart-idle, nothing more is needed.

But they are definitely not equivalent.


- Paul
Cousson, Benoit - June 14, 2012, 8:13 p.m.
On 6/14/2012 8:04 PM, Paul Walmsley wrote:
> Hi
>
> On Thu, 14 Jun 2012, Cousson, Benoit wrote:
>
>> On 6/11/2012 2:45 AM, Paul Walmsley wrote:
>>> Kuvin discovered that commit c8d82ff68fb6873691536cf33021977efbf5593c
>>
>> I guess you meant Kevin?
>
> ...
>
>>> The IP block itself pbobably does not have any native idle
>>
>> typo
>
> Thanks for noticing these, but they are not in what was sent out by the
> list server:
>
> http://www.spinics.net/lists/linux-omap/msg71503.html
>
> Some software/hardware issue on your end, maybe?

Hehe, funny, first time outlook server is editing email automatically... 
That's probably a new feature.

>
>>> handling at all, due to its simplicity.
>>
>> I don't think this description is really accurate, due to the usual confusing
>> definition of IDLE for the PRCM standpoint :-)
>> The counter_32k will follow PRCM requirement toward SIdleReq/SIdleAck.
>> It has to be "idle" for PRCM standpoint to allow the transition of the L4_WKUP
>> to inactive (SIdleAck=IDLE). And it will be functional as soon as the L4_WKUP
>> domain is active.
>>
>> The fact that the smartidle mode is missing does not change anything in the
>> way the PRCM handle the protocol. It is just an issue at IP level.
>>
>> In that case force-idle = smart-idle, just because this module does not have
>> anything to do to delay the SIdleAck upon SIdleReq request. The IP is probably
>> connecting the SIdleAck to the SIdleReq signal.
>> Please note that there are a bunch of IPs that are doing that even if they do
>> support the smartidle mode.
>
> Right, that's exactly my point -- perhaps made in an unclear way.
>
> My point was that the 32k counter IP block doesn't do anything with the
> incoming IdleReq signal to determine whether or not to assert IdleAck back
> to the PRCM.  But rather than implementing smart-idle mode to handle this,
> like the other IP blocks do, the only two modes implemented were the
> debugging modes, force-idle and no-idle.
>
> So depending on one's point of view, this patch is either:
>
> 1. a hardware bug workaround, because the hardware should have a
> smart-idle mode that acts the same way as the force-idle mode, just like
> the other IP blocks do; or

Well it is not even a real HW bug. That's more a recommendation that was 
not followed than a real HW bug. It is not the only one BTW, hence the 
number of flags we have in hwmod already :-)

A HW bug will be more for an IP that is supposed to have smart-idle but 
it does work properly and thus has to be disabled. In that case the mode 
is clearly not documented, so I'm not calling that an HW bug.

> 2. a software workaround, because we don't have a 32k counter device
> driver that implements runtime PM around counter reads.
>
> Of course #2 would be rather pointless and the patch description tries to
> convey this too.
>
>>> Furthermore, the PRCM will never request target idle for this IP block
>>> while the kernel is running, due to the sleep dependency that prevents
>>> the WKUP clockdomain from idling while the CORE_L3 clockdomain is
>>> active.  So we can safely leave the 32k sync timer in target-no-idle
>>> mode, even while we continue to access it.
>>
>> Do you mean force-idle? Because accessing a module in no-idle is always
>> possible.
>
> Thanks, that's indeed a description bug.

I'm not sure to follow you? My point was it should be: "So we can safely 
leave the 32k sync timer in force-idle mode, even while we continue to 
access it."
This is what the WA is doing.

>>> This workaround is implemented by defining a new hwmod flag,
>>> HWMOD_ALWAYS_FORCE_SIDLE, that places the IP block into target
>>> force-idle mode even when enabled.  The 32k sync timer hwmods are
>>> marked with this flag for OMAP3 and OMAP4 SoCs.  (On OMAP2xxx, no OCP
>>> header existed on the 32k sync timer.)
>>
>> I still don't see the need for this flag. It looks to me that we are adding a
>> redundant information and thus make things more complex than it should.
>>
>> 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO),
>>
>> It the real root cause of the problem. There is no need to re-encode that
>> using an extra flag. Especially at hwmod level where the issue is at sysconfig
>> level.
>>
>> I did not really get the reason why you changed your mind on that point.
>>
>> As soon as there is no SIDLE_SMART mode, what choice do we have but using the
>> SIDLE_FORCE?
>
> SIDLE_NO is the option that makes more sense to me.
>
> Consider an IP block with SIDLE_NO and SIDLE_FORCE but without
> SIDLE_SMART.  When an initiator will access it, the default setting should
> be SIDLE_NO, for the reason that you identified above: "because accessing
> a module in no-idle is always possible."  On the other hand, we don't know
> when it's safe to access a module in SIDLE_FORCE unless we have additional
> information as to how the IP block handles the SIdleReq signal internally,
> and the characteristics of the clock domain in which it's integrated.

Nope, that's not really accurate. The SIDLE mode is just the local 
configuration for the IP behavior during clock domain transition to 
idle. It does not have any influence on the re-activation of the clock.
In that case, both force-idle and smart-idle will behave the same. Only 
the transition to *idle* is supposed to be different between these two 
modes, hence the name.
What is missing here is the modulemode and the clock domain control:

counter_32k modulemode description:
"Module is managed automatically by HW according to clock domain 
transition. A clock domain sleep transition put module into idle. A 
wakeup domain transition put it back into function.
If CLKTRCTRL=3, any OCP access to module is always granted. Module 
clocks may be gated according to the clock domain state."

This is the modulemode and clkdm static dependency / dynamic on OMAP3/4 
that will guaranty that the OCP clock will be enabled upon any access to 
this L4_WKUP clock domain IPs. This has nothing to do with the SIDLE mode.

So SIDLE_FORCE is the right solution to still allow the proper 
clockdomain transition. It will behave exactly like a SIDLE_SMART.
And that's probably why they did not implement the smart-idle for that IP.

> For
> example, as mentioned earlier in the discussion on this patch, the AM335x
> documentation states "By definition, initiator may generate read/write
> transaction as long as it is out of IDLE state."

I saw that sentence, but what is this supposed to mean?


Moreover if AM335X require some specific trick, we should add some flags 
for that, and not force the usage of a flag that is mostly irrelevant 
for OMAP3 and 4 just because AM might need it.

>> BTW, I even think the HWMOD_SWSUP_SIDLE hwmod flag is useless now. It
>> was needed before probably because the idlemodes were wrongly populated.
>>
>> In fact the hwmod flags is really there to *flag* some real HW bug we cannot
>> figure out otherwise, but in that case, the sysc.idlemodes already contains
>> all the information we need to set the proper mode during enable/disable.
>>
>> Duplicating the information is always a source of confusion and might lead to
>> nasty bugs due to the increase of complexity.
>
> You may be misunderstanding the meaning of the HWMOD_SWSUP_SIDLE flag.  It
> was added to work around IP blocks that had broken smart-idle.  These
> modules definitely do exist; see for example the OMAP3 wdt2, usbhsotg, and
> usb_host_hs hwmods as some examples.

I know, I was mostly referring to the HWMOD_ALWAYS_FORCE_SIDLE. My point 
about HWMOD_SWSUP_SIDLE is: it is useless for that IP, not in general.

> It might be reasonable to remove the HWMOD_SWSUP_SIDLE flag from the 32k
> counter and just test again for HWMOD_ALWAYS_FORCE_SIDLE in the module
> disable.  I'll take a look at this.

Both should be removed as explained before. There is clearly no need for 
HWMOD_ALWAYS_FORCE_SIDLE.

We are already explicitly listing the limitation through the idlemodes 
attribute. Only SIDLE_FORCE and SIDLE_NO are supported, so we already 
know that SIDLE_FORCE is the proper way to fix that limitation for the 
current OMAPs.
Since there is no other IP with such limitation, we know as well that 
there will be no side effect.
If, in the future, some more IPs will have that limitation and will not 
work as expected, it will mean that some other HW bugs will be there, 
and only these ones will have to be flagged.

But my point is let's keep it simple and not try to anticipate future 
bugs. This flag is not require today, let's not add it.

>>> Another theoretically clean fix for this problem would be to implement
>>> PM runtime-based control for 32k sync timer accesses.  These PM
>>> runtime calls would need to located in a custom clocksource, since the
>>> 32k sync timer is currently used as an MMIO clocksource.  But in
>>> practice, there would be little benefit to doing so; and there would
>>> be some cost, due to the addition of unnecessary lines of code and the
>>> additional CPU overhead of the PM runtime and hwmod code - unnecessary
>>> in this case.
>>
>> I don't think that part is really relevant anymore.
>
> Why?

I'm not sure anymore now :-)
I guess it was because I thought this is mostly a HW issue, and it was 
detected because HWMOD_SWSUP_SIDLE was set previously. Without that, 
this IP will behave like a GPTIMER.

>>> Another possible fix would have been to modify the pm34xx.c code to
>>> force the IP block idle before entering WFI.  But this would not have
>>> been an acceptable approach: we are trying to remove this type of
>>> centralized IP block idle control from the PM code.
>>
>> Neither that one I guess. As soon as we consider force-idle to be equivalent
>> to smart-idle, nothing more is needed.
>
> But they are definitely not equivalent.

Indeed, but in that case, yes, and this is what really matter.

Regards,
Benoit
Paul Walmsley - June 15, 2012, 12:18 a.m.
On Thu, 14 Jun 2012, Cousson, Benoit wrote:

> On 6/14/2012 8:04 PM, Paul Walmsley wrote:
> > On Thu, 14 Jun 2012, Cousson, Benoit wrote:

(attribution lost)

> > > > Furthermore, the PRCM will never request target idle for this IP block
> > > > while the kernel is running, due to the sleep dependency that prevents
> > > > the WKUP clockdomain from idling while the CORE_L3 clockdomain is
> > > > active.  So we can safely leave the 32k sync timer in target-no-idle
> > > > mode, even while we continue to access it.
> > > 
> > > Do you mean force-idle? Because accessing a module in no-idle is always
> > > possible.
> > 
> > Thanks, that's indeed a description bug.
> 
> I'm not sure to follow you? My point was it should be: "So we can safely leave
> the 32k sync timer in force-idle mode, even while we continue to access it."
> This is what the WA is doing.

I am expressing appreciation to you for pointing out something incorrect 
in the patch description, which has been fixed in the current version of 
the patch.

> > SIDLE_NO is the option that makes more sense to me.
> > 
> > Consider an IP block with SIDLE_NO and SIDLE_FORCE but without
> > SIDLE_SMART.  When an initiator will access it, the default setting should
> > be SIDLE_NO, for the reason that you identified above: "because accessing
> > a module in no-idle is always possible."  On the other hand, we don't know
> > when it's safe to access a module in SIDLE_FORCE unless we have additional
> > information as to how the IP block handles the SIdleReq signal internally,
> > and the characteristics of the clock domain in which it's integrated.
> 

...

> This is the modulemode and clkdm static dependency / dynamic on OMAP3/4 that
> will guaranty that the OCP clock will be enabled upon any access to this
> L4_WKUP clock domain IPs. This has nothing to do with the SIDLE mode.

I want to implement a behavior that does not implicitly assume that an IP 
block without smart-idle will only exist inside clockdomains which are 
guaranteed to be active when the kernel is running.  That might be true 
for current WBU chips, but it seems unwise to make that assumption in 
general.

> > It might be reasonable to remove the HWMOD_SWSUP_SIDLE flag from the 32k
> > counter and just test again for HWMOD_ALWAYS_FORCE_SIDLE in the module
> > disable.  I'll take a look at this.
> 
> Both should be removed as explained before. There is clearly no need for
> HWMOD_ALWAYS_FORCE_SIDLE.
> 
> We are already explicitly listing the limitation through the idlemodes 
> attribute. Only SIDLE_FORCE and SIDLE_NO are supported, so we already 
> know that SIDLE_FORCE is the proper way to fix that limitation for the 
> current OMAPs. Since there is no other IP with such limitation, we know 
> as well that there will be no side effect. If, in the future, some more 
> IPs will have that limitation and will not work as expected, it will 
> mean that some other HW bugs will be there, and only these ones will 
> have to be flagged.
> 
> But my point is let's keep it simple and not try to anticipate future 
> bugs. This flag is not require today, let's not add it.

Which of these two behaviors do you feel is more "future-proof," in 
general:

1. Implementing a target idle policy that could break if a hardware 
   designer were to skip adding a target smart-idle mode to a module in 
   a clockdomain that might go idle while the kernel was active?

2. Implementing a target idle policy that is guaranteed to allow
   initiator accesses to succeed by definition?

considering that the implementation cost of either approach is identical?


- Paul
Cousson, Benoit - June 15, 2012, 1:28 p.m.
Hi Paul,

On 6/15/2012 2:18 AM, Paul Walmsley wrote:
> On Thu, 14 Jun 2012, Cousson, Benoit wrote:
>
>> On 6/14/2012 8:04 PM, Paul Walmsley wrote:
>>> On Thu, 14 Jun 2012, Cousson, Benoit wrote:
>
> (attribution lost)
>
>>>>> Furthermore, the PRCM will never request target idle for this IP block
>>>>> while the kernel is running, due to the sleep dependency that prevents
>>>>> the WKUP clockdomain from idling while the CORE_L3 clockdomain is
>>>>> active.  So we can safely leave the 32k sync timer in target-no-idle
>>>>> mode, even while we continue to access it.
>>>>
>>>> Do you mean force-idle? Because accessing a module in no-idle is always
>>>> possible.
>>>
>>> Thanks, that's indeed a description bug.
>>
>> I'm not sure to follow you? My point was it should be: "So we can safely leave
>> the 32k sync timer in force-idle mode, even while we continue to access it."
>> This is what the WA is doing.
>
> I am expressing appreciation to you for pointing out something incorrect
> in the patch description, which has been fixed in the current version of
> the patch.

OK, thanks for the clarification, I was confused by the sentence :-(.

>>> SIDLE_NO is the option that makes more sense to me.>>>
>>> Consider an IP block with SIDLE_NO and SIDLE_FORCE but without
>>> SIDLE_SMART.  When an initiator will access it, the default setting should
>>> be SIDLE_NO, for the reason that you identified above: "because accessing
>>> a module in no-idle is always possible."  On the other hand, we don't know
>>> when it's safe to access a module in SIDLE_FORCE unless we have additional
>>> information as to how the IP block handles the SIdleReq signal internally,
>>> and the characteristics of the clock domain in which it's integrated.
>
> ...
>
>> This is the modulemode and clkdm static dependency / dynamic on OMAP3/4 that
>> will guaranty that the OCP clock will be enabled upon any access to this
>> L4_WKUP clock domain IPs. This has nothing to do with the SIDLE mode.
>
> I want to implement a behavior that does not implicitly assume that an IP
> block without smart-idle will only exist inside clockdomains which are
> guaranteed to be active when the kernel is running.  That might be true
> for current WBU chips, but it seems unwise to make that assumption in
> general.

1- The fact that the IP does not support smart-idle does not change 
anything regarding the clock domain behavior.
2- So far the assumption is true for all the existing OMAP devices. 
Let's not anticipate something that will potentially never ever happen.

>>> It might be reasonable to remove the HWMOD_SWSUP_SIDLE flag from the 32k
>>> counter and just test again for HWMOD_ALWAYS_FORCE_SIDLE in the module
>>> disable.  I'll take a look at this.
>>
>> Both should be removed as explained before. There is clearly no need for
>> HWMOD_ALWAYS_FORCE_SIDLE.
>>
>> We are already explicitly listing the limitation through the idlemodes
>> attribute. Only SIDLE_FORCE and SIDLE_NO are supported, so we already
>> know that SIDLE_FORCE is the proper way to fix that limitation for the
>> current OMAPs. Since there is no other IP with such limitation, we know
>> as well that there will be no side effect. If, in the future, some more
>> IPs will have that limitation and will not work as expected, it will
>> mean that some other HW bugs will be there, and only these ones will
>> have to be flagged.
>>
>> But my point is let's keep it simple and not try to anticipate future
>> bugs. This flag is not require today, let's not add it.
>
> Which of these two behaviors do you feel is more "future-proof," in
> general:
>
> 1. Implementing a target idle policy that could break if a hardware
>     designer were to skip adding a target smart-idle mode to a module in
>     a clockdomain that might go idle while the kernel was active?

That's not fully accurate. Even with smart-idle implemented in this 
module, it will still go to idle automatically without any clock domain 
dependency properly handled.

The goal of this fix is to adapt the SIDLE management for such module, 
it will not solve the overall idle management that is anyway handled at 
clock domain level whatever the SIDLE implementation.

> 2. Implementing a target idle policy that is guaranteed to allow
>     initiator accesses to succeed by definition?

It will not, as explained before. This is the clock domain settings that 
will make the whole thing working properly.

> considering that the implementation cost of either approach is identical?

My point is simple, we should not add any new custom flag when all the 
information to detect such exception are already there in the current 
data and represent accurately what the HW is doing.

So far every IPs that do not support SIDLE_SMART can/should use 
SIDLE_FORCE instead.

Regards,
Benoit
Kevin Hilman - July 4, 2012, 2:27 p.m.
Paul Walmsley <paul@pwsan.com> writes:

> On Wed, 4 Jul 2012, Paul Walmsley wrote:
>
>> So the updated patch below uses a clockdomain data flag for this 
>> instead.
>
> Here's a version that's a little cleaner.  No functional changes.
>
>
> - Paul
>
> From: Paul Walmsley <paul@pwsan.com>
> Date: Wed, 4 Jul 2012 05:22:53 -0600
> Subject: [PATCH] ARM: OMAP2+: hwmod code/clockdomain data: fix 32K sync timer
>
> Kevin discovered that commit c8d82ff68fb6873691536cf33021977efbf5593c
> ("ARM: OMAP2/3: hwmod data: Add 32k-sync timer data to hwmod
> database") broke CORE idle on OMAP3.  This prevents device low power
> states.
>
> The root cause is that the 32K sync timer IP block does not support
> smart-idle mode[1], and so the hwmod code keeps the IP block in
> no-idle mode while it is active.  This in turn prevents the WKUP
> clockdomain from transitioning to idle.  There is a hardcoded sleep
> dependency that prevents the CORE_L3 and CORE_CM clockdomains from
> transitioning to idle when the WKUP clockdomain is active[2], so the
> chip cannot enter any device low power states.
>
> It turns out that there is no need to take the 32k sync timer out of
> idle.  The IP block itself probably does not have any native idle
> handling at all, due to its simplicity.  Furthermore, the PRCM will
> never request target idle for this IP block while the kernel is
> running, due to the sleep dependency that prevents the WKUP
> clockdomain from idling while the CORE_L3 clockdomain is active.  So
> we can safely leave the 32k sync timer in target-force-idle mode, even
> while we continue to access it.
>
> This workaround is implemented by defining a new clockdomain flag,
> CLKDM_ACTIVE_WITH_MPU, that indicates that the clockdomain is
> guaranteed to be active whenever the MPU is inactive.  If an IP
> block's main functional clock exists inside this clockdomain, and the
> IP block does not support smart-idle modes, then the hwmod code will
> place the IP block into target force-idle mode even when enabled.  The
> WKUP clockdomains on OMAP3/4 are marked with this flag.  (On OMAP2xxx,
> no OCP header existed on the 32k sync timer.)   Other clockdomains also
> should be marked with this flag, but those changes are deferred until
> a later merge window, to create a minimal fix.
>
> Another theoretically clean fix for this problem would be to implement
> PM runtime-based control for 32k sync timer accesses.  These PM
> runtime calls would need to located in a custom clocksource, since the
> 32k sync timer is currently used as an MMIO clocksource.  But in
> practice, there would be little benefit to doing so; and there would
> be some cost, due to the addition of unnecessary lines of code and the
> additional CPU overhead of the PM runtime and hwmod code - unnecessary
> in this case.
>
> Another possible fix would have been to modify the pm34xx.c code to
> force the IP block idle before entering WFI.  But this would not have
> been an acceptable approach: we are trying to remove this type of
> centralized IP block idle control from the PM code.
>
> This patch is a collaboration between Kevin Hilman <khilman@ti.com>
> and Paul Walmsley <paul@pwsan.com>.
>
> Thanks to Vaibhav Hiremath <hvaibhav@ti.com> for providing comments on
> an earlier version of this patch.  Thanks to Tero Kristo
> <t-kristo@ti.com> for identifying a bug in an earlier version of this
> patch.  Thanks to Benoît Cousson <b-cousson@ti.com> for identifying some
> bugs in an earlier version of this patch and for implementation comments.
>
> References:
>
> 1. Table 16-96 "REG_32KSYNCNT_SYSCONFIG" of the OMAP34xx TRM Rev. ZU
>    (SWPU223U), available from:
>    http://www.ti.com/pdfs/wtbu/OMAP34x_ES3.1.x_PUBLIC_TRM_vzU.zip
>
> 2. Table 4-72 "Sleep Dependencies" of the OMAP34xx TRM Rev. ZU
>    (SWPU223U)
>
> 3. ibid.
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Vaibhav Hiremath <hvaibhav@ti.com>
> Cc: Benoît Cousson <b-cousson@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Cc: Kevin Hilman <khilman@ti.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>

Tested-by: Kevin Hilman <khilman@ti.com>

I confirm this version is now allowing CORE to hit retention during
suspend.

Benoit, I hope this is OK with you.  We need a fix for this in v3.5
since this is last core bug preventing CORE retention in v3.5.

Kevin
Paul Walmsley - July 4, 2012, 2:53 p.m.
On Wed, 4 Jul 2012, Kevin Hilman wrote:

> Tested-by: Kevin Hilman <khilman@ti.com>
> 
> I confirm this version is now allowing CORE to hit retention during
> suspend.

Thanks, want me to add your S-o-b also?

- Paul
Cousson, Benoit - July 4, 2012, 4:01 p.m.
Hi Paul,

On 07/04/2012 02:48 PM, Paul Walmsley wrote:
> Hi Benoît,
> 
> On Fri, 15 Jun 2012, Cousson, Benoit wrote:
> 
>> My point is simple, we should not add any new custom flag when all the 
>> information to detect such exception are already there in the current 
>> data and represent accurately what the HW is doing.
> 
> This seems like the crux of the issue.  We don't have sufficient 
> information to detect this exception in our current data, in my view.  
> The software additionally must know when it can rely on a clockdomain 
> remaining active when the MPU is active.

Yes, indeed, but that will be accurate mostly for OMAP3.
On OMAP4, *in theory* the dynamic dependency should always ensure that a
clock domain will be accessible upon any initiator requests.
In that case the notion of a domain active when the MPU is active become
a little bit less accurate.
Except when we have to force the static dep because of HW bugs
workaround. In that case, we are in the same situation than OMAP3.

> Ideally the hardcoded clockdomain sleep dependencies would be encoded, and 
> we could test the intersection of those and the software-programmable 
> clockdomain sleep dependencies.  But such a change seems too complex for 
> the -rc cycle.  So the updated patch below uses a clockdomain data flag 
> for this instead.

Yep, I do agree that encoding that kind of information will require more
thought and much more code.

I have the feeling that some information about the IP idle behavior
should help figuring out that module like counter_32k, GPIO or mailbox
can be handled without real SW control.

This intermediate approach is thus good for the moment.


> From: Paul Walmsley <paul@pwsan.com>
> Date: Wed, 4 Jul 2012 05:22:53 -0600
> Subject: [PATCH] ARM: OMAP2+: hwmod code/clockdomain data: fix 32K sync timer

[...]

> @@ -1208,8 +1219,13 @@ static void _idle_sysc(struct omap_hwmod *oh)
>  	sf = oh->class->sysc->sysc_flags;
>  
>  	if (sf & SYSC_HAS_SIDLEMODE) {
> -		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
> -			HWMOD_IDLEMODE_FORCE : HWMOD_IDLEMODE_SMART;
> +		/* XXX What about HWMOD_IDLEMODE_SMART_WKUP? */

What do you mean here?

Regards,
Benoit
Cousson, Benoit - July 4, 2012, 4:14 p.m.
Hi Kevin,

On 07/04/2012 04:27 PM, Kevin Hilman wrote:

[...]

> Tested-by: Kevin Hilman <khilman@ti.com>
> 
> I confirm this version is now allowing CORE to hit retention during
> suspend.
> 
> Benoit, I hope this is OK with you.  We need a fix for this in v3.5
> since this is last core bug preventing CORE retention in v3.5.

On OMAP4 as well? I've just tried with the hwmod fix for AESS and USB
and still cannot reach RET.

Am I missing some other patches for OMAP4?

Regards,
Benoit
Tero Kristo - July 4, 2012, 4:41 p.m.
On Wed, 2012-07-04 at 18:14 +0200, Benoit Cousson wrote:
> Hi Kevin,
> 
> On 07/04/2012 04:27 PM, Kevin Hilman wrote:
> 
> [...]
> 
> > Tested-by: Kevin Hilman <khilman@ti.com>
> > 
> > I confirm this version is now allowing CORE to hit retention during
> > suspend.
> > 
> > Benoit, I hope this is OK with you.  We need a fix for this in v3.5
> > since this is last core bug preventing CORE retention in v3.5.
> 
> On OMAP4 as well? I've just tried with the hwmod fix for AESS and USB
> and still cannot reach RET.
> 
> Am I missing some other patches for OMAP4?

You need also fixes for sl2if and mcpdm in addition to above.

-Tero
Cousson, Benoit - July 4, 2012, 4:43 p.m.
On 07/04/2012 06:41 PM, Tero Kristo wrote:
> On Wed, 2012-07-04 at 18:14 +0200, Benoit Cousson wrote:
>> Hi Kevin,
>>
>> On 07/04/2012 04:27 PM, Kevin Hilman wrote:
>>
>> [...]
>>
>>> Tested-by: Kevin Hilman <khilman@ti.com>
>>>
>>> I confirm this version is now allowing CORE to hit retention during
>>> suspend.
>>>
>>> Benoit, I hope this is OK with you.  We need a fix for this in v3.5
>>> since this is last core bug preventing CORE retention in v3.5.
>>
>> On OMAP4 as well? I've just tried with the hwmod fix for AESS and USB
>> and still cannot reach RET.
>>
>> Am I missing some other patches for OMAP4?
> 
> You need also fixes for sl2if and mcpdm in addition to above.

Are they in some lo branch already?

Benoit
Cousson, Benoit - July 4, 2012, 4:57 p.m.
Hi Paul,

On 07/04/2012 02:53 PM, Paul Walmsley wrote:
> On Wed, 4 Jul 2012, Paul Walmsley wrote:
> 
>> So the updated patch below uses a clockdomain data flag for this 
>> instead.
> 
> Here's a version that's a little cleaner.  No functional changes.

[...]

> @@ -1141,8 +1144,16 @@ static void _enable_sysc(struct omap_hwmod *oh)
>  	sf = oh->class->sysc->sysc_flags;
>  
>  	if (sf & SYSC_HAS_SIDLEMODE) {
> -		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
> -			HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
> +		clkdm_act = ((oh->clkdm &&
> +			      oh->clkdm->flags & CLKDM_ACTIVE_WITH_MPU) ||
> +			     (oh->_clk->clkdm &&

This is crashing on OMAP4 due to a NULL oh->_clk that can happen on some
hwmod.

+			     (oh->_clk && oh->_clk->clkdm &&

Is fixing the issue.


Regards,
Benoit


> +			      oh->_clk->clkdm->flags & CLKDM_ACTIVE_WITH_MPU));
> +		if (clkdm_act && !(oh->class->sysc->idlemodes &
> +				   (SIDLE_SMART | SIDLE_SMART_WKUP)))
> +			idlemode = HWMOD_IDLEMODE_FORCE;
> +		else
> +			idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
> +				HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
>  		_set_slave_idlemode(oh, idlemode, &v);
>  	}
>
Paul Walmsley - July 4, 2012, 6:59 p.m.
Hi Benoît

On Wed, 4 Jul 2012, Benoit Cousson wrote:

> > @@ -1141,8 +1144,16 @@ static void _enable_sysc(struct omap_hwmod *oh)
> >  	sf = oh->class->sysc->sysc_flags;
> >  
> >  	if (sf & SYSC_HAS_SIDLEMODE) {
> > -		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
> > -			HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
> > +		clkdm_act = ((oh->clkdm &&
> > +			      oh->clkdm->flags & CLKDM_ACTIVE_WITH_MPU) ||
> > +			     (oh->_clk->clkdm &&
> 
> This is crashing on OMAP4 due to a NULL oh->_clk that can happen on some
> hwmod.
> 
> +			     (oh->_clk && oh->_clk->clkdm &&
> 
> Is fixing the issue.

Thanks, just made the change and pushed the patch up to 
git://git.pwsan.com/linux-2.6 in the branch 'omap_fixes_c_3.5rc'


- Paul
Paul Walmsley - July 4, 2012, 7:02 p.m.
On Wed, 4 Jul 2012, Benoit Cousson wrote:

> On 07/04/2012 06:41 PM, Tero Kristo wrote:
> > On Wed, 2012-07-04 at 18:14 +0200, Benoit Cousson wrote:
> >>
> >> On OMAP4 as well? I've just tried with the hwmod fix for AESS and USB
> >> and still cannot reach RET.
> >>
> >> Am I missing some other patches for OMAP4?
> > 
> > You need also fixes for sl2if and mcpdm in addition to above.
> 
> Are they in some lo branch already?

The old version of these patches are in the 'sl2if_mcpdm_reset_fix_3.5rc` 
branch of git://git.pwsan.com/linux-2.6 - they haven't yet been updated 
and tested per Benoît's comments.  If someone else wants to take that over 
for 3.5-rc, by all means please do.


- Paul
Paul Walmsley - July 4, 2012, 7:05 p.m.
Hi Benoît

On Wed, 4 Jul 2012, Benoit Cousson wrote:

> > From: Paul Walmsley <paul@pwsan.com>
> > Date: Wed, 4 Jul 2012 05:22:53 -0600
> > Subject: [PATCH] ARM: OMAP2+: hwmod code/clockdomain data: fix 32K sync timer
> 
> [...]
> 
> > @@ -1208,8 +1219,13 @@ static void _idle_sysc(struct omap_hwmod *oh)
> >  	sf = oh->class->sysc->sysc_flags;
> >  
> >  	if (sf & SYSC_HAS_SIDLEMODE) {
> > -		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
> > -			HWMOD_IDLEMODE_FORCE : HWMOD_IDLEMODE_SMART;
> > +		/* XXX What about HWMOD_IDLEMODE_SMART_WKUP? */
> 
> What do you mean here?

We're not programming IP block target idle modes to smart idle + wakeup on 
OMAP4.  We're only programming them to smart idle :-(


- Paul
Kevin Hilman - July 5, 2012, 10:06 p.m.
Paul Walmsley <paul@pwsan.com> writes:

> Hi Benoît
>
> On Wed, 4 Jul 2012, Benoit Cousson wrote:
>
>> > @@ -1141,8 +1144,16 @@ static void _enable_sysc(struct omap_hwmod *oh)
>> >  	sf = oh->class->sysc->sysc_flags;
>> >  
>> >  	if (sf & SYSC_HAS_SIDLEMODE) {
>> > -		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
>> > -			HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
>> > +		clkdm_act = ((oh->clkdm &&
>> > +			      oh->clkdm->flags & CLKDM_ACTIVE_WITH_MPU) ||
>> > +			     (oh->_clk->clkdm &&
>> 
>> This is crashing on OMAP4 due to a NULL oh->_clk that can happen on some
>> hwmod.
>> 
>> +			     (oh->_clk && oh->_clk->clkdm &&
>> 
>> Is fixing the issue.
>
> Thanks, just made the change and pushed the patch up to 
> git://git.pwsan.com/linux-2.6 in the branch 'omap_fixes_c_3.5rc'

OK, to ensure this fix gets into v3.5-rc, I'm taking the version from
this branch and queuing up as a PM fix Tony.

Kevin

Patch

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index bf86f7e..096474c 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1124,10 +1124,12 @@  static struct omap_hwmod_addr_space * __init _find_mpu_rt_addr_space(struct omap
  * _enable_sysc - try to bring a module out of idle via OCP_SYSCONFIG
  * @oh: struct omap_hwmod *
  *
- * If module is marked as SWSUP_SIDLE, force the module out of slave
- * idle; otherwise, configure it for smart-idle.  If module is marked
- * as SWSUP_MSUSPEND, force the module out of master standby;
- * otherwise, configure it for smart-standby.  No return value.
+ * Ensure that the OCP_SYSCONFIG register for the IP block represented
+ * by @oh is set to indicate to the PRCM that the IP block is active.
+ * Usually this means placing the module into smart-idle mode and
+ * smart-standby, but if there is a bug in the automatic idle handling
+ * for the IP block, it may need to be placed into the force-idle or
+ * no-idle variants of these modes.  No return value.
  */
 static void _enable_sysc(struct omap_hwmod *oh)
 {
@@ -1141,8 +1143,11 @@  static void _enable_sysc(struct omap_hwmod *oh)
 	sf = oh->class->sysc->sysc_flags;
 
 	if (sf & SYSC_HAS_SIDLEMODE) {
-		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
-			HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
+		if (oh->flags & HWMOD_ALWAYS_FORCE_SIDLE)
+			idlemode = HWMOD_IDLEMODE_FORCE;
+		else
+			idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
+				HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
 		_set_slave_idlemode(oh, idlemode, &v);
 	}
 
diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index b26d3c9..f8ac9e7 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -2018,7 +2018,7 @@  static struct omap_hwmod omap3xxx_counter_32k_hwmod = {
 	.name		= "counter_32k",
 	.class		= &omap3xxx_counter_hwmod_class,
 	.clkdm_name	= "wkup_clkdm",
-	.flags		= HWMOD_SWSUP_SIDLE,
+	.flags		= HWMOD_ALWAYS_FORCE_SIDLE | HWMOD_SWSUP_SIDLE,
 	.main_clk	= "wkup_32k_fck",
 	.prcm		= {
 		.omap2	= {
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 950454a..4aaaa84 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -408,7 +408,7 @@  static struct omap_hwmod omap44xx_counter_32k_hwmod = {
 	.name		= "counter_32k",
 	.class		= &omap44xx_counter_hwmod_class,
 	.clkdm_name	= "l4_wkup_clkdm",
-	.flags		= HWMOD_SWSUP_SIDLE,
+	.flags		= HWMOD_ALWAYS_FORCE_SIDLE | HWMOD_SWSUP_SIDLE,
 	.main_clk	= "sys_32k_ck",
 	.prcm = {
 		.omap4 = {
diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-omap/include/plat/omap_hwmod.h
index c835b71..038c0d7 100644
--- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
+++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
@@ -409,6 +409,14 @@  struct omap_hwmod_omap4_prcm {
  *     in order to complete the reset. Optional clocks will be disabled
  *     again after the reset.
  * HWMOD_16BIT_REG: Module has 16bit registers
+ * HWMOD_ALWAYS_FORCE_SIDLE: Always program this module's SIDLEMODE to
+ *     force-idle mode, even when enabled.  This is needed for IP blocks
+ *     which do not support smart idle, which do not have a software
+ *     controllable functional or interface clock, and which the PRCM
+ *     will not assert SIdleReq until the kernel is not currently
+ *     running on the chip (e.g., the MPU is in idle).  For such modules,
+ *     fine-grained PM runtime-based idle control is simply a waste of
+ *     CPU cycles.
  */
 #define HWMOD_SWSUP_SIDLE			(1 << 0)
 #define HWMOD_SWSUP_MSTANDBY			(1 << 1)
@@ -419,6 +427,7 @@  struct omap_hwmod_omap4_prcm {
 #define HWMOD_NO_IDLEST				(1 << 6)
 #define HWMOD_CONTROL_OPT_CLKS_IN_RESET		(1 << 7)
 #define HWMOD_16BIT_REG				(1 << 8)
+#define HWMOD_ALWAYS_FORCE_SIDLE		(1 << 9)
 
 /*
  * omap_hwmod._int_flags definitions