Patchwork [U-Boot,1/5] treewide: include libfdt_env.h before fdt.h

login
register
mail settings
Submitter Kim Phillips
Date Jan. 16, 2013, 11:59 p.m.
Message ID <20130116175904.47ee25bd2596f28796698759@freescale.com>
Download mbox | patch
Permalink /patch/213107/
State Accepted
Delegated to: Jerry Van Baren
Headers show

Comments

Kim Phillips - Jan. 16, 2013, 11:59 p.m.
and, if including libfdt.h which includes libfdt_env.h in
the correct order, don't include fdt.h before libfdt.h.

this is needed to get the fdt type definitions set from
the project environment before fdt.h uses them.

Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
Cc: Jerry Van Baren <gvb.uboot@gmail.com>
---
 arch/arm/lib/bootm.c                         | 1 -
 arch/powerpc/lib/bootm.c                     | 1 -
 arch/x86/include/asm/arch-coreboot/sysinfo.h | 1 +
 common/cmd_bootm.c                           | 1 -
 common/cmd_fdt.c                             | 1 -
 common/fdt_support.c                         | 1 -
 common/image.c                               | 1 -
 include/fdt_support.h                        | 1 +
 include/image.h                              | 1 -
 tools/fdt_host.h                             | 1 -
 tools/imls/imls.c                            | 1 -
 11 files changed, 2 insertions(+), 9 deletions(-)
Scott Wood - Jan. 17, 2013, 12:36 a.m.
On 01/16/2013 05:59:04 PM, Kim Phillips wrote:
> and, if including libfdt.h which includes libfdt_env.h in
> the correct order, don't include fdt.h before libfdt.h.
> 
> this is needed to get the fdt type definitions set from
> the project environment before fdt.h uses them.
> 
> Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
> Cc: Jerry Van Baren <gvb.uboot@gmail.com>

Maybe fdt.h should include libfdt_env.h?

Or just always use libfdt.h as the public header.

-Scott
Kim Phillips - Jan. 17, 2013, 5:54 p.m.
On Wed, 16 Jan 2013 18:36:03 -0600
Scott Wood <scottwood@freescale.com> wrote:

> On 01/16/2013 05:59:04 PM, Kim Phillips wrote:
> > and, if including libfdt.h which includes libfdt_env.h in
> > the correct order, don't include fdt.h before libfdt.h.
> > 
> > this is needed to get the fdt type definitions set from
> > the project environment before fdt.h uses them.
> > 
> > Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
> > Cc: Jerry Van Baren <gvb.uboot@gmail.com>
> 
> Maybe fdt.h should include libfdt_env.h?
> 
> Or just always use libfdt.h as the public header.

Was just following along the same lines as the dtc commits 38ad79d3
"dtc/tests: don't include fdt.h prior to libfdt.h" and 20b866a7
"dtc/fdtdump: include libfdt_env.h prior to fdt.h", acked by David
G.  I don't know why some only include fdt.h.

devicetree-discuss/David: is there a prescribed way to go here?
Change all fdt.h includers to just always include libfdt.h instead
of libfdt_env.h prior to fdt.h?

Kim
Jerry Van Baren - Jan. 17, 2013, 6:32 p.m.
Hi Scott, Kim, David,

On 01/17/2013 12:54 PM, Kim Phillips wrote:
> On Wed, 16 Jan 2013 18:36:03 -0600
> Scott Wood <scottwood@freescale.com> wrote:
>
>> On 01/16/2013 05:59:04 PM, Kim Phillips wrote:
>>> and, if including libfdt.h which includes libfdt_env.h in
>>> the correct order, don't include fdt.h before libfdt.h.
>>>
>>> this is needed to get the fdt type definitions set from
>>> the project environment before fdt.h uses them.
>>>
>>> Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
>>> Cc: Jerry Van Baren <gvb.uboot@gmail.com>
>>
>> Maybe fdt.h should include libfdt_env.h?
>>
>> Or just always use libfdt.h as the public header.
>
> Was just following along the same lines as the dtc commits 38ad79d3
> "dtc/tests: don't include fdt.h prior to libfdt.h" and 20b866a7
> "dtc/fdtdump: include libfdt_env.h prior to fdt.h", acked by David
> G.  I don't know why some only include fdt.h.
>
> devicetree-discuss/David: is there a prescribed way to go here?
> Change all fdt.h includers to just always include libfdt.h instead
> of libfdt_env.h prior to fdt.h?

