[U-Boot-Users] RFA & Update: Using libfdt in u-boot for fdt command

Jerry Van Baren
2007-03-02T04:08:38Z

Hi David,

David Gibson wrote:
> On Thu, Mar 01, 2007 at 09:01:24AM -0500, Jerry Van Baren wrote:
>> Hi all,
>>
>> This is a Request for Advice.
>>
>> First off, for those on both the u-boot and linuxppc-dev lists, sorry
>> for cross posting. :-)
>>
>> Git repo pointers...
>> libfdt hacks:
>> <http://www.cideas.us/cgi-bin/gitweb.cgi?p=linux/libfdt.git;a=summary>
>> u-boot hacks:
>> <http://www.cideas.us/cgi-bin/gitweb.cgi?p=u-boot/u-boot-mpc83xx;a=summary>

[snip]

>> 2) Not capturing libfdt in the u-boot repo makes it more difficult for
>> integrating it with and maintaining it in u-boot, I'm not sure how to
>> actually do it in a useful/usable manner.
>
> I think ultimately, you'll have to pull libfdt into the u-boot
> sources. libfdt is supposed to run in many possible environments, and
> there's no realistic way it can be compiled independently of thost
> environments.
>
> That said, any changes to the guts of libfdt that you need should go
> upstream (in the end; right now, there are bureaucratic problems with
> that, more below). That should reduce things to a one way pull, with
> some trivial tweaks to integrate with the u-boot environment.

OK, I figured as much, but was hoping for a miracle.

>> There are three problem areas with libfdt:
>> 1) The official Makefile is stand-alone which doesn't work well with
>> u-boot. I took the expedient route for the PoC of simply replacing it
>> with a u-boot style Makefile from a different lib* subdirectory. There
>> should be a better way.
>
> I think it might well be necessary to replace the Makefile for
> building libfdt into other packages. It would be nice to avoid that
> if possible, but a sensible method is not obvious to me.

OK, no surprise.

>> 2) The official libfdt uses two header files that are not in u-boot. I
>> "fixed" this by substituting u-boot headers with equivalent functionality.
>> * I need to address this and see what the best compromise for header
>> files is...
>> a) If the u-boot headers are acceptable for the stand-alone version
>> of libfdt, that would be the simplest.
>> b) It may be more effective to add the necessary linux headers to
>> u-boot.
>> c) We could use #ifdefs to conditionally include the right files. (but
>> does u-boot have a distinctive configuration def? Probably...)
>
> Ok, the way this is supposed to work is that the environment into
> which you're building should provide a replacement version of
> libfdt_env.h. The supplied version of libfdt_env.h is just for
> userland builds. u-boot's version will obviously use u-boot headers
> instead of standard library headers.
>
> I should provide a comprehensive list of what libfdt_env.h needs to
> provide, but I haven't gotten around to it. From memory it's
> basically just the fixed-with integer types, a smallish subset of the
> str*() and mem*() functions, and the endian conversion functions.
>
> I've really tried to keep libfdt's environment dependencies down, so I
> suggest you just start with an empty libfdt_env.h and add stuff based
> on the error messages until the compiler stops whinging about
> undefined things. It shouldn't take long.

Yes, all the functionality is in both linux headers and u-boot headers
that it stole^Winherited from linux. The problem is, the set is
disjoint. If libfdt is willing to change its headers to use
linux/types.h, asm/byteorder.h, and linux/string.h, the problem would go
away:

fdt.h:
-#include <stdint.h>
+#include <linux/types.h>

libfdt_env.h:
-#include <stdint.h>
-#include <string.h>
-#include <endian.h>
-#include <byteswap.h>
+#include <linux/types.h>
+#include <asm/byteorder.h>
+#include <linux/string.h>

Note that asm/byteorder.h provides __be32_to_cpu(x) and friends, which
can be used directly rather than having to synthesize swap/noswap (i.e.
bswap_32(x)) based on #if __BYTE_ORDER == __BIG_ENDIAN. They can be
used directly, or fdt32_to_cpu and friends can be defined in terms
of__be32_to_cpu(x) and friends:
+#define fdt32_to_cpu(x) __be32_to_cpu(x)
+#define cpu_to_fdt32(x) __cpu_to_be32(x)
+#define fdt64_to_cpu(x) __be64_to_cpu(x)
+#define cpu_to_fdt64(x) __cpu_to_be64(x)

