[U-Boot] [PATCH v5 1/4] core support of arm64
FengHua
fenghua at phytium.com.cn
Sat Sep 7 16:56:39 CEST 2013
Fisrt, thank scott for checking the patch.
> -----原始邮件-----
> 发件人: "Scott Wood" <scottwood at freescale.com>
> 发送时间: 2013年9月7日 星期六
> 收件人: fenghua at phytium.com.cn
> 抄送: u-boot at lists.denx.de, trini at ti.com
> 主题: Re: [U-Boot] [PATCH v5 1/4] core support of arm64
>
> 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.
>
Actually, the naming is so confusing. The directory's name is armv8,
but it only represents aarch64 here. So, whether CONFIG_ARMV8 or CONFIG_ARM64 should be used
is difficult to make decision.
> > +/*
> > + * 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?
>
Is there any convention with underscores? I just know the underscore represent that the function is best not be called directly.
> > +/* 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"?
It's a bug, I will fix it.
>
> > +#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?
It's socs. I don't know how the armv8 processor would like to be. It's just derived from __v7_outer_cache*.
>
> > 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?
>
I am not sure and will check it.
> > 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).
>
I will rewrite it.
> > 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...).
>
I will modify the comments.
> > 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.
Maybe, it is better that config.h is included in hashtable.c
>
> You should be considering these warnings to be errors.
>
I will remove all the warnings.
> > 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().
>
That's right.
> > 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.
>
__ARMEB__ is used in a few files, we can replace it with __BYTE_ORDER here.
other file has no relationship with armv8, I will keep it.
> > 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.
Sorry, It's a neglect.
>
> -Scott
>
>
>
More information about the U-Boot
mailing list