I started applying Kim's "sparse" patches to the u-boot source and ran 
into this issue pretty hard.

In u-boot, there is an added complexity that the "tools" (host-based 
u-boot support tools) support flattened device trees, but explicitly 
include the u-boot version of libfdt declarations so they don't fall out 
of sync if the host has a non-compatible libfdt version.  Having them 
out of sync would be a *horrible* situation to sort out - everything 
would build OK but nothing would work right, probably with no useful 
diagnostic information.  This originated in 2008, so life may be better 
nowadays.  Or maybe not.

I would be in favor of explicitly including all the *fdt* headers in the 
sources.  Alternately, Scott's suggestion of just including libfdt.h as 
the public header seems good, but I'm pretty sure it will mess me up 
with the explicit #including in the host-based "tools" build, leaving us 
with nasty work-arounds or a risk of hard to identify incompatible host 
vs. u-boot fdt versions.

Who Includes Who

fdt.h - no includes

fdt_support.h - (u-boot only file)
   29 #include <fdt.h>

libfdt.h
   54 #include <libfdt_env.h>
   55 #include <fdt.h>

libfdt_env.h
  - u-boot version is minimal, uses pre-existing macros for byte swapping
  - dtc version implements byte swapping, includes:
    4 #include <stddef.h>
    5 #include <stdint.h>
    6 #include <string.h>

libfdt_env.h is where Kim typedef'ed fdt16_t, fdt32_t, fdt64_t

I suspect the original intent was to have <libfdt.h> be the file that 
people #included.  For whatever reason, most includes are (picking on 
fdt_ro.c arbitrarily):
   51 #include "libfdt_env.h"
   53 #include <fdt.h>
   54 #include <libfdt.h>
Since libfdt.h #includes fdt.h and libfdt_env.h, lines 51 and 53 (above) 
are redundant.  It sorts out OK in dtc because libfdt_env.h includes 
stdint.h and defines fdt*_t, but it messes me up in u-boot where 
(currently) libfdt_env.h does *not* include stdint.h... and then things 
get really wonky in the u-boot "tools" directory due to the need to use 
the u-boot version of the *fdt*.h headers, not random stuff installed on 
the computer.

HTH,
gvb
David Gibson - Jan. 17, 2013, 11:48 p.m.
On Thu, Jan 17, 2013 at 11:54:56AM -0600, Kim Phillips wrote:
> On Wed, 16 Jan 2013 18:36:03 -0600
> Scott Wood <scottwood@freescale.com> wrote:
> 
> > On 01/16/2013 05:59:04 PM, Kim Phillips wrote:
> > > and, if including libfdt.h which includes libfdt_env.h in
> > > the correct order, don't include fdt.h before libfdt.h.
> > > 
> > > this is needed to get the fdt type definitions set from
> > > the project environment before fdt.h uses them.
> > > 
> > > Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
> > > Cc: Jerry Van Baren <gvb.uboot@gmail.com>
> > 
> > Maybe fdt.h should include libfdt_env.h?
> > 
> > Or just always use libfdt.h as the public header.
> 
> Was just following along the same lines as the dtc commits 38ad79d3
> "dtc/tests: don't include fdt.h prior to libfdt.h" and 20b866a7
> "dtc/fdtdump: include libfdt_env.h prior to fdt.h", acked by David
> G.  I don't know why some only include fdt.h.
> 
> devicetree-discuss/David: is there a prescribed way to go here?
> Change all fdt.h includers to just always include libfdt.h instead
> of libfdt_env.h prior to fdt.h?

