[PATCH v7 0/9] Device Tree Overlays; the neverending saga

[PATCH v7 0/9] Device Tree Overlays; the neverending saga

[Pantelis Antoniou]

The following patchset introduces Device Tree overlays, a method
of dynamically altering the kernel's live Device Tree, along with
a generic interface to use it in a board agnostic manner.

It is against linux mainline as of today 24/7/2014
"88b5a850c88a3fd0e9d050d35962259e029e43f4
 Merge tag 'sound-3.16-rc4' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound"

It relies on the following previously submitted patches/patchsets:

* Generic OF infrastructure fixes
* configfs: Implement binary attributes (v2)
* Transaction DT support (v2)

To compile overlays you need the DTC compiler patch

* "dtc: Dynamic symbols & fixup support (v2)"

The full patchset is applied to the following tree/branch:

git:    git@github.com:pantoniou/linux-beagle-track-mainline.git
branch: dt-ng/bbb
http:   https://github.com/pantoniou/linux-beagle-track-mainline/tree/dt-ng/bbb

Changes since V6:
* Now using the updated transaction API.

Changes since V5:
* Rely on transactions for affecting changes to the live tree.
This makes the patchset considerably smaller, and easier to grok.
* Removed the internal API, simplyfing the interface.

Changes since V4:
* New API of_overlay_create/destroy being able to support stacked
overlays correctly.
* Removed use of notifiers internally.
* Removed own-grown bus handler notifiers; using already in-place DT
notification infrastructure.
* Split SPI notifier patch to one patch of generic changes and one
for DT overlay notifier.
* Removed unused overlay depth feature.
* Updated documentation.
* Moved configfs based interface to using the new API.
* Added overlay removal stacking tests.

Changes since V3:
* Added overlay self-tests.
* Fix bug in of_init_overlay_info (wrong sizeof)
* Platform bus handler handles parent_pdev == NULL
* of_resolve fixes according to comments by robh
  + changed if (foo == NULL) to if (!foo)
  + changed if (foo != NULL) to if (foo)
  + drivers/of/Kconfig added dep on OF && !SPARC
  + convert to using be32_to_cpup
  + u32 -> __be32 when modifying property values
  + cosmetic fixes

Changes since V2:
* Use of a configfs board agnostic overlay method
* Use of per bus handlers instead of hardcoded behaviour
* Optional target-path overlay target, which allows one to use standard
DTBs without resolution options.

Changes since V1:

* Removal of any bits related to a specific board (beaglebone).
* Introduced a platform agnostic interface using /proc/device-tree-overlay
* Various bug fixes related to i2c device handling have been squashed in.

Pantelis Antoniou (9):
  OF: Introduce Device Tree resolve support.
  OF: Introduce DT overlay support.
  OF: selftest: Add overlay self-test support.
  OF: DT-Overlay configfs interface
  OF: platform: Add OF notifier handler
  of: i2c: Export single device registration method
  OF: i2c: Add OF notifier handler
  of: spi: Export single device registration method and accessors
  OF: spi: Add OF notifier handler

 Documentation/devicetree/bindings/selftest.txt     |  14 +
 .../devicetree/dynamic-resolution-notes.txt        |  25 +
 Documentation/devicetree/overlay-notes.txt         | 137 ++++
 drivers/base/platform.c                            |  18 +-
 drivers/i2c/i2c-core.c                             | 164 +++--
 drivers/of/Kconfig                                 |  20 +
 drivers/of/Makefile                                |   3 +
 drivers/of/configfs.c                              | 340 ++++++++++
 drivers/of/overlay.c                               | 690 +++++++++++++++++++++
 drivers/of/platform.c                              |  62 ++
 drivers/of/resolver.c                              | 403 ++++++++++++
 drivers/of/selftest.c                              | 481 ++++++++++++++
 drivers/of/testcase-data/testcases.dtsi            |   1 +
 drivers/of/testcase-data/tests-overlay.dtsi        | 180 ++++++
 drivers/spi/spi.c                                  | 320 +++++++---
 include/linux/i2c.h                                |  10 +
 include/linux/of.h                                 |  49 ++
 include/linux/of_platform.h                        |  10 +
 18 files changed, 2779 insertions(+), 148 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/selftest.txt
 create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
 create mode 100644 Documentation/devicetree/overlay-notes.txt
 create mode 100644 drivers/of/configfs.c
 create mode 100644 drivers/of/overlay.c
 create mode 100644 drivers/of/resolver.c
 create mode 100644 drivers/of/testcase-data/tests-overlay.dtsi

-- 
1.7.12

[PATCH v7 1/9] 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.

Export of of_resolve and bug fix of double free by
	Guenter Roeck <groeck@juniper.net>

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 .../devicetree/dynamic-resolution-notes.txt        |  25 ++
 drivers/of/Kconfig                                 |   6 +
 drivers/of/Makefile                                |   1 +
 drivers/of/resolver.c                              | 403 +++++++++++++++++++++
 include/linux/of.h                                 |  16 +
 5 files changed, 451 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 2dcb054..0236a4e 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -78,4 +78,10 @@ config OF_RESERVED_MEM
 	help
 	  Helpers to allow for reservation of memory regions
 
+config OF_RESOLVE
+	bool
+	depends on OF && !SPARC
+	select OF_DYNAMIC
+	select OF_DEVICE
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 9a68cc9..35a148a 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -12,6 +12,7 @@ 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_RESERVED_MEM) += of_reserved_mem.o
+obj-$(CONFIG_OF_RESOLVE)  += resolver.o
 
 CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
 CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
new file mode 100644
index 0000000..18da5ca
--- /dev/null
+++ b/drivers/of/resolver.c
@@ -0,0 +1,403 @@
+/*
+ * 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/string.h>
+#include <linux/ctype.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+
+/**
+ * Find a node with the give full name by recursively following any of
+ * the child node links.
+ */
+static 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;
+}
+
+/*
+ * Find live tree's maximum phandle value.
+ */
+static phandle of_get_tree_max_phandle(void)
+{
+	struct device_node *node;
+	phandle phandle;
+	unsigned long flags;
+
+	/* now search recursively */
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	phandle = 0;
+	for_each_of_allnodes(node) {
+		if (node->phandle != OF_PHANDLE_ILLEGAL &&
+				node->phandle > phandle)
+			phandle = node->phandle;
+	}
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	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_cpup(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)
+		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) {
+			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) {
+				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) {
+				pr_err("%s: Illegal symbol entry '%s' (2)\n",
+					__func__, (char *)rprop->value);
+				err = -EINVAL;
+				goto err_fail;
+			}
+
+			*s++ = '\0';
+			err = kstrtoint(s, 10, &offset);
+			if (err != 0) {
+				pr_err("%s: Could get offset '%s'\n",
+					__func__, (char *)rprop->value);
+				goto err_fail;
+			}
+
+			/* look into the resolve node for the full path */
+			refnode = __of_find_node_by_full_name(node, nodestr);
+			if (!refnode) {
+				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) {
+				pr_err("%s: Could not find property '%s'\n",
+					__func__, (char *)rprop->value);
+				err = -ENOENT;
+				goto err_fail;
+			}
+
+			phandle = be32_to_cpup(sprop->value + offset);
+			*(__be32 *)(sprop->value + offset) =
+				cpu_to_be32(phandle + phandle_delta);
+		}
+
+		kfree(propval);
+		propval = NULL;
+	}
+
+	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 || !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 &&
+				of_node_cmp(child->name, "__symbols__") == 0)
+			resolve_sym = child;
+
+		if (!resolve_fix &&
+				of_node_cmp(child->name, "__fixups__") == 0)
+			resolve_fix = child;
+
+		/* both found, don't bother anymore */
+		if (resolve_sym && resolve_fix)
+			break;
+	}
+
+	/* we do allow for the case where no fixups are needed */
+	if (!resolve_fix) {
+		err = 0;	/* no error */
+		goto out;
+	}
+
+	/* we need to fixup, but no root symbols... */
+	if (!root_sym) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	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 out;
+		}
+
+		refnode = of_find_node_by_path(refpath);
+		if (!refnode) {
+			pr_err("%s: Could not find node by path '%s'\n",
+					__func__, refpath);
+			err = -ENOENT;
+			goto out;
+		}
+
+		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) {
+			pr_err("%s: Could not copy value of '%s'\n",
+					__func__, rprop->name);
+			err = -ENOMEM;
+			goto out;
+		}
+
+		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) {
+				pr_err("%s: Illegal symbol entry '%s' (1)\n",
+					__func__, (char *)rprop->value);
+				err = -EINVAL;
+				goto err_fail_free;
+			}
+			*s++ = '\0';
+
+			propstr = s;
+			s = strchr(s, ':');
+			if (!s) {
+				pr_err("%s: Illegal symbol entry '%s' (2)\n",
+					__func__, (char *)rprop->value);
+				err = -EINVAL;
+				goto err_fail_free;
+			}
+
+			*s++ = '\0';
+			err = kstrtoint(s, 10, &offset);
+			if (err != 0) {
+				pr_err("%s: Could get offset '%s'\n",
+					__func__, (char *)rprop->value);
+				goto err_fail_free;
+			}
+
+			/* look into the resolve node for the full path */
+			refnode = __of_find_node_by_full_name(resolve,
+					nodestr);
+			if (!refnode) {
+				pr_err("%s: Could not find refnode '%s'\n",
+					__func__, (char *)rprop->value);
+				err = -ENOENT;
+				goto err_fail_free;
+			}
+
+			/* now find the property */
+			for_each_property_of_node(refnode, sprop) {
+				if (of_prop_cmp(sprop->name, propstr) == 0)
+					break;
+			}
+
+			if (!sprop) {
+				pr_err("%s: Could not find property '%s'\n",
+					__func__, (char *)rprop->value);
+				err = -ENOENT;
+				goto err_fail_free;
+			}
+
+			*(__be32 *)(sprop->value + offset) =
+				cpu_to_be32(phandle);
+		}
+
+		kfree(propval);
+		propval = NULL;
+	}
+
+err_fail_free:
+	kfree(propval);
+
+out:
+	/* NULL is handled by of_node_put as NOP */
+	of_node_put(root_sym);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(of_resolve);
diff --git a/include/linux/of.h b/include/linux/of.h
index fa81b42..e9be31c 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -903,4 +903,20 @@ static inline int of_transaction_update_property(struct of_transaction *oft,
 }
 #endif
 
+/* 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

[PATCH v7 2/9] OF: Introduce DT overlay support.

[Pantelis Antoniou]

Introduce DT overlay support.

Makes it possible to dynamically overlay a part of the kernel's
tree with another tree that's been dynamically loaded.
Removal of nodes and properties is also possible.

The hard part of applying and reverting the overlay is done
using transactions.

Documentation about internal and APIs is provided in
	Documentation/devicetree/overlay-notes.txt

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 Documentation/devicetree/overlay-notes.txt | 137 ++++++
 drivers/of/Kconfig                         |   7 +
 drivers/of/Makefile                        |   1 +
 drivers/of/overlay.c                       | 690 +++++++++++++++++++++++++++++
 include/linux/of.h                         |  33 ++
 5 files changed, 868 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..b060bd7
--- /dev/null
+++ b/Documentation/devicetree/overlay-notes.txt
@@ -0,0 +1,137 @@
+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 API is quite easy to use.
+
+1. Call of_overlay_create() to create and apply an overlay. The return value
+is a cookie identifying this overlay.
+
+2. Call of_overlay_destroy() to remove and cleanup the overlay previously
+created via the call to of_overlay_create(). Removal of an overlay that
+is stacked by another will not be permitted.
+
+Finally, if you need to remove all overlays in one-go, just call
+of_overlay_destroy_all() which will remove every single one in the correct
+order.
+
+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>;	/* phandle target of the overlay */
+	or
+		target-path="/path";	/* target path 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 */
+}
+
+Using the non-phandle based target method allows one to use a base DT which does
+not contain a __symbols__ node, i.e. it was not compiled with the -@ option.
+The __symbols__ node is only required for the target=<phandle> method, since it
+contains the information required to map from a phandle to a tree location.
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 0236a4e..5ebde98 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -84,4 +84,11 @@ config OF_RESOLVE
 	select OF_DYNAMIC
 	select OF_DEVICE
 
+config OF_OVERLAY
+	bool
+	depends on OF
+	select OF_DYNAMIC
+	select OF_DEVICE
+	select OF_RESOLVE
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 35a148a..a9ba136 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
 obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
 obj-$(CONFIG_OF_RESOLVE)  += resolver.o
+obj-$(CONFIG_OF_OVERLAY) += overlay.o
 
 CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
 CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
