From patchwork Tue Mar 6 19:00:39 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Sascha Hauer X-Patchwork-Id: 7117 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id E544323E80 for ; Tue, 6 Mar 2012 19:01:14 +0000 (UTC) Received: from mail-iy0-f180.google.com (mail-iy0-f180.google.com [209.85.210.180]) by fiordland.canonical.com (Postfix) with ESMTP id 86426A1827E for ; Tue, 6 Mar 2012 19:01:14 +0000 (UTC) Received: by iage36 with SMTP id e36so9991638iag.11 for ; Tue, 06 Mar 2012 11:01:14 -0800 (PST) Received: from mr.google.com ([10.42.147.199]) by 10.42.147.199 with SMTP id o7mr17095932icv.50.1331060473943 (num_hops = 1); Tue, 06 Mar 2012 11:01:13 -0800 (PST) Received: by 10.42.147.199 with SMTP id o7mr14114307icv.50.1331060473826; Tue, 06 Mar 2012 11:01:13 -0800 (PST) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.231.53.18 with SMTP id k18csp65863ibg; Tue, 6 Mar 2012 11:01:12 -0800 (PST) Received: by 10.213.29.8 with SMTP id o8mr3456541ebc.84.1331060470970; Tue, 06 Mar 2012 11:01:10 -0800 (PST) Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de. [92.198.50.35]) by mx.google.com with ESMTPS id a7si16969101eeg.109.2012.03.06.11.01.09 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 06 Mar 2012 11:01:10 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of sha@pengutronix.de designates 92.198.50.35 as permitted sender) client-ip=92.198.50.35; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of sha@pengutronix.de designates 92.198.50.35 as permitted sender) smtp.mail=sha@pengutronix.de Received: from dude.hi.pengutronix.de ([2001:6f8:1178:2:21e:67ff:fe11:9c5c]) by metis.ext.pengutronix.de with esmtp (Exim 4.72) (envelope-from ) id 1S4zct-0006YH-1C; Tue, 06 Mar 2012 20:00:47 +0100 Received: from sha by dude.hi.pengutronix.de with local (Exim 4.77) (envelope-from ) id 1S4zcl-0007lC-66; Tue, 06 Mar 2012 20:00:39 +0100 Date: Tue, 6 Mar 2012 20:00:39 +0100 From: Sascha Hauer To: "Turquette, Mike" Cc: Andrew Lunn , Paul Walmsley , linaro-dev@lists.linaro.org, Linus Walleij , patches@linaro.org, Stephen Boyd , Mark Brown , Magnus Damm , linux-kernel@vger.kernel.org, Rob Herring , Richard Zhao , Grant Likely , Deepak Saxena , Saravana Kannan , Thomas Gleixner , Shawn Guo , Amit Kucheria , Russell King , Jeremy Kerr , Arnd Bergman , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v5 3/4] clk: introduce the common clock framework Message-ID: <20120306190039.GJ3852@pengutronix.de> References: <1330763341-3437-1-git-send-email-mturquette@linaro.org> <1330763341-3437-4-git-send-email-mturquette@linaro.org> <20120303133158.GA26882@pengutronix.de> <20120304115201.GB26882@pengutronix.de> <20120305073836.GU3852@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 18:09:37 up 115 days, 56 min, 66 users, load average: 0.00, 0.01, 0.05 User-Agent: Mutt/1.5.21 (2010-09-15) X-SA-Exim-Connect-IP: 2001:6f8:1178:2:21e:67ff:fe11:9c5c X-SA-Exim-Mail-From: sha@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: patches@linaro.org X-Gm-Message-State: ALoCoQmdpcP3UOY7/wkEfHs+VePhs3jMVdQjSeifVusyUztrNDN+PLC6ivaose2pUK+RnzJVuFlJ On Mon, Mar 05, 2012 at 12:03:15PM -0800, Turquette, Mike wrote: > On Sun, Mar 4, 2012 at 11:38 PM, Sascha Hauer wrote: > > On Sun, Mar 04, 2012 at 04:12:21PM -0800, Turquette, Mike wrote: > >> >> > >> >> I believe this patch already does what you suggest, but I might be > >> >> missing your point. > >> > > >> > In include/linux/clk-private.h you expose struct clk outside the core. > >> > This has to be done to make static initializers possible. There is a big > >> > warning in this file that it must not be included from files implementing > >> > struct clk_ops. You can simply avoid this warning by declaring struct clk > >> > with only a single member: > >> > > >> > include/linux/clk.h: > >> > > >> > struct clk { > >> >        struct clk_internal *internal; > >> > }; > >> > > >> > This way everybody knows struct clk (thus can embed it in their static > >> > initializers), but doesn't know anything about the internal members. Now > >> > in drivers/clk/clk.c you declare struct clk_internal exactly like struct > >> > clk was declared before: > >> > > >> > struct clk_internal { > >> >        const char              *name; > >> >        const struct clk_ops    *ops; > >> >        struct clk_hw           *hw; > >> >        struct clk              *parent; > >> >        char                    **parent_names; > >> >        struct clk              **parents; > >> >        u8                      num_parents; > >> >        unsigned long           rate; > >> >        unsigned long           flags; > >> >        unsigned int            enable_count; > >> >        unsigned int            prepare_count; > >> >        struct hlist_head       children; > >> >        struct hlist_node       child_node; > >> >        unsigned int            notifier_count; > >> > #ifdef CONFIG_COMMON_CLK_DEBUG > >> >        struct dentry           *dentry; > >> > #endif > >> > }; > >> > > >> > An instance of struct clk_internal will be allocated in > >> > __clk_init/clk_register. Now the private data stays completely inside > >> > the core and noone can abuse it. > >> > >> Hi Sascha, > >> > >> I see the disconnect here.  For OMAP (and possibly other platforms) at > >> least some clock data is necessary during early boot, before the > >> regular allocation methods are available (timers for instance). > > > > We had this problem on i.MX aswell. It turned out that the timer clock > > is the only clock that is needed so early. We solved this by moving the > > clock init to the system timer init function. > > When you say "mov[ed] the clock init to the system timer init > function" do you mean that you statically allocated struct clk and > used the clk framework api, or instead you just did some direct > register writes to initialize things properly? I meant that on i.MX we do the clock tree initialization when kmalloc is available, see the attached patch for omap4 based on your branch which does the same for Omap. The first clock you need is the one for the timer, so you can initialize the clocktree at the beginning of time_init() and don't need statically initialized clocks anymore. > > > > Well, the file is work in progress, you probably fix this before sending > > it out, but I bet people will include clk-private.h and nobody else > > notices it. > > clock44xx_data.c does not violate that rule. None of the logic that > implements ops for those clocks is present clock44xx_data.c. Indeed, I missed that. It only has the ops but not the individual functions. > All of > the code in that file is simply initialization and registration of > OMAP4 clocks. Many of the clocks are basic clock types (divider, > multiplexer and fixed-rate are used in that file) with protected code > drivers/clk/clk-*.c and the remaining clocks are of the struct > clk_hw_omap variety, which has code spread across several files: > > arch/arm/mach-omap2/clock.c > arch/arm/mach-omap2/clock.h > arch/arm/mach-omap2/clkt_dpll.c > arch/arm/mach-omap2/clkt_clksel.c > arch/arm/mach-omap2/dpll3xxx.c > arch/arm/mach-omap2/dpll4xxx.c > > All of the above files include linux/clk-provider.h, not > linux/clk-private.h. That code makes heavy use of the > __clk_get_whatever helpers and shows how a platform might honor the > layer of separation between struct clk and stuct clk_ops/struct > clk_foo. You are correct that the code is a work-in-progress, but > there are no layering violations that I can see. > > I also think we are talking past each other to some degree. One point > I would like to make (and maybe you already know this from code > review) is that it is unnecessary to have pointers to your parent > struct clk*'s when either initializing or registering your clocks. In > fact the existing clk_register_foo functions don't even allow you to > pass in parent pointers and rely wholly on string name matching. I > just wanted to point that out in case it went unnoticed, as it is a > new way of doing things from the previous series and was born out of > Thomas' review of V4 and multi-parent handling. This also keeps > device-tree in mind where we might not know the struct clk *pointer at > compile time for "connecting" discrete devices. Yes, I've seen this and I really like it. Also the change that multiplexers return an index to an array instead of the parent clock is very nice. Sascha 8<----------------------------------------------------- ARM omap4: move clocktree init to timer init time so we don't need static clocks Signed-off-by: Sascha Hauer --- arch/arm/mach-omap2/clock44xx_data.c | 4 +++- arch/arm/mach-omap2/io.c | 1 - arch/arm/mach-omap2/timer.c | 3 +++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c index 7f833a7..5aa8dd2 100644 --- a/arch/arm/mach-omap2/clock44xx_data.c +++ b/arch/arm/mach-omap2/clock44xx_data.c @@ -6032,7 +6032,9 @@ int __init omap4xxx_clk_init(void) } # else clkdev_add(c); - __clk_init(NULL, c->clk); + + clk_register(NULL, c->clk->name, c->clk->ops, c->clk->hw, + c->clk->parent_names, c->clk->num_parents, c->clk->flags); #endif } diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c index eb50c29..8db4380 100644 --- a/arch/arm/mach-omap2/io.c +++ b/arch/arm/mach-omap2/io.c @@ -476,7 +476,6 @@ void __init omap4430_init_early(void) omap44xx_clockdomains_init(); omap44xx_hwmod_init(); omap_hwmod_init_postsetup(); - omap4xxx_clk_init(); } #endif diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index 5c9acea..2cca796 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -323,9 +323,12 @@ OMAP_SYS_TIMER_INIT(3_secure, OMAP3_SECURE_TIMER, OMAP3_CLKEV_SOURCE, OMAP_SYS_TIMER(3_secure) #endif +int __init omap4xxx_clk_init(void); + #ifdef CONFIG_ARCH_OMAP4 static void __init omap4_timer_init(void) { + omap4xxx_clk_init(); #ifdef CONFIG_LOCAL_TIMERS twd_base = ioremap(OMAP44XX_LOCAL_TWD_BASE, SZ_256); BUG_ON(!twd_base);