Yeah, I think just including libfdt.h instead of fdt.h is the way to
go.  The distinction is that fdt.h contains only "passive"
declarations.  That is, defines and structure/type declarations but no
function prorotypes or other code.  In particular that means that it
is strictly about the FDT tree structure only, and not any of the
libfdt specific entry points.
David Gibson - Jan. 17, 2013, 11:50 p.m.
On Thu, Jan 17, 2013 at 01:32:45PM -0500, Jerry Van Baren wrote:
> Hi Scott, Kim, David,
> 
> On 01/17/2013 12:54 PM, Kim Phillips wrote:
> >On Wed, 16 Jan 2013 18:36:03 -0600
> >Scott Wood <scottwood@freescale.com> wrote:
> >
> >>On 01/16/2013 05:59:04 PM, Kim Phillips wrote:
> >>>and, if including libfdt.h which includes libfdt_env.h in
> >>>the correct order, don't include fdt.h before libfdt.h.
> >>>
> >>>this is needed to get the fdt type definitions set from
> >>>the project environment before fdt.h uses them.
> >>>
> >>>Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
> >>>Cc: Jerry Van Baren <gvb.uboot@gmail.com>
> >>
> >>Maybe fdt.h should include libfdt_env.h?
> >>
> >>Or just always use libfdt.h as the public header.
> >
> >Was just following along the same lines as the dtc commits 38ad79d3
> >"dtc/tests: don't include fdt.h prior to libfdt.h" and 20b866a7
> >"dtc/fdtdump: include libfdt_env.h prior to fdt.h", acked by David
> >G.  I don't know why some only include fdt.h.
> >
> >devicetree-discuss/David: is there a prescribed way to go here?
> >Change all fdt.h includers to just always include libfdt.h instead
> >of libfdt_env.h prior to fdt.h?
> 
> I started applying Kim's "sparse" patches to the u-boot source and
> ran into this issue pretty hard.
> 
> In u-boot, there is an added complexity that the "tools" (host-based
> u-boot support tools) support flattened device trees, but explicitly
> include the u-boot version of libfdt declarations so they don't fall
> out of sync if the host has a non-compatible libfdt version.  Having
> them out of sync would be a *horrible* situation to sort out -
> everything would build OK but nothing would work right, probably
> with no useful diagnostic information.  This originated in 2008, so
> life may be better nowadays.  Or maybe not.
> 
> I would be in favor of explicitly including all the *fdt* headers in
> the sources.  Alternately, Scott's suggestion of just including
> libfdt.h as the public header seems good, but I'm pretty sure it
> will mess me up with the explicit #including in the host-based
> "tools" build, leaving us with nasty work-arounds or a risk of hard
> to identify incompatible host vs. u-boot fdt versions.
> 
> Who Includes Who
> 
> fdt.h - no includes
> 
> fdt_support.h - (u-boot only file)
>   29 #include <fdt.h>
> 
> libfdt.h
>   54 #include <libfdt_env.h>
>   55 #include <fdt.h>
> 
> libfdt_env.h
>  - u-boot version is minimal, uses pre-existing macros for byte swapping
>  - dtc version implements byte swapping, includes:
>    4 #include <stddef.h>
>    5 #include <stdint.h>
>    6 #include <string.h>
> 
> libfdt_env.h is where Kim typedef'ed fdt16_t, fdt32_t, fdt64_t
> 
> I suspect the original intent was to have <libfdt.h> be the file
> that people #included.  For whatever reason, most includes are
> (picking on fdt_ro.c arbitrarily):
>   51 #include "libfdt_env.h"
>   53 #include <fdt.h>
>   54 #include <libfdt.h>
> Since libfdt.h #includes fdt.h and libfdt_env.h, lines 51 and 53
> (above) are redundant.  It sorts out OK in dtc because libfdt_env.h
> includes stdint.h and defines fdt*_t, but it messes me up in u-boot
> where (currently) libfdt_env.h does *not* include stdint.h...

Ok, so, the uboot libfdt_env.h should be fixed to define uintXX_t and
fdtXX_t (either by including stdint or my other means).  The purpose
of libfdt_env.h is to define the things that libfdt requires, and
those types are (now) such a requirement.
Jerry Van Baren - Jan. 18, 2013, 12:02 p.m.
Hi Kim,

On 01/16/2013 06:59 PM, Kim Phillips wrote:
> and, if including libfdt.h which includes libfdt_env.h in
> the correct order, don't include fdt.h before libfdt.h.
> 
> this is needed to get the fdt type definitions set from
> the project environment before fdt.h uses them.
> 
> Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
> Cc: Jerry Van Baren <gvb.uboot@gmail.com>

Thanks, that was very helpful.  I applied the series to my personal
tree, will push when the dust settles.

I created a patch that uses just libfdt.h as the interface #include,
will publish when the dust settles on the dtc (device tree) list discussion.

