Re: [PATCH v4 15/19] arm: initial drop

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

On Thu, Apr 10, 2014 at 06:56:56PM +0200, Andrew Jones wrote:
> This is the initial drop of the arm test framework and a first test
> that just checks that setup completed (a selftest). kvm isn't needed
> to run this test unless testing with smp > 1.
> 
> Try it out with
>   yum install gcc-arm-linux-gnu
>   export QEMU=[qemu with mach-virt and virtio-testdev]
>   ./configure --cross-prefix=arm-linux-gnu- --arch=arm
>   make
>   ./run_tests.sh
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> v4:
>   - moved fdt to just after stacktop (it was in the middle of free memory)
>   - switched from using heap to memregions
>   - get nr_cpus and added smp=<num> test
>   - added barrier.h
>   - use new report()/report_summary()
>   - config/config-arm.mak cleanup
> ---
>  arm/cstart.S           |  35 ++++++++++++
>  arm/flat.lds           |  18 +++++++
>  arm/run                |  31 +++++++++++
>  arm/selftest.c         |  87 ++++++++++++++++++++++++++++++
>  arm/unittests.cfg      |  18 +++++++
>  config/config-arm.mak  |  72 +++++++++++++++++++++++++
>  configure              |  12 ++++-
>  lib/argv.c             |   6 +++
>  lib/arm/asm/barrier.h  |  18 +++++++
>  lib/arm/asm/io.h       |  24 +++++++++
>  lib/arm/asm/setup.h    |  39 ++++++++++++++
>  lib/arm/asm/spinlock.h |  16 ++++++
>  lib/arm/eabi_compat.c  |  20 +++++++
>  lib/arm/io.c           |  66 +++++++++++++++++++++++
>  lib/arm/setup.c        | 142 +++++++++++++++++++++++++++++++++++++++++++++++++
>  15 files changed, 602 insertions(+), 2 deletions(-)
>  create mode 100644 arm/cstart.S
>  create mode 100644 arm/flat.lds
>  create mode 100755 arm/run
>  create mode 100644 arm/selftest.c
>  create mode 100644 arm/unittests.cfg
>  create mode 100644 config/config-arm.mak
>  create mode 100644 lib/arm/asm/barrier.h
>  create mode 100644 lib/arm/asm/io.h
>  create mode 100644 lib/arm/asm/setup.h
>  create mode 100644 lib/arm/asm/spinlock.h
>  create mode 100644 lib/arm/eabi_compat.c
>  create mode 100644 lib/arm/io.c
>  create mode 100644 lib/arm/setup.c
> 
> diff --git a/arm/cstart.S b/arm/cstart.S
> new file mode 100644
> index 0000000000000..e28251db2950d
> --- /dev/null
> +++ b/arm/cstart.S
> @@ -0,0 +1,35 @@
> +/*
> + * Boot entry point and assembler functions for armv7 tests.
> + *
> + * Copyright (C) 2014, Red Hat Inc, Andrew Jones <drjones@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +
> +.arm
> +
> +.section .init
> +
> +.globl start
> +start:
> +	/*
> +	 * bootloader params are in r0-r2
> +	 * See the kernel doc Documentation/arm/Booting
> +	 */
> +	ldr	sp, =stacktop
> +	bl	setup
> +
> +	/* run the test */
> +	ldr	r0, =__argc
> +	ldr	r0, [r0]
> +	ldr	r1, =__argv
> +	bl	main
> +	bl	exit
> +	b	halt
> +
> +.text
> +
> +.globl halt
> +halt:
> +1:	wfi
> +	b	1b
> diff --git a/arm/flat.lds b/arm/flat.lds
> new file mode 100644
> index 0000000000000..3e5d72e24989b
> --- /dev/null
> +++ b/arm/flat.lds
> @@ -0,0 +1,18 @@
> +
> +SECTIONS
> +{
> +    .text : { *(.init) *(.text) *(.text.*) }
> +    . = ALIGN(4K);
> +    .data : { *(.data) }
> +    . = ALIGN(16);
> +    .rodata : { *(.rodata) }
> +    . = ALIGN(16);
> +    .bss : { *(.bss) }
> +    . = ALIGN(4K);
> +    edata = .;
> +    . += 8K;
> +    . = ALIGN(4K);
> +    stacktop = .;
> +}
> +
> +ENTRY(start)
> diff --git a/arm/run b/arm/run
> new file mode 100755
> index 0000000000000..985b26e1334da
> --- /dev/null
> +++ b/arm/run
> @@ -0,0 +1,31 @@
> +#!/bin/bash
> +
> +if [ ! -f config.mak ]; then
> +	echo run ./configure first. See ./configure -h
> +	exit 2
> +fi
> +source config.mak
> +
> +qemu="${QEMU:-qemu-system-arm}"
> +testdev=virtio-testdev
> +
> +if ! $qemu -device '?' 2>&1 | grep $testdev > /dev/null; then
> +	qpath=$(which $qemu 2>/dev/null)
> +	if [ "$qpath" ]; then
> +		echo $qpath doesn\'t support virtio-testdev. Exiting.
> +	else
> +		echo $qemu not found.
> +	fi
> +	exit 2
> +fi
> +
> +command="$qemu -device $testdev -display none -serial stdio "
> +command+="-M virt -cpu $PROCESSOR "
> +#command+="-enable-kvm "
> +command+="-kernel"
> +
> +echo $command "$@"
> +$command "$@"
> +ret=$?
> +echo Return value from qemu: $ret
> +exit $ret
> diff --git a/arm/selftest.c b/arm/selftest.c
> new file mode 100644
> index 0000000000000..45729e9ac85d9
> --- /dev/null
> +++ b/arm/selftest.c
> @@ -0,0 +1,87 @@
> +/*
> + * Test the framework itself. These tests confirm that setup works.
> + *
> + * Copyright (C) 2014, Red Hat Inc, Andrew Jones <drjones@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include "libcflat.h"
> +#include "asm/setup.h"
> +
> +#define TESTGRP "selftest"
> +
> +static char testnam[64];

is testnam testname?

> +
> +static void testnam_set(const char *subtest)
> +{
> +	strcpy(testnam, TESTGRP);
> +	strcat(testnam, "::");
> +	strcat(testnam, subtest);
> +}
> +
> +static void assert_args(int num_args, int needed_args)
> +{
> +	if (num_args < needed_args) {
> +		printf("%s: not enough arguments\n", testnam);
> +		abort();
> +	}
> +}
> +
> +static char *split_var(char *s, long *val)
> +{
> +	char *p;
> +
> +	p = strchr(s, '=');
> +	if (!p)
> +		return NULL;
> +
> +	*val = atol(p+1);
> +	*p = '\0';
> +
> +	return s;
> +}
> +
> +static void check_setup(int argc, char **argv)
> +{
> +	int nr_tests = 0, i;
> +	char *var;
> +	long val;
> +
> +	for (i = 0; i < argc; ++i) {
> +
> +		var = split_var(argv[i], &val);
> +		if (!var)
> +			continue;
> +
> +		if (strcmp(argv[i], "mem") == 0) {
> +
> +			phys_addr_t memsize =
> +					memregions[nr_memregions-1].addr
> +					+ memregions[nr_memregions-1].size
> +					- PHYS_OFFSET;
> +			phys_addr_t expected = ((phys_addr_t)val)*1024*1024;
> +
> +			report("%s[%s]", memsize == expected, testnam, "mem");
> +			++nr_tests;
> +
> +		} else if (strcmp(argv[i], "smp") == 0) {
> +
> +			report("%s[%s]", nr_cpus == (int)val, testnam, "smp");
> +			++nr_tests;
> +		}
> +	}
> +
> +	assert_args(nr_tests, 2);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	assert_args(argc, 1);
> +
> +	testnam_set(argv[0]);
> +
> +	if (strcmp(argv[0], "setup") == 0)
> +		check_setup(argc-1, &argv[1]);
> +
> +	return report_summary();
> +}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> new file mode 100644
> index 0000000000000..da9dfd7b1f118
> --- /dev/null
> +++ b/arm/unittests.cfg
> @@ -0,0 +1,18 @@
> +# Define your new unittest following the convention:
> +# [unittest_name]
> +# file = foo.flat # Name of the flat file to be used
> +# smp  = 2        # Number of processors the VM will use during this test
> +# extra_params = -append <params...> # Additional parameters used
> +# arch = arm/arm64                   # Only if test case is specific to one
> +# groups = group1 group2 # Used to identify test cases with run_tests -g ...
> +
> +#
> +# Test that the configured number of processors (smp = <num>), and
> +# that the configured amount of memory (-m <MB>) are correctly setup
> +# by the framework.
> +#
> +[selftest::setup]
> +file = selftest.flat
> +smp  = 1
> +extra_params = -m 256 -append 'setup smp=1 mem=256'
> +groups = selftest
> diff --git a/config/config-arm.mak b/config/config-arm.mak
> new file mode 100644
> index 0000000000000..915b1cc318d79
> --- /dev/null
> +++ b/config/config-arm.mak
> @@ -0,0 +1,72 @@
> +#
> +# arm makefile
> +#
> +# Authors: Andrew Jones <drjones@redhat.com>
> +#
> +
> +tests-common = \
> +	$(TEST_DIR)/selftest.flat
> +
> +tests =
> +
> +all: test_cases
> +
> +##################################################################
> +bits = 32
> +ldarch = elf32-littlearm
> +
> +ifeq ($(LOADADDR),)
> +	LOADADDR = 0x40000000
> +endif
> +phys_base = $(LOADADDR)
> +kernel_offset = 0x10000
> +
> +CFLAGS += -D__arm__
> +CFLAGS += -marm
> +CFLAGS += -mcpu=$(PROCESSOR)
> +CFLAGS += -std=gnu99
> +CFLAGS += -ffreestanding
> +CFLAGS += -Wextra
> +CFLAGS += -O2
> +CFLAGS += -I lib -I lib/libfdt
> +
> +cflatobjs += \
> +	lib/devicetree.o \
> +	lib/virtio.o \
> +	lib/virtio-testdev.o \
> +	lib/arm/io.o \
> +	lib/arm/setup.o
> +
> +libeabi = lib/arm/libeabi.a
> +eabiobjs = lib/arm/eabi_compat.o
> +
> +libgcc := $(shell $(CC) -m$(ARCH) --print-libgcc-file-name)
> +start_addr := $(shell printf "%x\n" $$(( $(phys_base) + $(kernel_offset) )))
> +
> +FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc) $(libeabi)
> +%.elf: LDFLAGS = $(CFLAGS) -nostdlib
> +%.elf: %.o $(FLATLIBS) arm/flat.lds
> +	$(CC) $(LDFLAGS) -o $@ \
> +		-Wl,-T,arm/flat.lds,--build-id=none,-Ttext=$(start_addr) \
> +		$(filter %.o, $^) $(FLATLIBS)
> +
> +%.flat: %.elf
> +	$(OBJCOPY) -O binary $^ $@
> +
> +$(libeabi): $(eabiobjs)
> +	$(AR) rcs $@ $^
> +
> +arch_clean: libfdt_clean
> +	$(RM) $(TEST_DIR)/*.{o,flat,elf} $(libeabi) $(eabiobjs) \
> +	      $(TEST_DIR)/.*.d lib/arm/.*.d
> +
> +##################################################################
> +
> +tests_and_config = $(TEST_DIR)/*.flat $(TEST_DIR)/unittests.cfg
> +
> +cstart.o = $(TEST_DIR)/cstart.o
> +
> +test_cases: $(tests-common) $(tests)
> +
> +$(TEST_DIR)/selftest.elf: $(cstart.o) $(TEST_DIR)/selftest.o
> +
> diff --git a/configure b/configure
> index 8a81bf92e27b7..f92f585724bb1 100755
> --- a/configure
> +++ b/configure
> @@ -6,8 +6,7 @@ cc=gcc
>  ld=ld
>  objcopy=objcopy
>  ar=ar
> -arch=`uname -m | sed -e s/i.86/i386/`
> -processor="$arch"
> +arch=`uname -m | sed -e s/i.86/i386/ | sed -e 's/arm.*/arm/'`
>  cross_prefix=
>  
>  usage() {
> @@ -17,6 +16,7 @@ usage() {
>  	Options include:
>  	    --test-dir=DIR         the main directory for tests ($arch)
>  	    --arch=ARCH            architecture to compile for ($arch)
> +	    --processor=PROCESSOR  processor to compile for ($arch)
>  	    --cross-prefix=PREFIX  cross compiler prefix
>  	    --cc=CC		   c compiler to use ($cc)
>  	    --ld=LD		   ld linker to use ($ld)
> @@ -66,6 +66,12 @@ while [[ "$1" = -* ]]; do
>  	    ;;
>      esac
>  done
> +[ -z "$processor" ] && processor="$arch"
> +
> +if [ "$processor" = "arm" ]; then
> +    processor="cortex-a15"
> +fi
> +
>  if [ -z "$testdir" -a \( "$arch" = "i386" -o "$arch" = "x86_64" \) ]; then
>      testdir=x86
>  elif [ -z "$testdir" ]; then
> @@ -80,6 +86,7 @@ if [ -f $testdir/run ]; then
>  fi
>  
>  # check for dependent 32 bit libraries
> +if [ "$arch" != "arm" ]; then
>  cat << EOF > lib_test.c
>  #include <stdc++.h>
>  #include <boost_thread-mt.h>
> @@ -94,6 +101,7 @@ if [ $exit -eq 0 ]; then
>      api=true
>  fi
>  rm -f lib_test.c
> +fi
>  
>  # link lib/asm for the architecture
>  rm -f lib/asm
> diff --git a/lib/argv.c b/lib/argv.c
> index 4ee54a6eeac3e..078a05faffebf 100644
> --- a/lib/argv.c
> +++ b/lib/argv.c
> @@ -31,3 +31,9 @@ void __setup_args(void)
>      }
>      __argc = argv - __argv;
>  }
> +
> +void setup_args(char *args)
> +{
> +    __args = args;
> +    __setup_args();
> +}
> diff --git a/lib/arm/asm/barrier.h b/lib/arm/asm/barrier.h
> new file mode 100644
> index 0000000000000..acaeab5123431
> --- /dev/null
> +++ b/lib/arm/asm/barrier.h
> @@ -0,0 +1,18 @@
> +#ifndef _ASMARM_BARRIER_H_
> +#define _ASMARM_BARRIER_H_
> +/*
> + * Adapted form arch/arm/include/asm/barrier.h
> + */
> +
> +#define isb(option) __asm__ __volatile__ ("isb " #option : : : "memory")
> +#define dsb(option) __asm__ __volatile__ ("dsb " #option : : : "memory")
> +#define dmb(option) __asm__ __volatile__ ("dmb " #option : : : "memory")
> +
> +#define mb()		dsb()
> +#define rmb()		dsb()
> +#define wmb()		dsb(st)
> +#define smp_mb()	dmb(ish)
> +#define smp_rmb()	smp_mb()
> +#define smp_wmb()	dmb(ishst)
> +
> +#endif /* _ASMARM_BARRIER_H_ */
> diff --git a/lib/arm/asm/io.h b/lib/arm/asm/io.h
> new file mode 100644
> index 0000000000000..51ec6e9aa2e99
> --- /dev/null
> +++ b/lib/arm/asm/io.h
> @@ -0,0 +1,24 @@
> +#ifndef _ASMARM_IO_H_
> +#define _ASMARM_IO_H_
> +#include "libcflat.h"
> +#include "asm/barrier.h"
> +
> +#define __bswap16 bswap16
> +static inline u16 bswap16(u16 val)
> +{
> +	u16 ret;
> +	asm volatile("rev16 %0, %1" : "=r" (ret) :  "r" (val));
> +	return ret;
> +}
> +
> +#define __bswap32 bswap32
> +static inline u32 bswap32(u32 val)
> +{
> +	u32 ret;
> +	asm volatile("rev %0, %1" : "=r" (ret) :  "r" (val));
> +	return ret;
> +}
> +
> +#include "asm-generic/io.h"
> +
> +#endif /* _ASMARM_IO_H_ */
> diff --git a/lib/arm/asm/setup.h b/lib/arm/asm/setup.h
> new file mode 100644
> index 0000000000000..d451b23434041
> --- /dev/null
> +++ b/lib/arm/asm/setup.h
> @@ -0,0 +1,39 @@
> +#ifndef _ASMARM_SETUP_H_
> +#define _ASMARM_SETUP_H_
> +/*
> + * Copyright (C) 2014, Red Hat Inc, Andrew Jones <drjones@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include "libcflat.h"
> +
> +#define NR_CPUS			8
> +extern u32 cpus[NR_CPUS];
> +extern int nr_cpus;
> +
> +typedef u64 phys_addr_t;
> +
> +#define NR_MEMREGIONS		16
> +struct memregion {
> +	phys_addr_t addr;
> +	phys_addr_t size;
> +	bool free;
> +};
> +
> +extern struct memregion memregions[NR_MEMREGIONS];
> +extern int nr_memregions;
> +
> +extern struct memregion *memregion_new(phys_addr_t size);
> +extern void memregions_show(void);
> +
> +#define PHYS_OFFSET		({ memregions[0].addr; })
> +#define PHYS_SHIFT		40
> +#define PHYS_SIZE		(1ULL << PHYS_SHIFT)
> +#define PHYS_MASK		(PHYS_SIZE - 1ULL)

it's pretty funky that you define PHYS_MASK the opposite of PAGE_MASK?

> +
> +#define PAGE_SHIFT		12
> +#define PAGE_SIZE		(1UL << PAGE_SHIFT)
> +#define PAGE_MASK		(~(PAGE_SIZE - 1UL))
> +#define PAGE_ALIGN(addr)	(((addr) + (PAGE_SIZE-1UL)) & PAGE_MASK)
> +
> +#endif /* _ASMARM_SETUP_H_ */
> diff --git a/lib/arm/asm/spinlock.h b/lib/arm/asm/spinlock.h
> new file mode 100644
> index 0000000000000..04f5a1a5538e2
> --- /dev/null
> +++ b/lib/arm/asm/spinlock.h
> @@ -0,0 +1,16 @@
> +#ifndef _ASMARM_SPINLOCK_H_
> +#define _ASMARM_SPINLOCK_H_
> +
> +struct spinlock {
> +	int v;
> +};
> +
> +//TODO
> +static inline void spin_lock(struct spinlock *lock __unused)
> +{
> +}
> +static inline void spin_unlock(struct spinlock *lock __unused)
> +{
> +}
> +
> +#endif /* _ASMARM_SPINLOCK_H_ */
> diff --git a/lib/arm/eabi_compat.c b/lib/arm/eabi_compat.c
> new file mode 100644
> index 0000000000000..59d624dcd9277
> --- /dev/null
> +++ b/lib/arm/eabi_compat.c
> @@ -0,0 +1,20 @@
> +/*
> + * Adapted from u-boot's arch/arm/lib/eabi_compat.c
> + */
> +#include "libcflat.h"
> +
> +int raise(int signum __unused)
> +{
> +	printf("Divide by zero!\n");
> +	abort();
> +	return 0;
> +}
> +
> +/* Dummy functions to avoid linker complaints */
> +void __aeabi_unwind_cpp_pr0(void)
> +{
> +}
> +
> +void __aeabi_unwind_cpp_pr1(void)
> +{
> +}
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> new file mode 100644
> index 0000000000000..c865f61c94934
> --- /dev/null
> +++ b/lib/arm/io.c
> @@ -0,0 +1,66 @@
> +/*
> + * Each architecture must implement puts() and exit() with the I/O
> + * devices exposed from QEMU, e.g. pl011 and virtio-testdev. That's
> + * what's done here, along with initialization functions for those
> + * devices.
> + *
> + * Copyright (C) 2014, Red Hat Inc, Andrew Jones <drjones@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include "libcflat.h"
> +#include "devicetree.h"
> +#include "virtio-testdev.h"
> +#include "asm/spinlock.h"
> +#include "asm/io.h"
> +
> +extern void halt(int code);
> +
> +/*
> + * Use this guess for the pl011 base in order to make an attempt at
> + * having earlier printf support. We'll overwrite it with the real
> + * base address that we read from the device tree later.
> + */
> +#define QEMU_MACH_VIRT_PL011_BASE 0x09000000UL
> +
> +static struct spinlock uart_lock;
> +static volatile u8 *uart0_base = (u8 *)QEMU_MACH_VIRT_PL011_BASE;
> +
> +static void uart0_init(void)
> +{
> +	const char *compatible = "arm,pl011";
> +	dt_pbus_addr_t base;
> +	int ret;
> +
> +	ret = dt_pbus_get_baseaddr_compatible(compatible, &base);
> +	assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
> +
> +	if (ret == 0) {
> +		assert(sizeof(long) == 8 || !(base >> 32));

didn't we already check this somewhere?  Or was that virtio specific?

This confirms my suspicion that you should add a dummy identity ioremap
function that can do this check once.

> +		uart0_base = (u8 *)(unsigned long)base;

hmmm, u8*, I guess that sort of makes sense...

> +	} else {
> +		printf("%s: %s not found in the device tree, aborting...\n",
> +			__func__, compatible);
> +		abort();

I think this function would read nicer if you did this right after your
assert:

if (ret) {
	printf(....);
	abort();
	}

uart0_base = ....;

> +	}
> +}
> +
> +void io_init(void)
> +{
> +	uart0_init();
> +	virtio_testdev_init();
> +}
> +
> +void puts(const char *s)
> +{
> +	spin_lock(&uart_lock);
> +	while (*s)
> +		writel(*s++, uart0_base);
> +	spin_unlock(&uart_lock);
> +}
> +
> +void exit(int code)
> +{
> +	virtio_testdev_exit(code);
> +	halt(code);
> +}
> diff --git a/lib/arm/setup.c b/lib/arm/setup.c
> new file mode 100644
> index 0000000000000..3794291e94e01
> --- /dev/null
> +++ b/lib/arm/setup.c
> @@ -0,0 +1,142 @@
> +/*
> + * Initialize machine setup information and I/O.
> + *
> + * After running setup() a flat file knows how many cpus it has

'a flat file knows' ?

> + * (nr_cpus), how much free memory it has, and at what physical
> + * address that free memory starts (memregions[1].{addr,size}),
> + * printf() and exit() will both work, and (argc, argv) are ready
> + * to be passed to main().
> + *
> + * Copyright (C) 2014, Red Hat Inc, Andrew Jones <drjones@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include "libcflat.h"
> +#include "libfdt/libfdt.h"
> +#include "devicetree.h"
> +#include "asm/spinlock.h"
> +#include "asm/setup.h"
> +
> +extern unsigned long stacktop;
> +extern void io_init(void);
> +extern void setup_args(const char *args);
> +
> +u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0UL) };
> +int nr_cpus;
> +
> +static struct spinlock memregion_lock;
> +struct memregion memregions[NR_MEMREGIONS];
> +int nr_memregions;
> +
> +static void cpu_set(int fdtnode __unused, u32 regval, void *info)
> +{
> +	unsigned *i = (unsigned *)info;

shouldn't you assert (*i) < NR_CPUS?

> +	cpus[*i] = regval;
> +	*i += 1;

holy cow that's a convoluted way of counting.  Can't you just bump the
nr_cpus here?

> +}
> +
> +static void cpu_init(void)
> +{
> +	unsigned i = 0;
> +	assert(dt_for_each_cpu_node(cpu_set, &i) == 0);
> +	nr_cpus = i;
> +}
> +
> +static void memregions_init(phys_addr_t freemem_start)
> +{
> +	/* we only expect one membank to be defined in the DT */
> +	struct dt_pbus_reg regs[1];
> +	phys_addr_t addr, size, mem_end;
> +
> +	nr_memregions = dt_get_memory_params(regs, 1);
> +
> +	assert(nr_memregions > 0);
> +
> +	addr = regs[0].addr;
> +	size = regs[0].size;
> +	mem_end = addr + size;
> +
> +	assert(!(addr & ~PHYS_MASK) && !((mem_end-1) & ~PHYS_MASK));
> +
> +#ifdef __arm__
> +	/* TODO: support highmem? */

