Implements new features for updating existing device tree nodes.
PATCH 1/1 Implements new features for updating existing device tree nodes.
This is interesting when the /include/ "<filename>" feature is used. This way we can create base device tree source files for a family of systems and modify the device tree for a specific system.
The current sytem allows an existing node to be extended with new properties and subnodes.
The new features allow an existing node to be replaced completely by the new properties and subnodes. The new features also allow an existing node to be deleted.
This patch does not yet provide a way to remove properties.
- John
---
dtc-lexer.l | 12 ++++++++++ dtc-parser.y | 24 ++++++++++++++++++++ dtc.h | 2 ++ livetree.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 0 deletions(-)
dtc-lexer.l +12 -0
@@ -96,6 +96,18 @@ static int pop_input_file(void); return DT_MEMRESERVE; } +<*>"/trim-node/" { + DPRINT("Keyword: /trim-node/\n"); + BEGIN_DEFAULT(); + return DT_REPLACENODE; + } + +<*>"/remove-node/" { + DPRINT("Keyword: /remove-node/\n"); + BEGIN_DEFAULT(); + return DT_REMOVENODE; + } + <*>{LABEL}: { DPRINT("Label: %s\n", yytext); yylval.labelref = xstrdup(yytext);
dtc-parser.y +24 -0
@@ -54,6 +54,8 @@ static unsigned long long eval_literal(const char *s, int base, int bits); %token DT_V1 %token DT_MEMRESERVE +%token DT_REPLACENODE +%token DT_REMOVENODE %token <propnodename> DT_PROPNODENAME %token <literal> DT_LITERAL %token <cbase> DT_BASE
@@ -140,6 +142,28 @@ devicetree: " node extension", $2); $$ = $1; } + | devicetree DT_REPLACENODE DT_REF nodedef + { + struct node *target; + + target = get_node_by_label($1, $3); + if (target) + replace_nodes(target, $4); + else + yyerror("label, '%s' does not exist in" + " node redefinition", $3); + $$ = $1; + } + | devicetree DT_REMOVENODE DT_REF ';' + { + struct node *target; + + target = get_node_by_label($1, $3); + if (target) + remove_nodes(target); + else + yyerror("label, '%s' does not exist in" + " node removal", $3); ; nodedef:
dtc.h +2 -0
@@ -175,6 +175,8 @@ struct node *build_node(struct property *proplist, struct node *children);struct node *name_node (struct node *node,char *name);struct node *chain_node (struct node *first,struct node *list);struct node *merge_nodes (struct node *old_node,struct node *new_node); +struct node *replace_nodes (struct node *old_node,struct node *new_node); +struct node *remove_nodes (struct node *node);void add_property (struct node *node,struct property *prop);void add_child (struct node *parent,struct node *child);
livetree.c +69 -0
75 lines not on this page · raw message
Hi John.
Quoted 7 lines · John Bonesio
This is interesting when the /include/ "<filename>" feature is used. This way we can create base device tree source files for a family of systems and modify the device tree for a specific system.
The current sytem allows an existing node to be extended with new properties and subnodes.
Quoted 7 lines
The new features allow an existing node to be replaced completely by the new properties and subnodes. The new features also allow an existing node to be deleted.
This patch does not yet provide a way to remove properties.
- John
Also need s-o-b line, and anything that doesn't belong as part of the commit message, such as '- John', needs to be below the '---' line so that git will ignore it.
The patch looks pretty good, but I cannot comment fully until the keyword handling works when deeper in the tree (see below).
Quoted 61 lines
---
dtc-lexer.l | 12 ++++++++++
dtc-parser.y | 24 ++++++++++++++++++++
dtc.h | 2 ++
livetree.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++4 files changed, 107 insertions(+), 0 deletions(-)dtc-lexer.l +12
-0@@ -96,6 +96,18 @@ static int pop_input_file(void); return DT_MEMRESERVE; } +<*>"/trim-node/" { + DPRINT("Keyword: /trim-node/\n"); + BEGIN_DEFAULT(); + return DT_REPLACENODE; + } + +<*>"/remove-node/" { + DPRINT("Keyword: /remove-node/\n"); + BEGIN_DEFAULT(); + return DT_REMOVENODE; + } + <*>{LABEL}: { DPRINT("Label: %s\n", yytext); yylval.labelref = xstrdup(yytext);dtc-parser.y +0
-0@@ -54,6 +54,8 @@ static unsigned long long eval_literal(const char *s, int base, int bits);%token
DT_V1
%tokenDT_MEMRESERVE
+%tokenDT_REPLACENODE
+%tokenDT_REMOVENODE
%token <propnodename>DT_PROPNODENAME
%token <literal>DT_LITERAL
%token <cbase>DT_BASEdtc-parser.y +13
-0@@ -140,6 +142,28 @@ devicetree: " node extension", $2); $$ = $1; } + | devicetree DT_REPLACENODE DT_REF nodedef + { + struct node *target; + + target = get_node_by_label($1, $3); + if (target) + replace_nodes(target, $4); + else + yyerror("label, '%s' does not exist in" + " node redefinition", $3); + $$ = $1; + } + | devicetree DT_REMOVENODE DT_REF ';'
dtc-parser.y devicetree: · line 167
Quoted 10 lines
dtc-parser.y +9
-0+ { + struct node *target; + + target = get_node_by_label($1, $3); + if (target) + remove_nodes(target); + else + yyerror("label, '%s' does not exist in" + " node removal", $3); ;
This handles the syntax at the top level, but doesn't deal with the /*-node/ keywords when they are within the overlay tree.
Also, I don't know if you'll be able to handle it in the same way when it comes to handling the new tokens embedded within an overlay tree because it isn't so easy to resolve the node to be removed. In order to handle both cases in the same way, it might be better to set a 'replace' flag in struct node that defers the actual remove of the original node until merge_nodes() time.
Quoted 20 lines
nodedef:
dtc.h +2
-0@@ -175,6 +175,8 @@ struct node *build_node(struct property *proplist, struct node *children); struct node *name_node(struct node *node, char *name); struct node *chain_node(struct node *first, struct node *list); struct node *merge_nodes(struct node *old_node, struct node *new_node); +struct node *replace_nodes (struct node *old_node,struct node *new_node); +struct node *remove_nodes (struct node *node); void add_property(struct node *node, struct property *prop); void add_child(struct node *parent, struct node *child);livetree.c +0
-0
livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 170
Quoted 4 lines
livetree.c +1
-0@@ -167,6 +167,75 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node) return old_node; } +struct node *remove_nodes (struct node *node) {
Can be void. The return value of remove_nodes() is never used.
livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 181
Quoted 11 lines
livetree.c +11
-0+ struct property *prop; + struct node *child, *parent; + + while (node->proplist) { + /* Pop the property off the list */ + prop = node->proplist; + node->proplist = prop->next; + prop->next = NULL; + + free(prop); + }
It would be useful to have a 'pop_property()' macro so this could be simplified to:
while (prop = pop_child_node(&node->proplist)) free(prop);
But then on the other hand, most of the list management is open coded in dtc. It would be nice to use the list_head macros like used in the kernel, but that is probably work for another day. (if it is even worth the effort).
livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 193
Quoted 12 lines
livetree.c +12
-0+ + /* Free up the node's children */ + /* This assumes we have a tree and not a cylic or acylic graph */ + while (node->children) { + /* Pop the child node off the list */ + child = node->children; + node->children = child->next_sibling; + child->parent = NULL; + child->next_sibling = NULL; + + child = remove_nodes(child); + }
Ditto here, except as pop_child_node().
Quoted 14 lines
livetree.c +14
-0+ + /* Remove the node from it's parent's child list */ + if (node->parent) { + int found = 0; + + parent = node->parent; + + /* if the node is the first child ... */ + if (parent->children == node) { + parent->children = node->next_sibling; + found = 1; + } else { + for_each_child(parent, child) { + if (child->next_sibling == node) {
livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 217
Quoted 10 lines
livetree.c +10
-0+ child->next_sibling = node->next_sibling; + found = 1; + break; + } + } + } + assert(found); /* Woah! We've got an inconsistent tree here */ + node->parent = NULL; + node->next_sibling = NULL; + }
This hunk is redundantly located because recursive calls have already removed the node from the parent. Should split into a separate function called by dtc-parse.y so that the recursive bit is simpler.
livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 222
Quoted 5 lines
livetree.c +5
-0+ + free(node); + + return NULL; +}
This block looks correct to me. It seemed a little verbose, but when I went through it line by line I saw that it pretty much needs to be that way. Especially with the list operations being open coded.
Quoted 25 lines
livetree.c +16
-0+ +struct node *replace_nodes (struct node *old_node,struct node *new_node) +{ + struct label *l; + + /* Add old node labels to new node, so we can still refer to the + * new node by the old labels. */ + for_each_label(old_node->labels, l) + add_label(&new_node->labels, l->label); + + /* Free up the old_node */ + remove_nodes(old_node); + + return new_node; +} + struct node *chain_node(struct node *first, struct node *list) { assert(first->next_sibling == NULL);_______________________________________________
devicetree-discuss mailing list devicetree-discuss-uLR06cmDAlY/[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
Signature · 2 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd.
A message that is not in the archive (sent off-list or not yet received): AANLkTimx4z-YkH+PXcnKYJpq3MEd3oJZ+ow7mWJzn1BN-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 100 lines · Grant Likely
Hi John.
Quoted 7 lines · John Bonesio
This is interesting when the /include/ "<filename>" feature is used. This way we can create base device tree source files for a family of systems and modify the device tree for a specific system.
The current sytem allows an existing node to be extended with new properties and subnodes.
Quoted 7 lines
The new features allow an existing node to be replaced completely by the new properties and subnodes. The new features also allow an existing node to be deleted.
This patch does not yet provide a way to remove properties.
- John
Also need s-o-b line, and anything that doesn't belong as part of the commit message, such as '- John', needs to be below the '---' line so that git will ignore it.
The patch looks pretty good, but I cannot comment fully until the keyword handling works when deeper in the tree (see below).
Quoted 61 lines
---
dtc-lexer.l | 12 ++++++++++ dtc-parser.y | 24 ++++++++++++++++++++ dtc.h | 2 ++ livetree.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 0 deletions(-)dtc-lexer.l +12
-0@@ -96,6 +96,18 @@ static int pop_input_file(void); return DT_MEMRESERVE; } +<*>"/trim-node/" { + DPRINT("Keyword: /trim-node/\n"); + BEGIN_DEFAULT(); + return DT_REPLACENODE; + } + +<*>"/remove-node/" { + DPRINT("Keyword: /remove-node/\n"); + BEGIN_DEFAULT(); + return DT_REMOVENODE; + } + <*>{LABEL}: { DPRINT("Label: %s\n", yytext); yylval.labelref = xstrdup(yytext);dtc-parser.y +15
-0@@ -54,6 +54,8 @@ static unsigned long long eval_literal(const char *s, int base, int bits); %token DT_V1 %token DT_MEMRESERVE +%token DT_REPLACENODE +%token DT_REMOVENODE %token <propnodename> DT_PROPNODENAME %token <literal> DT_LITERAL %token <cbase> DT_BASE@@ -140,6 +142,28 @@ devicetree: " node extension", $2); $$ = $1; } + | devicetree DT_REPLACENODE DT_REF nodedef + { + struct node *target; + + target = get_node_by_label($1, $3); + if (target) + replace_nodes(target, $4); + else + yyerror("label, '%s' does not exist in" + " node redefinition", $3); + $$ = $1; + } + | devicetree DT_REMOVENODE DT_REF ';'dtc-parser.y devicetree: · line 167
Quoted 10 lines
dtc-parser.y +9
-0+ { + struct node *target; + + target = get_node_by_label($1, $3); + if (target) + remove_nodes(target); + else + yyerror("label, '%s' does not exist in" + " node removal", $3); ;This handles the syntax at the top level, but doesn't deal with the /*-node/ keywords when they are within the overlay tree.
Quoted 6 lines
Also, I don't know if you'll be able to handle it in the same way when it comes to handling the new tokens embedded within an overlay tree because it isn't so easy to resolve the node to be removed. In order to handle both cases in the same way, it might be better to set a 'replace' flag in struct node that defers the actual remove of the original node until
merge_nodes()time.
Ok, so maybe I misunderstood. I thought we had decided that node modifications would only happen at the top level, but now that I'm looking back through your examples earlier today, and I see this isn't true.
I'll take a look at changing the implementation.
Quoted 135 lines
Quoted 21 lines
dtc-parser.y +0
-0nodedef:dtc.h +2
-0@@ -175,6 +175,8 @@ struct node *build_node(struct property *proplist, struct node *children);struct node *name_node (struct node *node,char *name);struct node *chain_node (struct node *first,struct node *list);struct node *merge_nodes (struct node *old_node,struct node *new_node); +struct node *replace_nodes (struct node *old_node,struct node *new_node); +struct node *remove_nodes (struct node *node);void add_property (struct node *node,struct property *prop);void add_child (struct node *parent,struct node *child);livetree.c +0
-0livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 170
Quoted 4 lines
livetree.c +1
-0@@ -167,6 +167,75 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)return old_node; } +struct node *remove_nodes (struct node *node) {Can be void. The return value of
remove_nodes()is never used.livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 181
Quoted 11 lines
livetree.c +11
-0+struct property *prop; +struct node *child, *parent; + +while (node->proplist) { +/* Pop the property off the list */ + prop = node->proplist; + node->proplist = prop->next; + prop->next =NULL ; + +free (prop); + }It would be useful to have a '
pop_property()' macro so this could be simplified to:while (prop = pop_child_node(&node->proplist)) free(prop);But then on the other hand, most of the list management is open coded in dtc. It would be nice to use the
list_headmacros like used in the kernel, but that is probably work for another day. (if it is even worth the effort).livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 193
Quoted 12 lines
livetree.c +12
-0+ +/* Free up the node's children */ +/* This assumes we have a tree and not a cylic or acylic graph */ +while (node->children) { +/* Pop the child node off the list */ + child = node->children; + node->children = child->next_sibling; + child->parent =NULL ; + child->next_sibling =NULL ; + + child =remove_nodes (child); + }Ditto here, except as
pop_child_node().Quoted 14 lines
livetree.c +14
-0+ +/* Remove the node from it's parent's child list */ +if (node->parent) { +int found =0 ; + + parent = node->parent; + +/* if the node is the first child ... */ +if (parent->children == node) { + parent->children = node->next_sibling; + found =1 ; + }else { +for_each_child (parent, child) { +if (child->next_sibling == node) {livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 217
Quoted 10 lines
livetree.c +10
-0+ child->next_sibling = node->next_sibling; + found =1 ; +break ; + } + } + } +assert (found);/* Woah! We've got an inconsistent tree here */ + node->parent =NULL ; + node->next_sibling =NULL ; + }This hunk is redundantly located because recursive calls have already removed the node from the parent. Should split into a separate function called by dtc-parse.y so that the recursive bit is simpler.
livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 222
Quoted 5 lines
livetree.c +5
-0+ +free (node); + +return NULL ; +}This block looks correct to me. It seemed a little verbose, but when I went through it line by line I saw that it pretty much needs to be that way. Especially with the list operations being open coded.
Quoted 25 lines
livetree.c +16
-0+ +struct node *replace_nodes (struct node *old_node,struct node *new_node) +{ +struct label *l; + +/* Add old node labels to new node, so we can still refer to the +* new node by the old labels. */ +for_each_label (old_node->labels, l) +add_label (&new_node->labels, l->label); + +/* Free up the old_node */ +remove_nodes (old_node); + +return new_node; +} +struct node *chain_node (struct node *first,struct node *list) {assert (first->next_sibling ==NULL );_______________________________________________
devicetree-discuss mailing list devicetree-discuss-uLR06cmDAlY/[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
Quoted 1 line · John Bonesio
On Fri, 2010-10-15 at 20:35 -0600, Grant Likely wrote:
[snip]
Quoted 22 lines
Quoted 15 lines
dtc-parser.y devicetree: · line 167
Quoted 11 lines
dtc-parser.y +10
-0+ | devicetree DT_REMOVENODE DT_REF ';' + { + struct node *target; + + target = get_node_by_label($1, $3); + if (target) + remove_nodes(target); + else + yyerror("label, '%s' does not exist in" + " node removal", $3); ;This handles the syntax at the top level, but doesn't deal with the /*-node/ keywords when they are within the overlay tree.
Quoted 6 lines
Also, I don't know if you'll be able to handle it in the same way when it comes to handling the new tokens embedded within an overlay tree because it isn't so easy to resolve the node to be removed. In order to handle both cases in the same way, it might be better to set a 'replace' flag in struct node that defers the actual remove of the original node until
merge_nodes()time.
Quoted 4 lines
Ok, so maybe I misunderstood. I thought we had decided that node modifications would only happen at the top level, but now that I'm looking back through your examples earlier today, and I see this isn't true.
Um.. that part of the discussion got confusing. I think only allowing the operations (and I still don't think the set implemented here are quite right, though they're close) at the top-level is a better idea.
Grant, is there a reason you think we need to have replacement as an option within each tree, rather than a "per-overlay-sheet" option.
Signature · 4 lines
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
Quoted 30 lines · David Gibson
Quoted 1 line · John Bonesio
On Fri, 2010-10-15 at 20:35 -0600, Grant Likely wrote:
[snip]
Quoted 22 lines
Quoted 15 lines
dtc-parser.y devicetree: · line 167
Quoted 11 lines
dtc-parser.y +10
-0+ | devicetree DT_REMOVENODE DT_REF ';' + { + struct node *target; + + target = get_node_by_label($1, $3); + if (target) + remove_nodes(target); + else + yyerror("label, '%s' does not exist in" + " node removal", $3); ;This handles the syntax at the top level, but doesn't deal with the /*-node/ keywords when they are within the overlay tree.
Quoted 6 lines
Also, I don't know if you'll be able to handle it in the same way when it comes to handling the new tokens embedded within an overlay tree because it isn't so easy to resolve the node to be removed. In order to handle both cases in the same way, it might be better to set a 'replace' flag in struct node that defers the actual remove of the original node until
merge_nodes()time.Quoted 4 lines
Ok, so maybe I misunderstood. I thought we had decided that node modifications would only happen at the top level, but now that I'm looking back through your examples earlier today, and I see this isn't true.
Quoted 6 lines
Um.. that part of the discussion got confusing. I think only allowing the operations (and I still don't think the set implemented here are quite right, though they're close) at the top-level is a better idea.
Grant, is there a reason you think we need to have replacement as an option within each tree, rather than a "per-overlay-sheet" option.
Yes, because I need the ability to remove/replace a child of a labelled node. Not every node that needs to be manipulated will necessarily have a label.
For example; I could have a labelled spi bus node with child nodes for each of the spi devices. I want to be able to remove/replace the child nodes without having to label them first.
g.
A message that is not in the archive (sent off-list or not yet received): 20101016081012.GH653-MrY2KI0G/OVr83L8+7iqerDks+cytr/[email protected]
Quoted 3 lines · Grant Likely
Quoted 2 lines · David Gibson
Quoted 1 line · John Bonesio
On Fri, 2010-10-15 at 20:35 -0600, Grant Likely wrote:
[snip]
Quoted 12 lines
Quoted 11 lines
Quoted 4 lines
Ok, so maybe I misunderstood. I thought we had decided that node modifications would only happen at the top level, but now that I'm looking back through your examples earlier today, and I see this isn't true.
Um.. that part of the discussion got confusing. I think only allowing the operations (and I still don't think the set implemented here are quite right, though they're close) at the top-level is a better idea.
Grant, is there a reason you think we need to have replacement as an option within each tree, rather than a "per-overlay-sheet" option.
Quoted 7 lines
Yes, because I need the ability to remove/replace a child of a labelled node. Not every node that needs to be manipulated will necessarily have a label.
For example; I could have a labelled spi bus node with child nodes for each of the spi devices. I want to be able to remove/replace the child nodes without having to label them first.
Ah, I see. This could also be accomplished if we allowed overlays to be targeted at (label + relative path) which we were talking about anyway.
Signature · 4 lines
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
Quoted 42 lines · Grant Likely
Quoted 30 lines · David Gibson
Quoted 1 line · John Bonesio
On Fri, 2010-10-15 at 20:35 -0600, Grant Likely wrote:
[snip]
Quoted 22 lines
Quoted 15 lines
Review
Quoted 11 lines
(quoted diff) +10
-0+ | devicetree DT_REMOVENODE DT_REF ';' + { + struct node *target; + + target = get_node_by_label($1, $3); + if (target) + remove_nodes(target); + else + yyerror("label, '%s' does not exist in" + " node removal", $3); ;This handles the syntax at the top level, but doesn't deal with the /*-node/ keywords when they are within the overlay tree.
Quoted 6 lines
Also, I don't know if you'll be able to handle it in the same way when it comes to handling the new tokens embedded within an overlay tree because it isn't so easy to resolve the node to be removed. In order to handle both cases in the same way, it might be better to set a 'replace' flag in struct node that defers the actual remove of the original node until
merge_nodes()time.Quoted 4 lines
Ok, so maybe I misunderstood. I thought we had decided that node modifications would only happen at the top level, but now that I'm looking back through your examples earlier today, and I see this isn't true.
Quoted 6 lines
Um.. that part of the discussion got confusing. I think only allowing the operations (and I still don't think the set implemented here are quite right, though they're close) at the top-level is a better idea.
Grant, is there a reason you think we need to have replacement as an option within each tree, rather than a "per-overlay-sheet" option.
Yes, because I need the ability to remove/replace a child of a labelled node. Not every node that needs to be manipulated will necessarily have a label.
Quoted 6 lines
For example; I could have a labelled spi bus node with child nodes for each of the spi devices. I want to be able to remove/replace the child nodes without having to label them first.
g.
Why not just add the label to the node you need first?
- John
Quoted 19 lines · John Bonesio
Quoted 18 lines · Grant Likely
Quoted 6 lines · David Gibson
Quoted 4 lines · John Bonesio
Ok, so maybe I misunderstood. I thought we had decided that node modifications would only happen at the top level, but now that I'm looking back through your examples earlier today, and I see this isn't true.
Quoted 6 lines
Um.. that part of the discussion got confusing. I think only allowing the operations (and I still don't think the set implemented here are quite right, though they're close) at the top-level is a better idea.
Grant, is there a reason you think we need to have replacement as an option within each tree, rather than a "per-overlay-sheet" option.
Yes, because I need the ability to remove/replace a child of a labelled node. Not every node that needs to be manipulated will necessarily have a label.
Quoted 8 lines
Quoted 6 lines
For example; I could have a labelled spi bus node with child nodes for each of the spi devices. I want to be able to remove/replace the child nodes without having to label them first.
g.
Why not just add the label to the node you need first?
That may not be possible and/or desirable. For instance, when the tree is extracted from /proc. Or if the base tree in stored in the kernel source and an external tool is modifying it for a specific purpose (when unable to patch the kernel tree). Or for generated device trees from the Xilinx toolchain for FPGAs. Modifying the source data isn't always an option.
Plus I want the ability to logically group related changes. ie. delete a device and add a device to the same bus at the same time. Or delete a phy node, add a replacement phy node at a different address, and update the ethernet phy-device phandle in the same block.
g.
A message that is not in the archive (sent off-list or not yet received): 20101016183217.GA3774-MrY2KI0G/OVr83L8+7iqerDks+cytr/[email protected]
Quoted 4 lines · Grant Likely
Quoted 3 lines · John Bonesio
Quoted 2 lines · Grant Likely
Quoted 1 line · David Gibson
On Fri, Oct 15, 2010 at 08:21:17PM -0700, John Bonesio wrote:
[snip]
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
For example; I could have a labelled spi bus node with child nodes for each of the spi devices. I want to be able to remove/replace the child nodes without having to label them first.
g.
Quoted 8 lines
Quoted 1 line
Why not just add the label to the node you need first?
That may not be possible and/or desirable. For instance, when the tree is extracted from /proc. Or if the base tree in stored in the kernel source and an external tool is modifying it for a specific purpose (when unable to patch the kernel tree). Or for generated device trees from the Xilinx toolchain for FPGAs. Modifying the source data isn't always an option.
You can apply your overlays by path in this case..
Quoted 5 lines
Plus I want the ability to logically group related changes. ie. delete a device and add a device to the same bus at the same time. Or delete a phy node, add a replacement phy node at a different address, and update the ethernet phy-device phandle in the same block.
Hrm, I really don't think this is a good argument. You're conflating the grouping of logically related changes with the tree structure which I think is a mistake. Putting the changes in the same file and/or adjusting your spacing and comments to show the logical grouping seems less likely to cause other problems.
Signature · 4 lines
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
Quoted 14 lines · David Gibson
Quoted 4 lines · Grant Likely
Quoted 3 lines · John Bonesio
Quoted 2 lines · Grant Likely
Quoted 1 line · David Gibson
On Fri, Oct 15, 2010 at 08:21:17PM -0700, John Bonesio wrote:
[snip]
Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
For example; I could have a labelled spi bus node with child nodes for each of the spi devices. I want to be able to remove/replace the child nodes without having to label them first.
g.
Why not just add the label to the node you need first?
Quoted 8 lines
Quoted 6 lines
That may not be possible and/or desirable. For instance, when the tree is extracted from /proc. Or if the base tree in stored in the kernel source and an external tool is modifying it for a specific purpose (when unable to patch the kernel tree). Or for generated device trees from the Xilinx toolchain for FPGAs. Modifying the source data isn't always an option.
You can apply your overlays by path in this case..
Right, but my argument is that in this case it is a useful syntax to be able to delve deep into the tree, or deeper off of a label and delete nodes within it. And it is actually a really simple thing to implement cleanly (as shown in the patch I posted).
Quoted 7 lines
Quoted 6 lines
Plus I want the ability to logically group related changes. ie. delete a device and add a device to the same bus at the same time. Or delete a phy node, add a replacement phy node at a different address, and update the ethernet phy-device phandle in the same block.
Quoted 5 lines
Hrm, I really don't think this is a good argument. You're conflating the grouping of logically related changes with the tree structure which I think is a mistake. Putting the changes in the same file and/or adjusting your spacing and comments to show the logical grouping seems less likely to cause other problems.
Though I'm not happy with it, I can live with only allowing removal of nodes at the top level. In fact, that constraint also completely removes the need for a /replace-node/ keyword since the remove operation is performed at the top level where ordering is explicitly allowed. So, instead of:
/{
node { oldprop; };
};
/{
/replace-node/ the-node { newprop; };
};do this instead:
/{
node { oldprop; };
};
/remove-node/ &{/the-node};
/{
the-node { newprop; };
};It means only the /remove-node/ keyword needs to be defined. However, it does mean that the syntax for full path references must be defined immediately before it is useful.
g.
A message that is not in the archive (sent off-list or not yet received): 20101018160115.GN19399-MrY2KI0G/OVr83L8+7iqerDks+cytr/[email protected]
Quoted 8 lines · On Mon, 18 Oct 2010 10:01:15 -0600 Grant Likely…
Quoted 7 lines · David Gibson
Quoted 5 lines · Grant Likely
Plus I want the ability to logically group related changes. ie. delete a device and add a device to the same bus at the same time. Or delete a phy node, add a replacement phy node at a different address, and update the ethernet phy-device phandle in the same block.
Quoted 5 lines
Quoted 5 lines
Hrm, I really don't think this is a good argument. You're conflating the grouping of logically related changes with the tree structure which I think is a mistake. Putting the changes in the same file and/or adjusting your spacing and comments to show the logical grouping seems less likely to cause other problems.
Shouldn't the tree structure, if practical, represent the logical structure? The actual operations (as the user intends them) are on particular properties or subnodes, not on top-level trees.
Likewise with the extension of nodes being unordered in the output tree (or allowing forward references) to disallowing any situation where ordering of dts statements matters. It seems to be sticking with a certain design idea just for the sake of it (or possibly for the convenience of the existing dtc implementation?), at the cost of readability/intuitiveness.
It's not an unbearable cost, but I wonder what we really gain from it.
Quoted 23 lines
Though I'm not happy with it, I can live with only allowing removal of nodes at the top level. In fact, that constraint also completely removes the need for a /replace-node/ keyword since the remove operation is performed at the top level where ordering is explicitly allowed. So, instead of:
/{ node { oldprop; }; }; /{ /replace-node/ the-node { newprop; }; };do this instead:
/{ node { oldprop; }; }; /remove-node/ &{/the-node}; /{ the-node { newprop; }; };
Quoted 1 line
It means only the /remove-node/ keyword needs to be defined.
What about property removal? Or do you mean the only one related to nodes?
-Scott
A message that is not in the archive (sent off-list or not yet received): 20101018124220.50a4c097-N/[email protected]
Quoted 21 lines · Scott Wood
Quoted 8 lines · On Mon, 18 Oct 2010 10:01:15 -0600 Grant Likely…
Quoted 7 lines · David Gibson
Quoted 5 lines · Grant Likely
Plus I want the ability to logically group related changes. ie. delete a device and add a device to the same bus at the same time. Or delete a phy node, add a replacement phy node at a different address, and update the ethernet phy-device phandle in the same block.
Quoted 5 lines
Quoted 5 lines
Hrm, I really don't think this is a good argument. You're conflating the grouping of logically related changes with the tree structure which I think is a mistake. Putting the changes in the same file and/or adjusting your spacing and comments to show the logical grouping seems less likely to cause other problems.
Shouldn't the tree structure, if practical, represent the logical structure? The actual operations (as the user intends them) are on particular properties or subnodes, not on top-level trees.
Quoted 6 lines
Likewise with the extension of nodes being unordered in the output tree (or allowing forward references) to disallowing any situation where ordering of dts statements matters. It seems to be sticking with a certain design idea just for the sake of it (or possibly for the convenience of the existing dtc implementation?), at the cost of readability/intuitiveness.
The current structure of dtc is allowed to be quite simple because it builds up each top level tree in memory and then merge them wholesale. It doesn't have to merge at two levels (ie. merge each node as it is added, and then merge the trees again at the top level), and the parser doesn't need to have any context about the rest of the tree until the trees get merged.
It really does keep things simple without being too onerous on the user. At the very least, I don't have any use cases that require the ability to reference the same node twice from within a single top level tree.
Quoted 25 lines
It's not an unbearable cost, but I wonder what we really gain from it.
Quoted 23 lines
Though I'm not happy with it, I can live with only allowing removal of nodes at the top level. In fact, that constraint also completely removes the need for a /replace-node/ keyword since the remove operation is performed at the top level where ordering is explicitly allowed. So, instead of:
/{ node { oldprop; }; }; /{ /replace-node/ the-node { newprop; }; };do this instead:
/{ node { oldprop; }; }; /remove-node/ &{/the-node}; /{ the-node { newprop; }; };
Quoted 4 lines
Quoted 1 line
It means only the /remove-node/ keyword needs to be defined.
What about property removal? Or do you mean the only one related to nodes?
Property removal will have its own syntax because it isn't the same thing, and properties live in a different namespace from nodes. Also, the use case is simpler because 'extend' isn't assumed for properties like it is for nodes. Properties can either be replaced, or removed outright. If we ever do have an extend syntax it will mostly likely just be another form of replace which a syntax to pull in parts of the old value (or parts of another properties value).
So anyway, the property removal syntax will be implemented in a separate patch.
g.
A message that is not in the archive (sent off-list or not yet received): 20101018195629.GB2259-MrY2KI0G/OVr83L8+7iqerDks+cytr/[email protected]
Quoted 3 lines · Grant Likely
Quoted 2 lines · Scott Wood
On Mon, 18 Oct 2010 10:01:15 -0600
Grant Likely <grant.likely-s3s/[email protected]> wrote:
[snip]
Quoted 14 lines
Quoted 6 lines
Likewise with the extension of nodes being unordered in the output tree (or allowing forward references) to disallowing any situation where ordering of dts statements matters. It seems to be sticking with a certain design idea just for the sake of it (or possibly for the convenience of the existing dtc implementation?), at the cost of readability/intuitiveness.
The current structure of dtc is allowed to be quite simple because it builds up each top level tree in memory and then merge them wholesale. It doesn't have to merge at two levels (ie. merge each node as it is added, and then merge the trees again at the top level), and the parser doesn't need to have any context about the rest of the tree until the trees get merged.
Quoted 4 lines
It really does keep things simple without being too onerous on the user. At the very least, I don't have any use cases that require the ability to reference the same node twice from within a single top level tree.
It does simplify the implementation a little, but that's not the real reason for it. The main reason is that:
somebus {
ethernet@100100 {
reg = <0x100100>;
...
};
ethernet@100100 {
reg = <0x100200>;
...
};
};Is probably just a mistake, forgetting to adjust unit address when they copy/pasted for the second ethernet. But if we allowed merging/overriding everywhere, not just at the top level it would be silently accepted, clobbering most of the first ethernet node's info with stuff intended for the second.
Allowing the extension only at the top level makes the user do something explicit to signal that they intend to update/override existing parts of the tree.
Quoted 30 lines
Quoted 25 lines
It's not an unbearable cost, but I wonder what we really gain from it.
Quoted 23 lines
Though I'm not happy with it, I can live with only allowing removal of nodes at the top level. In fact, that constraint also completely removes the need for a /replace-node/ keyword since the remove operation is performed at the top level where ordering is explicitly allowed. So, instead of:
/{ node { oldprop; }; }; /{ /replace-node/ the-node { newprop; }; };do this instead:
/{ node { oldprop; }; }; /remove-node/ &{/the-node}; /{ the-node { newprop; }; };Quoted 4 lines
Quoted 1 line
It means only the /remove-node/ keyword needs to be defined.
What about property removal? Or do you mean the only one related to nodes?
Quoted 7 lines
Property removal will have its own syntax because it isn't the same thing, and properties live in a different namespace from nodes. Also, the use case is simpler because 'extend' isn't assumed for properties like it is for nodes. Properties can either be replaced, or removed outright. If we ever do have an extend syntax it will mostly likely just be another form of replace which a syntax to pull in parts of the old value (or parts of another properties value).
Right. I think property value expressions are pretty much a prerequisite for a sane implementation of this.
Quoted 2 lines
So anyway, the property removal syntax will be implemented in a separate patch.
I'd actually prefer to see that patch come first, I have far less doubts about how best to implement that than I do with the node removing/replacing stuff.
Specifically, I'd happily ack a patch which implemented
someprop = /undef-prop/;
immediately.
Signature · 4 lines
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
Quoted 3 lines · David Gibson
Specifically, I'd happily ack a patch which implemented someprop = /undef-prop/;
immediately.
I would to.
g.
Quoted 22 lines · Grant Likely
Quoted 21 lines · David Gibson
Quoted 4 lines · Grant Likely
Quoted 3 lines · John Bonesio
Quoted 2 lines · Grant Likely
Quoted 1 line · David Gibson
On Fri, Oct 15, 2010 at 08:21:17PM -0700, John Bonesio wrote:
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 5 lines
For example; I could have a labelled spi bus node with child nodes for each of the spi devices. I want to be able to remove/replace the child nodes without having to label them first.
g.
Why not just add the label to the node you need first?
That may not be possible and/or desirable. For instance, when the tree is extracted from /proc. Or if the base tree in stored in the kernel source and an external tool is modifying it for a specific purpose (when unable to patch the kernel tree). Or for generated device trees from the Xilinx toolchain for FPGAs. Modifying the source data isn't always an option.
Quoted 6 lines
Quoted 1 line
You can apply your overlays by path in this case..
Right, but my argument is that in this case it is a useful syntax to be able to delve deep into the tree, or deeper off of a label and delete nodes within it. And it is actually a really simple thing to implement cleanly (as shown in the patch I posted).
Oh, it's perfectly easy to implement, yes. It's the step up in complexity of the mental model required that concerns me about it (now each overlay in the stack can have some "transparent" and some "opaque" parts).
Signature · 4 lines
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
On Mon, Oct 18, 2010 at 10:01:15AM -0600, Grant Likely wrote:
[snip]
Quoted 3 lines
It means only the /remove-node/ keyword needs to be defined. However, it does mean that the syntax for full path references must be defined immediately before it is useful.
Actually, because of the way DT_REF is lexed, I believe
&{/some/path} {
prop = "new-value";
};Will work already.
Or do you mean implementing references to a label + relative path?
Signature · 4 lines
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
Quoted 8 lines · David Gibson
On Mon, Oct 18, 2010 at 10:01:15AM -0600, Grant Likely wrote:
[snip]Quoted 3 lines
It means only the /remove-node/ keyword needs to be defined. However, it does mean that the syntax for full path references must be defined immediately before it is useful.
Actually, because of the way
DT_REFis lexed, I believe
Quoted 7 lines
&{/some/path} {
prop = "new-value";
};Will work already.
Or do you mean implementing references to a label + relative path?
yes.
Quoted 24 lines · Grant Likely
Hi John.
Quoted 7 lines · John Bonesio
This is interesting when the /include/ "<filename>" feature is used. This way we can create base device tree source files for a family of systems and modify the device tree for a specific system.
The current sytem allows an existing node to be extended with new properties and subnodes.
Quoted 7 lines
The new features allow an existing node to be replaced completely by the new properties and subnodes. The new features also allow an existing node to be deleted.
This patch does not yet provide a way to remove properties.
- John
Also need s-o-b line, and anything that doesn't belong as part of the commit message, such as '- John', needs to be below the '---' line so that git will ignore it.
The patch looks pretty good, but I cannot comment fully until the keyword handling works when deeper in the tree (see below).
[snip]
Quoted 15 lines
livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 181
Quoted 11 lines
+ struct property *prop;
+ struct node *child, *parent;livetree.c +9
-0+ + while (node->proplist) { + /* Pop the property off the list */ + prop = node->proplist; + node->proplist = prop->next; + prop->next = NULL; + + free(prop); + }It would be useful to have a '
pop_property()' macro so this could be simplified to:
Quoted 7 lines
while (prop = pop_child_node(&node->proplist)) free(prop);But then on the other hand, most of the list management is open coded in dtc. It would be nice to use the
list_headmacros like used in the kernel, but that is probably work for another day. (if it is even worth the effort).
I had a look into this at one point. The way we build up the tree in a kinda-sorta functional programming like way in dts parsing actually makes using the list.h form kind of hard. At least without a fancier allocator (maybe I should bite the bullet and do a talloc conversion).
Signature · 4 lines
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
Quoted 4 lines · John Bonesio
This is interesting when the /include/ "<filename>" feature is used. This way we can create base device tree source files for a family of systems and modify the device tree for a specific system.
Quoted 8 lines
The current sytem allows an existing node to be extended with new properties and subnodes.
The new features allow an existing node to be replaced completely by the new properties and subnodes. The new features also allow an existing node to be deleted.
This patch does not yet provide a way to remove properties.
Detailed comments below.
Quoted 11 lines
dtc-lexer.l | 12 ++++++++++ dtc-parser.y | 24 ++++++++++++++++++++ dtc.h | 2 ++ livetree.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 0 deletions(-)dtc-lexer.l +0
-0
dtc-lexer.l static int pop_input_file(void); · line 103
Quoted 8 lines
dtc-lexer.l +5
-0@@ -96,6 +96,18 @@ static int pop_input_file(void); return DT_MEMRESERVE; } +<*>"/trim-node/" { + DPRINT("Keyword: /trim-node/\n"); + BEGIN_DEFAULT(); + return DT_REPLACENODE; + }
Hrm. The keyword is "/trim-node/", but the C symbol is REPLACENODE. Potentially confusing.
Also, while "trim-node" seemed a reasonable option to me in some contexts, with the semantics implemented in this patch, I think "replace-node" would be far clearer.
Quoted 90 lines
dtc-lexer.l +6
-0+<*>"/remove-node/" { + DPRINT("Keyword: /remove-node/\n"); + BEGIN_DEFAULT(); + return DT_REMOVENODE; + } + <*>{LABEL}: { DPRINT("Label: %s\n", yytext); yylval.labelref = xstrdup(yytext);dtc-parser.y +24
-0@@ -54,6 +54,8 @@ static unsigned long long eval_literal(const char *s, int base, int bits); %token DT_V1 %token DT_MEMRESERVE +%token DT_REPLACENODE +%token DT_REMOVENODE %token <propnodename> DT_PROPNODENAME %token <literal> DT_LITERAL %token <cbase> DT_BASE@@ -140,6 +142,28 @@ devicetree: " node extension", $2); $$ = $1; } + | devicetree DT_REPLACENODE DT_REF nodedef + { + struct node *target; + + target = get_node_by_label($1, $3); + if (target) + replace_nodes(target, $4); + else + yyerror("label, '%s' does not exist in" + " node redefinition", $3); + $$ = $1; + } + | devicetree DT_REMOVENODE DT_REF ';' + { + struct node *target; + + target = get_node_by_label($1, $3); + if (target) + remove_nodes(target); + else + yyerror("label, '%s' does not exist in" + " node removal", $3); ; nodedef:dtc.h +2
-0@@ -175,6 +175,8 @@ struct node *build_node(struct property *proplist, struct node *children);struct node *name_node (struct node *node,char *name);struct node *chain_node (struct node *first,struct node *list);struct node *merge_nodes (struct node *old_node,struct node *new_node); +struct node *replace_nodes (struct node *old_node,struct node *new_node); +struct node *remove_nodes (struct node *node);void add_property (struct node *node,struct property *prop);void add_child (struct node *parent,struct node *child);livetree.c +18
-0@@ -167,6 +167,75 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)return old_node; } +struct node *remove_nodes (struct node *node) { +struct property *prop; +struct node *child, *parent; + +while (node->proplist) { +/* Pop the property off the list */ + prop = node->proplist; + node->proplist = prop->next; + prop->next =NULL ; + +free (prop); + } + +/* Free up the node's children */ +/* This assumes we have a tree and not a cylic or acylic graph */ +while (node->children) { +/* Pop the child node off the list */ + child = node->children;
livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 197
Quoted 10 lines
livetree.c +10
-0+ node->children = child->next_sibling; + child->parent =NULL ; + child->next_sibling =NULL ; + + child =remove_nodes (child); + } + +/* Remove the node from it's parent's child list */ +if (node->parent) { +int found =0 ;
This should go first, before the recursive free()ing. I know we don't have any concurrency to worry about in dtc, but as a general habit it should always be remove from the structure first, then do the disposal.
And then, don't bother with the disposal. I'm pretty sure we already have some leaks and I really don't care. dtc is a strictly transient process, so we can basically treating malloc() as a pool allocator with exactly one pool, freed at exit. If we do need something more subtle infuture, we can use an actual pool allocator (talloc, for preference). The fact that in dts parsing we build the tree from the leaves, but in dtb parsing we bulid it from the root makes lifetimes complex enough that fully correct explicit free()ing is actually quite a pain.
(Oh, and, in the case where you're removing, not replacing, you already leak the labels. See my point :)
Quoted 5 lines
livetree.c +5
-0+ parent = node->parent; + +/* if the node is the first child ... */ +if (parent->children == node) { + parent->children = node->next_sibling;
livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 213
Quoted 10 lines
livetree.c +10
-0+ found =1 ; + }else { +for_each_child (parent, child) { +if (child->next_sibling == node) { + child->next_sibling = node->next_sibling; + found =1 ; +break ; + } + } + }
You can do better:
ptr = &parent->children;
for_each_child(...) {
if (child == node) {
*ptr = child->next_sibling;
found = 1;
ptr = &child->next_sibling;
}
}This pointer-to-pointer idiom is used in a number of other places.
Quoted 8 lines
livetree.c +8
-0+assert (found);/* Woah! We've got an inconsistent tree here */ + node->parent =NULL ; + node->next_sibling =NULL ; + } + +free (node); + +return NULL ;
livetree.c struct node *merge_nodes(struct node *old_node, struct node *new_node) · line 231
Quoted 10 lines
livetree.c +10
-0+} + +struct node *replace_nodes (struct node *old_node,struct node *new_node) +{ +struct label *l; + +/* Add old node labels to new node, so we can still refer to the +* new node by the old labels. */ +for_each_label (old_node->labels, l) +add_label (&new_node->labels, l->label);
Yeah. labels to subnodes of the original still disappear without trace, and could therefore be re-used. I think this has the potential to be very confusing. We need to be more careful with the label semantics.
Quoted 15 lines
livetree.c +6
-0+/* Free up the old_node */ +remove_nodes (old_node); + +return new_node; +} +struct node *chain_node (struct node *first,struct node *list) {assert (first->next_sibling ==NULL );_______________________________________________
devicetree-discuss mailing list devicetree-discuss-uLR06cmDAlY/[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
Signature · 4 lines
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