[U-Boot] [PATCH v2 0/5] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[U-Boot] [PATCH v2 0/5] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[Philipp Tomsich]

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.

For v2, we force bootrom.o to alway be emitted as A32 (not T32), so we
can safely call save_boot_params_ret().

It also turned out that I had not caught the RK3188 in my earlier
series... and my luck being what it is, the RK3188 needed some extra
handholding to adapt to the new regime: instead of passing the context
address (for returning to the BROM) from the TPL to the SPL, the SPL
now returns to the TPL and the TPL then returns to the BROM.

Changes in v2:
- [added in v2] chain back_to_bootrom calls for SPL, first returning
  to the TPL (using the same mechanism) and further calling through
  to the BROM from the TPL by invoking back_to_bootrom again
- adapt the RK3188 spl support file (that I had originally missed)

Philipp Tomsich (5):
  arm: make save_boot_params_ret prototype visible for AArch64
  rockchip: back-to-bootrom: replace assembly-implementation with C-code
  rockchip: back-to-bootrom: rk3188: chain from SPL via TPL to the BROM
  rockchip: back-to-bootrom: allow passing a cmd to the bootrom
  rockchip: back-to-bootrom: do not compile bootrom.o in thumb mode

 arch/arm/include/asm/arch-rockchip/bootrom.h | 30 +++++++++---
 arch/arm/include/asm/system.h                | 62 ++++++++++++-------------
 arch/arm/mach-rockchip/Makefile              | 10 +++-
 arch/arm/mach-rockchip/bootrom.c             | 54 +++++++++++++++++++++-
 arch/arm/mach-rockchip/rk3036-board-spl.c    |  2 +-
 arch/arm/mach-rockchip/rk3188-board-spl.c    | 14 +-----
 arch/arm/mach-rockchip/rk3188-board-tpl.c    | 19 ++++----
 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 ----------------------------
 12 files changed, 133 insertions(+), 137 deletions(-)
 delete mode 100644 arch/arm/mach-rockchip/save_boot_param.S

-- 
2.1.4

[U-Boot] [PATCH v2 1/5] arm: make save_boot_params_ret prototype visible for AArch64

[Philipp Tomsich]

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>
---

Changes in v2: None

 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
-- 
2.1.4

[U-Boot] [PATCH v2 2/5] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[Philipp Tomsich]

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>
---

Changes in v2: None

 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
-- 
2.1.4

[U-Boot] [PATCH v2 3/5] rockchip: back-to-bootrom: rk3188: chain from SPL via TPL to the BROM

[Philipp Tomsich]

The RK3188 implementation previously passed the address of the stack
frame created during save_boot_params via pmu->os_reg[2]. This was not
strictly necessary, as the save_boot_params() function was called
twice (first: for TPL, saving the context for the BROM; next: for SPL,
saving the context for the TPL) and a back-to-bootrom from the SPL
would thus return to TPL.

To simplify things, we now assume that the state of the TPL is not
corrupted during SPL (the binaries are non-overlapping) and that the
SPL can safely return to TPL using the back-to-bootrom mechanism.
Consequently, the TPL should expect the SPL to return control and then
further return to the actual bootrom by performing another
back-to-bootrom transition.

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

Changes in v2:
- [added in v2] chain back_to_bootrom calls for SPL, first returning
  to the TPL (using the same mechanism) and the to the BROM from the
  TPL

 arch/arm/mach-rockchip/rk3188-board-spl.c | 10 ----------
 arch/arm/mach-rockchip/rk3188-board-tpl.c | 17 ++++++++++-------
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-rockchip/rk3188-board-spl.c b/arch/arm/mach-rockchip/rk3188-board-spl.c
index d3866bf..05d4ae6 100644
--- a/arch/arm/mach-rockchip/rk3188-board-spl.c
+++ b/arch/arm/mach-rockchip/rk3188-board-spl.c
@@ -101,7 +101,6 @@ static int setup_arm_clock(void)
 void board_init_f(ulong dummy)
 {
 	struct udevice *pinctrl, *dev;
-	struct rk3188_pmu *pmu;
 	int ret;
 
 	/* Example code showing how to enable the debug UART on RK3188 */
@@ -145,15 +144,6 @@ void board_init_f(ulong dummy)
 		return;
 	}
 
