[RFC v2 1/9] arm/versatile*: merge all versatile struct clk definitions

[RFC v2 1/9] arm/versatile*: merge all versatile struct clk definitions

[Grant Likely]

There is no little value to each versatile type platform having it's own
struct clk definition.  Merge all of them into plat-versatile to eliminate
duplication.

This patch probably conflicts with the common struct clk work, but the
timeline for that larger work is still uncertain, and this patch can be
considered a step in that direction.  Also, a conflict with this patch
will be minor and easily resolved.

Note: I use a couple of #ifdef CONFIG_ARCH_INTEGRATOR block to avoid
including the module pointer for configurations that don't need it, but
that is only a space optimization.  I'd rather include the module
pointer unconditionally, and if other agree then I'll remove the #ifdef

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Mike Turquette <mturquette@ti.com>
---
 arch/arm/mach-integrator/include/mach/clkdev.h |   19 ---------------
 arch/arm/mach-realview/include/mach/clkdev.h   |   10 --------
 arch/arm/mach-versatile/include/mach/clkdev.h  |   10 --------
 arch/arm/mach-vexpress/include/mach/clkdev.h   |    9 -------
 arch/arm/mach-zynq/include/mach/clkdev.h       |   10 --------
 arch/arm/plat-versatile/include/plat/clock.h   |   29 ++++++++++++++++++++++++
 6 files changed, 29 insertions(+), 58 deletions(-)

diff --git a/arch/arm/mach-integrator/include/mach/clkdev.h b/arch/arm/mach-integrator/include/mach/clkdev.h
index bfe0767..96ae8a4 100644
--- a/arch/arm/mach-integrator/include/mach/clkdev.h
+++ b/arch/arm/mach-integrator/include/mach/clkdev.h
@@ -4,23 +4,4 @@
 #include <linux/module.h>
 #include <plat/clock.h>
 
-struct clk {
-	unsigned long		rate;
-	const struct clk_ops	*ops;
-	struct module		*owner;
-	const struct icst_params *params;
-	void __iomem		*vcoreg;
-	void			*data;
-};
-
-static inline int __clk_get(struct clk *clk)
-{
-	return try_module_get(clk->owner);
-}
-
-static inline void __clk_put(struct clk *clk)
-{
-	module_put(clk->owner);
-}
-
 #endif
diff --git a/arch/arm/mach-realview/include/mach/clkdev.h b/arch/arm/mach-realview/include/mach/clkdev.h
index e58d077..629b0ef 100644
--- a/arch/arm/mach-realview/include/mach/clkdev.h
+++ b/arch/arm/mach-realview/include/mach/clkdev.h
@@ -3,14 +3,4 @@
 
 #include <plat/clock.h>
 
-struct clk {
-	unsigned long		rate;
-	const struct clk_ops	*ops;
-	const struct icst_params *params;
-	void __iomem		*vcoreg;
-};
-
-#define __clk_get(clk) ({ 1; })
-#define __clk_put(clk) do { } while (0)
-
 #endif
diff --git a/arch/arm/mach-versatile/include/mach/clkdev.h b/arch/arm/mach-versatile/include/mach/clkdev.h
index e58d077..629b0ef 100644
--- a/arch/arm/mach-versatile/include/mach/clkdev.h
+++ b/arch/arm/mach-versatile/include/mach/clkdev.h
@@ -3,14 +3,4 @@
 
 #include <plat/clock.h>
 
-struct clk {
-	unsigned long		rate;
-	const struct clk_ops	*ops;
-	const struct icst_params *params;
-	void __iomem		*vcoreg;
-};
-
-#define __clk_get(clk) ({ 1; })
-#define __clk_put(clk) do { } while (0)
-
 #endif
diff --git a/arch/arm/mach-vexpress/include/mach/clkdev.h b/arch/arm/mach-vexpress/include/mach/clkdev.h
index 3f8307d..629b0ef 100644
--- a/arch/arm/mach-vexpress/include/mach/clkdev.h
+++ b/arch/arm/mach-vexpress/include/mach/clkdev.h
@@ -3,13 +3,4 @@
 
 #include <plat/clock.h>
 
-struct clk {
-	const struct clk_ops	*ops;
-	unsigned long		rate;
-	const struct icst_params *params;
-};
-
-#define __clk_get(clk) ({ 1; })
-#define __clk_put(clk) do { } while (0)
-
 #endif
diff --git a/arch/arm/mach-zynq/include/mach/clkdev.h b/arch/arm/mach-zynq/include/mach/clkdev.h
index c6e73d8..bcce0e6 100644
--- a/arch/arm/mach-zynq/include/mach/clkdev.h
+++ b/arch/arm/mach-zynq/include/mach/clkdev.h
@@ -19,14 +19,4 @@
 
 #include <plat/clock.h>
 
-struct clk {
-	unsigned long		rate;
-	const struct clk_ops	*ops;
-	const struct icst_params *params;
-	void __iomem		*vcoreg;
-};
-
-#define __clk_get(clk) ({ 1; })
-#define __clk_put(clk) do { } while (0)
-
 #endif
diff --git a/arch/arm/plat-versatile/include/plat/clock.h b/arch/arm/plat-versatile/include/plat/clock.h
index 3cfb024..2117701 100644
--- a/arch/arm/plat-versatile/include/plat/clock.h
+++ b/arch/arm/plat-versatile/include/plat/clock.h
@@ -3,6 +3,19 @@
 
 #include <asm/hardware/icst.h>
 
+struct module;
+
+struct clk {
+	unsigned long		rate;
+	const struct clk_ops	*ops;
+	const struct icst_params *params;
+	void __iomem		*vcoreg;
+#ifdef CONFIG_ARCH_INTEGRATOR
+	struct module		*owner;
+	void			*data;
+#endif
+};
+
 struct clk_ops {
 	long	(*round)(struct clk *, unsigned long);
 	int	(*set)(struct clk *, unsigned long);
@@ -12,4 +25,20 @@ struct clk_ops {
 int icst_clk_set(struct clk *, unsigned long);
 long icst_clk_round(struct clk *, unsigned long);
 
+#ifdef CONFIG_ARCH_INTEGRATOR
+static inline int __clk_get(struct clk *clk)
+{
+	return try_module_get(clk->owner);
+}
+
+static inline void __clk_put(struct clk *clk)
+{
+	module_put(clk->owner);
+}
+#else
+#define __clk_get(clk) ({ 1; })
+#define __clk_put(clk) do { } while (0)
+#endif
+
+
 #endif
-- 
1.7.5.4

[RFC v2 2/9] arm/versatile*: Consolidate clk_ops and setvco implementations.

[Grant Likely]

All of the icst setvco routines for ARM development boards are pretty close
to identical.  Consolidate them all to one implementation.

Note: This might be broken on Integrator.  To simplify the common function,
it always does a read/modify/write on the register.  Integrator only does
a simple write without preserving some of the bits in the old value.

Russell, if this will break Integrator, then I can do it slightly differently
to avoid the problem.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Mike Turquette <mturquette@ti.com>
---
 arch/arm/mach-integrator/impd1.c             |   30 ++-----------------------
 arch/arm/mach-integrator/integrator_cp.c     |   21 +----------------
 arch/arm/mach-realview/core.c                |   22 +-----------------
 arch/arm/mach-versatile/core.c               |   22 +-----------------
 arch/arm/plat-versatile/clock.c              |   21 ++++++++++++++++++
 arch/arm/plat-versatile/include/plat/clock.h |    4 ++-
 6 files changed, 33 insertions(+), 87 deletions(-)

diff --git a/arch/arm/mach-integrator/impd1.c b/arch/arm/mach-integrator/impd1.c
index 8cbb75a..a42f6bd 100644
--- a/arch/arm/mach-integrator/impd1.c
+++ b/arch/arm/mach-integrator/impd1.c
@@ -52,31 +52,6 @@ static const struct icst_params impd1_vco_params = {
 	.idx2s		= icst525_idx2s,
 };
 
-static void impd1_setvco(struct clk *clk, struct icst_vco vco)
-{
-	struct impd1_module *impd1 = clk->data;
-	u32 val = vco.v | (vco.r << 9) | (vco.s << 16);
-
-	writel(0xa05f, impd1->base + IMPD1_LOCK);
-	writel(val, clk->vcoreg);
-	writel(0, impd1->base + IMPD1_LOCK);
-
-#ifdef DEBUG
-	vco.v = val & 0x1ff;
-	vco.r = (val >> 9) & 0x7f;
-	vco.s = (val >> 16) & 7;
-
-	pr_debug("IM-PD1: VCO%d clock is %ld Hz\n",
-		 vconr, icst525_hz(&impd1_vco_params, vco));
-#endif
-}
-
-static const struct clk_ops impd1_clk_ops = {
-	.round	= icst_clk_round,
-	.set	= icst_clk_set,
-	.setvco	= impd1_setvco,
-};
-
 void impd1_tweak_control(struct device *dev, u32 mask, u32 val)
 {
 	struct impd1_module *impd1 = dev_get_drvdata(dev);
@@ -377,13 +352,14 @@ static int impd1_probe(struct lm_device *dev)
 		(unsigned long)dev->resource.start);
 
 	for (i = 0; i < ARRAY_SIZE(impd1->vcos); i++) {
-		impd1->vcos[i].ops = &impd1_clk_ops,
+		impd1->vcos[i].ops = &icst_clk_default_ops,
 		impd1->vcos[i].owner = THIS_MODULE,
 		impd1->vcos[i].params = &impd1_vco_params,
-		impd1->vcos[i].data = impd1;
 	}
 	impd1->vcos[0].vcoreg = impd1->base + IMPD1_OSC1;
+	impd1->vcos[0].lockreg = impd1->base + IMPD1_LOCK;
 	impd1->vcos[1].vcoreg = impd1->base + IMPD1_OSC2;
+	impd1->vcos[1].lockreg = impd1->base + IMPD1_LOCK;
 
 	impd1->clks[0] = clkdev_alloc(&impd1->vcos[0], NULL, "lm%x:01000",
 					dev->id);
diff --git a/arch/arm/mach-integrator/integrator_cp.c b/arch/arm/mach-integrator/integrator_cp.c
index 5de49c3..3342926 100644
--- a/arch/arm/mach-integrator/integrator_cp.c
+++ b/arch/arm/mach-integrator/integrator_cp.c
@@ -205,28 +205,11 @@ static const struct icst_params cp_auxvco_params = {
 	.idx2s		= icst525_idx2s,
 };
 
-static void cp_auxvco_set(struct clk *clk, struct icst_vco vco)
-{
-	u32 val;
-
-	val = readl(clk->vcoreg) & ~0x7ffff;
-	val |= vco.v | (vco.r << 9) | (vco.s << 16);
-
-	writel(0xa05f, CM_LOCK);
-	writel(val, clk->vcoreg);
-	writel(0, CM_LOCK);
-}
-
-static const struct clk_ops cp_auxclk_ops = {
-	.round	= icst_clk_round,
-	.set	= icst_clk_set,
-	.setvco	= cp_auxvco_set,
-};
-
 static struct clk cp_auxclk = {
-	.ops	= &cp_auxclk_ops,
+	.ops	= &icst_clk_default_ops,
 	.params	= &cp_auxvco_params,
 	.vcoreg	= CM_AUXOSC,
+	.lockreg= CM_LOCK,
 };
 
 static struct clk sp804_clk = {
diff --git a/arch/arm/mach-realview/core.c b/arch/arm/mach-realview/core.c
index d5ed5d4..41b2f91 100644
--- a/arch/arm/mach-realview/core.c
+++ b/arch/arm/mach-realview/core.c
@@ -242,27 +242,8 @@ static const struct icst_params realview_oscvco_params = {
 	.idx2s		= icst307_idx2s,
 };
 
-static void realview_oscvco_set(struct clk *clk, struct icst_vco vco)
-{
-	void __iomem *sys_lock = __io_address(REALVIEW_SYS_BASE) + REALVIEW_SYS_LOCK_OFFSET;
-	u32 val;
-
-	val = readl(clk->vcoreg) & ~0x7ffff;
-	val |= vco.v | (vco.r << 9) | (vco.s << 16);
-
-	writel(0xa05f, sys_lock);
-	writel(val, clk->vcoreg);
-	writel(0, sys_lock);
-}
-
-static const struct clk_ops oscvco_clk_ops = {
-	.round	= icst_clk_round,
-	.set	= icst_clk_set,
-	.setvco	= realview_oscvco_set,
-};
-
 static struct clk oscvco_clk = {
-	.ops	= &oscvco_clk_ops,
+	.ops	= &icst_clk_default_ops,
 	.params	= &realview_oscvco_params,
 };
 
@@ -333,6 +314,7 @@ void __init realview_init_early(void)
 		oscvco_clk.vcoreg = sys + REALVIEW_SYS_OSC0_OFFSET;
 	else
 		oscvco_clk.vcoreg = sys + REALVIEW_SYS_OSC4_OFFSET;
+	oscvco_clk.lockreg = sys + REALVIEW_SYS_LOCK_OFFSET;
 
 	clkdev_add_table(lookups, ARRAY_SIZE(lookups));
 
diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
index e340a54..47f4531 100644
--- a/arch/arm/mach-versatile/core.c
+++ b/arch/arm/mach-versatile/core.c
@@ -337,27 +337,8 @@ static const struct icst_params versatile_oscvco_params = {
 	.idx2s		= icst307_idx2s,
 };
 
-static void versatile_oscvco_set(struct clk *clk, struct icst_vco vco)
-{
-	void __iomem *sys_lock = __io_address(VERSATILE_SYS_BASE) + VERSATILE_SYS_LOCK_OFFSET;
-	u32 val;
-
-	val = readl(clk->vcoreg) & ~0x7ffff;
-	val |= vco.v | (vco.r << 9) | (vco.s << 16);
-
-	writel(0xa05f, sys_lock);
-	writel(val, clk->vcoreg);
-	writel(0, sys_lock);
-}
-
-static const struct clk_ops osc4_clk_ops = {
-	.round	= icst_clk_round,
-	.set	= icst_clk_set,
-	.setvco	= versatile_oscvco_set,
-};
-
 static struct clk osc4_clk = {
-	.ops	= &osc4_clk_ops,
+	.ops	= &icst_clk_default_ops,
 	.params	= &versatile_oscvco_params,
 };
 
@@ -751,6 +732,7 @@ void __init versatile_init_early(void)
 	void __iomem *sys = __io_address(VERSATILE_SYS_BASE);
 
 	osc4_clk.vcoreg	= sys + VERSATILE_SYS_OSCCLCD_OFFSET;
+	osc4_clk.lockreg = sys + VERSATILE_SYS_LOCK_OFFSET;
 	clkdev_add_table(lookups, ARRAY_SIZE(lookups));
 
 	versatile_sched_clock_init(sys + VERSATILE_SYS_24MHz_OFFSET, 24000000);
diff --git a/arch/arm/plat-versatile/clock.c b/arch/arm/plat-versatile/clock.c
index 5c8b656..98a9dd8 100644
--- a/arch/arm/plat-versatile/clock.c
+++ b/arch/arm/plat-versatile/clock.c
@@ -12,6 +12,7 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/clk.h>
+#include <linux/io.h>
 #include <linux/mutex.h>
 
 #include <asm/hardware/icst.h>
@@ -53,6 +54,19 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 }
 EXPORT_SYMBOL(clk_set_rate);
 
+void icst_clk_setvco(struct clk *clk, struct icst_vco vco)
+{
+	u32 val;
+
+	val = readl(clk->vcoreg) & ~0x7ffff;
+	val |= vco.v | (vco.r << 9) | (vco.s << 16);
+
+	writel(0xa05f, clk->lockreg);
+	writel(val, clk->vcoreg);
+	writel(0, clk->lockreg);
+}
+EXPORT_SYMBOL(icst_clk_setvco);
+
 long icst_clk_round(struct clk *clk, unsigned long rate)
 {
 	struct icst_vco vco;
@@ -72,3 +86,10 @@ int icst_clk_set(struct clk *clk, unsigned long rate)
 	return 0;
 }
 EXPORT_SYMBOL(icst_clk_set);
+
+const struct clk_ops icst_clk_default_ops = {
+	.round	= icst_clk_round,
+	.set	= icst_clk_set,
+	.setvco	= icst_clk_setvco,
+};
+EXPORT_SYMBOL(icst_clk_default_ops);
diff --git a/arch/arm/plat-versatile/include/plat/clock.h b/arch/arm/plat-versatile/include/plat/clock.h
index 2117701..5749f79 100644
--- a/arch/arm/plat-versatile/include/plat/clock.h
+++ b/arch/arm/plat-versatile/include/plat/clock.h
@@ -10,9 +10,9 @@ struct clk {
 	const struct clk_ops	*ops;
 	const struct icst_params *params;
 	void __iomem		*vcoreg;
+	void __iomem		*lockreg;
 #ifdef CONFIG_ARCH_INTEGRATOR
 	struct module		*owner;
-	void			*data;
 #endif
 };
 
@@ -22,8 +22,10 @@ struct clk_ops {
 	void	(*setvco)(struct clk *, struct icst_vco);
 };
 
+void icst_clk_setvco(struct clk *clk, struct icst_vco vco);
 int icst_clk_set(struct clk *, unsigned long);
 long icst_clk_round(struct clk *, unsigned long);
+extern const struct clk_ops icst_clk_default_ops;
 
 #ifdef CONFIG_ARCH_INTEGRATOR
 static inline int __clk_get(struct clk *clk)
-- 
1.7.5.4

[RFC v2 3/9] of: Add of_property_match_string() to find index into a string list

[Grant Likely]

Add a helper function for finding the index of a string in a string
list property.  This helper is useful for bindings that use a separate
*-name property for attaching names to tuples in another property such
as 'reg' or 'gpios'.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
 arch/arm/boot/dts/testcases/tests-phandle.dtsi |    2 +
 drivers/of/base.c                              |   36 ++++++++++++++++++++++++
 drivers/of/selftest.c                          |   29 +++++++++++++++++++
 include/linux/of.h                             |    3 ++
 4 files changed, 70 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/testcases/tests-phandle.dtsi b/arch/arm/boot/dts/testcases/tests-phandle.dtsi
index ec0c4e6..0007d3c 100644
--- a/arch/arm/boot/dts/testcases/tests-phandle.dtsi
+++ b/arch/arm/boot/dts/testcases/tests-phandle.dtsi
@@ -31,6 +31,8 @@
 				phandle-list-bad-phandle = <12345678 0 0>;
 				phandle-list-bad-args = <&provider2 1 0>,
 							<&provider3 0>;
+				empty-property;
+				unterminated-string = [40 41 42 43];
 			};
 		};
 	};
diff --git a/drivers/of/base.c b/drivers/of/base.c
index c6db9ab..ff3c1fb 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -761,6 +761,42 @@ int of_property_read_string_index(struct device_node *np, const char *propname,
 }
 EXPORT_SYMBOL_GPL(of_property_read_string_index);
 
