libfdt: going forward for u-boot

libfdt: going forward for u-boot

[Jerry Van Baren]

[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]

Hi David,

I'm using libfdt in u-boot and getting close to pushing u-boot changes 
upstream.  I'm curious what libfdt's legal status is WRT dual licensing 
and the legal dogs you've unleashed.

One thought that occurred to me since we last discussed this is to 
relicense it LGPL.  That way it could be linked into proprietary code 
without having to maintain a dual license and the copyright assignment 
headaches that come with outside contributors.

I made some trivial changes to fdt.h and libfdt_env.h to allow libfdt to 
compiler both on linux and for u-boot.  I've attached a patch since it 
_is_ trivial.

Here is the thumbnail sketch of my current changes to use libfdt in u-boot:

Makefile
--------
* Replaced with a u-boot version, unavoidable.

fdt.h
-----
* See attached patchfile for trivial change to allow compiling under
     linux and u-boot.

libfdt_env.h
------------
* See attached patchfile for change to allow compiling under linux
     and u-boot.
* #include <asm/byteorder.h> which gives us direct byteswapping
     utilities without having to do a #if __BYTE_ORDER == __BIG_ENDIAN
#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_be32(x))
    (could change the source to use __be32_to_cpu(x) and friends, but
    I was too lazy to do this).

fdt_ro.c
--------
* Added comments as I figured out how things worked... ;-)
* Added a new function to allow me to step through tags, which I needed
     to recursively print the tree/subtree.  This returns a pointer to
     the name of the next node/property (if appropriate) via **namep.
     The caller can then use the node type (return value) and the string
     to print the property or follow the node.

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

     *This is the only thing I would consider non-trivial, and that is
     seriously debatable since it is mostly a copy of _fdt_next_tag()*

     Not done: because I protected namep against NULL, the existing
     _fdt_next_tag() could be trivially rewritten to be:
     fdt_next_tag(fdt, offset, nextoffset, NULL);

* Added a (void *) typecast to the fdt_getprop() return value to get rid
     of a compiler warning.

fdt_rw.c
--------
* Nonessential improvements to fdt_open_into()
   * Validate the header (fdt_move() does the validation, but I skip
       the move if buf == fdt so I needed to add the validation)
   * Do the fdt_move() only if (buf != fdt) - I call this at times with
       a new (longer) length but don't want to actually move the blob,
       just want the longer length so I can modify the fdt in-place.  The
       current code works, but it seemed silly to do a move if I'm not
       actually moving it.

gvb

