[PATCH v2 0/6] of: overlays: New target methods

[PATCH v2 0/6] of: overlays: New target methods

[Pantelis Antoniou]

This patchset implements two new target methods.

A target index method which allows selecting different
targets according to an argument using an extended API and
a target root method that fences the target only
to a specific given root.

Documentation and unit-tests are included.

Changes since v1:
* Addressed reviewer comments.
* Converted target_indirect to target_index

Pantelis Antoniou (6):
  of: overlay: Implement target index support
  of: unittest: Add indirect overlay target test
  doc: dt: Document the indirect overlay method.
  of: overlay: Introduce target root capability.
  of: unittest: Unit-tests for target root overlays.
  doc: dt: Document the target root overlay method

 Documentation/devicetree/overlay-notes.txt  |  22 ++-
 drivers/of/overlay.c                        | 158 +++++++++++++---
 drivers/of/unittest-data/testcases.dts      |  10 +
 drivers/of/unittest-data/tests-overlay.dtsi |  63 +++++++
 drivers/of/unittest.c                       | 273 ++++++++++++++++++++++++++++
 include/linux/of.h                          |  16 ++
 6 files changed, 517 insertions(+), 25 deletions(-)

-- 
1.7.12

[PATCH v2 1/6] of: overlay: Implement target index support

[Pantelis Antoniou]

Some applications require applying the same overlay to a different
target according to some external condition (for instance depending
on the slot a card has been inserted, the overlay target is different).

The target index functionality use requires using the new
of_overlay_create_target_index() API which uses an index argument.

The format changes as follow

	fragment@0 {
		target = <&foo_target>, <&bar_target>;
	};

Calling of_overlay_create_target_index() with a 0 index selects
the foo_target, while using an index of 1 selects the bar_target.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/overlay.c | 65 ++++++++++++++++++++++++++++++++++++----------------
 include/linux/of.h   |  8 +++++++
 2 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index fdfc487..2efd4b7 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -75,6 +75,7 @@ struct of_overlay {
 	const struct attribute_group **attr_groups;
 	struct of_changeset cset;
 	struct kobject kobj;
+	int target_index;
 };
 
 /* master enable switch; once set to 0 can't be re-enabled */
@@ -222,30 +223,29 @@ static int of_overlay_apply(struct of_overlay *ov)
 
 /*
  * Find the target node using a number of different strategies
- * in order of preference
+ * in order of preference. Respects the target index if available.
  *
  * "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)
+static struct device_node *find_target_node(struct of_overlay *ov,
+		struct device_node *info_node, int index)
 {
 	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);
+	ret = of_property_read_u32_index(info_node, "target", index, &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);
+	/* failed, try to locate by path */
+	ret = of_property_read_string_index(info_node, "target-path", index,
+			&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;
 }
 
@@ -270,7 +270,7 @@ static int of_fill_overlay_info(struct of_overlay *ov,
 	if (ovinfo->overlay == NULL)
 		goto err_fail;
 
-	ovinfo->target = find_target_node(info_node);
+	ovinfo->target = find_target_node(ov, info_node, ov->target_index);
 	if (ovinfo->target == NULL)
 		goto err_fail;
 
@@ -473,17 +473,8 @@ static struct kobj_type of_overlay_ktype = {
 
 static struct kset *ov_kset;
 
-/**
- * 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 used to prevent
- * illegal overlay removals.
- *
- * Returns the id of the created overlay, or a negative error number
- */
-int of_overlay_create(struct device_node *tree)
+static int __of_overlay_create(struct device_node *tree,
+		int target_index)
 {
 	struct of_overlay *ov;
 	int err, id;
@@ -498,6 +489,8 @@ int of_overlay_create(struct device_node *tree)
 		return -ENOMEM;
 	ov->id = -1;
 
+	ov->target_index = target_index;
+
 	INIT_LIST_HEAD(&ov->node);
 
 	of_changeset_init(&ov->cset);
@@ -577,8 +570,40 @@ err_destroy_trans:
 
 	return err;
 }
+
+/**
+ * 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 used to prevent
+ * illegal overlay removals.
+ *
+ * Returns the id of the created overlay, or a negative error number
+ */
+int of_overlay_create(struct device_node *tree)
+{
+	return __of_overlay_create(tree, 0);
+}
 EXPORT_SYMBOL_GPL(of_overlay_create);
 
+/**
+ * of_overlay_create_target_index() - Create and apply an overlay
+ * @tree:	Device node containing all the overlays
+ * @index:	Index to use in the target properties
+ *
+ * Creates and applies an overlay while also keeping track
+ * of the overlay in a list. This list can be used to prevent
+ * illegal overlay removals.
+ *
+ * Returns the id of the created overlay, or a negative error number
+ */
+int of_overlay_create_target_index(struct device_node *tree, int index)
+{
+	return __of_overlay_create(tree, index);
+}
+EXPORT_SYMBOL_GPL(of_overlay_create_target_index);
+
 /* check whether the given node, lies under the given tree */
 static int overlay_subtree_check(struct device_node *tree,
 		struct device_node *dn)
diff --git a/include/linux/of.h b/include/linux/of.h
index 97ac5c8..4548773 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1420,6 +1420,8 @@ int of_overlay_create(struct device_node *tree);
 int of_overlay_destroy(int id);
 int of_overlay_destroy_all(void);
 
+int of_overlay_create_target_index(struct device_node *tree, int index);
+
 #else
 
 static inline int of_overlay_create(struct device_node *tree)
@@ -1437,6 +1439,12 @@ static inline int of_overlay_destroy_all(void)
 	return -ENOTSUPP;
 }
 
+static inline int of_overlay_create_target_index(struct device_node *tree,
+		int index)
+{
+	return -ENOTSUPP;
+}
+
 #endif
 
 #endif /* _LINUX_OF_H */
-- 
1.7.12

[PATCH v2 2/6] of: unittest: Add indirect overlay target test

[Pantelis Antoniou]

Add a unittest for the indirect overlay target case.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/unittest-data/testcases.dts      |  5 +++
 drivers/of/unittest-data/tests-overlay.dtsi | 15 ++++++++
 drivers/of/unittest.c                       | 60 +++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+)

diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
index 12f7c3d..74e8805 100644
--- a/drivers/of/unittest-data/testcases.dts
+++ b/drivers/of/unittest-data/testcases.dts
@@ -75,5 +75,10 @@
 				target = <0x00000000>;
 			};
 		};
+		overlay16 {
+			fragment@0 {
+				target = <0x00000000 0x00000004>;
+			};
+		};
 	};
 }; };
diff --git a/drivers/of/unittest-data/tests-overlay.dtsi b/drivers/of/unittest-data/tests-overlay.dtsi
index 02ba56c..ab32996 100644
--- a/drivers/of/unittest-data/tests-overlay.dtsi
+++ b/drivers/of/unittest-data/tests-overlay.dtsi
@@ -110,6 +110,12 @@
 						};
 					};
 				};
+
+				unittest16: test-unittest16 {
+					compatible = "unittest";
+					status = "disabled";
+					reg = <16>;
+				};
 			};
 		};
 
@@ -325,5 +331,14 @@
 			};
 		};
 
+		/* test enable using indirect functionality */
+		overlay16 {
+			fragment@0 {
+				target = <&unittest17>, <&unittest16>;
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
 	};
 };
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index ff6939b..af4c571 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1921,6 +1921,64 @@ static inline void of_unittest_overlay_i2c_15(void) { }
 
 #endif
 
