omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n

PATCH Cousson, Benoit 4 participants 8 messages Oct 12, 2010 – Oct 20, 2010 3 messages not in the archive

Subsystems (4)

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

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Cousson, Benoit

author

2010-10-12 13:09 UTC #3 lore raw

Adding more folks to the discussion.

Quoted 65 lines · Charulatha

Quoted 43 lines
From: Cousson, Benoit
Sent: Tuesday, October 12, 2010 12:45 PM
To: Varadarajan, Charulatha
Hi Charu,
Quoted 3 lines · Charulatha
Fix the below warning during boot
[    0.000000] WARNING: at arch/arm/mach-omap2/omap_hwmod.c:1185
_omap_hwmod_enable+0x34/0x150()
Quoted 1 line
[    0.000000] omap_hwmod: wd_timer2: enabled state can only be entered
from initialized, idle, or disabled state
Quoted 2 lines
[    0.000000] Modules linked in:
[    0.000000] [<c0050460>] (unwind_backtrace+0x0/0xe4) from
[<c0083954>] (warn_slowpath_common+0x4c/0x64)
Quoted 1 line
[    0.000000] [<c0083954>] (warn_slowpath_common+0x4c/0x64) from
[<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c)
Quoted 1 line
[    0.000000] [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c) from
[<c0059be4>] (_omap_hwmod_enable+0x34/0x150)
Quoted 1 line
[    0.000000] [<c0059be4>] (_omap_hwmod_enable+0x34/0x150) from
[<c0059d28>] (omap_hwmod_enable+0x28/0x3c)
Quoted 1 line
[    0.000000] [<c0059d28>] (omap_hwmod_enable+0x28/0x3c) from
[<c005580c>] (omap2_disable_wdt+0x48/0xdc)
Quoted 1 line
[    0.000000] [<c005580c>] (omap2_disable_wdt+0x48/0xdc) from
[<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
Quoted 1 line
[    0.000000] [<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
from [<c000f6d0>] (omap2_init_devices+0x44/0x330)
Quoted 1 line
[    0.000000] [<c000f6d0>] (omap2_init_devices+0x44/0x330) from
[<c0049578>] (do_one_initcall+0xcc/0x1a4)
Quoted 1 line
[    0.000000] [<c0049578>] (do_one_initcall+0xcc/0x1a4) from
[<c0008780>] (kernel_init+0x148/0x210)
Quoted 1 line
[    0.000000] [<c0008780>] (kernel_init+0x148/0x210) from [<c004acb8>]
(kernel_thread_exit+0x0/0x8)
Quoted 7 lines
[    0.000000] ---[ end trace 1b75b31a2719ed1f ]---
[    0.000000] wd_timer2: Could not enable clocks for omap2_disable_wdt
When CONFIG_PM_RUNTIME is not defined, watchdog timer clocks are not disabled after a watchdog reset. Hence in omap2_disable_wdt(), it is not required to enable clocks before accessing the watchdog timer registers if CONFIG_PM_RUNTIME is not defined.
Quoted 5 lines
I'm not a big fan of adding some dependencies with CONFIG_PM_RUNTIME inside this code.
The real root cause is not CONFIG_PM_RUNTIME, but mostly the skip_setup_idle variable added in arch/arm/mach-omap2/io.c by Paul.
Yes.
Quoted 2 lines
So I'd rather use that parameter as an input for the omap2_disable_wdt.
I also thought about this, but we need to
1. call omap2_disable_wdt() in omap2_init_common_hw() rather than omap2_init_devices(). That would involve movement of omap2_disable_wdt() from devices.c to io.c.
(or)
2. make skip_setup_idle global
(or)
3. make omap2_disable_wdt() global and call it from omap2_init_common_hw() and pass skip_setup_idle as parameter.

Quoted 3 lines

I felt that the usage of CONFIG_PM_RUNTIME is better than the above. But still we may consider any of the above options or any other option. Please suggest.

It is maybe easier, but I don't think it is better...

As I said, CONFIG_PM_RUNTIME is not the root cause of your issue, just an indirect cause. OK, maybe strictly speaking, it is the root cause, since it started for that... but that not the cause that we should consider in your case.

If we decide to remove that CONFIG_PM_RUNTIME dependency in the io / hwmod code, nobody will be able to find the relation with your code in WDT.

That's why you should not do that, for my point of view.

Paul, Kevin,
Any thoughts on that point?

Thanks,
Benoit

Quoted 62 lines

Quoted 61 lines
Regards,
Benoit
arch/arm/mach-
Quoted 9 lines
Tested on OMAP3430 SDP and OMAP4430 SDP.
Signed-off-by Charulatha V<[email protected]>
---
   arch/arm/mach-omap2/devices.c |    6 +++++-
   1 files changed, 5 insertions(+), 1 deletions(-)
arch/arm/mach- renamed from arch/arm/mach-omap2/devices.c +0 -0
omap2/devices.c
arch/arm/mach-omap2/devices.c static int omap2_disable_wdt(struct omap_hwmod *oh,
Quoted 13 lines
index eaf3799..b592952 100644
arch/arm/mach-omap2/devices.c +1 -1
@@ -926,7 +926,7 @@ static inline void omap_init_vout(void) {}
   static int omap2_disable_wdt(struct omap_hwmod *oh, void *unused)
   {
   	void __iomem *base;
-	int ret;
+	int ret = 0;
 
   	if (!oh) {
   		pr_err("%s: Could not look up wdtimer_hwmod\n", __func__);
@@ -940,6 +940,7 @@ static int omap2_disable_wdt(struct omap_hwmod *oh,
void *unused)
arch/arm/mach-omap2/devices.c static int omap2_disable_wdt(struct omap_hwmod *oh,
Quoted 8 lines
arch/arm/mach-omap2/devices.c +1 -0
   		return -EINVAL;
   	}
 
+#ifdef CONFIG_PM_RUNTIME
   	/* Enable the clocks before accessing the WDT registers */
   	ret = omap_hwmod_enable(oh);
   	if (ret) {
@@ -947,6 +948,7 @@ static int omap2_disable_wdt(struct omap_hwmod *oh,
void *unused)
arch/arm/mach-omap2/devices.c static int omap2_disable_wdt(struct omap_hwmod *oh,
Quoted 8 lines
arch/arm/mach-omap2/devices.c +1 -0
   				oh->name, __func__);
   		return ret;
   	}
+#endif
 
   	/* sequence required to disable watchdog */
   	__raw_writel(0xAAAA, base + OMAP_WDT_SPR);
@@ -957,10 +959,12 @@ static int omap2_disable_wdt(struct omap_hwmod *oh,
void *unused)
Quoted 12 lines
arch/arm/mach-omap2/devices.c +2 -0
   	while (__raw_readl(base + OMAP_WDT_WPS)&   0x10)
   		cpu_relax();
 
+#ifdef CONFIG_PM_RUNTIME
   	ret = omap_hwmod_idle(oh);
   	if (ret)
   		pr_err("%s: Could not disable clocks for %s\n",
   				oh->name, __func__);
+#endif
 
   	return ret;
   }

Kevin Hilman

2010-10-12 18:27 UTC #4 lore raw

Quoted 79 lines · Benoit

Adding more folks to the discussion.
Quoted 65 lines · Charulatha
Quoted 43 lines
From: Cousson, Benoit
Sent: Tuesday, October 12, 2010 12:45 PM
To: Varadarajan, Charulatha
Hi Charu,
Quoted 3 lines · Charulatha
Fix the below warning during boot
[    0.000000] WARNING: at arch/arm/mach-omap2/omap_hwmod.c:1185
_omap_hwmod_enable+0x34/0x150()
Quoted 1 line
[    0.000000] omap_hwmod: wd_timer2: enabled state can only be entered
from initialized, idle, or disabled state
Quoted 2 lines
[    0.000000] Modules linked in:
[    0.000000] [<c0050460>] (unwind_backtrace+0x0/0xe4) from
[<c0083954>] (warn_slowpath_common+0x4c/0x64)
Quoted 1 line
[    0.000000] [<c0083954>] (warn_slowpath_common+0x4c/0x64) from
[<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c)
Quoted 1 line
[    0.000000] [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c) from
[<c0059be4>] (_omap_hwmod_enable+0x34/0x150)
Quoted 1 line
[    0.000000] [<c0059be4>] (_omap_hwmod_enable+0x34/0x150) from
[<c0059d28>] (omap_hwmod_enable+0x28/0x3c)
Quoted 1 line
[    0.000000] [<c0059d28>] (omap_hwmod_enable+0x28/0x3c) from
[<c005580c>] (omap2_disable_wdt+0x48/0xdc)
Quoted 1 line
[    0.000000] [<c005580c>] (omap2_disable_wdt+0x48/0xdc) from
[<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
Quoted 1 line
[    0.000000] [<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
from [<c000f6d0>] (omap2_init_devices+0x44/0x330)
Quoted 1 line
[    0.000000] [<c000f6d0>] (omap2_init_devices+0x44/0x330) from
[<c0049578>] (do_one_initcall+0xcc/0x1a4)
Quoted 1 line
[    0.000000] [<c0049578>] (do_one_initcall+0xcc/0x1a4) from
[<c0008780>] (kernel_init+0x148/0x210)
Quoted 1 line
[    0.000000] [<c0008780>] (kernel_init+0x148/0x210) from [<c004acb8>]
(kernel_thread_exit+0x0/0x8)
Quoted 7 lines
[    0.000000] ---[ end trace 1b75b31a2719ed1f ]---
[    0.000000] wd_timer2: Could not enable clocks for omap2_disable_wdt
When CONFIG_PM_RUNTIME is not defined, watchdog timer clocks are not disabled after a watchdog reset. Hence in omap2_disable_wdt(), it is not required to enable clocks before accessing the watchdog timer registers if CONFIG_PM_RUNTIME is not defined.
Quoted 5 lines
I'm not a big fan of adding some dependencies with CONFIG_PM_RUNTIME inside this code.
The real root cause is not CONFIG_PM_RUNTIME, but mostly the skip_setup_idle variable added in arch/arm/mach-omap2/io.c by Paul.
Yes.
Quoted 2 lines
So I'd rather use that parameter as an input for the omap2_disable_wdt.
I also thought about this, but we need to
1. call omap2_disable_wdt() in omap2_init_common_hw() rather than omap2_init_devices(). That would involve movement of omap2_disable_wdt() from devices.c to io.c.
(or)
2. make skip_setup_idle global
(or)
3. make omap2_disable_wdt() global and call it from omap2_init_common_hw() and pass skip_setup_idle as parameter.
Quoted 3 lines
I felt that the usage of CONFIG_PM_RUNTIME is better than the above. But still we may consider any of the above options or any other option. Please suggest.
It is maybe easier, but I don't think it is better...
As I said, CONFIG_PM_RUNTIME is not the root cause of your issue, just an indirect cause. OK, maybe strictly speaking, it is the root cause, since it started for that... but that not the cause that we should consider in your case.

Quoted 8 lines

If we decide to remove that CONFIG_PM_RUNTIME dependency in the io / hwmod code, nobody will be able to find the relation with your code in WDT.
That's why you should not do that, for my point of view.
Paul, Kevin,
Any thoughts on that point?

I agree that this is not strictly a dependency on PM_RUNTIME.

What's really needed is a way for low-level hwmod users like this to be able to check whether the hwmod is already enabled. Something like this (completely untested):

bool omap_hwmod_is_enabled(struct omap_hwmod *oh)

{
	return (oh->_state == _HWMOD_STATE_ENABLED);
}

Alternatively, maybe calling omap_hwmod_enable() when the state is already enabled should not be so verbose, and it should just return success, but that option needs a little more thought...

Kevin

Quoted 62 lines

Quoted 62 lines
Quoted 61 lines
Regards,
Benoit
arch/arm/mach-
Quoted 9 lines
Tested on OMAP3430 SDP and OMAP4430 SDP.
Signed-off-by Charulatha V<[email protected]>
---
   arch/arm/mach-omap2/devices.c |    6 +++++-
   1 files changed, 5 insertions(+), 1 deletions(-)
arch/arm/mach- renamed from arch/arm/mach-omap2/devices.c +0 -0
omap2/devices.c
arch/arm/mach-omap2/devices.c static int omap2_disable_wdt(struct omap_hwmod *oh,
Quoted 13 lines
index eaf3799..b592952 100644
arch/arm/mach-omap2/devices.c +1 -1
@@ -926,7 +926,7 @@ static inline void omap_init_vout(void) {}
   static int omap2_disable_wdt(struct omap_hwmod *oh, void *unused)
   {
   	void __iomem *base;
-	int ret;
+	int ret = 0;
 
   	if (!oh) {
   		pr_err("%s: Could not look up wdtimer_hwmod\n", __func__);
@@ -940,6 +940,7 @@ static int omap2_disable_wdt(struct omap_hwmod *oh,
void *unused)
arch/arm/mach-omap2/devices.c static int omap2_disable_wdt(struct omap_hwmod *oh,
Quoted 8 lines
arch/arm/mach-omap2/devices.c +1 -0
   		return -EINVAL;
   	}
 
+#ifdef CONFIG_PM_RUNTIME
   	/* Enable the clocks before accessing the WDT registers */
   	ret = omap_hwmod_enable(oh);
   	if (ret) {
@@ -947,6 +948,7 @@ static int omap2_disable_wdt(struct omap_hwmod *oh,
void *unused)
arch/arm/mach-omap2/devices.c static int omap2_disable_wdt(struct omap_hwmod *oh,
Quoted 8 lines
arch/arm/mach-omap2/devices.c +1 -0
   				oh->name, __func__);
   		return ret;
   	}
+#endif
 
   	/* sequence required to disable watchdog */
   	__raw_writel(0xAAAA, base + OMAP_WDT_SPR);
@@ -957,10 +959,12 @@ static int omap2_disable_wdt(struct omap_hwmod *oh,
void *unused)
Quoted 12 lines
arch/arm/mach-omap2/devices.c +2 -0
   	while (__raw_readl(base + OMAP_WDT_WPS)&   0x10)
   		cpu_relax();
 
+#ifdef CONFIG_PM_RUNTIME
   	ret = omap_hwmod_idle(oh);
   	if (ret)
   		pr_err("%s: Could not disable clocks for %s\n",
   				oh->name, __func__);
+#endif
 
   	return ret;
   }

Varadarajan, Charulatha

2010-10-14 14:13 UTC #5 lore raw

Paul, Benoit,

Please provide your input on this.

Quoted 119 lines

-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Tuesday, October 12, 2010 11:57 PM
To: Cousson, Benoit
Cc: Varadarajan, Charulatha; Paul Walmsley; [email protected]
Subject: Re: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n
Quoted 16 lines · Benoit
Adding more folks to the discussion.
Quoted 13 lines · Charulatha
Quoted 13 lines
From: Cousson, Benoit
Sent: Tuesday, October 12, 2010 12:45 PM
To: Varadarajan, Charulatha
Hi Charu,
Quoted 3 lines · Charulatha
Fix the below warning during boot
[    0.000000] WARNING: at arch/arm/mach-omap2/omap_hwmod.c:1185
_omap_hwmod_enable+0x34/0x150()
Quoted 1 line
[    0.000000] omap_hwmod: wd_timer2: enabled state can 
only be entered
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
from initialized, idle, or disabled state
Quoted 2 lines
[    0.000000] Modules linked in:
[    0.000000] [<c0050460>] (unwind_backtrace+0x0/0xe4) from
[<c0083954>] (warn_slowpath_common+0x4c/0x64)
Quoted 1 line
[    0.000000] [<c0083954>] (warn_slowpath_common+0x4c/0x64) from
[<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c)
Quoted 1 line
[    0.000000] [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c) from
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
[<c0059be4>] (_omap_hwmod_enable+0x34/0x150)
Quoted 1 line
[    0.000000] [<c0059be4>] (_omap_hwmod_enable+0x34/0x150) from
[<c0059d28>] (omap_hwmod_enable+0x28/0x3c)
Quoted 1 line
[    0.000000] [<c0059d28>] (omap_hwmod_enable+0x28/0x3c) from
[<c005580c>] (omap2_disable_wdt+0x48/0xdc)
Quoted 1 line
[    0.000000] [<c005580c>] (omap2_disable_wdt+0x48/0xdc) from
[<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
Quoted 1 line
[    0.000000] [<c0058898>] 
(omap_hwmod_for_each_by_class+0x60/0xa4)
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
from [<c000f6d0>] (omap2_init_devices+0x44/0x330)
Quoted 1 line
[    0.000000] [<c000f6d0>] (omap2_init_devices+0x44/0x330) from
[<c0049578>] (do_one_initcall+0xcc/0x1a4)
Quoted 1 line
[    0.000000] [<c0049578>] (do_one_initcall+0xcc/0x1a4) from
[<c0008780>] (kernel_init+0x148/0x210)
Quoted 1 line
[    0.000000] [<c0008780>] (kernel_init+0x148/0x210) 
from [<c004acb8>]
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
(kernel_thread_exit+0x0/0x8)
Quoted 2 lines
[    0.000000] ---[ end trace 1b75b31a2719ed1f ]---
[    0.000000] wd_timer2: Could not enable clocks for 
omap2_disable_wdt
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
When CONFIG_PM_RUNTIME is not defined, watchdog timer
clocks are not
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
disabled after a watchdog reset. Hence in
omap2_disable_wdt(), it is
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
not required to enable clocks before accessing the watchdog timer registers if CONFIG_PM_RUNTIME is not defined.
I'm not a big fan of adding some dependencies with
CONFIG_PM_RUNTIME
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
inside this code.
The real root cause is not CONFIG_PM_RUNTIME, but mostly the skip_setup_idle variable added in
arch/arm/mach-omap2/io.c by Paul.
Quoted 5 lines
Quoted 5 lines
Yes.
Quoted 2 lines
So I'd rather use that parameter as an input for the
omap2_disable_wdt.
Quoted 12 lines
Quoted 12 lines
I also thought about this, but we need to
1. call omap2_disable_wdt() in omap2_init_common_hw() rather than omap2_init_devices(). That would involve movement of omap2_disable_wdt() from devices.c to io.c.
(or)
2. make skip_setup_idle global
(or)
3. make omap2_disable_wdt() global and call it from omap2_init_common_hw() and pass skip_setup_idle as parameter.
Quoted 1 line
Quoted 1 line
I felt that the usage of CONFIG_PM_RUNTIME is better than
the above.
Quoted 6 lines
Quoted 2 lines
But still we may consider any of the above options or any other option. Please suggest.
It is maybe easier, but I don't think it is better...
As I said, CONFIG_PM_RUNTIME is not the root cause of your
issue, just
Quoted 1 line
an indirect cause. OK, maybe strictly speaking, it is the
root cause,
Quoted 5 lines
since it started for that... but that not the cause that we should consider in your case.
If we decide to remove that CONFIG_PM_RUNTIME dependency in the io / hwmod code, nobody will be able to find the relation with
your code in
Quoted 6 lines
WDT.
That's why you should not do that, for my point of view.
Paul, Kevin,
Any thoughts on that point?
I agree that this is not strictly a dependency on PM_RUNTIME.
What's really needed is a way for low-level hwmod users like this to be
able to check whether the hwmod is already enabled. Something like this
(completely untested):

Quoted 4 lines

bool omap_hwmod_is_enabled(struct omap_hwmod *oh)
{
	return (oh->_state == _HWMOD_STATE_ENABLED);
}

Kevin,

Thanks. This is similar to what we were discussing internally and I would prefer this. But is it a good idea to allow drivers to know the state of oh and then enable/idle the device?

Or should it be used only in exceptional situations like the scenario addressed in this patch?

Quoted 77 lines

Alternatively, maybe calling omap_hwmod_enable() when the state is already enabled should not be so verbose, and it should just return success, but that option needs a little more thought...
Kevin
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Regards,
Benoit
Quoted 8 lines
Tested on OMAP3430 SDP and OMAP4430 SDP.
Signed-off-by Charulatha V<[email protected]>
---
   arch/arm/mach-omap2/devices.c |    6 +++++-
   1 files changed, 5 insertions(+), 1 deletions(-)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
arch/arm/mach-
Quoted 1 line
arch/arm/mach- renamed from arch/arm/mach-omap2/devices.c +0 -0
omap2/devices.c
arch/arm/mach-omap2/devices.c static inline void omap_init_vout(void) {} · line 926
Quoted 5 lines
index eaf3799..b592952 100644
arch/arm/mach-omap2/devices.c +0 -0
@@ -926,7 +926,7 @@ static inline void omap_init_vout(void) {}
   static int omap2_disable_wdt(struct omap_hwmod *oh, 
void *unused)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
arch/arm/mach-omap2/devices.c static inline void omap_init_vout(void) {} · line 932
Quoted 7 lines
arch/arm/mach-omap2/devices.c +1 -1
   {
   	void __iomem *base;
-	int ret;
+	int ret = 0;
 
   	if (!oh) {
   		pr_err("%s: Could not look up 
wdtimer_hwmod\n", __func__);
Quoted 1 line
Quoted 1 line
Quoted 1 line
arch/arm/mach-omap2/devices.c static int omap2_disable_wdt(struct
Quoted 1 line
arch/arm/mach-omap2/devices.c +0 -0
@@ -940,6 +940,7 @@ static int omap2_disable_wdt(struct 
omap_hwmod *oh,
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
void *unused)
Quoted 5 lines
arch/arm/mach-omap2/devices.c +1 -0
   		return -EINVAL;
   	}
 
+#ifdef CONFIG_PM_RUNTIME
   	/* Enable the clocks before accessing the WDT 
registers */
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
arch/arm/mach-omap2/devices.c static int omap2_disable_wdt(struct
Quoted 3 lines
arch/arm/mach-omap2/devices.c +0 -0
   	ret = omap_hwmod_enable(oh);
   	if (ret) {
@@ -947,6 +948,7 @@ static int omap2_disable_wdt(struct 
omap_hwmod *oh,
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
void *unused)
arch/arm/mach-omap2/devices.c static int
Quoted 8 lines
arch/arm/mach-omap2/devices.c +1 -0
   				oh->name, __func__);
   		return ret;
   	}
+#endif
 
   	/* sequence required to disable watchdog */
   	__raw_writel(0xAAAA, base + OMAP_WDT_SPR);
@@ -957,10 +959,12 @@ static int 
omap2_disable_wdt(struct omap_hwmod *oh,
Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
void *unused)
Quoted 12 lines
arch/arm/mach-omap2/devices.c +2 -0
   	while (__raw_readl(base + OMAP_WDT_WPS)&   0x10)
   		cpu_relax();
 
+#ifdef CONFIG_PM_RUNTIME
   	ret = omap_hwmod_idle(oh);
   	if (ret)
   		pr_err("%s: Could not disable clocks for %s\n",
   				oh->name, __func__);
+#endif
 
   	return ret;
   }

Paul Walmsley

maintainer

2010-10-14 18:37 UTC #6 lore raw

Hello Charu, Kevin, Benoît,

Quoted 1 line · Charulatha

Please provide your input on this.

Thinking about this problem -- and the earlier problem of how to determine what state to leave the watchdog in -- what occurs to me is that we should probably:

1. add a function pointer in the struct omap_hwmod_class for a custom pre-shutdown function, called before clocks are disabled, etc.; that can do the appropriate register writes needed during an omap_hwmod_shutdown() while the device is still considered to be enabled;

2. add a function to allow OMAP core code or board-*.c code to tell the hwmod code to leave a hwmod disabled at the end of _setup(), rather than idled or left enabled -- something like omap_hwmod_set_postsetup_state(HWMOD_STATE_DISABLED);

3. look to see if we can detect if the watchdog is enabled before the hwmod code starts -- perhaps by reading WSPR? -- and if it is not enabled, calling omap_hwmod_set_postsetup_state(HWMOD_STATE_DISABLED) prior to calling omap_hwmod_late_init();

4. disable the MPU watchdog by default if #3 is not possible, and then allow specific board files to indicate that it should be enabled if they want full kernel boot watchdog coverage with something like omap_hwmod_set_postsetup_state(HWMOD_STATE_IDLE); and possibly also add a kernel command line parameter like "enable_omapwdt_boot" that can specify that the watchdog should be left enabled upon boot.

This arrangement is a little more complicated, but it ensures that omap_hwmod_shutdown() really will shut down the watchdog. It also makes it possible for individual board files or bootloader configurations to ensure full watchdog coverage of the kernel initialization process. If I were designing a commercial OMAP-based device, I'd probably want full watchdog coverage of the entire boot process from the bootloader onwards. But simultaneously we don't want to change the existing kernel behavior too much -- most of our developer users would probably be shocked if, upon upgrading to a new kernel, suddenly the watchdog started rebooting their machines -- so it makes sense to me to disable the watchdog unless instructed otherwise.

Enclosed below is a work-in-progress patch to illustrate the proposal. It clearly needs some more work and should be split into at least two patches, but it might help clarify what I am proposing.

We also should, IMHO, move the struct omap_hwmod_class definitions out of the main hwmod data files, since they may contain function pointers. (We will probably also need a .reset function pointer there also to deal with complicated IPs like the IVA2, as we discussed a few months ago.) We should try to restrict the main omap_hwmod data files to be relatively OS-independent data structures.

Regarding the omap_hwmod_is_enabled() proposal: it seems racy to me. In theory the system state can change after the result of omap_hwmod_is_enabled() is tested, but before the consequence of the conditional statement runs. There used to be a similar problem in the clock framework with a clk_is_enabled() function (a similar race still exists with the clk_get_rate() function). If possible, I think we should try to avoid creating APIs that are explicitly racy.

Charu, I think that functions like omap2_disable_wdt() and any related watchdog device manipulation functions should go into their own file -- something like arch/arm/mach-omap2/wd_timer.c ? -- and not live in devices.c.

One last thing: Benoît, in the hwmod data, I think we should separate watchdog IPs that are automatically enabled upon reset (like the MPU watchdog), from watchdogs that are disabled after reset (like the IVA2 watchdog). It doesn't matter to me whether we create a second class, or whether we just use a dev_attr flag. If we separate these, then code can use omap_hwmod_for_each_class() to ensure that all watchdogs of this type are disabled upon boot, rather than hardcoding a specific hwmod.

Thoughts?

...

As an aside, we seem to again be skirting issues of what it means for a hwmod to be "enabled" and "idle." If we read "enabled" as "fully functional and registers accessible," that seems to make sense; but if we consider idle as meaning "configured but nonfunctional," that does not match up to the state that modules like WDT and GPIO are left in. The issue with WDT is that it probably does not IdleAck when enabled and its clocks are turned off, as Benoît mentioned; and the issue with GPIO is that it can generate asynchronous interrupts, even when all its clocks are off. It would be good if we could clarify the semantics for these states in some formal way. Perhaps more states are needed at the hwmod level? Or perhaps we just need to clarify what "idle" really means...

- Paul

From 2e05f1c33b2c5b081146079a2908fe5bbb4350d6 Mon Sep 17 00:00:00 2001
From: Paul Walmsley <[email protected]>
Date: Thu, 14 Oct 2010 11:47:46 -0600
Subject: [RFC PATCH] OMAP2+: hwmod: add postsetup state suppotr

Experimental, didactic patch to add postsetup state, selected at runtime. This patch needs major work before merging; it's simply intended to illustrate a proposal.

Not-yet-signed-off-by Paul Walmsley <[email protected]>

---
 arch/arm/mach-omap2/io.c                     |   53 ++++++++++++++++++++---
 arch/arm/mach-omap2/omap_hwmod.c             |   59 +++++++++++++++++++-------
 arch/arm/plat-omap/include/plat/omap_hwmod.h |   16 +++++++-
 3 files changed, 105 insertions(+), 23 deletions(-)

arch/arm/mach-omap2/io.c +46 -7

index 40562dd..e3a423d 100644
@@ -311,10 +311,15 @@ static int __init _omap2_init_reprogram_sdrc(void)
 	return v;
 }
 
-void __init omap2_init_common_hw(struct omap_sdrc_params *sdrc_cs0,
-				 struct omap_sdrc_params *sdrc_cs1)
+/* XXX document */
+static int _set_hwmod_postsetup_state(struct omap_hwmod *oh, void *data)
+{
+	return omap_hwmod_set_postsetup_state(oh, *(u8 *)data);
+}
+
+static void __init omap2_init_common_infra(void)
 {
-	u8 skip_setup_idle = 0;
+	u8 postsetup_state;
 
 	pwrdm_init(powerdomains_omap);
 	clkdm_init(clockdomains_omap, clkdm_autodeps);

@@ -327,6 +332,24 @@ void __init omap2_init_common_hw(struct omap_sdrc_params *sdrc_cs0,
 	else if (cpu_is_omap44xx())
 		omap44xx_hwmod_init();
 
+	/* Set the default postsetup state for all hwmods ... */
+#ifdef CONFIG_PM_RUNTIME
+	postsetup_state = _HWMOD_STATE_IDLE;
+#else
+	postsetup_state = _HWMOD_STATE_ENABLED;
+#endif
+	omap_hwmod_for_each(_set_hwmod_postsetup_state, &postsetup_state);
+
+	/* ... then handle the exceptions for unusual modules (like MPU WDT) */
+	/*
+	 * 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_enabled_on_reset",
+				     _set_hwmod_postsetup_state,
+				     &postsetup_state);
+
 	/* The OPP tables have to be registered before a clk init */
 	omap_pm_if_early_init(mpu_opps, dsp_opps, l3_opps);

@@ -340,16 +363,32 @@ void __init omap2_init_common_hw(struct omap_sdrc_params *sdrc_cs0,
 		omap4xxx_clk_init();
 	else
 		pr_err("Could not init clock framework - unknown CPU\n");
+}
 
+static void __init omap2_init_common_devices(struct omap_sdrc_params *sdrc_cs0,
+					     struct omap_sdrc_params *sdrc_cs1)
+{
 	omap_serial_early_init();
 
-#ifndef CONFIG_PM_RUNTIME
-	skip_setup_idle = 1;
-#endif
-	omap_hwmod_late_init(skip_setup_idle);
+	omap_hwmod_late_init();
+
 	if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
 		omap2_sdrc_init(sdrc_cs0, sdrc_cs1);
 		_omap2_init_reprogram_sdrc();
 	}
 	gpmc_init();
 }
+
+/*
+ * XXX this function should go away and the board-*.c files should call
+ * both omap2_init_common_infra() and omap2_init_common_devices(),
+ * so the board-*.c files can override some default hwmod postsetup_states
+ * (e.g., for watchdog) between the two functions.
+ */
+void __init omap2_init_common_hw(struct omap_sdrc_params *sdrc_cs0,
+				 struct omap_sdrc_params *sdrc_cs1)
+{
+	omap2_init_common_infra();
+	omap2_init_common_devices(sdrc_cs0, sdrc_cs1);
+}
+

arch/arm/mach-omap2/omap_hwmod.c +44 ~~-15~~

102 lines not on this page · raw message

arch/arm/plat-omap/include/plat/omap_hwmod.h +15 -1

index 7eaa8ed..a566efc 100644
@@ -415,14 +415,24 @@ struct omap_hwmod_omap4_prcm {
  * @name: name of the hwmod_class
  * @sysc: device SYSCONFIG/SYSSTATUS register data
  * @rev: revision of the IP class
+ * @pre_shutdown: fn ptr to be executed immediately prior to device shutdown
  *
  * Represent the class of a OMAP hardware "modules" (e.g. timer,
  * smartreflex, gpio, uart...)
+ *
+ * @pre_shutdown is a function that will be run immediately before
+ * hwmod clocks are disabled, etc.  It is intended for use for hwmods
+ * like the MPU watchdog, which cannot be disabled with the standard
+ * omap_hwmod_shutdown().  The function should return 0 upon success,
+ * or some negative error upon failure.  Returning an error will cause
+ * omap_hwmod_shutdown() to abort the device shutdown and return an
+ * error.
  */
 struct omap_hwmod_class {
 	const char				*name;
 	struct omap_hwmod_class_sysconfig	*sysc;
 	u32					rev;
+	int					(*pre_shutdown)(struct omap_hwmod *oh);
 };
 
 /**

@@ -452,6 +462,7 @@ struct omap_hwmod_class {
  * @response_lat: device OCP response latency (in interface clock cycles)
  * @_int_flags: internal-use hwmod flags
  * @_state: internal-use hwmod state
+ * @_postsetup_state: internal-use state to leave the hwmod in after _setup()
  * @flags: hwmod flags (documented below)
  * @omap_chip: OMAP chips this hwmod is present on
  * @_mutex: mutex serializing operations on this hwmod

@@ -500,6 +511,7 @@ struct omap_hwmod {
 	u8				hwmods_cnt;
 	u8				_int_flags;
 	u8				_state;
+	u8				_postsetup_state;
 	const struct omap_chip_id	omap_chip;
 };

@@ -509,7 +521,7 @@ int omap_hwmod_unregister(struct omap_hwmod *oh);
 struct omap_hwmod *omap_hwmod_lookup(const char *name);
 int omap_hwmod_for_each(int (*fn)(struct omap_hwmod *oh, void *data),
 			void *data);
-int omap_hwmod_late_init(u8 skip_setup_idle);
+int omap_hwmod_late_init(void);
 
 int omap_hwmod_enable(struct omap_hwmod *oh);
 int _omap_hwmod_enable(struct omap_hwmod *oh);

@@ -556,6 +568,8 @@ int omap_hwmod_for_each_by_class(const char *classname,
 					   void *user),
 				 void *user);
 
+int omap_hwmod_set_postsetup_state(struct omap_hwmod *oh, u8 state);
+
 /*
  * Chip variant-specific hwmod init routines - XXX should be converted
  * to use initcalls once the initial boot ordering is straightened out

Signature · 1 line

1.7.1

Paul Walmsley

maintainer

2010-10-14 18:52 UTC #7 lore raw

Quoted 3 lines · Paul Walmsley

Enclosed below is a work-in-progress patch to illustrate the proposal. It clearly needs some more work and should be split into at least two patches, but it might help clarify what I am proposing.

Here's one important missing piece of the patch that I wanted to illustrate...

- Paul

Quoted 1 line

From de3b800f9a6db027846f68de3aa4202eb713b391 Mon Sep 17 00:00:00 2001

From: Paul Walmsley <[email protected]>
Date: Thu, 14 Oct 2010 12:48:00 -0600
Subject: [RFC PATCH] OMAP2+ hwmod: illustrate pre-shutdown hook implementation

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

arch/arm/mach-omap2/omap_hwmod.c +15 -0

index 618e135..92dd369 100644
@@ -1261,6 +1261,9 @@ int _omap_hwmod_idle(struct omap_hwmod *oh)
  */
 static int _shutdown(struct omap_hwmod *oh)
 {
+	int ret;
+	u8 prev_state;
+
 	if (oh->_state != _HWMOD_STATE_IDLE &&
 	    oh->_state != _HWMOD_STATE_ENABLED) {
 		WARN(1, "omap_hwmod: %s: disabled state can only be entered "

@@ -1270,6 +1273,18 @@ static int _shutdown(struct omap_hwmod *oh)
 
 	pr_debug("omap_hwmod: %s: disabling\n", oh->name);
 
+	if (oh->class->pre_shutdown) {
+		prev_state = oh->_state;
+		if (oh->_state == _HWMOD_STATE_IDLE)
+			_omap_hwmod_enable(oh);
+		ret = oh->class->pre_shutdown(oh);
+		if (ret) {
+			if (prev_state == _HWMOD_STATE_IDLE)
+				_omap_hwmod_idle(oh);
+			return ret;
+		}
+	}
+
 	if (oh->class->sysc)
 		_shutdown_sysc(oh);

Signature · 1 line

1.7.1

Varadarajan, Charulatha

2010-10-19 11:27 UTC #8 lore raw

In reply to Paul Walmsley

Paul,

Quoted 10 lines

-----Original Message-----
From: Paul Walmsley [mailto:[email protected]]
Sent: Friday, October 15, 2010 12:08 AM
To: Varadarajan, Charulatha
Cc: Kevin Hilman; Cousson, Benoit; [email protected]
Subject: RE: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n
Hello Charu, Kevin, Benoît,

Quoted 7 lines

Quoted 1 line · Charulatha
Please provide your input on this.
Thinking about this problem -- and the earlier problem of how to determine what state to leave the watchdog in -- what occurs to me is that we should probably:

Thanks for providing detailed input on this.

Quoted 57 lines

1. add a function pointer in the struct omap_hwmod_class for a custom pre-shutdown function, called before clocks are disabled, etc.; that can do the appropriate register writes needed during an omap_hwmod_shutdown() while the device is still considered to be enabled;
2. add a function to allow OMAP core code or board-*.c code to tell the hwmod code to leave a hwmod disabled at the end of _setup(), rather than idled or left enabled -- something like omap_hwmod_set_postsetup_state(HWMOD_STATE_DISABLED);
3. look to see if we can detect if the watchdog is enabled before the hwmod code starts -- perhaps by reading WSPR? -- and if it is not enabled, calling omap_hwmod_set_postsetup_state(HWMOD_STATE_DISABLED) prior to calling omap_hwmod_late_init();
4. disable the MPU watchdog by default if #3 is not possible, and then allow specific board files to indicate that it should be enabled if they want full kernel boot watchdog coverage with something like omap_hwmod_set_postsetup_state(HWMOD_STATE_IDLE); and possibly also add a kernel command line parameter like "enable_omapwdt_boot" that can specify that the watchdog should be left enabled upon boot.
This arrangement is a little more complicated, but it ensures that omap_hwmod_shutdown() really will shut down the watchdog. It also makes it possible for individual board files or bootloader configurations to ensure full watchdog coverage of the kernel initialization process. If I were designing a commercial OMAP-based device, I'd probably want full watchdog coverage of the entire boot process from the bootloader onwards. But simultaneously we don't want to change the existing kernel behavior too much -- most of our developer users would probably be shocked if, upon upgrading to a new kernel, suddenly the watchdog started rebooting their machines -- so it makes sense to me to disable the watchdog unless instructed otherwise.
Enclosed below is a work-in-progress patch to illustrate the proposal. It clearly needs some more work and should be split into at least two patches, but it might help clarify what I am proposing.
We also should, IMHO, move the struct omap_hwmod_class definitions out of the main hwmod data files, since they may contain function pointers. (We will probably also need a .reset function pointer there also to deal with complicated IPs like the IVA2, as we discussed a few months ago.) We should try to restrict the main omap_hwmod data files to be relatively OS-independent data structures.
Regarding the omap_hwmod_is_enabled() proposal: it seems racy to me. In theory the system state can change after the result of omap_hwmod_is_enabled() is tested, but before the consequence of the conditional statement runs. There used to be a similar problem in the clock framework with a clk_is_enabled() function (a similar race still exists with the clk_get_rate() function). If possible, I think we should try to avoid creating APIs that are explicitly racy.

Quoted 4 lines

Charu, I think that functions like omap2_disable_wdt() and any related watchdog device manipulation functions should go into their own file -- something like arch/arm/mach-omap2/wd_timer.c ? -- and not live in devices.c.

Yes, you are right. I can do this in a new file arch/arm/mach-omap2/wd_timer.c. After doing this, I can also move the omap_device_build of wdt device from devices.c to mach-omap2/wd_timer.c.

-V Charulatha

Quoted 326 lines

Thoughts?

...

- Paul

Experimental, didactic patch to add postsetup state, selected at runtime. This patch needs major work before merging; it's simply intended to illustrate a proposal.

Not-yet-signed-off-by Paul Walmsley <[email protected]>

---
 arch/arm/mach-omap2/io.c                     |   53 ++++++++++++++++++++-
--
 arch/arm/mach-omap2/omap_hwmod.c             |   59 +++++++++++++++++++--
-----
 arch/arm/plat-omap/include/plat/omap_hwmod.h |   16 +++++++-
 3 files changed, 105 insertions(+), 23 deletions(-)

arch/arm/mach-omap2/io.c +26 -3

index 40562dd..e3a423d 100644
@@ -311,10 +311,15 @@ static int __init _omap2_init_reprogram_sdrc(void)
      return v;
 }
 
-void __init omap2_init_common_hw(struct omap_sdrc_params *sdrc_cs0,
-                              struct omap_sdrc_params *sdrc_cs1)
+/* XXX document */
+static int _set_hwmod_postsetup_state(struct omap_hwmod *oh, void *data)
+{
+     return omap_hwmod_set_postsetup_state(oh, *(u8 *)data);
+}
+
+static void __init omap2_init_common_infra(void)
 {
-     u8 skip_setup_idle = 0;
+     u8 postsetup_state;
 
      pwrdm_init(powerdomains_omap);
      clkdm_init(clockdomains_omap, clkdm_autodeps);

@@ -327,6 +332,24 @@ void __init omap2_init_common_hw(struct
 omap_sdrc_params *sdrc_cs0,
      else if (cpu_is_omap44xx())
              omap44xx_hwmod_init();
 
+     /* Set the default postsetup state for all hwmods ... */
+#ifdef CONFIG_PM_RUNTIME
+     postsetup_state = _HWMOD_STATE_IDLE;
+#else
+     postsetup_state = _HWMOD_STATE_ENABLED;
+#endif
+     omap_hwmod_for_each(_set_hwmod_postsetup_state, &postsetup_state);
+
+     /* ... then handle the exceptions for unusual modules (like MPU WDT)
 */
+     /*
+      * 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_enabled_on_reset",
+                                  _set_hwmod_postsetup_state,
+                                  &postsetup_state);
+
      /* The OPP tables have to be registered before a clk init */

      omap_pm_if_early_init(mpu_opps, dsp_opps, l3_opps);

arch/arm/mach-omap2/io.c +11 -4

@@ -340,16 +363,32 @@ void __init omap2_init_common_hw(struct
 omap_sdrc_params *sdrc_cs0,
              omap4xxx_clk_init();
      else
              pr_err("Could not init clock framework - unknown CPU\n");
+}
 
+static void __init omap2_init_common_devices(struct omap_sdrc_params
 *sdrc_cs0,
+                                          struct omap_sdrc_params *sdrc_cs1)
+{
      omap_serial_early_init();
 
-#ifndef CONFIG_PM_RUNTIME
-     skip_setup_idle = 1;
-#endif
-     omap_hwmod_late_init(skip_setup_idle);
+     omap_hwmod_late_init();
+
      if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
              omap2_sdrc_init(sdrc_cs0, sdrc_cs1);
              _omap2_init_reprogram_sdrc();
      }
      gpmc_init();
 }
+
+/*
+ * XXX this function should go away and the board-*.c files should call
+ * both omap2_init_common_infra() and omap2_init_common_devices(),
+ * so the board-*.c files can override some default hwmod

postsetup_states
+ * (e.g., for watchdog) between the two functions.
+ */

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

+void __init omap2_init_common_hw(struct omap_sdrc_params *sdrc_cs0,
+                              struct omap_sdrc_params *sdrc_cs1)
+{
+     omap2_init_common_infra();
+     omap2_init_common_devices(sdrc_cs0, sdrc_cs1);
+}
+

arch/arm/mach- renamed from arch/arm/mach-omap2/omap_hwmod.c +0 -0

omap2/omap_hwmod.c
index 5a30658..618e135 100644

arch/arm/mach-omap2/omap_hwmod.c +10 -9

@@ -1298,24 +1298,19 @@ static int _shutdown(struct omap_hwmod *oh)
 /**
  * _setup - do initial configuration of omap_hwmod
  * @oh: struct omap_hwmod *
- * @skip_setup_idle_p: do not idle hwmods at the end of the fn if 1
  *
  * Writes the CLOCKACTIVITY bits @clockact to the hwmod @oh
- * OCP_SYSCONFIG register.  @skip_setup_idle is intended to be used on
- * a system that will not call omap_hwmod_enable() to enable devices
- * (e.g., a system without PM runtime).  Returns -EINVAL if the hwmod
- * is in the wrong state or returns 0.
+ * OCP_SYSCONFIG register.  Returns -EINVAL if the hwmod is in the
+ * wrong state or returns 0.
  */
 static int _setup(struct omap_hwmod *oh, void *data)
 {
      int i, r;
-     u8 skip_setup_idle;
+     u8 postsetup_state;
 
      if (!oh || !data)
              return -EINVAL;
 
-     skip_setup_idle = *(u8 *)data;
-
      /* Set iclk autoidle mode */
      if (oh->slaves_cnt > 0) {
              for (i = 0; i < oh->slaves_cnt; i++) {

@@ -1368,8 +1363,24 @@ static int _setup(struct omap_hwmod *oh, void
 *data)
              }
      }
 
-     if (!(oh->flags & HWMOD_INIT_NO_IDLE) && !skip_setup_idle)
+     postsetup_state = oh->_postsetup_state;
+     if (postsetup_state == _HWMOD_STATE_UNKNOWN)
+             postsetup_state = _HWMOD_STATE_ENABLED;
+
+     /*
+      * XXX HWMOD_INIT_NO_IDLE does not belong in hwmod data -
+      * it should be set by the core code as a runtime flag during

startup

arch/arm/mach-omap2/omap_hwmod.c +9 -0

+      */
+     if (!(oh->flags & HWMOD_INIT_NO_IDLE))
+             postsetup_state = _HWMOD_STATE_IDLE;
+
+     if (postsetup_state == _HWMOD_STATE_IDLE)
              _omap_hwmod_idle(oh);
+     else if (postsetup_state == _HWMOD_STATE_DISABLED)
+             _shutdown(oh);
+     else
+             WARN(1, "hwmod: %s: unknown postsetup state %d! defaulting to

enabled\n",

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

+                  oh->name, postsetup_state);
 
      return 0;
 }

@@ -1570,13 +1581,12 @@ int omap_hwmod_init(struct omap_hwmod **ohs)
 
 /**
  * omap_hwmod_late_init - do some post-clock framework initialization
- * @skip_setup_idle: if 1, do not idle hwmods in _setup()
  *
  * Must be called after omap2_clk_init().  Resolves the struct clk names
  * to struct clk pointers for each registered omap_hwmod.  Also calls
  * _setup() on each hwmod.  Returns 0.
  */
-int omap_hwmod_late_init(u8 skip_setup_idle)
+int omap_hwmod_late_init(void)
 {
      int r;

@@ -1588,10 +1598,7 @@ int omap_hwmod_late_init(u8 skip_setup_idle)
      WARN(!mpu_oh, "omap_hwmod: could not find MPU initiator hwmod %s\n",
           MPU_INITIATOR_NAME);
 
-     if (skip_setup_idle)
-             pr_debug("omap_hwmod: will leave hwmods enabled during
 setup\n");
-
-     omap_hwmod_for_each(_setup, &skip_setup_idle);
+     omap_hwmod_for_each(_setup, NULL);
 
      return 0;

}

arch/arm/mach-omap2/omap_hwmod.c +3 -0

@@ -2117,3 +2124,25 @@ int omap_hwmod_for_each_by_class(const char
 *classname,
      return ret;
 }
 
+
+/**
+ * omap_hwmod_set_postsetup_state - set the post-_setup() state for this

hwmod
+ * @oh: struct omap_hwmod *
+ * @state: state that _setup() should leave the hwmod in
+ *
+ * XXX document
+ *
+ */

arch/arm/mach-omap2/omap_hwmod.c +13 -0

+int omap_hwmod_set_postsetup_state(struct omap_hwmod *oh, u8 state)
+{
+     if (!oh)
+             return -EINVAL;
+
+     /* XXX insure valid @state passed in */
+
+     mutex_lock(&oh->_mutex);
+     oh->_postsetup_state = state;
+     mutex_unlock(&oh->_mutex);
+
+     return 0;
+}

arch/arm/plat- renamed from arch/arm/plat-omap/include/plat/omap_hwmod.h +0 -0

omap/include/plat/omap_hwmod.h
index 7eaa8ed..a566efc 100644

arch/arm/plat-omap/include/plat/omap_hwmod.h +1 -0

@@ -415,14 +415,24 @@ struct omap_hwmod_omap4_prcm {
  * @name: name of the hwmod_class
  * @sysc: device SYSCONFIG/SYSSTATUS register data
  * @rev: revision of the IP class
+ * @pre_shutdown: fn ptr to be executed immediately prior to device

shutdown
*
* Represent the class of a OMAP hardware "modules" (e.g. timer,
* smartreflex, gpio, uart...)
+ *
+ * @pre_shutdown is a function that will be run immediately before
+ * hwmod clocks are disabled, etc. It is intended for use for hwmods
+ * like the MPU watchdog, which cannot be disabled with the standard
+ * omap_hwmod_shutdown(). The function should return 0 upon success,
+ * or some negative error upon failure. Returning an error will cause
+ * omap_hwmod_shutdown() to abort the device shutdown and return an
+ * error.
*/

 struct omap_hwmod_class {
      const char                              *name;
      struct omap_hwmod_class_sysconfig       *sysc;
      u32                                     rev;

+ int (*pre_shutdown)(struct omap_hwmod
*oh);
};

/**

arch/arm/plat-omap/include/plat/omap_hwmod.h +1 -0

@@ -452,6 +462,7 @@ struct omap_hwmod_class {
  * @response_lat: device OCP response latency (in interface clock cycles)
  * @_int_flags: internal-use hwmod flags
  * @_state: internal-use hwmod state
+ * @_postsetup_state: internal-use state to leave the hwmod in after
 _setup()
  * @flags: hwmod flags (documented below)
  * @omap_chip: OMAP chips this hwmod is present on

* @_mutex: mutex serializing operations on this hwmod

arch/arm/plat-omap/include/plat/omap_hwmod.h +4 -2

@@ -500,6 +511,7 @@ struct omap_hwmod {
      u8                              hwmods_cnt;
      u8                              _int_flags;
      u8                              _state;
+     u8                              _postsetup_state;
      const struct omap_chip_id       omap_chip;
 };

@@ -509,7 +521,7 @@ int omap_hwmod_unregister(struct omap_hwmod *oh);
 struct omap_hwmod *omap_hwmod_lookup(const char *name);
 int omap_hwmod_for_each(int (*fn)(struct omap_hwmod *oh, void *data),
                      void *data);
-int omap_hwmod_late_init(u8 skip_setup_idle);
+int omap_hwmod_late_init(void);
 
 int omap_hwmod_enable(struct omap_hwmod *oh);
 int _omap_hwmod_enable(struct omap_hwmod *oh);

@@ -556,6 +568,8 @@ int omap_hwmod_for_each_by_class(const char *classname,
                                         void *user),
                               void *user);
 
+int omap_hwmod_set_postsetup_state(struct omap_hwmod *oh, u8 state);
+
 /*
  * Chip variant-specific hwmod init routines - XXX should be converted
  * to use initcalls once the initial boot ordering is straightened out
--

Quoted 1 line

1.7.1

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

Kevin Hilman

2010-10-20 00:00 UTC #9 lore raw

In reply to Varadarajan, Charulatha

Quoted 128 lines · Charulatha

Paul, Benoit,
Please provide your input on this.
Quoted 119 lines
-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Tuesday, October 12, 2010 11:57 PM
To: Cousson, Benoit
Cc: Varadarajan, Charulatha; Paul Walmsley; [email protected]
Subject: Re: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n
Quoted 16 lines · Benoit
Adding more folks to the discussion.
Quoted 13 lines · Charulatha
Quoted 13 lines
From: Cousson, Benoit
Sent: Tuesday, October 12, 2010 12:45 PM
To: Varadarajan, Charulatha
Hi Charu,
Quoted 3 lines · Charulatha
Fix the below warning during boot
[    0.000000] WARNING: at arch/arm/mach-omap2/omap_hwmod.c:1185
_omap_hwmod_enable+0x34/0x150()
Quoted 1 line
[    0.000000] omap_hwmod: wd_timer2: enabled state can 
only be entered
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
from initialized, idle, or disabled state
Quoted 2 lines
[    0.000000] Modules linked in:
[    0.000000] [<c0050460>] (unwind_backtrace+0x0/0xe4) from
[<c0083954>] (warn_slowpath_common+0x4c/0x64)
Quoted 1 line
[    0.000000] [<c0083954>] (warn_slowpath_common+0x4c/0x64) from
[<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c)
Quoted 1 line
[    0.000000] [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c) from
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
[<c0059be4>] (_omap_hwmod_enable+0x34/0x150)
Quoted 1 line
[    0.000000] [<c0059be4>] (_omap_hwmod_enable+0x34/0x150) from
[<c0059d28>] (omap_hwmod_enable+0x28/0x3c)
Quoted 1 line
[    0.000000] [<c0059d28>] (omap_hwmod_enable+0x28/0x3c) from
[<c005580c>] (omap2_disable_wdt+0x48/0xdc)
Quoted 1 line
[    0.000000] [<c005580c>] (omap2_disable_wdt+0x48/0xdc) from
[<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
Quoted 1 line
[    0.000000] [<c0058898>] 
(omap_hwmod_for_each_by_class+0x60/0xa4)
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
from [<c000f6d0>] (omap2_init_devices+0x44/0x330)
Quoted 1 line
[    0.000000] [<c000f6d0>] (omap2_init_devices+0x44/0x330) from
[<c0049578>] (do_one_initcall+0xcc/0x1a4)
Quoted 1 line
[    0.000000] [<c0049578>] (do_one_initcall+0xcc/0x1a4) from
[<c0008780>] (kernel_init+0x148/0x210)
Quoted 1 line
[    0.000000] [<c0008780>] (kernel_init+0x148/0x210) 
from [<c004acb8>]
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
(kernel_thread_exit+0x0/0x8)
Quoted 2 lines
[    0.000000] ---[ end trace 1b75b31a2719ed1f ]---
[    0.000000] wd_timer2: Could not enable clocks for 
omap2_disable_wdt
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
When CONFIG_PM_RUNTIME is not defined, watchdog timer
clocks are not
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
disabled after a watchdog reset. Hence in
omap2_disable_wdt(), it is
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
not required to enable clocks before accessing the watchdog timer registers if CONFIG_PM_RUNTIME is not defined.
I'm not a big fan of adding some dependencies with
CONFIG_PM_RUNTIME
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
inside this code.
The real root cause is not CONFIG_PM_RUNTIME, but mostly the skip_setup_idle variable added in
arch/arm/mach-omap2/io.c by Paul.
Quoted 5 lines
Quoted 5 lines
Yes.
Quoted 2 lines
So I'd rather use that parameter as an input for the
omap2_disable_wdt.
Quoted 12 lines
Quoted 12 lines
I also thought about this, but we need to
1. call omap2_disable_wdt() in omap2_init_common_hw() rather than omap2_init_devices(). That would involve movement of omap2_disable_wdt() from devices.c to io.c.
(or)
2. make skip_setup_idle global
(or)
3. make omap2_disable_wdt() global and call it from omap2_init_common_hw() and pass skip_setup_idle as parameter.
Quoted 1 line
Quoted 1 line
I felt that the usage of CONFIG_PM_RUNTIME is better than
the above.
Quoted 6 lines
Quoted 2 lines
But still we may consider any of the above options or any other option. Please suggest.
It is maybe easier, but I don't think it is better...
As I said, CONFIG_PM_RUNTIME is not the root cause of your
issue, just
Quoted 1 line
an indirect cause. OK, maybe strictly speaking, it is the
root cause,
Quoted 5 lines
since it started for that... but that not the cause that we should consider in your case.
If we decide to remove that CONFIG_PM_RUNTIME dependency in the io / hwmod code, nobody will be able to find the relation with
your code in
Quoted 6 lines
WDT.
That's why you should not do that, for my point of view.
Paul, Kevin,
Any thoughts on that point?
I agree that this is not strictly a dependency on PM_RUNTIME.
What's really needed is a way for low-level hwmod users like this to be
able to check whether the hwmod is already enabled. Something like this
(completely untested):
Quoted 4 lines
bool omap_hwmod_is_enabled(struct omap_hwmod *oh)
{
	return (oh->_state == _HWMOD_STATE_ENABLED);
}

Quoted 8 lines

Kevin,
Thanks. This is similar to what we were discussing internally and I would prefer this. But is it a good idea to allow drivers to know the state of oh and then enable/idle the device?
Or should it be used only in exceptional situations like the scenario addressed in this patch?

For the long term (2.6.38), we should follow the direction that Paul laid out.

For the short term (2.6.37) what we need is a fix for this regression. I propose you follow the above approach (checking if it's already enabled.)

As it is a temporary hack, it's probably best not to create the new API that could be abused. Instead, just directly check oh->_state in the devices.c code (documented with FIXME comments.)

I'm OK with this short term fix for 2.6.37 if you agree to work on Paul's solution for 2.6.38.

Kevin

Varadarajan, Charulatha

2010-10-20 11:19 UTC #10 lore raw

Quoted 119 lines

-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Wednesday, October 20, 2010 5:31 AM
To: Varadarajan, Charulatha
Cc: Cousson, Benoit; Paul Walmsley; [email protected]
Subject: Re: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n
Quoted 9 lines · Charulatha
Paul, Benoit,
Please provide your input on this.
Quoted 5 lines
-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Tuesday, October 12, 2010 11:57 PM
To: Cousson, Benoit
Cc: Varadarajan, Charulatha; Paul Walmsley;
[email protected]
Quoted 13 lines
Quoted 13 lines
Subject: Re: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n
Quoted 8 lines · Benoit
Adding more folks to the discussion.
Quoted 5 lines · Charulatha
Quoted 5 lines
From: Cousson, Benoit
Sent: Tuesday, October 12, 2010 12:45 PM
To: Varadarajan, Charulatha
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Hi Charu,
Quoted 3 lines · Charulatha
Fix the below warning during boot
[    0.000000] WARNING: at 
arch/arm/mach-omap2/omap_hwmod.c:1185
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
_omap_hwmod_enable+0x34/0x150()
Quoted 1 line
[    0.000000] omap_hwmod: wd_timer2: enabled state can 
only be entered
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
from initialized, idle, or disabled state
Quoted 2 lines
[    0.000000] Modules linked in:
[    0.000000] [<c0050460>] (unwind_backtrace+0x0/0xe4) from
[<c0083954>] (warn_slowpath_common+0x4c/0x64)
Quoted 1 line
[    0.000000] [<c0083954>] 
(warn_slowpath_common+0x4c/0x64) from
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
[<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c)
Quoted 1 line
[    0.000000] [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c) from
[<c0059be4>] (_omap_hwmod_enable+0x34/0x150)
Quoted 1 line
[    0.000000] [<c0059be4>] 
(_omap_hwmod_enable+0x34/0x150) from
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
[<c0059d28>] (omap_hwmod_enable+0x28/0x3c)
Quoted 1 line
[    0.000000] [<c0059d28>] (omap_hwmod_enable+0x28/0x3c) from
[<c005580c>] (omap2_disable_wdt+0x48/0xdc)
Quoted 1 line
[    0.000000] [<c005580c>] (omap2_disable_wdt+0x48/0xdc) from
[<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
Quoted 1 line
[    0.000000] [<c0058898>] 
(omap_hwmod_for_each_by_class+0x60/0xa4)
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
from [<c000f6d0>] (omap2_init_devices+0x44/0x330)
Quoted 1 line
[    0.000000] [<c000f6d0>] 
(omap2_init_devices+0x44/0x330) from
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
[<c0049578>] (do_one_initcall+0xcc/0x1a4)
Quoted 1 line
[    0.000000] [<c0049578>] (do_one_initcall+0xcc/0x1a4) from
[<c0008780>] (kernel_init+0x148/0x210)
Quoted 1 line
[    0.000000] [<c0008780>] (kernel_init+0x148/0x210) 
from [<c004acb8>]
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
(kernel_thread_exit+0x0/0x8)
Quoted 2 lines
[    0.000000] ---[ end trace 1b75b31a2719ed1f ]---
[    0.000000] wd_timer2: Could not enable clocks for 
Quoted 7 lines
Quoted 7 lines
omap2_disable_wdt
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
When CONFIG_PM_RUNTIME is not defined, watchdog timer
clocks are not
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
disabled after a watchdog reset. Hence in
omap2_disable_wdt(), it is
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
not required to enable clocks before accessing the
watchdog timer
Quoted 13 lines
Quoted 13 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
registers if CONFIG_PM_RUNTIME is not defined.
I'm not a big fan of adding some dependencies with
CONFIG_PM_RUNTIME
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
inside this code.
The real root cause is not CONFIG_PM_RUNTIME, but mostly the skip_setup_idle variable added in
arch/arm/mach-omap2/io.c by Paul.
Quoted 4 lines
Quoted 4 lines
Yes.
Quoted 1 line
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
So I'd rather use that parameter as an input for the
omap2_disable_wdt.
Quoted 4 lines
Quoted 4 lines
I also thought about this, but we need to
1. call omap2_disable_wdt() in omap2_init_common_hw()
rather than
Quoted 17 lines
Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
omap2_init_devices(). That would involve movement of omap2_disable_wdt() from devices.c to io.c.
(or)
2. make skip_setup_idle global
(or)
3. make omap2_disable_wdt() global and call it from omap2_init_common_hw() and pass skip_setup_idle as parameter.
I felt that the usage of CONFIG_PM_RUNTIME is better than
the above.
Quoted 6 lines
Quoted 2 lines
But still we may consider any of the above options or any other option. Please suggest.
It is maybe easier, but I don't think it is better...
As I said, CONFIG_PM_RUNTIME is not the root cause of your
issue, just

Quoted 30 lines

Quoted 3 lines
Quoted 3 lines
Quoted 1 line
an indirect cause. OK, maybe strictly speaking, it is the
root cause,
Quoted 1 line
since it started for that... but that not the cause that
we should
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
consider in your case.
If we decide to remove that CONFIG_PM_RUNTIME dependency
in the io /
Quoted 22 lines
Quoted 21 lines
Quoted 1 line
hwmod code, nobody will be able to find the relation with
your code in
Quoted 6 lines
WDT.
That's why you should not do that, for my point of view.
Paul, Kevin,
Any thoughts on that point?
I agree that this is not strictly a dependency on PM_RUNTIME.
What's really needed is a way for low-level hwmod users like this to be
able to check whether the hwmod is already enabled. Something like this
(completely untested):
bool omap_hwmod_is_enabled(struct omap_hwmod *oh)
{
	return (oh->_state == _HWMOD_STATE_ENABLED);
}

Okay. Would do the needful and send the patch soon.

Quoted 28 lines

Quoted 7 lines
Kevin,
Thanks. This is similar to what we were discussing internally and I would prefer this. But is it a good idea to allow drivers to know the state of oh and then enable/idle the device?
Or should it be used only in exceptional situations like
the scenario
Quoted 1 line
addressed in this patch?
For the long term (2.6.38), we should follow the direction that Paul laid out.
For the short term (2.6.37) what we need is a fix for this regression. I propose you follow the above approach (checking if it's already enabled.)
As it is a temporary hack, it's probably best not to create the new API
that could be abused. Instead, just directly check oh->_state in the devices.c code (documented with FIXME comments.)
I'm OK with this short term fix for 2.6.37 if you agree to work on Paul's solution for 2.6.38.
Kevin