* [RFC] Allow device tree to be modified by additonal device tree sections
@ 2010-02-23 19:28 Grant Likely
2010-02-24 1:36 ` David Gibson
0 siblings, 1 reply; 5+ messages in thread
From: Grant Likely @ 2010-02-23 19:28 UTC (permalink / raw)
To: david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+, jdl-CYoMK+44s/E,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ
This patch allows the following construct:
/ {
property-a = "old";
property-b = "does not change";
};
/ {
property-a = "changed";
property-c = "new";
node-a {
};
};
Where the later device tree overrides the properties found in the
earlier tree. This is useful for laying down a template device tree
in an include file and modifying it for a specific board without having
to clone the entire tree.
Signed-off-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---
I haven't extensively tested this patch yet, and I haven't figured out yet
how to properly write the test cases, but I want to get this out there to
make sure I'm taking the right approach.
Cheers,
g.
dtc-parser.y | 14 ++++++++-
dtc.h | 1 +
livetree.c | 63 ++++++++++++++++++++++++++++++++++++++++
tests/redefine-nodes-test1.dts | 8 +++++
tests/redefine-nodes-test2.dts | 9 ++++++
tests/redefine-nodes-test3.dts | 9 ++++++
tests/redefine-nodes-test4.dts | 11 +++++++
tests/redefine-nodes-test5.dts | 9 ++++++
tests/redefine-nodes-test6.dts | 11 +++++++
tests/redefine-nodes-test7.dts | 13 ++++++++
10 files changed, 147 insertions(+), 1 deletions(-)
create mode 100644 tests/redefine-nodes-test1.dts
create mode 100644 tests/redefine-nodes-test2.dts
create mode 100644 tests/redefine-nodes-test3.dts
create mode 100644 tests/redefine-nodes-test4.dts
create mode 100644 tests/redefine-nodes-test5.dts
create mode 100644 tests/redefine-nodes-test6.dts
create mode 100644 tests/redefine-nodes-test7.dts
diff --git a/dtc-parser.y b/dtc-parser.y
index bd9e097..8f5c4a3 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -75,6 +75,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
%type <proplist> proplist
%type <node> devicetree
+%type <node> devicetrees
%type <node> nodedef
%type <node> subnode
%type <nodelist> subnodes
@@ -83,7 +84,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
%%
sourcefile:
- DT_V1 ';' memreserves devicetree
+ DT_V1 ';' memreserves devicetrees
{
the_boot_info = build_boot_info($3, $4,
guess_boot_cpuid($4));
@@ -115,6 +116,17 @@ addr:
}
;
+devicetrees:
+ /* empty */
+ {
+ $$ = NULL;
+ }
+ | devicetree devicetrees
+ {
+ $$ = merge_nodes($1, $2);
+ }
+ ;
+
devicetree:
'/' nodedef
{
diff --git a/dtc.h b/dtc.h
index 5367198..815494a 100644
--- a/dtc.h
+++ b/dtc.h
@@ -164,6 +164,7 @@ struct property *reverse_properties(struct property *first);
struct node *build_node(struct property *proplist, struct node *children);
struct node *name_node(struct node *node, char *name, char *label);
struct node *chain_node(struct node *first, struct node *list);
+struct node *merge_nodes(struct node *old_node, struct node *new_node);
void add_property(struct node *node, struct property *prop);
void add_child(struct node *parent, struct node *child);
diff --git a/livetree.c b/livetree.c
index aa0edf1..0691599 100644
--- a/livetree.c
+++ b/livetree.c
@@ -89,6 +89,69 @@ struct node *name_node(struct node *node, char *name, char * label)
return node;
}
+struct node *merge_nodes(struct node *old_node, struct node *new_node)
+{
+ struct property *new_prop, *old_prop;
+ struct node *new_child, *old_child;
+
+ printf("Merge node, old_node:%s new_node:%s\n", old_node->name,
+ new_node ? new_node->name : "<NULL>" );
+ if (!new_node)
+ return old_node;
+
+ /* Move the override properties into the old node. If there
+ * is a collision, replace the old definition with the new */
+ while (new_node->proplist) {
+ /* Pop the property off the list */
+ new_prop = new_node->proplist;
+ new_node->proplist = new_prop->next;
+ new_prop->next = NULL;
+
+ /* Look for a collision, set new value if there is */
+ for_each_property(old_node, old_prop) {
+ if (strcmp(old_prop->name, new_prop->name) == 0) {
+ old_prop->val = new_prop->val;
+ free(new_prop);
+ new_prop = NULL;
+ break;
+ }
+ }
+
+ /* Assuming no collision, add the property to the old node. */
+ if (new_prop)
+ add_property(old_node, new_prop);
+ }
+
+ /* Move the override child nodes into the primary node. If
+ * there is a collision, then merge the nodes. */
+ while (new_node->children) {
+ /* Pop the child node off the list */
+ new_child = new_node->children;
+ new_node->children = new_child->next_sibling;
+ new_child->parent = NULL;
+ new_child->next_sibling = NULL;
+
+ /* Search for a collision. Merge if there is */
+ for_each_child(old_node, old_child) {
+ if (strcmp(old_child->name, new_child->name) == 0) {
+ merge_nodes(old_child, new_child);
+ new_child = NULL;
+ break;
+ }
+ }
+
+ /* Assuming no collision, add the child to the old node. */
+ if (new_child)
+ add_child(old_node, new_child);
+ }
+
+ /* The new node contents are now merged into the old node. Free
+ * the new node. */
+ free(new_node);
+
+ return old_node;
+}
+
struct node *chain_node(struct node *first, struct node *list)
{
assert(first->next_sibling == NULL);
diff --git a/tests/redefine-nodes-test1.dts b/tests/redefine-nodes-test1.dts
new file mode 100644
index 0000000..a9ccaa2
--- /dev/null
+++ b/tests/redefine-nodes-test1.dts
@@ -0,0 +1,8 @@
+/dts-v1/;
+
+/ {
+};
+
+/ {
+ prop = "new";
+};
diff --git a/tests/redefine-nodes-test2.dts b/tests/redefine-nodes-test2.dts
new file mode 100644
index 0000000..9e0b55e
--- /dev/null
+++ b/tests/redefine-nodes-test2.dts
@@ -0,0 +1,9 @@
+/dts-v1/;
+
+/ {
+ prop = "old";
+};
+
+/ {
+ prop = "new";
+};
diff --git a/tests/redefine-nodes-test3.dts b/tests/redefine-nodes-test3.dts
new file mode 100644
index 0000000..cb135f3
--- /dev/null
+++ b/tests/redefine-nodes-test3.dts
@@ -0,0 +1,9 @@
+/dts-v1/;
+
+/ {
+ prop1 = "old";
+};
+
+/ {
+ prop2 = "new";
+};
diff --git a/tests/redefine-nodes-test4.dts b/tests/redefine-nodes-test4.dts
new file mode 100644
index 0000000..a331c62
--- /dev/null
+++ b/tests/redefine-nodes-test4.dts
@@ -0,0 +1,11 @@
+/dts-v1/;
+
+/ {
+ prop1 = "olda";
+ prop2 = "oldb";
+ prop3 = "oldc";
+};
+
+/ {
+ prop2 = "new";
+};
diff --git a/tests/redefine-nodes-test5.dts b/tests/redefine-nodes-test5.dts
new file mode 100644
index 0000000..b12de06
--- /dev/null
+++ b/tests/redefine-nodes-test5.dts
@@ -0,0 +1,9 @@
+/dts-v1/;
+
+/ {
+};
+
+/ {
+ subnode {
+ };
+};
diff --git a/tests/redefine-nodes-test6.dts b/tests/redefine-nodes-test6.dts
new file mode 100644
index 0000000..de05b49
--- /dev/null
+++ b/tests/redefine-nodes-test6.dts
@@ -0,0 +1,11 @@
+/dts-v1/;
+
+/ {
+ subnode {
+ };
+};
+
+/ {
+ subnode {
+ };
+};
diff --git a/tests/redefine-nodes-test7.dts b/tests/redefine-nodes-test7.dts
new file mode 100644
index 0000000..7b9ab92
--- /dev/null
+++ b/tests/redefine-nodes-test7.dts
@@ -0,0 +1,13 @@
+/dts-v1/;
+
+/ {
+ subnode {
+ prop1 = "val1";
+ };
+};
+
+/ {
+ subnode {
+ prop2 = "val2";
+ };
+};
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [RFC] Allow device tree to be modified by additonal device tree sections
2010-02-23 19:28 [RFC] Allow device tree to be modified by additonal device tree sections Grant Likely
@ 2010-02-24 1:36 ` David Gibson
2010-02-24 2:05 ` Grant Likely
0 siblings, 1 reply; 5+ messages in thread
From: David Gibson @ 2010-02-24 1:36 UTC (permalink / raw)
To: Grant Likely; +Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ
On Tue, Feb 23, 2010 at 12:28:13PM -0700, Grant Likely wrote:
> This patch allows the following construct:
>
> / {
> property-a = "old";
> property-b = "does not change";
> };
>
> / {
> property-a = "changed";
> property-c = "new";
> node-a {
> };
> };
Heh. I'm glad I didn't far last night with my own implementation of
the concept.
> Where the later device tree overrides the properties found in the
> earlier tree. This is useful for laying down a template device tree
> in an include file and modifying it for a specific board without having
> to clone the entire tree.
>
> Signed-off-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> ---
>
> I haven't extensively tested this patch yet, and I haven't figured out yet
> how to properly write the test cases, but I want to get this out there to
> make sure I'm taking the right approach.
Ok, I have a test case from my start to this, which you can use. Your
dts files give a wider range of cases to check, but they'll also need
more test code to verify.
To add testcases, you basically just list them in run_tests.sh. For
simple things, like just invoking dtc, or checking things for which
there is already a test program, it may be sufficient just to add the
right things into there. For more complex and specific testing you'll
need to write a testcase binary, which should use the macros in
tests.h to indicate success or failure.
In this case, I think the simplest way to go is to write dts files
which use the merge functionality to create a tree identical to one of
the existing samples (like test_tree1.dts). Then the testcase
consists of two parts, one invoking dtc on the merge tree (to check
that it even processes it without error), then a second comparing the
output tree to the thing it's supposed to match.
We won't be able to use the existing dtbs_equal_ordered,
unfortunately, because the merge behaviour could do odd things to the
order. Still, I've been meaning to implement a dtbs_equal_unordered
for ages.
> diff --git a/dtc-parser.y b/dtc-parser.y
> index bd9e097..8f5c4a3 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -75,6 +75,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
> %type <proplist> proplist
>
> %type <node> devicetree
> +%type <node> devicetrees
> %type <node> nodedef
> %type <node> subnode
> %type <nodelist> subnodes
> @@ -83,7 +84,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
> %%
>
> sourcefile:
> - DT_V1 ';' memreserves devicetree
> + DT_V1 ';' memreserves devicetrees
> {
> the_boot_info = build_boot_info($3, $4,
> guess_boot_cpuid($4));
> @@ -115,6 +116,17 @@ addr:
> }
> ;
>
> +devicetrees:
> + /* empty */
We always want at least one device tree block, so the base case here
should be 'devicetree', rather than empty.
> + {
> + $$ = NULL;
> + }
> + | devicetree devicetrees
> + {
> + $$ = merge_nodes($1, $2);
> + }
> + ;
> +
> devicetree:
> '/' nodedef
> {
> diff --git a/dtc.h b/dtc.h
> index 5367198..815494a 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -164,6 +164,7 @@ struct property *reverse_properties(struct property *first);
> struct node *build_node(struct property *proplist, struct node *children);
> struct node *name_node(struct node *node, char *name, char *label);
> struct node *chain_node(struct node *first, struct node *list);
> +struct node *merge_nodes(struct node *old_node, struct node *new_node);
>
> void add_property(struct node *node, struct property *prop);
> void add_child(struct node *parent, struct node *child);
> diff --git a/livetree.c b/livetree.c
> index aa0edf1..0691599 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -89,6 +89,69 @@ struct node *name_node(struct node *node, char *name, char * label)
> return node;
> }
>
> +struct node *merge_nodes(struct node *old_node, struct node *new_node)
> +{
> + struct property *new_prop, *old_prop;
> + struct node *new_child, *old_child;
> +
> + printf("Merge node, old_node:%s new_node:%s\n", old_node->name,
> + new_node ? new_node->name : "<NULL>" );
There already exist some debug() macros in dtc.h for this sort of
message.
> + if (!new_node)
> + return old_node;
With the grammar change suggested above, you don't need this.
> + /* Move the override properties into the old node. If there
> + * is a collision, replace the old definition with the new */
> + while (new_node->proplist) {
> + /* Pop the property off the list */
> + new_prop = new_node->proplist;
> + new_node->proplist = new_prop->next;
> + new_prop->next = NULL;
> +
> + /* Look for a collision, set new value if there is */
> + for_each_property(old_node, old_prop) {
> + if (strcmp(old_prop->name, new_prop->name) == 0) {
> + old_prop->val = new_prop->val;
> + free(new_prop);
> + new_prop = NULL;
> + break;
I guess there should be a data_free() here of the old value. Mind you
memory management in dtc is already a bit of a mess. I've considered
moving it to talloc() (Tridge's nifty hierarchical pool allocator) or,
more controversially, just never bothering to free() anything on the
grounds that dtc processes are always shortlived anyway.
> + }
> + }
> +
> + /* Assuming no collision, add the property to the old node. */
> + if (new_prop)
> + add_property(old_node, new_prop);
> + }
> +
> + /* Move the override child nodes into the primary node. If
> + * there is a collision, then merge the nodes. */
> + while (new_node->children) {
> + /* Pop the child node off the list */
> + new_child = new_node->children;
> + new_node->children = new_child->next_sibling;
> + new_child->parent = NULL;
> + new_child->next_sibling = NULL;
> +
> + /* Search for a collision. Merge if there is */
> + for_each_child(old_node, old_child) {
> + if (strcmp(old_child->name, new_child->name)
> == 0) {
Ok, you should use the streq() macro instead of an explicit strcmp()
== 0. However, there's also a bigger question here. How precise do
we want the nodename matching to be. Should we be using the OF-style
matchin where you can omit the unit address when it's unambiguous.
i.e. Should we allow:
/ {
...
somebus@1234 {
widget@17 {
};
};
};
/ {
...
somebus {
widget {
new-property;
};
};
};
> + merge_nodes(old_child, new_child);
> + new_child = NULL;
> + break;
> + }
> + }
> +
> + /* Assuming no collision, add the child to the old node. */
> + if (new_child)
> + add_child(old_node, new_child);
> + }
> +
> + /* The new node contents are now merged into the old node. Free
> + * the new node. */
> + free(new_node);
> +
> + return old_node;
> +}
[snip]
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC] Allow device tree to be modified by additonal device tree sections
2010-02-24 1:36 ` David Gibson
@ 2010-02-24 2:05 ` Grant Likely
[not found] ` <fa686aa41002231805q1f5c6179j6fb7f6cd0558972e-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: Grant Likely @ 2010-02-24 2:05 UTC (permalink / raw)
To: David Gibson; +Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ
On Tue, Feb 23, 2010 at 6:36 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Tue, Feb 23, 2010 at 12:28:13PM -0700, Grant Likely wrote:
>> This patch allows the following construct:
>>
>> / {
>> property-a = "old";
>> property-b = "does not change";
>> };
>>
>> / {
>> property-a = "changed";
>> property-c = "new";
>> node-a {
>> };
>> };
>
> Heh. I'm glad I didn't far last night with my own implementation of
> the concept.
:-)
>> Where the later device tree overrides the properties found in the
>> earlier tree. This is useful for laying down a template device tree
>> in an include file and modifying it for a specific board without having
>> to clone the entire tree.
>>
>> Signed-off-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>> ---
>>
>> I haven't extensively tested this patch yet, and I haven't figured out yet
>> how to properly write the test cases, but I want to get this out there to
>> make sure I'm taking the right approach.
>
> Ok, I have a test case from my start to this, which you can use. Your
> dts files give a wider range of cases to check, but they'll also need
> more test code to verify.
>
> To add testcases, you basically just list them in run_tests.sh. For
> simple things, like just invoking dtc, or checking things for which
> there is already a test program, it may be sufficient just to add the
> right things into there. For more complex and specific testing you'll
> need to write a testcase binary, which should use the macros in
> tests.h to indicate success or failure.
>
> In this case, I think the simplest way to go is to write dts files
> which use the merge functionality to create a tree identical to one of
> the existing samples (like test_tree1.dts). Then the testcase
> consists of two parts, one invoking dtc on the merge tree (to check
> that it even processes it without error), then a second comparing the
> output tree to the thing it's supposed to match.
>
> We won't be able to use the existing dtbs_equal_ordered,
> unfortunately, because the merge behaviour could do odd things to the
> order. Still, I've been meaning to implement a dtbs_equal_unordered
> for ages.
>
>> diff --git a/dtc-parser.y b/dtc-parser.y
>> index bd9e097..8f5c4a3 100644
>> --- a/dtc-parser.y
>> +++ b/dtc-parser.y
>> @@ -75,6 +75,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
>> %type <proplist> proplist
>>
>> %type <node> devicetree
>> +%type <node> devicetrees
>> %type <node> nodedef
>> %type <node> subnode
>> %type <nodelist> subnodes
>> @@ -83,7 +84,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
>> %%
>>
>> sourcefile:
>> - DT_V1 ';' memreserves devicetree
>> + DT_V1 ';' memreserves devicetrees
>> {
>> the_boot_info = build_boot_info($3, $4,
>> guess_boot_cpuid($4));
>> @@ -115,6 +116,17 @@ addr:
>> }
>> ;
>>
>> +devicetrees:
>> + /* empty */
>
> We always want at least one device tree block, so the base case here
> should be 'devicetree', rather than empty.
Okay, I'll change this. I'm unclear however, if a single 'devicetree'
can get promoted up to a 'devicetrees', then will that cause problems
with the 'devicetree devicetrees' rule because the stack will contain
a 'devicetrees' when the rule wants a 'devicetree' first?
>> + printf("Merge node, old_node:%s new_node:%s\n", old_node->name,
>> + new_node ? new_node->name : "<NULL>" );
>
> There already exist some debug() macros in dtc.h for this sort of
> message.
I hadn't meant to leave this in. It's already gone.
>
>> + if (!new_node)
>> + return old_node;
>
> With the grammar change suggested above, you don't need this.
Cool.
>
>> + /* Move the override properties into the old node. If there
>> + * is a collision, replace the old definition with the new */
>> + while (new_node->proplist) {
>> + /* Pop the property off the list */
>> + new_prop = new_node->proplist;
>> + new_node->proplist = new_prop->next;
>> + new_prop->next = NULL;
>> +
>> + /* Look for a collision, set new value if there is */
>> + for_each_property(old_node, old_prop) {
>> + if (strcmp(old_prop->name, new_prop->name) == 0) {
>> + old_prop->val = new_prop->val;
>> + free(new_prop);
>> + new_prop = NULL;
>> + break;
>
> I guess there should be a data_free() here of the old value. Mind you
> memory management in dtc is already a bit of a mess. I've considered
> moving it to talloc() (Tridge's nifty hierarchical pool allocator) or,
> more controversially, just never bothering to free() anything on the
> grounds that dtc processes are always shortlived anyway.
Okay, think about it and let me know what you want me to do here.
>> + }
>> + }
>> +
>> + /* Assuming no collision, add the property to the old node. */
>> + if (new_prop)
>> + add_property(old_node, new_prop);
>> + }
>> +
>> + /* Move the override child nodes into the primary node. If
>> + * there is a collision, then merge the nodes. */
>> + while (new_node->children) {
>> + /* Pop the child node off the list */
>> + new_child = new_node->children;
>> + new_node->children = new_child->next_sibling;
>> + new_child->parent = NULL;
>> + new_child->next_sibling = NULL;
>> +
>> + /* Search for a collision. Merge if there is */
>> + for_each_child(old_node, old_child) {
>> + if (strcmp(old_child->name, new_child->name)
>> == 0) {
>
> Ok, you should use the streq() macro instead of an explicit strcmp()
> == 0.
okay.
> However, there's also a bigger question here. How precise do
> we want the nodename matching to be. Should we be using the OF-style
> matchin where you can omit the unit address when it's unambiguous.
>
> i.e. Should we allow:
>
> / {
> ...
> somebus@1234 {
> widget@17 {
> };
> };
> };
>
> / {
> ...
> somebus {
> widget {
> new-property;
> };
> };
> };
>
My opinion: no way. We're not working with real OF, and none of the
use cases I see have any need of such a feature. While we could
forbid it now and permit it later, we cannot permit it now and forbid
it later. I say no until someone comes up with a reasonable use case
on why it should be implemented.
g.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-02-24 4:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-23 19:28 [RFC] Allow device tree to be modified by additonal device tree sections Grant Likely
2010-02-24 1:36 ` David Gibson
2010-02-24 2:05 ` Grant Likely
[not found] ` <fa686aa41002231805q1f5c6179j6fb7f6cd0558972e-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-02-24 4:42 ` David Gibson
2010-02-24 4:57 ` Grant Likely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox