[U-Boot] rockchip: add support for backing to bootrom download mode
Philipp Tomsich
philipp.tomsich at theobroma-systems.com
Tue Sep 12 19:30:13 UTC 2017
On Mon, 11 Sep 2017, Andy Yan wrote:
> Rockchip bootrom will enter download mode if it returns from
> spl/tpl with a none-zero value and couldn't find a valid image
> in the backup partition.
> This patch provide a method to instruct the system to back to
> bootrom download mode by checking the BROM_DOWNLOAD_FLAG register.
> As the bootrom download function relys on some modules such as
> interrupts, so we need to back to bootrom as early as possbile
> before the tpl/tps code override the interrupt settings.
I was not aware that the TPL/SPL overrides interrupt settings. What
exactly does this comment refer to?
>
> Signed-off-by: Andy Yan <andy.yan at rock-chips.com>
> Reviewed-by: Kever Yang <kever.yang at rock-chips.com>
> Acked-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
> ---
>
> arch/arm/include/asm/arch-rockchip/bootrom.h | 2 +-
> arch/arm/mach-rockchip/Kconfig | 13 +++++++
> arch/arm/mach-rockchip/bootrom.c | 2 +-
> arch/arm/mach-rockchip/save_boot_param.S | 57 +++++++++++++++++++++++-----
> 4 files changed, 63 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h b/arch/arm/include/asm/arch-rockchip/bootrom.h
> index 92eb878..6ae3e94 100644
> --- a/arch/arm/include/asm/arch-rockchip/bootrom.h
> +++ b/arch/arm/include/asm/arch-rockchip/bootrom.h
> @@ -22,6 +22,6 @@ void back_to_bootrom(void);
> /**
> * Assembler component for the above (do not call this directly)
> */
> -void _back_to_bootrom_s(void);
> +void _back_to_bootrom_s(int mode);
Please add documentation for externally visible functions.
>
> #endif
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index d9b25d5..3ab0c30 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -113,6 +113,7 @@ config ROCKCHIP_RK3399
> select SPL_SERIAL_SUPPORT
> select SPL_DRIVERS_MISC_SUPPORT
> select ENABLE_ARM_SOC_BOOT0_HOOK
> + select ROCKCHIP_BROM_HELPER
> select DEBUG_UART_BOARD_INIT
> help
> The Rockchip RK3399 is a ARM-based SoC with a dual-core Cortex-A72
> @@ -149,6 +150,18 @@ config TPL_ROCKCHIP_BACK_TO_BROM
> SPL will return to the boot rom, which will then load the U-Boot
> binary to keep going on.
>
> +config ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG
> + hex "Bootrom download mode flag register address"
> + default 0x200081c8 if ROCKCHIP_RK3036
> + default 0xff730094 if ROCKCHIP_RK3288
> + default 0xff738200 if ROCKCHIP_RK3368
> + default 0xff320300 if ROCKCHIP_RK3399
> + default 0x10300580 if ROCKCHIP_RV1108
> + default 0x00
If this is not user-configurable (i.e. if it is a per-chip constant), we
should define this in a header file. I would suggest to do the
detection/mapping either in include/asm/arch-rockchip/bootrom.h or in a
cpu-specific header that gets included from there.
> + help
> + The Soc will return to bootrom download mode if this register set
> + to BOOTROM_DOWNLOAD_FLAG.
Documenting here what BOOTROM_DOWNLOAD_FLAG is or where it can be found
would be very helpful to anyone coming across this in the future.
> +
> config ROCKCHIP_SPL_RESERVE_IRAM
> hex "Size of IRAM reserved in SPL"
> default 0x4000
> diff --git a/arch/arm/mach-rockchip/bootrom.c b/arch/arm/mach-rockchip/bootrom.c
> index 8380e4e..6f0d583 100644
> --- a/arch/arm/mach-rockchip/bootrom.c
> +++ b/arch/arm/mach-rockchip/bootrom.c
> @@ -12,5 +12,5 @@ void back_to_bootrom(void)
> #if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
> puts("Returning to boot ROM...\n");
> #endif
> - _back_to_bootrom_s();
> + _back_to_bootrom_s(0);
> }
> diff --git a/arch/arm/mach-rockchip/save_boot_param.S b/arch/arm/mach-rockchip/save_boot_param.S
> index 50fce20..f1bed0b 100644
> --- a/arch/arm/mach-rockchip/save_boot_param.S
> +++ b/arch/arm/mach-rockchip/save_boot_param.S
> @@ -7,11 +7,25 @@
>
> #include <linux/linkage.h>
>
> +#define BACK_TO_BROM_DOWNLOAD_FLAG 0xEF08A53C
This looks like it should be defined in bootrom.h
> +
> #if defined(CONFIG_ARM64)
> .globl SAVE_SP_ADDR
> SAVE_SP_ADDR:
> .quad 0
>
> +ENTRY(check_back_to_brom_dnl_flag)
> + ldr x8, =CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG
> + ldr x9, [x8]
> + ldr x0, =BACK_TO_BROM_DOWNLOAD_FLAG
> + cmp x9, x0
> + b.ne save_boot_params_ret
> + mov x9, xzr
> + str x9, [x8] /* clear flag */
> + mov x0, #1 /* indicate the bootrom to enter download mode */
> + b _back_to_bootrom_s
How does this ever get entered? If the download flag is already set prior
to this code being executed, then the BROM would certainly not even come
here?
If you just always save the boot_params and check the download flag later
from C code, then you could have this implemented in C. This will remove
the need to write two separate assembly functions (for AArch64 and
AArch32) and generally be more readable. Please revise.
> +ENDPROC(check_back_to_brom_dnl_flag)
> +
> ENTRY(save_boot_params)
> sub sp, sp, #0x60
> stp x29, x30, [sp, #0x50]
> @@ -23,14 +37,22 @@ ENTRY(save_boot_params)
> ldr x8, =SAVE_SP_ADDR
> mov x9, sp
> str x9, [x8]
> +#if CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG
> + b check_back_to_brom_dnl_flag
> +#else
> b save_boot_params_ret /* back to my caller */
> +#endif
Please avoid the #if #else #endif here. Could you simply call into a
function that handles this correctly for both cases?
However, this should fold back onto just the save_boot_params case anyway,
if you can implement the checking function in C (as suggested above).
> ENDPROC(save_boot_params)
>
> +/*
> + * x0: return value for bootrom, none-zero for bootrom download
typo: non-zero
> + * mode and zero for normal boot mode
> + */
> .globl _back_to_bootrom_s
> ENTRY(_back_to_bootrom_s)
> - ldr x0, =SAVE_SP_ADDR
> - ldr x0, [x0]
> - mov sp, x0
> + ldr x1, =SAVE_SP_ADDR
> + ldr x1, [x1]
> + mov sp, x1
> ldp x29, x30, [sp, #0x50]
> ldp x27, x28, [sp, #0x40]
> ldp x25, x26, [sp, #0x30]
> @@ -38,7 +60,6 @@ ENTRY(_back_to_bootrom_s)
> ldp x21, x22, [sp, #0x10]
> ldp x19, x20, [sp]
> add sp, sp, #0x60
> - mov x0, xzr
> ret
> ENDPROC(_back_to_bootrom_s)
> #else
> @@ -46,6 +67,18 @@ ENDPROC(_back_to_bootrom_s)
> SAVE_SP_ADDR:
> .word 0
>
> +ENTRY(check_back_to_brom_dnl_flag)
> + ldr r0, =CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG
> + ldr r1, [r0]
> + ldr r2, =BACK_TO_BROM_DOWNLOAD_FLAG
> + cmp r1, r2
> + bne save_boot_params_ret
> + mov r3, #0
> + str r3, [r0] @clear flag
> + mov r0, #1 @indicate the bootrom to enter download mode
> + b _back_to_bootrom_s
> +ENDPROC(check_back_to_brom_dnl_flag)
See above: should be possible to do in C.
> +
> /*
> * void save_boot_params
> *
> @@ -55,15 +88,21 @@ ENTRY(save_boot_params)
> push {r1-r12, lr}
> ldr r0, =SAVE_SP_ADDR
> str sp, [r0]
> - b save_boot_params_ret @ back to my caller
> +#if CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG
> + b check_back_to_brom_dnl_flag
> +#else
> + b save_boot_params_ret
> +#endif
See above.
> ENDPROC(save_boot_params)
>
> -
> +/*
> + * r0: return value for bootrom, none-zero for bootrom download
> + * mode and zero for normal boot mode
> + */
> .globl _back_to_bootrom_s
> ENTRY(_back_to_bootrom_s)
> - ldr r0, =SAVE_SP_ADDR
> - ldr sp, [r0]
> - mov r0, #0
> + ldr r1, =SAVE_SP_ADDR
> + ldr sp, [r1]
> pop {r1-r12, pc}
> ENDPROC(_back_to_bootrom_s)
> #endif
>
More information about the U-Boot
mailing list