[PATCH 0/6] Introducing Device Tree Overlays

[PATCH 0/6] Introducing Device Tree Overlays

[Pantelis Antoniou]

The following patchset introduces Device Tree overlays, a method
of dynamically altering the kernel's live Device Tree.

This patchset is against mainline as of Friday Jan 4 2013.
(4956964 Merge tag 'driver-core-3.8-rc2' of \
 	git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core)

Note that a separate patch for the DTC compiler has been posted and
is required to compile the DTS files according to the documentation.
The patch is "dtc: Dynamic symbols & fixup support"

An implementation patchset for a beaglebone cape loader will follow,
but if you want to check out a working kernel for the beaglebone please
pull from:

git://github.com/pantoniou/linux-bbxm.git branch not-capebus-v8

Pantelis Antoniou (6):
  OF: Introduce device tree node flag helpers.
  OF: export of_property_notify
  OF: Export all DT proc update functions
  OF: Introduce utility helper functions
  OF: Introduce Device Tree resolve support.
  OF: Introduce DT overlay support.

 .../devicetree/dynamic-resolution-notes.txt        |  25 +
 Documentation/devicetree/overlay-notes.txt         | 179 +++++
 drivers/of/Kconfig                                 |  19 +
 drivers/of/Makefile                                |   4 +-
 drivers/of/base.c                                  | 114 +--
 drivers/of/overlay.c                               | 831 +++++++++++++++++++++
 drivers/of/resolver.c                              | 394 ++++++++++
 drivers/of/util.c                                  | 253 +++++++
 include/linux/of.h                                 | 243 ++++++
 9 files changed, 2005 insertions(+), 57 deletions(-)
 create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
 create mode 100644 Documentation/devicetree/overlay-notes.txt
 create mode 100644 drivers/of/overlay.c
 create mode 100644 drivers/of/resolver.c
 create mode 100644 drivers/of/util.c

-- 
1.7.12

[PATCH 1/6] OF: Introduce device tree node flag helpers.

[Pantelis Antoniou]

Helper functions for working with device node flags.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 include/linux/of.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index 5ebcc5c..2ff35b5 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -114,6 +114,26 @@ static inline void of_node_set_flag(struct device_node *n, unsigned long flag)
 	set_bit(flag, &n->_flags);
 }
 
+static inline void of_node_clear_flag(struct device_node *n, unsigned long flag)
+{
+	clear_bit(flag, &n->_flags);
+}
+
+static inline int of_property_check_flag(struct property *p, unsigned long flag)
+{
+	return test_bit(flag, &p->_flags);
+}
+
+static inline void of_property_set_flag(struct property *p, unsigned long flag)
+{
+	set_bit(flag, &p->_flags);
+}
+
+static inline void of_property_clear_flag(struct property *p, unsigned long flag)
+{
+	clear_bit(flag, &p->_flags);
+}
+
 extern struct device_node *of_find_all_nodes(struct device_node *prev);
 
 /*
-- 
1.7.12

[PATCH 2/6] OF: export of_property_notify

[Pantelis Antoniou]

of_property_notify can be utilized by other users too, export it.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/base.c  |  8 +-------
 include/linux/of.h | 11 +++++++++++
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 2390ddb..d598216 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1115,7 +1115,7 @@ int of_parse_phandle_with_args(const struct device_node *np, const char *list_na
 EXPORT_SYMBOL(of_parse_phandle_with_args);
 
 #if defined(CONFIG_OF_DYNAMIC)
-static int of_property_notify(int action, struct device_node *np,
+int of_property_notify(int action, struct device_node *np,
 			      struct property *prop)
 {
 	struct of_prop_reconfig pr;
@@ -1124,12 +1124,6 @@ static int of_property_notify(int action, struct device_node *np,
 	pr.prop = prop;
 	return of_reconfig_notify(action, &pr);
 }
-#else
-static int of_property_notify(int action, struct device_node *np,
-			      struct property *prop)
-{
-	return 0;
-}
 #endif
 
 /**
diff --git a/include/linux/of.h b/include/linux/of.h
index 2ff35b5..aea3694 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -298,6 +298,17 @@ extern int of_parse_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name, int index,
 	struct of_phandle_args *out_args);
 
+#if defined(CONFIG_OF_DYNAMIC)
+extern int of_property_notify(int action, struct device_node *np,
+			      struct property *prop);
+#else
+static inline int of_property_notify(int action, struct device_node *np,
+			      struct property *prop)
+{
+	return 0;
+}
+#endif
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 
-- 
1.7.12

[PATCH 3/6] OF: Export all DT proc update functions

[Pantelis Antoniou]

There are other users for the proc DT functions.
Export them.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/base.c  | 108 ++++++++++++++++++++++++++++-------------------------
 include/linux/of.h |  29 ++++++++++++++
 2 files changed, 87 insertions(+), 50 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index d598216..526db99 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1126,6 +1126,61 @@ int of_property_notify(int action, struct device_node *np,
 }
 #endif
 
+#ifdef CONFIG_PROC_DEVICETREE
+
+void of_add_proc_dt_entry(struct device_node *dn)
+{
+	struct proc_dir_entry *ent;
+
+	ent = proc_mkdir(strrchr(dn->full_name, '/') + 1, dn->parent->pde);
+	if (ent)
+		proc_device_tree_add_node(dn, ent);
+}
+
+void of_remove_proc_dt_entry(struct device_node *dn)
+{
+	struct device_node *parent;
+	struct property *prop;
+
+	if (!dn)
+		return;
+
+	parent = dn->parent;
+	prop = dn->properties;
+	while (prop) {
+		if (dn->pde)
+			remove_proc_entry(prop->name, dn->pde);
+		prop = prop->next;
+	}
+
+	if (dn->pde)
+		remove_proc_entry(dn->pde->name,
+				parent ? parent->pde : NULL);
+}
+
+void of_add_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop)
+{
+	if (np && prop && np->pde)
+		proc_device_tree_add_prop(np->pde, prop);
+}
+
+void of_remove_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop)
+{
+	if (np && prop && np->pde)
+		proc_device_tree_remove_prop(np->pde, prop);
+}
+
+void of_update_proc_dt_prop_entry(struct device_node *np,
+		struct property *newprop, struct property *oldprop)
+{
+	if (np && newprop && oldprop && np->pde)
+		proc_device_tree_update_prop(np->pde, newprop, oldprop);
+}
+
+#endif /* CONFIG_PROC_DEVICETREE */
+
 /**
  * of_add_property - Add a property to a node
  */
@@ -1153,11 +1208,8 @@ int of_add_property(struct device_node *np, struct property *prop)
 	*next = prop;
 	write_unlock_irqrestore(&devtree_lock, flags);
 
-#ifdef CONFIG_PROC_DEVICETREE
 	/* try to add to proc as well if it was initialized */
-	if (np->pde)
-		proc_device_tree_add_prop(np->pde, prop);
-#endif /* CONFIG_PROC_DEVICETREE */
+	of_add_proc_dt_prop_entry(np, prop);
 
 	return 0;
 }
@@ -1199,11 +1251,7 @@ int of_remove_property(struct device_node *np, struct property *prop)
 	if (!found)
 		return -ENODEV;
 
-#ifdef CONFIG_PROC_DEVICETREE
-	/* try to remove the proc node as well */
-	if (np->pde)
-		proc_device_tree_remove_prop(np->pde, prop);
-#endif /* CONFIG_PROC_DEVICETREE */
+	of_remove_proc_dt_prop_entry(np, prop);
 
 	return 0;
 }
@@ -1253,11 +1301,8 @@ int of_update_property(struct device_node *np, struct property *newprop)
 	if (!found)
 		return -ENODEV;
 
-#ifdef CONFIG_PROC_DEVICETREE
 	/* try to add to proc as well if it was initialized */
-	if (np->pde)
-		proc_device_tree_update_prop(np->pde, newprop, oldprop);
-#endif /* CONFIG_PROC_DEVICETREE */
+	of_update_proc_dt_prop_entry(np, newprop, oldprop);
 
 	return 0;
 }
@@ -1293,22 +1338,6 @@ int of_reconfig_notify(unsigned long action, void *p)
 	return notifier_to_errno(rc);
 }
 
-#ifdef CONFIG_PROC_DEVICETREE
-static void of_add_proc_dt_entry(struct device_node *dn)
-{
-	struct proc_dir_entry *ent;
-
-	ent = proc_mkdir(strrchr(dn->full_name, '/') + 1, dn->parent->pde);
-	if (ent)
-		proc_device_tree_add_node(dn, ent);
-}
-#else
-static void of_add_proc_dt_entry(struct device_node *dn)
-{
-	return;
-}
-#endif
-
 /**
  * of_attach_node - Plug a device node into the tree and global list.
  */
@@ -1332,27 +1361,6 @@ int of_attach_node(struct device_node *np)
 	return 0;
 }
 
-#ifdef CONFIG_PROC_DEVICETREE
-static void of_remove_proc_dt_entry(struct device_node *dn)
-{
-	struct device_node *parent = dn->parent;
-	struct property *prop = dn->properties;
-
-	while (prop) {
-		remove_proc_entry(prop->name, dn->pde);
-		prop = prop->next;
-	}
-
-	if (dn->pde)
-		remove_proc_entry(dn->pde->name, parent->pde);
-}
-#else
-static void of_remove_proc_dt_entry(struct device_node *dn)
-{
-	return;
-}
-#endif
-
 /**
  * of_detach_node - "Unplug" a node from the device tree.
  *
diff --git a/include/linux/of.h b/include/linux/of.h
index aea3694..305b087 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -309,6 +309,35 @@ static inline int of_property_notify(int action, struct device_node *np,
 }
 #endif
 
+#ifdef CONFIG_PROC_DEVICETREE
+
+extern void of_add_proc_dt_entry(struct device_node *dn);
+extern void of_remove_proc_dt_entry(struct device_node *dn);
+
+extern void of_add_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop);
+
+extern void of_remove_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop);
+
+extern void of_update_proc_dt_prop_entry(struct device_node *np,
+		struct property *newprop, struct property *oldprop);
+#else
+
+static inline void of_add_proc_dt_entry(struct device_node *dn) { }
+static inline void of_remove_proc_dt_entry(struct device_node *dn) { }
+
+static inline void of_add_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop) { }
+
+static inline void of_remove_proc_dt_prop_entry(struct device_node *np,
+		struct property *prop) { }
+
+static inline void of_update_proc_dt_prop_entry(struct device_node *np,
+		struct property *newprop, struct property *oldprop) { }
+
+#endif
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 
-- 
1.7.12

Re: [PATCH 3/6] OF: Export all DT proc update functions

[Grant Likely]

On Fri,  4 Jan 2013 21:31:07 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> There are other users for the proc DT functions.
> Export them.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>

Hi Pantelis.

Patches 1 & 2 look good. No comments there.

This patch bothers me. The manipulation of the proc entries is part and
parcel of adding and removing nodes. The real problem seems to be that
the node addition/removal APIs need to be made usable by the overlay
code.

g.

Re: [PATCH 3/6] OF: Export all DT proc update functions

[Pantelis Antoniou]

Hi Grant,

The 3rd patch is in preparation for some patches I have in WIP that would allow
drivers to set notifications for properties that are changed in runtime.

I think that since you have the all configuration taking place via DT and you
have a method to alter DT properties in run-time via /proc/device-tree, it's quite
straightforward to be able to alter runtime behavior via the same mechanism.

I.e. if you have some property that controls a devices behavior, it's intuitive that
when you modify that property, the device's state changes accordingly.

Regards

-- Pantelis

On Mar 16, 2013, at 11:45 AM, Grant Likely wrote:

> On Fri,  4 Jan 2013 21:31:07 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
>> There are other users for the proc DT functions.
>> Export them.
>> 
>> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> 
> Hi Pantelis.
> 
> Patches 1 & 2 look good. No comments there.
> 
> This patch bothers me. The manipulation of the proc entries is part and
> parcel of adding and removing nodes. The real problem seems to be that
> the node addition/removal APIs need to be made usable by the overlay
> code.
> 
> g.

Re: [PATCH 3/6] OF: Export all DT proc update functions

[Grant Likely]

On Tue, 19 Mar 2013 13:42:32 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> Hi Grant,
> 
> The 3rd patch is in preparation for some patches I have in WIP that would allow
> drivers to set notifications for properties that are changed in runtime.

Okay, submit it with that series then please.

g.

Re: [PATCH 3/6] OF: Export all DT proc update functions

[Grant Likely]

On Fri,  4 Jan 2013 21:31:07 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> There are other users for the proc DT functions.
> Export them.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>

Actually, I cannot find the user of this patch. Why is it needed?

g.

[PATCH 4/6] OF: Introduce utility helper functions

[Pantelis Antoniou]

Introduce helper functions for working with the live DT tree.

__of_free_property() frees a dynamically created property
__of_free_tree() recursively frees a device node tree
__of_copy_property() copies a property dynamically
__of_create_empty_node() creates an empty node
__of_find_node_by_full_name() finds the node with the full name
and
of_multi_prop_cmp() performs a multi property compare but without
having to take locks.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/Makefile |   2 +-
 drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h  |  59 ++++++++++++
 3 files changed, 313 insertions(+), 1 deletion(-)
 create mode 100644 drivers/of/util.c

diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..abcc08a 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -1,4 +1,4 @@
-obj-y = base.o
+obj-y = base.o util.o
 obj-$(CONFIG_OF_FLATTREE) += fdt.o
 obj-$(CONFIG_OF_PROMTREE) += pdt.o
 obj-$(CONFIG_OF_ADDRESS)  += address.o
diff --git a/drivers/of/util.c b/drivers/of/util.c
new file mode 100644
index 0000000..5117e2b
--- /dev/null
+++ b/drivers/of/util.c
@@ -0,0 +1,253 @@
+/*
+ * Utility functions for working with device tree(s)
+ *
+ * Copyright (C) 2012 Pantelis Antoniou <panto@antoniou-consulting.com>
+ * Copyright (C) 2012 Texas Instruments Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/string.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+
+/**
+ * __of_free_property - release the memory of an allocated property
+ * @prop:	Property to release
+ *
+ * Release the memory of an allocated property only after checking
+ * that the property has been marked as OF_DYNAMIC.
+ * Only call on known allocated properties.
+ */
+void __of_free_property(struct property *prop)
+{
+	if (prop == NULL)
+		return;
+
+	if (of_property_check_flag(prop, OF_DYNAMIC)) {
+		kfree(prop->value);
+		kfree(prop->name);
+		kfree(prop);
+	} else {
+		pr_warn("%s: property %p cannot be freed; memory is gone\n",
+				__func__, prop);
+	}
+}
+
+/**
+ * __of_free_tree - release the memory of a device tree node and
+ *		    of all it's children + properties.
+ * @node:	Device Tree node to release
+ *
+ * Release the memory of a device tree node and of all it's children.
+ * Also release the properties and the dead properties.
+ * Only call on detached node trees, and you better be sure that
+ * no pointer exist for any properties. Only safe to do if you 
+ * absolutely control the life cycle of the node.
+ * Also note that the node is not removed from the all_nodes list,
+ * neither from the parent's child list; this should be handled before
+ * calling this function.
+ */
+void __of_free_tree(struct device_node *node)
+{
+	struct property *prop;
+	struct device_node *noden;
+
+	/* sanity check */
+	if (!node)
+		return;
+
+	/* free recursively any children */
+	while ((noden = node->child) != NULL) {
+		node->child = noden->sibling;
+		__of_free_tree(noden);
+	}
+
+	/* free every property already allocated */
+	while ((prop = node->properties) != NULL) {
+		node->properties = prop->next;
+		__of_free_property(prop);
+	}
+
+	/* free dead properties already allocated */
+	while ((prop = node->deadprops) != NULL) {
+		node->deadprops = prop->next;
+		__of_free_property(prop);
+	}
+
+	if (of_node_check_flag(node, OF_DYNAMIC)) {
+		kfree(node->type);
+		kfree(node->name);
+		kfree(node);
+	} else {
+		pr_warn("%s: node %p cannot be freed; memory is gone\n",
+				__func__, node);
+	}
+}
+
+/**
+ * __of_copy_property - Copy a property dynamically.
+ * @prop:	Property to copy
+ * @flags:	Allocation flags (typically pass GFP_KERNEL)
+ *
+ * Copy a property by dynamically allocating the memory of both the
+ * property stucture and the property name & contents. The property's
+ * flags have the OF_DYNAMIC bit set so that we can differentiate between
+ * dynamically allocated properties and not.
+ * Returns the newly allocated property or NULL on out of memory error.
+ */
+struct property *__of_copy_property(const struct property *prop, gfp_t flags)
+{
+	struct property *propn;
+
+	propn = kzalloc(sizeof(*prop), flags);
+	if (propn == NULL)
+		return NULL;
+
+	propn->name = kstrdup(prop->name, flags);
+	if (propn->name == NULL)
+		goto err_fail_name;
+
+	if (prop->length > 0) {
+		propn->value = kmalloc(prop->length, flags);
+		if (propn->value == NULL)
+			goto err_fail_value;
+		memcpy(propn->value, prop->value, prop->length);
+		propn->length = prop->length;
+	}
+
+	/* mark the property as dynamic */
+	of_property_set_flag(propn, OF_DYNAMIC);
+
+	return propn;
+
+err_fail_value:
+	kfree(propn->name);
+err_fail_name:
+	kfree(propn);
+	return NULL;
+}
+
+/**
+ * __of_create_empty_node - Create an empty device node dynamically.
+ * @name:	Name of the new device node
+ * @type:	Type of the new device node
+ * @full_name:	Full name of the new device node
+ * @phandle:	Phandle of the new device node
+ * @flags:	Allocation flags (typically pass GFP_KERNEL)
+ *
+ * Create an empty device tree node, suitable for further modification.
+ * The node data are dynamically allocated and all the node flags
+ * have the OF_DYNAMIC & OF_DETACHED bits set.
+ * Returns the newly allocated node or NULL on out of memory error.
+ */
+struct device_node *__of_create_empty_node(
+		const char *name, const char *type, const char *full_name,
+		phandle phandle, gfp_t flags)
+{
+	struct device_node *node;
+
+	node = kzalloc(sizeof(*node), flags);
+	if (node == NULL)
+		return NULL;
+
+	node->name = kstrdup(name, flags);
+	if (node->name == NULL)
+		goto err_return;
+
+	node->type = kstrdup(type, flags);
+	if (node->type == NULL)
+		goto err_return;
+
+	node->full_name = kstrdup(full_name, flags);
+	if (node->type == NULL)
+		goto err_return;
+
+	node->phandle = phandle;
+	kref_init(&node->kref);
+	of_node_set_flag(node, OF_DYNAMIC);
+	of_node_set_flag(node, OF_DETACHED);
+
+	return node;
+
+err_return:
+	__of_free_tree(node);
+	return NULL;
+}
+
+/**
+ * __of_find_node_by_full_name - Find a node with the full name recursively
+ * @node:	Root of the tree to perform the search
+ * @full_name:	Full name of the node to find.
+ *
+ * Find a node with the give full name by recursively following any of 
+ * the child node links.
+ * Returns the matching node, or NULL if not found.
+ * Note that the devtree lock is not taken, so this function is only
+ * safe to call on either detached trees, or when devtree lock is already
+ * taken.
+ */
+struct device_node *__of_find_node_by_full_name(struct device_node *node,
+		const char *full_name)
+{
+	struct device_node *child, *found;
+
+	if (node == NULL)
+		return NULL;
+
+	/* check */
+	if (of_node_cmp(node->full_name, full_name) == 0)
+		return node;
+
+	__for_each_child_of_node(node, child) {
+		found = __of_find_node_by_full_name(child, full_name);
+		if (found != NULL)
+			return found;
+	}
+
+	return NULL;
+}
+
+/**
+ * of_multi_prop_cmp - Check if a property matches a value
+ * @prop:	Property to check
+ * @value:	Value to check against
+ *
+ * Check whether a property matches a value, using the standard
+ * of_compat_cmp() test on each string. It is similar to the test
+ * of_device_is_compatible() makes, but it can be performed without
+ * taking the devtree_lock, which is required in some cases.
+ * Returns 0 on a match, -1 on no match.
+ */
+int of_multi_prop_cmp(const struct property *prop, const char *value)
+{
+	const char *cp;
+	int cplen, vlen, l;
+
+	if (prop == NULL || value == NULL)
+		return -1;
+
+	cp = prop->value;
+	cplen = prop->length;
+	vlen = strlen(value);
+
+	while (cplen > 0) {
+		if (of_compat_cmp(cp, value, vlen) == 0)
+			return 0;
+		l = strlen(cp) + 1;
+		cp += l;
+		cplen -= l;
+	}
+
+	return -1;
+}
+
diff --git a/include/linux/of.h b/include/linux/of.h
index 305b087..c38e41a 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -591,4 +591,63 @@ static inline int of_property_read_u32(const struct device_node *np,
 	return of_property_read_u32_array(np, propname, out_value, 1);
 }
 
+/**
+ * General utilities for working with live trees.
+ *
+ * All functions with two leading underscores operate
+ * without taking node references, so you either have to
+ * own the devtree lock or work on detached trees only.
+ */
+
+#ifdef CONFIG_OF
+
+/* iterator for internal use; not references, neither affects devtree lock */
+#define __for_each_child_of_node(dn, chld) \
+	for (chld = (dn)->child; chld != NULL; chld = chld->sibling)
+
+void __of_free_property(struct property *prop);
+void __of_free_tree(struct device_node *node);
+struct property *__of_copy_property(const struct property *prop, gfp_t flags);
+struct device_node *__of_create_empty_node( const char *name,
+		const char *type, const char *full_name,
+		phandle phandle, gfp_t flags);
+struct device_node *__of_find_node_by_full_name(struct device_node *node,
+		const char *full_name);
+int of_multi_prop_cmp(const struct property *prop, const char *value);
+
+#else /* !CONFIG_OF */
+
+#define __for_each_child_of_node(dn, chld) \
+	while (0)
+
+static inline void __of_free_property(struct property *prop) { }
+
+static inline void __of_free_tree(struct device_node *node) { }
+
+static inline struct property *__of_copy_property(const struct property *prop,
+		gfp_t flags)
+{
+	return NULL;
+}
+
+static inline struct device_node *__of_create_empty_node( const char *name,
+		const char *type, const char *full_name,
+		phandle phandle, gfp_t flags)
+{
+	return NULL;
+}
+
+static inline struct device_node *__of_find_node_by_full_name(struct device_node *node,
+		const char *full_name)
+{
+	return NULL;
+}
+
+static inline int of_multi_prop_cmp(const struct property *prop, const char *value)
+{
+	return -1;
+}
+
+#endif	/* !CONFIG_OF */
+
 #endif /* _LINUX_OF_H */
-- 
1.7.12

[PATCH 5/6] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

Introduce support for dynamic device tree resolution.
Using it, it is possible to prepare a device tree that's
been loaded on runtime to be modified and inserted at the kernel
live tree.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 .../devicetree/dynamic-resolution-notes.txt        |  25 ++
 drivers/of/Kconfig                                 |   9 +
 drivers/of/Makefile                                |   1 +
 drivers/of/resolver.c                              | 394 +++++++++++++++++++++
 include/linux/of.h                                 |  17 +
 5 files changed, 446 insertions(+)
 create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
 create mode 100644 drivers/of/resolver.c

diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt b/Documentation/devicetree/dynamic-resolution-notes.txt
new file mode 100644
index 0000000..0b396c4
--- /dev/null
+++ b/Documentation/devicetree/dynamic-resolution-notes.txt
@@ -0,0 +1,25 @@
+Device Tree Dynamic Resolver Notes
+----------------------------------
+
+This document describes the implementation of the in-kernel
+Device Tree resolver, residing in drivers/of/resolver.c and is a
+companion document to Documentation/devicetree/dt-object-internal.txt[1]
+
+How the resolver works
+----------------------
+
+The resolver is given as an input an arbitrary tree compiled with the
+proper dtc option and having a /plugin/ tag. This generates the
+appropriate __fixups__ & __local_fixups__ nodes as described in [1].
+
+In sequence the resolver works by the following steps:
+
+1. Get the maximum device tree phandle value from the live tree + 1.
+2. Adjust all the local phandles of the tree to resolve by that amount.
+3. Using the __local__fixups__ node information adjust all local references
+   by the same amount.
+4. For each property in the __fixups__ node locate the node it references
+   in the live tree. This is the label used to tag the node.
+5. Retrieve the phandle of the target of the fixup.
+5. For each fixup in the property locate the node:property:offset location
+   and replace it with the phandle value.
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index d37bfcf..f9a6193 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,13 @@ config OF_MTD
 	depends on MTD
 	def_bool y
 
