[PATCH 0/4] of: Handle multi-parent version of msi-parent

[PATCH 0/4] of: Handle multi-parent version of msi-parent

[Marc Zyngier]

Now that we have a useable and documented version of msi-parent that
can deal with multiple parenting, we can properly handle it in the
kernel. This leads to a new OF helper, some rework in the PCI and
platform layers, as well as a last patch for the ITS driver, which is
the only thing in the kernel requirering this functionality so far.

Patches on top of 4.3-rc2.

Marc Zyngier (4):
  of: Add of_parse_phandle_with_opt_args() helper function
  of: irq: Add support for the new definition of "msi-parent"
  PCI/MSI: Add support for the new definition of "msi-parent"
  irqchip/gic-v3-its: Parse new version of msi-parent property

 drivers/irqchip/irq-gic-v3-its-platform-msi.c | 18 ++++++--
 drivers/of/base.c                             | 64 ++++++++++++++++++++++++++-
 drivers/of/irq.c                              | 26 +++++++----
 drivers/pci/of.c                              | 25 ++++++++---
 include/linux/of.h                            |  3 ++
 5 files changed, 115 insertions(+), 21 deletions(-)

-- 
2.1.4

[PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function

[Marc Zyngier]

of_parse_phandle_with_args() is slightly inflexible as it doesn't
allow the (unusual) case where the #*-cells property is not defined.
In order to support this, introduce of_parse_phandle_with_opt_args()
which assumes that #*-cells is zero when it is not defined,
as required by the msi-parent binding

This is done by turning __of_parse_phandle_with_args into an even
bigger monster, which is a bit frightening.

Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/of/base.c  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/of.h |  3 +++
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 8b5a187..1612342e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1479,6 +1479,10 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 			 * (i.e. cells_name not set, but cell_count is set),
 			 * except when we're going to return the found node
 			 * below.
+			 *
+			 * If #*-cells is not found, but cell_count is set
+			 * to a non-zero value, use (cell_count-1) as a
+			 * fallback value.
 			 */
 			if (cells_name || cur_index == index) {
 				node = of_find_node_by_phandle(phandle);
@@ -1490,13 +1494,21 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 			}
 
 			if (cells_name) {
-				if (of_property_read_u32(node, cells_name,
-							 &count)) {
+				int ret;
+				ret = of_property_read_u32(node, cells_name,
+							   &count);
+				if (ret && !cell_count) {
 					pr_err("%s: could not get %s for %s\n",
 						np->full_name, cells_name,
 						node->full_name);
 					goto err;
 				}
+				if (ret) {
+					count = cell_count - 1;
+					pr_debug("%s: could not get %s for %s, assuming %d\n",
+						 np->full_name, cells_name,
+						 node->full_name, count);
+				}
 			} else {
 				count = cell_count;
 			}
@@ -1628,6 +1640,54 @@ int of_parse_phandle_with_args(const struct device_node *np, const char *list_na
 EXPORT_SYMBOL(of_parse_phandle_with_args);
 
 /**
+ * of_parse_phandle_with_opt_args() - Find a node pointed by phandle in a list
+ * @np:		pointer to a device tree node containing a list
+ * @list_name:	property name that contains a list
+ * @cells_name:	property name that specifies phandles' arguments count
+ * @index:	index of a phandle to parse out
+ * @out_args:	optional pointer to output arguments structure (will be filled)
+ *
+ * This function is useful to parse lists of phandles and their arguments.
+ * If cells_name is not found, then it is assumed to be zero.
+ * Returns 0 on success and fills out_args, on error returns appropriate
+ * errno value.
+ *
+ * Caller is responsible to call of_node_put() on the returned out_args->np
+ * pointer.
+ *
+ * Example:
+ *
+ * phandle1: node1 {
+ *	#list-cells = <2>;
+ * }
+ *
+ * phandle2: node2 {
+ * }
+ *
+ * phandle3: node3 {
+ *	#list-cells = <1>;
+ * }
+ *
+ * node3 {
+ *	list = <&phandle1 1 2 &phandle2 &phandle 3>;
+ * }
+ *
+ * To get a device_node of the `node2' node you may call this:
+ * of_parse_phandle_with_args(node3, "list", "#list-cells", 1, &args);
+ */
+int of_parse_phandle_with_opt_args(const struct device_node *np,
+				   const char *list_name,
+				   const char *cells_name, int index,
+				   struct of_phandle_args *out_args)
+{
+	if (index < 0)
+		return -EINVAL;
+	return __of_parse_phandle_with_args(np, list_name, cells_name, 1,
+					    index, out_args);
+}
+EXPORT_SYMBOL(of_parse_phandle_with_opt_args);
+
+/**
  * of_parse_phandle_with_fixed_args() - Find a node pointed by phandle in a list
  * @np:		pointer to a device tree node containing a list
  * @list_name:	property name that contains a list
diff --git a/include/linux/of.h b/include/linux/of.h
index 2194b8c..ae6cd03 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -328,6 +328,9 @@ extern struct device_node *of_parse_phandle(const struct device_node *np,
 extern int of_parse_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name, int index,
 	struct of_phandle_args *out_args);
+extern int of_parse_phandle_with_opt_args(const struct device_node *np,
+	const char *list_name, const char *cells_name, int index,
+	struct of_phandle_args *out_args);
 extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
 	const char *list_name, int cells_count, int index,
 	struct of_phandle_args *out_args);
-- 
2.1.4

[PATCH 2/4] of: irq: Add support for the new definition of "msi-parent"

[Marc Zyngier]

Since 126b16e2ad98 ("Docs: dt: add generic MSI bindings"),
the definition of "msi-parent" has evolved, while maintaining
some degree of compatibility. It can now express multiple MSI
controllers as parents, as well as some sideband data being
communicated to the controller.

This patch revamps the parsing of the property, iterating over
the multiple parents until a suitable irqdomain is found.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/of/irq.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 55317fa..c9637cf 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -586,15 +586,23 @@ err:
  */
 void of_msi_configure(struct device *dev, struct device_node *np)
 {
-	struct device_node *msi_np;
-	struct irq_domain *d;
+	struct of_phandle_args args;
+	int index = 0;
 
-	msi_np = of_parse_phandle(np, "msi-parent", 0);
-	if (!msi_np)
-		return;
+	while (!of_parse_phandle_with_opt_args(np, "msi-parent", "#msi-cells",
+					       index, &args)) {
+		struct irq_domain *d;
 
-	d = irq_find_matching_host(msi_np, DOMAIN_BUS_PLATFORM_MSI);
-	if (!d)
-		d = irq_find_host(msi_np);
-	dev_set_msi_domain(dev, d);
+		d = irq_find_matching_host(args.np, DOMAIN_BUS_PLATFORM_MSI);
+		if (!d)
+			d = irq_find_host(args.np);
+
+		if (d) {
+			dev_set_msi_domain(dev, d);
+			return;
+		}
+
+		of_node_put(args.np);
+		index++;
+	}
 }
-- 
2.1.4

[PATCH 3/4] PCI/MSI: Add support for the new definition of "msi-parent"

[Marc Zyngier]

Since 126b16e2ad98 ("Docs: dt: add generic MSI bindings"),
the definition of "msi-parent" has evolved, while maintaining
some degree of compatibility. It can now express multiple MSI
controllers as parents, as well as some sideband data being
communicated to the controller.

This patch revamps the parsing of the property, iterating over
the multiple parents until a suitable irqdomain is found.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/pci/of.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 2e99a50..c3c6c18 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -64,27 +64,38 @@ struct device_node * __weak pcibios_get_phb_of_node(struct pci_bus *bus)
 struct irq_domain *pci_host_bridge_of_msi_domain(struct pci_bus *bus)
 {
 #ifdef CONFIG_IRQ_DOMAIN
-	struct device_node *np;
+	struct of_phandle_args args;
 	struct irq_domain *d;
+	int index = 0;
 
 	if (!bus->dev.of_node)
 		return NULL;
 
 	/* Start looking for a phandle to an MSI controller. */
-	np = of_parse_phandle(bus->dev.of_node, "msi-parent", 0);
+	while (!of_parse_phandle_with_opt_args(bus->dev.of_node,
+					       "msi-parent", "#msi-cells",
+					       index, &args)) {
+		d = irq_find_matching_host(args.np, DOMAIN_BUS_PCI_MSI);
+		if (d)
+			return d;
+
+		d = irq_find_host(args.np);
+		if (d)
+			return d;
+
+		of_node_put(args.np);
+		index++;
+	}
 
 	/*
 	 * If we don't have an msi-parent property, look for a domain
 	 * directly attached to the host bridge.
 	 */
-	if (!np)
-		np = bus->dev.of_node;
-
-	d = irq_find_matching_host(np, DOMAIN_BUS_PCI_MSI);
+	d = irq_find_matching_host(bus->dev.of_node, DOMAIN_BUS_PCI_MSI);
 	if (d)
 		return d;
 
-	return irq_find_host(np);
+	return irq_find_host(bus->dev.of_node);
 #else
 	return NULL;
 #endif
-- 
2.1.4

[PATCH 4/4] irqchip/gic-v3-its: Parse new version of msi-parent property

[Marc Zyngier]

Now that 126b16e2ad98 ("Docs: dt: add generic MSI bindings")
has made it into the tree, the time has come to get rid of the
old hack, and to parse msi-parent in its full glory.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3-its-platform-msi.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
index a865505..5a58a53 100644
--- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
@@ -29,13 +29,25 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
 {
 	struct msi_domain_info *msi_info;
 	u32 dev_id;
-	int ret;
+	int ret, index = 0;
 
 	msi_info = msi_get_domain_info(domain->parent);
 
 	/* Suck the DeviceID out of the msi-parent property */
-	ret = of_property_read_u32_index(dev->of_node, "msi-parent",
-					 1, &dev_id);
+	do {
+		struct of_phandle_args args;
+
+		ret = of_parse_phandle_with_opt_args(dev->of_node,
+						     "msi-parent", "#msi-cells",
+						     index, &args);
+		if (args.np == domain->of_node) {
+			if (WARN_ON(args.args_count != 1))
+				return -EINVAL;
+			dev_id = args.args[0];
+			break;
+		}
+	} while (!ret);
+
 	if (ret)
 		return ret;
 
-- 
2.1.4

Re: [PATCH 2/4] of: irq: Add support for the new definition of "msi-parent"

[Sergei Shtylyov]

Hello.

On 9/22/2015 8:52 PM, Marc Zyngier wrote:

> Since 126b16e2ad98 ("Docs: dt: add generic MSI bindings"),
> the definition of "msi-parent" has evolved, while maintaining
> some degree of compatibility. It can now express multiple MSI
> controllers as parents, as well as some sideband data being
> communicated to the controller.
>
> This patch revamps the parsing of the property, iterating over
> the multiple parents until a suitable irqdomain is found.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   drivers/of/irq.c | 26 +++++++++++++++++---------
>   1 file changed, 17 insertions(+), 9 deletions(-)

    Small stylistic nit below...

> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 55317fa..c9637cf 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -586,15 +586,23 @@ err:
>    */
>   void of_msi_configure(struct device *dev, struct device_node *np)
>   {
> -	struct device_node *msi_np;
> -	struct irq_domain *d;
> +	struct of_phandle_args args;
> +	int index = 0;
>
> -	msi_np = of_parse_phandle(np, "msi-parent", 0);
> -	if (!msi_np)
> -		return;
> +	while (!of_parse_phandle_with_opt_args(np, "msi-parent", "#msi-cells",
> +					       index, &args)) {
> +		struct irq_domain *d;
>
> -	d = irq_find_matching_host(msi_np, DOMAIN_BUS_PLATFORM_MSI);
> -	if (!d)
> -		d = irq_find_host(msi_np);
> -	dev_set_msi_domain(dev, d);
> +		d = irq_find_matching_host(args.np, DOMAIN_BUS_PLATFORM_MSI);
> +		if (!d)

		if (!d) {

> +			d = irq_find_host(args.np);
> +
> +		if (d) {

		} else {

> +			dev_set_msi_domain(dev, d);
> +			return;
> +		}
> +
> +		of_node_put(args.np);
> +		index++;
> +	}
>   }
>

MBR, Sergei

Re: [PATCH 2/4] of: irq: Add support for the new definition of "msi-parent"

[Thomas Gleixner]

On Wed, 23 Sep 2015, Sergei Shtylyov wrote:
> > -	d = irq_find_matching_host(msi_np, DOMAIN_BUS_PLATFORM_MSI);
> > -	if (!d)
> > -		d = irq_find_host(msi_np);
> > -	dev_set_msi_domain(dev, d);
> > +		d = irq_find_matching_host(args.np, DOMAIN_BUS_PLATFORM_MSI);
> > +		if (!d)
> 
> 		if (!d) {
> 
> > +			d = irq_find_host(args.np);
> > +
> > +		if (d) {
> 
> 		} else {
> 
> > +			dev_set_msi_domain(dev, d);
> > +			return;
> > +		}

Errm, no. How is that equivalent?

Marc:

      d = foo();
      if (!d)
      	 d = bar();
      if (d) {
         bla(d);
	 return;
      }

Yours:

      d = foo();
      if (!d) {
      	 d = bar();
      } else {
         bla(d);
	 return;
      }

Hmm?

Thanks,

	tglx

Re: [PATCH 2/4] of: irq: Add support for the new definition of "msi-parent"

[Sergei Shtylyov]

Hello.

On 9/27/2015 1:29 PM, Thomas Gleixner wrote:

>>> -	d = irq_find_matching_host(msi_np, DOMAIN_BUS_PLATFORM_MSI);
>>> -	if (!d)
>>> -		d = irq_find_host(msi_np);
>>> -	dev_set_msi_domain(dev, d);
>>> +		d = irq_find_matching_host(args.np, DOMAIN_BUS_PLATFORM_MSI);
>>> +		if (!d)
>>
>> 		if (!d) {
>>
>>> +			d = irq_find_host(args.np);
>>> +
>>> +		if (d) {
>>
>> 		} else {
>>
>>> +			dev_set_msi_domain(dev, d);
>>> +			return;
>>> +		}
>
> Errm, no. How is that equivalent?
>
> Marc:
>
>        d = foo();
>        if (!d)
>        	 d = bar();
>        if (d) {
>           bla(d);
> 	 return;
>        }
>
> Yours:
>
>        d = foo();
>        if (!d) {
>        	 d = bar();
>        } else {
>           bla(d);
> 	 return;
>        }
>
> Hmm?

    Ah, sorry, didn't notice the kind of assignment in the first branch. :-<

> Thanks,
> 	tglx

MBR, Sergei

Re: [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function

[Rob Herring]

On Tue, Sep 22, 2015 at 12:52 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> of_parse_phandle_with_args() is slightly inflexible as it doesn't
> allow the (unusual) case where the #*-cells property is not defined.
> In order to support this, introduce of_parse_phandle_with_opt_args()
> which assumes that #*-cells is zero when it is not defined,

zero or cell_count - 1?

I would be okay with always assuming zero rather than being an error
if that simplifies things. It is not really the kernel's job to be a
dtb validator.

Also, I assume this was done for some compatibility? In general, we
should be explicit, so "#msi-cells = <0>" should be recommended and we
should update dts files if they are not.

Rob

> as required by the msi-parent binding
>
> This is done by turning __of_parse_phandle_with_args into an even
> bigger monster, which is a bit frightening.
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/of/base.c  | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/of.h |  3 +++
>  2 files changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 8b5a187..1612342e 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1479,6 +1479,10 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
>                          * (i.e. cells_name not set, but cell_count is set),
>                          * except when we're going to return the found node
>                          * below.
> +                        *
> +                        * If #*-cells is not found, but cell_count is set
> +                        * to a non-zero value, use (cell_count-1) as a
> +                        * fallback value.
>                          */
>                         if (cells_name || cur_index == index) {
>                                 node = of_find_node_by_phandle(phandle);
> @@ -1490,13 +1494,21 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
>                         }
>
>                         if (cells_name) {
> -                               if (of_property_read_u32(node, cells_name,
> -                                                        &count)) {
> +                               int ret;
> +                               ret = of_property_read_u32(node, cells_name,
> +                                                          &count);
> +                               if (ret && !cell_count) {
>                                         pr_err("%s: could not get %s for %s\n",
>                                                 np->full_name, cells_name,
>                                                 node->full_name);
>                                         goto err;
>                                 }
> +                               if (ret) {
> +                                       count = cell_count - 1;
> +                                       pr_debug("%s: could not get %s for %s, assuming %d\n",
> +                                                np->full_name, cells_name,
> +                                                node->full_name, count);
> +                               }
>                         } else {
>                                 count = cell_count;
>                         }
> @@ -1628,6 +1640,54 @@ int of_parse_phandle_with_args(const struct device_node *np, const char *list_na
>  EXPORT_SYMBOL(of_parse_phandle_with_args);
>
>  /**
> + * of_parse_phandle_with_opt_args() - Find a node pointed by phandle in a list
> + * @np:                pointer to a device tree node containing a list
> + * @list_name: property name that contains a list
> + * @cells_name:        property name that specifies phandles' arguments count
> + * @index:     index of a phandle to parse out
> + * @out_args:  optional pointer to output arguments structure (will be filled)
> + *
> + * This function is useful to parse lists of phandles and their arguments.
> + * If cells_name is not found, then it is assumed to be zero.
> + * Returns 0 on success and fills out_args, on error returns appropriate
> + * errno value.
> + *
> + * Caller is responsible to call of_node_put() on the returned out_args->np
> + * pointer.
> + *
> + * Example:
> + *
> + * phandle1: node1 {
> + *     #list-cells = <2>;
> + * }
> + *
> + * phandle2: node2 {
> + * }
> + *
> + * phandle3: node3 {
> + *     #list-cells = <1>;
> + * }
> + *
> + * node3 {
> + *     list = <&phandle1 1 2 &phandle2 &phandle 3>;
> + * }
> + *
> + * To get a device_node of the `node2' node you may call this:
> + * of_parse_phandle_with_args(node3, "list", "#list-cells", 1, &args);
> + */
> +int of_parse_phandle_with_opt_args(const struct device_node *np,
> +                                  const char *list_name,
> +                                  const char *cells_name, int index,
> +                                  struct of_phandle_args *out_args)
> +{
> +       if (index < 0)
> +               return -EINVAL;
> +       return __of_parse_phandle_with_args(np, list_name, cells_name, 1,
> +                                           index, out_args);
> +}
> +EXPORT_SYMBOL(of_parse_phandle_with_opt_args);
> +
> +/**
>   * of_parse_phandle_with_fixed_args() - Find a node pointed by phandle in a list
>   * @np:                pointer to a device tree node containing a list
>   * @list_name: property name that contains a list
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 2194b8c..ae6cd03 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -328,6 +328,9 @@ extern struct device_node *of_parse_phandle(const struct device_node *np,
>  extern int of_parse_phandle_with_args(const struct device_node *np,
>         const char *list_name, const char *cells_name, int index,
>         struct of_phandle_args *out_args);
> +extern int of_parse_phandle_with_opt_args(const struct device_node *np,
> +       const char *list_name, const char *cells_name, int index,
> +       struct of_phandle_args *out_args);
>  extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
>         const char *list_name, int cells_count, int index,
>         struct of_phandle_args *out_args);
> --
> 2.1.4
>

Re: [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function

[Marc Zyngier]

On 29/09/15 18:28, Rob Herring wrote:
> On Tue, Sep 22, 2015 at 12:52 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> of_parse_phandle_with_args() is slightly inflexible as it doesn't
>> allow the (unusual) case where the #*-cells property is not defined.
>> In order to support this, introduce of_parse_phandle_with_opt_args()
>> which assumes that #*-cells is zero when it is not defined,
> 
> zero or cell_count - 1?

Zero is how the lack of #msi-cells property is interpreted. (cell_count
- 1) is how this is implemented.

> I would be okay with always assuming zero rather than being an error
> if that simplifies things. It is not really the kernel's job to be a
> dtb validator.

I'd be fine with that too. I'd just like it to be a defined behaviour,
not an unexpected side effect.

> Also, I assume this was done for some compatibility? In general, we
> should be explicit, so "#msi-cells = <0>" should be recommended and we
> should update dts files if they are not.

I agree that over time, we should update the existing DTS. But I hear
Mark chanting "Stable DT" behind me, so I'm inclined to provide an
transition path... ;-)

I'll respin the series later today.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function

[Mark Rutland]

On Tue, Sep 29, 2015 at 06:28:11PM +0100, Rob Herring wrote:
> On Tue, Sep 22, 2015 at 12:52 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > of_parse_phandle_with_args() is slightly inflexible as it doesn't
> > allow the (unusual) case where the #*-cells property is not defined.
> > In order to support this, introduce of_parse_phandle_with_opt_args()
> > which assumes that #*-cells is zero when it is not defined,
> 
> zero or cell_count - 1?
> 
> I would be okay with always assuming zero rather than being an error
> if that simplifies things. It is not really the kernel's job to be a
> dtb validator.

In most other cases #$foo-cells is strictly required, and you could get
bizarre behaviour in drivers by assuming 0. It would be good to keep a
warning for those.

That said, I guess drivers should be checking that the number of cells
is what they expect, so maybe any warnings should exist there.

> Also, I assume this was done for some compatibility?

Yup. There are existing users without #msi-cells (which is effectively
the same as #msi-cells = 0).

> In general, we should be explicit, so "#msi-cells = <0>" should be
> recommended and we should update dts files if they are not.

I agree, assuming we retain support for existing DTBs which lack
#msi-cells.

Mark.

Re: [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function

[Robin Murphy]

Hi Marc,

On 22/09/15 18:52, Marc Zyngier wrote:
> of_parse_phandle_with_args() is slightly inflexible as it doesn't
> allow the (unusual) case where the #*-cells property is not defined.
> In order to support this, introduce of_parse_phandle_with_opt_args()
> which assumes that #*-cells is zero when it is not defined,
> as required by the msi-parent binding
>
> This is done by turning __of_parse_phandle_with_args into an even
> bigger monster, which is a bit frightening.

A monster indeed; I can't quite figure out the exact effect this change 
has on of_count_phandle_with_args(), but I have a lingering doubt it may 
be something undesirable, since AFAICS that's now going to proceed from 
where it would have errored out before, with a count of -2.

I think it might be nicer to implement this by passing an extra "assume 
zero if #cells not found" boolean to __of_parse_phandle_with_args().

Alternatively, what's the actual likelihood of legacy bindings being 
mixed in with new ones? Could we not simply mandate that anyone adding 
an MSI controller with #msi-cells to a DT must ensure any existing nodes 
are also updated with #msi-cells = 0, and keep the legacy workaround 
self-contained in the MSI layer? e.g. paraphrasing from patch 2/2:

msi_np = of_parse_phandle(np, "msi-parent", 0);
if (!of_property_read_bool(msi_np, "#msi-cells"))
	return parse_this_thing(...);
else
	while (!of_parse_phandle_with_opt_args(np, "msi-parent"...
		if (parse_this_thing(...))
			return;

Robin.

Re: [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function

[Marc Zyngier]

On 30/09/15 16:39, Robin Murphy wrote:
> Hi Marc,
> 
> On 22/09/15 18:52, Marc Zyngier wrote:
>> of_parse_phandle_with_args() is slightly inflexible as it doesn't
>> allow the (unusual) case where the #*-cells property is not defined.
>> In order to support this, introduce of_parse_phandle_with_opt_args()
>> which assumes that #*-cells is zero when it is not defined,
>> as required by the msi-parent binding
>>
>> This is done by turning __of_parse_phandle_with_args into an even
>> bigger monster, which is a bit frightening.
> 
> A monster indeed; I can't quite figure out the exact effect this change 
> has on of_count_phandle_with_args(), but I have a lingering doubt it may 
> be something undesirable, since AFAICS that's now going to proceed from 
> where it would have errored out before, with a count of -2.
> 
> I think it might be nicer to implement this by passing an extra "assume 
> zero if #cells not found" boolean to __of_parse_phandle_with_args().
> 
> Alternatively, what's the actual likelihood of legacy bindings being 
> mixed in with new ones? Could we not simply mandate that anyone adding 
> an MSI controller with #msi-cells to a DT must ensure any existing nodes 
> are also updated with #msi-cells = 0, and keep the legacy workaround 
> self-contained in the MSI layer? e.g. paraphrasing from patch 2/2:
> 
> msi_np = of_parse_phandle(np, "msi-parent", 0);
> if (!of_property_read_bool(msi_np, "#msi-cells"))
> 	return parse_this_thing(...);
> else
> 	while (!of_parse_phandle_with_opt_args(np, "msi-parent"...
> 		if (parse_this_thing(...))
> 			return;

Having tried this, it doesn't look too bad. Specially turned into some
kind of library function. that can be shared between platform and PCI.

So I'll drop this patch altogether and repost an updated series.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...