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

[U-Boot] [PATCH v3 0/6] 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 turned out to require a some tweaking to system.h (making sure
that the prototype for save_boot_params_ret is visible for A64)and
start.S (so binutils knows that this is a possible function entry and
it can correctly insert A32-to-Thumb transitions) and taking the axe
to setjmp.h (which created quite a few issues with it not expecting
A32/T32/Thumb call-sites and some fragility from GCC being smart about
the clobber-list of the inline assembly... which led to r9 not being
saved or restored).

Changes in v3:
- tracked the root-cause why no interwork branch was emitted and fixed
  it using a '.type'-directive in start.S to mark save_boot_params_ret
  as a (possible) function-entry.
- converted setjmp/longjmp from inline-assembly to separate .S files
  to improve predicatability if emitted code

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
- also covers the RK3188 (which I had originally missed)

Philipp Tomsich (6):
  arm: make save_boot_params_ret prototype visible for AArch64
  arm: mark save_boot_params_ret as a function
  arm: provide a PCS-compliant setjmp implementation
  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

 arch/arm/cpu/armv7/start.S                   |  1 +
 arch/arm/include/asm/arch-rockchip/bootrom.h | 30 ++++++---
 arch/arm/include/asm/setjmp.h                | 94 ++++------------------------
 arch/arm/include/asm/system.h                | 62 +++++++++---------
 arch/arm/lib/Makefile                        |  6 ++
 arch/arm/lib/setjmp.S                        | 37 +++++++++++
 arch/arm/lib/setjmp_aarch64.S                | 42 +++++++++++++
 arch/arm/mach-rockchip/Makefile              |  4 +-
 arch/arm/mach-rockchip/bootrom.c             | 54 +++++++++++++++-
 arch/arm/mach-rockchip/rk3036-board-spl.c    |  2 +-
 arch/arm/mach-rockchip/rk3188-board-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 --------------------
 17 files changed, 226 insertions(+), 218 deletions(-)
 create mode 100644 arch/arm/lib/setjmp.S
 create mode 100644 arch/arm/lib/setjmp_aarch64.S
 delete mode 100644 arch/arm/mach-rockchip/save_boot_param.S

-- 
2.1.4

[U-Boot] [PATCH v3 1/6] 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>
Tested-by: Andy Yan <andy.yan@rock-chips.com>
---

Changes in v3: None
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 v3 2/6] arm: mark save_boot_params_ret as a function

[Philipp Tomsich]

As no '.type' was set for save_boot_params_ret in start.S, binutils
did not track whether it was emitted as A32 or T32.  By properly
marking save_boot_params_ret as a potential function entry, we can
make sure that the compiler will insert the appropriate instructions
for branching to save_boot_params_ret both for call-sites emitted as
A32 and T32.

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

---

Changes in v3:
- tracked the root-cause why no interwork branch was emitted and fixed
  it using a '.type'-directive in start.S to mark save_boot_params_ret
  as a (possible) function-entry.

Changes in v2: None

 arch/arm/cpu/armv7/start.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index 7b84a7a..95a0b52 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -31,6 +31,7 @@
 
 	.globl	reset
 	.globl	save_boot_params_ret
+	.type   save_boot_params_ret,%function
 #ifdef CONFIG_ARMV7_LPAE
 	.global	switch_to_hypervisor_ret
 #endif
-- 
2.1.4

[U-Boot] [PATCH v3 3/6] arm: provide a PCS-compliant setjmp implementation

[Philipp Tomsich]

The previous setjmp-implementation (as a static inline function that
contained an 'asm volatile' sequence) was extremely fragile: (some
versions of) GCC optimised the set of registers.  One critical example
was the removal of 'r9' from the clobber list, if -ffixed-reg9 was
supplied.

To increase robustness and ensure PCS-compliant behaviour, the setjmp
and longjmp implementation are now in assembly and closely match what
one would expect to find in a libc implementation.

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

---

Changes in v3:
- converted setjmp/longjmp from inline-assembly to separate .S files
  to improve predicatability if emitted code

