[RFC PATCH 0/2] of: Implement iterator for phandles

[RFC PATCH 0/2] 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.

There is still some work to to on this change, but it would
be cool to get some early feedback on the code and the
direction it takes. Here is also my todo-list for this
before next submission:

TODO:

* Reorder members of 'struct of_phandle_iterator' and do
  some renaming and documentation of them.

* Split up first patch to make it easier to review and
  bisect.

* Re-add and update some comments which got lost during the
  conversion.

Any feedback is appreciated.

Thanks,

	Joerg

Joerg Roedel (2):
  of: Implement iterator for phandles
  iommu/arm-smmu: Make use of phandle iterators in device-tree parsing

 drivers/iommu/arm-smmu.c |  27 ++++--
 drivers/of/base.c        | 219 +++++++++++++++++++++++++++--------------------
 include/linux/of.h       |  95 ++++++++++++++++++++
 3 files changed, 241 insertions(+), 100 deletions(-)

-- 
1.9.1

[PATCH 1/2] of: Implement iterator for phandles

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Getting the arguments of phandles is somewhat limited at the
moement, because the number of arguments supported by core
code is limited to MAX_PHANDLE_ARGS, which is set to 16
currently.

In case of the arm smmu this is not enough, as the 128
supported stream-ids are encoded in device-tree with
arguments to phandles. On some systems with more than 16
stream-ids this causes a WARN_ON being triggered at boot and
an incomplete initialization of the smmu.

This patch-set implements an iterator interface over
phandles which allows to parse out arbitrary numbers of
arguments per phandle. The existing function for parsing
them, __of_parse_phandle_with_args(), is converted to make
use of the iterator.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/of/base.c  | 219 ++++++++++++++++++++++++++++++-----------------------
 include/linux/of.h |  95 +++++++++++++++++++++++
 2 files changed, 220 insertions(+), 94 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 017dd94..9a5f199 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1439,119 +1439,150 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
 	printk("\n");
 }
 
-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)
+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, *list_end;
-	int rc = 0, size, cur_index = 0;
-	uint32_t count = 0;
-	struct device_node *node = NULL;
-	phandle phandle;
+	const __be32 *list;
+	int size;
 
 	/* Retrieve the phandle list property */
 	list = of_get_property(np, list_name, &size);
 	if (!list)
 		return -ENOENT;
-	list_end = list + size / sizeof(*list);
 
