Re: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n

Re: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n

[Cousson, Benoit]

Adding more folks to the discussion.

On 10/12/2010 9:30 AM, Varadarajan, Charulatha wrote:
>
>> From: Cousson, Benoit
>> Sent: Tuesday, October 12, 2010 12:45 PM
>> To: Varadarajan, Charulatha
>>
>> Hi Charu,
>>
>> On 10/11/2010 2:02 PM, Varadarajan, Charulatha wrote:
>>> Fix the below warning during boot
>>>
>>> [    0.000000] WARNING: at arch/arm/mach-omap2/omap_hwmod.c:1185
>> _omap_hwmod_enable+0x34/0x150()
>>> [    0.000000] omap_hwmod: wd_timer2: enabled state can only be entered
>> from initialized, idle, or disabled state
>>> [    0.000000] Modules linked in:
>>> [    0.000000] [<c0050460>] (unwind_backtrace+0x0/0xe4) from
>> [<c0083954>] (warn_slowpath_common+0x4c/0x64)
>>> [    0.000000] [<c0083954>] (warn_slowpath_common+0x4c/0x64) from
>> [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c)
>>> [    0.000000] [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c) from
>> [<c0059be4>] (_omap_hwmod_enable+0x34/0x150)
>>> [    0.000000] [<c0059be4>] (_omap_hwmod_enable+0x34/0x150) from
>> [<c0059d28>] (omap_hwmod_enable+0x28/0x3c)
>>> [    0.000000] [<c0059d28>] (omap_hwmod_enable+0x28/0x3c) from
>> [<c005580c>] (omap2_disable_wdt+0x48/0xdc)
>>> [    0.000000] [<c005580c>] (omap2_disable_wdt+0x48/0xdc) from
>> [<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
>>> [    0.000000] [<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
>> from [<c000f6d0>] (omap2_init_devices+0x44/0x330)
>>> [    0.000000] [<c000f6d0>] (omap2_init_devices+0x44/0x330) from
>> [<c0049578>] (do_one_initcall+0xcc/0x1a4)
>>> [    0.000000] [<c0049578>] (do_one_initcall+0xcc/0x1a4) from
>> [<c0008780>] (kernel_init+0x148/0x210)
>>> [    0.000000] [<c0008780>] (kernel_init+0x148/0x210) from [<c004acb8>]
>> (kernel_thread_exit+0x0/0x8)
>>> [    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.
>>
>> 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.
>
>>
>> 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.
>
> 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

>
>>
>> Regards,
>> Benoit
>>
>>
>>
>>>
>>> Tested on OMAP3430 SDP and OMAP4430 SDP.
>>>
>>> Signed-off-by: Charulatha V<charu@ti.com>
>>> ---
>>>    arch/arm/mach-omap2/devices.c |    6 +++++-
>>>    1 files changed, 5 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-
>> omap2/devices.c
>>> index eaf3799..b592952 100644
>>> --- a/arch/arm/mach-omap2/devices.c
>>> +++ b/arch/arm/mach-omap2/devices.c
>>> @@ -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)
>>>    		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)
>>>    				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)
>>>    	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;
>>>    }
>

Re: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n

[Kevin Hilman]

"Cousson, Benoit" <b-cousson@ti.com> writes:

