[RFC] Clock binding

[RFC] Clock binding

[Benjamin Herrenschmidt]

So here's a followup to my discussion about the clock API.

I'm cooking up a patch that replace our current primitive implementation
in arch/powerpc/kernel/clock.c with something along the lines of what I
described. However, I want a bit more churn here on the device-tree
related bits.

So, basically, the goal here is to define a binding so that we can link
a device clock inputs to a clock provider clock outputs.

In general, in a system, there's actually 3 "names" involved. The clock
provider output name, the clock signal name, and the clock input name on
the device. However, I want to avoid involving the clock signal name as
it's a "global" name and it will just end up being a mess if we start
exposing that.

So basically, it boils down to a device having some clock inputs,
referenced by names, that need to be linked to another node which is a
clock provider, which has outputs, references either by number or names,
see discussion below.

First, why names, and not numbers ? IE. It's the OF "tradition" for
resources to just be an array, like interrupts, or address ranges in
"reg" properties, and one has to know what the Nth interrupt correspond
too.

My answer here is that maybe the tradition but it's crap :-) Names are
much better in the long run, besides it makes it easier to represent if
not all inputs have been wired. Also, to some extent, things like PCI do
encode a "name" with "reg" or "assigned-addresses" properties as part of
the config space offset in the top part of the address, and that has
proved very useful.

Thus I think using names is the way to go, and we should even generalize
that and add a new "interrupt-names" property to name the members of an
"interrupts" :-)

So back to the subject at hand. That leaves us with having to populate
the driver with some kind of map (I call it clock-map). Ideally, if
everything is named, which is the best approach imho, that map would
bind a list of:

	- clock input name
	- clock provider phandle
	- clock output name on provider

However, it's a bit nasty to mix strings and numbers (phandles) in a
single property. It's possible, but would likely lead to the phandle not
being aligned and tools such as lsprop to fail miserably to display
those properties in any kind of readable form.

My earlier emails proposed an approach like this:

	- clock input names go into a "clock-names" property
	  (which I suggest naming instead "clock-input-names" btw)

	- the map goes into a "clock-map" property and for each input
	  provides a phandle and a one cell numerical ID that identifies
	  the clock on the source.

However, I really dislike that numerical clock ID. Magic numbers suck.
It should be a string. But I don't want to add a 3rd property in there.

Hence my idea below. It's not perfect but it's the less sucky i've come
up with so far. And then we can do some small refinements.

	* Device has:

		- "clock-input-names" as above
		- "clock-map" contains list of phandle,index

	* Clock source has:

		- "clock-output-names" list of strings

The "index" in the clock map thus would reference the
"clock-output-names" array in the clock provider. That means that the
"magic number" here is entirely local to a given device-tree, doesn't
leak into driver code, which continues using names.

In addition, we can even have some smooth "upgrade" path from existing
"clock-frequency" properties by assuming that if "clock-output-names" is
absent, but "clock-frequency" exist, then index 0 references a fixed
frequency clock source without a driver. This could be generally handy
anyway to represent crystals of fixed bus clocks without having to write
a clock source driver for them.

Any comments ?

I'll post a patch, maybe later today, implementing the above (I may or
may not have time to also convert the existing 512x code to it, we'll
see).

Cheers,
Ben.


 

[RFC/PATCH] Clock binding prototype implementation

[Benjamin Herrenschmidt]

powerpc: New implementation of the "clk" API with device-tree binding

This replaces the struct clk_interface, as far as I know only ever
used by the mpc512x platforms, with a new scheme:

struct clk is now an object containing function pointers for all the
base API routines. The implementation of those routines call into
those pointers. struct clk is meant to be embedded in the "real"
structure for a given clock type.

clk_get is hooked via ppc_md. A default implementation is provided
that uses the device-tree binding (still being discussed, a patch
to Documentation/* will come when it's final). It can also create
simple objects (no control, working get_rate()) based on nodes that
have a "clock-frequency" property.

Finally, the mpc512x code is adapted. The adaptation is minimal as
I don't have test gear, so I'm not using nor touching the device-tree
on this one, but instead hooking ppc_md.get_clk() and keeping the
old code mostly intact (just some type changes to embed the new
struct clk into the mpc512x specific variants which gets renamed
to struct mpc512x_clk).

No S-O-B yet as this is totally untested and the binding isn't
final yet neither, but gives an idea of where I'm going.

Ben.

Index: linux-work/arch/powerpc/include/asm/clk_interface.h
===================================================================
--- linux-work.orig/arch/powerpc/include/asm/clk_interface.h	2009-02-05 16:22:24.000000000 +1100
+++ /dev/null	1970-01-01 00:00:00.000000000 +0000
@@ -1,20 +0,0 @@
-#ifndef __ASM_POWERPC_CLK_INTERFACE_H
-#define __ASM_POWERPC_CLK_INTERFACE_H
-
-#include <linux/clk.h>
-
-struct clk_interface {
-	struct clk*	(*clk_get)	(struct device *dev, const char *id);
-	int		(*clk_enable)	(struct clk *clk);
-	void		(*clk_disable)	(struct clk *clk);
-	unsigned long	(*clk_get_rate)	(struct clk *clk);
-	void		(*clk_put)	(struct clk *clk);
-	long		(*clk_round_rate) (struct clk *clk, unsigned long rate);
-	int 		(*clk_set_rate)	(struct clk *clk, unsigned long rate);
-	int		(*clk_set_parent) (struct clk *clk, struct clk *parent);
-	struct clk*	(*clk_get_parent) (struct clk *clk);
-};
-
-extern struct clk_interface clk_functions;
-
-#endif /* __ASM_POWERPC_CLK_INTERFACE_H */
Index: linux-work/arch/powerpc/kernel/clock.c
===================================================================
--- linux-work.orig/arch/powerpc/kernel/clock.c	2009-02-05 16:22:24.000000000 +1100
+++ linux-work/arch/powerpc/kernel/clock.c	2009-08-18 17:38:08.000000000 +1000
@@ -1,82 +1,282 @@
 /*
- * Dummy clk implementations for powerpc.
- * These need to be overridden in platform code.
+ * Clock infrastructure for PowerPC platform
+ *
  */
 
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/module.h>
-#include <asm/clk_interface.h>
+#include <linux/of.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <asm/clk.h>
+#include <asm/machdep.h>
+
+struct clk_provider {
+	struct list_head	link;
+	struct device_node     	*node;
+
+	/* Return NULL if no such clock output, return PTR_ERR for
+	 * other errors
+	 */
+	struct clk *		(*get)(struct device_node *np,
+				       const char *output_id,
+				       void *data);
+	void			*data;
+};
+
+static LIST_HEAD(clk_providers);
+static DEFINE_MUTEX(clk_lock);
+
+int clk_add_provider(struct device_node *np,
+		     struct clk *(*clk_src_get)(struct device_node *np,
+						const char *output_id,
+						void *data),
+		     void *data)
+{
+	struct clk_provider *cp;
 
-struct clk_interface clk_functions;
+	cp = kzalloc(sizeof(struct clk_provider), GFP_KERNEL);
+	if (!cp)
+		return -ENOMEM;
+
+	cp->node = of_node_get(np);
+	cp->data = data;
+	cp->get = clk_src_get;
+
+	mutex_lock(&clk_lock);
+	list_add(&cp->link, &clk_providers);
+	mutex_unlock(&clk_lock);
 
-struct clk *clk_get(struct device *dev, const char *id)
+	return 0;
+}
+
+void clk_del_provider(struct device_node *np,
+		      struct clk *(*clk_src_get)(struct device_node *np,
+						 const char *output_id,
+						 void *data),
+		      void *data)
 {
-	if (clk_functions.clk_get)
-		return clk_functions.clk_get(dev, id);
-	return ERR_PTR(-ENOSYS);
+	struct clk_provider *cp, *tmp;
+
+	mutex_lock(&clk_lock);
+	list_for_each_entry_safe(cp, tmp, &clk_providers, link) {
+		if (cp->node == np && cp->get == clk_src_get && cp->data == data) {
+			list_del(&cp->link);
+			of_node_put(cp->node);
+			kfree(cp);
+			break;
+		}
+	}
+	mutex_unlock(&clk_lock);
 }
-EXPORT_SYMBOL(clk_get);
 
-void clk_put(struct clk *clk)
+static unsigned long clk_simple_get_rate(struct clk *clk)
 {
-	if (clk_functions.clk_put)
-		clk_functions.clk_put(clk);
+	struct clk_simple *cs = to_clk_simple(clk);
+
+	return cs->rate;
 }
-EXPORT_SYMBOL(clk_put);
+
+static void clk_simple_put(struct clk *clk)
+{
+	struct clk_simple *cs = to_clk_simple(clk);
+
+	kfree(cs);
+}
+
+static struct clk *__clk_get_simple(struct device_node *np)
+{
+	const u32 *freq;
+	struct clk_simple *cs;
+
+	freq = of_get_property(np, "clock-frequency", NULL);
+	if (!freq)
+		return ERR_PTR(-ENXIO);
+
+	cs = kzalloc(sizeof(struct clk_simple), GFP_KERNEL);
+	if (!cs)
+		return ERR_PTR(-ENOMEM);
+
+	cs->rate = *freq;
+	cs->clk.get_rate = clk_simple_get_rate;
+	cs->clk.put = clk_simple_put;
+
+	return &cs->clk;
+}
+
+static struct clk *__clk_get_from_provider(struct device_node *np, u32 index)
+{
+	struct clk_provider *provider;
+	const char *names, *id = NULL;
+	int sz, l, i;
+	struct clk *clk = NULL;
+
+	/* Look for "clock-output-names" in the provider's node, if it's missing
+	 * we don't bother looking for a clock provider, though we do fallback
+	 * to the "simple" clock case
+	 */
+	names = of_get_property(np, "clock-output-names", &sz);
+	if (!names)
+		return __clk_get_simple(np);
+
+	/* Lookup index */
+	for (i = 0; sz > 0; i++) {
+		if (index == i) {
+			id = names;
+			break;
+		}
+		l = strlen(names) + 1;
+		sz -= l;
+		names += l;
+	}
+	if (id == NULL)
+		return ERR_PTR(-ENXIO);
+
+	/* Check if we have such a provider in our array */
+	mutex_lock(&clk_lock);
+	list_for_each_entry(provider, &clk_providers, link) {
+		if (provider->node == np)
+			clk = provider->get(np, id, provider->data);
+		if (clk)
+			break;
+	}
+	mutex_unlock(&clk_lock);
+
+	/* Clock not found, return an error */
+	if (clk == NULL)
+		clk = ERR_PTR(-EINVAL);
+
+	return clk;
+}
+
+struct clk *of_clk_get(struct device_node *np, const char *id)
+{
+	int sz, l, index = 0;
+	struct device_node *provnode;
+	const u32 *map;
+	u32 provhandle, provindex;
+	struct clk *clk;
+
+	/* look for an id match in clock-names property */
+	if (id) {
+		const char *names = of_get_property(np, "clock-input-names", &sz);
+
+		/* no such property, fail for now. Maybe in the long run we might
+		 * consider allowing "generic" names such as "bus" for the bus
+		 * clock, effectively routing to the parent node...
+		 */
+		if (!names)
+			return ERR_PTR(-ENXIO);
+		while (sz > 0) {
+			if (strcasecmp(id, names) == 0)
+				break;
+			l = strlen(names) + 1;
+			sz -= l;
+			names += l;
+			index++;
+		}
+		if (sz < 1)
+			return ERR_PTR(-ENOENT);
+	}
+
+	/* now we have an index, lookup in clock-map */
+	map = of_get_property(np, "clock-map", &sz);
+	if (!map)
+		return ERR_PTR(-ENXIO);
+	sz /= sizeof(u32) * 2;
+	if (index >= sz)
+		return ERR_PTR(-EINVAL);
+	provhandle = map[index * 2];
+	provindex = map[index * 2] + 1;
+
+	provnode = of_find_node_by_phandle(provhandle);
+	if (!provnode)
+		return ERR_PTR(-EINVAL);
+
+	clk = __clk_get_from_provider(provnode, provindex);
+	of_node_put(provnode);
+
+	return clk;
+}
+
+struct clk *clk_get(struct device *dev, const char *id)
+{
+	if (ppc_md.clk_get)
+		return ppc_md.clk_get(dev, id);
+
+	if (dev->archdata.of_node)
+		return of_clk_get(dev->archdata.of_node, id);
+
+	return ERR_PTR(-ENODEV);
+}
+EXPORT_SYMBOL(clk_get);
+
+/*
+ * clk_* API is just wrappers to the function pointers
+ * inside of struct clk
+ */
 
 int clk_enable(struct clk *clk)
 {
-	if (clk_functions.clk_enable)
-		return clk_functions.clk_enable(clk);
-	return -ENOSYS;
+	if (clk->enable)
+		return clk->enable(clk);
+	return 0;
 }
 EXPORT_SYMBOL(clk_enable);
 
 void clk_disable(struct clk *clk)
 {
-	if (clk_functions.clk_disable)
-		clk_functions.clk_disable(clk);
+	if (clk->disable)
+		clk->disable(clk);
 }
 EXPORT_SYMBOL(clk_disable);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
-	if (clk_functions.clk_get_rate)
-		return clk_functions.clk_get_rate(clk);
+	if (clk->get_rate)
+		return clk->get_rate(clk);
 	return 0;
 }
 EXPORT_SYMBOL(clk_get_rate);
 