-	/*
-	 * Recover the bootrom's stackpointer.
-	 * For whatever reason needs to run after rockchip_get_clk.
-	 */
-	pmu = syscon_get_first_range(ROCKCHIP_SYSCON_PMU);
-	if (IS_ERR(pmu))
-		error("pmu syscon returned %ld\n", PTR_ERR(pmu));
-	SAVE_SP_ADDR = readl(&pmu->sys_reg[2]);
-
 	ret = uclass_get_device(UCLASS_PINCTRL, 0, &pinctrl);
 	if (ret) {
 		debug("Pinctrl init failed: %d\n", ret);
diff --git a/arch/arm/mach-rockchip/rk3188-board-tpl.c b/arch/arm/mach-rockchip/rk3188-board-tpl.c
index b458ef6..c714278 100644
--- a/arch/arm/mach-rockchip/rk3188-board-tpl.c
+++ b/arch/arm/mach-rockchip/rk3188-board-tpl.c
@@ -21,15 +21,16 @@ static int rk3188_num_entries __attribute__ ((section(".data")));
 
 static void jump_to_spl(void)
 {
-	typedef void __noreturn (*image_entry_noargs_t)(void);
+	typedef void (*image_entry_noargs_t)(void);
 
-	struct rk3188_pmu * const pmu = (void *)PMU_BASE;
 	image_entry_noargs_t tpl_entry =
 		(image_entry_noargs_t)(unsigned long)SPL_ENTRY;
 
-	/* Store the SAVE_SP_ADDR in a location shared with SPL. */
-	writel(SAVE_SP_ADDR, &pmu->sys_reg[2]);
 	tpl_entry();
+	/*
+	 * If the SPL stage triggers a 'return to bootrom', it will
+	 * return to here.
+	 */
 }
 
 void board_init_f(ulong dummy)
@@ -77,10 +78,12 @@ void board_init_f(ulong dummy)
 		back_to_bootrom();
 	} else {
 		/*
-		 * TPL part of the loader should now wait for us
-		 * at offset 0xC00 in the sram. Should never return
-		 * from there.
+		 * SPL part of the loader should now wait for us at
+		 * offset 0xC00 in the sram. If the SPL returns to us,
+		 * we should in turn return to the BROM (i.e. chain
+		 * through).
 		 */
 		jump_to_spl();
+		back_to_bootrom();
 	}
 }
-- 
2.1.4

[U-Boot] [PATCH v2 3/5] rockchip: back-to-bootrom: rk3188: chain from SPL via TPL to the BROM

[Simon Glass]

On 18 September 2017 at 12:18, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> The RK3188 implementation previously passed the address of the stack
> frame created during save_boot_params via pmu->os_reg[2]. This was not
> strictly necessary, as the save_boot_params() function was called
> twice (first: for TPL, saving the context for the BROM; next: for SPL,
> saving the context for the TPL) and a back-to-bootrom from the SPL
> would thus return to TPL.
>
> To simplify things, we now assume that the state of the TPL is not
> corrupted during SPL (the binaries are non-overlapping) and that the
> SPL can safely return to TPL using the back-to-bootrom mechanism.
> Consequently, the TPL should expect the SPL to return control and then
> further return to the actual bootrom by performing another
> back-to-bootrom transition.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
>
> Changes in v2:
> - [added in v2] chain back_to_bootrom calls for SPL, first returning
>   to the TPL (using the same mechanism) and the to the BROM from the
>   TPL
>
>  arch/arm/mach-rockchip/rk3188-board-spl.c | 10 ----------
>  arch/arm/mach-rockchip/rk3188-board-tpl.c | 17 ++++++++++-------
>  2 files changed, 10 insertions(+), 17 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2 4/5] rockchip: back-to-bootrom: allow passing a cmd to the bootrom

[Philipp Tomsich]

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>

---

Changes in v2:
- adapt the RK3188 spl support file (that I had originally missed)

 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-spl.c    | 4 ++--
 arch/arm/mach-rockchip/rk3188-board-tpl.c    | 4 ++--
 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 +-
 9 files changed, 16 insertions(+), 13 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-spl.c b/arch/arm/mach-rockchip/rk3188-board-spl.c
index 05d4ae6..8e3b8ae 100644
--- a/arch/arm/mach-rockchip/rk3188-board-spl.c
+++ b/arch/arm/mach-rockchip/rk3188-board-spl.c
@@ -158,7 +158,7 @@ void board_init_f(ulong dummy)
 
 	setup_arm_clock();
 #if CONFIG_IS_ENABLED(ROCKCHIP_BACK_TO_BROM) && !defined(CONFIG_SPL_BOARD_INIT)
-	back_to_bootrom();
+	back_to_bootrom(BROM_BOOT_NEXTSTAGE);
 #endif
 }
 
@@ -219,7 +219,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;
 
