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

[Christoffer Dall <christoffer.dall@linaro.org>]

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.

> 
> 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?

> +    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>