[-- Attachment #2: 0001-Use-a-common-header-set-linux-u-boot.txt --]
[-- Type: text/plain, Size: 1783 bytes --]

>From 6772dfdf27e153d0698f7fbecff767417ae76eab Mon Sep 17 00:00:00 2001
From: Gerald Van Baren <vanbaren@cideas.com>
Date: Wed, 21 Mar 2007 19:40:38 -0400
Subject: [PATCH] Use a common header set linux/u-boot

Using the "right" header files along with the __KERNEL__ discriminator
allows libfdt to be compiled under linux (userland) or u-boot.  It
also simplifies the byteswap #defines.

Signed-off-by: Gerald Van Baren <vanbaren@cideas.com>
---
 fdt.h        |    4 ++++
 libfdt_env.h |   21 +++++++++------------
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/fdt.h b/fdt.h
index 4687a31..64507a9 100644
--- a/fdt.h
+++ b/fdt.h
@@ -3,7 +3,11 @@
 
 #ifndef __ASSEMBLY__
 
+#ifndef __KERNEL__
 #include <stdint.h>
+#else
+#include <linux/types.h>
+#endif
 
 struct fdt_header {
 	uint32_t magic;                  /* magic word FDT_MAGIC */
diff --git a/libfdt_env.h b/libfdt_env.h
index 59f2536..dc6562e 100644
--- a/libfdt_env.h
+++ b/libfdt_env.h
@@ -2,21 +2,18 @@
 #define _LIBFDT_ENV_H
 
 #include <stddef.h>
+#ifndef __KERNEL__
 #include <stdint.h>
 #include <string.h>
-#include <endian.h>
-#include <byteswap.h>
-
-#if __BYTE_ORDER == __BIG_ENDIAN
-#define fdt32_to_cpu(x)		(x)
-#define cpu_to_fdt32(x)		(x)
-#define fdt64_to_cpu(x)		(x)
-#define cpu_to_fdt64(x)		(x)
 #else
-#define fdt32_to_cpu(x)		(bswap_32((x)))
-#define cpu_to_fdt32(x)		(bswap_32((x)))
-#define fdt64_to_cpu(x)		(bswap_64((x)))
-#define cpu_to_fdt64(x)		(bswap_64((x)))
+#include <linux/types.h>
+#include <linux/string.h>
 #endif
+#include <asm/byteorder.h>
+
+#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_be32(x))
 
 #endif /* _LIBFDT_ENV_H */
-- 
1.4.4.4

Re: libfdt: going forward for u-boot

[David Gibson]

On Wed, Mar 21, 2007 at 08:08:23PM -0400, Jerry Van Baren wrote:
> Hi David,
> 
> I'm using libfdt in u-boot and getting close to pushing u-boot changes 
> upstream.  I'm curious what libfdt's legal status is WRT dual licensing 
> and the legal dogs you've unleashed.

Um.. still working on it.  I've been through one round with the
lawyers, and I've spent the last few days gathering my energy to
adjust my proposal for another pass.  Progress is happening, albeit
fairly slowly.

> One thought that occurred to me since we last discussed this is to 
> relicense it LGPL.  That way it could be linked into proprietary code 
> without having to maintain a dual license and the copyright assignment 
> headaches that come with outside contributors.

Heh.  The lawyers suggested that too, but I talked them out of it.
The trouble with LGPL is that to satisfy its conditions you need to be
pretty careful about how you link things together.  Working in the
constrained environment of a bootloader also puts technical conditions
on how you can link things.

Satisfying both at once might be possibly, but it's fiddly at the very
least.  I don't want to make life that difficult for libfdt users.  I
think what we'll end up with is GPL with a special exception allowing
fairly unrestricted use of libfdt.  This is like the license on bison,
though the exception text will probably be different.

> I made some trivial changes to fdt.h and libfdt_env.h to allow libfdt to 
> compiler both on linux and for u-boot.  I've attached a patch since it 
> _is_ trivial.

> Here is the thumbnail sketch of my current changes to use libfdt in u-boot:
> 
> Makefile
> --------
> * Replaced with a u-boot version, unavoidable.
> 
> fdt.h
> -----
> * See attached patchfile for trivial change to allow compiling under
>     linux and u-boot.

I'm actually more inclined to just remove the #include from fdt.h
entirely and require users to provide the stdint types, by whatever
means, before they include fdt.h.  It's a bit icky, but I think it's
the sanest way to allow building in multiple environment.

> libfdt_env.h
> ------------

Gah.  Let me say this again:
	libfdt_env.h is *supposed* to be replaced for the environment
	into which you're building libfdt.

	To be replaced is libfdt_env.h's entire purpose in existence.

	The provided libfdt_env.h is an example for userspace builds
	only.

[snip]
> fdt_rw.c
> --------
> * Nonessential improvements to fdt_open_into()
>   * Validate the header (fdt_move() does the validation, but I skip
>       the move if buf == fdt so I needed to add the validation)
>   * Do the fdt_move() only if (buf != fdt) - I call this at times with
>       a new (longer) length but don't want to actually move the blob,
>       just want the longer length so I can modify the fdt in-place.  The
>       current code works, but it seemed silly to do a move if I'm not
>       actually moving it.

If fdt == buf, the memmove() in fdt_move() will become a no-op in any
case.  I don't see any point to conditionalizing the fdt_move().

-- 
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: libfdt: going forward for u-boot

[Jerry Van Baren]

David Gibson wrote:
> On Wed, Mar 21, 2007 at 08:08:23PM -0400, Jerry Van Baren wrote:
>> Hi David,
>>
>> I'm using libfdt in u-boot and getting close to pushing u-boot changes 
>> upstream.  I'm curious what libfdt's legal status is WRT dual licensing 
>> and the legal dogs you've unleashed.
> 
> Um.. still working on it.  I've been through one round with the
> lawyers, and I've spent the last few days gathering my energy to
> adjust my proposal for another pass.  Progress is happening, albeit
> fairly slowly.

Good luck.

[snip]

>> fdt.h
>> -----
>> * See attached patchfile for trivial change to allow compiling under
>>     linux and u-boot.
> 
> I'm actually more inclined to just remove the #include from fdt.h
> entirely and require users to provide the stdint types, by whatever
> means, before they include fdt.h.  It's a bit icky, but I think it's
> the sanest way to allow building in multiple environment.

Yeah, chose your icky.  The icky I did was to use __KERNEL__ because 
u-boot -D defines it.  That implies it should compile in a linux kernel 
environment, but I didn't verify that it actually does compile.  Same 
file, different icky.

>> libfdt_env.h
>> ------------
> 
> Gah.  Let me say this again:
> 	libfdt_env.h is *supposed* to be replaced for the environment
> 	into which you're building libfdt.
> 
> 	To be replaced is libfdt_env.h's entire purpose in existence.
> 
> 	The provided libfdt_env.h is an example for userspace builds
> 	only.

Understood, but if I can do a (fairly) trivial change that allows me to 
compile under linux for regression testing and u-boot without changing 
the file, the lazy engineer in me says Yaaaya!  Maybe I'm just working 
to hard. :-/  Different file, same icky.

>> fdt_rw.c
>> --------
>> * Nonessential improvements to fdt_open_into()
>>   * Validate the header (fdt_move() does the validation, but I skip
>>       the move if buf == fdt so I needed to add the validation)
>>   * Do the fdt_move() only if (buf != fdt) - I call this at times with
>>       a new (longer) length but don't want to actually move the blob,
>>       just want the longer length so I can modify the fdt in-place.  The
>>       current code works, but it seemed silly to do a move if I'm not
>>       actually moving it.
> 
> If fdt == buf, the memmove() in fdt_move() will become a no-op in any
> case.  I don't see any point to conditionalizing the fdt_move().

But it allows me to make the blob bigger.  Ahh, it looks like all I need 
to do is:
         fdt_set_header(fdt, totalsize, bufsize);
so I agree, I'm working way too hard on this one.

gvb

Re: libfdt: going forward for u-boot

[David Gibson]

On Wed, Mar 21, 2007 at 09:12:31PM -0400, Jerry Van Baren wrote:
> David Gibson wrote:
> > On Wed, Mar 21, 2007 at 08:08:23PM -0400, Jerry Van Baren wrote:
> >> Hi David,
> >>
> >> I'm using libfdt in u-boot and getting close to pushing u-boot changes 
> >> upstream.  I'm curious what libfdt's legal status is WRT dual licensing 
> >> and the legal dogs you've unleashed.
> > 
> > Um.. still working on it.  I've been through one round with the
> > lawyers, and I've spent the last few days gathering my energy to
> > adjust my proposal for another pass.  Progress is happening, albeit
> > fairly slowly.
> 
> Good luck.
> 
> [snip]
> 
> >> fdt.h
> >> -----
> >> * See attached patchfile for trivial change to allow compiling under
> >>     linux and u-boot.
> > 
> > I'm actually more inclined to just remove the #include from fdt.h
> > entirely and require users to provide the stdint types, by whatever
> > means, before they include fdt.h.  It's a bit icky, but I think it's
> > the sanest way to allow building in multiple environment.
> 
> Yeah, chose your icky.  The icky I did was to use __KERNEL__ because 
> u-boot -D defines it.  That implies it should compile in a linux kernel 
> environment, but I didn't verify that it actually does compile.  Same 
> file, different icky.
> 
> >> libfdt_env.h
> >> ------------
> > 
> > Gah.  Let me say this again:
> > 	libfdt_env.h is *supposed* to be replaced for the environment
> > 	into which you're building libfdt.
> > 
> > 	To be replaced is libfdt_env.h's entire purpose in existence.
> > 
> > 	The provided libfdt_env.h is an example for userspace builds
> > 	only.
> 
> Understood, but if I can do a (fairly) trivial change that allows me to 
> compile under linux for regression testing and u-boot without changing 
> the file, the lazy engineer in me says Yaaaya!  Maybe I'm just working 
> to hard. :-/  Different file, same icky.

Once you have a uboot version of libfdt_env.h (which you can do by
copying the example and tweaking slightly) it shouldn't be any
harder.  Just move across all the files except libfdt_env.h.

> >> fdt_rw.c
> >> --------
> >> * Nonessential improvements to fdt_open_into()
> >>   * Validate the header (fdt_move() does the validation, but I skip
> >>       the move if buf == fdt so I needed to add the validation)
> >>   * Do the fdt_move() only if (buf != fdt) - I call this at times with
> >>       a new (longer) length but don't want to actually move the blob,
> >>       just want the longer length so I can modify the fdt in-place.  The
> >>       current code works, but it seemed silly to do a move if I'm not
> >>       actually moving it.
> > 
> > If fdt == buf, the memmove() in fdt_move() will become a no-op in any
> > case.  I don't see any point to conditionalizing the fdt_move().
> 
> But it allows me to make the blob bigger.  Ahh, it looks like all I need 
> to do is:
>          fdt_set_header(fdt, totalsize, bufsize);
> so I agree, I'm working way too hard on this one.

No, if you want to make more space to do edits in, you should
definitely be using fdt_open_into() - that's the purpose of
fdt_open_into().  I'm just saying there's no need to conditionalize
the fdt_move(), because fdt_move() *already* correctly handles the
case where fdt==buf.

-- 
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: libfdt: going forward for u-boot

[Jerry Van Baren]

David Gibson wrote:
> On Wed, Mar 21, 2007 at 09:12:31PM -0400, Jerry Van Baren wrote:
>> David Gibson wrote:

[snip]

>>>> fdt_rw.c
>>>> --------
>>>> * Nonessential improvements to fdt_open_into()
>>>>   * Validate the header (fdt_move() does the validation, but I skip
>>>>       the move if buf == fdt so I needed to add the validation)
>>>>   * Do the fdt_move() only if (buf != fdt) - I call this at times with
>>>>       a new (longer) length but don't want to actually move the blob,
>>>>       just want the longer length so I can modify the fdt in-place.  The
>>>>       current code works, but it seemed silly to do a move if I'm not
>>>>       actually moving it.
>>> If fdt == buf, the memmove() in fdt_move() will become a no-op in any
>>> case.  I don't see any point to conditionalizing the fdt_move().
>> But it allows me to make the blob bigger.  Ahh, it looks like all I need 
>> to do is:
>>          fdt_set_header(fdt, totalsize, bufsize);
>> so I agree, I'm working way too hard on this one.
> 
> No, if you want to make more space to do edits in, you should
> definitely be using fdt_open_into() - that's the purpose of
> fdt_open_into().  I'm just saying there's no need to conditionalize
> the fdt_move(), because fdt_move() *already* correctly handles the
> case where fdt==buf.

Not in my copy unless the memmove() implementation counts.
--------------------------------
int fdt_move(const void *fdt, void *buf, int bufsize)
{
         int err = _fdt_check_header(fdt);

         if (err)
                 return err;

         if (fdt_totalsize(fdt) > bufsize)
                 return -FDT_ERR_NOSPACE;

         memmove(buf, fdt, fdt_totalsize(fdt));
         return 0;
}
--------------------------------
--- a/fdt_rw.c
+++ b/fdt_rw.c
@@ -260,11 +260,19 @@ int fdt_del_node(void *fdt, int nodeoffset)

  int fdt_open_into(void *fdt, void *buf, int bufsize)
  {
-       int err;
-
-       err = fdt_move(fdt, buf, bufsize);
-       if (err)
-               return err;
+        int err = rw_check_header(fdt);
+
+        if (err)
+                return err;
+
+       /*
+        * If we are just expanding the size in-place, no need to do the 
move.
+        */
+       if (buf != fdt) {
+               err = fdt_move(fdt, buf, bufsize);
+               if (err)
+                       return err;
+       }

         fdt = buf;

--------------------------------
(Yes, the above is whitespace damaged.  It ain't a patch ;-)

Best regards,
gvb

Re: libfdt: going forward for u-boot

[David Gibson]

On Wed, Mar 21, 2007 at 09:37:24PM -0400, Jerry Van Baren wrote:
> David Gibson wrote:
> > On Wed, Mar 21, 2007 at 09:12:31PM -0400, Jerry Van Baren wrote:
> >> David Gibson wrote:
> 
> [snip]
> 
> >>>> fdt_rw.c
> >>>> --------
> >>>> * Nonessential improvements to fdt_open_into()
> >>>>   * Validate the header (fdt_move() does the validation, but I skip
> >>>>       the move if buf == fdt so I needed to add the validation)
> >>>>   * Do the fdt_move() only if (buf != fdt) - I call this at times with
> >>>>       a new (longer) length but don't want to actually move the blob,
> >>>>       just want the longer length so I can modify the fdt in-place.  The
> >>>>       current code works, but it seemed silly to do a move if I'm not
> >>>>       actually moving it.
> >>> If fdt == buf, the memmove() in fdt_move() will become a no-op in any
> >>> case.  I don't see any point to conditionalizing the fdt_move().
> >> But it allows me to make the blob bigger.  Ahh, it looks like all I need 
> >> to do is:
> >>          fdt_set_header(fdt, totalsize, bufsize);
> >> so I agree, I'm working way too hard on this one.
> > 
> > No, if you want to make more space to do edits in, you should
> > definitely be using fdt_open_into() - that's the purpose of
> > fdt_open_into().  I'm just saying there's no need to conditionalize
> > the fdt_move(), because fdt_move() *already* correctly handles the
> > case where fdt==buf.
> 
> Not in my copy unless the memmove() implementation counts.

The memmove() implementation is what I mean.  memmove() is defined to
handle overlapping areas - fdt==buf is a degenerate case of
overlapping areas.

-- 
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: libfdt: going forward for u-boot

[Segher Boessenkool]

>> One thought that occurred to me since we last discussed this is to
>> relicense it LGPL.  That way it could be linked into proprietary code
>> without having to maintain a dual license and the copyright assignment
>> headaches that come with outside contributors.
>
> Heh.  The lawyers suggested that too, but I talked them out of it.
> The trouble with LGPL is that to satisfy its conditions you need to be
> pretty careful about how you link things together.

Yeah, the LGPL is even more work for lawyers than the GPL.

You could try a BSD- or MIT-style license perhaps, those
are *really* easy to understand, even to lawyers :-)


Segher

Re: libfdt: going forward for u-boot

[Hollis Blanchard]

On Thu, 2007-03-22 at 11:49 +1100, David Gibson wrote:
> 
> I'm actually more inclined to just remove the #include from fdt.h
> entirely and require users to provide the stdint types, by whatever
> means, before they include fdt.h.  It's a bit icky, but I think it's
> the sanest way to allow building in multiple environment. 

I agree. I've done something similar in a few places now, and it's the
best solution I've found.

-Hollis

Re: libfdt: going forward for u-boot

[David Gibson]

On Thu, Mar 22, 2007 at 11:42:06AM -0500, Hollis Blanchard wrote:
> On Thu, 2007-03-22 at 11:49 +1100, David Gibson wrote:
> > 
> > I'm actually more inclined to just remove the #include from fdt.h
> > entirely and require users to provide the stdint types, by whatever
> > means, before they include fdt.h.  It's a bit icky, but I think it's
> > the sanest way to allow building in multiple environment. 
> 
> I agree. I've done something similar in a few places now, and it's the
> best solution I've found.

I just pushed out a new libfdt which removes the #include from fdt.h
as discussed.

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