new file mode 100644
index 0000000..9a6ace6
--- /dev/null
+++ b/drivers/of/overlay.c
@@ -0,0 +1,690 @@
+/*
+ * Functions for working with device tree overlays
+ *
+ * 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.
+ */
+#undef DEBUG
+#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>
+
+#include "of_private.h"
+
+/**
+ * struct of_overlay_info	- Holds a single overlay info
+ * @target:	target of the overlay operation
+ * @overlay:	pointer to the overlay contents node
+ *
+ * Holds a single overlay state, including all the overlay logs &
+ * records.
+ */
+struct of_overlay_info {
+	struct device_node *target;
+	struct device_node *overlay;
+};
+
+/**
+ * struct of_overlay - Holds a complete overlay transaction
+ * @node:	List on which we are located
+ * @count:	Count of ovinfo structures
+ * @ovinfo:	Overlay info array (count size)
+ * @le_list:	List of the overlay logs
+ *
+ * Holds a complete overlay transaction
+ */
+struct of_overlay {
+	int id;
+	struct list_head node;
+	int count;
+	struct of_overlay_info *ovinfo_tab;
+	struct of_transaction trans;
+};
+
+static int of_overlay_apply_one(struct of_overlay *ov,
+		struct device_node *target, const struct device_node *overlay);
+
+static int of_overlay_apply_single_property(struct of_overlay *ov,
+		struct device_node *target, struct property *prop)
+{
+	const char *pname;
+	struct property *propn, *tprop;
+	int remove;
+
+	/* don't touch, 'name' */
+	if (of_prop_cmp(prop->name, "name") == 0)
+		return 0;
+
+	/* 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,
+				OF_PROP_ALLOCALL);
+		if (propn == NULL)
+			return -ENOMEM;
+	}
+
+	tprop = of_transaction_find_property(&ov->trans, target, pname, NULL);
+
+	/* found? */
+	if (tprop != NULL) {
+		if (propn != NULL)
+			return of_transaction_update_property(&ov->trans,
+					target, propn);
+		else
+			return of_transaction_remove_property(&ov->trans,
+					target, tprop);
+	}
+
+	if (propn != NULL)
+		return of_transaction_add_property(&ov->trans, target, propn);
+
+	return 0;
+}
+
+static int of_overlay_apply_single_device_node(struct of_overlay *ov,
+		struct device_node *target, struct device_node *child)
+{
+	const char *cname;
+	struct device_node *tchild;
+	int remove;
+	char *full_name;
+	const char *suffix;
+	int ret;
+
+	/* 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);
+		if (cname == NULL)
+			return -ENOMEM;
+
+		if (*cname == '-')
+			cname++;
+	}
+
+	ret = 0;
+
+	tchild = of_transaction_get_child_by_name(&ov->trans, target, cname);
+	if (tchild != NULL) {
+
+		if (!remove)
+			/* apply overlay recursively */
+			ret = of_overlay_apply_one(ov, tchild,
+					child);
+		else
+			ret = of_transaction_detach_node(&ov->trans, 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,
+				OF_NODE_ALLOCALL);
+
+		/* free either way */
+		kfree(full_name);
+
+		if (tchild == NULL)
+			return -ENOMEM;
+
+		/* point to parent */
+		tchild->parent = target;
+
+		ret = of_transaction_attach_node(&ov->trans, tchild);
+		if (ret != 0)
+			return ret;
+
+		/* apply the overlay */
+		ret = of_overlay_apply_one(ov, tchild,
+				child);
+	}
+
+	return ret;
+}
+
+/*
+ * Apply a single overlay node recursively.
+ *
+ * Property or node names that start with '-' signal that
+ * the property/node is to be removed.
+ *
+ * Note that the in case of an error the target node is left
+ * in a inconsistent state. Error recovery should be performed
+ * by using the tree changes list.
+ */
+static int of_overlay_apply_one(struct of_overlay *ov,
+		struct device_node *target, const struct device_node *overlay)
+{
+	struct device_node *child;
+	struct property *prop;
+	int prev_avail, prop_avail, pass;
+	int ret;
+
+	/* both properties existing is wrong */
+	if (of_transaction_find_property(&ov->trans,
+				overlay, "status", NULL) &&
+	    of_transaction_find_property(&ov->trans,
+				overlay, "-status", NULL)) {
+		pr_err("%s: Both status & -status properties exist\n",
+				__func__);
+		return -EINVAL;
+	}
+
+	/*
+	 * Special consideration for status properties
+	 *
+	 * In order to make status property changes work
+	 * we have the following cases:
+	 *
+	 * Enabled device with status change to 'disabled'
+	 *   -> Status property must be first on the record list
+	 *
+	 * Disabled device with status change to 'okay'
+	 *   -> Status property must be last in the record list
+	 *
+	 * That way we don't need a special notifier for
+	 * device status change, a simple notifier on the status
+	 * property is enough.
+	 *
+	 */
+
+	/* note that we require the existence of a status property */
+	prev_avail = of_transaction_device_is_available(&ov->trans, target) &&
+		of_transaction_find_property(&ov->trans, target,
+				"compatible", NULL) &&
+		of_transaction_find_property(&ov->trans, target,
+				"status", NULL);
+
+	/* we make two passes */
+	for (pass = 1; pass <= 2; pass++) {
+
+		for_each_property_of_node(overlay, prop) {
+
+			prop_avail = -1;
+
+			if (of_prop_cmp(prop->name, "status") == 0)
+				prop_avail = strcmp(prop->value, "okay") == 0 ||
+						strcmp(prop->value, "ok") == 0;
+			else if (of_prop_cmp(prop->name, "-status") == 0)
+				prop_avail = 1;
+
+			/* skip activation property */
+			if (prev_avail == 0) {
+				/* 0 -> 1, pass #1, skip */
+				if (pass == 1) {
+					if (prop_avail == 1)
+						continue;
+				} else {
+					/* 0 -> 1, pass #2, process */
+					if (prop_avail != 1)
+						continue;
+				}
+			} else {
+				if (pass == 1) {
+					/* 1 -> 0, pass #1, process */
+					if (prop_avail != 0)
+						continue;
+				} else {
+					/* 1 -> 0, pass #2, skip */
+					if (prop_avail == 0)
+						continue;
+				}
+			}
+
+			ret = of_overlay_apply_single_property(ov,
+					target, prop);
+			if (ret != 0) {
+				pr_err("%s: Failed to apply prop @%s/%s\n",
+						__func__, target->full_name,
+						prop->name);
+				return ret;
+			}
+		}
+	}
+
+	for_each_child_of_node(overlay, child) {
+		ret = of_overlay_apply_single_device_node(ov, target, child);
+		if (ret != 0) {
+			pr_err("%s: Failed to apply single node @%s/%s\n",
+					__func__, target->full_name,
+					child->name);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * of_overlay_apply - Apply @count overlays pointed at by @ovinfo_tab
+ * @ov:		Overlay 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.
+ */
+static int of_overlay_apply(struct of_overlay *ov)
+{
+	struct of_overlay_info *ovinfo;
+	int i, err;
+
+	/* first we apply the overlays atomically */
+	for (i = 0; i < ov->count; i++) {
+
+		ovinfo = &ov->ovinfo_tab[i];
+
+		err = of_overlay_apply_one(ov, ovinfo->target,
+				ovinfo->overlay);
+		if (err != 0) {
+			pr_err("%s: overlay failed '%s'\n",
+				__func__, ovinfo->target->full_name);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Find the target node using a number of different strategies
+ * in order of preference
+ *
+ * "target" property containing the phandle of the target
+ * "target-path" property containing the path of the target
+ *
+ */
+static struct device_node *find_target_node(struct device_node *info_node)
+{
+	const char *path;
+	u32 val;
+	int ret;
+
+	/* first try to go by using the target as a phandle */
+	ret = of_property_read_u32(info_node, "target", &val);
+	if (ret == 0)
+		return of_find_node_by_phandle(val);
+
+	/* now try to locate by path */
+	ret = of_property_read_string(info_node, "target-path", &path);
+	if (ret == 0)
+		return of_find_node_by_path(path);
+
+	pr_err("%s: Failed to find target for node %p (%s)\n", __func__,
+			info_node, info_node->name);
+
+	return NULL;
+}
+
+/**
+ * of_fill_overlay_info	- Fill an overlay info structure
+ * @ov		Overlay to fill
+ * @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.
+ */
+static int of_fill_overlay_info(struct of_overlay *ov,
+		struct device_node *info_node, struct of_overlay_info *ovinfo)
+{
+	ovinfo->overlay = of_get_child_by_name(info_node, "__overlay__");
+	if (ovinfo->overlay == NULL)
+		goto err_fail;
+
+	ovinfo->target = find_target_node(info_node);
+	if (ovinfo->target == 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
+ * @ov		Overlay to build
+ * @tree:	Device node containing all the overlays
+ *
+ * Helper function that given a tree containing overlay information,
+ * allocates and builds an overlay info array containing it, ready
+ * for use using of_overlay_apply.
+ *
+ * Returns 0 on success with the @cntp @ovinfop pointers valid,
+ * while on error a negative error value is returned.
+ */
+static int of_build_overlay_info(struct of_overlay *ov,
+		struct device_node *tree)
+{
+	struct device_node *node;
+	struct of_overlay_info *ovinfo;
+	int cnt, err;
+
+	/* worst case; every child is a node */
+	cnt = 0;
+	for_each_child_of_node(tree, node)
+		cnt++;
+
+	ovinfo = kcalloc(cnt, sizeof(*ovinfo), GFP_KERNEL);
+	if (ovinfo == NULL)
+		return -ENOMEM;
+
+	cnt = 0;
+	for_each_child_of_node(tree, node) {
+
+		memset(&ovinfo[cnt], 0, sizeof(*ovinfo));
+		err = of_fill_overlay_info(ov, node, &ovinfo[cnt]);
+		if (err == 0)
+			cnt++;
+	}
+
+	/* if nothing filled, return error */
+	if (cnt == 0) {
+		kfree(ovinfo);
+		return -ENODEV;
+	}
+
+	ov->count = cnt;
+	ov->ovinfo_tab = ovinfo;
+
+	return 0;
+}
+
+/**
+ * of_free_overlay_info	- Free an overlay info array
+ * @ov		Overlay to free the overlay info from
+ * @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.
+ */
+static int of_free_overlay_info(struct of_overlay *ov)
+{
+	struct of_overlay_info *ovinfo;
+	int i;
+
+	/* do it in reverse */
+	for (i = ov->count - 1; i >= 0; i--) {
+		ovinfo = &ov->ovinfo_tab[i];
+
+		of_node_put(ovinfo->target);
+		of_node_put(ovinfo->overlay);
+	}
+	kfree(ov->ovinfo_tab);
+
+	return 0;
+}
+
+static LIST_HEAD(ov_list);
+static DEFINE_MUTEX(ov_lock);
+static DEFINE_IDR(ov_idr);
+
+/**
+ * of_overlay_create	- Create and apply an overlay
+ * @tree:	Device node containing all the overlays
+ *
+ * Creates and applies an overlay while also keeping track
+ * of the overlay in a list. This list can be use to prevent
+ * illegal overlay removals.
+ *
+ * Returns the id of the created overlay, or an negative error number
+ */
+int of_overlay_create(struct device_node *tree)
+{
+	struct of_overlay *ov;
+	int err, id;
+
+	/* allocate the overlay structure */
+	ov = kzalloc(sizeof(*ov), GFP_KERNEL);
+	if (ov == NULL)
+		return -ENOMEM;
+	ov->id = -1;
+
+	INIT_LIST_HEAD(&ov->node);
+	mutex_lock(&ov_lock);
+
+	of_transaction_init(&ov->trans);
+
+	id = idr_alloc(&ov_idr, ov, 0, 0, GFP_KERNEL);
+	if (id < 0) {
+		pr_err("%s: idr_alloc() failed for tree@%s\n",
+				__func__, tree->full_name);
+		err = id;
+		goto err_destroy_trans;
+	}
+	ov->id = id;
+
+	/* build the overlay info structures */
+	err = of_build_overlay_info(ov, tree);
+	if (err) {
+		pr_err("%s: of_build_overlay_info() failed for tree@%s\n",
+				__func__, tree->full_name);
+		goto err_free_idr;
+	}
+
+	of_transaction_start(&ov->trans);
+
+	/* apply the overlay */
+	err = of_overlay_apply(ov);
+	if (err) {
+		pr_err("%s: of_overlay_apply() failed for tree@%s\n",
+				__func__, tree->full_name);
+		goto err_abort_trans;
+	}
+
+	/* apply the transaction */
+	err = of_transaction_apply(&ov->trans, 0);
+	if (err) {
+		pr_err("%s: of_transaction_apply() failed for tree@%s\n",
+				__func__, tree->full_name);
+		goto err_revert_overlay;
+	}
+
+	/* add to the tail of the overlay list */
+	list_add_tail(&ov->node, &ov_list);
+
+	mutex_unlock(&ov_lock);
+
+	return id;
+
+err_revert_overlay:
+err_abort_trans:
+	of_transaction_abort(&ov->trans);
+	of_free_overlay_info(ov);
+err_free_idr:
+	idr_remove(&ov_idr, ov->id);
+err_destroy_trans:
+	of_transaction_destroy(&ov->trans);
+	mutex_unlock(&ov_lock);
+	kfree(ov);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(of_overlay_create);
+
+/* check whether the given node, lies under the given tree */
+static int overlay_subtree_check(struct device_node *tree,
+		struct device_node *dn)
+{
+	struct device_node *child;
+
+	/* match? */
+	if (tree == dn)
+		return 1;
+
+	for_each_child_of_node(tree, child) {
+		if (overlay_subtree_check(child, dn))
+			return 1;
+	}
+
+	return 0;
+}
+
+/* check whether this overlay is the topmost */
+static int overlay_is_topmost(struct of_overlay *ov, struct device_node *dn)
+{
+	struct of_overlay *ovt;
+	struct of_transaction_entry *te;
+
+	list_for_each_entry_reverse(ovt, &ov_list, node) {
+
+		/* if we hit ourselves, we're done */
+		if (ovt == ov)
+			break;
+
+		/* check against each subtree affected by this overlay */
+		for_each_transaction_entry(&ovt->trans, te) {
+			if (overlay_subtree_check(te->np, dn)) {
+				pr_err("%s: #%d clashes #%d @%s\n",
+					__func__, ov->id, ovt->id,
+					dn->full_name);
+				return 0;
+			}
+		}
+	}
+
+	/* overlay is topmost */
+	return 1;
+}
+
+/*
+ * We can safely remove the overlay only if it's the top-most one.
+ * Newly applied overlays are inserted at the tail of the overlay list,
+ * so a top most overlay is the one that is closest to the tail.
+ *
+ * The topmost check is done by exploiting this property. For each
+ * affected device node in the log list we check if this overlay is
+ * the one closest to the tail. If another overlay has affected this
+ * device node and is closest to the tail, then removal is not permited.
+ */
+static int overlay_removal_is_ok(struct of_overlay *ov)
+{
+	struct of_transaction_entry *te;
+
+	for_each_transaction_entry(&ov->trans, te) {
+		if (!overlay_is_topmost(ov, te->np)) {
+			pr_err("%s: overlay #%d is not topmost\n",
+					__func__, ov->id);
+			return 0;
+		}
+	}
+
+	return 1;
+}
+
+/**
+ * of_overlay_destroy	- Removes an overlay
+ * @id:	Overlay id number returned by a previous call to of_overlay_create
+ *
+ * Removes an overlay if it is permissible.
+ *
+ * Returns 0 on success, or an negative error number
+ */
+int of_overlay_destroy(int id)
+{
+	struct of_overlay *ov;
+	int err;
+
+	mutex_lock(&ov_lock);
+	ov = idr_find(&ov_idr, id);
+	if (ov == NULL) {
+		err = -ENODEV;
+		pr_err("%s: Could not find overlay #%d\n",
+				__func__, id);
+		goto out;
+	}
+
+	/* check whether the overlay is safe to remove */
+	if (!overlay_removal_is_ok(ov)) {
+		err = -EBUSY;
+		pr_err("%s: removal check failed for overlay #%d\n",
+				__func__, id);
+		goto out;
+	}
+
+	list_del(&ov->node);
+	of_transaction_revert(&ov->trans, 1);
+	of_free_overlay_info(ov);
+	idr_remove(&ov_idr, id);
+	of_transaction_destroy(&ov->trans);
+	kfree(ov);
+
+	err = 0;
+
+out:
+	mutex_unlock(&ov_lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(of_overlay_destroy);
+
+/**
+ * of_overlay_destroy_all	- Removes all overlays from the system
+ *
+ * Removes all overlays from the system in the correct order.
+ *
+ * Returns 0 on success, or an negative error number
+ */
+int of_overlay_destroy_all(void)
+{
+	struct of_overlay *ov, *ovn;
+
+	mutex_lock(&ov_lock);
+
+	/* the tail of list is guaranteed to be safe to remove */
+	list_for_each_entry_safe_reverse(ov, ovn, &ov_list, node) {
+		list_del(&ov->node);
+		of_transaction_revert(&ov->trans, 1);
+		of_free_overlay_info(ov);
+		idr_remove(&ov_idr, ov->id);
+		kfree(ov);
+	}
+
+	mutex_unlock(&ov_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_overlay_destroy_all);
diff --git a/include/linux/of.h b/include/linux/of.h
index e9be31c..8467cd6 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -23,6 +23,7 @@
 #include <linux/spinlock.h>
 #include <linux/topology.h>
 #include <linux/notifier.h>
+#include <linux/list.h>
 
 #include <asm/byteorder.h>
 #include <asm/errno.h>
@@ -331,6 +332,8 @@ extern int of_update_property(struct device_node *np, struct property *newprop);
 #define OF_RECONFIG_ADD_PROPERTY	0x0003
 #define OF_RECONFIG_REMOVE_PROPERTY	0x0004
 #define OF_RECONFIG_UPDATE_PROPERTY	0x0005
+#define OF_RECONFIG_DYNAMIC_CREATE_DEV	0x0006
+#define OF_RECONFIG_DYNAMIC_DESTROY_DEV	0x0007
 
 struct of_prop_reconfig {
 	struct device_node	*dn;
@@ -919,4 +922,34 @@ static inline int of_resolve(struct device_node *resolve)
 
 #endif
 
+/**
+ * Overlay support
+ */
+
+#ifdef CONFIG_OF_OVERLAY
+
+/* ID based overlays; the API for external users */
+int of_overlay_create(struct device_node *tree);
+int of_overlay_destroy(int id);
+int of_overlay_destroy_all(void);
+
+#else
+
+int of_overlay_create(struct device_node *tree)
+{
+	return -ENOTSUPP;
+}
+
+int of_overlay_destroy(int id)
+{
+	return -ENOTSUPP;
+}
+
+int of_overlay_destroy_all(void)
+{
+	return -ENOTSUPP;
+}
+
+#endif
+
 #endif /* _LINUX_OF_H */
-- 
1.7.12

[PATCH v7 3/9] OF: selftest: Add overlay self-test support.

[Pantelis Antoniou]

This patch adds overlay tests to the OF selftest.

It tests overlay device addition/removal and whether
the apply revert sequence is correct.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 Documentation/devicetree/bindings/selftest.txt |  14 +
 drivers/of/selftest.c                          | 481 +++++++++++++++++++++++++
 drivers/of/testcase-data/testcases.dtsi        |   1 +
 drivers/of/testcase-data/tests-overlay.dtsi    | 180 +++++++++
 4 files changed, 676 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/selftest.txt
 create mode 100644 drivers/of/testcase-data/tests-overlay.dtsi

diff --git a/Documentation/devicetree/bindings/selftest.txt b/Documentation/devicetree/bindings/selftest.txt
new file mode 100644
index 0000000..0f92a22
--- /dev/null
+++ b/Documentation/devicetree/bindings/selftest.txt
@@ -0,0 +1,14 @@
+* OF selftest platform device
+
+** selftest
+
+Required properties:
+- compatible: must be "selftest"
+
+All other properties are optional.
+
+Example:
+	selftest {
+		compatible = "selftest";
+		status = "okay";
+	};
diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index 077314e..1c74676 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -15,6 +15,8 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
 
 static struct selftest_results {
 	int passed;
@@ -517,6 +519,484 @@ static void __init of_selftest_platform_populate(void)
 	}
 }
 
+#ifdef CONFIG_OF_OVERLAY
+
+static int selftest_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	if (np == NULL) {
+		dev_err(dev, "No OF data for device\n");
+		return -EINVAL;
+
+	}
+
+	dev_dbg(dev, "%s for node @%s\n", __func__, np->full_name);
+	return 0;
+}
+
+static int selftest_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	dev_dbg(dev, "%s for node @%s\n", __func__, np->full_name);
+	return 0;
+}
+
+static struct of_device_id selftest_match[] = {
+	{ .compatible = "selftest", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, altera_jtaguart_match);
+
+static struct platform_driver selftest_driver = {
+	.probe			= selftest_probe,
+	.remove			= selftest_remove,
+	.driver = {
+		.name		= "selftest",
+		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(selftest_match),
+	},
+};
+
+/* get the platform device instantiated at the path */
+static struct platform_device *of_path_to_platform_device(const char *path)
+{
+	struct device_node *np;
+	struct platform_device *pdev;
+
+	np = of_find_node_by_path(path);
+	if (np == NULL)
+		return NULL;
+
+	pdev = of_find_device_by_node(np);
+	of_node_put(np);
+
+	return pdev;
+}
+
+/* find out if a platform device exists at that path */
+static int of_path_platform_device_exists(const char *path)
+{
+	struct platform_device *pdev;
+
+	pdev = of_path_to_platform_device(path);
+	platform_device_put(pdev);
+	return pdev != NULL;
+}
+
+static const char *selftest_path(int nr)
+{
+	static char buf[256];
+
+	snprintf(buf, sizeof(buf) - 1,
+		"/testcase-data/overlay-node/test-bus/test-selftest%d", nr);
+	buf[sizeof(buf) - 1] = '\0';
+
+	return buf;
+}
+
+static const char *overlay_path(int nr)
+{
+	static char buf[256];
+
+	snprintf(buf, sizeof(buf) - 1,
+		"/testcase-data/overlay%d", nr);
+	buf[sizeof(buf) - 1] = '\0';
+
+	return buf;
+}
+
+static const char *bus_path = "/testcase-data/overlay-node/test-bus";
+
+static int of_selftest_apply_overlay(int selftest_nr, int overlay_nr,
+		int *overlay_id)
+{
+	struct device_node *np = NULL;
+	int ret, id = -1;
+
+	np = of_find_node_by_path(overlay_path(overlay_nr));
+	if (np == NULL) {
+		selftest(0, "could not find overlay node @\"%s\"\n",
+				overlay_path(overlay_nr));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = of_overlay_create(np);
+	if (ret < 0) {
+		selftest(0, "could not create overlay from \"%s\"\n",
+				overlay_path(overlay_nr));
+		goto out;
+	}
+	id = ret;
+
+	ret = 0;
+
+out:
+	of_node_put(np);
+
+	if (overlay_id)
+		*overlay_id = id;
+
+	return ret;
+}
+
+/* apply an overlay while checking before and after states */
+static int of_selftest_apply_overlay_check(int overlay_nr, int selftest_nr,
+		int before, int after)
+{
+	int ret;
+
+	/* selftest device must not be in before state */
+	if (of_path_platform_device_exists(selftest_path(selftest_nr))
+			!= before) {
+		selftest(0, "overlay @\"%s\" with device @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				selftest_path(selftest_nr),
+				!before ? "enabled" : "disabled");
+		return -EINVAL;
+	}
+
+	ret = of_selftest_apply_overlay(overlay_nr, selftest_nr, NULL);
+	if (ret != 0) {
+		/* of_selftest_apply_overlay already called selftest() */
+		return ret;
+	}
+
+	/* selftest device must be to set to after state */
+	if (of_path_platform_device_exists(selftest_path(selftest_nr))
+			!= after) {
+		selftest(0, "overlay @\"%s\" failed to create @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				selftest_path(selftest_nr),
+				!after ? "enabled" : "disabled");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* apply an overlay and then revert it while checking before, after states */
+static int of_selftest_apply_revert_overlay_check(int overlay_nr,
+		int selftest_nr, int before, int after)
+{
+	int ret, ov_id;
+
+	/* selftest device must be in before state */
+	if (of_path_platform_device_exists(selftest_path(selftest_nr))
+			!= before) {
+		selftest(0, "overlay @\"%s\" with device @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				selftest_path(selftest_nr),
+				!before ? "enabled" : "disabled");
+		return -EINVAL;
+	}
+
+	/* apply the overlay */
+	ret = of_selftest_apply_overlay(overlay_nr, selftest_nr, &ov_id);
+	if (ret != 0) {
+		/* of_selftest_apply_overlay already called selftest() */
+		return ret;
+	}
+
+	/* selftest device must be in after state */
+	if (of_path_platform_device_exists(selftest_path(selftest_nr))
+			!= after) {
+		selftest(0, "overlay @\"%s\" failed to create @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				selftest_path(selftest_nr),
+				!after ? "enabled" : "disabled");
+		return -EINVAL;
+	}
+
+	ret = of_overlay_destroy(ov_id);
+	if (ret != 0) {
+		selftest(0, "overlay @\"%s\" failed to be destroyed @\"%s\"\n",
+				overlay_path(overlay_nr),
+				selftest_path(selftest_nr));
+		return ret;
+	}
+
+	/* selftest device must be again in before state */
+	if (of_path_platform_device_exists(selftest_path(selftest_nr))
+			!= before) {
+		selftest(0, "overlay @\"%s\" with device @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				selftest_path(selftest_nr),
+				!before ? "enabled" : "disabled");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* test activation of device */
+static void of_selftest_overlay_0(void)
+{
+	int ret;
+
+	/* device should enable */
+	ret = of_selftest_apply_overlay_check(0, 0, 0, 1);
+	if (ret != 0)
+		return;
+
+	selftest(1, "overlay test %d passed\n", 0);
+}
+
+/* test deactivation of device */
+static void of_selftest_overlay_1(void)
+{
+	int ret;
+
+	/* device should disable */
+	ret = of_selftest_apply_overlay_check(1, 1, 1, 0);
+	if (ret != 0)
+		return;
+
+	selftest(1, "overlay test %d passed\n", 1);
+}
+
+/* test activation of device */
+static void of_selftest_overlay_2(void)
+{
+	int ret;
+
+	/* device should enable */
+	ret = of_selftest_apply_overlay_check(2, 2, 0, 1);
+	if (ret != 0)
+		return;
+
+	selftest(1, "overlay test %d passed\n", 2);
+}
+
+/* test deactivation of device */
+static void of_selftest_overlay_3(void)
+{
+	int ret;
+
+	/* device should disable */
+	ret = of_selftest_apply_overlay_check(3, 3, 1, 0);
+	if (ret != 0)
+		return;
+
+	selftest(1, "overlay test %d passed\n", 3);
+}
+
+/* test activation of a full device node */
+static void of_selftest_overlay_4(void)
+{
+	int ret;
+
+	/* device should disable */
+	ret = of_selftest_apply_overlay_check(4, 4, 0, 1);
+	if (ret != 0)
+		return;
+
+	selftest(1, "overlay test %d passed\n", 4);
+}
+
+/* test overlay apply/revert sequence */
+static void of_selftest_overlay_5(void)
+{
+	int ret;
+
+	/* device should disable */
+	ret = of_selftest_apply_revert_overlay_check(5, 5, 0, 1);
+	if (ret != 0)
+		return;
+
+	selftest(1, "overlay test %d passed\n", 5);
+}
+
+/* test overlay application in sequence */
+static void of_selftest_overlay_6(void)
+{
+	struct device_node *np;
+	int ret, i, ov_id[2];
+	int overlay_nr = 6, selftest_nr = 6;
+	int before = 0, after = 1;
+
+	/* selftest device must be in before state */
+	for (i = 0; i < 2; i++) {
+		if (of_path_platform_device_exists(
+					selftest_path(selftest_nr + i))
+				!= before) {
+			selftest(0, "overlay @\"%s\" with device @\"%s\" %s\n",
+					overlay_path(overlay_nr + i),
+					selftest_path(selftest_nr + i),
+					!before ? "enabled" : "disabled");
+			return;
+		}
+	}
+
+	/* apply the overlays */
+	for (i = 0; i < 2; i++) {
+
+		np = of_find_node_by_path(overlay_path(overlay_nr + i));
+		if (np == NULL) {
+			selftest(0, "could not find overlay node @\"%s\"\n",
+					overlay_path(overlay_nr + i));
+			return;
+		}
+
+		ret = of_overlay_create(np);
+		if (ret < 0)  {
+			selftest(0, "could not create overlay from \"%s\"\n",
+					overlay_path(overlay_nr + i));
+			return;
+		}
+		ov_id[i] = ret;
+	}
+
+	for (i = 0; i < 2; i++) {
+		/* selftest device must be in after state */
+		if (of_path_platform_device_exists(
+					selftest_path(selftest_nr + i))
+				!= after) {
+			selftest(0, "overlay @\"%s\" failed @\"%s\" %s\n",
+					overlay_path(overlay_nr + i),
+					selftest_path(selftest_nr + i),
+					!after ? "enabled" : "disabled");
+			return;
+		}
+	}
+
+	for (i = 1; i >= 0; i--) {
+		ret = of_overlay_destroy(ov_id[i]);
+		if (ret != 0) {
+			selftest(0, "overlay @\"%s\" failed destroy @\"%s\"\n",
+					overlay_path(overlay_nr + i),
+					selftest_path(selftest_nr + i));
+			return;
+		}
+	}
+
+	for (i = 0; i < 2; i++) {
+		/* selftest device must be again in before state */
+		if (of_path_platform_device_exists(
+					selftest_path(selftest_nr + i))
+				!= before) {
+			selftest(0, "overlay @\"%s\" with device @\"%s\" %s\n",
+					overlay_path(overlay_nr + i),
+					selftest_path(selftest_nr + i),
+					!before ? "enabled" : "disabled");
+			return;
+		}
+	}
+
+	selftest(1, "overlay test %d passed\n", 6);
+}
+
+/* test overlay application in sequence */
+static void of_selftest_overlay_8(void)
+{
+	struct device_node *np;
+	int ret, i, ov_id[2];
+	int overlay_nr = 8, selftest_nr = 8;
+
+	/* we don't care about device state in this test */
+
+	/* apply the overlays */
+	for (i = 0; i < 2; i++) {
+
+		np = of_find_node_by_path(overlay_path(overlay_nr + i));
+		if (np == NULL) {
+			selftest(0, "could not find overlay node @\"%s\"\n",
+					overlay_path(overlay_nr + i));
+			return;
+		}
+
+		ret = of_overlay_create(np);
+		if (ret < 0)  {
+			selftest(0, "could not create overlay from \"%s\"\n",
+					overlay_path(overlay_nr + i));
+			return;
+		}
+		ov_id[i] = ret;
+	}
+
+	/* now try to remove first overlay (it should fail) */
+	ret = of_overlay_destroy(ov_id[0]);
+	if (ret == 0) {
+		selftest(0, "overlay @\"%s\" was destroyed @\"%s\"\n",
+				overlay_path(overlay_nr + 0),
+				selftest_path(selftest_nr));
+		return;
+	}
+
+	/* removing them in order should work */
+	for (i = 1; i >= 0; i--) {
+		ret = of_overlay_destroy(ov_id[i]);
+		if (ret != 0) {
+			selftest(0, "overlay @\"%s\" not destroyed @\"%s\"\n",
+					overlay_path(overlay_nr + i),
+					selftest_path(selftest_nr));
+			return;
+		}
+	}
+
+	selftest(1, "overlay test %d passed\n", 8);
+}
+
+static void __init of_selftest_overlay(void)
+{
+	struct device_node *bus_np = NULL;
+	int ret;
+
+	ret = platform_driver_register(&selftest_driver);
+	if (ret != 0) {
+		selftest(0, "could not register selftest driver\n");
+		goto out;
+	}
+
+	bus_np = of_find_node_by_path(bus_path);
+	if (bus_np == NULL) {
+		selftest(0, "could not find bus_path \"%s\"\n", bus_path);
+		goto out;
+	}
+
+	ret = of_platform_populate(bus_np, of_default_bus_match_table,
+			NULL, NULL);
+	if (ret != 0) {
+		selftest(0, "could not populate bus @ \"%s\"\n", bus_path);
+		goto out;
+	}
+
+	if (!of_path_platform_device_exists(selftest_path(100))) {
+		selftest(0, "could not find selftest0 @ \"%s\"\n",
+				selftest_path(100));
+		goto out;
+	}
+
+	if (of_path_platform_device_exists(selftest_path(101))) {
+		selftest(0, "selftest1 @ \"%s\" should not exist\n",
+				selftest_path(101));
+		goto out;
+	}
+
+	selftest(1, "basic infrastructure of overlays passed");
+
+	/* tests in sequence */
+	of_selftest_overlay_0();
+	of_selftest_overlay_1();
+	of_selftest_overlay_2();
+	of_selftest_overlay_3();
+	of_selftest_overlay_4();
+	of_selftest_overlay_5();
+	of_selftest_overlay_6();
+	of_selftest_overlay_8();
+
+out:
+	of_node_put(bus_np);
+}
+
+#else
+static inline void __init of_selftest_overlay(void) { }
+#endif
+
 static int __init of_selftest(void)
 {
 	struct device_node *np;
@@ -537,6 +1017,7 @@ static int __init of_selftest(void)
 	of_selftest_parse_interrupts_extended();
 	of_selftest_match_node();
 	of_selftest_platform_populate();
+	of_selftest_overlay();
 	pr_info("end of selftest - %i passed, %i failed\n",
 		selftest_results.passed, selftest_results.failed);
 	return 0;
diff --git a/drivers/of/testcase-data/testcases.dtsi b/drivers/of/testcase-data/testcases.dtsi
index 6d8d980a..a54fd68 100644
--- a/drivers/of/testcase-data/testcases.dtsi
+++ b/drivers/of/testcase-data/testcases.dtsi
@@ -2,3 +2,4 @@
 #include "tests-interrupts.dtsi"
 #include "tests-match.dtsi"
 #include "tests-platform.dtsi"
+#include "tests-overlay.dtsi"
diff --git a/drivers/of/testcase-data/tests-overlay.dtsi b/drivers/of/testcase-data/tests-overlay.dtsi
new file mode 100644
index 0000000..75976da
--- /dev/null
+++ b/drivers/of/testcase-data/tests-overlay.dtsi
@@ -0,0 +1,180 @@
+
+/ {
+	testcase-data {
+		overlay-node {
+
+			/* test bus */
+			selftestbus: test-bus {
+				compatible = "simple-bus";
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				selftest100: test-selftest100 {
+					compatible = "selftest";
+					status = "okay";
+					reg = <100>;
+				};
+
+				selftest101: test-selftest101 {
+					compatible = "selftest";
+					status = "disabled";
+					reg = <101>;
+				};
+
+				selftest0: test-selftest0 {
+					compatible = "selftest";
+					status = "disabled";
+					reg = <0>;
+				};
+
+				selftest1: test-selftest1 {
+					compatible = "selftest";
+					status = "okay";
+					reg = <1>;
+				};
+
+				selftest2: test-selftest2 {
+					compatible = "selftest";
+					status = "disabled";
+					reg = <2>;
+				};
+
+				selftest3: test-selftest3 {
+					compatible = "selftest";
+					status = "okay";
+					reg = <3>;
+				};
+
+				selftest5: test-selftest5 {
+					compatible = "selftest";
+					status = "disabled";
+					reg = <5>;
+				};
+
+				selftest6: test-selftest6 {
+					compatible = "selftest";
+					status = "disabled";
+					reg = <6>;
+				};
+
+				selftest7: test-selftest7 {
+					compatible = "selftest";
+					status = "disabled";
+					reg = <7>;
+				};
+
+				selftest8: test-selftest8 {
+					compatible = "selftest";
+					status = "disabled";
+					reg = <8>;
+				};
+			};
+		};
+
+		/* test enable using absolute target path */
+		overlay0 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus/test-selftest0";
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
+
+		/* test disable using absolute target path */
+		overlay1 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus/test-selftest1";
+				__overlay__ {
+					status = "disabled";
+				};
+			};
+		};
+
+		/* test enable using label */
+		overlay2 {
+			fragment@0 {
+				target = <&selftest2>;
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
+
+		/* test disable using label */
+		overlay3 {
+			fragment@0 {
+				target = <&selftest3>;
+				__overlay__ {
+					status = "disabled";
+				};
+			};
+		};
+
+		/* test insertion of a full node */
+		overlay4 {
+			fragment@0 {
+				target = <&selftestbus>;
+				__overlay__ {
+
+					/* suppress DTC warning */
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					test-selftest4 {
+						compatible = "selftest";
+						status = "okay";
+						reg = <4>;
+					};
+				};
+			};
+		};
+
+		/* test overlay apply revert */
+		overlay5 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus/test-selftest5";
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
+
+		/* test overlays application and removal in sequence */
+		overlay6 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus/test-selftest6";
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
+		overlay7 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus/test-selftest7";
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
+
+		/* test overlays application and removal in bad sequence */
+		overlay8 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus/test-selftest8";
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
+		overlay9 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus/test-selftest8";
+				__overlay__ {
+					property-foo = "bar";
+				};
+			};
+		};
+
+	};
+};
-- 
1.7.12

[PATCH v7 4/9] OF: DT-Overlay configfs interface

[Pantelis Antoniou]

Add a runtime interface to using configfs for generic device tree overlay
usage.

A device-tree configfs entry is created in /config/device-tree/overlays

* To create an overlay you mkdir the directory:

	# mkdir /config/device-tree/overlays/foo

* Either you echo the overlay firmware file to the path property file.

	# echo foo.dtbo >/config/device-tree/overlays/foo/path

* Or you cat the contents of the overlay to the dtbo file

	# cat foo.dtbo >/config/device-tree/overlays/foo/dtbo

The overlay file will be applied.

To remove it simply rmdir the directory.

	# rmdir /config/device-tree/overlays/foo

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/Kconfig    |   7 ++
 drivers/of/Makefile   |   1 +
 drivers/of/configfs.c | 340 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 348 insertions(+)
 create mode 100644 drivers/of/configfs.c

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 5ebde98..1a2ff48 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -91,4 +91,11 @@ config OF_OVERLAY
 	select OF_DEVICE
 	select OF_RESOLVE
 
+config OF_CONFIGFS
+	bool "OpenFirmware Overlay ConfigFS interface"
+	select CONFIGFS_FS
+	select OF_OVERLAY
+	help
+	  Enable a simple user-space driver DT overlay interface.
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index a9ba136..4f067ca 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_OF_MTD)	+= of_mtd.o
 obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
 obj-$(CONFIG_OF_RESOLVE)  += resolver.o
 obj-$(CONFIG_OF_OVERLAY) += overlay.o
+obj-$(CONFIG_OF_CONFIGFS) += configfs.o
 
 CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
 CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
diff --git a/drivers/of/configfs.c b/drivers/of/configfs.c
new file mode 100644
index 0000000..d82d709
--- /dev/null
+++ b/drivers/of/configfs.c
@@ -0,0 +1,340 @@
+/*
+ * Configfs entries for device-tree
+ *
+ * Copyright (C) 2013 - Pantelis Antoniou <panto@antoniou-consulting.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+#include <linux/ctype.h>
+#include <linux/cpu.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/proc_fs.h>
+#include <linux/configfs.h>
+#include <linux/types.h>
+#include <linux/stat.h>
+#include <linux/limits.h>
+#include <linux/file.h>
+#include <linux/vmalloc.h>
+#include <linux/firmware.h>
+
+#include "of_private.h"
+
+#ifdef CONFIG_OF_OVERLAY
+
+struct cfs_overlay_item {
+	struct config_item	item;
+
+	char			path[PATH_MAX];
+
+	const struct firmware	*fw;
+	struct device_node	*overlay;
+	int			ov_id;
+
+	void			*dtbo;
+	int			dtbo_size;
+};
+
+static int create_overlay(struct cfs_overlay_item *overlay, void *blob)
+{
+	int err;
+
+	/* unflatten the tree */
+	of_fdt_unflatten_tree((void *)blob, &overlay->overlay);
+	if (overlay->overlay == NULL) {
+		pr_err("%s: failed to unflatten tree\n", __func__);
+		err = -EINVAL;
+		goto out_err;
+	}
+	pr_debug("%s: unflattened OK\n", __func__);
+
+	/* mark it as detached */
+	of_node_set_flag(overlay->overlay, OF_DETACHED);
+
+	/* perform resolution */
+	err = of_resolve(overlay->overlay);
+	if (err != 0) {
+		pr_err("%s: Failed to resolve tree\n", __func__);
+		goto out_err;
+	}
+	pr_debug("%s: resolved OK\n", __func__);
+
+	err = of_overlay_create(overlay->overlay);
+	if (err < 0) {
+		pr_err("%s: Failed to create overlay (err=%d)\n",
+				__func__, err);
+		goto out_err;
+	}
+	overlay->ov_id = err;
+
+out_err:
+	return err;
+}
+
+static inline struct cfs_overlay_item *to_cfs_overlay_item(
+		struct config_item *item)
+{
+	return item ? container_of(item, struct cfs_overlay_item, item) : NULL;
+}
+
+CONFIGFS_ATTR_STRUCT(cfs_overlay_item);
+#define CFS_OVERLAY_ITEM_ATTR(_name, _mode, _show, _store)	\
+struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
+	__CONFIGFS_ATTR(_name, _mode, _show, _store)
+#define CFS_OVERLAY_ITEM_ATTR_RO(_name, _show)	\
+struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
+	__CONFIGFS_ATTR_RO(_name, _show)
+
+CONFIGFS_BIN_ATTR_STRUCT(cfs_overlay_item);
+#define CFS_OVERLAY_ITEM_BIN_ATTR(_name, _mode, _read, _write, _priv, _max) \
+struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
+	__CONFIGFS_BIN_ATTR(_name, _mode, _read, _write, _priv, _max)
+#define CFS_OVERLAY_ITEM_BIN_ATTR_RO(_name, _read, _priv, _max)	\
+struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
+	__CONFIGFS_BIN_ATTR_RO(_name, _read, _priv, _max)
+
+static ssize_t cfs_overlay_item_path_show(struct cfs_overlay_item *overlay,
+		char *page)
+{
+	return sprintf(page, "%s\n", overlay->path);
+}
+
+static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
+		const char *page, size_t count)
+{
+	const char *p = page;
+	char *s;
+	int err;
+
+	/* if it's set do not allow changes */
+	if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
+		return -EPERM;
+
+	/* copy to path buffer (and make sure it's always zero terminated */
+	count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
+	overlay->path[sizeof(overlay->path) - 1] = '\0';
+
+	/* strip trailing newlines */
+	s = overlay->path + strlen(overlay->path);
+	while (s > overlay->path && *--s == '\n')
+		*s = '\0';
+
+	pr_debug("%s: path is '%s'\n", __func__, overlay->path);
+
+	err = request_firmware(&overlay->fw, overlay->path, NULL);
+	if (err != 0)
+		goto out_err;
+
+	err = create_overlay(overlay, (void *)overlay->fw->data);
+	if (err != 0)
+		goto out_err;
+
+	return count;
+
+out_err:
+
+	release_firmware(overlay->fw);
+	overlay->fw = NULL;
+
+	overlay->path[0] = '\0';
+	return err;
+}
+
+static ssize_t cfs_overlay_item_status_show(struct cfs_overlay_item *overlay,
+		char *page)
+{
+	return sprintf(page, "%s\n",
+			overlay->ov_id >= 0 ? "applied" : "unapplied");
+}
+
+CFS_OVERLAY_ITEM_ATTR(path, S_IRUGO | S_IWUSR,
+		cfs_overlay_item_path_show, cfs_overlay_item_path_store);
+CFS_OVERLAY_ITEM_ATTR_RO(status, cfs_overlay_item_status_show);
+
+static struct configfs_attribute *cfs_overlay_attrs[] = {
+	&cfs_overlay_item_attr_path.attr,
+	&cfs_overlay_item_attr_status.attr,
+	NULL,
+};
+
+ssize_t cfs_overlay_item_dtbo_read(struct cfs_overlay_item *overlay,
+		void *buf, size_t max_count)
+{
+	pr_debug("%s: buf=%p max_count=%u\n", __func__,
+			buf, max_count);
+
+	if (overlay->dtbo == NULL)
+		return 0;
+
+	/* copy if buffer provided */
+	if (buf != NULL) {
+		/* the buffer must be large enough */
+		if (overlay->dtbo_size > max_count)
+			return -ENOSPC;
+
+		memcpy(buf, overlay->dtbo, overlay->dtbo_size);
+	}
+
+	return overlay->dtbo_size;
+}
+
+ssize_t cfs_overlay_item_dtbo_write(struct cfs_overlay_item *overlay,
+		const void *buf, size_t count)
+{
+	int err;
+
+	/* if it's set do not allow changes */
+	if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
+		return -EPERM;
+
+	/* copy the contents */
+	overlay->dtbo = kmemdup(buf, count, GFP_KERNEL);
+	if (overlay->dtbo == NULL)
+		return -ENOMEM;
+
+	overlay->dtbo_size = count;
+
+	err = create_overlay(overlay, overlay->dtbo);
+	if (err != 0)
+		goto out_err;
+
+	return count;
+
+out_err:
+	kfree(overlay->dtbo);
+	overlay->dtbo = NULL;
+	overlay->dtbo_size = 0;
+
+	return err;
+}
+
+CFS_OVERLAY_ITEM_BIN_ATTR(dtbo, S_IRUGO | S_IWUSR,
+		cfs_overlay_item_dtbo_read, cfs_overlay_item_dtbo_write,
+		NULL, SZ_1M);
+
+static struct configfs_bin_attribute *cfs_overlay_bin_attrs[] = {
+	&cfs_overlay_item_bin_attr_dtbo.bin_attr,
+	NULL,
+};
+
+static void cfs_overlay_release(struct config_item *item)
+{
+	struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
+
+	if (overlay->ov_id >= 0)
+		of_overlay_destroy(overlay->ov_id);
+	if (overlay->fw)
+		release_firmware(overlay->fw);
+	/* kfree with NULL is safe */
+	kfree(overlay->dtbo);
+	kfree(overlay);
+}
+
+CONFIGFS_ATTR_OPS(cfs_overlay_item);
+CONFIGFS_BIN_ATTR_OPS(cfs_overlay_item);
+static struct configfs_item_operations cfs_overlay_item_ops = {
+	.release		= cfs_overlay_release,
+	.show_attribute		= cfs_overlay_item_attr_show,
+	.store_attribute	= cfs_overlay_item_attr_store,
+	.read_bin_attribute	= cfs_overlay_item_bin_attr_read,
+	.write_bin_attribute	= cfs_overlay_item_bin_attr_write,
+};
+
+static struct config_item_type cfs_overlay_type = {
+	.ct_item_ops	= &cfs_overlay_item_ops,
+	.ct_attrs	= cfs_overlay_attrs,
+	.ct_bin_attrs	= cfs_overlay_bin_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_item *cfs_overlay_group_make_item(
+		struct config_group *group, const char *name)
+{
+	struct cfs_overlay_item *overlay;
+
+	overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
+	if (!overlay)
+		return ERR_PTR(-ENOMEM);
+	overlay->ov_id = -1;
+
+	config_item_init_type_name(&overlay->item, name, &cfs_overlay_type);
+	return &overlay->item;
+}
+
+static void cfs_overlay_group_drop_item(struct config_group *group,
+		struct config_item *item)
+{
+	struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
+
+	config_item_put(&overlay->item);
+}
+
+static struct configfs_group_operations overlays_ops = {
+	.make_item	= cfs_overlay_group_make_item,
+	.drop_item	= cfs_overlay_group_drop_item,
+};
+
+static struct config_item_type overlays_type = {
+	.ct_group_ops   = &overlays_ops,
+	.ct_owner       = THIS_MODULE,
+};
+
+#endif /* CONFIG_OF_OVERLAY */
+
+static struct configfs_group_operations of_cfs_ops = {
+	/* empty - we don't allow anything to be created */
+};
+
+static struct config_item_type of_cfs_type = {
+	.ct_group_ops   = &of_cfs_ops,
+	.ct_owner       = THIS_MODULE,
+};
+
+struct config_group of_cfs_overlay_group;
+
+struct config_group *of_cfs_def_groups[] = {
+#ifdef CONFIG_OF_OVERLAY
+	&of_cfs_overlay_group,
+#endif
+	NULL
+};
+
+static struct configfs_subsystem of_cfs_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "device-tree",
+			.ci_type = &of_cfs_type,
+		},
+		.default_groups = of_cfs_def_groups,
+	},
+	.su_mutex = __MUTEX_INITIALIZER(of_cfs_subsys.su_mutex),
+};
+
+static int __init of_cfs_init(void)
+{
+	int ret;
+
+	pr_info("%s\n", __func__);
+
+	config_group_init(&of_cfs_subsys.su_group);
+#ifdef CONFIG_OF_OVERLAY
+	config_group_init_type_name(&of_cfs_overlay_group, "overlays",
+			&overlays_type);
+#endif
+
+	ret = configfs_register_subsystem(&of_cfs_subsys);
+	if (ret != 0) {
+		pr_err("%s: failed to register subsys\n", __func__);
+		goto out;
+	}
+	pr_info("%s: OK\n", __func__);
+out:
+	return ret;
+}
+late_initcall(of_cfs_init);
-- 
1.7.12

[PATCH v7 5/9] OF: platform: Add OF notifier handler

[Pantelis Antoniou]

Add OF notifier handler needed for creating/destroying platform devices
according to dynamic runtime changes in the DT live tree.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/base/platform.c     | 18 ++++++++++---
 drivers/of/platform.c       | 62 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_platform.h | 10 ++++++++
 3 files changed, 87 insertions(+), 3 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 9e9227e..ebba70b 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -938,10 +938,22 @@ int __init platform_bus_init(void)
 
 	error = device_register(&platform_bus);
 	if (error)
-		return error;
-	error =  bus_register(&platform_bus_type);
+		goto err_out;
+
+	error = bus_register(&platform_bus_type);
+	if (error)
+		goto err_unreg_dev;
+
+	error = of_platform_register_reconfig_notifier();
 	if (error)
-		device_unregister(&platform_bus);
+		goto err_unreg_bus;
+
+	return 0;
+err_unreg_bus:
+	bus_unregister(&platform_bus_type);
+err_unreg_dev:
+	device_unregister(&platform_bus);
+err_out:
 	return error;
 }
 
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 0197725..b5d21e2 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -547,4 +547,66 @@ void of_platform_depopulate(struct device *parent)
 }
 EXPORT_SYMBOL_GPL(of_platform_depopulate);
 
+#ifdef CONFIG_OF_DYNAMIC
+
+static struct notifier_block platform_of_notifier;
+
+static int of_platform_notify(struct notifier_block *nb,
+				unsigned long action, void *arg)
+{
+	struct device_node *dn;
+	struct platform_device *pdev_parent, *pdev;
+	bool children_left;
+
+	if (action == OF_RECONFIG_DYNAMIC_CREATE_DEV) {
+
+		dn = arg;
+
+		/* verify that the parent is a bus */
+		if (!of_match_node(of_default_bus_match_table, dn->parent))
+			return NOTIFY_OK;	/* not for us */
+
+		/* pdev_parent may be NULL when no bus platform device */
+		pdev_parent = of_find_device_by_node(dn->parent);
+		pdev = of_platform_device_create(dn, NULL,
+				pdev_parent ? &pdev_parent->dev : NULL);
+		of_dev_put(pdev_parent);
+
+		if (pdev == NULL) {
+			pr_err("%s: failed to create for '%s'\n",
+					__func__, dn->full_name);
+			/* of_platform_device_create tosses the error code */
+			return notifier_from_errno(-EINVAL);
+		}
+
+	} else if (action == OF_RECONFIG_DYNAMIC_DESTROY_DEV) {
+
+		dn = arg;
+
+		/* find our device by node */
+		pdev = of_find_device_by_node(dn);
+		if (pdev == NULL)
+			return NOTIFY_OK;	/* no? not meant for us */
+
+		/* unregister takes one ref away */
+		of_platform_device_destroy(&pdev->dev, &children_left);
+
+		/* and put the reference of the find */
+		of_dev_put(pdev);
+
+	} else
+		return NOTIFY_OK;
+
+	return NOTIFY_STOP;
+}
+
+int of_platform_register_reconfig_notifier(void)
+{
+	platform_of_notifier.notifier_call = of_platform_notify;
+	return of_reconfig_notifier_register(&platform_of_notifier);
+}
+EXPORT_SYMBOL_GPL(of_platform_register_reconfig_notifier);
+
+#endif
+
 #endif /* CONFIG_OF_ADDRESS */
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index c2b0627..01fe5d6 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -84,4 +84,14 @@ static inline int of_platform_populate(struct device_node *root,
 static inline void of_platform_depopulate(struct device *parent) { }
 #endif
 
+#ifdef CONFIG_OF_DYNAMIC
+extern int of_platform_register_reconfig_notifier(void);
+#else
+static inline int of_platform_register_reconfig_notifier(void)
+{
+	return 0;
+}
+#endif
+
+
 #endif	/* _LINUX_OF_PLATFORM_H */
-- 
1.7.12

[PATCH v7 6/9] of: i2c: Export single device registration method

[Pantelis Antoniou]

Dynamically inserting i2c client device nodes requires the use
of a single device registration method. Rework and export it.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/i2c/i2c-core.c | 99 +++++++++++++++++++++++++++-----------------------
 include/linux/i2c.h    | 10 +++++
 2 files changed, 64 insertions(+), 45 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7c7f4b8..6e47f73 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -50,6 +50,7 @@
 #include <linux/acpi.h>
 #include <linux/jump_label.h>
 #include <asm/uaccess.h>
+#include <linux/err.h>
 
 #include "i2c-core.h"
 
@@ -997,63 +998,71 @@ static void i2c_scan_static_board_info(struct i2c_adapter *adapter)
 /* OF support code */
 
 #if IS_ENABLED(CONFIG_OF)
-static void of_i2c_register_devices(struct i2c_adapter *adap)
+struct i2c_client *
+of_i2c_register_device(struct i2c_adapter *adap,
+		struct device_node *node)
 {
-	void *result;
-	struct device_node *node;
+	struct i2c_client *result;
+	struct i2c_board_info info = {};
+	struct dev_archdata dev_ad = {};
+	const __be32 *addr;
+	int len;
 
-	/* Only register child devices if the adapter has a node pointer set */
-	if (!adap->dev.of_node)
-		return;
+	dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name);
 
-	dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");
+	if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) {
+		dev_err(&adap->dev, "of_i2c: modalias failure on %s\n",
+			node->full_name);
+		return ERR_PTR(-EINVAL);
+	}
 
-	for_each_available_child_of_node(adap->dev.of_node, node) {
-		struct i2c_board_info info = {};
-		struct dev_archdata dev_ad = {};
-		const __be32 *addr;
-		int len;
+	addr = of_get_property(node, "reg", &len);
+	if (!addr || (len < sizeof(int))) {
+		dev_err(&adap->dev, "of_i2c: invalid reg on %s\n",
+			node->full_name);
+		return ERR_PTR(-EINVAL);
+	}
 
-		dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name);
+	info.addr = be32_to_cpup(addr);
+	if (info.addr > (1 << 10) - 1) {
+		dev_err(&adap->dev, "of_i2c: invalid addr=%x on %s\n",
+			info.addr, node->full_name);
+		return ERR_PTR(-EINVAL);
+	}
 
-		if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) {
-			dev_err(&adap->dev, "of_i2c: modalias failure on %s\n",
-				node->full_name);
-			continue;
-		}
+	info.irq = irq_of_parse_and_map(node, 0);
+	info.of_node = of_node_get(node);
+	info.archdata = &dev_ad;
 
-		addr = of_get_property(node, "reg", &len);
-		if (!addr || (len < sizeof(int))) {
-			dev_err(&adap->dev, "of_i2c: invalid reg on %s\n",
-				node->full_name);
-			continue;
-		}
+	if (of_get_property(node, "wakeup-source", NULL))
+		info.flags |= I2C_CLIENT_WAKE;
 
-		info.addr = be32_to_cpup(addr);
-		if (info.addr > (1 << 10) - 1) {
-			dev_err(&adap->dev, "of_i2c: invalid addr=%x on %s\n",
-				info.addr, node->full_name);
-			continue;
-		}
+	request_module("%s%s", I2C_MODULE_PREFIX, info.type);
 
-		info.irq = irq_of_parse_and_map(node, 0);
-		info.of_node = of_node_get(node);
-		info.archdata = &dev_ad;
+	result = i2c_new_device(adap, &info);
+	if (result == NULL) {
+		dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
+			node->full_name);
+		of_node_put(node);
+		irq_dispose_mapping(info.irq);
+		return ERR_PTR(-EINVAL);
+	}
+	return result;
+}
+EXPORT_SYMBOL(of_i2c_register_device);
 
-		if (of_get_property(node, "wakeup-source", NULL))
-			info.flags |= I2C_CLIENT_WAKE;
+static void of_i2c_register_devices(struct i2c_adapter *adap)
+{
+	struct device_node *node;
 
-		request_module("%s%s", I2C_MODULE_PREFIX, info.type);
+	/* Only register child devices if the adapter has a node pointer set */
+	if (!adap->dev.of_node)
+		return;
 
-		result = i2c_new_device(adap, &info);
-		if (result == NULL) {
-			dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
-				node->full_name);
-			of_node_put(node);
-			irq_dispose_mapping(info.irq);
-			continue;
-		}
-	}
+	dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");
+
+	for_each_available_child_of_node(adap->dev.of_node, node)
+		of_i2c_register_device(adap, node);
 }
 
 static int of_dev_node_match(struct device *dev, void *data)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..22a8f44 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -558,6 +558,9 @@ static inline int i2c_adapter_id(struct i2c_adapter *adap)
 #endif /* I2C */
 
 #if IS_ENABLED(CONFIG_OF)
+struct i2c_client *
+of_i2c_register_device(struct i2c_adapter *adap, struct device_node *node);
+
 /* must call put_device() when done with returned i2c_client device */
 extern struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
 
@@ -566,6 +569,13 @@ extern struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
 
 #else
 
+static inline struct i2c_client *
+of_i2c_register_device(struct i2c_adapter *adap,
+		struct device_node *node)
+{
+	return ERR_PTR(-ENODEV);
+}
+
 static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
 {
 	return NULL;
-- 
1.7.12

[PATCH v7 7/9] OF: i2c: Add OF notifier handler

[Pantelis Antoniou]

Add OF notifier handler needed for creating/destroying i2c devices
according to dynamic runtime changes in the DT live tree.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/i2c/i2c-core.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 6e47f73..8df7cd0 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1097,6 +1097,7 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
 	return i2c_verify_adapter(dev);
 }
 EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
+
 #else
 static void of_i2c_register_devices(struct i2c_adapter *adap) { }
 #endif /* CONFIG_OF */
@@ -1675,6 +1676,57 @@ void i2c_clients_command(struct i2c_adapter *adap, unsigned int cmd, void *arg)
 }
 EXPORT_SYMBOL(i2c_clients_command);
 
+#if IS_ENABLED(CONFIG_OF)
+
+static int of_i2c_notify(struct notifier_block *nb,
+				unsigned long action, void *arg)
+{
+	struct device_node *dn;
+	struct i2c_adapter *adap;
+	struct i2c_client *client;
+
+	if (action == OF_RECONFIG_DYNAMIC_CREATE_DEV) {
+
+		dn = arg;
+
+		adap = of_find_i2c_adapter_by_node(dn->parent);
+		if (adap == NULL)
+			return NOTIFY_OK;	/* not for us */
+
+		client = of_i2c_register_device(adap, dn);
+		put_device(&adap->dev);
+
+		if (IS_ERR(client)) {
+			pr_err("%s: failed to create for '%s'\n",
+					__func__, dn->full_name);
+			return notifier_from_errno(PTR_ERR(client));
+		}
+
+	} else if (action == OF_RECONFIG_DYNAMIC_DESTROY_DEV) {
+
+		dn = arg;
+
+		/* find our device by node */
+		client = of_find_i2c_device_by_node(dn);
+		if (client == NULL)
+			return NOTIFY_OK;	/* no? not meant for us */
+
+		/* unregister takes one ref away */
+		i2c_unregister_device(client);
+
+		/* and put the reference of the find */
+		put_device(&client->dev);
+
+	} else
+		return NOTIFY_OK;
+
+	return NOTIFY_STOP;
+}
+
+static struct notifier_block i2c_of_notifier;
+
+#endif
+
 static int __init i2c_init(void)
 {
 	int retval;
@@ -1692,8 +1744,19 @@ static int __init i2c_init(void)
 	retval = i2c_add_driver(&dummy_driver);
 	if (retval)
 		goto class_err;
-	return 0;
 
+#if IS_ENABLED(CONFIG_OF)
+	i2c_of_notifier.notifier_call = of_i2c_notify;
+	retval = of_reconfig_notifier_register(&i2c_of_notifier);
+	if (retval)
+		goto notifier_err;
+#endif
+
+	return 0;
+#if IS_ENABLED(CONFIG_OF)
+notifier_err:
+	i2c_del_driver(&dummy_driver);
+#endif
 class_err:
 #ifdef CONFIG_I2C_COMPAT
 	class_compat_unregister(i2c_adapter_compat_class);
-- 
1.7.12

[PATCH v7 8/9] of: spi: Export single device registration method and accessors

[Pantelis Antoniou]

Dynamically inserting spi device nodes requires the use of a single
device registration method. Rework and export it.

Methods to lookup a device/master using a device node are added
as well, of_find_spi_master_by_node() & of_find_spi_device_by_node().

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/spi/spi.c | 256 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 158 insertions(+), 98 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index d4f9670..cd98bf2 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1200,6 +1200,123 @@ err_init_queue:
 /*-------------------------------------------------------------------------*/
 
 #if defined(CONFIG_OF)
+
+static struct spi_device *
+of_register_spi_device(struct spi_master *master, struct device_node *node)
+{
+	struct spi_device *spi;
+	struct device_node *nc;
+	int rc;
+	u32 value;
+
+	/* Alloc an spi_device */
+	spi = spi_alloc_device(master);
+	if (!spi) {
+		dev_err(&master->dev, "spi_device alloc error for %s\n",
+			nc->full_name);
+		rc = -ENOMEM;
+		goto err_out;
+	}
+
+	/* Select device driver */
+	rc = of_modalias_node(nc, spi->modalias,
+				sizeof(spi->modalias));
+	if (rc < 0) {
+		dev_err(&master->dev, "cannot find modalias for %s\n",
+			nc->full_name);
+		goto err_out;
+	}
+
+	/* Device address */
+	rc = of_property_read_u32(nc, "reg", &value);
+	if (rc) {
+		dev_err(&master->dev, "%s has no valid 'reg' property (%d)\n",
+			nc->full_name, rc);
+		goto err_out;
+	}
+	spi->chip_select = value;
+
+	/* Mode (clock phase/polarity/etc.) */
+	if (of_find_property(nc, "spi-cpha", NULL))
+		spi->mode |= SPI_CPHA;
+	if (of_find_property(nc, "spi-cpol", NULL))
+		spi->mode |= SPI_CPOL;
+	if (of_find_property(nc, "spi-cs-high", NULL))
+		spi->mode |= SPI_CS_HIGH;
+	if (of_find_property(nc, "spi-3wire", NULL))
+		spi->mode |= SPI_3WIRE;
+	if (of_find_property(nc, "spi-lsb-first", NULL))
+		spi->mode |= SPI_LSB_FIRST;
+
+	/* Device DUAL/QUAD mode */
+	if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
+		switch (value) {
+		case 1:
+			break;
+		case 2:
+			spi->mode |= SPI_TX_DUAL;
+			break;
+		case 4:
+			spi->mode |= SPI_TX_QUAD;
+			break;
+		default:
+			dev_warn(&master->dev,
+				"spi-tx-bus-width %d not supported\n",
+				value);
+			break;
+		}
+	}
+
+	if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) {
+		switch (value) {
+		case 1:
+			break;
+		case 2:
+			spi->mode |= SPI_RX_DUAL;
+			break;
+		case 4:
+			spi->mode |= SPI_RX_QUAD;
+			break;
+		default:
+			dev_warn(&master->dev,
+				"spi-rx-bus-width %d not supported\n",
+				value);
+			break;
+		}
+	}
+
+	/* Device speed */
+	rc = of_property_read_u32(nc, "spi-max-frequency", &value);
+	if (rc) {
+		dev_err(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n",
+			nc->full_name, rc);
+		goto err_out;
+	}
+	spi->max_speed_hz = value;
+
+	/* IRQ */
+	spi->irq = irq_of_parse_and_map(nc, 0);
+
+	/* Store a pointer to the node in the device structure */
+	of_node_get(nc);
+	spi->dev.of_node = nc;
+
+	/* Register the new device */
+	request_module("%s%s", SPI_MODULE_PREFIX, spi->modalias);
+	rc = spi_add_device(spi);
+	if (rc) {
+		dev_err(&master->dev, "spi_device register error %s\n",
+			nc->full_name);
+		goto err_out;
+	}
+
+	return spi;
+
+err_out:
+	spi_dev_put(spi);
+	return ERR_PTR(rc);
+}
+
 /**
  * of_register_spi_devices() - Register child devices onto the SPI bus
  * @master:	Pointer to spi_master device
@@ -1209,120 +1326,63 @@ err_init_queue:
  */
 static void of_register_spi_devices(struct spi_master *master)
 {
-	struct spi_device *spi;
 	struct device_node *nc;
-	int rc;
-	u32 value;
+	struct spi_device *spi;
 
 	if (!master->dev.of_node)
 		return;
 
 	for_each_available_child_of_node(master->dev.of_node, nc) {
-		/* Alloc an spi_device */
-		spi = spi_alloc_device(master);
-		if (!spi) {
-			dev_err(&master->dev, "spi_device alloc error for %s\n",
+		spi = of_register_spi_device(master, nc);
+		if (IS_ERR(spi))
+			dev_warn(&master->dev, "Failed to create SPI device for %s\n",
 				nc->full_name);
-			spi_dev_put(spi);
-			continue;
-		}
+	}
+}
 
-		/* Select device driver */
-		if (of_modalias_node(nc, spi->modalias,
-				     sizeof(spi->modalias)) < 0) {
-			dev_err(&master->dev, "cannot find modalias for %s\n",
-				nc->full_name);
-			spi_dev_put(spi);
-			continue;
-		}
+static int of_dev_node_match(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
 
-		/* Device address */
-		rc = of_property_read_u32(nc, "reg", &value);
-		if (rc) {
-			dev_err(&master->dev, "%s has no valid 'reg' property (%d)\n",
-				nc->full_name, rc);
-			spi_dev_put(spi);
-			continue;
-		}
-		spi->chip_select = value;
-
-		/* Mode (clock phase/polarity/etc.) */
-		if (of_find_property(nc, "spi-cpha", NULL))
-			spi->mode |= SPI_CPHA;
-		if (of_find_property(nc, "spi-cpol", NULL))
-			spi->mode |= SPI_CPOL;
-		if (of_find_property(nc, "spi-cs-high", NULL))
-			spi->mode |= SPI_CS_HIGH;
-		if (of_find_property(nc, "spi-3wire", NULL))
-			spi->mode |= SPI_3WIRE;
-		if (of_find_property(nc, "spi-lsb-first", NULL))
-			spi->mode |= SPI_LSB_FIRST;
-
-		/* Device DUAL/QUAD mode */
-		if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
-			switch (value) {
-			case 1:
-				break;
-			case 2:
-				spi->mode |= SPI_TX_DUAL;
-				break;
-			case 4:
-				spi->mode |= SPI_TX_QUAD;
-				break;
-			default:
-				dev_warn(&master->dev,
-					 "spi-tx-bus-width %d not supported\n",
-					 value);
-				break;
-			}
-		}
+/* bah; the match functions differ just by const-ness */
+static int of_dev_node_match_const(struct device *dev, const void *data)
+{
+	return dev->of_node == data;
+}
 
-		if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) {
-			switch (value) {
-			case 1:
-				break;
-			case 2:
-				spi->mode |= SPI_RX_DUAL;
-				break;
-			case 4:
-				spi->mode |= SPI_RX_QUAD;
-				break;
-			default:
-				dev_warn(&master->dev,
-					 "spi-rx-bus-width %d not supported\n",
-					 value);
-				break;
-			}
-		}
+/* must call put_device() when done with returned spi_device device */
+struct spi_device *of_find_spi_device_by_node(struct device_node *node)
+{
+	struct device *dev;
 
-		/* Device speed */
-		rc = of_property_read_u32(nc, "spi-max-frequency", &value);
-		if (rc) {
-			dev_err(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n",
-				nc->full_name, rc);
-			spi_dev_put(spi);
-			continue;
-		}
-		spi->max_speed_hz = value;
+	dev = bus_find_device(&spi_bus_type, NULL, node,
+					 of_dev_node_match);
+	if (!dev)
+		return NULL;
 
-		/* IRQ */
-		spi->irq = irq_of_parse_and_map(nc, 0);
+	return to_spi_device(dev);
+}
+EXPORT_SYMBOL(of_find_spi_device_by_node);
 
-		/* Store a pointer to the node in the device structure */
-		of_node_get(nc);
-		spi->dev.of_node = nc;
+/* forward decl */
+static struct class spi_master_class;
 
-		/* Register the new device */
-		request_module("%s%s", SPI_MODULE_PREFIX, spi->modalias);
-		rc = spi_add_device(spi);
-		if (rc) {
-			dev_err(&master->dev, "spi_device register error %s\n",
-				nc->full_name);
-			spi_dev_put(spi);
-		}
+/* the spi masters are not using spi_bus, so we find it with another way */
+struct spi_master *of_find_spi_master_by_node(struct device_node *node)
+{
+	struct device *dev;
 
-	}
+	dev = class_find_device(&spi_master_class, NULL, node,
+				of_dev_node_match_const);
+	if (!dev)
+		return NULL;
+
+	/* reference got in class_find_device */
+	return container_of(dev, struct spi_master, dev);
 }
