[U-Boot] [PATCH] rockchip: add support for backing to bootrom download mode

[U-Boot] [PATCH] rockchip: add support for backing to bootrom download mode

[Andy Yan]

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.

Signed-off-by: Andy Yan <andy.yan@rock-chips.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);
 
 #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
+	help
+	  The Soc will return to bootrom download mode if this register set
+	  to BOOTROM_DOWNLOAD_FLAG.
+
 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
+
 #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
+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
 ENDPROC(save_boot_params)
 
+/*
+ * x0: 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	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)
+
 /*
  * 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
 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
-- 
2.7.4

[U-Boot] [PATCH] rockchip: add support for backing to bootrom download mode

[Kever Yang]

Hi Andy,


On 09/11/2017 02:27 PM, 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.
>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

It looks good to me,
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>
>   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);
>   
>   #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
> +	help
> +	  The Soc will return to bootrom download mode if this register set
> +	  to BOOTROM_DOWNLOAD_FLAG.
> +
>   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
> +
>   #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
> +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
>   ENDPROC(save_boot_params)
>   
> +/*
> + * x0: 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	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)
> +
>   /*
>    * 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
>   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] rockchip: add support for backing to bootrom download mode

[Philipp Tomsich]

> 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.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> Reviewed-by: Kever Yang <kever.yang@rock-chips.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(-)
> 

Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

[U-Boot] rockchip: add support for backing to bootrom download mode

[Philipp Tomsich]

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@rock-chips.com>
> Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
> Acked-by: Philipp Tomsich <philipp.tomsich@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
>

[U-Boot] rockchip: add support for backing to bootrom download mode

[Andy Yan]

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.
>
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>> Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
>> Acked-by: Philipp Tomsich <philipp.tomsich@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 .
>
>> +    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.
>
> 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.
>
>> +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.
>
> 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] rockchip: add support for backing to bootrom download mode

[Dr. Philipp Tomsich]

> On 13 Sep 2017, at 03:23, Andy Yan <andy.yan@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@rock-chips.com>
>>> Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
>>> Acked-by: Philipp Tomsich <philipp.tomsich@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

[U-Boot] rockchip: add support for backing to bootrom download mode

[Andy Yan]

Hi Philipp:


On 2017年09月13日 16:01, Dr. Philipp Tomsich wrote:
>
>> On 13 Sep 2017, at 03:23, Andy Yan <andy.yan@rock-chips.com 
>> <mailto:andy.yan@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@rock-chips.com 
>>>> <mailto:andy.yan@rock-chips.com>>
>>>> Reviewed-by: Kever Yang <kever.yang@rock-chips.com 
>>>> <mailto:kever.yang@rock-chips.com>>
>>>> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com 
>>>> <mailto:philipp.tomsich@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.


     When I hack this function first time, I indeed wrote a c 
implemented code in board_init_f on kylin-rk3036, but the usb failed 
connect to my PC
when back to bootrom, after a long time checking whit the bootrom code 
author, we found the interrupts related configurations are changed.
Then I try to save and restore the VBAR, then the bootrom download 
function works. But when I tested this function on rk3288 based board, 
it failed
again, and I found rk3288 bootrom require a different interrupt 
configuration with rk3036 after a long time dig, the interrupts vector 
base should be
  0xffff0000(other arm32 based boards are 0x0000), so the V bit of SCTLR 
should be set to 1, but the SPL startup code set it to zero.
     Then I have the idea back to bootrom download mode as early as 
possible when the download flag is set, Because no matter how many 
parameters
I saved and restored today, no one can make sure that other parameters  
will not changed by start.S in the future(maybe more properties changed 
about
the interrupt, maybe the change of mmu /caches, ), because we always 
have the chance to modify the startup code by the desire of new feature 
or the need
to workaround something for a new soc, what's more the start.S it's now 
have many #if / #else configuration, this still have risk to change the 
default configurations
which will be used in bootrom download mode. If we rely on a save and 
restore policy, this function may work well today, but may failed some 
days later just because
some one changed another configuration in start.S.
>
>>>
>>>> +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] rockchip: add support for backing to bootrom download mode

[Dr. Philipp Tomsich]

> On 13 Sep 2017, at 10:36, Andy Yan <andy.yan@rock-chips.com> wrote:
> 
>>>> 
>>>> 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.
> 
> 
>     When I hack this function first time, I indeed wrote a c implemented code in board_init_f on kylin-rk3036, but the usb failed connect to my PC
> when back to bootrom, after a long time checking whit the bootrom code author, we found the interrupts related configurations are changed.
> Then I try to save and restore the VBAR, then the bootrom download function works. But when I tested this function on rk3288 based board, it failed
> again, and I found rk3288 bootrom require a different interrupt configuration with rk3036 after a long time dig, the interrupts vector base should be
>  0xffff0000(other arm32 based boards are 0x0000), so the V bit of SCTLR should be set to 1, but the SPL startup code set it to zero.
>     Then I have the idea back to bootrom download mode as early as possible when the download flag is set, Because no matter how many parameters 
> I saved and restored today, no one can make sure that other parameters  will not changed by start.S in the future(maybe more properties changed about
> the interrupt, maybe the change of mmu /caches, ), because we always have the chance to modify the startup code by the desire of new feature or the need
> to workaround something for a new soc, what's more the start.S it's now have many #if / #else configuration, this still have risk to change the default configurations
> which will be used in bootrom download mode. If we rely on a save and restore policy, this function may work well today, but may failed some days later just because 
> some one changed another configuration in start.S. 

When looking at what happens (on armv7) between the save_boot_params
and the call to _main (which in turn just sets up SP and calls into board_init_f),
there isn’t much happening: it really is only cpu_init_cp15 and cpu_init_crit
that will be of concern.
I really wonder whether we could have a sufficient C runtime (i.e. SP valid)
available before those run.

Does your BROM always call us with the SP valid (I know that that’s the case
on the RK3399 and RK3368), so that we could run on the BROM’s stack here?

[U-Boot] rockchip: add support for backing to bootrom download mode

[Andy Yan]

Hi Philipp:


On 2017年09月13日 16:50, Dr. Philipp Tomsich wrote:
>> On 13 Sep 2017, at 10:36, Andy Yan <andy.yan@rock-chips.com> wrote:
>>
>>>>> 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.
>>
>>      When I hack this function first time, I indeed wrote a c implemented code in board_init_f on kylin-rk3036, but the usb failed connect to my PC
>> when back to bootrom, after a long time checking whit the bootrom code author, we found the interrupts related configurations are changed.
>> Then I try to save and restore the VBAR, then the bootrom download function works. But when I tested this function on rk3288 based board, it failed
>> again, and I found rk3288 bootrom require a different interrupt configuration with rk3036 after a long time dig, the interrupts vector base should be
>>   0xffff0000(other arm32 based boards are 0x0000), so the V bit of SCTLR should be set to 1, but the SPL startup code set it to zero.
>>      Then I have the idea back to bootrom download mode as early as possible when the download flag is set, Because no matter how many parameters
>> I saved and restored today, no one can make sure that other parameters  will not changed by start.S in the future(maybe more properties changed about
>> the interrupt, maybe the change of mmu /caches, ), because we always have the chance to modify the startup code by the desire of new feature or the need
>> to workaround something for a new soc, what's more the start.S it's now have many #if / #else configuration, this still have risk to change the default configurations
>> which will be used in bootrom download mode. If we rely on a save and restore policy, this function may work well today, but may failed some days later just because
>> some one changed another configuration in start.S.
> When looking at what happens (on armv7) between the save_boot_params
> and the call to _main (which in turn just sets up SP and calls into board_init_f),
> there isn’t much happening: it really is only cpu_init_cp15 and cpu_init_crit
> that will be of concern.

   The bellow codes changes the V bit and VBAR address, that are what 
cause problems on rockchip armv7 based boards.

    As for armv7, I haven't check what will cause problem.

   As bootrom is a very critical environment and very hard to debug, So 
I hope to keep it as clean as possible when we jump back to it.

/*
  * Setup vector:
  * (OMAP4 spl TEXT_BASE is not 32 byte aligned.
  * Continue to use ROM code vector only in OMAP4 spl)
  */