+/**
+ * of_property_match_string() - Find string in a list and return index
+ * @np: pointer to node containing string list property
+ * @propname: string list property name
+ * @string: pointer to string to search for in string list
+ *
+ * This function searches a string list property and returns the index
+ * of a specific string value.
+ */
+int of_property_match_string(struct device_node *np, const char *propname,
+			     const char *string)
+{
+	struct property *prop = of_find_property(np, propname, NULL);
+	size_t l;
+	int i;
+	const char *p, *end;
+
+	if (!prop)
+		return -EINVAL;
+	if (!prop->value)
+		return -ENODATA;
+
+	p = prop->value;
+	end = p + prop->length;
+
+	for (i = 0; p < end; i++, p += l) {
+		l = strlen(p) + 1;
+		if (p + l > end)
+			return -EILSEQ;
+		pr_debug("comparing %s with %s\n", string, p);
+		if (strcmp(string, p) == 0)
+			return i; /* Found it; return index */
+	}
+	return -ENODATA;
+}
+EXPORT_SYMBOL_GPL(of_property_match_string);
 
 /**
  * of_property_count_strings - Find and return the number of strings from a
diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index 9d2b480..f24ffd7 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -120,6 +120,34 @@ static void __init of_selftest_parse_phandle_with_args(void)
 	pr_info("end - %s\n", passed_all ? "PASS" : "FAIL");
 }
 
+static void __init of_selftest_property_match_string(void)
+{
+	struct device_node *np;
+	int rc;
+
+	pr_info("start\n");
+	np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
+	if (!np) {
+		pr_err("No testcase data in device tree\n");
+		return;
+	}
+
+	rc = of_property_match_string(np, "phandle-list-names", "first");
+	selftest(rc == 0, "first expected:0 got:%i\n", rc);
+	rc = of_property_match_string(np, "phandle-list-names", "second");
+	selftest(rc == 1, "second expected:0 got:%i\n", rc);
+	rc = of_property_match_string(np, "phandle-list-names", "third");
+	selftest(rc == 2, "third expected:0 got:%i\n", rc);
+	rc = of_property_match_string(np, "phandle-list-names", "fourth");
+	selftest(rc == -ENODATA, "unmatched string; rc=%i", rc);
+	rc = of_property_match_string(np, "missing-property", "blah");
+	selftest(rc == -EINVAL, "missing property; rc=%i", rc);
+	rc = of_property_match_string(np, "empty-property", "blah");
+	selftest(rc == -ENODATA, "empty property; rc=%i", rc);
+	rc = of_property_match_string(np, "unterminated-string", "blah");
+	selftest(rc == -EILSEQ, "unterminated string; rc=%i", rc);
+}
+
 static int __init of_selftest(void)
 {
 	struct device_node *np;
@@ -133,6 +161,7 @@ static int __init of_selftest(void)
 
 	pr_info("start of selftest - you will see error messages\n");
 	of_selftest_parse_phandle_with_args();
+	of_selftest_property_match_string();
 	pr_info("end of selftest - %s\n", selftest_passed ? "PASS" : "FAIL");
 	return 0;
 }
diff --git a/include/linux/of.h b/include/linux/of.h
index ea44fd7..f02794e 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -218,6 +218,9 @@ extern int of_property_read_string(struct device_node *np,
 extern int of_property_read_string_index(struct device_node *np,
 					 const char *propname,
 					 int index, const char **output);
+extern int of_property_match_string(struct device_node *np,
+				    const char *propname,
+				    const char *string);
 extern int of_property_count_strings(struct device_node *np,
 				     const char *propname);
 extern int of_device_is_compatible(const struct device_node *device,
-- 
1.7.5.4

[RFC v2 4/9] of: add clock providers

[Grant Likely]

Based on work by Ben Herrenschmidt and Jeremy Kerr, this patch adds an
of_clk_get function to allow platforms to retrieve clock data from the
device tree.

Platform register a provider through of_clk_add_provider, which will be
called when a device references the provider's OF node for a clock
reference.

v2: - fixed errant ';' causing compile error
    - Editorial fixes from Shawn Guo
    - merged in adding lookup to clkdev
    - changed property names to match established convention. After
      working with the binding a bit it really made more sense to follow the
      lead of 'reg', 'gpios' and 'interrupts' by making the input simply
      'clocks' & 'clock-names' instead of 'clock-input-*', and to only use
      clock-output* for the producer nodes. (Sorry Shawn, this will mean
      you need to change some code, but it should be trivial)
    - Add ability to inherit clocks from parent nodes by using an empty
      'clock-ranges' property.  Useful for busses.  I could use some feedback
      on the new property name, 'clock-ranges' doesn't feel right to me.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Reviewed-by: Shawn Guo <shawn.guo@freescale.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Mike Turquette <mturquette@ti.com>
---
 .../devicetree/bindings/clock/clock-bindings.txt   |  114 ++++++++++++++
 .../devicetree/bindings/clock/fixed-clock.txt      |   21 +++
 drivers/clk/clkdev.c                               |    9 +
 drivers/of/Kconfig                                 |    6 +
 drivers/of/Makefile                                |    1 +
 drivers/of/clock.c                                 |  165 ++++++++++++++++++++
 include/linux/of_clk.h                             |   37 +++++
 7 files changed, 353 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/clock-bindings.txt
 create mode 100644 Documentation/devicetree/bindings/clock/fixed-clock.txt
 create mode 100644 drivers/of/clock.c
 create mode 100644 include/linux/of_clk.h

diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
new file mode 100644
index 0000000..e40c436
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -0,0 +1,114 @@
+This binding is a work-in-progress, and are based on some experimental
+work by benh[1].
+
+Sources of clock signal can be represented by any node in the device
+tree.  Those nodes are designated as clock providers.  Clock consumer
+nodes use a phandle and clock specifier pair to connect clock provider
+outputs to clock inputs.  Similar to the gpio specifiers, a clock
+specifier is an array of one more more cells identifying the clock
+output on a device.  The length of a clock specifier is defined by the
+value of a #clock-cells property in the clock provider node.
+
+[1] http://patchwork.ozlabs.org/patch/31551/
+
+==Clock providers==
+
+Required properties:
+#clock-cells:	   Number of cells in a clock specifier; typically will be
+		   set to 1
+
+Optional properties:
+clock-output-names: Recommended to be a list of strings of clock output signal
+		    names indexed by the first cell in the clock specifier.
+		    However, the meaning of clock-output-name is domain
+		    specific to the clock provider, and is only provided to
+		    encourage using the same meaning for the majority of clock
+		    providers.  This format may not work for clock providers
+		    using a complex clock specifier format.  In those cases it
+		    is recommended to omit this property and create a binding
+		    specific names property.
+
+		   Clock consumer nodes must never directly reference
+		   the provider's clock-output-name property.
+
+For example:
+
+    oscillator {
+        #clock-cells = <1>;
+        clock-output-names = "ckil", "ckih";
+    };
+
+- this node defines a device with two clock outputs, the first named
+  "ckil" and the second named "ckih".  Consumer nodes always reference
+  clocks by index. The names should reflect the clock output signal
+  names for the device.
+
+==Clock consumers==
+
+Required properties:
+clocks:		List of phandle and clock specifier pairs, one pair
+		for each clock input to the device.
+clock-names:	List of clock input name strings sorted in the same
+		order as the clocks property.  Consumers drivers
+		will use clock-names to match clock input names
+		with clocks specifiers.
+
+Optional properties:
+clock-ranges:	Empty property indicating that child nodes can inherit named
+		clocks from this node. Useful for bus nodes to provide a
+		clock to their children.
+
+For example:
+
+    device {
+        clocks = <&osc 1>, <&ref 0>;
+        clock-names = "baud", "register";
+    };
+
+
+This represents a device with two clock inputs, named "baud" and "register".
+The baud clock is connected to output 1 of the &osc device, and the register
+clock is connected to output 0 of the &ref.
+
+==Example==
+
+    /* external oscillator */
+    osc: oscillator {
+        compatible = "fixed-clock";
+        #clock-cells = <1>;
+        clock-frequency  = <32678>;
+        clock-output-names = "osc";
+    };
+
+    /* phase-locked-loop device, generates a higher frequency clock
+     * from the external oscillator reference */
+    pll: pll@4c000 {
+        compatible = "vendor,some-pll-interface"
+        #clock-cells = <1>;
+        clocks = <&osc 0>;
+        clock-names = "ref";
+        reg = <0x4c000 0x1000>;
+        clock-output-names = "pll", "pll-switched";
+    };
+
+    /* UART, using the low frequency oscillator for the baud clock,
+     * and the high frequency switched PLL output for register
+     * clocking */
+    uart@a000 {
+        compatible = "fsl,imx-uart";
+        reg = <0xa000 0x1000>;
+        interrupts = <33>;
+        clocks = <&osc 0>, <&pll 1>;
+        clock-names = "baud", "register";
+    };
+
+This DT fragment defines three devices: an external oscillator to provide a
+low-frequency reference clock, a PLL device to generate a higher frequency
+clock signal, and a UART.
+
+* The oscillator is fixed-frequency, and provides one clock output, named "osc".
+* The PLL is both a clock provider and a clock consumer. It uses the clock
+  signal generated by the external oscillator, and provides two output signals
+  ("pll" and "pll-switched").
+* The UART has its baud clock connected the external oscillator and its
+  register clock connected to the PLL clock (the "pll-switched" signal)
diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
new file mode 100644
index 0000000..9a75342
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
@@ -0,0 +1,21 @@
+Binding for simple fixed-rate clock sources.
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/fixed-clock.txt
+
+Required properties:
+- compatible : shall be "fixed-clock".
+- #clock-cells : from common clock binding; shall be set to 0.
+- clock-frequency : frequency of clock in Hz. May be multiple cells.
+
+Optional properties:
+- gpios : From common gpio binding; gpio connection to clock enable pin.
+- clock-output-names : From common clock binding
+
+Example:
+	clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <1000000000>;
+	};
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 6db161f..43628d0 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -19,6 +19,8 @@
 #include <linux/mutex.h>
 #include <linux/clk.h>
 #include <linux/clkdev.h>
+#include <linux/of.h>
+#include <linux/of_clk.h>
 
 static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);
@@ -78,6 +80,13 @@ EXPORT_SYMBOL(clk_get_sys);
 struct clk *clk_get(struct device *dev, const char *con_id)
 {
 	const char *dev_id = dev ? dev_name(dev) : NULL;
+	struct clk *clk;
+
+	if (dev) {
+		clk = of_clk_get_by_name(dev->of_node, con_id);
+		if (clk && __clk_get(clk))
+			return clk;
+	}
 
 	return clk_get_sys(dev_id, con_id);
 }
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 268163d..b49ab9c 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -47,6 +47,12 @@ config OF_IRQ
 	def_bool y
 	depends on !SPARC
 
+config OF_CLOCK
+	def_bool y
+	depends on HAVE_CLK
+	help
+	  OpenFirmware clock accessors
+
 config OF_DEVICE
 	def_bool y
 
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index a73f5a5..e7ad1e9 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_OF_ADDRESS)  += address.o
 obj-$(CONFIG_OF_IRQ)    += irq.o
 obj-$(CONFIG_OF_DEVICE) += device.o platform.o
 obj-$(CONFIG_OF_GPIO)   += gpio.o
+obj-$(CONFIG_OF_CLOCK)	+= clock.o
 obj-$(CONFIG_OF_I2C)	+= of_i2c.o
 obj-$(CONFIG_OF_NET)	+= of_net.o
 obj-$(CONFIG_OF_SPI)	+= of_spi.o
diff --git a/drivers/of/clock.c b/drivers/of/clock.c
new file mode 100644
index 0000000..6519e96
--- /dev/null
+++ b/drivers/of/clock.c
@@ -0,0 +1,165 @@
+/*
+ * Clock infrastructure for device tree platforms
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_clk.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+
+/**
+ * struct of_clk_provider - Clock provider registration structure
+ * @link: Entry in global list of clock providers
+ * @node: Pointer to device tree node of clock provider
+ * @get: Get clock callback.  Returns NULL or a struct clk for the
+ *       given clock specifier
+ * @data: context pointer to be passed into @get callback
+ */
+struct of_clk_provider {
+	struct list_head link;
+
+	struct device_node *node;
+	struct clk *(*get)(struct of_phandle_args *clkspec, void *data);
+	void *data;
+};
+
+static LIST_HEAD(of_clk_providers);
+static DEFINE_MUTEX(of_clk_lock);
+
+/**
+ * of_clk_add_provider() - Register a clock provider for a node
+ * @np: Device node pointer associated with clock provider
+ * @clk_src_get: callback for decoding clock
+ * @data: context pointer for @clk_src_get callback.
+ */
+int of_clk_add_provider(struct device_node *np,
+			struct clk *(*clk_src_get)(struct of_phandle_args *clkspec,
+						   void *data),
+			void *data)
+{
+	struct of_clk_provider *cp;
+
+	cp = kzalloc(sizeof(struct of_clk_provider), GFP_KERNEL);
+	if (!cp)
+		return -ENOMEM;
+
+	cp->node = of_node_get(np);
+	cp->data = data;
+	cp->get = clk_src_get;
+
+	mutex_lock(&of_clk_lock);
+	list_add(&cp->link, &of_clk_providers);
+	mutex_unlock(&of_clk_lock);
+	pr_debug("Added clock from %s\n", np->full_name);
+
+	return 0;
+}
+
+/**
+ * of_clk_del_provider() - Remove a previously registered clock provider
+ * @np: Device node pointer associated with clock provider
+ */
+void of_clk_del_provider(struct device_node *np)
+{
+	struct of_clk_provider *cp;
+
+	mutex_lock(&of_clk_lock);
+	list_for_each_entry(cp, &of_clk_providers, link) {
+		if (cp->node == np) {
+			list_del(&cp->link);
+			of_node_put(cp->node);
+			kfree(cp);
+			break;
+		}
+	}
+	mutex_unlock(&of_clk_lock);
+}
+
+static struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec)
+{
+	struct of_clk_provider *provider;
+	struct clk *clk = NULL;
+
+	/* Check if we have such a provider in our array */
+	mutex_lock(&of_clk_lock);
+	list_for_each_entry(provider, &of_clk_providers, link) {
+		if (provider->node == clkspec->np)
+			clk = provider->get(clkspec, provider->data);
+		if (clk)
+			break;
+	}
+	mutex_unlock(&of_clk_lock);
+
+	return clk;
+}
+
+struct clk *of_clk_get(struct device_node *np, int index)
+{
+	struct of_phandle_args clkspec;
+	struct clk *clk;
+	int rc;
+
+	if (index < 0)
+		return NULL;
+
+	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
+					&clkspec);
+	if (rc)
+		return NULL;
+
+	clk = __of_clk_get_from_provider(&clkspec);
+	of_node_put(clkspec.np);
+	return clk;
+}
+
+/**
+ * of_clk_get_by_name() - Parse and lookup a clock referenced by a device node
+ * @np: pointer to clock consumer node
+ * @name: name of consumer's clock input, or NULL for the first clock reference
+ *
+ * This function parses the clocks and clock-names properties,
+ * and uses them to look up the struct clk from the registered list of clock
+ * providers.
+ */
+struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
+{
+	struct clk *clk = NULL;
+
+	/* Walk up the tree of devices looking for a clock that matches */
+	while (np) {
+		int index = 0;
+
+		/*
+		 * For named clocks, first look up the name in the
+		 * "clock-names" property.  If it cannot be found, then
+		 * index will be an error code, and of_clk_get() will fail.
+		 */
+		if (name)
+			index = of_property_match_string(np, "clock-names", name);
+		clk = of_clk_get(np, index);
+		if (clk)
+			break;
+		else if (name && index >= 0) {
+			pr_err("ERROR: could not get clock %s:%s(%i)\n",
+				 np->full_name, name ? name : "", index);
+			return NULL;
+		}
+
+		/*
+		 * No matching clock found on this node.  If the parent node
+		 * has a "clock-ranges" property, then we can try one of its
+		 * clocks.
+		 */
+		np = np->parent;
+		if (np && !of_get_property(np, "clock-ranges", NULL))
+			break;
+	}
+
+	return clk;
+}
diff --git a/include/linux/of_clk.h b/include/linux/of_clk.h
new file mode 100644
index 0000000..dcbd27b
--- /dev/null
+++ b/include/linux/of_clk.h
@@ -0,0 +1,37 @@
+/*
+ * Clock infrastructure for device tree platforms
+ */
+#ifndef __OF_CLK_H
+#define __OF_CLK_H
+
+struct device;
+struct clk;
+
+#ifdef CONFIG_OF_CLOCK
+
+struct device_node;
+
+int of_clk_add_provider(struct device_node *np,
+			struct clk *(*clk_src_get)(struct of_phandle_args *args,
+						   void *data),
+			void *data);
+
+void of_clk_del_provider(struct device_node *np);
+
+struct clk *of_clk_get(struct device_node *np, int index);
+struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
+
+#else
+static struct clk *of_clk_get(struct device_node *np, int index)
+{
+	return NULL;
+}
+
+static struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
+{
+	return NULL;
+}
+#endif
+
+#endif /* __OF_CLK_H */
+
-- 
1.7.5.4

[RFC v2 5/9] dt/clock: Add handling for fixed clocks and a clock node setup iterator

[Grant Likely]

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/of/clock.c     |   43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_clk.h |    4 ++++
 2 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/drivers/of/clock.c b/drivers/of/clock.c
index 6519e96..1c189b4 100644
--- a/drivers/of/clock.c
+++ b/drivers/of/clock.c
@@ -3,6 +3,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clkdev.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/module.h>
@@ -163,3 +164,45 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
 
 	return clk;
 }
+
+/**
+ * of_clk_init() - Scan and init clock providers from the DT
+ * @matches: array of compatible values and init functions for providers.
+ *
+ * This function scans the device tree for matching clock providers and
+ * calls their initialization functions
+ */
+void __init of_clk_init(const struct of_device_id *matches)
+{
+	struct device_node *np;
+
+	for_each_matching_node(np, matches) {
+		const struct of_device_id *match = of_match_node(matches, np);
+		of_clk_init_cb_t clk_init_cb = match->data;
+		clk_init_cb(np);
+	}
+}
+
+static struct clk *of_fixed_clk_get(struct of_phandle_args *a, void *data)
+{
+	return data;
+}
+
+/**
+ * of_fixed_clk_setup() - Setup function for simple fixed rate clock
+ */
+void __init of_fixed_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+	u32 rate;
+
+	if (of_property_read_u32(node, "clock-frequency", &rate))
+		return;
+
+	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
+	if (!clk)
+		return;
+	clk->rate = rate;
+
+	of_clk_add_provider(node, of_fixed_clk_get, clk);
+}
diff --git a/include/linux/of_clk.h b/include/linux/of_clk.h
index dcbd27b..02ecc98 100644
--- a/include/linux/of_clk.h
+++ b/include/linux/of_clk.h
@@ -10,6 +10,7 @@ struct clk;
 #ifdef CONFIG_OF_CLOCK
 
 struct device_node;