+void clk_put(struct clk *clk)
+{
+	if (clk->put)
+		clk->put(clk);
+}
+EXPORT_SYMBOL(clk_put);
+
 long clk_round_rate(struct clk *clk, unsigned long rate)
 {
-	if (clk_functions.clk_round_rate)
-		return clk_functions.clk_round_rate(clk, rate);
+	if (clk->round_rate)
+		return clk->round_rate(clk, rate);
 	return -ENOSYS;
 }
 EXPORT_SYMBOL(clk_round_rate);
 
 int clk_set_rate(struct clk *clk, unsigned long rate)
 {
-	if (clk_functions.clk_set_rate)
-		return clk_functions.clk_set_rate(clk, rate);
+	if (clk->set_rate)
+		return clk->set_rate(clk, rate);
 	return -ENOSYS;
 }
 EXPORT_SYMBOL(clk_set_rate);
 
-struct clk *clk_get_parent(struct clk *clk)
-{
-	if (clk_functions.clk_get_parent)
-		return clk_functions.clk_get_parent(clk);
-	return ERR_PTR(-ENOSYS);
-}
-EXPORT_SYMBOL(clk_get_parent);
-
 int clk_set_parent(struct clk *clk, struct clk *parent)
 {
-	if (clk_functions.clk_set_parent)
-		return clk_functions.clk_set_parent(clk, parent);
+	if (clk->set_parent)
+		return clk->set_parent(clk, parent);
 	return -ENOSYS;
 }
 EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	if (clk->get_parent)
+		return clk->get_parent(clk);
+	return ERR_PTR(-ENOSYS);
+}
+EXPORT_SYMBOL(clk_get_parent);
Index: linux-work/arch/powerpc/include/asm/clk.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-work/arch/powerpc/include/asm/clk.h	2009-08-18 17:38:08.000000000 +1000
@@ -0,0 +1,61 @@
+/*
+ * Clock infrastructure for PowerPC platform
+ */
+#ifndef __ASM_POWERPC_CLK_H
+#define __ASM_POWERPC_CLK_H
+
+struct device_node;
+
+/* The definition of struct clk for arch/powerpc is global, it's
+ * expected that clock providers generate "subclasses" embedding
+ * struct clk
+ */
+struct clk {
+	int		(*enable)(struct clk *);
+	void		(*disable)(struct clk *);
+	unsigned long	(*get_rate)(struct clk *);
+	void		(*put)(struct clk *);
+	long		(*round_rate)(struct clk *, unsigned long);
+	int		(*set_rate)(struct clk *, unsigned long);
+	int		(*set_parent)(struct clk *, struct clk *);
+	struct clk*	(*get_parent)(struct clk *);
+};
+
+/* The simple clock is exposed here for implementations that want
+ * to re-use it in case they don't need anything else
+ */
+struct clk_simple {
+	struct clk	clk;
+	unsigned long	rate;
+};
+#define	to_clk_simple(n) container_of(n, struct clk_simple, clk)
+
+
+/**
+ * clk_add_provier - Attach a clock provider to a clock source device node
+ * @node: device node of the clock source
+ * @clk_src_get: function pointer called to obtain a struct clk
+ * @data: arbitrary data passed back to clk_src_get() when called
+ *
+ * This will attach the clk_src_get() function to a given device_node,
+ * that function will then be called whenever a drivers does a clk_get()
+ * and that driver's clock-map references that clock source node.
+ */
+extern int clk_add_provider(struct device_node *np,
+			    struct clk *(*clk_src_get)(struct device_node *np,
+						       const char *output_id,
+						       void *data),
+			    void *data);
+/**
+ * clk_del_provier - Remove a clock provider from a clock source device node
+ * @node: device node of the clock source
+ * @clk_src_get: function pointer used in clk_add_provider()
+ * @data: data used in clk_add_provider()
+ */
+extern void clk_del_provider(struct device_node *np,
+			     struct clk *(*clk_src_get)(struct device_node *np,
+							const char *output_id,
+							void *data),
+			     void *data);
+
+#endif /* __ASM_POWERPC_CLK_H */
Index: linux-work/arch/powerpc/include/asm/machdep.h
===================================================================
--- linux-work.orig/arch/powerpc/include/asm/machdep.h	2009-08-18 17:36:53.000000000 +1000
+++ linux-work/arch/powerpc/include/asm/machdep.h	2009-08-18 17:38:08.000000000 +1000
@@ -27,6 +27,8 @@ struct iommu_table;
 struct rtc_time;
 struct file;
 struct pci_controller;
+struct clk;
+struct device;
 #ifdef CONFIG_KEXEC
 struct kimage;
 #endif
@@ -266,7 +268,11 @@ struct machdep_calls {
 	 */
 	void (*suspend_disable_irqs)(void);
 	void (*suspend_enable_irqs)(void);
-#endif
+#endif /* CONFIG_SUSPEND */
+
+#ifdef CONFIG_PPC_CLOCK
+	struct clk *(*clk_get)(struct device *dev, const char *id);
+#endif /* CONFIG_PPC_CLOCK */
 };
 
 extern void e500_idle(void);
Index: linux-work/arch/powerpc/platforms/512x/clock.c
===================================================================
--- linux-work.orig/arch/powerpc/platforms/512x/clock.c	2009-08-18 17:33:15.000000000 +1000
+++ linux-work/arch/powerpc/platforms/512x/clock.c	2009-08-18 17:38:30.000000000 +1000
@@ -25,7 +25,7 @@
 
 #include <linux/of_platform.h>
 #include <asm/mpc5xxx.h>
-#include <asm/clk_interface.h>
+#include <asm/clk.h>
 
 #undef CLK_DEBUG
 
@@ -34,25 +34,28 @@ static int clocks_initialized;
 #define CLK_HAS_RATE	0x1	/* has rate in MHz */
 #define CLK_HAS_CTRL	0x2	/* has control reg and bit */
 
