[PATCH 0/6 v2] of: Implement iterator for phandles

[PATCH 0/6 v2] of: Implement iterator for phandles

[Joerg Roedel]

Hi,

here is a new version of the implementation of the iterator
over phandles concept which Rob Herring suggested to me some
time ago. My approach is a little bit different from what
the diff showed back then, but it gets rid of the allocation
and 'struct of_phandle_args' misuse.

I also converted the arm-smmu driver to make use of the
iterator. The resulting kernel boots on my AMD Seattle
system and fixes the warning triggered there. The patches
now also pass all dt-unittests in my kvm environment.

Patches are based on v4.6-rc2

Thanks,

	Joerg

Changes since v1:

* Rebased to v4.6-rc2

* Removed the 'if (i == 0)...' hunk from the last patch

* Tested again with DT unit-tests and on the seattle system

Changes since RFC post:

* Reordered members of 'struct of_phandle_iterator' and did
  some renaming

* Removed index counting from the iterator

* Split up iterator implementation into multiple patches

* Fixed the code to survive all dt-unittests, tested with each
  patch in this series

* Re-added and updated some comments which got lost during the
  conversion.

* Added of_for_each_phandle macro for easier handling

* Moved the counting special-case from __of_parse_phandle_with_args
  directly to of_count_phandle_with_args for code
  simplification

* Removed some iterator helper functions

* Formatting and style changes

Joerg Roedel (6):
  of: Introduce struct of_phandle_iterator
  of: Move phandle walking to of_phandle_iterator_next()
  of: Remove counting special case from __of_parse_phandle_with_args()
  of: Introduce of_for_each_phandle() helper macro
  of: Introduce of_phandle_iterator_args()
  iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

 drivers/iommu/arm-smmu.c |  38 +++++++--
 drivers/of/base.c        | 206 ++++++++++++++++++++++++++++++-----------------
 include/linux/of.h       |  56 +++++++++++++
 3 files changed, 219 insertions(+), 81 deletions(-)

-- 
1.9.1

[PATCH 1/6] of: Introduce struct of_phandle_iterator

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This struct carrys all necessary information to iterate over
a list of phandles and extract the arguments. Add an
init-function for the iterator and make use of it in
__of_parse_phandle_with_args().

Signed-off-by: Joerg Roedel <jroedel@suse.de>
Acked-by: Rob Herring <robh@kernel.org>
---
 drivers/of/base.c  | 99 +++++++++++++++++++++++++++++++++---------------------
 include/linux/of.h | 33 ++++++++++++++++++
 2 files changed, 93 insertions(+), 39 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index b299de2..1c6f43b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1440,35 +1440,56 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
 	printk("\n");
 }
 