Changes in v2: None

 arch/arm/include/asm/setjmp.h | 94 ++++++-------------------------------------
 arch/arm/lib/Makefile         |  6 +++
 arch/arm/lib/setjmp.S         | 37 +++++++++++++++++
 arch/arm/lib/setjmp_aarch64.S | 42 +++++++++++++++++++
 4 files changed, 98 insertions(+), 81 deletions(-)
 create mode 100644 arch/arm/lib/setjmp.S
 create mode 100644 arch/arm/lib/setjmp_aarch64.S

diff --git a/arch/arm/include/asm/setjmp.h b/arch/arm/include/asm/setjmp.h
index c3399a7..517beeb 100644
--- a/arch/arm/include/asm/setjmp.h
+++ b/arch/arm/include/asm/setjmp.h
@@ -1,6 +1,6 @@
 /*
- * (C) Copyright 2016
- * Alexander Graf <agraf@suse.de>
+ * (C) Copyright 2017 Theobroma Systems Design und Consulting GmbH
+ * (C) Copyright 2016 Alexander Graf <agraf@suse.de>
  *
  * SPDX-License-Identifier:	GPL-2.0+
  */
@@ -8,89 +8,21 @@
 #ifndef _SETJMP_H_
 #define _SETJMP_H_	1
 
+/*
+ * This really should be opaque, but the EFI implementation wrongly
+ * assumes that a 'struct jmp_buf_data' is defined.
+ */
 struct jmp_buf_data {
-	ulong target;
-	ulong regs[5];
-	int ret;
-};
-
-typedef struct jmp_buf_data jmp_buf[1];
-
-static inline int setjmp(jmp_buf jmp)
-{
-	jmp->ret = 0;
-
-#ifdef CONFIG_ARM64
-	asm volatile(
-		"adr x1, jmp_target\n"
-		"str x1, %0\n"
-		"stp x26, x27, %1\n"
-		"stp x28, x29, %2\n"
-		"mov x1, sp\n"
-		"str x1, %3\n"
-		"jmp_target: "
-		: "=m" (jmp->target), "=m" (jmp->regs[0]),
-		  "=m" (jmp->regs[2]), "=m" (jmp->regs[4])
-		:
-		: "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7",
-		  "x8", "x9", "x10", "x11", "x12", "x13", "x14", "x15",
-		  "x16", "x17", "x18", "x19", "x20", "x21", "x22",
-		  "x23", "x24", "x25", /* x26, x27, x28, x29, sp */
-		  "x30", "cc", "memory");
-#else
-	asm volatile(
-#if CONFIG_IS_ENABLED(SYS_THUMB_BUILD)
-		".align 2\n"
-		"adr r0, jmp_target\n"
-		"add r0, r0, $1\n"
+#if defined(__aarch64__)
+	u64  regs[13];
 #else
-		"adr r0, jmp_target\n"
-#endif
-		"mov r1, %0\n"
-		"mov r2, sp\n"
-		"stm r1!, {r0, r2, r4, r5, r6, r7}\n"
-		".align 2\n"
-		"jmp_target: \n"
-		:
-		: "l" (&jmp->target)
-		: "r0", "r1", "r2", "r3", /* "r4", "r5", "r6", "r7", */
-		  "r8", "r9", "r10", "r11", /* sp, */ "ip", "lr",
-		  "cc", "memory");
-#endif
-
-	return jmp->ret;
-}
-
-static inline __noreturn void longjmp(jmp_buf jmp, int ret)
-{
-	jmp->ret = ret;
-
-#ifdef CONFIG_ARM64
-	asm volatile(
-		"ldr x0, %0\n"
-		"ldr x1, %3\n"
-		"mov sp, x1\n"
-		"ldp x26, x27, %1\n"
-		"ldp x28, x25, %2\n"
-		"mov x29, x25\n"
-		"br x0\n"
-		:
-		: "m" (jmp->target), "m" (jmp->regs[0]), "m" (jmp->regs[2]),
-		  "m" (jmp->regs[4])
-		: "x0", "x1", "x25", "x26", "x27", "x28");
-#else
-	asm volatile(
-		"mov r1, %0\n"
-		"ldm r1!, {r0, r2, r4, r5, r6, r7}\n"
-		"mov sp, r2\n"
-		"bx r0\n"
-		:
-		: "l" (&jmp->target)
-		: "r1");
+	u32  regs[10];  /* r4-r9, sl, fp, sp, lr */
 #endif
+};
 