+EXPORT_SYMBOL(of_find_spi_master_by_node);
+
 #else
 static void of_register_spi_devices(struct spi_master *master) { }
 #endif
-- 
1.7.12

[PATCH v7 9/9] OF: spi: Add OF notifier handler

[Pantelis Antoniou]

Add OF notifier handler needed for creating/destroying spi devices
according to dynamic runtime changes in the DT live tree.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/spi/spi.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index cd98bf2..d2a02ba 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2343,6 +2343,57 @@ EXPORT_SYMBOL_GPL(spi_write_then_read);
 
 /*-------------------------------------------------------------------------*/
 
+#if IS_ENABLED(CONFIG_OF)
+
+static int of_spi_notify(struct notifier_block *nb,
+				unsigned long action, void *arg)
+{
+	struct device_node *dn;
+	struct spi_master *master;
+	struct spi_device *spi;
+
+	if (action == OF_RECONFIG_DYNAMIC_CREATE_DEV) {
+
+		dn = arg;
+
+		master = of_find_spi_master_by_node(dn->parent);
+		if (master == NULL)
+			return NOTIFY_OK;	/* not for us */
+
+		spi = of_register_spi_device(master, dn);
+		put_device(&master->dev);
+
+		if (IS_ERR(spi)) {
+			pr_err("%s: failed to create for '%s'\n",
+					__func__, dn->full_name);
+			return notifier_from_errno(PTR_ERR(spi));
+		}
+
+	} else if (action == OF_RECONFIG_DYNAMIC_DESTROY_DEV) {
+
+		dn = arg;
+
+		/* find our device by node */
+		spi = of_find_spi_device_by_node(dn);
+		if (spi == NULL)
+			return NOTIFY_OK;	/* no? not meant for us */
+
+		/* unregister takes one ref away */
+		spi_unregister_device(spi);
+
+		/* and put the reference of the find */
+		put_device(&spi->dev);
+
+	} else
+		return NOTIFY_OK;
+
+	return NOTIFY_STOP;
+}
+
+static struct notifier_block spi_of_notifier;
+
+#endif
+
 static int __init spi_init(void)
 {
 	int	status;
@@ -2360,8 +2411,19 @@ static int __init spi_init(void)
 	status = class_register(&spi_master_class);
 	if (status < 0)
 		goto err2;
-	return 0;
 
+#if IS_ENABLED(CONFIG_OF)
+	spi_of_notifier.notifier_call = of_spi_notify;
+	status = of_reconfig_notifier_register(&spi_of_notifier);
+	if (status)
+		goto err3;
+#endif
+
+	return 0;
+#if IS_ENABLED(CONFIG_OF)
+err3:
+	class_unregister(&spi_master_class);
+#endif
 err2:
 	bus_unregister(&spi_bus_type);
 err1:
-- 
1.7.12

Re: [PATCH v7 8/9] of: spi: Export single device registration method and accessors

[Alexander Sverdlin]

Hello Pantelis,

On 04/07/14 18:59, ext Pantelis Antoniou wrote:
> Dynamically inserting spi device nodes requires the use of a single
> device registration method. Rework and export it.
> 
> Methods to lookup a device/master using a device node are added
> as well, of_find_spi_master_by_node() & of_find_spi_device_by_node().
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  drivers/spi/spi.c | 256 +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 158 insertions(+), 98 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index d4f9670..cd98bf2 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1200,6 +1200,123 @@ err_init_queue:
>  /*-------------------------------------------------------------------------*/
>  
>  #if defined(CONFIG_OF)
> +
> +static struct spi_device *
> +of_register_spi_device(struct spi_master *master, struct device_node *node)

During the test Wladislav has found that node is actually not used,

> +{
> +	struct spi_device *spi;
> +	struct device_node *nc;

but non-initialized nc is used further in the code.
Should not nc be a parameter of the function instead of a local variable?

> +	int rc;
> +	u32 value;
> +
> +	/* Alloc an spi_device */
> +	spi = spi_alloc_device(master);
> +	if (!spi) {
> +		dev_err(&master->dev, "spi_device alloc error for %s\n",
> +			nc->full_name);
> +		rc = -ENOMEM;
> +		goto err_out;
> +	}
> +
> +	/* Select device driver */
> +	rc = of_modalias_node(nc, spi->modalias,
> +				sizeof(spi->modalias));
> +	if (rc < 0) {
> +		dev_err(&master->dev, "cannot find modalias for %s\n",
> +			nc->full_name);
> +		goto err_out;
> +	}
> +
> +	/* Device address */
> +	rc = of_property_read_u32(nc, "reg", &value);
> +	if (rc) {
> +		dev_err(&master->dev, "%s has no valid 'reg' property (%d)\n",
> +			nc->full_name, rc);
> +		goto err_out;
> +	}
> +	spi->chip_select = value;
> +
> +	/* Mode (clock phase/polarity/etc.) */
> +	if (of_find_property(nc, "spi-cpha", NULL))
> +		spi->mode |= SPI_CPHA;
> +	if (of_find_property(nc, "spi-cpol", NULL))
> +		spi->mode |= SPI_CPOL;
> +	if (of_find_property(nc, "spi-cs-high", NULL))
> +		spi->mode |= SPI_CS_HIGH;
> +	if (of_find_property(nc, "spi-3wire", NULL))
> +		spi->mode |= SPI_3WIRE;
> +	if (of_find_property(nc, "spi-lsb-first", NULL))
> +		spi->mode |= SPI_LSB_FIRST;
> +
> +	/* Device DUAL/QUAD mode */
> +	if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
> +		switch (value) {
> +		case 1:
> +			break;
> +		case 2:
> +			spi->mode |= SPI_TX_DUAL;
> +			break;
> +		case 4:
> +			spi->mode |= SPI_TX_QUAD;
> +			break;
> +		default:
> +			dev_warn(&master->dev,
> +				"spi-tx-bus-width %d not supported\n",
> +				value);
> +			break;
> +		}
> +	}
> +
> +	if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) {
> +		switch (value) {
> +		case 1:
> +			break;
> +		case 2:
> +			spi->mode |= SPI_RX_DUAL;
> +			break;
> +		case 4:
> +			spi->mode |= SPI_RX_QUAD;
> +			break;
> +		default:
> +			dev_warn(&master->dev,
> +				"spi-rx-bus-width %d not supported\n",
> +				value);
> +			break;
> +		}
> +	}
> +
> +	/* Device speed */
> +	rc = of_property_read_u32(nc, "spi-max-frequency", &value);
> +	if (rc) {
> +		dev_err(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n",
> +			nc->full_name, rc);
> +		goto err_out;
> +	}
> +	spi->max_speed_hz = value;
> +
> +	/* IRQ */
> +	spi->irq = irq_of_parse_and_map(nc, 0);
> +
> +	/* Store a pointer to the node in the device structure */
> +	of_node_get(nc);
> +	spi->dev.of_node = nc;
> +
> +	/* Register the new device */
> +	request_module("%s%s", SPI_MODULE_PREFIX, spi->modalias);
> +	rc = spi_add_device(spi);
> +	if (rc) {
> +		dev_err(&master->dev, "spi_device register error %s\n",
> +			nc->full_name);
> +		goto err_out;
> +	}
> +
> +	return spi;
> +
> +err_out:
> +	spi_dev_put(spi);
> +	return ERR_PTR(rc);
> +}
> +
>  /**
>   * of_register_spi_devices() - Register child devices onto the SPI bus
>   * @master:	Pointer to spi_master device
> @@ -1209,120 +1326,63 @@ err_init_queue:
>   */
>  static void of_register_spi_devices(struct spi_master *master)
>  {
> -	struct spi_device *spi;
>  	struct device_node *nc;
> -	int rc;
> -	u32 value;
> +	struct spi_device *spi;
>  
>  	if (!master->dev.of_node)
>  		return;
>  
>  	for_each_available_child_of_node(master->dev.of_node, nc) {
> -		/* Alloc an spi_device */
> -		spi = spi_alloc_device(master);
> -		if (!spi) {
> -			dev_err(&master->dev, "spi_device alloc error for %s\n",
> +		spi = of_register_spi_device(master, nc);
> +		if (IS_ERR(spi))
> +			dev_warn(&master->dev, "Failed to create SPI device for %s\n",
>  				nc->full_name);
> -			spi_dev_put(spi);
> -			continue;
> -		}
> +	}
> +}
>  
> -		/* Select device driver */
> -		if (of_modalias_node(nc, spi->modalias,
> -				     sizeof(spi->modalias)) < 0) {
> -			dev_err(&master->dev, "cannot find modalias for %s\n",
> -				nc->full_name);
> -			spi_dev_put(spi);
> -			continue;
> -		}
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> +	return dev->of_node == data;
> +}
>  
> -		/* Device address */
> -		rc = of_property_read_u32(nc, "reg", &value);
> -		if (rc) {
> -			dev_err(&master->dev, "%s has no valid 'reg' property (%d)\n",
> -				nc->full_name, rc);
> -			spi_dev_put(spi);
> -			continue;
> -		}
> -		spi->chip_select = value;
> -
> -		/* Mode (clock phase/polarity/etc.) */
> -		if (of_find_property(nc, "spi-cpha", NULL))
> -			spi->mode |= SPI_CPHA;
> -		if (of_find_property(nc, "spi-cpol", NULL))
> -			spi->mode |= SPI_CPOL;
> -		if (of_find_property(nc, "spi-cs-high", NULL))
> -			spi->mode |= SPI_CS_HIGH;
> -		if (of_find_property(nc, "spi-3wire", NULL))
> -			spi->mode |= SPI_3WIRE;
> -		if (of_find_property(nc, "spi-lsb-first", NULL))
> -			spi->mode |= SPI_LSB_FIRST;
> -
> -		/* Device DUAL/QUAD mode */
> -		if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
> -			switch (value) {
> -			case 1:
> -				break;
> -			case 2:
> -				spi->mode |= SPI_TX_DUAL;
> -				break;
> -			case 4:
> -				spi->mode |= SPI_TX_QUAD;
> -				break;
> -			default:
> -				dev_warn(&master->dev,
> -					 "spi-tx-bus-width %d not supported\n",
> -					 value);
> -				break;
> -			}
> -		}
> +/* bah; the match functions differ just by const-ness */
> +static int of_dev_node_match_const(struct device *dev, const void *data)
> +{
> +	return dev->of_node == data;
> +}
>  
> -		if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) {
> -			switch (value) {
> -			case 1:
> -				break;
> -			case 2:
> -				spi->mode |= SPI_RX_DUAL;
> -				break;
> -			case 4:
> -				spi->mode |= SPI_RX_QUAD;
> -				break;
> -			default:
> -				dev_warn(&master->dev,
> -					 "spi-rx-bus-width %d not supported\n",
> -					 value);
> -				break;
> -			}
> -		}
> +/* must call put_device() when done with returned spi_device device */
> +struct spi_device *of_find_spi_device_by_node(struct device_node *node)
> +{
> +	struct device *dev;
>  
> -		/* Device speed */
> -		rc = of_property_read_u32(nc, "spi-max-frequency", &value);
> -		if (rc) {
> -			dev_err(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n",
> -				nc->full_name, rc);
> -			spi_dev_put(spi);
> -			continue;
> -		}
> -		spi->max_speed_hz = value;
> +	dev = bus_find_device(&spi_bus_type, NULL, node,
> +					 of_dev_node_match);
> +	if (!dev)
> +		return NULL;
>  
> -		/* IRQ */
> -		spi->irq = irq_of_parse_and_map(nc, 0);
> +	return to_spi_device(dev);
> +}
> +EXPORT_SYMBOL(of_find_spi_device_by_node);
>  
> -		/* Store a pointer to the node in the device structure */
> -		of_node_get(nc);
> -		spi->dev.of_node = nc;
> +/* forward decl */
> +static struct class spi_master_class;
>  
> -		/* Register the new device */
> -		request_module("%s%s", SPI_MODULE_PREFIX, spi->modalias);
> -		rc = spi_add_device(spi);
> -		if (rc) {
> -			dev_err(&master->dev, "spi_device register error %s\n",
> -				nc->full_name);
> -			spi_dev_put(spi);
> -		}
> +/* the spi masters are not using spi_bus, so we find it with another way */
> +struct spi_master *of_find_spi_master_by_node(struct device_node *node)
> +{
> +	struct device *dev;
>  
> -	}
> +	dev = class_find_device(&spi_master_class, NULL, node,
> +				of_dev_node_match_const);
> +	if (!dev)
> +		return NULL;
> +
> +	/* reference got in class_find_device */
> +	return container_of(dev, struct spi_master, dev);
>  }
> +EXPORT_SYMBOL(of_find_spi_master_by_node);
> +
>  #else
>  static void of_register_spi_devices(struct spi_master *master) { }
>  #endif
> 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v7 1/9] OF: Introduce Device Tree resolve support.