When I did the above change and tried to compile it natively under
linux, there were problems with unresolved types. I did not pursue the
resolution of the problem since I am currently concentrating on u-boot
usage.

>> 3) I added a "fdt_next_tag()" function to fdt_ro.c to allow me to step
>> through the blob's tags:
>> uint32_t fdt_next_tag(const void *fdt, int offset,
>> int *nextoffset, char **namep);
>>
>> This is similar to "_fdt_next_tag()" (a "private" function, note the
>> leading underscore) in fdt.c, but with a related but different purpose -
>> the "_fdt_next_tag()" steps through _node_ tags (skipping property
>> tags) where I need to step through all tags sequentially.
>
> Um... no. _fdt_next_tag() steps through all tags (how else could it
> be used internally to find properties...?). If you really need this,
> we can change the function to be exported, which I've considered
> before. However, what are you using this function for? I had some
> node and property traversal functions on the drawing board.

Yes, I copied and augmented _fdt_next_tag():

uint32_t fdt_next_tag(const void *fdt, int offset, int *nextoffset, char
**namep)

to give me a pointer to the node name for node tags and property name
for property tags. Now that I have it working, it would be trivial to
change the calls to _fdt_next_tag() to instead call fdt_next_tag()
passing NULL for the new fourth parameter **namep. ;-)

The reason I need it, I'm printing an unknown tree by stepping through
the tree discovering the node and property names. I need to have
fdt_next_tag() return the *name* of the node/property as well as the tag
so that I can print and indent for nodes or look up the property value
and print the name=value combination.

>> Usability trivia for David: libfdt distinguishes between nodes and
>> properties, which makes sense since that is how the fdt is structured.
>> From a usability standpoint, however, it is annoying to have to
>> separate the property name from the node, find the node, then find the
>> property. I will probably create Yet Another Function:
>> int fdt_split(char *path, char **property);
>> Call it with a path string and the function will separate it into the
>> node portion and the property name. If the path is invalid, it will
>> return an error. If the path is a node, it will set **property to NULL
>> and return the node's offset. If the path is a property, it will return
>> the owning node's offset and set the **property pointer to point to the
>> start of the property portion of the path (i.e. the next character after
>> the last '/').
>
> I don't like combining property and node name into a single path,
> because technically the property names occupy a different namespace
> from subnode names. Insane though it is, there exist some Apple
> device trees where a node has both a property and a subnode of the
> same name.

Oh gaak! What I hear you saying... if you have node a with subnode b
and property b, subnode b has a property c:
/a => node
/a/b => node
/a/b => property (inside node a)
/a/b/c => property (inside node b)

Where I am right now is I created a new function fdt_fullpath_offset:

int fdt_fullpath_offset(const void *fdt, const char *path, char **prop);

which will return the _node_ /a/b in the gaak illustration above. It
looks up nodes until it either runs out of path to look up or there is
an error. On a lookup error, it tries again with the last part of the
path used as a property name. As a result, if you pass in /a it will
return the node "a", if you pass in /a/b it will return the _node_ "b".
This is unchanged behavior compared to fdt_path_offset(). (Getting
property "b" is unchanged: you would have to look up node /a with either
fdt_fullpath_offset(... "/a" ...) or fdt_path_offset(... "/a" ...) and
then use that offset with fdt_property() to get the property "b".)

I've changed the original fdt_path_offset() to simply call
fdt_fullpath_offset() passing in NULL for **prop and everything (should)
work the same as before ("should" because I haven't debugged it yet).

For my u-boot commands, I have "fdt get <prop>" and "fdt print <node>"
and was going to consolidate them into one, but the gaak illustration
says I should _not_ consolidate them. (Trivia: my "fdt print" of "/"
and "/a" will (should anyway ;-) print the gaak tree properly.)

What are the odds that someone will pull an Apple on hardware supported
by u-boot? Hmmm... extra code to handle stupidity.

Best regards,
gvb