+typedef void (*of_clk_init_cb_t)(struct device_node *);
 
 int of_clk_add_provider(struct device_node *np,
 			struct clk *(*clk_src_get)(struct of_phandle_args *args,
@@ -21,6 +22,9 @@ void of_clk_del_provider(struct device_node *np);
 struct clk *of_clk_get(struct device_node *np, int index);
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
 
+void of_clk_init(const struct of_device_id *matches);
+extern void of_fixed_clk_setup(struct device_node *np);
+
 #else
 static struct clk *of_clk_get(struct device_node *np, int index)
 {
-- 
1.7.5.4

[RFC v2 6/9] arm/dt: add devicetree support to sp804 timer support

[Grant Likely]

This patch adds support to the sp804 code for retrieving timer
configuration from the device tree.  sp804 channels can be used as
a clock event device or a clock source.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Russell King <linux@arm.linux.org.uk>
---
 arch/arm/common/timer-sp.c               |   72 ++++++++++++++++++++++++++---
 arch/arm/include/asm/hardware/timer-sp.h |    2 +
 2 files changed, 66 insertions(+), 8 deletions(-)

diff --git a/arch/arm/common/timer-sp.c b/arch/arm/common/timer-sp.c
index 2393b5b..93aed48 100644
--- a/arch/arm/common/timer-sp.c
+++ b/arch/arm/common/timer-sp.c
@@ -25,16 +25,18 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_clk.h>
+#include <linux/of_irq.h>
 
 #include <asm/hardware/arm_timer.h>
 
-static long __init sp804_get_clock_rate(const char *name)
+static long __init sp804_get_clock_rate(struct clk *clk, const char *name)
 {
-	struct clk *clk;
 	long rate;
 	int err;
 
-	clk = clk_get_sys("sp804", name);
 	if (IS_ERR(clk)) {
 		pr_err("sp804: %s clock not found: %d\n", name,
 			(int)PTR_ERR(clk));
@@ -67,9 +69,10 @@ static long __init sp804_get_clock_rate(const char *name)
 	return rate;
 }
 
-void __init sp804_clocksource_init(void __iomem *base, const char *name)
+static void __init __sp804_clocksource_init(void __iomem *base,
+					const char *name, struct clk *clk)
 {
-	long rate = sp804_get_clock_rate(name);
+	long rate = sp804_get_clock_rate(clk, name);
 
 	if (rate < 0)
 		return;
@@ -85,6 +88,10 @@ void __init sp804_clocksource_init(void __iomem *base, const char *name)
 		rate, 200, 32, clocksource_mmio_readl_down);
 }
 
+void __init sp804_clocksource_init(void __iomem *base, const char *name)
+{
+	__sp804_clocksource_init(base, name, clk_get_sys("sp804", name));
+}
 
 static void __iomem *clkevt_base;
 static unsigned long clkevt_reload;
@@ -158,11 +165,11 @@ static struct irqaction sp804_timer_irq = {
 	.dev_id		= &sp804_clockevent,
 };
 
-void __init sp804_clockevents_init(void __iomem *base, unsigned int irq,
-	const char *name)
+static void __init __sp804_clockevents_init(void __iomem *base,
+			unsigned int irq, const char *name, struct clk *clk)
 {
 	struct clock_event_device *evt = &sp804_clockevent;
-	long rate = sp804_get_clock_rate(name);
+	long rate = sp804_get_clock_rate(clk, name);
 
 	if (rate < 0)
 		return;
@@ -179,3 +186,52 @@ void __init sp804_clockevents_init(void __iomem *base, unsigned int irq,
 	setup_irq(irq, &sp804_timer_irq);
 	clockevents_register_device(evt);
 }
+
+void __init sp804_clockevents_init(void __iomem *base, unsigned int irq,
+	const char *name)
+{
+	__sp804_clockevents_init(base, irq, name, clk_get_sys("sp804", name));
+}
+
+#ifdef CONFIG_OF
+void __init sp804_dt_setup(struct device_node *node)
+{
+	struct clk *clk;
+	unsigned int irq;
+	__iomem void * base;
+
+	pr_debug("%s(): Setting up sp804 clock %s\n", __func__, node->name);
+
+	/* Find the base address of the clock */
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("ERROR: Cannot get base address of sp804 %s\n",
+			node->full_name);
+		return;
+	}
+
+	writel(0, base + TIMER_CTRL); /* Disable the clock */
+
+	clk = of_clk_get(node, 0);
+	if (!clk) {
+		pr_err("ERROR: Cannot get clock for sp804 %s\n", node->name);
+		return;
+	}
+
+	/*
+	 * Figure out how to use this clock
+	 *
+	 * Note: This is kind of ugly since it requires linux-specific
+	 * properties in the device tree so that Linux knows which sp804
+	 * channels can be used as the clock source and the clock events
+	 * trigger.  Something OS agnostic would be nicer, but it isn't
+	 * obvious what that should look like.
+	 */
+	if (of_get_property(node, "linux,clock-source", NULL)) {
+		__sp804_clocksource_init(base, node->full_name, clk);
+	} else if (of_get_property(node, "linux,clockevents-device", NULL)) {
+		irq = irq_of_parse_and_map(node, 0);
+		__sp804_clockevents_init(base, irq, node->full_name, clk);
+	};
+}
+#endif /* CONFIG_OF */
diff --git a/arch/arm/include/asm/hardware/timer-sp.h b/arch/arm/include/asm/hardware/timer-sp.h
index 4384d81..edd0ec3 100644
--- a/arch/arm/include/asm/hardware/timer-sp.h
+++ b/arch/arm/include/asm/hardware/timer-sp.h
@@ -1,2 +1,4 @@
+struct device_node;
 void sp804_clocksource_init(void __iomem *, const char *);
 void sp804_clockevents_init(void __iomem *, unsigned int, const char *);
+void sp804_dt_setup(struct device_node *node);
-- 
1.7.5.4

[RFC v2 7/9] arm/dt: Common plat-versatile support for icst and sp804 based system clocks

[Grant Likely]

Signed-off-by: <grant.likely@secretlab.ca>
Cc: Russell King <linux@arm.linux.org.uk>
---
 arch/arm/plat-versatile/clock.c              |  130 ++++++++++++++++++++++++++
 arch/arm/plat-versatile/include/plat/clock.h |    3 +
 2 files changed, 133 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-versatile/clock.c b/arch/arm/plat-versatile/clock.c
index 98a9dd8..2b0682a 100644
--- a/arch/arm/plat-versatile/clock.c
+++ b/arch/arm/plat-versatile/clock.c
@@ -13,9 +13,15 @@
 #include <linux/errno.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_clk.h>
+#include <linux/slab.h>
 #include <linux/mutex.h>
 
 #include <asm/hardware/icst.h>
+#include <asm/hardware/timer-sp.h>
+#include <asm/mach/time.h>
 
 #include <mach/clkdev.h>
 
@@ -93,3 +99,127 @@ const struct clk_ops icst_clk_default_ops = {
 	.setvco	= icst_clk_setvco,
 };
 EXPORT_SYMBOL(icst_clk_default_ops);
+
+#ifdef CONFIG_OF
+static struct icst_params __initdata icst307_default_params = {
+	.vco_max = ICST307_VCO_MAX,
+	.vco_min = ICST307_VCO_MIN,
+	.s2div = icst307_s2div,
+	.idx2s = icst307_idx2s,
+};
+
+static struct icst_params __initdata icst525_5v_default_params = {
+	.vco_max = ICST525_VCO_MAX_5V,
+	.vco_min = ICST525_VCO_MIN,
+	.s2div = icst525_s2div,
+	.idx2s = icst525_idx2s,
+};
+
+static struct icst_params __initdata icst525_3v_default_params = {
+	.vco_max = ICST525_VCO_MAX_3V,
+	.vco_min = ICST525_VCO_MIN,
+	.s2div = icst525_s2div,
+	.idx2s = icst525_idx2s,
+};
+
+struct icst_of_clk {
+	struct clk clk;
+	struct clk ref;
+	struct icst_params params;
+};
+
+static struct clk *icst_clk_src_get(struct of_phandle_args *args, void *data)
+{
+	struct icst_of_clk *icst = data;
+	return &icst->clk;
+}
+
+struct of_device_id versatile_dt_icst_match[] __initdata = {
+	{ .compatible="idt,ics307", .data=&icst307_default_params, },
+	{ .compatible="idt,ics525-5v", .data=&icst525_5v_default_params, },
+	{ .compatible="idt,ics525-3.3v", .data=&icst525_3v_default_params, },
+	{ /* sentinel */ }
+};
+
+void __init versatile_dt_icst_clk_setup(struct device_node *np)
+{
+	const struct of_device_id *match;
+	struct icst_of_clk *icst;
+	const struct icst_params *params;
+	u32 ref, vd_range[2], rd_range[2], vco_offset, lock_offset;
+	__iomem void * base;
+
+	/*
+	 * Only try to drive clocks that we know how to control.
+	 * ie. attached to an ARM Versatile block of system registers
+	 */
+	if (!of_device_is_compatible(np->parent, "arm,versatile-sys-regs"))
+		return;
+
+	match = of_match_node(versatile_dt_icst_match, np);
+	if (!match)
+		return;
+	params = match->data;
+
+	icst = kzalloc(sizeof(*icst), GFP_KERNEL);
+	if (!icst)
+		return;
+	icst->params = *params; /* Copy the default params */
+
+	base = of_iomap(np->parent, 0);
+	if (!base || of_property_read_u32(np, "idt,ref-clock-freq", &ref) ||
+	    of_property_read_u32(np, "arm,reg-vco-offset", &vco_offset) ||
+	    of_property_read_u32(np, "arm,reg-lock-offset", &lock_offset) ||
+	    of_property_read_u32_array(np, "idt,vco-div-range", vd_range, 2) ||
+	    of_property_read_u32_array(np, "idt,ref-div-range", rd_range, 2)) {
+		pr_err("ERROR: Unable to set up clock %s\n", np->full_name);
+		goto err_setup;
+	}
+
+	icst->params.ref = ref;
+	icst->params.vd_min = vd_range[0];
+	icst->params.vd_max = vd_range[1];
+	icst->params.rd_min = rd_range[0];
+	icst->params.rd_max = rd_range[1];
+	icst->clk.ops = &icst_clk_default_ops;
+	icst->clk.vcoreg = base + vco_offset;
+	icst->clk.lockreg = base + lock_offset;
+	icst->clk.params = &icst->params;
+
+	icst->ref.rate = ref;
+
+	if (of_clk_add_provider(np, icst_clk_src_get, icst))
+		goto err_setup;
+
+	return;
+
+ err_setup:
+	kfree(icst);
+}
+
+static struct of_device_id versatile_dt_clk_match_table[] __initdata = {
+	{ .compatible = "idt,ics307", .data = versatile_dt_icst_clk_setup, },
+	{ .compatible = "idt,ics525", .data = versatile_dt_icst_clk_setup, },
+	{ .compatible = "fixed-clock", .data = of_fixed_clk_setup, },
+	{}
+};
+
+/*
+ * Set up timer interrupt, and return the current time in seconds.
+ */
+static void __init versatile_dt_timer_init(void)
+{
+	struct device_node *np;
+
+	of_clk_init(versatile_dt_clk_match_table);
+
+	pr_debug("%s(): Looking for sp804 clocks\n", __func__);
+	for_each_compatible_node(np, NULL, "arm,sp804")
+		sp804_dt_setup(np);
+}
+
+struct sys_timer versatile_dt_timer = {
+	.init		= versatile_dt_timer_init,
+};
+
+#endif
diff --git a/arch/arm/plat-versatile/include/plat/clock.h b/arch/arm/plat-versatile/include/plat/clock.h
index 5749f79..ca81bfe 100644
--- a/arch/arm/plat-versatile/include/plat/clock.h
+++ b/arch/arm/plat-versatile/include/plat/clock.h
@@ -42,5 +42,8 @@ static inline void __clk_put(struct clk *clk)
 #define __clk_put(clk) do { } while (0)
 #endif
 
+#ifdef CONFIG_OF
+extern struct sys_timer versatile_dt_timer;
+#endif
 
 #endif
-- 
1.7.5.4

[RFC v2 8/9] dt/arm: versatile add clock parsing

[Grant Likely]

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Mike Turquette <mturquette@ti.com>
---
 arch/arm/boot/dts/versatile-ab.dts     |   79 ++++++++++++++++++++++++++++++++
 arch/arm/mach-versatile/core.c         |   39 +--------------
 arch/arm/mach-versatile/versatile_dt.c |   39 +++++++++++++++-
 3 files changed, 119 insertions(+), 38 deletions(-)

diff --git a/arch/arm/boot/dts/versatile-ab.dts b/arch/arm/boot/dts/versatile-ab.dts
index 0b32925..d29fc2b 100644
--- a/arch/arm/boot/dts/versatile-ab.dts
+++ b/arch/arm/boot/dts/versatile-ab.dts
@@ -8,6 +8,10 @@
 	#size-cells = <1>;
 	interrupt-parent = <&vic>;
 
+	clocks = <&ref24_clk>;
+	clock-names = "apb_pclk";
+	clock-ranges;
+
 	aliases {
 		serial0 = &uart0;
 		serial1 = &uart1;
@@ -19,6 +23,42 @@
 		reg = <0x0 0x08000000>;
 	};
 
+	clocks {
+		ref24_clk: ref24clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <24000000>;
+		};
+
+		tim_clk: timclk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <1000000>;
+		};
+
+		ref_clk: refclk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <32000>;
+		};
+	};
+
+	sys@10000000 {
+		compatible = "arm,versatile-sys-regs";
+		reg = <0x10000000 0x100>;
+
+		osc4_clk: clock {
+			compatible = "idt,ics307";
+			#clock-cells = <1>;
+			idt,ref-clock-freq = <24000000>;
+			idt,vco-div-range = <12 519>;
+			idt,ref-div-range = <3 129>;
+
+			arm,reg-vco-offset = <0x1c>;
+			arm,reg-lock-offset = <0x20>;
+		};
+	};
+
 	flash@34000000 {
 		compatible = "arm,versatile-flash";
 		reg = <0x34000000 0x4000000>;
@@ -53,6 +93,7 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges;
+		clock-ranges;
 
 		vic: intc@10140000 {
 			compatible = "arm,versatile-vic";
@@ -108,6 +149,8 @@
 			compatible = "arm,pl110", "arm,primecell";
 			reg = <0x10120000 0x1000>;
 			interrupts = <16>;
+			clocks = <&osc4_clk 0>;
+			clock-names = "apb_pclk";
 		};
 
 		sctl@101e0000 {
@@ -121,6 +164,36 @@
 			interrupts = <0>;
 		};
 
+		timer@101e2000 {
+			compatible = "arm,sp804";
+			reg = <0x101e2000 0x1000>;
+			clocks = <&tim_clk>;
+			interrupts = <4>;
+			linux,clockevents-device;
+		};
+
+		timer@101e2020 {
+			compatible = "arm,sp804";
+			reg = <0x101e2020 0x1000>;
+			clocks = <&tim_clk>;
+			interrupts = <4>;
+		};
+
+		timer@101e3000 {
+			compatible = "arm,sp804";
+			reg = <0x101e3000 0x1000>;
+			clocks = <&tim_clk>;
+			interrupts = <5>;
+			linux,clock-source;
+		};
+
+		timer@101e3020 {
+			compatible = "arm,sp804";
+			reg = <0x101e3020 0x1000>;
+			clocks = <&tim_clk>;
+			interrupts = <5>;
+		};
+
 		gpio0: gpio@101e4000 {
 			compatible = "arm,pl061", "arm,primecell";
 			reg = <0x101e4000 0x1000>;
@@ -165,6 +238,8 @@
 			#size-cells = <1>;
 			ranges = <0 0x10000000 0x10000>;
 
+			clock-ranges;
+
 			aaci@4000 {
 				compatible = "arm,primecell";
 				reg = <0x4000 0x1000>;
@@ -180,12 +255,16 @@
 				reg = <0x6000 0x1000>;
 				interrupt-parent = <&sic>;
 				interrupts = <3>;
+				clocks = <&ref24_clk>;
+				clock-names = "KMIREFCLK";
 			};
 			kmi@7000 {
 				compatible = "arm,pl050", "arm,primecell";
 				reg = <0x7000 0x1000>;
 				interrupt-parent = <&sic>;
 				interrupts = <4>;
+				clocks = <&ref24_clk>;
+				clock-names = "KMIREFCLK";
 			};
 		};
 	};
diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
index 47f4531..7b4e00a 100644
--- a/arch/arm/mach-versatile/core.c
+++ b/arch/arm/mach-versatile/core.c
@@ -26,6 +26,7 @@
 #include <linux/interrupt.h>
 #include <linux/irqdomain.h>
 #include <linux/of_address.h>
+#include <linux/of_clk.h>
 #include <linux/of_platform.h>
 #include <linux/amba/bus.h>
 #include <linux/amba/clcd.h>
@@ -647,44 +648,11 @@ static struct amba_device *amba_devs[] __initdata = {
 /*
  * Lookup table for attaching a specific name and platform_data pointer to
  * devices as they get created by of_platform_populate().  Ideally this table
- * would not exist, but the current clock implementation depends on some devices
- * having a specific name.
+ * would not exist, but some devices still need platform data.
  */
 struct of_dev_auxdata versatile_auxdata_lookup[] __initdata = {
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_MMCI0_BASE, "fpga:05", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_KMI0_BASE, "fpga:06", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_KMI1_BASE, "fpga:07", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_UART3_BASE, "fpga:09", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_MMCI1_BASE, "fpga:0b", NULL),
-
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_CLCD_BASE, "dev:20", &clcd_plat_data),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_UART0_BASE, "dev:f1", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_UART1_BASE, "dev:f2", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_UART2_BASE, "dev:f3", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_SSP_BASE, "dev:f4", NULL),
 
-#if 0
-	/*
-	 * These entries are unnecessary because no clocks referencing
-	 * them.  I've left them in for now as place holders in case
-	 * any of them need to be added back, but they should be
-	 * removed before actually committing this patch.  --gcl
-	 */
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_AACI_BASE, "fpga:04", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_SCI1_BASE, "fpga:0a", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_SMC_BASE, "dev:00", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_MPMC_BASE, "dev:10", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_DMAC_BASE, "dev:30", NULL),
-
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_SCTL_BASE, "dev:e0", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_WATCHDOG_BASE, "dev:e1", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_GPIO0_BASE, "dev:e4", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_GPIO1_BASE, "dev:e5", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_GPIO2_BASE, "dev:e6", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_GPIO3_BASE, "dev:e7", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_RTC_BASE, "dev:e8", NULL),
-	OF_DEV_AUXDATA("arm,primecell", VERSATILE_SCI_BASE, "dev:f0", NULL),
-#endif
+	OF_DEV_AUXDATA("arm,primecell", VERSATILE_CLCD_BASE, NULL, &clcd_plat_data),
 	{}
 };
 #endif
@@ -799,4 +767,3 @@ static void __init versatile_timer_init(void)
 struct sys_timer versatile_timer = {
 	.init		= versatile_timer_init,
 };
