On Sat, 2013-09-07 at 22:56 +0800, FengHua wrote:
> Fisrt, thank scott for checking the patch.
>
> > -----原始邮件-----
> > 发件人: "Scott Wood" <scottw...@freescale.com>
> > 发送时间: 2013年9月7日 星期六
> > 收件人: feng...@phytium.com.cn
> > 抄送: u-boot@lists.denx.de, tr...@ti.com
> > 主题: Re: [U-Boot] [PATCH v5 1/4] core support of arm64
> >
> > On Sat, 2013-08-24 at 09:06 +0800, feng...@phytium.com.cn wrote:
> > > From: David Feng <feng...@phytium.com.cn>
> > >
> > > Signed-off-by: David Feng <feng...@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.
Files that are about 64-bit rather than armv8 should not go in the armv8
directory. E.g. relocate should be arch/arm/lib/relocate64.S.
> > > +/*
> > > + * 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.
Sometimes that's done when there are multiple versions of a function,
but it's bad practice since you're in a reserved namespace (especially
if you go and do the same thing in a normal userspace program). In this
case there isn't even a non-underscore version, so it's just gratuitous.
> > > +/*
> > > + * 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*.
Is this for flushing caches that are outside the core? On powerpc you
could do that with architected instructions for a range flush -- is that
not the case with ARM? Do the IC/DC instructions not operate on caches
outside the core?
> > > 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 <feng...@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.
GCC complains without --fixed-x18 because it views it as a volatile
register. The ABI says this about it: "The Platform Register. If the
platform ABI needs a dedicated register, this is it. Otherwise a
temporary register."
> > > 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.
Please be aware that modifying the comments is generally not an
acceptable alternative to rewriting from scratch. The non-comment
portion of the above quote might not be regarded as copyrightable, but
we're not lawyers and shouldn't rely on such things if we can avoid it.
> > > 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
No. If you use a config symbol in this file, then this file should
include config.h. Otherwise we'll silently break on the next file that
has a similar problem.
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
