[PATCH] ARM: OMAP2+: hwmod code/data: fix 32K sync timer

[PATCH] ARM: OMAP2+: hwmod code/data: fix 32K sync timer

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7456 bytes --]


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 <khilman@ti.com>
and Paul Walmsley <paul@pwsan.com>.

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

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 fd48797..fddf63e 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -2029,6 +2029,7 @@ static struct omap_hwmod omap3xxx_counter_32k_hwmod = {
 			.idlest_idle_bit = OMAP3430_ST_32KSYNC_SHIFT,
 		},
 	},
+	.flags		= HWMOD_ALWAYS_FORCE_SIDLE,
 };
 
 /*
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 950454a..900b4f0 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -416,6 +416,7 @@ static struct omap_hwmod omap44xx_counter_32k_hwmod = {
 			.context_offs = OMAP4_RM_WKUP_SYNCTIMER_CONTEXT_OFFSET,
 		},
 	},
+	.flags		= HWMOD_ALWAYS_FORCE_SIDLE,
 };
 
 /*
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
-- 
1.7.10

Re: [PATCH] ARM: OMAP2+: hwmod code/data: fix 32K sync timer

[Cousson, Benoit]

Hi Paul,

On 5/25/2012 11:56 PM, Paul Walmsley wrote:
> 
> 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.

On OMAP4, that hard coded dependency does not exist anymore from WKUP to CORE.
That being said, if the WKUP cannot go to INACTIVE, the chip cannot go to OFF anyway :-(

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

The OMAP4 PRCM spec indeed list a couple of modules (counter_32k, timer12 and wdt1) that are always under HW auto modulemode. The modulemode is read only in this case.
The modules are thus following the clock domain transitions.

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

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.

On OMAP4, since timer12 and wdt1 are secured IPs not usable on GP, counter_32k seems to be the only one that does support only these two modes.

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:

--- a/outputs/omap4430/2.0/omap_hwmod_44xx_data.c
+++ b/outputs/omap4430/2.0/omap_hwmod_44xx_data.c
@@ -412,8 +412,7 @@ static struct omap_hwmod_class_sysconfig omap44xx_counter_sysc = {
        .rev_offs       = 0x0000,
        .sysc_offs      = 0x0004,
        .sysc_flags     = SYSC_HAS_SIDLEMODE,
-       .idlemodes      = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART |
-                          SIDLE_SMART_WKUP),
+       .idlemodes      = (SIDLE_FORCE | SIDLE_NO),
        .sysc_fields    = &omap_hwmod_sysc_type1,
 };

> 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<khilman@ti.com>
> and Paul Walmsley<paul@pwsan.com>.
> 
> 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>
> 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   |    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(-)
> 
> 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 fd48797..fddf63e 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> @@ -2029,6 +2029,7 @@ static struct omap_hwmod omap3xxx_counter_32k_hwmod = {
>   			.idlest_idle_bit = OMAP3430_ST_32KSYNC_SHIFT,
>   		},
>   	},
> +	.flags		= HWMOD_ALWAYS_FORCE_SIDLE,
>   };
> 
>   /*
> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> index 950454a..900b4f0 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> @@ -416,6 +416,7 @@ static struct omap_hwmod omap44xx_counter_32k_hwmod = {
>   			.context_offs = OMAP4_RM_WKUP_SYNCTIMER_CONTEXT_OFFSET,
>   		},
>   	},
> +	.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",
-	.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?

Regards,
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: OMAP2+: hwmod code/data: fix 32K sync timer

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3008 bytes --]

Hello Benoît

On Tue, 29 May 2012, Cousson, Benoit wrote:

> On 5/25/2012 11:56 PM, Paul Walmsley wrote:
>
> > 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.

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

> 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 :-)

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

> >   			.context_offs = OMAP4_RM_WKUP_SYNCTIMER_CONTEXT_OFFSET,
> >   		},
> >   	},
> > +	.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",
> -	.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?

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


- Paul

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: OMAP2+: hwmod code/data: fix 32K sync timer

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 940 bytes --]

Hello Benoît

On Tue, 5 Jun 2012, Paul Walmsley wrote:

> On Tue, 29 May 2012, Cousson, Benoit wrote:
> 
> > 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 :-)

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.


- Paul

Re: [PATCH] ARM: OMAP2+: hwmod code/data: fix 32K sync timer

[Cousson, Benoit]

On 6/6/2012 2:28 AM, Paul Walmsley wrote:
> Hello Benoît
>
> On Tue, 5 Jun 2012, Paul Walmsley wrote:
>
>> On Tue, 29 May 2012, Cousson, Benoit wrote:
>>
>>> 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 :-)
>
> 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.

Not really, because this behavior is implemented in the IP itself. The 
point is the this IP is so dumb that force_idle = smart_idle. There is 
not reason to delay the idle_ack since this IP does not do any internal 
processing. That's probably why it was not implemented in the HW.

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

Well, for the long run we can expect the HW to be fixed, so since we do 
have only one IP with that and we can detect that with the current 
flags, I still do not think it is needed.

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

Yeah, I'm still not convinced, but it is not a big deal either so it is 
up to you.

Regards,
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: OMAP2+: hwmod code/data: fix 32K sync timer

[Cousson, Benoit]

Salut Paul,

On 6/6/2012 12:55 AM, Paul Walmsley wrote:
> Hello Benoît
>
> On Tue, 29 May 2012, Cousson, Benoit wrote:
>
>> On 5/25/2012 11:56 PM, Paul Walmsley wrote:
>>
>>> 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.
>
>> 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.

I know... I'm dreaming as well of HW that stick to the spec :-)

>> 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 :-)

Hehe, not really, I did it for OMAP4/5. counter_32k is the only IP with 
that broken mode. GPU and IVA does have some missing modes as well but 
at least smart_idle is there.

Regards,
Benoit

>> 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.
>
>>>    			.context_offs = OMAP4_RM_WKUP_SYNCTIMER_CONTEXT_OFFSET,
>>>    		},
>>>    	},
>>> +	.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",
>> -	.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?
>
> The wrong one, evidently.  Will ensure this is fixed.
>
>
> - Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html