-
diff --git a/arch/arm/mach-versatile/versatile_dt.c b/arch/arm/mach-versatile/versatile_dt.c
index 54e037c..26c9705 100644
--- a/arch/arm/mach-versatile/versatile_dt.c
+++ b/arch/arm/mach-versatile/versatile_dt.c
@@ -21,14 +21,49 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#include <linux/io.h>
 #include <linux/init.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/of_clk.h>
+#include <linux/slab.h>
+
+#include <plat/sched_clock.h>
+#include <mach/hardware.h>
+#include <mach/clkdev.h>
+#include <asm/irq.h>
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
 
 #include "core.h"
 
+static void __init versatile_dt_init_early(void)
+{
+	void __iomem *sys = __io_address(VERSATILE_SYS_BASE);
+	u32 val;
+
+	/*
+	 * TODO: DT generalization of early init code; the ARM Versatile,
+	 * Realview, Integrator, and VExpress boards are pretty similar here.
+	 * All call versatile_sched_clock_init(), and need to configure the
+	 * correct clock input to the sp804 timers
+	 */
+	versatile_sched_clock_init(sys + VERSATILE_SYS_24MHz_OFFSET, 24000000);
+
+	/*
+	 * set clock frequency to the sp804 timers:
+	 * VERSATILE_REFCLK is 32KHz
+	 * VERSATILE_TIMCLK is 1MHz
+	 */
+	sys = __io_address(VERSATILE_SCTL_BASE);
+	val = readl(sys);
+	val |= (VERSATILE_TIMCLK << VERSATILE_TIMER1_EnSel);
+	val |= (VERSATILE_TIMCLK << VERSATILE_TIMER2_EnSel);
+	val |= (VERSATILE_TIMCLK << VERSATILE_TIMER3_EnSel);
+	val |= (VERSATILE_TIMCLK << VERSATILE_TIMER4_EnSel);
+	writel(val, sys);
+}
+
 static void __init versatile_dt_init(void)
 {
 	of_platform_populate(NULL, of_default_bus_match_table,
@@ -43,9 +78,9 @@ static const char *versatile_dt_match[] __initconst = {
 
 DT_MACHINE_START(VERSATILE_PB, "ARM-Versatile (Device Tree Support)")
 	.map_io		= versatile_map_io,
-	.init_early	= versatile_init_early,
+	.init_early	= versatile_dt_init_early,
 	.init_irq	= versatile_init_irq,
-	.timer		= &versatile_timer,
+	.timer		= &versatile_dt_timer,
 	.init_machine	= versatile_dt_init,
 	.dt_compat	= versatile_dt_match,
 MACHINE_END
-- 
1.7.5.4

[RFC v2 9/9] arm/highbank: Use clock binding common support code

[Grant Likely]

This patch makes the Calxeda Highbank platform use the common DT clock
binding support code for setting up timers and fixed clocks.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
---
 arch/arm/Kconfig                             |    1 +
 arch/arm/boot/dts/highbank.dts               |   32 ++++++++++++++++++++++++-
 arch/arm/mach-highbank/clock.c               |   19 ---------------
 arch/arm/mach-highbank/core.h                |    1 -
 arch/arm/mach-highbank/highbank.c            |   13 +---------
 arch/arm/mach-highbank/include/mach/clkdev.h |   11 +++++++++
 6 files changed, 44 insertions(+), 33 deletions(-)
 create mode 100644 arch/arm/mach-highbank/include/mach/clkdev.h

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index e084b7e..33c2958 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -341,6 +341,7 @@ config ARCH_HIGHBANK
 	select ARM_GIC
 	select ARM_TIMER_SP804
 	select CLKDEV_LOOKUP
+	select HAVE_MACH_CLKDEV
 	select CPU_V7
 	select GENERIC_CLOCKEVENTS
 	select HAVE_ARM_SCU
diff --git a/arch/arm/boot/dts/highbank.dts b/arch/arm/boot/dts/highbank.dts
index aeb1a75..a60e41d 100644
--- a/arch/arm/boot/dts/highbank.dts
+++ b/arch/arm/boot/dts/highbank.dts
@@ -25,6 +25,10 @@
 	#address-cells = <1>;
 	#size-cells = <1>;
 
+	clocks = <&pclk>;
+	clock-names = "apb_pclk";
+	clock-ranges;
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
@@ -54,6 +58,20 @@
 		};
 	};
 
+	clocks {
+		eclk: eclk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <200000000>;
+		};
+
+		pclk: pclk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <150000000>;
+		};
+	};
+
 	memory {
 		name = "memory";
 		device_type = "memory";
@@ -70,6 +88,7 @@
 		compatible = "simple-bus";
 		interrupt-parent = <&intc>;
 		ranges;
+		clock-ranges;
 
 		timer@fff10600 {
 			compatible = "arm,smp-twd";
@@ -117,6 +136,7 @@
 			compatible = "calxeda,hb-sdhci";
 			reg = <0xffe0e000 0x1000>;
 			interrupts = <0 90 4>;
+			clocks = <&eclk>;
 		};
 
 		ipc@fff20000 {
@@ -157,10 +177,18 @@
 			interrupts = <0 17 4>;
 		};
 
-		timer {
+		timer@fff34000 {
+			compatible = "arm,sp804", "arm,primecell";
+			reg = <0xfff34000 0x20>;
+			interrupts = <0 18 4>;
+			linux,clockevents-device;
+		};
+
+		timer@fff34020 {
 			compatible = "arm,sp804", "arm,primecell";
-			reg = <0xfff34000 0x1000>;
+			reg = <0xfff34020 0x20>;
 			interrupts = <0 18 4>;
+			linux,clock-source;
 		};
 
 		rtc@fff35000 {
diff --git a/arch/arm/mach-highbank/clock.c b/arch/arm/mach-highbank/clock.c
index c25a2ae..96f4599 100644
--- a/arch/arm/mach-highbank/clock.c
+++ b/arch/arm/mach-highbank/clock.c
@@ -19,10 +19,6 @@
 #include <linux/clk.h>
 #include <linux/clkdev.h>
 
-struct clk {
-	unsigned long rate;
-};
-
 int clk_enable(struct clk *clk)
 {
 	return 0;
@@ -45,18 +41,3 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 {
 	return 0;
 }
-
-static struct clk eclk = { .rate = 200000000 };
-static struct clk pclk = { .rate = 150000000 };
-
-static struct clk_lookup lookups[] = {
-	{ .clk = &pclk, .con_id = "apb_pclk", },
-	{ .clk = &pclk, .dev_id = "sp804", },
-	{ .clk = &eclk, .dev_id = "ffe0e000.sdhci", },
-	{ .clk = &pclk, .dev_id = "fff36000.serial", },
-};
-
-void __init highbank_clocks_init(void)
-{
-	clkdev_add_table(lookups, ARRAY_SIZE(lookups));
-}
diff --git a/arch/arm/mach-highbank/core.h b/arch/arm/mach-highbank/core.h
index 7e33fc9..490f430 100644
--- a/arch/arm/mach-highbank/core.h
+++ b/arch/arm/mach-highbank/core.h
@@ -1,5 +1,4 @@
 extern void highbank_set_cpu_jump(int cpu, void *jump_addr);
-extern void highbank_clocks_init(void);
 extern void __iomem *scu_base_addr;
 #ifdef CONFIG_DEBUG_HIGHBANK_UART
 extern void highbank_lluart_map_io(void);
diff --git a/arch/arm/mach-highbank/highbank.c b/arch/arm/mach-highbank/highbank.c
index 88660d5..13a3b15 100644
--- a/arch/arm/mach-highbank/highbank.c
+++ b/arch/arm/mach-highbank/highbank.c
@@ -95,24 +95,15 @@ static void __init highbank_init_irq(void)
 
 static void __init highbank_timer_init(void)
 {
-	int irq;
 	struct device_node *np;
-	void __iomem *timer_base;
 
 	/* Map system registers */
 	np = of_find_compatible_node(NULL, NULL, "calxeda,hb-sregs");
 	sregs_base = of_iomap(np, 0);
 	WARN_ON(!sregs_base);
 
-	np = of_find_compatible_node(NULL, NULL, "arm,sp804");
-	timer_base = of_iomap(np, 0);
-	WARN_ON(!timer_base);
-	irq = irq_of_parse_and_map(np, 0);
-
-	highbank_clocks_init();
-
-	sp804_clocksource_init(timer_base + 0x20, "timer1");
-	sp804_clockevents_init(timer_base, irq, "timer0");
+	for_each_compatible_node(np, NULL, "arm,sp804")
+		sp804_dt_setup(np);
 }
 
 static struct sys_timer highbank_timer = {
diff --git a/arch/arm/mach-highbank/include/mach/clkdev.h b/arch/arm/mach-highbank/include/mach/clkdev.h
new file mode 100644
index 0000000..0fe8f0e
--- /dev/null
+++ b/arch/arm/mach-highbank/include/mach/clkdev.h
@@ -0,0 +1,11 @@
+#ifndef __ASM_MACH_CLKDEV_H
+#define __ASM_MACH_CLKDEV_H
+
+struct clk {
+	unsigned long rate;
+};
+
+#define __clk_get(clk)	({ 1; })
+#define __clk_put(clk)	do { } while (0)
+
+#endif
-- 
1.7.5.4

Re: [RFC v2 4/9] of: add clock providers

[Jamie Iles]

Hi Grant,

I'm still going through these and trying to digest them but a couple of 
quick questions/comments.

Jamie

On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> new file mode 100644
> index 0000000..e40c436
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -0,0 +1,114 @@
> +This binding is a work-in-progress, and are based on some experimental
> +work by benh[1].
> +
> +Sources of clock signal can be represented by any node in the device
> +tree.  Those nodes are designated as clock providers.  Clock consumer
> +nodes use a phandle and clock specifier pair to connect clock provider
> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
> +specifier is an array of one more more cells identifying the clock
> +output on a device.  The length of a clock specifier is defined by the
> +value of a #clock-cells property in the clock provider node.
> +
> +[1] http://patchwork.ozlabs.org/patch/31551/
> +
> +==Clock providers==
> +
> +Required properties:
> +#clock-cells:	   Number of cells in a clock specifier; typically will be
> +		   set to 1

I'm not sure I fully understand what the extra cells actually mean for 
clocks.  I think the first integer is the clock output to use but some 
of the versatile and highbank ones only have a phandle or is it more 
implementation defined?  The clock-output-names description hints at 
recommended, so I find this a little confusing, but that could just be 
me!

> +Optional properties:
> +clock-output-names: Recommended to be a list of strings of clock output signal
> +		    names indexed by the first cell in the clock specifier.
> +		    However, the meaning of clock-output-name is domain
> +		    specific to the clock provider, and is only provided to
> +		    encourage using the same meaning for the majority of clock
> +		    providers.  This format may not work for clock providers
> +		    using a complex clock specifier format.  In those cases it
> +		    is recommended to omit this property and create a binding
> +		    specific names property.
> +
> +		   Clock consumer nodes must never directly reference
> +		   the provider's clock-output-name property.
> +
> +For example:
> +
> +    oscillator {
> +        #clock-cells = <1>;
> +        clock-output-names = "ckil", "ckih";
> +    };
> +
> +- this node defines a device with two clock outputs, the first named
> +  "ckil" and the second named "ckih".  Consumer nodes always reference
> +  clocks by index. The names should reflect the clock output signal
> +  names for the device.
> +
> +==Clock consumers==
> +
> +Required properties:
> +clocks:		List of phandle and clock specifier pairs, one pair
> +		for each clock input to the device.

Some of the highbank and versatile devicetree nodes have clocks 
properties that aren't a pair e.g. versatile timer has
"clocks = <&tim_clk>;".  

> +clock-names:	List of clock input name strings sorted in the same
> +		order as the clocks property.  Consumers drivers
> +		will use clock-names to match clock input names
> +		with clocks specifiers.

The versatile and highbank patches appears to omit this required 
property in several nodes.  So is this really optional?

Jamie

Re: [RFC v2 6/9] arm/dt: add devicetree support to sp804 timer support

[Rob Herring]

Grant,

On 12/12/2011 04:02 PM, Grant Likely wrote:
> This patch adds support to the sp804 code for retrieving timer
> configuration from the device tree.  sp804 channels can be used as
> a clock event device or a clock source.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> Cc: Russell King <linux@arm.linux.org.uk>
> ---

[snip]

> +
> +	/*
> +	 * Figure out how to use this clock
> +	 *
> +	 * Note: This is kind of ugly since it requires linux-specific
> +	 * properties in the device tree so that Linux knows which sp804
> +	 * channels can be used as the clock source and the clock events
> +	 * trigger.  Something OS agnostic would be nicer, but it isn't
> +	 * obvious what that should look like.
> +	 */
> +	if (of_get_property(node, "linux,clock-source", NULL)) {
> +		__sp804_clocksource_init(base, node->full_name, clk);
> +	} else if (of_get_property(node, "linux,clockevents-device", NULL)) {
> +		irq = irq_of_parse_and_map(node, 0);
> +		__sp804_clockevents_init(base, irq, node->full_name, clk);

At least in the case of highbank, there is no interrupt connected for
2nd timer in the h/w. So we could use that fact and presence of a clock
for each timer to determine which to use. Some of the ARM boards have 2
sp804's (4 timers) though and you could use any combination I think.
Does it really matter which one is selected as long as it meets the
needs of the OS? Yes, we could think of possible scenarios that don't
work, but it's not likely to see a slew of new platforms with sp804's as
new ARM core integrate the timers in. Although, bcmring is a bit strange
setting up 2 clksrc's, but that doesn't really present a problem.

The fact that you split the timer to 2 nodes is a bit of Linux's needs
defining the binding. The h/w block is a block with 2 timers. It's not
really split. The block does have a single set of primecell ID registers
at 0xfe0 for example.

Rob

Re: [RFC v2 4/9] of: add clock providers

[Grant Likely]

On Mon, Dec 12, 2011 at 4:29 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> Hi Grant,
>
> I'm still going through these and trying to digest them but a couple of
> quick questions/comments.
>
> Jamie
>
> On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> new file mode 100644
>> index 0000000..e40c436
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> @@ -0,0 +1,114 @@
>> +This binding is a work-in-progress, and are based on some experimental
>> +work by benh[1].
>> +
>> +Sources of clock signal can be represented by any node in the device
>> +tree.  Those nodes are designated as clock providers.  Clock consumer
>> +nodes use a phandle and clock specifier pair to connect clock provider
>> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
>> +specifier is an array of one more more cells identifying the clock
>> +output on a device.  The length of a clock specifier is defined by the
>> +value of a #clock-cells property in the clock provider node.
>> +
>> +[1] http://patchwork.ozlabs.org/patch/31551/
>> +
>> +==Clock providers==
>> +
>> +Required properties:
>> +#clock-cells:           Number of cells in a clock specifier; typically will be
>> +                set to 1
>
> I'm not sure I fully understand what the extra cells actually mean for
> clocks.  I think the first integer is the clock output to use but some
> of the versatile and highbank ones only have a phandle or is it more
> implementation defined?  The clock-output-names description hints at
> recommended, so I find this a little confusing, but that could just be
> me!

I'm following convention here that has been established with
interrupts, gpios, and others.  Sometimes more information is needed
that just the clock number.  Using #clock-cells gives a clock provider
the option of having additional fields for clock flags or other data.
This is very much implementation defined.  Simple clock providers that
only have a single clock output can easily use #clock-cells = <0>.
Providers with multiple outputs will need to use 1 or more cells.

>> +Optional properties:
>> +clock-output-names: Recommended to be a list of strings of clock output signal
>> +                 names indexed by the first cell in the clock specifier.
>> +                 However, the meaning of clock-output-name is domain
>> +                 specific to the clock provider, and is only provided to
>> +                 encourage using the same meaning for the majority of clock
>> +                 providers.  This format may not work for clock providers
>> +                 using a complex clock specifier format.  In those cases it
>> +                 is recommended to omit this property and create a binding
>> +                 specific names property.
>> +
>> +                Clock consumer nodes must never directly reference
>> +                the provider's clock-output-name property.
>> +
>> +For example:
>> +
>> +    oscillator {
>> +        #clock-cells = <1>;
>> +        clock-output-names = "ckil", "ckih";
>> +    };
>> +
>> +- this node defines a device with two clock outputs, the first named
>> +  "ckil" and the second named "ckih".  Consumer nodes always reference
>> +  clocks by index. The names should reflect the clock output signal
>> +  names for the device.
>> +
>> +==Clock consumers==
>> +
>> +Required properties:
>> +clocks:              List of phandle and clock specifier pairs, one pair
>> +             for each clock input to the device.
>
> Some of the highbank and versatile devicetree nodes have clocks
> properties that aren't a pair e.g. versatile timer has
> "clocks = <&tim_clk>;".

It's still a pair.... it's just that the specifier portion has a zero
length.  :-)  I do agree that the documentation needs work though.

>
>> +clock-names: List of clock input name strings sorted in the same
>> +             order as the clocks property.  Consumers drivers
>> +             will use clock-names to match clock input names
>> +             with clocks specifiers.
>
> The versatile and highbank patches appears to omit this required
> property in several nodes.  So is this really optional?

You're right, it's not required.  I'll move it to optional.

g.

Re: [RFC v2 4/9] of: add clock providers

[Rob Herring]

Grant,

On 12/13/2011 11:54 AM, Grant Likely wrote:
> On Mon, Dec 12, 2011 at 4:29 PM, Jamie Iles <jamie@jamieiles.com> wrote:
>> Hi Grant,
>>
>> I'm still going through these and trying to digest them but a couple of
>> quick questions/comments.
>>
>> Jamie
>>
>> On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
>>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> new file mode 100644
>>> index 0000000..e40c436
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> @@ -0,0 +1,114 @@
>>> +This binding is a work-in-progress, and are based on some experimental
>>> +work by benh[1].
>>> +
>>> +Sources of clock signal can be represented by any node in the device
>>> +tree.  Those nodes are designated as clock providers.  Clock consumer
>>> +nodes use a phandle and clock specifier pair to connect clock provider
>>> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
>>> +specifier is an array of one more more cells identifying the clock
>>> +output on a device.  The length of a clock specifier is defined by the
>>> +value of a #clock-cells property in the clock provider node.
>>> +
>>> +[1] http://patchwork.ozlabs.org/patch/31551/
>>> +
>>> +==Clock providers==
>>> +
>>> +Required properties:
>>> +#clock-cells:           Number of cells in a clock specifier; typically will be
>>> +                set to 1
>>
>> I'm not sure I fully understand what the extra cells actually mean for
>> clocks.  I think the first integer is the clock output to use but some
>> of the versatile and highbank ones only have a phandle or is it more
>> implementation defined?  The clock-output-names description hints at
>> recommended, so I find this a little confusing, but that could just be
>> me!
> 
> I'm following convention here that has been established with
> interrupts, gpios, and others.  Sometimes more information is needed
> that just the clock number.  Using #clock-cells gives a clock provider
> the option of having additional fields for clock flags or other data.
> This is very much implementation defined.  Simple clock providers that
> only have a single clock output can easily use #clock-cells = <0>.
> Providers with multiple outputs will need to use 1 or more cells.
> 

Aren't you off by 1 here? The minimum cells is 1 to hold the phandle of
the source/parent. Multiple outputs will need a cell size of 2 (typically).

Rob

Re: [RFC v2 4/9] of: add clock providers

[Grant Likely]

On Tue, Dec 13, 2011 at 11:01 AM, Rob Herring <robherring2@gmail.com> wrote:
> Grant,
>
> On 12/13/2011 11:54 AM, Grant Likely wrote:
>> On Mon, Dec 12, 2011 at 4:29 PM, Jamie Iles <jamie@jamieiles.com> wrote:
>>> Hi Grant,
>>>
>>> I'm still going through these and trying to digest them but a couple of
>>> quick questions/comments.
>>>
>>> Jamie
>>>
>>> On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
>>>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> new file mode 100644
>>>> index 0000000..e40c436
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> @@ -0,0 +1,114 @@
>>>> +This binding is a work-in-progress, and are based on some experimental
>>>> +work by benh[1].
>>>> +
>>>> +Sources of clock signal can be represented by any node in the device
>>>> +tree.  Those nodes are designated as clock providers.  Clock consumer
>>>> +nodes use a phandle and clock specifier pair to connect clock provider
>>>> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
>>>> +specifier is an array of one more more cells identifying the clock
>>>> +output on a device.  The length of a clock specifier is defined by the
>>>> +value of a #clock-cells property in the clock provider node.
>>>> +
>>>> +[1] http://patchwork.ozlabs.org/patch/31551/
>>>> +
>>>> +==Clock providers==
>>>> +
>>>> +Required properties:
>>>> +#clock-cells:           Number of cells in a clock specifier; typically will be
>>>> +                set to 1
>>>
>>> I'm not sure I fully understand what the extra cells actually mean for
>>> clocks.  I think the first integer is the clock output to use but some
>>> of the versatile and highbank ones only have a phandle or is it more
>>> implementation defined?  The clock-output-names description hints at
>>> recommended, so I find this a little confusing, but that could just be
>>> me!
>>
>> I'm following convention here that has been established with
>> interrupts, gpios, and others.  Sometimes more information is needed
>> that just the clock number.  Using #clock-cells gives a clock provider
>> the option of having additional fields for clock flags or other data.
>> This is very much implementation defined.  Simple clock providers that
>> only have a single clock output can easily use #clock-cells = <0>.
>> Providers with multiple outputs will need to use 1 or more cells.
>>
>
> Aren't you off by 1 here? The minimum cells is 1 to hold the phandle of
> the source/parent. Multiple outputs will need a cell size of 2 (typically).

No.  The #clock-cells does not include the phandle.  Never did.

g.

Re: [RFC v2 4/9] of: add clock providers

[Shawn Guo]

On Tue, Dec 13, 2011 at 10:54:58AM -0700, Grant Likely wrote:
> On Mon, Dec 12, 2011 at 4:29 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> > Hi Grant,
> >
> > I'm still going through these and trying to digest them but a couple of
> > quick questions/comments.
> >
> > Jamie
> >
> > On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
> >> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> new file mode 100644
> >> index 0000000..e40c436
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> @@ -0,0 +1,114 @@
> >> +This binding is a work-in-progress, and are based on some experimental
> >> +work by benh[1].
> >> +
> >> +Sources of clock signal can be represented by any node in the device
> >> +tree.  Those nodes are designated as clock providers.  Clock consumer
> >> +nodes use a phandle and clock specifier pair to connect clock provider
> >> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
> >> +specifier is an array of one more more cells identifying the clock
> >> +output on a device.  The length of a clock specifier is defined by the
> >> +value of a #clock-cells property in the clock provider node.
> >> +
> >> +[1] http://patchwork.ozlabs.org/patch/31551/
> >> +
> >> +==Clock providers==
> >> +
> >> +Required properties:
> >> +#clock-cells:           Number of cells in a clock specifier; typically will be
> >> +                set to 1
> >
> > I'm not sure I fully understand what the extra cells actually mean for
> > clocks.  I think the first integer is the clock output to use but some
> > of the versatile and highbank ones only have a phandle or is it more
> > implementation defined?  The clock-output-names description hints at
> > recommended, so I find this a little confusing, but that could just be
> > me!
> 
> I'm following convention here that has been established with
> interrupts, gpios, and others.  Sometimes more information is needed
> that just the clock number.  Using #clock-cells gives a clock provider
> the option of having additional fields for clock flags or other data.
> This is very much implementation defined.  Simple clock providers that
> only have a single clock output can easily use #clock-cells = <0>.
> Providers with multiple outputs will need to use 1 or more cells.
> 
It totally destroys my understanding on #clock-cells :)

I thought it's introduced to reduce the clock nodes in dts.  That said,
#clock-cells stands for the number of clks we describe in the node.
When #clock-cells > 1 for a node, the node becomes a clk blob which
actually contains multiple clks.  I migrated the imx6 clock to your
first post with this approach [1], using 70 nodes to describe 110
clocks (~35% nodes reduced).

Now, you are saying it's not designed for this purpose.  I'm pretty
confused, because the only reasonable use of #clock-cells to me is
just that way, and I fail to see why we need #clock-cells if we do
not use it that way.

http://comments.gmane.org/gmane.linux.drivers.devicetree/9631

-- 
Regards,
Shawn

Re: [RFC v2 4/9] of: add clock providers

[Rob Herring]

On 12/15/2011 07:51 AM, Shawn Guo wrote:
> On Tue, Dec 13, 2011 at 10:54:58AM -0700, Grant Likely wrote:
>> On Mon, Dec 12, 2011 at 4:29 PM, Jamie Iles <jamie@jamieiles.com> wrote:
>>> Hi Grant,
>>>
>>> I'm still going through these and trying to digest them but a couple of
>>> quick questions/comments.
>>>
>>> Jamie
>>>
>>> On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
>>>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> new file mode 100644
>>>> index 0000000..e40c436
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> @@ -0,0 +1,114 @@
>>>> +This binding is a work-in-progress, and are based on some experimental
>>>> +work by benh[1].
>>>> +
>>>> +Sources of clock signal can be represented by any node in the device
>>>> +tree.  Those nodes are designated as clock providers.  Clock consumer
>>>> +nodes use a phandle and clock specifier pair to connect clock provider
>>>> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
>>>> +specifier is an array of one more more cells identifying the clock
>>>> +output on a device.  The length of a clock specifier is defined by the
>>>> +value of a #clock-cells property in the clock provider node.
>>>> +
>>>> +[1] http://patchwork.ozlabs.org/patch/31551/
>>>> +
>>>> +==Clock providers==
>>>> +
>>>> +Required properties:
>>>> +#clock-cells:           Number of cells in a clock specifier; typically will be
>>>> +                set to 1
>>>
>>> I'm not sure I fully understand what the extra cells actually mean for
>>> clocks.  I think the first integer is the clock output to use but some
>>> of the versatile and highbank ones only have a phandle or is it more
>>> implementation defined?  The clock-output-names description hints at
>>> recommended, so I find this a little confusing, but that could just be
>>> me!
>>
>> I'm following convention here that has been established with
>> interrupts, gpios, and others.  Sometimes more information is needed
>> that just the clock number.  Using #clock-cells gives a clock provider
>> the option of having additional fields for clock flags or other data.
>> This is very much implementation defined.  Simple clock providers that
>> only have a single clock output can easily use #clock-cells = <0>.
>> Providers with multiple outputs will need to use 1 or more cells.
>>
> It totally destroys my understanding on #clock-cells :)
> 
> I thought it's introduced to reduce the clock nodes in dts.  That said,
> #clock-cells stands for the number of clks we describe in the node.
> When #clock-cells > 1 for a node, the node becomes a clk blob which
> actually contains multiple clks.  I migrated the imx6 clock to your
> first post with this approach [1], using 70 nodes to describe 110
> clocks (~35% nodes reduced).
> 
> Now, you are saying it's not designed for this purpose.  I'm pretty
> confused, because the only reasonable use of #clock-cells to me is
> just that way, and I fail to see why we need #clock-cells if we do
> not use it that way.
> 
> http://comments.gmane.org/gmane.linux.drivers.devicetree/9631

Think of clock-cells as the size of array elements (minus 1), not the
number of array elements. If you have more than 1 clock per node, then
you need more information on the clock input side than just a phandle to
describe which clock you are using. Typically this would just be the
phandle plus index of the clock you want. This is just like gpio where
you have controller phandle plus gpio line number.

Rob

Re: [RFC v2 4/9] of: add clock providers

[Shawn Guo]

On Thu, Dec 15, 2011 at 08:23:17AM -0600, Rob Herring wrote:
> 
> 
> On 12/15/2011 07:51 AM, Shawn Guo wrote:
> > On Tue, Dec 13, 2011 at 10:54:58AM -0700, Grant Likely wrote:
> >> On Mon, Dec 12, 2011 at 4:29 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> >>> Hi Grant,
> >>>
> >>> I'm still going through these and trying to digest them but a couple of
> >>> quick questions/comments.
> >>>
> >>> Jamie
> >>>
> >>> On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
> >>>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>> new file mode 100644
> >>>> index 0000000..e40c436
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>> @@ -0,0 +1,114 @@
> >>>> +This binding is a work-in-progress, and are based on some experimental
> >>>> +work by benh[1].
> >>>> +
> >>>> +Sources of clock signal can be represented by any node in the device
> >>>> +tree.  Those nodes are designated as clock providers.  Clock consumer
> >>>> +nodes use a phandle and clock specifier pair to connect clock provider
> >>>> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
> >>>> +specifier is an array of one more more cells identifying the clock
> >>>> +output on a device.  The length of a clock specifier is defined by the
> >>>> +value of a #clock-cells property in the clock provider node.
> >>>> +
> >>>> +[1] http://patchwork.ozlabs.org/patch/31551/
> >>>> +
> >>>> +==Clock providers==
> >>>> +
> >>>> +Required properties:
> >>>> +#clock-cells:           Number of cells in a clock specifier; typically will be
> >>>> +                set to 1
> >>>
> >>> I'm not sure I fully understand what the extra cells actually mean for
> >>> clocks.  I think the first integer is the clock output to use but some
> >>> of the versatile and highbank ones only have a phandle or is it more
> >>> implementation defined?  The clock-output-names description hints at
> >>> recommended, so I find this a little confusing, but that could just be
> >>> me!
> >>
> >> I'm following convention here that has been established with
> >> interrupts, gpios, and others.  Sometimes more information is needed
> >> that just the clock number.  Using #clock-cells gives a clock provider
> >> the option of having additional fields for clock flags or other data.
> >> This is very much implementation defined.  Simple clock providers that
> >> only have a single clock output can easily use #clock-cells = <0>.
> >> Providers with multiple outputs will need to use 1 or more cells.
> >>
> > It totally destroys my understanding on #clock-cells :)
> > 
> > I thought it's introduced to reduce the clock nodes in dts.  That said,
> > #clock-cells stands for the number of clks we describe in the node.
> > When #clock-cells > 1 for a node, the node becomes a clk blob which
> > actually contains multiple clks.  I migrated the imx6 clock to your
> > first post with this approach [1], using 70 nodes to describe 110
> > clocks (~35% nodes reduced).
> > 
> > Now, you are saying it's not designed for this purpose.  I'm pretty
> > confused, because the only reasonable use of #clock-cells to me is
> > just that way, and I fail to see why we need #clock-cells if we do
> > not use it that way.
> > 
> > http://comments.gmane.org/gmane.linux.drivers.devicetree/9631
> 
> Think of clock-cells as the size of array elements (minus 1), not the
> number of array elements. If you have more than 1 clock per node, then
> you need more information on the clock input side than just a phandle to
> describe which clock you are using. Typically this would just be the
> phandle plus index of the clock you want. This is just like gpio where
> you have controller phandle plus gpio line number.
> 
It seems I misunderstood/misused the #clock-cells.  So even we have
multiple clks embedded in a node, #clock-cells=1 works for most of the
cases, since what really matters is just the index of the clk we want
the phandle to point to.  And the clock driver has to figure out how
many clks are in there by some other means.  Correct me please, if I
still misunderstand it.

-- 
Regards,
Shawn

Re: [RFC v2 5/9] dt/clock: Add handling for fixed clocks and a clock node setup iterator

[Shawn Guo]

Hi Grant,

On Mon, Dec 12, 2011 at 03:02:05PM -0700, Grant Likely wrote:
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>  drivers/of/clock.c     |   43 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_clk.h |    4 ++++
>  2 files changed, 47 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/clock.c b/drivers/of/clock.c
> index 6519e96..1c189b4 100644
> --- a/drivers/of/clock.c
> +++ b/drivers/of/clock.c
> @@ -3,6 +3,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/clkdev.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
>  #include <linux/module.h>
> @@ -163,3 +164,45 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
>  
>  	return clk;
>  }
> +
> +/**
> + * of_clk_init() - Scan and init clock providers from the DT
> + * @matches: array of compatible values and init functions for providers.
> + *
> + * This function scans the device tree for matching clock providers and
> + * calls their initialization functions
> + */
> +void __init of_clk_init(const struct of_device_id *matches)
> +{
> +	struct device_node *np;
> +
> +	for_each_matching_node(np, matches) {
> +		const struct of_device_id *match = of_match_node(matches, np);
> +		of_clk_init_cb_t clk_init_cb = match->data;
> +		clk_init_cb(np);
> +	}
> +}
> +
> +static struct clk *of_fixed_clk_get(struct of_phandle_args *a, void *data)
> +{
> +	return data;
> +}
> +
> +/**
> + * of_fixed_clk_setup() - Setup function for simple fixed rate clock
> + */
> +void __init of_fixed_clk_setup(struct device_node *node)
> +{
> +	struct clk *clk;
> +	u32 rate;
> +
> +	if (of_property_read_u32(node, "clock-frequency", &rate))
> +		return;
> +
> +	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> +	if (!clk)
> +		return;
> +	clk->rate = rate;
> +
> +	of_clk_add_provider(node, of_fixed_clk_get, clk);
> +}

Just take the fixed clk as example, I have a node below as a blob,
which has 3 fixed clks encoded.

ref_clk: ref {
	compatible = "fixed-clock", "basic-clock";
	#clock-cells = <1>;
	clock-frequency = <24000000 32768 0>;
	clock-output-name = "osc",
			    "ckil",
			    "ckih";
};

If this is valid, the of_fixed_clk_setup() should scan the node for
multiple clks, right?

-- 
Regards,
Shawn

Re: [RFC v2 4/9] of: add clock providers

[Grant Likely]

On Thu, Dec 15, 2011 at 8:13 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Thu, Dec 15, 2011 at 08:23:17AM -0600, Rob Herring wrote:
>>
>>
>> On 12/15/2011 07:51 AM, Shawn Guo wrote:
>> > On Tue, Dec 13, 2011 at 10:54:58AM -0700, Grant Likely wrote:
>> >> On Mon, Dec 12, 2011 at 4:29 PM, Jamie Iles <jamie@jamieiles.com> wrote:
>> >>> Hi Grant,
>> >>>
>> >>> I'm still going through these and trying to digest them but a couple of
>> >>> quick questions/comments.
>> >>>
>> >>> Jamie
>> >>>
>> >>> On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
>> >>>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> >>>> new file mode 100644
>> >>>> index 0000000..e40c436
>> >>>> --- /dev/null
>> >>>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> >>>> @@ -0,0 +1,114 @@
>> >>>> +This binding is a work-in-progress, and are based on some experimental
>> >>>> +work by benh[1].
>> >>>> +
>> >>>> +Sources of clock signal can be represented by any node in the device
>> >>>> +tree.  Those nodes are designated as clock providers.  Clock consumer
>> >>>> +nodes use a phandle and clock specifier pair to connect clock provider
>> >>>> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
>> >>>> +specifier is an array of one more more cells identifying the clock
>> >>>> +output on a device.  The length of a clock specifier is defined by the
>> >>>> +value of a #clock-cells property in the clock provider node.
>> >>>> +
>> >>>> +[1] http://patchwork.ozlabs.org/patch/31551/
>> >>>> +
>> >>>> +==Clock providers==
>> >>>> +
>> >>>> +Required properties:
>> >>>> +#clock-cells:           Number of cells in a clock specifier; typically will be
>> >>>> +                set to 1
>> >>>
>> >>> I'm not sure I fully understand what the extra cells actually mean for
>> >>> clocks.  I think the first integer is the clock output to use but some
>> >>> of the versatile and highbank ones only have a phandle or is it more
>> >>> implementation defined?  The clock-output-names description hints at
>> >>> recommended, so I find this a little confusing, but that could just be
>> >>> me!
>> >>
>> >> I'm following convention here that has been established with
>> >> interrupts, gpios, and others.  Sometimes more information is needed
>> >> that just the clock number.  Using #clock-cells gives a clock provider
>> >> the option of having additional fields for clock flags or other data.
>> >> This is very much implementation defined.  Simple clock providers that
>> >> only have a single clock output can easily use #clock-cells = <0>.
>> >> Providers with multiple outputs will need to use 1 or more cells.
>> >>
>> > It totally destroys my understanding on #clock-cells :)
>> >
>> > I thought it's introduced to reduce the clock nodes in dts.  That said,
>> > #clock-cells stands for the number of clks we describe in the node.
>> > When #clock-cells > 1 for a node, the node becomes a clk blob which
>> > actually contains multiple clks.  I migrated the imx6 clock to your
>> > first post with this approach [1], using 70 nodes to describe 110
>> > clocks (~35% nodes reduced).
>> >
>> > Now, you are saying it's not designed for this purpose.  I'm pretty
>> > confused, because the only reasonable use of #clock-cells to me is
>> > just that way, and I fail to see why we need #clock-cells if we do
>> > not use it that way.
>> >
>> > http://comments.gmane.org/gmane.linux.drivers.devicetree/9631
>>
>> Think of clock-cells as the size of array elements (minus 1), not the
>> number of array elements. If you have more than 1 clock per node, then
>> you need more information on the clock input side than just a phandle to
>> describe which clock you are using. Typically this would just be the
>> phandle plus index of the clock you want. This is just like gpio where
>> you have controller phandle plus gpio line number.
>>
> It seems I misunderstood/misused the #clock-cells.  So even we have
> multiple clks embedded in a node, #clock-cells=1 works for most of the
> cases, since what really matters is just the index of the clk we want
> the phandle to point to.  And the clock driver has to figure out how
> many clks are in there by some other means.  Correct me please, if I
> still misunderstand it.

Yes, it sounds like you have it right.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC v2 4/9] of: add clock providers

[Jamie Iles]

Hi Grant,

On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
> Based on work by Ben Herrenschmidt and Jeremy Kerr, this patch adds an
> of_clk_get function to allow platforms to retrieve clock data from the
> device tree.
> 
> Platform register a provider through of_clk_add_provider, which will be
> called when a device references the provider's OF node for a clock
> reference.
[...]
> diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> new file mode 100644
> index 0000000..9a75342
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> @@ -0,0 +1,21 @@
> +Binding for simple fixed-rate clock sources.
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/fixed-clock.txt

This appears to reference itself?

> +
> +Required properties:
> +- compatible : shall be "fixed-clock".
> +- #clock-cells : from common clock binding; shall be set to 0.
> +- clock-frequency : frequency of clock in Hz. May be multiple cells.
> +
> +Optional properties:
> +- gpios : From common gpio binding; gpio connection to clock enable pin.

This seems a little odd to me to have in the common binding, but I'm not 
that familiar with too many platforms.

> +- clock-output-names : From common clock binding
> +
> +Example:
> +	clock {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <1000000000>;
> +	};

I wonder if this should have an optional clock consumer with a standard 
name for parenting?  For example, on picoxcell there is a fixed 200MHz 
APB clock that is really a PLL from a 20MHz reference input and I'd like 
to represent that in the clock tree.

I'm about to start trying to get this and Mike's common struct clk 
patches working on picoxcell, and the one thing I'm not understanding at 
the moment is how to handle the tree itself.  Currently I was planning 
on iterating over all clocks and finding a named input clock "ref" or 
"input" perhaps.  This would be fine for picoxcell, but I guess more 
complicated chips may need something else.

Jamie

Re: [RFC v2 4/9] of: add clock providers

[Grant Likely]

On Tue, Jan 10, 2012 at 2:33 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> Hi Grant,
>
> On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
>> Based on work by Ben Herrenschmidt and Jeremy Kerr, this patch adds an
>> of_clk_get function to allow platforms to retrieve clock data from the
>> device tree.
>>
>> Platform register a provider through of_clk_add_provider, which will be
>> called when a device references the provider's OF node for a clock
>> reference.
> [...]
>> diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
>> new file mode 100644
>> index 0000000..9a75342
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
>> @@ -0,0 +1,21 @@
>> +Binding for simple fixed-rate clock sources.
>> +
>> +This binding uses the common clock binding[1].
>> +
>> +[1] Documentation/devicetree/bindings/clock/fixed-clock.txt
>
> This appears to reference itself?

Heh, oops.

>
>> +
>> +Required properties:
>> +- compatible : shall be "fixed-clock".
>> +- #clock-cells : from common clock binding; shall be set to 0.
>> +- clock-frequency : frequency of clock in Hz. May be multiple cells.
>> +
>> +Optional properties:
>> +- gpios : From common gpio binding; gpio connection to clock enable pin.
>
> This seems a little odd to me to have in the common binding, but I'm not
> that familiar with too many platforms.
>
>> +- clock-output-names : From common clock binding
>> +
>> +Example:
>> +     clock {
>> +             compatible = "fixed-clock";
>> +             #clock-cells = <0>;
>> +             clock-frequency = <1000000000>;
>> +     };
>
> I wonder if this should have an optional clock consumer with a standard
> name for parenting?  For example, on picoxcell there is a fixed 200MHz
> APB clock that is really a PLL from a 20MHz reference input and I'd like
> to represent that in the clock tree.

If it depends on a parent clock, then it really isn't a fixed clock,
is it (from the perspective of the OS).  The point of this binding is
a trivial way to describe clocks that don't change.  If that
assumption doesn't hold true, then this binding isn't suitable for
that clock.  As you point out, even the gpio enable feature is pushing
things a bit.

That said, I'm open to extending this binding if something can be
defined that will have a lot of use cases.

> I'm about to start trying to get this and Mike's common struct clk
> patches working on picoxcell, and the one thing I'm not understanding at
> the moment is how to handle the tree itself.  Currently I was planning
> on iterating over all clocks and finding a named input clock "ref" or
> "input" perhaps.  This would be fine for picoxcell, but I guess more
> complicated chips may need something else.

It might be useful to have something like of_irq_init() for setting up
initial clocks, but the solution feels inelegant to me.  I suspect
that there will be end up being intertwined init order dependencies
between clocks and irqs and other early setup stuff that could be
handled better.  Again, I need to think about this some more.  There
might need to be something like an of_early_probe() call that accepts
a match table of compatible values and setup functions with some logic
or data to resolve dependencies.  The trick will be to not end up with
something complex.  I'll need to think about this more...

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC v2 4/9] of: add clock providers

[Jamie Iles]

On Wed, Jan 11, 2012 at 09:46:58PM -0700, Grant Likely wrote:
> On Tue, Jan 10, 2012 at 2:33 PM, Jamie Iles <jamie@jamieiles.com> wrote:
> > On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
> >> +- clock-output-names : From common clock binding
> >> +
> >> +Example:
> >> +     clock {
> >> +             compatible = "fixed-clock";
> >> +             #clock-cells = <0>;
> >> +             clock-frequency = <1000000000>;
> >> +     };
> >
> > I wonder if this should have an optional clock consumer with a standard
> > name for parenting?  For example, on picoxcell there is a fixed 200MHz
> > APB clock that is really a PLL from a 20MHz reference input and I'd like
> > to represent that in the clock tree.
> 
> If it depends on a parent clock, then it really isn't a fixed clock,
> is it (from the perspective of the OS).  The point of this binding is
> a trivial way to describe clocks that don't change.  If that
> assumption doesn't hold true, then this binding isn't suitable for
> that clock.  As you point out, even the gpio enable feature is pushing
> things a bit.
> 
> That said, I'm open to extending this binding if something can be
> defined that will have a lot of use cases.

Well for picoxcell where the 200MHz APB clock is just a PLL generated 
from a 20MHz clock then it kind of is a fixed clock as far as Linux is 
concerned isn't it?  The rate can't be changed and and the clock can't 
be disabled so it's just a dumb clock with a parent.

I guess I could just omit the parent as far as Linux is concerned, but 
it would be nice to represent the real clock tree if possible.

> > I'm about to start trying to get this and Mike's common struct clk
> > patches working on picoxcell, and the one thing I'm not understanding at
> > the moment is how to handle the tree itself.  Currently I was planning
> > on iterating over all clocks and finding a named input clock "ref" or
> > "input" perhaps.  This would be fine for picoxcell, but I guess more
> > complicated chips may need something else.
> 
> It might be useful to have something like of_irq_init() for setting up
> initial clocks, but the solution feels inelegant to me.  I suspect
> that there will be end up being intertwined init order dependencies
> between clocks and irqs and other early setup stuff that could be
> handled better.  Again, I need to think about this some more.  There
> might need to be something like an of_early_probe() call that accepts
> a match table of compatible values and setup functions with some logic
> or data to resolve dependencies.  The trick will be to not end up with
> something complex.  I'll need to think about this more...

Yes, probably not an easy problem to solve, especially for the platforms 
where the parent can change at runtime.

I wonder if an of_clk_init() could take a platform callback, so that 
of_clk_init() goes of and creates a struct clk for each clk in the DT, 
then for each registered clock calls a platform specific callback which 
returns the parent (if any).  It feels like a fairly platform specific 
problem to me.

Jamie

Re: [RFC v2 4/9] of: add clock providers

[Turquette, Mike]

On Thu, Jan 12, 2012 at 2:07 AM, Jamie Iles <jamie@jamieiles.com> wrote:
> On Wed, Jan 11, 2012 at 09:46:58PM -0700, Grant Likely wrote:
>> On Tue, Jan 10, 2012 at 2:33 PM, Jamie Iles <jamie@jamieiles.com> wrote:
>> > On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
>> > I'm about to start trying to get this and Mike's common struct clk
>> > patches working on picoxcell, and the one thing I'm not understanding at
>> > the moment is how to handle the tree itself.  Currently I was planning
>> > on iterating over all clocks and finding a named input clock "ref" or
>> > "input" perhaps.  This would be fine for picoxcell, but I guess more
>> > complicated chips may need something else.
>>
>> It might be useful to have something like of_irq_init() for setting up
>> initial clocks, but the solution feels inelegant to me.  I suspect
>> that there will be end up being intertwined init order dependencies
>> between clocks and irqs and other early setup stuff that could be
>> handled better.  Again, I need to think about this some more.  There
>> might need to be something like an of_early_probe() call that accepts
>> a match table of compatible values and setup functions with some logic
>> or data to resolve dependencies.  The trick will be to not end up with
>> something complex.  I'll need to think about this more...
>
> Yes, probably not an easy problem to solve, especially for the platforms
> where the parent can change at runtime.
>
> I wonder if an of_clk_init() could take a platform callback, so that
> of_clk_init() goes of and creates a struct clk for each clk in the DT,
> then for each registered clock calls a platform specific callback which
> returns the parent (if any).  It feels like a fairly platform specific
> problem to me.

Based on Thomas' feedback I'm removing the requirement for clocks to
be registered in-order with clk_init().  Any clock that cannot resolve
it's parent within clk_init() (via the .get_parent callback, or
otherwise having .parent statically initialized) will be put into an
orphaned clocks list, which will be walked every time a new clock is
registered.  Hurray for n^2 solutions.

Does the above help with the of_clk_init problems?

One final data point: I certainly plan on allowing for statically
allocated clocks to live alongside DT clocks.  In fact the clock trees
on OMAP are so large that there is some discussion about having some
of the clocks statically allocated and some in DT, but I don't know
what that split looks like right now.  I don't enjoy the idea of
packing 200+ of any entity into a .dts blob.

Regards,
Mike

Re: [RFC v2 4/9] of: add clock providers

[Grant Likely]

On Thu, Jan 12, 2012 at 11:44 AM, Turquette, Mike <mturquette@ti.com> wrote:
> On Thu, Jan 12, 2012 at 2:07 AM, Jamie Iles <jamie@jamieiles.com> wrote:
>> On Wed, Jan 11, 2012 at 09:46:58PM -0700, Grant Likely wrote:
>>> On Tue, Jan 10, 2012 at 2:33 PM, Jamie Iles <jamie@jamieiles.com> wrote:
>>> > On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
>>> > I'm about to start trying to get this and Mike's common struct clk
>>> > patches working on picoxcell, and the one thing I'm not understanding at
>>> > the moment is how to handle the tree itself.  Currently I was planning
>>> > on iterating over all clocks and finding a named input clock "ref" or
>>> > "input" perhaps.  This would be fine for picoxcell, but I guess more
>>> > complicated chips may need something else.
>>>
>>> It might be useful to have something like of_irq_init() for setting up
>>> initial clocks, but the solution feels inelegant to me.  I suspect
>>> that there will be end up being intertwined init order dependencies
>>> between clocks and irqs and other early setup stuff that could be
>>> handled better.  Again, I need to think about this some more.  There
>>> might need to be something like an of_early_probe() call that accepts
>>> a match table of compatible values and setup functions with some logic
>>> or data to resolve dependencies.  The trick will be to not end up with
>>> something complex.  I'll need to think about this more...
>>
>> Yes, probably not an easy problem to solve, especially for the platforms
>> where the parent can change at runtime.
>>
>> I wonder if an of_clk_init() could take a platform callback, so that
>> of_clk_init() goes of and creates a struct clk for each clk in the DT,
>> then for each registered clock calls a platform specific callback which
>> returns the parent (if any).  It feels like a fairly platform specific
>> problem to me.
>
> Based on Thomas' feedback I'm removing the requirement for clocks to
> be registered in-order with clk_init().  Any clock that cannot resolve
> it's parent within clk_init() (via the .get_parent callback, or
> otherwise having .parent statically initialized) will be put into an
> orphaned clocks list, which will be walked every time a new clock is
> registered.  Hurray for n^2 solutions.
>
> Does the above help with the of_clk_init problems?

