[U-Boot-Users] [PATCH] fdt: Add simple alias support to fdt print command

[U-Boot-Users] [PATCH] fdt: Add simple alias support to fdt print command

[Kumar Gala]

If the path we are trying to print doesn't exist see if it matches an
aliases.  We don't do anything fancy at this point, but just strip the
leading '/' if it exists and see if we have an exact match to an alias.

In the future we could try and prefix matching so the alias could be used
as a shorter path reference.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
 common/cmd_fdt.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c
index 97b9dd7..65c5cdf 100644
--- a/common/cmd_fdt.c
+++ b/common/cmd_fdt.c
@@ -678,9 +678,23 @@ static int fdt_print(const char *pathp, char *prop, int depth)
 		/*
 		 * Not found or something else bad happened.
 		 */
-		printf ("libfdt fdt_path_offset() returned %s\n",
-			fdt_strerror(nodeoffset));
-		return 1;
+
+		/* see if we match an alias */
+		int aliasoffset = fdt_path_offset(working_fdt, "/aliases");
+		if (aliasoffset >= 0) {
+			const char *aliasp = pathp;
+			if (pathp[0] == '/')
+				aliasp++;
+			aliasp = fdt_getprop(working_fdt,
+						aliasoffset, aliasp, NULL);
+			nodeoffset = fdt_path_offset(working_fdt, aliasp);
+		}
+
+		if (nodeoffset < 0) {
+			printf("libfdt fdt_path_offset() returned %s\n",
+					fdt_strerror(nodeoffset));
+			return 1;
+		}
 	}
 	/*
 	 * The user passed in a property as well as node path.
-- 
1.5.5.1

[U-Boot-Users] [PATCH] fdt: Add simple alias support to fdt print command

[Jerry Van Baren]

Kumar Gala wrote:
> If the path we are trying to print doesn't exist see if it matches an
> aliases.  We don't do anything fancy at this point, but just strip the
> leading '/' if it exists and see if we have an exact match to an alias.
> 
> In the future we could try and prefix matching so the alias could be used
> as a shorter path reference.
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

Cool and useful too.  Out of curiousity, does "real" Open Firmware do 
this sort of thing with aliases?

One reservation I have (which may disappear if the answer to the 
previous question is "yes"), it automatically and silently dereferences 
the /aliases/X node when asked to display /X or X (but only if /X 
doesn't exist in the dtb).  This is not an obvious behavior since X 
isn't real.

Should we have a different display syntax to force the dereference of an 
alias X?  Assuming "*" is a good choice, this would change the behavior
   fdt print *ethernet0
to dereference /aliases/ethernet0 and print out
   /soc8360 at e0000000/.../enet0 (or whatever).

Thanks,
gvb

[U-Boot-Users] [PATCH] fdt: Add simple alias support to fdt print command

[Kumar Gala]

On Jul 9, 2008, at 10:17 AM, Jerry Van Baren wrote:

> Kumar Gala wrote:
>> If the path we are trying to print doesn't exist see if it matches an
>> aliases.  We don't do anything fancy at this point, but just strip  
>> the
>> leading '/' if it exists and see if we have an exact match to an  
>> alias.
>> In the future we could try and prefix matching so the alias could  
>> be used
>> as a shorter path reference.
>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>
> Cool and useful too.  Out of curiousity, does "real" Open Firmware  
> do this sort of thing with aliases?

I'm pretty sure it the Apple OF implementation does.  However I dont  
remember to what extent it does from my playing around with OF on  
Apple HW.

> One reservation I have (which may disappear if the answer to the  
> previous question is "yes"), it automatically and silently  
> dereferences the /aliases/X node when asked to display /X or X (but  
> only if /X doesn't exist in the dtb).  This is not an obvious  
> behavior since X isn't real.

we could print out something about using an alias so the user knows  
that its happening.

> Should we have a different display syntax to force the dereference  
> of an alias X?  Assuming "*" is a good choice, this would change the  
> behavior
>  fdt print *ethernet0
> to dereference /aliases/ethernet0 and print out
>  /soc8360 at e0000000/.../enet0 (or whatever).


Lets says I have an alias for 'soc' to 'soc8360 at e000000'.  I want to  
be able to in the future do print /soc/enet0 and have that work.   
Introducing some new syntax would make that difficult and more ugly.

- k

[U-Boot-Users] [PATCH] fdt: Add simple alias support to fdt print command

[Jerry Van Baren]

Kumar Gala wrote:
> 
> On Jul 9, 2008, at 10:17 AM, Jerry Van Baren wrote:
> 
>> Kumar Gala wrote:
>>> If the path we are trying to print doesn't exist see if it matches an
>>> aliases.  We don't do anything fancy at this point, but just strip the
>>> leading '/' if it exists and see if we have an exact match to an alias.
>>> In the future we could try and prefix matching so the alias could be 
>>> used
>>> as a shorter path reference.
>>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>>
>> Cool and useful too.  Out of curiousity, does "real" Open Firmware do 
>> this sort of thing with aliases?
> 
> I'm pretty sure it the Apple OF implementation does.  However I dont 
> remember to what extent it does from my playing around with OF on Apple HW.

Sounds like I'm going to have to preempt my kids on the olpc and see 
what OF does on it.  :-)

>> One reservation I have (which may disappear if the answer to the 
>> previous question is "yes"), it automatically and silently 
>> dereferences the /aliases/X node when asked to display /X or X (but 
>> only if /X doesn't exist in the dtb).  This is not an obvious behavior 
>> since X isn't real.
> 
> we could print out something about using an alias so the user knows that 
> its happening.
> 
>> Should we have a different display syntax to force the dereference of 
>> an alias X?  Assuming "*" is a good choice, this would change the 
>> behavior
>>  fdt print *ethernet0
>> to dereference /aliases/ethernet0 and print out
>>  /soc8360 at e0000000/.../enet0 (or whatever).
> 
> Lets says I have an alias for 'soc' to 'soc8360 at e000000'.  I want to be 
> able to in the future do print /soc/enet0 and have that work.  
> Introducing some new syntax would make that difficult and more ugly.
> 
> - k

Thinking out loud... we could define the syntax that a leading "*" 
indicates the first part of the path is a dereference of /aliases.

Assuming
   /aliases/soc = /soc8360 at e000000
   /aliases/ethernet0 = /soc8360 at e0000000/.../enet0
then
   print *soc/enet0
and
   print *ethernet0
would both work and print the right thing.  You *would* have to know 
that the first element of the path is an /aliases dereference.  Your 
original patch did not require that piece of knowledge (but silently and 
automagically, which makes me nervous).

gvb

[U-Boot-Users] [PATCH] fdt: Add simple alias support to fdt print command

[Kumar Gala]

On Jul 9, 2008, at 12:02 PM, Jerry Van Baren wrote:

>
> Thinking out loud... we could define the syntax that a leading "*"  
> indicates the first part of the path is a dereference of /aliases.
>
> Assuming
>  /aliases/soc = /soc8360 at e000000
>  /aliases/ethernet0 = /soc8360 at e0000000/.../enet0
> then
>  print *soc/enet0
> and
>  print *ethernet0
> would both work and print the right thing.  You *would* have to know  
> that the first element of the path is an /aliases dereference.  Your  
> original patch did not require that piece of knowledge (but silently  
> and automagically, which makes me nervous).

did we come to resolution on this?  I'd like to see this in 1.3.5.

- k

[U-Boot-Users] [PATCH] fdt: Add simple alias support to fdt print command

[Jerry Van Baren]

Kumar Gala wrote:
> 
> On Jul 9, 2008, at 12:02 PM, Jerry Van Baren wrote:
> 
>>
>> Thinking out loud... we could define the syntax that a leading "*" 
>> indicates the first part of the path is a dereference of /aliases.
>>
>> Assuming
>>  /aliases/soc = /soc8360 at e000000
>>  /aliases/ethernet0 = /soc8360 at e0000000/.../enet0
>> then
>>  print *soc/enet0
>> and
>>  print *ethernet0
>> would both work and print the right thing.  You *would* have to know 
>> that the first element of the path is an /aliases dereference.  Your 
>> original patch did not require that piece of knowledge (but silently 
>> and automagically, which makes me nervous).
> 
> did we come to resolution on this?  I'd like to see this in 1.3.5.
> 
> - k

Hi Kumar,

I think we have basic resolution - I would like to see it in 1.3.5 too. 
  I haven't pushed on this, waiting for 1.3.5 window to open (or some 
free time, whichever comes last).

I've CC:ed David Gibson in case he has some advice - the concept is to 
indicate a dereference of /aliases nodes so that us lazy engineers don't 
have to cut'n'paste the whole long path from the alias.  Kumar 
originally proposed to do it automagically and I countered proposing 
using "*" to indicate the next path name should be looked up in /aliases 
and the result used instead (i.e. dereferenced).  Discussion thread:
<http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/43575/focus=44941>

Looking at the ieee1275 doc
<http://playground.sun.com/pub/1275/coredoc/1275-1994/1275.ps.gz>
it looks like "*" will work for a dereference delimiter as it is not 
listed as one of the permitted punctuation characters in a node name. 
Quoting 3.2.1.1 Node names:
----------------------------------------------------------------------
The driver name field is a sequence of between one and 31 letters, 
digits, and punctuation characters from the set ", . _ + - ". Uppercase 
and lowercase characters are distinct.
----------------------------------------------------------------------

We do have a problem with property names, where "*" _is_ a legal name 
component.  Quoting 3.2.2.1.1 Property names:
----------------------------------------------------------------------
The property name is a human-readable text string consisting of one to 
thirty-one printable characters. Property names shall not contain 
uppercase characters or the characters "/", "\", ":", "[", "]" and "@".
----------------------------------------------------------------------
Note that "*" is not proscribed, making it a legal character in a 
property name.

Having noted that, I'm willing to take the risk and use "*" for the 
"alias dereference" separator.

Looking back at the original patch, Kumar's original patch only did the 
/aliases dereference for the "fdt print" command.  I'm thinking more 
general purpose: being able to dereference /aliases in all "fdt" 
commands.  This seems helpful for the "fdt set" command, for instance. 
Whether this is reasonable to implement remains to be seen...

Best regards,
gvb

[U-Boot-Users] [PATCH] fdt: Add simple alias support to fdt print command

[David Gibson]

On Sat, Aug 02, 2008 at 08:51:54PM -0400, Jerry Van Baren wrote:
> Kumar Gala wrote:
>>
>> On Jul 9, 2008, at 12:02 PM, Jerry Van Baren wrote:
>>
>>>
>>> Thinking out loud... we could define the syntax that a leading "*"  
>>> indicates the first part of the path is a dereference of /aliases.
>>>
>>> Assuming
>>>  /aliases/soc = /soc8360 at e000000
>>>  /aliases/ethernet0 = /soc8360 at e0000000/.../enet0
>>> then
>>>  print *soc/enet0
>>> and
>>>  print *ethernet0
>>> would both work and print the right thing.  You *would* have to know  
>>> that the first element of the path is an /aliases dereference.  Your  
>>> original patch did not require that piece of knowledge (but silently  
>>> and automagically, which makes me nervous).
>>
>> did we come to resolution on this?  I'd like to see this in 1.3.5.
>>
>> - k
>
> Hi Kumar,
>
> I think we have basic resolution - I would like to see it in 1.3.5 too.  
> I haven't pushed on this, waiting for 1.3.5 window to open (or some free 
> time, whichever comes last).
>
> I've CC:ed David Gibson in case he has some advice - the concept is to  
> indicate a dereference of /aliases nodes so that us lazy engineers don't  
> have to cut'n'paste the whole long path from the alias.  Kumar  
> originally proposed to do it automagically and I countered proposing  
> using "*" to indicate the next path name should be looked up in /aliases  
> and the result used instead (i.e. dereferenced).  Discussion thread:
> <http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/43575/focus=44941>

No, I really don't think using this "dereference" character is a good
idea.  If you're going to expand aliases, you should do it as real OF
does - see section 4.3 of IEEE1275.  Essentially it's the *lack* of a
leading '/' character that triggers alias expansion.  So you could use
e.g.
	/soc8360 at e0000000/ethernet at e0000400
or
	soc/ethernet at e0000400
or
	ethernet0

> Looking at the ieee1275 doc
> <http://playground.sun.com/pub/1275/coredoc/1275-1994/1275.ps.gz>
> it looks like "*" will work for a dereference delimiter as it is not  
> listed as one of the permitted punctuation characters in a node name.  
> Quoting 3.2.1.1 Node names:
> ----------------------------------------------------------------------
> The driver name field is a sequence of between one and 31 letters,  
> digits, and punctuation characters from the set ", . _ + - ". Uppercase  
> and lowercase characters are distinct.
> ----------------------------------------------------------------------
>
> We do have a problem with property names, where "*" _is_ a legal name  
> component.  Quoting 3.2.2.1.1 Property names:
> ----------------------------------------------------------------------
> The property name is a human-readable text string consisting of one to  
> thirty-one printable characters. Property names shall not contain  
> uppercase characters or the characters "/", "\", ":", "[", "]" and "@".
> ----------------------------------------------------------------------
> Note that "*" is not proscribed, making it a legal character in a  
> property name.

I've observed a '*' in the device tree in the wild - some Apple tree,
unfortunately I can't find it right at the moment.  So I can't check
if the '*' was in a node node, making it IEEE1275 illegal, or in a
property name.  I remember thinking it was illegal at the time, but I
thought '*' was invalid in property names too, which seems to have
been a misinterpretation.

> Having noted that, I'm willing to take the risk and use "*" for the  
> "alias dereference" separator.
>
> Looking back at the original patch, Kumar's original patch only did the  
> /aliases dereference for the "fdt print" command.  I'm thinking more  
> general purpose: being able to dereference /aliases in all "fdt"  
> commands.  This seems helpful for the "fdt set" command, for instance.  
> Whether this is reasonable to implement remains to be seen...

If you're interepreting them in one place, you should probably
interpret them everywhwere and have a single "resolve pathname"
function.  In fact, I should quite possibly put such a function into
libfdt.

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

[U-Boot-Users] [PATCH] fdt: Add simple alias support to fdt print command

[Jerry Van Baren]

David Gibson wrote:
> On Sat, Aug 02, 2008 at 08:51:54PM -0400, Jerry Van Baren wrote:

[snip]

>> I've CC:ed David Gibson in case he has some advice - the concept is to  
>> indicate a dereference of /aliases nodes so that us lazy engineers don't  
>> have to cut'n'paste the whole long path from the alias.  Kumar  
>> originally proposed to do it automagically and I countered proposing  
>> using "*" to indicate the next path name should be looked up in /aliases  
>> and the result used instead (i.e. dereferenced).  Discussion thread:
>> <http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/43575/focus=44941>
> 
> No, I really don't think using this "dereference" character is a good
> idea.  If you're going to expand aliases, you should do it as real OF
> does - see section 4.3 of IEEE1275.  Essentially it's the *lack* of a
> leading '/' character that triggers alias expansion.  So you could use
> e.g.
> 	/soc8360 at e0000000/ethernet at e0000400
> or
> 	soc/ethernet at e0000400
> or
> 	ethernet0

Ahh, I didn't read far enough.  The algorithm in section 4.3 is much 
better thought out than either of our proposals.

[snip]

> If you're interepreting them in one place, you should probably
> interpret them everywhwere and have a single "resolve pathname"
> function.

Yes.

> In fact, I should quite possibly put such a function into libfdt.

That would be very useful.  :-)

Thanks,
gvb