Re: [PATCH 02/11] arm: arm926ej-s: add sunxi code

[Andre Przywara <andre.przywara@arm.com>]

On Tue,  4 Jan 2022 19:34:59 -0500
Jesse Taube <mr.bossman075@gmail.com> wrote:

Hi,

> From: Icenowy Zheng <icenowy@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@aosc.io>
> Signed-off-by: Jesse Taube <Mr.Bossman075@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@henriknordstrom.net>
> +#
> +# Based on some other Makefile
> +# (C) Copyright 2000-2003
> +# Wolfgang Denk, DENX Software Engineering, wd@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.

> +
> +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.

> +	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.

> +
> +.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? 

> +	/*
> +	 * 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.

> +	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.

> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: (GPL-2.0+)
> +/*
> + * (C) Copyright 2007-2011
> + * Allwinner Technology Co., Ltd. <www.allwinnertech.com>
> + * Tom Cubie <tangliang@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?

> +{
> +	/* 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@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@allwinnertech.com>
> + *

I think that's enough history at this point ...

> + * Based on omap-common/u-boot-spl.lds:
> + *
> + * (C) Copyright 2002
> + * Gary Jennejohn, DENX Software Engineering, <garyj@denx.de>
> + *
> + * (C) Copyright 2010
> + * Texas Instruments, <www.ti.com>
> + *	Aneesh V <aneesh@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?

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
> +}