+int of_phandle_iterator_init(struct of_phandle_iterator *it,
+		const struct device_node *np,
+		const char *list_name,
+		const char *cells_name,
+		int cell_count)
+{
+	const __be32 *list;
+	int size;
+
+	memset(it, 0, sizeof(*it));
+
+	list = of_get_property(np, list_name, &size);
+	if (!list)
+		return -ENOENT;
+
+	it->cells_name = cells_name;
+	it->cell_count = cell_count;
+	it->parent = np;
+	it->list_end = list + size / sizeof(*list);
+	it->phandle_end = list;
+	it->cur = list;
+
+	return 0;
+}
+
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name,
 					int cell_count, int index,
 					struct of_phandle_args *out_args)
 {
-	const __be32 *list, *list_end;
-	int rc = 0, size, cur_index = 0;
-	uint32_t count = 0;
-	struct device_node *node = NULL;
-	phandle phandle;
+	struct of_phandle_iterator it;
+	int rc, cur_index = 0;
 
-	/* Retrieve the phandle list property */
-	list = of_get_property(np, list_name, &size);
-	if (!list)
-		return -ENOENT;
-	list_end = list + size / sizeof(*list);
+	rc = of_phandle_iterator_init(&it, np, list_name,
+				      cells_name, cell_count);
+	if (rc)
+		return rc;
 
 	/* Loop over the phandles until all the requested entry is found */
-	while (list < list_end) {
+	while (it.cur < it.list_end) {
 		rc = -EINVAL;
-		count = 0;
+		it.cur_count = 0;
 
 		/*
 		 * If phandle is 0, then it is an empty entry with no
 		 * arguments.  Skip forward to the next entry.
 		 */
-		phandle = be32_to_cpup(list++);
-		if (phandle) {
+		it.phandle = be32_to_cpup(it.cur++);
+		if (it.phandle) {
 			/*
 			 * Find the provider node and parse the #*-cells
 			 * property to determine the argument length.
@@ -1478,34 +1499,34 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 			 * except when we're going to return the found node
 			 * below.
 			 */
-			if (cells_name || cur_index == index) {
-				node = of_find_node_by_phandle(phandle);
-				if (!node) {
+			if (it.cells_name || cur_index == index) {
+				it.node = of_find_node_by_phandle(it.phandle);
+				if (!it.node) {
 					pr_err("%s: could not find phandle\n",
-						np->full_name);
+						it.parent->full_name);
 					goto err;
 				}
 			}
 
-			if (cells_name) {
-				if (of_property_read_u32(node, cells_name,
-							 &count)) {
+			if (it.cells_name) {
+				if (of_property_read_u32(it.node, it.cells_name,
+							 &it.cur_count)) {
 					pr_err("%s: could not get %s for %s\n",
-						np->full_name, cells_name,
-						node->full_name);
+						it.parent->full_name, it.cells_name,
+						it.node->full_name);
 					goto err;
 				}
 			} else {
-				count = cell_count;
+				it.cur_count = it.cell_count;
 			}
 
 			/*
 			 * Make sure that the arguments actually fit in the
 			 * remaining property data length
 			 */
-			if (list + count > list_end) {
+			if (it.cur + it.cur_count > it.list_end) {
 				pr_err("%s: arguments longer than property\n",
-					 np->full_name);
+					 it.parent->full_name);
 				goto err;
 			}
 		}
@@ -1518,28 +1539,28 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 		 */
 		rc = -ENOENT;
 		if (cur_index == index) {
-			if (!phandle)
+			if (!it.phandle)
 				goto err;
 
 			if (out_args) {
 				int i;
-				if (WARN_ON(count > MAX_PHANDLE_ARGS))
-					count = MAX_PHANDLE_ARGS;
-				out_args->np = node;
-				out_args->args_count = count;
-				for (i = 0; i < count; i++)
-					out_args->args[i] = be32_to_cpup(list++);
+				if (WARN_ON(it.cur_count > MAX_PHANDLE_ARGS))
+					it.cur_count = MAX_PHANDLE_ARGS;
+				out_args->np = it.node;
+				out_args->args_count = it.cur_count;
+				for (i = 0; i < it.cur_count; i++)
+					out_args->args[i] = be32_to_cpup(it.cur++);
 			} else {
-				of_node_put(node);
+				of_node_put(it.node);
 			}
 
 			/* Found it! return success */
 			return 0;
 		}
 
-		of_node_put(node);
-		node = NULL;
-		list += count;
+		of_node_put(it.node);
+		it.node = NULL;
+		it.cur += it.cur_count;
 		cur_index++;
 	}
 
@@ -1551,8 +1572,8 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 	 */
 	rc = index < 0 ? cur_index : -ENOENT;
  err:
-	if (node)
-		of_node_put(node);
+	if (it.node)
+		of_node_put(it.node);
 	return rc;
 }
 
diff --git a/include/linux/of.h b/include/linux/of.h
index 7fcb681..0f187db 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -75,6 +75,23 @@ struct of_phandle_args {
 	uint32_t args[MAX_PHANDLE_ARGS];
 };
 
+struct of_phandle_iterator {
+	/* Common iterator information */
+	const char *cells_name;
+	int cell_count;
+	const struct device_node *parent;
+
+	/* List size information */
+	const __be32 *list_end;
+	const __be32 *phandle_end;
+
+	/* Current position state */
+	const __be32 *cur;
+	uint32_t cur_count;
+	phandle phandle;
+	struct device_node *node;
+};
+
 struct of_reconfig_data {
 	struct device_node	*dn;
 	struct property		*prop;
@@ -334,6 +351,13 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
 extern int of_count_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name);
 
+/* phandle iterator functions */
+extern int of_phandle_iterator_init(struct of_phandle_iterator *it,
+				    const struct device_node *np,
+				    const char *list_name,
+				    const char *cells_name,
+				    int cell_count);
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 extern int of_alias_get_highest_id(const char *stem);
@@ -608,6 +632,15 @@ static inline int of_count_phandle_with_args(struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline int of_phandle_iterator_init(struct of_phandle_iterator *it,
+					   const struct device_node *np,
+					   const char *list_name,
+					   const char *cells_name,
+					   int cell_count)
+{
+	return -ENOSYS;
+}
+
 static inline int of_alias_get_id(struct device_node *np, const char *stem)
 {
 	return -ENOSYS;
-- 
1.9.1

[PATCH 2/6] of: Move phandle walking to of_phandle_iterator_next()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Move the code to walk over the phandles out of the loop in
__of_parse_phandle_with_args() to a separate function that
just works with the iterator handle: of_phandle_iterator_next().

Signed-off-by: Joerg Roedel <jroedel@suse.de>
Acked-by: Rob Herring <robh@kernel.org>
---
 drivers/of/base.c  | 130 ++++++++++++++++++++++++++++++-----------------------
 include/linux/of.h |   7 +++
 2 files changed, 81 insertions(+), 56 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 1c6f43b..69286ec 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1465,6 +1465,75 @@ int of_phandle_iterator_init(struct of_phandle_iterator *it,
 	return 0;
 }
 
+int of_phandle_iterator_next(struct of_phandle_iterator *it)
+{
+	uint32_t count = 0;
+
+	if (it->node) {
+		of_node_put(it->node);
+		it->node = NULL;
+	}
+
+	if (!it->cur || it->phandle_end >= it->list_end)
+		return -ENOENT;
+
+	it->cur = it->phandle_end;
+
+	/* If phandle is 0, then it is an empty entry with no arguments. */
+	it->phandle = be32_to_cpup(it->cur++);
+
+	if (it->phandle) {
+
+		/*
+		 * Find the provider node and parse the #*-cells property to
+		 * determine the argument length.
+		 */
+		it->node = of_find_node_by_phandle(it->phandle);
+
+		if (it->cells_name) {
+			if (!it->node) {
+				pr_err("%s: could not find phandle\n",
+				       it->parent->full_name);
+				goto err;
+			}
+
+			if (of_property_read_u32(it->node, it->cells_name,
+						 &count)) {
+				pr_err("%s: could not get %s for %s\n",
+				       it->parent->full_name,
+				       it->cells_name,
+				       it->node->full_name);
+				goto err;
+			}
+		} else {
+			count = it->cell_count;
+		}
+
+		/*
+		 * Make sure that the arguments actually fit in the remaining
+		 * property data length
+		 */
+		if (it->cur + count > it->list_end) {
+			pr_err("%s: arguments longer than property\n",
+			       it->parent->full_name);
+			goto err;
+		}
+	}
+
+	it->phandle_end = it->cur + count;
+	it->cur_count = count;
+
+	return 0;
+
+err:
+	if (it->node) {
+		of_node_put(it->node);
+		it->node = NULL;
+	}
+
+	return -EINVAL;
+}
+
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name,
@@ -1480,59 +1549,9 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 		return rc;
 
 	/* Loop over the phandles until all the requested entry is found */
-	while (it.cur < it.list_end) {
-		rc = -EINVAL;
-		it.cur_count = 0;
-
-		/*
-		 * If phandle is 0, then it is an empty entry with no
-		 * arguments.  Skip forward to the next entry.
-		 */
-		it.phandle = be32_to_cpup(it.cur++);
-		if (it.phandle) {
-			/*
-			 * Find the provider node and parse the #*-cells
-			 * property to determine the argument length.
-			 *
-			 * This is not needed if the cell count is hard-coded
-			 * (i.e. cells_name not set, but cell_count is set),
-			 * except when we're going to return the found node
-			 * below.
-			 */
-			if (it.cells_name || cur_index == index) {
-				it.node = of_find_node_by_phandle(it.phandle);
-				if (!it.node) {
-					pr_err("%s: could not find phandle\n",
-						it.parent->full_name);
-					goto err;
-				}
-			}
-
-			if (it.cells_name) {
-				if (of_property_read_u32(it.node, it.cells_name,
-							 &it.cur_count)) {
-					pr_err("%s: could not get %s for %s\n",
-						it.parent->full_name, it.cells_name,
-						it.node->full_name);
-					goto err;
-				}
-			} else {
-				it.cur_count = it.cell_count;
-			}
-
-			/*
-			 * Make sure that the arguments actually fit in the
-			 * remaining property data length
-			 */
-			if (it.cur + it.cur_count > it.list_end) {
-				pr_err("%s: arguments longer than property\n",
-					 it.parent->full_name);
-				goto err;
-			}
-		}
-
+	while ((rc = of_phandle_iterator_next(&it)) == 0) {
 		/*
-		 * All of the error cases above bail out of the loop, so at
+		 * All of the error cases bail out of the loop, so at
 		 * this point, the parsing is successful. If the requested
 		 * index matches, then fill the out_args structure and return,
 		 * or return -ENOENT for an empty entry.
@@ -1558,9 +1577,6 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 			return 0;
 		}
 
-		of_node_put(it.node);
-		it.node = NULL;
-		it.cur += it.cur_count;
 		cur_index++;
 	}
 
@@ -1570,7 +1586,9 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 	 * -EINVAL : parsing error on data
 	 * [1..n]  : Number of phandle (count mode; when index = -1)
 	 */
-	rc = index < 0 ? cur_index : -ENOENT;
+	if (rc == -ENOENT && index < 0)
+		rc = cur_index;
+
  err:
 	if (it.node)
 		of_node_put(it.node);
diff --git a/include/linux/of.h b/include/linux/of.h
index 0f187db..1f5e108 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -358,6 +358,8 @@ extern int of_phandle_iterator_init(struct of_phandle_iterator *it,
 				    const char *cells_name,
 				    int cell_count);
 
+extern int of_phandle_iterator_next(struct of_phandle_iterator *it);
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 extern int of_alias_get_highest_id(const char *stem);
@@ -641,6 +643,11 @@ static inline int of_phandle_iterator_init(struct of_phandle_iterator *it,
 	return -ENOSYS;
 }
 
+static inline int of_phandle_iterator_next(struct of_phandle_iterator *it)
+{
+	return -ENOSYS;
+}
+
 static inline int of_alias_get_id(struct device_node *np, const char *stem)
 {
 	return -ENOSYS;
-- 
1.9.1

[PATCH 3/6] of: Remove counting special case from __of_parse_phandle_with_args()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The index = -1 case in __of_parse_phandle_with_args() is
used to just return the number of phandles. That special
case needs extra handling, so move it to the place where it
is needed: of_count_phandle_with_args().

This allows to further simplify __of_parse_phandle_with_args()
later on.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
Acked-by: Rob Herring <robh@kernel.org>
---
 drivers/of/base.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 69286ec..fcff2b6 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1584,10 +1584,7 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 	 * Unlock node before returning result; will be one of:
 	 * -ENOENT : index is for empty phandle
 	 * -EINVAL : parsing error on data
-	 * [1..n]  : Number of phandle (count mode; when index = -1)
 	 */
-	if (rc == -ENOENT && index < 0)
-		rc = cur_index;
 
  err:
 	if (it.node)
@@ -1723,8 +1720,20 @@ EXPORT_SYMBOL(of_parse_phandle_with_fixed_args);
 int of_count_phandle_with_args(const struct device_node *np, const char *list_name,
 				const char *cells_name)
 {
-	return __of_parse_phandle_with_args(np, list_name, cells_name, 0, -1,
-					    NULL);
+	struct of_phandle_iterator it;
+	int rc, cur_index = 0;
+
+	rc = of_phandle_iterator_init(&it, np, list_name, cells_name, 0);
+	if (rc)
+		return rc;
+
+	while ((rc = of_phandle_iterator_next(&it)) == 0)
+		cur_index += 1;
+
+	if (rc != -ENOENT)
+		return rc;
+
+	return cur_index;
 }
 EXPORT_SYMBOL(of_count_phandle_with_args);
 
-- 
1.9.1

[PATCH 4/6] of: Introduce of_for_each_phandle() helper macro

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

With this macro any user can easily iterate over a list of
phandles. The patch also converts __of_parse_phandle_with_args()
to make use of the macro.

The of_count_phandle_with_args() function is not converted,
because the macro hides the return value of of_phandle_iterator_init(),
which is needed in there.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
Acked-by: Rob Herring <robh@kernel.org>
---
 drivers/of/base.c  | 7 +------
 include/linux/of.h | 6 ++++++
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index fcff2b6..ea5a13d 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1543,13 +1543,8 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 	struct of_phandle_iterator it;
 	int rc, cur_index = 0;
 
-	rc = of_phandle_iterator_init(&it, np, list_name,
-				      cells_name, cell_count);
-	if (rc)
-		return rc;
-
 	/* Loop over the phandles until all the requested entry is found */
-	while ((rc = of_phandle_iterator_next(&it)) == 0) {
+	of_for_each_phandle(&it, rc, np, list_name, cells_name, cell_count) {
 		/*
 		 * All of the error cases bail out of the loop, so at
 		 * this point, the parsing is successful. If the requested
diff --git a/include/linux/of.h b/include/linux/of.h
index 1f5e108..b0b8071 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -908,6 +908,12 @@ static inline int of_property_read_s32(const struct device_node *np,
 	return of_property_read_u32(np, propname, (u32*) out_value);
 }
 
+#define of_for_each_phandle(it, err, np, ln, cn, cc)			\
+	for (of_phandle_iterator_init((it), (np), (ln), (cn), (cc)),	\
+	     err = of_phandle_iterator_next(it);			\
+	     err == 0;							\
+	     err = of_phandle_iterator_next(it))
+
 #define of_property_for_each_u32(np, propname, prop, p, u)	\
 	for (prop = of_find_property(np, propname, NULL),	\
 		p = of_prop_next_u32(prop, NULL, &u);		\
-- 
1.9.1

[PATCH 5/6] of: Introduce of_phandle_iterator_args()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This helper function can be used to copy the arguments of a
phandle to an array.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
Acked-by: Rob Herring <robh@kernel.org>
---
 drivers/of/base.c  | 29 +++++++++++++++++++++++------
 include/linux/of.h | 10 ++++++++++
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ea5a13d..e87e21d 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1534,6 +1534,23 @@ err:
 	return -EINVAL;
 }
 
+int of_phandle_iterator_args(struct of_phandle_iterator *it,
+			     uint32_t *args,
+			     int size)
+{
+	int i, count;
+
+	count = it->cur_count;
+
+	if (WARN_ON(size < count))
+		count = size;
+
+	for (i = 0; i < count; i++)
+		args[i] = be32_to_cpup(it->cur++);
+
+	return count;
+}
+
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name,
@@ -1557,13 +1574,13 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 				goto err;
 
 			if (out_args) {
-				int i;
-				if (WARN_ON(it.cur_count > MAX_PHANDLE_ARGS))
-					it.cur_count = MAX_PHANDLE_ARGS;
+				int c;
+
+				c = of_phandle_iterator_args(&it,
+							     out_args->args,
+							     MAX_PHANDLE_ARGS);
 				out_args->np = it.node;
-				out_args->args_count = it.cur_count;
-				for (i = 0; i < it.cur_count; i++)
-					out_args->args[i] = be32_to_cpup(it.cur++);
+				out_args->args_count = c;
 			} else {
 				of_node_put(it.node);
 			}
diff --git a/include/linux/of.h b/include/linux/of.h
index b0b8071..71e1c35 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -359,6 +359,9 @@ extern int of_phandle_iterator_init(struct of_phandle_iterator *it,
 				    int cell_count);
 
 extern int of_phandle_iterator_next(struct of_phandle_iterator *it);
+extern int of_phandle_iterator_args(struct of_phandle_iterator *it,
+				    uint32_t *args,
+				    int size);
 
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
@@ -648,6 +651,13 @@ static inline int of_phandle_iterator_next(struct of_phandle_iterator *it)
 	return -ENOSYS;
 }
 
+static inline int of_phandle_iterator_args(struct of_phandle_iterator *it,
+					   uint32_t *args,
+					   int size)
+{
+	return 0;
+}
+
 static inline int of_alias_get_id(struct device_node *np, const char *stem)
 {
 	return -ENOSYS;
-- 
1.9.1

[PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Remove the usage of of_parse_phandle_with_args() and replace
it by the phandle-iterator implementation so that we can
parse out all of the potentially present 128 stream-ids.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/arm-smmu.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 2409e3b..a1c965c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -49,7 +49,7 @@
 #include "io-pgtable.h"
 
 /* Maximum number of stream IDs assigned to a single device */
-#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
+#define MAX_MASTER_STREAMIDS		128
 
 /* Maximum number of context banks per SMMU */
 #define ARM_SMMU_MAX_CBS		128
@@ -357,6 +357,12 @@ struct arm_smmu_domain {
 	struct iommu_domain		domain;
 };
 
+struct arm_smmu_phandle_args {
+	struct device_node *np;
+	int args_count;
+	uint32_t args[MAX_MASTER_STREAMIDS];
+};
+
 static struct iommu_ops arm_smmu_ops;
 
 static DEFINE_SPINLOCK(arm_smmu_devices_lock);
@@ -466,7 +472,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
 
 static int register_smmu_master(struct arm_smmu_device *smmu,
 				struct device *dev,
-				struct of_phandle_args *masterspec)
+				struct arm_smmu_phandle_args *masterspec)
 {
 	int i;
 	struct arm_smmu_master *master;
@@ -1737,7 +1743,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	struct arm_smmu_device *smmu;
 	struct device *dev = &pdev->dev;
 	struct rb_node *node;
-	struct of_phandle_args masterspec;
+	struct of_phandle_iterator it;
+	struct arm_smmu_phandle_args *masterspec;
 	int num_irqs, i, err;
 
 	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
@@ -1798,20 +1805,35 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 
 	i = 0;
 	smmu->masters = RB_ROOT;
-	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
-					   "#stream-id-cells", i,
-					   &masterspec)) {
-		err = register_smmu_master(smmu, dev, &masterspec);
+
+	err = -ENOMEM;
+	/* No need to zero the memory for masterspec */
+	masterspec = kmalloc(sizeof(*masterspec), GFP_KERNEL);
+	if (!masterspec)
+		goto out_put_masters;
+
+	of_for_each_phandle(&it, err, dev->of_node,
+			    "mmu-masters", "#stream-id-cells", 0) {
+		int count = of_phandle_iterator_args(&it, masterspec->args,
+						     MAX_MASTER_STREAMIDS);
+		masterspec->np		= of_node_get(it.node);
+		masterspec->args_count	= count;
+
+		err = register_smmu_master(smmu, dev, masterspec);
 		if (err) {
 			dev_err(dev, "failed to add master %s\n",
-				masterspec.np->name);
+				masterspec->np->name);
+			kfree(masterspec);
 			goto out_put_masters;
 		}
 
 		i++;
 	}
+
 	dev_notice(dev, "registered %d master devices\n", i);
 
+	kfree(masterspec);
+
 	parse_driver_options(smmu);
 
 	if (smmu->version > ARM_SMMU_V1 &&
-- 
1.9.1

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Will Deacon]

Hi Joerg,

On Mon, Apr 04, 2016 at 05:49:22PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Remove the usage of of_parse_phandle_with_args() and replace
> it by the phandle-iterator implementation so that we can
> parse out all of the potentially present 128 stream-ids.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/arm-smmu.c | 38 ++++++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 8 deletions(-)

Looks good to me. Given that you probably want to keep this with the
related DT changes:

Acked-by: Will Deacon <will.deacon@arm.com>

Thanks,

Will

Re: [PATCH 0/6 v2] of: Implement iterator for phandles

[Rob Herring]

On Mon, Apr 4, 2016 at 10:49 AM, Joerg Roedel <joro@8bytes.org> wrote:
> Hi,
>
> here is a new version of the implementation of the iterator
> over phandles concept which Rob Herring suggested to me some
> time ago. My approach is a little bit different from what
> the diff showed back then, but it gets rid of the allocation
> and 'struct of_phandle_args' misuse.
>
> I also converted the arm-smmu driver to make use of the
> iterator. The resulting kernel boots on my AMD Seattle
> system and fixes the warning triggered there. The patches
> now also pass all dt-unittests in my kvm environment.
>
> Patches are based on v4.6-rc2

Series now applied.

Rob

[PATCH 0/6] of: Implement iterator for phandles

[Joerg Roedel]

Hi,

here is an implementation of the iterator over phandles
concept which Rob Herring suggested to me some time ago. My
approach is a little bit different from what the diff showed
back then, but it gets rid of the allocation and 'struct
'struct of_phandle_args' misuse.

I also converted the arm-smmu driver to make use of the
iterator. The resulting kernel boots on my AMD Seattle
system and fixes the warning triggered there. The patches
now also pass all dt-unittests in my kvm environment.

Please review. Patches are based on v4.5.

Thanks,

	Joerg

Changes since RFC post:

* Reordered members of 'struct of_phandle_iterator' and did
  some renaming

* Removed index counting from the iterator

* Split up iterator implementation into multiple patches

* Fixed the code to survive all dt-unittests, tested with each
  patch in this series

* Re-added and updated some comments which got lost during the
  conversion.

* Added of_for_each_phandle macro for easier handling

* Moved the counting special-case from __of_parse_phandle_with_args
  directly to of_count_phandle_with_args for code
  simplification

* Removed some iterator helper functions

* Formatting and style changes

Joerg Roedel (6):
  of: Introduce struct of_phandle_iterator
  of: Move phandle walking to of_phandle_iterator_next()
  of: Remove counting special case from __of_parse_phandle_with_args()
  of: Introduce of_for_each_phandle() helper macro
  of: Introduce of_phandle_iterator_args()
  iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

 drivers/iommu/arm-smmu.c |  28 +++++--
 drivers/of/base.c        | 206 ++++++++++++++++++++++++++++++-----------------
 include/linux/of.h       |  56 +++++++++++++
 3 files changed, 211 insertions(+), 79 deletions(-)

-- 
1.9.1

[PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Remove the usage of of_parse_phandle_with_args() and replace
it by the phandle-iterator implementation so that we can
parse out all of the potentially present 128 stream-ids.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 59ee4b8..413bd64 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -48,7 +48,7 @@
 #include "io-pgtable.h"
 
 /* Maximum number of stream IDs assigned to a single device */
-#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
+#define MAX_MASTER_STREAMIDS		128
 
 /* Maximum number of context banks per SMMU */
 #define ARM_SMMU_MAX_CBS		128
@@ -349,6 +349,12 @@ struct arm_smmu_domain {
 	struct iommu_domain		domain;
 };
 
+struct arm_smmu_phandle_args {
+	struct device_node *np;
+	int args_count;
+	uint32_t args[MAX_MASTER_STREAMIDS];
+};
+
 static struct iommu_ops arm_smmu_ops;
 
 static DEFINE_SPINLOCK(arm_smmu_devices_lock);
@@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
 
 static int register_smmu_master(struct arm_smmu_device *smmu,
 				struct device *dev,
-				struct of_phandle_args *masterspec)
+				struct arm_smmu_phandle_args *masterspec)
 {
 	int i;
 	struct arm_smmu_master *master;
@@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	struct arm_smmu_device *smmu;
 	struct device *dev = &pdev->dev;
 	struct rb_node *node;
-	struct of_phandle_args masterspec;
+	struct of_phandle_iterator it;
+	struct arm_smmu_phandle_args masterspec;
 	int num_irqs, i, err;
 
 	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
@@ -1777,9 +1784,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 
 	i = 0;
 	smmu->masters = RB_ROOT;
-	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
-					   "#stream-id-cells", i,
-					   &masterspec)) {
+
+	of_for_each_phandle(&it, err, dev->of_node,
+			    "mmu-masters", "#stream-id-cells", 0) {
+		int count = of_phandle_iterator_args(&it, masterspec.args,
+						     MAX_MASTER_STREAMIDS);
+		masterspec.np		= of_node_get(it.node);
+		masterspec.args_count	= count;
+
 		err = register_smmu_master(smmu, dev, &masterspec);
 		if (err) {
 			dev_err(dev, "failed to add master %s\n",
@@ -1789,6 +1801,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 
 		i++;
 	}
+
+	if (i == 0)
+		goto out_put_masters;
+
 	dev_notice(dev, "registered %d master devices\n", i);
 
 	parse_driver_options(smmu);
-- 
1.9.1

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Rob Herring]

On Tue, Mar 22, 2016 at 12:58 PM, Joerg Roedel <joro@8bytes.org> wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> Remove the usage of of_parse_phandle_with_args() and replace
> it by the phandle-iterator implementation so that we can
> parse out all of the potentially present 128 stream-ids.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 59ee4b8..413bd64 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -48,7 +48,7 @@
>  #include "io-pgtable.h"
>
>  /* Maximum number of stream IDs assigned to a single device */
> -#define MAX_MASTER_STREAMIDS           MAX_PHANDLE_ARGS
> +#define MAX_MASTER_STREAMIDS           128
>
>  /* Maximum number of context banks per SMMU */
>  #define ARM_SMMU_MAX_CBS               128
> @@ -349,6 +349,12 @@ struct arm_smmu_domain {
>         struct iommu_domain             domain;
>  };
>
> +struct arm_smmu_phandle_args {
> +       struct device_node *np;
> +       int args_count;
> +       uint32_t args[MAX_MASTER_STREAMIDS];
> +};
> +
>  static struct iommu_ops arm_smmu_ops;
>
>  static DEFINE_SPINLOCK(arm_smmu_devices_lock);
> @@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
>
>  static int register_smmu_master(struct arm_smmu_device *smmu,
>                                 struct device *dev,
> -                               struct of_phandle_args *masterspec)
> +                               struct arm_smmu_phandle_args *masterspec)
>  {
>         int i;
>         struct arm_smmu_master *master;
> @@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>         struct arm_smmu_device *smmu;
>         struct device *dev = &pdev->dev;
>         struct rb_node *node;
> -       struct of_phandle_args masterspec;
> +       struct of_phandle_iterator it;
> +       struct arm_smmu_phandle_args masterspec;

Isn't this a bit big to put on the stack being ~512 bytes?

>         int num_irqs, i, err;
>
>         smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Robin Murphy]

Hi Joerg,

On 22/03/16 17:58, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> Remove the usage of of_parse_phandle_with_args() and replace
> it by the phandle-iterator implementation so that we can
> parse out all of the potentially present 128 stream-ids.

In a stream-matching implementation, a device may quite legitimately own 
anything up to _all_ of the stream IDs (32768, or 65536 if we ever 
implement support for the SMMUv2 EXID extension), so this is only a 
genuine limit for stream indexing (and if anyone ever actually made one 
of those, I don't think they're running mainline on it).

Alternatively, how straightforward is it to change the DT on your 
machine? I'll be getting a v2 of [1] out in a couple of weeks (after 
imminent holidays), which already gets rid of MAX_MASTER_STREAMIDS 
altogether, and might also have grown proper SMR support by then.

Robin.

[1]:http://thread.gmane.org/gmane.linux.kernel.iommu/12454

> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 59ee4b8..413bd64 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -48,7 +48,7 @@
>   #include "io-pgtable.h"
>
>   /* Maximum number of stream IDs assigned to a single device */
> -#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
> +#define MAX_MASTER_STREAMIDS		128
>
>   /* Maximum number of context banks per SMMU */
>   #define ARM_SMMU_MAX_CBS		128
> @@ -349,6 +349,12 @@ struct arm_smmu_domain {
>   	struct iommu_domain		domain;
>   };
>
> +struct arm_smmu_phandle_args {
> +	struct device_node *np;
> +	int args_count;
> +	uint32_t args[MAX_MASTER_STREAMIDS];
> +};
> +
>   static struct iommu_ops arm_smmu_ops;
>
>   static DEFINE_SPINLOCK(arm_smmu_devices_lock);
> @@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
>
>   static int register_smmu_master(struct arm_smmu_device *smmu,
>   				struct device *dev,
> -				struct of_phandle_args *masterspec)
> +				struct arm_smmu_phandle_args *masterspec)
>   {
>   	int i;
>   	struct arm_smmu_master *master;
> @@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>   	struct arm_smmu_device *smmu;
>   	struct device *dev = &pdev->dev;
>   	struct rb_node *node;
> -	struct of_phandle_args masterspec;
> +	struct of_phandle_iterator it;
> +	struct arm_smmu_phandle_args masterspec;
>   	int num_irqs, i, err;
>
>   	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> @@ -1777,9 +1784,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>
>   	i = 0;
>   	smmu->masters = RB_ROOT;
> -	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
> -					   "#stream-id-cells", i,
> -					   &masterspec)) {
> +
> +	of_for_each_phandle(&it, err, dev->of_node,
> +			    "mmu-masters", "#stream-id-cells", 0) {
> +		int count = of_phandle_iterator_args(&it, masterspec.args,
> +						     MAX_MASTER_STREAMIDS);
> +		masterspec.np		= of_node_get(it.node);
> +		masterspec.args_count	= count;
> +
>   		err = register_smmu_master(smmu, dev, &masterspec);
>   		if (err) {
>   			dev_err(dev, "failed to add master %s\n",
> @@ -1789,6 +1801,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>
>   		i++;
>   	}
> +
> +	if (i == 0)
> +		goto out_put_masters;
> +
>   	dev_notice(dev, "registered %d master devices\n", i);
>
>   	parse_driver_options(smmu);
>

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Joerg Roedel]

Hi Robin,

On Tue, Mar 22, 2016 at 06:53:48PM +0000, Robin Murphy wrote:
> In a stream-matching implementation, a device may quite legitimately
> own anything up to _all_ of the stream IDs (32768, or 65536 if we
> ever implement support for the SMMUv2 EXID extension), so this is
> only a genuine limit for stream indexing (and if anyone ever
> actually made one of those, I don't think they're running mainline
> on it).

Do you mean we might see a lot more than the currently 128 supported
stream-ids for an smmu?

> Alternatively, how straightforward is it to change the DT on your
> machine? I'll be getting a v2 of [1] out in a couple of weeks (after
> imminent holidays), which already gets rid of MAX_MASTER_STREAMIDS
> altogether, and might also have grown proper SMR support by then.

Hmm, I don't know how to change the DT of this Seattle machine. I think
it is provided by the ACPI BIOS. At least it boots with grub2 and not
u-boot and there is no DT in /boot.


	Joerg

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Will Deacon]

On Wed, Mar 23, 2016 at 12:51:28PM +0100, Joerg Roedel wrote:
> On Tue, Mar 22, 2016 at 06:53:48PM +0000, Robin Murphy wrote:
> > In a stream-matching implementation, a device may quite legitimately
> > own anything up to _all_ of the stream IDs (32768, or 65536 if we
> > ever implement support for the SMMUv2 EXID extension), so this is
> > only a genuine limit for stream indexing (and if anyone ever
> > actually made one of those, I don't think they're running mainline
> > on it).
> 
> Do you mean we might see a lot more than the currently 128 supported
> stream-ids for an smmu?

We might, but this patch is still an improvement for now.

Will

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Will Deacon]

Hi Joerg,

On Tue, Mar 22, 2016 at 06:58:29PM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Remove the usage of of_parse_phandle_with_args() and replace
> it by the phandle-iterator implementation so that we can
> parse out all of the potentially present 128 stream-ids.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 59ee4b8..413bd64 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -48,7 +48,7 @@
>  #include "io-pgtable.h"
>  
>  /* Maximum number of stream IDs assigned to a single device */
> -#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
> +#define MAX_MASTER_STREAMIDS		128
>  
>  /* Maximum number of context banks per SMMU */
>  #define ARM_SMMU_MAX_CBS		128
> @@ -349,6 +349,12 @@ struct arm_smmu_domain {
>  	struct iommu_domain		domain;
>  };
>  
> +struct arm_smmu_phandle_args {
> +	struct device_node *np;
> +	int args_count;
> +	uint32_t args[MAX_MASTER_STREAMIDS];
> +};
> +
>  static struct iommu_ops arm_smmu_ops;
>  
>  static DEFINE_SPINLOCK(arm_smmu_devices_lock);
> @@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
>  
>  static int register_smmu_master(struct arm_smmu_device *smmu,
>  				struct device *dev,
> -				struct of_phandle_args *masterspec)
> +				struct arm_smmu_phandle_args *masterspec)
>  {
>  	int i;
>  	struct arm_smmu_master *master;
> @@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>  	struct arm_smmu_device *smmu;
>  	struct device *dev = &pdev->dev;
>  	struct rb_node *node;
> -	struct of_phandle_args masterspec;
> +	struct of_phandle_iterator it;
> +	struct arm_smmu_phandle_args masterspec;
>  	int num_irqs, i, err;
>  
>  	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> @@ -1777,9 +1784,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>  
>  	i = 0;
>  	smmu->masters = RB_ROOT;
> -	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
> -					   "#stream-id-cells", i,
> -					   &masterspec)) {
> +
> +	of_for_each_phandle(&it, err, dev->of_node,
> +			    "mmu-masters", "#stream-id-cells", 0) {
> +		int count = of_phandle_iterator_args(&it, masterspec.args,
> +						     MAX_MASTER_STREAMIDS);
> +		masterspec.np		= of_node_get(it.node);
> +		masterspec.args_count	= count;
> +
>  		err = register_smmu_master(smmu, dev, &masterspec);
>  		if (err) {
>  			dev_err(dev, "failed to add master %s\n",
> @@ -1789,6 +1801,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>  
>  		i++;
>  	}
> +
> +	if (i == 0)
> +		goto out_put_masters;

I'm confused by this hunk. If i == 0, then we shouldn't have registered
any masters and therefore out_put_masters won't have anything to do.

In fact, I'm not completely clear on how the of_node refcounting interacts
with your iterators. Does the iterator put the node after you call the
"next" function, or does it increment each thing exactly once?

Will

Re: [PATCH 6/6] iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

[Joerg Roedel]

Hi Will,

On Tue, Mar 29, 2016 at 06:22:16PM +0100, Will Deacon wrote:
> > +
> > +	if (i == 0)
> > +		goto out_put_masters;
> 
> I'm confused by this hunk. If i == 0, then we shouldn't have registered
> any masters and therefore out_put_masters won't have anything to do.

The idea was that there is nothing more to do in the function when it
didn't find any masters and so it can safely skip the rest of the
function.

But the original code doesn't do this either, so it certainly doesn't
belong into this patch. I remove it for the next post.

> In fact, I'm not completely clear on how the of_node refcounting interacts
> with your iterators. Does the iterator put the node after you call the
> "next" function, or does it increment each thing exactly once?

The iterator will put the current node at the following _next call, so
when you want to use each node, you need your own reference.

It works like the pci_dev iterators, so if you break out of the loop you
have to manually put the last node it returned.


	Joerg