It does.

> One final data point: I certainly plan on allowing for statically
> allocated clocks to live alongside DT clocks.  In fact the clock trees
> on OMAP are so large that there is some discussion about having some
> of the clocks statically allocated and some in DT, but I don't know
> what that split looks like right now.  I don't enjoy the idea of
> packing 200+ of any entity into a .dts blob.

I'm okay with that.

Re: [RFC v2 4/9] of: add clock providers

[Shawn Guo]

On Wed, Jan 11, 2012 at 09:46:58PM -0700, Grant Likely wrote:
...
> >> +Required properties:
> >> +- compatible : shall be "fixed-clock".
> >> +- #clock-cells : from common clock binding; shall be set to 0.
> >> +- clock-frequency : frequency of clock in Hz. May be multiple cells.
> >> +
> >> +Optional properties:
> >> +- gpios : From common gpio binding; gpio connection to clock enable pin.
> >
> > This seems a little odd to me to have in the common binding, but I'm not
> > that familiar with too many platforms.
> >
> >> +- clock-output-names : From common clock binding
> >> +
> >> +Example:
> >> +     clock {
> >> +             compatible = "fixed-clock";
> >> +             #clock-cells = <0>;
> >> +             clock-frequency = <1000000000>;
> >> +     };
> >
> > I wonder if this should have an optional clock consumer with a standard
> > name for parenting?  For example, on picoxcell there is a fixed 200MHz
> > APB clock that is really a PLL from a 20MHz reference input and I'd like
> > to represent that in the clock tree.
> 
> If it depends on a parent clock, then it really isn't a fixed clock,
> is it (from the perspective of the OS).  The point of this binding is
> a trivial way to describe clocks that don't change.  If that
> assumption doesn't hold true, then this binding isn't suitable for
> that clock.  As you point out, even the gpio enable feature is pushing
> things a bit.
> 
I recently ran into a use case perfectly fitting into this discussion.

