On Wed, Oct 17, 2012 at 08:19:23PM -0400, Jerry Van Baren wrote:
> Hi David, Jon,
>
> Kim Phillips created a series of patches to change variable declarations
> that are big endian to be __be32/__be64. Since the device tree is
> defined to be big endian, he created a patch to mark the appropriate
> libfdt entities as __be*.
So, in general I approve the idea of having endian annotations.
Obviously we do need to make sure it doesn't break things.
> On 10/16/2012 08:28 PM, Kim Phillips wrote:
> > This 32-patch series only begins to address making u-boot source more
> > 'sparseable,' or sparse-clean, ultimately to catch type, address space,
> > and endianness mismatches and generally improve code quality. E.g., in this
> > initial dose whose main purpose is to reduce the output volume to workable
> > levels, a couple of endianness bugs are found and fixed in
> > of_bus_default_translate() and fdt_get_base_address(). See [PATCH 14/32]
> > common/fdt_support.c: sparse fixes.
>
> Upside: This is very good for identifying endian errors early.
> Downside: It could break/complicate non-linux uses of libfdt.
This shouldn't be an inherent problem - we can always have the default
behaviour be that be32 etc. are #defined to be uint32_t, and we only
turn on "real" annotations when we have the right setup.
It does complicate things a bit, but I think it should be manageable.
I'd much prefer to see this done against the upstream dtc/libfdt, of
course, rather than the u-boot copy.
> What are your thoughts on this quest?
>
> [snip]
>
> > Note that there are two libfdt dependencies:
>
> [snipped #1, u-boot-fdt dependency, NBD]
>
> > 2. potential upstream dtc change dependencies, due to having to attribute
> > base
> > device tree header types to __be32 in include/libfdt. See patch 19/32
> > "include/fdt.h: sparse fixes". It is unknown whether such changes would
> > be welcome to dtc (but there's a way to find out :).
>
> [snip]
>
> > Build-tested in both endians :). Please help test.
> >
> > Thanks,
> >
> > Kim
>
> Below is the actual patch for reference (it was in a separate email).
> The impact in terms of changed lines is pretty small. I'm not sure how
> this impacts non-linux / non-gcc systems since the sparse checker comes
> from a linux background and uses gcc extensions.
>
> Possibly this could be handled a definition:
>
> #ifndef _LINUX_TYPES_H
> typedef uint32_t __be32
> typedef uint64_t __be64
> #endif
Yes, something along those lines would be appropriate, although I
think that condition isn't right.
> > include/fdt.h | 33 +++++++++++++++++----------------
> > include/fdt_support.h | 2 ++
> > include/libfdt.h | 4 ++--
> > lib/libfdt/fdt.c | 2 +-
> > lib/libfdt/fdt_ro.c | 2 +-
> > lib/libfdt/fdt_rw.c | 4 ++--
> > lib/libfdt/fdt_sw.c | 4 ++--
> > lib/libfdt/fdt_wip.c | 2 +-
> > 8 files changed, 28 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/fdt.h b/include/fdt.h
> > index c51212e..1b7f044 100644
> > --- a/include/fdt.h
> > +++ b/include/fdt.h
> > @@ -2,40 +2,41 @@
> > #define _FDT_H
> >
> > #ifndef __ASSEMBLY__
> > +#include <asm/byteorder.h>
This change, however, is not acceptable. libfdt is supposed to
compile in a wide range of environments (such as bootloaders and
firmwares) which may be very different from a typical Unix userland
environment. As such all the headers are built to have minimal
dependencies on external libraries. The byteorder headers are, alas,
horribly non-portable even amongst otherwise similar Unix systems, so
right out for libfdt.
In fact, the way this is supposed to work is that the *only* external
header directly included by the fdt headers is libfdt_env.h. It is
supplied by the surrounding build environment and is responsible for
providing the minimal things that libfdt does require. Roughly
speaking that's: stdint like types, some byteswap functions and a some
string.h prototypes - exactly what's required should be better
documented than it currently is. libfdt_env.h can do this either by
including appropriate pre-existing headers from the environment, or by
directly defining the required things, whichever makes sense. The
libfdt_env.h which is shipped with libfdt is essentially just an
example, for use in the environment of "POSIX like userspace".
Anyway, I think the right way to handle this is to decide on a name
like __FDT_ANNOTATIONS__ or something. In fdt.h we have
#ifndef __FDT_ANNOTATIONS__
typedef uint32_t fdt32_t;
/* etc */
#endif
And libfdt is reworked to use these fdt32_t and so forth types. Then,
by default everything should compile fine, but with no extra checking.
Environments which have sparse or a similar checker available can
#define __FDT_ANNOTATIONS__ from their version of libfdt_env.h in
which case they would also be required to define the annotated integer
types as necessary for their checker.
For convenience on Linux systems we can have the "default"
libfdt_env.h shipped with libfdt go either way depending on SPARSE,
LINUX_TYPES or whatever suitable pre-existing preprocessor tags we can
locate.
--
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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
