> -----Original Message----- > From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] > Sent: Wednesday, October 20, 2010 5:31 AM > To: Varadarajan, Charulatha > Cc: Cousson, Benoit; Paul Walmsley; linux-omap@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] omap2plus: wdt: Fix boot warn when > CONFIG_PM_RUNTIME=n > > "Varadarajan, Charulatha" <charu@xxxxxx> writes: > > > Paul, Benoit, > > > > Please provide your input on this. > > > >> -----Original Message----- > >> From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] > >> Sent: Tuesday, October 12, 2010 11:57 PM > >> To: Cousson, Benoit > >> Cc: Varadarajan, Charulatha; Paul Walmsley; > linux-omap@xxxxxxxxxxxxxxx > >> Subject: Re: [PATCH] omap2plus: wdt: Fix boot warn when > >> CONFIG_PM_RUNTIME=n > >> > >> "Cousson, Benoit" <b-cousson@xxxxxx> 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
