[PATCH 0/2] OMAP3: wdtimer: fix wdtimer blocking CORE idle

[PATCH 0/2] OMAP3: wdtimer: fix wdtimer blocking CORE idle

[Kalle Jokiniemi]

Currently the wdtimer2 active usage blocks CORE idle transitions.
This patch set fixes the issue by changing the wdtimer sidlemode
form smart idle to force idle.

Improvements were needed to omap_hwmod sysconfig configuration
mechanism to allow this setting.

Kalle Jokiniemi (2):
  OMAP: hw_mod: consider supported idlemodes in enable_sysc
  OMAP3: wdtimer: Disable SMART idle mode

 arch/arm/mach-omap2/omap_hwmod.c           |   28 ++++++++++++++++++++--------
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    7 +++++--
 2 files changed, 25 insertions(+), 10 deletions(-)

[PATCH 1/2] OMAP: hw_mod: consider supported idlemodes in enable_sysc

[Kalle Jokiniemi]

Current implementation of the hw_mod sysconfig configuration
always sets up SMART idle mode for HW modules. However, a certain
HW module may not support smart idle mode or may have
HW bugs that prevent using smart idle mode.

This patch introduces a mechanism to check supported
idle modes and select in following priority:
1. Smart idle mode
2. Force idle mode
3. No idle mode.

Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |   28 ++++++++++++++++++++--------
 1 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 028efda..ef77697 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -757,7 +757,7 @@ static void __iomem * __init _find_mpu_rt_base(struct omap_hwmod *oh, u8 index)
  */
 static void _enable_sysc(struct omap_hwmod *oh)
 {
-	u8 idlemode, sf;
+	u8 idlemode, sf, idlemode_flags;
 	u32 v;
 
 	if (!oh->class->sysc)
@@ -765,17 +765,28 @@ static void _enable_sysc(struct omap_hwmod *oh)
 
 	v = oh->_sysc_cache;
 	sf = oh->class->sysc->sysc_flags;
+	idlemode_flags = oh->class->sysc->idlemodes;
+
+	/* Set the idlemode, priority order: 1. SMART 2. FORCE 3. NOIDLE */
+	if (idlemode_flags & SIDLE_SMART)
+		idlemode = HWMOD_IDLEMODE_SMART;
+	else if (idlemode_flags & SIDLE_FORCE)
+		idlemode = HWMOD_IDLEMODE_FORCE;
+	else
+		idlemode = HWMOD_IDLEMODE_NO;
 
 	if (sf & SYSC_HAS_SIDLEMODE) {
-		idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
-			HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
-		_set_slave_idlemode(oh, idlemode, &v);
+		if (oh->flags & HWMOD_SWSUP_SIDLE)
+			_set_slave_idlemode(oh, HWMOD_IDLEMODE_NO, &v);
+		else
+			_set_slave_idlemode(oh, idlemode, &v);
 	}
 
 	if (sf & SYSC_HAS_MIDLEMODE) {
-		idlemode = (oh->flags & HWMOD_SWSUP_MSTANDBY) ?
-			HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
-		_set_master_standbymode(oh, idlemode, &v);
+		if (oh->flags & HWMOD_SWSUP_MSTANDBY)
+			_set_master_standbymode(oh, HWMOD_IDLEMODE_NO, &v);
+		else
+			_set_master_standbymode(oh, idlemode, &v);
 	}
 
 	/*
@@ -788,7 +799,8 @@ static void _enable_sysc(struct omap_hwmod *oh)
 		_set_clockactivity(oh, oh->class->sysc->clockact, &v);
 
 	/* If slave is in SMARTIDLE, also enable wakeup */
-	if ((sf & SYSC_HAS_SIDLEMODE) && !(oh->flags & HWMOD_SWSUP_SIDLE))
+	if ((sf & SYSC_HAS_SIDLEMODE) && !(oh->flags & HWMOD_SWSUP_SIDLE) &&
+			idlemode_flags & SIDLE_SMART)
 		_enable_wakeup(oh, &v);
 
 	_write_sysconfig(v, oh);
-- 
1.7.1

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

[PATCH 2/2] OMAP3: wdtimer: Disable SMART idle mode

[Kalle Jokiniemi]

The smart idle mode in wdtimer2 prevents CORE power domain
idle transitions. Disable SMART idle mode to allow
FORCE idle mode to be used.

Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
---
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index 196a834..0be56bd 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -1240,7 +1240,10 @@ static struct omap_hwmod_ocp_if omap3xxx_l4_wkup__wd_timer2 = {
 /*
  * 'wd_timer' class
  * 32-bit watchdog upward counter that generates a pulse on the reset pin on
- * overflow condition
+ * overflow condition.
+ *
+ * Disallow SIDLE_SMART to use SIDLE_FORCE instead as the CORE power domain
+ * sleep transitions only work when wdtimer2 is in SIDLE_FORCE mode.
  */
 
 static struct omap_hwmod_class_sysconfig omap3xxx_wd_timer_sysc = {
@@ -1250,7 +1253,7 @@ static struct omap_hwmod_class_sysconfig omap3xxx_wd_timer_sysc = {
 	.sysc_flags	= (SYSC_HAS_SIDLEMODE | SYSC_HAS_EMUFREE |
 			   SYSC_HAS_ENAWAKEUP | SYSC_HAS_SOFTRESET |
 			   SYSC_HAS_AUTOIDLE | SYSC_HAS_CLOCKACTIVITY),
-	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+	.idlemodes	= (SIDLE_FORCE | SIDLE_NO), /* SIDLE_SMART disabled */
 	.sysc_fields    = &omap_hwmod_sysc_type1,
 };
 
-- 
1.7.1

Re: [PATCH 0/2] OMAP3: wdtimer: fix wdtimer blocking CORE idle

[Paul Walmsley]

Thanks Kalle for tracking this down,

could you try this patch along with the patch from this message:

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

and see if it also fixes the problem?

This patch should also prevent the hwmod code from putting the WDT2 into 
smart-idle mode.


- Paul


---
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index 457df3e..4366b85 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -1294,6 +1294,7 @@ static struct omap_hwmod omap3xxx_wd_timer2_hwmod = {
 	.slaves		= omap3xxx_wd_timer2_slaves,
 	.slaves_cnt	= ARRAY_SIZE(omap3xxx_wd_timer2_slaves),
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 /* UART common */
-- 
1.7.2.3

RE: [PATCH 0/2] OMAP3: wdtimer: fix wdtimer blocking CORE idle

[kalle.jokiniemi]

> -----Original Message-----
> From: ext Paul Walmsley [mailto:paul@pwsan.com]
> Sent: 10. maaliskuuta 2011 11:50
> To: Jokiniemi Kalle (Nokia-MS/Tampere)
> Cc: khilman@ti.com; linux-omap@vger.kernel.org; Koskinen Ilkka (Nokia-
> MS/Tampere); jhnikula@gmail.com; b-cousson@ti.com
> Subject: Re: [PATCH 0/2] OMAP3: wdtimer: fix wdtimer blocking CORE idle
> 
> 
> Thanks Kalle for tracking this down,
> 
> could you try this patch along with the patch from this message:
> 
>   http://www.spinics.net/lists/linux-omap/msg48115.html
> 
> and see if it also fixes the problem?

It works as well it seems :)

I assume you prefer this fix to be applied, so I'll send patches on this.

- Kalle


> 
> This patch should also prevent the hwmod code from putting the WDT2 into
> smart-idle mode.
> 
> 
> - Paul
> 
> 
> ---
>  arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> index 457df3e..4366b85 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> @@ -1294,6 +1294,7 @@ static struct omap_hwmod
> omap3xxx_wd_timer2_hwmod = {
>  	.slaves		= omap3xxx_wd_timer2_slaves,
>  	.slaves_cnt	= ARRAY_SIZE(omap3xxx_wd_timer2_slaves),
>  	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
> +	.flags		= HWMOD_SWSUP_SIDLE,
>  };
> 
>  /* UART common */
> --
> 1.7.2.3

RE: [PATCH 0/2] OMAP3: wdtimer: fix wdtimer blocking CORE idle

[Paul Walmsley]

On Thu, 10 Mar 2011, kalle.jokiniemi@nokia.com wrote:

> It works as well it seems :)

Cool.

> I assume you prefer this fix to be applied, so I'll send patches on this.

Yes, I'd prefer this approach.


regards,

- Paul

Re: [PATCH 0/2] OMAP3: wdtimer: fix wdtimer blocking CORE idle

[Cousson, Benoit]

Paul,

On 3/10/2011 11:28 AM, kalle.jokiniemi@nokia.com wrote:
>
>
>> -----Original Message-----
>> From: ext Paul Walmsley [mailto:paul@pwsan.com]
>> Sent: 10. maaliskuuta 2011 11:50
>> To: Jokiniemi Kalle (Nokia-MS/Tampere)
>> Cc: khilman@ti.com; linux-omap@vger.kernel.org; Koskinen Ilkka (Nokia-
>> MS/Tampere); jhnikula@gmail.com; b-cousson@ti.com
>> Subject: Re: [PATCH 0/2] OMAP3: wdtimer: fix wdtimer blocking CORE idle
>>
>>
>> Thanks Kalle for tracking this down,
>>
>> could you try this patch along with the patch from this message:
>>
>>    http://www.spinics.net/lists/linux-omap/msg48115.html

I have some question with that patch.
By using pm_runtime API, you are suppose to idle the whole IP.
In this case, just because you cannot idle a wdt that is running, it 
works, but the point is that the pm_runtime state will not reflect the 
HW state.
For pm_runtime point of view, the wdt is supposed to be fully idle (both 
fclk and iclk). Whereas in that case, only the iclk will be gated.

Did I miss something?

Benoit


>> and see if it also fixes the problem?
>
> It works as well it seems :)
>
> I assume you prefer this fix to be applied, so I'll send patches on this.
>
> - Kalle
>
>
>>
>> This patch should also prevent the hwmod code from putting the WDT2 into
>> smart-idle mode.
>>
>>
>> - Paul
>>
>>
>> ---
>>   arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
>> b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
>> index 457df3e..4366b85 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
>> @@ -1294,6 +1294,7 @@ static struct omap_hwmod
>> omap3xxx_wd_timer2_hwmod = {
>>   	.slaves		= omap3xxx_wd_timer2_slaves,
>>   	.slaves_cnt	= ARRAY_SIZE(omap3xxx_wd_timer2_slaves),
>>   	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>> +	.flags		= HWMOD_SWSUP_SIDLE,
>>   };
>>
>>   /* UART common */
>> --
>> 1.7.2.3
>

Re: [PATCH 0/2] OMAP3: wdtimer: fix wdtimer blocking CORE idle

[Paul Walmsley]

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

Hi Benoît,

On Thu, 10 Mar 2011, Cousson, Benoit wrote:

> On 3/10/2011 11:28 AM, kalle.jokiniemi@nokia.com wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: ext Paul Walmsley [mailto:paul@pwsan.com]
> > > Sent: 10. maaliskuuta 2011 11:50
> > > To: Jokiniemi Kalle (Nokia-MS/Tampere)
> > > Cc: khilman@ti.com; linux-omap@vger.kernel.org; Koskinen Ilkka (Nokia-
> > > MS/Tampere); jhnikula@gmail.com; b-cousson@ti.com
> > > Subject: Re: [PATCH 0/2] OMAP3: wdtimer: fix wdtimer blocking CORE idle
> > > 
> > > 
> > > Thanks Kalle for tracking this down,
> > > 
> > > could you try this patch along with the patch from this message:
> > > 
> > >    http://www.spinics.net/lists/linux-omap/msg48115.html
> 
> I have some question with that patch.
> By using pm_runtime API, you are suppose to idle the whole IP.
> In this case, just because you cannot idle a wdt that is running, it works,
> but the point is that the pm_runtime state will not reflect the HW state.
> For pm_runtime point of view, the wdt is supposed to be fully idle (both fclk
> and iclk). Whereas in that case, only the iclk will be gated.
> 
> Did I miss something?

Looking at it, I don't know how the old driver managed to work at all.  
Commit 7ec5ad0f3c1e28b693185c35f768953c5db32291 ("OMAP: WDT: Use PM 
runtime APIs instead of clk FW APIs") removed this from omap_wdt_probe():

-       /* autogate OCP interface clock */
-       __raw_writel(0x01, wdev->base + OMAP_WATCHDOG_SYS_CONFIG);

This would put the WDTIMER into force-idle, and nothing ever took it out 
of force-idle.  So I don't quite understand why the PRCM wouldn't just 
turn off the functional clock to the WDTIMER at this point.  Any thoughts 
on this?


- Paul

Re: [PATCH 0/2] OMAP3: wdtimer: fix wdtimer blocking CORE idle

[Cousson, Benoit]

On 3/10/2011 2:08 PM, Paul Walmsley wrote:
> Hi Benoît,
>
> On Thu, 10 Mar 2011, Cousson, Benoit wrote:
>
>> On 3/10/2011 11:28 AM, kalle.jokiniemi@nokia.com wrote:
>>>
>>>> From: ext Paul Walmsley [mailto:paul@pwsan.com]
>>>> Sent: 10. maaliskuuta 2011 11:50
>>>>
>>>> Thanks Kalle for tracking this down,
>>>>
>>>> could you try this patch along with the patch from this message:
>>>>
>>>>     http://www.spinics.net/lists/linux-omap/msg48115.html
>>
>> I have some question with that patch.
>> By using pm_runtime API, you are suppose to idle the whole IP.
>> In this case, just because you cannot idle a wdt that is running, it works,
>> but the point is that the pm_runtime state will not reflect the HW state.
>> For pm_runtime point of view, the wdt is supposed to be fully idle (both fclk
>> and iclk). Whereas in that case, only the iclk will be gated.
>>
>> Did I miss something?
>
> Looking at it, I don't know how the old driver managed to work at all.
> Commit 7ec5ad0f3c1e28b693185c35f768953c5db32291 ("OMAP: WDT: Use PM
> runtime APIs instead of clk FW APIs") removed this from omap_wdt_probe():
>
> -       /* autogate OCP interface clock */
> -       __raw_writel(0x01, wdev->base + OMAP_WATCHDOG_SYS_CONFIG);
>
> This would put the WDTIMER into force-idle, and nothing ever took it out
> of force-idle.  So I don't quite understand why the PRCM wouldn't just
> turn off the functional clock to the WDTIMER at this point.  Any thoughts
> on this?

The idle definition of the PRCM is just: idle of the ocp clock (iclk). 
The fclk can be active while the module is "idle" for the PRCM point of 
view.
That's why we have a mismatch in that case with the pm_runtime idle 
definition, where the module should be fully idle.

In that case, since the WDT is in the wakeup domain, we do not even have 
to re-enable the iclk since the wakeup domain is always on whenever the 
MPU is running.

That fact that we have to explicitly do a force idle, probably means 
that the autoidle is broken in the IP.

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 0/2] OMAP3: wdtimer: fix wdtimer blocking CORE idle

[Cousson, Benoit]

Paul,

On 3/10/2011 2:54 PM, Cousson, Benoit wrote:
> On 3/10/2011 2:08 PM, Paul Walmsley wrote:
>> Hi Benoît,
>>
>> On Thu, 10 Mar 2011, Cousson, Benoit wrote:
>>
>>> On 3/10/2011 11:28 AM, kalle.jokiniemi@nokia.com wrote:
>>>>
>>>>> From: ext Paul Walmsley [mailto:paul@pwsan.com]
>>>>> Sent: 10. maaliskuuta 2011 11:50
>>>>>
>>>>> Thanks Kalle for tracking this down,
>>>>>
>>>>> could you try this patch along with the patch from this message:
>>>>>
>>>>>      http://www.spinics.net/lists/linux-omap/msg48115.html
>>>
>>> I have some question with that patch.
>>> By using pm_runtime API, you are suppose to idle the whole IP.
>>> In this case, just because you cannot idle a wdt that is running, it works,
>>> but the point is that the pm_runtime state will not reflect the HW state.
>>> For pm_runtime point of view, the wdt is supposed to be fully idle (both fclk
>>> and iclk). Whereas in that case, only the iclk will be gated.
>>>
>>> Did I miss something?
>>
>> Looking at it, I don't know how the old driver managed to work at all.
>> Commit 7ec5ad0f3c1e28b693185c35f768953c5db32291 ("OMAP: WDT: Use PM
>> runtime APIs instead of clk FW APIs") removed this from omap_wdt_probe():
>>
>> -       /* autogate OCP interface clock */
>> -       __raw_writel(0x01, wdev->base + OMAP_WATCHDOG_SYS_CONFIG);
>>
>> This would put the WDTIMER into force-idle, and nothing ever took it out
>> of force-idle.  So I don't quite understand why the PRCM wouldn't just
>> turn off the functional clock to the WDTIMER at this point.  Any thoughts
>> on this?
>
> The idle definition of the PRCM is just: idle of the ocp clock (iclk).
> The fclk can be active while the module is "idle" for the PRCM point of
> view.
> That's why we have a mismatch in that case with the pm_runtime idle
> definition, where the module should be fully idle.
>
> In that case, since the WDT is in the wakeup domain, we do not even have
> to re-enable the iclk since the wakeup domain is always on whenever the
> MPU is running.
>
> That fact that we have to explicitly do a force idle, probably means
> that the autoidle is broken in the IP.

Meanwhile, this patch is probably the best approach anyway.
I do not like going backward with a clock level management solution.

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