diff --git a/arch/arm/mach-rockchip/rk3188-board-tpl.c b/arch/arm/mach-rockchip/rk3188-board-tpl.c
index c714278..dc5cbf3 100644
--- a/arch/arm/mach-rockchip/rk3188-board-tpl.c
+++ b/arch/arm/mach-rockchip/rk3188-board-tpl.c
@@ -75,7 +75,7 @@ void board_init_f(ulong dummy)
 		 * really early on.
 		 */
 
-		back_to_bootrom();
+		back_to_bootrom(BROM_BOOT_NEXTSTAGE);
 	} else {
 		/*
 		 * SPL part of the loader should now wait for us at
@@ -84,6 +84,6 @@ void board_init_f(ulong dummy)
 		 * through).
 		 */
 		jump_to_spl();
-		back_to_bootrom();
+		back_to_bootrom(BROM_BOOT_NEXTSTAGE);
 	}
 }
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] = {
-- 
2.1.4

[U-Boot] [PATCH v2 4/5] rockchip: back-to-bootrom: allow passing a cmd to the bootrom

[Simon Glass]

On 18 September 2017 at 12:18, 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>
>
> ---
>
> Changes in v2:
> - adapt the RK3188 spl support file (that I had originally missed)
>
>  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-spl.c    | 4 ++--
>  arch/arm/mach-rockchip/rk3188-board-tpl.c    | 4 ++--
>  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 +-
>  9 files changed, 16 insertions(+), 13 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2 5/5] rockchip: back-to-bootrom: do not compile bootrom.o in thumb mode

[Philipp Tomsich]

With start.o being compiled to A32 for ARMv7 (and no Thumb mode), we
need to ensure that the call to save_boot_params_ret either happens
from ARM mode or uses an interwork branch.

To keep things simple, we force bootrom.o to always be A32 code by
setting specific CFLAGS for this compilation unit.  Note that marking
save_boot_params_ret with the 'target(arm)'-function attribute did not
generate an interwork branch when calling save_boot_params_ret.

Reported-by: Andy Yan <andy.yan@rock-chips.com>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

---

Changes in v2: None

 arch/arm/mach-rockchip/Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile
index f8b23ea..405b30c 100644
--- a/arch/arm/mach-rockchip/Makefile
+++ b/arch/arm/mach-rockchip/Makefile
@@ -4,6 +4,12 @@
 # SPDX-License-Identifier:	GPL-2.0+
 #
 
+# The bootrom-helper needs to be ARM (i.e. not Thumb code) due to the
+# way setjmp/longjmp is implemented.
+ifndef CONFIG_ARM64
+CFLAGS_bootrom.o := -marm
+endif
+
 # We don't want the bootrom-helper present in a full U-Boot build, as
 # this may have entered from ATF with the stack-pointer pointing to
 # inaccessible/protected memory (and the bootrom-helper assumes that
-- 
2.1.4

[U-Boot] [PATCH v2 0/5] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[Andy Yan]

Hi Philipp:


On 2017年09月19日 02:18, Philipp Tomsich wrote:
> 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.
>
> For v2, we force bootrom.o to alway be emitted as A32 (not T32), so we
> can safely call save_boot_params_ret().

     This still have a problem, because the setjmp  implementation for 
ARM32 platform  has humb code when CONFIG_SYS_THUMB_BUILD is
enabled, this is a default setting for most ARMv7 boards.
#if CONFIG_IS_ENABLED(SYS_THUMB_BUILD)
  ".align 2\n"
  "adr r0, jmp_target\n"
  "add r0, r0, $1\n"  // r0 stored the jump target address and with 
bit[0] = 1, this will trigger a thumb switch in longjmp with code "bx r0"
#endif

