[PATCH 02/11] arm: arm926ej-s: add sunxi code
Andre Przywara
andre.przywara at arm.com
Mon Jan 24 02:45:45 CET 2022
On Thu, 20 Jan 2022 22:16:41 -0500
Jesse Taube <mr.bossman075 at gmail.com> wrote:
Hi Jesse,
> On 1/20/22 21:25, Andre Przywara wrote:
> > On Tue, 4 Jan 2022 19:34:59 -0500
> > Jesse Taube <mr.bossman075 at gmail.com> wrote:
> >
> > Hi,
> >
> >> From: Icenowy Zheng <icenowy at aosc.io>
> >>
> >> Some Allwinner SoCs use ARM926EJ-S core.
> >>
> >> Add Allwinner/sunXi specific code to ARM926EJ-S CPU dircetory.
> >>
> >> Signed-off-by: Icenowy Zheng <icenowy at aosc.io>
> >> Signed-off-by: Jesse Taube <Mr.Bossman075 at gmail.com>
> >> ---
> >> arch/arm/cpu/arm926ejs/Makefile | 1 +
> >> arch/arm/cpu/arm926ejs/sunxi/Makefile | 15 +++
> >> arch/arm/cpu/arm926ejs/sunxi/config.mk | 6 +
> >> arch/arm/cpu/arm926ejs/sunxi/fel_utils.S | 37 ++++++
> >> arch/arm/cpu/arm926ejs/sunxi/lowlevel_init.S | 67 +++++++++++
> >> arch/arm/cpu/arm926ejs/sunxi/start.c | 1 +
> >> arch/arm/cpu/arm926ejs/sunxi/timer.c | 114 +++++++++++++++++++
> >> arch/arm/cpu/arm926ejs/sunxi/u-boot-spl.lds | 62 ++++++++++
> >> 8 files changed, 303 insertions(+)
> >> create mode 100644 arch/arm/cpu/arm926ejs/sunxi/Makefile
> >> create mode 100644 arch/arm/cpu/arm926ejs/sunxi/config.mk
> >> create mode 100644 arch/arm/cpu/arm926ejs/sunxi/fel_utils.S
> >> create mode 100644 arch/arm/cpu/arm926ejs/sunxi/lowlevel_init.S
> >> create mode 100644 arch/arm/cpu/arm926ejs/sunxi/start.c
> >> create mode 100644 arch/arm/cpu/arm926ejs/sunxi/timer.c
> >> create mode 100644 arch/arm/cpu/arm926ejs/sunxi/u-boot-spl.lds
> >>
> >> diff --git a/arch/arm/cpu/arm926ejs/Makefile b/arch/arm/cpu/arm926ejs/Makefile
> >> index b901b7c5c9..7f1436d76e 100644
> >> --- a/arch/arm/cpu/arm926ejs/Makefile
> >> +++ b/arch/arm/cpu/arm926ejs/Makefile
> >> @@ -15,6 +15,7 @@ endif
> >> obj-$(CONFIG_MX27) += mx27/
> >> obj-$(if $(filter mxs,$(SOC)),y) += mxs/
> >> obj-$(if $(filter spear,$(SOC)),y) += spear/
> >> +obj-$(CONFIG_ARCH_SUNXI) += sunxi/
> >>
> >> # some files can only build in ARM or THUMB2, not THUMB1
> >>
> >> diff --git a/arch/arm/cpu/arm926ejs/sunxi/Makefile b/arch/arm/cpu/arm926ejs/sunxi/Makefile
> >> new file mode 100644
> >> index 0000000000..894c461c50
> >> --- /dev/null
> >> +++ b/arch/arm/cpu/arm926ejs/sunxi/Makefile
> >> @@ -0,0 +1,15 @@
> >> +# SPDX-License-Identifier: GPL-2.0+
> >> +#
> >> +# (C) Copyright 2012 Henrik Nordstrom <henrik at henriknordstrom.net>
> >> +#
> >> +# Based on some other Makefile
> >> +# (C) Copyright 2000-2003
> >> +# Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> >
> > Please remove this heritage. I appreciate the effort to give credit,
> > but for a trivial Makefile stub (which even differs significantly) this
> > is surely overkill.
> > Just one line with some current copyright should be enough.
> >
> Fixed.
> >> +
> >> +obj-y += timer.o
> >> +obj-y += lowlevel_init.o
> >> +
> >> +ifdef CONFIG_SPL_BUILD
> >> +obj-y += fel_utils.o
> >> +CFLAGS_fel_utils.o := -marm
> >> +endif
> >> diff --git a/arch/arm/cpu/arm926ejs/sunxi/config.mk b/arch/arm/cpu/arm926ejs/sunxi/config.mk
> >> new file mode 100644
> >> index 0000000000..76ffec9df6
> >> --- /dev/null
> >> +++ b/arch/arm/cpu/arm926ejs/sunxi/config.mk
> >> @@ -0,0 +1,6 @@
> >> +# Build a combined spl + u-boot image
> >> +ifdef CONFIG_SPL
> >> +ifndef CONFIG_SPL_BUILD
> >> +ALL-y += u-boot-sunxi-with-spl.bin
> >> +endif
> >> +endif
> >> diff --git a/arch/arm/cpu/arm926ejs/sunxi/fel_utils.S b/arch/arm/cpu/arm926ejs/sunxi/fel_utils.S
> >> new file mode 100644
> >> index 0000000000..0997a2dc65
> >> --- /dev/null
> >> +++ b/arch/arm/cpu/arm926ejs/sunxi/fel_utils.S
> >> @@ -0,0 +1,37 @@
> >> +/* SPDX-License-Identifier: GPL-2.0+ */
> >> +/*
> >> + * Utility functions for FEL mode.
> >> + *
> >> + * Copyright (c) 2015 Google, Inc
> >> + */
> >> +
> >> +#include <asm-offsets.h>
> >> +#include <config.h>
> >> +#include <asm/system.h>
> >> +#include <linux/linkage.h>
> >> +
> >> +ENTRY(save_boot_params)
> >> + ldr r0, =fel_stash
> >> + str sp, [r0, #0]
> >> + str lr, [r0, #4]
> >> + mrs lr, cpsr @ Read CPSR
> >> + str lr, [r0, #8]
> >> + mrc p15, 0, lr, c1, c0, 0 @ Read CP15 SCTLR Register
> >> + str lr, [r0, #12]
> >> + mrc p15, 0, lr, c1, c0, 0 @ Read CP15 Control Register
> >> + str lr, [r0, #16]
> >
> > This is the very same register twice, also written to the wrong offset.
> > Please remove the last two lines.
> > Yes, this is a bug in armv7/sunxi/fel_utils.S as well, I will send a
> > fix.
> Please CC me.
> >
> >> + b save_boot_params_ret
> >> +ENDPROC(save_boot_params)
> >> +
> >> +ENTRY(return_to_fel)
> >> + mov sp, r0
> >> + mov lr, r1
> >> + ldr r0, =fel_stash
> >> + ldr r1, [r0, #16]
> >> + mcr p15, 0, r1, c1, c0, 0 @ Write CP15 Control Register
> >
> > Same here, those two lines can be removed.
> >
> >> + ldr r1, [r0, #12]
> >> + mcr p15, 0, r1, c1, c0, 0 @ Write CP15 SCTLR Register
> >> + ldr r1, [r0, #8]
> >> + msr cpsr, r1 @ Write CPSR
> >> + bx lr
> >> +ENDPROC(return_to_fel)
> >> diff --git a/arch/arm/cpu/arm926ejs/sunxi/lowlevel_init.S b/arch/arm/cpu/arm926ejs/sunxi/lowlevel_init.S
> >> new file mode 100644
> >> index 0000000000..db09bcc4d0
> >> --- /dev/null
> >> +++ b/arch/arm/cpu/arm926ejs/sunxi/lowlevel_init.S
> >> @@ -0,0 +1,67 @@
> >> +/* SPDX-License-Identifier: GPL-2.0+ */
> >> +/*
> >> + * A lowlevel_init function that sets up the stack to call a C function to
> >> + * perform further init.
> >> + *
> >> + * Based on lowlevel_init.S in armv7 directory, which is:
> >> + * (C) Copyright 2010 Texas Instruments, <www.ti.com>
> >> + */
> >> +
> >> +#include <asm-offsets.h>
> >> +#include <config.h>
> >> +#include <linux/linkage.h>
> >> +
> >> +.pushsection .text.s_init, "ax"
> >> +WEAK(s_init)
> >> + bx lr
> >> +ENDPROC(s_init)
> >> +.popsection
> >
> > I would say we don't need this weak stub. In contrast to armv7, we are
> > the only user of this file, and s_init should be provided by
> > arch/arm/mach-sunxi/board.c.
> >
> > In general the toplevel README seems to deprecate lowlevel_init, but
> > this probably needs some fixing in the other sunxi code first.
> I removed it its defined in board like you said.
> >> +
> >> +.pushsection .text.lowlevel_init, "ax"
> >> +WEAK(lowlevel_init)
> >> + /*
> >> + * Setup a temporary stack. Global data is not available yet.
> >> + */
> >> +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
> >> + ldr sp, =CONFIG_SPL_STACK
> >> +#else
> >> + ldr sp, =CONFIG_SYS_INIT_SP_ADDR
> >> +#endif
> >> + bic sp, sp, #7 /* 8-byte alignment for ABI compliance */
> >> +#ifdef CONFIG_SPL_DM
> >> + mov r9, #0
> >> +#else
> >
> > We probably don't need this SPL_DM part?
> Hmmmmm you are right im assuiming this got copied from somewhere. I
> replaced the ifdef with `mov r9, #0`.
Well, we don't define SPL_DM, and probably will never do. Setting r9 to
0 does not make much sense, since we overwrite it below. So remove all
three lines and the corresponding #endif below.
But ...
> >
> >> + /*
> >> + * Set up global data for boards that still need it. This will be
> >> + * removed soon.
> >> + */
> >> +#ifdef CONFIG_SPL_BUILD
> >> + ldr r9, =gdata
> >> +#else
> >> + sub sp, sp, #GD_SIZE
> >> + bic sp, sp, #7
> >> + mov r9, sp
> >> +#endif
> >> +#endif
> >> + /*
> >> + * Save the old lr(passed in ip) and the current lr to stack
> >> + */
> >> + push {ip, lr}
> >> +
> >> + /*
> >> + * Call the very early init function. This should do only the
> >> + * absolute bare minimum to get started. It should not:
> >> + *
> >> + * - set up DRAM
> >> + * - use global_data
> >> + * - clear BSS
> >> + * - try to start a console
> >> + *
> >> + * For boards with SPL this should be empty since SPL can do all of
> >> + * this init in the SPL board_init_f() function which is called
> >> + * immediately after this.
> >> + */
> >
> > Yeah, this (copied) comments seems to suggest we are somewhat off here.
> > But I think we rely on board.c:s_init() too much to easily remove this.
> >
> > So for the sake of not blocking this I am willing to keep this part for
> > now.
> Much apretiated!
Actually I had a look, and I was able to remove the need for
lowlevel_init from sunxi altogether (in arch/.../board.c), basically by
moving the code to board_init_f() and filling the gaps. So we don't need
to copy this whole function here at all. I am not sure those patches
make it in time, though (need some more testing), so we might just go
with this (fixed) patch here, and remove lowlevel_init later.
> >
> >> + bl s_init
> >> + pop {ip, pc}
> >> +ENDPROC(lowlevel_init)
> >> +.popsection
> >> diff --git a/arch/arm/cpu/arm926ejs/sunxi/start.c b/arch/arm/cpu/arm926ejs/sunxi/start.c
> >> new file mode 100644
> >> index 0000000000..6b392fa835
> >> --- /dev/null
> >> +++ b/arch/arm/cpu/arm926ejs/sunxi/start.c
> >> @@ -0,0 +1 @@
> >> +/* Intentionally empty. Only needed to get FEL SPL link line right */
> >> diff --git a/arch/arm/cpu/arm926ejs/sunxi/timer.c b/arch/arm/cpu/arm926ejs/sunxi/timer.c
> >> new file mode 100644
> >> index 0000000000..e624174581
> >> --- /dev/null
> >> +++ b/arch/arm/cpu/arm926ejs/sunxi/timer.c
> >
> > I don't see immediately why the original file lives in
> > arch/arm/cpu/armv7/sunxi/timer.c. Can we move that to
> > arch/arm/mach-sunxi or board/sunxi, then just not compile it for arm64?
> > Because otherwise this looks identical to its donor, minus some smaller
> > (but good) cleanups.
> > So I'm curious if we can move and then reuse the original file.
> I will try thanks for the pointer.
> >
> >> @@ -0,0 +1,114 @@
> >> +// SPDX-License-Identifier: (GPL-2.0+)
> >> +/*
> >> + * (C) Copyright 2007-2011
> >> + * Allwinner Technology Co., Ltd. <www.allwinnertech.com>
> >> + * Tom Cubie <tangliang at allwinnertech.com>
> >> + */
> >> +
> >> +#include <common.h>
> >> +#include <asm/io.h>
> >> +#include <asm/arch/timer.h>
> >> +#include <asm/global_data.h>
> >> +
> >> +DECLARE_GLOBAL_DATA_PTR;
> >> +
> >> +#define TIMER_MODE (0x0 << 7) /* continuous mode */
> >> +#define TIMER_DIV (0x0 << 4) /* pre scale 1 */
> >> +#define TIMER_SRC (0x1 << 2) /* osc24m */
> >> +#define TIMER_RELOAD (0x1 << 1) /* reload internal value */
> >> +#define TIMER_EN (0x1 << 0) /* enable timer */
> >> +
> >> +#define TIMER_CLOCK (24 * 1000 * 1000)
> >> +#define COUNT_TO_USEC(x) ((x) / 24)
> >> +#define USEC_TO_COUNT(x) ((x) * 24)
> >> +#define TICKS_PER_HZ (TIMER_CLOCK / CONFIG_SYS_HZ)
> >> +#define TICKS_TO_HZ(x) ((x) / TICKS_PER_HZ)
> >> +
> >> +#define TIMER_LOAD_VAL 0xffffffff
> >> +
> >> +#define TIMER_NUM 0 /* we use timer 0 */
> >> +
> >> +/* read the 32-bit timer */
> >> +static ulong read_timer(void)
> >> +{
> >> + struct sunxi_timer_reg *timers =
> >> + (struct sunxi_timer_reg *)SUNXI_TIMER_BASE;
> >> + struct sunxi_timer *timer = &timers->timer[TIMER_NUM];
> >> +
> >> + /*
> >> + * The hardware timer counts down, therefore we invert to
> >> + * produce an incrementing timer.
> >> + */
> >> + return ~readl(&timer->val);
> >> +}
> >> +
> >> +/* init timer register */
> >> +int timer_init(void)
> >> +{
> >> + struct sunxi_timer_reg *timers =
> >> + (struct sunxi_timer_reg *)SUNXI_TIMER_BASE;
> >> + struct sunxi_timer *timer = &timers->timer[TIMER_NUM];
> >> +
> >> + writel(TIMER_LOAD_VAL, &timer->inter);
> >> + writel(TIMER_MODE | TIMER_DIV | TIMER_SRC | TIMER_RELOAD | TIMER_EN,
> >> + &timer->ctl);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +ulong get_timer_masked(void)
> >
> > Any reason you lost the static here?
> No, presumibly becuse it got changed sence 2018.
> >
> >> +{
> >> + /* current tick value */
> >> + ulong now = TICKS_TO_HZ(read_timer());
> >> +
> >> + if (now >= gd->arch.lastinc) { /* normal (non rollover) */
> >> + gd->arch.tbl += (now - gd->arch.lastinc);
> >> + } else {
> >> + /* rollover */
> >> + gd->arch.tbl += (TICKS_TO_HZ(TIMER_LOAD_VAL)
> >> + - gd->arch.lastinc) + now;
> >> + }
> >> + gd->arch.lastinc = now;
> >> +
> >> + return gd->arch.tbl;
> >> +}
> >> +
> >> +/* timer without interrupts */
> >> +ulong get_timer(ulong base)
> >> +{
> >> + return get_timer_masked() - base;
> >> +}
> >> +
> >> +/* delay x useconds */
> >> +void __udelay(unsigned long usec)
> >> +{
> >> + long tmo = USEC_TO_COUNT(usec);
> >> + ulong now, last = read_timer();
> >> +
> >> + while (tmo > 0) {
> >> + now = read_timer();
> >> + if (now > last) /* normal (non rollover) */
> >> + tmo -= now - last;
> >> + else /* rollover */
> >> + tmo -= TIMER_LOAD_VAL - last + now;
> >> + last = now;
> >> + }
> >> +}
> >> +
> >> +/*
> >> + * This function is derived from PowerPC code (read timebase as long long).
> >> + * On ARM it just returns the timer value.
> >> + */
> >> +unsigned long long get_ticks(void)
> >> +{
> >> + return get_timer(0);
> >> +}
> >> +
> >> +/*
> >> + * This function is derived from PowerPC code (timebase clock frequency).
> >> + * On ARM it returns the number of timer ticks per second.
> >> + */
> >> +ulong get_tbclk(void)
> >> +{
> >> + return CONFIG_SYS_HZ;
> >> +}
> >> diff --git a/arch/arm/cpu/arm926ejs/sunxi/u-boot-spl.lds b/arch/arm/cpu/arm926ejs/sunxi/u-boot-spl.lds
> >> new file mode 100644
> >> index 0000000000..048aab788a
> >> --- /dev/null
> >> +++ b/arch/arm/cpu/arm926ejs/sunxi/u-boot-spl.lds
> >> @@ -0,0 +1,62 @@
> >> +/* SPDX-License-Identifier: GPL-2.0+ */
> >> +/*
> >> + * (C) Copyright 2018
> >> + * Icenowy Zheng <icenowy at aosc.io>
> >> + *
> >> + * Based on arch/arm/cpu/armv7/sunxi/u-boot-spl.lds:
> >> + *
> >> + * (C) Copyright 2012
> >> + * Allwinner Technology Co., Ltd. <www.allwinnertech.com>
> >> + * Tom Cubie <tangliang at allwinnertech.com>
> >> + *
> >
> > I think that's enough history at this point ...
> LOL I'll fix this, I think Icenowy wanted to make licencing clear.
Well, the licensing is covered by the initial line, the copyright just
means who needs to file the lawsuit in case someone violates the GPL ;-)
It's nice to give credit, but how much copyright-able material there is
in a linker script is somewhat debatable ;-)
> >
> >> + * Based on omap-common/u-boot-spl.lds:
> >> + *
> >> + * (C) Copyright 2002
> >> + * Gary Jennejohn, DENX Software Engineering, <garyj at denx.de>
> >> + *
> >> + * (C) Copyright 2010
> >> + * Texas Instruments, <www.ti.com>
> >> + * Aneesh V <aneesh at ti.com>
> >> + */
> >> +MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE,\
> >> + LENGTH = CONFIG_SPL_MAX_SIZE }
> >> +MEMORY { .sdram : ORIGIN = CONFIG_SPL_BSS_START_ADDR, \
> >> + LENGTH = CONFIG_SPL_BSS_MAX_SIZE }
> >> +
> >> +OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
> >> +OUTPUT_ARCH(arm)
> >> +ENTRY(_start)
> >> +SECTIONS
> >> +{
> >> + .text :
> >> + {
> >> + __start = .;
> >> + *(.vectors)
> >> + arch/arm/cpu/arm926ejs/start.o (.text)
> >
> > Do we actually need this, if it's empty? Isn't that file just to
> > satisfy the needs of the generic linker scripts?
> >
> Nope I droped it and the file.
>
> I'm impresesd with how fast you reviewd this one thanks for the
> feedback. You don't have to review patch 6 unless its okay that dram
> isnt using device tree, I think it should use it so thats on my todo list.
No, don't bother, adding DT and/or DM to the SPL is not going to happen
anytime soon - it just complicated things for no real benefit. You can
check what it takes to move that code to drivers/ram/sunxi, though. I
think I moved one DRAM driver to boards/sunxi as an experiment before,
and that worked rather smoothly, so we might then move the other
drivers as well (though that doesn't really improve anything).
Cheers,
Andre
>
> Thanks,
> Jesse Taube
> > Cheers,
> > Andre.
> >
> >> + *(.text*)
> >> + } > .sram
> >> +
> >> + . = ALIGN(4);
> >> + .rodata : { *(SORT_BY_ALIGNMENT(.rodata*)) } >.sram
> >> +
> >> + . = ALIGN(4);
> >> + .data : { *(SORT_BY_ALIGNMENT(.data*)) } >.sram
> >> +
> >> + . = ALIGN(4);
> >> + .u_boot_list : {
> >> + KEEP(*(SORT(.u_boot_list*)));
> >> + } > .sram
> >> +
> >> + . = ALIGN(4);
> >> + __image_copy_end = .;
> >> + _end = .;
> >> +
> >> + .bss :
> >> + {
> >> + . = ALIGN(4);
> >> + __bss_start = .;
> >> + *(.bss*)
> >> + . = ALIGN(4);
> >> + __bss_end = .;
> >> + } > .sdram
> >> +}
> >
More information about the U-Boot
mailing list