+config OF_RESOLVE
+	bool "OF Dynamic resolution support"
+	depends on OF
+	select OF_DYNAMIC
+	select OF_DEVICE
+	help
+	  Enable OF dynamic resolution support. This allows you to
+	  load Device Tree object fragments are run time.
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index abcc08a..9a809c9 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
+obj-$(CONFIG_OF_RESOLVE)  += resolver.o
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
new file mode 100644
index 0000000..016d7da
--- /dev/null
+++ b/drivers/of/resolver.c
@@ -0,0 +1,394 @@
+/*
+ * Functions for dealing with DT resolution
+ *
+ * Copyright (C) 2012 Pantelis Antoniou <panto@antoniou-consulting.com>
+ * Copyright (C) 2012 Texas Instruments Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_fdt.h>
+#include <linux/string.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+
+/**
+ * Find a subtree's maximum phandle value.
+ */
+static phandle __of_get_tree_max_phandle(struct device_node *node,
+		phandle max_phandle)
+{
+	struct device_node *child;
+
+	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
+			node->phandle > max_phandle)
+		max_phandle = node->phandle;
+
+	__for_each_child_of_node(node, child)
+		max_phandle = __of_get_tree_max_phandle(child, max_phandle);
+
+	return max_phandle;
+}
+
+/**
+ * Find live tree's maximum phandle value.
+ */
+static phandle of_get_tree_max_phandle(void)
+{
+	struct device_node *node;
+	phandle phandle;
+
+	/* get root node */
+	node = of_find_node_by_path("/");
+	if (node == NULL)
+		return OF_PHANDLE_ILLEGAL;
+
+	/* now search recursively */
+	read_lock(&devtree_lock);
+	phandle = __of_get_tree_max_phandle(node, 0);
+	read_unlock(&devtree_lock);
+
+	of_node_put(node);
+
+	return phandle;
+}
+
+/**
+ * Adjust a subtree's phandle values by a given delta.
+ * Makes sure not to just adjust the device node's phandle value,
+ * but modify the phandle properties values as well.
+ */
+static void __of_adjust_tree_phandles(struct device_node *node,
+		int phandle_delta)
+{
+	struct device_node *child;
+	struct property *prop;
+	phandle phandle;
+
+	/* first adjust the node's phandle direct value */
+	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
+		node->phandle += phandle_delta;
+
+	/* now adjust phandle & linux,phandle values */
+	for_each_property_of_node(node, prop) {
+
+		/* only look for these two */
+		if (of_prop_cmp(prop->name, "phandle") != 0 &&
+		    of_prop_cmp(prop->name, "linux,phandle") != 0)
+			continue;
+
+		/* must be big enough */
+		if (prop->length < 4)
+			continue;
+
+		/* read phandle value */
+		phandle = be32_to_cpu(*(uint32_t *)prop->value);
+		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
+			continue;
+
+		/* adjust */
+		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
+	}
+
+	/* now do the children recursively */
+	__for_each_child_of_node(node, child)
+		__of_adjust_tree_phandles(child, phandle_delta);
+}
+
+/**
+ * Adjust the local phandle references by the given phandle delta.
+ * Assumes the existances of a __local_fixups__ node at the root
+ * of the tree. Does not take any devtree locks so make sure you
+ * call this on a tree which is at the detached state.
+ */
+static int __of_adjust_tree_phandle_references(struct device_node *node,
+		int phandle_delta)
+{
+	phandle phandle;
+	struct device_node *refnode, *child;
+	struct property *rprop, *sprop;
+	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
+	int offset, propcurlen;
+	int err;
+
+	/* locate the symbols & fixups nodes on resolve */
+	__for_each_child_of_node(node, child)
+		if (of_node_cmp(child->name, "__local_fixups__") == 0)
+			break;
+
+	/* no local fixups */
+	if (child == NULL)
+		return 0;
+
+	/* find the local fixups property */
+	for_each_property_of_node(child, rprop) {
+
+		/* skip properties added automatically */
+		if (of_prop_cmp(rprop->name, "name") == 0)
+			continue;
+
+		/* make a copy */
+		propval = kmalloc(rprop->length, GFP_KERNEL);
+		if (propval == NULL) {
+			pr_err("%s: Could not copy value of '%s'\n",
+					__func__, rprop->name);
+			return -ENOMEM;
+		}
+		memcpy(propval, rprop->value, rprop->length);
+
+		propend = propval + rprop->length;
+		for (propcur = propval; propcur < propend;
+				propcur += propcurlen + 1) {
+
+			propcurlen = strlen(propcur);
+
+			nodestr = propcur;
+			s = strchr(propcur, ':');
+			if (s == NULL) {
+				pr_err("%s: Illegal symbol entry '%s' (1)\n",
+					__func__, propcur);
+				err = -EINVAL;
+				goto err_fail;
+			}
+			*s++ = '\0';
+
+			propstr = s;
+			s = strchr(s, ':');
+			if (s == NULL) {
+				pr_err("%s: Illegal symbol entry '%s' (2)\n",
+					__func__, (char *)rprop->value);
+				err = -EINVAL;
+				goto err_fail;
+			}
+
+			*s++ = '\0';
+			offset = simple_strtoul(s, NULL, 10);
+
+			/* look into the resolve node for the full path */
+			refnode = __of_find_node_by_full_name(node, nodestr);
+			if (refnode == NULL) {
+				pr_warn("%s: Could not find refnode '%s'\n",
+					__func__, (char *)rprop->value);
+				continue;
+			}
+
+			/* now find the property */
+			for_each_property_of_node(refnode, sprop) {
+				if (of_prop_cmp(sprop->name, propstr) == 0)
+					break;
+			}
+
+			if (sprop == NULL) {
+				pr_err("%s: Could not find property '%s'\n",
+					__func__, (char *)rprop->value);
+				err = -ENOENT;
+				goto err_fail;
+			}
+
+			phandle = be32_to_cpu(*(uint32_t *)
+					(sprop->value + offset));
+			*(uint32_t *)(sprop->value + offset) =
+				cpu_to_be32(phandle + phandle_delta);
+		}
+
+		kfree(propval);
+	}
+
+	return 0;
+
+err_fail:
+	kfree(propval);
+	return err;
+}
+
+/**
+ * of_resolve	- Resolve the given node against the live tree.
+ *
+ * @resolve:	Node to resolve
+ *
+ * Perform dynamic Device Tree resolution against the live tree
+ * to the given node to resolve. This depends on the live tree
+ * having a __symbols__ node, and the resolve node the __fixups__ &
+ * __local_fixups__ nodes (if needed).
+ * The result of the operation is a resolve node that it's contents
+ * are fit to be inserted or operate upon the live tree.
+ * Returns 0 on success or a negative error value on error.
+ */
+int of_resolve(struct device_node *resolve)
+{
+	struct device_node *child, *refnode;
+	struct device_node *root_sym, *resolve_sym, *resolve_fix;
+	struct property *rprop, *sprop;
+	const char *refpath;
+	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
+	int offset, propcurlen;
+	phandle phandle, phandle_delta;
+	int err;
+
+	/* the resolve node must exist, and be detached */
+	if (resolve == NULL ||
+			!of_node_check_flag(resolve, OF_DETACHED)) {
+		return -EINVAL;
+	}
+
+	/* first we need to adjust the phandles */
+	phandle_delta = of_get_tree_max_phandle() + 1;
+	__of_adjust_tree_phandles(resolve, phandle_delta);
+	err = __of_adjust_tree_phandle_references(resolve, phandle_delta);
+	if (err != 0)
+		return err;
+
+	root_sym = NULL;
+	resolve_sym = NULL;
+	resolve_fix = NULL;
+
+	/* this may fail (if no fixups are required) */
+	root_sym = of_find_node_by_path("/__symbols__");
+
+	/* locate the symbols & fixups nodes on resolve */
+	__for_each_child_of_node(resolve, child) {
+
+		if (resolve_sym == NULL &&
+				of_node_cmp(child->name, "__symbols__") == 0)
+			resolve_sym = child;
+
+		if (resolve_fix == NULL &&
+				of_node_cmp(child->name, "__fixups__") == 0)
+			resolve_fix = child;
+
+		/* both found, don't bother anymore */
+		if (resolve_sym != NULL && resolve_fix != NULL)
+			break;
+	}
+
+	/* we do allow for the case where no fixups are needed */
+	if (resolve_fix == NULL)
+		goto merge_sym;
+
+	/* we need to fixup, but no root symbols... */
+	if (root_sym == NULL)
+		return -EINVAL;
+
+	for_each_property_of_node(resolve_fix, rprop) {
+
+		/* skip properties added automatically */
+		if (of_prop_cmp(rprop->name, "name") == 0)
+			continue;
+
+		err = of_property_read_string(root_sym,
+				rprop->name, &refpath);
+		if (err != 0) {
+			pr_err("%s: Could not find symbol '%s'\n",
+					__func__, rprop->name);
+			goto err_fail;
+		}
+
+		refnode = of_find_node_by_path(refpath);
+		if (refnode == NULL) {
+			pr_err("%s: Could not find node by path '%s'\n",
+					__func__, refpath);
+			err = -ENOENT;
+			goto err_fail;
+		}
+
+		phandle = refnode->phandle;
+		of_node_put(refnode);
+
+		pr_debug("%s: %s phandle is 0x%08x\n",
+				__func__, rprop->name, phandle);
+
+		/* make a copy */
+		propval = kmalloc(rprop->length, GFP_KERNEL);
+		if (propval == NULL) {
+			pr_err("%s: Could not copy value of '%s'\n",
+					__func__, rprop->name);
+			err = -ENOMEM;
+			goto err_fail;
+		}
+
+		memcpy(propval, rprop->value, rprop->length);
+
+		propend = propval + rprop->length;
+		for (propcur = propval; propcur < propend;
+				propcur += propcurlen + 1) {
+			propcurlen = strlen(propcur);
+
+			nodestr = propcur;
+			s = strchr(propcur, ':');
+			if (s == NULL) {
+				pr_err("%s: Illegal symbol "
+					"entry '%s' (1)\n",
+					__func__, (char *)rprop->value);
+				kfree(propval);
+				err = -EINVAL;
+				goto err_fail;
+			}
+			*s++ = '\0';
+
+			propstr = s;
+			s = strchr(s, ':');
+			if (s == NULL) {
+				pr_err("%s: Illegal symbol "
+					"entry '%s' (2)\n",
+					__func__, (char *)rprop->value);
+				kfree(propval);
+				err = -EINVAL;
+				goto err_fail;
+			}
+
+			*s++ = '\0';
+			offset = simple_strtoul(s, NULL, 10);
+
+			/* look into the resolve node for the full path */
+			refnode = __of_find_node_by_full_name(resolve,
+					nodestr);
+			if (refnode == NULL) {
+				pr_err("%s: Could not find refnode '%s'\n",
+					__func__, (char *)rprop->value);
+				kfree(propval);
+				err = -ENOENT;
+				goto err_fail;
+			}
+
+			/* now find the property */
+			for_each_property_of_node(refnode, sprop) {
+				if (of_prop_cmp(sprop->name, propstr) == 0)
+					break;
+			}
+
+			if (sprop == NULL) {
+				pr_err("%s: Could not find property '%s'\n",
+					__func__, (char *)rprop->value);
+				kfree(propval);
+				err = -ENOENT;
+				goto err_fail;
+			}
+
+			*(uint32_t *)(sprop->value + offset) =
+				cpu_to_be32(phandle);
+		}
+
+		kfree(propval);
+	}
+
+merge_sym:
+
+	of_node_put(root_sym);
+
+	return 0;
+
+err_fail:
+
+	if (root_sym != NULL)
+		of_node_put(root_sym);
+
+	return err;
+}
diff --git a/include/linux/of.h b/include/linux/of.h
index c38e41a..ab52243 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -650,4 +650,21 @@ static inline int of_multi_prop_cmp(const struct property *prop, const char *val
 
 #endif	/* !CONFIG_OF */
 
+
+/* illegal phandle value (set when unresolved) */
+#define OF_PHANDLE_ILLEGAL	0xdeadbeef
+
+#ifdef CONFIG_OF_RESOLVE
+
+int of_resolve(struct device_node *resolve);
+
+#else
+
+static inline int of_resolve(struct device_node *resolve)
+{
+	return -ENOTSUPP;
+}
+
+#endif
+
 #endif /* _LINUX_OF_H */
-- 
1.7.12

Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 16365 bytes --]

On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou wrote:
> Introduce support for dynamic device tree resolution.
> Using it, it is possible to prepare a device tree that's
> been loaded on runtime to be modified and inserted at the kernel
> live tree.
> 
> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> ---
>  .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>  drivers/of/Kconfig                                 |   9 +
>  drivers/of/Makefile                                |   1 +
>  drivers/of/resolver.c                              | 394 +++++++++++++++++++++
>  include/linux/of.h                                 |  17 +
>  5 files changed, 446 insertions(+)
>  create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
>  create mode 100644 drivers/of/resolver.c
> 
> diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt b/Documentation/devicetree/dynamic-resolution-notes.txt
> new file mode 100644
> index 0000000..0b396c4
> --- /dev/null
> +++ b/Documentation/devicetree/dynamic-resolution-notes.txt
> @@ -0,0 +1,25 @@
> +Device Tree Dynamic Resolver Notes
> +----------------------------------
> +
> +This document describes the implementation of the in-kernel
> +Device Tree resolver, residing in drivers/of/resolver.c and is a
> +companion document to Documentation/devicetree/dt-object-internal.txt[1]
> +
> +How the resolver works
> +----------------------
> +
> +The resolver is given as an input an arbitrary tree compiled with the
> +proper dtc option and having a /plugin/ tag. This generates the
> +appropriate __fixups__ & __local_fixups__ nodes as described in [1].
> +
> +In sequence the resolver works by the following steps:
> +
> +1. Get the maximum device tree phandle value from the live tree + 1.
> +2. Adjust all the local phandles of the tree to resolve by that amount.
> +3. Using the __local__fixups__ node information adjust all local references
> +   by the same amount.
> +4. For each property in the __fixups__ node locate the node it references
> +   in the live tree. This is the label used to tag the node.
> +5. Retrieve the phandle of the target of the fixup.
> +5. For each fixup in the property locate the node:property:offset location
> +   and replace it with the phandle value.

Hrm.  So, I'm really still not convinced by this approach.

	First, I think it's unwise to allow overlays to change
essentially anything in the base tree, rather than having the base
tree define sockets of some sort where things can be attached.

	Second, even allowing overlays to change anything, I don't see
a lot of reason to do this kind of resolution within the kernel and
with data stored in the dtb itself, rather than doing the resolution
in userspace from an annotated overlay dts or dtb, then inserting the
fully resolved product into the kernel.  In either case, the overlay
needs to be constructed with pretty intimate knowledge of the base
tree.

That said, I have some implementation comments below.

[snip]
> +/**
> + * Find a subtree's maximum phandle value.
> + */
> +static phandle __of_get_tree_max_phandle(struct device_node *node,
> +		phandle max_phandle)
> +{
> +	struct device_node *child;
> +
> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
> +			node->phandle > max_phandle)
> +		max_phandle = node->phandle;
> +
> +	__for_each_child_of_node(node, child)
> +		max_phandle = __of_get_tree_max_phandle(child, max_phandle);

Recursion is best avoided given the kernel's limited stack space.
This is also trivial to implement non-recursively, using the allnext
pointer.