When I force the setjmp code go arm code path, I can back to bootrom 
successfully, But I got a data abort exception in later. it seems it 
happens when bootrom finished the uboot code
copy, when jump to sdram, I need a further debug.
>
> It also turned out that I had not caught the RK3188 in my earlier
> series... and my luck being what it is, the RK3188 needed some extra
> handholding to adapt to the new regime: instead of passing the context
> address (for returning to the BROM) from the TPL to the SPL, the SPL
> now returns to the TPL and the TPL then returns to the BROM.
>
> Changes in v2:
> - [added in v2] chain back_to_bootrom calls for SPL, first returning
>    to the TPL (using the same mechanism) and further calling through
>    to the BROM from the TPL by invoking back_to_bootrom again
> - adapt the RK3188 spl support file (that I had originally missed)
>
> Philipp Tomsich (5):
>    arm: make save_boot_params_ret prototype visible for AArch64
>    rockchip: back-to-bootrom: replace assembly-implementation with C-code
>    rockchip: back-to-bootrom: rk3188: chain from SPL via TPL to the BROM
>    rockchip: back-to-bootrom: allow passing a cmd to the bootrom
>    rockchip: back-to-bootrom: do not compile bootrom.o in thumb mode
>
>   arch/arm/include/asm/arch-rockchip/bootrom.h | 30 +++++++++---
>   arch/arm/include/asm/system.h                | 62 ++++++++++++-------------
>   arch/arm/mach-rockchip/Makefile              | 10 +++-
>   arch/arm/mach-rockchip/bootrom.c             | 54 +++++++++++++++++++++-
>   arch/arm/mach-rockchip/rk3036-board-spl.c    |  2 +-
>   arch/arm/mach-rockchip/rk3188-board-spl.c    | 14 +-----
>   arch/arm/mach-rockchip/rk3188-board-tpl.c    | 19 ++++----
>   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 ----------------------------
>   12 files changed, 133 insertions(+), 137 deletions(-)
>   delete mode 100644 arch/arm/mach-rockchip/save_boot_param.S
>

[U-Boot] [PATCH v2 0/5] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[Andy Yan]

Hi Philipp:


On 2017年09月19日 10:06, Andy Yan wrote:
> Hi Philipp:
>
>
> On 2017年09月19日 02:18, Philipp Tomsich wrote:
>> 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.
>>
>> For v2, we force bootrom.o to alway be emitted as A32 (not T32), so we
>> can safely call save_boot_params_ret().
>
>     This still have a problem, because the setjmp  implementation for 
> ARM32 platform  has humb code when CONFIG_SYS_THUMB_BUILD is
> enabled, this is a default setting for most ARMv7 boards.
> #if CONFIG_IS_ENABLED(SYS_THUMB_BUILD)
>  ".align 2\n"
>  "adr r0, jmp_target\n"
>  "add r0, r0, $1\n"  // r0 stored the jump target address and with 
> bit[0] = 1, this will trigger a thumb switch in longjmp with code "bx r0"
> #endif
>
> When I force the setjmp code go arm code path, I can back to bootrom 
> successfully, But I got a data abort exception in later. it seems it 
> happens when bootrom finished the uboot code
> copy, when jump to sdram, I need a further debug.

I found that r9 also need to be preserved, it seems that it hold the 
sdram base.
>>
>> It also turned out that I had not caught the RK3188 in my earlier
>> series... and my luck being what it is, the RK3188 needed some extra
>> handholding to adapt to the new regime: instead of passing the context
>> address (for returning to the BROM) from the TPL to the SPL, the SPL
>> now returns to the TPL and the TPL then returns to the BROM.
>>
>> Changes in v2:
>> - [added in v2] chain back_to_bootrom calls for SPL, first returning
>>    to the TPL (using the same mechanism) and further calling through
>>    to the BROM from the TPL by invoking back_to_bootrom again
>> - adapt the RK3188 spl support file (that I had originally missed)
>>
>> Philipp Tomsich (5):
>>    arm: make save_boot_params_ret prototype visible for AArch64
>>    rockchip: back-to-bootrom: replace assembly-implementation with 
>> C-code
>>    rockchip: back-to-bootrom: rk3188: chain from SPL via TPL to the BROM
>>    rockchip: back-to-bootrom: allow passing a cmd to the bootrom
>>    rockchip: back-to-bootrom: do not compile bootrom.o in thumb mode
>>
>>   arch/arm/include/asm/arch-rockchip/bootrom.h | 30 +++++++++---
>>   arch/arm/include/asm/system.h                | 62 
>> ++++++++++++-------------
>>   arch/arm/mach-rockchip/Makefile              | 10 +++-
>>   arch/arm/mach-rockchip/bootrom.c             | 54 
>> +++++++++++++++++++++-
>>   arch/arm/mach-rockchip/rk3036-board-spl.c    |  2 +-
>>   arch/arm/mach-rockchip/rk3188-board-spl.c    | 14 +-----
>>   arch/arm/mach-rockchip/rk3188-board-tpl.c    | 19 ++++----
>>   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 
>> ----------------------------
>>   12 files changed, 133 insertions(+), 137 deletions(-)
>>   delete mode 100644 arch/arm/mach-rockchip/save_boot_param.S
>>
>

[U-Boot] [PATCH v2 0/5] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[Dr. Philipp Tomsich]

Andy,

> On 19 Sep 2017, at 09:19, Andy Yan <andy.yan@rock-chips.com> wrote:
> 
> Hi Philipp:
> 
> 
> On 2017年09月19日 10:06, Andy Yan wrote:
>> Hi Philipp:
>> 
>> 
>> On 2017年09月19日 02:18, Philipp Tomsich wrote:
>>> 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.
>>> 
>>> For v2, we force bootrom.o to alway be emitted as A32 (not T32), so we
>>> can safely call save_boot_params_ret().
>> 
>>    This still have a problem, because the setjmp  implementation for ARM32 platform  has humb code when CONFIG_SYS_THUMB_BUILD is
>> enabled, this is a default setting for most ARMv7 boards.
>> #if CONFIG_IS_ENABLED(SYS_THUMB_BUILD)
>> ".align 2\n"
>> "adr r0, jmp_target\n"
>> "add r0, r0, $1\n"  // r0 stored the jump target address and with bit[0] = 1, this will trigger a thumb switch in longjmp with code "bx r0"
>> #endif
>> 
>> When I force the setjmp code go arm code path, I can back to bootrom successfully, But I got a data abort exception in later. it seems it happens when bootrom finished the uboot code
>> copy, when jump to sdram, I need a further debug.
> 
> I found that r9 also need to be preserved, it seems that it hold the sdram base.

Thanks for testing and debugging: this is invaluable support, as I only have AArch64 boards to test.

The r9 issue will be easy enough to resolve.
However, it looks like I will need more work on setjmp/longjmp to make this safe both for T32 and A32.
Plus: I need to figure out why this didn’t show in my disassembly (I don’t remember whether it was a rk3188 or rk3288 board I looked at).

Might be tomorrow or Thursday until I can provide an new version.

Regards,
Philipp.

[U-Boot] [PATCH v2 0/5] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[Heiko Stübner]

Am Dienstag, 19. September 2017, 11:10:29 CEST schrieb Dr. Philipp Tomsich:
> Andy,
> 
> > On 19 Sep 2017, at 09:19, Andy Yan <andy.yan@rock-chips.com> wrote:
> > 
> > Hi Philipp:
> > 
> > On 2017年09月19日 10:06, Andy Yan wrote:
> >> Hi Philipp:
> >> 
> >> On 2017年09月19日 02:18, Philipp Tomsich wrote:
> >>> 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.
> >>> 
> >>> For v2, we force bootrom.o to alway be emitted as A32 (not T32), so we
> >>> can safely call save_boot_params_ret().
> >>> 
> >>    This still have a problem, because the setjmp  implementation for
> >>    ARM32 platform  has humb code when CONFIG_SYS_THUMB_BUILD is>> 
> >> enabled, this is a default setting for most ARMv7 boards.
> >> #if CONFIG_IS_ENABLED(SYS_THUMB_BUILD)
> >> ".align 2\n"
> >> "adr r0, jmp_target\n"
> >> "add r0, r0, $1\n"  // r0 stored the jump target address and with bit[0]
> >> = 1, this will trigger a thumb switch in longjmp with code "bx r0"
> >> #endif
> >> 
> >> When I force the setjmp code go arm code path, I can back to bootrom
> >> successfully, But I got a data abort exception in later. it seems it
> >> happens when bootrom finished the uboot code copy, when jump to sdram, I
> >> need a further debug.
> > 
> > I found that r9 also need to be preserved, it seems that it hold the sdram
> > base.
> Thanks for testing and debugging: this is invaluable support, as I only have
> AArch64 boards to test.
> 
> The r9 issue will be easy enough to resolve.
> However, it looks like I will need more work on setjmp/longjmp to make this
> safe both for T32 and A32. Plus: I need to figure out why this didn’t show
> in my disassembly (I don’t remember whether it was a rk3188 or rk3288 board
> I looked at).
> 
> Might be tomorrow or Thursday until I can provide an new version.

From this conversation, it looks to me that I should wait for that new
version for testing on rk3188, as it will likely show the same issues, right?


Heiko

[U-Boot] [PATCH v2 0/5] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[Dr. Philipp Tomsich]

