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

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

[Rob Herring]

Fixing DT list address...

On Wed, Jan 15, 2014 at 9:16 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Jan 15, 2014 at 7:46 AM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> On Wed, Jan 15, 2014 at 12:40:41PM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> On 12:17 Wed 15 Jan     , Mark Rutland wrote:
>>> > 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.
>>>
>>> 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.
>>
>>>
>>> >
>>> > 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
--
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: [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property

[Mark Rutland]

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

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:12 Wed 15 Jan     , Mark Rutland wrote:
> > 
> > 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 <plagnioj@jcrosoft.com>

you finish it before me :(

Best Regards,
J.
> 
> 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
> 

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

[Grant Likely]

On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> > 
> > 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?

I think that is reasonable.

g.

> 
> Thanks,
> Mark.
> 
> ---->8----
> 
> From 88be0036b6a966bd7506f58e3cb9ce9ea4c5ac48 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> 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-5wv7dgnIgG8@public.gmane.org>
> ---
>  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
> 

--
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: [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property

[Olof Johansson]

On Mon, Jan 20, 2014 at 2:47 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> >
>> > 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?
>
> I think that is reasonable.


The main problem with this (same for clocks) is if you need to append
something with a name when the original didn't have any.

Reordering entries might not work for interrupts, since the bindings
might have requirements on order.

I'm not aware of a good solution for this. Suggestions welcome.


-Olof
--
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: [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

On Mon, Jan 20, 2014 at 05:03:23PM -0800, Olof Johansson wrote:
> On Mon, Jan 20, 2014 at 2:47 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> >> >
> >> > 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?
> >
> > I think that is reasonable.
> 
> 
> The main problem with this (same for clocks) is if you need to append
> something with a name when the original didn't have any.
> 
> Reordering entries might not work for interrupts, since the bindings
> might have requirements on order.
> 
> I'm not aware of a good solution for this. Suggestions welcome.

So, in principle my preferred way to handle things like this would be
to add richer expression support, including a token for "previous
value" when overlaying.

But although I've recently had another look at it, that's still a ways
off, so I wouldn't object to another approach as a stopgap, as long as
its not too hideous.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Mark Rutland]

On Tue, Jan 21, 2014 at 01:03:23AM +0000, Olof Johansson wrote:
> On Mon, Jan 20, 2014 at 2:47 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> >> >
> >> > 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?
> >
> > I think that is reasonable.
> 
> 
> The main problem with this (same for clocks) is if you need to append
> something with a name when the original didn't have any.

Can you not just add a name in the original file? I assume we're not
going to use this to adjust dts files we're not already in full control
of.

> 
> Reordering entries might not work for interrupts, since the bindings
> might have requirements on order.

That's a fair point.

Do we currently have any optional/board-specific interrupts which must
appear at the start or middle of the list?

For those, could we add names? The kernel should be abel to fall back to
ordering if names aren't present, and we can recommend a particular
ordering for compatiblity with older kernels.

As a general preventative measure it would be nice to have named
elements whenever elements can be optional.

> 
> I'm not aware of a good solution for this. Suggestions welcome.

Me neither. Prepending and appending is easy.

Inserting and/or modifying the list requires knowledge of the size of
each element (and for variable-sized entries requires knowledge of the
particular binding, which we cannot embed in dtc). 

I suspect adding richer syntax for modifying properties in arbitrary
ways will devolve into a turing tarpit.

Thanks,
Mark.
--
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: [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property

[Jean-Christophe PLAGNIOL-VILLARD]

On 12:02 Tue 21 Jan     , Mark Rutland wrote:
> On Tue, Jan 21, 2014 at 01:03:23AM +0000, Olof Johansson wrote:
> > On Mon, Jan 20, 2014 at 2:47 PM, Grant Likely <grant.likely@linaro.org> wrote:
> > > On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland@arm.com> wrote:
> > >> >
> > >> > 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?
> > >
> > > I think that is reasonable.
> > 
> > 
> > The main problem with this (same for clocks) is if you need to append
> > something with a name when the original didn't have any.
> 
> Can you not just add a name in the original file? I assume we're not
> going to use this to adjust dts files we're not already in full control
> of.
> 
> > 
> > Reordering entries might not work for interrupts, since the bindings
> > might have requirements on order.
> 
> That's a fair point.
> 
> Do we currently have any optional/board-specific interrupts which must
> appear at the start or middle of the list?
> 
> For those, could we add names? The kernel should be abel to fall back to
> ordering if names aren't present, and we can recommend a particular
> ordering for compatiblity with older kernels.
> 
> As a general preventative measure it would be nice to have named
> elements whenever elements can be optional.

I never was a fanof index search I do agree the names irq is the best way
> 
> > 
> > I'm not aware of a good solution for this. Suggestions welcome.
> 
> Me neither. Prepending and appending is easy.
> 
> Inserting and/or modifying the list requires knowledge of the size of
> each element (and for variable-sized entries requires knowledge of the
> particular binding, which we cannot embed in dtc). 
> 
> I suspect adding richer syntax for modifying properties in arbitrary
> ways will devolve into a turing tarpit.

no this need to stay simple if too much complexe => mess up, unmaintainable

if you really have complex stuff duplicate the info

Best Regards,
J.
> 
> Thanks,
> Mark.

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

[Grant Likely]

On Mon, 20 Jan 2014 17:03:23 -0800, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
> On Mon, Jan 20, 2014 at 2:47 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> >> >
> >> > 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?
> >
> > I think that is reasonable.
> 
> 
> The main problem with this (same for clocks) is if you need to append
> something with a name when the original didn't have any.
> 
> Reordering entries might not work for interrupts, since the bindings
> might have requirements on order.

Hmmm, yes. to handle that case would need a higher level construct that
keeps the name and interrupt value together. Maybe something like this?

device {
	add-interrupt(27);
	add-interrupt(29, 5, controller=&intc1);
	add-interrupt(2, 4, name="error");
};

I'm using python-like syntax here where controller and name are optional
arguments.

We could get DTC to generate the appropriate interrupt-parent,
interrupts, interrupts-extended and interrupt-names properties as
needed. Of course the shared context would be lost when the tree is
compiled, but it would be useful for building up parsing data. It would
also work for gpio, clocks and similar bindings. Kind of like an
intelligent macro.

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