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

Christoffer Dall
2014-02-02T02:25:33Z

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>