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

[Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com>]

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