| Message ID | 20140115161224.GH25824@e106331-lin.cambridge.arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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. g. > > 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 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. 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
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@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. > > 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.
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'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.
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.
On Mon, 20 Jan 2014 17:03:23 -0800, Olof Johansson <olof@lixom.net> 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. > > 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.
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;