We have audio codec sgtl5000 on imx51-babbage board requiring a 26 MHz
clock input to its SYS_MCLK pin.  The board has a 26 MHz oscillator with
a gpio control that outputs 26M_OSC_CLK.  And this clock goes through a
3-state buffer component with another gpio control, and then goes to
sgtl5000 SYS_MCLK pin.  The following is what I have in my mind to
describe them in device tree.

soc_26m_clk: soc-26m-clk {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <26000000>;
	clock-output-names = "soc-26m-clk";
	gpios = <&gpio3 1 0>;
};

sgtl5000_clk: sgtl5000-sys-mclk {
	#clock-cells = <0>;
	gpios = <&gpio4 26 0>;
	clocks = <&soc_26m_clk>;
	clock-names = "soc-26m-clk";
	clock-output-names = "sgtl5000-sys-mclk";
};

codec: sgtl5000@0a {
	compatible = "fsl,sgtl5000";
	reg = <0x0a>;
	clocks = <&sgtl5000_clk>;
	clock-names = "sgtl5000-sys-mclk";
};

The sgtl5000-sys-mclk is a clock with fixed rate, but the rate is really
defined by its parent soc-26m-clk, which is anothe fixed-rate clock.  We
have no doubt that soc-26m-clk is a "fixed-clock".  Is sgtl5000-sys-mclk
a "fixed-clock"?  The following is the "fixed-clock" defined by Mike's
implementation.

/*
 * DOC: basic fixed-rate clock that cannot gate
 *
 * Traits of this clock:
 * prepare - clock never gates.  clk_prepare & clk_unprepare do nothing
 * enable - clock never gates.  clk_enable & clk_disable do nothing
 * rate - rate is always a fixed value.  No clk_set_rate support
 * parent - fixed parent.  No clk_set_parent support
 *
 * note: parent can be NULL, which implies that this clock is a root clock.
 */
struct clk_hw_ops clk_hw_fixed_ops = {
       .recalc_rate = clk_hw_fixed_recalc_rate,
       .get_parent = clk_hw_fixed_get_parent,
};

It says that the "fixed-clock" can have a fixed parent.  So I should
probably add compatible = "fixed-clock" for node sgtl5000-sys-mclk too.
Then should I add clock-frequency property for it too?  I'm not sure
about that, since the frequency is really defined by its parent.  IOW,
should the clock-frequency property for "fixed-clock" be optional?  On
the other hand, let clock-frequency property be optional seems a little
illogical to the concept of "fixed-clock".  So I'm really confused here.

With the proper clock support, I would expect that sgtl5000 driver only
needs to make the following two calls to have its clock enabled.

	mclk = clk_get(&dev, "sgtl5000-sys-mclk");
	clk_prepare_enable(mclk);

But looking at the current fixed-clock implementation, there is even
no clk_enable operation for fixed-clock.  How can we control the clock
with gpio?

All I'm saying is that we need some level of alignment between clock
binding and common-clk implementation.

Regards,
Shawn

> That said, I'm open to extending this binding if something can be
> defined that will have a lot of use cases.

Re: [RFC v2 4/9] of: add clock providers

[Shawn Guo]

On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
> +==Clock providers==
> +
> +Required properties:
> +#clock-cells:	   Number of cells in a clock specifier; typically will be
> +		   set to 1

Shouldn't it be 0 typically, which means it represents only one clock
in the node?

-- 
Regards,
Shawn

Re: [RFC v2 4/9] of: add clock providers

[Rob Herring]

On 01/13/2012 07:50 AM, Shawn Guo wrote:
> On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
>> +==Clock providers==
>> +
>> +Required properties:
>> +#clock-cells:	   Number of cells in a clock specifier; typically will be
>> +		   set to 1
> 
> Shouldn't it be 0 typically, which means it represents only one clock
> in the node?
> 

We have no idea what will be typical. I would say: Typically 0 for nodes
with a single clock output and 1 for nodes with multiple clock outputs.

Rob

Re: [RFC v2 4/9] of: add clock providers

[Shawn Guo]

On Fri, Jan 13, 2012 at 08:05:21AM -0600, Rob Herring wrote:
> On 01/13/2012 07:50 AM, Shawn Guo wrote:
> > On Mon, Dec 12, 2011 at 03:02:04PM -0700, Grant Likely wrote:
> >> +==Clock providers==
> >> +
> >> +Required properties:
> >> +#clock-cells:	   Number of cells in a clock specifier; typically will be
> >> +		   set to 1
> > 
> > Shouldn't it be 0 typically, which means it represents only one clock
> > in the node?
> > 
> 
> We have no idea what will be typical. I would say: Typically 0 for nodes
> with a single clock output and 1 for nodes with multiple clock outputs.
> 
That's definitely better to me.

-- 
Regards,
Shawn

Re: [RFC v2 4/9] of: add clock providers

[Turquette, Mike]

