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? > > 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@xxxxxx> > >>>> --- > >>>> 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; > >>>> } > >> > > -- 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