-	/* Loop over the phandles until all the requested entry is found */
-	while (list < list_end) {
-		rc = -EINVAL;
-		count = 0;
+	it->np		= np;
+	it->node	= NULL;
+	it->list	= list;
+	it->phandle_end	= list;
+	it->list_end	= list + size / sizeof(*list);
+	it->cells_name	= cells_name;
+	it->cell_count	= cell_count;
+	it->cur_index	= -1;
 
-		/*
-		 * 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) {
-			/*
-			 * 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 (cells_name || cur_index == index) {
-				node = of_find_node_by_phandle(phandle);
-				if (!node) {
-					pr_err("%s: could not find phandle\n",
-						np->full_name);
-					goto err;
-				}
-			}
+	return 0;
+}
 
-			if (cells_name) {
-				if (of_property_read_u32(node, cells_name,
-							 &count)) {
-					pr_err("%s: could not get %s for %s\n",
-						np->full_name, cells_name,
-						node->full_name);
-					goto err;
-				}
-			} else {
-				count = cell_count;
-			}
+int of_phandle_iterator_next(struct of_phandle_iterator *it)
+{
+	phandle phandle = 0;
+	uint32_t count = 0;
 
-			/*
-			 * Make sure that the arguments actually fit in the
-			 * remaining property data length
-			 */
-			if (list + count > list_end) {
-				pr_err("%s: arguments longer than property\n",
-					 np->full_name);
-				goto err;
-			}
+	if (it->phandle_end >= it->list_end)
+		return -ENOENT;
+
+	if (it->node) {
+		of_node_put(it->node);
+		it->node = NULL;
+	}
+
+	it->list	 = it->phandle_end;
+	it->cur_index	+= 1;
+	phandle		 = be32_to_cpup(it->list++);
+
+	if (phandle) {
+		if (it->cells_name) {
+			it->node = of_find_node_by_phandle(phandle);
+			if (!it->node)
+				goto no_node;
+
+			if (of_property_read_u32(it->node,
+						 it->cells_name,
+						 &count))
+				goto no_property;
+		} else {
+			count = it->cell_count;
 		}
 
-		/*
-		 * All of the error cases above 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.
-		 */
+		if (it->list + count > it->list_end)
+			goto too_many_args;
+	}
+
+	it->phandle_end = it->list + count;
+	it->cur_count   = count;
+	it->phandle	= phandle;
+
+	return 0;
+
+no_node:
+	pr_err("%s: could not find phandle\n", it->np->full_name);
+
+	return -EINVAL;
+
+no_property:
+	pr_err("%s: could not get %s for %s\n", it->np->full_name,
+	       it->cells_name, it->node->full_name);
+
+	return -EINVAL;
+
+too_many_args:
+	pr_err("%s: arguments longer than property\n", it->np->full_name);
+
+	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->list++);
+
+	return count;
+}
+
+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)
+{
+	struct of_phandle_iterator it;
+	int rc;
+
+	rc = of_phandle_iterator_init(&it, np, list_name,
+				      cells_name, cell_count);
+	if (rc)
+		return rc;
+
+	while ((rc = of_phandle_iterator_next(&it)) == 0) {
+		uint32_t count;
+		int cur_index;
+
+		count     = of_phandle_iterator_count(&it);
+		cur_index = of_phandle_iterator_index(&it);
+
 		rc = -ENOENT;
 		if (cur_index == index) {
-			if (!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++);
-			} else {
-				of_node_put(node);
-			}
+			if (!of_phandle_iterator_phandle(&it))
+				break;
+
+			rc = 0;
 
 			/* Found it! return success */
-			return 0;
-		}
+			if (!out_args)
+				break;
 
-		of_node_put(node);
-		node = NULL;
-		list += count;
-		cur_index++;
+			out_args->np	     = of_phandle_iterator_node(&it);
+			out_args->args_count = of_phandle_iterator_args(&it,
+									out_args->args,
+									MAX_PHANDLE_ARGS);
+
+			break;
+		}
 	}
 
-	/*
-	 * 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)
-	 */
-	rc = index < 0 ? cur_index : -ENOENT;
- err:
-	if (node)
-		of_node_put(node);
+	of_phandle_iterator_destroy(&it);
+
 	return rc;
 }
 
diff --git a/include/linux/of.h b/include/linux/of.h
index dc6e396..31e6ee9 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -75,6 +75,19 @@ struct of_phandle_args {
 	uint32_t args[MAX_PHANDLE_ARGS];
 };
 
+struct of_phandle_iterator {
+	const struct device_node *np;
+	const __be32 *list;
+	const __be32 *list_end;
+	const __be32 *phandle_end;
+	phandle phandle;
+	struct device_node *node;
+	const char *cells_name;
+	int cell_count;
+	int cur_index;
+	uint32_t cur_count;
+};
+
 struct of_reconfig_data {
 	struct device_node	*dn;
 	struct property		*prop;
@@ -334,6 +347,50 @@ 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);
+
+static inline void of_phandle_iterator_destroy(struct of_phandle_iterator *it)
+{
+	if (it->node)
+		of_node_put(it->node);
+}
+
+extern int of_phandle_iterator_next(struct of_phandle_iterator *it);
+
+static inline int of_phandle_iterator_index(const struct of_phandle_iterator *it)
+{
+	return it->cur_index;
+}
+
+static inline uint32_t of_phandle_iterator_count(const struct of_phandle_iterator *it)
+{
+	return it->cur_count;
+}
+
+static inline uint32_t of_phandle_iterator_phandle(const struct of_phandle_iterator *it)
+{
+	return it->phandle;
+}
+
+static inline struct device_node *of_phandle_iterator_node(struct of_phandle_iterator *it)
+{
+	if (!it->node)
+		it->node = of_find_node_by_phandle(it->phandle);
+
+	if (it->node)
+		of_node_get(it->node);
+
+	return it->node;
+}
+
+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);
 extern int of_alias_get_highest_id(const char *stem);