[Grant Likely]

On Fri,  4 Jul 2014 19:59:20 +0300
, Pantelis Antoniou <pantelis.antoniou@konsulko.com>
 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.
> 
> Export of of_resolve and bug fix of double free by
> 	Guenter Roeck <groeck@juniper.net>
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

Hi Pantelis,

I suspect you've got a slightly updated version of this patch not posted
yet, but I do have a few comments. Overall they're pretty minor.

> ---
>  .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>  drivers/of/Kconfig                                 |   6 +
>  drivers/of/Makefile                                |   1 +
>  drivers/of/resolver.c                              | 403 +++++++++++++++++++++
>  include/linux/of.h                                 |  16 +
>  5 files changed, 451 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 2dcb054..0236a4e 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -78,4 +78,10 @@ config OF_RESERVED_MEM
>  	help
>  	  Helpers to allow for reservation of memory regions
>  
> +config OF_RESOLVE
> +	bool
> +	depends on OF && !SPARC

What part of this code breaks on SPARC?

> +	select OF_DYNAMIC
> +	select OF_DEVICE

This code doesn't appear to depend on either OF_DYNAMIC or OF_DEVICE. You
should be able to drop the above 2 selects (at least it worked when I
tried it here)

> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index 9a68cc9..35a148a 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -12,6 +12,7 @@ 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_RESERVED_MEM) += of_reserved_mem.o
> +obj-$(CONFIG_OF_RESOLVE)  += resolver.o
>  
>  CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
>  CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> new file mode 100644
> index 0000000..18da5ca
> --- /dev/null
> +++ b/drivers/of/resolver.c
> @@ -0,0 +1,403 @@
> +/*
> + * 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/string.h>
> +#include <linux/ctype.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +
> +/**
> + * Find a node with the give full name by recursively following any of
> + * the child node links.
> + */
> +static 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;
> +}
> +
> +/*
> + * Find live tree's maximum phandle value.
> + */
> +static phandle of_get_tree_max_phandle(void)
> +{
> +	struct device_node *node;
> +	phandle phandle;
> +	unsigned long flags;
> +
> +	/* now search recursively */
> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +	phandle = 0;
> +	for_each_of_allnodes(node) {
> +		if (node->phandle != OF_PHANDLE_ILLEGAL &&
> +				node->phandle > phandle)
> +			phandle = node->phandle;
> +	}
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +
> +	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_cpup(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)
> +		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;
> +

Everything below this line....

> +		/* make a copy */
> +		propval = kmalloc(rprop->length, GFP_KERNEL);
> +		if (!propval) {
> +			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) {
> +				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) {
> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
> +					__func__, (char *)rprop->value);
> +				err = -EINVAL;
> +				goto err_fail;
> +			}
> +
> +			*s++ = '\0';
> +			err = kstrtoint(s, 10, &offset);
> +			if (err != 0) {
> +				pr_err("%s: Could get offset '%s'\n",
> +					__func__, (char *)rprop->value);
> +				goto err_fail;
> +			}
> +
> +			/* look into the resolve node for the full path */
> +			refnode = __of_find_node_by_full_name(node, nodestr);
> +			if (!refnode) {
> +				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) {
> +				pr_err("%s: Could not find property '%s'\n",
> +					__func__, (char *)rprop->value);
> +				err = -ENOENT;
> +				goto err_fail;
> +			}
> +
> +			phandle = be32_to_cpup(sprop->value + offset);
> +			*(__be32 *)(sprop->value + offset) =
> +				cpu_to_be32(phandle + phandle_delta);
> +		}
> +
> +		kfree(propval);
> +		propval = NULL;