#if !(defined(CONFIG_OMAP44XX) && defined(CONFIG_SPL_BUILD))
         /* Set V=0 in CP15 SCTLR register - for VBAR to point to vector */
         mrc     p15, 0, r0, c1, c0, 0   @ Read CP15 SCTLR Register
         bic     r0, #CR_V               @ V = 0
         mcr     p15, 0, r0, c1, c0, 0   @ Write CP15 SCTLR Register

         /* Set vector address in CP15 VBAR register */
         ldr     r0, =_start
         mcr     p15, 0, r0, c12, c0, 0  @Set VBAR
#endif

> I really wonder whether we could have a sufficient C runtime (i.e. SP valid)
> available before those run.
>
> Does your BROM always call us with the SP valid (I know that that’s the case
> on the RK3399 and RK3368), so that we could run on the BROM’s stack here?
>
>
>
>

[U-Boot] rockchip: add support for backing to bootrom download mode

[Dr. Philipp Tomsich]

> On 13 Sep 2017, at 10:50, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:
> 
> 
>> On 13 Sep 2017, at 10:36, Andy Yan <andy.yan@rock-chips.com> wrote:
>> 
>>>>> 
>>>>> 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.
>> 
>> 
>>    When I hack this function first time, I indeed wrote a c implemented code in board_init_f on kylin-rk3036, but the usb failed connect to my PC
>> when back to bootrom, after a long time checking whit the bootrom code author, we found the interrupts related configurations are changed.
>> Then I try to save and restore the VBAR, then the bootrom download function works. But when I tested this function on rk3288 based board, it failed
>> again, and I found rk3288 bootrom require a different interrupt configuration with rk3036 after a long time dig, the interrupts vector base should be
>> 0xffff0000(other arm32 based boards are 0x0000), so the V bit of SCTLR should be set to 1, but the SPL startup code set it to zero.
>>    Then I have the idea back to bootrom download mode as early as possible when the download flag is set, Because no matter how many parameters 
>> I saved and restored today, no one can make sure that other parameters  will not changed by start.S in the future(maybe more properties changed about
>> the interrupt, maybe the change of mmu /caches, ), because we always have the chance to modify the startup code by the desire of new feature or the need
>> to workaround something for a new soc, what's more the start.S it's now have many #if / #else configuration, this still have risk to change the default configurations
>> which will be used in bootrom download mode. If we rely on a save and restore policy, this function may work well today, but may failed some days later just because 
>> some one changed another configuration in start.S. 
> 
> When looking at what happens (on armv7) between the save_boot_params
> and the call to _main (which in turn just sets up SP and calls into board_init_f),
> there isn’t much happening: it really is only cpu_init_cp15 and cpu_init_crit
> that will be of concern.
> I really wonder whether we could have a sufficient C runtime (i.e. SP valid)
> available before those run.
> 
> Does your BROM always call us with the SP valid (I know that that’s the case
> on the RK3399 and RK3368), so that we could run on the BROM’s stack here?