@@ -608,6 +665,44 @@ 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 void of_phandle_iterator_destroy(struct of_phandle_iterator *it)
+{
+}
+
+static inline int of_phandle_iterator_next(struct of_phandle_iterator *it)
+{
+	return -ENOSYS;
+}
+
+static inline int of_phandle_iterator_index(const struct of_phandle_iterator *it)
+{
+	return -1;
+}
+
+static inline uint32_t of_phandle_iterator_count(const struct of_phandle_iterator *it)
+{
+	return 0;
+}
+
+static inline uint32_t of_phandle_iterator_phandle(const struct of_phandle_iterator *it)
+{
+	return 0;
+}
+
+static inline struct device_node *of_phandle_iterator_node(struct of_phandle_iterator *it)
+{
+	return NULL;
+}
+
 static inline int of_alias_get_id(struct device_node *np, const char *stem)
 {
 	return -ENOSYS;
-- 
1.9.1

Re: [PATCH 1/2] of: Implement iterator for phandles

[Rob Herring]

On Wed, Mar 16, 2016 at 11:42 AM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
>
> Getting the arguments of phandles is somewhat limited at the
> moement, because the number of arguments supported by core
> code is limited to MAX_PHANDLE_ARGS, which is set to 16
> currently.
>
> In case of the arm smmu this is not enough, as the 128
> supported stream-ids are encoded in device-tree with
> arguments to phandles. On some systems with more than 16
> stream-ids this causes a WARN_ON being triggered at boot and
> an incomplete initialization of the smmu.
>
> This patch-set implements an iterator interface over
> phandles which allows to parse out arbitrary numbers of
> arguments per phandle. The existing function for parsing
> them, __of_parse_phandle_with_args(), is converted to make
> use of the iterator.

This mostly looks fine to me, but it is kind of a lot of functions
just for this one thing. For example, I think the caller can track the
index themselves if they care about it. I'd also like to see a more
standard style for_each type iterator define rather than open coded
while loops.

Some minor style comments below.

> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/of/base.c  | 219 ++++++++++++++++++++++++++++++-----------------------
>  include/linux/of.h |  95 +++++++++++++++++++++++
>  2 files changed, 220 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 017dd94..9a5f199 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1439,119 +1439,150 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
>         printk("\n");
>  }
>
> -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)
> +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, *list_end;
> -       int rc = 0, size, cur_index = 0;
> -       uint32_t count = 0;
> -       struct device_node *node = NULL;
> -       phandle phandle;
> +       const __be32 *list;
> +       int size;
>
>         /* Retrieve the phandle list property */
>         list = of_get_property(np, list_name, &size);
>         if (!list)
>                 return -ENOENT;
> -       list_end = list + size / sizeof(*list);
>
> -       /* Loop over the phandles until all the requested entry is found */
> -       while (list < list_end) {
> -               rc = -EINVAL;
> -               count = 0;
> +       it->np          = np;
> +       it->node        = NULL;
> +       it->list        = list;
> +       it->phandle_end = list;
> +       it->list_end    = list + size / sizeof(*list);
> +       it->cells_name  = cells_name;
> +       it->cell_count  = cell_count;
> +       it->cur_index   = -1;
>
> -               /*
> -                * 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) {
> -                       /*
> -                        * 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 (cells_name || cur_index == index) {
> -                               node = of_find_node_by_phandle(phandle);
> -                               if (!node) {
> -                                       pr_err("%s: could not find phandle\n",
> -                                               np->full_name);
> -                                       goto err;
> -                               }
> -                       }
> +       return 0;
> +}
>
> -                       if (cells_name) {
> -                               if (of_property_read_u32(node, cells_name,
> -                                                        &count)) {
> -                                       pr_err("%s: could not get %s for %s\n",
> -                                               np->full_name, cells_name,
> -                                               node->full_name);
> -                                       goto err;
> -                               }
> -                       } else {
> -                               count = cell_count;
> -                       }
> +int of_phandle_iterator_next(struct of_phandle_iterator *it)
> +{
> +       phandle phandle = 0;
> +       uint32_t count = 0;
>
> -                       /*
> -                        * Make sure that the arguments actually fit in the
> -                        * remaining property data length
> -                        */
> -                       if (list + count > list_end) {
> -                               pr_err("%s: arguments longer than property\n",
> -                                        np->full_name);
> -                               goto err;
> -                       }
> +       if (it->phandle_end >= it->list_end)
> +               return -ENOENT;
> +
> +       if (it->node) {
> +               of_node_put(it->node);
> +               it->node = NULL;
> +       }
> +
> +       it->list         = it->phandle_end;
> +       it->cur_index   += 1;

it->cur_index++;

> +       phandle          = be32_to_cpup(it->list++);

Please, just <space>=<space>.

> +
> +       if (phandle) {
> +               if (it->cells_name) {
> +                       it->node = of_find_node_by_phandle(phandle);
> +                       if (!it->node)
> +                               goto no_node;

All these goto's are pretty pointless and can just be printk and return.

> +
> +                       if (of_property_read_u32(it->node,
> +                                                it->cells_name,
> +                                                &count))
> +                               goto no_property;
> +               } else {
> +                       count = it->cell_count;
>                 }
>
> -               /*
> -                * All of the error cases above 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.
> -                */
> +               if (it->list + count > it->list_end)
> +                       goto too_many_args;
> +       }
> +
> +       it->phandle_end = it->list + count;
> +       it->cur_count   = count;
> +       it->phandle     = phandle;

single space.

> +
> +       return 0;
> +
> +no_node:
> +       pr_err("%s: could not find phandle\n", it->np->full_name);
> +
> +       return -EINVAL;
> +
> +no_property:
> +       pr_err("%s: could not get %s for %s\n", it->np->full_name,
> +              it->cells_name, it->node->full_name);
> +
> +       return -EINVAL;
> +
> +too_many_args:
> +       pr_err("%s: arguments longer than property\n", it->np->full_name);
> +
> +       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->list++);
> +
> +       return count;
> +}
> +
> +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)
> +{
> +       struct of_phandle_iterator it;
> +       int rc;
> +
> +       rc = of_phandle_iterator_init(&it, np, list_name,
> +                                     cells_name, cell_count);
> +       if (rc)
> +               return rc;
> +
> +       while ((rc = of_phandle_iterator_next(&it)) == 0) {
> +               uint32_t count;
> +               int cur_index;
> +
> +               count     = of_phandle_iterator_count(&it);
> +               cur_index = of_phandle_iterator_index(&it);
> +
>                 rc = -ENOENT;
>                 if (cur_index == index) {
> -                       if (!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++);
> -                       } else {
> -                               of_node_put(node);
> -                       }
> +                       if (!of_phandle_iterator_phandle(&it))
> +                               break;
> +
> +                       rc = 0;
>
>                         /* Found it! return success */
> -                       return 0;
> -               }
> +                       if (!out_args)
> +                               break;
>
> -               of_node_put(node);
> -               node = NULL;
> -               list += count;
> -               cur_index++;
> +                       out_args->np         = of_phandle_iterator_node(&it);
> +                       out_args->args_count = of_phandle_iterator_args(&it,
> +                                                                       out_args->args,
> +                                                                       MAX_PHANDLE_ARGS);
> +
> +                       break;
> +               }
>         }
>
> -       /*
> -        * 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)
> -        */
> -       rc = index < 0 ? cur_index : -ENOENT;
> - err:
> -       if (node)
> -               of_node_put(node);
> +       of_phandle_iterator_destroy(&it);
> +
>         return rc;
>  }
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index dc6e396..31e6ee9 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -75,6 +75,19 @@ struct of_phandle_args {
>         uint32_t args[MAX_PHANDLE_ARGS];
>  };
>
> +struct of_phandle_iterator {
> +       const struct device_node *np;
> +       const __be32 *list;
> +       const __be32 *list_end;
> +       const __be32 *phandle_end;
> +       phandle phandle;
> +       struct device_node *node;

np and node? If you need both, name them based on what they point to.

> +       const char *cells_name;
> +       int cell_count;
> +       int cur_index;
> +       uint32_t cur_count;
> +};
> +
>  struct of_reconfig_data {
>         struct device_node      *dn;
>         struct property         *prop;
> @@ -334,6 +347,50 @@ 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);
> +
> +static inline void of_phandle_iterator_destroy(struct of_phandle_iterator *it)
> +{
> +       if (it->node)
> +               of_node_put(it->node);
> +}
> +
> +extern int of_phandle_iterator_next(struct of_phandle_iterator *it);
> +
> +static inline int of_phandle_iterator_index(const struct of_phandle_iterator *it)
> +{
> +       return it->cur_index;
> +}
> +
> +static inline uint32_t of_phandle_iterator_count(const struct of_phandle_iterator *it)
> +{
> +       return it->cur_count;
> +}
> +
> +static inline uint32_t of_phandle_iterator_phandle(const struct of_phandle_iterator *it)
> +{
> +       return it->phandle;
> +}
> +
> +static inline struct device_node *of_phandle_iterator_node(struct of_phandle_iterator *it)
> +{
> +       if (!it->node)
> +               it->node = of_find_node_by_phandle(it->phandle);
> +
> +       if (it->node)
> +               of_node_get(it->node);

The above function may have already done the get. Not sure offhand.

> +
> +       return it->node;
> +}
> +
> +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);
>  extern int of_alias_get_highest_id(const char *stem);
> @@ -608,6 +665,44 @@ 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 void of_phandle_iterator_destroy(struct of_phandle_iterator *it)
> +{
> +}
> +
> +static inline int of_phandle_iterator_next(struct of_phandle_iterator *it)
> +{
> +       return -ENOSYS;
> +}
> +
> +static inline int of_phandle_iterator_index(const struct of_phandle_iterator *it)
> +{
> +       return -1;
> +}
> +
> +static inline uint32_t of_phandle_iterator_count(const struct of_phandle_iterator *it)
> +{
> +       return 0;
> +}
> +
> +static inline uint32_t of_phandle_iterator_phandle(const struct of_phandle_iterator *it)
> +{
> +       return 0;
> +}
> +
> +static inline struct device_node *of_phandle_iterator_node(struct of_phandle_iterator *it)
> +{
> +       return NULL;
> +}
> +
>  static inline int of_alias_get_id(struct device_node *np, const char *stem)
>  {
>         return -ENOSYS;
> --
> 1.9.1
>
--
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 1/2] of: Implement iterator for phandles

