[U-Boot-Users] libfdt release?

[David Gibson]

On Fri, Oct 12, 2007 at 07:58:28AM -0400, Jerry Van Baren wrote:
> David Gibson wrote:
> >On Thu, Oct 11, 2007 at 07:35:45PM -0400, Jerry Van Baren wrote:
> 
> [snip]
> 
> >>>>>* Add some utilities to manipulate the reserved memory map. (think
> >>>>>your recent patch my cover that)
> >>>>Should do.
> >>>>
> >>>>>* libfdt: Make fdt_check_header() public
> >>>>Hrm.  Why?
> >>>Jerry?
> >>Because we needed to check the validity of the header outside of libfdt 
> >>and decide whether we wanted to use the blob or not.
> >
> >That seems reasonable.  I'll look at exporting that.  Although.. I'm a
> >little surprised that u-boot doesn't just to an fdt_open_into() as the
> >first thing, which would include the check.
> 
> Yes, that would be the alternative, but fdt_open_into() potentially 
> changes things (copies the blob) and doing an fdt_open_into() just to 
> check the validity of the header is not intuitive.
> 
> While it is true that, by giving the same start and end address, you can 
> avoid doing the actual copy, it isn't obvious that the read/write 
> doesn't actually occur if source == dest... the code goes through all 
> the motions and relies on the underlying memory utility library to short 
> circuit the actual operation.  If the underlying memory utility library 
> *doesn't* short circuit the operation, we could have severe problems 
> with blobs that are burned into flash (potential write protection 
> exceptions).
> 
> I submit that is a lot of ugly to avoid a simple export of an already 
> existing, useful, function.

Yeah, fair enough.  I'll export the function.

> >>>>>* libfdt: Enhanced and published fdt_next_tag()
> >>>>Hrm.  I'm not categorically opposed to publishing fdt_next_tag(), but
> >>>>I'm disinclined to do so without good reason.
> >>>Jerry?
> >>>
> >>>Hopefully Jerry will comment on why the additions were made to u-boot.
> >>I needed it to implement my u-boot "fdt print" command, to step through 
> >>the tags so I could print them out in a human readable format (it 
> >>actually is dtc-input-compatible and, in most cases, matches the 
> >>original dtc input source code).
> >
> >Hrm, I see.  For debugging purposes, essentially.
> >
> >I have been thinking for some time that I needed to add user-accesible
> >traversal functions - I just haven't managed to come up with an
> >interface I like, yet.
> 
> More than debugging purposes, the "fdt print" command is extremely 
> useful for the u-boot command line to see what you have to start with, 
> figure out what parameters need to be modified, and format a correct 
> string to do the modification.  This is doubly important when the blob 
> was created /n/ steps before you got it (a board manufacturer, reseller, 
> or some poor luser) and you have no clue what the blob is to start with.

That would be debugging purposes.  I didn't mean to imply that
debugging purposes were unimportant..

> It would be very useful for the kernel as well, to implement the /proc 
> blob display.  Again, on the face of it, your "debugging purposes" would 
> apply, but it is essential debugging, not nicety debugging.  I suspect 
> kernel developers are going to be a lot happier getting an ascii dump of 
> /proc/fdt/asciiblob (sorry, forgot where it lives) than a mime encoded 
> binary lump that they then have to decode (only to find out that the 
> poor luser didn't dd or mime encode it properly).

Uh.. how did mime encoding get into this!?  We already have
/proc/device-tree giving the unpacked device tree.  I'm not sure if we
currently export the blob or not, but if we did it should be as a raw
binary.  A user can point dtc at that to decode it, there's absolutely
no call for the kernel to do so.

> In common/cmd_fdt.c line 551 (line 601 is where the traversal starts) 
> you can see how I used it to walk through the blob:
> <http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=common/cmd_fdt.c;h=571b8f14d56f3bc97dcd49eb8465cfdc9988d786;hb=HEAD#l551>
> 
> IMHO it is an ideal user-accessible traversal function.  It is simple 
> yet sufficient. 

The existing function was already sufficient - and would have
discouraged at least one stupidity in the way you use it in this
example.

If you want to look into the details of the returned tags, it's easy
to use fdt_offset_ptr() to look at the data in place as either a
struct fdt_node_header or struct fdt_property (or anything else).

Using that approach for properties, you wouldn't need to *re*-iterate
from the last BEGIN_NODE with fdt_getprop() to find the property that
next_tag() just found anyway.  Which would in turn entirely eliminate
the need for offstack.

Oh, *and* current libfdt has fdt_get_name() which makes it even easier
to get a node's name, given it's offset.

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