> Adding more folks to the discussion.
>
> On 10/12/2010 9:30 AM, Varadarajan, Charulatha wrote:
>>
>>> From: Cousson, Benoit
>>> Sent: Tuesday, October 12, 2010 12:45 PM
>>> To: Varadarajan, Charulatha
>>>
>>> Hi Charu,
>>>
>>> On 10/11/2010 2:02 PM, Varadarajan, Charulatha wrote:
>>>> Fix the below warning during boot
>>>>
>>>> [    0.000000] WARNING: at arch/arm/mach-omap2/omap_hwmod.c:1185
>>> _omap_hwmod_enable+0x34/0x150()
>>>> [    0.000000] omap_hwmod: wd_timer2: enabled state can only be entered
>>> from initialized, idle, or disabled state
>>>> [    0.000000] Modules linked in:
>>>> [    0.000000] [<c0050460>] (unwind_backtrace+0x0/0xe4) from
>>> [<c0083954>] (warn_slowpath_common+0x4c/0x64)
>>>> [    0.000000] [<c0083954>] (warn_slowpath_common+0x4c/0x64) from
>>> [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c)
>>>> [    0.000000] [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c) from
>>> [<c0059be4>] (_omap_hwmod_enable+0x34/0x150)
>>>> [    0.000000] [<c0059be4>] (_omap_hwmod_enable+0x34/0x150) from
>>> [<c0059d28>] (omap_hwmod_enable+0x28/0x3c)
>>>> [    0.000000] [<c0059d28>] (omap_hwmod_enable+0x28/0x3c) from
>>> [<c005580c>] (omap2_disable_wdt+0x48/0xdc)
>>>> [    0.000000] [<c005580c>] (omap2_disable_wdt+0x48/0xdc) from
>>> [<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
>>>> [    0.000000] [<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
>>> from [<c000f6d0>] (omap2_init_devices+0x44/0x330)
>>>> [    0.000000] [<c000f6d0>] (omap2_init_devices+0x44/0x330) from
>>> [<c0049578>] (do_one_initcall+0xcc/0x1a4)
>>>> [    0.000000] [<c0049578>] (do_one_initcall+0xcc/0x1a4) from
>>> [<c0008780>] (kernel_init+0x148/0x210)
>>>> [    0.000000] [<c0008780>] (kernel_init+0x148/0x210) from [<c004acb8>]
>>> (kernel_thread_exit+0x0/0x8)
>>>> [    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.
>>>
>>> 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.
>>
>>>
>>> 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.
>>
>> 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?

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

>>
>>>
>>> Regards,
>>> Benoit
>>>
>>>
>>>
>>>>
>>>> Tested on OMAP3430 SDP and OMAP4430 SDP.
>>>>
>>>> Signed-off-by: Charulatha V<charu@ti.com>
>>>> ---
>>>>    arch/arm/mach-omap2/devices.c |    6 +++++-
>>>>    1 files changed, 5 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-
>>> omap2/devices.c
>>>> index eaf3799..b592952 100644
>>>> --- a/arch/arm/mach-omap2/devices.c
>>>> +++ b/arch/arm/mach-omap2/devices.c
>>>> @@ -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)
>>>>    		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)
>>>>    				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)
>>>>    	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;
>>>>    }
>>

RE: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n

[Varadarajan, Charulatha]

Paul, Benoit, 

Please provide your input on this.

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: Tuesday, October 12, 2010 11:57 PM
> To: Cousson, Benoit
> Cc: Varadarajan, Charulatha; Paul Walmsley; linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap2plus: wdt: Fix boot warn when 
> CONFIG_PM_RUNTIME=n
> 
> "Cousson, Benoit" <b-cousson@ti.com> writes:
> 
> > Adding more folks to the discussion.
> >
> > On 10/12/2010 9:30 AM, Varadarajan, Charulatha wrote:
> >>
> >>> From: Cousson, Benoit
> >>> Sent: Tuesday, October 12, 2010 12:45 PM
> >>> To: Varadarajan, Charulatha
> >>>
> >>> Hi Charu,
> >>>
> >>> On 10/11/2010 2:02 PM, Varadarajan, Charulatha wrote:
> >>>> Fix the below warning during boot
> >>>>
> >>>> [    0.000000] WARNING: at arch/arm/mach-omap2/omap_hwmod.c:1185
> >>> _omap_hwmod_enable+0x34/0x150()
> >>>> [    0.000000] omap_hwmod: wd_timer2: enabled state can 
> only be entered
> >>> from initialized, idle, or disabled state
> >>>> [    0.000000] Modules linked in:
> >>>> [    0.000000] [<c0050460>] (unwind_backtrace+0x0/0xe4) from
> >>> [<c0083954>] (warn_slowpath_common+0x4c/0x64)
> >>>> [    0.000000] [<c0083954>] (warn_slowpath_common+0x4c/0x64) from
> >>> [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c)
> >>>> [    0.000000] [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c) from
> >>> [<c0059be4>] (_omap_hwmod_enable+0x34/0x150)
> >>>> [    0.000000] [<c0059be4>] (_omap_hwmod_enable+0x34/0x150) from
> >>> [<c0059d28>] (omap_hwmod_enable+0x28/0x3c)
> >>>> [    0.000000] [<c0059d28>] (omap_hwmod_enable+0x28/0x3c) from
> >>> [<c005580c>] (omap2_disable_wdt+0x48/0xdc)
> >>>> [    0.000000] [<c005580c>] (omap2_disable_wdt+0x48/0xdc) from
> >>> [<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
> >>>> [    0.000000] [<c0058898>] 
> (omap_hwmod_for_each_by_class+0x60/0xa4)
> >>> from [<c000f6d0>] (omap2_init_devices+0x44/0x330)
> >>>> [    0.000000] [<c000f6d0>] (omap2_init_devices+0x44/0x330) from
> >>> [<c0049578>] (do_one_initcall+0xcc/0x1a4)
> >>>> [    0.000000] [<c0049578>] (do_one_initcall+0xcc/0x1a4) from
> >>> [<c0008780>] (kernel_init+0x148/0x210)
> >>>> [    0.000000] [<c0008780>] (kernel_init+0x148/0x210) 
> from [<c004acb8>]
> >>> (kernel_thread_exit+0x0/0x8)
> >>>> [    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.
> >>>
> >>> 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.
> >>
> >>>
> >>> 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.
> >>
> >> 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?
> 
> 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);
> }

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?

