* [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function
@ 2015-09-22 17:52 ` Marc Zyngier
0 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2015-09-22 17:52 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: Mark Rutland, Rob Herring, Jason Cooper, Thomas Gleixner,
Bjorn Helgaas, Mark Rutland
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
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function
2015-09-22 17:52 ` Marc Zyngier
@ 2015-09-29 17:28 ` Rob Herring
-1 siblings, 0 replies; 26+ messages in thread
From: Rob Herring @ 2015-09-29 17:28 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function
@ 2015-09-29 17:28 ` Rob Herring
0 siblings, 0 replies; 26+ messages in thread
From: Rob Herring @ 2015-09-29 17:28 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Mark Rutland, Rob Herring,
Jason Cooper, Thomas Gleixner, Bjorn Helgaas
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function
2015-09-29 17:28 ` Rob Herring
@ 2015-09-30 9:08 ` Marc Zyngier
-1 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2015-09-30 9:08 UTC (permalink / raw)
To: linux-arm-kernel
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...
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function
@ 2015-09-30 9:08 ` Marc Zyngier
0 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2015-09-30 9:08 UTC (permalink / raw)
To: Rob Herring
Cc: linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Mark Rutland, Rob Herring,
Jason Cooper, Thomas Gleixner, Bjorn Helgaas
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...
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function
2015-09-29 17:28 ` Rob Herring
@ 2015-09-30 13:57 ` Mark Rutland
-1 siblings, 0 replies; 26+ messages in thread
From: Mark Rutland @ 2015-09-30 13:57 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function
@ 2015-09-30 13:57 ` Mark Rutland
0 siblings, 0 replies; 26+ messages in thread
From: Mark Rutland @ 2015-09-30 13:57 UTC (permalink / raw)
To: Rob Herring
Cc: Marc Zyngier, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Rob Herring, Jason Cooper,
Thomas Gleixner, Bjorn Helgaas
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function
2015-09-22 17:52 ` Marc Zyngier
@ 2015-09-30 15:39 ` Robin Murphy
-1 siblings, 0 replies; 26+ messages in thread
From: Robin Murphy @ 2015-09-30 15:39 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function
@ 2015-09-30 15:39 ` Robin Murphy
0 siblings, 0 replies; 26+ messages in thread
From: Robin Murphy @ 2015-09-30 15:39 UTC (permalink / raw)
To: Marc Zyngier, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Cc: Mark Rutland, Bjorn Helgaas, Thomas Gleixner, Rob Herring,
Jason Cooper
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function
2015-09-30 15:39 ` Robin Murphy
@ 2015-09-30 17:18 ` Marc Zyngier
-1 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2015-09-30 17:18 UTC (permalink / raw)
To: linux-arm-kernel
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...
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] of: Add of_parse_phandle_with_opt_args() helper function
@ 2015-09-30 17:18 ` Marc Zyngier
0 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2015-09-30 17:18 UTC (permalink / raw)
To: Robin Murphy, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Cc: Mark Rutland, Bjorn Helgaas, Thomas Gleixner, Rob Herring,
Jason Cooper
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...
^ permalink raw reply [flat|nested] 26+ messages in thread