+static void of_unittest_overlay_16(void)
+{
+	int ret;
+	int overlay_nr = 16;
+	int unittest_nr = 16;
+	enum overlay_type ovtype = PDEV_OVERLAY;
+	int before = 0;
+	int after = 1;
+	struct device_node *np = NULL;
+	int id = -1;
+
+	/* unittest device must not be in before state */
+	if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
+		unittest(0, "overlay @\"%s\" with device @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				unittest_path(unittest_nr, ovtype),
+				!before ? "enabled" : "disabled");
+		return;
+	}
+
+	np = of_find_node_by_path(overlay_path(overlay_nr));
+	if (np == NULL) {
+		unittest(0, "could not find overlay node @\"%s\"\n",
+				overlay_path(overlay_nr));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* unittest16 is at index #1 */
+	ret = of_overlay_create_target_index(np, 1);
+	if (ret < 0) {
+		unittest(0, "could not create overlay from \"%s\"\n",
+				overlay_path(overlay_nr));
+		goto out;
+	}
+	id = ret;
+	of_unittest_track_overlay(id);
+
+	ret = 0;
+
+out:
+	of_node_put(np);
+
+	if (ret)
+		return;
+
+	/* unittest device must be to set to after state */
+	if (of_unittest_device_exists(unittest_nr, ovtype) != after) {
+		unittest(0, "overlay @\"%s\" failed to create @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				unittest_path(unittest_nr, ovtype),
+				!after ? "enabled" : "disabled");
+		return;
+	}
+
+	unittest(1, "overlay test %d passed\n", 16);
+}
+
 static void __init of_unittest_overlay(void)
 {
 	struct device_node *bus_np = NULL;
@@ -1972,6 +2030,8 @@ static void __init of_unittest_overlay(void)
 	of_unittest_overlay_10();
 	of_unittest_overlay_11();
 
+	of_unittest_overlay_16();
+
 #if IS_BUILTIN(CONFIG_I2C)
 	if (unittest(of_unittest_overlay_i2c_init() == 0, "i2c init failed\n"))
 		goto out;
-- 
1.7.12

[PATCH v2 3/6] doc: dt: Document the indirect overlay method.

[Pantelis Antoniou]

Add a description of the target index overlay method to the overlay
documention file.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 Documentation/devicetree/overlay-notes.txt | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
index d418a6c..6995fc1 100644
--- a/Documentation/devicetree/overlay-notes.txt
+++ b/Documentation/devicetree/overlay-notes.txt
@@ -100,6 +100,10 @@ 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.
 
+If your board has multiple slots/places where a single overlay can work
+and each slot is defined by a node, you can use the
+of_overlay_create_target_index() method to select the target.
+
 Overlay DTS Format
 ------------------
 
@@ -110,9 +114,11 @@ The DTS of an overlay should have the following format:
 
 	fragment@0 {	/* first child node */
 
-		target=<phandle>;	/* phandle target of the overlay */
+		/* phandle target of the overlay */
+		target=<phandle> [, <phandle>, ...];
 	or
-		target-path="/path";	/* target path of the overlay */
+		/* target path of the overlay */
+		target-path="/path" [ , "/path", ...];
 
 		__overlay__ {
 			property-a;	/* add property-a to the target */
@@ -131,3 +137,7 @@ 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.
+
+Using a target index requires the use of a selector target on the call to
+of_overlay_create_target_index(). I.e. passing an index of 0 will select the
+target in the foo node, an index of 1 the bar node, etc.
-- 
1.7.12

[PATCH v2 4/6] of: overlay: Introduce target root capability.

[Pantelis Antoniou]

The target facility of an overlay allows the target to be any point
in the live kernel tree, since it usually that's required when
creating overlays for internal SoC devices. The target ends up
to be a single node in the tree.

However when we're dealing with probeable busses this is a problem
since the target node differs according to the bus the plugged
device lies.

Using an overlay creating method using a target root node allows
us to use a single overlay for those cases.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/overlay.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++----
 include/linux/of.h   |   8 ++++
 2 files changed, 102 insertions(+), 7 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 2efd4b7..49bdcba 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -76,6 +76,7 @@ struct of_overlay {
 	struct of_changeset cset;
 	struct kobject kobj;
 	int target_index;
+	struct device_node *target_root;
 };
 
 /* master enable switch; once set to 0 can't be re-enabled */
@@ -231,22 +232,85 @@ static int of_overlay_apply(struct of_overlay *ov)
 static struct device_node *find_target_node(struct of_overlay *ov,
 		struct device_node *info_node, int index)
 {
+	struct device_node *target = NULL, *np;
 	const char *path;
+	char *newpath;
 	u32 val;
 	int ret;
 
 	/* first try to go by using the target as a phandle */
 	ret = of_property_read_u32_index(info_node, "target", index, &val);
-	if (ret == 0)
-		return of_find_node_by_phandle(val);
+	if (ret == 0) {
+		target = of_find_node_by_phandle(val);
+		if (!target) {
+			pr_err("%s: Could not find target phandle 0x%x\n",
+					__func__, val);
+			return NULL;
+		}
+		goto check_root;
+	}
 
 	/* failed, try to locate by path */
 	ret = of_property_read_string_index(info_node, "target-path", index,
 			&path);
-	if (ret == 0)
-		return of_find_node_by_path(path);
+	if (ret == 0) {
+
+		if (!ov->target_root) {
+			target = of_find_node_by_path(path);
+			if (!target)
+				pr_err("%s: Could not find target path \"%s\"\n",
+						__func__, path);
+			return target;
+		}
+
+		/* remove preceding '/' from path; relative path */
+		if (*path == '/') {
+			while (*path == '/')
+				path++;
+
+			newpath = kasprintf(GFP_KERNEL, "%s%s%s",
+					of_node_full_name(ov->target_root),
+					*path ? "/" : "", path);
+			if (!newpath) {
+				pr_err("%s: Could not allocate \"%s%s%s\"\n",
+					__func__,
+					of_node_full_name(ov->target_root),
+					*path ? "/" : "", path);
+				return NULL;
+			}
+			target = of_find_node_by_path(newpath);
+			kfree(newpath);
+
+			return target;
+
+		}
+		/* target is an alias, need to check */
+		target = of_find_node_by_path(path);
+		if (!target) {
+			pr_err("%s: Could not find alias \"%s\"\n",
+					__func__, path);
+			return NULL;
+		}
+		goto check_root;
+	}
 
 	return NULL;
+
+check_root:
+	if (!ov->target_root)
+		return target;
+
+	/* got a target, but we have to check it's under target root */
+	for (np = target; np; np = np->parent) {
+		if (np == ov->target_root)
+			return target;
+	}
+	pr_err("%s: target \"%s\" not under target_root \"%s\"\n",
+			__func__, of_node_full_name(target),
+			of_node_full_name(ov->target_root));
+	/* target is not under target_root */
+	of_node_put(target);
+	return NULL;
 }
 
 /**
@@ -418,6 +482,7 @@ void of_overlay_release(struct kobject *kobj)
 {
 	struct of_overlay *ov = kobj_to_overlay(kobj);
 
+	of_node_put(ov->target_root);
 	kfree(ov);
 }
 
@@ -474,7 +539,7 @@ static struct kobj_type of_overlay_ktype = {
 static struct kset *ov_kset;
 
 static int __of_overlay_create(struct device_node *tree,
-		int target_index)
+		int target_index, struct device_node *target_root)
 {
 	struct of_overlay *ov;
 	int err, id;
@@ -490,6 +555,7 @@ static int __of_overlay_create(struct device_node *tree,
 	ov->id = -1;
 
 	ov->target_index = target_index;
+	ov->target_root = of_node_get(target_root);
 
 	INIT_LIST_HEAD(&ov->node);
 
@@ -565,6 +631,7 @@ err_free_idr:
 	idr_remove(&ov_idr, ov->id);
 err_destroy_trans:
 	of_changeset_destroy(&ov->cset);
+	of_node_put(ov->target_root);
 	kfree(ov);
 	mutex_unlock(&of_mutex);
 
@@ -583,7 +650,7 @@ err_destroy_trans:
  */
 int of_overlay_create(struct device_node *tree)
 {
-	return __of_overlay_create(tree, 0);
+	return __of_overlay_create(tree, 0, NULL);
 }
 EXPORT_SYMBOL_GPL(of_overlay_create);
 
@@ -600,10 +667,30 @@ EXPORT_SYMBOL_GPL(of_overlay_create);
  */
 int of_overlay_create_target_index(struct device_node *tree, int index)
 {
-	return __of_overlay_create(tree, index);
+	return __of_overlay_create(tree, index, NULL);
 }
 EXPORT_SYMBOL_GPL(of_overlay_create_target_index);
 
+/**
+ * of_overlay_create_target_root() - Create and apply an overlay
+ *			under which will be limited to target_root
+ * @tree:		Device node containing all the overlays
+ * @target_root:	Target root for the overlay.
+ *
+ * Creates and applies an overlay while also keeping track
+ * of the overlay in a list. This list can be used to prevent
+ * illegal overlay removals. The overlay is only allowed to
+ * target nodes under the target_root node.
+ *
+ * Returns the id of the created overlay, or an negative error number
+ */
+int of_overlay_create_target_root(struct device_node *tree,
+		struct device_node *target_root)
+{
+	return __of_overlay_create(tree, 0, target_root);
+}
+EXPORT_SYMBOL_GPL(of_overlay_create_target_root);
+
 /* check whether the given node, lies under the given tree */
 static int overlay_subtree_check(struct device_node *tree,
 		struct device_node *dn)
diff --git a/include/linux/of.h b/include/linux/of.h
index 4548773..23d334c 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1421,6 +1421,8 @@ int of_overlay_destroy(int id);
 int of_overlay_destroy_all(void);
 
 int of_overlay_create_target_index(struct device_node *tree, int index);
+int of_overlay_create_target_root(struct device_node *tree,
+		struct device_node *target_root);
 
 #else
 
@@ -1445,6 +1447,12 @@ static inline int of_overlay_create_target_index(struct device_node *tree,
 	return -ENOTSUPP;
 }
 
+static inline int of_overlay_create_target_root(struct device_node *tree,
+		struct device_node *target_root)
+{
+	return -ENOTSUPP;
+}
+
 #endif
 
 #endif /* _LINUX_OF_H */
-- 
1.7.12

[PATCH v2 5/6] of: unittest: Unit-tests for target root overlays.

[Pantelis Antoniou]

Add unittests for target-root based overlays.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/unittest-data/testcases.dts      |   5 +
 drivers/of/unittest-data/tests-overlay.dtsi |  48 +++++++
 drivers/of/unittest.c                       | 213 ++++++++++++++++++++++++++++
 3 files changed, 266 insertions(+)

diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
index 74e8805..a6ded1b6 100644
--- a/drivers/of/unittest-data/testcases.dts
+++ b/drivers/of/unittest-data/testcases.dts
@@ -80,5 +80,10 @@
 				target = <0x00000000 0x00000004>;
 			};
 		};
+		overlay18 {
+			fragment@0 {
+				target = <0x00000000>;
+			};
+		};
 	};
 }; };
