[RFC PATCH 1/1] of/irq: create interrupts-extended-2 property

The new interrupts-extended property, which reuses the phandle+arguments
pattern used by GPIOs and other core bindings, still have some issue.

If an SoC have already specifiy interrupt and a board want to add specific
interrupt such as GPIO (which can be optionnal) be need to re-define
interrupts-extended. So allow to have an optionnale interrupts-extended-2
property.

Today the problem is ofen solve by defining a gpio-xxx property and then do a gpio_to_irq
in the C code. *Which is wrong!!*. We need to describe the IRQ.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <redacted>
Cc: Grant Likely <redacted>
Cc: Rob Herring <redacted>
---

We have the same issue on pinctrl
 .../devicetree/bindings/interrupt-controller/interrupts.txt    |  4 ++++
 drivers/of/irq.c                                               | 10 ++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
index 1486497..5d559fd 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

@@ -25,8 +25,12 @@ to reference multiple interrupt parents. Each entry in this property contains
 both the parent phandle and the interrupt specifier. "interrupts-extended"
 should only be used when a device has multiple interrupt parents.
 
+The "interrupts-extended-2" allow to extend at board level node interrupt without
+having to re-define the SoC interrupts.
+
   Example:
 	interrupts-extended = <&intc1 5 1>, <&intc2 1 0>;
+	interrupts-extended-2 = <&intc1 6 1>
 
 A device node may contain either "interrupts" or "interrupts-extended", but not
 both. If both properties are present, then the operating system should log an

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 786b0b4..bc36710 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c

@@ -307,8 +307,14 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
 		/* Try the new-style interrupts-extended */
 		res = of_parse_phandle_with_args(device, "interrupts-extended",
 						"#interrupt-cells", index, out_irq);
-		if (res)
-			return -EINVAL;
+		if (res) {
+			/* Try the new-style interrupts-extended-2 */
+			res = of_parse_phandle_with_args(device, "interrupts-extended-2",
+							"#interrupt-cells", index, out_irq);
+			if (res)
+				return -EINVAL;
+		}
+
 		return of_irq_parse_raw(addr, out_irq);
 	}
 	intlen /= sizeof(*intspec);

-- 
1.8.4.3

On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:

The new interrupts-extended property, which reuses the phandle+arguments
pattern used by GPIOs and other core bindings, still have some issue.

If an SoC have already specifiy interrupt and a board want to add specific
interrupt such as GPIO (which can be optionnal) be need to re-define
interrupts-extended. So allow to have an optionnale interrupts-extended-2
property.

NAK.

This is a hack that works around a dts organisation issue. This is _not_
a binding or parsing issue.

Properties can be overridden - just describe all of the interrupts in
the final dts file.

This also breaks error reporting.

What if you have an interrupt in the middle of interrupts-extended which
fails to parse? This will jump to the interrupts-extended-2 property and
grab the wrong interrupt rather than propagating the error.

Thanks,
Mark.

On 12:17 Wed 15 Jan     , Mark Rutland wrote:

On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:

quoted

The new interrupts-extended property, which reuses the phandle+arguments
pattern used by GPIOs and other core bindings, still have some issue.

If an SoC have already specifiy interrupt and a board want to add specific
interrupt such as GPIO (which can be optionnal) be need to re-define
interrupts-extended. So allow to have an optionnale interrupts-extended-2
property.

NAK.

This is a hack that works around a dts organisation issue. This is _not_
a binding or parsing issue.

So the DT is stupid. Yes w have board and SoC information
so we do not want to duplicate them. Having a way to descript SoC vs board
specific information need to be provided.

Properties can be overridden - just describe all of the interrupts in
the final dts file.

this is wrong, which mean you duplicate informaation and duplicate bug and
fixes

quoted

Today the problem is ofen solve by defining a gpio-xxx property and then do a gpio_to_irq
in the C code. *Which is wrong!!*. We need to describe the IRQ.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <redacted>
Cc: Grant Likely <redacted>
Cc: Rob Herring <redacted>
---

We have the same issue on pinctrl
 .../devicetree/bindings/interrupt-controller/interrupts.txt    |  4 ++++
 drivers/of/irq.c                                               | 10 ++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
index 1486497..5d559fd 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

@@ -25,8 +25,12 @@ to reference multiple interrupt parents. Each entry in this property contains
 both the parent phandle and the interrupt specifier. "interrupts-extended"
 should only be used when a device has multiple interrupt parents.
 
+The "interrupts-extended-2" allow to extend at board level node interrupt without
+having to re-define the SoC interrupts.
+
   Example:
 	interrupts-extended = <&intc1 5 1>, <&intc2 1 0>;
+	interrupts-extended-2 = <&intc1 6 1>
 
 A device node may contain either "interrupts" or "interrupts-extended", but not
 both. If both properties are present, then the operating system should log an

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 786b0b4..bc36710 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c