-	while (1) { }
-}
+typedef struct jmp_buf_data jmp_buf[1];
 
+int setjmp(jmp_buf jmp);
+void longjmp(jmp_buf jmp, int ret);
 
 #endif /* _SETJMP_H_ */
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 6e1c436..abffa10 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -17,6 +17,12 @@ else
 obj-y	+= vectors.o crt0.o
 endif
 
+ifdef CONFIG_ARM64
+obj-y   += setjmp_aarch64.o
+else
+obj-y   += setjmp.o
+endif
+
 ifndef CONFIG_SPL_BUILD
 ifdef CONFIG_ARM64
 obj-y	+= relocate_64.o
diff --git a/arch/arm/lib/setjmp.S b/arch/arm/lib/setjmp.S
new file mode 100644
index 0000000..6746e5e
--- /dev/null
+++ b/arch/arm/lib/setjmp.S
@@ -0,0 +1,37 @@
+/*
+ * (C) 2017 Theobroma Systems Design und Consulting GmbH
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <config.h>
+#include <asm/assembler.h>
+#include <linux/linkage.h>
+
+.pushsection .text.setjmp, "ax"
+ENTRY(setjmp)
+	/*
+	 * A subroutine must preserve the contents of the registers
+	 * r4-r8, r10, r11 (v1-v5, v7 and v8) and SP (and r9 in PCS
+	 * variants that designate r9 as v6).
+	 */
+	mov  ip, sp
+	stm  a1, {v1-v8, ip, lr}
+	mov  a1, #0
+	bx   lr
+ENDPROC(setjmp)
+.popsection
+
+.pushsection .text.longjmp, "ax"
+ENTRY(longjmp)
+	ldm  a1, {v1-v8, ip, lr}
+	mov  sp, ip
+	mov  a1, a2
+	/* If we were passed a return value of zero, return one instead */
+	cmp  a1, #0
+	bne  1f
+	mov  a1, #1
+1:
+	bx   lr
+ENDPROC(longjmp)
+.popsection
diff --git a/arch/arm/lib/setjmp_aarch64.S b/arch/arm/lib/setjmp_aarch64.S
new file mode 100644
index 0000000..b68edb8
--- /dev/null
+++ b/arch/arm/lib/setjmp_aarch64.S
@@ -0,0 +1,42 @@
+/*
+ * (C) 2017 Theobroma Systems Design und Consulting GmbH
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <config.h>
+#include <asm/macro.h>
+#include <linux/linkage.h>
+
+.pushsection .text.setjmp, "ax"
+ENTRY(setjmp)
+	/* Preserve all callee-saved registers and the SP */
+	stp  x19, x20, [x0,#0]
+	stp  x21, x22, [x0,#16]
+	stp  x23, x24, [x0,#32]
+	stp  x25, x26, [x0,#48]
+	stp  x27, x28, [x0,#64]
+	stp  x29, x30, [x0,#80]
+	mov  x2, sp
+	str  x2, [x0, #96]
+	mov  x0, #0
+	ret
+ENDPROC(setjmp)
+.popsection
+
+.pushsection .text.longjmp, "ax"
+ENTRY(longjmp)
+	ldp  x19, x20, [x0,#0]
+	ldp  x21, x22, [x0,#16]
+	ldp  x23, x24, [x0,#32]
+	ldp  x25, x26, [x0,#48]
+	ldp  x27, x28, [x0,#64]
+	ldp  x29, x30, [x0,#80]
+	ldr  x2, [x0,#96]
+	mov  sp, x2
+	/* Move the return value in place, but return 1 if passed 0. */
+	adds x0, xzr, x1
+	csinc x0, x0, xzr, ne
+	ret
+ENDPROC(longjmp)
+.popsection
-- 
2.1.4

[U-Boot] [PATCH v3 4/6] 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>
Tested-by: Andy Yan <andy.yan@rock-chips.com>
---

Changes in v3: None
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 v3 5/6] 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 v3: None
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 v3 6/6] 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>
Tested-by: Andy Yan <andy.yan@rock-chips.com>

---

Changes in v3: None
Changes in v2:
- also covers the RK3188 (which 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 v3 0/6] rockchip: back-to-bootrom: replace assembly-implementation with C-code

[Heiko Stuebner]

Am Donnerstag, 21. September 2017, 10:19:23 CEST schrieb 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 turned out to require a some tweaking to system.h (making sure
> that the prototype for save_boot_params_ret is visible for A64)and
> start.S (so binutils knows that this is a possible function entry and
> it can correctly insert A32-to-Thumb transitions) and taking the axe
> to setjmp.h (which created quite a few issues with it not expecting
> A32/T32/Thumb call-sites and some fragility from GCC being smart about
> the clobber-list of the inline assembly... which led to r9 not being
> saved or restored).

This is missing information on dependant series. Using the u-boot-rockchip
repository which is at
782088de7be7 ("rockchip: imply ADC and SARADC_ROCKCHIP on supported SoCs")

patches 1-3 apply, but patch 4 fails to apply as I seem to be missing some
dependencies.

And the u-boot mailinglist seems to be configured very strangely, as it
seems to rip apart patch-series only sending me some parts.

So far I can at least say, that the u-boot-rockchip repo at the above
commit still boots. Could you please point me to mbox versions
of needed base patches?


Thanks
Heiko

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

[Heiko Stuebner]

Am Donnerstag, 21. September 2017, 11:09:49 CEST schrieb Heiko Stuebner:
> Am Donnerstag, 21. September 2017, 10:19:23 CEST schrieb 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 turned out to require a some tweaking to system.h (making sure
> > that the prototype for save_boot_params_ret is visible for A64)and
> > start.S (so binutils knows that this is a possible function entry and
> > it can correctly insert A32-to-Thumb transitions) and taking the axe
> > to setjmp.h (which created quite a few issues with it not expecting
> > A32/T32/Thumb call-sites and some fragility from GCC being smart about
> > the clobber-list of the inline assembly... which led to r9 not being
> > saved or restored).
> 
> This is missing information on dependant series. Using the u-boot-rockchip
> repository which is at
> 782088de7be7 ("rockchip: imply ADC and SARADC_ROCKCHIP on supported SoCs")
> 
> patches 1-3 apply, but patch 4 fails to apply as I seem to be missing some
> dependencies.
> 
> And the u-boot mailinglist seems to be configured very strangely, as it
> seems to rip apart patch-series only sending me some parts.
> 
> So far I can at least say, that the u-boot-rockchip repo at the above
> commit still boots. Could you please point me to mbox versions
> of needed base patches?

Also, with patches 1-3 and 5 applied the radxarock board fails to start.
I see the SPL banner and a "Returning to boot ROM..." and then nothing.

I do belive it may have something to do with the TPL's + SPL's stack both
being at the end of SRAM? Having the SPL go back to TPL and then
back to bootrom was my original intention as well, but didn't work at
the time.


Heiko

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

[Dr. Philipp Tomsich]

> On 21 Sep 2017, at 11:44, Heiko Stuebner <heiko@sntech.de> wrote:
> 
> Am Donnerstag, 21. September 2017, 11:09:49 CEST schrieb Heiko Stuebner:
>> Am Donnerstag, 21. September 2017, 10:19:23 CEST schrieb 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 turned out to require a some tweaking to system.h (making sure
>>> that the prototype for save_boot_params_ret is visible for A64)and
>>> start.S (so binutils knows that this is a possible function entry and
>>> it can correctly insert A32-to-Thumb transitions) and taking the axe
>>> to setjmp.h (which created quite a few issues with it not expecting
>>> A32/T32/Thumb call-sites and some fragility from GCC being smart about
>>> the clobber-list of the inline assembly... which led to r9 not being
>>> saved or restored).
>> 
>> This is missing information on dependant series. Using the u-boot-rockchip
>> repository which is at
>> 782088de7be7 ("rockchip: imply ADC and SARADC_ROCKCHIP on supported SoCs")
>> 
>> patches 1-3 apply, but patch 4 fails to apply as I seem to be missing some
>> dependencies.
>> 
>> And the u-boot mailinglist seems to be configured very strangely, as it
>> seems to rip apart patch-series only sending me some parts.
>> 
>> So far I can at least say, that the u-boot-rockchip repo at the above
>> commit still boots. Could you please point me to mbox versions
>> of needed base patches?
> 
> Also, with patches 1-3 and 5 applied the radxarock board fails to start.
> I see the SPL banner and a "Returning to boot ROM..." and then nothing.
> 
> I do belive it may have something to do with the TPL's + SPL's stack both
> being at the end of SRAM? Having the SPL go back to TPL and then
> back to bootrom was my original intention as well, but didn't work at
> the time.

I didn’t expect the stacks to overlap… so returning from SPL to TPL can’t
work.  However, the jump_to_spl() is at least partially to blame (we already
have a working C-runtime and there’s no point in reentering through the
reset entry-point).

I need to ponder this a bit, but my gut feeling is that the TPL->SPL transition
can be done in a less intrusive way and may allow us to retain the TPL stack.

Regards,
Philipp.

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

[Dr. Philipp Tomsich]

> On 21 Sep 2017, at 11:09, Heiko Stuebner <heiko@sntech.de> wrote:
> 
> Am Donnerstag, 21. September 2017, 10:19:23 CEST schrieb 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 turned out to require a some tweaking to system.h (making sure
>> that the prototype for save_boot_params_ret is visible for A64)and
>> start.S (so binutils knows that this is a possible function entry and
>> it can correctly insert A32-to-Thumb transitions) and taking the axe
>> to setjmp.h (which created quite a few issues with it not expecting
>> A32/T32/Thumb call-sites and some fragility from GCC being smart about
>> the clobber-list of the inline assembly... which led to r9 not being
>> saved or restored).
> 
> This is missing information on dependant series. Using the u-boot-rockchip
> repository which is at
> 782088de7be7 ("rockchip: imply ADC and SARADC_ROCKCHIP on supported SoCs")
> 
> patches 1-3 apply, but patch 4 fails to apply as I seem to be missing some
> dependencies.
> 
> And the u-boot mailinglist seems to be configured very strangely, as it
> seems to rip apart patch-series only sending me some parts.
> 
> So far I can at least say, that the u-boot-rockchip repo at the above
> commit still boots. Could you please point me to mbox versions
> of needed base patches?

I seem to be suffering from “too many trees” syndrome.
The next reroll will be a clean one again.

Thanks,
Philipp.

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

[Dr. Philipp Tomsich]

> On 21 Sep 2017, at 12:25, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:
> 
>> 
>> On 21 Sep 2017, at 11:44, Heiko Stuebner <heiko at sntech.de <mailto:heiko@sntech.de>> wrote:
>> 
>> Am Donnerstag, 21. September 2017, 11:09:49 CEST schrieb Heiko Stuebner:
>>> Am Donnerstag, 21. September 2017, 10:19:23 CEST schrieb 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 turned out to require a some tweaking to system.h (making sure
>>>> that the prototype for save_boot_params_ret is visible for A64)and
>>>> start.S (so binutils knows that this is a possible function entry and
>>>> it can correctly insert A32-to-Thumb transitions) and taking the axe
>>>> to setjmp.h (which created quite a few issues with it not expecting
>>>> A32/T32/Thumb call-sites and some fragility from GCC being smart about
>>>> the clobber-list of the inline assembly... which led to r9 not being
>>>> saved or restored).
>>> 
>>> This is missing information on dependant series. Using the u-boot-rockchip
>>> repository which is at
>>> 782088de7be7 ("rockchip: imply ADC and SARADC_ROCKCHIP on supported SoCs")
>>> 
>>> patches 1-3 apply, but patch 4 fails to apply as I seem to be missing some
>>> dependencies.
>>> 
>>> And the u-boot mailinglist seems to be configured very strangely, as it
>>> seems to rip apart patch-series only sending me some parts.
>>> 
>>> So far I can at least say, that the u-boot-rockchip repo at the above
>>> commit still boots. Could you please point me to mbox versions
>>> of needed base patches?
>> 
>> Also, with patches 1-3 and 5 applied the radxarock board fails to start.
>> I see the SPL banner and a "Returning to boot ROM..." and then nothing.
>> 
>> I do belive it may have something to do with the TPL's + SPL's stack both
>> being at the end of SRAM? Having the SPL go back to TPL and then
>> back to bootrom was my original intention as well, but didn't work at
>> the time.
> 
> I didn’t expect the stacks to overlap… so returning from SPL to TPL can’t
> work.  However, the jump_to_spl() is at least partially to blame (we already
> have a working C-runtime and there’s no point in reentering through the
> reset entry-point).
> 
> I need to ponder this a bit, but my gut feeling is that the TPL->SPL transition
> can be done in a less intrusive way and may allow us to retain the TPL stack.

I had to draw this out on a whiteboard to keep track of what stacks are in use
and when they get destroyed (or returned to).
Clearly, returning to the TPL and expecting the C-runtime there to be intact
will not work.  The problem is that when returning to jump_to_spl(), the TPL’s
stack will have been overwritten by SPL.

I’ll need to look at the disassembly, but it might be sufficient if I chain into the
longjmp directly in jump_to_spl()… or I may need to do something disruptive
(e.g. switch stacks) in jump_to_spl.

Regards,
Philipp.

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

[Heiko Stübner]

Am Donnerstag, 21. September 2017, 12:25:17 CEST schrieb Dr. Philipp Tomsich:
> > On 21 Sep 2017, at 11:44, Heiko Stuebner <heiko@sntech.de> wrote:
> > 
> > Am Donnerstag, 21. September 2017, 11:09:49 CEST schrieb Heiko Stuebner:
> >> Am Donnerstag, 21. September 2017, 10:19:23 CEST schrieb 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 turned out to require a some tweaking to system.h (making sure
> >>> that the prototype for save_boot_params_ret is visible for A64)and
> >>> start.S (so binutils knows that this is a possible function entry and
> >>> it can correctly insert A32-to-Thumb transitions) and taking the axe
> >>> to setjmp.h (which created quite a few issues with it not expecting
> >>> A32/T32/Thumb call-sites and some fragility from GCC being smart about
> >>> the clobber-list of the inline assembly... which led to r9 not being
> >>> saved or restored).
> >> 
> >> This is missing information on dependant series. Using the
> >> u-boot-rockchip
> >> repository which is at
> >> 782088de7be7 ("rockchip: imply ADC and SARADC_ROCKCHIP on supported
> >> SoCs")
> >> 
> >> patches 1-3 apply, but patch 4 fails to apply as I seem to be missing
> >> some
> >> dependencies.
> >> 
> >> And the u-boot mailinglist seems to be configured very strangely, as it
> >> seems to rip apart patch-series only sending me some parts.
> >> 
> >> So far I can at least say, that the u-boot-rockchip repo at the above
> >> commit still boots. Could you please point me to mbox versions
> >> of needed base patches?
> > 
> > Also, with patches 1-3 and 5 applied the radxarock board fails to start.
> > I see the SPL banner and a "Returning to boot ROM..." and then nothing.
> > 
> > I do belive it may have something to do with the TPL's + SPL's stack both
> > being at the end of SRAM? Having the SPL go back to TPL and then
> > back to bootrom was my original intention as well, but didn't work at
> > the time.
> 
> I didn’t expect the stacks to overlap… so returning from SPL to TPL can’t
> work.  However, the jump_to_spl() is at least partially to blame (we already
> have a working C-runtime and there’s no point in reentering through the
> reset entry-point).
> 
> I need to ponder this a bit, but my gut feeling is that the TPL->SPL
> transition can be done in a less intrusive way and may allow us to retain
> the TPL stack.

Alternatively, if you can think of an easier solution we could do away with
the TPL in its current form. When I did the rk3188 support, this looked like
the least-messy way to me, but it really only does the one jump back
to the bootrom - so I'm not sure if there isn't simply an easier solution.

And for example the (still wip?) rk3066 series did use spl+tpl in a different
way due to bootrom-nand-limitations. rk3066 and rk3188 are quite similar
with the rk3066 simply not having the sd-boot capability, so if we want to
have nand-boot on rk3188 as well in the future, this may need a different
rework again.


Heiko

[U-Boot] [PATCH v3 3/6] arm: provide a PCS-compliant setjmp implementation

[Alexander Graf]

On 21.09.17 10:19, Philipp Tomsich wrote:
> The previous setjmp-implementation (as a static inline function that
> contained an 'asm volatile' sequence) was extremely fragile: (some
> versions of) GCC optimised the set of registers.  One critical example
> was the removal of 'r9' from the clobber list, if -ffixed-reg9 was
> supplied.

I wouldn't call that fragile, but "works as intended". Gcc only saves 
the registers it really needs to save - and if r9 is fixed it can safely 
assume that between setjmp/longjmp it did not change.

Did you encounter other cases where it did something wrong?

> To increase robustness and ensure PCS-compliant behaviour, the setjmp
> and longjmp implementation are now in assembly and closely match what
> one would expect to find in a libc implementation.

I'm personally quite indifferent on which version we take, but I 
personally found the inline asm version more readable. At least it was 
half-way self-documenting and struct offset independent ;).

But again, I really don't have strong feelings. I only wrote the inline 
asm version because we didn't have any implementation at all. If you opt 
to maintain yours, be my guest :).


Alex

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

[Andy Yan]

Hi Philipp, Heiko:


I finally got the upstream u-boot run on a rk3188 board which can be 
attached by DS5 debugger,

if you have some registers info want to check, please let me know.


On 2017年09月21日 18:44, Heiko Stübner wrote:
> Am Donnerstag, 21. September 2017, 12:25:17 CEST schrieb Dr. Philipp Tomsich:
>>> On 21 Sep 2017, at 11:44, Heiko Stuebner <heiko@sntech.de> wrote:
>>>
>>> Am Donnerstag, 21. September 2017, 11:09:49 CEST schrieb Heiko Stuebner:
>>>> Am Donnerstag, 21. September 2017, 10:19:23 CEST schrieb 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 turned out to require a some tweaking to system.h (making sure
>>>>> that the prototype for save_boot_params_ret is visible for A64)and
>>>>> start.S (so binutils knows that this is a possible function entry and
>>>>> it can correctly insert A32-to-Thumb transitions) and taking the axe
>>>>> to setjmp.h (which created quite a few issues with it not expecting
>>>>> A32/T32/Thumb call-sites and some fragility from GCC being smart about
>>>>> the clobber-list of the inline assembly... which led to r9 not being
>>>>> saved or restored).
>>>> This is missing information on dependant series. Using the
>>>> u-boot-rockchip
>>>> repository which is at
>>>> 782088de7be7 ("rockchip: imply ADC and SARADC_ROCKCHIP on supported
>>>> SoCs")
>>>>
>>>> patches 1-3 apply, but patch 4 fails to apply as I seem to be missing
>>>> some
>>>> dependencies.
>>>>
>>>> And the u-boot mailinglist seems to be configured very strangely, as it
>>>> seems to rip apart patch-series only sending me some parts.
>>>>
>>>> So far I can at least say, that the u-boot-rockchip repo at the above
>>>> commit still boots. Could you please point me to mbox versions
>>>> of needed base patches?
>>> Also, with patches 1-3 and 5 applied the radxarock board fails to start.
>>> I see the SPL banner and a "Returning to boot ROM..." and then nothing.
>>>
>>> I do belive it may have something to do with the TPL's + SPL's stack both
>>> being at the end of SRAM? Having the SPL go back to TPL and then
>>> back to bootrom was my original intention as well, but didn't work at
>>> the time.
>> I didn’t expect the stacks to overlap… so returning from SPL to TPL can’t
>> work.  However, the jump_to_spl() is at least partially to blame (we already
>> have a working C-runtime and there’s no point in reentering through the
>> reset entry-point).
>>
>> I need to ponder this a bit, but my gut feeling is that the TPL->SPL
>> transition can be done in a less intrusive way and may allow us to retain
>> the TPL stack.
> Alternatively, if you can think of an easier solution we could do away with
> the TPL in its current form. When I did the rk3188 support, this looked like
> the least-messy way to me, but it really only does the one jump back
> to the bootrom - so I'm not sure if there isn't simply an easier solution.
>
> And for example the (still wip?) rk3066 series did use spl+tpl in a different
> way due to bootrom-nand-limitations. rk3066 and rk3188 are quite similar
> with the rk3066 simply not having the sd-boot capability, so if we want to
> have nand-boot on rk3188 as well in the future, this may need a different
> rework again.
>
>
> Heiko
>
>
>

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

[Dr. Philipp Tomsich]

Andy,

Excellent news.
Looks like Heiko and I figured out what breaks the series last week (i.e. the SPL corrupts
the TPL’s stack—so my chaining will break things).

I’ll resubmit without the chained returns later and then we can have a final test tomorrow.

Regards,
Philipp.

> On 25 Sep 2017, at 10:46, Andy Yan <andy.yan@rock-chips.com> wrote:
> 
> Hi Philipp, Heiko:
> 
> 
> I finally got the upstream u-boot run on a rk3188 board which can be attached by DS5 debugger,
> 
> if you have some registers info want to check, please let me know.
> 
> 
> On 2017年09月21日 18:44, Heiko Stübner wrote:
>> Am Donnerstag, 21. September 2017, 12:25:17 CEST schrieb Dr. Philipp Tomsich:
>>>> On 21 Sep 2017, at 11:44, Heiko Stuebner <heiko@sntech.de> wrote:
>>>> 
>>>> Am Donnerstag, 21. September 2017, 11:09:49 CEST schrieb Heiko Stuebner:
>>>>> Am Donnerstag, 21. September 2017, 10:19:23 CEST schrieb 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 turned out to require a some tweaking to system.h (making sure
>>>>>> that the prototype for save_boot_params_ret is visible for A64)and
>>>>>> start.S (so binutils knows that this is a possible function entry and
>>>>>> it can correctly insert A32-to-Thumb transitions) and taking the axe
>>>>>> to setjmp.h (which created quite a few issues with it not expecting
>>>>>> A32/T32/Thumb call-sites and some fragility from GCC being smart about
>>>>>> the clobber-list of the inline assembly... which led to r9 not being
>>>>>> saved or restored).
>>>>> This is missing information on dependant series. Using the
>>>>> u-boot-rockchip
>>>>> repository which is at
>>>>> 782088de7be7 ("rockchip: imply ADC and SARADC_ROCKCHIP on supported
>>>>> SoCs")
>>>>> 
>>>>> patches 1-3 apply, but patch 4 fails to apply as I seem to be missing
>>>>> some
>>>>> dependencies.
>>>>> 
>>>>> And the u-boot mailinglist seems to be configured very strangely, as it
>>>>> seems to rip apart patch-series only sending me some parts.
>>>>> 
>>>>> So far I can at least say, that the u-boot-rockchip repo at the above
>>>>> commit still boots. Could you please point me to mbox versions
>>>>> of needed base patches?
>>>> Also, with patches 1-3 and 5 applied the radxarock board fails to start.
>>>> I see the SPL banner and a "Returning to boot ROM..." and then nothing.
>>>> 
>>>> I do belive it may have something to do with the TPL's + SPL's stack both
>>>> being at the end of SRAM? Having the SPL go back to TPL and then
>>>> back to bootrom was my original intention as well, but didn't work at
>>>> the time.
>>> I didn’t expect the stacks to overlap… so returning from SPL to TPL can’t
>>> work.  However, the jump_to_spl() is at least partially to blame (we already
>>> have a working C-runtime and there’s no point in reentering through the
>>> reset entry-point).
>>> 
>>> I need to ponder this a bit, but my gut feeling is that the TPL->SPL
>>> transition can be done in a less intrusive way and may allow us to retain
>>> the TPL stack.
>> Alternatively, if you can think of an easier solution we could do away with
>> the TPL in its current form. When I did the rk3188 support, this looked like
>> the least-messy way to me, but it really only does the one jump back
>> to the bootrom - so I'm not sure if there isn't simply an easier solution.
>> 
>> And for example the (still wip?) rk3066 series did use spl+tpl in a different
>> way due to bootrom-nand-limitations. rk3066 and rk3188 are quite similar
>> with the rk3066 simply not having the sd-boot capability, so if we want to
>> have nand-boot on rk3188 as well in the future, this may need a different
>> rework again.
>> 
>> 
>> Heiko
>> 
>> 
>> 
> 
>