> On 13 Sep 2017, at 03:23, Andy Yan <[email protected]> wrote: > > Hi Philipp: > > > On 2017年09月13日 03:30, Philipp Tomsich wrote: >> >> >> 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? > > For armv7, the VBAR and V in SCTLR(which should be 1 for rk3288 and zero > for other arm32 platforms) > are override in arch/cpu/armv7/start.S > For armv8, I also find the VBAR related settings, but I didn't test it. > I am not sure is there any other settings in the TPL/SPL startup code that > will override the default setting in > bootrom(maybe mmu, cache and other feathers added to the startup code in the > future, this is out of control), > but the VBAR and V are indeed override on arm32 platforms. So I think back to > bootrom download mode if the > flag is set before anything changed is a efficient way.
Should we save these flags as part of the "save_boot_params + back_to_bootrom_s” processing? >> >>> >>> Signed-off-by: Andy Yan <[email protected]> >>> Reviewed-by: Kever Yang <[email protected]> >>> Acked-by: Philipp Tomsich <[email protected]> >>> --- >>> >>> 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. > Okay. >> >>> >>> #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. > > Actually this is user-configuarable, we just chose a register that not be > used by the system to pass the boot mode flag. > And we also have boards that don't want to enable this function, so they can > set the register address to 0. then we will ship > the mode check . Understood. >> >>> + 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. > > okay >> >>> + >>> 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 > > It has been move to boot-mode.h in new version. >> >>> + >>> #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? > > BROM would not check the boot mode register, it only enter bootrom > download mode if we return a non-zere value for it or it can't find a valid > image from all the storage > device. We should document this somewhere. A comment in this code might be great to let everyone know why we are checking this 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. > > We can't predict how many settings the TPL/SPL startup code changed now > and future > will affect the bootrom download function, So back to bootrom download mode > before > anything been changed is a simple way. Ok. I’d still like to have this in C. The only requirement for this will be having a valid stack-pointer, so we should be able to do this early (before the various initialisation runs). I think board_init_f() would be a suitable place. >> >>> +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? > > I have to skip the check if the REG address is zero. Good idea. Having a check for zero would match exactly how you described the handling of the Kconfig variable. Note that you should document the special meaning of zero both in a comment and in the Kconfig help text. >> >> 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). > > Please see my explanation 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