diff --git a/drivers/of/unittest-data/tests-overlay.dtsi b/drivers/of/unittest-data/tests-overlay.dtsi
index ab32996..170b04d 100644
--- a/drivers/of/unittest-data/tests-overlay.dtsi
+++ b/drivers/of/unittest-data/tests-overlay.dtsi
@@ -116,6 +116,24 @@
 					status = "disabled";
 					reg = <16>;
 				};
+
+				unittest17: test-unittest17 {
+					compatible = "unittest";
+					status = "disabled";
+					reg = <17>;
+				};
+
+				unittest18: test-unittest18 {
+					compatible = "unittest";
+					status = "disabled";
+					reg = <18>;
+				};
+
+				unittest19: test-unittest19 {
+					compatible = "unittest";
+					status = "disabled";
+					reg = <19>;
+				};
 			};
 		};
 
@@ -340,5 +358,35 @@
 				};
 			};
 		};
+
+		/* test enable using target root (relative path) */
+		overlay17 {
+			fragment@0 {
+				target-path = "/";
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
+
+		/* test enable using target phandle */
+		overlay18 {
+			fragment@0 {
+				target = <&unittest18>;
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
+
+		/* test trying to enable out of root (should fail) */
+		overlay19 {
+			fragment@0 {
+				target = <&unittest19>;
+				__overlay__ {
+					status = "okay";
+				};
+			};
+		};
 	};
 };
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index af4c571..7f288e6 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1979,6 +1979,215 @@ out:
 	unittest(1, "overlay test %d passed\n", 16);
 }
 
+static void of_unittest_overlay_17(void)
+{
+	int ret;
+	int overlay_nr = 17;
+	int unittest_nr = 17;
+	enum overlay_type ovtype = PDEV_OVERLAY;
+	int before = 0;
+	int after = 1;
+	const char *root_path;
+	struct device_node *np = NULL, *target_root = NULL;
+	int id = -1;
+
+	/* unittest device must not be in before state */
+	if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
+		unittest(0, "overlay @\"%s\" with device @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				unittest_path(unittest_nr, ovtype),
+				!before ? "enabled" : "disabled");
+		return;
+	}
+
+	np = of_find_node_by_path(overlay_path(overlay_nr));
+	if (np == NULL) {
+		unittest(0, "could not find overlay node @\"%s\"\n",
+				overlay_path(overlay_nr));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	root_path = "/testcase-data/overlay-node/test-bus/test-unittest17";
+	target_root = of_find_node_by_path(root_path);
+	if (!target_root) {
+		unittest(0, "could not find target_root node @\"%s\"\n",
+				root_path);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = of_overlay_create_target_root(np, target_root);
+	of_node_put(target_root);
+
+	if (ret < 0) {
+		unittest(0, "could not create overlay from \"%s\"\n",
+				overlay_path(overlay_nr));
+		goto out;
+	}
+	id = ret;
+	of_unittest_track_overlay(id);
+
+	ret = 0;
+
+out:
+	of_node_put(np);
+
+	if (ret)
+		return;
+
+	/* unittest device must be to set to after state */
+	if (of_unittest_device_exists(unittest_nr, ovtype) != after) {
+		unittest(0, "overlay @\"%s\" failed to create @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				unittest_path(unittest_nr, ovtype),
+				!after ? "enabled" : "disabled");
+		return;
+	}
+
+	unittest(1, "overlay test %d passed\n", 17);
+}
+
+static void of_unittest_overlay_18(void)
+{
+	int ret;
+	int overlay_nr = 18;
+	int unittest_nr = 18;
+	enum overlay_type ovtype = PDEV_OVERLAY;
+	int before = 0;
+	int after = 1;
+	const char *root_path;
+	struct device_node *np = NULL, *target_root = NULL;
+	int id = -1;
+
+	/* unittest device must not be in before state */
+	if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
+		unittest(0, "overlay @\"%s\" with device @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				unittest_path(unittest_nr, ovtype),
+				!before ? "enabled" : "disabled");
+		return;
+	}
+
+	np = of_find_node_by_path(overlay_path(overlay_nr));
+	if (np == NULL) {
+		unittest(0, "could not find overlay node @\"%s\"\n",
+				overlay_path(overlay_nr));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	root_path = "/testcase-data/overlay-node/test-bus/test-unittest18";
+	target_root = of_find_node_by_path(root_path);
+	if (!target_root) {
+		unittest(0, "could not find target_root node @\"%s\"\n",
+				root_path);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = of_overlay_create_target_root(np, target_root);
+	of_node_put(target_root);
+
+	if (ret < 0) {
+		unittest(0, "could not create overlay from \"%s\"\n",
+				overlay_path(overlay_nr));
+		goto out;
+	}
+	id = ret;
+	of_unittest_track_overlay(id);
+
+	ret = 0;
+
+out:
+	of_node_put(np);
+
+	if (ret)
+		return;
+
+	/* unittest device must be to set to after state */
+	if (of_unittest_device_exists(unittest_nr, ovtype) != after) {
+		unittest(0, "overlay @\"%s\" failed to create @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				unittest_path(unittest_nr, ovtype),
+				!after ? "enabled" : "disabled");
+		return;
+	}
+
+	unittest(1, "overlay test %d passed\n", 18);
+}
+
+static void of_unittest_overlay_19(void)
+{
+	int ret;
+	int overlay_nr = 19;
+	int unittest_nr = 19;
+	enum overlay_type ovtype = PDEV_OVERLAY;
+	int before = 0;
+	int after = 0;
+	const char *root_path;
+	struct device_node *np = NULL, *target_root = NULL;
+	int id = -1;
+
+	/* unittest device must not be in before state */
+	if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
+		unittest(0, "overlay @\"%s\" with device @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				unittest_path(unittest_nr, ovtype),
+				!before ? "enabled" : "disabled");
+		return;
+	}
+
+	np = of_find_node_by_path(overlay_path(overlay_nr));
+	if (np == NULL) {
+		unittest(0, "could not find overlay node @\"%s\"\n",
+				overlay_path(overlay_nr));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	root_path = "/testcase-data/overlay-node/test-bus/test-unittest19";
+	target_root = of_find_node_by_path(root_path);
+	if (!target_root) {
+		unittest(0, "could not find target_root node @\"%s\"\n",
+				root_path);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = of_overlay_create_target_root(np, target_root);
+	of_node_put(target_root);
+
+	if (ret >= 0) {
+		unittest(0, "created overlay from \"%s\" while we shouldn't\n",
+				overlay_path(overlay_nr));
+		id = ret;
+		of_unittest_track_overlay(id);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = 0;
+
+out:
+	of_node_put(np);
+
+	if (ret)
+		return;
+
+	/* unittest device must be to set to after state */
+	if (of_unittest_device_exists(unittest_nr, ovtype) != after) {
+		unittest(0, "overlay @\"%s\" failed to create @\"%s\" %s\n",
+				overlay_path(overlay_nr),
+				unittest_path(unittest_nr, ovtype),
+				!after ? "enabled" : "disabled");
+		return;
+	}
+
+	unittest(1, "overlay test %d passed\n", 16);
+}
+
+
 static void __init of_unittest_overlay(void)
 {
 	struct device_node *bus_np = NULL;
@@ -2032,6 +2241,10 @@ static void __init of_unittest_overlay(void)
 
 	of_unittest_overlay_16();
 
+	of_unittest_overlay_17();
+	of_unittest_overlay_18();
+	of_unittest_overlay_19();
+
 #if IS_BUILTIN(CONFIG_I2C)
 	if (unittest(of_unittest_overlay_i2c_init() == 0, "i2c init failed\n"))
 		goto out;
-- 
1.7.12

Re: [PATCH v2 5/6] of: unittest: Unit-tests for target root overlays.

[Geert Uytterhoeven]

On Mon, May 16, 2016 at 10:18 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Add unittests for target-root based overlays.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  drivers/of/unittest-data/testcases.dts      |   5 +
>  drivers/of/unittest-data/tests-overlay.dtsi |  48 +++++++
>  drivers/of/unittest.c                       | 213 ++++++++++++++++++++++++++++
>  3 files changed, 266 insertions(+)
>
> diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
> index 74e8805..a6ded1b6 100644
> --- a/drivers/of/unittest-data/testcases.dts
> +++ b/drivers/of/unittest-data/testcases.dts
> @@ -80,5 +80,10 @@
>                                 target = <0x00000000 0x00000004>;
>                         };
>                 };
> +               overlay18 {
> +                       fragment@0 {

Warning (unit_address_vs_reg): Node .../fragment@0 has a unit name,
but no reg property

(I didn't really run dtc, just copying a warning message from somewhere else)

> +                               target = <0x00000000>;
> +                       };
> +               };
>         };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v2 6/6] doc: dt: Document the target root overlay method

[Pantelis Antoniou]

Add a description of the target root overlay method to the overlay
documention file.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 Documentation/devicetree/overlay-notes.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
index 6995fc1..3e8df30 100644
--- a/Documentation/devicetree/overlay-notes.txt
+++ b/Documentation/devicetree/overlay-notes.txt
@@ -104,6 +104,10 @@ If your board has multiple slots/places where a single overlay can work
 and each slot is defined by a node, you can use the
 of_overlay_create_target_index() method to select the target.
 
+For overlays on probeable busses, use the of_overlay_create_target_root() method
+in which you supply a device node as a target root, and which all target
+references in the overlay are performed relative to that node.
+
 Overlay DTS Format
 ------------------
 
@@ -141,3 +145,7 @@ contains the information required to map from a phandle to a tree location.
 Using a target index requires the use of a selector target on the call to
 of_overlay_create_target_index(). I.e. passing an index of 0 will select the
 target in the foo node, an index of 1 the bar node, etc.
+
+Note that when using the target root create method all target references must
+lie under the target root node. I.e. the overlay is not allowed to 'break' out
+of the root.
-- 
1.7.12

Re: [PATCH v2 6/6] doc: dt: Document the target root overlay method

[Rob Herring]

On Mon, May 16, 2016 at 3:18 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Add a description of the target root overlay method to the overlay
> documention file.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  Documentation/devicetree/overlay-notes.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
> index 6995fc1..3e8df30 100644
> --- a/Documentation/devicetree/overlay-notes.txt
> +++ b/Documentation/devicetree/overlay-notes.txt
> @@ -104,6 +104,10 @@ If your board has multiple slots/places where a single overlay can work
>  and each slot is defined by a node, you can use the
>  of_overlay_create_target_index() method to select the target.
>
> +For overlays on probeable busses, use the of_overlay_create_target_root() method
> +in which you supply a device node as a target root, and which all target
> +references in the overlay are performed relative to that node.

This needs a better explanation and an example. "Applying overlays to
multiple places for probeable busses" is not sufficient. Describe what
is the problem/flow, and then how does the implementation work. I
don't think this problem is limited to probeable busses either. Not
knowing the target to apply the overlay to is the same problem for
connectors with non-probeable signals.

I find a couple of things problematic with the implementation. There's
no way to validate that an overlay should apply to a base node. If
users just always make the target "/", then any overlay can apply to
any location the nodes could apply anywhere. It requires a kernel
change for every location you want to apply the overlay to. Maybe I
don't understand the usecase.

I feel like we are creating too many syntax's to apply overlays. I get
that there are different usecases, but that doesn't necessarily mean
the syntax needs to be different.

> +
>  Overlay DTS Format
>  ------------------
>
> @@ -141,3 +145,7 @@ contains the information required to map from a phandle to a tree location.
>  Using a target index requires the use of a selector target on the call to
>  of_overlay_create_target_index(). I.e. passing an index of 0 will select the
>  target in the foo node, an index of 1 the bar node, etc.
> +
> +Note that when using the target root create method all target references must
> +lie under the target root node. I.e. the overlay is not allowed to 'break' out
> +of the root.

That is not enforceable though, right?

Rob

Re: [PATCH v2 6/6] doc: dt: Document the target root overlay method

[Pantelis Antoniou]

Hi Rob,

> On May 17, 2016, at 15:58 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Mon, May 16, 2016 at 3:18 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Add a description of the target root overlay method to the overlay
>> documention file.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> Documentation/devicetree/overlay-notes.txt | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt
>> index 6995fc1..3e8df30 100644
>> --- a/Documentation/devicetree/overlay-notes.txt
>> +++ b/Documentation/devicetree/overlay-notes.txt
>> @@ -104,6 +104,10 @@ If your board has multiple slots/places where a single overlay can work
>> and each slot is defined by a node, you can use the
>> of_overlay_create_target_index() method to select the target.
>> 
>> +For overlays on probeable busses, use the of_overlay_create_target_root() method
>> +in which you supply a device node as a target root, and which all target
>> +references in the overlay are performed relative to that node.
> 
> This needs a better explanation and an example. "Applying overlays to
> multiple places for probeable busses" is not sufficient. Describe what
> is the problem/flow, and then how does the implementation work. I
> don't think this problem is limited to probeable busses either. Not
> knowing the target to apply the overlay to is the same problem for
> connectors with non-probeable signals.
> 

OK.

> I find a couple of things problematic with the implementation. There's
> no way to validate that an overlay should apply to a base node. If
> users just always make the target "/", then any overlay can apply to
> any location the nodes could apply anywhere. It requires a kernel
> change for every location you want to apply the overlay to. Maybe I
> don't understand the usecase.
> 

No, this doesn’t work like this. This is an in kernel interface.
The piece of kernel software that applies the overlay has an additional
security policy about where the target root may point to.

> I feel like we are creating too many syntax's to apply overlays. I get
> that there are different usecases, but that doesn't necessarily mean
> the syntax needs to be different.
> 
>> +
>> Overlay DTS Format
>> ------------------
>> 
>> @@ -141,3 +145,7 @@ contains the information required to map from a phandle to a tree location.
>> Using a target index requires the use of a selector target on the call to
>> of_overlay_create_target_index(). I.e. passing an index of 0 will select the
>> target in the foo node, an index of 1 the bar node, etc.
>> +
>> +Note that when using the target root create method all target references must
>> +lie under the target root node. I.e. the overlay is not allowed to 'break' out
>> +of the root.
> 
> That is not enforceable though, right?
> 

It is enforceable. All the targets of an overlay incur an additional check that
make sure that no targets break out.

> Rob

Regards

— Pantelis

Re: [PATCH v2 0/6] of: overlays: New target methods

[Frank Rowand]

On 5/16/2016 1:18 PM, Pantelis Antoniou wrote:
> This patchset implements two new target methods.
> 
> A target index method which allows selecting different
> targets according to an argument using an extended API and
> a target root method that fences the target only
> to a specific given root.
> 
> Documentation and unit-tests are included.

I think you are attacking the problem the wrong way.

If I understand correctly, the problem statement is:

  In some cases, a devicetree overlay is meant to describe
  a pluggable piece of hardware, which may be plugged into
  various locations on a platform.  It should be possible
  to apply a single devicetree to one or more locations
  on a given platform.

If that is the case, then putting the locations where the
overlay can be applied into the devicetree is not the
approach that I would use.  It seems it would be better
to specify the target location as a separate item from
the overlay to the method that applies the devicetree.
In that case, I would put the node(s) describing the
pluggable hardware in the root node of the overlay
devicetree (dtc expects a root node).  The apply
method can easily find the node(s) and relocate them
to the appropriate location in the platform's
devicetree.

-Frank

Re: [PATCH v2 0/6] of: overlays: New target methods

[Pantelis Antoniou]

Hi Frank,

> On May 27, 2016, at 00:15 , Frank Rowand <frowand.list@gmail.com> wrote:
> 
> On 5/16/2016 1:18 PM, Pantelis Antoniou wrote:
>> This patchset implements two new target methods.
>> 
>> A target index method which allows selecting different
>> targets according to an argument using an extended API and
>> a target root method that fences the target only
>> to a specific given root.
>> 
>> Documentation and unit-tests are included.
> 
> I think you are attacking the problem the wrong way.
> 
> If I understand correctly, the problem statement is:
> 
>  In some cases, a devicetree overlay is meant to describe
>  a pluggable piece of hardware, which may be plugged into
>  various locations on a platform.  It should be possible
>  to apply a single devicetree to one or more locations
>  on a given platform.
> 
> If that is the case, then putting the locations where the
> overlay can be applied into the devicetree is not the
> approach that I would use.  It seems it would be better
> to specify the target location as a separate item from
> the overlay to the method that applies the devicetree.
> In that case, I would put the node(s) describing the
> pluggable hardware in the root node of the overlay
> devicetree (dtc expects a root node).  The apply
> method can easily find the node(s) and relocate them
> to the appropriate location in the platform's
> devicetree.
> 

It’s a bit more complicated. This was considered initially
but we ended up with the targets on the overlay.

It can work either way, but the problem with storing the
indirect targets in the base tree is that there is no
change in the bindings of the targets.

Putting things in the overlay seemed like it would have
no effect on the base tree whatsoever.


> -Frank
> 

Regards

— Pantelis

Re: [PATCH v2 0/6] of: overlays: New target methods

[Frank Rowand]

On 5/27/2016 7:46 AM, Pantelis Antoniou wrote:
> Hi Frank,
> 
>> On May 27, 2016, at 00:15 , Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 5/16/2016 1:18 PM, Pantelis Antoniou wrote:
>>> This patchset implements two new target methods.
>>>
>>> A target index method which allows selecting different
>>> targets according to an argument using an extended API and
>>> a target root method that fences the target only
>>> to a specific given root.
>>>
>>> Documentation and unit-tests are included.
>>
>> I think you are attacking the problem the wrong way.
>>
>> If I understand correctly, the problem statement is:
>>
>>  In some cases, a devicetree overlay is meant to describe
>>  a pluggable piece of hardware, which may be plugged into
>>  various locations on a platform.  It should be possible
>>  to apply a single devicetree to one or more locations
>>  on a given platform.
>>
>> If that is the case, then putting the locations where the
>> overlay can be applied into the devicetree is not the
>> approach that I would use.  It seems it would be better
>> to specify the target location as a separate item from
>> the overlay to the method that applies the devicetree.
>> In that case, I would put the node(s) describing the
>> pluggable hardware in the root node of the overlay
>> devicetree (dtc expects a root node).  The apply
>> method can easily find the node(s) and relocate them
>> to the appropriate location in the platform's
>> devicetree.
>>
> 
> It’s a bit more complicated. This was considered initially
> but we ended up with the targets on the overlay.
> 
> It can work either way, but the problem with storing the
> indirect targets in the base tree is that there is no
> change in the bindings of the targets.

I am not suggesting putting the targets in the base tree.

I do not know where it should be.  Still thinking about
that part.

> 
> Putting things in the overlay seemed like it would have
> no effect on the base tree whatsoever.
> 
> 
>> -Frank
>>
> 
> Regards
> 
> — Pantelis
> 
>