On Thu, 18 Oct 2012 23:11:12 +1100 David Gibson <da...@gibson.dropbear.id.au> wrote:
> 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. cool. > > 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. understood. > > 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. right, we don't want all uint32_t's to be one endian, we just want fdt32 types to have big endian annotation when being checked by a checker such as sparse. > > > 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. this is named __CHECKER__ in linux and u-boot. Ifdef __CHECKER__, then the code is being parsed by a checker. > 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. understood, that's how I envisaged things going too. Let me see what I can do. Kim _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