Just to explain my thinking here…
If we had a valid SP, we could mark our devices via Kconfig accordingly
by introducing a new config (e.g. $(SPL_TPL_)BROM_PERFORMS_C_CALL
for this) and then do something along these lines:

in start.S:
#if CONFIG_IS_ENABLED(BROM_PERFORMS_C_CALL)
	b brom_handoff_f
	.globl brom_handoff_f_ret
brom_handoff_f_ret:
#endif

and have a C file implementing the following (if setjmp/longjmp is not
yet implemented for AArch32 and AArch64, this will be a quick exercise):

/* The following is functionally identical to mach-rockchip/save_boot_param.S */
jmp_buf* brom_ctx = NULL;

enum {
	BROM_SETJMP_CONTINUE = 0
	BROM_RETURN_NORMAL = 1,
	BROM_RETURN_DNL = 2,
}

int brom_handoff_t(void)
{
	jmp_buf   saved_brom_ctx;
	int res ;

	res = setjmp(saved_brom_ctx);
	switch (res) {
	case BROM_SETJMP_CONTINUE:
		brom_ctx = &saved_brom_ctx;
		res = brom_handoff_f_ret();		/* needs a better name */
		break;

	case BROM_RETURN_NORMAL:
		res = 0;
		break;

	case BROM_RETURN_DNL:
		res = MAGIC_VALUE_TO_PASS_BACK;
		break;
	}

	return res;
}

void back_to_bootrom(void)
{
	longjmp(*brom_ctx, BROM_RETURN_NORMAL);
}