-struct clk {
+struct mpc512x_clk {
+	struct clk clk;
 	struct list_head node;
 	char name[32];
 	int flags;
 	struct device *dev;
 	unsigned long rate;
 	struct module *owner;
-	void (*calc) (struct clk *);
-	struct clk *parent;
+	void (*calc) (struct mpc512x_clk *);
+	struct mpc512x_clk *parent;
 	int reg, bit;		/* CLK_HAS_CTRL */
 	int div_shift;		/* only used by generic_div_clk_calc */
 };
+#define	to_mpc512x_clk(n) container_of(n, struct mpc512x_clk, clk)
 
 static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);
 
 static struct clk *mpc5121_clk_get(struct device *dev, const char *id)
 {
-	struct clk *p, *clk = ERR_PTR(-ENOENT);
+	struct clk *clk = ERR_PTR(-ENOENT);
+	struct mpc512x_clk *p;
 	int dev_match = 0;
 	int id_match = 0;
 
@@ -66,7 +69,7 @@ static struct clk *mpc5121_clk_get(struc
 		if (strcmp(id, p->name) == 0)
 			id_match++;
 		if ((dev_match || id_match) && try_module_get(p->owner)) {
-			clk = p;
+			clk = &p->clk;
 			break;
 		}
 	}
@@ -78,7 +81,7 @@ static struct clk *mpc5121_clk_get(struc
 #ifdef CLK_DEBUG
 static void dump_clocks(void)
 {
-	struct clk *p;
+	struct mpc512x_clk *p;
 
 	mutex_lock(&clocks_mutex);
 	printk(KERN_INFO "CLOCKS:\n");
@@ -101,7 +104,9 @@ static void dump_clocks(void)
 
 static void mpc5121_clk_put(struct clk *clk)
 {
-	module_put(clk->owner);
+	struct mpc512x_clk *mpclk = to_mpc512x_clk(clk);
+
+	module_put(mpclk->owner);
 }
 
 #define NRPSC 12
@@ -123,31 +128,35 @@ struct mpc512x_clockctl __iomem *clockct
 
 static int mpc5121_clk_enable(struct clk *clk)
 {
+	struct mpc512x_clk *mpclk = to_mpc512x_clk(clk);
 	unsigned int mask;
 
-	if (clk->flags & CLK_HAS_CTRL) {
-		mask = in_be32(&clockctl->sccr[clk->reg]);
-		mask |= 1 << clk->bit;
-		out_be32(&clockctl->sccr[clk->reg], mask);
+	if (mpclk->flags & CLK_HAS_CTRL) {
+		mask = in_be32(&clockctl->sccr[mpclk->reg]);
+		mask |= 1 << mpclk->bit;
+		out_be32(&clockctl->sccr[mpclk->reg], mask);
 	}
 	return 0;
 }
 
 static void mpc5121_clk_disable(struct clk *clk)
 {
+	struct mpc512x_clk *mpclk = to_mpc512x_clk(clk);
 	unsigned int mask;
 
-	if (clk->flags & CLK_HAS_CTRL) {
-		mask = in_be32(&clockctl->sccr[clk->reg]);
-		mask &= ~(1 << clk->bit);
-		out_be32(&clockctl->sccr[clk->reg], mask);
+	if (mpclk->flags & CLK_HAS_CTRL) {
+		mask = in_be32(&clockctl->sccr[mpclk->reg]);
+		mask &= ~(1 << mpclk->bit);
+		out_be32(&clockctl->sccr[mpclk->reg], mask);
 	}
 }
 
 static unsigned long mpc5121_clk_get_rate(struct clk *clk)
 {
-	if (clk->flags & CLK_HAS_RATE)
-		return clk->rate;
+	struct mpc512x_clk *mpclk = to_mpc512x_clk(clk);
+
+	if (mpclk->flags & CLK_HAS_RATE)
+		return mpclk->rate;
 	else
 		return 0;
 }
@@ -162,11 +171,21 @@ static int mpc5121_clk_set_rate(struct c
 	return 0;
 }
 
-static int clk_register(struct clk *clk)
+static int clk_register(struct mpc512x_clk *mpclk)
 {
+	mpclk.clk.enable	= mpc5121_clk_enable;
+	mpclk.clk.disable	= mpc5121_clk_disable;
+	mpclk.clk.get_rate	= mpc5121_clk_get_rate;
+	mpclk.clk.put		= mpc5121_clk_put;
+	mpclk.clk.round_rate	= mpc5121_clk_round_rate;
+	mpclk.clk.set_rate	= mpc5121_clk_set_rate;
+	mpclk.clk.set_paqrent	= NULL;
+	mpclk.clk.get_parent	= NULL;
+
 	mutex_lock(&clocks_mutex);
-	list_add(&clk->node, &clocks);
+	list_add(&mpclk->node, &clocks);
 	mutex_unlock(&clocks_mutex);
+
 	return 0;
 }
 
@@ -250,7 +269,7 @@ static unsigned long devtree_getfreq(cha
 	return val;
 }
 
-static void ref_clk_calc(struct clk *clk)
+static void ref_clk_calc(struct mpc512x_clk *mpclk)
 {
 	unsigned long rate;
 
@@ -260,26 +279,26 @@ static void ref_clk_calc(struct clk *clk
 		clk->rate = 0;
 		return;
 	}
-	clk->rate = ips_to_ref(rate);
+	mpclk->rate = ips_to_ref(rate);
 }
 
-static struct clk ref_clk = {
+static struct mpc512x_clk ref_clk = {
 	.name = "ref_clk",
 	.calc = ref_clk_calc,
 };
 
 
-static void sys_clk_calc(struct clk *clk)
+static void sys_clk_calc(struct mpc512x_clk *clk)
 {
 	clk->rate = ref_to_sys(ref_clk.rate);
 }
 
-static struct clk sys_clk = {
+static struct mpc512x_clk sys_clk = {
 	.name = "sys_clk",
 	.calc = sys_clk_calc,
 };
 
-static void diu_clk_calc(struct clk *clk)
+static void diu_clk_calc(struct mpc512x_clk *clk)
 {
 	int diudiv_x_2 = clockctl->scfr1 & 0xff;
 	unsigned long rate;
@@ -292,30 +311,30 @@ static void diu_clk_calc(struct clk *clk
 	clk->rate = rate;
 }
 
-static void half_clk_calc(struct clk *clk)
+static void half_clk_calc(struct mpc512x_clk *clk)
 {
 	clk->rate = clk->parent->rate / 2;
 }
 
-static void generic_div_clk_calc(struct clk *clk)
+static void generic_div_clk_calc(struct mpc512x_clk *clk)
 {
 	int div = (clockctl->scfr1 >> clk->div_shift) & 0x7;
 
 	clk->rate = clk->parent->rate / div;
 }
 
-static void unity_clk_calc(struct clk *clk)
+static void unity_clk_calc(struct mpc512x_clk *clk)
 {
 	clk->rate = clk->parent->rate;
 }
 
-static struct clk csb_clk = {
+static struct mpc512x_clk csb_clk = {
 	.name = "csb_clk",
 	.calc = half_clk_calc,
 	.parent = &sys_clk,
 };
 
-static void e300_clk_calc(struct clk *clk)
+static void e300_clk_calc(struct mpc512x_clk *clk)
 {
 	int spmf = (clockctl->spmr >> 16) & 0xf;
 	int ratex2 = clk->parent->rate * spmf;
@@ -323,13 +342,13 @@ static void e300_clk_calc(struct clk *cl
 	clk->rate = ratex2 / 2;
 }
 
-static struct clk e300_clk = {
+static struct mpc512x_clk e300_clk = {
 	.name = "e300_clk",
 	.calc = e300_clk_calc,
 	.parent = &csb_clk,
 };
 
-static struct clk ips_clk = {
+static struct mpc512x_clk ips_clk = {
 	.name = "ips_clk",
 	.calc = generic_div_clk_calc,
 	.parent = &csb_clk,
@@ -339,7 +358,7 @@ static struct clk ips_clk = {
 /*
  * Clocks controlled by SCCR1 (.reg = 0)
  */
-static struct clk lpc_clk = {
+static struct mpc512x_clk lpc_clk = {
 	.name = "lpc_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 0,
@@ -349,7 +368,7 @@ static struct clk lpc_clk = {
 	.div_shift = 11,
 };
 
-static struct clk nfc_clk = {
+static struct mpc512x_clk nfc_clk = {
 	.name = "nfc_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 0,
@@ -359,7 +378,7 @@ static struct clk nfc_clk = {
 	.div_shift = 8,
 };
 
-static struct clk pata_clk = {
+static struct mpc512x_clk pata_clk = {
 	.name = "pata_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 0,
@@ -373,7 +392,7 @@ static struct clk pata_clk = {
  * are setup elsewhere
  */
 
-static struct clk sata_clk = {
+static struct mpc512x_clk sata_clk = {
 	.name = "sata_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 0,
@@ -382,7 +401,7 @@ static struct clk sata_clk = {
 	.parent = &ips_clk,
 };
 
-static struct clk fec_clk = {
+static struct mpc512x_clk fec_clk = {
 	.name = "fec_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 0,
@@ -391,7 +410,7 @@ static struct clk fec_clk = {
 	.parent = &ips_clk,
 };
 
-static struct clk pci_clk = {
+static struct mpc512x_clk pci_clk = {
 	.name = "pci_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 0,
@@ -404,7 +423,7 @@ static struct clk pci_clk = {
 /*
  * Clocks controlled by SCCR2 (.reg = 1)
  */
-static struct clk diu_clk = {
+static struct mpc512x_clk diu_clk = {
 	.name = "diu_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 1,
@@ -412,7 +431,7 @@ static struct clk diu_clk = {
 	.calc = diu_clk_calc,
 };
 
-static struct clk axe_clk = {
+static struct mpc512x_clk axe_clk = {
 	.name = "axe_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 1,
@@ -421,7 +440,7 @@ static struct clk axe_clk = {
 	.parent = &csb_clk,
 };
 
-static struct clk usb1_clk = {
+static struct mpc512x_clk usb1_clk = {
 	.name = "usb1_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 1,
@@ -430,7 +449,7 @@ static struct clk usb1_clk = {
 	.parent = &csb_clk,
 };
 
-static struct clk usb2_clk = {
+static struct mpc512x_clk usb2_clk = {
 	.name = "usb2_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 1,
@@ -439,7 +458,7 @@ static struct clk usb2_clk = {
 	.parent = &csb_clk,
 };
 
-static struct clk i2c_clk = {
+static struct mpc512x_clk i2c_clk = {
 	.name = "i2c_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 1,
@@ -448,7 +467,7 @@ static struct clk i2c_clk = {
 	.parent = &ips_clk,
 };
 
-static struct clk mscan_clk = {
+static struct mpc512x_clk mscan_clk = {
 	.name = "mscan_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 1,
@@ -457,7 +476,7 @@ static struct clk mscan_clk = {
 	.parent = &ips_clk,
 };
 
-static struct clk sdhc_clk = {
+static struct mpc512x_clk sdhc_clk = {
 	.name = "sdhc_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 1,
@@ -466,7 +485,7 @@ static struct clk sdhc_clk = {
 	.parent = &ips_clk,
 };
 
-static struct clk mbx_bus_clk = {
+static struct mpc512x_clk mbx_bus_clk = {
 	.name = "mbx_bus_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 1,
@@ -475,7 +494,7 @@ static struct clk mbx_bus_clk = {
 	.parent = &csb_clk,
 };
 
-static struct clk mbx_clk = {
+static struct mpc512x_clk mbx_clk = {
 	.name = "mbx_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 1,
@@ -484,7 +503,7 @@ static struct clk mbx_clk = {
 	.parent = &csb_clk,
 };
 
-static struct clk mbx_3d_clk = {
+static struct mpc512x_clk mbx_3d_clk = {
 	.name = "mbx_3d_clk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 1,
@@ -494,44 +513,44 @@ static struct clk mbx_3d_clk = {
 	.div_shift = 14,
 };
 
-static void psc_mclk_in_calc(struct clk *clk)
+static void psc_mclk_in_calc(struct mpc512x_clk *clk)
 {
 	clk->rate = devtree_getfreq("psc_mclk_in");
 	if (!clk->rate)
 		clk->rate = 25000000;
 }
 
-static struct clk psc_mclk_in = {
+static struct mpc512x_clk psc_mclk_in = {
 	.name = "psc_mclk_in",
 	.calc = psc_mclk_in_calc,
 };
 
-static struct clk spdif_txclk = {
+static struct mpc512x_clk spdif_txclk = {
 	.name = "spdif_txclk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 1,
 	.bit = 23,
 };
 
-static struct clk spdif_rxclk = {
+static struct mpc512x_clk spdif_rxclk = {
 	.name = "spdif_rxclk",
 	.flags = CLK_HAS_CTRL,
 	.reg = 1,
 	.bit = 23,
 };
 
-static void ac97_clk_calc(struct clk *clk)
+static void ac97_clk_calc(struct mpc512x_clk *clk)
 {
 	/* ac97 bit clock is always 24.567 MHz */
 	clk->rate = 24567000;
 }
 
-static struct clk ac97_clk = {
+static struct mpc512x_clk ac97_clk = {
 	.name = "ac97_clk_in",
 	.calc = ac97_clk_calc,
 };
 
-struct clk *rate_clks[] = {
+struct mpc512x_clk *rate_clks[] = {
 	&ref_clk,
 	&sys_clk,
 	&diu_clk,
@@ -560,7 +579,7 @@ struct clk *rate_clks[] = {
 	NULL
 };
 
-static void rate_clk_init(struct clk *clk)
+static void rate_clk_init(struct mpc512x_clk *clk)
 {
 	if (clk->calc) {
 		clk->calc(clk);
@@ -575,7 +594,7 @@ static void rate_clk_init(struct clk *cl
 
 static void rate_clks_init(void)
 {
-	struct clk **cpp, *clk;
+	struct mpc512x_clk **cpp, *clk;
 
 	cpp = rate_clks;
 	while ((clk = *cpp++))
@@ -586,16 +605,16 @@ static void rate_clks_init(void)
  * There are two clk enable registers with 32 enable bits each
  * psc clocks and device clocks are all stored in dev_clks
  */
-struct clk dev_clks[2][32];
+struct mpc512x_clk dev_clks[2][32];
 
 /*
  * Given a psc number return the dev_clk
  * associated with it
  */
-static struct clk *psc_dev_clk(int pscnum)
+static struct mpc512x_clk *psc_dev_clk(int pscnum)
 {
 	int reg, bit;
-	struct clk *clk;
+	struct mpc512x_clk *clk;
 
 	reg = 0;
 	bit = 27 - pscnum;
@@ -609,7 +628,7 @@ static struct clk *psc_dev_clk(int pscnu
 /*
  * PSC clock rate calculation
  */
-static void psc_calc_rate(struct clk *clk, int pscnum, struct device_node *np)
+static void psc_calc_rate(struct mpc512x_clk *clk, int pscnum, struct device_node *np)
 {
 	unsigned long mclk_src = sys_clk.rate;
 	unsigned long mclk_div;
@@ -666,7 +685,7 @@ static void psc_clks_init(void)
 		cell_index = of_get_property(np, "cell-index", NULL);
 		if (cell_index) {
 			int pscnum = *cell_index;
-			struct clk *clk = psc_dev_clk(pscnum);
+			struct mpc512x_clk *clk = psc_dev_clk(pscnum);
 
 			clk->flags = CLK_HAS_RATE | CLK_HAS_CTRL;
 			ofdev = of_find_device_by_node(np);
@@ -686,18 +705,6 @@ static void psc_clks_init(void)
 	}
 }
 
-static struct clk_interface mpc5121_clk_functions = {
-	.clk_get		= mpc5121_clk_get,
-	.clk_enable		= mpc5121_clk_enable,
-	.clk_disable		= mpc5121_clk_disable,
-	.clk_get_rate		= mpc5121_clk_get_rate,
-	.clk_put		= mpc5121_clk_put,
-	.clk_round_rate		= mpc5121_clk_round_rate,
-	.clk_set_rate		= mpc5121_clk_set_rate,
-	.clk_set_parent		= NULL,
-	.clk_get_parent		= NULL,
-};
-
 static int
 mpc5121_clk_init(void)
 {
@@ -721,7 +728,7 @@ mpc5121_clk_init(void)
 	/*iounmap(clockctl); */
 	DEBUG_CLK_DUMP();
 	clocks_initialized++;
-	clk_functions = mpc5121_clk_functions;
+	ppc_md.clk_get = mpc5121_clk_get;
 	return 0;
 }
 

Re: [RFC] Clock binding

[Benjamin Herrenschmidt]

On Tue, 2009-08-18 at 14:21 +1000, Benjamin Herrenschmidt wrote:
> So here's a followup to my discussion about the clock API.

Really nobody has a comment here ? :-) Not even Mitch ?

Cheers,
Ben.

> I'm cooking up a patch that replace our current primitive implementation
> in arch/powerpc/kernel/clock.c with something along the lines of what I
> described. However, I want a bit more churn here on the device-tree
> related bits.
> 
> So, basically, the goal here is to define a binding so that we can link
> a device clock inputs to a clock provider clock outputs.
> 
> In general, in a system, there's actually 3 "names" involved. The clock
> provider output name, the clock signal name, and the clock input name on
> the device. However, I want to avoid involving the clock signal name as
> it's a "global" name and it will just end up being a mess if we start
> exposing that.
> 
> So basically, it boils down to a device having some clock inputs,
> referenced by names, that need to be linked to another node which is a
> clock provider, which has outputs, references either by number or names,
> see discussion below.
> 
> First, why names, and not numbers ? IE. It's the OF "tradition" for
> resources to just be an array, like interrupts, or address ranges in
> "reg" properties, and one has to know what the Nth interrupt correspond
> too.
> 
> My answer here is that maybe the tradition but it's crap :-) Names are
> much better in the long run, besides it makes it easier to represent if
> not all inputs have been wired. Also, to some extent, things like PCI do
> encode a "name" with "reg" or "assigned-addresses" properties as part of
> the config space offset in the top part of the address, and that has
> proved very useful.
> 
> Thus I think using names is the way to go, and we should even generalize
> that and add a new "interrupt-names" property to name the members of an
> "interrupts" :-)
> 
> So back to the subject at hand. That leaves us with having to populate
> the driver with some kind of map (I call it clock-map). Ideally, if
> everything is named, which is the best approach imho, that map would
> bind a list of:
> 
> 	- clock input name
> 	- clock provider phandle
> 	- clock output name on provider
> 
> However, it's a bit nasty to mix strings and numbers (phandles) in a
> single property. It's possible, but would likely lead to the phandle not
> being aligned and tools such as lsprop to fail miserably to display
> those properties in any kind of readable form.
> 
> My earlier emails proposed an approach like this:
> 
> 	- clock input names go into a "clock-names" property
> 	  (which I suggest naming instead "clock-input-names" btw)
> 
> 	- the map goes into a "clock-map" property and for each input
> 	  provides a phandle and a one cell numerical ID that identifies
> 	  the clock on the source.
> 
> However, I really dislike that numerical clock ID. Magic numbers suck.
> It should be a string. But I don't want to add a 3rd property in there.
> 
> Hence my idea below. It's not perfect but it's the less sucky i've come
> up with so far. And then we can do some small refinements.
> 
> 	* Device has:
> 
> 		- "clock-input-names" as above
> 		- "clock-map" contains list of phandle,index
> 
> 	* Clock source has:
> 
> 		- "clock-output-names" list of strings
> 
> The "index" in the clock map thus would reference the
> "clock-output-names" array in the clock provider. That means that the
> "magic number" here is entirely local to a given device-tree, doesn't
> leak into driver code, which continues using names.
> 
> In addition, we can even have some smooth "upgrade" path from existing
> "clock-frequency" properties by assuming that if "clock-output-names" is
> absent, but "clock-frequency" exist, then index 0 references a fixed
> frequency clock source without a driver. This could be generally handy
> anyway to represent crystals of fixed bus clocks without having to write
> a clock source driver for them.
> 
> Any comments ?
> 
> I'll post a patch, maybe later today, implementing the above (I may or
> may not have time to also convert the existing 512x code to it, we'll
> see).
> 
> Cheers,
> Ben.
> 
> 
>  
> 
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [RFC] Clock binding

[Grant Likely]

On Wed, Aug 26, 2009 at 10:09 PM, Benjamin
Herrenschmidt<benh@kernel.crashing.org> wrote:
> On Tue, 2009-08-18 at 14:21 +1000, Benjamin Herrenschmidt wrote:
>> So here's a followup to my discussion about the clock API.
>
> Really nobody has a comment here ? :-) Not even Mitch ?

Been wanting too.. a little swamped with kids going back to school
tomorrow.  Hopefully I'll be able to dig in before the end of the
week.

g.

Re: [RFC] Clock binding

[Mitch Bradley]

>
> On Tue, 2009-08-18 at 14:21 +1000, Benjamin Herrenschmidt wrote:
>   
>> > So here's a followup to my discussion about the clock API.
>>     
>
> Really nobody has a comment here ?  :-)  Not even Mitch ?
>   

I refrained from commenting as I didn't want to get involved in an 
endless argument about "goodness".

Indexed arrays are appropriate for some cases and names are better for 
others.  Names are especially good for large spaces that are sparse or 
weakly-structured.  The same is true for subroutine arguments.  It's 
nice to be able to write setup_uart(baud=115200, flow_control=rts_cts), 
but you would go crazy (or become a COBOL programmer) if you had to name 
every argument to every subroutine.

Open Firmware often avoids indexed structures.  Cases in point include 
the use of named properties instead of fixed structures and named 
methods instead of function pointer arrays.  Open Firmware's use of 
arrays for reg properties seems like the right choice for that 
particular case, but shouldn't be construed to suggest that arrays are 
good for everything.

One problem you run into with names is the "registration authority" 
problem.  Who maintains the list of valid names to avoid collisions and 
to ensure consistent spelling?  It's a solvable problem, but one that 
must be considered.  Of course, a related problem exists with indices - 
what is the meaning of index value "N", and how do you manage the 
addition of new fields and deletion of others?  Names are easier to 
manage in some cases and indices easier in others.

In the particular case of a clock binding, I don't have enough 
experience with the problem details to have formed a strong opinion.

Are there well-known clock names that will be used in common code that 
is shared among different vendors (e.g. "primary-clock")?  If so, the 
binding should "preregister" a list of common names.

Will implementors of new hardware have to scratch their heads to decide 
what to name their clock inputs?  The binding should offer some guidance 
about good name choices and spelling rules, to avoid an eventuall mess 
as new people come on board and pull names out of the air, with 
different conventions for capitalization and punctuation and abbreviation.

One advantage of indices is that they avoid endless arguments about the 
exact name (and spelling) of things.  In my current project, there are 
several different hardware manuals for the same that must all be 
consulted to get the full picture, and they often use different names or 
inconsistent spellings for the same thing.  It makes finding things very 
challenging.

With a name-based interface, it pays to keep in mind that lots of people 
will ultimately be involved. Many of them will be new so they won't know 
the conventions well, and few will be careful and precise in their use 
of language (i.e. names).  Provide a lot of guidance about how to choose 
a set of names.

Suppose there were a conventional set of names like "clock0", "clock1", 
..., essentially boiling down to verbosely-spelled indices.  I expect 
that a lot of people would choose to use it just to avoid having to 
thing of better names and having "bike shed" arguments with coworkers.

So there you have it - my incoherent rambling commentary with no 
particular conclusion.

> Cheers,
> Ben.
>
>   
>> > I'm cooking up a patch that replace our current primitive implementation
>> > in arch/powerpc/kernel/clock.c with something along the lines of what I
>> > described. However, I want a bit more churn here on the device-tree
>> > related bits.
>> > 
>> > So, basically, the goal here is to define a binding so that we can link
>> > a device clock inputs to a clock provider clock outputs.
>> > 
>> > In general, in a system, there's actually 3 "names" involved. The clock
>> > provider output name, the clock signal name, and the clock input name on
>> > the device. However, I want to avoid involving the clock signal name as
>> > it's a "global" name and it will just end up being a mess if we start
>> > exposing that.
>> > 
>> > So basically, it boils down to a device having some clock inputs,
>> > referenced by names, that need to be linked to another node which is a
>> > clock provider, which has outputs, references either by number or names,
>> > see discussion below.
>> > 
>> > First, why names, and not numbers ? IE. It's the OF "tradition" for
>> > resources to just be an array, like interrupts, or address ranges in
>> > "reg" properties, and one has to know what the Nth interrupt correspond
>> > too.
>> > 
>> > My answer here is that maybe the tradition but it's crap  :-)  Names are
>> > much better in the long run, besides it makes it easier to represent if
>> > not all inputs have been wired. Also, to some extent, things like PCI do
>> > encode a "name" with "reg" or "assigned-addresses" properties as part of
>> > the config space offset in the top part of the address, and that has
>> > proved very useful.
>> > 
>> > Thus I think using names is the way to go, and we should even generalize
>> > that and add a new "interrupt-names" property to name the members of an
>> > "interrupts"  :-) 
>> > 
>> > So back to the subject at hand. That leaves us with having to populate
>> > the driver with some kind of map (I call it clock-map). Ideally, if
>> > everything is named, which is the best approach imho, that map would
>> > bind a list of:
>> > 
>> > 	- clock input name
>> > 	- clock provider phandle
>> > 	- clock output name on provider
>> > 
>> > However, it's a bit nasty to mix strings and numbers (phandles) in a
>> > single property. It's possible, but would likely lead to the phandle not
>> > being aligned and tools such as lsprop to fail miserably to display
>> > those properties in any kind of readable form.
>> > 
>> > My earlier emails proposed an approach like this:
>> > 
>> > 	- clock input names go into a "clock-names" property
>> > 	  (which I suggest naming instead "clock-input-names" btw)
>> > 
>> > 	- the map goes into a "clock-map" property and for each input
>> > 	  provides a phandle and a one cell numerical ID that identifies
>> > 	  the clock on the source.
>> > 
>> > However, I really dislike that numerical clock ID. Magic numbers suck.
>> > It should be a string. But I don't want to add a 3rd property in there.
>> > 
>> > Hence my idea below. It's not perfect but it's the less sucky i've come
>> > up with so far. And then we can do some small refinements.
>> > 
>> > 	* Device has:
>> > 
>> > 		- "clock-input-names" as above
>> > 		- "clock-map" contains list of phandle,index
>> > 
>> > 	* Clock source has:
>> > 
>> > 		- "clock-output-names" list of strings
>> > 
>> > The "index" in the clock map thus would reference the
>> > "clock-output-names" array in the clock provider. That means that the
>> > "magic number" here is entirely local to a given device-tree, doesn't
>> > leak into driver code, which continues using names.
>> > 
>> > In addition, we can even have some smooth "upgrade" path from existing
>> > "clock-frequency" properties by assuming that if "clock-output-names" is
>> > absent, but "clock-frequency" exist, then index 0 references a fixed
>> > frequency clock source without a driver. This could be generally handy
>> > anyway to represent crystals of fixed bus clocks without having to write
>> > a clock source driver for them.
>> > 
>> > Any comments ?
>> > 
>> > I'll post a patch, maybe later today, implementing the above (I may or
>> > may not have time to also convert the existing 512x code to it, we'll
>> > see).
>> > 
>> > Cheers,
>> > Ben.
>> > 
>> > 
>> >  
>> > 
>> > 
>> > _______________________________________________
>> > devicetree-discuss mailing list
>> > devicetree-discuss@lists.ozlabs.org
>> > https://lists.ozlabs.org/listinfo/devicetree-discuss
>>     
>
>   

Re: [RFC] Clock binding

[Benjamin Herrenschmidt]

On Thu, 2009-08-27 at 11:11 -1000, Mitch Bradley wrote:

> I refrained from commenting as I didn't want to get involved in an 
> endless argument about "goodness".

Oh well, I asked for it, didn't it ? :-)

> Indexed arrays are appropriate for some cases and names are better for 
> others.  Names are especially good for large spaces that are sparse or 
> weakly-structured.  The same is true for subroutine arguments.  It's 
> nice to be able to write setup_uart(baud=115200, flow_control=rts_cts), 
> but you would go crazy (or become a COBOL programmer) if you had to name 
> every argument to every subroutine.

Agreed.

> Open Firmware often avoids indexed structures.  Cases in point include 
> the use of named properties instead of fixed structures and named 
> methods instead of function pointer arrays.  Open Firmware's use of 
> arrays for reg properties seems like the right choice for that 
> particular case, but shouldn't be construed to suggest that arrays are 
> good for everything.

Well, the "reg" property is fine for the common cases of devices with
one IO (or MMIO) range, no confusion possible, or PCI since it encodes
the BAR number. For other cases, especially random embedded stuff that
maps several regions of memory, it's a bit harder since we go back to
the need of having "somebody" define which region is which.

> One problem you run into with names is the "registration authority" 
> problem.

Right.

>   Who maintains the list of valid names to avoid collisions and 
> to ensure consistent spelling?  It's a solvable problem, but one that 
> must be considered.  Of course, a related problem exists with indices - 
> what is the meaning of index value "N", and how do you manage the 
> addition of new fields and deletion of others?  Names are easier to 
> manage in some cases and indices easier in others.

Maybe to some extent. I tend to prefer names whenever possible, but I
agree that the "central authority" problem is always around.

> In the particular case of a clock binding, I don't have enough 
> experience with the problem details to have formed a strong opinion.

I'm in a similar situation, hence my request for comment. From my
observations though, clock inputs are generally named on a given chip,
IO core, SoC, etc... hence, I would suggest using a case insensitive
matching with a recommendation to use the name of the clock pin or clock
input signal in the case of IP cores as per the spec of the chip. This
sounds better than an arbitrary number. So at least for the naming of
clock inputs on a device, I do have a warm feeling with using names.

It has the added advantage from my selfish point of view which is that I
have to deal in Linux with an existing name based API and existing
drivers using it :-) Though the common case for 1 clock per device is to
pass NULL, which makes the problem go away.

In the same idea of simplifying the easy cases, you'll note that I made
it legit to have a clock provider simply have a "clock-frequency"
property and as such "fit" within existing practices for fixed frequency
non-programable clock sources.

Now, where I'm less comfortable is the naming on the clock provider
side. IE. My initial proposal was calling for binding named inputs to
indexed outputs on the provider. My new proposal makes that index a
simple convention in a given tree by having the provider also map those
to names since I didn't like the discrepancy here.  

I suppose we could go roughly with the same idea, which is that the name
is case insensitive and matched on the part manufacturer's output
pin/signal name.

In any case, I'm sure what we do won't be absolutely perfect but it's
that or doing nothing and at this stage I believe we are in a situation
where even an imperfect solution will be greatly beneficial. In any
case, driver and architecture code can always work around problems such
as a device-tree providing a different name than what the driver
expects.

> Are there well-known clock names that will be used in common code that 
> is shared among different vendors (e.g. "primary-clock")?  If so, the 
> binding should "preregister" a list of common names.

Well... I was more thinking about the signal name on the part. I want to
avoid using global clock names. IE. I want to avoid having to put in the
picture the name of the clock wires in the SoC or chip (SYSCLK,
PCICLK, ...) because typically those do vary from board to board and
vendor to vendor. But the name of the clock input on a device chip and
the name of the clock output on a PLL chip tend to be defined by the
datasheet of the said chip :-) Of course there are going to be
interesting variations such as compatible chips from different
manufacturers but, mostly we should cope better than magic indices.

> Will implementors of new hardware have to scratch their heads to decide 
> what to name their clock inputs?  The binding should offer some guidance 
> about good name choices and spelling rules, to avoid an eventuall mess 
> as new people come on board and pull names out of the air, with 
> different conventions for capitalization and punctuation and abbreviation.

Should they ? IE. If the names come from the part datasheet, and we make
the matching case insensitive, I don't see that much need of the chip
vendors or board designers caring that much about rules for naming the
clocks.

> One advantage of indices is that they avoid endless arguments about the 
> exact name (and spelling) of things.

Right, though in that case, nobody gets to have to decide on the name,
it comes from the chip manufacturer pin naming or data sheet.

>   In my current project, there are 
> several different hardware manuals for the same that must all be 
> consulted to get the full picture, and they often use different names or 
> inconsistent spellings for the same thing.  It makes finding things very 
> challenging.

Agreed about the general problem area.

> With a name-based interface, it pays to keep in mind that lots of people 
> will ultimately be involved. Many of them will be new so they won't know 
> the conventions well, and few will be careful and precise in their use 
> of language (i.e. names).  Provide a lot of guidance about how to choose 
> a set of names.

And the easier the problem appears to be, the more people without a clue
about what they are talking about will feel compelled to intervene in
the discussion :-)

> Suppose there were a conventional set of names like "clock0", "clock1", 
> ..., essentially boiling down to verbosely-spelled indices.  I expect 
> that a lot of people would choose to use it just to avoid having to 
> thing of better names and having "bike shed" arguments with coworkers.
> 
> So there you have it - my incoherent rambling commentary with no 
> particular conclusion.

Nah, it's interesting, it also forces me to write down my thought with
better details.

Cheers,
Ben.

> > Cheers,
> > Ben.
> >
> >   
> >> > I'm cooking up a patch that replace our current primitive implementation
> >> > in arch/powerpc/kernel/clock.c with something along the lines of what I
> >> > described. However, I want a bit more churn here on the device-tree
> >> > related bits.
> >> > 
> >> > So, basically, the goal here is to define a binding so that we can link
> >> > a device clock inputs to a clock provider clock outputs.
> >> > 
> >> > In general, in a system, there's actually 3 "names" involved. The clock
> >> > provider output name, the clock signal name, and the clock input name on
> >> > the device. However, I want to avoid involving the clock signal name as
> >> > it's a "global" name and it will just end up being a mess if we start
> >> > exposing that.
> >> > 
> >> > So basically, it boils down to a device having some clock inputs,
> >> > referenced by names, that need to be linked to another node which is a
> >> > clock provider, which has outputs, references either by number or names,
> >> > see discussion below.
> >> > 
> >> > First, why names, and not numbers ? IE. It's the OF "tradition" for
> >> > resources to just be an array, like interrupts, or address ranges in
> >> > "reg" properties, and one has to know what the Nth interrupt correspond
> >> > too.
> >> > 
> >> > My answer here is that maybe the tradition but it's crap  :-)  Names are
> >> > much better in the long run, besides it makes it easier to represent if
> >> > not all inputs have been wired. Also, to some extent, things like PCI do
> >> > encode a "name" with "reg" or "assigned-addresses" properties as part of
> >> > the config space offset in the top part of the address, and that has
> >> > proved very useful.
> >> > 
> >> > Thus I think using names is the way to go, and we should even generalize
> >> > that and add a new "interrupt-names" property to name the members of an
> >> > "interrupts"  :-) 
> >> > 
> >> > So back to the subject at hand. That leaves us with having to populate
> >> > the driver with some kind of map (I call it clock-map). Ideally, if
> >> > everything is named, which is the best approach imho, that map would
> >> > bind a list of:
> >> > 
> >> > 	- clock input name
> >> > 	- clock provider phandle
> >> > 	- clock output name on provider
> >> > 
> >> > However, it's a bit nasty to mix strings and numbers (phandles) in a
> >> > single property. It's possible, but would likely lead to the phandle not
> >> > being aligned and tools such as lsprop to fail miserably to display
> >> > those properties in any kind of readable form.
> >> > 
> >> > My earlier emails proposed an approach like this:
> >> > 
> >> > 	- clock input names go into a "clock-names" property
> >> > 	  (which I suggest naming instead "clock-input-names" btw)
> >> > 
> >> > 	- the map goes into a "clock-map" property and for each input
> >> > 	  provides a phandle and a one cell numerical ID that identifies
> >> > 	  the clock on the source.
> >> > 
> >> > However, I really dislike that numerical clock ID. Magic numbers suck.
> >> > It should be a string. But I don't want to add a 3rd property in there.
> >> > 
> >> > Hence my idea below. It's not perfect but it's the less sucky i've come
> >> > up with so far. And then we can do some small refinements.
> >> > 
> >> > 	* Device has:
> >> > 
> >> > 		- "clock-input-names" as above
> >> > 		- "clock-map" contains list of phandle,index
> >> > 
> >> > 	* Clock source has:
> >> > 
> >> > 		- "clock-output-names" list of strings
> >> > 
> >> > The "index" in the clock map thus would reference the
> >> > "clock-output-names" array in the clock provider. That means that the
> >> > "magic number" here is entirely local to a given device-tree, doesn't
> >> > leak into driver code, which continues using names.
> >> > 
> >> > In addition, we can even have some smooth "upgrade" path from existing
> >> > "clock-frequency" properties by assuming that if "clock-output-names" is
> >> > absent, but "clock-frequency" exist, then index 0 references a fixed
> >> > frequency clock source without a driver. This could be generally handy
> >> > anyway to represent crystals of fixed bus clocks without having to write
> >> > a clock source driver for them.
> >> > 
> >> > Any comments ?
> >> > 
> >> > I'll post a patch, maybe later today, implementing the above (I may or
> >> > may not have time to also convert the existing 512x code to it, we'll
> >> > see).
> >> > 
> >> > Cheers,
> >> > Ben.
> >> > 
> >> > 
> >> >  
> >> > 
> >> > 
> >> > _______________________________________________
> >> > devicetree-discuss mailing list
> >> > devicetree-discuss@lists.ozlabs.org
> >> > https://lists.ozlabs.org/listinfo/devicetree-discuss
> >>     
> >
> >   

Re: [RFC] Clock binding

[Mitch Bradley]

>
>
>> > Open Firmware often avoids indexed structures.  Cases in point include 
>> > the use of named properties instead of fixed structures and named 
>> > methods instead of function pointer arrays.  Open Firmware's use of 
>> > arrays for reg properties seems like the right choice for that 
>> > particular case, but shouldn't be construed to suggest that arrays are 
>> > good for everything.
>>     
>
> Well, the "reg" property is fine for the common cases of devices with
> one IO (or MMIO) range, no confusion possible, or PCI since it encodes
> the BAR number. For other cases, especially random embedded stuff that
> maps several regions of memory, it's a bit harder since we go back to
> the need of having "somebody" define which region is which.
>
>
>   

Indeed.  You choose based on the common case and eventually there will 
be some case that stretches the boundaries.

I suppose that, if the problem became severe enough, one could invent a 
new "reg-names" property, a list of strings naming the reg entries.

Re: [RFC] Clock binding

[Mitch Bradley]

>
>> > One advantage of indices is that they avoid endless arguments about the 
>> > exact name (and spelling) of things.
>>     
>
> Right, though in that case, nobody gets to have to decide on the name,
> it comes from the chip manufacturer pin naming or data sheet.
>
>   


I agree in general.  It has long been a convention of mine to follow the 
vendor's names as exactly as possible.  But that often presents 
difficulties. Many of them have been touched on in our previous 
discussion but I'll list some here just to emphasize the problem we face:

a) Inconsistent naming within a vendor's documentation set - datasheet 
spells it one way, programmer's manual another, appnotes/porting guide 
still another, reference schematic spells it two different ways (pin 
name on part versus net name of signal wire).

b) Sometimes the name is abbreviated and sometimes spelled out.  SMBALRT 
vs. SMbus Alert.

c) Different tools (CAD programs, word processors) have different 
conventions leading to existence of ambiguously-representable name 
components - particular cases in point are overbars for active low 
signals and embedded spaces/underscores/hyphens in names.

d) Compatible part from different vendors leads to confusion about which 
vendor's names are canonical.  Or leading vendor goes out of business or 
gets bought.

These problems are getting worse rapidly as more devices are being 
sourced from Asia, where the linguistic connection to the Roman alphabet 
is tenuous.

You need a Pope to decide what is canonical.

Re: [RFC] Clock binding

[Benjamin Herrenschmidt]

> I agree in general.  It has long been a convention of mine to follow the 
> vendor's names as exactly as possible.  But that often presents 
> difficulties. Many of them have been touched on in our previous 
> discussion but I'll list some here just to emphasize the problem we face:
> 
> a) Inconsistent naming within a vendor's documentation set - datasheet 
> spells it one way, programmer's manual another, appnotes/porting guide 
> still another, reference schematic spells it two different ways (pin 
> name on part versus net name of signal wire).

Right. I should emphasis that what the device-tree should contain is the
pin name on part and -not- the net name of the wire. Of course, bogus
datasheet and inconsistent spelling will still hurt, nothing much we can
do about it, other than having the first to write a driver become a
de-facto "guide" to other implementors :-)

Maybe we could have a Wiki where the first time a given chip is used for
a binding, we put down the names used to encourage people to use the
same. To some extent it's going to be the responsibility of the person
writing the .dts to pick up names that will work with existing drivers. 

In fact that problem is no different than picking up "compatible" names,
and that Wiki thing might help in both cases.

> b) Sometimes the name is abbreviated and sometimes spelled out.  SMBALRT 
> vs. SMbus Alert.

Right. Still better than a magic number where you simply have no
reference whatsoever to what it is.

> c) Different tools (CAD programs, word processors) have different 
> conventions leading to existence of ambiguously-representable name 
> components - particular cases in point are overbars for active low 
> signals and embedded spaces/underscores/hyphens in names.

Yes, that can be an issue, though overbar is less common for clocks.

> d) Compatible part from different vendors leads to confusion about which 
> vendor's names are canonical.  Or leading vendor goes out of business or 
> gets bought.

Sure, same problem with "compatible" properties. There's no silver
bullet here. But we could try the Wiki approach to provide an unofficial
"reference" of canonical names for common devices. For -very- common
ones such as 8250-compatible UARTs, a full blown binding is probably the
way to go.

> These problems are getting worse rapidly as more devices are being 
> sourced from Asia, where the linguistic connection to the Roman alphabet 
> is tenuous.

True.

> You need a Pope to decide what is canonical.

Perhaps. But we can try our best with something like a wiki in the
meantime and see how it works out.

Cheers,
Ben.

> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [RFC] Clock binding

[Mitch Bradley]

The idea of a wiki as a registration authority is a good one, but I'm 
not volunteering to maintain it :-)

Re: [RFC] Clock binding

[Benjamin Herrenschmidt]

On Thu, 2009-08-27 at 16:36 -1000, Mitch Bradley wrote:
> The idea of a wiki as a registration authority is a good one, but I'm 
> not volunteering to maintain it :-)

here goes my hope :-)

Do we have wiki's we could use on power.org or should we aim for a
community place ? Anybody has suggestions ? I wouldn't even try to
maintain a web site myself, that would be irresponsible of anybody to
let me do so !

Cheers,
Ben.

Re: [RFC] Clock binding

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

On Fri, 2009-08-28 at 12:43 +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2009-08-27 at 16:36 -1000, Mitch Bradley wrote:
> > The idea of a wiki as a registration authority is a good one, but I'm 
> > not volunteering to maintain it :-)
> 
> here goes my hope :-)
> 
> Do we have wiki's we could use on power.org or should we aim for a
> community place ? Anybody has suggestions ? I wouldn't even try to
> maintain a web site myself, that would be irresponsible of anybody to
> let me do so !

There are a bunch on kernel.org, not sure what the story is with setting
one up / maintenance etc.

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [RFC] Clock binding

[Josh Boyer]

On Fri, Aug 28, 2009 at 12:43:08PM +1000, Benjamin Herrenschmidt wrote:
>On Thu, 2009-08-27 at 16:36 -1000, Mitch Bradley wrote:
>> The idea of a wiki as a registration authority is a good one, but I'm 
>> not volunteering to maintain it :-)
>
>here goes my hope :-)
>
>Do we have wiki's we could use on power.org or should we aim for a
>community place ? Anybody has suggestions ? I wouldn't even try to
>maintain a web site myself, that would be irresponsible of anybody to
>let me do so !

This is about the 3rd or 4th time this idea has come up over the past couple
of years.  Maybe this time it will stick?

Btw, you probably want some kind of contributor agreement so that whatever
is posted under the wiki is freely usable by others.

josh

Re: [RFC] Clock binding

[David Gibson]

On Fri, Aug 28, 2009 at 06:58:37AM -0400, Josh Boyer wrote:
> On Fri, Aug 28, 2009 at 12:43:08PM +1000, Benjamin Herrenschmidt wrote:
> >On Thu, 2009-08-27 at 16:36 -1000, Mitch Bradley wrote:
> >> The idea of a wiki as a registration authority is a good one, but I'm 
> >> not volunteering to maintain it :-)
> >
> >here goes my hope :-)
> >
> >Do we have wiki's we could use on power.org or should we aim for a
> >community place ? Anybody has suggestions ? I wouldn't even try to
> >maintain a web site myself, that would be irresponsible of anybody to
> >let me do so !
> 
> This is about the 3rd or 4th time this idea has come up over the past couple
> of years.  Maybe this time it will stick?

There actually was one set up on power.org, for epapr bindings.  I'm
still digging around to try to relocate the address, though.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [RFC] Clock binding

[Benjamin Herrenschmidt]

> > This is about the 3rd or 4th time this idea has come up over the past couple
> > of years.  Maybe this time it will stick?
> 
> There actually was one set up on power.org, for epapr bindings.  I'm
> still digging around to try to relocate the address, though.

We need to double check that indeed the content is freely licenced,
especially if, in the future, for some reasons, power.org goes down,
that we are legally allowed to take that content and put it elsewhere.

Cheers,
Ben.

Re: [RFC] Clock binding

[Stuart Yoder]

On Thu, Aug 27, 2009 at 9:43 PM, Benjamin
Herrenschmidt<benh@kernel.crashing.org> wrote:
> On Thu, 2009-08-27 at 16:36 -1000, Mitch Bradley wrote:
>> The idea of a wiki as a registration authority is a good one, but I'm
>> not volunteering to maintain it :-)
>
> here goes my hope :-)
>
> Do we have wiki's we could use on power.org or should we aim for a
> community place ? Anybody has suggestions ? I wouldn't even try to
> maintain a web site myself, that would be irresponsible of anybody to
> let me do so !

There was an ePAPR wiki on power.org, that I had started developing,
but it seems to have disappeared.  I'm looking into what happened.

Stuart

Re: [RFC] Clock binding

[Grant Likely]

On Fri, Aug 28, 2009 at 6:16 AM, Stuart Yoder<stuyoder@gmail.com> wrote:
> On Thu, Aug 27, 2009 at 9:43 PM, Benjamin
> Herrenschmidt<benh@kernel.crashing.org> wrote:
>> On Thu, 2009-08-27 at 16:36 -1000, Mitch Bradley wrote:
>>> The idea of a wiki as a registration authority is a good one, but I'm
>>> not volunteering to maintain it :-)
>>
>> here goes my hope :-)
>>
>> Do we have wiki's we could use on power.org or should we aim for a
>> community place ? Anybody has suggestions ? I wouldn't even try to
>> maintain a web site myself, that would be irresponsible of anybody to
>> let me do so !
>
> There was an ePAPR wiki on power.org, that I had started developing,
> but it seems to have disappeared. =A0I'm looking into what happened.

Lets *not* do it on power.org.  I'd like to see the bindings used by
more than just powerpc people, and power.org might become a bit of a
mental barrier for non-powerpc folks.  kernel.org would be a good
host.  So would ozlabs or infradead.  Or I'd be happy to maintain one
on secretlab.

What about openfirmware.info?  I don't know anything about Core
Systems who maintains that site though.

g.

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

Re: [RFC] Clock binding

[Stuart Yoder]

> Lets *not* do it on power.org. =A0I'd like to see the bindings used by
> more than just powerpc people, and power.org might become a bit of a
> mental barrier for non-powerpc folks. =A0kernel.org would be a good
> host. =A0So would ozlabs or infradead. =A0Or I'd be happy to maintain one
> on secretlab.

I think broadening it beyond power.org is good.  Along those lines I'd pref=
er
not having this on kernel.org, as device trees are not Linux-centric.  Ther=
e
are other OS vendors I've interacted with that I'd like to see use device t=
rees
and separating it from Linux may be better IMO.

We could also create a new domain like "devicetree.org" (?)..but it still
would nee a host.

Stuart

Re: [RFC] Clock binding

[M. Warner Losh]

In message: <955e48b80908281105q60c057e8pfc16213f17da9138@mail.gmail.co=
m>
            Stuart Yoder <stuyoder@gmail.com> writes:
: > Lets *not* do it on power.org. =A0I'd like to see the bindings used=
 by
: > more than just powerpc people, and power.org might become a bit of =
a
: > mental barrier for non-powerpc folks. =A0kernel.org would be a good=

: > host. =A0So would ozlabs or infradead. =A0Or I'd be happy to mainta=
in one
: > on secretlab.
: =

: I think broadening it beyond power.org is good.  Along those lines I'=
d prefer
: not having this on kernel.org, as device trees are not Linux-centric.=
  There
: are other OS vendors I've interacted with that I'd like to see use de=
vice trees
: and separating it from Linux may be better IMO.

I know of at least one other free OS that will be migrating to device
trees for enumeration of devices in the embedded space...

Warner

Re: [RFC] Clock binding

[Grant Likely]

On Fri, Aug 28, 2009 at 12:05 PM, Stuart Yoder<stuyoder@gmail.com> wrote:
>> Lets *not* do it on power.org. =A0I'd like to see the bindings used by
>> more than just powerpc people, and power.org might become a bit of a
>> mental barrier for non-powerpc folks. =A0kernel.org would be a good
>> host. =A0So would ozlabs or infradead. =A0Or I'd be happy to maintain on=
e
>> on secretlab.
>
> I think broadening it beyond power.org is good. =A0Along those lines I'd =
prefer
> not having this on kernel.org, as device trees are not Linux-centric. =A0=
There
> are other OS vendors I've interacted with that I'd like to see use device=
 trees
> and separating it from Linux may be better IMO.
>
> We could also create a new domain like "devicetree.org" (?)..but it still
> would nee a host.

How about right here:  http://fdt.secretlab.ca/

I've only just created the site.  I'll fill in some documentation and
structure in the next few days.  Feel free to create an account and
start adding stuff.

We'll need to talk about how best to manage bindings and have some
form of review/agreement before a binding is marked as "stable".  And
the site URL could change as well.  But in the meantime we've got a
sandbox to start playing in.

g.

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

Re: [RFC] Clock binding

[Stuart Yoder]

> How about right here: =A0http://fdt.secretlab.ca/
>
> I've only just created the site. =A0I'll fill in some documentation and
> structure in the next few days. =A0Feel free to create an account and
> start adding stuff.
>
> We'll need to talk about how best to manage bindings and have some
> form of review/agreement before a binding is marked as "stable". =A0And
> the site URL could change as well. =A0But in the meantime we've got a
> sandbox to start playing in.

Unless there are better suggestions, your site is fine with me.

One other thing-- we need to clarify how the content of the wiki
is licensed.

I assume (and hope) that the intent is for the content to be freely
usable in the most flexible way.  But, this has to be explicit.

An issue we ran into with the ePAPR was that on the IEEE
1275 working group site (http://playground.sun.com/1275/home.html)
that there were bindings that we wanted to include in the ePAPR,
but the stuff on 1275 site was not copyrighted and no license
was specified.   This meant that we were in a legally murky
situation if we cut and pasted anything from one of those
document.

My suggestion is to specify that all content of this wiki are
released to the public domain.  How to do this is a bit tricky
but Creative Commons has a license called CC0 that does
this by waving your copyright rights.

Your wiki currently has the following statement above the commit
message:

    Please note that all contributions to FDTWiki may be edited, altered,
    or removed by other contributors. If you do not want your writing to
    be edited mercilessly, then do not submit it here.
    You are also promising us that you wrote this yourself, or copied it
    from a public domain or similar free resource (see
    FDTWiki:Copyrights for details). Do not submit copyrighted work
    without permission!

I would suggest appending the following statement to the above
text:

    You irrevocably agree to release your contribution to the public
    domain.  To the extent possible under law, you waive all copyright or
    neighboring rights to your contribution.  See [CC0|link-to-CC0].

The above language comes from generated text of the CC0 tool from
Creative Commons.

Stuart

Re: [RFC] Clock binding

[Mitch Bradley]

>
> What about openfirmware.info?  I don't know anything about Core
> Systems who maintains that site though.
>   

Stefan Reinauer is the main guy there.  He has been very helpful in 
hosting the Open Firmware source tree.  I expect that he would be 
willing to host the wiki, since he has been interested in Open Firmware 
for many years.

Re: [RFC] Clock binding

[Rafal Jaworowski]

On 2009-08-28, at 18:06, Grant Likely wrote:

> On Fri, Aug 28, 2009 at 6:16 AM, Stuart Yoder<stuyoder@gmail.com>  
> wrote:
>> On Thu, Aug 27, 2009 at 9:43 PM, Benjamin
>> Herrenschmidt<benh@kernel.crashing.org> wrote:
>>> On Thu, 2009-08-27 at 16:36 -1000, Mitch Bradley wrote:
>>>> The idea of a wiki as a registration authority is a good one, but  
>>>> I'm
>>>> not volunteering to maintain it :-)
>>>
>>> here goes my hope :-)
>>>
>>> Do we have wiki's we could use on power.org or should we aim for a
>>> community place ? Anybody has suggestions ? I wouldn't even try to
>>> maintain a web site myself, that would be irresponsible of anybody  
>>> to
>>> let me do so !
>>
>> There was an ePAPR wiki on power.org, that I had started developing,
>> but it seems to have disappeared.  I'm looking into what happened.
>
> Lets *not* do it on power.org.  I'd like to see the bindings used by
> more than just powerpc people, and power.org might become a bit of a
> mental barrier for non-powerpc folks.  kernel.org would be a good
> host.  So would ozlabs or infradead.  Or I'd be happy to maintain one
> on secretlab.
>
> What about openfirmware.info?  I don't know anything about Core
> Systems who maintains that site though.

Grant,
When choosing the best location for the bindings page please consider  
it uniform enough so that various OSes can use it as a reference. We  
are very much interested in bringing FDT support for embedded FreeBSD  
(arm, powerpc), and one of the uncertainties is how to deal with  
existing Linux bindings definitions: at the moment they are maintained  
as part of kernel source tree, and there are doubts whether we should  
come up with our own set (most likely *very* similar), which we'd like  
to avoid.

Rafal

Re: [RFC] Clock binding

[Benjamin Herrenschmidt]

On Fri, 2009-08-28 at 20:24 +0200, Rafal Jaworowski wrote:
> Grant,
> When choosing the best location for the bindings page please consider  
> it uniform enough so that various OSes can use it as a reference. We  
> are very much interested in bringing FDT support for embedded FreeBSD  
> (arm, powerpc), and one of the uncertainties is how to deal with  
> existing Linux bindings definitions: at the moment they are maintained  
> as part of kernel source tree, and there are doubts whether we should  
> come up with our own set (most likely *very* similar), which we'd like  
> to avoid.

I agree. We need to be OS neutral.

In fact, one of the main issue right now with the core binding is the
phandles, which are put into "linux,phandle" properties. That must
change and we need to make the kernel aware of the change.

We will still keep linux-specific properties around, I suppose, mostly
for linux-specific things exchanged between the boot wrapper and the
kernel, but the base tree should generally be devoid of a "linux,"
property.

Cheers,
Ben.

Re: [RFC] Clock binding

[Grant Likely]

On Mon, Aug 17, 2009 at 10:21 PM, Benjamin
Herrenschmidt<benh@kernel.crashing.org> wrote:
> However, it's a bit nasty to mix strings and numbers (phandles) in a
> single property. It's possible, but would likely lead to the phandle not
> being aligned and tools such as lsprop to fail miserably to display
> those properties in any kind of readable form.
[...]
> However, I really dislike that numerical clock ID. Magic numbers suck.
> It should be a string.

Agreed.

> Hence my idea below. It's not perfect but it's the less sucky i've come
> up with so far. And then we can do some small refinements.
>
> =A0 =A0 =A0 =A0* Device has:
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0- "clock-input-names" as above
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0- "clock-map" contains list of phandle,ind=
ex
>
> =A0 =A0 =A0 =A0* Clock source has:
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0- "clock-output-names" list of strings

The big problem as you say is the referencing of both phandles and
strings in the same property, but it's not particularly pretty to
disperse the data across multiple properties.  Changes to the two
properties must be kept in sync, and if order changes in one and not
the other then badness occurs...  Actually, I don't *dislike* you're
proposed binding, but in the interest of generating and endless
goodness argument, what about the following?

As above, clock source has:
        - "clock-output-names" list of strings.  All possible inputs
must be in this list in the interest of clarity and sanity checking,
but drivers must not depend on the order of the list.  Drivers must
always resolve clocks by name.

Clock consumer has:
        - "clock-inputs" property.  Array of string 'tuples' in the
form: "input-name","clock-source-path","output-name".

This sidesteps the phandle issue by using the path string instead.
It's not as efficient space wise, but (at least when using dtc) the
syntax stays pretty sane, and the output remains mere-mortal parsable:
  ie: clock-inputs =3D "sysclk", &clocksource, "clkout_orange";

> The "index" in the clock map thus would reference the
> "clock-output-names" array in the clock provider. That means that the
> "magic number" here is entirely local to a given device-tree, doesn't
> leak into driver code, which continues using names.
>
> In addition, we can even have some smooth "upgrade" path from existing
> "clock-frequency" properties by assuming that if "clock-output-names" is
> absent, but "clock-frequency" exist, then index 0 references a fixed
> frequency clock source without a driver. This could be generally handy
> anyway to represent crystals of fixed bus clocks without having to write
> a clock source driver for them.

The analog of this could be:

clock-inputs =3D "sysclk", &clocksource, "0";

Otherwise, I think the approach is sound and I agree fully that
referencing names makes sense.

g.

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