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

[Andrew Jones <drjones@redhat.com>]

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>