> 
> 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
> 
> >>
> >>>
> >>> Regards,
> >>> Benoit
> >>>
> >>>
> >>>
> >>>>
> >>>> Tested on OMAP3430 SDP and OMAP4430 SDP.
> >>>>
> >>>> Signed-off-by: Charulatha V<charu@ti.com>
> >>>> ---
> >>>>    arch/arm/mach-omap2/devices.c |    6 +++++-
> >>>>    1 files changed, 5 insertions(+), 1 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-
> >>> omap2/devices.c
> >>>> index eaf3799..b592952 100644
> >>>> --- a/arch/arm/mach-omap2/devices.c
> >>>> +++ b/arch/arm/mach-omap2/devices.c
> >>>> @@ -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)
> >>>>    		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)
> >>>>    				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)
> >>>>    	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;
> >>>>    }
> >>
> 
> 

RE: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n

[Paul Walmsley]

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

Hello Charu, Kevin, Benoît,

On Thu, 14 Oct 2010, Varadarajan, Charulatha wrote:

> 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 <paul@pwsan.com>
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 <paul@pwsan.com>
---
 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(-)

diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 40562dd..e3a423d 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -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);
+}
+
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 5a30658..618e135 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -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
+	 */
+	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",
+		     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;
 }
@@ -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
+ *
+ */
+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;
+}
diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-omap/include/plat/omap_hwmod.h
index 7eaa8ed..a566efc 100644
--- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
+++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
@@ -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
-- 
1.7.1

RE: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n

[Paul Walmsley]

On Thu, 14 Oct 2010, Paul Walmsley wrote:

> 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

>From de3b800f9a6db027846f68de3aa4202eb713b391 Mon Sep 17 00:00:00 2001
From: Paul Walmsley <paul@pwsan.com>
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(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 618e135..92dd369 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -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);
 
-- 
1.7.1

RE: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n

[Varadarajan, Charulatha]

Paul,

> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com]
> Sent: Friday, October 15, 2010 12:08 AM
> To: Varadarajan, Charulatha
> Cc: Kevin Hilman; Cousson, Benoit; linux-omap@vger.kernel.org
> Subject: RE: [PATCH] omap2plus: wdt: Fix boot warn when
> CONFIG_PM_RUNTIME=n
>
> Hello Charu, Kevin, Benoît,
>
> On Thu, 14 Oct 2010, Varadarajan, Charulatha wrote:
>
> > 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.

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

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

>
> 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 <paul@pwsan.com>
> 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 <paul@pwsan.com>
> ---
>  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(-)
>
> diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
> index 40562dd..e3a423d 100644
> --- a/arch/arm/mach-omap2/io.c
> +++ b/arch/arm/mach-omap2/io.c
> @@ -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);
> +}
> +
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-
> omap2/omap_hwmod.c
> index 5a30658..618e135 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -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
> +      */
> +     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",
> +                  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;
>  }
> @@ -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
> + *
> + */
> +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;
> +}
> diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-
> omap/include/plat/omap_hwmod.h
> index 7eaa8ed..a566efc 100644
> --- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
> +++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
> @@ -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
> --
> 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

Re: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n

[Kevin Hilman]

"Varadarajan, Charulatha" <charu@ti.com> writes:

> Paul, Benoit, 
>
> Please provide your input on this.
>
>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>> Sent: Tuesday, October 12, 2010 11:57 PM
>> To: Cousson, Benoit
>> Cc: Varadarajan, Charulatha; Paul Walmsley; linux-omap@vger.kernel.org
>> Subject: Re: [PATCH] omap2plus: wdt: Fix boot warn when 
>> CONFIG_PM_RUNTIME=n
>> 
>> "Cousson, Benoit" <b-cousson@ti.com> writes:
>> 
>> > Adding more folks to the discussion.
>> >
>> > On 10/12/2010 9:30 AM, Varadarajan, Charulatha wrote:
>> >>
>> >>> From: Cousson, Benoit
>> >>> Sent: Tuesday, October 12, 2010 12:45 PM
>> >>> To: Varadarajan, Charulatha
>> >>>
>> >>> Hi Charu,
>> >>>
>> >>> On 10/11/2010 2:02 PM, Varadarajan, Charulatha wrote:
>> >>>> Fix the below warning during boot
>> >>>>
>> >>>> [    0.000000] WARNING: at arch/arm/mach-omap2/omap_hwmod.c:1185
>> >>> _omap_hwmod_enable+0x34/0x150()
>> >>>> [    0.000000] omap_hwmod: wd_timer2: enabled state can 
>> only be entered
>> >>> from initialized, idle, or disabled state
>> >>>> [    0.000000] Modules linked in:
>> >>>> [    0.000000] [<c0050460>] (unwind_backtrace+0x0/0xe4) from
>> >>> [<c0083954>] (warn_slowpath_common+0x4c/0x64)
>> >>>> [    0.000000] [<c0083954>] (warn_slowpath_common+0x4c/0x64) from
>> >>> [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c)
>> >>>> [    0.000000] [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c) from
>> >>> [<c0059be4>] (_omap_hwmod_enable+0x34/0x150)
>> >>>> [    0.000000] [<c0059be4>] (_omap_hwmod_enable+0x34/0x150) from
>> >>> [<c0059d28>] (omap_hwmod_enable+0x28/0x3c)
>> >>>> [    0.000000] [<c0059d28>] (omap_hwmod_enable+0x28/0x3c) from
>> >>> [<c005580c>] (omap2_disable_wdt+0x48/0xdc)
>> >>>> [    0.000000] [<c005580c>] (omap2_disable_wdt+0x48/0xdc) from
>> >>> [<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
>> >>>> [    0.000000] [<c0058898>] 
>> (omap_hwmod_for_each_by_class+0x60/0xa4)
>> >>> from [<c000f6d0>] (omap2_init_devices+0x44/0x330)
>> >>>> [    0.000000] [<c000f6d0>] (omap2_init_devices+0x44/0x330) from
>> >>> [<c0049578>] (do_one_initcall+0xcc/0x1a4)
>> >>>> [    0.000000] [<c0049578>] (do_one_initcall+0xcc/0x1a4) from
>> >>> [<c0008780>] (kernel_init+0x148/0x210)
>> >>>> [    0.000000] [<c0008780>] (kernel_init+0x148/0x210) 
>> from [<c004acb8>]
>> >>> (kernel_thread_exit+0x0/0x8)
>> >>>> [    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.
>> >>>
>> >>> 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.
>> >>
>> >>>
>> >>> 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.
>> >>
>> >> 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?
>> 
>> 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);
>> }
>
> 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

RE: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n

[Varadarajan, Charulatha]

 

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: Wednesday, October 20, 2010 5:31 AM
> To: Varadarajan, Charulatha
> Cc: Cousson, Benoit; Paul Walmsley; linux-omap@vger.kernel.org
> Subject: Re: [PATCH] omap2plus: wdt: Fix boot warn when 
> CONFIG_PM_RUNTIME=n
> 
> "Varadarajan, Charulatha" <charu@ti.com> writes:
> 
> > Paul, Benoit, 
> >
> > Please provide your input on this.
> >
> >> -----Original Message-----
> >> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> >> Sent: Tuesday, October 12, 2010 11:57 PM
> >> To: Cousson, Benoit
> >> Cc: Varadarajan, Charulatha; Paul Walmsley; 
> linux-omap@vger.kernel.org
> >> Subject: Re: [PATCH] omap2plus: wdt: Fix boot warn when 
> >> CONFIG_PM_RUNTIME=n
> >> 
> >> "Cousson, Benoit" <b-cousson@ti.com> writes:
> >> 
> >> > Adding more folks to the discussion.
> >> >
> >> > On 10/12/2010 9:30 AM, Varadarajan, Charulatha wrote:
> >> >>
> >> >>> From: Cousson, Benoit
> >> >>> Sent: Tuesday, October 12, 2010 12:45 PM
> >> >>> To: Varadarajan, Charulatha
> >> >>>
> >> >>> Hi Charu,
> >> >>>
> >> >>> On 10/11/2010 2:02 PM, Varadarajan, Charulatha wrote:
> >> >>>> Fix the below warning during boot
> >> >>>>
> >> >>>> [    0.000000] WARNING: at 
> arch/arm/mach-omap2/omap_hwmod.c:1185
> >> >>> _omap_hwmod_enable+0x34/0x150()
> >> >>>> [    0.000000] omap_hwmod: wd_timer2: enabled state can 
> >> only be entered
> >> >>> from initialized, idle, or disabled state
> >> >>>> [    0.000000] Modules linked in:
> >> >>>> [    0.000000] [<c0050460>] (unwind_backtrace+0x0/0xe4) from
> >> >>> [<c0083954>] (warn_slowpath_common+0x4c/0x64)
> >> >>>> [    0.000000] [<c0083954>] 
> (warn_slowpath_common+0x4c/0x64) from
> >> >>> [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c)
> >> >>>> [    0.000000] [<c00839ec>] (warn_slowpath_fmt+0x2c/0x3c) from
> >> >>> [<c0059be4>] (_omap_hwmod_enable+0x34/0x150)
> >> >>>> [    0.000000] [<c0059be4>] 
> (_omap_hwmod_enable+0x34/0x150) from
> >> >>> [<c0059d28>] (omap_hwmod_enable+0x28/0x3c)
> >> >>>> [    0.000000] [<c0059d28>] (omap_hwmod_enable+0x28/0x3c) from
> >> >>> [<c005580c>] (omap2_disable_wdt+0x48/0xdc)
> >> >>>> [    0.000000] [<c005580c>] (omap2_disable_wdt+0x48/0xdc) from
> >> >>> [<c0058898>] (omap_hwmod_for_each_by_class+0x60/0xa4)
> >> >>>> [    0.000000] [<c0058898>] 
> >> (omap_hwmod_for_each_by_class+0x60/0xa4)
> >> >>> from [<c000f6d0>] (omap2_init_devices+0x44/0x330)
> >> >>>> [    0.000000] [<c000f6d0>] 
> (omap2_init_devices+0x44/0x330) from
> >> >>> [<c0049578>] (do_one_initcall+0xcc/0x1a4)
> >> >>>> [    0.000000] [<c0049578>] (do_one_initcall+0xcc/0x1a4) from
> >> >>> [<c0008780>] (kernel_init+0x148/0x210)
> >> >>>> [    0.000000] [<c0008780>] (kernel_init+0x148/0x210) 
> >> from [<c004acb8>]
> >> >>> (kernel_thread_exit+0x0/0x8)
> >> >>>> [    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.
> >> >>>
> >> >>> 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.
> >> >>
> >> >>>
> >> >>> 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.
> >> >>
> >> >> 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?
> >> 
> >> 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.


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