[U-Boot] [PATCH v5 1/4] core support of arm64
Scott Wood
scottwood at freescale.com
Fri Sep 6 22:31:39 CEST 2013
On Sat, 2013-08-24 at 09:06 +0800, fenghua at phytium.com.cn wrote:
> From: David Feng <fenghua at phytium.com.cn>
>
> Signed-off-by: David Feng <fenghua at phytium.com.cn>
> ---
> Changeds for v4:
> - Replace __ARMEB__ with __AARCH64EB__ in byteorder.h and unaligned.h,
> gcc for aarch64 use __AARCH64EB__ and __AARCH64EL__ to identify endian.
> - Some modification to README.armv8
>
> arch/arm/config.mk | 4 +
> arch/arm/cpu/armv8/Makefile | 56 ++++++
> arch/arm/cpu/armv8/cache.S | 145 +++++++++++++++
> arch/arm/cpu/armv8/cache_v8.c | 291 +++++++++++++++++++++++++++++++
> arch/arm/cpu/armv8/config.mk | 31 ++++
> arch/arm/cpu/armv8/cpu.c | 68 ++++++++
> arch/arm/cpu/armv8/crt0.S | 130 ++++++++++++++
> arch/arm/cpu/armv8/exceptions.S | 182 +++++++++++++++++++
> arch/arm/cpu/armv8/interrupts.c | 116 ++++++++++++
> arch/arm/cpu/armv8/relocate.S | 71 ++++++++
> arch/arm/cpu/armv8/start.S | 200 +++++++++++++++++++++
> arch/arm/cpu/armv8/timer.c | 95 ++++++++++
> arch/arm/cpu/armv8/tlb.S | 38 ++++
> arch/arm/cpu/armv8/u-boot.lds | 83 +++++++++
Subject says "arm64", files say "armv8".
Some of these, such as relocate.S, probably are not going to be
armv8-specific but just arm64-specific.
> arch/arm/include/asm/arch-armv8/armv8.h | 44 +++++
> arch/arm/include/asm/arch-armv8/gpio.h | 26 +++
> arch/arm/include/asm/arch-armv8/mmu.h | 117 +++++++++++++
> arch/arm/include/asm/byteorder.h | 12 ++
> arch/arm/include/asm/config.h | 10 ++
> arch/arm/include/asm/global_data.h | 6 +-
> arch/arm/include/asm/io.h | 12 +-
> arch/arm/include/asm/macro.h | 26 +++
> arch/arm/include/asm/posix_types.h | 31 ++++
> arch/arm/include/asm/proc-armv/ptrace.h | 38 ++++
> arch/arm/include/asm/proc-armv/system.h | 58 +++++-
> arch/arm/include/asm/types.h | 14 ++
> arch/arm/include/asm/u-boot.h | 4 +
> arch/arm/include/asm/unaligned.h | 14 ++
> arch/arm/lib/Makefile | 8 +
> arch/arm/lib/board.c | 18 ++
> arch/arm/lib/bootm.c | 16 ++
> common/image.c | 1 +
> doc/README.armv8 | 14 ++
> examples/standalone/stubs.c | 15 ++
> include/image.h | 1 +
> 35 files changed, 1987 insertions(+), 8 deletions(-)
> create mode 100644 arch/arm/cpu/armv8/Makefile
> create mode 100644 arch/arm/cpu/armv8/cache.S
> create mode 100644 arch/arm/cpu/armv8/cache_v8.c
> create mode 100644 arch/arm/cpu/armv8/config.mk
> create mode 100644 arch/arm/cpu/armv8/cpu.c
> create mode 100644 arch/arm/cpu/armv8/crt0.S
> create mode 100644 arch/arm/cpu/armv8/exceptions.S
> create mode 100644 arch/arm/cpu/armv8/interrupts.c
> create mode 100644 arch/arm/cpu/armv8/relocate.S
> create mode 100644 arch/arm/cpu/armv8/start.S
> create mode 100644 arch/arm/cpu/armv8/timer.c
> create mode 100644 arch/arm/cpu/armv8/tlb.S
> create mode 100644 arch/arm/cpu/armv8/u-boot.lds
> create mode 100644 arch/arm/include/asm/arch-armv8/armv8.h
> create mode 100644 arch/arm/include/asm/arch-armv8/gpio.h
> create mode 100644 arch/arm/include/asm/arch-armv8/mmu.h
> create mode 100644 doc/README.armv8
>
> diff --git a/arch/arm/config.mk b/arch/arm/config.mk
> index ce3903b..f1c6a7b 100644
> --- a/arch/arm/config.mk
> +++ b/arch/arm/config.mk
> @@ -74,7 +74,9 @@ endif
> endif
>
> # needed for relocation
> +ifndef CONFIG_ARMV8
> LDFLAGS_u-boot += -pie
> +endif
CONFIG_ARM64 (until we fix this, of course)
> #
> # FIXME: binutils versions < 2.22 have a bug in the assembler where
> @@ -95,6 +97,8 @@ endif
> endif
>
> # check that only R_ARM_RELATIVE relocations are generated
> +ifndef CONFIG_ARMV8
> ifneq ($(CONFIG_SPL_BUILD),y)
> ALL-y += checkarmreloc
> endif
> +endif
ARM64, though I've got a patch coming that will make the check work with
arm64.
Likewise elsewhere -- most if not all of the CONFIG_ARMV8 uses should be
CONFIG_ARM64.
> +/*
> + * void __asm_flush_dcache_level(level)
> + *
> + * clean and invalidate one level cache.
> + *
> + * x0: cache level
> + * x1~x9: clobbered
> + */
> +ENTRY(__asm_flush_dcache_level)
Why the leading underscores?
> +/* to activate the MMU we need to set up virtual memory */
> +static inline void mmu_setup(void)
> +{
> + int i, j;
> + bd_t *bd = gd->bd;
> + static int table_initialized = 0;
> +
> + if (!table_initialized) {
Instead of this, have separate mmu_setup() and mmu_enable().
> + /* Setup an identity-mapping for all spaces */
> + for (i = 0; i < (PAGE_SIZE >> 3); i++)
> + set_pgtable_section(i, MT_DEVICE_nGnRnE);
> +
> + /* Setup an identity-mapping for all RAM space */
> + for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
> + debug("%s: bank: %d\n", __func__, i);
> + for (j = bd->bi_dram[i].start >> SECTION_SHIFT;
> + j < (bd->bi_dram[i].start + bd->bi_dram[i].size) >> SECTION_SHIFT;
> + j++) {
> + set_pgtable_section(i, MT_NORMAL);
> + }
> + }
This is odd alignment... Why are the second and third instances of "j"
lined up with "bd->..." rather than with the first "j"?
> +#endif /* CONFIG_SYS_DCACHE_OFF */
> +
> +#ifndef CONFIG_SYS_ICACHE_OFF
This isn't new to this patch, but I'd like to point out that the whole
"CONFIG" versus "CONFIG_SYS" thing is widely misused and should probably
just go away when we kconfigize.
> +/*
> + * Stub implementations for outer cache operations
> + */
> +void __v8_outer_cache_enable(void) {}
> +void v8_outer_cache_enable(void)
> + __attribute__((weak, alias("__v8_outer_cache_enable")));
> +
> +void __v8_outer_cache_disable(void) {}
> +void v8_outer_cache_disable(void)
> + __attribute__((weak, alias("__v8_outer_cache_disable")));
> +
> +void __v8_outer_cache_flush_all(void) {}
> +void v8_outer_cache_flush_all(void)
> + __attribute__((weak, alias("__v8_outer_cache_flush_all")));
> +
> +void __v8_outer_cache_inval_all(void) {}
> +void v8_outer_cache_inval_all(void)
> + __attribute__((weak, alias("__v8_outer_cache_inval_all")));
> +
> +void __v8_outer_cache_flush_range(u64 start, u64 end) {}
> +void v8_outer_cache_flush_range(u64 start, u64 end)
> + __attribute__((weak, alias("__v8_outer_cache_flush_range")));
> +
> +void __v8_outer_cache_inval_range(u64 start, u64 end) {}
> +void v8_outer_cache_inval_range(u64 start, u64 end)
> + __attribute__((weak, alias("__v8_outer_cache_inval_range")));
What level do you anticipate these being overriden at? Individual
chips? Why?
> diff --git a/arch/arm/cpu/armv8/config.mk b/arch/arm/cpu/armv8/config.mk
> new file mode 100644
> index 0000000..aae2170
> --- /dev/null
> +++ b/arch/arm/cpu/armv8/config.mk
> @@ -0,0 +1,31 @@
> +#
> +# Copyright (c) 2013 FengHua <fenghua at phytium.com.cn>
> +#
> +# See file CREDITS for list of people who contributed to this
> +# project.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation; either version 2 of
> +# the License, or (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> +# MA 02111-1307 USA
> +#
> +PLATFORM_RELFLAGS += -fno-common -ffixed-x18
Is -ffixed-x18 necessary, or will GCC do the right thing when it sees
that x18 has been declared as a register variable?
> diff --git a/arch/arm/cpu/armv8/exceptions.S b/arch/arm/cpu/armv8/exceptions.S
> new file mode 100644
> index 0000000..c64a326
> --- /dev/null
> +++ b/arch/arm/cpu/armv8/exceptions.S
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright (c) 2013 David Feng <fenghua at phytium.com.cn>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
I'm skeptical that you actually rewrote this from scratch (e.g. same
registers are used in some places where there's no reason to pick any
particular register, overall structure very similar, etc). It looks
like you just changed the comments and names up a bit.
Any code that you did not write *from scratch* must be identified, with
copyrights retained, and can only be used if it can be licensed GPLv2+
(unless Tom/Wolfgang say they'll accept v2-only code from Linux in this
case).
> +void show_regs (struct pt_regs *regs)
> +{
> + int i;
> +
> + printf("PC is at %lx\n", regs->pc);
> + printf("LR is at %lx\n", regs->regs[30]);
> + printf("PSTATE: %08lx\n", regs->pstate);
> + printf("SP : %lx\n", regs->sp);
> + for (i = 29; i >= 0; i--) {
> + printf("x%-2d: %016lx ", i, regs->regs[i]);
> + if (i % 2 == 0)
> + printf("\n");
> + }
> + printf("\n");
> +}
This is almost identical to the Linux code, yet I don't see any
attribution (and the license is incompatible).
> diff --git a/arch/arm/cpu/armv8/relocate.S b/arch/arm/cpu/armv8/relocate.S
> new file mode 100644
> index 0000000..87b5ef0
> --- /dev/null
> +++ b/arch/arm/cpu/armv8/relocate.S
> @@ -0,0 +1,71 @@
> +/*
> + * relocate - common relocation function for ARM64 U-Boot
> + *
> + * Copyright (c) 2013 David Feng <fenghua at phytium.com.cn>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
This is obviously derived from arch/arm/lib/relocate.S, but I see no
attribution or retained copyright notice.
> +#ifdef __ASSEMBLY__
> +#define _AC(X,Y) X
> +#define _AT(T,X) X
> +#else
> +#define __AC(X,Y) (X##Y)
> +#define _AC(X,Y) __AC(X,Y)
> +#define _AT(T,X) ((T)(X))
> +#endif
> +
> +#define UL(x) _AC(x, UL)
This needs a code comment, better names, and should be moved to a more
common place.
Where do you use _AT?
> diff --git a/arch/arm/include/asm/macro.h b/arch/arm/include/asm/macro.h
> index ff13f36..70be196 100644
> --- a/arch/arm/include/asm/macro.h
> +++ b/arch/arm/include/asm/macro.h
> @@ -54,5 +54,31 @@
> bcs 1b
> .endm
>
> +#ifdef CONFIG_ARMV8
> +/*
> + * Register aliases.
> + */
> +lr .req x30 // link register
> +
> +/*
> + * Stack pushing/popping (register pairs only).
> + * Equivalent to store decrement before,
> + * load increment after.
> + */
> +.macro push, xreg1, xreg2
> + stp \xreg1, \xreg2, [sp, #-16]!
> +.endm
> +
> +.macro pop, xreg1, xreg2
> + ldp \xreg1, \xreg2, [sp], #16
> +.endm
More uncredited and license-incompatible Linux copying (the macros
themselves might not be copyrightable, but the comments are...).
> diff --git a/arch/arm/include/asm/posix_types.h b/arch/arm/include/asm/posix_types.h
> index c412486..0da38cb 100644
> --- a/arch/arm/include/asm/posix_types.h
> +++ b/arch/arm/include/asm/posix_types.h
> @@ -19,6 +19,8 @@
> * assume GCC is being used.
> */
>
> +#ifndef CONFIG_ARMV8
You need to include config.h first. For example, the warnings in
lib/hashtable.c are due to picking up the 32-bit definition of
__kernel_size_t.
You should be considering these warnings to be errors.
> typedef unsigned short __kernel_dev_t;
> typedef unsigned long __kernel_ino_t;
> typedef unsigned short __kernel_mode_t;
> @@ -44,6 +46,35 @@ typedef unsigned int __kernel_gid32_t;
> typedef unsigned short __kernel_old_uid_t;
> typedef unsigned short __kernel_old_gid_t;
>
> +#else /* CONFIG_ARMV8 */
> +
> +typedef unsigned short __kernel_dev_t;
> +typedef unsigned long __kernel_ino_t;
> +typedef unsigned short __kernel_mode_t;
> +typedef unsigned short __kernel_nlink_t;
> +typedef long __kernel_off_t;
> +typedef int __kernel_pid_t;
> +typedef unsigned short __kernel_ipc_pid_t;
> +typedef unsigned short __kernel_uid_t;
> +typedef unsigned short __kernel_gid_t;
> +typedef unsigned long __kernel_size_t;
> +typedef long __kernel_ssize_t;
> +typedef long __kernel_ptrdiff_t;
> +typedef long __kernel_time_t;
> +typedef long __kernel_suseconds_t;
> +typedef long __kernel_clock_t;
> +typedef long __kernel_daddr_t;
> +typedef char * __kernel_caddr_t;
> +typedef unsigned short __kernel_uid16_t;
> +typedef unsigned short __kernel_gid16_t;
> +typedef unsigned int __kernel_uid32_t;
> +typedef unsigned int __kernel_gid32_t;
> +
> +typedef __kernel_uid_t __kernel_old_uid_t;
> +typedef __kernel_gid_t __kernel_old_gid_t;
> +
> +#endif /* CONFIG_ARMV8 */
Most of these types do not need to be different between 32 and 64 bit
(FWIW, I find it pretty obnoxious that GCC insists on "unsigned int" for
%zu on 32-bit, rather than "unsigned long").
Also please use positive logic for if/else -- that is, use
"ifdef/else/endif" rather than "ifndef/else/endif".
> iff --git a/arch/arm/include/asm/types.h b/arch/arm/include/asm/types.h
> index 71dc049..128dd3e 100644
> --- a/arch/arm/include/asm/types.h
> +++ b/arch/arm/include/asm/types.h
> @@ -18,8 +18,13 @@ typedef __signed__ int __s32;
> typedef unsigned int __u32;
>
> #if defined(__GNUC__)
> +#ifndef CONFIG_ARMV8
> __extension__ typedef __signed__ long long __s64;
> __extension__ typedef unsigned long long __u64;
> +#else /* CONFIG_ARMV8 */
> +__extension__ typedef __signed__ long __s64;
> +__extension__ typedef unsigned long __u64;
> +#endif /* CONFIG_ARMV8 */
> #endif
Please just always use long long here. It makes it easier to use u64 in
printf().
> diff --git a/arch/arm/include/asm/unaligned.h b/arch/arm/include/asm/unaligned.h
> index 44593a8..a051e97 100644
> --- a/arch/arm/include/asm/unaligned.h
> +++ b/arch/arm/include/asm/unaligned.h
> @@ -8,6 +8,8 @@
> /*
> * Select endianness
> */
> +#ifndef CONFIG_ARMV8
> +
> #ifndef __ARMEB__
> #define get_unaligned __get_unaligned_le
> #define put_unaligned __put_unaligned_le
> @@ -16,4 +18,16 @@
> #define put_unaligned __put_unaligned_be
> #endif
>
> +#else /* CONFIG_ARMV8 */
> +
> +#ifndef __AARCH64EB__
Can you use __BYTE_ORDER__ instead so that 32/64 becomes irrelevant?
Likewise elsewhere.
> diff --git a/examples/standalone/stubs.c b/examples/standalone/stubs.c
> index 8fb1765..10676f1 100644
> --- a/examples/standalone/stubs.c
> +++ b/examples/standalone/stubs.c
> @@ -39,6 +39,7 @@ gd_t *global_data;
> " bctr\n" \
> : : "i"(offsetof(gd_t, jt)), "i"(XF_ ## x * sizeof(void *)) : "r11");
> #elif defined(CONFIG_ARM)
> +#ifndef CONFIG_ARMV8
CONFIG_ARM64, and use positive logic for if/else
> /*
> * r8 holds the pointer to the global_data, ip is a call-clobbered
> * register
> @@ -50,6 +51,20 @@ gd_t *global_data;
> " ldr ip, [r8, %0]\n" \
> " ldr pc, [ip, %1]\n" \
> : : "i"(offsetof(gd_t, jt)), "i"(XF_ ## x * sizeof(void *)) : "ip");
> +#else
> +/*
> + * x18 holds the pointer to the global_data, x9 is a call-clobbered
> + * register
> + */
> +#define EXPORT_FUNC(x) \
> + asm volatile ( \
> +" .globl " #x "\n" \
> +#x ":\n" \
> +" ldr x9, [x18, %0]\n" \
> +" ldr x9, [x9, %1]\n" \
> +" br x9\n" \
> + : : "i"(offsetof(gd_t, jt)), "i"(XF_ ## x * sizeof(void *)) : "x19");
> +#endif
Clobber should be x9, not x19.
-Scott
More information about the U-Boot
mailing list