... to this line is almost identical to the block in of_resolve(). Can
it be moved into a helper function?

> +	}
> +
> +	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 || !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 &&
> +				of_node_cmp(child->name, "__symbols__") == 0)
> +			resolve_sym = child;

Can /aliases be used instead of __symbols__? __symbols__ requires the
.dtb to be compiled in a particular way and it means an overlay will
never work with an existing dtb.

We also need to review how overlays match up with nodes in the base
tree. I can see a board with multiple identical expansion connectors,
but each connector wires up to different pins. If we made the symbols
resolution local to the expansion connector, then the same overlay could
be used for different ports.

> +
> +		if (!resolve_fix &&
> +				of_node_cmp(child->name, "__fixups__") == 0)
> +			resolve_fix = child;
> +
> +		/* both found, don't bother anymore */
> +		if (resolve_sym && resolve_fix)
> +			break;
> +	}
> +
> +	/* we do allow for the case where no fixups are needed */
> +	if (!resolve_fix) {
> +		err = 0;	/* no error */
> +		goto out;
> +	}
> +
> +	/* we need to fixup, but no root symbols... */
> +	if (!root_sym) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	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 out;
> +		}
> +
> +		refnode = of_find_node_by_path(refpath);
> +		if (!refnode) {
> +			pr_err("%s: Could not find node by path '%s'\n",
> +					__func__, refpath);
> +			err = -ENOENT;
> +			goto out;
> +		}
> +
> +		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) {
> +			pr_err("%s: Could not copy value of '%s'\n",
> +					__func__, rprop->name);
> +			err = -ENOMEM;
> +			goto out;
> +		}
> +
> +		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) {
> +				pr_err("%s: Illegal symbol entry '%s' (1)\n",
> +					__func__, (char *)rprop->value);
> +				err = -EINVAL;
> +				goto err_fail_free;
> +			}
> +			*s++ = '\0';
> +
> +			propstr = s;
> +			s = strchr(s, ':');
> +			if (!s) {
> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
> +					__func__, (char *)rprop->value);
> +				err = -EINVAL;
> +				goto err_fail_free;
> +			}
> +
> +			*s++ = '\0';
> +			err = kstrtoint(s, 10, &offset);
> +			if (err != 0) {
> +				pr_err("%s: Could get offset '%s'\n",
> +					__func__, (char *)rprop->value);
> +				goto err_fail_free;
> +			}
> +
> +			/* look into the resolve node for the full path */
> +			refnode = __of_find_node_by_full_name(resolve,
> +					nodestr);
> +			if (!refnode) {
> +				pr_err("%s: Could not find refnode '%s'\n",
> +					__func__, (char *)rprop->value);
> +				err = -ENOENT;
> +				goto err_fail_free;
> +			}
> +
> +			/* now find the property */
> +			for_each_property_of_node(refnode, sprop) {
> +				if (of_prop_cmp(sprop->name, propstr) == 0)
> +					break;
> +			}
> +
> +			if (!sprop) {
> +				pr_err("%s: Could not find property '%s'\n",
> +					__func__, (char *)rprop->value);
> +				err = -ENOENT;
> +				goto err_fail_free;
> +			}
> +
> +			*(__be32 *)(sprop->value + offset) =
> +				cpu_to_be32(phandle);
> +		}
> +
> +		kfree(propval);
> +		propval = NULL;
> +	}
> +
> +err_fail_free:
> +	kfree(propval);
> +
> +out:
> +	/* NULL is handled by of_node_put as NOP */
> +	of_node_put(root_sym);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(of_resolve);
> diff --git a/include/linux/of.h b/include/linux/of.h
> index fa81b42..e9be31c 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -903,4 +903,20 @@ static inline int of_transaction_update_property(struct of_transaction *oft,
>  }
>  #endif
>  
> +/* illegal phandle value (set when unresolved) */
> +#define OF_PHANDLE_ILLEGAL	0xdeadbeef

