[PATCH 1/2] Implements new features for updating existing device tree nodes.

[PATCH 1/2] Implements new features for updating existing device tree nodes.

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

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.

Signed-off-by: John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

I was originally going to submit just one patch, as the change in the
second one is pretty trivial. However, I thought people might appreciate
that the change to refer to nodes by path is highlighted separately.

Comment away!

- John

 dtc-lexer.l  |    6 ++++++
 dtc-parser.y |   14 ++++++++++++--
 dtc.h        |    3 +++
 livetree.c   |   25 +++++++++++++++++++++++++
 4 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/dtc-lexer.l b/dtc-lexer.l
index 081e13a..80a886a 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -96,6 +96,12 @@ static int pop_input_file(void);
 			return DT_MEMRESERVE;
 		}
 
+<*>"/remove-node/"	{
+			DPRINT("Keyword: /remove-node/\n");
+			BEGIN_DEFAULT();
+			return DT_REMOVENODE;
+		}
+
 <*>{LABEL}:	{
 			DPRINT("Label: %s\n", yytext);
 			yylval.labelref = xstrdup(yytext);
diff --git a/dtc-parser.y b/dtc-parser.y
index e1846d4..56d7789 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -55,6 +55,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
 
 %token DT_V1
 %token DT_MEMRESERVE
+%token DT_REMOVENODE
 %token <propnodename> DT_PROPNODENAME
 %token <literal> DT_LITERAL
 %token <cbase> DT_BASE
@@ -137,10 +138,19 @@ devicetree:
 			if (target)
 				merge_nodes(target, $3);
 			else
-				print_error("label, '%s' does not exist in"
-					" node extension", $2);
+				print_error("label, '%s' not found", $2);
 			$$ = $1;
 		}
+	| devicetree DT_REMOVENODE DT_REF ';'
+		{
+			struct node *target;
+
+			target = get_node_by_label($1, $3);
+			if (target)
+				remove_nodes(target);
+			else
+				print_error("label, '%s' not found", $3);
+		}
 	;
 
 nodedef:
diff --git a/dtc.h b/dtc.h
index b36ac5d..e7cb45c 100644
--- a/dtc.h
+++ b/dtc.h
@@ -175,9 +175,12 @@ 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);
+void remove_nodes(struct node *node);
 
 void add_property(struct node *node, struct property *prop);
 void add_child(struct node *parent, struct node *child);
+void remove_child(struct node *parent, struct node *child);
 
 const char *get_unitname(struct node *node);
 struct property *get_property(struct node *node, const char *propname);
diff --git a/livetree.c b/livetree.c
index 13c5f10..924c060 100644
--- a/livetree.c
+++ b/livetree.c
@@ -167,6 +167,14 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 	return old_node;
 }
 
+void remove_nodes(struct node *node) {
+	/*
+	 * For now, just unlink the node from the tree structure.
+	 * This leaks memory, but at this point the developers are ok with this.
+	 */
+	remove_child(node->parent, node);
+}
+
 struct node *chain_node(struct node *first, struct node *list)
 {
 	assert(first->next_sibling == NULL);
@@ -202,6 +210,23 @@ void add_child(struct node *parent, struct node *child)
 	*p = child;
 }
 
+void remove_child(struct node *parent, struct node *child)
+{
+	struct node **p;
+
+	/* Make sure we've got a consistent tree here */
+	assert(child->parent == parent);
+
+	p = &parent->children;
+	while (*p) {
+		if (*p == child) {
+			*p = (*p)->next_sibling;
+		}
+		p = &((*p)->next_sibling);
+	}
+	child->parent = NULL;
+}
+
 struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
 {
 	struct reserve_info *new = xmalloc(sizeof(*new));

[PATCH 2/2] Allow nodes at the root to be specified by path as well as by label.

[John Bonesio]

Changes to allow us to specify a node by it's path. A path can be used in
place of a label.

This is particularly useful when overriding existing nodes.
This way we don't have to label every possible node in a device tree we know
is a base device tree for a class of systems, and we know the tree will be
modified for the specific systems.

Signed-off-by: John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

 dtc-parser.y |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/dtc-parser.y b/dtc-parser.y
index 56d7789..0efd0cc 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -134,22 +134,30 @@ devicetree:
 		{
 			struct node *target;
 
-			target = get_node_by_label($1, $2);
+			if ($2[0] == '/')
+				target = get_node_by_path($1, $2);
+			else
+				target = get_node_by_label($1, $2);
+
 			if (target)
 				merge_nodes(target, $3);
 			else
-				print_error("label, '%s' not found", $2);
+				print_error("label or path, '%s', not found", $2);
 			$$ = $1;
 		}
 	| devicetree DT_REMOVENODE DT_REF ';'
 		{
 			struct node *target;
 
-			target = get_node_by_label($1, $3);
+			if ($3[0] == '/')
+				target = get_node_by_path($1, $3);
+			else
+				target = get_node_by_label($1, $3);
+
 			if (target)
 				remove_nodes(target);
 			else
-				print_error("label, '%s' not found", $3);
+				print_error("label or path, '%s', not found", $3);
 		}
 	;

Re: [PATCH 2/2] Allow nodes at the root to be specified by path as well as by label.

[Grant Likely]

On Mon, Oct 18, 2010 at 01:25:50PM -0700, John Bonesio wrote:
> Changes to allow us to specify a node by it's path. A path can be used in
> place of a label.
> 
> This is particularly useful when overriding existing nodes.
> This way we don't have to label every possible node in a device tree we know
> is a base device tree for a class of systems, and we know the tree will be
> modified for the specific systems.
> 
> Signed-off-by: John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> ---
> 
>  dtc-parser.y |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)