> On 19 Sep 2017, at 11:12, Heiko Stübner <heiko@sntech.de> wrote:
> 
> Am Dienstag, 19. September 2017, 11:10:29 CEST schrieb Dr. Philipp Tomsich:
>> Andy,
>> 
>>> On 19 Sep 2017, at 09:19, Andy Yan <andy.yan@rock-chips.com> wrote:
>>> 
>>> Hi Philipp:
>>> 
>>> On 2017年09月19日 10:06, Andy Yan wrote:
>>>> Hi Philipp:
>>>> 
>>>> On 2017年09月19日 02:18, Philipp Tomsich wrote:
>>>>> 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.
>>>>> 
>>>>> For v2, we force bootrom.o to alway be emitted as A32 (not T32), so we
>>>>> can safely call save_boot_params_ret().
>>>>> 
>>>>   This still have a problem, because the setjmp  implementation for
>>>>   ARM32 platform  has humb code when CONFIG_SYS_THUMB_BUILD is>> 
>>>> enabled, this is a default setting for most ARMv7 boards.
>>>> #if CONFIG_IS_ENABLED(SYS_THUMB_BUILD)
>>>> ".align 2\n"
>>>> "adr r0, jmp_target\n"
>>>> "add r0, r0, $1\n"  // r0 stored the jump target address and with bit[0]
>>>> = 1, this will trigger a thumb switch in longjmp with code "bx r0"
>>>> #endif
>>>> 
>>>> When I force the setjmp code go arm code path, I can back to bootrom
>>>> successfully, But I got a data abort exception in later. it seems it
>>>> happens when bootrom finished the uboot code copy, when jump to sdram, I
>>>> need a further debug.
>>> 
>>> I found that r9 also need to be preserved, it seems that it hold the sdram
>>> base.
>> Thanks for testing and debugging: this is invaluable support, as I only have
>> AArch64 boards to test.
>> 
>> The r9 issue will be easy enough to resolve.
>> However, it looks like I will need more work on setjmp/longjmp to make this
>> safe both for T32 and A32. Plus: I need to figure out why this didn’t show
>> in my disassembly (I don’t remember whether it was a rk3188 or rk3288 board
>> I looked at).
>> 
>> Might be tomorrow or Thursday until I can provide an new version.
> 
> From this conversation, it looks to me that I should wait for that new
> version for testing on rk3188, as it will likely show the same issues, right?

Yes.

[U-Boot] [PATCH v2 0/5] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[Dr. Philipp Tomsich]

Heiko,

> On 19 Sep 2017, at 12:16, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:
> 
>> 
>> On 19 Sep 2017, at 11:12, Heiko Stübner <heiko@sntech.de> wrote:
>> 
>> Am Dienstag, 19. September 2017, 11:10:29 CEST schrieb Dr. Philipp Tomsich:
>>> Andy,
>>> 
>>>> On 19 Sep 2017, at 09:19, Andy Yan <andy.yan@rock-chips.com> wrote:
>>>> 
>>>> Hi Philipp:
>>>> 
>>>> On 2017年09月19日 10:06, Andy Yan wrote:
>>>>> Hi Philipp:
>>>>> 
>>>>> On 2017年09月19日 02:18, Philipp Tomsich wrote:
>>>>>> 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.
>>>>>> 
>>>>>> For v2, we force bootrom.o to alway be emitted as A32 (not T32), so we
>>>>>> can safely call save_boot_params_ret().
>>>>>> 
>>>>>  This still have a problem, because the setjmp  implementation for
>>>>>  ARM32 platform  has humb code when CONFIG_SYS_THUMB_BUILD is>> 
>>>>> enabled, this is a default setting for most ARMv7 boards.
>>>>> #if CONFIG_IS_ENABLED(SYS_THUMB_BUILD)
>>>>> ".align 2\n"
>>>>> "adr r0, jmp_target\n"
>>>>> "add r0, r0, $1\n"  // r0 stored the jump target address and with bit[0]
>>>>> = 1, this will trigger a thumb switch in longjmp with code "bx r0"
>>>>> #endif
>>>>> 
>>>>> When I force the setjmp code go arm code path, I can back to bootrom
>>>>> successfully, But I got a data abort exception in later. it seems it
>>>>> happens when bootrom finished the uboot code copy, when jump to sdram, I
>>>>> need a further debug.
>>>> 
>>>> I found that r9 also need to be preserved, it seems that it hold the sdram
>>>> base.
>>> Thanks for testing and debugging: this is invaluable support, as I only have
>>> AArch64 boards to test.
>>> 
>>> The r9 issue will be easy enough to resolve.
>>> However, it looks like I will need more work on setjmp/longjmp to make this
>>> safe both for T32 and A32. Plus: I need to figure out why this didn’t show
>>> in my disassembly (I don’t remember whether it was a rk3188 or rk3288 board
>>> I looked at).
>>> 
>>> Might be tomorrow or Thursday until I can provide an new version.
>> 
>> From this conversation, it looks to me that I should wait for that new
>> version for testing on rk3188, as it will likely show the same issues, right?
> 
> Yes.

A new version with a reworked setjmp/longjmp implementation is available at
	https://patchwork.ozlabs.org/project/uboot/list/?series=4327