> +
> +	return max_phandle;
> +}
> +
> +/**
> + * Find live tree's maximum phandle value.
> + */
> +static phandle of_get_tree_max_phandle(void)
> +{
> +	struct device_node *node;
> +	phandle phandle;
> +
> +	/* get root node */
> +	node = of_find_node_by_path("/");
> +	if (node == NULL)
> +		return OF_PHANDLE_ILLEGAL;
> +
> +	/* now search recursively */
> +	read_lock(&devtree_lock);
> +	phandle = __of_get_tree_max_phandle(node, 0);
> +	read_unlock(&devtree_lock);
> +
> +	of_node_put(node);
> +
> +	return phandle;
> +}
> +
> +/**
> + * Adjust a subtree's phandle values by a given delta.
> + * Makes sure not to just adjust the device node's phandle value,
> + * but modify the phandle properties values as well.
> + */
> +static void __of_adjust_tree_phandles(struct device_node *node,
> +		int phandle_delta)
> +{
> +	struct device_node *child;
> +	struct property *prop;
> +	phandle phandle;
> +
> +	/* first adjust the node's phandle direct value */
> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
> +		node->phandle += phandle_delta;

You need to have some kind of check for overflow here, or the adjusted
phandle could be one of the illegal values (0 or -1) - or wrap around
and colllide with existing phandle values in the base tree.  dtc
(currently) allocates phandles from the bottom, but there's no
guarantee that a base tree will only have low phandle values - it only
takes one node with phandle set to 0xfffffffe in the base tree to have
this function make a mess of things.


> +	/* now adjust phandle & linux,phandle values */
> +	for_each_property_of_node(node, prop) {
> +
> +		/* only look for these two */
> +		if (of_prop_cmp(prop->name, "phandle") != 0 &&
> +		    of_prop_cmp(prop->name, "linux,phandle") != 0)
> +			continue;
> +
> +		/* must be big enough */
> +		if (prop->length < 4)
> +			continue;

If prop->length != 4 (including > 4) something is pretty wrong, and
you should probably bail with an error message.

> +
> +		/* read phandle value */
> +		phandle = be32_to_cpu(*(uint32_t *)prop->value);
> +		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
> +			continue;
> +
> +		/* adjust */
> +		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
> +	}
> +
> +	/* now do the children recursively */
> +	__for_each_child_of_node(node, child)
> +		__of_adjust_tree_phandles(child, phandle_delta);

Again, recursion is not a good idea.

> +}
> +
> +/**
> + * Adjust the local phandle references by the given phandle delta.
> + * Assumes the existances of a __local_fixups__ node at the root
> + * of the tree. Does not take any devtree locks so make sure you
> + * call this on a tree which is at the detached state.
> + */
> +static int __of_adjust_tree_phandle_references(struct device_node *node,
> +		int phandle_delta)
> +{
> +	phandle phandle;
> +	struct device_node *refnode, *child;
> +	struct property *rprop, *sprop;
> +	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
> +	int offset, propcurlen;
> +	int err;
> +
> +	/* locate the symbols & fixups nodes on resolve */
> +	__for_each_child_of_node(node, child)
> +		if (of_node_cmp(child->name, "__local_fixups__") == 0)
> +			break;
> +
> +	/* no local fixups */
> +	if (child == NULL)
> +		return 0;
> +
> +	/* find the local fixups property */
> +	for_each_property_of_node(child, rprop) {
> +
> +		/* skip properties added automatically */
> +		if (of_prop_cmp(rprop->name, "name") == 0)
> +			continue;

Ok, so you're interpreting any property except name in the
__local_fixups__ node in exactly the same way?  That's a bit strange.
Why not just have a single property rather than a node's worth in that
case.

> +		/* make a copy */
> +		propval = kmalloc(rprop->length, GFP_KERNEL);
> +		if (propval == NULL) {
> +			pr_err("%s: Could not copy value of '%s'\n",
> +					__func__, rprop->name);
> +			return -ENOMEM;
> +		}
> +		memcpy(propval, rprop->value, rprop->length);
> +
> +		propend = propval + rprop->length;
> +		for (propcur = propval; propcur < propend;
> +				propcur += propcurlen + 1) {
> +
> +			propcurlen = strlen(propcur);
> +
> +			nodestr = propcur;
> +			s = strchr(propcur, ':');

So, using strings with separators like this doesn't sit will with
existing device tree practice.  More similar to existing things would
have NUL separators and the integer values in binary, rather than
text (and yes, there is precedent for mixed string and integer content
in properties).


> +			if (s == NULL) {
> +				pr_err("%s: Illegal symbol entry '%s' (1)\n",
> +					__func__, propcur);
> +				err = -EINVAL;
> +				goto err_fail;
> +			}
> +			*s++ = '\0';

And using the separators you do leads to this rather ugly copy then
mangle-in-place approach to parsing.

> +			propstr = s;
> +			s = strchr(s, ':');
> +			if (s == NULL) {
> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
> +					__func__, (char *)rprop->value);
> +				err = -EINVAL;
> +				goto err_fail;
> +			}
> +
> +			*s++ = '\0';
> +			offset = simple_strtoul(s, NULL, 10);
> +
> +			/* look into the resolve node for the full path */
> +			refnode = __of_find_node_by_full_name(node, nodestr);
> +			if (refnode == NULL) {
> +				pr_warn("%s: Could not find refnode '%s'\n",
> +					__func__, (char *)rprop->value);
> +				continue;
> +			}
> +
> +			/* now find the property */
> +			for_each_property_of_node(refnode, sprop) {
> +				if (of_prop_cmp(sprop->name, propstr) == 0)
> +					break;
> +			}
> +
> +			if (sprop == NULL) {
> +				pr_err("%s: Could not find property '%s'\n",
> +					__func__, (char *)rprop->value);
> +				err = -ENOENT;
> +				goto err_fail;
> +			}
> +
> +			phandle = be32_to_cpu(*(uint32_t *)
> +					(sprop->value + offset));
> +			*(uint32_t *)(sprop->value + offset) =
> +				cpu_to_be32(phandle + phandle_delta);
> +		}
> +
> +		kfree(propval);
> +	}
> +
> +	return 0;
> +
> +err_fail:
> +	kfree(propval);
> +	return err;
> +}
> +
> +/**
> + * of_resolve	- Resolve the given node against the live tree.
> + *
> + * @resolve:	Node to resolve
> + *
> + * Perform dynamic Device Tree resolution against the live tree
> + * to the given node to resolve. This depends on the live tree
> + * having a __symbols__ node, and the resolve node the __fixups__ &
> + * __local_fixups__ nodes (if needed).
> + * The result of the operation is a resolve node that it's contents
> + * are fit to be inserted or operate upon the live tree.
> + * Returns 0 on success or a negative error value on error.
> + */
> +int of_resolve(struct device_node *resolve)
> +{
> +	struct device_node *child, *refnode;
> +	struct device_node *root_sym, *resolve_sym, *resolve_fix;
> +	struct property *rprop, *sprop;
> +	const char *refpath;
> +	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
> +	int offset, propcurlen;
> +	phandle phandle, phandle_delta;
> +	int err;
> +
> +	/* the resolve node must exist, and be detached */
> +	if (resolve == NULL ||
> +			!of_node_check_flag(resolve, OF_DETACHED)) {
> +		return -EINVAL;
> +	}
> +
> +	/* first we need to adjust the phandles */
> +	phandle_delta = of_get_tree_max_phandle() + 1;
> +	__of_adjust_tree_phandles(resolve, phandle_delta);
> +	err = __of_adjust_tree_phandle_references(resolve, phandle_delta);
> +	if (err != 0)
> +		return err;
> +
> +	root_sym = NULL;
> +	resolve_sym = NULL;
> +	resolve_fix = NULL;
> +
> +	/* this may fail (if no fixups are required) */
> +	root_sym = of_find_node_by_path("/__symbols__");
> +
> +	/* locate the symbols & fixups nodes on resolve */
> +	__for_each_child_of_node(resolve, child) {
> +
> +		if (resolve_sym == NULL &&

No need for the NULL check.  If there are duplicate node names, you've
already got big trouble, and picking the last matching will do no
worse than picking the first matching.

> +				of_node_cmp(child->name, "__symbols__") == 0)
> +			resolve_sym = child;
> +
> +		if (resolve_fix == NULL &&
> +				of_node_cmp(child->name, "__fixups__") == 0)
> +			resolve_fix = child;
> +
> +		/* both found, don't bother anymore */
> +		if (resolve_sym != NULL && resolve_fix != NULL)
> +			break;
> +	}
> +
> +	/* we do allow for the case where no fixups are needed */
> +	if (resolve_fix == NULL)
> +		goto merge_sym;

Hrm.  I'm not convinced that's one of the kernel-allowed forms of
goto.

> +
> +	/* we need to fixup, but no root symbols... */
> +	if (root_sym == NULL)
> +		return -EINVAL;
> +
> +	for_each_property_of_node(resolve_fix, rprop) {
> +
> +		/* skip properties added automatically */
> +		if (of_prop_cmp(rprop->name, "name") == 0)
> +			continue;
> +
> +		err = of_property_read_string(root_sym,
> +				rprop->name, &refpath);
> +		if (err != 0) {
> +			pr_err("%s: Could not find symbol '%s'\n",
> +					__func__, rprop->name);
> +			goto err_fail;
> +		}
> +
> +		refnode = of_find_node_by_path(refpath);
> +		if (refnode == NULL) {
> +			pr_err("%s: Could not find node by path '%s'\n",
> +					__func__, refpath);
> +			err = -ENOENT;
> +			goto err_fail;
> +		}
> +
> +		phandle = refnode->phandle;
> +		of_node_put(refnode);
> +
> +		pr_debug("%s: %s phandle is 0x%08x\n",
> +				__func__, rprop->name, phandle);
> +
> +		/* make a copy */
> +		propval = kmalloc(rprop->length, GFP_KERNEL);
> +		if (propval == NULL) {
> +			pr_err("%s: Could not copy value of '%s'\n",
> +					__func__, rprop->name);
> +			err = -ENOMEM;
> +			goto err_fail;
> +		}
> +
> +		memcpy(propval, rprop->value, rprop->length);
> +
> +		propend = propval + rprop->length;
> +		for (propcur = propval; propcur < propend;
> +				propcur += propcurlen + 1) {
> +			propcurlen = strlen(propcur);
> +
> +			nodestr = propcur;
> +			s = strchr(propcur, ':');
> +			if (s == NULL) {
> +				pr_err("%s: Illegal symbol "
> +					"entry '%s' (1)\n",
> +					__func__, (char *)rprop->value);
> +				kfree(propval);
> +				err = -EINVAL;
> +				goto err_fail;
> +			}
> +			*s++ = '\0';
> +
> +			propstr = s;
> +			s = strchr(s, ':');
> +			if (s == NULL) {
> +				pr_err("%s: Illegal symbol "
> +					"entry '%s' (2)\n",
> +					__func__, (char *)rprop->value);
> +				kfree(propval);
> +				err = -EINVAL;
> +				goto err_fail;
> +			}
> +
> +			*s++ = '\0';
> +			offset = simple_strtoul(s, NULL, 10);
> +
> +			/* look into the resolve node for the full path */
> +			refnode = __of_find_node_by_full_name(resolve,
> +					nodestr);

Re-using the 'refnode' variable here is pretty confusing, since it
means very different things earlier and here (node pointed to, versus
node containing the property which points).

> +			if (refnode == NULL) {
> +				pr_err("%s: Could not find refnode '%s'\n",
> +					__func__, (char *)rprop->value);
> +				kfree(propval);
> +				err = -ENOENT;
> +				goto err_fail;
> +			}
> +
> +			/* now find the property */
> +			for_each_property_of_node(refnode, sprop) {
> +				if (of_prop_cmp(sprop->name, propstr) == 0)
> +					break;
> +			}
> +
> +			if (sprop == NULL) {
> +				pr_err("%s: Could not find property '%s'\n",
> +					__func__, (char *)rprop->value);
> +				kfree(propval);
> +				err = -ENOENT;
> +				goto err_fail;
> +			}
> +
> +			*(uint32_t *)(sprop->value + offset) =
> +				cpu_to_be32(phandle);
> +		}
> +
> +		kfree(propval);
> +	}
> +
> +merge_sym:
> +
> +	of_node_put(root_sym);
> +
> +	return 0;
> +
> +err_fail:
> +
> +	if (root_sym != NULL)
> +		of_node_put(root_sym);
> +
> +	return err;
> +}
> diff --git a/include/linux/of.h b/include/linux/of.h
> index c38e41a..ab52243 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -650,4 +650,21 @@ static inline int of_multi_prop_cmp(const struct property *prop, const char *val
>  
>  #endif	/* !CONFIG_OF */
>  
> +
> +/* illegal phandle value (set when unresolved) */
> +#define OF_PHANDLE_ILLEGAL	0xdeadbeef

Ugh.  0 and -1 are already reserved as illegal phandle values, don't
invent a new one.

> +
> +#ifdef CONFIG_OF_RESOLVE
> +
> +int of_resolve(struct device_node *resolve);
> +
> +#else
> +
> +static inline int of_resolve(struct device_node *resolve)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +#endif
> +
>  #endif /* _LINUX_OF_H */

-- 
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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

Hi David

On Jan 21, 2013, at 6:48 AM, David Gibson wrote:

> On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou wrote:
>> Introduce support for dynamic device tree resolution.
>> Using it, it is possible to prepare a device tree that's
>> been loaded on runtime to be modified and inserted at the kernel
>> live tree.
>> 
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>> ---
>> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>> drivers/of/Kconfig                                 |   9 +
>> drivers/of/Makefile                                |   1 +
>> drivers/of/resolver.c                              | 394 +++++++++++++++++++++
>> include/linux/of.h                                 |  17 +
>> 5 files changed, 446 insertions(+)
>> create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
>> create mode 100644 drivers/of/resolver.c
>> 
>> diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt b/Documentation/devicetree/dynamic-resolution-notes.txt
>> new file mode 100644
>> index 0000000..0b396c4
>> --- /dev/null
>> +++ b/Documentation/devicetree/dynamic-resolution-notes.txt
>> @@ -0,0 +1,25 @@
>> +Device Tree Dynamic Resolver Notes
>> +----------------------------------
>> +
>> +This document describes the implementation of the in-kernel
>> +Device Tree resolver, residing in drivers/of/resolver.c and is a
>> +companion document to Documentation/devicetree/dt-object-internal.txt[1]
>> +
>> +How the resolver works
>> +----------------------
>> +
>> +The resolver is given as an input an arbitrary tree compiled with the
>> +proper dtc option and having a /plugin/ tag. This generates the
>> +appropriate __fixups__ & __local_fixups__ nodes as described in [1].
>> +
>> +In sequence the resolver works by the following steps:
>> +
>> +1. Get the maximum device tree phandle value from the live tree + 1.
>> +2. Adjust all the local phandles of the tree to resolve by that amount.
>> +3. Using the __local__fixups__ node information adjust all local references
>> +   by the same amount.
>> +4. For each property in the __fixups__ node locate the node it references
>> +   in the live tree. This is the label used to tag the node.
>> +5. Retrieve the phandle of the target of the fixup.
>> +5. For each fixup in the property locate the node:property:offset location
>> +   and replace it with the phandle value.
> 
> Hrm.  So, I'm really still not convinced by this approach.
> 
> 	First, I think it's unwise to allow overlays to change
> essentially anything in the base tree, rather than having the base
> tree define sockets of some sort where things can be attached.
> 

One could say that the labels define the sockets. It's not just things
to be attached, properties might have to change, or something more complex,
as we've found out in practice.

As far as the unwise part, a good deal of care has been taken so that 
people that don't use the overlay functionality have absolutely no
overhead, or anything modified in the way they use DT.

> 	Second, even allowing overlays to change anything, I don't see
> a lot of reason to do this kind of resolution within the kernel and
> with data stored in the dtb itself, rather than doing the resolution
> in userspace from an annotated overlay dts or dtb, then inserting the
> fully resolved product into the kernel.  In either case, the overlay
> needs to be constructed with pretty intimate knowledge of the base
> tree.
> 

Fair enough, but that's one more thing of user-space crud to drag along, which
will get enabled pretty late in the boot sequence. Meaning a whole bunch of devices,
like consoles, and root filesystems on the devices that need an overlay to operate
won't work easily enough.

> That said, I have some implementation comments below.
> 
> [snip]
>> +/**
>> + * Find a subtree's maximum phandle value.
>> + */
>> +static phandle __of_get_tree_max_phandle(struct device_node *node,
>> +		phandle max_phandle)
>> +{
>> +	struct device_node *child;
>> +
>> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
>> +			node->phandle > max_phandle)
>> +		max_phandle = node->phandle;
>> +
>> +	__for_each_child_of_node(node, child)
>> +		max_phandle = __of_get_tree_max_phandle(child, max_phandle);
> 
> Recursion is best avoided given the kernel's limited stack space.
> This is also trivial to implement non-recursively, using the allnext
> pointer.
> 

The caller passes a tree that's not yet been inserted in the live tree.
So there's no allnodes pointer yet. Care has been taken for the function
to not have excessive local variables. I would guess about 20-32 bytes for
the stack frame + the local variables, so with a 4K stack we would overflow at a 
nest level of 128, which has a pretty slim chance for a real system.
  

>> +
>> +	return max_phandle;
>> +}
>> +
>> +/**
>> + * Find live tree's maximum phandle value.
>> + */
>> +static phandle of_get_tree_max_phandle(void)
>> +{
>> +	struct device_node *node;
>> +	phandle phandle;
>> +
>> +	/* get root node */
>> +	node = of_find_node_by_path("/");
>> +	if (node == NULL)
>> +		return OF_PHANDLE_ILLEGAL;
>> +
>> +	/* now search recursively */
>> +	read_lock(&devtree_lock);
>> +	phandle = __of_get_tree_max_phandle(node, 0);
>> +	read_unlock(&devtree_lock);
>> +
>> +	of_node_put(node);
>> +
>> +	return phandle;
>> +}
>> +
>> +/**
>> + * Adjust a subtree's phandle values by a given delta.
>> + * Makes sure not to just adjust the device node's phandle value,
>> + * but modify the phandle properties values as well.
>> + */
>> +static void __of_adjust_tree_phandles(struct device_node *node,
>> +		int phandle_delta)
>> +{
>> +	struct device_node *child;
>> +	struct property *prop;
>> +	phandle phandle;
>> +
>> +	/* first adjust the node's phandle direct value */
>> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
>> +		node->phandle += phandle_delta;
> 
> You need to have some kind of check for overflow here, or the adjusted
> phandle could be one of the illegal values (0 or -1) - or wrap around
> and colllide with existing phandle values in the base tree.  dtc
> (currently) allocates phandles from the bottom, but there's no
> guarantee that a base tree will only have low phandle values - it only
> takes one node with phandle set to 0xfffffffe in the base tree to have
> this function make a mess of things.

Correct, I'll take care of handling the overflow.

> 
> 
>> +	/* now adjust phandle & linux,phandle values */
>> +	for_each_property_of_node(node, prop) {
>> +
>> +		/* only look for these two */
>> +		if (of_prop_cmp(prop->name, "phandle") != 0 &&
>> +		    of_prop_cmp(prop->name, "linux,phandle") != 0)
>> +			continue;
>> +
>> +		/* must be big enough */
>> +		if (prop->length < 4)
>> +			continue;
> 
> If prop->length != 4 (including > 4) something is pretty wrong, and
> you should probably bail with an error message.

OK, just playing it safe here.

> 
>> +
>> +		/* read phandle value */
>> +		phandle = be32_to_cpu(*(uint32_t *)prop->value);
>> +		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
>> +			continue;
>> +
>> +		/* adjust */
>> +		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
>> +	}
>> +
>> +	/* now do the children recursively */
>> +	__for_each_child_of_node(node, child)
>> +		__of_adjust_tree_phandles(child, phandle_delta);
> 
> Again, recursion is not a good idea.
> 

No other way to handle it. This is not a node that's in the live tree yet.

>> +}
>> +
>> +/**
>> + * Adjust the local phandle references by the given phandle delta.
>> + * Assumes the existances of a __local_fixups__ node at the root
>> + * of the tree. Does not take any devtree locks so make sure you
>> + * call this on a tree which is at the detached state.
>> + */
>> +static int __of_adjust_tree_phandle_references(struct device_node *node,
>> +		int phandle_delta)
>> +{
>> +	phandle phandle;
>> +	struct device_node *refnode, *child;
>> +	struct property *rprop, *sprop;
>> +	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
>> +	int offset, propcurlen;
>> +	int err;
>> +
>> +	/* locate the symbols & fixups nodes on resolve */
>> +	__for_each_child_of_node(node, child)
>> +		if (of_node_cmp(child->name, "__local_fixups__") == 0)
>> +			break;
>> +
>> +	/* no local fixups */
>> +	if (child == NULL)
>> +		return 0;
>> +
>> +	/* find the local fixups property */
>> +	for_each_property_of_node(child, rprop) {
>> +
>> +		/* skip properties added automatically */
>> +		if (of_prop_cmp(rprop->name, "name") == 0)
>> +			continue;
> 
> Ok, so you're interpreting any property except name in the
> __local_fixups__ node in exactly the same way?  That's a bit strange.
> Why not just have a single property rather than a node's worth in that
> case.

It saves space. For example you might have to resolve a label reference
more than once. So instead of doing:

	label = "/foo:bar:0";
	label = "/bar:foo:4";

You can do this:

	label = "/foo:bar:0", "/bar/foo:4";
> 
>> +		/* make a copy */
>> +		propval = kmalloc(rprop->length, GFP_KERNEL);
>> +		if (propval == NULL) {
>> +			pr_err("%s: Could not copy value of '%s'\n",
>> +					__func__, rprop->name);
>> +			return -ENOMEM;
>> +		}
>> +		memcpy(propval, rprop->value, rprop->length);
>> +
>> +		propend = propval + rprop->length;
>> +		for (propcur = propval; propcur < propend;
>> +				propcur += propcurlen + 1) {
>> +
>> +			propcurlen = strlen(propcur);
>> +
>> +			nodestr = propcur;
>> +			s = strchr(propcur, ':');
> 
> So, using strings with separators like this doesn't sit will with
> existing device tree practice.  More similar to existing things would
> have NUL separators and the integer values in binary, rather than
> text (and yes, there is precedent for mixed string and integer content
> in properties).
> 

Hmm, I guess it can be done, but I wouldn't expect any space savings.
Most offsets are very small integer multiple of 4 since the usual case is 

	foo =<&label>;

And you lose the ability to dump a string and figure out what's going on.

Not a big problem to change.

> 
>> +			if (s == NULL) {
>> +				pr_err("%s: Illegal symbol entry '%s' (1)\n",
>> +					__func__, propcur);
>> +				err = -EINVAL;
>> +				goto err_fail;
>> +			}
>> +			*s++ = '\0';
> 
> And using the separators you do leads to this rather ugly copy then
> mangle-in-place approach to parsing.
> 
>> +			propstr = s;
>> +			s = strchr(s, ':');
>> +			if (s == NULL) {
>> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail;
>> +			}
>> +
>> +			*s++ = '\0';
>> +			offset = simple_strtoul(s, NULL, 10);
>> +
>> +			/* look into the resolve node for the full path */
>> +			refnode = __of_find_node_by_full_name(node, nodestr);
>> +			if (refnode == NULL) {
>> +				pr_warn("%s: Could not find refnode '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				continue;
>> +			}
>> +
>> +			/* now find the property */
>> +			for_each_property_of_node(refnode, sprop) {
>> +				if (of_prop_cmp(sprop->name, propstr) == 0)
>> +					break;
>> +			}
>> +
>> +			if (sprop == NULL) {
>> +				pr_err("%s: Could not find property '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail;
>> +			}
>> +
>> +			phandle = be32_to_cpu(*(uint32_t *)
>> +					(sprop->value + offset));
>> +			*(uint32_t *)(sprop->value + offset) =
>> +				cpu_to_be32(phandle + phandle_delta);
>> +		}
>> +
>> +		kfree(propval);
>> +	}
>> +
>> +	return 0;
>> +
>> +err_fail:
>> +	kfree(propval);
>> +	return err;
>> +}
>> +
>> +/**
>> + * of_resolve	- Resolve the given node against the live tree.
>> + *
>> + * @resolve:	Node to resolve
>> + *
>> + * Perform dynamic Device Tree resolution against the live tree
>> + * to the given node to resolve. This depends on the live tree
>> + * having a __symbols__ node, and the resolve node the __fixups__ &
>> + * __local_fixups__ nodes (if needed).
>> + * The result of the operation is a resolve node that it's contents
>> + * are fit to be inserted or operate upon the live tree.
>> + * Returns 0 on success or a negative error value on error.
>> + */
>> +int of_resolve(struct device_node *resolve)
>> +{
>> +	struct device_node *child, *refnode;
>> +	struct device_node *root_sym, *resolve_sym, *resolve_fix;
>> +	struct property *rprop, *sprop;
>> +	const char *refpath;
>> +	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
>> +	int offset, propcurlen;
>> +	phandle phandle, phandle_delta;
>> +	int err;
>> +
>> +	/* the resolve node must exist, and be detached */
>> +	if (resolve == NULL ||
>> +			!of_node_check_flag(resolve, OF_DETACHED)) {
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* first we need to adjust the phandles */
>> +	phandle_delta = of_get_tree_max_phandle() + 1;
>> +	__of_adjust_tree_phandles(resolve, phandle_delta);
>> +	err = __of_adjust_tree_phandle_references(resolve, phandle_delta);
>> +	if (err != 0)
>> +		return err;
>> +
>> +	root_sym = NULL;
>> +	resolve_sym = NULL;
>> +	resolve_fix = NULL;
>> +
>> +	/* this may fail (if no fixups are required) */
>> +	root_sym = of_find_node_by_path("/__symbols__");
>> +
>> +	/* locate the symbols & fixups nodes on resolve */
>> +	__for_each_child_of_node(resolve, child) {
>> +
>> +		if (resolve_sym == NULL &&
> 
> No need for the NULL check.  If there are duplicate node names, you've
> already got big trouble, and picking the last matching will do no
> worse than picking the first matching.

OK

> 
>> +				of_node_cmp(child->name, "__symbols__") == 0)
>> +			resolve_sym = child;
>> +
>> +		if (resolve_fix == NULL &&
>> +				of_node_cmp(child->name, "__fixups__") == 0)
>> +			resolve_fix = child;
>> +
>> +		/* both found, don't bother anymore */
>> +		if (resolve_sym != NULL && resolve_fix != NULL)
>> +			break;
>> +	}
>> +
>> +	/* we do allow for the case where no fixups are needed */
>> +	if (resolve_fix == NULL)
>> +		goto merge_sym;
> 
> Hrm.  I'm not convinced that's one of the kernel-allowed forms of
> goto.
> 

OK

>> +
>> +	/* we need to fixup, but no root symbols... */
>> +	if (root_sym == NULL)
>> +		return -EINVAL;
>> +
>> +	for_each_property_of_node(resolve_fix, rprop) {
>> +
>> +		/* skip properties added automatically */
>> +		if (of_prop_cmp(rprop->name, "name") == 0)
>> +			continue;
>> +
>> +		err = of_property_read_string(root_sym,
>> +				rprop->name, &refpath);
>> +		if (err != 0) {
>> +			pr_err("%s: Could not find symbol '%s'\n",
>> +					__func__, rprop->name);
>> +			goto err_fail;
>> +		}
>> +
>> +		refnode = of_find_node_by_path(refpath);
>> +		if (refnode == NULL) {
>> +			pr_err("%s: Could not find node by path '%s'\n",
>> +					__func__, refpath);
>> +			err = -ENOENT;
>> +			goto err_fail;
>> +		}
>> +
>> +		phandle = refnode->phandle;
>> +		of_node_put(refnode);
>> +
>> +		pr_debug("%s: %s phandle is 0x%08x\n",
>> +				__func__, rprop->name, phandle);
>> +
>> +		/* make a copy */
>> +		propval = kmalloc(rprop->length, GFP_KERNEL);
>> +		if (propval == NULL) {
>> +			pr_err("%s: Could not copy value of '%s'\n",
>> +					__func__, rprop->name);
>> +			err = -ENOMEM;
>> +			goto err_fail;
>> +		}
>> +
>> +		memcpy(propval, rprop->value, rprop->length);
>> +
>> +		propend = propval + rprop->length;
>> +		for (propcur = propval; propcur < propend;
>> +				propcur += propcurlen + 1) {
>> +			propcurlen = strlen(propcur);
>> +
>> +			nodestr = propcur;
>> +			s = strchr(propcur, ':');
>> +			if (s == NULL) {
>> +				pr_err("%s: Illegal symbol "
>> +					"entry '%s' (1)\n",
>> +					__func__, (char *)rprop->value);
>> +				kfree(propval);
>> +				err = -EINVAL;
>> +				goto err_fail;
>> +			}
>> +			*s++ = '\0';
>> +
>> +			propstr = s;
>> +			s = strchr(s, ':');
>> +			if (s == NULL) {
>> +				pr_err("%s: Illegal symbol "
>> +					"entry '%s' (2)\n",
>> +					__func__, (char *)rprop->value);
>> +				kfree(propval);
>> +				err = -EINVAL;
>> +				goto err_fail;
>> +			}
>> +
>> +			*s++ = '\0';
>> +			offset = simple_strtoul(s, NULL, 10);
>> +
>> +			/* look into the resolve node for the full path */
>> +			refnode = __of_find_node_by_full_name(resolve,
>> +					nodestr);
> 
> Re-using the 'refnode' variable here is pretty confusing, since it
> means very different things earlier and here (node pointed to, versus
> node containing the property which points).
> 

OK

>> +			if (refnode == NULL) {
>> +				pr_err("%s: Could not find refnode '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				kfree(propval);
>> +				err = -ENOENT;
>> +				goto err_fail;
>> +			}
>> +
>> +			/* now find the property */
>> +			for_each_property_of_node(refnode, sprop) {
>> +				if (of_prop_cmp(sprop->name, propstr) == 0)
>> +					break;
>> +			}
>> +
>> +			if (sprop == NULL) {
>> +				pr_err("%s: Could not find property '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				kfree(propval);
>> +				err = -ENOENT;
>> +				goto err_fail;
>> +			}
>> +
>> +			*(uint32_t *)(sprop->value + offset) =
>> +				cpu_to_be32(phandle);
>> +		}
>> +
>> +		kfree(propval);
>> +	}
>> +
>> +merge_sym:
>> +
>> +	of_node_put(root_sym);
>> +
>> +	return 0;
>> +
>> +err_fail:
>> +
>> +	if (root_sym != NULL)
>> +		of_node_put(root_sym);
>> +
>> +	return err;
>> +}
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index c38e41a..ab52243 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -650,4 +650,21 @@ static inline int of_multi_prop_cmp(const struct property *prop, const char *val
>> 
>> #endif	/* !CONFIG_OF */
>> 
>> +
>> +/* illegal phandle value (set when unresolved) */
>> +#define OF_PHANDLE_ILLEGAL	0xdeadbeef
> 
> Ugh.  0 and -1 are already reserved as illegal phandle values, don't
> invent a new one.

OK

> 
>> +
>> +#ifdef CONFIG_OF_RESOLVE
>> +
>> +int of_resolve(struct device_node *resolve);
>> +
>> +#else
>> +
>> +static inline int of_resolve(struct device_node *resolve)
>> +{
>> +	return -ENOTSUPP;
>> +}
>> +
>> +#endif
>> +
>> #endif /* _LINUX_OF_H */
> 
> -- 
> 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: [PATCH 5/6] OF: Introduce Device Tree resolve support.

[David Gibson]

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

On Mon, Jan 21, 2013 at 12:59:15PM +0200, Pantelis Antoniou wrote:
> Hi David
> 
> On Jan 21, 2013, at 6:48 AM, David Gibson wrote:
> 
> > On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou wrote:
> >> Introduce support for dynamic device tree resolution.
> >> Using it, it is possible to prepare a device tree that's
> >> been loaded on runtime to be modified and inserted at the kernel
> >> live tree.
> >> 
> >> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> >> ---
> >> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
> >> drivers/of/Kconfig                                 |   9 +
> >> drivers/of/Makefile                                |   1 +
> >> drivers/of/resolver.c                              | 394 +++++++++++++++++++++
> >> include/linux/of.h                                 |  17 +
> >> 5 files changed, 446 insertions(+)
> >> create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
> >> create mode 100644 drivers/of/resolver.c
> >> 
> >> diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt b/Documentation/devicetree/dynamic-resolution-notes.txt
> >> new file mode 100644
> >> index 0000000..0b396c4
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/dynamic-resolution-notes.txt
> >> @@ -0,0 +1,25 @@
> >> +Device Tree Dynamic Resolver Notes
> >> +----------------------------------
> >> +
> >> +This document describes the implementation of the in-kernel
> >> +Device Tree resolver, residing in drivers/of/resolver.c and is a
> >> +companion document to Documentation/devicetree/dt-object-internal.txt[1]
> >> +
> >> +How the resolver works
> >> +----------------------
> >> +
> >> +The resolver is given as an input an arbitrary tree compiled with the
> >> +proper dtc option and having a /plugin/ tag. This generates the
> >> +appropriate __fixups__ & __local_fixups__ nodes as described in [1].
> >> +
> >> +In sequence the resolver works by the following steps:
> >> +
> >> +1. Get the maximum device tree phandle value from the live tree + 1.
> >> +2. Adjust all the local phandles of the tree to resolve by that amount.
> >> +3. Using the __local__fixups__ node information adjust all local references
> >> +   by the same amount.
> >> +4. For each property in the __fixups__ node locate the node it references
> >> +   in the live tree. This is the label used to tag the node.
> >> +5. Retrieve the phandle of the target of the fixup.
> >> +5. For each fixup in the property locate the node:property:offset location
> >> +   and replace it with the phandle value.
> > 
> > Hrm.  So, I'm really still not convinced by this approach.
> > 
> > 	First, I think it's unwise to allow overlays to change
> > essentially anything in the base tree, rather than having the base
> > tree define sockets of some sort where things can be attached.
> > 
> 
> One could say that the labels define the sockets. It's not just things
> to be attached, properties might have to change, or something more complex,
> as we've found out in practice.

Hrm.  I know a number of these have come up previously in that big
thread, but can you summarise some of these cases here.  If things
need modification in the base tree that still seems to me like the
base tree hasn't properly described the socket arrangement (I realise
that allowing such descriptions may require extensions to some of our
device tree conventions).

> As far as the unwise part, a good deal of care has been taken so that 
> people that don't use the overlay functionality have absolutely no
> overhead, or anything modified in the way they use DT.

Yeah, that's not what I'm concerned about.  I'm concerned about hard
to debug problems because some subtle change in the base tree or the
overlay or both causes the overlay to alter something in the base tree
it really shouldn't.

> > 	Second, even allowing overlays to change anything, I don't see
> > a lot of reason to do this kind of resolution within the kernel and
> > with data stored in the dtb itself, rather than doing the resolution
> > in userspace from an annotated overlay dts or dtb, then inserting the
> > fully resolved product into the kernel.  In either case, the overlay
> > needs to be constructed with pretty intimate knowledge of the base
> > tree.
> 
> Fair enough, but that's one more thing of user-space crud to drag along, which
> will get enabled pretty late in the boot sequence. Meaning a whole bunch of devices,
> like consoles, and root filesystems on the devices that need an overlay to operate
> won't work easily enough.

Hrm.  But doesn't your scheme already require userspace to identify
the hardware and load the overlay?  So why is having it resolve the
overlay significantly harder?

AFAICT devices wanted early can be handled in several possible ways
without having the resolved in kernel: an initramfs is the most
obvious, but for things you want really early, it should be possible
to do the resolution from the platform's bootloader update tool - so
the pre-resolved overlay gets bundled with the kernel/initrd/whatever
to get fired up from the bootloader.

> 
> > That said, I have some implementation comments below.
> > 
> > [snip]
> >> +/**
> >> + * Find a subtree's maximum phandle value.
> >> + */
> >> +static phandle __of_get_tree_max_phandle(struct device_node *node,
> >> +		phandle max_phandle)
> >> +{
> >> +	struct device_node *child;
> >> +
> >> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
> >> +			node->phandle > max_phandle)
> >> +		max_phandle = node->phandle;
> >> +
> >> +	__for_each_child_of_node(node, child)
> >> +		max_phandle = __of_get_tree_max_phandle(child, max_phandle);
> > 
> > Recursion is best avoided given the kernel's limited stack space.
> > This is also trivial to implement non-recursively, using the allnext
> > pointer.
> 
> The caller passes a tree that's not yet been inserted in the live
> tree.

Um.. isn't this used to find the max phandle in the base tree, so how
is it not inserted in the live tree yet?

> So there's no allnodes pointer yet. 

I would strongly suggest populating that in the subtree as you build
it, then.  Except that I don't think it is a detached subtree in this
case, though it is in some of the cases below.

> Care has been taken for the function
> to not have excessive local variables. I would guess about 20-32 bytes for
> the stack frame + the local variables, so with a 4K stack we would overflow at a 
> nest level of 128, which has a pretty slim chance for a real system.

Hrm.  Recursion still makes me nervous.  I believe there are platforms
that have a non-trivial lower-bound on stack usage per frame, even
with few local variables.

> >> +
> >> +	return max_phandle;
> >> +}
> >> +
> >> +/**
> >> + * Find live tree's maximum phandle value.
> >> + */
> >> +static phandle of_get_tree_max_phandle(void)
> >> +{
> >> +	struct device_node *node;
> >> +	phandle phandle;
> >> +
> >> +	/* get root node */
> >> +	node = of_find_node_by_path("/");
> >> +	if (node == NULL)
> >> +		return OF_PHANDLE_ILLEGAL;
> >> +
> >> +	/* now search recursively */
> >> +	read_lock(&devtree_lock);
> >> +	phandle = __of_get_tree_max_phandle(node, 0);
> >> +	read_unlock(&devtree_lock);
> >> +
> >> +	of_node_put(node);
> >> +
> >> +	return phandle;
> >> +}
> >> +
> >> +/**
> >> + * Adjust a subtree's phandle values by a given delta.
> >> + * Makes sure not to just adjust the device node's phandle value,
> >> + * but modify the phandle properties values as well.
> >> + */
> >> +static void __of_adjust_tree_phandles(struct device_node *node,
> >> +		int phandle_delta)
> >> +{
> >> +	struct device_node *child;
> >> +	struct property *prop;
> >> +	phandle phandle;
> >> +
> >> +	/* first adjust the node's phandle direct value */
> >> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
> >> +		node->phandle += phandle_delta;
> > 
> > You need to have some kind of check for overflow here, or the adjusted
> > phandle could be one of the illegal values (0 or -1) - or wrap around
> > and colllide with existing phandle values in the base tree.  dtc
> > (currently) allocates phandles from the bottom, but there's no
> > guarantee that a base tree will only have low phandle values - it only
> > takes one node with phandle set to 0xfffffffe in the base tree to have
> > this function make a mess of things.
> 
> Correct, I'll take care of handling the overflow.
> 
> > 
> > 
> >> +	/* now adjust phandle & linux,phandle values */
> >> +	for_each_property_of_node(node, prop) {
> >> +
> >> +		/* only look for these two */
> >> +		if (of_prop_cmp(prop->name, "phandle") != 0 &&
> >> +		    of_prop_cmp(prop->name, "linux,phandle") != 0)
> >> +			continue;
> >> +
> >> +		/* must be big enough */
> >> +		if (prop->length < 4)
> >> +			continue;
> > 
> > If prop->length != 4 (including > 4) something is pretty wrong, and
> > you should probably bail with an error message.
> 
> OK, just playing it safe here.
> 
> > 
> >> +
> >> +		/* read phandle value */
> >> +		phandle = be32_to_cpu(*(uint32_t *)prop->value);
> >> +		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
> >> +			continue;
> >> +
> >> +		/* adjust */
> >> +		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
> >> +	}
> >> +
> >> +	/* now do the children recursively */
> >> +	__for_each_child_of_node(node, child)
> >> +		__of_adjust_tree_phandles(child, phandle_delta);
> > 
> > Again, recursion is not a good idea.
> > 
> 
> No other way to handle it. This is not a node that's in the live
> tree yet.

There's always another way to handle it, although it may be less
elegant.  In this case it should be easy though - populate the allnext
pointers when you unflatten the tree.  Or, have the offset applied
when you actually apply the overlay rather than as a separate pass.
 
> >> +}
> >> +
> >> +/**
> >> + * Adjust the local phandle references by the given phandle delta.
> >> + * Assumes the existances of a __local_fixups__ node at the root
> >> + * of the tree. Does not take any devtree locks so make sure you
> >> + * call this on a tree which is at the detached state.
> >> + */
> >> +static int __of_adjust_tree_phandle_references(struct device_node *node,
> >> +		int phandle_delta)
> >> +{
> >> +	phandle phandle;
> >> +	struct device_node *refnode, *child;
> >> +	struct property *rprop, *sprop;
> >> +	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
> >> +	int offset, propcurlen;
> >> +	int err;
> >> +
> >> +	/* locate the symbols & fixups nodes on resolve */
> >> +	__for_each_child_of_node(node, child)
> >> +		if (of_node_cmp(child->name, "__local_fixups__") == 0)
> >> +			break;
> >> +
> >> +	/* no local fixups */
> >> +	if (child == NULL)
> >> +		return 0;
> >> +
> >> +	/* find the local fixups property */
> >> +	for_each_property_of_node(child, rprop) {
> >> +
> >> +		/* skip properties added automatically */
> >> +		if (of_prop_cmp(rprop->name, "name") == 0)
> >> +			continue;
> > 
> > Ok, so you're interpreting any property except name in the
> > __local_fixups__ node in exactly the same way?  That's a bit strange.
> > Why not just have a single property rather than a node's worth in that
> > case.
> 
> It saves space. For example you might have to resolve a label reference
> more than once. So instead of doing:
> 
> 	label = "/foo:bar:0";
> 	label = "/bar:foo:4";
> 
> You can do this:
> 
> 	label = "/foo:bar:0", "/bar/foo:4";

Um.. you seem to have read me as saying the exact opposite of what I
said.  I'm _suggesting_ that you use:
	__local_fixups__ = "/foo:bar:0", "/bar/foo:4";
Since the 'label' name has no meaning in the case of local fixups.

> >> +		/* make a copy */
> >> +		propval = kmalloc(rprop->length, GFP_KERNEL);
> >> +		if (propval == NULL) {
> >> +			pr_err("%s: Could not copy value of '%s'\n",
> >> +					__func__, rprop->name);
> >> +			return -ENOMEM;
> >> +		}
> >> +		memcpy(propval, rprop->value, rprop->length);
> >> +
> >> +		propend = propval + rprop->length;
> >> +		for (propcur = propval; propcur < propend;
> >> +				propcur += propcurlen + 1) {
> >> +
> >> +			propcurlen = strlen(propcur);
> >> +
> >> +			nodestr = propcur;
> >> +			s = strchr(propcur, ':');
> > 
> > So, using strings with separators like this doesn't sit will with
> > existing device tree practice.  More similar to existing things would
> > have NUL separators and the integer values in binary, rather than
> > text (and yes, there is precedent for mixed string and integer content
> > in properties).
> > 
> 
> Hmm, I guess it can be done, but I wouldn't expect any space savings.

I'm not after space savings, just least-surprise by matching existing
common practices.

-- 
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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

Hi

On Jan 22, 2013, at 6:05 AM, David Gibson wrote:

> On Mon, Jan 21, 2013 at 12:59:15PM +0200, Pantelis Antoniou wrote:
>> Hi David
>> 
>> On Jan 21, 2013, at 6:48 AM, David Gibson wrote:
>> 
>>> On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou wrote:
>>>> Introduce support for dynamic device tree resolution.
>>>> Using it, it is possible to prepare a device tree that's
>>>> been loaded on runtime to be modified and inserted at the kernel
>>>> live tree.
>>>> 
>>>> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
>>>> ---
>>>> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>>>> drivers/of/Kconfig                                 |   9 +
>>>> drivers/of/Makefile                                |   1 +
>>>> drivers/of/resolver.c                              | 394 +++++++++++++++++++++
>>>> include/linux/of.h                                 |  17 +
>>>> 5 files changed, 446 insertions(+)
>>>> create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
>>>> create mode 100644 drivers/of/resolver.c
>>>> 
>>>> diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt b/Documentation/devicetree/dynamic-resolution-notes.txt
>>>> new file mode 100644
>>>> index 0000000..0b396c4
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/dynamic-resolution-notes.txt
>>>> @@ -0,0 +1,25 @@
>>>> +Device Tree Dynamic Resolver Notes
>>>> +----------------------------------
>>>> +
>>>> +This document describes the implementation of the in-kernel
>>>> +Device Tree resolver, residing in drivers/of/resolver.c and is a
>>>> +companion document to Documentation/devicetree/dt-object-internal.txt[1]
>>>> +
>>>> +How the resolver works
>>>> +----------------------
>>>> +
>>>> +The resolver is given as an input an arbitrary tree compiled with the
>>>> +proper dtc option and having a /plugin/ tag. This generates the
>>>> +appropriate __fixups__ & __local_fixups__ nodes as described in [1].
>>>> +
>>>> +In sequence the resolver works by the following steps:
>>>> +
>>>> +1. Get the maximum device tree phandle value from the live tree + 1.
>>>> +2. Adjust all the local phandles of the tree to resolve by that amount.
>>>> +3. Using the __local__fixups__ node information adjust all local references
>>>> +   by the same amount.
>>>> +4. For each property in the __fixups__ node locate the node it references
>>>> +   in the live tree. This is the label used to tag the node.
>>>> +5. Retrieve the phandle of the target of the fixup.
>>>> +5. For each fixup in the property locate the node:property:offset location
>>>> +   and replace it with the phandle value.
>>> 
>>> Hrm.  So, I'm really still not convinced by this approach.
>>> 
>>> 	First, I think it's unwise to allow overlays to change
>>> essentially anything in the base tree, rather than having the base
>>> tree define sockets of some sort where things can be attached.
>>> 
>> 
>> One could say that the labels define the sockets. It's not just things
>> to be attached, properties might have to change, or something more complex,
>> as we've found out in practice.
> 
> Hrm.  I know a number of these have come up previously in that big
> thread, but can you summarise some of these cases here.  If things
> need modification in the base tree that still seems to me like the
> base tree hasn't properly described the socket arrangement (I realise
> that allowing such descriptions may require extensions to some of our
> device tree conventions).
> 

It would be pointless to number all the use-cases that Grant put in that
long document, but I can summarize the cases that we've seen on the bone.

* Addition of child device nodes to the ocp node, creates new platform
devices of various kind (audio/video/pwms/timers) - almost any kind of
platform device that the SoC supports. Removing the overlay unregisters
the devices (but precious few drivers support that cleanly ATM). Since
the capes don't support hotplug that's not a big deal.

* Addition of pinctrl configuration nodes.

* Addition of i2c/spi etc device nodes and modification of the parent's
node status property to "okay", creates the bus platform device & registers
the devices on the bus. Slight complication with i2c client devices which
are not platform devices need special handling.

* Modification of configuration parameters of a disabled device and subsequent
enablement. 

>> As far as the unwise part, a good deal of care has been taken so that 
>> people that don't use the overlay functionality have absolutely no
>> overhead, or anything modified in the way they use DT.
> 
> Yeah, that's not what I'm concerned about.  I'm concerned about hard
> to debug problems because some subtle change in the base tree or the
> overlay or both causes the overlay to alter something in the base tree
> it really shouldn't.
> 

Define shouldn't. Let's say someone inserts a bogus overlay that makes the system
fail, or misbehave in some other manner. What is the different between 
using the overlay and inserting kernel module that fails in a similar manner?

As far as supporting arbitrary overlays, that is not the case, and never will
be. Now since the beaglebone is for hobbyists too, we can't restrict what
a user can do in any way; but that person is free to do whatever he wants.

>>> 	Second, even allowing overlays to change anything, I don't see
>>> a lot of reason to do this kind of resolution within the kernel and
>>> with data stored in the dtb itself, rather than doing the resolution
>>> in userspace from an annotated overlay dts or dtb, then inserting the
>>> fully resolved product into the kernel.  In either case, the overlay
>>> needs to be constructed with pretty intimate knowledge of the base
>>> tree.
>> 
>> Fair enough, but that's one more thing of user-space crud to drag along, which
>> will get enabled pretty late in the boot sequence. Meaning a whole bunch of devices,
>> like consoles, and root filesystems on the devices that need an overlay to operate
>> won't work easily enough.
> 
> Hrm.  But doesn't your scheme already require userspace to identify
> the hardware and load the overlay?  So why is having it resolve the
> overlay significantly harder?
> 

There is no user-space involved what so ever. The only place where
user-space might be involved is supplying the dtbo firmware image as a
result of the loader driver issuing request_firmware().  

> AFAICT devices wanted early can be handled in several possible ways
> without having the resolved in kernel: an initramfs is the most
> obvious, but for things you want really early, it should be possible
> to do the resolution from the platform's bootloader update tool - so
> the pre-resolved overlay gets bundled with the kernel/initrd/whatever
> to get fired up from the bootloader.
> 

No. Just no. It is a support nightmare when dealing with the hobbyist market.
Something similar has been tried before. While I don't have an exact figure, 
having users tinker with any part of the bootloader/init sequence in order to 
add some new cape lead to an unacceptable number of RMAed boards, which
had absolutely nothing wrong with them, besides the fact that the person tinkering
with it couldn't get it booting after screwing it.
This wiped off any amount of profit for the board maker; possibly even made the
whole operation a loss.

>> 
>>> That said, I have some implementation comments below.
>>> 
>>> [snip]
>>>> +/**
>>>> + * Find a subtree's maximum phandle value.
>>>> + */
>>>> +static phandle __of_get_tree_max_phandle(struct device_node *node,
>>>> +		phandle max_phandle)
>>>> +{
>>>> +	struct device_node *child;
>>>> +
>>>> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
>>>> +			node->phandle > max_phandle)
>>>> +		max_phandle = node->phandle;
>>>> +
>>>> +	__for_each_child_of_node(node, child)
>>>> +		max_phandle = __of_get_tree_max_phandle(child, max_phandle);
>>> 
>>> Recursion is best avoided given the kernel's limited stack space.
>>> This is also trivial to implement non-recursively, using the allnext
>>> pointer.
>> 
>> The caller passes a tree that's not yet been inserted in the live
>> tree.
> 
> Um.. isn't this used to find the max phandle in the base tree, so how
> is it not inserted in the live tree yet?
> 

We have a use case (for devices that must be inserted very early in the boot
process) that the overlay is stored in a part of the base tree, not in a
different dtbo file. Some phandle juggling there uses this function.
This patchset only uses it on the base tree.

>> So there's no allnodes pointer yet. 
> 
> I would strongly suggest populating that in the subtree as you build
> it, then.  Except that I don't think it is a detached subtree in this
> case, though it is in some of the cases below.
> 
>> Care has been taken for the function
>> to not have excessive local variables. I would guess about 20-32 bytes for
>> the stack frame + the local variables, so with a 4K stack we would overflow at a 
>> nest level of 128, which has a pretty slim chance for a real system.
> 
> Hrm.  Recursion still makes me nervous.  I believe there are platforms
> that have a non-trivial lower-bound on stack usage per frame, even
> with few local variables.

How about having a recursion limit argument set to something sane like 10?

> 
>>>> +
>>>> +	return max_phandle;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Find live tree's maximum phandle value.
>>>> + */
>>>> +static phandle of_get_tree_max_phandle(void)
>>>> +{
>>>> +	struct device_node *node;
>>>> +	phandle phandle;
>>>> +
>>>> +	/* get root node */
>>>> +	node = of_find_node_by_path("/");
>>>> +	if (node == NULL)
>>>> +		return OF_PHANDLE_ILLEGAL;
>>>> +
>>>> +	/* now search recursively */
>>>> +	read_lock(&devtree_lock);
>>>> +	phandle = __of_get_tree_max_phandle(node, 0);
>>>> +	read_unlock(&devtree_lock);
>>>> +
>>>> +	of_node_put(node);
>>>> +
>>>> +	return phandle;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Adjust a subtree's phandle values by a given delta.
>>>> + * Makes sure not to just adjust the device node's phandle value,
>>>> + * but modify the phandle properties values as well.
>>>> + */
>>>> +static void __of_adjust_tree_phandles(struct device_node *node,
>>>> +		int phandle_delta)
>>>> +{
>>>> +	struct device_node *child;
>>>> +	struct property *prop;
>>>> +	phandle phandle;
>>>> +
>>>> +	/* first adjust the node's phandle direct value */
>>>> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
>>>> +		node->phandle += phandle_delta;
>>> 
>>> You need to have some kind of check for overflow here, or the adjusted
>>> phandle could be one of the illegal values (0 or -1) - or wrap around
>>> and colllide with existing phandle values in the base tree.  dtc
>>> (currently) allocates phandles from the bottom, but there's no
>>> guarantee that a base tree will only have low phandle values - it only
>>> takes one node with phandle set to 0xfffffffe in the base tree to have
>>> this function make a mess of things.
>> 
>> Correct, I'll take care of handling the overflow.
>> 
>>> 
>>> 
>>>> +	/* now adjust phandle & linux,phandle values */
>>>> +	for_each_property_of_node(node, prop) {
>>>> +
>>>> +		/* only look for these two */
>>>> +		if (of_prop_cmp(prop->name, "phandle") != 0 &&
>>>> +		    of_prop_cmp(prop->name, "linux,phandle") != 0)
>>>> +			continue;
>>>> +
>>>> +		/* must be big enough */
>>>> +		if (prop->length < 4)
>>>> +			continue;
>>> 
>>> If prop->length != 4 (including > 4) something is pretty wrong, and
>>> you should probably bail with an error message.
>> 
>> OK, just playing it safe here.
>> 
>>> 
>>>> +
>>>> +		/* read phandle value */
>>>> +		phandle = be32_to_cpu(*(uint32_t *)prop->value);
>>>> +		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
>>>> +			continue;
>>>> +
>>>> +		/* adjust */
>>>> +		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
>>>> +	}
>>>> +
>>>> +	/* now do the children recursively */
>>>> +	__for_each_child_of_node(node, child)
>>>> +		__of_adjust_tree_phandles(child, phandle_delta);
>>> 
>>> Again, recursion is not a good idea.
>>> 
>> 
>> No other way to handle it. This is not a node that's in the live
>> tree yet.
> 
> There's always another way to handle it, although it may be less
> elegant.  In this case it should be easy though - populate the allnext
> pointers when you unflatten the tree.  Or, have the offset applied
> when you actually apply the overlay rather than as a separate pass.
> 

Previous comment applies here too. Recursion limit counter?

>>>> +}
>>>> +
>>>> +/**
>>>> + * Adjust the local phandle references by the given phandle delta.
>>>> + * Assumes the existances of a __local_fixups__ node at the root
>>>> + * of the tree. Does not take any devtree locks so make sure you
>>>> + * call this on a tree which is at the detached state.
>>>> + */
>>>> +static int __of_adjust_tree_phandle_references(struct device_node *node,
>>>> +		int phandle_delta)
>>>> +{
>>>> +	phandle phandle;
>>>> +	struct device_node *refnode, *child;
>>>> +	struct property *rprop, *sprop;
>>>> +	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
>>>> +	int offset, propcurlen;
>>>> +	int err;
>>>> +
>>>> +	/* locate the symbols & fixups nodes on resolve */
>>>> +	__for_each_child_of_node(node, child)
>>>> +		if (of_node_cmp(child->name, "__local_fixups__") == 0)
>>>> +			break;
>>>> +
>>>> +	/* no local fixups */
>>>> +	if (child == NULL)
>>>> +		return 0;
>>>> +
>>>> +	/* find the local fixups property */
>>>> +	for_each_property_of_node(child, rprop) {
>>>> +
>>>> +		/* skip properties added automatically */
>>>> +		if (of_prop_cmp(rprop->name, "name") == 0)
>>>> +			continue;
>>> 
>>> Ok, so you're interpreting any property except name in the
>>> __local_fixups__ node in exactly the same way?  That's a bit strange.
>>> Why not just have a single property rather than a node's worth in that
>>> case.
>> 
>> It saves space. For example you might have to resolve a label reference
>> more than once. So instead of doing:
>> 
>> 	label = "/foo:bar:0";
>> 	label = "/bar:foo:4";
>> 
>> You can do this:
>> 
>> 	label = "/foo:bar:0", "/bar/foo:4";
> 
> Um.. you seem to have read me as saying the exact opposite of what I
> said.  I'm _suggesting_ that you use:
> 	__local_fixups__ = "/foo:bar:0", "/bar/foo:4";
> Since the 'label' name has no meaning in the case of local fixups.

Err, I misread that.

At first I tried to do it the way you suggested but run into the following
problem: The DTC enforces the properties to always precede the child nodes.
I wasn't able to find a way to insert the __local_fixups__ property easily.
Perhaps you can suggest where would you put it? 

> 
>>>> +		/* make a copy */
>>>> +		propval = kmalloc(rprop->length, GFP_KERNEL);
>>>> +		if (propval == NULL) {
>>>> +			pr_err("%s: Could not copy value of '%s'\n",
>>>> +					__func__, rprop->name);
>>>> +			return -ENOMEM;
>>>> +		}
>>>> +		memcpy(propval, rprop->value, rprop->length);
>>>> +
>>>> +		propend = propval + rprop->length;
>>>> +		for (propcur = propval; propcur < propend;
>>>> +				propcur += propcurlen + 1) {
>>>> +
>>>> +			propcurlen = strlen(propcur);
>>>> +
>>>> +			nodestr = propcur;
>>>> +			s = strchr(propcur, ':');
>>> 
>>> So, using strings with separators like this doesn't sit will with
>>> existing device tree practice.  More similar to existing things would
>>> have NUL separators and the integer values in binary, rather than
>>> text (and yes, there is precedent for mixed string and integer content
>>> in properties).
>>> 
>> 
>> Hmm, I guess it can be done, but I wouldn't expect any space savings.
> 
> I'm not after space savings, just least-surprise by matching existing
> common practices.

Maybe. You also lose the ability to print the contents of the node and
easily inspect. It's your call really, I don't mind that much.

Regards

-- Pantelis

> 
> -- 
> 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: [PATCH 5/6] OF: Introduce Device Tree resolve support.

[David Gibson]

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

On Tue, Jan 22, 2013 at 01:06:09PM +0200, Pantelis Antoniou wrote:
> Hi
> 
> On Jan 22, 2013, at 6:05 AM, David Gibson wrote:
> 
> > On Mon, Jan 21, 2013 at 12:59:15PM +0200, Pantelis Antoniou wrote:
> >> Hi David
> >> 
> >> On Jan 21, 2013, at 6:48 AM, David Gibson wrote:
> >> 
> >>> On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou wrote:
> >>>> Introduce support for dynamic device tree resolution.
> >>>> Using it, it is possible to prepare a device tree that's
> >>>> been loaded on runtime to be modified and inserted at the kernel
> >>>> live tree.
> >>>> 
> >>>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> >>>> ---
> >>>> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
> >>>> drivers/of/Kconfig                                 |   9 +
> >>>> drivers/of/Makefile                                |   1 +
> >>>> drivers/of/resolver.c                              | 394 +++++++++++++++++++++
> >>>> include/linux/of.h                                 |  17 +
> >>>> 5 files changed, 446 insertions(+)
> >>>> create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
> >>>> create mode 100644 drivers/of/resolver.c
> >>>> 
> >>>> diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt b/Documentation/devicetree/dynamic-resolution-notes.txt
> >>>> new file mode 100644
> >>>> index 0000000..0b396c4
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/dynamic-resolution-notes.txt
> >>>> @@ -0,0 +1,25 @@
> >>>> +Device Tree Dynamic Resolver Notes
> >>>> +----------------------------------
> >>>> +
> >>>> +This document describes the implementation of the in-kernel
> >>>> +Device Tree resolver, residing in drivers/of/resolver.c and is a
> >>>> +companion document to Documentation/devicetree/dt-object-internal.txt[1]
> >>>> +
> >>>> +How the resolver works
> >>>> +----------------------
> >>>> +
> >>>> +The resolver is given as an input an arbitrary tree compiled with the
> >>>> +proper dtc option and having a /plugin/ tag. This generates the
> >>>> +appropriate __fixups__ & __local_fixups__ nodes as described in [1].
> >>>> +
> >>>> +In sequence the resolver works by the following steps:
> >>>> +
> >>>> +1. Get the maximum device tree phandle value from the live tree + 1.
> >>>> +2. Adjust all the local phandles of the tree to resolve by that amount.
> >>>> +3. Using the __local__fixups__ node information adjust all local references
> >>>> +   by the same amount.
> >>>> +4. For each property in the __fixups__ node locate the node it references
> >>>> +   in the live tree. This is the label used to tag the node.
> >>>> +5. Retrieve the phandle of the target of the fixup.
> >>>> +5. For each fixup in the property locate the node:property:offset location
> >>>> +   and replace it with the phandle value.
> >>> 
> >>> Hrm.  So, I'm really still not convinced by this approach.
> >>> 
> >>> 	First, I think it's unwise to allow overlays to change
> >>> essentially anything in the base tree, rather than having the base
> >>> tree define sockets of some sort where things can be attached.
> >>> 
> >> 
> >> One could say that the labels define the sockets. It's not just things
> >> to be attached, properties might have to change, or something more complex,
> >> as we've found out in practice.
> > 
> > Hrm.  I know a number of these have come up previously in that big
> > thread, but can you summarise some of these cases here.  If things
> > need modification in the base tree that still seems to me like the
> > base tree hasn't properly described the socket arrangement (I realise
> > that allowing such descriptions may require extensions to some of our
> > device tree conventions).
> > 
> 
> It would be pointless to number all the use-cases that Grant put in that
> long document, but I can summarize the cases that we've seen on the bone.
> 
> * Addition of child device nodes to the ocp node, creates new platform
> devices of various kind (audio/video/pwms/timers) - almost any kind of
> platform device that the SoC supports. Removing the overlay unregisters
> the devices (but precious few drivers support that cleanly ATM). Since
> the capes don't support hotplug that's not a big deal.

Ok, that's just adding nodes, which is straightforward.

> * Addition of pinctrl configuration nodes.

Ok, do you know where I can look to see how the pinctrl stuff works?

> * Addition of i2c/spi etc device nodes and modification of the parent's
> node status property to "okay",

Ok.  I'm assuming this is basically just to enable the bus controller
which was previously disabled because it had nothing attached to it?

> creates the bus platform device & registers
> the devices on the bus. Slight complication with i2c client devices which
> are not platform devices need special handling.

And this part is again just adding nodes.

> * Modification of configuration parameters of a disabled device and subsequent
> enablement. 

What sorts of modification are necessary to the parameters?  Other
than changing status = "disabled" of course.  This is the only case I
see here which might be changing data other than the status property,
which if it were the only one could reasonably be special cased, I
think.

> >> As far as the unwise part, a good deal of care has been taken so that 
> >> people that don't use the overlay functionality have absolutely no
> >> overhead, or anything modified in the way they use DT.
> > 
> > Yeah, that's not what I'm concerned about.  I'm concerned about hard
> > to debug problems because some subtle change in the base tree or the
> > overlay or both causes the overlay to alter something in the base tree
> > it really shouldn't.
> 
> Define shouldn't. Let's say someone inserts a bogus overlay that makes the system
> fail, or misbehave in some other manner. What is the different between 
> using the overlay and inserting kernel module that fails in a
> similar manner?

Hm, you do have a point there.

Another reason for a more constrained format did occur to me though -
it opens the possibility for multiple interchangeable sockets on the
same board.  e.g. we get the BeaglePlus or whatever which can take two
capes simultaneously.  With the current scheme an identical cape would
need different overlays for insertion in socket A and socket B because
it would need different labels for all the fixups.

> As far as supporting arbitrary overlays, that is not the case, and never will
> be. Now since the beaglebone is for hobbyists too, we can't restrict what
> a user can do in any way; but that person is free to do whatever he wants.
> 
> >>> 	Second, even allowing overlays to change anything, I don't see
> >>> a lot of reason to do this kind of resolution within the kernel and
> >>> with data stored in the dtb itself, rather than doing the resolution
> >>> in userspace from an annotated overlay dts or dtb, then inserting the
> >>> fully resolved product into the kernel.  In either case, the overlay
> >>> needs to be constructed with pretty intimate knowledge of the base
> >>> tree.
> >> 
> >> Fair enough, but that's one more thing of user-space crud to drag along, which
> >> will get enabled pretty late in the boot sequence. Meaning a whole bunch of devices,
> >> like consoles, and root filesystems on the devices that need an overlay to operate
> >> won't work easily enough.
> > 
> > Hrm.  But doesn't your scheme already require userspace to identify
> > the hardware and load the overlay?  So why is having it resolve the
> > overlay significantly harder?
> 
> There is no user-space involved what so ever. The only place where
> user-space might be involved is supplying the dtbo firmware image as a
> result of the loader driver issuing request_firmware().  

Right.. and couldn't the request_firmware() involve processing the
resolutions, rather than just grabbing the file.

> > AFAICT devices wanted early can be handled in several possible ways
> > without having the resolved in kernel: an initramfs is the most
> > obvious, but for things you want really early, it should be possible
> > to do the resolution from the platform's bootloader update tool - so
> > the pre-resolved overlay gets bundled with the kernel/initrd/whatever
> > to get fired up from the bootloader.
> 
> No. Just no. It is a support nightmare when dealing with the hobbyist market.
> Something similar has been tried before. While I don't have an exact figure, 
> having users tinker with any part of the bootloader/init sequence in order to 
> add some new cape lead to an unacceptable number of RMAed boards, which
> had absolutely nothing wrong with them, besides the fact that the person tinkering
> with it couldn't get it booting after screwing it.
> This wiped off any amount of profit for the board maker; possibly even made the
> whole operation a loss.

Ok.  I certainly don't want to dismiss usability concerns. I was
envisaging this pre-resolving process asn being part of the platform's
equivalent of running the lilo / update-grub / whatever script.  So
I'm trying to understand what's different about the platform's boot
sequence it so much more fraught in this case.

The other thing I notice is that this sort of concern is exactly why I
lean towards the idea of a more constrained "overlay" format where the
base tree desribes what a socket can add / change, rather than
allowing arbitrary alterations to the base tree.  With such
constraints, it's going to be much harder to screw up the basic
platform booting due to a bad overlay.  Obviously the cape won't work
properly without correct device information, but you won't be able to
clobber the basic device information for the board.

[snip]
> >>> That said, I have some implementation comments below.
> >>> 
> >>> [snip]
> >>>> +/**
> >>>> + * Find a subtree's maximum phandle value.
> >>>> + */
> >>>> +static phandle __of_get_tree_max_phandle(struct device_node *node,
> >>>> +		phandle max_phandle)
> >>>> +{
> >>>> +	struct device_node *child;
> >>>> +
> >>>> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
> >>>> +			node->phandle > max_phandle)
> >>>> +		max_phandle = node->phandle;
> >>>> +
> >>>> +	__for_each_child_of_node(node, child)
> >>>> +		max_phandle = __of_get_tree_max_phandle(child, max_phandle);
> >>> 
> >>> Recursion is best avoided given the kernel's limited stack space.
> >>> This is also trivial to implement non-recursively, using the allnext
> >>> pointer.
> >> 
> >> The caller passes a tree that's not yet been inserted in the live
> >> tree.
> > 
> > Um.. isn't this used to find the max phandle in the base tree, so how
> > is it not inserted in the live tree yet?
> 
> We have a use case (for devices that must be inserted very early in the boot
> process) that the overlay is stored in a part of the base tree, not in a
> different dtbo file. Some phandle juggling there uses this function.
> This patchset only uses it on the base tree.

Ok.  Nonetheless it's not hard to avoid a recursive approach here.

> >> So there's no allnodes pointer yet. 
> > 
> > I would strongly suggest populating that in the subtree as you build
> > it, then.  Except that I don't think it is a detached subtree in this
> > case, though it is in some of the cases below.
> > 
> >> Care has been taken for the function
> >> to not have excessive local variables. I would guess about 20-32 bytes for
> >> the stack frame + the local variables, so with a 4K stack we would overflow at a 
> >> nest level of 128, which has a pretty slim chance for a real system.
> > 
> > Hrm.  Recursion still makes me nervous.  I believe there are platforms
> > that have a non-trivial lower-bound on stack usage per frame, even
> > with few local variables.
> 
> How about having a recursion limit argument set to something sane
> like 10?

Ugh.  It's really best just to avoid recursion in the kernel.

> >>>> +
> >>>> +	return max_phandle;
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * Find live tree's maximum phandle value.
> >>>> + */
> >>>> +static phandle of_get_tree_max_phandle(void)
> >>>> +{
> >>>> +	struct device_node *node;
> >>>> +	phandle phandle;
> >>>> +
> >>>> +	/* get root node */
> >>>> +	node = of_find_node_by_path("/");
> >>>> +	if (node == NULL)
> >>>> +		return OF_PHANDLE_ILLEGAL;
> >>>> +
> >>>> +	/* now search recursively */
> >>>> +	read_lock(&devtree_lock);
> >>>> +	phandle = __of_get_tree_max_phandle(node, 0);
> >>>> +	read_unlock(&devtree_lock);
> >>>> +
> >>>> +	of_node_put(node);
> >>>> +
> >>>> +	return phandle;
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * Adjust a subtree's phandle values by a given delta.
> >>>> + * Makes sure not to just adjust the device node's phandle value,
> >>>> + * but modify the phandle properties values as well.
> >>>> + */
> >>>> +static void __of_adjust_tree_phandles(struct device_node *node,
> >>>> +		int phandle_delta)
> >>>> +{
> >>>> +	struct device_node *child;
> >>>> +	struct property *prop;
> >>>> +	phandle phandle;
> >>>> +
> >>>> +	/* first adjust the node's phandle direct value */
> >>>> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
> >>>> +		node->phandle += phandle_delta;
> >>> 
> >>> You need to have some kind of check for overflow here, or the adjusted
> >>> phandle could be one of the illegal values (0 or -1) - or wrap around
> >>> and colllide with existing phandle values in the base tree.  dtc
> >>> (currently) allocates phandles from the bottom, but there's no
> >>> guarantee that a base tree will only have low phandle values - it only
> >>> takes one node with phandle set to 0xfffffffe in the base tree to have
> >>> this function make a mess of things.
> >> 
> >> Correct, I'll take care of handling the overflow.
> >> 
> >>> 
> >>> 
> >>>> +	/* now adjust phandle & linux,phandle values */
> >>>> +	for_each_property_of_node(node, prop) {
> >>>> +
> >>>> +		/* only look for these two */
> >>>> +		if (of_prop_cmp(prop->name, "phandle") != 0 &&
> >>>> +		    of_prop_cmp(prop->name, "linux,phandle") != 0)
> >>>> +			continue;
> >>>> +
> >>>> +		/* must be big enough */
> >>>> +		if (prop->length < 4)
> >>>> +			continue;
> >>> 
> >>> If prop->length != 4 (including > 4) something is pretty wrong, and
> >>> you should probably bail with an error message.
> >> 
> >> OK, just playing it safe here.
> >> 
> >>> 
> >>>> +
> >>>> +		/* read phandle value */
> >>>> +		phandle = be32_to_cpu(*(uint32_t *)prop->value);
> >>>> +		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
> >>>> +			continue;
> >>>> +
> >>>> +		/* adjust */
> >>>> +		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
> >>>> +	}
> >>>> +
> >>>> +	/* now do the children recursively */
> >>>> +	__for_each_child_of_node(node, child)
> >>>> +		__of_adjust_tree_phandles(child, phandle_delta);
> >>> 
> >>> Again, recursion is not a good idea.
> >>> 
> >> 
> >> No other way to handle it. This is not a node that's in the live
> >> tree yet.
> > 
> > There's always another way to handle it, although it may be less
> > elegant.  In this case it should be easy though - populate the allnext
> > pointers when you unflatten the tree.  Or, have the offset applied
> > when you actually apply the overlay rather than as a separate pass.
> 
> Previous comment applies here too. Recursion limit counter?

Is a nasty band-aid, when it's really not that hard to avoid recursion
altogether.

> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * Adjust the local phandle references by the given phandle delta.
> >>>> + * Assumes the existances of a __local_fixups__ node at the root
> >>>> + * of the tree. Does not take any devtree locks so make sure you
> >>>> + * call this on a tree which is at the detached state.
> >>>> + */
> >>>> +static int __of_adjust_tree_phandle_references(struct device_node *node,
> >>>> +		int phandle_delta)
> >>>> +{
> >>>> +	phandle phandle;
> >>>> +	struct device_node *refnode, *child;
> >>>> +	struct property *rprop, *sprop;
> >>>> +	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
> >>>> +	int offset, propcurlen;
> >>>> +	int err;
> >>>> +
> >>>> +	/* locate the symbols & fixups nodes on resolve */
> >>>> +	__for_each_child_of_node(node, child)
> >>>> +		if (of_node_cmp(child->name, "__local_fixups__") == 0)
> >>>> +			break;
> >>>> +
> >>>> +	/* no local fixups */
> >>>> +	if (child == NULL)
> >>>> +		return 0;
> >>>> +
> >>>> +	/* find the local fixups property */
> >>>> +	for_each_property_of_node(child, rprop) {
> >>>> +
> >>>> +		/* skip properties added automatically */
> >>>> +		if (of_prop_cmp(rprop->name, "name") == 0)
> >>>> +			continue;
> >>> 
> >>> Ok, so you're interpreting any property except name in the
> >>> __local_fixups__ node in exactly the same way?  That's a bit strange.
> >>> Why not just have a single property rather than a node's worth in that
> >>> case.
> >> 
> >> It saves space. For example you might have to resolve a label reference
> >> more than once. So instead of doing:
> >> 
> >> 	label = "/foo:bar:0";
> >> 	label = "/bar:foo:4";
> >> 
> >> You can do this:
> >> 
> >> 	label = "/foo:bar:0", "/bar/foo:4";
> > 
> > Um.. you seem to have read me as saying the exact opposite of what I
> > said.  I'm _suggesting_ that you use:
> > 	__local_fixups__ = "/foo:bar:0", "/bar/foo:4";
> > Since the 'label' name has no meaning in the case of local fixups.
> 
> Err, I misread that.
> 
> At first I tried to do it the way you suggested but run into the following
> problem: The DTC enforces the properties to always precede the child nodes.
> I wasn't able to find a way to insert the __local_fixups__ property easily.
> Perhaps you can suggest where would you put it? 

Ah, ok.  I haven't looked at your dtc patch in any detail yet, since I
wanted to review the concept first, through these patches.

So the basic answer is that you should add the extra annotations for
overlays onto the live tree that dtc maintains before flatenning it,
rather than doing it as part of the flattening process.  On the live
tree you can add properties and subnodes in whatever order.  It also
means that you'll see the right overlay annotations for all dtc output
formats.

> >>>> +		/* make a copy */
> >>>> +		propval = kmalloc(rprop->length, GFP_KERNEL);
> >>>> +		if (propval == NULL) {
> >>>> +			pr_err("%s: Could not copy value of '%s'\n",
> >>>> +					__func__, rprop->name);
> >>>> +			return -ENOMEM;
> >>>> +		}
> >>>> +		memcpy(propval, rprop->value, rprop->length);
> >>>> +
> >>>> +		propend = propval + rprop->length;
> >>>> +		for (propcur = propval; propcur < propend;
> >>>> +				propcur += propcurlen + 1) {
> >>>> +
> >>>> +			propcurlen = strlen(propcur);
> >>>> +
> >>>> +			nodestr = propcur;
> >>>> +			s = strchr(propcur, ':');
> >>> 
> >>> So, using strings with separators like this doesn't sit will with
> >>> existing device tree practice.  More similar to existing things would
> >>> have NUL separators and the integer values in binary, rather than
> >>> text (and yes, there is precedent for mixed string and integer content
> >>> in properties).
> >>> 
> >> 
> >> Hmm, I guess it can be done, but I wouldn't expect any space savings.
> > 
> > I'm not after space savings, just least-surprise by matching existing
> > common practices.
> 
> Maybe. You also lose the ability to print the contents of the node and
> easily inspect. It's your call really, I don't mind that much.

Hm, ok I'll give it some thought.  There are bigger things I'd like to
be convinced on before worrying too much about details like this :).

-- 
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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

Hi David,

On Jan 23, 2013, at 6:40 AM, David Gibson wrote:

> On Tue, Jan 22, 2013 at 01:06:09PM +0200, Pantelis Antoniou wrote:
>> Hi
>> 
>> On Jan 22, 2013, at 6:05 AM, David Gibson wrote:
>> 
>>> On Mon, Jan 21, 2013 at 12:59:15PM +0200, Pantelis Antoniou wrote:
>>>> Hi David
>>>> 
>>>> On Jan 21, 2013, at 6:48 AM, David Gibson wrote:
>>>> 
>>>>> On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou wrote:
>>>>>> Introduce support for dynamic device tree resolution.
>>>>>> Using it, it is possible to prepare a device tree that's
>>>>>> been loaded on runtime to be modified and inserted at the kernel
>>>>>> live tree.
>>>>>> 
>>>>>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>>>>>> ---
>>>>>> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>>>>>> drivers/of/Kconfig                                 |   9 +
>>>>>> drivers/of/Makefile                                |   1 +
>>>>>> drivers/of/resolver.c                              | 394 +++++++++++++++++++++
>>>>>> include/linux/of.h                                 |  17 +
>>>>>> 5 files changed, 446 insertions(+)
>>>>>> create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
>>>>>> create mode 100644 drivers/of/resolver.c
>>>>>> 
>>>>>> diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt b/Documentation/devicetree/dynamic-resolution-notes.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..0b396c4
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/dynamic-resolution-notes.txt
>>>>>> @@ -0,0 +1,25 @@
>>>>>> +Device Tree Dynamic Resolver Notes
>>>>>> +----------------------------------
>>>>>> +
>>>>>> +This document describes the implementation of the in-kernel
>>>>>> +Device Tree resolver, residing in drivers/of/resolver.c and is a
>>>>>> +companion document to Documentation/devicetree/dt-object-internal.txt[1]
>>>>>> +
>>>>>> +How the resolver works
>>>>>> +----------------------
>>>>>> +
>>>>>> +The resolver is given as an input an arbitrary tree compiled with the
>>>>>> +proper dtc option and having a /plugin/ tag. This generates the
>>>>>> +appropriate __fixups__ & __local_fixups__ nodes as described in [1].
>>>>>> +
>>>>>> +In sequence the resolver works by the following steps:
>>>>>> +
>>>>>> +1. Get the maximum device tree phandle value from the live tree + 1.
>>>>>> +2. Adjust all the local phandles of the tree to resolve by that amount.
>>>>>> +3. Using the __local__fixups__ node information adjust all local references
>>>>>> +   by the same amount.
>>>>>> +4. For each property in the __fixups__ node locate the node it references
>>>>>> +   in the live tree. This is the label used to tag the node.
>>>>>> +5. Retrieve the phandle of the target of the fixup.
>>>>>> +5. For each fixup in the property locate the node:property:offset location
>>>>>> +   and replace it with the phandle value.
>>>>> 
>>>>> Hrm.  So, I'm really still not convinced by this approach.
>>>>> 
>>>>> 	First, I think it's unwise to allow overlays to change
>>>>> essentially anything in the base tree, rather than having the base
>>>>> tree define sockets of some sort where things can be attached.
>>>>> 
>>>> 
>>>> One could say that the labels define the sockets. It's not just things
>>>> to be attached, properties might have to change, or something more complex,
>>>> as we've found out in practice.
>>> 
>>> Hrm.  I know a number of these have come up previously in that big
>>> thread, but can you summarise some of these cases here.  If things
>>> need modification in the base tree that still seems to me like the
>>> base tree hasn't properly described the socket arrangement (I realise
>>> that allowing such descriptions may require extensions to some of our
>>> device tree conventions).
>>> 
>> 
>> It would be pointless to number all the use-cases that Grant put in that
>> long document, but I can summarize the cases that we've seen on the bone.
>> 
>> * Addition of child device nodes to the ocp node, creates new platform
>> devices of various kind (audio/video/pwms/timers) - almost any kind of
>> platform device that the SoC supports. Removing the overlay unregisters
>> the devices (but precious few drivers support that cleanly ATM). Since
>> the capes don't support hotplug that's not a big deal.
> 
> Ok, that's just adding nodes, which is straightforward.
> 
>> * Addition of pinctrl configuration nodes.
> 
> Ok, do you know where I can look to see how the pinctrl stuff works?
> 

There's information about it in Documentation/pinctrl.txt, and there's the
source in drivers/pinctrl.

>> * Addition of i2c/spi etc device nodes and modification of the parent's
>> node status property to "okay",
> 
> Ok.  I'm assuming this is basically just to enable the bus controller
> which was previously disabled because it had nothing attached to it?
> 

Yes, but the configuration of the device might need to be altered. For
example the clock, or adding/removing some properties that affect the 
driver.

>> creates the bus platform device & registers
>> the devices on the bus. Slight complication with i2c client devices which
>> are not platform devices need special handling.
> 
> And this part is again just adding nodes.
> 
>> * Modification of configuration parameters of a disabled device and subsequent
>> enablement. 
> 
> What sorts of modification are necessary to the parameters?  Other
> than changing status = "disabled" of course.  This is the only case I
> see here which might be changing data other than the status property,
> which if it were the only one could reasonably be special cased, I
> think.

Some devices might not work reliably with a high clock value. Others
might need to add a flag that modifies the way the driver handles the
hardware attached to the cape (for instance whether an SDcard is 
removable or not) etc. It is all up to the person that designed the
hardware which is connected, so we can't really come up with a list of 
allowed changes.

> 
>>>> As far as the unwise part, a good deal of care has been taken so that 
>>>> people that don't use the overlay functionality have absolutely no
>>>> overhead, or anything modified in the way they use DT.
>>> 
>>> Yeah, that's not what I'm concerned about.  I'm concerned about hard
>>> to debug problems because some subtle change in the base tree or the
>>> overlay or both causes the overlay to alter something in the base tree
>>> it really shouldn't.
>> 
>> Define shouldn't. Let's say someone inserts a bogus overlay that makes the system
>> fail, or misbehave in some other manner. What is the different between 
>> using the overlay and inserting kernel module that fails in a
>> similar manner?
> 
> Hm, you do have a point there.
> 
> Another reason for a more constrained format did occur to me though -
> it opens the possibility for multiple interchangeable sockets on the
> same board.  e.g. we get the BeaglePlus or whatever which can take two
> capes simultaneously.  With the current scheme an identical cape would
> need different overlays for insertion in socket A and socket B because
> it would need different labels for all the fixups.

I see your point, and that's where the beauty of DT helps :)

There's nothing that forces you to have a single label per node, so the number
of labels a node can have is unlimited. 

As part of a convention laid down by the board maker, if one was to support a number
of capes the labels of the nodes must be known in advance.

For instance, let's take the case of beaglebone capes where they typically
hook into &ocp & &pinmux etc. The manufacturer can lay down a policy that
states the only allowable overlay targets, and make sure that every board
that support a given set of capes to label the nodes consistently.

What we are describing here, the device tree resolution & overlay though is not policy,
it is a mechanism. It's not by accident that there is no direct user-space way
to use the DT overlays, the access to it should come from a board/family specific
driver that knows and will enforce the policy.

> 
>> As far as supporting arbitrary overlays, that is not the case, and never will
>> be. Now since the beaglebone is for hobbyists too, we can't restrict what
>> a user can do in any way; but that person is free to do whatever he wants.
>> 
>>>>> 	Second, even allowing overlays to change anything, I don't see
>>>>> a lot of reason to do this kind of resolution within the kernel and
>>>>> with data stored in the dtb itself, rather than doing the resolution
>>>>> in userspace from an annotated overlay dts or dtb, then inserting the
>>>>> fully resolved product into the kernel.  In either case, the overlay
>>>>> needs to be constructed with pretty intimate knowledge of the base
>>>>> tree.
>>>> 
>>>> Fair enough, but that's one more thing of user-space crud to drag along, which
>>>> will get enabled pretty late in the boot sequence. Meaning a whole bunch of devices,
>>>> like consoles, and root filesystems on the devices that need an overlay to operate
>>>> won't work easily enough.
>>> 
>>> Hrm.  But doesn't your scheme already require userspace to identify
>>> the hardware and load the overlay?  So why is having it resolve the
>>> overlay significantly harder?
>> 
>> There is no user-space involved what so ever. The only place where
>> user-space might be involved is supplying the dtbo firmware image as a
>> result of the loader driver issuing request_firmware().  
> 
> Right.. and couldn't the request_firmware() involve processing the
> resolutions, rather than just grabbing the file.
> 

That's not possible AFAIKT. All that request_firmware() can do is hand you
a blob that you've requested.

>>> AFAICT devices wanted early can be handled in several possible ways
>>> without having the resolved in kernel: an initramfs is the most
>>> obvious, but for things you want really early, it should be possible
>>> to do the resolution from the platform's bootloader update tool - so
>>> the pre-resolved overlay gets bundled with the kernel/initrd/whatever
>>> to get fired up from the bootloader.
>> 
>> No. Just no. It is a support nightmare when dealing with the hobbyist market.
>> Something similar has been tried before. While I don't have an exact figure, 
>> having users tinker with any part of the bootloader/init sequence in order to 
>> add some new cape lead to an unacceptable number of RMAed boards, which
>> had absolutely nothing wrong with them, besides the fact that the person tinkering
>> with it couldn't get it booting after screwing it.
>> This wiped off any amount of profit for the board maker; possibly even made the
>> whole operation a loss.
> 
> Ok.  I certainly don't want to dismiss usability concerns. I was
> envisaging this pre-resolving process asn being part of the platform's
> equivalent of running the lilo / update-grub / whatever script.  So
> I'm trying to understand what's different about the platform's boot
> sequence it so much more fraught in this case.
> 

Don't get me started on the insanity of lilo / update-grub process :)
Let's just say there is no need to force anyone to deal with again, there's
absolutely no need to.

> The other thing I notice is that this sort of concern is exactly why I
> lean towards the idea of a more constrained "overlay" format where the
> base tree desribes what a socket can add / change, rather than
> allowing arbitrary alterations to the base tree.  With such
> constraints, it's going to be much harder to screw up the basic
> platform booting due to a bad overlay.  Obviously the cape won't work
> properly without correct device information, but you won't be able to
> clobber the basic device information for the board.
> 

I understand your concern, really. That's why the patches do not expose
any kind of policy management, they're only a low level mechanism that
a policy aware loader driver can use.

I feel that the label method is sufficiently generic, and easily
understood by both users & developers. Now as part of the policy of the
loader driver we can easily add policy constraints as to which 
device nodes are allowed as targets of the overlay. If need be, the loader
can use an extra verification pass.

> [snip]
>>>>> That said, I have some implementation comments below.
>>>>> 
>>>>> [snip]
>>>>>> +/**
>>>>>> + * Find a subtree's maximum phandle value.
>>>>>> + */
>>>>>> +static phandle __of_get_tree_max_phandle(struct device_node *node,
>>>>>> +		phandle max_phandle)
>>>>>> +{
>>>>>> +	struct device_node *child;
>>>>>> +
>>>>>> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
>>>>>> +			node->phandle > max_phandle)
>>>>>> +		max_phandle = node->phandle;
>>>>>> +
>>>>>> +	__for_each_child_of_node(node, child)
>>>>>> +		max_phandle = __of_get_tree_max_phandle(child, max_phandle);
>>>>> 
>>>>> Recursion is best avoided given the kernel's limited stack space.
>>>>> This is also trivial to implement non-recursively, using the allnext
>>>>> pointer.
>>>> 
>>>> The caller passes a tree that's not yet been inserted in the live
>>>> tree.
>>> 
>>> Um.. isn't this used to find the max phandle in the base tree, so how
>>> is it not inserted in the live tree yet?
>> 
>> We have a use case (for devices that must be inserted very early in the boot
>> process) that the overlay is stored in a part of the base tree, not in a
>> different dtbo file. Some phandle juggling there uses this function.
>> This patchset only uses it on the base tree.
> 
> Ok.  Nonetheless it's not hard to avoid a recursive approach here.

How can I find the maximum phandle value of a subtree without using recursion.
Note that the whole function is just 6 lines long.

> 
>>>> So there's no allnodes pointer yet. 
>>> 
>>> I would strongly suggest populating that in the subtree as you build
>>> it, then.  Except that I don't think it is a detached subtree in this
>>> case, though it is in some of the cases below.
>>> 
>>>> Care has been taken for the function
>>>> to not have excessive local variables. I would guess about 20-32 bytes for
>>>> the stack frame + the local variables, so with a 4K stack we would overflow at a 
>>>> nest level of 128, which has a pretty slim chance for a real system.
>>> 
>>> Hrm.  Recursion still makes me nervous.  I believe there are platforms
>>> that have a non-trivial lower-bound on stack usage per frame, even
>>> with few local variables.
>> 
>> How about having a recursion limit argument set to something sane
>> like 10?
> 
> Ugh.  It's really best just to avoid recursion in the kernel.
> 

It is not always possible, these are non-balanced tree structures after all.
Look at lib/btree.c btree_insert_level() function for example. It uses recursion
and uses a recursion level check.

>>>>>> +
>>>>>> +	return max_phandle;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * Find live tree's maximum phandle value.
>>>>>> + */
>>>>>> +static phandle of_get_tree_max_phandle(void)
>>>>>> +{
>>>>>> +	struct device_node *node;
>>>>>> +	phandle phandle;
>>>>>> +
>>>>>> +	/* get root node */
>>>>>> +	node = of_find_node_by_path("/");
>>>>>> +	if (node == NULL)
>>>>>> +		return OF_PHANDLE_ILLEGAL;
>>>>>> +
>>>>>> +	/* now search recursively */
>>>>>> +	read_lock(&devtree_lock);
>>>>>> +	phandle = __of_get_tree_max_phandle(node, 0);
>>>>>> +	read_unlock(&devtree_lock);
>>>>>> +
>>>>>> +	of_node_put(node);
>>>>>> +
>>>>>> +	return phandle;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * Adjust a subtree's phandle values by a given delta.
>>>>>> + * Makes sure not to just adjust the device node's phandle value,
>>>>>> + * but modify the phandle properties values as well.
>>>>>> + */
>>>>>> +static void __of_adjust_tree_phandles(struct device_node *node,
>>>>>> +		int phandle_delta)
>>>>>> +{
>>>>>> +	struct device_node *child;
>>>>>> +	struct property *prop;
>>>>>> +	phandle phandle;
>>>>>> +
>>>>>> +	/* first adjust the node's phandle direct value */
>>>>>> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
>>>>>> +		node->phandle += phandle_delta;
>>>>> 
>>>>> You need to have some kind of check for overflow here, or the adjusted
>>>>> phandle could be one of the illegal values (0 or -1) - or wrap around
>>>>> and colllide with existing phandle values in the base tree.  dtc
>>>>> (currently) allocates phandles from the bottom, but there's no
>>>>> guarantee that a base tree will only have low phandle values - it only
>>>>> takes one node with phandle set to 0xfffffffe in the base tree to have
>>>>> this function make a mess of things.
>>>> 
>>>> Correct, I'll take care of handling the overflow.
>>>> 
>>>>> 
>>>>> 
>>>>>> +	/* now adjust phandle & linux,phandle values */
>>>>>> +	for_each_property_of_node(node, prop) {
>>>>>> +
>>>>>> +		/* only look for these two */
>>>>>> +		if (of_prop_cmp(prop->name, "phandle") != 0 &&
>>>>>> +		    of_prop_cmp(prop->name, "linux,phandle") != 0)
>>>>>> +			continue;
>>>>>> +
>>>>>> +		/* must be big enough */
>>>>>> +		if (prop->length < 4)
>>>>>> +			continue;
>>>>> 
>>>>> If prop->length != 4 (including > 4) something is pretty wrong, and
>>>>> you should probably bail with an error message.
>>>> 
>>>> OK, just playing it safe here.
>>>> 
>>>>> 
>>>>>> +
>>>>>> +		/* read phandle value */
>>>>>> +		phandle = be32_to_cpu(*(uint32_t *)prop->value);
>>>>>> +		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
>>>>>> +			continue;
>>>>>> +
>>>>>> +		/* adjust */
>>>>>> +		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
>>>>>> +	}
>>>>>> +
>>>>>> +	/* now do the children recursively */
>>>>>> +	__for_each_child_of_node(node, child)
>>>>>> +		__of_adjust_tree_phandles(child, phandle_delta);
>>>>> 
>>>>> Again, recursion is not a good idea.
>>>>> 
>>>> 
>>>> No other way to handle it. This is not a node that's in the live
>>>> tree yet.
>>> 
>>> There's always another way to handle it, although it may be less
>>> elegant.  In this case it should be easy though - populate the allnext
>>> pointers when you unflatten the tree.  Or, have the offset applied
>>> when you actually apply the overlay rather than as a separate pass.
>> 
>> Previous comment applies here too. Recursion limit counter?
> 
> Is a nasty band-aid, when it's really not that hard to avoid recursion
> altogether.
> 
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * Adjust the local phandle references by the given phandle delta.
>>>>>> + * Assumes the existances of a __local_fixups__ node at the root
>>>>>> + * of the tree. Does not take any devtree locks so make sure you
>>>>>> + * call this on a tree which is at the detached state.
>>>>>> + */
>>>>>> +static int __of_adjust_tree_phandle_references(struct device_node *node,
>>>>>> +		int phandle_delta)
>>>>>> +{
>>>>>> +	phandle phandle;
>>>>>> +	struct device_node *refnode, *child;
>>>>>> +	struct property *rprop, *sprop;
>>>>>> +	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
>>>>>> +	int offset, propcurlen;
>>>>>> +	int err;
>>>>>> +
>>>>>> +	/* locate the symbols & fixups nodes on resolve */
>>>>>> +	__for_each_child_of_node(node, child)
>>>>>> +		if (of_node_cmp(child->name, "__local_fixups__") == 0)
>>>>>> +			break;
>>>>>> +
>>>>>> +	/* no local fixups */
>>>>>> +	if (child == NULL)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	/* find the local fixups property */
>>>>>> +	for_each_property_of_node(child, rprop) {
>>>>>> +
>>>>>> +		/* skip properties added automatically */
>>>>>> +		if (of_prop_cmp(rprop->name, "name") == 0)
>>>>>> +			continue;
>>>>> 
>>>>> Ok, so you're interpreting any property except name in the
>>>>> __local_fixups__ node in exactly the same way?  That's a bit strange.
>>>>> Why not just have a single property rather than a node's worth in that
>>>>> case.
>>>> 
>>>> It saves space. For example you might have to resolve a label reference
>>>> more than once. So instead of doing:
>>>> 
>>>> 	label = "/foo:bar:0";
>>>> 	label = "/bar:foo:4";
>>>> 
>>>> You can do this:
>>>> 
>>>> 	label = "/foo:bar:0", "/bar/foo:4";
>>> 
>>> Um.. you seem to have read me as saying the exact opposite of what I
>>> said.  I'm _suggesting_ that you use:
>>> 	__local_fixups__ = "/foo:bar:0", "/bar/foo:4";
>>> Since the 'label' name has no meaning in the case of local fixups.
>> 
>> Err, I misread that.
>> 
>> At first I tried to do it the way you suggested but run into the following
>> problem: The DTC enforces the properties to always precede the child nodes.
>> I wasn't able to find a way to insert the __local_fixups__ property easily.
>> Perhaps you can suggest where would you put it? 
> 
> Ah, ok.  I haven't looked at your dtc patch in any detail yet, since I
> wanted to review the concept first, through these patches.
> 
> So the basic answer is that you should add the extra annotations for
> overlays onto the live tree that dtc maintains before flatenning it,
> rather than doing it as part of the flattening process.  On the live
> tree you can add properties and subnodes in whatever order.  It also
> means that you'll see the right overlay annotations for all dtc output
> formats.

Already do (partly). The annotations are added before the flattening process.
But I don't add the properties at that point. My life is complicated by
the fact that I have to be extra careful to not affect non-uses of overlays.

Sigh.

> 
>>>>>> +		/* make a copy */
>>>>>> +		propval = kmalloc(rprop->length, GFP_KERNEL);
>>>>>> +		if (propval == NULL) {
>>>>>> +			pr_err("%s: Could not copy value of '%s'\n",
>>>>>> +					__func__, rprop->name);
>>>>>> +			return -ENOMEM;
>>>>>> +		}
>>>>>> +		memcpy(propval, rprop->value, rprop->length);
>>>>>> +
>>>>>> +		propend = propval + rprop->length;
>>>>>> +		for (propcur = propval; propcur < propend;
>>>>>> +				propcur += propcurlen + 1) {
>>>>>> +
>>>>>> +			propcurlen = strlen(propcur);
>>>>>> +
>>>>>> +			nodestr = propcur;
>>>>>> +			s = strchr(propcur, ':');
>>>>> 
>>>>> So, using strings with separators like this doesn't sit will with
>>>>> existing device tree practice.  More similar to existing things would
>>>>> have NUL separators and the integer values in binary, rather than
>>>>> text (and yes, there is precedent for mixed string and integer content
>>>>> in properties).
>>>>> 
>>>> 
>>>> Hmm, I guess it can be done, but I wouldn't expect any space savings.
>>> 
>>> I'm not after space savings, just least-surprise by matching existing
>>> common practices.
>> 
>> Maybe. You also lose the ability to print the contents of the node and
>> easily inspect. It's your call really, I don't mind that much.
> 
> Hm, ok I'll give it some thought.  There are bigger things I'd like to
> be convinced on before worrying too much about details like this :).

You worry too much! :)

> 
> -- 
> 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: [PATCH 5/6] OF: Introduce Device Tree resolve support.

[Grant Likely]

On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> Hi David,
> 
> On Jan 23, 2013, at 6:40 AM, David Gibson wrote:
> > Ok.  Nonetheless it's not hard to avoid a recursive approach here.
> 
> How can I find the maximum phandle value of a subtree without using recursion.
> Note that the whole function is just 6 lines long.

It's a failure in the existing kernel DT data structures. We need a hash
lookup for the phandles to eliminate the search entirely. Then you'd be
able to allocated new phandles on the fly easily and resolve phandles
without searching the whole tree (which has always been horrible).

That said, I'd like to punt on the whole phandle resolution thing. The
DT overlay support can be merged without the phandle resolution support
if the core rejects any overlays with phandle collisions.

g.

Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

Hi Grant,

On Mar 16, 2013, at 11:24 AM, Grant Likely wrote:

> On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>> Hi David,
>> 
>> On Jan 23, 2013, at 6:40 AM, David Gibson wrote:
>>> Ok.  Nonetheless it's not hard to avoid a recursive approach here.
>> 
>> How can I find the maximum phandle value of a subtree without using recursion.
>> Note that the whole function is just 6 lines long.
> 
> It's a failure in the existing kernel DT data structures. We need a hash
> lookup for the phandles to eliminate the search entirely. Then you'd be
> able to allocated new phandles on the fly easily and resolve phandles
> without searching the whole tree (which has always been horrible).
> 

Yes, it is pretty obvious that the in-kernel data structures are sub-optimal.
But I was not after modifying them, since that's a different kind of problem.

Since we're having a 'sub-optimal' data structures, I'd like to point out that
the usage of of_find_by_name(), mostly by drivers trying to find a child
of their own node, works by a lucky accident of how the device nodes are instantiated
by the flat tree loader. Most of the use cases should be replaced by a call
to of_get_child_by_name() which does the right thing.

> That said, I'd like to punt on the whole phandle resolution thing. The
> DT overlay support can be merged without the phandle resolution support
> if the core rejects any overlays with phandle collisions.
> 


Fair enough, but be warned that phandle resolution the overlay feature is mostly useless.

In actual practice the amount of driver nodes that can be overlaid without a single case
of referencing phandles outside (or within) their own blob is close to zero.

> g.

Regards

-- Pantelis

Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.

[Grant Likely]

On Tue, 19 Mar 2013 13:51:01 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> Hi Grant,
> 
> On Mar 16, 2013, at 11:24 AM, Grant Likely wrote:
> 
> > On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> >> Hi David,
> >> 
> >> On Jan 23, 2013, at 6:40 AM, David Gibson wrote:
> >>> Ok.  Nonetheless it's not hard to avoid a recursive approach here.
> >> 
> >> How can I find the maximum phandle value of a subtree without using recursion.
> >> Note that the whole function is just 6 lines long.
> > 
> > It's a failure in the existing kernel DT data structures. We need a hash
> > lookup for the phandles to eliminate the search entirely. Then you'd be
> > able to allocated new phandles on the fly easily and resolve phandles
> > without searching the whole tree (which has always been horrible).
> > 
> 
> Yes, it is pretty obvious that the in-kernel data structures are sub-optimal.
> But I was not after modifying them, since that's a different kind of problem.

Think about it this way; fixing up that aspect of the data structure
makes the job you're trying to do a lot easier. I don't feel bad about
asking you to add a radix tree for phandle lookups when it makes your
patches a whole lot better.  :-)

> Since we're having a 'sub-optimal' data structures, I'd like to point out that
> the usage of of_find_by_name(), mostly by drivers trying to find a child
> of their own node, works by a lucky accident of how the device nodes are instantiated
> by the flat tree loader. Most of the use cases should be replaced by a call
> to of_get_child_by_name() which does the right thing.

It is true. In fact, calling of_find_node_by_name() when using .dtb is
most likely a bug since using node name to determine behaviour is
strongly discouraged.

> Fair enough, but be warned that phandle resolution the overlay feature is mostly useless.
> 
> In actual practice the amount of driver nodes that can be overlaid without a single case
> of referencing phandles outside (or within) their own blob is close to zero.

That's not what I'm saying. I'm saying that (at least for now) we should
require the overlay to already know the phandles from the parent and to
refuse to load an overlay that defines phandles already in use in the
base. Overlays do become usable at that point. A mechanism for phandle
resolution so that conflicts can be found and resolved can be added
as a feature enhancement. By splitting it out you'll be able to get the
overlay feature merged even if we don't have agreement on the resolution
mechanism yet.

g.

Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

Hi Grant,

On Mar 19, 2013, at 7:18 PM, Grant Likely wrote:

> On Tue, 19 Mar 2013 13:51:01 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>> Hi Grant,
>> 
>> On Mar 16, 2013, at 11:24 AM, Grant Likely wrote:
>> 
>>> On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>>>> Hi David,
>>>> 
>>>> On Jan 23, 2013, at 6:40 AM, David Gibson wrote:
>>>>> Ok.  Nonetheless it's not hard to avoid a recursive approach here.
>>>> 
>>>> How can I find the maximum phandle value of a subtree without using recursion.
>>>> Note that the whole function is just 6 lines long.
>>> 
>>> It's a failure in the existing kernel DT data structures. We need a hash
>>> lookup for the phandles to eliminate the search entirely. Then you'd be
>>> able to allocated new phandles on the fly easily and resolve phandles
>>> without searching the whole tree (which has always been horrible).
>>> 
>> 
>> Yes, it is pretty obvious that the in-kernel data structures are sub-optimal.
>> But I was not after modifying them, since that's a different kind of problem.
> 
> Think about it this way; fixing up that aspect of the data structure
> makes the job you're trying to do a lot easier. I don't feel bad about
> asking you to add a radix tree for phandle lookups when it makes your
> patches a whole lot better.  :-)
> 

Oh that's going to be fun... maybe.

>> Since we're having a 'sub-optimal' data structures, I'd like to point out that
>> the usage of of_find_by_name(), mostly by drivers trying to find a child
>> of their own node, works by a lucky accident of how the device nodes are instantiated
>> by the flat tree loader. Most of the use cases should be replaced by a call
>> to of_get_child_by_name() which does the right thing.
> 
> It is true. In fact, calling of_find_node_by_name() when using .dtb is
> most likely a bug since using node name to determine behaviour is
> strongly discouraged.

Maybe mark the function as deprecated then? Easier to get people to fix it.

> 
>> Fair enough, but be warned that phandle resolution the overlay feature is mostly useless.
>> 
>> In actual practice the amount of driver nodes that can be overlaid without a single case
>> of referencing phandles outside (or within) their own blob is close to zero.
> 
> That's not what I'm saying. I'm saying that (at least for now) we should
> require the overlay to already know the phandles from the parent and to
> refuse to load an overlay that defines phandles already in use in the
> base. Overlays do become usable at that point. A mechanism for phandle
> resolution so that conflicts can be found and resolved can be added
> as a feature enhancement. By splitting it out you'll be able to get the
> overlay feature merged even if we don't have agreement on the resolution
> mechanism yet.
> 

As long as we don't come up with something that's too difficult to use for non
kernel developers. I would hate to punt and push all the complexity of phandle
resolution to the driver/cape developer.

FWIW, the resolution mechanism we use on the beaglebone seems to work just fine,
and people seem to handle it just fine. The part that trips them is mostly how
to install the updated dtc compiler that's required.

> g.
> 

Regards

-- Pantelis

[PATCH 6/6] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Introduce DT overlay support.
Using this functionality it is possible to dynamically overlay a part of
the kernel's tree with another tree that's been dynamically loaded.
It is also possible to remove node and properties.

Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
---
 Documentation/devicetree/overlay-notes.txt | 179 +++++++
 drivers/of/Kconfig                         |  10 +
 drivers/of/Makefile                        |   1 +
 drivers/of/overlay.c                       | 831 +++++++++++++++++++++++++++++
 include/linux/of.h                         | 107 ++++
 5 files changed, 1128 insertions(+)
 create mode 100644 Documentation/devicetree/overlay-notes.txt
 create mode 100644 drivers/of/overlay.c

diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
new file mode 100644
index 0000000..5289cbb
--- /dev/null
+++ b/Documentation/devicetree/overlay-notes.txt
@@ -0,0 +1,179 @@
+Device Tree Overlay Notes
+-------------------------
+
+This document describes the implementation of the in-kernel
+device tree overlay functionality residing in drivers/of/overlay.c and is a
+companion document to Documentation/devicetree/dt-object-internal.txt[1] &
+Documentation/devicetree/dynamic-resolution-notes.txt[2]
+
+How overlays work
+-----------------
+
+A Device Tree's overlay purpose is to modify the kernel's live tree, and
+have the modification affecting the state of the the kernel in a way that
+is reflecting the changes.
+Since the kernel mainly deals with devices, any new device node that result
+in an active device should have it created while if the device node is either
+disabled or removed all together, the affected device should be deregistered.
+
+Lets take an example where we have a foo board with the following base tree
+which is taken from [1].
+
+---- foo.dts -----------------------------------------------------------------
+	/* FOO platform */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+		}
+	};
+---- foo.dts -----------------------------------------------------------------
+
+The overlay bar.dts, when loaded (and resolved as described in [2]) should
+
+---- bar.dts -----------------------------------------------------------------
+/plugin/;	/* allow undefined label references and record them */
+/ {
+	....	/* various properties for loader use; i.e. part id etc. */
+	fragment@0 {
+		target = <&ocp>;
+		__overlay__ {
+			/* bar peripheral */
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			}
+		};
+	};
+};
+---- bar.dts -----------------------------------------------------------------
+
+result in foo+bar.dts
+
+---- foo+bar.dts -------------------------------------------------------------
+	/* FOO platform + bar peripheral */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+
+			/* bar peripheral */
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			}
+		}
+	};
+---- foo+bar.dts -------------------------------------------------------------
+
+As a result of the the overlay, a new device node (bar) has been created
+so a bar platform device will be registered and if a matching device driver
+is loaded the device will be created as expected.
+
+Overlay in-kernel API
+---------------------
+
+The steps typically required to get an overlay to work are as follows:
+
+1. Use of_build_overlay_info() to create an array of initialized and
+ready to use of_overlay_info structures.
+2. Call of_overlay() to apply the overlays declared in the array.
+3. If the overlay needs to be removed, call of_overlay_revert().
+4. Finally release the memory taken by the overlay info array by
+of_free_overlay_info().
+
+/**
+ * of_build_overlay_info	- Build an overlay info array
+ * @tree:	Device node containing all the overlays
+ * @cntp:	Pointer to where the overlay info count will be help
+ * @ovinfop:	Pointer to the pointer of an overlay info structure.
+ *
+ * Helper function that given a tree containing overlay information,
+ * allocates and builds an overlay info array containing it, ready
+ * for use using of_overlay.
+ *
+ * Returns 0 on success with the @cntp @ovinfop pointers valid,
+ * while on error a negative error value is returned.
+ */
+int of_build_overlay_info(struct device_node *tree,
+		int *cntp, struct of_overlay_info **ovinfop);
+
+/**
+ * of_free_overlay_info	- Free an overlay info array
+ * @count:	Number of of_overlay_info's
+ * @ovinfo_tab:	Array of overlay_info's to free
+ *
+ * Releases the memory of a previously allocate ovinfo array
+ * by of_build_overlay_info.
+ * Returns 0, or an error if the arguments are bogus.
+ */
+int of_free_overlay_info(int count, struct of_overlay_info *ovinfo_tab);
+
+/**
+ * of_overlay	- Apply @count overlays pointed at by @ovinfo_tab
+ * @count:	Number of of_overlay_info's
+ * @ovinfo_tab:	Array of overlay_info's to apply
+ *
+ * Applies the overlays given, while handling all error conditions
+ * appropriately. Either the operation succeeds, or if it fails the
+ * live tree is reverted to the state before the attempt.
+ * Returns 0, or an error if the overlay attempt failed.
+ */
+int of_overlay(int count, struct of_overlay_info *ovinfo_tab);
+
+/**
+ * of_overlay_revert	- Revert a previously applied overlay
+ * @count:	Number of of_overlay_info's
+ * @ovinfo_tab:	Array of overlay_info's to apply
+ *
+ * Revert a previous overlay. The state of the live tree
+ * is reverted to the one before the overlay.
+ * Returns 0, or an error if the overlay table is not given.
+ */
+int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab);
+
+Overlay DTS Format
+------------------
+
+The DTS of an overlay should have the following format:
+
+{
+	/* ignored properties by the overlay */
+
+	fragment@0 {	/* first child node */
+		target=<phandle>;	/* target of the overlay */
+		__overlay__ {
+			property-a;	/* add property-a to the target */
+			-property-b;	/* remove property-b from target */
+			node-a {	/* add to an existing, or create a node-a */
+				...
+			};
+			-node-b {	/* remove an existing node-b */
+				...
+			};
+		};
+	}
+	fragment@1 {	/* second child node */
+		...
+	};
+	/* more fragments follow */
+}
+
+It should be noted that the DT overlay format described is the one expected
+by the of_build_overlay_info() function, which is a helper function. There
+is nothing stopping someone coming up with his own DTS format and that will
+end up filling in the fields of the of_overlay_info array. 
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index f9a6193..964a1c2 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -92,4 +92,14 @@ config OF_RESOLVE
 	  Enable OF dynamic resolution support. This allows you to
 	  load Device Tree object fragments are run time.
 
+config OF_OVERLAY
+	bool "OF overlay support"
+	depends on OF
+	select OF_DYNAMIC
+	select OF_DEVICE
+	select OF_RESOLVE
+	help
+	  OpenFirmware overlay support. Allows you to modify on runtime the
+	  live tree using overlays.
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 9a809c9..bc06912 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
 obj-$(CONFIG_OF_RESOLVE)  += resolver.o
+obj-$(CONFIG_OF_OVERLAY) += overlay.o
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
new file mode 100644
index 0000000..f65c3a3
--- /dev/null
+++ b/drivers/of/overlay.c
@@ -0,0 +1,831 @@
+/*
+ * Functions for working with device tree overlays
+ *
+ * Copyright (C) 2012 Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
+ * Copyright (C) 2012 Texas Instruments Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/string.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+
+/**
+ * Apply a single overlay node recursively.
+ *
+ * Property or node names that start with '-' signal that
+ * the property/node is to be removed.
+ *
+ * All the property notifiers are appropriately called.
+ * Note that the in case of an error the target node is left
+ * in a inconsistent state. Error recovery should be performed
+ * by recording the modification using the of notifiers.
+ */
+static int of_overlay_apply_one(struct device_node *target,
+		const struct device_node *overlay)
+{
+	const char *pname, *cname;
+	struct device_node *child, *tchild;
+	struct property *prop, *propn, *tprop;
+	int remove;
+	char *full_name;
+	const char *suffix;
+	int ret;
+
+	/* sanity checks */
+	if (target == NULL || overlay == NULL)
+		return -EINVAL;
+
+	for_each_property_of_node(overlay, prop) {
+
+		/* don't touch, 'name' */
+		if (of_prop_cmp(prop->name, "name") == 0)
+			continue;
+
+		/* default is add */
+		remove = 0;
+		pname = prop->name;
+		if (*pname == '-') {	/* skip, - notes removal */
+			pname++;
+			remove = 1;
+			propn = NULL;
+		} else {
+			propn = __of_copy_property(prop,
+					GFP_KERNEL);
+			if (propn == NULL)
+				return -ENOMEM;
+		}
+
+		tprop = of_find_property(target, pname, NULL);
+
+		/* found? */
+		if (tprop != NULL) {
+			if (propn != NULL)
+				ret = of_update_property(target, propn);
+			else
+				ret = of_remove_property(target, tprop);
+		} else {
+			if (propn != NULL)
+				ret = of_add_property(target, propn);
+			else
+				ret = 0;
+		}
+		if (ret != 0)
+			return ret;
+	}
+
+	__for_each_child_of_node(overlay, child) {
+
+		/* default is add */
+		remove = 0;
+		cname = child->name;
+		if (*cname == '-') {	/* skip, - notes removal */
+			cname++;
+			remove = 1;
+		}
+
+		/* special case for nodes with a suffix */
+		suffix = strrchr(child->full_name, '@');
+		if (suffix != NULL) {
+			cname = kbasename(child->full_name);
+			WARN_ON(cname == NULL);	/* sanity check */
+			if (cname == NULL)
+				continue;
+			if (*cname == '-')
+				cname++;
+		}
+
+		tchild = of_get_child_by_name(target, cname);
+		if (tchild != NULL) {
+
+			if (!remove) {
+
+				/* apply overlay recursively */
+				ret = of_overlay_apply_one(tchild, child);
+				of_node_put(tchild);
+
+				if (ret != 0)
+					return ret;
+
+			} else {
+
+				ret = of_detach_node(tchild);
+				of_node_put(tchild);
+			}
+
+		} else {
+
+			if (!remove) {
+				full_name = kasprintf(GFP_KERNEL, "%s/%s",
+						target->full_name, cname);
+				if (full_name == NULL)
+					return -ENOMEM;
+
+				/* create empty tree as a target */
+				tchild = __of_create_empty_node(cname,
+						child->type, full_name,
+						child->phandle, GFP_KERNEL);
+
+				/* free either way */
+				kfree(full_name);
+
+				if (tchild == NULL)
+					return -ENOMEM;
+
+				/* point to parent */
+				tchild->parent = target;
+
+				ret = of_attach_node(tchild);
+				if (ret != 0)
+					return ret;
+
+				/* apply the overlay */
+				ret = of_overlay_apply_one(tchild, child);
+				if (ret != 0) {
+					__of_free_tree(tchild);
+					return ret;
+				}
+			}
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * Lookup an overlay device entry
+ */
+struct of_overlay_device_entry *of_overlay_device_entry_lookup(
+		struct of_overlay_info *ovinfo, struct device_node *node)
+{
+	struct of_overlay_device_entry *re;
+
+	/* no need for locks, we're under the ovinfo->lock */
+	list_for_each_entry(re, &ovinfo->de_list, node) {
+		if (re->np == node)
+			return re;
+	}
+	return NULL;
+}
+
+/**
+ * Add an overlay log entry
+ */
+static int of_overlay_log_entry_entry_add(struct of_overlay_info *ovinfo,
+		unsigned long action, struct device_node *dn,
+		struct property *prop)
+{
+	struct of_overlay_log_entry *le;
+
+	/* check */
+	if (ovinfo == NULL || dn == NULL)
+		return -EINVAL;
+
+	le = kzalloc(sizeof(*le), GFP_KERNEL);
+	if (le == NULL) {
+		pr_err("%s: Failed to allocate\n", __func__);
+		return -ENOMEM;
+	}
+
+	/* get a reference to the node */
+	le->action = action;
+	le->np = of_node_get(dn);
+	le->prop = prop;
+
+	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop)
+		le->old_prop = of_find_property(dn, prop->name, NULL);
+
+	list_add_tail(&le->node, &ovinfo->le_list);
+
+	return 0;
+}
+
+/**
+ * Add an overlay device entry
+ */
+static void of_overlay_device_entry_entry_add(struct of_overlay_info *ovinfo,
+		struct device_node *node, struct platform_device *pdev,
+		int state)
+{
+	struct of_overlay_device_entry *re;
+	int fresh;
+
+	/* check */
+	if (ovinfo == NULL)
+		return;
+
+	fresh = 0;
+	re = of_overlay_device_entry_lookup(ovinfo, node);
+	if (re == NULL) {
+		re = kzalloc(sizeof(*re), GFP_KERNEL);
+		if (re == NULL) {
+			pr_err("%s: Failed to allocate\n", __func__);
+			return;
+		}
+		fresh = 1;
+	}
+
+	if (re->np == NULL)
+		re->np = of_node_get(node);
+	if (re->pdev == NULL)
+		re->pdev = of_dev_get(pdev);
+	re->state = state;
+
+	if (fresh)
+		list_add_tail(&re->node, &ovinfo->de_list);
+}
+
+/**
+ * Overlay OF notifier
+ *
+ * Called every time there's a property/node modification
+ * Every modification causes a log entry addition, while
+ * any modification that causes a node's state to change
+ * from/to disabled to/from enabled causes a device entry
+ * addition.
+ */
+static int of_overlay_notify(struct notifier_block *nb,
+				unsigned long action, void *arg)
+{
+#ifdef DEBUG
+	char *propstr = NULL, *spropstr = NULL;
+#endif
+	struct of_overlay_info *ovinfo;
+	struct device_node *node;
+	struct property *prop, *sprop, *cprop;
+	struct of_prop_reconfig *pr;
+	struct platform_device *pdev;
+	int state;
+	int err = 0;
+
+	ovinfo = container_of(nb, struct of_overlay_info, notifier);
+
+	/* prep vars */
+	switch (action) {
+	case OF_RECONFIG_ATTACH_NODE:
+	case OF_RECONFIG_DETACH_NODE:
+		node = arg;
+		if (node == NULL)
+			return notifier_from_errno(-EINVAL);
+		prop = NULL;
+#ifdef DEBUG
+		propstr = NULL;
+#endif
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+	case OF_RECONFIG_REMOVE_PROPERTY:
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		pr = arg;
+		if (pr == NULL)
+			return notifier_from_errno(-EINVAL);
+		node = pr->dn;
+		if (node == NULL)
+			return notifier_from_errno(-EINVAL);
+		prop = pr->prop;
+		if (prop == NULL)
+			return notifier_from_errno(-EINVAL);
+#ifdef DEBUG
+		propstr = __of_dump_prop(prop);
+#endif
+		break;
+	default:
+		return notifier_from_errno(0);
+	}
+
+	/* add to the log */
+	err = of_overlay_log_entry_entry_add(ovinfo, action, node, prop);
+	if (err != 0)
+		return notifier_from_errno(err);
+
+#ifdef DEBUG
+	switch (action) {
+	case OF_RECONFIG_ATTACH_NODE:
+		pr_debug("ATTACH_NODE: %s\n", node->full_name);
+		break;
+	case OF_RECONFIG_DETACH_NODE:
+		pr_debug("DETACH_NODE: %s\n", node->full_name);
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+		pr_debug("ADD_PROP:    %s %s%s\n", node->full_name,
+				prop->name, propstr);
+		break;
+	case OF_RECONFIG_REMOVE_PROPERTY:
+		pr_debug("REMOVE_PROP: %s %s%s\n", node->full_name,
+				prop->name, propstr);
+		break;
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		sprop = of_find_property(node, prop->name, NULL);
+		if (sprop)
+			spropstr = __of_dump_prop(sprop);
+		pr_debug("UPDATE_PROP: %s '%s%s' -> '%s%s'\n", node->full_name,
+				prop->name, spropstr,
+				prop->name, propstr);
+		break;
+
+	}
+
+	/* NULL is fine */
+	kfree(propstr);
+	kfree(spropstr);
+#endif
+
+	/* come up with the device entry (if any) */
+	pdev = NULL;
+	state = 0;
+
+	/* determine the state the node will end up */
+	switch (action) {
+	case OF_RECONFIG_ATTACH_NODE:
+		/* we demand that a compatible node is present */
+		state = of_find_property(node, "compatible", NULL) &&
+			of_device_is_available(node);
+		break;
+	case OF_RECONFIG_DETACH_NODE:
+		state = 0;
+		pdev = of_find_device_by_node(node);
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+	case OF_RECONFIG_REMOVE_PROPERTY:
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		/* either one cause a change in state */
+		if (strcmp(prop->name, "status") != 0 &&
+				strcmp(prop->name, "compatible") != 0)
+			return notifier_from_errno(0);
+
+		if (strcmp(prop->name, "status") == 0) {
+			/* status */
+			cprop = of_find_property(node, "compatible", NULL);
+			sprop = action != OF_RECONFIG_REMOVE_PROPERTY ?
+				prop : NULL;
+		} else {
+			/* compatible */
+			sprop = of_find_property(node, "status", NULL);
+			cprop = action != OF_RECONFIG_REMOVE_PROPERTY ?
+				prop : NULL;
+		}
+
+		state = cprop && cprop->length > 0 &&
+			    (!sprop || (sprop->length > 0 &&
+				(strcmp(sprop->value, "okay") == 0 ||
+				 strcmp(sprop->value, "ok") == 0)));
+		break;
+
+	default:
+		return notifier_from_errno(0);
+	}
+
+	if (state == 0)
+		pdev = of_find_device_by_node(node);
+
+	of_overlay_device_entry_entry_add(ovinfo, node, pdev, state);
+
+	return notifier_from_errno(err);
+}
+
+/**
+ * Prepare for the overlay, for now it just registers the
+ * notifier.
+ */
+static int of_overlay_prep_one(struct of_overlay_info *ovinfo)
+{
+	int err;
+
+	err = of_reconfig_notifier_register(&ovinfo->notifier);
+	if (err != 0) {
+		pr_err("%s: failed to register notifier for '%s'\n",
+			__func__, ovinfo->target->full_name);
+		return err;
+	}
+	return 0;
+}
+
+/**
+ * Revert one overlay
+ * Either due to an error, or due to normal overlay removal.
+ * Using the log entries, we revert any change to the live tree.
+ * In the same manner, using the device entries we enable/disable
+ * the platform devices appropriately.
+ */
+static void of_overlay_revert_one(struct of_overlay_info *ovinfo)
+{
+	struct of_overlay_device_entry *re, *ren;
+	struct of_overlay_log_entry *le, *len;
+	struct property *prop, **propp;
+	struct platform_device *pdev, *parent_pdev;
+	int ret;
+	unsigned long flags;
+
+	if (!ovinfo || !ovinfo->target || !ovinfo->overlay)
+		return;
+
+	pr_debug("%s: Reverting overlay on '%s'\n", __func__,
+			ovinfo->target->full_name);
+
+	/* overlay applied correctly, now create/destroy pdevs */
+	list_for_each_entry_safe_reverse(re, ren, &ovinfo->de_list, node) {
+
+		if (!re->state) {
+
+			parent_pdev = of_find_device_by_node(re->np->parent);
+
+			pr_debug("%s: creating new platform device "
+					"new_node='%s' %p\n",
+					__func__, re->np->full_name, re->np);
+
+			pdev = of_platform_device_create(re->np, NULL,
+					parent_pdev ? &parent_pdev->dev : NULL);
+			of_dev_put(parent_pdev);
+
+			if (pdev == NULL) {
+				pr_warn("%s: Failed to create platform device "
+						"for '%s'\n",
+						__func__, re->np->full_name);
+			}
+
+		} else {
+
+			if (re->pdev) {
+				pr_debug("%s: removing pdev %s\n", __func__,
+						dev_name(&re->pdev->dev));
+				platform_device_unregister(re->pdev);
+			}
+		}
+
+		of_node_put(re->np);
+
+		list_del(&re->node);
+
+		kfree(re);
+	}
+
+	list_for_each_entry_safe_reverse(le, len, &ovinfo->le_list, node) {
+
+		ret = 0;
+		switch (le->action) {
+		case OF_RECONFIG_ATTACH_NODE:
+			pr_debug("Reverting ATTACH_NODE %s\n",
+					le->np->full_name);
+			ret = of_detach_node(le->np);
+			break;
+
+		case OF_RECONFIG_DETACH_NODE:
+			pr_debug("Reverting DETACH_NODE %s\n",
+					le->np->full_name);
+			ret = of_attach_node(le->np);
+			break;
+
+		case OF_RECONFIG_ADD_PROPERTY:
+			pr_debug("Reverting ADD_PROPERTY %s %s\n",
+					le->np->full_name, le->prop->name);
+			ret = of_remove_property(le->np, le->prop);
+			break;
+
+		case OF_RECONFIG_REMOVE_PROPERTY:
+		case OF_RECONFIG_UPDATE_PROPERTY:
+
+			pr_debug("Reverting %s_PROPERTY %s %s\n",
+				le->action == OF_RECONFIG_REMOVE_PROPERTY ?
+					"REMOVE" : "UPDATE",
+					le->np->full_name, le->prop->name);
+
+			/* property is possibly on deadprops (avoid alloc) */
+			write_lock_irqsave(&devtree_lock, flags);
+			prop = le->action == OF_RECONFIG_REMOVE_PROPERTY ?
+				le->prop : le->old_prop;
+			propp = &le->np->deadprops;
+			while (*propp != NULL) {
+				if (*propp == prop)
+					break;
+				propp = &(*propp)->next;
+			}
+			if (*propp != NULL) {
+				/* remove it from deadprops */
+				(*propp)->next = prop->next;
+				write_unlock_irqrestore(&devtree_lock, flags);
+			} else {
+				write_unlock_irqrestore(&devtree_lock, flags);
+				/* not found, just make a copy */
+				prop = __of_copy_property(prop, GFP_KERNEL);
+				if (prop == NULL) {
+					pr_err("%s: Failed to copy property\n",
+							__func__);
+					break;
+				}
+			}
+
+			if (le->action == OF_RECONFIG_REMOVE_PROPERTY)
+				ret = of_add_property(le->np, prop);
+			else
+				ret = of_update_property(le->np, prop);
+			break;
+
+		default:
+			/* nothing */
+			break;
+		}
+
+		if (ret != 0)
+			pr_err("%s: revert on node %s Failed!\n",
+					__func__, le->np->full_name);
+
+		of_node_put(le->np);
+
+		list_del(&le->node);
+
+		kfree(le);
+	}
+}
+
+/**
+ * Perform the post overlay work.
+ *
+ * We unregister the notifier, and in the case on an error we
+ * revert the overlay.
+ * If the overlay applied correctly, we iterare over the device entries
+ * and create/destroy the platform devices appropriately.
+ */
+static int of_overlay_post_one(struct of_overlay_info *ovinfo, int err)
+{
+	struct of_overlay_device_entry *re, *ren;
+	struct platform_device *pdev, *parent_pdev;
+
+	of_reconfig_notifier_unregister(&ovinfo->notifier);
+
+	if (err != 0) {
+		/* revert this (possible partially applied) overlay */
+		of_overlay_revert_one(ovinfo);
+		return 0;
+	}
+
+	/* overlay applied correctly, now create/destroy pdevs */
+	list_for_each_entry_safe(re, ren, &ovinfo->de_list, node) {
+
+		if (re->state) {
+
+			parent_pdev = of_find_device_by_node(re->np->parent);
+
+			pr_debug("%s: creating new platform device "
+					"new_node='%s' %p\n",
+					__func__, re->np->full_name, re->np);
+
+			pdev = of_platform_device_create(re->np, NULL,
+					parent_pdev ? &parent_pdev->dev : NULL);
+			of_dev_put(parent_pdev);
+
+			if (pdev == NULL) {
+				pr_warn("%s: Failed to create platform device "
+						"for '%s'\n",
+						__func__, re->np->full_name);
+			}
+
+			/* keep it around */
+			re->pdev = pdev;
+
+		} else {
+
+			if (re->pdev) {
+				platform_device_unregister(re->pdev);
+			}
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * of_overlay	- Apply @count overlays pointed at by @ovinfo_tab
+ * @count:	Number of of_overlay_info's
+ * @ovinfo_tab:	Array of overlay_info's to apply
+ *
+ * Applies the overlays given, while handling all error conditions
+ * appropriately. Either the operation succeeds, or if it fails the
+ * live tree is reverted to the state before the attempt.
+ * Returns 0, or an error if the overlay attempt failed.
+ */
+int of_overlay(int count, struct of_overlay_info *ovinfo_tab)
+{
+	struct of_overlay_info *ovinfo;
+	int i, err;
+
+	if (!ovinfo_tab)
+		return -EINVAL;
+
+	/* first we apply the overlays atomically */
+	for (i = 0; i < count; i++) {
+
+		ovinfo = &ovinfo_tab[i];
+
+		mutex_lock(&ovinfo->lock);
+
+		err = of_overlay_prep_one(ovinfo);
+		if (err == 0)
+			err = of_overlay_apply_one(ovinfo->target,
+					ovinfo->overlay);
+		of_overlay_post_one(ovinfo, err);
+
+		mutex_unlock(&ovinfo->lock);
+
+		if (err != 0) {
+			pr_err("%s: overlay failed '%s'\n",
+				__func__, ovinfo->target->full_name);
+			goto err_fail;
+		}
+	}
+
+	return 0;
+
+err_fail:
+	while (--i >= 0) {
+		ovinfo = &ovinfo_tab[i];
+
+		mutex_lock(&ovinfo->lock);
+		of_overlay_revert_one(ovinfo);
+		mutex_unlock(&ovinfo->lock);
+	}
+
+	return err;
+}
+
+/**
+ * of_overlay_revert	- Revert a previously applied overlay
+ * @count:	Number of of_overlay_info's
+ * @ovinfo_tab:	Array of overlay_info's to apply
+ *
+ * Revert a previous overlay. The state of the live tree
+ * is reverted to the one before the overlay.
+ * Returns 0, or an error if the overlay table is not given.
+ */
+int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab)
+{
+	struct of_overlay_info *ovinfo;
+	int i;
+
+	if (!ovinfo_tab)
+		return -EINVAL;
+
+	/* revert the overlays in reverse */
+	for (i = count - 1; i >= 0; i--) {
+
+		ovinfo = &ovinfo_tab[i];
+
+		mutex_lock(&ovinfo->lock);
+		of_overlay_revert_one(ovinfo);
+		mutex_unlock(&ovinfo->lock);
+
+	}
+
+	return 0;
+}
+
+/**
+ * of_init_overlay_info	- Initialize a single of_overlay_info structure
+ * @ovinfo:	Pointer to the overlay info structure to initialize
+ *
+ * Initialize a single overlay info structure.
+ */
+void of_init_overlay_info(struct of_overlay_info *ovinfo)
+{
+	memset(ovinfo, 0, sizeof(ovinfo));
+	mutex_init(&ovinfo->lock);
+	INIT_LIST_HEAD(&ovinfo->de_list);
+	INIT_LIST_HEAD(&ovinfo->le_list);
+
+	ovinfo->notifier.notifier_call = of_overlay_notify;
+}
+
+/**
+ * of_fill_overlay_info	- Fill an overlay info structure
+ * @info_node:	Device node containing the overlay
+ * @ovinfo:	Pointer to the overlay info structure to fill
+ *
+ * Fills an overlay info structure with the overlay information
+ * from a device node. This device node must have a target property
+ * which contains a phandle of the overlay target node, and an
+ * __overlay__ child node which has the overlay contents.
+ * Both ovinfo->target & ovinfo->overlay have their references taken.
+ *
+ * Returns 0 on success, or a negative error value.
+ */
+int of_fill_overlay_info(struct device_node *info_node,
+		struct of_overlay_info *ovinfo)
+{
+	u32 val;
+	int ret;
+
+	if (!info_node || !ovinfo)
+		return -EINVAL;
+
+	ret = of_property_read_u32(info_node, "target", &val);
+	if (ret != 0)
+		goto err_fail;
+
+	ovinfo->target = of_find_node_by_phandle(val);
+	if (ovinfo->target == NULL)
+		goto err_fail;
+
+	ovinfo->overlay = of_get_child_by_name(info_node, "__overlay__");
+	if (ovinfo->overlay == NULL)
+		goto err_fail;
+
+	return 0;
+
+err_fail:
+	of_node_put(ovinfo->target);
+	of_node_put(ovinfo->overlay);
+
+	memset(ovinfo, 0, sizeof(*ovinfo));
+	return -EINVAL;
+}
+
+/**
+ * of_build_overlay_info	- Build an overlay info array
+ * @tree:	Device node containing all the overlays
+ * @cntp:	Pointer to where the overlay info count will be help
+ * @ovinfop:	Pointer to the pointer of an overlay info structure.
+ *
+ * Helper function that given a tree containing overlay information,
+ * allocates and builds an overlay info array containing it, ready
+ * for use using of_overlay.
+ *
+ * Returns 0 on success with the @cntp @ovinfop pointers valid,
+ * while on error a negative error value is returned.
+ */
+int of_build_overlay_info(struct device_node *tree,
+		int *cntp, struct of_overlay_info **ovinfop)
+{
+	struct device_node *node;
+	struct of_overlay_info *ovinfo;
+	int cnt, err;
+
+	if (tree == NULL || cntp == NULL || ovinfop == NULL)
+		return -EINVAL;
+
+	/* worst case; every child is a node */
+	cnt = 0;
+	for_each_child_of_node(tree, node)
+		cnt++;
+
+	ovinfo = kzalloc(cnt * sizeof(*ovinfo), GFP_KERNEL);
+	if (ovinfo == NULL)
+		return -ENOMEM;
+
+	cnt = 0;
+	for_each_child_of_node(tree, node) {
+
+		of_init_overlay_info(&ovinfo[cnt]);
+		err = of_fill_overlay_info(node, &ovinfo[cnt]);
+		if (err == 0)
+			cnt++;
+	}
+
+	/* if nothing filled, return error */
+	if (cnt == 0) {
+		kfree(ovinfo);
+		return -ENODEV;
+	}
+
+	*cntp = cnt;
+	*ovinfop = ovinfo;
+
+	return 0;
+}
+
+/**
+ * of_free_overlay_info	- Free an overlay info array
+ * @count:	Number of of_overlay_info's
+ * @ovinfo_tab:	Array of overlay_info's to free
+ *
+ * Releases the memory of a previously allocate ovinfo array
+ * by of_build_overlay_info.
+ * Returns 0, or an error if the arguments are bogus.
+ */
+int of_free_overlay_info(int count, struct of_overlay_info *ovinfo_tab)
+{
+	struct of_overlay_info *ovinfo;
+	int i;
+
+	if (!ovinfo_tab || count < 0)
+		return -EINVAL;
+
+	/* do it in reverse */
+	for (i = count - 1; i >= 0; i--) {
+		ovinfo = &ovinfo_tab[i];
+
+		of_node_put(ovinfo->target);
+		of_node_put(ovinfo->overlay);
+	}
+	kfree(ovinfo_tab);
+
+	return 0;
+}
+
diff --git a/include/linux/of.h b/include/linux/of.h
index ab52243..8a908f0 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -667,4 +667,111 @@ static inline int of_resolve(struct device_node *resolve)
 
 #endif
 
+/**
+ * Overlay support
+ */
+
+/**
+ * struct of_overlay_log_entry	- Holds a DT log entry
+ * @node:	list_head for the log list
+ * @action:	notifier action
+ * @np:		pointer to the device node affected
+ * @prop:	pointer to the property affected
+ * @old_prop:	hold a pointer to the original property
+ *
+ * Every modification of the device tree during application of the
+ * overlay is held in a list of of_overlay_log_entry structures.
+ * That way we can recover from a partial application, or we can
+ * revert the overlay properly.
+ */
+struct of_overlay_log_entry {
+	struct list_head node;
+	unsigned long action;
+	struct device_node *np;
+	struct property *prop;
+	struct property *old_prop;
+};
+
+/**
+ * struct of_overlay_device_entry	- Holds an overlay device entry
+ * @node:	list_head for the device list
+ * @np:		device node pointer to the device node affected
+ * @pdev:	pointer to the platform device affected
+ * @state:	new device state
+ *
+ * When the overlay results in a device node's state to change this
+ * fact is recorded in a list of device entries. After the overlay
+ * is applied we can create/destroy the platform devices according
+ * to the new state of the live tree.
+ */
+struct of_overlay_device_entry {
+	struct list_head node;
+	struct device_node *np;
+	struct platform_device *pdev;
+	int state;
+};
+
+/**
+ * struct of_overlay_info	- Holds a single overlay info
+ * @target:	target of the overlay operation
+ * @overlay:	pointer to the overlay contents node
+ * @lock:	Lock to hold when accessing the lists
+ * @le_list:	List of the overlay logs
+ * @de_list:	List of the overlay records
+ * @notifier:	of reconfiguration notifier
+ *
+ * Holds a single overlay state, including all the overlay logs &
+ * records.
+ */
+struct of_overlay_info {
+	struct device_node *target;
+	struct device_node *overlay;
+	struct mutex lock;
+	struct list_head le_list;
+	struct list_head de_list;
+	struct notifier_block notifier;
+};
+
+#ifdef CONFIG_OF_OVERLAY
+
+int of_overlay(int count, struct of_overlay_info *ovinfo_tab);
+int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab);
+
+int of_fill_overlay_info(struct device_node *info_node,
+		struct of_overlay_info *ovinfo);
+int of_build_overlay_info(struct device_node *tree,
+		int *cntp, struct of_overlay_info **ovinfop);
+int of_free_overlay_info(int cnt, struct of_overlay_info *ovinfo);
+
+#else
+
+static inline int of_overlay(int count, struct of_overlay_info *ovinfo_tab)
+{
+	return -ENOTSUPP;
+}
+
+static inline int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab)
+{
+	return -ENOTSUPP;
+}
+
+static inline int of_fill_overlay_info(struct device_node *info_node,
+		struct of_overlay_info *ovinfo)
+{
+	return -ENOTSUPP;
+}
+
+static inline int of_build_overlay_info(struct device_node *tree,
+		int *cntp, struct of_overlay_info **ovinfop)
+{
+	return -ENOTSUPP;
+}
+
+int of_free_overlay_info(int cnt, struct of_overlay_info *ovinfo)
+{
+	return -ENOTSUPP;
+}
+
+#endif
+
 #endif /* _LINUX_OF_H */
-- 
1.7.12

Re: [PATCH 6/6] OF: Introduce DT overlay support.

[Rob Landley]

On 01/04/2013 01:31:10 PM, Pantelis Antoniou wrote:
> Introduce DT overlay support.
> Using this functionality it is possible to dynamically overlay a part  
> of
> the kernel's tree with another tree that's been dynamically loaded.
> It is also possible to remove node and properties.
> 
> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>

Just commenting on the documentation a bit...

> ---
>  Documentation/devicetree/overlay-notes.txt | 179 +++++++
>  drivers/of/Kconfig                         |  10 +
>  drivers/of/Makefile                        |   1 +
>  drivers/of/overlay.c                       | 831  
> +++++++++++++++++++++++++++++
>  include/linux/of.h                         | 107 ++++
>  5 files changed, 1128 insertions(+)
>  create mode 100644 Documentation/devicetree/overlay-notes.txt
>  create mode 100644 drivers/of/overlay.c
> 
> diff --git a/Documentation/devicetree/overlay-notes.txt  
> b/Documentation/devicetree/overlay-notes.txt
> new file mode 100644
> index 0000000..5289cbb
> --- /dev/null
> +++ b/Documentation/devicetree/overlay-notes.txt
> @@ -0,0 +1,179 @@
> +Device Tree Overlay Notes
> +-------------------------
> +
> +This document describes the implementation of the in-kernel
> +device tree overlay functionality residing in drivers/of/overlay.c  
> and is a
> +companion document to  
> Documentation/devicetree/dt-object-internal.txt[1] &
> +Documentation/devicetree/dynamic-resolution-notes.txt[2]
> +
> +How overlays work
> +-----------------
> +
> +A Device Tree's overlay purpose is to modify the kernel's live tree,  
> and
> +have the modification affecting the state of the the kernel in a way  
> that
> +is reflecting the changes.

My wild guess here is this has something to do with hotplug support,  
but I don't know if modules are expected to do this or if userspace  
does it and modules respond... Could you give a couple sentences about  
the purpose and potential users of this mechanism in the summary?

> +Since the kernel mainly deals with devices, any new device node that  
> result

results

> +in an active device should have it created while if the device node  
> is either
> +disabled or removed all together, the affected device should be  
> deregistered.

I'm not following this bit. It looks like some test is missing between  
"while if"?

> +Lets take an example where we have a foo board with the following  
> base tree
> +which is taken from [1].
> +
> +---- foo.dts  
> -----------------------------------------------------------------
> +	/* FOO platform */
> +	/ {
> +		compatible = "corp,foo";
> +
> +		/* shared resources */
> +		res: res {
> +		};
> +
> +		/* On chip peripherals */
> +		ocp: ocp {
> +			/* peripherals that are always instantiated */
> +			peripheral1 { ... };
> +		}
> +	};
> +---- foo.dts  
> -----------------------------------------------------------------
> +
> +The overlay bar.dts, when loaded (and resolved as described in [2])  
> should
> +
> +---- bar.dts  
> -----------------------------------------------------------------
> +/plugin/;	/* allow undefined label references and record them */
> +/ {
> +	....	/* various properties for loader use; i.e. part id etc.  
> */
> +	fragment@0 {
> +		target = <&ocp>;
> +		__overlay__ {
> +			/* bar peripheral */
> +			bar {
> +				compatible = "corp,bar";
> +				... /* various properties and child  
> nodes */
> +			}
> +		};
> +	};
> +};
> +---- bar.dts  
> -----------------------------------------------------------------
> +
> +result in foo+bar.dts
> +
> +---- foo+bar.dts  
> -------------------------------------------------------------
> +	/* FOO platform + bar peripheral */
> +	/ {
> +		compatible = "corp,foo";
> +
> +		/* shared resources */
> +		res: res {
> +		};
> +
> +		/* On chip peripherals */
> +		ocp: ocp {
> +			/* peripherals that are always instantiated */
> +			peripheral1 { ... };
> +
> +			/* bar peripheral */
> +			bar {
> +				compatible = "corp,bar";
> +				... /* various properties and child  
> nodes */
> +			}
> +		}
> +	};
> +---- foo+bar.dts  
> -------------------------------------------------------------
> +
> +As a result of the the overlay, a new device node (bar) has been  
> created
> +so a bar platform device will be registered and if a matching device  
> driver
> +is loaded the device will be created as expected.

Is this done by a module, or does doing this then trigger a hotplug  
event that requests a module? (Or is this a syntax allowing a  
bootloader to collate multiple device tree segments and then Linux  
links them when parsing the device tree...?)

> +Overlay in-kernel API
> +---------------------
> +
> +The steps typically required to get an overlay to work are as  
> follows:
> +
> +1. Use of_build_overlay_info() to create an array of initialized and
> +ready to use of_overlay_info structures.
> +2. Call of_overlay() to apply the overlays declared in the array.
> +3. If the overlay needs to be removed, call of_overlay_revert().
> +4. Finally release the memory taken by the overlay info array by
> +of_free_overlay_info().
> +
> +/**
> + * of_build_overlay_info	- Build an overlay info array
> + * @tree:	Device node containing all the overlays
> + * @cntp:	Pointer to where the overlay info count will be help
> + * @ovinfop:	Pointer to the pointer of an overlay info structure.
> + *
> + * Helper function that given a tree containing overlay information,
> + * allocates and builds an overlay info array containing it, ready
> + * for use using of_overlay.
> + *
> + * Returns 0 on success with the @cntp @ovinfop pointers valid,
> + * while on error a negative error value is returned.
> + */
> +int of_build_overlay_info(struct device_node *tree,
> +		int *cntp, struct of_overlay_info **ovinfop);

Copying the htmldocs info blocks into a Documentation text file means  
you have to keep them in sync by hand. Possibly you want this  
documentation in a docbook template instead?

> +/**
> + * of_free_overlay_info	- Free an overlay info array
> + * @count:	Number of of_overlay_info's
> + * @ovinfo_tab:	Array of overlay_info's to free
> + *
> + * Releases the memory of a previously allocate ovinfo array
> + * by of_build_overlay_info.
> + * Returns 0, or an error if the arguments are bogus.
> + */
> +int of_free_overlay_info(int count, struct of_overlay_info  
> *ovinfo_tab);
> +
> +/**
> + * of_overlay	- Apply @count overlays pointed at by  
> @ovinfo_tab
> + * @count:	Number of of_overlay_info's
> + * @ovinfo_tab:	Array of overlay_info's to apply
> + *
> + * Applies the overlays given, while handling all error conditions
> + * appropriately. Either the operation succeeds, or if it fails the
> + * live tree is reverted to the state before the attempt.
> + * Returns 0, or an error if the overlay attempt failed.
> + */
> +int of_overlay(int count, struct of_overlay_info *ovinfo_tab);
> +
> +/**
> + * of_overlay_revert	- Revert a previously applied overlay
> + * @count:	Number of of_overlay_info's
> + * @ovinfo_tab:	Array of overlay_info's to apply
> + *
> + * Revert a previous overlay. The state of the live tree
> + * is reverted to the one before the overlay.
> + * Returns 0, or an error if the overlay table is not given.
> + */
> +int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab);
> +
> +Overlay DTS Format
> +------------------
> +
> +The DTS of an overlay should have the following format:
> +
> +{
> +	/* ignored properties by the overlay */
> +
> +	fragment@0 {	/* first child node */
> +		target=<phandle>;	/* target of the overlay */
> +		__overlay__ {
> +			property-a;	/* add property-a to the target  
> */
> +			-property-b;	/* remove property-b from  
> target */
> +			node-a {	/* add to an existing, or  
> create a node-a */
> +				...
> +			};
> +			-node-b {	/* remove an existing node-b */
> +				...
> +			};
> +		};
> +	}
> +	fragment@1 {	/* second child node */
> +		...
> +	};
> +	/* more fragments follow */
> +}
> +
> +It should be noted that the DT overlay format described is the one  
> expected
> +by the of_build_overlay_info() function, which is a helper function.  
> There
> +is nothing stopping someone coming up with his own DTS format and  
> that will
> +end up filling in the fields of the of_overlay_info array.
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index f9a6193..964a1c2 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -92,4 +92,14 @@ config OF_RESOLVE
>  	  Enable OF dynamic resolution support. This allows you to
>  	  load Device Tree object fragments are run time.
> 
> +config OF_OVERLAY
> +	bool "OF overlay support"
> +	depends on OF
> +	select OF_DYNAMIC
> +	select OF_DEVICE
> +	select OF_RESOLVE
> +	help
> +	  OpenFirmware overlay support. Allows you to modify on runtime  
> the
> +	  live tree using overlays.

"You" being... a module? The bootloader? A userspace program that just  
loaded some firmware? A udev hook responding to a hotplug event?

Rob

Re: [PATCH 6/6] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi Rob,

On Jan 6, 2013, at 4:24 AM, Rob Landley wrote:

> On 01/04/2013 01:31:10 PM, Pantelis Antoniou wrote:
>> Introduce DT overlay support.
>> Using this functionality it is possible to dynamically overlay a part of
>> the kernel's tree with another tree that's been dynamically loaded.
>> It is also possible to remove node and properties.
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> 
> Just commenting on the documentation a bit...
> 
>> ---
>> Documentation/devicetree/overlay-notes.txt | 179 +++++++
>> drivers/of/Kconfig                         |  10 +
>> drivers/of/Makefile                        |   1 +
>> drivers/of/overlay.c                       | 831 +++++++++++++++++++++++++++++
>> include/linux/of.h                         | 107 ++++
>> 5 files changed, 1128 insertions(+)
>> create mode 100644 Documentation/devicetree/overlay-notes.txt
>> create mode 100644 drivers/of/overlay.c
>> diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
>> new file mode 100644
>> index 0000000..5289cbb
>> --- /dev/null
>> +++ b/Documentation/devicetree/overlay-notes.txt
>> @@ -0,0 +1,179 @@
>> +Device Tree Overlay Notes
>> +-------------------------
>> +
>> +This document describes the implementation of the in-kernel
>> +device tree overlay functionality residing in drivers/of/overlay.c and is a
>> +companion document to Documentation/devicetree/dt-object-internal.txt[1] &
>> +Documentation/devicetree/dynamic-resolution-notes.txt[2]
>> +
>> +How overlays work
>> +-----------------
>> +
>> +A Device Tree's overlay purpose is to modify the kernel's live tree, and
>> +have the modification affecting the state of the the kernel in a way that
>> +is reflecting the changes.
> 
> My wild guess here is this has something to do with hotplug support, but I don't know if modules are expected to do this or if userspace does it and modules respond... Could you give a couple sentences about the purpose and potential users of this mechanism in the summary?
> 

In a nutshell, the idea is that you have an unspecified method of detecting some kind of hardware
interface(s) and you want to insert to the live tree the fragment that will enable your hardware
to function.

Which method to use is up to what comes naturally for each case.

>> +Since the kernel mainly deals with devices, any new device node that result
> 
> results
> 

OK

>> +in an active device should have it created while if the device node is either
>> +disabled or removed all together, the affected device should be deregistered.
> 
> I'm not following this bit. It looks like some test is missing between "while if"?
> 

I'll reword it to make it clearer.

>> +Lets take an example where we have a foo board with the following base tree
>> +which is taken from [1].
>> +
>> +---- foo.dts -----------------------------------------------------------------
>> +	/* FOO platform */
>> +	/ {
>> +		compatible = "corp,foo";
>> +
>> +		/* shared resources */
>> +		res: res {
>> +		};
>> +
>> +		/* On chip peripherals */
>> +		ocp: ocp {
>> +			/* peripherals that are always instantiated */
>> +			peripheral1 { ... };
>> +		}
>> +	};
>> +---- foo.dts -----------------------------------------------------------------
>> +
>> +The overlay bar.dts, when loaded (and resolved as described in [2]) should
>> +
>> +---- bar.dts -----------------------------------------------------------------
>> +/plugin/;	/* allow undefined label references and record them */
>> +/ {
>> +	....	/* various properties for loader use; i.e. part id etc. */
>> +	fragment@0 {
>> +		target = <&ocp>;
>> +		__overlay__ {
>> +			/* bar peripheral */
>> +			bar {
>> +				compatible = "corp,bar";
>> +				... /* various properties and child nodes */
>> +			}
>> +		};
>> +	};
>> +};
>> +---- bar.dts -----------------------------------------------------------------
>> +
>> +result in foo+bar.dts
>> +
>> +---- foo+bar.dts -------------------------------------------------------------
>> +	/* FOO platform + bar peripheral */
>> +	/ {
>> +		compatible = "corp,foo";
>> +
>> +		/* shared resources */
>> +		res: res {
>> +		};
>> +
>> +		/* On chip peripherals */
>> +		ocp: ocp {
>> +			/* peripherals that are always instantiated */
>> +			peripheral1 { ... };
>> +
>> +			/* bar peripheral */
>> +			bar {
>> +				compatible = "corp,bar";
>> +				... /* various properties and child nodes */
>> +			}
>> +		}
>> +	};
>> +---- foo+bar.dts -------------------------------------------------------------
>> +
>> +As a result of the the overlay, a new device node (bar) has been created
>> +so a bar platform device will be registered and if a matching device driver
>> +is loaded the device will be created as expected.
> 
> Is this done by a module, or does doing this then trigger a hotplug event that requests a module? (Or is this a syntax allowing a bootloader to collate multiple device tree segments and then Linux links them when parsing the device tree...?)

This is the general way to support all of the above. The existing implementation
is by a board specific loader, which I will post shortly.

> 
>> +Overlay in-kernel API
>> +---------------------
>> +
>> +The steps typically required to get an overlay to work are as follows:
>> +
>> +1. Use of_build_overlay_info() to create an array of initialized and
>> +ready to use of_overlay_info structures.
>> +2. Call of_overlay() to apply the overlays declared in the array.
>> +3. If the overlay needs to be removed, call of_overlay_revert().
>> +4. Finally release the memory taken by the overlay info array by
>> +of_free_overlay_info().
>> +
>> +/**
>> + * of_build_overlay_info	- Build an overlay info array
>> + * @tree:	Device node containing all the overlays
>> + * @cntp:	Pointer to where the overlay info count will be help
>> + * @ovinfop:	Pointer to the pointer of an overlay info structure.
>> + *
>> + * Helper function that given a tree containing overlay information,
>> + * allocates and builds an overlay info array containing it, ready
>> + * for use using of_overlay.
>> + *
>> + * Returns 0 on success with the @cntp @ovinfop pointers valid,
>> + * while on error a negative error value is returned.
>> + */
>> +int of_build_overlay_info(struct device_node *tree,
>> +		int *cntp, struct of_overlay_info **ovinfop);
> 
> Copying the htmldocs info blocks into a Documentation text file means you have to keep them in sync by hand. Possibly you want this documentation in a docbook template instead?

At this point in time, there's no such thing, since the rest of the code in that
area doesn't do it. I know it's a chore to keep them in sync by hand, but if you go 
to the trouble of commenting, I thought I might as well make it in a form that can
be used later.

> 
>> +/**
>> + * of_free_overlay_info	- Free an overlay info array
>> + * @count:	Number of of_overlay_info's
>> + * @ovinfo_tab:	Array of overlay_info's to free
>> + *
>> + * Releases the memory of a previously allocate ovinfo array
>> + * by of_build_overlay_info.
>> + * Returns 0, or an error if the arguments are bogus.
>> + */
>> +int of_free_overlay_info(int count, struct of_overlay_info *ovinfo_tab);
>> +
>> +/**
>> + * of_overlay	- Apply @count overlays pointed at by @ovinfo_tab
>> + * @count:	Number of of_overlay_info's
>> + * @ovinfo_tab:	Array of overlay_info's to apply
>> + *
>> + * Applies the overlays given, while handling all error conditions
>> + * appropriately. Either the operation succeeds, or if it fails the
>> + * live tree is reverted to the state before the attempt.
>> + * Returns 0, or an error if the overlay attempt failed.
>> + */
>> +int of_overlay(int count, struct of_overlay_info *ovinfo_tab);
>> +
>> +/**
>> + * of_overlay_revert	- Revert a previously applied overlay
>> + * @count:	Number of of_overlay_info's
>> + * @ovinfo_tab:	Array of overlay_info's to apply
>> + *
>> + * Revert a previous overlay. The state of the live tree
>> + * is reverted to the one before the overlay.
>> + * Returns 0, or an error if the overlay table is not given.
>> + */
>> +int of_overlay_revert(int count, struct of_overlay_info *ovinfo_tab);
>> +
>> +Overlay DTS Format
>> +------------------
>> +
>> +The DTS of an overlay should have the following format:
>> +
>> +{
>> +	/* ignored properties by the overlay */
>> +
>> +	fragment@0 {	/* first child node */
>> +		target=<phandle>;	/* target of the overlay */
>> +		__overlay__ {
>> +			property-a;	/* add property-a to the target */
>> +			-property-b;	/* remove property-b from target */
>> +			node-a {	/* add to an existing, or create a node-a */
>> +				...
>> +			};
>> +			-node-b {	/* remove an existing node-b */
>> +				...
>> +			};
>> +		};
>> +	}
>> +	fragment@1 {	/* second child node */
>> +		...
>> +	};
>> +	/* more fragments follow */
>> +}
>> +
>> +It should be noted that the DT overlay format described is the one expected
>> +by the of_build_overlay_info() function, which is a helper function. There
>> +is nothing stopping someone coming up with his own DTS format and that will
>> +end up filling in the fields of the of_overlay_info array.
>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>> index f9a6193..964a1c2 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -92,4 +92,14 @@ config OF_RESOLVE
>> 	  Enable OF dynamic resolution support. This allows you to
>> 	  load Device Tree object fragments are run time.
>> +config OF_OVERLAY
>> +	bool "OF overlay support"
>> +	depends on OF
>> +	select OF_DYNAMIC
>> +	select OF_DEVICE
>> +	select OF_RESOLVE
>> +	help
>> +	  OpenFirmware overlay support. Allows you to modify on runtime the
>> +	  live tree using overlays.
> 
> "You" being... a module? The bootloader? A userspace program that just loaded some firmware? A udev hook responding to a hotplug event?
> 

A specific patchset which uses all of this will be forthcoming.

As I said earlier, this is the infrastructure code than allows all of this.

> Rob

Regards

-- Pantelis

Re: [PATCH 6/6] OF: Introduce DT overlay support.

[David Gibson]

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

On Fri, Jan 04, 2013 at 09:31:10PM +0200, Pantelis Antoniou wrote:
> Introduce DT overlay support.
> Using this functionality it is possible to dynamically overlay a part of
> the kernel's tree with another tree that's been dynamically loaded.
> It is also possible to remove node and properties.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  Documentation/devicetree/overlay-notes.txt | 179 +++++++
>  drivers/of/Kconfig                         |  10 +
>  drivers/of/Makefile                        |   1 +
>  drivers/of/overlay.c                       | 831 +++++++++++++++++++++++++++++
>  include/linux/of.h                         | 107 ++++
>  5 files changed, 1128 insertions(+)
>  create mode 100644 Documentation/devicetree/overlay-notes.txt
>  create mode 100644 drivers/of/overlay.c
> 
> diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
> new file mode 100644
> index 0000000..5289cbb
> --- /dev/null
> +++ b/Documentation/devicetree/overlay-notes.txt
> @@ -0,0 +1,179 @@
> +Device Tree Overlay Notes
> +-------------------------
> +
> +This document describes the implementation of the in-kernel
> +device tree overlay functionality residing in drivers/of/overlay.c and is a
> +companion document to Documentation/devicetree/dt-object-internal.txt[1] &
> +Documentation/devicetree/dynamic-resolution-notes.txt[2]
> +
> +How overlays work
> +-----------------
> +
> +A Device Tree's overlay purpose is to modify the kernel's live tree, and
> +have the modification affecting the state of the the kernel in a way that
> +is reflecting the changes.

Um.. I'm having a great deal of trouble parsing that sentence.

> +Since the kernel mainly deals with devices, any new device node that result
> +in an active device should have it created while if the device node is either
> +disabled or removed all together, the affected device should be deregistered.
> +
> +Lets take an example where we have a foo board with the following base tree
> +which is taken from [1].
> +
> +---- foo.dts -----------------------------------------------------------------
> +	/* FOO platform */
> +	/ {
> +		compatible = "corp,foo";
> +
> +		/* shared resources */
> +		res: res {
> +		};
> +
> +		/* On chip peripherals */
> +		ocp: ocp {
> +			/* peripherals that are always instantiated */
> +			peripheral1 { ... };
> +		}
> +	};
> +---- foo.dts -----------------------------------------------------------------
> +
> +The overlay bar.dts, when loaded (and resolved as described in [2]) should
> +
> +---- bar.dts -----------------------------------------------------------------
> +/plugin/;	/* allow undefined label references and record them */
> +/ {
> +	....	/* various properties for loader use; i.e. part id etc. */
> +	fragment@0 {
> +		target = <&ocp>;
> +		__overlay__ {
> +			/* bar peripheral */
> +			bar {
> +				compatible = "corp,bar";
> +				... /* various properties and child nodes */
> +			}
> +		};
> +	};
> +};
> +---- bar.dts -----------------------------------------------------------------
> +
> +result in foo+bar.dts
> +
> +---- foo+bar.dts -------------------------------------------------------------
> +	/* FOO platform + bar peripheral */
> +	/ {
> +		compatible = "corp,foo";
> +
> +		/* shared resources */
> +		res: res {
> +		};
> +
> +		/* On chip peripherals */
> +		ocp: ocp {
> +			/* peripherals that are always instantiated */
> +			peripheral1 { ... };
> +
> +			/* bar peripheral */
> +			bar {
> +				compatible = "corp,bar";
> +				... /* various properties and child nodes */
> +			}
> +		}
> +	};
> +---- foo+bar.dts -------------------------------------------------------------
> +
> +As a result of the the overlay, a new device node (bar) has been created
> +so a bar platform device will be registered and if a matching device driver
> +is loaded the device will be created as expected.

Hrm.  This all seems rather complicated.  Maybe it needs to be, but
I'm not entirely convinced yet.

One other point - both of these patches are assuming that the overlay
is in the "live tree" format, but it still needs a bunch of extra
mangling.  Would it simplify things to just go straight from the
overlay in flat tree form to modifications to the system-wide live
tree.

-- 
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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 6/6] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Hi

On Jan 22, 2013, at 5:50 AM, David Gibson wrote:

> On Fri, Jan 04, 2013 at 09:31:10PM +0200, Pantelis Antoniou wrote:
>> Introduce DT overlay support.
>> Using this functionality it is possible to dynamically overlay a part of
>> the kernel's tree with another tree that's been dynamically loaded.
>> It is also possible to remove node and properties.
>> 
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>> ---
>> Documentation/devicetree/overlay-notes.txt | 179 +++++++
>> drivers/of/Kconfig                         |  10 +
>> drivers/of/Makefile                        |   1 +
>> drivers/of/overlay.c                       | 831 +++++++++++++++++++++++++++++
>> include/linux/of.h                         | 107 ++++
>> 5 files changed, 1128 insertions(+)
>> create mode 100644 Documentation/devicetree/overlay-notes.txt
>> create mode 100644 drivers/of/overlay.c
>> 
>> diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
>> new file mode 100644
>> index 0000000..5289cbb
>> --- /dev/null
>> +++ b/Documentation/devicetree/overlay-notes.txt
>> @@ -0,0 +1,179 @@
>> +Device Tree Overlay Notes
>> +-------------------------
>> +
>> +This document describes the implementation of the in-kernel
>> +device tree overlay functionality residing in drivers/of/overlay.c and is a
>> +companion document to Documentation/devicetree/dt-object-internal.txt[1] &
>> +Documentation/devicetree/dynamic-resolution-notes.txt[2]
>> +
>> +How overlays work
>> +-----------------
>> +
>> +A Device Tree's overlay purpose is to modify the kernel's live tree, and
>> +have the modification affecting the state of the the kernel in a way that
>> +is reflecting the changes.
> 
> Um.. I'm having a great deal of trouble parsing that sentence.
> 
>> +Since the kernel mainly deals with devices, any new device node that result
>> +in an active device should have it created while if the device node is either
>> +disabled or removed all together, the affected device should be deregistered.
>> +
>> +Lets take an example where we have a foo board with the following base tree
>> +which is taken from [1].
>> +
>> +---- foo.dts -----------------------------------------------------------------
>> +	/* FOO platform */
>> +	/ {
>> +		compatible = "corp,foo";
>> +
>> +		/* shared resources */
>> +		res: res {
>> +		};
>> +
>> +		/* On chip peripherals */
>> +		ocp: ocp {
>> +			/* peripherals that are always instantiated */
>> +			peripheral1 { ... };
>> +		}
>> +	};
>> +---- foo.dts -----------------------------------------------------------------
>> +
>> +The overlay bar.dts, when loaded (and resolved as described in [2]) should
>> +
>> +---- bar.dts -----------------------------------------------------------------
>> +/plugin/;	/* allow undefined label references and record them */
>> +/ {
>> +	....	/* various properties for loader use; i.e. part id etc. */
>> +	fragment@0 {
>> +		target = <&ocp>;
>> +		__overlay__ {
>> +			/* bar peripheral */
>> +			bar {
>> +				compatible = "corp,bar";
>> +				... /* various properties and child nodes */
>> +			}
>> +		};
>> +	};
>> +};
>> +---- bar.dts -----------------------------------------------------------------
>> +
>> +result in foo+bar.dts
>> +
>> +---- foo+bar.dts -------------------------------------------------------------
>> +	/* FOO platform + bar peripheral */
>> +	/ {
>> +		compatible = "corp,foo";
>> +
>> +		/* shared resources */
>> +		res: res {
>> +		};
>> +
>> +		/* On chip peripherals */
>> +		ocp: ocp {
>> +			/* peripherals that are always instantiated */
>> +			peripheral1 { ... };
>> +
>> +			/* bar peripheral */
>> +			bar {
>> +				compatible = "corp,bar";
>> +				... /* various properties and child nodes */
>> +			}
>> +		}
>> +	};
>> +---- foo+bar.dts -------------------------------------------------------------
>> +
>> +As a result of the the overlay, a new device node (bar) has been created
>> +so a bar platform device will be registered and if a matching device driver
>> +is loaded the device will be created as expected.
> 
> Hrm.  This all seems rather complicated.  Maybe it needs to be, but
> I'm not entirely convinced yet.
> 
> One other point - both of these patches are assuming that the overlay
> is in the "live tree" format, but it still needs a bunch of extra
> mangling.  Would it simplify things to just go straight from the
> overlay in flat tree form to modifications to the system-wide live
> tree.

Sorry, I can't parse this. You mean apply the overlay without converting
to live tree format?

> 
> -- 
> 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: [PATCH 6/6] OF: Introduce DT overlay support.

[David Gibson]

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

On Tue, Jan 22, 2013 at 01:08:04PM +0200, Pantelis Antoniou wrote:
> Hi
> 
> On Jan 22, 2013, at 5:50 AM, David Gibson wrote:
> 
> > On Fri, Jan 04, 2013 at 09:31:10PM +0200, Pantelis Antoniou wrote:
> >> Introduce DT overlay support.
> >> Using this functionality it is possible to dynamically overlay a part of
> >> the kernel's tree with another tree that's been dynamically loaded.
> >> It is also possible to remove node and properties.
> >> 
> >> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> >> ---
> >> Documentation/devicetree/overlay-notes.txt | 179 +++++++
> >> drivers/of/Kconfig                         |  10 +
> >> drivers/of/Makefile                        |   1 +
> >> drivers/of/overlay.c                       | 831 +++++++++++++++++++++++++++++
> >> include/linux/of.h                         | 107 ++++
> >> 5 files changed, 1128 insertions(+)
> >> create mode 100644 Documentation/devicetree/overlay-notes.txt
> >> create mode 100644 drivers/of/overlay.c
> >> 
> >> diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
> >> new file mode 100644
> >> index 0000000..5289cbb
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/overlay-notes.txt
> >> @@ -0,0 +1,179 @@
> >> +Device Tree Overlay Notes
> >> +-------------------------
> >> +
> >> +This document describes the implementation of the in-kernel
> >> +device tree overlay functionality residing in drivers/of/overlay.c and is a
> >> +companion document to Documentation/devicetree/dt-object-internal.txt[1] &
> >> +Documentation/devicetree/dynamic-resolution-notes.txt[2]
> >> +
> >> +How overlays work
> >> +-----------------
> >> +
> >> +A Device Tree's overlay purpose is to modify the kernel's live tree, and
> >> +have the modification affecting the state of the the kernel in a way that
> >> +is reflecting the changes.
> > 
> > Um.. I'm having a great deal of trouble parsing that sentence.
> > 
> >> +Since the kernel mainly deals with devices, any new device node that result
> >> +in an active device should have it created while if the device node is either
> >> +disabled or removed all together, the affected device should be deregistered.
> >> +
> >> +Lets take an example where we have a foo board with the following base tree
> >> +which is taken from [1].
> >> +
> >> +---- foo.dts -----------------------------------------------------------------
> >> +	/* FOO platform */
> >> +	/ {
> >> +		compatible = "corp,foo";
> >> +
> >> +		/* shared resources */
> >> +		res: res {
> >> +		};
> >> +
> >> +		/* On chip peripherals */
> >> +		ocp: ocp {
> >> +			/* peripherals that are always instantiated */
> >> +			peripheral1 { ... };
> >> +		}
> >> +	};
> >> +---- foo.dts -----------------------------------------------------------------
> >> +
> >> +The overlay bar.dts, when loaded (and resolved as described in [2]) should
> >> +
> >> +---- bar.dts -----------------------------------------------------------------
> >> +/plugin/;	/* allow undefined label references and record them */
> >> +/ {
> >> +	....	/* various properties for loader use; i.e. part id etc. */
> >> +	fragment@0 {
> >> +		target = <&ocp>;
> >> +		__overlay__ {
> >> +			/* bar peripheral */
> >> +			bar {
> >> +				compatible = "corp,bar";
> >> +				... /* various properties and child nodes */
> >> +			}
> >> +		};
> >> +	};
> >> +};
> >> +---- bar.dts -----------------------------------------------------------------
> >> +
> >> +result in foo+bar.dts
> >> +
> >> +---- foo+bar.dts -------------------------------------------------------------
> >> +	/* FOO platform + bar peripheral */
> >> +	/ {
> >> +		compatible = "corp,foo";
> >> +
> >> +		/* shared resources */
> >> +		res: res {
> >> +		};
> >> +
> >> +		/* On chip peripherals */
> >> +		ocp: ocp {
> >> +			/* peripherals that are always instantiated */
> >> +			peripheral1 { ... };
> >> +
> >> +			/* bar peripheral */
> >> +			bar {
> >> +				compatible = "corp,bar";
> >> +				... /* various properties and child nodes */
> >> +			}
> >> +		}
> >> +	};
> >> +---- foo+bar.dts -------------------------------------------------------------
> >> +
> >> +As a result of the the overlay, a new device node (bar) has been created
> >> +so a bar platform device will be registered and if a matching device driver
> >> +is loaded the device will be created as expected.
> > 
> > Hrm.  This all seems rather complicated.  Maybe it needs to be, but
> > I'm not entirely convinced yet.
> > 
> > One other point - both of these patches are assuming that the overlay
> > is in the "live tree" format, but it still needs a bunch of extra
> > mangling.  Would it simplify things to just go straight from the
> > overlay in flat tree form to modifications to the system-wide live
> > tree.
> 
> Sorry, I can't parse this. You mean apply the overlay without converting
> to live tree format?

Yes.

-- 
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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 6/6] OF: Introduce DT overlay support.

[Pantelis Antoniou]

On Jan 23, 2013, at 7:12 AM, David Gibson wrote:

> On Tue, Jan 22, 2013 at 01:08:04PM +0200, Pantelis Antoniou wrote:
>> Hi
>> 
>> On Jan 22, 2013, at 5:50 AM, David Gibson wrote:
>> 
>>> On Fri, Jan 04, 2013 at 09:31:10PM +0200, Pantelis Antoniou wrote:
>>>> Introduce DT overlay support.
>>>> Using this functionality it is possible to dynamically overlay a part of
>>>> the kernel's tree with another tree that's been dynamically loaded.
>>>> It is also possible to remove node and properties.
>>>> 
>>>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>>>> ---
>>>> Documentation/devicetree/overlay-notes.txt | 179 +++++++
>>>> drivers/of/Kconfig                         |  10 +
>>>> drivers/of/Makefile                        |   1 +
>>>> drivers/of/overlay.c                       | 831 +++++++++++++++++++++++++++++
>>>> include/linux/of.h                         | 107 ++++
>>>> 5 files changed, 1128 insertions(+)
>>>> create mode 100644 Documentation/devicetree/overlay-notes.txt
>>>> create mode 100644 drivers/of/overlay.c
>>>> 
>>>> diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
>>>> new file mode 100644
>>>> index 0000000..5289cbb
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/overlay-notes.txt
>>>> @@ -0,0 +1,179 @@
>>>> +Device Tree Overlay Notes
>>>> +-------------------------
>>>> +
>>>> +This document describes the implementation of the in-kernel
>>>> +device tree overlay functionality residing in drivers/of/overlay.c and is a
>>>> +companion document to Documentation/devicetree/dt-object-internal.txt[1] &
>>>> +Documentation/devicetree/dynamic-resolution-notes.txt[2]
>>>> +
>>>> +How overlays work
>>>> +-----------------
>>>> +
>>>> +A Device Tree's overlay purpose is to modify the kernel's live tree, and
>>>> +have the modification affecting the state of the the kernel in a way that
>>>> +is reflecting the changes.
>>> 
>>> Um.. I'm having a great deal of trouble parsing that sentence.
>>> 
>>>> +Since the kernel mainly deals with devices, any new device node that result
>>>> +in an active device should have it created while if the device node is either
>>>> +disabled or removed all together, the affected device should be deregistered.
>>>> +
>>>> +Lets take an example where we have a foo board with the following base tree
>>>> +which is taken from [1].
>>>> +
>>>> +---- foo.dts -----------------------------------------------------------------
>>>> +	/* FOO platform */
>>>> +	/ {
>>>> +		compatible = "corp,foo";
>>>> +
>>>> +		/* shared resources */
>>>> +		res: res {
>>>> +		};
>>>> +
>>>> +		/* On chip peripherals */
>>>> +		ocp: ocp {
>>>> +			/* peripherals that are always instantiated */
>>>> +			peripheral1 { ... };
>>>> +		}
>>>> +	};
>>>> +---- foo.dts -----------------------------------------------------------------
>>>> +
>>>> +The overlay bar.dts, when loaded (and resolved as described in [2]) should
>>>> +
>>>> +---- bar.dts -----------------------------------------------------------------
>>>> +/plugin/;	/* allow undefined label references and record them */
>>>> +/ {
>>>> +	....	/* various properties for loader use; i.e. part id etc. */
>>>> +	fragment@0 {
>>>> +		target = <&ocp>;
>>>> +		__overlay__ {
>>>> +			/* bar peripheral */
>>>> +			bar {
>>>> +				compatible = "corp,bar";
>>>> +				... /* various properties and child nodes */
>>>> +			}
>>>> +		};
>>>> +	};
>>>> +};
>>>> +---- bar.dts -----------------------------------------------------------------
>>>> +
>>>> +result in foo+bar.dts
>>>> +
>>>> +---- foo+bar.dts -------------------------------------------------------------
>>>> +	/* FOO platform + bar peripheral */
>>>> +	/ {
>>>> +		compatible = "corp,foo";
>>>> +
>>>> +		/* shared resources */
>>>> +		res: res {
>>>> +		};
>>>> +
>>>> +		/* On chip peripherals */
>>>> +		ocp: ocp {
>>>> +			/* peripherals that are always instantiated */
>>>> +			peripheral1 { ... };
>>>> +
>>>> +			/* bar peripheral */
>>>> +			bar {
>>>> +				compatible = "corp,bar";
>>>> +				... /* various properties and child nodes */
>>>> +			}
>>>> +		}
>>>> +	};
>>>> +---- foo+bar.dts -------------------------------------------------------------
>>>> +
>>>> +As a result of the the overlay, a new device node (bar) has been created
>>>> +so a bar platform device will be registered and if a matching device driver
>>>> +is loaded the device will be created as expected.
>>> 
>>> Hrm.  This all seems rather complicated.  Maybe it needs to be, but
>>> I'm not entirely convinced yet.
>>> 
>>> One other point - both of these patches are assuming that the overlay
>>> is in the "live tree" format, but it still needs a bunch of extra
>>> mangling.  Would it simplify things to just go straight from the
>>> overlay in flat tree form to modifications to the system-wide live
>>> tree.
>> 
>> Sorry, I can't parse this. You mean apply the overlay without converting
>> to live tree format?
> 
> Yes.
> 

The gymnastics required when operating on the flat tree will make grown, tough as nails
s/w developers cry.

In essence you will have to replicate the unflattening functionality again, create a
similar tree structure as the live tree, do your work, and then discard it.

A bit excessive don't you think?

 
> -- 
> 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: [PATCH 6/6] OF: Introduce DT overlay support.

[David Gibson]

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

On Wed, Jan 23, 2013 at 01:01:58PM +0200, Pantelis Antoniou wrote:
> On Jan 23, 2013, at 7:12 AM, David Gibson wrote:
> > On Tue, Jan 22, 2013 at 01:08:04PM +0200, Pantelis Antoniou wrote:
> >> Hi
> >> 
> >> On Jan 22, 2013, at 5:50 AM, David Gibson wrote:
> >> 
> >>> On Fri, Jan 04, 2013 at 09:31:10PM +0200, Pantelis Antoniou wrote:
> >>>> Introduce DT overlay support.
> >>>> Using this functionality it is possible to dynamically overlay a part of
> >>>> the kernel's tree with another tree that's been dynamically loaded.
> >>>> It is also possible to remove node and properties.
> >>>> 
> >>>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> >>>> ---
> >>>> Documentation/devicetree/overlay-notes.txt | 179 +++++++
> >>>> drivers/of/Kconfig                         |  10 +
> >>>> drivers/of/Makefile                        |   1 +
> >>>> drivers/of/overlay.c                       | 831 +++++++++++++++++++++++++++++
> >>>> include/linux/of.h                         | 107 ++++
> >>>> 5 files changed, 1128 insertions(+)
> >>>> create mode 100644 Documentation/devicetree/overlay-notes.txt
> >>>> create mode 100644 drivers/of/overlay.c
> >>>> 
> >>>> diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
> >>>> new file mode 100644
> >>>> index 0000000..5289cbb
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/overlay-notes.txt
> >>>> @@ -0,0 +1,179 @@
> >>>> +Device Tree Overlay Notes
> >>>> +-------------------------
> >>>> +
> >>>> +This document describes the implementation of the in-kernel
> >>>> +device tree overlay functionality residing in drivers/of/overlay.c and is a
> >>>> +companion document to Documentation/devicetree/dt-object-internal.txt[1] &
> >>>> +Documentation/devicetree/dynamic-resolution-notes.txt[2]
> >>>> +
> >>>> +How overlays work
> >>>> +-----------------
> >>>> +
> >>>> +A Device Tree's overlay purpose is to modify the kernel's live tree, and
> >>>> +have the modification affecting the state of the the kernel in a way that
> >>>> +is reflecting the changes.
> >>> 
> >>> Um.. I'm having a great deal of trouble parsing that sentence.
> >>> 
> >>>> +Since the kernel mainly deals with devices, any new device node that result
> >>>> +in an active device should have it created while if the device node is either
> >>>> +disabled or removed all together, the affected device should be deregistered.
> >>>> +
> >>>> +Lets take an example where we have a foo board with the following base tree
> >>>> +which is taken from [1].
> >>>> +
> >>>> +---- foo.dts -----------------------------------------------------------------
> >>>> +	/* FOO platform */
> >>>> +	/ {
> >>>> +		compatible = "corp,foo";
> >>>> +
> >>>> +		/* shared resources */
> >>>> +		res: res {
> >>>> +		};
> >>>> +
> >>>> +		/* On chip peripherals */
> >>>> +		ocp: ocp {
> >>>> +			/* peripherals that are always instantiated */
> >>>> +			peripheral1 { ... };
> >>>> +		}
> >>>> +	};
> >>>> +---- foo.dts -----------------------------------------------------------------
> >>>> +
> >>>> +The overlay bar.dts, when loaded (and resolved as described in [2]) should
> >>>> +
> >>>> +---- bar.dts -----------------------------------------------------------------
> >>>> +/plugin/;	/* allow undefined label references and record them */
> >>>> +/ {
> >>>> +	....	/* various properties for loader use; i.e. part id etc. */
> >>>> +	fragment@0 {
> >>>> +		target = <&ocp>;
> >>>> +		__overlay__ {
> >>>> +			/* bar peripheral */
> >>>> +			bar {
> >>>> +				compatible = "corp,bar";
> >>>> +				... /* various properties and child nodes */
> >>>> +			}
> >>>> +		};
> >>>> +	};
> >>>> +};
> >>>> +---- bar.dts -----------------------------------------------------------------
> >>>> +
> >>>> +result in foo+bar.dts
> >>>> +
> >>>> +---- foo+bar.dts -------------------------------------------------------------
> >>>> +	/* FOO platform + bar peripheral */
> >>>> +	/ {
> >>>> +		compatible = "corp,foo";
> >>>> +
> >>>> +		/* shared resources */
> >>>> +		res: res {
> >>>> +		};
> >>>> +
> >>>> +		/* On chip peripherals */
> >>>> +		ocp: ocp {
> >>>> +			/* peripherals that are always instantiated */
> >>>> +			peripheral1 { ... };
> >>>> +
> >>>> +			/* bar peripheral */
> >>>> +			bar {
> >>>> +				compatible = "corp,bar";
> >>>> +				... /* various properties and child nodes */
> >>>> +			}
> >>>> +		}
> >>>> +	};
> >>>> +---- foo+bar.dts -------------------------------------------------------------
> >>>> +
> >>>> +As a result of the the overlay, a new device node (bar) has been created
> >>>> +so a bar platform device will be registered and if a matching device driver
> >>>> +is loaded the device will be created as expected.
> >>> 
> >>> Hrm.  This all seems rather complicated.  Maybe it needs to be, but
> >>> I'm not entirely convinced yet.
> >>> 
> >>> One other point - both of these patches are assuming that the overlay
> >>> is in the "live tree" format, but it still needs a bunch of extra
> >>> mangling.  Would it simplify things to just go straight from the
> >>> overlay in flat tree form to modifications to the system-wide live
> >>> tree.
> >> 
> >> Sorry, I can't parse this. You mean apply the overlay without converting
> >> to live tree format?
> > 
> > Yes.
> > 
> 
> The gymnastics required when operating on the flat tree will make grown, tough as nails
> s/w developers cry.

It's really not that bad.  In particular it's only tricky if you want
to make alterations to the flat tree, which you shouldn't need to
here.

> In essence you will have to replicate the unflattening functionality again, create a
> similar tree structure as the live tree, do your work, and then
> discard it.
> 
> A bit excessive don't you think?

It would be, if you actually needed to do that.  But what you can
actually do is:
	1. parse the information you need out of the base tree
	2. use the flat tree functions to retrieve the
__local_fixups__ and __fixups__ information from the overlay, parsing
just those into a temporary data structure
	3. Perform a single pass over the flat tree overlay, inserting
each node and and property into the live tree, applying fixups as you
go.

-- 
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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/6] Introducing Device Tree Overlays

[Richard Cochran]

On Fri, Jan 04, 2013 at 09:31:04PM +0200, Pantelis Antoniou wrote:
> The following patchset introduces Device Tree overlays, a method
> of dynamically altering the kernel's live Device Tree.

It would be nice to know the motivation for this code.

What is the use case? What problem or issue is being addressed?

Thanks,
Richard

Re: [PATCH 0/6] Introducing Device Tree Overlays

[Joel A Fernandes]

Hi Richard,

On Fri, Jan 4, 2013 at 9:35 PM, Richard Cochran
<richardcochran-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Jan 04, 2013 at 09:31:04PM +0200, Pantelis Antoniou wrote:
>> The following patchset introduces Device Tree overlays, a method
>> of dynamically altering the kernel's live Device Tree.
>
> It would be nice to know the motivation for this code.
>
> What is the use case? What problem or issue is being addressed?

The problem being addressed is discussed in this thread:
http://permalink.gmane.org/gmane.linux.kernel/1389017

Regards,
Joel

Re: [PATCH 0/6] Introducing Device Tree Overlays

[Richard Cochran]

On Sat, Jan 05, 2013 at 12:16:51AM -0600, Joel A Fernandes wrote:
> 
> The problem being addressed is discussed in this thread:
> http://permalink.gmane.org/gmane.linux.kernel/1389017

Thanks for the link.

Since the motivation is already documented in that post, why not add
it into Documentation/devicetree/overlay-notes.txt as well?

Thanks,
Richard

Re: [PATCH 0/6] Introducing Device Tree Overlays

[Pantelis Antoniou]

Hi Richard,

It should, but there are a few differences in the syntax plus the fact that I didn't write it.
It should go in by a separate patch by the original author. 

Regards

-- Pantelis

Από το iPad μου

5 Ιαν 2013, 11:35 π.μ., ο/η Richard Cochran <richardcochran@gmail.com> έγραψε:

> On Sat, Jan 05, 2013 at 12:16:51AM -0600, Joel A Fernandes wrote:
>> 
>> The problem being addressed is discussed in this thread:
>> http://permalink.gmane.org/gmane.linux.kernel/1389017
> 
> Thanks for the link.
> 
> Since the motivation is already documented in that post, why not add
> it into Documentation/devicetree/overlay-notes.txt as well?
> 
> Thanks,
> Richard
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 0/6] Introducing Device Tree Overlays

[Rob Landley]

On 01/05/2013 03:35:58 AM, Richard Cochran wrote:
> On Sat, Jan 05, 2013 at 12:16:51AM -0600, Joel A Fernandes wrote:
> >
> > The problem being addressed is discussed in this thread:
> > http://permalink.gmane.org/gmane.linux.kernel/1389017
> 
> Thanks for the link.
> 
> Since the motivation is already documented in that post, why not add
> it into Documentation/devicetree/overlay-notes.txt as well?

Seconded.

Rob

Re: [PATCH 0/6] Introducing Device Tree Overlays

[delicious quinoa]

I like where this is heading.  I'm interested in a use case where IP
can be loaded into a FPGA, then add a blob to the device tree and load
some drivers.

I see your github tree.  If I wanted to cherry-pick your code and play
around with it, which branch should I use?  not-capebus-21?

Thanks,
Alan Tull
Altera Corp

On Fri, Jan 4, 2013 at 1:31 PM, Pantelis Antoniou
<panto@antoniou-consulting.com> wrote:
> The following patchset introduces Device Tree overlays, a method
> of dynamically altering the kernel's live Device Tree.
>
> This patchset is against mainline as of Friday Jan 4 2013.
> (4956964 Merge tag 'driver-core-3.8-rc2' of \
>         git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core)
>
> Note that a separate patch for the DTC compiler has been posted and
> is required to compile the DTS files according to the documentation.
> The patch is "dtc: Dynamic symbols & fixup support"
>
> An implementation patchset for a beaglebone cape loader will follow,
> but if you want to check out a working kernel for the beaglebone please
> pull from:
>
> git://github.com/pantoniou/linux-bbxm.git branch not-capebus-v8
>
> Pantelis Antoniou (6):
>   OF: Introduce device tree node flag helpers.
>   OF: export of_property_notify
>   OF: Export all DT proc update functions
>   OF: Introduce utility helper functions
>   OF: Introduce Device Tree resolve support.
>   OF: Introduce DT overlay support.
>
>  .../devicetree/dynamic-resolution-notes.txt        |  25 +
>  Documentation/devicetree/overlay-notes.txt         | 179 +++++
>  drivers/of/Kconfig                                 |  19 +
>  drivers/of/Makefile                                |   4 +-
>  drivers/of/base.c                                  | 114 +--
>  drivers/of/overlay.c                               | 831 +++++++++++++++++++++
>  drivers/of/resolver.c                              | 394 ++++++++++
>  drivers/of/util.c                                  | 253 +++++++
>  include/linux/of.h                                 | 243 ++++++
>  9 files changed, 2005 insertions(+), 57 deletions(-)
>  create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
>  create mode 100644 Documentation/devicetree/overlay-notes.txt
>  create mode 100644 drivers/of/overlay.c
>  create mode 100644 drivers/of/resolver.c
>  create mode 100644 drivers/of/util.c
>
> --
> 1.7.12
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 0/6] Introducing Device Tree Overlays

[Pantelis Antoniou]

Hi Alan,

On Feb 21, 2013, at 1:25 PM, delicious quinoa wrote:

> I like where this is heading.  I'm interested in a use case where IP
> can be loaded into a FPGA, then add a blob to the device tree and load
> some drivers.
> 
> I see your github tree.  If I wanted to cherry-pick your code and play
> around with it, which branch should I use?  not-capebus-21?
> 

not-capebus-v21 is the latest one and indeed that has it in.

Please note that I had some other FPGA people interested with it.
Perhaps we can go through use cases to come up with your requirements


> Thanks,
> Alan Tull
> Altera Corp
> 

Regards

-- Pantelis

> On Fri, Jan 4, 2013 at 1:31 PM, Pantelis Antoniou
> <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
>> The following patchset introduces Device Tree overlays, a method
>> of dynamically altering the kernel's live Device Tree.
>> 
>> This patchset is against mainline as of Friday Jan 4 2013.
>> (4956964 Merge tag 'driver-core-3.8-rc2' of \
>>        git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core)
>> 
>> Note that a separate patch for the DTC compiler has been posted and
>> is required to compile the DTS files according to the documentation.
>> The patch is "dtc: Dynamic symbols & fixup support"
>> 
>> An implementation patchset for a beaglebone cape loader will follow,
>> but if you want to check out a working kernel for the beaglebone please
>> pull from:
>> 
>> git://github.com/pantoniou/linux-bbxm.git branch not-capebus-v8
>> 
>> Pantelis Antoniou (6):
>>  OF: Introduce device tree node flag helpers.
>>  OF: export of_property_notify
>>  OF: Export all DT proc update functions
>>  OF: Introduce utility helper functions
>>  OF: Introduce Device Tree resolve support.
>>  OF: Introduce DT overlay support.
>> 
>> .../devicetree/dynamic-resolution-notes.txt        |  25 +
>> Documentation/devicetree/overlay-notes.txt         | 179 +++++
>> drivers/of/Kconfig                                 |  19 +
>> drivers/of/Makefile                                |   4 +-
>> drivers/of/base.c                                  | 114 +--
>> drivers/of/overlay.c                               | 831 +++++++++++++++++++++
>> drivers/of/resolver.c                              | 394 ++++++++++
>> drivers/of/util.c                                  | 253 +++++++
>> include/linux/of.h                                 | 243 ++++++
>> 9 files changed, 2005 insertions(+), 57 deletions(-)
>> create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
>> create mode 100644 Documentation/devicetree/overlay-notes.txt
>> create mode 100644 drivers/of/overlay.c
>> create mode 100644 drivers/of/resolver.c
>> create mode 100644 drivers/of/util.c
>> 
>> --
>> 1.7.12
>> 
>> _______________________________________________
>> devicetree-discuss mailing list
>> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
>> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 0/6] Introducing Device Tree Overlays

[delicious quinoa]

Our use case is partial reconfiguration of the FPGA.  This will be
driven from userspace, so the bitstream to program the FPGA will come
from userspace, then the overlay for the DT, also from userspace.

So we want to take a compiled partial device tree and simply 'cat' it
to a point under /proc, such as 'cat new-ip.dtb > /proc/device-tree'

Alan Tull
Altera Corp.

On Thu, Feb 21, 2013 at 3:53 PM, Pantelis Antoniou
<panto@antoniou-consulting.com> wrote:
> Hi Alan,
>
> On Feb 21, 2013, at 1:25 PM, delicious quinoa wrote:
>
>> I like where this is heading.  I'm interested in a use case where IP
>> can be loaded into a FPGA, then add a blob to the device tree and load
>> some drivers.
>>
>> I see your github tree.  If I wanted to cherry-pick your code and play
>> around with it, which branch should I use?  not-capebus-21?
>>
>
> not-capebus-v21 is the latest one and indeed that has it in.
>
> Please note that I had some other FPGA people interested with it.
> Perhaps we can go through use cases to come up with your requirements
>
>
>> Thanks,
>> Alan Tull
>> Altera Corp
>>
>
> Regards
>
> -- Pantelis
>
>> On Fri, Jan 4, 2013 at 1:31 PM, Pantelis Antoniou
>> <panto@antoniou-consulting.com> wrote:
>>> The following patchset introduces Device Tree overlays, a method
>>> of dynamically altering the kernel's live Device Tree.
>>>
>>> This patchset is against mainline as of Friday Jan 4 2013.
>>> (4956964 Merge tag 'driver-core-3.8-rc2' of \
>>>        git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core)
>>>
>>> Note that a separate patch for the DTC compiler has been posted and
>>> is required to compile the DTS files according to the documentation.
>>> The patch is "dtc: Dynamic symbols & fixup support"
>>>
>>> An implementation patchset for a beaglebone cape loader will follow,
>>> but if you want to check out a working kernel for the beaglebone please
>>> pull from:
>>>
>>> git://github.com/pantoniou/linux-bbxm.git branch not-capebus-v8
>>>
>>> Pantelis Antoniou (6):
>>>  OF: Introduce device tree node flag helpers.
>>>  OF: export of_property_notify
>>>  OF: Export all DT proc update functions
>>>  OF: Introduce utility helper functions
>>>  OF: Introduce Device Tree resolve support.
>>>  OF: Introduce DT overlay support.
>>>
>>> .../devicetree/dynamic-resolution-notes.txt        |  25 +
>>> Documentation/devicetree/overlay-notes.txt         | 179 +++++
>>> drivers/of/Kconfig                                 |  19 +
>>> drivers/of/Makefile                                |   4 +-
>>> drivers/of/base.c                                  | 114 +--
>>> drivers/of/overlay.c                               | 831 +++++++++++++++++++++
>>> drivers/of/resolver.c                              | 394 ++++++++++
>>> drivers/of/util.c                                  | 253 +++++++
>>> include/linux/of.h                                 | 243 ++++++
>>> 9 files changed, 2005 insertions(+), 57 deletions(-)
>>> create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
>>> create mode 100644 Documentation/devicetree/overlay-notes.txt
>>> create mode 100644 drivers/of/overlay.c
>>> create mode 100644 drivers/of/resolver.c
>>> create mode 100644 drivers/of/util.c
>>>
>>> --
>>> 1.7.12
>>>
>>> _______________________________________________
>>> devicetree-discuss mailing list
>>> devicetree-discuss@lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/devicetree-discuss
>