| Message ID | 20120306190039.GJ3852@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
On Tue, Mar 6, 2012 at 11:00 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Mon, Mar 05, 2012 at 12:03:15PM -0800, Turquette, Mike wrote: >> On Sun, Mar 4, 2012 at 11:38 PM, Sascha Hauer <s.hauer@pengutronix.de> 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 <s.hauer@pengutronix.de> Hi Sascha, Thanks for the example code. This is in fact something I had considered doing before, but it is a bit complicated for my platform. We set up all of the OMAP clocks with __clk_init because this is a dependency of OMAP's hwmod code which models and interacts with specific resources for hardware modules, such as clocks. There are other dependencies such as our clockdomain code, which needs the clocks to be fully initialized. Some aspects of these layers get init'd before we can allocate memory. Admittedly I think that the OMAP code could migrate some of these bits to a lazy-registration model, specifically the hwmod object instances, but that requires an awful lot of refactoring for a fairly large stack of platform code. This might be something to achieve in the future but for now we *need* initialisation to be fully static. Assuming that some day OMAP code can be refactored to allow for lazy (or at least initcall-based) registration of clocks then perhaps your suggestion can take root. Which leads me to this question: are there any other platforms out there that require the level of expose to struct clk present in this patchset? OMAP does, for now, but if that changes then I need to know if others require this as well. Regards, Mike > --- > 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); > -- > 1.7.9.1 > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> Assuming that some day OMAP code can be refactored to allow for lazy > (or at least initcall-based) registration of clocks then perhaps your > suggestion can take root. Which leads me to this question: are there > any other platforms out there that require the level of expose to > struct clk present in this patchset? OMAP does, for now, but if that > changes then I need to know if others require this as well. Hi Mike For kirkwood, i use static clk's for all but my root clock. I cannot statically know the rate of the root clock, so i have to determine it at boot time using heuristics, PCI ID, etc. I used statics thinking it would be less code. No idea if it actually is, and there is nothing stopping me moving to creating the clocks after creating the root clock. One comment i have about the current static clks. I completely missing you need to call __clk_init() on them and so ended up with lots of division by zero errors, since they did not have a parent, and so the code seemed not be to able to determine the rate. So 1) Please add __clk_init() to the documentation in the section about static clocks. 2) Should maybe the name change? It seems strange having to call a __X() function. If this is a function which is supposed to be used, drop the __. Maybe clk_static_register()? 3) Maybe, if the parent is missing, clk_get_rate() should return an error? Andrew
On Thu, Mar 08, 2012 at 07:27:39AM +0100, Andrew Lunn wrote: > > Assuming that some day OMAP code can be refactored to allow for lazy > > (or at least initcall-based) registration of clocks then perhaps your > > suggestion can take root. Which leads me to this question: are there > > any other platforms out there that require the level of expose to > > struct clk present in this patchset? OMAP does, for now, but if that > > changes then I need to know if others require this as well. > > Hi Mike > > For kirkwood, i use static clk's for all but my root clock. I cannot > statically know the rate of the root clock, so i have to determine it > at boot time using heuristics, PCI ID, etc. > > I used statics thinking it would be less code. No idea if it actually > is, and there is nothing stopping me moving to creating the clocks > after creating the root clock. I'd say use the nonstatic ones. I think using the static initializers will cause us much pain in the future. I've been through several rebases on the i.MX clock rework and everytime I wish my sed foo would be better. Now imagine what happens when it turns out that the internal struct clk layout or the structs for the muxes/dividers/gates have to be changed. This task is next to impossible when we have thousands of clocks scattered around the tree. Sascha
On Wed, 7 Mar 2012, Turquette, Mike wrote: > Assuming that some day OMAP code can be refactored to allow for lazy > (or at least initcall-based) registration of clocks then perhaps your > suggestion can take root. Which leads me to this question: are there > any other platforms out there that require the level of expose to > struct clk present in this patchset? OMAP does, for now, but if that > changes then I need to know if others require this as well. I can't see the problem, really. Other than existing code doing stuff before the memory allocator is up and running. We allocate interrupt data structures in the early boot process today and I don't see a reason why you want clocks, which have not been configured by the boot loader, accesible before that point. Thanks, tglx
> I'd say use the nonstatic ones. I think using the static initializers > will cause us much pain in the future. I've been through several rebases > on the i.MX clock rework and everytime I wish my sed foo would be > better. Now imagine what happens when it turns out that the internal > struct clk layout or the structs for the muxes/dividers/gates have to > be changed. /***************************************************************************** * CLK tree ****************************************************************************/ static DEFINE_SPINLOCK(gating_lock); #define DEFINE_KIRKWOOD_CLK_GATE(_name, _bit) \ DEFINE_CLK_GATE(_name, "tclk", NULL, 0, \ (void __iomem *)CLOCK_GATING_CTRL, \ _bit, 0, &gating_lock) DEFINE_KIRKWOOD_CLK_GATE(clk_ge0, CGC_BIT_GE0); DEFINE_KIRKWOOD_CLK_GATE(clk_pex0, CGC_BIT_PEX0); DEFINE_KIRKWOOD_CLK_GATE(clk_usb0, CGC_BIT_USB0); DEFINE_KIRKWOOD_CLK_GATE(clk_sdio, CGC_BIT_SDIO); DEFINE_KIRKWOOD_CLK_GATE(clk_tsu, CGC_BIT_TSU); DEFINE_KIRKWOOD_CLK_GATE(clk_dunit, CGC_BIT_DUNIT); DEFINE_KIRKWOOD_CLK_GATE(clk_runit, CGC_BIT_RUNIT); I've so far not had any problems, and not needed an sed foo. I do only have a dozen or so clocks, which helps. But even so, all the real pain is hidden inside DEFINE_CLK_GATE() which Mike maintains. I guess the problem comes when you are not using the basic clk providers, but your own provider. What might help is if linux/clk-provider.h could provide some macros to do most of the generic definitions. Something like: #define DEFINE_CLK_GENERIC(_name, _flags, _ops) \ static struct clk _name; \ static char *_name##_parent_names[] = {}; \ static struct clk _name = { \ .name = #_name, \ .ops = &_ops, \ .hw = &_name##_hw.hw, \ .parent_names = _name##_parent_names, \ .num_parents = \ ARRAY_SIZE(_name##_parent_names), \ .flags = _flags, \ }; and then you have something like #define DEFINE_CLK_IMX(_name, _flags, _foo, _bar) \ static struct clk_imx _name##_hw = { \ .hw = { \ .clk = &_name, \ }, \ .foo = _foo, \ .bar = _bar, \ }; \ DEFINE_CLK_GENERIC(_name, _flags, clk_imx_ops) Andrew
On Wed, Mar 7, 2012 at 10:27 PM, Andrew Lunn <andrew@lunn.ch> wrote: >> Assuming that some day OMAP code can be refactored to allow for lazy >> (or at least initcall-based) registration of clocks then perhaps your >> suggestion can take root. Which leads me to this question: are there >> any other platforms out there that require the level of expose to >> struct clk present in this patchset? OMAP does, for now, but if that >> changes then I need to know if others require this as well. > > Hi Mike > > For kirkwood, i use static clk's for all but my root clock. I cannot > statically know the rate of the root clock, so i have to determine it > at boot time using heuristics, PCI ID, etc. > > I used statics thinking it would be less code. No idea if it actually > is, and there is nothing stopping me moving to creating the clocks > after creating the root clock. > > One comment i have about the current static clks. I completely missing > you need to call __clk_init() on them and so ended up with lots of > division by zero errors, since they did not have a parent, and so the > code seemed not be to able to determine the rate. > > So > > 1) Please add __clk_init() to the documentation in the section about > static clocks. Done. > > 2) Should maybe the name change? It seems strange having to call a > __X() function. If this is a function which is supposed to be used, > drop the __. Maybe clk_static_register()? That function is used internally by clk_register and is only exposed by clk-private.h, so I think the naming scheme is sane. I basically want to create a sense of worry in anyone using clk-private.h :-) > > 3) Maybe, if the parent is missing, clk_get_rate() should return an > error? Non-root, non-fixed-rate, orphan clocks can return an error in this case. Will update for V6. Any idea on best -EERROR? I'm thinking ENODEV, but ECHILD and EPIPE are kinda funny in this context. Regards, Mike > > Andrew
On Thu, Mar 8, 2012 at 11:57 PM, Andrew Lunn <andrew@lunn.ch> wrote: >> I'd say use the nonstatic ones. I think using the static initializers >> will cause us much pain in the future. I've been through several rebases >> on the i.MX clock rework and everytime I wish my sed foo would be >> better. Now imagine what happens when it turns out that the internal >> struct clk layout or the structs for the muxes/dividers/gates have to >> be changed. > > /***************************************************************************** > * CLK tree > ****************************************************************************/ > static DEFINE_SPINLOCK(gating_lock); > > #define DEFINE_KIRKWOOD_CLK_GATE(_name, _bit) \ > DEFINE_CLK_GATE(_name, "tclk", NULL, 0, \ > (void __iomem *)CLOCK_GATING_CTRL, \ > _bit, 0, &gating_lock) > > DEFINE_KIRKWOOD_CLK_GATE(clk_ge0, CGC_BIT_GE0); > DEFINE_KIRKWOOD_CLK_GATE(clk_pex0, CGC_BIT_PEX0); > DEFINE_KIRKWOOD_CLK_GATE(clk_usb0, CGC_BIT_USB0); > DEFINE_KIRKWOOD_CLK_GATE(clk_sdio, CGC_BIT_SDIO); > DEFINE_KIRKWOOD_CLK_GATE(clk_tsu, CGC_BIT_TSU); > DEFINE_KIRKWOOD_CLK_GATE(clk_dunit, CGC_BIT_DUNIT); > DEFINE_KIRKWOOD_CLK_GATE(clk_runit, CGC_BIT_RUNIT); > > I've so far not had any problems, and not needed an sed foo. I do > only have a dozen or so clocks, which helps. But even so, all the real > pain is hidden inside DEFINE_CLK_GATE() which Mike maintains. It's true that if the argument list for the macros doesn't change then the pain of static initialization is hidden from the platform data. However if you have the ability to use the clk_foo_register functions please do use them in place of static initialization. The static init stuff is only for folks backed into a corner and forced to use it... for now. I'm looking at ways to allow for kmalloc'ing in early boot, as well as reducing the number of clocks that my platform registers during early boot drastically. > > I guess the problem comes when you are not using the basic clk > providers, but your own provider. What might help is if > linux/clk-provider.h could provide some macros to do most of the > generic definitions. Something like: I'd rather people just used the registration functions instead. Thanks, Mike > > #define DEFINE_CLK_GENERIC(_name, _flags, _ops) \ > static struct clk _name; \ > static char *_name##_parent_names[] = {}; \ > static struct clk _name = { \ > .name = #_name, \ > .ops = &_ops, \ > .hw = &_name##_hw.hw, \ > .parent_names = _name##_parent_names, \ > .num_parents = \ > ARRAY_SIZE(_name##_parent_names), \ > .flags = _flags, \ > }; > > and then you have something like > > #define DEFINE_CLK_IMX(_name, _flags, _foo, _bar) \ > static struct clk_imx _name##_hw = { \ > .hw = { \ > .clk = &_name, \ > }, \ > .foo = _foo, \ > .bar = _bar, \ > }; \ > DEFINE_CLK_GENERIC(_name, _flags, clk_imx_ops) > > Andrew
On Fri, Mar 09, 2012 at 10:25:00AM -0800, Turquette, Mike wrote: ... > However if you have the ability to use the clk_foo_register functions > please do use them in place of static initialization. The static init > stuff is only for folks backed into a corner and forced to use it... > for now. I'm looking at ways to allow for kmalloc'ing in early boot, > as well as reducing the number of clocks that my platform registers > during early boot drastically. > While I agree using registration functions rather than static initialization will help make "struct clk" an opaque cookie, I also see some benefit with using static initialization over registration functions. That is we will be able to initialize parents statically rather than calling expensive __clk_lookup() to find them when using registration functions. I'm not sure if this will be a concern with the platforms that have hundreds of clocks. Keep it in mind, when we say one clock, there are generally 3 clks behind it, clk_gate, clk_divider and clk_mux.
On Mon, Mar 19, 2012 at 03:01:17PM +0800, Shawn Guo wrote: > On Fri, Mar 09, 2012 at 10:25:00AM -0800, Turquette, Mike wrote: > ... > > However if you have the ability to use the clk_foo_register functions > > please do use them in place of static initialization. The static init > > stuff is only for folks backed into a corner and forced to use it... > > for now. I'm looking at ways to allow for kmalloc'ing in early boot, > > as well as reducing the number of clocks that my platform registers > > during early boot drastically. > > > While I agree using registration functions rather than static > initialization will help make "struct clk" an opaque cookie, I also > see some benefit with using static initialization over registration > functions. That is we will be able to initialize parents statically > rather than calling expensive __clk_lookup() to find them when using > registration functions. > > I'm not sure if this will be a concern with the platforms that have > hundreds of clocks. Keep it in mind, when we say one clock, there > are generally 3 clks behind it, clk_gate, clk_divider and clk_mux. On an i.MX51 with a fully dynamically allocated clock tree it takes about 10ms to initialize the tree which I think is acceptable. The clock tree is not complete, but I would think that about 70% of the clocks are there. Normally less performant platforms will have less clocks, so I assume the times will be comparable. Sascha
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);