Re: [U-Boot] [PATCH] rockchip: add TPL_TINY_FRAMEWORK support

On 28 Aug 2018, at 11:23, Kever Yang kever.yang@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.yang@rock-chips.com

Reviewed-by: Philipp Tomsich philipp.tomsich@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