fdt.c should be made to use this macro also.

I've actually got a patches to makes all these changes. I've been trying
to get the selftest code to use the resolver. The current code passes
all the test cases, but while the testcase data is loaded there are
duplicate phandles in the tree. :-(

g.

Re: [PATCH v7 1/9] OF: Introduce Device Tree resolve support.

[Grant Likely]

On Fri,  4 Jul 2014 19:59:20 +0300
, Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
 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.
> 
> Export of of_resolve and bug fix of double free by
> 	Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

Hi Pantelis,

I suspect you've got a slightly updated version of this patch not posted
yet, but I do have a few comments. Overall they're pretty minor.

> ---
>  .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>  drivers/of/Kconfig                                 |   6 +
>  drivers/of/Makefile                                |   1 +
>  drivers/of/resolver.c                              | 403 +++++++++++++++++++++
>  include/linux/of.h                                 |  16 +
>  5 files changed, 451 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 2dcb054..0236a4e 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -78,4 +78,10 @@ config OF_RESERVED_MEM
>  	help
>  	  Helpers to allow for reservation of memory regions
>  
> +config OF_RESOLVE
> +	bool
> +	depends on OF && !SPARC

What part of this code breaks on SPARC?

> +	select OF_DYNAMIC
> +	select OF_DEVICE

This code doesn't appear to depend on either OF_DYNAMIC or OF_DEVICE. You
should be able to drop the above 2 selects (at least it worked when I
tried it here)

> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index 9a68cc9..35a148a 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -12,6 +12,7 @@ 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_RESERVED_MEM) += of_reserved_mem.o
> +obj-$(CONFIG_OF_RESOLVE)  += resolver.o
>  
>  CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
>  CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> new file mode 100644
> index 0000000..18da5ca
> --- /dev/null
> +++ b/drivers/of/resolver.c
> @@ -0,0 +1,403 @@
> +/*
> + * Functions for dealing with DT resolution
> + *
> + * 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>
> +
> +/**
> + * Find a node with the give full name by recursively following any of
> + * the child node links.
> + */
> +static 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;
> +}
> +
> +/*
> + * Find live tree's maximum phandle value.
> + */
> +static phandle of_get_tree_max_phandle(void)
> +{
> +	struct device_node *node;
> +	phandle phandle;
> +	unsigned long flags;
> +
> +	/* now search recursively */
> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +	phandle = 0;
> +	for_each_of_allnodes(node) {
> +		if (node->phandle != OF_PHANDLE_ILLEGAL &&
> +				node->phandle > phandle)
> +			phandle = node->phandle;
> +	}
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +
> +	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_cpup(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)
> +		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;
> +

Everything below this line....

> +		/* make a copy */
> +		propval = kmalloc(rprop->length, GFP_KERNEL);
> +		if (!propval) {
> +			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) {
> +				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) {
> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
> +					__func__, (char *)rprop->value);
> +				err = -EINVAL;
> +				goto err_fail;
> +			}
> +
> +			*s++ = '\0';
> +			err = kstrtoint(s, 10, &offset);
> +			if (err != 0) {
> +				pr_err("%s: Could get offset '%s'\n",
> +					__func__, (char *)rprop->value);
> +				goto err_fail;
> +			}
> +
> +			/* look into the resolve node for the full path */
> +			refnode = __of_find_node_by_full_name(node, nodestr);
> +			if (!refnode) {
> +				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) {
> +				pr_err("%s: Could not find property '%s'\n",
> +					__func__, (char *)rprop->value);
> +				err = -ENOENT;
> +				goto err_fail;
> +			}
> +
> +			phandle = be32_to_cpup(sprop->value + offset);
> +			*(__be32 *)(sprop->value + offset) =
> +				cpu_to_be32(phandle + phandle_delta);
> +		}
> +
> +		kfree(propval);
> +		propval = NULL;

... to this line is almost identical to the block in of_resolve(). Can
it be moved into a helper function?

> +	}
> +
> +	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 || !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 &&
> +				of_node_cmp(child->name, "__symbols__") == 0)
> +			resolve_sym = child;

Can /aliases be used instead of __symbols__? __symbols__ requires the
.dtb to be compiled in a particular way and it means an overlay will
never work with an existing dtb.

We also need to review how overlays match up with nodes in the base
tree. I can see a board with multiple identical expansion connectors,
but each connector wires up to different pins. If we made the symbols
resolution local to the expansion connector, then the same overlay could
be used for different ports.