Need to add test cases.

> 
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 56d7789..0efd0cc 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -134,22 +134,30 @@ devicetree:
>  		{
>  			struct node *target;
>  
> -			target = get_node_by_label($1, $2);
> +			if ($2[0] == '/')
> +				target = get_node_by_path($1, $2);
> +			else
> +				target = get_node_by_label($1, $2);
> +
>  			if (target)
>  				merge_nodes(target, $3);
>  			else
> -				print_error("label, '%s' not found", $2);
> +				print_error("label or path, '%s', not found", $2);
>  			$$ = $1;
>  		}
>  	| devicetree DT_REMOVENODE DT_REF ';'
>  		{
>  			struct node *target;
>  
> -			target = get_node_by_label($1, $3);
> +			if ($3[0] == '/')
> +				target = get_node_by_path($1, $3);
> +			else
> +				target = get_node_by_label($1, $3);
> +
>  			if (target)
>  				remove_nodes(target);
>  			else
> -				print_error("label, '%s' not found", $3);
> +				print_error("label or path, '%s', not found", $3);

Hmmm, this is effectively the same code block implemented twice.  Its
gotten complex enough with this patch that the common parts should be
factored out inot a common function...  In fact, after looking at the
code, it looks like it should be calling get_node_by_ref() here.

Also, this patch doesn't have any way to use the form
&<label>/path/based/on/label which is also a critical feature.

g.

Re: [PATCH 2/2] Allow nodes at the root to be specified by path as well as by label.

[John Bonesio]

[-- Attachment #1.1: Type: text/plain, Size: 5304 bytes --]

It seems we have conflicting syntax for path references and label
references.

Here's what we have now:

&label
&{/path/to/the/node}

It's not a problem until we want to combine them. If we want a path rooted
at label suddenly there is the question of the curly braces. What should we
have?

&label{/path/to/the/subnode/}
&{label/path/to/the/subnode/}
&label/path/to/the/subnode


In looking at the dtc code, it appears that '&' is only used to reference
exising nodes. So I don't think the curly braces are necessary.

How about we do the following:

   1. deprecate &{/path/to/the/node/} - if it's not used anywhere, can just
   remove the code and not worry about deprecating it
   2. use the following to reference nodes:

&label   /* just refers to a labeled node */
&/path/to/the/node /* refers to a node by it's path */
&label/path/to/the/subnode /* refers to a node rooted at the label and
follows the path down */

Thoughts?

- John


On Mon, Oct 18, 2010 at 2:51 PM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>wrote:

> On Mon, Oct 18, 2010 at 02:08:33PM -0700, John Bonesio wrote:
> > On Mon, Oct 18, 2010 at 1:50 PM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org
> >wrote:
> >
> > > On Mon, Oct 18, 2010 at 01:25:50PM -0700, John Bonesio wrote:
> > > > Changes to allow us to specify a node by it's path. A path can be
> used in
> > > > place of a label.
> > > >
> > > > This is particularly useful when overriding existing nodes.
> > > > This way we don't have to label every possible node in a device tree
> we
> > > know
> > > > is a base device tree for a class of systems, and we know the tree
> will
> > > be
> > > > modified for the specific systems.
> > > >
> > > > Signed-off-by: John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> > > > ---
> > > >
> > > >  dtc-parser.y |   16 ++++++++++++----
> > > >  1 files changed, 12 insertions(+), 4 deletions(-)
> > >
> > > Need to add test cases.
> > >
> > > >
> > > > diff --git a/dtc-parser.y b/dtc-parser.y
> > > > index 56d7789..0efd0cc 100644
> > > > --- a/dtc-parser.y
> > > > +++ b/dtc-parser.y
> > > > @@ -134,22 +134,30 @@ devicetree:
> > > >               {
> > > >                       struct node *target;
> > > >
> > > > -                     target = get_node_by_label($1, $2);
> > > > +                     if ($2[0] == '/')
> > > > +                             target = get_node_by_path($1, $2);
> > > > +                     else
> > > > +                             target = get_node_by_label($1, $2);
> > > > +
> > > >                       if (target)
> > > >                               merge_nodes(target, $3);
> > > >                       else
> > > > -                             print_error("label, '%s' not found",
> $2);
> > > > +                             print_error("label or path, '%s', not
> > > found", $2);
> > > >                       $$ = $1;
> > > >               }
> > > >       | devicetree DT_REMOVENODE DT_REF ';'
> > > >               {
> > > >                       struct node *target;
> > > >
> > > > -                     target = get_node_by_label($1, $3);
> > > > +                     if ($3[0] == '/')
> > > > +                             target = get_node_by_path($1, $3);
> > > > +                     else
> > > > +                             target = get_node_by_label($1, $3);
> > > > +
> > > >                       if (target)
> > > >                               remove_nodes(target);
> > > >                       else
> > > > -                             print_error("label, '%s' not found",
> $3);
> > > > +                             print_error("label or path, '%s', not
> > > found", $3);
> > >
> > > Hmmm, this is effectively the same code block implemented twice.  Its
> > > gotten complex enough with this patch that the common parts should be
> > > factored out inot a common function...  In fact, after looking at the
> > > code, it looks like it should be calling get_node_by_ref() here.
> > >
> > > Also, this patch doesn't have any way to use the form
> > > &<label>/path/based/on/label which is also a critical feature.
> > >
> > >
> > Ok, so now I'm glad I separated this patch out.
> >
> > You're right in that it should be call get_node_by_ref(). I missed that.
> >
> > As for the rest of it, allow me to explain ...
> >
> > The lexer already parses &{/ <node path>} as a single lexical unit. When
> it
> > does this, it returns the token DT_REF, which is the same as for a label.
> > All I had to do is call the right routine to go find the right node.
>
> Right, I understand this.
>
> But I don't currently see how the code can handle the use case of a
> subnode from a label.  ie:
>
>        /{
>                my-label: parent {
>                        my-node {
>                        };
>                };
>        };
>
>        &my-label/my-node {
>                hi-there;
>        };
>
> The current code seems to be able to handle a full path, or a single
> label, but not a child node of a labelled node.
>
>
> >
> > I've run this through a sample dts file, and it does indeed work.
>
> (BTW, I think I gave you bad information earlier about syntax.  We
> have a syntax for full path, but we're missing a syntax for
> label+subpath), which you'll have to define.
>
> g.
>
>

[-- Attachment #1.2: Type: text/html, Size: 7454 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/2] Allow nodes at the root to be specified by path as well as by label.

[David Gibson]

On Mon, Oct 18, 2010 at 04:36:14PM -0700, John Bonesio wrote:
> It seems we have conflicting syntax for path references and label
> references.
> 
> Here's what we have now:
> 
> &label
> &{/path/to/the/node}
> 
> It's not a problem until we want to combine them. If we want a path rooted
> at label suddenly there is the question of the curly braces. What should we
> have?
> 
> &label{/path/to/the/subnode/}
> &{label/path/to/the/subnode/}
> &label/path/to/the/subnode

Yeah, I've been thinking about this myself.

> In looking at the dtc code, it appears that '&' is only used to reference
> exising nodes. So I don't think the curly braces are necessary.
> 
> How about we do the following:
> 
>    1. deprecate &{/path/to/the/node/} - if it's not used anywhere, can just
>    remove the code and not worry about deprecating it

>    2. use the following to reference nodes:
> 
> &label   /* just refers to a labeled node */
> &/path/to/the/node /* refers to a node by it's path */

Absolutely not.  We got rid of bare &/path/to/node when we went from
dts-v0 to dts-v1 for good reason.

The problem is that node (and property) names can contain all manner
of strange characters, including things that would usually be
separators or delimiters.  To sanely lex and parse this, they can only
be recognized in special lexical contexts, and not just anywhere -
such as where references can appear.  The extra { } delimiters serve
to introduce the special lexical context which can recognize bare node
names, and therefore a full path.

That gives us several options:

	1) &{label/path/to/subnode}

Good: Very straightforward to implement

Bad:  Mild conflict with OF conventions, where you'd expect an alias
as the first element
      Is &{label} prohibited or just equivalent to &label.  First is
an odd exception, second means we have two ways of doing something for
non-obvious reason.


	2) &label{/path/to/subnode}

Good: Nice orthogonality; a reference is always &[<label>][{<path>}]

Bad: leading / suggests an absolute path, which it isn't
     potentially nasty ambiguity with the currently legal construct
		&label{property="something;};
(i.e. does the {} introduce a reference path, or the beginning of the 
path or the actual node block)

	3) &label{path/to/subnode}

Good: relative path gives right hint to reader
      nearly as orthogonal

Bad: same ambiguity as above

	3) Always allow relative paths rather than single component to
introduce a node block, thus allowing:

&label {
	path/to/subnode {
		new-property;
	};
};

Good: avoids grammatical ambiguities
      may have uses other than with labels

Bad: more verbose
     requires per-subtree rather than per toplevel-tree
remove/replace attributes

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

node reference: path from a label

[John Bonesio]

Hi,

I was talking to Grant Likely a bit about the path from a label syntax,
and we came to what we thought was a good solution. I was supposed to
propose it with another patch submission, but now I'm rethinking this. I
thought I'd go into detail on the problem so we might all work together
to come to a good solution.

My concern here is we'll end up doing what happened with the Perl
language. In Perl they started out with a good functional scripting
language, but then later adopted an object oriented approach along with
other new features. Eventually it got to where certain syntax didn't
work with other syntax. It is legal syntax, but doesn't produce the
expected results, because the combination could have multiple meanings.
In my opinion, the design of that language is now broken, and in face a
newer version of this language is being worked on which will break
backward compatibility with existing code (even the developers recognize
the problem).

A part of our problem is that we are trying to make a syntax for 'path
from a label' in the context of an undefined syntax for 'path from an
alias'. It makes discussion of 'path from a label' difficult because I'm
trying to fit it in with a 'path from an alias' syntax that is in
people's heads.

I think we need to talk about how we might implement path from an alias
so that we can talk concretely about whole concept of how we want to
specify references to nodes.

(Syntax #1)
When Grant and I chatted, we can come to the following:
	& - means reference

	&{&label/path/from/label}
	&{alias/path/from/alias}  (future syntax)
	&{/path/to/node}
	&label
	&{alias}
	&{/}


As I thought about it, the problem with this is that the syntax requires
differing contexts to resolve what '&' means.

We have &{&label/path/from/label} to mean reference the labeled node
first, then resolve the rest of the path reference from there (sort of a
nested referencing).

So then what do we do with the alias? We stared with
&{alias/path/from/alias}. But then in the syntax it doesn't look like
we're referencing the alias node like we would for the label node, we're
just sticking the alias name in there. So the label and the alias don't
appear to be treated the same.

Perhaps we could add a different character to mean reference to an
alias, perhaps %alias. 
(Syntax #1a)
So we use
	&{%alias/path/from/alias}.

With this approach we have 3 syntaxes to specify a refernce;
	&label - reference to label
	%alias - reference to an alias.
	&{/path/to/node}
	&{&label/path/to/node}
	&{%alias/path/to/node}

It could work, but we have multiple syntaxes to reference nodes
depending on if by label or alias. Or depending on how you look at it,
we have '&' meaning label identifier or reference to node depending.

Some of the other suggestions:

(Syntax #2)
As David Gibson suggested we could have:
&label{/path/from/label}
or
&label{path/from/label}

Both of these still begs the question of what we do with the path from
an alias. Do we use the following?
	&{alias/path/from/alias}

To the casual observer, they probably will be wondering why the label is
treated so differently from the alias when conceptually they're so
similar? I might also be hard to for dts writers to remember which
syntax to use for the label vs the alias.

(Syntax #3)
Another syntax was proposed:
	&label {
		    path/to/subnode {
		            new-property;
		    };
	};

So what would do to reference from an alias?
	&{alias/path/from/alias}
	
Again we end up with very different methods to do very similar things
depending on if the start of the reference is an alias or a label.

(Syntax #4 - My actual proposal here)
For this syntax (below) to work, we need to set up some semantics first.
The first rule is this:
Every alias defined in the dts file also defines a label with the same
name on the aliased node.

>From tests/aliases.dts:
/ {
        aliases {
                s1 = &sub1;
                ss1 = &subsub1;
                sss1 = &subsubsub1;
        };

        sub1: subnode@1 {
                compatible = "subnode1";

                subsub1: subsubnode {
                        compatible = "subsubnode1", "subsubnode";

                        subsubsub1: subsubsubnode {
                                compatible = "subsubsubnode1",
"subsubsubnode";
                        };
                };
        };
};

In the above example, /subnode@1 ends up having labels: &sub1 and &s1.
The aliases nodes still exist in the tree for referencing at run time.

If desired, aliases can re-use label names. For example the following is
valid:

	aliases {
                sub1 = &sub1;
        };

In this case, an alias node is created for 'sub1', and the label on
subnode@1 is just &sub1.

With these semantics, then we only need a syntax for the following:
	* reference by label
	* reference by path
	* reference a path from a label (also covers path from an alias)

And we can use the following syntax:
	&label (syntactic shortcut for &{label})
	&{label}
	&{/path/to/a/node}
	&{label/path/from/label}

With this '&' always means reference, and a reference to a path from an
alias is built in by the semantics.

For this, we will probably want to go ahead and implement the label
semantics on an alias to make sure we don't forget all this and do
something unintended. Or we can just document it somewhere where we're
sure to see it.

(Syntax #5)
I think other options would have to include deprecating some or all of
the current syntax. This might include deprecating
	&{/path/to/node}
	
So that we can make '&label' always be just a label identifier and then
have something else to mean reference (maybe '%'). Then we'd have to
have something else that means alias identifier (maybe '$'). This would
produce something like
	%{&label}
	%{/path/to/node}
	%{&label/path/to/node}
	%{$alias/path/to/node}

I haven't had much time to work through the details on this one, and
there might be other options for what syntax we keep and what we
deprecate.

Thoughts?

Re: node reference: path from a label

[David Gibson]

On Sat, Oct 23, 2010 at 04:14:31PM -0700, John Bonesio wrote:
> Hi,
> 
> I was talking to Grant Likely a bit about the path from a label syntax,
> and we came to what we thought was a good solution. I was supposed to
> propose it with another patch submission, but now I'm rethinking this. I
> thought I'd go into detail on the problem so we might all work together
> to come to a good solution.
> 
> My concern here is we'll end up doing what happened with the Perl
> language. In Perl they started out with a good functional scripting
> language, but then later adopted an object oriented approach along with
> other new features. Eventually it got to where certain syntax didn't
> work with other syntax. It is legal syntax, but doesn't produce the
> expected results, because the combination could have multiple meanings.

Right, I'm also concerned about keeping the syntax clean, which is
exactly why I'm always so difficult when new syntax is proposed.

> In my opinion, the design of that language is now broken, and in face a
> newer version of this language is being worked on which will break
> backward compatibility with existing code (even the developers recognize
> the problem).

Well, sure.  Of course, Perl had about a zillion times more complexity
than dtc to begin with..

> A part of our problem is that we are trying to make a syntax for 'path
> from a label' in the context of an undefined syntax for 'path from an
> alias'. It makes discussion of 'path from a label' difficult because I'm
> trying to fit it in with a 'path from an alias' syntax that is in
> people's heads.
> 
> I think we need to talk about how we might implement path from an alias
> so that we can talk concretely about whole concept of how we want to
> specify references to nodes.
> 
> (Syntax #1)
> When Grant and I chatted, we can come to the following:
> 	& - means reference
> 
> 	&{&label/path/from/label}

This one occurred to me, but I really dislike the double & for reasona
I'll go into below.

> 	&{alias/path/from/alias}  (future syntax)

Not so much future syntax as existing syntax from contexts other than dtc.

> 	&{/path/to/node}
> 	&label
> 	&{alias}
> 	&{/}
> 
> 
> As I thought about it, the problem with this is that the syntax requires
> differing contexts to resolve what '&' means.

I'm not sure what you mean by that.

> We have &{&label/path/from/label} to mean reference the labeled node
> first, then resolve the rest of the path reference from there (sort of a
> nested referencing).
> 
> So then what do we do with the alias? We stared with
> &{alias/path/from/alias}. But then in the syntax it doesn't look like
> we're referencing the alias node like we would for the label node, we're
> just sticking the alias name in there. So the label and the alias don't
> appear to be treated the same.

Yeah, I agree.

> 
> Perhaps we could add a different character to mean reference to an
> alias, perhaps %alias. 
> (Syntax #1a)
> So we use
> 	&{%alias/path/from/alias}.
> 
> With this approach we have 3 syntaxes to specify a refernce;
> 	&label - reference to label
> 	%alias - reference to an alias.
> 	&{/path/to/node}
> 	&{&label/path/to/node}
> 	&{%alias/path/to/node}
> 
> It could work, but we have multiple syntaxes to reference nodes
> depending on if by label or alias. Or depending on how you look at it,
> we have '&' meaning label identifier or reference to node depending.

Ugh, yuck, yuck, yuck.

> Some of the other suggestions:
> 
> (Syntax #2)
> As David Gibson suggested we could have:
> &label{/path/from/label}
> or
> &label{path/from/label}
> 
> Both of these still begs the question of what we do with the path from
> an alias. Do we use the following?

(Off Topic Pet Peeve:  That's not what begging the question means
http://en.wikipedia.org/wiki/Begging_the_question)

> 	&{alias/path/from/alias}

Yes, in this context that's the only thing that makes sense, because
it's precisely "alias/path/from/alias" which is the syntax already
used in IEEE1275 and libfdt.

> To the casual observer, they probably will be wondering why the label is
> treated so differently from the alias when conceptually they're so
> similar? I might also be hard to for dts writers to remember which
> syntax to use for the label vs the alias.

Yes, a very valid point.

> (Syntax #3)
> Another syntax was proposed:
> 	&label {
> 		    path/to/subnode {
> 		            new-property;
> 		    };
> 	};
> 
> So what would do to reference from an alias?
> 	&{alias/path/from/alias}
> 	
> Again we end up with very different methods to do very similar things
> depending on if the start of the reference is an alias or a label.

This version bothers me less in this way than the other, though I'm
not sure why.  However it raises other issues with the semantics of
node deletion.

> (Syntax #4 - My actual proposal here)
> For this syntax (below) to work, we need to set up some semantics first.
> The first rule is this:
> Every alias defined in the dts file also defines a label with the same
> name on the aliased node.

That's a good basic concept, given the similarity.  For other reasons
I've considered in the past doing this roughly backwards - defining a
special label syntax which automatically adds a matching alias, e.g.
	label: node {
		...
	};
defines a plain label, whereas:
	label:: node { 
		...
	};
defines the same label, and also adds a matching alias.

> >From tests/aliases.dts:
> / {
>         aliases {
>                 s1 = &sub1;
>                 ss1 = &subsub1;
>                 sss1 = &subsubsub1;
>         };
> 
>         sub1: subnode@1 {
>                 compatible = "subnode1";
> 
>                 subsub1: subsubnode {
>                         compatible = "subsubnode1", "subsubnode";
> 
>                         subsubsub1: subsubsubnode {
>                                 compatible = "subsubsubnode1",
> "subsubsubnode";
>                         };
>                 };
>         };
> };
> 
> In the above example, /subnode@1 ends up having labels: &sub1 and &s1.
> The aliases nodes still exist in the tree for referencing at run time.

Obviously.

> If desired, aliases can re-use label names. For example the following is
> valid:
> 
> 	aliases {
>                 sub1 = &sub1;
>         };
> 
> In this case, an alias node is created for 'sub1', and the label on
> subnode@1 is just &sub1.

Ok.

> With these semantics, then we only need a syntax for the following:
> 	* reference by label
> 	* reference by path
> 	* reference a path from a label (also covers path from an alias)
> 
> And we can use the following syntax:
> 	&label (syntactic shortcut for &{label})
> 	&{label}
> 	&{/path/to/a/node}
> 	&{label/path/from/label}
> 
> With this '&' always means reference, and a reference to a path from an
> alias is built in by the semantics.
> 
> For this, we will probably want to go ahead and implement the label
> semantics on an alias to make sure we don't forget all this and do
> something unintended. Or we can just document it somewhere where we're
> sure to see it.

Alrighty.  Best idea so far, by far.  I can see two difficulties, one
minor, one curly.

The minor one is that an alias name - being a property name - is
technically permitted to contain a whole bunch of weird characters
that are not valid in labels.  So we'd probably to have to have an odd
edge case where an alias does not create a label if it contains bad
characters.  Since aliases in practice don't contain weird characters,
I don't think that's overly problematic, especially since we could add
a warning if there are any properties in /aliases with bad names.

The curlier problem (in the sense of harder to analyze rather than
necessarily bad) is the question of lifetimes and scope.  At present
labels are kind of "out of band" and have global scope.  They can be
referenced either before or after their definition, and have a single
well defined meaning over the whole input file.

With node / property removals we've already raised the question of
what happens to labels on them.  If the labels disappear with their
node then they no longer have a guaranteed uniform meaning across the
whole file, which is a very odd thing to combine with forward
references.

If aliases auto-create labels we have the same issue more directly -
aliases certainly *can* be changed by overlays, so does that alter the
label definition as well.

> (Syntax #5)
> I think other options would have to include deprecating some or all of
> the current syntax. This might include deprecating
> 	&{/path/to/node}
> 	
> So that we can make '&label' always be just a label identifier and then
> have something else to mean reference (maybe '%').

So, '&' is *supposed* to mean reference, not "here is a label".  I
chose than syntax based on the &variable syntax in C, figuring that
"address of" is in reasonable analogy with "phandle of" or "path of"
(because they're replacing an abstract handle on an object with a
specific encoding of the object's location).

Arguably, our new use of &label in tree overlays breaks this analogy.
Dang.  Now that I think of it, that was probably the half-formed
objection I had in mind before I forgot it again and acked Grant's
adding of this extension.

> Then we'd have to
> have something else that means alias identifier (maybe '$'). This would
> produce something like
> 	%{&label}
> 	%{/path/to/node}
> 	%{&label/path/to/node}
> 	%{$alias/path/to/node}
> 
> I haven't had much time to work through the details on this one, and
> there might be other options for what syntax we keep and what we
> deprecate.

So, I think there might be a case for deprecating syntax, but in
roughly the opposite direction to the one you suggest here.

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

Re: node reference: path from a label

[Grant Likely]

On Mon, Oct 25, 2010 at 12:26 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Sat, Oct 23, 2010 at 04:14:31PM -0700, John Bonesio wrote:
>> Hi,
>>
>> I was talking to Grant Likely a bit about the path from a label syntax,
>> and we came to what we thought was a good solution. I was supposed to
>> propose it with another patch submission, but now I'm rethinking this. I
>> thought I'd go into detail on the problem so we might all work together
>> to come to a good solution.
>>
>> My concern here is we'll end up doing what happened with the Perl
>> language. In Perl they started out with a good functional scripting
>> language, but then later adopted an object oriented approach along with
>> other new features. Eventually it got to where certain syntax didn't
>> work with other syntax. It is legal syntax, but doesn't produce the
>> expected results, because the combination could have multiple meanings.
>
> Right, I'm also concerned about keeping the syntax clean, which is
> exactly why I'm always so difficult when new syntax is proposed.
>
>> In my opinion, the design of that language is now broken, and in face a
>> newer version of this language is being worked on which will break
>> backward compatibility with existing code (even the developers recognize
>> the problem).
>
> Well, sure.  Of course, Perl had about a zillion times more complexity
> than dtc to begin with..
>
>> A part of our problem is that we are trying to make a syntax for 'path
>> from a label' in the context of an undefined syntax for 'path from an
>> alias'. It makes discussion of 'path from a label' difficult because I'm
>> trying to fit it in with a 'path from an alias' syntax that is in
>> people's heads.
>>
>> I think we need to talk about how we might implement path from an alias
>> so that we can talk concretely about whole concept of how we want to
>> specify references to nodes.
>>
>> (Syntax #1)
>> When Grant and I chatted, we can come to the following:
>>       & - means reference
>>
>>       &{&label/path/from/label}
>
> This one occurred to me, but I really dislike the double & for reasona
> I'll go into below.
>
>>       &{alias/path/from/alias}  (future syntax)
>
> Not so much future syntax as existing syntax from contexts other than dtc.
>
>>       &{/path/to/node}
>>       &label
>>       &{alias}
>>       &{/}
>>
>>
>> As I thought about it, the problem with this is that the syntax requires
>> differing contexts to resolve what '&' means.
>
> I'm not sure what you mean by that.
>
>> We have &{&label/path/from/label} to mean reference the labeled node
>> first, then resolve the rest of the path reference from there (sort of a
>> nested referencing).
>>
>> So then what do we do with the alias? We stared with
>> &{alias/path/from/alias}. But then in the syntax it doesn't look like
>> we're referencing the alias node like we would for the label node, we're
>> just sticking the alias name in there. So the label and the alias don't
>> appear to be treated the same.
>
> Yeah, I agree.
>
>>
>> Perhaps we could add a different character to mean reference to an
>> alias, perhaps %alias.
>> (Syntax #1a)
>> So we use
>>       &{%alias/path/from/alias}.
>>
>> With this approach we have 3 syntaxes to specify a refernce;
>>       &label - reference to label
>>       %alias - reference to an alias.
>>       &{/path/to/node}
>>       &{&label/path/to/node}
>>       &{%alias/path/to/node}
>>
>> It could work, but we have multiple syntaxes to reference nodes
>> depending on if by label or alias. Or depending on how you look at it,
>> we have '&' meaning label identifier or reference to node depending.
>
> Ugh, yuck, yuck, yuck.
>
>> Some of the other suggestions:
>>
>> (Syntax #2)
>> As David Gibson suggested we could have:
>> &label{/path/from/label}
>> or
>> &label{path/from/label}
>>
>> Both of these still begs the question of what we do with the path from
>> an alias. Do we use the following?
>
> (Off Topic Pet Peeve:  That's not what begging the question means
> http://en.wikipedia.org/wiki/Begging_the_question)
>
>>       &{alias/path/from/alias}
>
> Yes, in this context that's the only thing that makes sense, because
> it's precisely "alias/path/from/alias" which is the syntax already
> used in IEEE1275 and libfdt.
>
>> To the casual observer, they probably will be wondering why the label is
>> treated so differently from the alias when conceptually they're so
>> similar? I might also be hard to for dts writers to remember which
>> syntax to use for the label vs the alias.
>
> Yes, a very valid point.
>
>> (Syntax #3)
>> Another syntax was proposed:
>>       &label {
>>                   path/to/subnode {
>>                           new-property;
>>                   };
>>       };
>>
>> So what would do to reference from an alias?
>>       &{alias/path/from/alias}
>>
>> Again we end up with very different methods to do very similar things
>> depending on if the start of the reference is an alias or a label.
>
> This version bothers me less in this way than the other, though I'm
> not sure why.  However it raises other issues with the semantics of
> node deletion.
>
>> (Syntax #4 - My actual proposal here)
>> For this syntax (below) to work, we need to set up some semantics first.
>> The first rule is this:
>> Every alias defined in the dts file also defines a label with the same
>> name on the aliased node.
>
> That's a good basic concept, given the similarity.  For other reasons
> I've considered in the past doing this roughly backwards - defining a
> special label syntax which automatically adds a matching alias, e.g.
>        label: node {
>                ...
>        };
> defines a plain label, whereas:
>        label:: node {
>                ...
>        };
> defines the same label, and also adds a matching alias.
>
>> >From tests/aliases.dts:
>> / {
>>         aliases {
>>                 s1 = &sub1;
>>                 ss1 = &subsub1;
>>                 sss1 = &subsubsub1;
>>         };
>>
>>         sub1: subnode@1 {
>>                 compatible = "subnode1";
>>
>>                 subsub1: subsubnode {
>>                         compatible = "subsubnode1", "subsubnode";
>>
>>                         subsubsub1: subsubsubnode {
>>                                 compatible = "subsubsubnode1",
>> "subsubsubnode";
>>                         };
>>                 };
>>         };
>> };
>>
>> In the above example, /subnode@1 ends up having labels: &sub1 and &s1.
>> The aliases nodes still exist in the tree for referencing at run time.
>
> Obviously.
>
>> If desired, aliases can re-use label names. For example the following is
>> valid:
>>
>>       aliases {
>>                 sub1 = &sub1;
>>         };
>>
>> In this case, an alias node is created for 'sub1', and the label on
>> subnode@1 is just &sub1.
>
> Ok.
>
>> With these semantics, then we only need a syntax for the following:
>>       * reference by label
>>       * reference by path
>>       * reference a path from a label (also covers path from an alias)
>>
>> And we can use the following syntax:
>>       &label (syntactic shortcut for &{label})
>>       &{label}
>>       &{/path/to/a/node}
>>       &{label/path/from/label}
>>
>> With this '&' always means reference, and a reference to a path from an
>> alias is built in by the semantics.
>>
>> For this, we will probably want to go ahead and implement the label
>> semantics on an alias to make sure we don't forget all this and do
>> something unintended. Or we can just document it somewhere where we're
>> sure to see it.
>
> Alrighty.  Best idea so far, by far.  I can see two difficulties, one
> minor, one curly.
>
> The minor one is that an alias name - being a property name - is
> technically permitted to contain a whole bunch of weird characters
> that are not valid in labels.  So we'd probably to have to have an odd
> edge case where an alias does not create a label if it contains bad
> characters.  Since aliases in practice don't contain weird characters,
> I don't think that's overly problematic, especially since we could add
> a warning if there are any properties in /aliases with bad names.
>
> The curlier problem (in the sense of harder to analyze rather than
> necessarily bad) is the question of lifetimes and scope.  At present
> labels are kind of "out of band" and have global scope.  They can be
> referenced either before or after their definition, and have a single
> well defined meaning over the whole input file.
>
> With node / property removals we've already raised the question of
> what happens to labels on them.  If the labels disappear with their
> node then they no longer have a guaranteed uniform meaning across the
> whole file, which is a very odd thing to combine with forward
> references.
>
> If aliases auto-create labels we have the same issue more directly -
> aliases certainly *can* be changed by overlays, so does that alter the
> label definition as well.
>
>> (Syntax #5)
>> I think other options would have to include deprecating some or all of
>> the current syntax. This might include deprecating
>>       &{/path/to/node}
>>
>> So that we can make '&label' always be just a label identifier and then
>> have something else to mean reference (maybe '%').
>
> So, '&' is *supposed* to mean reference, not "here is a label".  I
> chose than syntax based on the &variable syntax in C, figuring that
> "address of" is in reasonable analogy with "phandle of" or "path of"
> (because they're replacing an abstract handle on an object with a
> specific encoding of the object's location).
>
> Arguably, our new use of &label in tree overlays breaks this analogy.
> Dang.  Now that I think of it, that was probably the half-formed
> objection I had in mind before I forgot it again and acked Grant's
> adding of this extension.

+1 to pretty much everything you say in your reply here.  It is a good analysis.

With regard to the '&' usage, we could very easily change the use of
'&' at this point as it has only recently been merged (Sept 20) so it
is very unlikely that there are significant users.  What would a
better syntax be?

g.

Re: [PATCH 2/2] Allow nodes at the root to be specified by path as well as by label.

[David Gibson]

On Mon, Oct 18, 2010 at 01:25:50PM -0700, John Bonesio wrote:
> Changes to allow us to specify a node by it's path. A path can be used in
> place of a label.
> 
> This is particularly useful when overriding existing nodes.
> This way we don't have to label every possible node in a device tree we know
> is a base device tree for a class of systems, and we know the tree will be
> modified for the specific systems.

Nack.  This patch is pointless, the DT_REF lexing will already resolve
paths in the form &{/some/path} and I see no reason that won't work
for tree merging as well.  Oh, and bare &/path/to/node is a bad idea
anyway, more about that later.

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

Re: [PATCH 2/2] Allow nodes at the root to be specified by path as well as by label.

[John Bonesio]

On Tue, 2010-10-19 at 13:14 +1100, David Gibson wrote:
> On Mon, Oct 18, 2010 at 01:25:50PM -0700, John Bonesio wrote:
> > Changes to allow us to specify a node by it's path. A path can be used in
> > place of a label.
> > 
> > This is particularly useful when overriding existing nodes.
> > This way we don't have to label every possible node in a device tree we know
> > is a base device tree for a class of systems, and we know the tree will be
> > modified for the specific systems.
> 
> Nack.  This patch is pointless, the DT_REF lexing will already resolve
> paths in the form &{/some/path} and I see no reason that won't work
> for tree merging as well.  Oh, and bare &/path/to/node is a bad idea
> anyway, more about that later.
> 

I believe a patch is still necessary. I just tried dtc from git without
changes. I didn't work.

It also looks to me like get_node_by_label() will not resolve nodes that
are specified by path.

Perhaps I'm just doing something wrong.

- John

Re: [PATCH 2/2] Allow nodes at the root to be specified by path as well as by label.

[David Gibson]

On Tue, Oct 19, 2010 at 09:05:55AM -0700, John Bonesio wrote:
> On Tue, 2010-10-19 at 13:14 +1100, David Gibson wrote:
> > On Mon, Oct 18, 2010 at 01:25:50PM -0700, John Bonesio wrote:
> > > Changes to allow us to specify a node by it's path. A path can be used in
> > > place of a label.
> > > 
> > > This is particularly useful when overriding existing nodes.
> > > This way we don't have to label every possible node in a device tree we know
> > > is a base device tree for a class of systems, and we know the tree will be
> > > modified for the specific systems.
> > 
> > Nack.  This patch is pointless, the DT_REF lexing will already resolve
> > paths in the form &{/some/path} and I see no reason that won't work
> > for tree merging as well.  Oh, and bare &/path/to/node is a bad idea
> > anyway, more about that later.
> 
> I believe a patch is still necessary. I just tried dtc from git without
> changes. I didn't work.
> 
> It also looks to me like get_node_by_label() will not resolve nodes that
> are specified by path.
> 
> Perhaps I'm just doing something wrong.

Ah, yes, sorry.  The grammar will accept the full path, but
get_node_by_label() won't look it up.  If you change that to
get_node_by_ref(), however, I think it should work.

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

Re: [PATCH 1/2] Implements new features for updating existing device tree nodes.

[Grant Likely]

On Mon, Oct 18, 2010 at 01:25:29PM -0700, John Bonesio wrote:
> 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.
> 
> Signed-off-by: John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

A few comments, but I'm otherwise happy with this patch.

g.

> ---
> 
> I was originally going to submit just one patch, as the change in the
> second one is pretty trivial. However, I thought people might appreciate
> that the change to refer to nodes by path is highlighted separately.
> 
> Comment away!
> 
> - John
> 
>  dtc-lexer.l  |    6 ++++++
>  dtc-parser.y |   14 ++++++++++++--
>  dtc.h        |    3 +++
>  livetree.c   |   25 +++++++++++++++++++++++++

Need to add test cases to the patch

>  4 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 081e13a..80a886a 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -96,6 +96,12 @@ static int pop_input_file(void);
>  			return DT_MEMRESERVE;
>  		}
>  
> +<*>"/remove-node/"	{
> +			DPRINT("Keyword: /remove-node/\n");
> +			BEGIN_DEFAULT();
> +			return DT_REMOVENODE;
> +		}
> +
>  <*>{LABEL}:	{
>  			DPRINT("Label: %s\n", yytext);
>  			yylval.labelref = xstrdup(yytext);
> diff --git a/dtc-parser.y b/dtc-parser.y
> index e1846d4..56d7789 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -55,6 +55,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
>  
>  %token DT_V1
>  %token DT_MEMRESERVE
> +%token DT_REMOVENODE
>  %token <propnodename> DT_PROPNODENAME
>  %token <literal> DT_LITERAL
>  %token <cbase> DT_BASE
> @@ -137,10 +138,19 @@ devicetree:
>  			if (target)
>  				merge_nodes(target, $3);
>  			else
> -				print_error("label, '%s' does not exist in"
> -					" node extension", $2);
> +				print_error("label, '%s' not found", $2);
>  			$$ = $1;

This hunk should perhaps go in the previous patch?  It is unrelated.

>  		}
> +	| devicetree DT_REMOVENODE DT_REF ';'
> +		{
> +			struct node *target;
> +
> +			target = get_node_by_label($1, $3);

The above two lines could be merged for conciseness.

> +			if (target)
> +				remove_nodes(target);
> +			else
> +				print_error("label, '%s' not found", $3);
> +		}

Nice and simple!

>  	;
>  
>  nodedef:
> diff --git a/dtc.h b/dtc.h
> index b36ac5d..e7cb45c 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -175,9 +175,12 @@ 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);
> +void remove_nodes(struct node *node);
>  
>  void add_property(struct node *node, struct property *prop);
>  void add_child(struct node *parent, struct node *child);
> +void remove_child(struct node *parent, struct node *child);
>  
>  const char *get_unitname(struct node *node);
>  struct property *get_property(struct node *node, const char *propname);
> diff --git a/livetree.c b/livetree.c
> index 13c5f10..924c060 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -167,6 +167,14 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>  	return old_node;
>  }
>  
> +void remove_nodes(struct node *node) {
> +	/*
> +	 * For now, just unlink the node from the tree structure.
> +	 * This leaks memory, but at this point the developers are ok with this.
> +	 */
> +	remove_child(node->parent, node);
> +}
> +
>  struct node *chain_node(struct node *first, struct node *list)
>  {
>  	assert(first->next_sibling == NULL);
> @@ -202,6 +210,23 @@ void add_child(struct node *parent, struct node *child)
>  	*p = child;
>  }
>  
> +void remove_child(struct node *parent, struct node *child)

Since remove_child() is only used by remove_nodes(), just roll this
implementation into remove_nodes().  If we need to do more in that
function later, then we'll refactor at that time.

> +{
> +	struct node **p;
> +
> +	/* Make sure we've got a consistent tree here */
> +	assert(child->parent == parent);
> +
> +	p = &parent->children;
> +	while (*p) {

This while loop will never complete because &((*p)->next_sibling
probably resolves to 16 on x86 and 32 on x86_64.  next_sibling is not
the first element in the structure.  The NULL condition still needs
to be tested for explicitly.

> +		if (*p == child) {
> +			*p = (*p)->next_sibling;

break;

> +		}
> +		p = &((*p)->next_sibling);

One way to solve the problem is with:

		p = (*p)->next_sibling ? &(*p)->next_sibling : NULL;
> +	}
> +	child->parent = NULL;
> +}
> +
>  struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
>  {
>  	struct reserve_info *new = xmalloc(sizeof(*new));
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss