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

[Kevin Hilman <khilman@deeprootsystems.com>]

"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