This passed testing for Andy (see the Tested-by: credit) on the boards he works
on, but is still pending a test of a RK3188.

Regards,
Philipp.

[U-Boot] [PATCH v2 0/5] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[Heiko Stuebner]

Hi Philipp,

Am Donnerstag, 21. September 2017, 10:26:19 CEST schrieb Dr. Philipp Tomsich:
> > On 19 Sep 2017, at 12:16, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:
> > 
> >> 
> >> On 19 Sep 2017, at 11:12, Heiko Stübner <heiko@sntech.de> wrote:
> >> 
> >> Am Dienstag, 19. September 2017, 11:10:29 CEST schrieb Dr. Philipp Tomsich:
> >>> Andy,
> >>> 
> >>>> On 19 Sep 2017, at 09:19, Andy Yan <andy.yan@rock-chips.com> wrote:
> >>>> 
> >>>> Hi Philipp:
> >>>> 
> >>>> On 2017年09月19日 10:06, Andy Yan wrote:
> >>>>> Hi Philipp:
> >>>>> 
> >>>>> On 2017年09月19日 02:18, Philipp Tomsich wrote:
> >>>>>> 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.
> >>>>>> 
> >>>>>> For v2, we force bootrom.o to alway be emitted as A32 (not T32), so we
> >>>>>> can safely call save_boot_params_ret().
> >>>>>> 
> >>>>>  This still have a problem, because the setjmp  implementation for
> >>>>>  ARM32 platform  has humb code when CONFIG_SYS_THUMB_BUILD is>> 
> >>>>> enabled, this is a default setting for most ARMv7 boards.
> >>>>> #if CONFIG_IS_ENABLED(SYS_THUMB_BUILD)
> >>>>> ".align 2\n"
> >>>>> "adr r0, jmp_target\n"
> >>>>> "add r0, r0, $1\n"  // r0 stored the jump target address and with bit[0]
> >>>>> = 1, this will trigger a thumb switch in longjmp with code "bx r0"
> >>>>> #endif
> >>>>> 
> >>>>> When I force the setjmp code go arm code path, I can back to bootrom
> >>>>> successfully, But I got a data abort exception in later. it seems it
> >>>>> happens when bootrom finished the uboot code copy, when jump to sdram, I
> >>>>> need a further debug.
> >>>> 
> >>>> I found that r9 also need to be preserved, it seems that it hold the sdram
> >>>> base.
> >>> Thanks for testing and debugging: this is invaluable support, as I only have
> >>> AArch64 boards to test.
> >>> 
> >>> The r9 issue will be easy enough to resolve.
> >>> However, it looks like I will need more work on setjmp/longjmp to make this
> >>> safe both for T32 and A32. Plus: I need to figure out why this didn’t show
> >>> in my disassembly (I don’t remember whether it was a rk3188 or rk3288 board
> >>> I looked at).
> >>> 
> >>> Might be tomorrow or Thursday until I can provide an new version.
> >> 
> >> From this conversation, it looks to me that I should wait for that new
> >> version for testing on rk3188, as it will likely show the same issues, right?
> > 
> > Yes.
> 
> A new version with a reworked setjmp/longjmp implementation is available at
> 	https://patchwork.ozlabs.org/project/uboot/list/?series=4327
> 
> This passed testing for Andy (see the Tested-by: credit) on the boards he works
> on, but is still pending a test of a RK3188.

Testing that v3 is actually the thing I currently doing, after I found the
v3 in my inbox :-) . 

Results shortly in the v3 series.


Heiko

[U-Boot] [PATCH v2 0/5] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[Dr. Philipp Tomsich]

Andy,

> On 19 Sep 2017, at 11:10, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:
> 
> Andy,
> 
>> On 19 Sep 2017, at 09:19, Andy Yan <andy.yan@rock-chips.com> wrote:
>> 
>> Hi Philipp:
>> 
>> 
>> On 2017年09月19日 10:06, Andy Yan wrote:
>>> Hi Philipp:
>>> 
>>> 
>>> On 2017年09月19日 02:18, Philipp Tomsich wrote:
>>>> 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.
>>>> 
>>>> For v2, we force bootrom.o to alway be emitted as A32 (not T32), so we
>>>> can safely call save_boot_params_ret().
>>> 
>>>   This still have a problem, because the setjmp  implementation for ARM32 platform  has humb code when CONFIG_SYS_THUMB_BUILD is
>>> enabled, this is a default setting for most ARMv7 boards.
>>> #if CONFIG_IS_ENABLED(SYS_THUMB_BUILD)
>>> ".align 2\n"
>>> "adr r0, jmp_target\n"
>>> "add r0, r0, $1\n"  // r0 stored the jump target address and with bit[0] = 1, this will trigger a thumb switch in longjmp with code "bx r0"
>>> #endif
>>> 
>>> When I force the setjmp code go arm code path, I can back to bootrom successfully, But I got a data abort exception in later. it seems it happens when bootrom finished the uboot code
>>> copy, when jump to sdram, I need a further debug.
>> 
>> I found that r9 also need to be preserved, it seems that it hold the sdram base.
> 
> Thanks for testing and debugging: this is invaluable support, as I only have AArch64 boards to test.
> 
> The r9 issue will be easy enough to resolve.
> However, it looks like I will need more work on setjmp/longjmp to make this safe both for T32 and A32.
> Plus: I need to figure out why this didn’t show in my disassembly (I don’t remember whether it was a rk3188 or rk3288 board I looked at).

