| Message ID | 1353439206-1654-1-git-send-email-swarren@wwwdotorg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Stephen, On Tue, 20 Nov 2012 12:20:05 -0700, Stephen Warren wrote: > It is desirable to move all clocksource drivers to drivers/clocksource, > yet each requires its own initialization function. We'd rather not > pollute <linux/> with a header for each function. Instead, create a > single of_clksrc_init() function which will determine which clocksource > driver to initialize based on device tree. Sounds great! I'm still planning to send irqchip soon, but I've been busy preparing things for the merge window in the mvebu architecture, which has been quite active during this cycle. > +#include <linux/init.h> > +#include <linux/of.h> > + > +extern struct of_device_id *__start_clksrc_of_match_tables[]; > +extern struct of_device_id *__stop_clksrc_of_match_tables[]; > + > +void __init clocksource_of_init(void) > +{ > + struct of_device_id **table; > + struct of_device_id **stop; > + struct device_node *np; > + const struct of_device_id *match; > + void (*init_func)(void); > + > + table = __start_clksrc_of_match_tables; > + stop = __stop_clksrc_of_match_tables; > + > + for ( ; table < stop; table++) { > + for_each_matching_node(np, *table) { > + match = of_match_node(*table, np); > + init_func = match->data; > + init_func(); > + } > + } > +} The way I did things for irqchip is a bit different: I used the standard of_irq_init(), which requires a table that is terminated by a sentinel. In order to guarantee that I have a sentinel, I did: #define IRQCHIP_DECLARE(compstr,fn) \ static const struct of_device_id irqchip_of_match \ __used __section(.init.irqchip) \ = { .compatible = compstr, .data = fn } which is used to declare all irq drivers. But then in the main irqchip.c file, I did: static const struct of_device_id irqchip_of_match_end __used __section(.init.irqchip.end); and my linker script looks like: .init.irqchip : { __irqchip_begin = .; *(.init.irqchip) *(.init.irqchip.end) } So I gather all the of_device_id definitions and then the special sentinel of_device_id. I don't know if it is better than your solution, or even if my solution is good at all. Best regards, Thomas
On 11/20/2012 12:31 PM, Thomas Petazzoni wrote: > Stephen, > > On Tue, 20 Nov 2012 12:20:05 -0700, Stephen Warren wrote: > >> It is desirable to move all clocksource drivers to drivers/clocksource, >> yet each requires its own initialization function. We'd rather not >> pollute <linux/> with a header for each function. Instead, create a >> single of_clksrc_init() function which will determine which clocksource >> driver to initialize based on device tree. ... > The way I did things for irqchip is a bit different: I used the > standard of_irq_init(), which requires a table that is terminated by a > sentinel. In order to guarantee that I have a sentinel, I did: > > #define IRQCHIP_DECLARE(compstr,fn) \ > static const struct of_device_id irqchip_of_match \ > __used __section(.init.irqchip) \ > = { .compatible = compstr, .data = fn } > > which is used to declare all irq drivers. But then in the main > irqchip.c file, I did: > > static const struct of_device_id > irqchip_of_match_end __used __section(.init.irqchip.end); > > and my linker script looks like: > > .init.irqchip : { > __irqchip_begin = .; > *(.init.irqchip) > *(.init.irqchip.end) > } > > So I gather all the of_device_id definitions and then the special > sentinel of_device_id. I don't know if it is better than your solution, > or even if my solution is good at all. That does have the advantage of constructing a single table for all irqchips, rather than the approach in my patch was to construct an array of pointers to those tables. Your approach is one less loop at runtime, and one less sentinel and pointer data item per irqchip. Perhaps I should convert mine to do the same thing, unless anyone gives feedback against it.
On Tue, Nov 20, 2012 at 12:20:05PM -0700, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > It is desirable to move all clocksource drivers to drivers/clocksource, > yet each requires its own initialization function. We'd rather not > pollute <linux/> with a header for each function. Instead, create a > single of_clksrc_init() function which will determine which clocksource > driver to initialize based on device tree. > > Inspired by a similar patch for drivers/irqchip by Thomas Petazzoni. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > v3: Use a linker section to replace manually maintained table. > v2: New patch. > [..] > --- /dev/null > +++ b/drivers/clocksource/clksrc-of.c > @@ -0,0 +1,41 @@ > +/* > + * Copyright (c) 2012, NVIDIA CORPORATION. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/init.h> > +#include <linux/of.h> > + > +extern struct of_device_id *__start_clksrc_of_match_tables[]; > +extern struct of_device_id *__stop_clksrc_of_match_tables[]; > + > +void __init clocksource_of_init(void) > +{ > + struct of_device_id **table; > + struct of_device_id **stop; > + struct device_node *np; > + const struct of_device_id *match; > + void (*init_func)(void); > + > + table = __start_clksrc_of_match_tables; > + stop = __stop_clksrc_of_match_tables; > + > + for ( ; table < stop; table++) { > + for_each_matching_node(np, *table) { > + match = of_match_node(*table, np); > + init_func = match->data; > + init_func(); Hmm. Am I crazy, or does this for_each_matching_node()/of_match_node() pattern end up walking the match table twice, unnecessarily? I'm wondering if we can come up with a for_each_matching_node_id() macro that also provides a pointer to the matching of_device_id... Josh
On 11/20/2012 01:10 PM, Josh Cartwright wrote: > On Tue, Nov 20, 2012 at 12:20:05PM -0700, Stephen Warren wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> It is desirable to move all clocksource drivers to drivers/clocksource, >> yet each requires its own initialization function. We'd rather not >> pollute <linux/> with a header for each function. Instead, create a >> single of_clksrc_init() function which will determine which clocksource >> driver to initialize based on device tree. >> >> Inspired by a similar patch for drivers/irqchip by Thomas Petazzoni. >> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> >> --- >> v3: Use a linker section to replace manually maintained table. >> v2: New patch. >> > [..] >> --- /dev/null >> +++ b/drivers/clocksource/clksrc-of.c >> +void __init clocksource_of_init(void) ... >> + for ( ; table < stop; table++) { >> + for_each_matching_node(np, *table) { >> + match = of_match_node(*table, np); >> + init_func = match->data; >> + init_func(); > > Hmm. Am I crazy, or does this for_each_matching_node()/of_match_node() > pattern end up walking the match table twice, unnecessarily? Yes, that's true. > I'm wondering if we can come up with a for_each_matching_node_id() macro > that also provides a pointer to the matching of_device_id... Good idea; I'll do that.
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index c9f67de..4f300c5 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -1,3 +1,6 @@ +config CLKSRC_OF + bool + config CLKSRC_I8253 bool diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile index 24fb888..29017a3 100644 --- a/drivers/clocksource/Makefile +++ b/drivers/clocksource/Makefile @@ -1,3 +1,4 @@ +obj-$(CONFIG_CLKSRC_OF) += clksrc-of.o obj-$(CONFIG_ATMEL_TCB_CLKSRC) += tcb_clksrc.o obj-$(CONFIG_X86_CYCLONE_TIMER) += cyclone.o obj-$(CONFIG_X86_PM_TIMER) += acpi_pm.o diff --git a/drivers/clocksource/clksrc-of.c b/drivers/clocksource/clksrc-of.c new file mode 100644 index 0000000..04c7820 --- /dev/null +++ b/drivers/clocksource/clksrc-of.c @@ -0,0 +1,41 @@ +/* + * Copyright (c) 2012, NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/init.h> +#include <linux/of.h> + +extern struct of_device_id *__start_clksrc_of_match_tables[]; +extern struct of_device_id *__stop_clksrc_of_match_tables[]; + +void __init clocksource_of_init(void) +{ + struct of_device_id **table; + struct of_device_id **stop; + struct device_node *np; + const struct of_device_id *match; + void (*init_func)(void); + + table = __start_clksrc_of_match_tables; + stop = __stop_clksrc_of_match_tables; + + for ( ; table < stop; table++) { + for_each_matching_node(np, *table) { + match = of_match_node(*table, np); + init_func = match->data; + init_func(); + } + } +} diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index d1ea7ce..b17f793 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -149,6 +149,15 @@ #define TRACE_SYSCALLS() #endif +#ifdef CONFIG_CLKSRC_OF +#define CLKSRC_OF_MATCH_TABLES() \ + . = ALIGN(8); \ + VMLINUX_SYMBOL(__start_clksrc_of_match_tables) = .; \ + *(__clksrc_of_table) \ + VMLINUX_SYMBOL(__stop_clksrc_of_match_tables) = .; +#else +#define CLKSRC_OF_MATCH_TABLES() +#endif #define KERNEL_DTB() \ STRUCT_ALIGN(); \ @@ -490,6 +499,7 @@ *(.init.rodata) \ FTRACE_EVENTS() \ TRACE_SYSCALLS() \ + CLKSRC_OF_MATCH_TABLES() \ DEV_DISCARD(init.rodata) \ CPU_DISCARD(init.rodata) \ MEM_DISCARD(init.rodata) \ diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h index 4dceaf8..3b0f30e 100644 --- a/include/linux/clocksource.h +++ b/include/linux/clocksource.h @@ -332,4 +332,14 @@ extern int clocksource_mmio_init(void __iomem *, const char *, extern int clocksource_i8253_init(void); +#ifdef CONFIG_CLKSRC_OF +extern void clocksource_of_init(void); + +#define CLKSRC_OF_MATCH_TABLE(name, table) \ + static const struct of_device_id __used \ + __attribute__((section("__clksrc_of_table"))) \ + *__p_clksrc_of_match_table_##name = table; + +#endif + #endif /* _LINUX_CLOCKSOURCE_H */