> +
> +		if (!resolve_fix &&
> +				of_node_cmp(child->name, "__fixups__") == 0)
> +			resolve_fix = child;
> +
> +		/* both found, don't bother anymore */
> +		if (resolve_sym && resolve_fix)
> +			break;
> +	}
> +
> +	/* we do allow for the case where no fixups are needed */
> +	if (!resolve_fix) {
> +		err = 0;	/* no error */
> +		goto out;
> +	}
> +
> +	/* we need to fixup, but no root symbols... */
> +	if (!root_sym) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	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 out;
> +		}
> +
> +		refnode = of_find_node_by_path(refpath);
> +		if (!refnode) {
> +			pr_err("%s: Could not find node by path '%s'\n",
> +					__func__, refpath);
> +			err = -ENOENT;
> +			goto out;
> +		}
> +
> +		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) {
> +			pr_err("%s: Could not copy value of '%s'\n",
> +					__func__, rprop->name);
> +			err = -ENOMEM;
> +			goto out;
> +		}
> +
> +		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) {
> +				pr_err("%s: Illegal symbol entry '%s' (1)\n",
> +					__func__, (char *)rprop->value);
> +				err = -EINVAL;
> +				goto err_fail_free;
> +			}
> +			*s++ = '\0';
> +
> +			propstr = s;
> +			s = strchr(s, ':');
> +			if (!s) {
> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
> +					__func__, (char *)rprop->value);
> +				err = -EINVAL;
> +				goto err_fail_free;
> +			}
> +
> +			*s++ = '\0';
> +			err = kstrtoint(s, 10, &offset);
> +			if (err != 0) {
> +				pr_err("%s: Could get offset '%s'\n",
> +					__func__, (char *)rprop->value);
> +				goto err_fail_free;
> +			}
> +
> +			/* look into the resolve node for the full path */
> +			refnode = __of_find_node_by_full_name(resolve,
> +					nodestr);
> +			if (!refnode) {
> +				pr_err("%s: Could not find refnode '%s'\n",
> +					__func__, (char *)rprop->value);
> +				err = -ENOENT;
> +				goto err_fail_free;
> +			}
> +
> +			/* now find the property */
> +			for_each_property_of_node(refnode, sprop) {
> +				if (of_prop_cmp(sprop->name, propstr) == 0)
> +					break;
> +			}
> +
> +			if (!sprop) {
> +				pr_err("%s: Could not find property '%s'\n",
> +					__func__, (char *)rprop->value);
> +				err = -ENOENT;
> +				goto err_fail_free;
> +			}
> +
> +			*(__be32 *)(sprop->value + offset) =
> +				cpu_to_be32(phandle);
> +		}
> +
> +		kfree(propval);
> +		propval = NULL;
> +	}
> +
> +err_fail_free:
> +	kfree(propval);
> +
> +out:
> +	/* NULL is handled by of_node_put as NOP */
> +	of_node_put(root_sym);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(of_resolve);
> diff --git a/include/linux/of.h b/include/linux/of.h
> index fa81b42..e9be31c 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -903,4 +903,20 @@ static inline int of_transaction_update_property(struct of_transaction *oft,
>  }
>  #endif
>  
> +/* illegal phandle value (set when unresolved) */
> +#define OF_PHANDLE_ILLEGAL	0xdeadbeef

fdt.c should be made to use this macro also.

I've actually got a patches to makes all these changes. I've been trying
to get the selftest code to use the resolver. The current code passes
all the test cases, but while the testcase data is loaded there are
duplicate phandles in the tree. :-(

g.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/9] OF: Introduce Device Tree resolve support.

[Grant Likely]

On Fri,  4 Jul 2014 19:59:20 +0300
, Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
 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.
> 
> Export of of_resolve and bug fix of double free by
> 	Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

Hi Pantelis,

I suspect you've got a slightly updated version of this patch not posted
yet, but I do have a few comments. Overall they're pretty minor.

> ---
>  .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>  drivers/of/Kconfig                                 |   6 +
>  drivers/of/Makefile                                |   1 +
>  drivers/of/resolver.c                              | 403 +++++++++++++++++++++
>  include/linux/of.h                                 |  16 +
>  5 files changed, 451 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 2dcb054..0236a4e 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -78,4 +78,10 @@ config OF_RESERVED_MEM
>  	help
>  	  Helpers to allow for reservation of memory regions
>  
> +config OF_RESOLVE
> +	bool
> +	depends on OF && !SPARC

What part of this code breaks on SPARC?

> +	select OF_DYNAMIC
> +	select OF_DEVICE

This code doesn't appear to depend on either OF_DYNAMIC or OF_DEVICE. You
should be able to drop the above 2 selects (at least it worked when I
tried it here)

> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index 9a68cc9..35a148a 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -12,6 +12,7 @@ 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_RESERVED_MEM) += of_reserved_mem.o
> +obj-$(CONFIG_OF_RESOLVE)  += resolver.o
>  
>  CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
>  CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> new file mode 100644
> index 0000000..18da5ca
> --- /dev/null
> +++ b/drivers/of/resolver.c
> @@ -0,0 +1,403 @@
> +/*
> + * Functions for dealing with DT resolution
> + *
> + * 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>
> +
> +/**
> + * Find a node with the give full name by recursively following any of
> + * the child node links.
> + */
> +static 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;
> +}
> +
> +/*
> + * Find live tree's maximum phandle value.
> + */
> +static phandle of_get_tree_max_phandle(void)
> +{
> +	struct device_node *node;
> +	phandle phandle;
> +	unsigned long flags;
> +
> +	/* now search recursively */
> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +	phandle = 0;
> +	for_each_of_allnodes(node) {
> +		if (node->phandle != OF_PHANDLE_ILLEGAL &&
> +				node->phandle > phandle)
> +			phandle = node->phandle;
> +	}
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +
> +	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_cpup(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)
> +		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;
> +

Everything below this line....

> +		/* make a copy */
> +		propval = kmalloc(rprop->length, GFP_KERNEL);
> +		if (!propval) {
> +			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) {
> +				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) {
> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
> +					__func__, (char *)rprop->value);
> +				err = -EINVAL;
> +				goto err_fail;
> +			}
> +
> +			*s++ = '\0';
> +			err = kstrtoint(s, 10, &offset);
> +			if (err != 0) {
> +				pr_err("%s: Could get offset '%s'\n",
> +					__func__, (char *)rprop->value);
> +				goto err_fail;
> +			}
> +
> +			/* look into the resolve node for the full path */
> +			refnode = __of_find_node_by_full_name(node, nodestr);
> +			if (!refnode) {
> +				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) {
> +				pr_err("%s: Could not find property '%s'\n",
> +					__func__, (char *)rprop->value);
> +				err = -ENOENT;
> +				goto err_fail;
> +			}
> +
> +			phandle = be32_to_cpup(sprop->value + offset);
> +			*(__be32 *)(sprop->value + offset) =
> +				cpu_to_be32(phandle + phandle_delta);
> +		}
> +
> +		kfree(propval);
> +		propval = NULL;

... to this line is almost identical to the block in of_resolve(). Can
it be moved into a helper function?

> +	}
> +
> +	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 || !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 &&
> +				of_node_cmp(child->name, "__symbols__") == 0)
> +			resolve_sym = child;

Can /aliases be used instead of __symbols__? __symbols__ requires the
.dtb to be compiled in a particular way and it means an overlay will
never work with an existing dtb.

We also need to review how overlays match up with nodes in the base
tree. I can see a board with multiple identical expansion connectors,
but each connector wires up to different pins. If we made the symbols
resolution local to the expansion connector, then the same overlay could
be used for different ports.

> +
> +		if (!resolve_fix &&
> +				of_node_cmp(child->name, "__fixups__") == 0)
> +			resolve_fix = child;
> +
> +		/* both found, don't bother anymore */
> +		if (resolve_sym && resolve_fix)
> +			break;
> +	}
> +
> +	/* we do allow for the case where no fixups are needed */
> +	if (!resolve_fix) {
> +		err = 0;	/* no error */
> +		goto out;
> +	}
> +
> +	/* we need to fixup, but no root symbols... */
> +	if (!root_sym) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	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 out;
> +		}
> +
> +		refnode = of_find_node_by_path(refpath);
> +		if (!refnode) {
> +			pr_err("%s: Could not find node by path '%s'\n",
> +					__func__, refpath);
> +			err = -ENOENT;
> +			goto out;
> +		}
> +
> +		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) {
> +			pr_err("%s: Could not copy value of '%s'\n",
> +					__func__, rprop->name);
> +			err = -ENOMEM;
> +			goto out;
> +		}
> +
> +		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) {
> +				pr_err("%s: Illegal symbol entry '%s' (1)\n",
> +					__func__, (char *)rprop->value);
> +				err = -EINVAL;
> +				goto err_fail_free;
> +			}
> +			*s++ = '\0';
> +
> +			propstr = s;
> +			s = strchr(s, ':');
> +			if (!s) {
> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
> +					__func__, (char *)rprop->value);
> +				err = -EINVAL;
> +				goto err_fail_free;
> +			}
> +
> +			*s++ = '\0';
> +			err = kstrtoint(s, 10, &offset);
> +			if (err != 0) {
> +				pr_err("%s: Could get offset '%s'\n",
> +					__func__, (char *)rprop->value);
> +				goto err_fail_free;
> +			}
> +
> +			/* look into the resolve node for the full path */
> +			refnode = __of_find_node_by_full_name(resolve,
> +					nodestr);
> +			if (!refnode) {
> +				pr_err("%s: Could not find refnode '%s'\n",
> +					__func__, (char *)rprop->value);
> +				err = -ENOENT;
> +				goto err_fail_free;
> +			}
> +
> +			/* now find the property */
> +			for_each_property_of_node(refnode, sprop) {
> +				if (of_prop_cmp(sprop->name, propstr) == 0)
> +					break;
> +			}
> +
> +			if (!sprop) {
> +				pr_err("%s: Could not find property '%s'\n",
> +					__func__, (char *)rprop->value);
> +				err = -ENOENT;
> +				goto err_fail_free;
> +			}
> +
> +			*(__be32 *)(sprop->value + offset) =
> +				cpu_to_be32(phandle);
> +		}
> +
> +		kfree(propval);
> +		propval = NULL;
> +	}
> +
> +err_fail_free:
> +	kfree(propval);
> +
> +out:
> +	/* NULL is handled by of_node_put as NOP */
> +	of_node_put(root_sym);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(of_resolve);
> diff --git a/include/linux/of.h b/include/linux/of.h
> index fa81b42..e9be31c 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -903,4 +903,20 @@ static inline int of_transaction_update_property(struct of_transaction *oft,
>  }
>  #endif
>  
> +/* illegal phandle value (set when unresolved) */
> +#define OF_PHANDLE_ILLEGAL	0xdeadbeef

fdt.c should be made to use this macro also.

I've actually got a patches to makes all these changes. I've been trying
to get the selftest code to use the resolver. The current code passes
all the test cases, but while the testcase data is loaded there are
duplicate phandles in the tree. :-(

g.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/9] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

Hi Grant,

On Oct 1, 2014, at 3:46 PM, Grant Likely <grant.likely@secretlab.ca> wrote:

> On Fri,  4 Jul 2014 19:59:20 +0300
> , Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> 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.
>> 
>> Export of of_resolve and bug fix of double free by
>> 	Guenter Roeck <groeck@juniper.net>
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> 
> Hi Pantelis,
> 
> I suspect you've got a slightly updated version of this patch not posted
> yet, but I do have a few comments. Overall they're pretty minor.
> 

Last time I checked (circa rc2), the last patch set applied. I’ll have to rebase and 
take a look again but I don’t expect major changes.

>> ---
>> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>> drivers/of/Kconfig                                 |   6 +
>> drivers/of/Makefile                                |   1 +
>> drivers/of/resolver.c                              | 403 +++++++++++++++++++++
>> include/linux/of.h                                 |  16 +
>> 5 files changed, 451 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 2dcb054..0236a4e 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -78,4 +78,10 @@ config OF_RESERVED_MEM
>> 	help
>> 	  Helpers to allow for reservation of memory regions
>> 
>> +config OF_RESOLVE
>> +	bool
>> +	depends on OF && !SPARC
> 
> What part of this code breaks on SPARC?
> 

There are some extra properties on the SPARC device nodes, which I don’t know exactly how they work.
I don’t have access to a SPARC box, lest I break something this conditional.

>> +	select OF_DYNAMIC
>> +	select OF_DEVICE
> 
> This code doesn't appear to depend on either OF_DYNAMIC or OF_DEVICE. You
> should be able to drop the above 2 selects (at least it worked when I
> tried it here)
> 

Yes, good catch. It wouldn’t work of-course since all the calls to insert nodes would
fail.

>> +
>> endmenu # OF
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index 9a68cc9..35a148a 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -12,6 +12,7 @@ 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_RESERVED_MEM) += of_reserved_mem.o
>> +obj-$(CONFIG_OF_RESOLVE)  += resolver.o
>> 
>> CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
>> CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> new file mode 100644
>> index 0000000..18da5ca
>> --- /dev/null
>> +++ b/drivers/of/resolver.c
>> @@ -0,0 +1,403 @@
>> +/*
>> + * 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/string.h>
>> +#include <linux/ctype.h>
>> +#include <linux/errno.h>
>> +#include <linux/string.h>
>> +#include <linux/slab.h>
>> +
>> +/**
>> + * Find a node with the give full name by recursively following any of
>> + * the child node links.
>> + */
>> +static 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;
>> +}
>> +
>> +/*
>> + * Find live tree's maximum phandle value.
>> + */
>> +static phandle of_get_tree_max_phandle(void)
>> +{
>> +	struct device_node *node;
>> +	phandle phandle;
>> +	unsigned long flags;
>> +
>> +	/* now search recursively */
>> +	raw_spin_lock_irqsave(&devtree_lock, flags);
>> +	phandle = 0;
>> +	for_each_of_allnodes(node) {
>> +		if (node->phandle != OF_PHANDLE_ILLEGAL &&
>> +				node->phandle > phandle)
>> +			phandle = node->phandle;
>> +	}
>> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> +
>> +	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_cpup(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)
>> +		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;
>> +
> 
> Everything below this line....
> 
>> +		/* make a copy */
>> +		propval = kmalloc(rprop->length, GFP_KERNEL);
>> +		if (!propval) {
>> +			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) {
>> +				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) {
>> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail;
>> +			}
>> +
>> +			*s++ = '\0';
>> +			err = kstrtoint(s, 10, &offset);
>> +			if (err != 0) {
>> +				pr_err("%s: Could get offset '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				goto err_fail;
>> +			}
>> +
>> +			/* look into the resolve node for the full path */
>> +			refnode = __of_find_node_by_full_name(node, nodestr);
>> +			if (!refnode) {
>> +				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) {
>> +				pr_err("%s: Could not find property '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail;
>> +			}
>> +
>> +			phandle = be32_to_cpup(sprop->value + offset);
>> +			*(__be32 *)(sprop->value + offset) =
>> +				cpu_to_be32(phandle + phandle_delta);
>> +		}
>> +
>> +		kfree(propval);
>> +		propval = NULL;
> 
> ... to this line is almost identical to the block in of_resolve(). Can
> it be moved into a helper function?
> 

It appears so at first glance.


>> +	}
>> +
>> +	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 || !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 &&
>> +				of_node_cmp(child->name, "__symbols__") == 0)
>> +			resolve_sym = child;
> 
> Can /aliases be used instead of __symbols__? __symbols__ requires the
> .dtb to be compiled in a particular way and it means an overlay will
> never work with an existing dtb.
> 

Hmm. Aliases in theory can be used as symbols sure enough.
It is going to be very error prone (since you will need to explicitly define every symbol,
instead of just relying on the compiler to generate an entry for each label).

As a change it’s simple enough...

> We also need to review how overlays match up with nodes in the base
> tree. I can see a board with multiple identical expansion connectors,
> but each connector wires up to different pins. If we made the symbols
> resolution local to the expansion connector, then the same overlay could
> be used for different ports.
> 

I have something in mind how to fix this, and some even more wonky cases.
Think overlays applied on devices on busses that can be probed like PCI/USB etc.

I think we can work the details at ELCE. Perhaps ask the organizers to setup the DT
session we discussed.