@@ -307,8 +307,14 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
 		/* Try the new-style interrupts-extended */
 		res = of_parse_phandle_with_args(device, "interrupts-extended",
 						"#interrupt-cells", index, out_irq);
-		if (res)
-			return -EINVAL;
+		if (res) {
+			/* Try the new-style interrupts-extended-2 */
+			res = of_parse_phandle_with_args(device, "interrupts-extended-2",
+							"#interrupt-cells", index, out_irq);

This also breaks error reporting.

What if you have an interrupt in the middle of interrupts-extended which
fails to parse? This will jump to the interrupts-extended-2 property and
grab the wrong interrupt rather than propagating the error.

Thanks,
Mark.

On Wed, Jan 15, 2014 at 12:40:41PM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:

On 12:17 Wed 15 Jan     , Mark Rutland wrote:

quoted

On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:

quoted

The new interrupts-extended property, which reuses the phandle+arguments
pattern used by GPIOs and other core bindings, still have some issue.

If an SoC have already specifiy interrupt and a board want to add specific
interrupt such as GPIO (which can be optionnal) be need to re-define
interrupts-extended. So allow to have an optionnale interrupts-extended-2
property.

NAK.

This is a hack that works around a dts organisation issue. This is _not_
a binding or parsing issue.

So the DT is stupid. Yes w have board and SoC information
so we do not want to duplicate them. Having a way to descript SoC vs board
specific information need to be provided.

I agree that the current situation isn't fantastic. That doesn't make
the DT stupid. There are other solutions to this problem.

quoted

Properties can be overridden - just describe all of the interrupts in
the final dts file.

this is wrong, which mean you duplicate informaation and duplicate bug and
fixes

It's certainly not nice, but it doesn't make it wrong.

The same problem can be found with any other list property that varies
per-board. Hacking the parsing of each property is not a solution.

Today the only way to implement what you want is to have the list in the
final file.

One way of working with that would be to have the common interrupts
described in a shared macro in the soc file:

#define SOC_COMMON_INTERRUPTS <&intc1 5 1>, <&intc2 1 0>

Then the soc file can have:

	interrupts = SOC_COMMON_INTERRUPTS;

And the board can have:

	interrupts = SOC_COMMON_INTERRUPTS, <&intc1 6 1>;

That gains functionality equivalent to your proposal, without
introducing new code or new edge cases in interrupt parsing. However, it
does make it unclear how many interrupts are described, and it won't
interact well with interrupt-names.

Another, more invasive option would be extend the dts syntax and teach
dtc to handle property appending. Then the soc dts could stay as it is,
and the board dts could have something like:

	/append-property/ interrupts = <&intc1 6 1>;
	/append-property/ interrupt-names = "board-specific-irq";

Both these options solve the issue at the source, are general to other
properties, and allow more than one level of hierarchy (the proposed
interrupts-extended-2 only allows one level).

Thanks,
Mark.

On 13:46 Wed 15 Jan     , Mark Rutland wrote:

On Wed, Jan 15, 2014 at 12:40:41PM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:

quoted

On 12:17 Wed 15 Jan     , Mark Rutland wrote:

quoted

On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:

quoted

The new interrupts-extended property, which reuses the phandle+arguments
pattern used by GPIOs and other core bindings, still have some issue.

If an SoC have already specifiy interrupt and a board want to add specific
interrupt such as GPIO (which can be optionnal) be need to re-define
interrupts-extended. So allow to have an optionnale interrupts-extended-2
property.

NAK.

This is a hack that works around a dts organisation issue. This is _not_
a binding or parsing issue.

So the DT is stupid. Yes w have board and SoC information
so we do not want to duplicate them. Having a way to descript SoC vs board
specific information need to be provided.

I agree that the current situation isn't fantastic. That doesn't make
the DT stupid. There are other solutions to this problem.

quoted

quoted

Properties can be overridden - just describe all of the interrupts in
the final dts file.

this is wrong, which mean you duplicate informaation and duplicate bug and
fixes

It's certainly not nice, but it doesn't make it wrong.

The same problem can be found with any other list property that varies
per-board. Hacking the parsing of each property is not a solution.

agreed we have the same issue on pinctrl as example and it's become a
nightmare

Today the only way to implement what you want is to have the list in the
final file.

One way of working with that would be to have the common interrupts
described in a shared macro in the soc file:

#define SOC_COMMON_INTERRUPTS <&intc1 5 1>, <&intc2 1 0>

Then the soc file can have:

	interrupts = SOC_COMMON_INTERRUPTS;

And the board can have:

	interrupts = SOC_COMMON_INTERRUPTS, <&intc1 6 1>;

hugly for me as a board could work on x SoC so the final dt could be compile
for more than 1 SoC if you one alias

That gains functionality equivalent to your proposal, without
introducing new code or new edge cases in interrupt parsing. However, it
does make it unclear how many interrupts are described, and it won't
interact well with interrupt-names.

Another, more invasive option would be extend the dts syntax and teach
dtc to handle property appending. Then the soc dts could stay as it is,
and the board dts could have something like:

	/append-property/ interrupts = <&intc1 6 1>;
	/append-property/ interrupt-names = "board-specific-irq";

Both these options solve the issue at the source, are general to other
properties, and allow more than one level of hierarchy (the proposed
interrupts-extended-2 only allows one level).

agreed here but this touch dtc

Best Regards,
J.

Thanks,
Mark.

On Wed, Jan 15, 2014 at 12:17 PM, Mark Rutland [off-list ref] wrote:

On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:

quoted

The new interrupts-extended property, which reuses the phandle+arguments
pattern used by GPIOs and other core bindings, still have some issue.

If an SoC have already specifiy interrupt and a board want to add specific
interrupt such as GPIO (which can be optionnal) be need to re-define
interrupts-extended. So allow to have an optionnale interrupts-extended-2
property.

NAK.

This is a hack that works around a dts organisation issue. This is _not_
a binding or parsing issue.

Properties can be overridden - just describe all of the interrupts in
the final dts file.

Agreed. The current binding handles the case of multiple interrupts just fine.

g.

On Wed, Jan 15, 2014 at 7:46 AM, Mark Rutland [off-list ref] wrote:

On Wed, Jan 15, 2014 at 12:40:41PM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:

quoted

On 12:17 Wed 15 Jan     , Mark Rutland wrote:

quoted

On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:

quoted

The new interrupts-extended property, which reuses the phandle+arguments
pattern used by GPIOs and other core bindings, still have some issue.

If an SoC have already specifiy interrupt and a board want to add specific
interrupt such as GPIO (which can be optionnal) be need to re-define
interrupts-extended. So allow to have an optionnale interrupts-extended-2
property.

NAK.

This is a hack that works around a dts organisation issue. This is _not_
a binding or parsing issue.

So the DT is stupid. Yes w have board and SoC information
so we do not want to duplicate them. Having a way to descript SoC vs board
specific information need to be provided.

I agree that the current situation isn't fantastic. That doesn't make
the DT stupid. There are other solutions to this problem.

quoted

quoted

Properties can be overridden - just describe all of the interrupts in
the final dts file.

this is wrong, which mean you duplicate informaation and duplicate bug and
fixes

It's certainly not nice, but it doesn't make it wrong.

The same problem can be found with any other list property that varies
per-board. Hacking the parsing of each property is not a solution.

Today the only way to implement what you want is to have the list in the
final file.

One way of working with that would be to have the common interrupts
described in a shared macro in the soc file:

#define SOC_COMMON_INTERRUPTS <&intc1 5 1>, <&intc2 1 0>

Yuck. Please, let's not encourage this pattern.

Then the soc file can have:

        interrupts = SOC_COMMON_INTERRUPTS;

And the board can have:

        interrupts = SOC_COMMON_INTERRUPTS, <&intc1 6 1>;

Unless this is solved in dtc include mechanism, just duplicate the
interrupts in each board file. I'd rather have the duplication than
continued churn of refactoring dtsi files.

Rob

Another, more invasive option would be extend the dts syntax and teach
dtc to handle property appending. Then the soc dts could stay as it is,
and the board dts could have something like:

	/append-property/ interrupts = <&intc1 6 1>;
	/append-property/ interrupt-names = "board-specific-irq";

Both these options solve the issue at the source, are general to other
properties, and allow more than one level of hierarchy (the proposed
interrupts-extended-2 only allows one level).

I've just had a go at implementing the append-property mechanism above
in dtc, and it was far easier than I expected (patch below).

Does anyone have any issues with the /append-property/ idea?

Thanks,
Mark.

---->8----

From 88be0036b6a966bd7506f58e3cb9ce9ea4c5ac48 Mon Sep 17 00:00:00 2001

From: Mark Rutland <mark.rutland@arm.com>
Date: Wed, 15 Jan 2014 15:43:51 +0000
Subject: [PATCH] dtc: add ability to append properties

When dealing with hierarchies of dtsi files, handling minute differences
between individual boards can become very painful. Adding a single
board-specific interrupt requires duplicating the entire interrupts
property, which requires duplication of common values. This makes bug
fixing painful and if not handled very carefully files diverge rapdily.

To ameliorate this, this patch adds the ability to append properties,
allowing board files to describe only the additional values required in
a property. This functionality also works with strings, so parallel
properties like interupts and interrupt-names stay in sync. Properties
may be appended multiple times, and deleting properties clears all
previous appended values.

To append a property, secondary definitions must be prefixed with
/append-property/. This is longer than a possible '+=' syntax, but makes
it far easier to spot when appending behaviour is requested, and so
hopefully will lead to fewer buggy dts files.

For example, if the following dts fragements are compiled together:

  / {
      interrupts = <0>, <1>;
      interrupt-names = "zero", "one";
  };

  / {
      /append-property/ interrupts = <2>, <3>;
      /append-property/ interrupt-names = "two", "three";
  };

They will result in a dtb equivalent to the following dts fragment:

  / {
      interrupts = <0>, <1>, <2>, <3>;
      interrupt-names = "zero", "one", "two", "three";
  };

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 dtc-lexer.l  |  7 +++++++
 dtc-parser.y |  5 +++++
 dtc.h        |  2 ++
 livetree.c   | 15 ++++++++++++++-
 4 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/dtc-lexer.l b/dtc-lexer.l
index 0cd7e67..7989abe 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l

@@ -131,6 +131,13 @@ static bool pop_input_file(void);
 			return DT_DEL_PROP;
 		}
 
