ARM: OMAP2+: hwmod code/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 blocks 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 oversight. 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.

This patch is a collaboration between Kevin Hilman<[email protected]> and Paul Walmsley<[email protected]>.

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<[email protected]>

Cc Vaibhav Hiremath<[email protected]>

Cc Beno?t Cousson<[email protected]>

Tested-by Kevin Hilman<[email protected]>

Signed-off-by Kevin Hilman<[email protected]>

Signed-off-by Paul Walmsley<[email protected]>

---
  arch/arm/mach-omap2/omap_hwmod.c             |   17 +++++++++++------
  arch/arm/mach-omap2/omap_hwmod_3xxx_data.c   |    1 +
  arch/arm/mach-omap2/omap_hwmod_44xx_data.c   |    1 +
  arch/arm/plat-omap/include/plat/omap_hwmod.h |    9 +++++++++
  4 files changed, 22 insertions(+), 6 deletions(-)

arch/arm/mach-omap2/omap_hwmod.c +11 -6

index bf86f7e..096474c 100644
@@ -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);
  	}
 

arch/arm/mach-omap2/omap_hwmod_3xxx_data.c +1 -0

index fd48797..fddf63e 100644
@@ -2029,6 +2029,7 @@ static struct omap_hwmod omap3xxx_counter_32k_hwmod = {
  			.idlest_idle_bit = OMAP3430_ST_32KSYNC_SHIFT,
  		},
  	},
+	.flags		= HWMOD_ALWAYS_FORCE_SIDLE,
  };
 
  /*

arch/arm/mach-omap2/omap_hwmod_44xx_data.c +0 -0

index 950454a..900b4f0 100644

Quoted 4 lines · Paul Walmsley

This patch is effectively a workaround for a hardware oversight. 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.

I'm not sure to follow you here :-)

force-idle is almost equivalent to smart-idle, so it is the right workaround when smart-idle is not implemented.

In force-idle idleack = idlereq. Whereas in smart-idle idleack = idlereq if internal /OCP activity is completed.

So in fact, I'm wondering if a new flag is needed. We can potentially apply that if idlemodes == (SIDLE_FORCE | SIDLE_NO).

We need to check which IP will have that to ensure that does not add any side-effects.

BTW, please note that the current idlemodes flags are wrong for the counter 32k. I've just figured out that fixing the STANDBY flags for some OMAP5 IPs. I'll add that fix in my WIP OMAP4 hwmod data cleanup for 3.6.

Something like that:

Review

Quoted 4 lines

  			.context_offs = OMAP4_RM_WKUP_SYNCTIMER_CONTEXT_OFFSET,
  		},
  	},

(quoted diff) +1 -0

+	.flags		= HWMOD_ALWAYS_FORCE_SIDLE,

I'm confused by the location and the value, both linus/master and lo/master branches does already have a flag for the counter_32k:

/* counter_32k */

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,
        .main_clk       = "sys_32k_ck",
        .prcm = {
                .omap4 = {
                        .clkctrl_offs = OMAP4_CM_WKUP_SYNCTIMER_CLKCTRL_OFFSET,
                        .context_offs = OMAP4_RM_WKUP_SYNCTIMER_CONTEXT_OFFSET,
                },
        },
};

So the patch should be slightly different, I guess ?

(quoted diff) +1 -1

 	.class          = &omap44xx_counter_hwmod_class,
 	.clkdm_name     = "l4_wkup_clkdm",
-	.flags          = HWMOD_SWSUP_SIDLE,
+	.flags		= HWMOD_ALWAYS_FORCE_SIDLE,
 	.main_clk       = "sys_32k_ck",
 	.prcm = {

This is the same issue for OMAP3 data.

What base branch are you using?

Quoted 5 lines · Benoit

So in fact, I'm wondering if a new flag is needed. We can potentially apply that if idlemodes == (SIDLE_FORCE | SIDLE_NO).

We need to check which IP will have that to ensure that does not add any side-effects.

I guess that means me :-)

Hello Beno?t

Quoted 8 lines · Paul Walmsley

Quoted 5 lines · Benoit

So in fact, I'm wondering if a new flag is needed. We can potentially apply that if idlemodes == (SIDLE_FORCE | SIDLE_NO).

We need to check which IP will have that to ensure that does not add any side-effects.

Quoted 1 line

I guess that means me :-)

I looked at a few TRMs. Based on that incomplete survey, the above behavior would probably work for existing chips.

But it seems perverse to assume that it is generally safe to set an IP block to force-idle while it's being used. That seems really dependent on the IP block's integration.

So in terms of a general fix, the flag approach seems safest to me in the long run.

The other advantage of using a flag is that it indicates that this is a hardware workaround; something that it would be nice to fix in future chips.

Hello Beno?t

Quoted 8 lines · Benoit

Quoted 4 lines · Paul Walmsley

This patch is effectively a workaround for a hardware oversight. 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.

I'm not sure to follow you here :-)

I should rewrite that paragraph. It is not as clear as it could be.

Quoted 5 lines

force-idle is almost equivalent to smart-idle, so it is the right workaround when smart-idle is not implemented.

In force-idle idleack = idlereq. Whereas in smart-idle idleack = idlereq if internal /OCP activity is completed.

It would be nice if the hardware people just implemented smart-idle on all IP blocks. Section 3.1.1.1.2 "Module-Level Clock Management" of The OMAP4430 TRM Rev. vAA states:

"Smart-idle mode is the preferred mode of operation, while forced-idle and no-idle modes are intended for debugging purposes."

Then no flags or software workarounds would be needed, except for debugging.

Quoted 4 lines

So in fact, I'm wondering if a new flag is needed. We can potentially apply that if idlemodes == (SIDLE_FORCE | SIDLE_NO).

We need to check which IP will have that to ensure that does not add any side-effects.

I guess that means me :-)

Quoted 6 lines

BTW, please note that the current idlemodes flags are wrong for the counter 32k. I've just figured out that fixing the STANDBY flags for some OMAP5 IPs. I'll add that fix in my WIP OMAP4 hwmod data cleanup for 3.6.

Something like that:

Okay will queue up a fix for 3.5-rc.

Quoted 33 lines

Review

Quoted 4 lines

   			.context_offs = OMAP4_RM_WKUP_SYNCTIMER_CONTEXT_OFFSET,
   		},
   	},

(quoted diff) +1 -0

+	.flags		= HWMOD_ALWAYS_FORCE_SIDLE,

I'm confused by the location and the value, both linus/master and lo/master branches does already have a flag for the counter_32k:

/* counter_32k */

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,
         .main_clk       = "sys_32k_ck",
         .prcm = {
                 .omap4 = {
                         .clkctrl_offs = OMAP4_CM_WKUP_SYNCTIMER_CLKCTRL_OFFSET,
                         .context_offs = OMAP4_RM_WKUP_SYNCTIMER_CONTEXT_OFFSET,
                 },
         },
};

So the patch should be slightly different, I guess ?

.class =&omap44xx_counter_hwmod_class,
.clkdm_name = "l4_wkup_clkdm",

(quoted diff) +1 -1

-	.flags          = HWMOD_SWSUP_SIDLE,
+	.flags		= HWMOD_ALWAYS_FORCE_SIDLE,
  	.main_clk       = "sys_32k_ck",
  	.prcm = {

Quoted 3 lines

This is the same issue for OMAP3 data.

What base branch are you using?

The wrong one, evidently. Will ensure this is fixed.

- Paul