[U-Boot] rockchip: add support for backing to bootrom download mode
Dr. Philipp Tomsich
philipp.tomsich at theobroma-systems.com
Wed Sep 13 08:01:59 UTC 2017
> On 13 Sep 2017, at 03:23, Andy Yan <andy.yan at rock-chips.com> 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 <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.
> 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
More information about the U-Boot
mailing list