> On 28 Aug 2018, at 11:23, Kever Yang <kever.y...@rock-chips.com> wrote:
>
> Rockchip BootRom support load firmware from storage media twice,
> one for ddr sdram init code into sram and another time to load
> firmware into DDR. For ddr sdram init code(which is TPL in U-Boot
> project), it's OK to re-use the stack and vector table. In order
> to get more available sram space, we need to skip all the init code
> from U-Boot project including start.S, reset.S and framework.
>
> Signed-off-by: Kever Yang <kever.y...@rock-chips.com>
Reviewed-by: Philipp Tomsich <philipp.toms...@theobroma-systems.com>
See below for a few recommended changes, a question and one required change.
> ---
>
> arch/arm/cpu/armv7/start.S | 2 +
> arch/arm/cpu/armv8/start.S | 2 +
> arch/arm/include/asm/arch-rockchip/boot0.h | 10 ++
> arch/arm/mach-rockchip/Kconfig | 12 +++
> arch/arm/mach-rockchip/Makefile | 2 +
> arch/arm/mach-rockchip/tiny_tpl.c | 106 +++++++++++++++++++++
> 6 files changed, 134 insertions(+)
> create mode 100644 arch/arm/mach-rockchip/tiny_tpl.c
>
> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> index 81edec01bf..548d9ff23a 100644
> --- a/arch/arm/cpu/armv7/start.S
> +++ b/arch/arm/cpu/armv7/start.S
> @@ -38,7 +38,9 @@
> reset:
> /* Allow the board to save important registers */
> b save_boot_params
> +#if !CONFIG_IS_ENABLED(TINY_FRAMEWORK)
> save_boot_params_ret:
> +#endif
Having the label here will not increase code-size, so there’s no point in
making this conditional.
> #ifdef CONFIG_ARMV7_LPAE
> /*
> * check for Hypervisor support
> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> index d4db4d044f..e0f8fad10c 100644
> --- a/arch/arm/cpu/armv8/start.S
> +++ b/arch/arm/cpu/armv8/start.S
> @@ -31,6 +31,7 @@ _start:
> b reset
> #endif
>
> +#if !CONFIG_IS_ENABLED(TINY_FRAMEWORK)
> .align 3
>
> .globl _TEXT_BASE
> @@ -361,3 +362,4 @@ ENDPROC(c_runtime_cpu_setup)
> WEAK(save_boot_params)
> b save_boot_params_ret /* back to my caller */
> ENDPROC(save_boot_params)
> +#endif
> diff --git a/arch/arm/include/asm/arch-rockchip/boot0.h
> b/arch/arm/include/asm/arch-rockchip/boot0.h
> index 9ea4708ada..7f00494513 100644
> --- a/arch/arm/include/asm/arch-rockchip/boot0.h
> +++ b/arch/arm/include/asm/arch-rockchip/boot0.h
> @@ -41,8 +41,18 @@ entry_counter:
>
> #if (defined(CONFIG_SPL_BUILD) || defined(CONFIG_ARM64))
> /* U-Boot proper of armv7 do not need this */
> +#if CONFIG_IS_ENABLED(TINY_FRAMEWORK)
> + /* Allow the board to save important registers */
> + b save_boot_params
> +.globl save_boot_params_ret
> +.type save_boot_params_ret, %function
> +
> +save_boot_params_ret:
> + b board_init_f
> +#else
> b reset
> #endif
> +#endif
I don’t like these nested #if primitives.
Is there a way to do this without nesting?
E.g.
#if CONFIG_IS_ENABLED(TINY_FRAMEWORK)
b reset
#elif …
original code
#endif
>
> #if !defined(CONFIG_ARM64)
> /*
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index 187aa14184..be23bcc37e 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -306,6 +306,18 @@ config TPL_ROCKCHIP_EARLYRETURN_TO_BROM
> config SPL_MMC_SUPPORT
> default y if !SPL_ROCKCHIP_BACK_TO_BROM
>
> +config TPL_TINY_FRAMEWORK
> + bool "Tiny TPL for DDR init"
> + depends on TPL && !TPL_FRAMEWORK
> + default y
> + help
> + Rockchip BootRom support load firmware from storage media twice,
> + one for ddr sdram init code into sram and another time to load
> + firmware into DDR. For ddr sdram init code(which is TPL in U-Boot
> + project), it's OK to re-use the stack and vector table. In order
> + to get more available sram space, we need to skip all the init code
> + from U-Boot project including start.S, reset.S and framework.
> +
> source "arch/arm/mach-rockchip/rk3036/Kconfig"
> source "arch/arm/mach-rockchip/rk3128/Kconfig"
> source "arch/arm/mach-rockchip/rk3188/Kconfig"
> diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile
> index 83afdac99d..c88700f10d 100644
> --- a/arch/arm/mach-rockchip/Makefile
> +++ b/arch/arm/mach-rockchip/Makefile
> @@ -9,6 +9,8 @@
> obj-spl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o
> obj-tpl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o
>
> +obj-tpl-$(CONFIG_TPL_TINY_FRAMEWORK) += tiny_tpl.o
> +
> obj-tpl-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board-tpl.o
> obj-tpl-$(CONFIG_ROCKCHIP_RK3368) += rk3368-board-tpl.o
> obj-tpl-$(CONFIG_ROCKCHIP_RK322X) += rk322x-board-tpl.o
> diff --git a/arch/arm/mach-rockchip/tiny_tpl.c
> b/arch/arm/mach-rockchip/tiny_tpl.c
> new file mode 100644
> index 0000000000..47f15c0af1
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/tiny_tpl.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2018 Rockchip Electronics Co., Ltd
> + */
> +
> +#include <common.h>
> +#include <debug_uart.h>
> +#include <spl.h>
> +#include <version.h>
> +#include <asm/io.h>
> +#include <asm/arch/bootrom.h>
> +#include <asm/arch-rockchip/sys_proto.h>
> +
> +#ifndef CONFIG_TPL_LIBGENERIC_SUPPORT
> +#ifdef CONFIG_ARM64
> +/* for ARM64,it don't have define timer_init and __udelay except lib/timer.c
> */
> +int __weak timer_init(void)
> +{
> + return 0;
> +}
> +
> +void __weak __udelay(unsigned long usec)
> +{
> + u64 i, j, count;
> +
> + asm volatile ("MRS %0, CNTPCT_EL0" : "=r"(count));
> + i = count;
> + /* usec to count,24MHz */
> + j = usec * 24;
> + i += j;
> + while (1) {
> + asm volatile ("MRS %0, CNTPCT_EL0" : "=r"(count));
> + if (count > i)
> + break;
> + }
> +}
> +#endif /* CONFIG_ARM64 */
> +void udelay(unsigned long usec)
> +{
> + __udelay(usec);
> +}
> +
> +void hang(void)
> +{
> + for (;;)
> + ;
> +}
> +#endif /* CONFIG_TPL_LIBGENERIC_SUPPORT */
> +
> +u32 spl_boot_device(void)
> +{
> + return BOOT_DEVICE_BOOTROM;
> +}
> +
> +__weak void rockchip_stimer_init(void)
> +{
> +#ifndef CONFIG_ARM64
If we need to keep an #if here, then please have this not inverted.
I.e.
#if defined(CONFIG_ARM)
armv8 code here
#else
armv7 code here
#endif
> + asm volatile("mcr p15, 0, %0, c14, c0, 0"
> + : : "r"(COUNTER_FREQUENCY));
> +#else
> + /*
> + * For ARM64,generally initialize CNTFRQ in start.S,
> + * but if defined CONFIG_TPL_TINY_FRAMEWORK should skip start.S.
> + * So initialize CNTFRQ to 24MHz here.
> + */
> + asm volatile("msr CNTFRQ_EL0, %0"
> + : : "r" (COUNTER_FREQUENCY));
> +#endif
Can we do this without having the #ifdef here?
Maybe call an inline function that is implemented differently in the armv7 and
armv8 header files?
> + writel(0, CONFIG_ROCKCHIP_STIMER_BASE + 0x10);
> + writel(0xffffffff, CONFIG_ROCKCHIP_STIMER_BASE);
> + writel(0xffffffff, CONFIG_ROCKCHIP_STIMER_BASE + 4);
> + writel(1, CONFIG_ROCKCHIP_STIMER_BASE + 0x10);
NAK.
Don’t make CONFIG_ROCKCHIP_STIMER_BASE configurable via Kconfig.
This is not a user-configurable option, but a direct consequence of the chip
we’re using.
In other words: it’s not a CONFIG-option.
> +}
> +
> +__weak int arch_cpu_init(void)
> +{
> + return 0;
> +}
> +
> +void board_init_f(ulong dummy)
> +{
> + rockchip_stimer_init();
> + arch_cpu_init();
> +#define EARLY_DEBUG
> +#ifdef EARLY_DEBUG
> + /*
> + * Debug UART can be used from here if required:
> + *
> + * debug_uart_init();
> + * printch('a');
> + * printhex8(0x1234);
> + * printascii("string");
> + */
> + debug_uart_init();
> + printascii("\nU-Boot Tiny TPL " PLAIN_VERSION " (" U_BOOT_DATE " - " \
> + U_BOOT_TIME ")\n");
> +#endif
> +
> + /* Init ARM arch timer */
> + timer_init();
> +
> + /* We are not going to use DM framework in tiny TPL */
> + sdram_init();
> +
> + back_to_bootrom(BROM_BOOT_NEXTSTAGE);
> +}
> --
> 2.18.0
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