On Fri, Jan 13, 2012 at 4:47 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Wed, Jan 11, 2012 at 09:46:58PM -0700, Grant Likely wrote:
> ...
>> >> +Required properties:
>> >> +- compatible : shall be "fixed-clock".
>> >> +- #clock-cells : from common clock binding; shall be set to 0.
>> >> +- clock-frequency : frequency of clock in Hz. May be multiple cells.
>> >> +
>> >> +Optional properties:
>> >> +- gpios : From common gpio binding; gpio connection to clock enable pin.
>> >
>> > This seems a little odd to me to have in the common binding, but I'm not
>> > that familiar with too many platforms.
>> >
>> >> +- clock-output-names : From common clock binding
>> >> +
>> >> +Example:
>> >> +     clock {
>> >> +             compatible = "fixed-clock";
>> >> +             #clock-cells = <0>;
>> >> +             clock-frequency = <1000000000>;
>> >> +     };
>> >
>> > I wonder if this should have an optional clock consumer with a standard
>> > name for parenting?  For example, on picoxcell there is a fixed 200MHz
>> > APB clock that is really a PLL from a 20MHz reference input and I'd like
>> > to represent that in the clock tree.
>>
>> If it depends on a parent clock, then it really isn't a fixed clock,
>> is it (from the perspective of the OS).  The point of this binding is
>> a trivial way to describe clocks that don't change.  If that
>> assumption doesn't hold true, then this binding isn't suitable for
>> that clock.  As you point out, even the gpio enable feature is pushing
>> things a bit.
>>
> I recently ran into a use case perfectly fitting into this discussion.
>
> We have audio codec sgtl5000 on imx51-babbage board requiring a 26 MHz
> clock input to its SYS_MCLK pin.  The board has a 26 MHz oscillator with
> a gpio control that outputs 26M_OSC_CLK.  And this clock goes through a
> 3-state buffer component with another gpio control, and then goes to
> sgtl5000 SYS_MCLK pin.  The following is what I have in my mind to
> describe them in device tree.
>
> soc_26m_clk: soc-26m-clk {
>        compatible = "fixed-clock";
>        #clock-cells = <0>;
>        clock-frequency = <26000000>;
>        clock-output-names = "soc-26m-clk";
>        gpios = <&gpio3 1 0>;
> };
>
> sgtl5000_clk: sgtl5000-sys-mclk {
>        #clock-cells = <0>;
>        gpios = <&gpio4 26 0>;
>        clocks = <&soc_26m_clk>;
>        clock-names = "soc-26m-clk";
>        clock-output-names = "sgtl5000-sys-mclk";
> };
>
> codec: sgtl5000@0a {
>        compatible = "fsl,sgtl5000";
>        reg = <0x0a>;
>        clocks = <&sgtl5000_clk>;
>        clock-names = "sgtl5000-sys-mclk";
> };
>
> The sgtl5000-sys-mclk is a clock with fixed rate, but the rate is really
> defined by its parent soc-26m-clk, which is anothe fixed-rate clock.  We
> have no doubt that soc-26m-clk is a "fixed-clock".  Is sgtl5000-sys-mclk
> a "fixed-clock"?  The following is the "fixed-clock" defined by Mike's
> implementation.
>
> /*
>  * DOC: basic fixed-rate clock that cannot gate
>  *
>  * Traits of this clock:
>  * prepare - clock never gates.  clk_prepare & clk_unprepare do nothing
>  * enable - clock never gates.  clk_enable & clk_disable do nothing
>  * rate - rate is always a fixed value.  No clk_set_rate support
>  * parent - fixed parent.  No clk_set_parent support
>  *
>  * note: parent can be NULL, which implies that this clock is a root clock.
>  */
> struct clk_hw_ops clk_hw_fixed_ops = {
>       .recalc_rate = clk_hw_fixed_recalc_rate,
>       .get_parent = clk_hw_fixed_get_parent,
> };
>
> It says that the "fixed-clock" can have a fixed parent.  So I should
> probably add compatible = "fixed-clock" for node sgtl5000-sys-mclk too.
> Then should I add clock-frequency property for it too?  I'm not sure
> about that, since the frequency is really defined by its parent.  IOW,
> should the clock-frequency property for "fixed-clock" be optional?  On
> the other hand, let clock-frequency property be optional seems a little
> illogical to the concept of "fixed-clock".  So I'm really confused here.

I had envisioned fixed clocks as being clocks whose rates could never
change; obviously this is mostly useful for root clocks like
oscillators and whatnot.

There is nothing wrong with using fixed clock for sgtl5000-sys-mclk,
but it's rate *can* change if it's parent rate ever changes.  This may
be very unlikely on your platform, in which case again it is OK to use
fixed clock here if you want to.

However... I'm inclined to say that sgtl5000-sys-mclk is good
candidate for a dummy clock: it follows it's parent rate, doesn't
gate, doesn't divide it's parent rate, only has one input.  There
isn't a common dummy clock in the v4 patches, but there is in v5.  The
key difference between the fixed rate clock and the dummy clock is
that the dummy clock looks at clk->parent->rate in it's .get_rate,
whereas a fixed rate clock will have it's rate cached in struct
clk_fixed.

Thoughts?

Mike

> With the proper clock support, I would expect that sgtl5000 driver only
> needs to make the following two calls to have its clock enabled.
>
>        mclk = clk_get(&dev, "sgtl5000-sys-mclk");
>        clk_prepare_enable(mclk);
>
> But looking at the current fixed-clock implementation, there is even
> no clk_enable operation for fixed-clock.  How can we control the clock
> with gpio?
>
> All I'm saying is that we need some level of alignment between clock
> binding and common-clk implementation.
>
> Regards,
> Shawn
>
>> That said, I'm open to extending this binding if something can be
>> defined that will have a lot of use cases.

Re: [RFC v2 4/9] of: add clock providers

[Shawn Guo]

On Fri, Jan 13, 2012 at 08:30:30PM -0800, Turquette, Mike wrote:
...
> I had envisioned fixed clocks as being clocks whose rates could never
> change; obviously this is mostly useful for root clocks like
> oscillators and whatnot.
> 
> There is nothing wrong with using fixed clock for sgtl5000-sys-mclk,
> but it's rate *can* change if it's parent rate ever changes.  This may
> be very unlikely on your platform, in which case again it is OK to use
> fixed clock here if you want to.
> 
> However... I'm inclined to say that sgtl5000-sys-mclk is good
> candidate for a dummy clock: it follows it's parent rate, doesn't
> gate, doesn't divide it's parent rate, only has one input.  There
> isn't a common dummy clock in the v4 patches, but there is in v5.  The
> key difference between the fixed rate clock and the dummy clock is
> that the dummy clock looks at clk->parent->rate in it's .get_rate,
> whereas a fixed rate clock will have it's rate cached in struct
> clk_fixed.
> 
> Thoughts?
> 
I would say this modeling looks all sane to me, except both the
fixed-clock soc-26m-clk and dummy-clock sgtl5000-sys-mclk in this
case have a gate which is controlled by gpio.

-- 
Regards,
Shawn

RE: [RFC v2 4/9] of: add clock providers

[Stephen Warren]

Grant Likely wrote at Monday, December 12, 2011 3:02 PM:
> Based on work by Ben Herrenschmidt and Jeremy Kerr, this patch adds an
> of_clk_get function to allow platforms to retrieve clock data from the
> device tree.
> 
> Platform register a provider through of_clk_add_provider, which will be
> called when a device references the provider's OF node for a clock
> reference.
...
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt
...
> +==Example==
> +
> +    /* external oscillator */
> +    osc: oscillator {
> +        compatible = "fixed-clock";
> +        #clock-cells = <1>;
> +        clock-frequency  = <32678>;
> +        clock-output-names = "osc";
> +    };
> +
> +    /* phase-locked-loop device, generates a higher frequency clock
> +     * from the external oscillator reference */
> +    pll: pll@4c000 {
> +        compatible = "vendor,some-pll-interface"
> +        #clock-cells = <1>;
> +        clocks = <&osc 0>;
> +        clock-names = "ref";
> +        reg = <0x4c000 0x1000>;
> +        clock-output-names = "pll", "pll-switched";
> +    };
> +
> +    /* UART, using the low frequency oscillator for the baud clock,
> +     * and the high frequency switched PLL output for register
> +     * clocking */
> +    uart@a000 {
> +        compatible = "fsl,imx-uart";
> +        reg = <0xa000 0x1000>;
> +        interrupts = <33>;
> +        clocks = <&osc 0>, <&pll 1>;
> +        clock-names = "baud", "register";
> +    };
> +
> +This DT fragment defines three devices: an external oscillator to provide a
> +low-frequency reference clock, a PLL device to generate a higher frequency
> +clock signal, and a UART.
> +
> +* The oscillator is fixed-frequency, and provides one clock output, named "osc".
> +* The PLL is both a clock provider and a clock consumer. It uses the clock
> +  signal generated by the external oscillator, and provides two output signals
> +  ("pll" and "pll-switched").
> +* The UART has its baud clock connected the external oscillator and its
> +  register clock connected to the PLL clock (the "pll-switched" signal)

In the example above, the UART's register clock's parent is the PLL, and
the PLL's parent is the OSC. Is the intention to have this parenting set
up automatically by the core OF clock code, or is this something that each
individual driver needs to set up itself.

In other words, does the UART driver need to do something like:

clk_reg = clk_get(dev, "register");
clk_parent = of_clk_get_by_name(np, "register);
clk_set_parent(clk_reg, clk_parent);

Or will that all happen transparently within just the of_clk_get_by_name
call?

(I suppose this question makes slightly more sense for the PLL itself,
since both the upstream and downstream clocks are represented in the PLL
node, whereas the UART's node only represents the clock consumer side,
so the above code isn't really possible automatically).

Somewhat related to this: How does dynamic reparenting interact with
the DT clock binding; is the DT just the default/initial clock setup,
and anything beyond that needs a custom binding and code in the consumer?

I'm thinking of say a system with 1 I2S controller, and both an internal
and external I2S clock source, where perhaps the internal source needs
to be used to capture from an I2S interface on one set of pins (e.g.
analog mic) but the other clock source needs to be used to capture from
I2S on another set of pins (e.g. digital baseband unit connection).
(This example is theoretical, but I'm sure there are other dynamic clock
cases in practice).

-- 
nvpublic

RE: [RFC v2 7/9] arm/dt: Common plat-versatile support for icst and sp804 based system clocks

[Stephen Warren]

Grant Likely wrote at Monday, December 12, 2011 3:02 PM:
...
> diff --git a/arch/arm/plat-versatile/clock.c b/arch/arm/plat-versatile/clock.c
...
> +void __init versatile_dt_icst_clk_setup(struct device_node *np)
> +{
> +	const struct of_device_id *match;
> +	struct icst_of_clk *icst;
> +	const struct icst_params *params;
> +	u32 ref, vd_range[2], rd_range[2], vco_offset, lock_offset;
> +	__iomem void * base;
> +
> +	/*
> +	 * Only try to drive clocks that we know how to control.
> +	 * ie. attached to an ARM Versatile block of system registers
> +	 */
> +	if (!of_device_is_compatible(np->parent, "arm,versatile-sys-regs"))
> +		return;
...
> +	base = of_iomap(np->parent, 0);

Is this pattern (that of "re-using" a parent node's registers) something
legitimate that you'd expect to see wide-spread use?

I ask because Tegra's CAR (Clock And Reset) HW module controls all the
clocks in the system. The registers for the various clocks are often
inter-mingled, so it's not possible to give each clock a node with a
reg property, since they'd all overlap and conflict. Instead, should we
do something like the following, coupled with the above pattern in the
per-clock drivers:

car@60006000 {
    compatible = "nvidia,tegra20-car";
    reg = < 0x60006000 0x1000 >;

    osc@0 {
        compatible = "nvidia,tegra20-clk-osc";
    };
    pll_p@1 {
        compatible = "nvidia,tegra20-clk-pll";
        pll_id = <0>;
    };
    pll_c@2 {
        compatible = "nvidia,tegra20-clk-pll";
        pll_id = <1>;
    };
    ...
};

Similar, we have many "peripheral" clocks; leaf nodes that are the clocks
for UART, I2C, SPI, ... that are all controlled in a very similar fashion.
I can see a couple of alternative ways of representing this:

a)

One big node with a ton of clock inputs and outputs:

    periphclk {
        compatible = "nvidia,tegra20-clk-periphs";
        #clock-cells = <1>;
        /*
         * This ends up listing almost all clocks in the system;
         * most are usable as a source for /some/ peripheral clock
         */
        clocks = <&pll_p 0> <&pll_c 0> <&pll_m 0 ...>;
        clock-names = "pll_p", "pll_c", "pll_m", ...;
        /* And there are maybe 50 of these */
        clock-output-names = "uart1", "uart2", "i2c1", "i2c2", "spi1", ...;
    };

Simon Glass proposed something similar to this on the U-Boot mailing
list for Tegra's peripheral clocks (since he's working on upstreaming
Tegra USB support and adding device tree at the same time, and needs
to parameterize clock IDs in the device tree, since he's not using
AUXDATA). I'm worried that the huge number of peripheral clocks in the
above binding would be unwieldy. I'm also unclear how to know which of
the "clock-names" is the parent for each of the "clock-output-names",
or if this isn't something the current clock bindings are supports to
address. Each peripclk is an independent mux, divider, and gate.

b)

A node for each peripheral clock:

    uart1@0 {
        compatible = "nvidia,tegra20-clk-uart";
        clocks = <&pll_p 0>;
        clock-names = "clk";
    };

    uart2@1 {
        compatible = "nvidia,tegra20-clk-uart";
        clocks = <&pll_p 0>;
        clock-names = "clk";
    };

    i2s@2 {
        compatible = "nvidia,tegra20-clk-i2s";
        clocks = <&audio_clk 0>;
        clock-names = "clk";
    };

etc.

Thanks for cluing me in!

-- 
nvpublic

Re: [RFC v2 7/9] arm/dt: Common plat-versatile support for icst and sp804 based system clocks

[Rob Herring]

On 01/17/2012 03:05 PM, Stephen Warren wrote:
> Grant Likely wrote at Monday, December 12, 2011 3:02 PM:
> ...
>> diff --git a/arch/arm/plat-versatile/clock.c b/arch/arm/plat-versatile/clock.c
> ...
>> +void __init versatile_dt_icst_clk_setup(struct device_node *np)
>> +{
>> +	const struct of_device_id *match;
>> +	struct icst_of_clk *icst;
>> +	const struct icst_params *params;
>> +	u32 ref, vd_range[2], rd_range[2], vco_offset, lock_offset;
>> +	__iomem void * base;
>> +
>> +	/*
>> +	 * Only try to drive clocks that we know how to control.
>> +	 * ie. attached to an ARM Versatile block of system registers
>> +	 */
>> +	if (!of_device_is_compatible(np->parent, "arm,versatile-sys-regs"))
>> +		return;
> ...
>> +	base = of_iomap(np->parent, 0);
> 
> Is this pattern (that of "re-using" a parent node's registers) something
> legitimate that you'd expect to see wide-spread use?
> 

I don't think there is any clear direction for this. Probably doesn't
matter too much because it is typically things closely tied to a
particular SOC. If there's a whole bunch of random bits, it's not likely
to be reused on another SOC. If you can put registers into separate
nodes which are distinct functions even if you have multiple nodes
within a 4KB page you should.

> I ask because Tegra's CAR (Clock And Reset) HW module controls all the
> clocks in the system. The registers for the various clocks are often
> inter-mingled, so it's not possible to give each clock a node with a
> reg property, since they'd all overlap and conflict. Instead, should we
> do something like the following, coupled with the above pattern in the
> per-clock drivers:
> 
> car@60006000 {
>     compatible = "nvidia,tegra20-car";
>     reg = < 0x60006000 0x1000 >;
> 
>     osc@0 {
>         compatible = "nvidia,tegra20-clk-osc";
>     };
>     pll_p@1 {
>         compatible = "nvidia,tegra20-clk-pll";
>         pll_id = <0>;
>     };
>     pll_c@2 {
>         compatible = "nvidia,tegra20-clk-pll";
>         pll_id = <1>;
>     };
>     ...
> };

This is roughly how I'm doing clocks as a sub-node of the clock module.

> 
> Similar, we have many "peripheral" clocks; leaf nodes that are the clocks
> for UART, I2C, SPI, ... that are all controlled in a very similar fashion.
> I can see a couple of alternative ways of representing this:
> 
> a)
> 
> One big node with a ton of clock inputs and outputs:
> 
>     periphclk {
>         compatible = "nvidia,tegra20-clk-periphs";
>         #clock-cells = <1>;
>         /*
>          * This ends up listing almost all clocks in the system;
>          * most are usable as a source for /some/ peripheral clock
>          */
>         clocks = <&pll_p 0> <&pll_c 0> <&pll_m 0 ...>;
>         clock-names = "pll_p", "pll_c", "pll_m", ...;
>         /* And there are maybe 50 of these */
>         clock-output-names = "uart1", "uart2", "i2c1", "i2c2", "spi1", ...;
>     };
> 
> Simon Glass proposed something similar to this on the U-Boot mailing
> list for Tegra's peripheral clocks (since he's working on upstreaming
> Tegra USB support and adding device tree at the same time, and needs
> to parameterize clock IDs in the device tree, since he's not using
> AUXDATA). I'm worried that the huge number of peripheral clocks in the
> above binding would be unwieldy. I'm also unclear how to know which of
> the "clock-names" is the parent for each of the "clock-output-names",
> or if this isn't something the current clock bindings are supports to
> address. Each peripclk is an independent mux, divider, and gate.
> 

Both *-names properties are completely optional. Matching is typically
done by node and phandles rather than name strings. But without names it
is harder to know which clock a number in the cell is referring to as
clocks typically aren't documented that way as opposed to interrupts.

> b)
> 
> A node for each peripheral clock:
> 
>     uart1@0 {
>         compatible = "nvidia,tegra20-clk-uart";
>         clocks = <&pll_p 0>;
>         clock-names = "clk";
>     };
> 
>     uart2@1 {
>         compatible = "nvidia,tegra20-clk-uart";
>         clocks = <&pll_p 0>;
>         clock-names = "clk";
>     };
> 
>     i2s@2 {
>         compatible = "nvidia,tegra20-clk-i2s";
>         clocks = <&audio_clk 0>;
>         clock-names = "clk";
>     };
> 
> etc.
> 
> Thanks for cluing me in!

Either way is fine. I think the former is probably better if there are a
huge number of clocks.

Rob

Re: [RFC v2 4/9] of: add clock providers

[Grant Likely]