I had a quick look and things may be quicker to resolve than I thought.
Before I create a new version, I was wondering what the requirements on the BROM end are:
Without changes to setjmp/longjmp, I can currently preserve "r4-r11, lr, sp” (i.e "r1-r3, ip" will be clobbered).
If the BROM need any of these additional registers preserved (i.e. r1,r2,r3,ip): let me know and I will change setjmp/longjmp to be more conservative.

[U-Boot] [PATCH v2 0/5] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[Andy Yan]

Hi Philipp:


On 2017年09月19日 20:45, Dr. Philipp Tomsich wrote:
> Andy,
>
>> On 19 Sep 2017, at 11:10, Dr. Philipp Tomsich 
>> <philipp.tomsich@theobroma-systems.com 
>> <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>>
>> Andy,
>>
>>> On 19 Sep 2017, at 09:19, Andy Yan <andy.yan@rock-chips.com 
>>> <mailto:andy.yan@rock-chips.com>> wrote:
>>>
>>> Hi Philipp:
>>>
>>>
>>> On 2017年09月19日 10:06, Andy Yan wrote:
>>>> Hi Philipp:
>>>>
>>>>
>>>> On 2017年09月19日 02:18, Philipp Tomsich wrote:
>>>>> 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.
>>>>>
>>>>> For v2, we force bootrom.o to alway be emitted as A32 (not T32), so we
>>>>> can safely call save_boot_params_ret().
>>>>
>>>>   This still have a problem, because the setjmp  implementation for 
>>>> ARM32 platform  has humb code when CONFIG_SYS_THUMB_BUILD is
>>>> enabled, this is a default setting for most ARMv7 boards.
>>>> #if CONFIG_IS_ENABLED(SYS_THUMB_BUILD)
>>>> ".align 2\n"
>>>> "adr r0, jmp_target\n"
>>>> "add r0, r0, $1\n"  // r0 stored the jump target address and with 
>>>> bit[0] = 1, this will trigger a thumb switch in longjmp with code 
>>>> "bx r0"
>>>> #endif
>>>>
>>>> When I force the setjmp code go arm code path, I can back to 
>>>> bootrom successfully, But I got a data abort exception in later. it 
>>>> seems it happens when bootrom finished the uboot code
>>>> copy, when jump to sdram, I need a further debug.
>>>
>>> I found that r9 also need to be preserved, it seems that it hold the 
>>> sdram base.
>>
>> Thanks for testing and debugging: this is invaluable support, as I 
>> only have AArch64 boards to test.
>>
>> The r9 issue will be easy enough to resolve.
>> However, it looks like I will need more work on setjmp/longjmp to 
>> make this safe both for T32 and A32.
>> Plus: I need to figure out why this didn’t show in my disassembly (I 
>> don’t remember whether it was a rk3188 or rk3288 board I looked at).
>
> I had a quick look and things may be quicker to resolve than I thought.
> Before I create a new version, I was wondering what the requirements 
> on the BROM end are:
> Without changes to setjmp/longjmp, I can currently preserve "r4-r11, 
> lr, sp” (i.e "r1-r3, ip" will be clobbered).
> If the BROM need any of these additional registers preserved (i.e. 
> r1,r2,r3,ip): let me know and I will change setjmp/longjmp to be more 
> conservative.
>

  The BROM code that call TPL/SPL also write in C like bellow:
  fp = (pFunc)(ptr + 4);
  ret = (*fp)();     // fp point to TPL/SPL first address
  if (ret)
      return;
  ptr = (uint8*)SDRAM_ADDR;

So the code doesn't touch the register directly, It's the compile stored 
SDRAM_ADDR in r9(I saw it on rk3036 platform).