[Joerg Roedel]

Hi Rob,

On Fri, Mar 18, 2016 at 10:54:57AM -0500, Rob Herring wrote:
> This mostly looks fine to me, but it is kind of a lot of functions
> just for this one thing. For example, I think the caller can track the
> index themselves if they care about it. I'd also like to see a more
> standard style for_each type iterator define rather than open coded
> while loops.

Thanks for your feedback. I think I worked everything into the patches
and am about to send a new version.

> > +struct of_phandle_iterator {
> > +       const struct device_node *np;
> > +       const __be32 *list;
> > +       const __be32 *list_end;
> > +       const __be32 *phandle_end;
> > +       phandle phandle;
> > +       struct device_node *node;
> 
> np and node? If you need both, name them based on what they point to.

Yes, 'np' is the parent node, while 'node' is the current node the
iterator points to. The parent node is needed for the error messages
later.

> > +static inline struct device_node *of_phandle_iterator_node(struct of_phandle_iterator *it)
> > +{
> > +       if (!it->node)
> > +               it->node = of_find_node_by_phandle(it->phandle);
> > +
> > +       if (it->node)
> > +               of_node_get(it->node);
> 
> The above function may have already done the get. Not sure offhand.

Yes, it does. But when the iterator moves on the node will be put again.
So when it is returned here we get an extra reference.

But it doesn't matter anymore as I changed the code to unconditionally
get the node in of_phandle_iterator_next now.



	Joerg

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

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Remove the usage of of_parse_phandle_with_args() and replace
it by the phandle-iterator implementation to be able to
parse out all of the potentially present 128 stream-ids.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 59ee4b8..d5d6a1d 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,17 @@ 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 = of_phandle_iterator_init(&it, dev->of_node,
+				       "mmu-masters", "#stream-id-cells", 0);
+	if (err)
+		goto out_put_masters;
+
+	while (!of_phandle_iterator_next(&it)) {
+		masterspec.np		= of_phandle_iterator_node(&it);
+		masterspec.args_count	= of_phandle_iterator_args(&it,
+								   masterspec.args,
+								   MAX_MASTER_STREAMIDS);
 		err = register_smmu_master(smmu, dev, &masterspec);
 		if (err) {
 			dev_err(dev, "failed to add master %s\n",
-- 
1.9.1

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