Thanks,
gvb

Patch

diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 1bd2730..f3b30c5 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -30,7 +30,6 @@ 
 #include <image.h>
 #include <u-boot/zlib.h>
 #include <asm/byteorder.h>
-#include <fdt.h>
 #include <libfdt.h>
 #include <fdt_support.h>
 #include <asm/bootm.h>
diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c
index 7088293..33d013f 100644
--- a/arch/powerpc/lib/bootm.c
+++ b/arch/powerpc/lib/bootm.c
@@ -36,7 +36,6 @@ 
 #include <asm/mp.h>
 
 #if defined(CONFIG_OF_LIBFDT)
-#include <fdt.h>
 #include <libfdt.h>
 #include <fdt_support.h>
 
diff --git a/arch/x86/include/asm/arch-coreboot/sysinfo.h b/arch/x86/include/asm/arch-coreboot/sysinfo.h
index 77ae304..bd88eb5 100644
--- a/arch/x86/include/asm/arch-coreboot/sysinfo.h
+++ b/arch/x86/include/asm/arch-coreboot/sysinfo.h
@@ -32,6 +32,7 @@ 
 
 #include <common.h>
 #include <compiler.h>
+#include <libfdt_env.h>
 #include <fdt.h>
 #include <asm/arch/tables.h>
 
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index 1b8a8c1..e886c4a 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -47,7 +47,6 @@ 
 #endif
 
 #if defined(CONFIG_OF_LIBFDT)
-#include <fdt.h>
 #include <libfdt.h>
 #include <fdt_support.h>
 #endif
diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c
index 9e2de34..6eec947 100644
--- a/common/cmd_fdt.c
+++ b/common/cmd_fdt.c
@@ -29,7 +29,6 @@ 
 #include <linux/ctype.h>
 #include <linux/types.h>
 #include <asm/global_data.h>
-#include <fdt.h>
 #include <libfdt.h>
 #include <fdt_support.h>
 
diff --git a/common/fdt_support.c b/common/fdt_support.c
index 6b9fa05..7a3fee4 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -28,7 +28,6 @@ 
 #include <linux/ctype.h>
 #include <linux/types.h>
 #include <asm/global_data.h>
-#include <fdt.h>
 #include <libfdt.h>
 #include <fdt_support.h>
 #include <exports.h>
diff --git a/common/image.c b/common/image.c
index 95498e6..ae1a9d3 100644
--- a/common/image.c
+++ b/common/image.c
@@ -47,7 +47,6 @@ 
 #include <image.h>
 
 #if defined(CONFIG_FIT) || defined(CONFIG_OF_LIBFDT)
-#include <fdt.h>
 #include <libfdt.h>
 #include <fdt_support.h>
 #endif
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 4b9f84a..a08c90b 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -26,6 +26,7 @@ 
 
 #ifdef CONFIG_OF_LIBFDT
 
+#include <libfdt_env.h>
 #include <fdt.h>
 
 u32 fdt_getprop_u32_default(const void *fdt, const char *path,
diff --git a/include/image.h b/include/image.h
index f5adc50..8e285f9 100644
--- a/include/image.h
+++ b/include/image.h
@@ -52,7 +52,6 @@ 
 #endif /* USE_HOSTCC */
 
 #if defined(CONFIG_FIT)
-#include <fdt.h>
 #include <libfdt.h>
 #include <fdt_support.h>
 #define CONFIG_MD5		/* FIT images need MD5 support */
diff --git a/tools/fdt_host.h b/tools/fdt_host.h
index 085013e..b08bf86 100644
--- a/tools/fdt_host.h
+++ b/tools/fdt_host.h
@@ -21,7 +21,6 @@ 
 #define __FDT_HOST_H__
 
 /* Make sure to include u-boot version of libfdt include files */
-#include "../include/fdt.h"
 #include "../include/libfdt.h"
 #include "../include/fdt_support.h"
 
diff --git a/tools/imls/imls.c b/tools/imls/imls.c
index b21c505..a982a13 100644
--- a/tools/imls/imls.c
+++ b/tools/imls/imls.c
@@ -38,7 +38,6 @@ 
 #endif
 
 #include <sha1.h>
-#include <fdt.h>
 #include <libfdt.h>
 #include <fdt_support.h>
 #include <image.h>