diff mbox series

[U-Boot,2/3] rockchip: back-to-bootrom: replace assembly-implementation with C-code

Message ID 1505476946-64991-3-git-send-email-philipp.tomsich@theobroma-systems.com
State Superseded
Delegated to: Philipp Tomsich
Headers show
Series rockchip: back-to-bootrom: replace assembly-implementation with C-code | expand

Commit Message

Philipp Tomsich Sept. 15, 2017, 12:02 p.m. UTC
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

Comments

Andy Yan Sept. 18, 2017, 8:17 a.m. UTC | #1
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
Philipp Tomsich Sept. 18, 2017, 9 a.m. UTC | #2
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
Philipp Tomsich Sept. 18, 2017, 6:49 p.m. UTC | #3
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.
diff mbox series

Patch

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