>> +
>> +		if (!resolve_fix &&
>> +				of_node_cmp(child->name, "__fixups__") == 0)
>> +			resolve_fix = child;
>> +
>> +		/* both found, don't bother anymore */
>> +		if (resolve_sym && resolve_fix)
>> +			break;
>> +	}
>> +
>> +	/* we do allow for the case where no fixups are needed */
>> +	if (!resolve_fix) {
>> +		err = 0;	/* no error */
>> +		goto out;
>> +	}
>> +
>> +	/* we need to fixup, but no root symbols... */
>> +	if (!root_sym) {
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	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 out;
>> +		}
>> +
>> +		refnode = of_find_node_by_path(refpath);
>> +		if (!refnode) {
>> +			pr_err("%s: Could not find node by path '%s'\n",
>> +					__func__, refpath);
>> +			err = -ENOENT;
>> +			goto out;
>> +		}
>> +
>> +		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) {
>> +			pr_err("%s: Could not copy value of '%s'\n",
>> +					__func__, rprop->name);
>> +			err = -ENOMEM;
>> +			goto out;
>> +		}
>> +
>> +		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) {
>> +				pr_err("%s: Illegal symbol entry '%s' (1)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail_free;
>> +			}
>> +			*s++ = '\0';
>> +
>> +			propstr = s;
>> +			s = strchr(s, ':');
>> +			if (!s) {
>> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail_free;
>> +			}
>> +
>> +			*s++ = '\0';
>> +			err = kstrtoint(s, 10, &offset);
>> +			if (err != 0) {
>> +				pr_err("%s: Could get offset '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				goto err_fail_free;
>> +			}
>> +
>> +			/* look into the resolve node for the full path */
>> +			refnode = __of_find_node_by_full_name(resolve,
>> +					nodestr);
>> +			if (!refnode) {
>> +				pr_err("%s: Could not find refnode '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail_free;
>> +			}
>> +
>> +			/* now find the property */
>> +			for_each_property_of_node(refnode, sprop) {
>> +				if (of_prop_cmp(sprop->name, propstr) == 0)
>> +					break;
>> +			}
>> +
>> +			if (!sprop) {
>> +				pr_err("%s: Could not find property '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail_free;
>> +			}
>> +
>> +			*(__be32 *)(sprop->value + offset) =
>> +				cpu_to_be32(phandle);
>> +		}
>> +
>> +		kfree(propval);
>> +		propval = NULL;
>> +	}
>> +
>> +err_fail_free:
>> +	kfree(propval);
>> +
>> +out:
>> +	/* NULL is handled by of_node_put as NOP */
>> +	of_node_put(root_sym);
>> +
>> +	return err;
>> +}
>> +EXPORT_SYMBOL_GPL(of_resolve);
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index fa81b42..e9be31c 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -903,4 +903,20 @@ static inline int of_transaction_update_property(struct of_transaction *oft,
>> }
>> #endif
>> 
>> +/* illegal phandle value (set when unresolved) */
>> +#define OF_PHANDLE_ILLEGAL	0xdeadbeef
> 
> fdt.c should be made to use this macro also.
> 

OK.

> I've actually got a patches to makes all these changes. I've been trying
> to get the selftest code to use the resolver. The current code passes
> all the test cases, but while the testcase data is loaded there are
> duplicate phandles in the tree. :-(
> 

Told you them things are tricky! :)

Remember that patch we talked about a few weeks earlier? About the implicit properties?
"of: dynamic: Handle special properties changes”. That should do the trick.

Speaking of which I think it’s also time to look over the next set of changes.
Like having a tree DT structure, phandle lists and perhaps copy based devm_* driver APIs.


> g.

Regards

— Pantelis

Re: [PATCH v7 1/9] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

Hi Grant,

On Oct 1, 2014, at 3:46 PM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> On Fri,  4 Jul 2014 19:59:20 +0300
> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> 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.
>> 
>> Export of of_resolve and bug fix of double free by
>> 	Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> 
> Hi Pantelis,
> 
> I suspect you've got a slightly updated version of this patch not posted
> yet, but I do have a few comments. Overall they're pretty minor.
> 

Last time I checked (circa rc2), the last patch set applied. I’ll have to rebase and 
take a look again but I don’t expect major changes.

>> ---
>> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>> drivers/of/Kconfig                                 |   6 +
>> drivers/of/Makefile                                |   1 +
>> drivers/of/resolver.c                              | 403 +++++++++++++++++++++
>> include/linux/of.h                                 |  16 +
>> 5 files changed, 451 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 2dcb054..0236a4e 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -78,4 +78,10 @@ config OF_RESERVED_MEM
>> 	help
>> 	  Helpers to allow for reservation of memory regions
>> 
>> +config OF_RESOLVE
>> +	bool
>> +	depends on OF && !SPARC
> 
> What part of this code breaks on SPARC?
> 

There are some extra properties on the SPARC device nodes, which I don’t know exactly how they work.
I don’t have access to a SPARC box, lest I break something this conditional.

>> +	select OF_DYNAMIC
>> +	select OF_DEVICE
> 
> This code doesn't appear to depend on either OF_DYNAMIC or OF_DEVICE. You
> should be able to drop the above 2 selects (at least it worked when I
> tried it here)
> 

Yes, good catch. It wouldn’t work of-course since all the calls to insert nodes would
fail.

>> +
>> endmenu # OF
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index 9a68cc9..35a148a 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -12,6 +12,7 @@ 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_RESERVED_MEM) += of_reserved_mem.o
>> +obj-$(CONFIG_OF_RESOLVE)  += resolver.o
>> 
>> CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
>> CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> new file mode 100644
>> index 0000000..18da5ca
>> --- /dev/null
>> +++ b/drivers/of/resolver.c
>> @@ -0,0 +1,403 @@
>> +/*
>> + * 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/string.h>
>> +#include <linux/ctype.h>
>> +#include <linux/errno.h>
>> +#include <linux/string.h>
>> +#include <linux/slab.h>
>> +
>> +/**
>> + * Find a node with the give full name by recursively following any of
>> + * the child node links.
>> + */
>> +static 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;
>> +}
>> +
>> +/*
>> + * Find live tree's maximum phandle value.
>> + */
>> +static phandle of_get_tree_max_phandle(void)
>> +{
>> +	struct device_node *node;
>> +	phandle phandle;
>> +	unsigned long flags;
>> +
>> +	/* now search recursively */
>> +	raw_spin_lock_irqsave(&devtree_lock, flags);
>> +	phandle = 0;
>> +	for_each_of_allnodes(node) {
>> +		if (node->phandle != OF_PHANDLE_ILLEGAL &&
>> +				node->phandle > phandle)
>> +			phandle = node->phandle;
>> +	}
>> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> +
>> +	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_cpup(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)
>> +		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;
>> +
> 
> Everything below this line....
> 
>> +		/* make a copy */
>> +		propval = kmalloc(rprop->length, GFP_KERNEL);
>> +		if (!propval) {
>> +			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) {
>> +				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) {
>> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail;
>> +			}
>> +
>> +			*s++ = '\0';
>> +			err = kstrtoint(s, 10, &offset);
>> +			if (err != 0) {
>> +				pr_err("%s: Could get offset '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				goto err_fail;
>> +			}
>> +
>> +			/* look into the resolve node for the full path */
>> +			refnode = __of_find_node_by_full_name(node, nodestr);
>> +			if (!refnode) {
>> +				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) {
>> +				pr_err("%s: Could not find property '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail;
>> +			}
>> +
>> +			phandle = be32_to_cpup(sprop->value + offset);
>> +			*(__be32 *)(sprop->value + offset) =
>> +				cpu_to_be32(phandle + phandle_delta);
>> +		}
>> +
>> +		kfree(propval);
>> +		propval = NULL;
> 
> ... to this line is almost identical to the block in of_resolve(). Can
> it be moved into a helper function?
> 

It appears so at first glance.


>> +	}
>> +
>> +	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 || !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 &&
>> +				of_node_cmp(child->name, "__symbols__") == 0)
>> +			resolve_sym = child;
> 
> Can /aliases be used instead of __symbols__? __symbols__ requires the
> .dtb to be compiled in a particular way and it means an overlay will
> never work with an existing dtb.
> 

Hmm. Aliases in theory can be used as symbols sure enough.
It is going to be very error prone (since you will need to explicitly define every symbol,
instead of just relying on the compiler to generate an entry for each label).

As a change it’s simple enough...

> We also need to review how overlays match up with nodes in the base
> tree. I can see a board with multiple identical expansion connectors,
> but each connector wires up to different pins. If we made the symbols
> resolution local to the expansion connector, then the same overlay could
> be used for different ports.
> 

I have something in mind how to fix this, and some even more wonky cases.
Think overlays applied on devices on busses that can be probed like PCI/USB etc.

I think we can work the details at ELCE. Perhaps ask the organizers to setup the DT
session we discussed.

>> +
>> +		if (!resolve_fix &&
>> +				of_node_cmp(child->name, "__fixups__") == 0)
>> +			resolve_fix = child;
>> +
>> +		/* both found, don't bother anymore */
>> +		if (resolve_sym && resolve_fix)
>> +			break;
>> +	}
>> +
>> +	/* we do allow for the case where no fixups are needed */
>> +	if (!resolve_fix) {
>> +		err = 0;	/* no error */
>> +		goto out;
>> +	}
>> +
>> +	/* we need to fixup, but no root symbols... */
>> +	if (!root_sym) {
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	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 out;
>> +		}
>> +
>> +		refnode = of_find_node_by_path(refpath);
>> +		if (!refnode) {
>> +			pr_err("%s: Could not find node by path '%s'\n",
>> +					__func__, refpath);
>> +			err = -ENOENT;
>> +			goto out;
>> +		}
>> +
>> +		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) {
>> +			pr_err("%s: Could not copy value of '%s'\n",
>> +					__func__, rprop->name);
>> +			err = -ENOMEM;
>> +			goto out;
>> +		}
>> +
>> +		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) {
>> +				pr_err("%s: Illegal symbol entry '%s' (1)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail_free;
>> +			}
>> +			*s++ = '\0';
>> +
>> +			propstr = s;
>> +			s = strchr(s, ':');
>> +			if (!s) {
>> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail_free;
>> +			}
>> +
>> +			*s++ = '\0';
>> +			err = kstrtoint(s, 10, &offset);
>> +			if (err != 0) {
>> +				pr_err("%s: Could get offset '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				goto err_fail_free;
>> +			}
>> +
>> +			/* look into the resolve node for the full path */
>> +			refnode = __of_find_node_by_full_name(resolve,
>> +					nodestr);
>> +			if (!refnode) {
>> +				pr_err("%s: Could not find refnode '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail_free;
>> +			}
>> +
>> +			/* now find the property */
>> +			for_each_property_of_node(refnode, sprop) {
>> +				if (of_prop_cmp(sprop->name, propstr) == 0)
>> +					break;
>> +			}
>> +
>> +			if (!sprop) {
>> +				pr_err("%s: Could not find property '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail_free;
>> +			}
>> +
>> +			*(__be32 *)(sprop->value + offset) =
>> +				cpu_to_be32(phandle);
>> +		}
>> +
>> +		kfree(propval);
>> +		propval = NULL;
>> +	}
>> +
>> +err_fail_free:
>> +	kfree(propval);
>> +
>> +out:
>> +	/* NULL is handled by of_node_put as NOP */
>> +	of_node_put(root_sym);
>> +
>> +	return err;
>> +}
>> +EXPORT_SYMBOL_GPL(of_resolve);
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index fa81b42..e9be31c 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -903,4 +903,20 @@ static inline int of_transaction_update_property(struct of_transaction *oft,
>> }
>> #endif
>> 
>> +/* illegal phandle value (set when unresolved) */
>> +#define OF_PHANDLE_ILLEGAL	0xdeadbeef
> 
> fdt.c should be made to use this macro also.
> 

OK.

> I've actually got a patches to makes all these changes. I've been trying
> to get the selftest code to use the resolver. The current code passes
> all the test cases, but while the testcase data is loaded there are
> duplicate phandles in the tree. :-(
> 

Told you them things are tricky! :)

Remember that patch we talked about a few weeks earlier? About the implicit properties?
"of: dynamic: Handle special properties changes”. That should do the trick.

Speaking of which I think it’s also time to look over the next set of changes.
Like having a tree DT structure, phandle lists and perhaps copy based devm_* driver APIs.


> g.

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/9] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

Hi Grant,

On Oct 1, 2014, at 3:46 PM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> On Fri,  4 Jul 2014 19:59:20 +0300
> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> 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.
>> 
>> Export of of_resolve and bug fix of double free by
>> 	Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> 
> Hi Pantelis,
> 
> I suspect you've got a slightly updated version of this patch not posted
> yet, but I do have a few comments. Overall they're pretty minor.
> 

Last time I checked (circa rc2), the last patch set applied. I’ll have to rebase and 
take a look again but I don’t expect major changes.

>> ---
>> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>> drivers/of/Kconfig                                 |   6 +
>> drivers/of/Makefile                                |   1 +
>> drivers/of/resolver.c                              | 403 +++++++++++++++++++++
>> include/linux/of.h                                 |  16 +
>> 5 files changed, 451 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 2dcb054..0236a4e 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -78,4 +78,10 @@ config OF_RESERVED_MEM
>> 	help
>> 	  Helpers to allow for reservation of memory regions
>> 
>> +config OF_RESOLVE
>> +	bool
>> +	depends on OF && !SPARC
> 
> What part of this code breaks on SPARC?
> 

There are some extra properties on the SPARC device nodes, which I don’t know exactly how they work.
I don’t have access to a SPARC box, lest I break something this conditional.

>> +	select OF_DYNAMIC
>> +	select OF_DEVICE
> 
> This code doesn't appear to depend on either OF_DYNAMIC or OF_DEVICE. You
> should be able to drop the above 2 selects (at least it worked when I
> tried it here)
> 

Yes, good catch. It wouldn’t work of-course since all the calls to insert nodes would
fail.

>> +
>> endmenu # OF
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index 9a68cc9..35a148a 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -12,6 +12,7 @@ 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_RESERVED_MEM) += of_reserved_mem.o
>> +obj-$(CONFIG_OF_RESOLVE)  += resolver.o
>> 
>> CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
>> CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> new file mode 100644
>> index 0000000..18da5ca
>> --- /dev/null
>> +++ b/drivers/of/resolver.c
>> @@ -0,0 +1,403 @@
>> +/*
>> + * 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/string.h>
>> +#include <linux/ctype.h>
>> +#include <linux/errno.h>
>> +#include <linux/string.h>
>> +#include <linux/slab.h>
>> +
>> +/**
>> + * Find a node with the give full name by recursively following any of
>> + * the child node links.
>> + */
>> +static 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;
>> +}
>> +
>> +/*
>> + * Find live tree's maximum phandle value.
>> + */
>> +static phandle of_get_tree_max_phandle(void)
>> +{
>> +	struct device_node *node;
>> +	phandle phandle;
>> +	unsigned long flags;
>> +
>> +	/* now search recursively */
>> +	raw_spin_lock_irqsave(&devtree_lock, flags);
>> +	phandle = 0;
>> +	for_each_of_allnodes(node) {
>> +		if (node->phandle != OF_PHANDLE_ILLEGAL &&
>> +				node->phandle > phandle)
>> +			phandle = node->phandle;
>> +	}
>> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> +
>> +	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_cpup(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)
>> +		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;
>> +
> 
> Everything below this line....
> 
>> +		/* make a copy */
>> +		propval = kmalloc(rprop->length, GFP_KERNEL);
>> +		if (!propval) {
>> +			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) {
>> +				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) {
>> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail;
>> +			}
>> +
>> +			*s++ = '\0';
>> +			err = kstrtoint(s, 10, &offset);
>> +			if (err != 0) {
>> +				pr_err("%s: Could get offset '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				goto err_fail;
>> +			}
>> +
>> +			/* look into the resolve node for the full path */
>> +			refnode = __of_find_node_by_full_name(node, nodestr);
>> +			if (!refnode) {
>> +				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) {
>> +				pr_err("%s: Could not find property '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail;
>> +			}
>> +
>> +			phandle = be32_to_cpup(sprop->value + offset);
>> +			*(__be32 *)(sprop->value + offset) =
>> +				cpu_to_be32(phandle + phandle_delta);
>> +		}
>> +
>> +		kfree(propval);
>> +		propval = NULL;
> 
> ... to this line is almost identical to the block in of_resolve(). Can
> it be moved into a helper function?
> 

It appears so at first glance.


>> +	}
>> +
>> +	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 || !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 &&
>> +				of_node_cmp(child->name, "__symbols__") == 0)
>> +			resolve_sym = child;
> 
> Can /aliases be used instead of __symbols__? __symbols__ requires the
> .dtb to be compiled in a particular way and it means an overlay will
> never work with an existing dtb.
> 

Hmm. Aliases in theory can be used as symbols sure enough.
It is going to be very error prone (since you will need to explicitly define every symbol,
instead of just relying on the compiler to generate an entry for each label).

As a change it’s simple enough...

> We also need to review how overlays match up with nodes in the base
> tree. I can see a board with multiple identical expansion connectors,
> but each connector wires up to different pins. If we made the symbols
> resolution local to the expansion connector, then the same overlay could
> be used for different ports.
> 

I have something in mind how to fix this, and some even more wonky cases.
Think overlays applied on devices on busses that can be probed like PCI/USB etc.

I think we can work the details at ELCE. Perhaps ask the organizers to setup the DT
session we discussed.

>> +
>> +		if (!resolve_fix &&
>> +				of_node_cmp(child->name, "__fixups__") == 0)
>> +			resolve_fix = child;
>> +
>> +		/* both found, don't bother anymore */
>> +		if (resolve_sym && resolve_fix)
>> +			break;
>> +	}
>> +
>> +	/* we do allow for the case where no fixups are needed */
>> +	if (!resolve_fix) {
>> +		err = 0;	/* no error */
>> +		goto out;
>> +	}
>> +
>> +	/* we need to fixup, but no root symbols... */
>> +	if (!root_sym) {
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	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 out;
>> +		}
>> +
>> +		refnode = of_find_node_by_path(refpath);
>> +		if (!refnode) {
>> +			pr_err("%s: Could not find node by path '%s'\n",
>> +					__func__, refpath);
>> +			err = -ENOENT;
>> +			goto out;
>> +		}
>> +
>> +		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) {
>> +			pr_err("%s: Could not copy value of '%s'\n",
>> +					__func__, rprop->name);
>> +			err = -ENOMEM;
>> +			goto out;
>> +		}
>> +
>> +		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) {
>> +				pr_err("%s: Illegal symbol entry '%s' (1)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail_free;
>> +			}
>> +			*s++ = '\0';
>> +
>> +			propstr = s;
>> +			s = strchr(s, ':');
>> +			if (!s) {
>> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail_free;
>> +			}
>> +
>> +			*s++ = '\0';
>> +			err = kstrtoint(s, 10, &offset);
>> +			if (err != 0) {
>> +				pr_err("%s: Could get offset '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				goto err_fail_free;
>> +			}
>> +
>> +			/* look into the resolve node for the full path */
>> +			refnode = __of_find_node_by_full_name(resolve,
>> +					nodestr);
>> +			if (!refnode) {
>> +				pr_err("%s: Could not find refnode '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail_free;
>> +			}
>> +
>> +			/* now find the property */
>> +			for_each_property_of_node(refnode, sprop) {
>> +				if (of_prop_cmp(sprop->name, propstr) == 0)
>> +					break;
>> +			}
>> +
>> +			if (!sprop) {
>> +				pr_err("%s: Could not find property '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail_free;
>> +			}
>> +
>> +			*(__be32 *)(sprop->value + offset) =
>> +				cpu_to_be32(phandle);
>> +		}
>> +
>> +		kfree(propval);
>> +		propval = NULL;
>> +	}
>> +
>> +err_fail_free:
>> +	kfree(propval);
>> +
>> +out:
>> +	/* NULL is handled by of_node_put as NOP */
>> +	of_node_put(root_sym);
>> +
>> +	return err;
>> +}
>> +EXPORT_SYMBOL_GPL(of_resolve);
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index fa81b42..e9be31c 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -903,4 +903,20 @@ static inline int of_transaction_update_property(struct of_transaction *oft,
>> }
>> #endif
>> 
>> +/* illegal phandle value (set when unresolved) */
>> +#define OF_PHANDLE_ILLEGAL	0xdeadbeef
> 
> fdt.c should be made to use this macro also.
> 

OK.

> I've actually got a patches to makes all these changes. I've been trying
> to get the selftest code to use the resolver. The current code passes
> all the test cases, but while the testcase data is loaded there are
> duplicate phandles in the tree. :-(
> 

Told you them things are tricky! :)

Remember that patch we talked about a few weeks earlier? About the implicit properties?
"of: dynamic: Handle special properties changes”. That should do the trick.

Speaking of which I think it’s also time to look over the next set of changes.
Like having a tree DT structure, phandle lists and perhaps copy based devm_* driver APIs.


> g.

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html