I don't think highmem is the right term here, the concept is LPAE

> +	assert(!((u64)addr >> 32) && !((u64)(mem_end-1) >> 32));
> +#endif
> +
> +	freemem_start = PAGE_ALIGN(freemem_start);
> +
> +	memregions[0].addr = PAGE_ALIGN(addr);
> +	memregions[0].size = freemem_start - PHYS_OFFSET;

don't you want to assert that the membank found in the dt actually
covers the addresses we're loaded at?

> +
> +	memregions[1].addr = freemem_start;
> +	memregions[1].size = mem_end - freemem_start;
> +	memregions[1].free = true;

you could have defines for 0 and 1 here if you have already decided
these places have special meanings.

> +	nr_memregions = 2;
> +}
> +

this is an allocator of sorts, essentially, right?  It would be most
helpful to document this seemingly very important function.

> +struct memregion *memregion_new(phys_addr_t size)
> +{
> +	phys_addr_t freemem_start, freemem_size, mem_end;
> +	struct memregion *m;
> +
> +	spin_lock(&memregion_lock);
> +
> +	assert(memregions[nr_memregions-1].free);
> +	assert(memregions[nr_memregions-1].size >= size);
> +
> +	mem_end = memregions[nr_memregions-1].addr
> +		+ memregions[nr_memregions-1].size;
> +
> +	freemem_start = PAGE_ALIGN(memregions[nr_memregions-1].addr + size);
> +	freemem_size = mem_end - freemem_start;
> +
> +	memregions[nr_memregions-1].size = freemem_start
> +					 - memregions[nr_memregions-1].addr;
> +	memregions[nr_memregions-1].free = false;
> +
> +	m = &memregions[nr_memregions-1];
> +
> +	if (nr_memregions < NR_MEMREGIONS) {
> +		memregions[nr_memregions].addr = freemem_start;
> +		memregions[nr_memregions].size = freemem_size;
> +		memregions[nr_memregions].free = true;
> +		++nr_memregions;
> +	}
> +
> +	spin_unlock(&memregion_lock);
> +
> +	return m;
> +}
> +
> +void memregions_show(void)
> +{
> +	int i;
> +	for (i = 0; i < nr_memregions; ++i)
> +		printf("%016llx-%016llx [%s]\n",
> +			memregions[i].addr,
> +			memregions[i].addr + memregions[i].size - 1,
> +			memregions[i].free ? "FREE" : "USED");
> +}
> +
> +void setup(unsigned long arg __unused, unsigned long id __unused,
> +	   const void *fdt)
> +{
> +	const char *bootargs;
> +	u32 fdt_size;
> +
> +	/*
> +	 * Move the fdt to just above the stack. The free memory
> +	 * then starts just after the fdt.
> +	 */
> +	fdt_size = fdt_totalsize(fdt);
> +	assert(fdt_move(fdt, &stacktop, fdt_size) == 0);
> +	assert(dt_init(&stacktop) == 0);
> +
> +	memregions_init((unsigned long)&stacktop + fdt_size);
> +
> +	io_init();
> +	cpu_init();
> +
> +	assert(dt_get_bootargs(&bootargs) == 0);
> +	setup_args(bootargs);
> +}
> -- 
> 1.8.1.4
> 

Thanks,
-Christoffer