Re: [PATCH 10/17] libfdt: get libfdt to build

Andrew Jones
2014-02-03T13:57:55Z

On Sat, Feb 01, 2014 at 06:25:37PM -0800, Christoffer Dall wrote:
> On Tue, Jan 21, 2014 at 05:21:56PM +0100, Andrew Jones wrote:
> > Add string functions needed for libfdt, modify libfdt_env.h
> > for this environment, and add a make target.
>
> The endianness changes you're making to CPU_TO_FDTxx are not trivial to
> review. A motivational commit text here would have been immensely
> helpful.

I'll improve the commit message in the next round.

>
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> > Makefile | 23 +++++++++++++++++++++-
> > lib/libcflat.h | 4 ++++
> > lib/libfdt/libfdt_env.h | 25 ++++++++++++------------
> > lib/string.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 90 insertions(+), 13 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 283655957f39f..94c565242c1b7 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -16,6 +16,13 @@ cflatobjs := \
> > lib/printf.o \
> > lib/string.o
> >
> > +# libfdt paths
> > +LIBFDT_objdir = lib/libfdt
> > +LIBFDT_srcdir = lib/libfdt
> > +LIBFDT_archive = $(LIBFDT_objdir)/libfdt.a
> > +LIBFDT_include = $(addprefix $(LIBFDT_srcdir)/,$(LIBFDT_INCLUDES))
> > +LIBFDT_version = $(addprefix $(LIBFDT_srcdir)/,$(LIBFDT_VERSION))
> > +
> > #include architecure specific make rules
> > include config/config-$(ARCH).mak
> >
> > @@ -41,6 +48,20 @@ LDFLAGS += -pthread -lrt
> > $(libcflat): $(cflatobjs)
> > $(AR) rcs $@ $^
> >
> > +include $(LIBFDT_srcdir)/Makefile.libfdt
> > +
> > +.PHONY: libfdt
> > +libfdt: $(LIBFDT_archive)
> > +
> > +$(LIBFDT_archive): CFLAGS += -ffreestanding -I lib -I lib/libfdt -Wno-sign-compare
> > +$(LIBFDT_archive): $(addprefix $(LIBFDT_objdir)/,$(LIBFDT_OBJS))
> > + $(AR) rcs $@ $^
> > +
> > +libfdt_clean:
> > + $(RM) $(LIBFDT_archive) \
> > + $(addprefix $(LIBFDT_objdir)/,$(LIBFDT_OBJS)) \
> > + $(LIBFDT_objdir)/.*.d
> > +
> > %.o: %.S
> > $(CC) $(CFLAGS) -c -nostdlib -o $@ $<
> >
> > @@ -53,7 +74,7 @@ install:
> > clean: arch_clean
> > $(RM) lib/.*.d $(libcflat) $(cflatobjs)
> >
> > -cscope: common_dirs = lib
> > +cscope: common_dirs = lib lib/libfdt
> > cscope:
> > rm -f ./cscope.*
> > find $(TEST_DIR) lib/$(TEST_DIR) $(common_dirs) -maxdepth 1 \
> > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > index a1be635ab4ee9..2cde64a560956 100644
> > --- a/lib/libcflat.h
> > +++ b/lib/libcflat.h
> > @@ -43,6 +43,7 @@ extern void exit(int code);
> > extern unsigned long strlen(const char *buf);
> > extern char *strcat(char *dest, const char *src);
> > extern int strcmp(const char *a, const char *b);
> > +extern char *strchr(const char *s, int c);
> >
> > extern int printf(const char *fmt, ...);
> > extern int vsnprintf(char *buf, int size, const char *fmt, va_list va);
> > @@ -51,6 +52,9 @@ extern void puts(const char *s);
> >
> > extern void *memset(void *s, int c, size_t n);
> > extern void *memcpy(void *dest, const void *src, size_t n);
> > +extern int memcmp(const void *s1, const void *s2, size_t n);
> > +extern void *memmove(void *dest, const void *src, size_t n);
> > +extern void *memchr(const void *s, int c, size_t n);
> >
> > extern long atol(const char *ptr);
> > #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0]))
> > diff --git a/lib/libfdt/libfdt_env.h b/lib/libfdt/libfdt_env.h
> > index 9dea97dfff818..46fcb6c29045f 100644
> > --- a/lib/libfdt/libfdt_env.h
> > +++ b/lib/libfdt/libfdt_env.h
> > @@ -52,9 +52,16 @@
> > * EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > */
> >
> > -#include <stddef.h>
> > -#include <stdint.h>
> > -#include <string.h>
> > +#include "libcflat.h"
> > +#include "libio.h"
> > +
> > +#ifndef HAVE_UINTS
> > +typedef u8 uint8_t;
> > +typedef u16 uint16_t;
> > +typedef u32 uint32_t;
> > +typedef u64 uint64_t;
> > +typedef unsigned long uintptr_t;
> > +#endif
> >
> > #ifdef __CHECKER__
> > #define __force __attribute__((force))
> > @@ -68,14 +75,9 @@ typedef uint16_t __bitwise fdt16_t;
> > typedef uint32_t __bitwise fdt32_t;
> > typedef uint64_t __bitwise fdt64_t;
> >
> > -#define EXTRACT_BYTE(x, n) ((unsigned long long)((uint8_t *)&x)[n])
> > -#define CPU_TO_FDT16(x) ((EXTRACT_BYTE(x, 0) << 8) | EXTRACT_BYTE(x, 1))
> > -#define CPU_TO_FDT32(x) ((EXTRACT_BYTE(x, 0) << 24) | (EXTRACT_BYTE(x, 1) << 16) | \
> > - (EXTRACT_BYTE(x, 2) << 8) | EXTRACT_BYTE(x, 3))
> > -#define CPU_TO_FDT64(x) ((EXTRACT_BYTE(x, 0) << 56) | (EXTRACT_BYTE(x, 1) << 48) | \
> > - (EXTRACT_BYTE(x, 2) << 40) | (EXTRACT_BYTE(x, 3) << 32) | \
> > - (EXTRACT_BYTE(x, 4) << 24) | (EXTRACT_BYTE(x, 5) << 16) | \
> > - (EXTRACT_BYTE(x, 6) << 8) | EXTRACT_BYTE(x, 7))
> > +#define CPU_TO_FDT16 cpu_to_be16
> > +#define CPU_TO_FDT32 cpu_to_be32
> > +#define CPU_TO_FDT64 cpu_to_be64
> >
> > static inline uint16_t fdt16_to_cpu(fdt16_t x)
> > {
> > @@ -106,6 +108,5 @@ static inline fdt64_t cpu_to_fdt64(uint64_t x)
> > #undef CPU_TO_FDT64
> > #undef CPU_TO_FDT32
> > #undef CPU_TO_FDT16
> > -#undef EXTRACT_BYTE
> >
> > #endif /* _LIBFDT_ENV_H */
> > diff --git a/lib/string.c b/lib/string.c
> > index 3a9caf720bf2b..234f96f4442f7 100644
> > --- a/lib/string.c
> > +++ b/lib/string.c
> > @@ -31,6 +31,17 @@ int strcmp(const char *a, const char *b)
> > return *a - *b;
> > }
> >
> > +char *strchr(const char *s, int c)
> > +{
> > + unsigned char chr = (unsigned char)c;
>
> why unsigned?

no good answer for you. I'll redo this function.

>
> > + int n = strlen(s) + 1;
>
> why scan the string twice? couldn't the whole function be written:
>
> {
> char chr = (char)c;
> while (*s != '\0' && *s != chr)
> s++;
> return (*s != chr) ? NULL : (char *)s;
> }
>
> > +
> > + while (n--)
> > + if (*s++ == chr)
> > + return (char *)(s - 1);
> > + return NULL;
> > +}
> > +
> > void *memset(void *s, int c, size_t n)
> > {
> > size_t i;
> > @@ -54,6 +65,46 @@ void *memcpy(void *dest, const void *src, size_t n)
> > return dest;
> > }
> >
> > +int memcmp(const void *s1, const void *s2, size_t n)
> > +{
> > + const unsigned char *a = s1, *b = s2;
> > + int ret = 0;
> > +
> > + while (n--) {
> > + ret = *a - *b;
> > + if (ret)
> > + break;
> > + ++a, ++b;
> > + }
> > + return ret;
> > +}
> > +
> > +void *memmove(void *dest, const void *src, size_t n)
> > +{
> > + const unsigned char *s = src;
> > + unsigned char *d = dest;
> > +
> > + if (d <= s) {
> > + while (n--)
> > + *d++ = *s++;
> > + } else {
> > + d += n, s += n;
> > + while (n--)
> > + *--d = *--s;
> > + }
> > + return dest;
> > +}
> > +
> > +void *memchr(const void *s, int c, size_t n)
> > +{
> > + const unsigned char *str = s, chr = (unsigned char)c;
> > +
> > + while (n--)
> > + if (*str++ == chr)
> > + return (void *)(str - 1);
> > + return NULL;
> > +}
> > +
> > long atol(const char *ptr)
> > {
> > long acc = 0;
> > --
> > 1.8.1.4
> >
>
> Otherwise:
>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>