On Tue, Jan 17, 2012 at 1:44 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Grant Likely wrote at Monday, December 12, 2011 3:02 PM:
>> +This DT fragment defines three devices: an external oscillator to provide a
>> +low-frequency reference clock, a PLL device to generate a higher frequency
>> +clock signal, and a UART.
>> +
>> +* The oscillator is fixed-frequency, and provides one clock output, named "osc".
>> +* The PLL is both a clock provider and a clock consumer. It uses the clock
>> +  signal generated by the external oscillator, and provides two output signals
>> +  ("pll" and "pll-switched").
>> +* The UART has its baud clock connected the external oscillator and its
>> +  register clock connected to the PLL clock (the "pll-switched" signal)
>
> In the example above, the UART's register clock's parent is the PLL, and
> the PLL's parent is the OSC. Is the intention to have this parenting set
> up automatically by the core OF clock code, or is this something that each
> individual driver needs to set up itself.
>
> In other words, does the UART driver need to do something like:
>
> clk_reg = clk_get(dev, "register");
> clk_parent = of_clk_get_by_name(np, "register);
> clk_set_parent(clk_reg, clk_parent);
>
> Or will that all happen transparently within just the of_clk_get_by_name
> call?
>
> (I suppose this question makes slightly more sense for the PLL itself,
> since both the upstream and downstream clocks are represented in the PLL
> node, whereas the UART's node only represents the clock consumer side,
> so the above code isn't really possible automatically).

The intent is that device only interacts with the leaf device.  If the
clocks are arranged into a hierarchy, then the clock driver is
responsible for any interactions with the parent clock.  Requiring the
driver to manipulate parent clocks directly defeats the purpose of
having a clock abstraction.

> Somewhat related to this: How does dynamic reparenting interact with
> the DT clock binding; is the DT just the default/initial clock setup,
> and anything beyond that needs a custom binding and code in the consumer?

As far as the clock binding goes, it only describes provider/consumer
relationships.  The fact that such relationships may resolve to a
hierarchy is beyond what the binding describes.  If a clock has
multiple possible parents, then that specific clock binding should
document how the multiple parent clocks are described and the clock
driver is responsible for implementing the correct behaviour.

Similarly, the DT clock binding provides no generic mechanism for
walking up the clock tree.  That behaviour must also be implemented by
each specific clock driver.

> I'm thinking of say a system with 1 I2S controller, and both an internal
> and external I2S clock source, where perhaps the internal source needs
> to be used to capture from an I2S interface on one set of pins (e.g.
> analog mic) but the other clock source needs to be used to capture from
> I2S on another set of pins (e.g. digital baseband unit connection).
> (This example is theoretical, but I'm sure there are other dynamic clock
> cases in practice).

That is a reasonable example.  In this case, the i2c controller would
include both in its clocks property, and the binding would document
when and why each clock source is used.

g.

Re: [RFC v2 7/9] arm/dt: Common plat-versatile support for icst and sp804 based system clocks

[Grant Likely]

On Tue, Jan 17, 2012 at 2:05 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Grant Likely wrote at Monday, December 12, 2011 3:02 PM:
> ...
>> diff --git a/arch/arm/plat-versatile/clock.c b/arch/arm/plat-versatile/clock.c
> ...
>> +void __init versatile_dt_icst_clk_setup(struct device_node *np)
>> +{
>> +     const struct of_device_id *match;
>> +     struct icst_of_clk *icst;
>> +     const struct icst_params *params;
>> +     u32 ref, vd_range[2], rd_range[2], vco_offset, lock_offset;
>> +     __iomem void * base;
>> +
>> +     /*
>> +      * Only try to drive clocks that we know how to control.
>> +      * ie. attached to an ARM Versatile block of system registers
>> +      */
>> +     if (!of_device_is_compatible(np->parent, "arm,versatile-sys-regs"))
>> +             return;
> ...
>> +     base = of_iomap(np->parent, 0);
>
> Is this pattern (that of "re-using" a parent node's registers) something
> legitimate that you'd expect to see wide-spread use?

Yes, it is completely legitimate.  In this particular case, the
binding must specifically state that the child node uses the parent
node's reg region, and that would place strict limits on where this
binding can be used.

> I ask because Tegra's CAR (Clock And Reset) HW module controls all the
> clocks in the system. The registers for the various clocks are often
> inter-mingled, so it's not possible to give each clock a node with a
> reg property, since they'd all overlap and conflict. Instead, should we
> do something like the following, coupled with the above pattern in the
> per-clock drivers:
>
> car@60006000 {
>    compatible = "nvidia,tegra20-car";
>    reg = < 0x60006000 0x1000 >;
>
>    osc@0 {
>        compatible = "nvidia,tegra20-clk-osc";
>    };
>    pll_p@1 {
>        compatible = "nvidia,tegra20-clk-pll";
>        pll_id = <0>;
>    };
>    pll_c@2 {
>        compatible = "nvidia,tegra20-clk-pll";
>        pll_id = <1>;
>    };
>    ...
> };

Sure, that's fine.

>
> Similar, we have many "peripheral" clocks; leaf nodes that are the clocks
> for UART, I2C, SPI, ... that are all controlled in a very similar fashion.
> I can see a couple of alternative ways of representing this:
>
> a)
>
> One big node with a ton of clock inputs and outputs:
>
>    periphclk {
>        compatible = "nvidia,tegra20-clk-periphs";
>        #clock-cells = <1>;
>        /*
>         * This ends up listing almost all clocks in the system;
>         * most are usable as a source for /some/ peripheral clock
>         */
>        clocks = <&pll_p 0> <&pll_c 0> <&pll_m 0 ...>;
>        clock-names = "pll_p", "pll_c", "pll_m", ...;
>        /* And there are maybe 50 of these */
>        clock-output-names = "uart1", "uart2", "i2c1", "i2c2", "spi1", ...;
>    };
>
> Simon Glass proposed something similar to this on the U-Boot mailing
> list for Tegra's peripheral clocks (since he's working on upstreaming
> Tegra USB support and adding device tree at the same time, and needs
> to parameterize clock IDs in the device tree, since he's not using
> AUXDATA). I'm worried that the huge number of peripheral clocks in the
> above binding would be unwieldy.

You've got a lot of latitude here.  This is fine, and I think for a
lot of cases, particularly if there is a kind of 'clock nexus' in an
soc, that it is an excellent option.  You can also have one node per
clock, or you can do something in between where clocks get logically
grouped together based on the system design.  It is also true that
there is no need to expose every soc clock in the dts.  Only the leaf
clocks provided to devices need to appear.  Internal intermediary
clocks can, and probably should, be hidden from the devices.

> I'm also unclear how to know which of
> the "clock-names" is the parent for each of the "clock-output-names",
> or if this isn't something the current clock bindings are supports to
> address. Each peripclk is an independent mux, divider, and gate.

The common binding doesn't say anything about this.  It is entirely up
to the specific clock device binding to specify how consumed clocks
map to produced clocks.

>
> b)
>
> A node for each peripheral clock:
>
>    uart1@0 {
>        compatible = "nvidia,tegra20-clk-uart";
>        clocks = <&pll_p 0>;
>        clock-names = "clk";
>    };
>
>    uart2@1 {
>        compatible = "nvidia,tegra20-clk-uart";
>        clocks = <&pll_p 0>;
>        clock-names = "clk";
>    };
>
>    i2s@2 {
>        compatible = "nvidia,tegra20-clk-i2s";
>        clocks = <&audio_clk 0>;
>        clock-names = "clk";
>    };
>
> etc.

My primary worry here is that it will result in an unwieldy number of
nodes in a hurry.

g.

Re: [RFC v2 4/9] of: add clock providers

[Turquette, Mike]

On Tue, Jan 17, 2012 at 2:47 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Tue, Jan 17, 2012 at 1:44 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> In other words, does the UART driver need to do something like:
>>
>> clk_reg = clk_get(dev, "register");
>> clk_parent = of_clk_get_by_name(np, "register);
>> clk_set_parent(clk_reg, clk_parent);
>>
>> Or will that all happen transparently within just the of_clk_get_by_name
>> call?
>>
>> (I suppose this question makes slightly more sense for the PLL itself,
>> since both the upstream and downstream clocks are represented in the PLL
>> node, whereas the UART's node only represents the clock consumer side,
>> so the above code isn't really possible automatically).
>
> The intent is that device only interacts with the leaf device.  If the
> clocks are arranged into a hierarchy, then the clock driver is
> responsible for any interactions with the parent clock.  Requiring the
> driver to manipulate parent clocks directly defeats the purpose of
> having a clock abstraction.

I don't think that we can get rid of all instances of drivers knowing
a bit about hierarchy.  I think we can get rid of most, but there are
cases where it is valid for a driver to know some of the details.
More on that below.

>> Somewhat related to this: How does dynamic reparenting interact with
>> the DT clock binding; is the DT just the default/initial clock setup,
>> and anything beyond that needs a custom binding and code in the consumer?
>
> As far as the clock binding goes, it only describes provider/consumer
> relationships.  The fact that such relationships may resolve to a
> hierarchy is beyond what the binding describes.  If a clock has
> multiple possible parents, then that specific clock binding should
> document how the multiple parent clocks are described and the clock
> driver is responsible for implementing the correct behaviour.

It also deserves to be said that the DT data says nothing about which
of the possible parents _should_ be the input to a mux clock.  It's
pretty common to want to make changes to hierarchy after taking a
device out of reset, since the reset values for a clock management IP
might be pretty conservative.  So someone, somewhere must know some
details about hierarchy and set things up correctly.  Maybe a "clock
driver" can do this, but for specific IPs such as the audio example
below it makes sense for that driver to have the knowledge.

> Similarly, the DT clock binding provides no generic mechanism for
> walking up the clock tree.  That behaviour must also be implemented by
> each specific clock driver.
>
>> I'm thinking of say a system with 1 I2S controller, and both an internal
>> and external I2S clock source, where perhaps the internal source needs
>> to be used to capture from an I2S interface on one set of pins (e.g.
>> analog mic) but the other clock source needs to be used to capture from
>> I2S on another set of pins (e.g. digital baseband unit connection).
>> (This example is theoretical, but I'm sure there are other dynamic clock
>> cases in practice).
>
> That is a reasonable example.  In this case, the i2c controller would
> include both in its clocks property, and the binding would document
> when and why each clock source is used.

I'm confused on this point.  How does the binding "document when and
why each clock is used"?  In the case where this I2S controller
expects to dynamically switch roles at run-time (analog mic versus
baseband) then clk_set_parent must still be invoked by the driver.  To
be clear I'm imagining the above example like:

i_I2S   e_I2S
   \       /
    I2S_mux
        |
I2S controller IP

Regards,
Mike

Re: [RFC v2 4/9] of: add clock providers

[Grant Likely]

On Tue, Jan 17, 2012 at 4:37 PM, Turquette, Mike <mturquette@ti.com> wrote:
> On Tue, Jan 17, 2012 at 2:47 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Tue, Jan 17, 2012 at 1:44 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> In other words, does the UART driver need to do something like:
>>>
>>> clk_reg = clk_get(dev, "register");
>>> clk_parent = of_clk_get_by_name(np, "register);
>>> clk_set_parent(clk_reg, clk_parent);
>>>
>>> Or will that all happen transparently within just the of_clk_get_by_name
>>> call?
>>>
>>> (I suppose this question makes slightly more sense for the PLL itself,
>>> since both the upstream and downstream clocks are represented in the PLL
>>> node, whereas the UART's node only represents the clock consumer side,
>>> so the above code isn't really possible automatically).
>>
>> The intent is that device only interacts with the leaf device.  If the
>> clocks are arranged into a hierarchy, then the clock driver is
>> responsible for any interactions with the parent clock.  Requiring the
>> driver to manipulate parent clocks directly defeats the purpose of
>> having a clock abstraction.
>
> I don't think that we can get rid of all instances of drivers knowing
> a bit about hierarchy.  I think we can get rid of most, but there are
> cases where it is valid for a driver to know some of the details.
> More on that below.
>
>>> Somewhat related to this: How does dynamic reparenting interact with
>>> the DT clock binding; is the DT just the default/initial clock setup,
>>> and anything beyond that needs a custom binding and code in the consumer?
>>
>> As far as the clock binding goes, it only describes provider/consumer
>> relationships.  The fact that such relationships may resolve to a
>> hierarchy is beyond what the binding describes.  If a clock has
>> multiple possible parents, then that specific clock binding should
>> document how the multiple parent clocks are described and the clock
>> driver is responsible for implementing the correct behaviour.
>
> It also deserves to be said that the DT data says nothing about which
> of the possible parents _should_ be the input to a mux clock.  It's
> pretty common to want to make changes to hierarchy after taking a
> device out of reset, since the reset values for a clock management IP
> might be pretty conservative.  So someone, somewhere must know some
> details about hierarchy and set things up correctly.  Maybe a "clock
> driver" can do this, but for specific IPs such as the audio example
> below it makes sense for that driver to have the knowledge.
>
>> Similarly, the DT clock binding provides no generic mechanism for
>> walking up the clock tree.  That behaviour must also be implemented by
>> each specific clock driver.
>>
>>> I'm thinking of say a system with 1 I2S controller, and both an internal
>>> and external I2S clock source, where perhaps the internal source needs
>>> to be used to capture from an I2S interface on one set of pins (e.g.
>>> analog mic) but the other clock source needs to be used to capture from
>>> I2S on another set of pins (e.g. digital baseband unit connection).
>>> (This example is theoretical, but I'm sure there are other dynamic clock
>>> cases in practice).
>>
>> That is a reasonable example.  In this case, the i2c controller would
>> include both in its clocks property, and the binding would document
>> when and why each clock source is used.
>
> I'm confused on this point.  How does the binding "document when and
> why each clock is used"?  In the case where this I2S controller
> expects to dynamically switch roles at run-time (analog mic versus
> baseband) then clk_set_parent must still be invoked by the driver.  To
> be clear I'm imagining the above example like:
>
> i_I2S   e_I2S
>   \       /
>    I2S_mux
>        |
> I2S controller IP

>From the description, I assumed that the i2s_mux was part of the i2s
controller driver which would select the appropriate clock based on
the configured mode.  If it is better to implement it as a separate
clock driver, then yes the i2s controller must have knowledge of that
and call clk_set_parent appropriately.  Regardless, some driver needs
to explicitly understand the relationship between pinmux and clock
source.

g.

RE: [RFC v2 4/9] of: add clock providers

[Stephen Warren]

Turquette, Mike wrote at Tuesday, January 17, 2012 4:37 PM:
> On Tue, Jan 17, 2012 at 2:47 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Tue, Jan 17, 2012 at 1:44 PM, Stephen Warren <swarren@nvidia.com> wrote:
> >> In other words, does the UART driver need to do something like:
> >>
> >> clk_reg = clk_get(dev, "register");
> >> clk_parent = of_clk_get_by_name(np, "register);
> >> clk_set_parent(clk_reg, clk_parent);
> >>
> >> Or will that all happen transparently within just the of_clk_get_by_name
> >> call?
> >>
> >> (I suppose this question makes slightly more sense for the PLL itself,
> >> since both the upstream and downstream clocks are represented in the PLL
> >> node, whereas the UART's node only represents the clock consumer side,
> >> so the above code isn't really possible automatically).
> >
> > The intent is that device only interacts with the leaf device.  If the
> > clocks are arranged into a hierarchy, then the clock driver is
> > responsible for any interactions with the parent clock.  Requiring the
> > driver to manipulate parent clocks directly defeats the purpose of
> > having a clock abstraction.

Ah OK, so the clock bindings are purely a way of naming the clocks that
devices use; nothing beyond that.

> I don't think that we can get rid of all instances of drivers knowing
> a bit about hierarchy.  I think we can get rid of most, but there are
> cases where it is valid for a driver to know some of the details.
> More on that below.
> 
> >> Somewhat related to this: How does dynamic reparenting interact with
> >> the DT clock binding; is the DT just the default/initial clock setup,
> >> and anything beyond that needs a custom binding and code in the consumer?
> >
> > As far as the clock binding goes, it only describes provider/consumer
> > relationships.  The fact that such relationships may resolve to a
> > hierarchy is beyond what the binding describes.  If a clock has
> > multiple possible parents, then that specific clock binding should
> > document how the multiple parent clocks are described and the clock
> > driver is responsible for implementing the correct behaviour.
> 
> It also deserves to be said that the DT data says nothing about which
> of the possible parents _should_ be the input to a mux clock.  It's
> pretty common to want to make changes to hierarchy after taking a
> device out of reset, since the reset values for a clock management IP
> might be pretty conservative.  So someone, somewhere must know some
> details about hierarchy and set things up correctly.  Maybe a "clock
> driver" can do this, but for specific IPs such as the audio example
> below it makes sense for that driver to have the knowledge.
> 
> > Similarly, the DT clock binding provides no generic mechanism for
> > walking up the clock tree.  That behaviour must also be implemented by
> > each specific clock driver.
> >
> >> I'm thinking of say a system with 1 I2S controller, and both an internal
> >> and external I2S clock source, where perhaps the internal source needs
> >> to be used to capture from an I2S interface on one set of pins (e.g.
> >> analog mic) but the other clock source needs to be used to capture from
> >> I2S on another set of pins (e.g. digital baseband unit connection).
> >> (This example is theoretical, but I'm sure there are other dynamic clock
> >> cases in practice).
> >
> > That is a reasonable example.  In this case, the i2c controller would
> > include both in its clocks property, and the binding would document
> > when and why each clock source is used.
> 
> I'm confused on this point.  How does the binding "document when and
> why each clock is used"?  In the case where this I2S controller
> expects to dynamically switch roles at run-time (analog mic versus
> baseband) then clk_set_parent must still be invoked by the driver.  To
> be clear I'm imagining the above example like:
> 
> i_I2S   e_I2S
>    \       /
>     I2S_mux
>         |
> I2S controller IP

Here's how I envisage this working specifically for the case of some
of Tegra's audio clocking:

There's an oscillator on the board:

osc: pclk {
    compatible = "fixed-clock";
    #clock-cells = <0>;
    clock-frequency = <32768>;
};

(that part might not be quite right, but it's not too important)

The Tegra CAR (Clock And Reset) controller is pretty bare; we rely on
the driver internally defining what all the clocks are, and at most
listing all the clocks that this block imports/exports:

    tegra_car: car@60006000 {
        compatible = "nvidia,tegra20-car";
        reg = < 0x60006000 0x1000 >;
        #clock-cells = <1>;
        clocks = <&osc 0>;
        clock-names = "osc";
        clock-output-names = "pll_a", "audio_2x", "audio", "i2s1", ...;
    };

The I2S controller is pretty dumb, and just receives a single clock that
it enables/disables based on when registers are being touched or audio
is streaming:

tegra_i2s1: i2s@70002800 {
    compatible = "nvidia,tegra20-i2s";
    reg = <0x70002800 0x200>;
    clocks = <&tegra_car 3>;
    clock-names = "clk";
};

So in fact, zero clock /tree/ knowledge there really.

All the clock tree setup is already handled by the ASoC machine driver.
I've already posted a binding for this for the Tegra-with-WM8903-codec
case. Right now, that driver hard-codes the clock names (it's the same
driver from before the days of device tree), but I'll update it to
extract them from device tree using this new binding. Since it touches
the audio PLL, an audio-specific mux, and the I2S clocks, that module
needs references to all of those:

sound {
    compatible = "nvidia,tegra-audio-wm8903-harmony",
                 "nvidia,tegra-audio-wm8903";
    clocks = <&tegra_car 0 &tegra_car 1 &tegra_car 2 &tegra_car 3>;
    clock-names = "pll_a", "audio_2x", "audio", "i2s1";
};

When this top-level sound driver probes, it'll call of_clk_get_by_name()
on each of those, and call clk_set_parent() on the appropriate pairs.
The binding for nvidia,tegra-audio-wm8903 will define the list of
clocks/clock-names that it requires to make this work. Later on, this
top-level driver also calls clk_set_rate() on pll_a to switch between
44.1KHz/48KHz-based audio, etc.


If the clock mux were inside the I2C controller for some other HW, the
I2C node would contain both those clocks and operate on them in a similar
fashion, and the binding would need to say what it expected those two
clocks to represent.

-- 
nvpublic