[U-Boot] [PATCH 0/3] rockchip: back-to-bootrom: replace assembly-implementation with C-code
Recent discussions confirmed (what the code always assumed): the Rockchip BROM always enters U-Boot with the stack-pointer valid (i.e. the U-Boot startup code is running off the BROM stack).
We can thus replace the back-to-bootrom code (i.e. both the save_boot_params and back_to_bootrom implementations) using C-code based on setjmp/longjmp. The new implementation is already structured to allow an easy drop-in of Andy's changes to enter download-mode when returning to the BROM.
This entails one minor tweak to asm/system.h, which only exported the save_boot_params_ret prototype for ARMv7, but not for AArch64.
Philipp Tomsich (3): arm: make save_boot_params_ret prototype visible for AArch64 rockchip: back-to-bootrom: replace assembly-implementation with C-code rockchip: back-to-bootrom: allow passing a cmd to the bootrom
arch/arm/include/asm/arch-rockchip/bootrom.h | 30 +++++++++--- arch/arm/include/asm/system.h | 62 ++++++++++++------------- arch/arm/mach-rockchip/Makefile | 4 +- arch/arm/mach-rockchip/bootrom.c | 54 +++++++++++++++++++++- arch/arm/mach-rockchip/rk3036-board-spl.c | 2 +- arch/arm/mach-rockchip/rk3188-board-tpl.c | 2 +- arch/arm/mach-rockchip/rk322x-board-spl.c | 2 +- arch/arm/mach-rockchip/rk3288-board-spl.c | 4 +- arch/arm/mach-rockchip/rk3368-board-tpl.c | 2 +- arch/arm/mach-rockchip/rk3399-board-spl.c | 2 +- arch/arm/mach-rockchip/save_boot_param.S | 69 ---------------------------- 11 files changed, 115 insertions(+), 118 deletions(-) delete mode 100644 arch/arm/mach-rockchip/save_boot_param.S
The save_boot_params_ret() prototype (for those of us, that have a valid SP on entry and can implement save_boot_params() in C), was previously only defined for !defined(CONFIG_ARM64).
This moves the declaration to a common block to ensure the prototype is available to everyone that might need it.
Signed-off-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com ---
arch/arm/include/asm/system.h | 62 +++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h index 79bd19a..e2af296 100644 --- a/arch/arm/include/asm/system.h +++ b/arch/arm/include/asm/system.h @@ -332,37 +332,6 @@ void psci_arch_init(void);
#ifndef __ASSEMBLY__
-/** - * save_boot_params() - Save boot parameters before starting reset sequence - * - * If you provide this function it will be called immediately U-Boot starts, - * both for SPL and U-Boot proper. - * - * All registers are unchanged from U-Boot entry. No registers need be - * preserved. - * - * This is not a normal C function. There is no stack. Return by branching to - * save_boot_params_ret. - * - * void save_boot_params(u32 r0, u32 r1, u32 r2, u32 r3); - */ - -/** - * save_boot_params_ret() - Return from save_boot_params() - * - * If you provide save_boot_params(), then you should jump back to this - * function when done. Try to preserve all registers. - * - * If your implementation of save_boot_params() is in C then it is acceptable - * to simply call save_boot_params_ret() at the end of your function. Since - * there is no link register set up, you cannot just exit the function. U-Boot - * will return to the (initialised) value of lr, and likely crash/hang. - * - * If your implementation of save_boot_params() is in assembler then you - * should use 'b' or 'bx' to return to save_boot_params_ret. - */ -void save_boot_params_ret(void); - #ifdef CONFIG_ARMV7_LPAE void switch_to_hypervisor_ret(void); #endif @@ -556,6 +525,37 @@ void mmu_page_table_flush(unsigned long start, unsigned long stop);
#ifndef __ASSEMBLY__ /** + * save_boot_params() - Save boot parameters before starting reset sequence + * + * If you provide this function it will be called immediately U-Boot starts, + * both for SPL and U-Boot proper. + * + * All registers are unchanged from U-Boot entry. No registers need be + * preserved. + * + * This is not a normal C function. There is no stack. Return by branching to + * save_boot_params_ret. + * + * void save_boot_params(u32 r0, u32 r1, u32 r2, u32 r3); + */ + +/** + * save_boot_params_ret() - Return from save_boot_params() + * + * If you provide save_boot_params(), then you should jump back to this + * function when done. Try to preserve all registers. + * + * If your implementation of save_boot_params() is in C then it is acceptable + * to simply call save_boot_params_ret() at the end of your function. Since + * there is no link register set up, you cannot just exit the function. U-Boot + * will return to the (initialised) value of lr, and likely crash/hang. + * + * If your implementation of save_boot_params() is in assembler then you + * should use 'b' or 'bx' to return to save_boot_params_ret. + */ +void save_boot_params_ret(void); + +/** * Change the cache settings for a region. * * \param start start address of memory region to change
On 15 September 2017 at 06:02, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
The save_boot_params_ret() prototype (for those of us, that have a valid SP on entry and can implement save_boot_params() in C), was previously only defined for !defined(CONFIG_ARM64).
This moves the declaration to a common block to ensure the prototype is available to everyone that might need it.
Signed-off-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
arch/arm/include/asm/system.h | 62 +++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
The back-to-bootrom implementation for Rockchip has always relied on the stack-pointer being valid on entry, so there was little reason to have this as an assembly implementation.
This provides a new C-only implementation of save_boot_params and back_to_bootrom (relying on setjmp/longjmp) and removes the older assembly-only implementation.
Signed-off-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com ---
arch/arm/include/asm/arch-rockchip/bootrom.h | 27 ++++++++--- arch/arm/mach-rockchip/Makefile | 4 +- arch/arm/mach-rockchip/bootrom.c | 52 ++++++++++++++++++++- arch/arm/mach-rockchip/save_boot_param.S | 69 ---------------------------- 4 files changed, 73 insertions(+), 79 deletions(-) delete mode 100644 arch/arm/mach-rockchip/save_boot_param.S
diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h b/arch/arm/include/asm/arch-rockchip/bootrom.h index 169cc5e..2f61a33 100644 --- a/arch/arm/include/asm/arch-rockchip/bootrom.h +++ b/arch/arm/include/asm/arch-rockchip/bootrom.h @@ -1,5 +1,6 @@ /* * (C) Copyright 2017 Heiko Stuebner heiko@sntech.de + * (C) Copyright 2017 Theobroma Systems Design und Consulting GmbH * * SPDX-License-Identifier: GPL-2.0 */ @@ -14,15 +15,27 @@ extern u32 SAVE_SP_ADDR;
/** - * Hand control back to the bootrom to load another - * boot stage. + * back_to_bootrom() - return to bootrom (for TPL/SPL), passing a + * result code + * + * Transfer control back to the Rockchip BROM, restoring necessary + * register context and passing a command/result code to the BROM + * to instruct its next actions (e.g. continue boot sequence, enter + * download mode, ...). + * + * This function does not return. */ -void back_to_bootrom(void); +enum rockchip_bootrom_cmd { + /* + * These can not start at 0, as 0 has a special meaning + * for setjmp(). + */
-/** - * Assembler component for the above (do not call this directly) - */ -void _back_to_bootrom_s(void); + BROM_BOOT_NEXTSTAGE = 1, /* continue boot-sequence */ + BROM_BOOT_ENTER_DNL, /* have BROM enter download-mode */ +}; + +void back_to_bootrom(void);
/** * Boot-device identifiers as used by the BROM diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index 79e9704..f8b23ea 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -8,8 +8,8 @@ # this may have entered from ATF with the stack-pointer pointing to # inaccessible/protected memory (and the bootrom-helper assumes that # the stack-pointer is valid before switching to the U-Boot stack). -obj-spl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o save_boot_param.o -obj-tpl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o save_boot_param.o +obj-spl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o +obj-tpl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o
obj-tpl-$(CONFIG_ROCKCHIP_RK3188) += rk3188-board-tpl.o obj-tpl-$(CONFIG_ROCKCHIP_RK3368) += rk3368-board-tpl.o diff --git a/arch/arm/mach-rockchip/bootrom.c b/arch/arm/mach-rockchip/bootrom.c index 8380e4e..7b9b307 100644 --- a/arch/arm/mach-rockchip/bootrom.c +++ b/arch/arm/mach-rockchip/bootrom.c @@ -6,11 +6,61 @@
#include <common.h> #include <asm/arch/bootrom.h> +#include <asm/setjmp.h> +#include <asm/system.h> + +/* + * Force the jmp_buf to the data-section, as .bss will not be valid + * when save_boot_params is invoked. + */ +static jmp_buf brom_ctx __section(".data");
void back_to_bootrom(void) { #if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT) puts("Returning to boot ROM...\n"); #endif - _back_to_bootrom_s(); + longjmp(brom_ctx, BROM_BOOT_NEXTSTAGE); +} + +/* + * All Rockchip BROM implementations enter with a valid stack-pointer, + * so this can safely be implemented in C (providing a single + * implementation both for ARMv7 and AArch64). + */ +int save_boot_params(void) +{ + int ret = setjmp(brom_ctx); + + switch (ret) { + case 0: + /* + * This is the initial pass through this function + * (i.e. saving the context), setjmp just setup up the + * brom_ctx: transfer back into the startup-code at + * 'save_boot_params_ret' and let the compiler know + * that this will not return. + */ + save_boot_params_ret(); + while (true) + /* does not return */; + break; + + case BROM_BOOT_NEXTSTAGE: + /* + * To instruct the BROM to boot the next stage, we + * need to return 0 to it: i.e. we need to rewrite + * the return code once more. + */ + ret = 0; + break; + + default: +#if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT) + puts("FATAL: unexpected command to back_to_bootrom()\n"); +#endif + hang(); + }; + + return ret; } diff --git a/arch/arm/mach-rockchip/save_boot_param.S b/arch/arm/mach-rockchip/save_boot_param.S deleted file mode 100644 index 50fce20..0000000 --- a/arch/arm/mach-rockchip/save_boot_param.S +++ /dev/null @@ -1,69 +0,0 @@ -/* - * (C) Copyright 2016 Rockchip Electronics Co., Ltd - * (C) Copyright 2017 Theobroma Systems Design und Consulting GmbH - * - * SPDX-License-Identifier: GPL-2.0+ - */ - -#include <linux/linkage.h> - -#if defined(CONFIG_ARM64) -.globl SAVE_SP_ADDR -SAVE_SP_ADDR: - .quad 0 - -ENTRY(save_boot_params) - sub sp, sp, #0x60 - stp x29, x30, [sp, #0x50] - stp x27, x28, [sp, #0x40] - stp x25, x26, [sp, #0x30] - stp x23, x24, [sp, #0x20] - stp x21, x22, [sp, #0x10] - stp x19, x20, [sp, #0] - ldr x8, =SAVE_SP_ADDR - mov x9, sp - str x9, [x8] - b save_boot_params_ret /* back to my caller */ -ENDPROC(save_boot_params) - -.globl _back_to_bootrom_s -ENTRY(_back_to_bootrom_s) - ldr x0, =SAVE_SP_ADDR - ldr x0, [x0] - mov sp, x0 - ldp x29, x30, [sp, #0x50] - ldp x27, x28, [sp, #0x40] - ldp x25, x26, [sp, #0x30] - ldp x23, x24, [sp, #0x20] - ldp x21, x22, [sp, #0x10] - ldp x19, x20, [sp] - add sp, sp, #0x60 - mov x0, xzr - ret -ENDPROC(_back_to_bootrom_s) -#else -.globl SAVE_SP_ADDR -SAVE_SP_ADDR: - .word 0 - -/* - * void save_boot_params - * - * Save sp, lr, r1~r12 - */ -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 -ENDPROC(save_boot_params) - - -.globl _back_to_bootrom_s -ENTRY(_back_to_bootrom_s) - ldr r0, =SAVE_SP_ADDR - ldr sp, [r0] - mov r0, #0 - pop {r1-r12, pc} -ENDPROC(_back_to_bootrom_s) -#endif
Hi Philipp:
On 2017年09月15日 20:02, Philipp Tomsich wrote:
The back-to-bootrom implementation for Rockchip has always relied on the stack-pointer being valid on entry, so there was little reason to have this as an assembly implementation.
This provides a new C-only implementation of save_boot_params and back_to_bootrom (relying on setjmp/longjmp) and removes the older assembly-only implementation.
Signed-off-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
arch/arm/include/asm/arch-rockchip/bootrom.h | 27 ++++++++--- arch/arm/mach-rockchip/Makefile | 4 +- arch/arm/mach-rockchip/bootrom.c | 52 ++++++++++++++++++++- arch/arm/mach-rockchip/save_boot_param.S | 69 ---------------------------- 4 files changed, 73 insertions(+), 79 deletions(-) delete mode 100644 arch/arm/mach-rockchip/save_boot_param.S
diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h b/arch/arm/include/asm/arch-rockchip/bootrom.h index 169cc5e..2f61a33 100644 --- a/arch/arm/include/asm/arch-rockchip/bootrom.h +++ b/arch/arm/include/asm/arch-rockchip/bootrom.h @@ -1,5 +1,6 @@ /*
- (C) Copyright 2017 Heiko Stuebner heiko@sntech.de
*/
- (C) Copyright 2017 Theobroma Systems Design und Consulting GmbH
- SPDX-License-Identifier: GPL-2.0
@@ -14,15 +15,27 @@ extern u32 SAVE_SP_ADDR;
/**
- Hand control back to the bootrom to load another
- boot stage.
- back_to_bootrom() - return to bootrom (for TPL/SPL), passing a
result code
- Transfer control back to the Rockchip BROM, restoring necessary
- register context and passing a command/result code to the BROM
- to instruct its next actions (e.g. continue boot sequence, enter
- download mode, ...).
*/
- This function does not return.
-void back_to_bootrom(void); +enum rockchip_bootrom_cmd {
- /*
* These can not start at 0, as 0 has a special meaning* for setjmp().*/-/**
- Assembler component for the above (do not call this directly)
- */
-void _back_to_bootrom_s(void);
- BROM_BOOT_NEXTSTAGE = 1, /* continue boot-sequence */
- BROM_BOOT_ENTER_DNL, /* have BROM enter download-mode */
+};
+void back_to_bootrom(void);
/**
- Boot-device identifiers as used by the BROM
diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index 79e9704..f8b23ea 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -8,8 +8,8 @@ # this may have entered from ATF with the stack-pointer pointing to # inaccessible/protected memory (and the bootrom-helper assumes that # the stack-pointer is valid before switching to the U-Boot stack). -obj-spl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o save_boot_param.o -obj-tpl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o save_boot_param.o +obj-spl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o +obj-tpl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o
obj-tpl-$(CONFIG_ROCKCHIP_RK3188) += rk3188-board-tpl.o obj-tpl-$(CONFIG_ROCKCHIP_RK3368) += rk3368-board-tpl.o diff --git a/arch/arm/mach-rockchip/bootrom.c b/arch/arm/mach-rockchip/bootrom.c index 8380e4e..7b9b307 100644 --- a/arch/arm/mach-rockchip/bootrom.c +++ b/arch/arm/mach-rockchip/bootrom.c @@ -6,11 +6,61 @@
#include <common.h> #include <asm/arch/bootrom.h> +#include <asm/setjmp.h> +#include <asm/system.h>
+/*
- Force the jmp_buf to the data-section, as .bss will not be valid
- when save_boot_params is invoked.
- */
+static jmp_buf brom_ctx __section(".data");
void back_to_bootrom(void) { #if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT) puts("Returning to boot ROM...\n"); #endif
- _back_to_bootrom_s();
- longjmp(brom_ctx, BROM_BOOT_NEXTSTAGE);
+}
+/*
- All Rockchip BROM implementations enter with a valid stack-pointer,
- so this can safely be implemented in C (providing a single
- implementation both for ARMv7 and AArch64).
- */
+int save_boot_params(void) +{
- int ret = setjmp(brom_ctx);
- switch (ret) {
- case 0:
/** This is the initial pass through this function* (i.e. saving the context), setjmp just setup up the* brom_ctx: transfer back into the startup-code at* 'save_boot_params_ret' and let the compiler know* that this will not return.*/save_boot_params_ret();
This function works fine on ARM64 platform, But I got problems on ARMv7. When trace the code flow with DS5 I found the core switch to thumb state when jump to save_boot_params_ret[0], but this code can't only execute in arm state as thumb instruction can't access cpsr register. I don't know how to make sure the core in arm state when jump to save_boot_params_ret.
save_boot_params_ret: #ifdef CONFIG_ARMV7_LPAE /* * check for Hypervisor support */ mrc p15, 0, r0, c0, c1, 1 @ read ID_PFR1 and r0, r0, #CPUID_ARM_VIRT_MASK @ mask virtualization bits cmp r0, #(1 << CPUID_ARM_VIRT_SHIFT) beq switch_to_hypervisor switch_to_hypervisor_ret: #endif /* * disable interrupts (FIQ and IRQ), also set the cpu to SVC32 mode, * except if in HYP mode already */ mrs r0, cpsr and r1, r0, #0x1f @ mask mode bits teq r1, #0x1a @ test for HYP mode bicne r0, r0, #0x1f @ clear all mode bits orrne r0, r0, #0x13 @ set SVC mode orr r0, r0, #0xc0 @ disable FIQ and IRQ msr cpsr,r0
while (true)/* does not return */;break;- case BROM_BOOT_NEXTSTAGE:
/** To instruct the BROM to boot the next stage, we* need to return 0 to it: i.e. we need to rewrite* the return code once more.*/ret = 0;break;- default:
+#if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
puts("FATAL: unexpected command to back_to_bootrom()\n");+#endif
hang();- };
- return ret; }
diff --git a/arch/arm/mach-rockchip/save_boot_param.S b/arch/arm/mach-rockchip/save_boot_param.S deleted file mode 100644 index 50fce20..0000000 --- a/arch/arm/mach-rockchip/save_boot_param.S +++ /dev/null @@ -1,69 +0,0 @@ -/*
- (C) Copyright 2016 Rockchip Electronics Co., Ltd
- (C) Copyright 2017 Theobroma Systems Design und Consulting GmbH
- SPDX-License-Identifier: GPL-2.0+
- */
-#include <linux/linkage.h>
-#if defined(CONFIG_ARM64) -.globl SAVE_SP_ADDR -SAVE_SP_ADDR:
- .quad 0
-ENTRY(save_boot_params)
- sub sp, sp, #0x60
- stp x29, x30, [sp, #0x50]
- stp x27, x28, [sp, #0x40]
- stp x25, x26, [sp, #0x30]
- stp x23, x24, [sp, #0x20]
- stp x21, x22, [sp, #0x10]
- stp x19, x20, [sp, #0]
- ldr x8, =SAVE_SP_ADDR
- mov x9, sp
- str x9, [x8]
- b save_boot_params_ret /* back to my caller */
-ENDPROC(save_boot_params)
-.globl _back_to_bootrom_s -ENTRY(_back_to_bootrom_s)
- ldr x0, =SAVE_SP_ADDR
- ldr x0, [x0]
- mov sp, x0
- ldp x29, x30, [sp, #0x50]
- ldp x27, x28, [sp, #0x40]
- ldp x25, x26, [sp, #0x30]
- ldp x23, x24, [sp, #0x20]
- ldp x21, x22, [sp, #0x10]
- ldp x19, x20, [sp]
- add sp, sp, #0x60
- mov x0, xzr
- ret
-ENDPROC(_back_to_bootrom_s) -#else -.globl SAVE_SP_ADDR -SAVE_SP_ADDR:
- .word 0
-/*
- void save_boot_params
- Save sp, lr, r1~r12
- */
-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
-ENDPROC(save_boot_params)
-.globl _back_to_bootrom_s -ENTRY(_back_to_bootrom_s)
- ldr r0, =SAVE_SP_ADDR
- ldr sp, [r0]
- mov r0, #0
- pop {r1-r12, pc}
-ENDPROC(_back_to_bootrom_s) -#endif
On 18 Sep 2017, at 10:17, Andy Yan andy.yan@rock-chips.com wrote:
Hi Philipp:
On 2017年09月15日 20:02, Philipp Tomsich wrote:
The back-to-bootrom implementation for Rockchip has always relied on the stack-pointer being valid on entry, so there was little reason to have this as an assembly implementation.
This provides a new C-only implementation of save_boot_params and back_to_bootrom (relying on setjmp/longjmp) and removes the older assembly-only implementation.
Signed-off-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
arch/arm/include/asm/arch-rockchip/bootrom.h | 27 ++++++++--- arch/arm/mach-rockchip/Makefile | 4 +- arch/arm/mach-rockchip/bootrom.c | 52 ++++++++++++++++++++- arch/arm/mach-rockchip/save_boot_param.S | 69 ---------------------------- 4 files changed, 73 insertions(+), 79 deletions(-) delete mode 100644 arch/arm/mach-rockchip/save_boot_param.S
diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h b/arch/arm/include/asm/arch-rockchip/bootrom.h index 169cc5e..2f61a33 100644 --- a/arch/arm/include/asm/arch-rockchip/bootrom.h +++ b/arch/arm/include/asm/arch-rockchip/bootrom.h @@ -1,5 +1,6 @@ /*
- (C) Copyright 2017 Heiko Stuebner heiko@sntech.de
*/
- (C) Copyright 2017 Theobroma Systems Design und Consulting GmbH
- SPDX-License-Identifier: GPL-2.0
@@ -14,15 +15,27 @@ extern u32 SAVE_SP_ADDR; /**
- Hand control back to the bootrom to load another
- boot stage.
- back_to_bootrom() - return to bootrom (for TPL/SPL), passing a
result code
- Transfer control back to the Rockchip BROM, restoring necessary
- register context and passing a command/result code to the BROM
- to instruct its next actions (e.g. continue boot sequence, enter
- download mode, ...).
*/
- This function does not return.
-void back_to_bootrom(void); +enum rockchip_bootrom_cmd {
- /*
* These can not start at 0, as 0 has a special meaning* for setjmp().*/-/**
- Assembler component for the above (do not call this directly)
- */
-void _back_to_bootrom_s(void);
- BROM_BOOT_NEXTSTAGE = 1, /* continue boot-sequence */
- BROM_BOOT_ENTER_DNL, /* have BROM enter download-mode */
+};
+void back_to_bootrom(void); /**
- Boot-device identifiers as used by the BROM
diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index 79e9704..f8b23ea 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -8,8 +8,8 @@ # this may have entered from ATF with the stack-pointer pointing to # inaccessible/protected memory (and the bootrom-helper assumes that # the stack-pointer is valid before switching to the U-Boot stack). -obj-spl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o save_boot_param.o -obj-tpl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o save_boot_param.o +obj-spl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o +obj-tpl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o obj-tpl-$(CONFIG_ROCKCHIP_RK3188) += rk3188-board-tpl.o obj-tpl-$(CONFIG_ROCKCHIP_RK3368) += rk3368-board-tpl.o diff --git a/arch/arm/mach-rockchip/bootrom.c b/arch/arm/mach-rockchip/bootrom.c index 8380e4e..7b9b307 100644 --- a/arch/arm/mach-rockchip/bootrom.c +++ b/arch/arm/mach-rockchip/bootrom.c @@ -6,11 +6,61 @@ #include <common.h> #include <asm/arch/bootrom.h> +#include <asm/setjmp.h> +#include <asm/system.h>
+/*
- Force the jmp_buf to the data-section, as .bss will not be valid
- when save_boot_params is invoked.
- */
+static jmp_buf brom_ctx __section(".data"); void back_to_bootrom(void) { #if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT) puts("Returning to boot ROM...\n"); #endif
- _back_to_bootrom_s();
- longjmp(brom_ctx, BROM_BOOT_NEXTSTAGE);
+}
+/*
- All Rockchip BROM implementations enter with a valid stack-pointer,
- so this can safely be implemented in C (providing a single
- implementation both for ARMv7 and AArch64).
- */
+int save_boot_params(void) +{
- int ret = setjmp(brom_ctx);
- switch (ret) {
- case 0:
/** This is the initial pass through this function* (i.e. saving the context), setjmp just setup up the* brom_ctx: transfer back into the startup-code at* 'save_boot_params_ret' and let the compiler know* that this will not return.*/save_boot_params_ret();This function works fine on ARM64 platform, But I got problems on ARMv7. When trace the code flow with DS5 I found the core switch to thumb state when jump to save_boot_params_ret[0], but this code can't only execute in arm state as thumb instruction can't access cpsr register. I don't know how to make sure the core in arm state when jump to save_boot_params_ret.
Thanks for the detailed diagnosis. I’ll take a look on how to best force the compiler to emit this function in ARM state: I hope to be able to use a target(arm) function attribute (see https://gcc.gnu.org/onlinedocs/gcc/ARM-Function-Attributes.html)
save_boot_params_ret: #ifdef CONFIG_ARMV7_LPAE /*
- check for Hypervisor support
*/ mrc p15, 0, r0, c0, c1, 1 @ read ID_PFR1 and r0, r0, #CPUID_ARM_VIRT_MASK @ mask virtualization bits cmp r0, #(1 << CPUID_ARM_VIRT_SHIFT) beq switch_to_hypervisor switch_to_hypervisor_ret: #endif /* * disable interrupts (FIQ and IRQ), also set the cpu to SVC32 mode, * except if in HYP mode already */ mrs r0, cpsr and r1, r0, #0x1f @ mask mode bits teq r1, #0x1a @ test for HYP mode bicne r0, r0, #0x1f @ clear all mode bits orrne r0, r0, #0x13 @ set SVC mode orr r0, r0, #0xc0 @ disable FIQ and IRQ msr cpsr,r0
while (true)/* does not return */;break;- case BROM_BOOT_NEXTSTAGE:
/** To instruct the BROM to boot the next stage, we* need to return 0 to it: i.e. we need to rewrite* the return code once more.*/ret = 0;break;- default:
+#if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)
puts("FATAL: unexpected command to back_to_bootrom()\n");+#endif
hang();- };
- return ret;
} diff --git a/arch/arm/mach-rockchip/save_boot_param.S b/arch/arm/mach-rockchip/save_boot_param.S deleted file mode 100644 index 50fce20..0000000 --- a/arch/arm/mach-rockchip/save_boot_param.S +++ /dev/null @@ -1,69 +0,0 @@ -/*
- (C) Copyright 2016 Rockchip Electronics Co., Ltd
- (C) Copyright 2017 Theobroma Systems Design und Consulting GmbH
- SPDX-License-Identifier: GPL-2.0+
- */
-#include <linux/linkage.h>
-#if defined(CONFIG_ARM64) -.globl SAVE_SP_ADDR -SAVE_SP_ADDR:
- .quad 0
-ENTRY(save_boot_params)
- sub sp, sp, #0x60
- stp x29, x30, [sp, #0x50]
- stp x27, x28, [sp, #0x40]
- stp x25, x26, [sp, #0x30]
- stp x23, x24, [sp, #0x20]
- stp x21, x22, [sp, #0x10]
- stp x19, x20, [sp, #0]
- ldr x8, =SAVE_SP_ADDR
- mov x9, sp
- str x9, [x8]
- b save_boot_params_ret /* back to my caller */
-ENDPROC(save_boot_params)
-.globl _back_to_bootrom_s -ENTRY(_back_to_bootrom_s)
- ldr x0, =SAVE_SP_ADDR
- ldr x0, [x0]
- mov sp, x0
- ldp x29, x30, [sp, #0x50]
- ldp x27, x28, [sp, #0x40]
- ldp x25, x26, [sp, #0x30]
- ldp x23, x24, [sp, #0x20]
- ldp x21, x22, [sp, #0x10]
- ldp x19, x20, [sp]
- add sp, sp, #0x60
- mov x0, xzr
- ret
-ENDPROC(_back_to_bootrom_s) -#else -.globl SAVE_SP_ADDR -SAVE_SP_ADDR:
- .word 0
-/*
- void save_boot_params
- Save sp, lr, r1~r12
- */
-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
-ENDPROC(save_boot_params)
-.globl _back_to_bootrom_s -ENTRY(_back_to_bootrom_s)
- ldr r0, =SAVE_SP_ADDR
- ldr sp, [r0]
- mov r0, #0
- pop {r1-r12, pc}
-ENDPROC(_back_to_bootrom_s) -#endif
Andy,
This function works fine on ARM64 platform, But I got problems on ARMv7. When trace the code flow with DS5 I found the core switch to thumb state when jump to save_boot_params_ret[0], but this code can't only execute in arm state as thumb instruction can't access cpsr register. I don't know how to make sure the core in arm state when jump to save_boot_params_ret.
save_boot_params_ret: #ifdef CONFIG_ARMV7_LPAE /*
- check for Hypervisor support
*/ mrc p15, 0, r0, c0, c1, 1 @ read ID_PFR1 and r0, r0, #CPUID_ARM_VIRT_MASK @ mask virtualization bits cmp r0, #(1 << CPUID_ARM_VIRT_SHIFT) beq switch_to_hypervisor switch_to_hypervisor_ret: #endif /* * disable interrupts (FIQ and IRQ), also set the cpu to SVC32 mode, * except if in HYP mode already */ mrs r0, cpsr and r1, r0, #0x1f @ mask mode bits teq r1, #0x1a @ test for HYP mode bicne r0, r0, #0x1f @ clear all mode bits orrne r0, r0, #0x13 @ set SVC mode orr r0, r0, #0xc0 @ disable FIQ and IRQ msr cpsr,r0
Thanks for tracing this to the missing T32->A32 transition on the return path. I update the series and things should now work better (I hope): https://patchwork.ozlabs.org/user/todo/uboot/?series=3697
I also had to touch the RK3188 support (and don’t have a board to test), so any testing for the RK3188 change will also be appreciated.
—Philipp.
The BROM supports forcing it to enter download-mode, if an appropriate result/cmd-word is returned to it. There already is a series to support this in review, so this prepares the (newly C-version) of the back-to-bootrom code to accept a cmd to passed on to the BROM.
All the existing call-sites are adjusted to match the changed function signature.
Signed-off-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com ---
arch/arm/include/asm/arch-rockchip/bootrom.h | 5 ++++- arch/arm/mach-rockchip/bootrom.c | 4 ++-- arch/arm/mach-rockchip/rk3036-board-spl.c | 2 +- arch/arm/mach-rockchip/rk3188-board-tpl.c | 2 +- arch/arm/mach-rockchip/rk322x-board-spl.c | 2 +- arch/arm/mach-rockchip/rk3288-board-spl.c | 4 ++-- arch/arm/mach-rockchip/rk3368-board-tpl.c | 2 +- arch/arm/mach-rockchip/rk3399-board-spl.c | 2 +- 8 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h b/arch/arm/include/asm/arch-rockchip/bootrom.h index 2f61a33..103b799 100644 --- a/arch/arm/include/asm/arch-rockchip/bootrom.h +++ b/arch/arm/include/asm/arch-rockchip/bootrom.h @@ -24,6 +24,9 @@ extern u32 SAVE_SP_ADDR; * download mode, ...). * * This function does not return. + * + * @brom_cmd: indicates how the bootrom should continue the boot + * sequence (e.g. load the next stage) */ enum rockchip_bootrom_cmd { /* @@ -35,7 +38,7 @@ enum rockchip_bootrom_cmd { BROM_BOOT_ENTER_DNL, /* have BROM enter download-mode */ };
-void back_to_bootrom(void); +void back_to_bootrom(enum rockchip_bootrom_cmd brom_cmd);
/** * Boot-device identifiers as used by the BROM diff --git a/arch/arm/mach-rockchip/bootrom.c b/arch/arm/mach-rockchip/bootrom.c index 7b9b307..e369fdc 100644 --- a/arch/arm/mach-rockchip/bootrom.c +++ b/arch/arm/mach-rockchip/bootrom.c @@ -15,12 +15,12 @@ */ static jmp_buf brom_ctx __section(".data");
-void back_to_bootrom(void) +void back_to_bootrom(enum rockchip_bootrom_cmd brom_cmd) { #if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT) puts("Returning to boot ROM...\n"); #endif - longjmp(brom_ctx, BROM_BOOT_NEXTSTAGE); + longjmp(brom_ctx, brom_cmd); }
/* diff --git a/arch/arm/mach-rockchip/rk3036-board-spl.c b/arch/arm/mach-rockchip/rk3036-board-spl.c index 9458201..550e3a1 100644 --- a/arch/arm/mach-rockchip/rk3036-board-spl.c +++ b/arch/arm/mach-rockchip/rk3036-board-spl.c @@ -40,7 +40,7 @@ void board_init_f(ulong dummy) sdram_init();
/* return to maskrom */ - back_to_bootrom(); + back_to_bootrom(BROM_BOOT_NEXTSTAGE); }
/* Place Holders */ diff --git a/arch/arm/mach-rockchip/rk3188-board-tpl.c b/arch/arm/mach-rockchip/rk3188-board-tpl.c index b458ef6..5c081ca 100644 --- a/arch/arm/mach-rockchip/rk3188-board-tpl.c +++ b/arch/arm/mach-rockchip/rk3188-board-tpl.c @@ -74,7 +74,7 @@ void board_init_f(ulong dummy) * really early on. */
- back_to_bootrom(); + back_to_bootrom(BROM_BOOT_NEXTSTAGE); } else { /* * TPL part of the loader should now wait for us diff --git a/arch/arm/mach-rockchip/rk322x-board-spl.c b/arch/arm/mach-rockchip/rk322x-board-spl.c index 4ddb8ba..35f4f97 100644 --- a/arch/arm/mach-rockchip/rk322x-board-spl.c +++ b/arch/arm/mach-rockchip/rk322x-board-spl.c @@ -76,6 +76,6 @@ void board_init_f(ulong dummy) /* Disable the ddr secure region setting to make it non-secure */ rk_clrreg(SGRF_DDR_CON0, 0x4000); #if defined(CONFIG_ROCKCHIP_SPL_BACK_TO_BROM) && !defined(CONFIG_SPL_BOARD_INIT) - back_to_bootrom(); + back_to_bootrom(BROM_BOOT_NEXTSTAGE); #endif } diff --git a/arch/arm/mach-rockchip/rk3288-board-spl.c b/arch/arm/mach-rockchip/rk3288-board-spl.c index 6b7bf85..6fa4909 100644 --- a/arch/arm/mach-rockchip/rk3288-board-spl.c +++ b/arch/arm/mach-rockchip/rk3288-board-spl.c @@ -250,7 +250,7 @@ void board_init_f(ulong dummy) return; } #if CONFIG_IS_ENABLED(ROCKCHIP_BACK_TO_BROM) && !defined(CONFIG_SPL_BOARD_INIT) - back_to_bootrom(); + back_to_bootrom(BROM_BOOT_NEXTSTAGE); #endif }
@@ -317,7 +317,7 @@ void spl_board_init(void)
preloader_console_init(); #if CONFIG_IS_ENABLED(ROCKCHIP_BACK_TO_BROM) - back_to_bootrom(); + back_to_bootrom(BROM_BOOT_NEXTSTAGE); #endif return; err: diff --git a/arch/arm/mach-rockchip/rk3368-board-tpl.c b/arch/arm/mach-rockchip/rk3368-board-tpl.c index b3e6ffa..60d5aea 100644 --- a/arch/arm/mach-rockchip/rk3368-board-tpl.c +++ b/arch/arm/mach-rockchip/rk3368-board-tpl.c @@ -148,7 +148,7 @@ void board_init_f(ulong dummy)
void board_return_to_bootrom(void) { - back_to_bootrom(); + back_to_bootrom(BROM_BOOT_NEXTSTAGE); }
u32 spl_boot_device(void) diff --git a/arch/arm/mach-rockchip/rk3399-board-spl.c b/arch/arm/mach-rockchip/rk3399-board-spl.c index 9c20f56..b96903e 100644 --- a/arch/arm/mach-rockchip/rk3399-board-spl.c +++ b/arch/arm/mach-rockchip/rk3399-board-spl.c @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR;
void board_return_to_bootrom(void) { - back_to_bootrom(); + back_to_bootrom(BROM_BOOT_NEXTSTAGE); }
static const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
On 15 September 2017 at 06:02, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
The BROM supports forcing it to enter download-mode, if an appropriate result/cmd-word is returned to it. There already is a series to support this in review, so this prepares the (newly C-version) of the back-to-bootrom code to accept a cmd to passed on to the BROM.
All the existing call-sites are adjusted to match the changed function signature.
Signed-off-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
arch/arm/include/asm/arch-rockchip/bootrom.h | 5 ++++- arch/arm/mach-rockchip/bootrom.c | 4 ++-- arch/arm/mach-rockchip/rk3036-board-spl.c | 2 +- arch/arm/mach-rockchip/rk3188-board-tpl.c | 2 +- arch/arm/mach-rockchip/rk322x-board-spl.c | 2 +- arch/arm/mach-rockchip/rk3288-board-spl.c | 4 ++-- arch/arm/mach-rockchip/rk3368-board-tpl.c | 2 +- arch/arm/mach-rockchip/rk3399-board-spl.c | 2 +- 8 files changed, 13 insertions(+), 10 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
participants (4)
-
Andy Yan -
Dr. Philipp Tomsich -
Philipp Tomsich
-
Simon Glass