From: Stephen Warren <[email protected]>
The following pattern of code is tempting:
for_each_matching_node(np, table) {
match = of_match_node(table, np);
However, this results in iterating over table twice; the second time
inside of_match_node(). The implementation of for_each_matching_node()
already found the match, so this is redundant. Invent new function
of_find_matching_node_and_match() and macro
for_each_matching_node_and_match() to remove the double iteration,
thus transforming the above code to:
for_each_matching_node_and_match(np, table, &match)
Signed-off-by: Stephen Warren <[email protected]>
---
v4: New patch.
This series is based on the ARM sys_timer rework that I posted yesterday,
although patch 1/3 should be completely independant of that, and could
perhaps even be applied for 3.8 right now.
I hope that these patches can be taken through arm-soc tree for 3.9;
I will repost them based on 3.8-rc1 at the appropriate time.
---
drivers/of/base.c | 18 +++++++++++++-----
include/linux/of.h | 15 +++++++++++++--
2 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index f2f63c8..094271e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -594,27 +594,35 @@ const struct of_device_id *of_match_node(const struct of_device_id *matches,
EXPORT_SYMBOL(of_match_node);
/**
- * of_find_matching_node - Find a node based on an of_device_id match
- * table.
+ * of_find_matching_node_and_match - Find a node based on an of_device_id
+ * match table.
* @from: The node to start searching from or NULL, the node
* you pass will not be searched, only the next one
* will; typically, you pass what the previous call
* returned. of_node_put() will be called on it
* @matches: array of of device match structures to search in
+ * @match Updated to point at the matches entry which matched
*
* Returns a node pointer with refcount incremented, use
* of_node_put() on it when done.
*/
-struct device_node *of_find_matching_node(struct device_node *from,
- const struct of_device_id *matches)
+struct device_node *of_find_matching_node_and_match(struct device_node *from,
+ const struct of_device_id *matches,
+ const struct of_device_id **match)
{
struct device_node *np;
+ if (match)
+ *match = NULL;
+
read_lock(&devtree_lock);
np = from ? from->allnext : allnodes;
for (; np; np = np->allnext) {
- if (of_match_node(matches, np) && of_node_get(np))
+ if (of_match_node(matches, np) && of_node_get(np)) {
+ if (match)
+ *match = matches;
break;
+ }
}
of_node_put(from);
read_unlock(&devtree_lock);
diff --git a/include/linux/of.h b/include/linux/of.h
index 681a6c8..1000496 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -180,11 +180,22 @@ extern struct device_node *of_find_compatible_node(struct device_node *from,
#define for_each_compatible_node(dn, type, compatible) \
for (dn = of_find_compatible_node(NULL, type, compatible); dn; \
dn = of_find_compatible_node(dn, type, compatible))
-extern struct device_node *of_find_matching_node(struct device_node *from,
- const struct of_device_id *matches);
+extern struct device_node *of_find_matching_node_and_match(
+ struct device_node *from,
+ const struct of_device_id *matches,
+ const struct of_device_id **match);
+static inline struct device_node *of_find_matching_node(
+ struct device_node *from,
+ const struct of_device_id *matches)
+{
+ return of_find_matching_node_and_match(from, matches, NULL);
+}
#define for_each_matching_node(dn, matches) \
for (dn = of_find_matching_node(NULL, matches); dn; \
dn = of_find_matching_node(dn, matches))
+#define for_each_matching_node_and_match(dn, matches, match) \
+ for (dn = of_find_matching_node_and_match(NULL, matches, match); \
+ dn; dn = of_find_matching_node_and_match(dn, matches, match))
extern struct device_node *of_find_node_by_path(const char *path);
extern struct device_node *of_find_node_by_phandle(phandle handle);
extern struct device_node *of_get_parent(const struct device_node *node);
--
1.7.10.4
From: Stephen Warren <[email protected]>
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.
Based on a similar patch for drivers/irqchip by Thomas Petazzoni.
Signed-off-by: Stephen Warren <[email protected]>
---
v4:
* Use for_each_matching_node_and_match().
* Build a single of_device_id table, rather than a table of pointers to
partial tables.
v3: Use a linker section to replace manually maintained table.
v2: New patch.
---
drivers/clocksource/Kconfig | 3 +++
drivers/clocksource/Makefile | 1 +
drivers/clocksource/clksrc-of.c | 35 +++++++++++++++++++++++++++++++++++
include/asm-generic/vmlinux.lds.h | 9 +++++++++
include/linux/clocksource.h | 9 +++++++++
5 files changed, 57 insertions(+)
create mode 100644 drivers/clocksource/clksrc-of.c
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..bdabdaa
--- /dev/null
+++ b/drivers/clocksource/clksrc-of.c
@@ -0,0 +1,35 @@
+/*
+ * 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 __clksrc_of_table[];
+
+static const struct of_device_id __clksrc_of_table_sentinel
+ __used __section(__clksrc_of_table_end);
+
+void __init clocksource_of_init(void)
+{
+ struct device_node *np;
+ const struct of_device_id *match;
+ void (*init_func)(void);
+
+ for_each_matching_node_and_match(np, __clksrc_of_table, &match) {
+ init_func = match->data;
+ init_func();
+ }
+}
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index d1ea7ce..1e744c5 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -149,6 +149,14 @@
#define TRACE_SYSCALLS()
#endif
+#ifdef CONFIG_CLKSRC_OF
+#define CLKSRC_OF_TABLES() . = ALIGN(8); \
+ VMLINUX_SYMBOL(__clksrc_of_table) = .; \
+ *(__clksrc_of_table) \
+ *(__clksrc_of_table_end)
+#else
+#define CLKSRC_OF_TABLES()
+#endif
#define KERNEL_DTB() \
STRUCT_ALIGN(); \
@@ -493,6 +501,7 @@
DEV_DISCARD(init.rodata) \
CPU_DISCARD(init.rodata) \
MEM_DISCARD(init.rodata) \
+ CLKSRC_OF_TABLES() \
KERNEL_DTB()
#define INIT_TEXT \
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 4dceaf8..7944f14 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -332,4 +332,13 @@ 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 CLOCKSOURCE_OF_DECLARE(name, compat, fn) \
+ static const struct of_device_id __clksrc_of_table_##name \
+ __used __section(__clksrc_of_table) \
+ = { .compatible = compat, .data = fn };
+#endif
+
#endif /* _LINUX_CLOCKSOURCE_H */
--
1.7.10.4
From: Stephen Warren <[email protected]>
Move arch/arm/mach-tegra/timer.c to drivers/clocksource/tegra20_timer.c
so that the code is co-located with other clocksource drivers, and to
reduce the size of the mach-tegra directory.
Signed-off-by: Stephen Warren <[email protected]>
---
v4: Adjust to changes in the rest of the patch series.
v3: Rework using CLKSRC_OF_MATCH_TABLE() for registration, rather than a
manually maintained table in clksrc-of.c.
v2: Rebase on ARM sys_timer rework, and addition of clocksource_of_init().
---
arch/arm/Kconfig | 1 +
arch/arm/mach-tegra/Makefile | 1 -
arch/arm/mach-tegra/board-dt-tegra20.c | 3 ++-
arch/arm/mach-tegra/board-dt-tegra30.c | 3 ++-
arch/arm/mach-tegra/board.h | 1 -
drivers/clocksource/Makefile | 1 +
.../mach-tegra/timer.c => drivers/clocksource/tegra20_timer.c | 7 ++-----
7 files changed, 8 insertions(+), 9 deletions(-)
rename arch/arm/mach-tegra/timer.c => drivers/clocksource/tegra20_timer.c (98%)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 14f8160..9eb54f6 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -633,6 +633,7 @@ config ARCH_TEGRA
select ARCH_HAS_CPUFREQ
select CLKDEV_LOOKUP
select CLKSRC_MMIO
+ select CLKSRC_OF
select COMMON_CLK
select GENERIC_CLOCKEVENTS
select GENERIC_GPIO
diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
index 6f224f7..6ab3ed5 100644
--- a/arch/arm/mach-tegra/Makefile
+++ b/arch/arm/mach-tegra/Makefile
@@ -2,7 +2,6 @@ obj-y += common.o
obj-y += io.o
obj-y += irq.o
obj-y += clock.o
-obj-y += timer.o
obj-y += fuse.o
obj-y += pmc.o
obj-y += flowctrl.o
diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c b/arch/arm/mach-tegra/board-dt-tegra20.c
index 794a7bf..0bee85a 100644
--- a/arch/arm/mach-tegra/board-dt-tegra20.c
+++ b/arch/arm/mach-tegra/board-dt-tegra20.c
@@ -15,6 +15,7 @@
*
*/
+#include <linux/clocksource.h>
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/platform_device.h>
@@ -194,7 +195,7 @@ DT_MACHINE_START(TEGRA_DT, "nVidia Tegra20 (Flattened Device Tree)")
.init_early = tegra20_init_early,
.init_irq = tegra_dt_init_irq,
.handle_irq = gic_handle_irq,
- .init_time = tegra_init_timer,
+ .init_time = clocksource_of_init,
.init_machine = tegra_dt_init,
.init_late = tegra_dt_init_late,
.restart = tegra_assert_system_reset,
diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c
index 08d3b19..a2b6cf1 100644
--- a/arch/arm/mach-tegra/board-dt-tegra30.c
+++ b/arch/arm/mach-tegra/board-dt-tegra30.c
@@ -23,6 +23,7 @@
*
*/
+#include <linux/clocksource.h>
#include <linux/kernel.h>
#include <linux/of.h>
#include <linux/of_address.h>
@@ -104,7 +105,7 @@ DT_MACHINE_START(TEGRA30_DT, "NVIDIA Tegra30 (Flattened Device Tree)")
.init_early = tegra30_init_early,
.init_irq = tegra_dt_init_irq,
.handle_irq = gic_handle_irq,
- .init_time = tegra_init_timer,
+ .init_time = clocksource_of_init,
.init_machine = tegra30_dt_init,
.init_late = tegra_init_late,
.restart = tegra_assert_system_reset,
diff --git a/arch/arm/mach-tegra/board.h b/arch/arm/mach-tegra/board.h
index 744cdd2..da8f5a3 100644
--- a/arch/arm/mach-tegra/board.h
+++ b/arch/arm/mach-tegra/board.h
@@ -55,5 +55,4 @@ static inline int harmony_pcie_init(void) { return 0; }
void __init tegra_paz00_wifikill_init(void);
-extern void tegra_init_timer(void);
#endif
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 29017a3..ecf37f3 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -16,5 +16,6 @@ obj-$(CONFIG_CLKSRC_NOMADIK_MTU) += nomadik-mtu.o
obj-$(CONFIG_CLKSRC_DBX500_PRCMU) += clksrc-dbx500-prcmu.o
obj-$(CONFIG_ARMADA_370_XP_TIMER) += time-armada-370-xp.o
obj-$(CONFIG_ARCH_BCM2835) += bcm2835_timer.o
+obj-$(CONFIG_ARCH_TEGRA) += tegra20_timer.o
obj-$(CONFIG_CLKSRC_ARM_GENERIC) += arm_generic.o
diff --git a/arch/arm/mach-tegra/timer.c b/drivers/clocksource/tegra20_timer.c
similarity index 98%
rename from arch/arm/mach-tegra/timer.c
rename to drivers/clocksource/tegra20_timer.c
index b0036e5..3b2f947 100644
--- a/arch/arm/mach-tegra/timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -1,6 +1,4 @@
/*
- * arch/arch/mach-tegra/timer.c
- *
* Copyright (C) 2010 Google, Inc.
*
* Author:
@@ -33,8 +31,6 @@
#include <asm/smp_twd.h>
#include <asm/sched_clock.h>
-#include "board.h"
-
#define RTC_SECONDS 0x08
#define RTC_SHADOW_SECONDS 0x0c
#define RTC_MILLISECONDS 0x10
@@ -168,7 +164,7 @@ static const struct of_device_id rtc_match[] __initconst = {
{}
};
-void __init tegra_init_timer(void)
+static void __init tegra20_init_timer(void)
{
struct device_node *np;
struct clk *clk;
@@ -272,6 +268,7 @@ void __init tegra_init_timer(void)
#endif
register_persistent_clock(NULL, tegra_read_persistent_clock);
}
+CLOCKSOURCE_OF_DECLARE(tegra20, "nvidia,tegra20-timer", tegra20_init_timer);
#ifdef CONFIG_PM
static u32 usec_config;
--
1.7.10.4
On 11/20/2012 05:12 PM, Stephen Warren wrote:
> From: Stephen Warren <[email protected]>
>
> The following pattern of code is tempting:
>
> for_each_matching_node(np, table) {
> match = of_match_node(table, np);
>
> However, this results in iterating over table twice; the second time
> inside of_match_node(). The implementation of for_each_matching_node()
> already found the match, so this is redundant. Invent new function
> of_find_matching_node_and_match() and macro
> for_each_matching_node_and_match() to remove the double iteration,
> thus transforming the above code to:
>
> for_each_matching_node_and_match(np, table, &match)
>
> Signed-off-by: Stephen Warren <[email protected]>
> ---
> v4: New patch.
>
> This series is based on the ARM sys_timer rework that I posted yesterday,
> although patch 1/3 should be completely independant of that, and could
> perhaps even be applied for 3.8 right now.
Agreed. I will apply for 3.8 assuming no further comments in the next
few days.
Rob
> I hope that these patches can be taken through arm-soc tree for 3.9;
> I will repost them based on 3.8-rc1 at the appropriate time.
> ---
> drivers/of/base.c | 18 +++++++++++++-----
> include/linux/of.h | 15 +++++++++++++--
> 2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index f2f63c8..094271e 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -594,27 +594,35 @@ const struct of_device_id *of_match_node(const struct of_device_id *matches,
> EXPORT_SYMBOL(of_match_node);
>
> /**
> - * of_find_matching_node - Find a node based on an of_device_id match
> - * table.
> + * of_find_matching_node_and_match - Find a node based on an of_device_id
> + * match table.
> * @from: The node to start searching from or NULL, the node
> * you pass will not be searched, only the next one
> * will; typically, you pass what the previous call
> * returned. of_node_put() will be called on it
> * @matches: array of of device match structures to search in
> + * @match Updated to point at the matches entry which matched
> *
> * Returns a node pointer with refcount incremented, use
> * of_node_put() on it when done.
> */
> -struct device_node *of_find_matching_node(struct device_node *from,
> - const struct of_device_id *matches)
> +struct device_node *of_find_matching_node_and_match(struct device_node *from,
> + const struct of_device_id *matches,
> + const struct of_device_id **match)
> {
> struct device_node *np;
>
> + if (match)
> + *match = NULL;
> +
> read_lock(&devtree_lock);
> np = from ? from->allnext : allnodes;
> for (; np; np = np->allnext) {
> - if (of_match_node(matches, np) && of_node_get(np))
> + if (of_match_node(matches, np) && of_node_get(np)) {
> + if (match)
> + *match = matches;
> break;
> + }
> }
> of_node_put(from);
> read_unlock(&devtree_lock);
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 681a6c8..1000496 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -180,11 +180,22 @@ extern struct device_node *of_find_compatible_node(struct device_node *from,
> #define for_each_compatible_node(dn, type, compatible) \
> for (dn = of_find_compatible_node(NULL, type, compatible); dn; \
> dn = of_find_compatible_node(dn, type, compatible))
> -extern struct device_node *of_find_matching_node(struct device_node *from,
> - const struct of_device_id *matches);
> +extern struct device_node *of_find_matching_node_and_match(
> + struct device_node *from,
> + const struct of_device_id *matches,
> + const struct of_device_id **match);
> +static inline struct device_node *of_find_matching_node(
> + struct device_node *from,
> + const struct of_device_id *matches)
> +{
> + return of_find_matching_node_and_match(from, matches, NULL);
> +}
> #define for_each_matching_node(dn, matches) \
> for (dn = of_find_matching_node(NULL, matches); dn; \
> dn = of_find_matching_node(dn, matches))
> +#define for_each_matching_node_and_match(dn, matches, match) \
> + for (dn = of_find_matching_node_and_match(NULL, matches, match); \
> + dn; dn = of_find_matching_node_and_match(dn, matches, match))
> extern struct device_node *of_find_node_by_path(const char *path);
> extern struct device_node *of_find_node_by_phandle(phandle handle);
> extern struct device_node *of_get_parent(const struct device_node *node);
>
On Tuesday 20 November 2012, Stephen Warren wrote:
> However, this results in iterating over table twice; the second time
> inside of_match_node(). The implementation of for_each_matching_node()
> already found the match, so this is redundant. Invent new function
> of_find_matching_node_and_match() and macro
> for_each_matching_node_and_match() to remove the double iteration,
> thus transforming the above code to:
>
> for_each_matching_node_and_match(np, table, &match)
>
> Signed-off-by: Stephen Warren <[email protected]>
This look useful, but I wonder if the interface would make more sense if you
make the last argument to the macro a normal pointer, rather than a
pointer-to-pointer. You can take the reference as part of the macro.
Arnd
On Wed, Nov 21, 2012 at 9:53 AM, Arnd Bergmann <[email protected]> wrote:
> On Tuesday 20 November 2012, Stephen Warren wrote:
>> However, this results in iterating over table twice; the second time
>> inside of_match_node(). The implementation of for_each_matching_node()
>> already found the match, so this is redundant. Invent new function
>> of_find_matching_node_and_match() and macro
>> for_each_matching_node_and_match() to remove the double iteration,
>> thus transforming the above code to:
>>
>> for_each_matching_node_and_match(np, table, &match)
>>
>> Signed-off-by: Stephen Warren <[email protected]>
>
> This look useful, but I wonder if the interface would make more sense if you
> make the last argument to the macro a normal pointer, rather than a
> pointer-to-pointer. You can take the reference as part of the macro.
To me that makes for harder to understand code. It *looks* like an
argument to a normal function call, but it gets changed by the caller.
g.
On Wed, 21 Nov 2012 10:06:16 +0000, Grant Likely wrote:
> On Wed, Nov 21, 2012 at 9:53 AM, Arnd Bergmann <[email protected]> wrote:
> > On Tuesday 20 November 2012, Stephen Warren wrote:
> >> However, this results in iterating over table twice; the second time
> >> inside of_match_node(). The implementation of for_each_matching_node()
> >> already found the match, so this is redundant. Invent new function
> >> of_find_matching_node_and_match() and macro
> >> for_each_matching_node_and_match() to remove the double iteration,
> >> thus transforming the above code to:
> >>
> >> for_each_matching_node_and_match(np, table, &match)
> >>
> >> Signed-off-by: Stephen Warren <[email protected]>
> >
> > This look useful, but I wonder if the interface would make more sense if you
> > make the last argument to the macro a normal pointer, rather than a
> > pointer-to-pointer. You can take the reference as part of the macro.
>
> To me that makes for harder to understand code. It *looks* like an
> argument to a normal function call, but it gets changed by the caller.
Agreed. Too much magic is too much.
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com