From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: Re: [PATCH] omap2plus: wdt: Fix boot warn when CONFIG_PM_RUNTIME=n Date: Tue, 12 Oct 2010 11:27:20 -0700 Message-ID: <87mxqj1d2v.fsf@deeprootsystems.com> References: <1286798556-22749-1-git-send-email-charu@ti.com> <4CB40B07.8080301@ti.com> <4CB45E0E.6080104@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail-iw0-f174.google.com ([209.85.214.174]:46453 "EHLO mail-iw0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754267Ab0JLS1Y (ORCPT ); Tue, 12 Oct 2010 14:27:24 -0400 Received: by iwn7 with SMTP id 7so1056187iwn.19 for ; Tue, 12 Oct 2010 11:27:23 -0700 (PDT) In-Reply-To: <4CB45E0E.6080104@ti.com> (Benoit Cousson's message of "Tue, 12 Oct 2010 15:09:34 +0200") Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: "Cousson, Benoit" Cc: "Varadarajan, Charulatha" , Paul Walmsley , "linux-omap@vger.kernel.org" "Cousson, Benoit" 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] [] (unwind_backtrace+0x0/0xe4) from >>> [] (warn_slowpath_common+0x4c/0x64) >>>> [ 0.000000] [] (warn_slowpath_common+0x4c/0x64) from >>> [] (warn_slowpath_fmt+0x2c/0x3c) >>>> [ 0.000000] [] (warn_slowpath_fmt+0x2c/0x3c) from >>> [] (_omap_hwmod_enable+0x34/0x150) >>>> [ 0.000000] [] (_omap_hwmod_enable+0x34/0x150) from >>> [] (omap_hwmod_enable+0x28/0x3c) >>>> [ 0.000000] [] (omap_hwmod_enable+0x28/0x3c) from >>> [] (omap2_disable_wdt+0x48/0xdc) >>>> [ 0.000000] [] (omap2_disable_wdt+0x48/0xdc) from >>> [] (omap_hwmod_for_each_by_class+0x60/0xa4) >>>> [ 0.000000] [] (omap_hwmod_for_each_by_class+0x60/0xa4) >>> from [] (omap2_init_devices+0x44/0x330) >>>> [ 0.000000] [] (omap2_init_devices+0x44/0x330) from >>> [] (do_one_initcall+0xcc/0x1a4) >>>> [ 0.000000] [] (do_one_initcall+0xcc/0x1a4) from >>> [] (kernel_init+0x148/0x210) >>>> [ 0.000000] [] (kernel_init+0x148/0x210) from [] >>> (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 >>>> --- >>>> 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; >>>> } >>