+<*>"/append-property/"	{
+			DPRINT("Keyword: /append-property/\n");
+			DPRINT("<PROPNODENAME>\n");
+			BEGIN(PROPNODENAME);
+			return DT_APPEND_PROP;
+		}
+
 <*>"/delete-node/"	{
 			DPRINT("Keyword: /delete-node/\n");
 			DPRINT("<PROPNODENAME>\n");

diff --git a/dtc-parser.y b/dtc-parser.y
index 4864631..a8409ed 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y

@@ -63,6 +63,7 @@ static unsigned char eval_char_literal(const char *s);
 %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
 %token DT_BITS
 %token DT_DEL_PROP
+%token DT_APPEND_PROP
 %token DT_DEL_NODE
 %token <propnodename> DT_PROPNODENAME
 %token <literal> DT_LITERAL

@@ -195,6 +196,10 @@ propdef:
 		{
 			$$ = build_property($1, empty_data);
 		}
+	| DT_APPEND_PROP DT_PROPNODENAME '=' propdata ';'
+		{
+			$$ = build_property_append($2, $4);
+		}
 	| DT_DEL_PROP DT_PROPNODENAME ';'
 		{
 			$$ = build_property_delete($2);

diff --git a/dtc.h b/dtc.h
index 20e4d56..8687530 100644
--- a/dtc.h
+++ b/dtc.h

@@ -133,6 +133,7 @@ struct label {
 };
 
 struct property {
+	bool appended;
 	bool deleted;
 	char *name;
 	struct data val;

@@ -186,6 +187,7 @@ void delete_labels(struct label **labels);
 
 struct property *build_property(char *name, struct data val);
 struct property *build_property_delete(char *name);
+struct property *build_property_append(char *name, struct data val);
 struct property *chain_property(struct property *first, struct property *list);
 struct property *reverse_properties(struct property *first);
 

diff --git a/livetree.c b/livetree.c
index b61465f..894e42b 100644
--- a/livetree.c
+++ b/livetree.c

@@ -74,6 +74,15 @@ struct property *build_property_delete(char *name)
 	return new;
 }
 
+struct property *build_property_append(char *name, struct data val)
+{
+	struct property *new = build_property(name, val);
+
+	new->appended = 1;
+
+	return new;
+}
+
 struct property *chain_property(struct property *first, struct property *list)
 {
 	assert(first->next == NULL);

@@ -167,7 +176,11 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 				for_each_label_withdel(new_prop->labels, l)
 					add_label(&old_prop->labels, l->label);
 
-				old_prop->val = new_prop->val;
+				if (new_prop->appended)
+					old_prop->val = data_merge(old_prop->val, new_prop->val);
+				else
+					old_prop->val = new_prop->val;
+
 				old_prop->deleted = 0;
 				free(new_prop);
 				new_prop = NULL;

-- 
1.8.1.1

On 16:12 Wed 15 Jan     , Mark Rutland wrote:

quoted

Another, more invasive option would be extend the dts syntax and teach
dtc to handle property appending. Then the soc dts could stay as it is,
and the board dts could have something like:

	/append-property/ interrupts = <&intc1 6 1>;
	/append-property/ interrupt-names = "board-specific-irq";

Both these options solve the issue at the source, are general to other
properties, and allow more than one level of hierarchy (the proposed
interrupts-extended-2 only allows one level).

I've just had a go at implementing the append-property mechanism above
in dtc, and it was far easier than I expected (patch below).

Does anyone have any issues with the /append-property/ idea?

Acked-by: Jean-Christophe PLAGNIOL-VILLARD <redacted>

you finish it before me :(

Best Regards,
J.