ARM: OMAP2+: watchdog: fix !PM boot crash, disarm timer after hwmod reset

PATCH New Kevin Hilman 4 participants 6 messages Apr 13, 2012 – May 3, 2012

Subsystems (4)

Arm (32-bit) Arm sub-architectures OMAP2+ support OMAP hwmod data

lore

Share & tools

lore

Kevin Hilman

author

2012-04-13 21:50 UTC #0 lore raw

PATCH 1/1 ARM: OMAP2+: watchdog: fix !PM boot crash, disarm timer after hwmod reset

Without runtime PM enabled, hwmod needs to leave all IP blocks in an enabled state by default so any driver access to the HW will succeed. This is accomplished by seting the postsetup_state to enabled for all hwmods during init when runtime PM is disabled.

Currently, we have a special case for WDT in that its postsetup_state is always set to disabled. This is done so that the WDT is disabled and the timer is disarmed at boot in case there is no WDT driver. This also means that when runtime PM is disabled, if a WDT driver *is* built in the kernel, the kernel will crash on the first access to the WDT hardware.

We can't simply leave the WDT module enabled, because the timer is armed by default after reset. That means that if there is no WDT driver initialzed or loaded before the timer expires, the kernel will reboot.

To fix this, a custom reset method is added to the watchdog class of omap_hwmod. This method will *always* disarm the timer after hwmod reset. The WDT timer then will only be rearmed when/if the driver is loaded for the WDT. With the timer disarmed by default, we no longer need a special-case for the postsetup_state of WDT during init, so it is removed.

Any platforms wishing to ensure the watchdog remains armed across the entire boot boot can simply disable the reset-on-init feature of the watchdog hwmod using omap_hwmod_no_setup_reset().

Tested on 3530/Overo, 4430/Panda.

NOTE: on 4430, the hwmod OCP reset does not seem to rearm the timer as documented in the TRM (and what happens on OMAP3.) I noticed this because testing the HWMOD_INIT_NO_RESET feature with no driver loaded, I expected a reboot part way through the boot, but did not see a reboot. Adding some debug to read the counter, I verified that right after OCP softreset, the counter is not firing. After writing the magic start sequence, the timer starts counting. This means that the timer disarm sequence added here does not seem to be needed for 4430, but is technically the correct way to ensure the timer is disarmed, so it is left in for OMAP4.

Special thanks to Paul Walmsley for helping brainstorm ideas to fix this problem.

Cc Paul Walmsley <[email protected]>

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

---
 arch/arm/mach-omap2/io.c                           |   18 ------------------
 arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c |    3 ++-
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c         |    3 ++-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c         |    1 +
 arch/arm/mach-omap2/wd_timer.c                     |    7 +++++++
 arch/arm/mach-omap2/wd_timer.h                     |    1 +
 6 files changed, 13 insertions(+), 20 deletions(-)

arch/arm/mach-omap2/io.c +0 ~~-18~~

index 065bd76..fafcc35 100644
@@ -363,24 +363,6 @@ static void __init omap_hwmod_init_postsetup(void)
 #endif
 	omap_hwmod_for_each(_set_hwmod_postsetup_state, &postsetup_state);
 
-	/*
-	 * Set the default postsetup state for unusual modules (like
-	 * MPU WDT).
-	 *
-	 * The postsetup_state is not actually used until
-	 * omap_hwmod_late_init(), so boards that desire full watchdog
-	 * coverage of kernel initialization can reprogram the
-	 * postsetup_state between the calls to
-	 * omap2_init_common_infra() and omap_sdrc_init().
-	 *
-	 * XXX ideally we could detect whether the MPU WDT was currently
-	 * enabled here and make this conditional
-	 */
-	postsetup_state = _HWMOD_STATE_DISABLED;
-	omap_hwmod_for_each_by_class("wd_timer",
-				     _set_hwmod_postsetup_state,
-				     &postsetup_state);
-
 	omap_pm_if_early_init();
 }

arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c +2 -1

index 2a67297..6cc70ed 100644
@@ -86,7 +86,8 @@ static struct omap_hwmod_class_sysconfig omap2xxx_wd_timer_sysc = {
 struct omap_hwmod_class omap2xxx_wd_timer_hwmod_class = {
 	.name		= "wd_timer",
 	.sysc		= &omap2xxx_wd_timer_sysc,
-	.pre_shutdown	= &omap2_wd_timer_disable
+	.pre_shutdown	= &omap2_wd_timer_disable,
+	.reset		= &omap2_wd_timer_reset,
 };
 
 /*

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

index 34b9766d..0aec9a1 100644
@@ -1200,7 +1200,8 @@ static struct omap_hwmod_class_sysconfig i2c_sysc = {
 static struct omap_hwmod_class omap3xxx_wd_timer_hwmod_class = {
 	.name		= "wd_timer",
 	.sysc		= &omap3xxx_wd_timer_sysc,
-	.pre_shutdown	= &omap2_wd_timer_disable
+	.pre_shutdown	= &omap2_wd_timer_disable,
+	.reset		= &omap2_wd_timer_reset,
 };
 
 /* wd_timer2 */

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

index cc9bd10..213de81 100644
@@ -5206,6 +5206,7 @@ static struct omap_hwmod_class omap44xx_wd_timer_hwmod_class = {
 	.name		= "wd_timer",
 	.sysc		= &omap44xx_wd_timer_sysc,
 	.pre_shutdown	= &omap2_wd_timer_disable,
+	.reset		= &omap2_wd_timer_reset,
 };
 
 /* wd_timer2 */

arch/arm/mach-omap2/wd_timer.c +7 -0

index 4067669..493541f 100644
@@ -54,3 +54,10 @@ int omap2_wd_timer_disable(struct omap_hwmod *oh)
 	return 0;
 }
 
+int omap2_wd_timer_reset(struct omap_hwmod *oh)
+{
+	omap_hwmod_softreset(oh);
+	omap2_wd_timer_disable(oh);
+
+	return 0;
+}

arch/arm/mach-omap2/wd_timer.h +1 -0

index e0054a2..f6bbba7 100644
@@ -13,5 +13,6 @@
 #include <plat/omap_hwmod.h>
 
 extern int omap2_wd_timer_disable(struct omap_hwmod *oh);
+extern int omap2_wd_timer_reset(struct omap_hwmod *oh);
 
 #endif

Signature · 1 line

1.7.9.2

Paul Walmsley

maintainer

2012-04-20 20:59 UTC #1 lore raw

PATCH 1/1 ARM: OMAP2+: watchdog: fix !PM boot crash, disarm timer after hwmod reset

cc Santosh

Hi Kevin,

Nice changelog!

Quoted 41 lines · Kevin Hilman

Without runtime PM enabled, hwmod needs to leave all IP blocks in an enabled state by default so any driver access to the HW will succeed. This is accomplished by seting the postsetup_state to enabled for all hwmods during init when runtime PM is disabled.
Currently, we have a special case for WDT in that its postsetup_state is always set to disabled. This is done so that the WDT is disabled and the timer is disarmed at boot in case there is no WDT driver. This also means that when runtime PM is disabled, if a WDT driver *is* built in the kernel, the kernel will crash on the first access to the WDT hardware.
We can't simply leave the WDT module enabled, because the timer is armed by default after reset. That means that if there is no WDT driver initialzed or loaded before the timer expires, the kernel will reboot.
To fix this, a custom reset method is added to the watchdog class of omap_hwmod. This method will *always* disarm the timer after hwmod reset. The WDT timer then will only be rearmed when/if the driver is loaded for the WDT. With the timer disarmed by default, we no longer need a special-case for the postsetup_state of WDT during init, so it is removed.
Any platforms wishing to ensure the watchdog remains armed across the entire boot boot can simply disable the reset-on-init feature of the watchdog hwmod using omap_hwmod_no_setup_reset().
Tested on 3530/Overo, 4430/Panda.
NOTE: on 4430, the hwmod OCP reset does not seem to rearm the timer as documented in the TRM (and what happens on OMAP3.) I noticed this because testing the HWMOD_INIT_NO_RESET feature with no driver loaded, I expected a reboot part way through the boot, but did not see a reboot. Adding some debug to read the counter, I verified that right after OCP softreset, the counter is not firing. After writing the magic start sequence, the timer starts counting. This means that the timer disarm sequence added here does not seem to be needed for 4430, but is technically the correct way to ensure the timer is disarmed, so it is left in for OMAP4.

