"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); >> } > > 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
