Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932668AbbBBRq5 (ORCPT ); Mon, 2 Feb 2015 12:46:57 -0500 Received: from mail-ig0-f179.google.com ([209.85.213.179]:38681 "EHLO mail-ig0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753579AbbBBRqv convert rfc822-to-8bit (ORCPT ); Mon, 2 Feb 2015 12:46:51 -0500 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT To: Tony Lindgren , "Geert Uytterhoeven" From: Mike Turquette In-Reply-To: <20150202161237.GG16250@atomide.com> Cc: "Tomeu Vizoso" , "Stephen Boyd" , "Linux MIPS Mailing List" , "linux-doc@vger.kernel.org" , "Chao Xie" , "Haojian Zhuang" , "Boris Brezillon" , "Russell King" , "Jonathan Corbet" , "Emilio L??pez" , "Linux-sh list" , "Alex Elder" , "Zhangfei Gao" , "Bintian Wang" , "Matt Porter" , "linux-omap@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "Ralf Baechle" , "Tero Kristo" , "Manuel Lauss" , "Maxime Ripard" , "Javier Martinez Canillas" References: <1422011024-32283-1-git-send-email-tomeu.vizoso@collabora.com> <1422011024-32283-5-git-send-email-tomeu.vizoso@collabora.com> <54CA8662.7040008@codeaurora.org> <20150131013158.GA4323@codeaurora.org> <20150201221856.421.6151@quantum> <20150202161237.GG16250@atomide.com> Message-ID: <20150202174646.421.52331@quantum> User-Agent: alot/0.3.5 Subject: Re: [PATCH v13 4/6] clk: Add rate constraints to clocks Date: Mon, 02 Feb 2015 09:46:46 -0800 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7203 Lines: 164 Quoting Tony Lindgren (2015-02-02 08:12:37) > * Geert Uytterhoeven [150202 00:03]: > > On Sun, Feb 1, 2015 at 11:18 PM, Mike Turquette wrote: > > > Quoting Tomeu Vizoso (2015-01-31 10:36:22) > > >> On 31 January 2015 at 02:31, Stephen Boyd wrote: > > >> > On 01/29, Stephen Boyd wrote: > > >> >> On 01/29/15 05:31, Geert Uytterhoeven wrote: > > >> >> > Hi Tomeu, Mike, > > >> >> > > > >> >> > On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso > > >> >> > wrote: > > >> >> >> --- a/drivers/clk/clk.c > > >> >> >> +++ b/drivers/clk/clk.c > > >> >> >> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk) > > >> >> >> return 1; > > >> >> >> } > > >> >> >> > > >> >> >> -static void clk_core_put(struct clk_core *core) > > >> >> >> +void __clk_put(struct clk *clk) > > >> >> >> { > > >> >> >> struct module *owner; > > >> >> >> > > >> >> >> - owner = core->owner; > > >> >> >> + if (!clk || WARN_ON_ONCE(IS_ERR(clk))) > > >> >> >> + return; > > >> >> >> > > >> >> >> clk_prepare_lock(); > > >> >> >> - kref_put(&core->ref, __clk_release); > > >> >> >> + > > >> >> >> + hlist_del(&clk->child_node); > > >> >> >> + clk_core_set_rate_nolock(clk->core, clk->core->req_rate); > > >> >> > At this point, clk->core->req_rate is still zero, causing > > >> >> > cpg_div6_clock_round_rate() to be called with a zero "rate" parameter, > > >> >> > e.g. on r8a7791: > > >> >> > > >> >> Hmm.. I wonder if we should assign core->req_rate to be the same as > > >> >> core->rate during __clk_init()? That would make this call to > > >> >> clk_core_set_rate_nolock() a nop in this case. > > >> >> > > >> > > > >> > Here's a patch to do this > > >> > > > >> > ---8<---- > > >> > From: Stephen Boyd > > >> > Subject: [PATCH] clk: Assign a requested rate by default > > >> > > > >> > We need to assign a requested rate here so that we avoid > > >> > requesting a rate of 0 on clocks when we remove clock consumers. > > >> > > >> Hi, this looks good to me. > > >> > > >> Reviewed-by: Tomeu Vizoso > > > > > > It seems to fix the total boot failure on OMAPs, and hopefully does the > > > same for SH Mobile and others. I've squashed this into Tomeu's rate > > > constraints patch to maintain bisect. > > > > Yes, it fixes shmobile. .round_rate() is now called with a sane value of rate. > > Looks like next-20150202 now produces tons of the following errors, > these from omap4: next-20150202 is the rolled-back changes from last Friday. I removed the clock constraints patch and in doing so also rolled back the TI clock driver migration and clk-private.h removal patches. Those are all back in clk-next as of last night and it looks as though they missed being pulled into todays linux-next by a matter of minutes :-/ > > [ 10.568206] ------------[ cut here ]------------ > [ 10.568206] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:925 clk_disable+0x28/0x34() > [ 10.568237] Modules linked in: > [ 10.568237] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 3.19.0-rc6-next-20150202 #2037 > [ 10.568237] Hardware name: Generic OMAP4 (Flattened Device Tree) > [ 10.568267] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [ 10.568267] [] (show_stack) from [] (dump_stack+0x84/0x9c) > [ 10.568267] [] (dump_stack) from [] (warn_slowpath_common+0x7c/0xb8) > [ 10.568298] [] (warn_slowpath_common) from [] (warn_slowpath_null+0x1c/0x24) > [ 10.568298] [] (warn_slowpath_null) from [] (clk_disable+0x28/0x34) > [ 10.568328] [] (clk_disable) from [] (_disable_clocks+0x18/0x68) > [ 10.568328] [] (_disable_clocks) from [] (_idle+0x10c/0x214) > [ 10.568328] [] (_idle) from [] (_setup+0x338/0x410) > [ 10.568359] [] (_setup) from [] (omap_hwmod_for_each+0x34/0x60) > [ 10.568359] [] (omap_hwmod_for_each) from [] (__omap_hwmod_setup_all+0x30/0x40) > [ 10.568389] [] (__omap_hwmod_setup_all) from [] (do_one_initcall+0x80/0x1dc) > [ 10.568389] [] (do_one_initcall) from [] (kernel_init_freeable+0x204/0x2d0) > [ 10.568420] [] (kernel_init_freeable) from [] (kernel_init+0x8/0xec) > [ 10.568420] [] (kernel_init) from [] (ret_from_fork+0x14/0x24) > [ 10.568420] ---[ end trace cb88537fdc8fa211 ]--- This looks like mis-matched enable/disable calls. We now have unique struct clk pointers for every call to clk_get. I haven't yet looked through the hwmod code but I have a feeling that we're doing something like this: /* enable clock */ my_clk = clk_get(...); clk_prepare_enable(my_clk); clk_put(my_clk); /* do some work */ do_work(); /* disable clock */ my_clk = clk_get(...); clk_disable_unprepare(my_clk); clk_put(my_clk); The above pattern no longer works since my_clk will be two different unique pointers, but it really should be one stable pointer across the whole usage of the clk. E.g: /* enable clock */ my_clk = clk_get(...); clk_prepare_enable(my_clk); /* do some work */ do_work(); /* disable clock */ clk_disable_unprepare(my_clk); clk_put(my_clk); Again, I haven't looked through the code, so the above is just an educated guess. Anyways I am testing with an OMAP4460 Panda ES and I didn't see the above. Is there a test you are running to get this? > > > [ 10.568450] ------------[ cut here ]------------ > [ 10.568450] WARNING: CPU: 0 PID: 1 at arch/arm/mach-omap2/dpll3xxx.c:436 omap3_noncore_dpll_enable+0xdc/0 > x10c() > [ 10.568450] Modules linked in: > [ 10.568481] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 3.19.0-rc6-next-20150202 #2037 > [ 10.568481] Hardware name: Generic OMAP4 (Flattened Device Tree) > [ 10.568481] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [ 10.568511] [] (show_stack) from [] (dump_stack+0x84/0x9c) > [ 10.568511] [] (dump_stack) from [] (warn_slowpath_common+0x7c/0xb8) > [ 10.568511] [] (warn_slowpath_common) from [] (warn_slowpath_null+0x1c/0x24) > [ 10.568542] [] (warn_slowpath_null) from [] (omap3_noncore_dpll_enable+0xdc/0x10c) > [ 10.568542] [] (omap3_noncore_dpll_enable) from [] (clk_core_enable+0x60/0x9c) > [ 10.568572] [] (clk_core_enable) from [] (clk_core_enable+0x40/0x9c) > [ 10.568572] ---[ end trace cb88537fdc8fa212 ]--- > ... This is the same issue discussed already in this thread[0]. Feedback from Tero & Paul on how to handle it would be nice. Please let me know if anything else breaks for you. Regards, Mike > > Regards, > > Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/