Quoted 5 lines

Special thanks to Paul Walmsley for helping brainstorm ideas to fix this problem.
Cc Paul Walmsley <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>

This looks great, looks like it will finally fix this longstanding bug. I think Santosh hit it too a long time ago, so I suspect he will be happy too.

One comment: I think that omap2_wd_timer_reset() needs to be updated in light of commit 3c55c1baffa5f719eb2ae9729088bc867f972f53 ("ARM: OMAP2+:
hwmod: Revert "ARM: OMAP2+: hwmod: Make omap_hwmod_softreset wait for reset status""). I did this here. It's passed basic build testing but haven't tried booting it yet. Care to take a look and see if you have any comments? It's also available in the 'hwmod_devel_a_3.5' branch of
git://git.pwsan.com/linux-2.6

Also, am thinking about queuing this for 3.5 rather than 3.4-rc fixes due to the massive hwmod data changes queued for 3.5, and since we've suffered with this bug for at least a year, but wanted to run it by you first. That should save Tony some rebase hassle. Any opinions on this?

- Paul

From: Kevin Hilman <[email protected]>
Date: Fri, 20 Apr 2012 14:48:14 -0600
Subject: [PATCH] ARM: OMAP2+: WDTIMER integration: fix !PM boot crash, disarm
timer after hwmod reset

Any platforms wishing to ensure the watchdog remains armed across the entire boot boot can simply disable the reset-on-init feature of the watchdog hwmod using omap_hwmod_no_setup_reset().

Tested on 3530/Overo, 4430/Panda.

Special thanks to Paul Walmsley for helping brainstorm ideas to fix this problem.

Cc Paul Walmsley <[email protected]>

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

Cc Santosh Shilimkar <[email protected]>

[paul at pwsan.com: updated the omap2_wd_timer_reset() function in the
wake of commit 3c55c1baffa5f719eb2ae9729088bc867f972f53 ("ARM:
OMAP2+: hwmod: Revert "ARM: OMAP2+: hwmod: Make omap_hwmod_softreset wait for reset status""); added kerneldoc]

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

---
 arch/arm/mach-omap2/io.c                           |   18 --------
 arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c |    3 +-
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c         |    3 +-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c         |    1 +
 arch/arm/mach-omap2/wd_timer.c                     |   46 ++++++++++++++++++++
 arch/arm/mach-omap2/wd_timer.h                     |    1 +
 6 files changed, 52 insertions(+), 20 deletions(-)

arch/arm/mach-omap2/io.c +0 ~~-18~~

index 065bd76..fafcc35 100644
@@ -363,24 +363,6 @@ static void __init omap_hwmod_init_postsetup(void)
 #endif
 	omap_hwmod_for_each(_set_hwmod_postsetup_state, &postsetup_state);
 
-	/*
-	 * Set the default postsetup state for unusual modules (like
-	 * MPU WDT).
-	 *
-	 * The postsetup_state is not actually used until
-	 * omap_hwmod_late_init(), so boards that desire full watchdog
-	 * coverage of kernel initialization can reprogram the
-	 * postsetup_state between the calls to
-	 * omap2_init_common_infra() and omap_sdrc_init().
-	 *
-	 * XXX ideally we could detect whether the MPU WDT was currently
-	 * enabled here and make this conditional
-	 */
-	postsetup_state = _HWMOD_STATE_DISABLED;
-	omap_hwmod_for_each_by_class("wd_timer",
-				     _set_hwmod_postsetup_state,
-				     &postsetup_state);
-
 	omap_pm_if_early_init();
 }

arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c +2 -1

index 2a67297..6cc70ed 100644
@@ -86,7 +86,8 @@ static struct omap_hwmod_class_sysconfig omap2xxx_wd_timer_sysc = {
 struct omap_hwmod_class omap2xxx_wd_timer_hwmod_class = {
 	.name		= "wd_timer",
 	.sysc		= &omap2xxx_wd_timer_sysc,
-	.pre_shutdown	= &omap2_wd_timer_disable
+	.pre_shutdown	= &omap2_wd_timer_disable,
+	.reset		= &omap2_wd_timer_reset,
 };
 
 /*

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

index db86ce9..77ccbfa 100644
@@ -1200,7 +1200,8 @@ static struct omap_hwmod_class_sysconfig i2c_sysc = {
 static struct omap_hwmod_class omap3xxx_wd_timer_hwmod_class = {
 	.name		= "wd_timer",
 	.sysc		= &omap3xxx_wd_timer_sysc,
-	.pre_shutdown	= &omap2_wd_timer_disable
+	.pre_shutdown	= &omap2_wd_timer_disable,
+	.reset		= &omap2_wd_timer_reset,
 };
 
 /* wd_timer2 */

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

index 6abc757..22fb63e 100644
@@ -5215,6 +5215,7 @@ static struct omap_hwmod_class omap44xx_wd_timer_hwmod_class = {
 	.name		= "wd_timer",
 	.sysc		= &omap44xx_wd_timer_sysc,
 	.pre_shutdown	= &omap2_wd_timer_disable,
+	.reset		= &omap2_wd_timer_reset,
 };
 
 /* wd_timer2 */

arch/arm/mach-omap2/wd_timer.c +46 -0

61 lines not on this page · raw message

arch/arm/mach-omap2/wd_timer.h +1 -0

index e0054a2..f6bbba7 100644
@@ -13,5 +13,6 @@
 #include <plat/omap_hwmod.h>
 
 extern int omap2_wd_timer_disable(struct omap_hwmod *oh);
+extern int omap2_wd_timer_reset(struct omap_hwmod *oh);
 
 #endif

Signature · 1 line

1.7.10

Kevin Hilman

author

2012-04-23 17:42 UTC #2 lore raw

PATCH 1/1 ARM: OMAP2+: watchdog: fix !PM boot crash, disarm timer after hwmod reset

Quoted 59 lines · Paul Walmsley

cc Santosh
Hi Kevin,
Nice changelog!
Quoted 41 lines · Kevin Hilman
Without runtime PM enabled, hwmod needs to leave all IP blocks in an enabled state by default so any driver access to the HW will succeed. This is accomplished by seting the postsetup_state to enabled for all hwmods during init when runtime PM is disabled.
Currently, we have a special case for WDT in that its postsetup_state is always set to disabled. This is done so that the WDT is disabled and the timer is disarmed at boot in case there is no WDT driver. This also means that when runtime PM is disabled, if a WDT driver *is* built in the kernel, the kernel will crash on the first access to the WDT hardware.
We can't simply leave the WDT module enabled, because the timer is armed by default after reset. That means that if there is no WDT driver initialzed or loaded before the timer expires, the kernel will reboot.
To fix this, a custom reset method is added to the watchdog class of omap_hwmod. This method will *always* disarm the timer after hwmod reset. The WDT timer then will only be rearmed when/if the driver is loaded for the WDT. With the timer disarmed by default, we no longer need a special-case for the postsetup_state of WDT during init, so it is removed.
Any platforms wishing to ensure the watchdog remains armed across the entire boot boot can simply disable the reset-on-init feature of the watchdog hwmod using omap_hwmod_no_setup_reset().
Tested on 3530/Overo, 4430/Panda.
NOTE: on 4430, the hwmod OCP reset does not seem to rearm the timer as documented in the TRM (and what happens on OMAP3.) I noticed this because testing the HWMOD_INIT_NO_RESET feature with no driver loaded, I expected a reboot part way through the boot, but did not see a reboot. Adding some debug to read the counter, I verified that right after OCP softreset, the counter is not firing. After writing the magic start sequence, the timer starts counting. This means that the timer disarm sequence added here does not seem to be needed for 4430, but is technically the correct way to ensure the timer is disarmed, so it is left in for OMAP4.
Quoted 5 lines
Special thanks to Paul Walmsley for helping brainstorm ideas to fix this problem.
Cc Paul Walmsley <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>
This looks great, looks like it will finally fix this longstanding bug. I think Santosh hit it too a long time ago, so I suspect he will be happy too.

Quoted 7 lines

One comment: I think that omap2_wd_timer_reset() needs to be updated in light of commit 3c55c1baffa5f719eb2ae9729088bc867f972f53 ("ARM: OMAP2+:
hwmod: Revert "ARM: OMAP2+: hwmod: Make omap_hwmod_softreset wait for reset status""). I did this here. It's passed basic build testing but haven't tried booting it yet. Care to take a look and see if you have any comments? It's also available in the 'hwmod_devel_a_3.5' branch of
git://git.pwsan.com/linux-2.6

I did some testing on OMAP3530/Overo and it still works fine with your change.

Quoted 4 lines

Also, am thinking about queuing this for 3.5 rather than 3.4-rc fixes due to the massive hwmod data changes queued for 3.5, and since we've suffered with this bug for at least a year, but wanted to run it by you first. That should save Tony some rebase hassle. Any opinions on this?

I'm fine with waiting for 3.5 since it's a long-standing bug and not really a regresstion.

Thanks for updating it,

Kevin

Shilimkar, Santosh

2012-04-24 06:09 UTC #3 lore raw

PATCH 1/1 ARM: OMAP2+: watchdog: fix !PM boot crash, disarm timer after hwmod reset

In reply to Paul Walmsley

Quoted 52 lines · Paul Walmsley

cc Santosh
Hi Kevin,
Nice changelog!
Quoted 44 lines · Kevin Hilman
Without runtime PM enabled, hwmod needs to leave all IP blocks in an enabled state by default so any driver access to the HW will succeed. This is accomplished by seting the postsetup_state to enabled for all hwmods during init when runtime PM is disabled.
Currently, we have a special case for WDT in that its postsetup_state is always set to disabled. ?This is done so that the WDT is disabled and the timer is disarmed at boot in case there is no WDT driver. This also means that when runtime PM is disabled, if a WDT driver *is* built in the kernel, the kernel will crash on the first access to the WDT hardware.
We can't simply leave the WDT module enabled, because the timer is armed by default after reset. That means that if there is no WDT driver initialzed or loaded before the timer expires, the kernel will reboot.
To fix this, a custom reset method is added to the watchdog class of omap_hwmod. ?This method will *always* disarm the timer after hwmod reset. ?The WDT timer then will only be rearmed when/if the driver is loaded for the WDT. ?With the timer disarmed by default, we no longer need a special-case for the postsetup_state of WDT during init, so it is removed.
Any platforms wishing to ensure the watchdog remains armed across the entire boot boot can simply disable the reset-on-init feature of the watchdog hwmod using omap_hwmod_no_setup_reset().
Tested on 3530/Overo, 4430/Panda.
NOTE: on 4430, the hwmod OCP reset does not seem to rearm the timer as documented in the TRM (and what happens on OMAP3.) ?I noticed this because testing the HWMOD_INIT_NO_RESET feature with no driver loaded, I expected a reboot part way through the boot, but did not see a reboot. ?Adding some debug to read the counter, I verified that right after OCP softreset, the counter is not firing. ?After writing the magic start sequence, the timer starts counting. ?This means that the timer disarm sequence added here does not seem to be needed for 4430, but is technically the correct way to ensure the timer is disarmed, so it is left in for OMAP4.
Special thanks to Paul Walmsley for helping brainstorm ideas to fix this problem.

Quoted 7 lines

Quoted 2 lines
Cc Paul Walmsley <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>
This looks great, looks like it will finally fix this longstanding bug. ?I think Santosh hit it too a long time ago, so I suspect he will be happy too.

Indeed !!
Thanks a lot Kevin/Paul for getting this sorted out.

FWIW,
Acked-by: Santosh Shilimkar ,santosh.shilimkar at ti.com>

Kevin Hilman

2012-05-03 18:19 UTC #4 lore raw

PATCH 1/1 ARM: OMAP2+: watchdog: fix !PM boot crash, disarm timer after hwmod reset

In reply to Paul Walmsley

Hi Paul,

On 04/20/2012 01:59 PM, Paul Walmsley wrote:

[...]

Quoted 11 lines

This looks great, looks like it will finally fix this longstanding bug. I think Santosh hit it too a long time ago, so I suspect he will be happy too.
One comment: I think that omap2_wd_timer_reset() needs to be updated in light of commit 3c55c1baffa5f719eb2ae9729088bc867f972f53 ("ARM: OMAP2+:
hwmod: Revert "ARM: OMAP2+: hwmod: Make omap_hwmod_softreset wait for reset status""). I did this here. It's passed basic build testing but haven't tried booting it yet. Care to take a look and see if you have any comments? It's also available in the 'hwmod_devel_a_3.5' branch of
git://git.pwsan.com/linux-2.6

I just noticed a compile warning with your updated version:

/work/kernel/omap/pm/arch/arm/mach-omap2/wd_timer.c: In function 'omap2_wd_timer_reset':
/work/kernel/omap/pm/arch/arm/mach-omap2/wd_timer.c:78:6: warning: unused variable 'v' [-Wunused-variable]

The diff below on top of your patch fixes it.

Kevin

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

index fcbb663..b2f1c67 100644
@@ -75,7 +75,6 @@ int omap2_wd_timer_disable(struct omap_hwmod *oh)
  */
 int omap2_wd_timer_reset(struct omap_hwmod *oh)
 {
-	u32 v;
 	int c = 0;
 
 	/* Write to the SOFTRESET bit */

Paul Walmsley

maintainer

2012-05-03 22:52 UTC #5 lore raw

PATCH 1/1 ARM: OMAP2+: watchdog: fix !PM boot crash, disarm timer after hwmod reset

Hi Kevin,

Quoted 16 lines · Kevin Hilman

On 04/20/2012 01:59 PM, Paul Walmsley wrote:
[...]
Quoted 11 lines
This looks great, looks like it will finally fix this longstanding bug. I think Santosh hit it too a long time ago, so I suspect he will be happy too.
One comment: I think that omap2_wd_timer_reset() needs to be updated in light of commit 3c55c1baffa5f719eb2ae9729088bc867f972f53 ("ARM: OMAP2+:
hwmod: Revert "ARM: OMAP2+: hwmod: Make omap_hwmod_softreset wait for reset status""). I did this here. It's passed basic build testing but haven't tried booting it yet. Care to take a look and see if you have any comments? It's also available in the 'hwmod_devel_a_3.5' branch of
git://git.pwsan.com/linux-2.6

Quoted 8 lines

I just noticed a compile warning with your updated version:
/work/kernel/omap/pm/arch/arm/mach-omap2/wd_timer.c: In function 'omap2_wd_timer_reset':
/work/kernel/omap/pm/arch/arm/mach-omap2/wd_timer.c:78:6: warning: unused variable 'v' [-Wunused-variable]
The diff below on top of your patch fixes it.

Thanks, just rolled this into the patch and added some credit.

- Paul