sunxi: Allwinner A64: SPL support

These days many Allwinner SoCs use clock_sun6i.c, although out of them only the (original sun6i) A31 has a second MBUS clock register. Also the requirement for setting up the PRCM PLL_CTLR1 register to provide the proper voltage seems to be a property of older SoCs only as well.

Restrict the MBUS initialization to this SoC only to avoid writing bogus values to (undefined) registers in other chips.
I can only verify that the PLL voltage setup is not needed for H3 and A64, so for now we only spare those two SoCs.

Signed-off-by Andre Przywara <[email protected]>

Reviewed-by Alexander Graf <[email protected]>

Reviewed-by Chen-Yu Tsai <[email protected]>

---
 arch/arm/mach-sunxi/clock_sun6i.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

These days many Allwinner SoCs use clock_sun6i.c, although out of them only the (original sun6i) A31 has a second MBUS clock register. Also the requirement for setting up the PRCM PLL_CTLR1 register to provide the proper voltage seems to be a property of older SoCs only as well.

Restrict the MBUS initialization to this SoC only to avoid writing bogus values to (undefined) registers in other chips.
I can only verify that the PLL voltage setup is not needed for H3 and A64, so for now we only spare those two SoCs.

Signed-off-by Andre Przywara <[email protected]>

Reviewed-by Alexander Graf <[email protected]>

Reviewed-by Chen-Yu Tsai <[email protected]>

The predominantely 32-bit ARM targets try to compile the SPL in Thumb mode to reduce code size.
The 64-bit AArch64 instruction set does not know an alternative, concise encoding, so the Thumb build option should only be set for 32-bit targets.
Likewise -marm machine options are only valid for ARMv7 targets.

Signed-off-by Andre Przywara <[email protected]>

Reviewed-by Alexander Graf <[email protected]>

---
 arch/arm/lib/Makefile          | 2 ++
 include/configs/sunxi-common.h | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

The predominantely 32-bit ARM targets try to compile the SPL in Thumb mode to reduce code size.
The 64-bit AArch64 instruction set does not know an alternative, concise encoding, so the Thumb build option should only be set for 32-bit targets.
Likewise -marm machine options are only valid for ARMv7 targets.

Signed-off-by Andre Przywara <[email protected]>

Reviewed-by Alexander Graf <[email protected]>

The predominantely 32-bit ARM targets try to compile the SPL in Thumb mode to reduce code size.
The 64-bit AArch64 instruction set does not know an alternative, concise encoding, so the Thumb build option should only be set for 32-bit targets.
Likewise -marm machine options are only valid for ARMv7 targets.

Signed-off-by Andre Przywara <[email protected]>

Reviewed-by Alexander Graf <[email protected]>

For boards that call s_init() when the SPL runs, we are expected to setup an early stack before calling this C function. Implement the proper AArch64 version of this based on the ARMv7 code. This allows sunxi boards to setup the basic peripherals even on with a 64-bit SPL.

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/cpu/armv8/Makefile        |  1 +
 arch/arm/cpu/armv8/lowlevel_init.S | 44 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 arch/arm/cpu/armv8/lowlevel_init.S

Hi Andre,

Quoted 6 lines · Andre Przywara

For boards that call s_init() when the SPL runs, we are expected to setup an early stack before calling this C function. Implement the proper AArch64 version of this based on the ARMv7 code. This allows sunxi boards to setup the basic peripherals even on with a 64-bit SPL.

Quoted 6 lines

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/cpu/armv8/Makefile        |  1 +
 arch/arm/cpu/armv8/lowlevel_init.S | 44 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 arch/arm/cpu/armv8/lowlevel_init.S

Is this actually needed / used for anything?

Quoted 9 lines · Simon Glass

Hi Andre,

Quoted 6 lines · Andre Przywara

For boards that call s_init() when the SPL runs, we are expected to setup an early stack before calling this C function. Implement the proper AArch64 version of this based on the ARMv7 code. This allows sunxi boards to setup the basic peripherals even on with a 64-bit SPL.

Quoted 8 lines

Quoted 6 lines

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/cpu/armv8/Makefile        |  1 +
 arch/arm/cpu/armv8/lowlevel_init.S | 44 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 arch/arm/cpu/armv8/lowlevel_init.S

Is this actually needed / used for anything?

All sunxi boards need to call s_init() in mach-sunxi/board.c. But I gave this a closer look:
Indeed I believe we don't need lowlevel_init.S and the early call to s_init(). We can just follow the recommendation in lowlevel_init.S and move that code to board_init_f().
I have a small series that reworks this, but this would affect all sunxi boards so should be considered separately.

Are you OK if we go ahead with this solution here for now, as it creates the least churn?

For boards that call s_init() when the SPL runs, we are expected to setup an early stack before calling this C function. Implement the proper AArch64 version of this based on the ARMv7 code. This allows sunxi boards to setup the basic peripherals even on with a 64-bit SPL.

Signed-off-by Andre Przywara <[email protected]>

Quoted 6 lines · Andre Przywara

For boards that call s_init() when the SPL runs, we are expected to setup an early stack before calling this C function. Implement the proper AArch64 version of this based on the ARMv7 code. This allows sunxi boards to setup the basic peripherals even on with a 64-bit SPL.

Quoted 1 line

Signed-off-by Andre Przywara <[email protected]>

This is going to override lowlevel_init from arch/arm/cpu/armv8/start.S and is that really desired? Thanks!

Quoted 8 lines · Tom Rini

Quoted 6 lines · Andre Przywara

For boards that call s_init() when the SPL runs, we are expected to setup an early stack before calling this C function. Implement the proper AArch64 version of this based on the ARMv7 code. This allows sunxi boards to setup the basic peripherals even on with a 64-bit SPL.

Quoted 4 lines

Quoted 1 line

Signed-off-by Andre Przywara <[email protected]>

This is going to override lowlevel_init from arch/arm/cpu/armv8/start.S and is that really desired? Thanks!

Not sure if it is desired, but it's needed. The weak function in start.S just initialises the GIC (if configured), that looks like a NOP for sunxi to me (we do this already much better in ARM Trusted Firmware). For 32-bit sunxi we call s_init() through lowlevel_init(), which is what I copy here for armv8.
Now there is this comment which discourages doing so and Alex already complained about it as well, so I might take a look at how we would skip this step. I was a bit wary going there as this would mean to rework the 32-bit code as well, which affects a lot of boards, which I can barely test here.

tiny-printf does not know about the "l" modifier so far, which breaks the crash dump on AArch64, because it uses %lx to print the registers. Add an easy way of handling longs correctly. Also there are printfs using the '-' modifier, which we choose to ignore for simplicity.

Using a relatively decent compiler (GCC 5.3.0) this does _not_ increase the code size of tiny-printf.o for 32-bit builds (where long and int are actually the same), actually it looses three (ARM Thumb2) instructions from the actual SPL (numbers for orangepi_plus_defconfig):

  text    data     bss     dec     hex filename
   758       0       0     758     2f6 spl/lib/tiny-printf.o    before
 18839     488     232   19559    4c67 spl/u-boot-spl           before
   758       0       0     758     2f6 spl/lib/tiny-printf.o    after
 18833     488     232   19553    4c61 spl/u-boot-spl           after

This adds some substantial amount of code to a 64-bit build, though:
(taken after a later commit, which enables the ARM64 SPL build for sunxi)
text data bss dec hex filename

  1542       0       0    1542     606 spl/lib/tiny-printf.o    before
 25830     392     360   26582    67d6 spl/u-boot-spl           before
  1758       0       0    1758     6de spl/lib/tiny-printf.o    after
 26040     392     360   26792    68a8 spl/u-boot-spl           after

Signed-off-by Andre Przywara <[email protected]>

---
 lib/tiny-printf.c | 50 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 11 deletions(-)

tiny-printf does not know about the "l" modifier so far, which breaks the crash dump on AArch64, because it uses %lx to print the registers. Add an easy way of handling longs correctly. Also there are printfs using the '-' modifier, which we choose to ignore for simplicity.

Using a relatively decent compiler (GCC 5.3.0) this does _not_ increase the code size of tiny-printf.o for 32-bit builds (where long and int are actually the same), actually it looses three (ARM Thumb2) instructions from the actual SPL (numbers for orangepi_plus_defconfig):

  text    data     bss     dec     hex filename
   758       0       0     758     2f6 spl/lib/tiny-printf.o	before

18839 488 232 19559 4c67 spl/u-boot-spl before
758 0 0 758 2f6 spl/lib/tiny-printf.o after
18833 488 232 19553 4c61 spl/u-boot-spl after

This adds some substantial amount of code to a 64-bit build, though:
(taken after a later commit, which enables the ARM64 SPL build for sunxi)
text data bss dec hex filename
1542 0 0 1542 606 spl/lib/tiny-printf.o before
25830 392 360 26582 67d6 spl/u-boot-spl before
1758 0 0 1758 6de spl/lib/tiny-printf.o after
26040 392 360 26792 68a8 spl/u-boot-spl after

Signed-off-by Andre Przywara <[email protected]>

---
 lib/tiny-printf.c | 50 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 11 deletions(-)

lib/tiny-printf.c +39 -11

index 30ac759..dfa8432 100644
@@ -38,8 +38,8 @@ static void out_dgt(struct printf_info *info, char dgt)
 	info->zs = 1;
 }
 
-static void div_out(struct printf_info *info, unsigned int *num,
-		    unsigned int div)
+static void div_out(struct printf_info *info, unsigned long *num,
+		    unsigned long div)
 {
 	unsigned char dgt = 0;
 

@@ -56,9 +56,9 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va)
 {
 	char ch;
 	char *p;
-	unsigned int num;
+	unsigned long num;
 	char buf[12];
-	unsigned int div;
+	unsigned long div;
 
 	while ((ch = *(fmt++))) {
 		if (ch != '%') {

@@ -66,8 +66,12 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va)
 		} else {
 			bool lz = false;
 			int width = 0;
+			bool islong = false;
 
 			ch = *(fmt++);
+			if (ch == '-')
+				ch = *(fmt++);
+
 			if (ch == '0') {
 				ch = *(fmt++);
 				lz = 1;

@@ -80,6 +84,11 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va)
 					ch = *fmt++;
 				}
 			}
+			if (ch == 'l') {
+				ch = *(fmt++);
+				islong = true;
+			}
+
 			info->bf = buf;
 			p = info->bf;
 			info->zs = 0;

@@ -89,24 +98,43 @@ int _vprintf(struct printf_info *info, const char *fmt, va_list va)
 				goto abort;
 			case 'u':
 			case 'd':
-				num = va_arg(va, unsigned int);
-				if (ch == 'd' && (int)num < 0) {
-					num = -(int)num;
-					out(info, '-');
+				div = 1000000000;
+				if (islong) {
+					num = va_arg(va, unsigned long);
+					if (sizeof(long) > 4)
+						div *= div * 10;
+				} else {
+					num = va_arg(va, unsigned int);
+				}
+
+				if (ch == 'd') {
+					if (islong && (long)num < 0) {
+						num = -(long)num;
+						out(info, '-');
+					} else if (!islong && (int)num < 0) {
+						num = -(int)num;
+						out(info, '-');
+					}
 				}
 				if (!num) {
 					out_dgt(info, 0);
 				} else {
-					for (div = 1000000000; div; div /= 10)
+					for (; div; div /= 10)
 						div_out(info, &num, div);
 				}
 				break;
 			case 'x':
-				num = va_arg(va, unsigned int);
+				if (islong) {
+					num = va_arg(va, unsigned long);
+					div = 1UL << (sizeof(long) * 8 - 4);
+				} else {
+					num = va_arg(va, unsigned int);
+					div = 0x10000000;
+				}
 				if (!num) {
 					out_dgt(info, 0);
 				} else {
-					for (div = 0x10000000; div; div /= 0x10)
+					for (; div; div /= 0x10)
 						div_out(info, &num, div);
 				}
 				break;

The UL() macro is pretty useful in sharing constants between assembly and C files while still being able to specify a type for C. Move the macro from an armv8 specific header into a common header file to be able to use it by arm code (for instance) as well.

Signed-off-by Andre Przywara <[email protected]>

Reviewed-by Alexander Graf <[email protected]>

---
 arch/arm/include/asm/armv8/mmu.h |  8 --------
 include/common.h                 | 10 +++++++++-
 2 files changed, 9 insertions(+), 9 deletions(-)

arch/arm/include/asm/armv8/mmu.h +0 -8

index aa0f3c4..e9b4cdb 100644
@@ -8,14 +8,6 @@
 #ifndef _ASM_ARMV8_MMU_H_
 #define _ASM_ARMV8_MMU_H_
 
-#ifdef __ASSEMBLY__
-#define _AC(X, Y)      X
-#else
-#define _AC(X, Y)      (X##Y)
-#endif
-
-#define UL(x)          _AC(x, UL)
-
 /***************************************************************/
 /*
  * The following definitions are related each other, shoud be

include/common.h +2 -0

index a8d833b..5fcd5f5 100644
@@ -15,6 +15,8 @@ typedef volatile unsigned long        vu_long;
 typedef volatile unsigned short vu_short;
 typedef volatile unsigned char vu_char;
 
+#define _AC(X, Y)       (X##Y)
+
 #include <config.h>
 #include <errno.h>
 #include <asm-offsets.h>

include/common.h +4 -1

+
+#endif /* __ASSEMBLY__ */
 
 #ifdef CONFIG_PPC
 /*

@@ -948,6 +954,8 @@ int cpu_release(int nr, int argc, char * const argv[]);
 
 /* Put only stuff here that the assembler can digest */
 
+#define UL(x)           _AC(x, UL)
+
 #ifdef CONFIG_POST
 #define CONFIG_HAS_POST
 #ifndef CONFIG_POST_ALT_LIST
--

2.8.2

Since entry_point and load_addr are addresses, they should be represented as longs to cover the whole address space and to avoid warning when compiling the SPL in 64-bit.
Also adjust debug prints to add the 'l' specifier, where needed.

Signed-off-by Andre Przywara <[email protected]>

Reviewed-by Alexander Graf <[email protected]>

---
 arch/arm/mach-omap2/boot-common.c | 2 +-
 arch/arm/mach-tegra/spl.c         | 2 +-
 common/spl/spl.c                  | 8 ++++----
 common/spl/spl_mmc.c              | 2 +-
 include/spl.h                     | 4 ++--
 5 files changed, 9 insertions(+), 9 deletions(-)

Since entry_point and load_addr are addresses, they should be represented as longs to cover the whole address space and to avoid warning when compiling the SPL in 64-bit.
Also adjust debug prints to add the 'l' specifier, where needed.

Signed-off-by Andre Przywara <[email protected]>

Reviewed-by Alexander Graf <[email protected]>

Since entry_point and load_addr are addresses, they should be represented as longs to cover the whole address space and to avoid warning when compiling the SPL in 64-bit.
Also adjust debug prints to add the 'l' specifier, where needed.

Signed-off-by Andre Przywara <[email protected]>

Reviewed-by Alexander Graf <[email protected]>

The sunxi DRAM setup code needs an sdelay() implementation, which wasn't defined for armv8 so far.
Shamelessly copy the armv7 version and adjust it to work in AArch64.

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/cpu/armv8/cpu.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

arch/arm/cpu/armv8/cpu.c +0 -0

index e06c3cc..0366ff4 100644

arch/arm/cpu/armv8/cpu.c +7 -1

+ *************************************************************/
+void sdelay(unsigned long loops)
+{
+       __asm__ volatile ("1:\n" "subs %0, %0, #1\n"
+                         "b.ne 1b" : "=r" (loops) : "0"(loops) : "cc");
+}
+
 int cleanup_before_linux(void)
 {
        /*
--

2.8.2

The boot0 hook we have so far is applied _after_ the initial branch to the "reset" entry point. An upcoming change requires even this branch to be changed, so we apply the hook macro at the earliest point, and have the branch in the hook file as well.
This is no functional change at this point, just refactoring to simplify upcoming patches.

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/cpu/armv8/start.S              | 4 ++--
 arch/arm/include/asm/arch-sunxi/boot0.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

Hi Andre,

Quoted 7 lines · Andre Przywara

The boot0 hook we have so far is applied _after_ the initial branch to the "reset" entry point. An upcoming change requires even this branch to be changed, so we apply the hook macro at the earliest point, and have the branch in the hook file as well.
This is no functional change at this point, just refactoring to simplify upcoming patches.

Quoted 5 lines

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/cpu/armv8/start.S              | 4 ++--
 arch/arm/include/asm/arch-sunxi/boot0.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

Will this not affect other boards which use ARM_SOC_BOOT0_HOOK?

Hi,

Quoted 10 lines · Simon Glass

Hi Andre,

Quoted 7 lines · Andre Przywara

The boot0 hook we have so far is applied _after_ the initial branch to the "reset" entry point. An upcoming change requires even this branch to be changed, so we apply the hook macro at the earliest point, and have the branch in the hook file as well.
This is no functional change at this point, just refactoring to simplify upcoming patches.

Quoted 7 lines

Quoted 5 lines

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/cpu/armv8/start.S              | 4 ++--
 arch/arm/include/asm/arch-sunxi/boot0.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

Will this not affect other boards which use ARM_SOC_BOOT0_HOOK?

That's a valid question, but the answer is: no.
This roughly same mechanism is used by two Broadcom ARMv7 boards, but the usage is different there: they include the boot0.h header file only after the vectors (and not only after the initial branch-to-reset). So this is already different and not compatible between armv7 and armv8 right now, so it's not a regression or change this patch is introducing.

I agree it's a bit confusing to have the same header and Kconfig name, but a different behaviour, but I don't see a good solution to unify this. If you do, I am all ears.

For prepending some board specific header area to U-Boot images we were so far including a header file with a macro definition containing the actual header specification.
This works fine if there are just a few statements and if there is only one alternative.
However adding more complex code quickly gets messy with this approach, so let's just drop that intermediate macro and let the #include actually insert the code directly.
This converts the callers and the callees, but doesn't change anything at this point.

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/cpu/armv8/start.S                 | 1 -
 arch/arm/include/asm/arch-bcm235xx/boot0.h | 8 +-------
 arch/arm/include/asm/arch-bcm281xx/boot0.h | 8 +-------
 arch/arm/include/asm/arch-sunxi/boot0.h    | 8 +-------
 arch/arm/lib/vectors.S                     | 1 -
 5 files changed, 3 insertions(+), 23 deletions(-)

For prepending some board specific header area to U-Boot images we were so far including a header file with a macro definition containing the actual header specification.
This works fine if there are just a few statements and if there is only one alternative.
However adding more complex code quickly gets messy with this approach, so let's just drop that intermediate macro and let the #include actually insert the code directly.
This converts the callers and the callees, but doesn't change anything at this point.

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/cpu/armv8/start.S                 | 1 -
 arch/arm/include/asm/arch-bcm235xx/boot0.h | 8 +-------
 arch/arm/include/asm/arch-bcm281xx/boot0.h | 8 +-------
 arch/arm/include/asm/arch-sunxi/boot0.h    | 8 +-------
 arch/arm/lib/vectors.S                     | 1 -
 5 files changed, 3 insertions(+), 23 deletions(-)

arch/arm/cpu/armv8/start.S +0 -1

index ee393d7..140609d 100644
@@ -26,7 +26,6 @@ _start:
  * use it here.
  */
 #include <asm/arch/boot0.h>
-ARM_SOC_BOOT0_HOOK
 #else
        b       reset
 #endif

arch/arm/include/asm/arch-bcm235xx/boot0.h +0 -0

index 7e72882..9ff90b8 100644

arch/arm/include/asm/arch-bcm235xx/boot0.h +0 -2

        .word   _end - _start
-
-#endif /* __BOOT0_H */

arch/arm/include/asm/arch-bcm281xx/boot0.h +0 -0

index 7e72882..9ff90b8 100644

arch/arm/include/asm/arch-bcm281xx/boot0.h +0 -2

        .word   _end - _start
-
-#endif /* __BOOT0_H */

arch/arm/include/asm/arch-sunxi/boot0.h +1 -7

index 6f28d63..6a13db5 100644
@@ -4,12 +4,6 @@
  * SPDX-License-Identifier:    GPL-2.0+
  */
 
-#ifndef __BOOT0_H
-#define __BOOT0_H
-
 /* reserve space for BOOT0 header information */
-#define ARM_SOC_BOOT0_HOOK     \
-       b       reset;          \
+       b       reset
        .space  1532
-
-#endif /* __BOOT0_H */

arch/arm/lib/vectors.S +0 -2

index 5cc132b..9fe7415 100644
@@ -67,7 +67,6 @@ _start:
  * use it here.
  */
 #include <asm/arch/boot0.h>
-ARM_SOC_BOOT0_HOOK
 #endif
 
 /*
--

2.8.2

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

The ENABLE_ARM_SOC_BOOT0_HOOK option is a generic option shared with other boards. To allow alternative code to be inserted, we create another, now function specific config symbol on top of it to simplify later additions. No functional change at this time.

Signed-off-by Andre Przywara <[email protected]>

---
 board/sunxi/Kconfig           | 9 +++++++++
 configs/pine64_plus_defconfig | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

board/sunxi/Kconfig +8 -0

+       bool "reserve space for Allwinner boot0 header"
+       select ENABLE_ARM_SOC_BOOT0_HOOK
+       ---help---
+       Prepend a 1536 byte (empty) header to the U-Boot image file, to be
+       filled with magic values post build. The Allwinner provided boot0
+       blob relies on this information to load and execute U-Boot.
+       Only needed on 64-bit Allwinner boards so far when using boot0.
+
 config DRAM_TYPE
        int "sunxi dram type"
        depends on MACH_SUN8I_A83T

configs/pine64_plus_defconfig +1 -2

index 6d0198f..ea53b96 100644
@@ -1,5 +1,5 @@
 CONFIG_ARM=y
-CONFIG_ENABLE_ARM_SOC_BOOT0_HOOK=y
+CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER=y
 CONFIG_ARCH_SUNXI=y
 CONFIG_MACH_SUN50I=y
 CONFIG_DRAM_CLK=672
--

2.8.2

Quoted 11 lines · Andre Przywara

The ENABLE_ARM_SOC_BOOT0_HOOK option is a generic option shared with other boards. To allow alternative code to be inserted, we create another, now function specific config symbol on top of it to simplify later additions. No functional change at this time.

Signed-off-by Andre Przywara <[email protected]>

---
 board/sunxi/Kconfig           | 9 +++++++++
 configs/pine64_plus_defconfig | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

Reviewed-by Simon Glass <[email protected]>

Quoted 8 lines

board/sunxi/Kconfig +0 -0

index e1d4ab1..0cd57a2 100644
@@ -133,6 +133,15 @@ config MACH_SUN8I
        bool
        default y if MACH_SUN8I_A23 || MACH_SUN8I_A33 || MACH_SUN8I_H3 || MACH_SUN8I_A83T
 

board/sunxi/Kconfig config MACH_SUN8I · line 136

Quoted 1 line

board/sunxi/Kconfig +1 -0

+config RESERVE_ALLWINNER_BOOT0_HEADER

Would RESERVE_SUNXI_BOOT0_HEADER be better?

Quoted 25 lines

board/sunxi/Kconfig +8 -0

+       bool "reserve space for Allwinner boot0 header"
+       select ENABLE_ARM_SOC_BOOT0_HOOK
+       ---help---
+       Prepend a 1536 byte (empty) header to the U-Boot image file, to be
+       filled with magic values post build. The Allwinner provided boot0
+       blob relies on this information to load and execute U-Boot.
+       Only needed on 64-bit Allwinner boards so far when using boot0.
+
 config DRAM_TYPE
        int "sunxi dram type"
        depends on MACH_SUN8I_A83T

configs/pine64_plus_defconfig +1 -2

index 6d0198f..ea53b96 100644
@@ -1,5 +1,5 @@
 CONFIG_ARM=y
-CONFIG_ENABLE_ARM_SOC_BOOT0_HOOK=y
+CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER=y
 CONFIG_ARCH_SUNXI=y
 CONFIG_MACH_SUN50I=y
 CONFIG_DRAM_CLK=672
--

2.8.2

The ENABLE_ARM_SOC_BOOT0_HOOK option is a generic option shared with other boards. To allow alternative code to be inserted, we create another, now function specific config symbol on top of it to simplify later additions. No functional change at this time.

Signed-off-by Andre Przywara <[email protected]>

The Allwinner A64 SoC starts execution in AArch32 mode, and both the boot ROM and Allwinner's boot0 keep running in this mode. So U-Boot gets entered in 32-bit, although we want it to run in AArch64.

By using a "magic" instruction, which happens to be an almost-NOP in AArch64 and a branch in AArch32, we differentiate between being entered in 64-bit or 32-bit mode.
If in 64-bit mode, we proceed with the branch to reset, but in 32-bit mode we trigger an RMR write to bring the core into AArch64/EL3 and re-enter U-Boot at CONFIG_SYS_TEXT_BASE.
This allows a 64-bit U-Boot to be both entered in 32 and 64-bit mode, so we can use the same start code for the SPL and the U-Boot proper.

We use the existing custom header (boot0.h) functionality, but restrict the existing boot0 header reservation to the non-SPL build now. A SPL wouldn't need such header anyway. This allows to have both options defined and lets us use one for the SPL and the other for U-Boot proper.

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/include/asm/arch-sunxi/boot0.h | 27 +++++++++++++++++++++++++++
 board/sunxi/Kconfig                     |  5 +++++
 2 files changed, 32 insertions(+)

arch/arm/include/asm/arch-sunxi/boot0.h +8 -0

index 6a13db5..7799a03 100644
@@ -4,6 +4,33 @@
  * SPDX-License-Identifier:    GPL-2.0+
  */
 
+#if defined(CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER) && !defined(CONFIG_SPL_BUILD)
 /* reserve space for BOOT0 header information */
        b       reset
        .space  1532
+#elif defined(CONFIG_ARM_BOOT_HOOK_RMR)
+/* switch into AArch64 if needed */
+       tst     x0, x0                  // this is "b #0x84" in ARM
+       b       reset
+       .space  0x7c
+       .word   0xe59f1024      // ldr     r1, [pc, #36] ; 0x170000a0
+       .word   0xe59f0024      // ldr     r0, [pc, #36] ; CONFIG_*_TEXT_BASE

arch/arm/include/asm/arch-sunxi/boot0.h +9 -0

+#ifdef CONFIG_SPL_BUILD
+       .word   CONFIG_SPL_TEXT_BASE
+#else
+       .word   CONFIG_SYS_TEXT_BASE
+#endif
+#else
+/* normal execution */
+       b       reset
+#endif

board/sunxi/Kconfig +0 -0

index 0cd57a2..ba72e76 100644

board/sunxi/Kconfig +1 -1

+
 config DRAM_TYPE
        int "sunxi dram type"
        depends on MACH_SUN8I_A83T
--

2.8.2

Hi Andre,

Quoted 18 lines · Andre Przywara

The Allwinner A64 SoC starts execution in AArch32 mode, and both the boot ROM and Allwinner's boot0 keep running in this mode. So U-Boot gets entered in 32-bit, although we want it to run in AArch64.

By using a "magic" instruction, which happens to be an almost-NOP in AArch64 and a branch in AArch32, we differentiate between being entered in 64-bit or 32-bit mode.
If in 64-bit mode, we proceed with the branch to reset, but in 32-bit mode we trigger an RMR write to bring the core into AArch64/EL3 and re-enter U-Boot at CONFIG_SYS_TEXT_BASE.
This allows a 64-bit U-Boot to be both entered in 32 and 64-bit mode, so we can use the same start code for the SPL and the U-Boot proper.

We use the existing custom header (boot0.h) functionality, but restrict the existing boot0 header reservation to the non-SPL build now. A SPL wouldn't need such header anyway. This allows to have both options defined and lets us use one for the SPL and the other for U-Boot proper.

Quoted 5 lines

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/include/asm/arch-sunxi/boot0.h | 27 +++++++++++++++++++++++++++
 board/sunxi/Kconfig                     |  5 +++++
 2 files changed, 32 insertions(+)

Reviewed-by Simon Glass <[email protected]>

Quoted 24 lines

arch/arm/include/asm/arch-sunxi/boot0.h +12 -0

index 6a13db5..7799a03 100644
@@ -4,6 +4,33 @@
  * SPDX-License-Identifier:    GPL-2.0+
  */
 
+#if defined(CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER) && !defined(CONFIG_SPL_BUILD)
 /* reserve space for BOOT0 header information */
        b       reset
        .space  1532
+#elif defined(CONFIG_ARM_BOOT_HOOK_RMR)
+/* switch into AArch64 if needed */
+       tst     x0, x0                  // this is "b #0x84" in ARM
+       b       reset
+       .space  0x7c
+       .word   0xe59f1024      // ldr     r1, [pc, #36] ; 0x170000a0
+       .word   0xe59f0024      // ldr     r0, [pc, #36] ; CONFIG_*_TEXT_BASE
+       .word   0xe5810000      // str     r0, [r1]
+       .word   0xf57ff04f      // dsb     sy
+       .word   0xf57ff06f      // isb     sy
+       .word   0xee1c0f50      // mrc     15, 0, r0, cr12, cr0, {2} ; RMR

arch/arm/include/asm/arch-sunxi/boot0.h line 27

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/boot0.h +6 -0

+       .word   0xe3800003      // orr     r0, r0, #3
+       .word   0xee0c0f50      // mcr     15, 0, r0, cr12, cr0, {2} ; RMR
+       .word   0xf57ff06f      // isb     sy
+       .word   0xe320f003      // wfi
+       .word   0xeafffffd      // b       @wfi
+       .word   0x017000a0      // writeable RVBAR mapping address

How come you cannot use the assembler here?

Quoted 17 lines

arch/arm/include/asm/arch-sunxi/boot0.h +9 -0

+#ifdef CONFIG_SPL_BUILD
+       .word   CONFIG_SPL_TEXT_BASE
+#else
+       .word   CONFIG_SYS_TEXT_BASE
+#endif
+#else
+/* normal execution */
+       b       reset
+#endif

board/sunxi/Kconfig +0 -0

index 0cd57a2..ba72e76 100644
@@ -142,6 +142,11 @@ config RESERVE_ALLWINNER_BOOT0_HEADER
        blob relies on this information to load and execute U-Boot.
        Only needed on 64-bit Allwinner boards so far when using boot0.
 

board/sunxi/Kconfig config RESERVE_ALLWINNER_BOOT0_HEADER · line 148

Quoted 4 lines

board/sunxi/Kconfig +4 -0

+config ARM_BOOT_HOOK_RMR
+       bool
+       default y if ARM64
+       select ENABLE_ARM_SOC_BOOT0_HOOK

help?

Quoted 8 lines

board/sunxi/Kconfig +1 -1

+
 config DRAM_TYPE
        int "sunxi dram type"
        depends on MACH_SUN8I_A83T
--

2.8.2

Regards,
Simon

Hi Simon,

thanks a lot for looking at this!

Quoted 53 lines · Simon Glass

Hi Andre,

Quoted 18 lines · Andre Przywara

The Allwinner A64 SoC starts execution in AArch32 mode, and both the boot ROM and Allwinner's boot0 keep running in this mode. So U-Boot gets entered in 32-bit, although we want it to run in AArch64.

By using a "magic" instruction, which happens to be an almost-NOP in AArch64 and a branch in AArch32, we differentiate between being entered in 64-bit or 32-bit mode.
If in 64-bit mode, we proceed with the branch to reset, but in 32-bit mode we trigger an RMR write to bring the core into AArch64/EL3 and re-enter U-Boot at CONFIG_SYS_TEXT_BASE.
This allows a 64-bit U-Boot to be both entered in 32 and 64-bit mode, so we can use the same start code for the SPL and the U-Boot proper.

We use the existing custom header (boot0.h) functionality, but restrict the existing boot0 header reservation to the non-SPL build now. A SPL wouldn't need such header anyway. This allows to have both options defined and lets us use one for the SPL and the other for U-Boot proper.

Quoted 5 lines

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/include/asm/arch-sunxi/boot0.h | 27 +++++++++++++++++++++++++++
 board/sunxi/Kconfig                     |  5 +++++
 2 files changed, 32 insertions(+)

Reviewed-by Simon Glass <[email protected]>

Quoted 24 lines

arch/arm/include/asm/arch-sunxi/boot0.h +12 -0

index 6a13db5..7799a03 100644
@@ -4,6 +4,33 @@
  * SPDX-License-Identifier:    GPL-2.0+
  */
 
+#if defined(CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER) && !defined(CONFIG_SPL_BUILD)
 /* reserve space for BOOT0 header information */
        b       reset
        .space  1532
+#elif defined(CONFIG_ARM_BOOT_HOOK_RMR)
+/* switch into AArch64 if needed */
+       tst     x0, x0                  // this is "b #0x84" in ARM
+       b       reset
+       .space  0x7c
+       .word   0xe59f1024      // ldr     r1, [pc, #36] ; 0x170000a0
+       .word   0xe59f0024      // ldr     r0, [pc, #36] ; CONFIG_*_TEXT_BASE
+       .word   0xe5810000      // str     r0, [r1]
+       .word   0xf57ff04f      // dsb     sy
+       .word   0xf57ff06f      // isb     sy
+       .word   0xee1c0f50      // mrc     15, 0, r0, cr12, cr0, {2} ; RMR

Quoted 8 lines

arch/arm/include/asm/arch-sunxi/boot0.h line 27

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/boot0.h +6 -0

+       .word   0xe3800003      // orr     r0, r0, #3
+       .word   0xee0c0f50      // mcr     15, 0, r0, cr12, cr0, {2} ; RMR
+       .word   0xf57ff06f      // isb     sy
+       .word   0xe320f003      // wfi
+       .word   0xeafffffd      // b       @wfi
+       .word   0x017000a0      // writeable RVBAR mapping address

How come you cannot use the assembler here?

Because this is ARM code, whereas this file is included in an AArch64 build. In contrast to x86 the AArch64 toolchain does not support both bitnesses in one build, mostly because the two architectures are really different.

The actual reason for this exercise is that the Allwinner boot ROM enters the payload in AArch32 mode, but we want to compile and run the SPL in AArch64. So we need some small ARM(32) stub to enter AArch64.

Running the whole SPL in 32-bit is the other option which the later patches enable, but I didn't want to call some 32-bit ARM (cross-)compiler just for this handful of instructions in this case here.

To avoid enumerating the very same DRAM values in defconfig files for each and every Allwinner A64 board out there, let's put some sane default values in the Kconfig file.
Boards with different needs can override them at any time.

Signed-off-by Andre Przywara <[email protected]>

---
 board/sunxi/Kconfig           | 2 ++
 configs/pine64_plus_defconfig | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

To avoid enumerating the very same DRAM values in defconfig files for each and every Allwinner A64 board out there, let's put some sane default values in the Kconfig file.
Boards with different needs can override them at any time.

Quoted 4 lines · Andre Przywara

To avoid enumerating the very same DRAM values in defconfig files for each and every Allwinner A64 board out there, let's put some sane default values in the Kconfig file.
Boards with different needs can override them at any time.

Did you check other boards to see what their values were before calling it a default?

Hi,

Quoted 8 lines · Maxime Ripard

Quoted 4 lines · Andre Przywara

To avoid enumerating the very same DRAM values in defconfig files for each and every Allwinner A64 board out there, let's put some sane default values in the Kconfig file.
Boards with different needs can override them at any time.

Did you check other boards to see what their values were before calling it a default?

I sampled all the boards (two ;-) I have access to and got a 100% coverage ;-)
As far as I know, this 672 MHz seems to be an Allwinner recommendation, though it needs to be worked out how stable this is under load. I have no idea if the ZQ value is similarly common.

If you have any other data, I am all ears and happy to use a different safe default if this is the intention.
But we could use a "most common value" approach here to avoid pointless defconfig entries with the same value for lots of boards, so even if there are board with can't do 672 MHz, for instance, we still put it in Kconfig and let that one board override it.

From: Jens Kuske <[email protected]>

The IOCR registers got renamed to BDLR to match the public documentation of similar controllers.

Signed-off-by Jens Kuske <[email protected]>

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h | 43 ++++++++++++++-----------
 arch/arm/mach-sunxi/dram_sun8i_h3.c             | 34 +++++++++----------
 2 files changed, 41 insertions(+), 36 deletions(-)

arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h +16 -13

index d0f2b8a..867fd12 100644
@@ -81,7 +81,7 @@ struct sunxi_mctl_ctl_reg {
        u32 rfshtmg;            /* 0x90 refresh timing */
        u32 rfshctl1;           /* 0x94 */
        u32 pwrtmg;             /* 0x98 */
-       u8  res3[0x20];         /* 0x9c */
+       u8 res3[0x20];          /* 0x9c */
        u32 dqsgmr;             /* 0xbc */
        u32 dtcr;               /* 0xc0 */
        u32 dtar[4];            /* 0xc4 */

@@ -106,20 +106,23 @@ struct sunxi_mctl_ctl_reg {
        u32 perfhpr[2];         /* 0x1c4 */
        u32 perflpr[2];         /* 0x1cc */
        u32 perfwr[2];          /* 0x1d4 */
-       u8 res8[0x2c];          /* 0x1dc */
-       u32 aciocr;             /* 0x208 */
-       u8 res9[0xf4];          /* 0x20c */
+       u8 res8[0x24];          /* 0x1dc */
+       u32 acmdlr;             /* 0x200 AC master delay line register */
+       u32 aclcdlr;            /* 0x204 AC local calibrated delay line register */
+       u32 aciocr;             /* 0x208 AC I/O configuration register */
+       u8 res9[0x4];           /* 0x20c */
+       u32 acbdlr[31];         /* 0x210 AC bit delay line registers */
+       u8 res10[0x74];         /* 0x28c */
        struct {                /* 0x300 DATX8 modules*/
-               u32 mdlr;               /* 0x00 */
-               u32 lcdlr[3];           /* 0x04 */
-               u32 iocr[11];           /* 0x10 IO configuration register */
-               u32 bdlr6;              /* 0x3c */
-               u32 gtr;                /* 0x40 */
-               u32 gcr;                /* 0x44 */
-               u32 gsr[3];             /* 0x48 */
+               u32 mdlr;               /* 0x00 master delay line register */
+               u32 lcdlr[3];           /* 0x04 local calibrated delay line registers */
+               u32 bdlr[12];           /* 0x10 bit delay line registers */
+               u32 gtr;                /* 0x40 general timing register */
+               u32 gcr;                /* 0x44 general configuration register */
+               u32 gsr[3];             /* 0x48 general status registers */
                u8 res0[0x2c];          /* 0x54 */
-       } datx[4];
-       u8 res10[0x388];        /* 0x500 */
+       } dx[4];
+       u8 res11[0x388];        /* 0x500 */
        u32 upd2;               /* 0x888 */
 };
 

arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h +5 -0

+#define DXBDLR_DQS     (9)             /* DQS BDLR index */
+#define DXBDLR_DQSN    (10)            /* DQSN BDLR index */
+
+#define DXBDLR_WRITE_DELAY(x)  ((x) << 8)
+#define DXBDLR_READ_DELAY(x)   ((x) << 0)
 
 #endif /* _SUNXI_DRAM_SUN8I_H3_H */

arch/arm/mach-sunxi/dram_sun8i_h3.c +11 -11

index b08b8e6..3dd6803 100644
@@ -72,21 +72,21 @@ static void mctl_dq_delay(u32 read, u32 write)
        u32 val;
 
        for (i = 0; i < 4; i++) {
-               val = DATX_IOCR_WRITE_DELAY((write >> (i * 4)) & 0xf) |
-                     DATX_IOCR_READ_DELAY(((read >> (i * 4)) & 0xf) * 2);
+               val = DXBDLR_WRITE_DELAY((write >> (i * 4)) & 0xf) |
+                     DXBDLR_READ_DELAY(((read >> (i * 4)) & 0xf) * 2);
 
-               for (j = DATX_IOCR_DQ(0); j <= DATX_IOCR_DM; j++)
-                       writel(val, &mctl_ctl->datx[i].iocr[j]);
+               for (j = DXBDLR_DQ(0); j <= DXBDLR_DM; j++)
+                       writel(val, &mctl_ctl->dx[i].bdlr[j]);
        }
 
        clrbits_le32(&mctl_ctl->pgcr[0], 1 << 26);
 
        for (i = 0; i < 4; i++) {
-               val = DATX_IOCR_WRITE_DELAY((write >> (16 + i * 4)) & 0xf) |
-                     DATX_IOCR_READ_DELAY((read >> (16 + i * 4)) & 0xf);
+               val = DXBDLR_WRITE_DELAY((write >> (16 + i * 4)) & 0xf) |
+                     DXBDLR_READ_DELAY((read >> (16 + i * 4)) & 0xf);
 
-               writel(val, &mctl_ctl->datx[i].iocr[DATX_IOCR_DQS]);
-               writel(val, &mctl_ctl->datx[i].iocr[DATX_IOCR_DQSN]);
+               writel(val, &mctl_ctl->dx[i].bdlr[DXBDLR_DQS]);
+               writel(val, &mctl_ctl->dx[i].bdlr[DXBDLR_DQSN]);
        }
 
        setbits_le32(&mctl_ctl->pgcr[0], 1 << 26);

@@ -344,7 +344,7 @@ static int mctl_channel_init(struct dram_para *para)
 
        /* set dramc odt */
        for (i = 0; i < 4; i++)
-               clrsetbits_le32(&mctl_ctl->datx[i].gcr, (0x3 << 4) |
+               clrsetbits_le32(&mctl_ctl->dx[i].gcr, (0x3 << 4) |
                                (0x1 << 1) | (0x3 << 2) | (0x3 << 12) |
                                (0x3 << 14),
                                IS_ENABLED(CONFIG_DRAM_ODT_EN) ? 0x0 : 0x2);

@@ -364,8 +364,8 @@ static int mctl_channel_init(struct dram_para *para)
 
        /* set half DQ */
        if (para->bus_width != 32) {
-               writel(0x0, &mctl_ctl->datx[2].gcr);
-               writel(0x0, &mctl_ctl->datx[3].gcr);
+               writel(0x0, &mctl_ctl->dx[2].gcr);
+               writel(0x0, &mctl_ctl->dx[3].gcr);
        }
 
        /* data training configuration */

arch/arm/mach-sunxi/dram_sun8i_h3.c +5 -5

+                   ((readl(&mctl_ctl->dx[1].gsr[0]) >> 24) & 0x2)) {
                        clrsetbits_le32(&mctl_ctl->dtcr, 0xf << 24, 0x1 << 24);
                        para->dual_rank = 0;
                }
 
                /* only half DQ width */
-               if (((readl(&mctl_ctl->datx[2].gsr[0]) >> 24) & 0x1) ||
-                   ((readl(&mctl_ctl->datx[3].gsr[0]) >> 24) & 0x1)) {
-                       writel(0x0, &mctl_ctl->datx[2].gcr);
-                       writel(0x0, &mctl_ctl->datx[3].gcr);
+               if (((readl(&mctl_ctl->dx[2].gsr[0]) >> 24) & 0x1) ||
+                   ((readl(&mctl_ctl->dx[3].gsr[0]) >> 24) & 0x1)) {
+                       writel(0x0, &mctl_ctl->dx[2].gcr);
+                       writel(0x0, &mctl_ctl->dx[3].gcr);
                        para->bus_width = 16;
                }
 
--

2.8.2

Quoted 11 lines

From: Jens Kuske <[email protected]>

The IOCR registers got renamed to BDLR to match the public documentation of similar controllers.

Signed-off-by Jens Kuske <[email protected]>

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h | 43 ++++++++++++++-----------
 arch/arm/mach-sunxi/dram_sun8i_h3.c             | 34 +++++++++----------
 2 files changed, 41 insertions(+), 36 deletions(-)

Reviewed-by Simon Glass <[email protected]>

Some suggestions below if you have the energy.

Quoted 51 lines

arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h +16 -13

index d0f2b8a..867fd12 100644
@@ -81,7 +81,7 @@ struct sunxi_mctl_ctl_reg {
        u32 rfshtmg;            /* 0x90 refresh timing */
        u32 rfshctl1;           /* 0x94 */
        u32 pwrtmg;             /* 0x98 */
-       u8  res3[0x20];         /* 0x9c */
+       u8 res3[0x20];          /* 0x9c */
        u32 dqsgmr;             /* 0xbc */
        u32 dtcr;               /* 0xc0 */
        u32 dtar[4];            /* 0xc4 */

@@ -106,20 +106,23 @@ struct sunxi_mctl_ctl_reg {
        u32 perfhpr[2];         /* 0x1c4 */
        u32 perflpr[2];         /* 0x1cc */
        u32 perfwr[2];          /* 0x1d4 */
-       u8 res8[0x2c];          /* 0x1dc */
-       u32 aciocr;             /* 0x208 */
-       u8 res9[0xf4];          /* 0x20c */
+       u8 res8[0x24];          /* 0x1dc */
+       u32 acmdlr;             /* 0x200 AC master delay line register */
+       u32 aclcdlr;            /* 0x204 AC local calibrated delay line register */
+       u32 aciocr;             /* 0x208 AC I/O configuration register */
+       u8 res9[0x4];           /* 0x20c */
+       u32 acbdlr[31];         /* 0x210 AC bit delay line registers */
+       u8 res10[0x74];         /* 0x28c */
        struct {                /* 0x300 DATX8 modules*/
-               u32 mdlr;               /* 0x00 */
-               u32 lcdlr[3];           /* 0x04 */
-               u32 iocr[11];           /* 0x10 IO configuration register */
-               u32 bdlr6;              /* 0x3c */
-               u32 gtr;                /* 0x40 */
-               u32 gcr;                /* 0x44 */
-               u32 gsr[3];             /* 0x48 */
+               u32 mdlr;               /* 0x00 master delay line register */
+               u32 lcdlr[3];           /* 0x04 local calibrated delay line registers */
+               u32 bdlr[12];           /* 0x10 bit delay line registers */
+               u32 gtr;                /* 0x40 general timing register */
+               u32 gcr;                /* 0x44 general configuration register */
+               u32 gsr[3];             /* 0x48 general status registers */
                u8 res0[0x2c];          /* 0x54 */
-       } datx[4];
-       u8 res10[0x388];        /* 0x500 */
+       } dx[4];
+       u8 res11[0x388];        /* 0x500 */
        u32 upd2;               /* 0x888 */
 };
 

arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h struct sunxi_mctl_ctl_reg { · line 178

Quoted 2 lines

arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h +0 -0

@@ -174,12 +177,14 @@ struct sunxi_mctl_ctl_reg {
 
 #define ZQCR_PWRDOWN   (0x1 << 31)     /* ZQ power down */

1U << 31

arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h struct sunxi_mctl_ctl_reg { · line 180

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h +1 -4

-#define DATX_IOCR_DQ(x)        (x)             /* DQ0-7 IOCR index */
-#define DATX_IOCR_DM   (8)             /* DM IOCR index */
-#define DATX_IOCR_DQS  (9)             /* DQS IOCR index */
-#define DATX_IOCR_DQSN (10)            /* DQSN IOCR index */
+#define ACBDLR_WRITE_DELAY(x)  ((x) << 8)

Better to have

#define ACBDLR_WRITE_DELAY_SHIFT 8
#define ACBDLR_WRITE_DELAY_MASK (0xff << ACBDLR_WRITE_DELAY_SHIFT)

and then use that in the code. Similarly with other accessors.

arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h struct sunxi_mctl_ctl_reg { · line 183

Quoted 5 lines

arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h +2 -2

-#define DATX_IOCR_WRITE_DELAY(x)       ((x) << 8)
-#define DATX_IOCR_READ_DELAY(x)                ((x) << 0)
+#define DXBDLR_DQ(x)   (x)             /* DQ0-7 BDLR index */
+#define DXBDLR_DM      (8)             /* DM BDLR index */

Can we drop the unnecessary brackets around constants?

Quoted 60 lines

arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h +5 -0

+#define DXBDLR_DQS     (9)             /* DQS BDLR index */
+#define DXBDLR_DQSN    (10)            /* DQSN BDLR index */
+
+#define DXBDLR_WRITE_DELAY(x)  ((x) << 8)
+#define DXBDLR_READ_DELAY(x)   ((x) << 0)
 
 #endif /* _SUNXI_DRAM_SUN8I_H3_H */

arch/arm/mach-sunxi/dram_sun8i_h3.c +11 -11

index b08b8e6..3dd6803 100644
@@ -72,21 +72,21 @@ static void mctl_dq_delay(u32 read, u32 write)
        u32 val;
 
        for (i = 0; i < 4; i++) {
-               val = DATX_IOCR_WRITE_DELAY((write >> (i * 4)) & 0xf) |
-                     DATX_IOCR_READ_DELAY(((read >> (i * 4)) & 0xf) * 2);
+               val = DXBDLR_WRITE_DELAY((write >> (i * 4)) & 0xf) |
+                     DXBDLR_READ_DELAY(((read >> (i * 4)) & 0xf) * 2);
 
-               for (j = DATX_IOCR_DQ(0); j <= DATX_IOCR_DM; j++)
-                       writel(val, &mctl_ctl->datx[i].iocr[j]);
+               for (j = DXBDLR_DQ(0); j <= DXBDLR_DM; j++)
+                       writel(val, &mctl_ctl->dx[i].bdlr[j]);
        }
 
        clrbits_le32(&mctl_ctl->pgcr[0], 1 << 26);
 
        for (i = 0; i < 4; i++) {
-               val = DATX_IOCR_WRITE_DELAY((write >> (16 + i * 4)) & 0xf) |
-                     DATX_IOCR_READ_DELAY((read >> (16 + i * 4)) & 0xf);
+               val = DXBDLR_WRITE_DELAY((write >> (16 + i * 4)) & 0xf) |
+                     DXBDLR_READ_DELAY((read >> (16 + i * 4)) & 0xf);
 
-               writel(val, &mctl_ctl->datx[i].iocr[DATX_IOCR_DQS]);
-               writel(val, &mctl_ctl->datx[i].iocr[DATX_IOCR_DQSN]);
+               writel(val, &mctl_ctl->dx[i].bdlr[DXBDLR_DQS]);
+               writel(val, &mctl_ctl->dx[i].bdlr[DXBDLR_DQSN]);
        }
 
        setbits_le32(&mctl_ctl->pgcr[0], 1 << 26);

@@ -344,7 +344,7 @@ static int mctl_channel_init(struct dram_para *para)
 
        /* set dramc odt */
        for (i = 0; i < 4; i++)
-               clrsetbits_le32(&mctl_ctl->datx[i].gcr, (0x3 << 4) |
+               clrsetbits_le32(&mctl_ctl->dx[i].gcr, (0x3 << 4) |
                                (0x1 << 1) | (0x3 << 2) | (0x3 << 12) |
                                (0x3 << 14),
                                IS_ENABLED(CONFIG_DRAM_ODT_EN) ? 0x0 : 0x2);

@@ -364,8 +364,8 @@ static int mctl_channel_init(struct dram_para *para)
 
        /* set half DQ */
        if (para->bus_width != 32) {
-               writel(0x0, &mctl_ctl->datx[2].gcr);
-               writel(0x0, &mctl_ctl->datx[3].gcr);
+               writel(0x0, &mctl_ctl->dx[2].gcr);
+               writel(0x0, &mctl_ctl->dx[3].gcr);
        }
 

arch/arm/mach-sunxi/dram_sun8i_h3.c static int mctl_channel_init(struct dram_para *para) · line 389

Quoted 8 lines

arch/arm/mach-sunxi/dram_sun8i_h3.c +1 -2

        /* data training configuration */

@@ -386,17 +386,17 @@ static int mctl_channel_init(struct dram_para *para)
        /* detect ranks and bus width */
        if (readl(&mctl_ctl->pgsr[0]) & (0xfe << 20)) {
                /* only one rank */
-               if (((readl(&mctl_ctl->datx[0].gsr[0]) >> 24) & 0x2) ||
-                   ((readl(&mctl_ctl->datx[1].gsr[0]) >> 24) & 0x2)) {
+               if (((readl(&mctl_ctl->dx[0].gsr[0]) >> 24) & 0x2) ||

Looks like these fields should have #defines also.

Quoted 18 lines

arch/arm/mach-sunxi/dram_sun8i_h3.c +5 -5

+                   ((readl(&mctl_ctl->dx[1].gsr[0]) >> 24) & 0x2)) {
                        clrsetbits_le32(&mctl_ctl->dtcr, 0xf << 24, 0x1 << 24);
                        para->dual_rank = 0;
                }
 
                /* only half DQ width */
-               if (((readl(&mctl_ctl->datx[2].gsr[0]) >> 24) & 0x1) ||
-                   ((readl(&mctl_ctl->datx[3].gsr[0]) >> 24) & 0x1)) {
-                       writel(0x0, &mctl_ctl->datx[2].gcr);
-                       writel(0x0, &mctl_ctl->datx[3].gcr);
+               if (((readl(&mctl_ctl->dx[2].gsr[0]) >> 24) & 0x1) ||
+                   ((readl(&mctl_ctl->dx[3].gsr[0]) >> 24) & 0x1)) {
+                       writel(0x0, &mctl_ctl->dx[2].gcr);
+                       writel(0x0, &mctl_ctl->dx[3].gcr);
                        para->bus_width = 16;
                }
 
--

Quoted 2 lines

2.8.2

Regards,
Simon

From: Jens Kuske <[email protected]>

The IOCR registers got renamed to BDLR to match the public documentation of similar controllers.

Signed-off-by Jens Kuske <[email protected]>

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h | 43 ++++++++++++++-----------
 arch/arm/mach-sunxi/dram_sun8i_h3.c             | 34 +++++++++----------
 2 files changed, 41 insertions(+), 36 deletions(-)

arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h +0 -0

index d0f2b8a..867fd12 100644

From: Jens Kuske <[email protected]>

Instead of setting the delay for whole bytes allow setting it for each individual bit. Also add support for address/command lane delays.

Signed-off-by Jens Kuske <[email protected]>

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/mach-sunxi/dram_sun8i_h3.c | 54 ++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

arch/arm/mach-sunxi/dram_sun8i_h3.c +0 -0

index 3dd6803..1647d76 100644

arch/arm/mach-sunxi/dram_sun8i_h3.c +26 -26

+       const u8 dx_write_delays[4][11];
+       const u8 ac_delays[31];
 };
 
 static inline int ns_to_t(int nanoseconds)

@@ -64,34 +65,25 @@ static void mctl_phy_init(u32 val)
        mctl_await_completion(&mctl_ctl->pgsr[0], PGSR_INIT_DONE, 0x1);
 }
 
-static void mctl_dq_delay(u32 read, u32 write)
+static void mctl_set_bit_delays(struct dram_para *para)
 {
        struct sunxi_mctl_ctl_reg * const mctl_ctl =
                        (struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
        int i, j;
-       u32 val;
-
-       for (i = 0; i < 4; i++) {
-               val = DXBDLR_WRITE_DELAY((write >> (i * 4)) & 0xf) |
-                     DXBDLR_READ_DELAY(((read >> (i * 4)) & 0xf) * 2);
-
-               for (j = DXBDLR_DQ(0); j <= DXBDLR_DM; j++)
-                       writel(val, &mctl_ctl->dx[i].bdlr[j]);
-       }
 
        clrbits_le32(&mctl_ctl->pgcr[0], 1 << 26);
 
-       for (i = 0; i < 4; i++) {
-               val = DXBDLR_WRITE_DELAY((write >> (16 + i * 4)) & 0xf) |
-                     DXBDLR_READ_DELAY((read >> (16 + i * 4)) & 0xf);
+       for (i = 0; i < 4; i++)
+               for (j = 0; j < 11; j++)
+                       writel(DXBDLR_WRITE_DELAY(para->dx_write_delays[i][j]) |
+                              DXBDLR_READ_DELAY(para->dx_read_delays[i][j]),
+                              &mctl_ctl->dx[i].bdlr[j]);
 
-               writel(val, &mctl_ctl->dx[i].bdlr[DXBDLR_DQS]);
-               writel(val, &mctl_ctl->dx[i].bdlr[DXBDLR_DQSN]);
-       }
+       for (i = 0; i < 31; i++)
+               writel(ACBDLR_WRITE_DELAY(para->ac_delays[i]),
+                      &mctl_ctl->acbdlr[i]);
 
        setbits_le32(&mctl_ctl->pgcr[0], 1 << 26);
-
-       udelay(1);
 }
 
 static void mctl_set_master_priority(void)

@@ -372,11 +364,8 @@ static int mctl_channel_init(struct dram_para *para)
        clrsetbits_le32(&mctl_ctl->dtcr, 0xf << 24,
                        (para->dual_rank ? 0x3 : 0x1) << 24);
 
-
-       if (para->read_delays || para->write_delays) {
-               mctl_dq_delay(para->read_delays, para->write_delays);
-               udelay(50);
-       }
+       mctl_set_bit_delays(para);
+       udelay(50);
 
        mctl_zq_calibration(para);
 

@@ -458,12 +447,23 @@ unsigned long sunxi_dram_init(void)
                        (struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
 
        struct dram_para para = {
-               .read_delays = 0x00007979,      /* dram_tpr12 */
-               .write_delays = 0x6aaa0000,     /* dram_tpr11 */
                .dual_rank = 0,
                .bus_width = 32,
                .row_bits = 15,
                .page_size = 4096,
+
+               .dx_read_delays =  {{ 18, 18, 18, 18, 18, 18, 18, 18, 18,  0,  0 },
+                                   { 14, 14, 14, 14, 14, 14, 14, 14, 14,  0,  0 },
+                                   { 18, 18, 18, 18, 18, 18, 18, 18, 18,  0,  0 },
+                                   { 14, 14, 14, 14, 14, 14, 14, 14, 14,  0,  0 }},
+               .dx_write_delays = {{  0,  0,  0,  0,  0,  0,  0,  0,  0, 10, 10 },
+                                   {  0,  0,  0,  0,  0,  0,  0,  0,  0, 10, 10 },
+                                   {  0,  0,  0,  0,  0,  0,  0,  0,  0, 10, 10 },
+                                   {  0,  0,  0,  0,  0,  0,  0,  0,  0,  6,  6 }},
+               .ac_delays = {  0,  0,  0,  0,  0,  0,  0,  0,
+                               0,  0,  0,  0,  0,  0,  0,  0,
+                               0,  0,  0,  0,  0,  0,  0,  0,
+                               0,  0,  0,  0,  0,  0,  0      },
        };
 
        mctl_sys_init(&para);
--

2.8.2

Hi Andre,

Quoted 11 lines · Andre Przywara

From: Jens Kuske <[email protected]>

Instead of setting the delay for whole bytes allow setting it for each individual bit. Also add support for address/command lane delays.

Signed-off-by Jens Kuske <[email protected]>

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/mach-sunxi/dram_sun8i_h3.c | 54 ++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

ACBDLR_WRITE_DELAY_SHIFT

Quoted 6 lines

arch/arm/mach-sunxi/dram_sun8i_h3.c +0 -0

index 3dd6803..1647d76 100644

arch/arm/mach-sunxi/dram_sun8i_h3.c line 23

Quoted 10 lines

arch/arm/mach-sunxi/dram_sun8i_h3.c +1 -2

@@ -16,12 +16,13 @@
 #include <linux/kconfig.h>
 
 struct dram_para {
-       u32 read_delays;
-       u32 write_delays;
        u16 page_size;
        u8 bus_width;
        u8 dual_rank;
        u8 row_bits;
+       const u8 dx_read_delays[4][11];

Can we have #defines for 4 and 11?

Quoted 88 lines

arch/arm/mach-sunxi/dram_sun8i_h3.c +26 -25

+       const u8 dx_write_delays[4][11];
+       const u8 ac_delays[31];
 };
 
 static inline int ns_to_t(int nanoseconds)

@@ -64,34 +65,25 @@ static void mctl_phy_init(u32 val)
        mctl_await_completion(&mctl_ctl->pgsr[0], PGSR_INIT_DONE, 0x1);
 }
 
-static void mctl_dq_delay(u32 read, u32 write)
+static void mctl_set_bit_delays(struct dram_para *para)
 {
        struct sunxi_mctl_ctl_reg * const mctl_ctl =
                        (struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
        int i, j;
-       u32 val;
-
-       for (i = 0; i < 4; i++) {
-               val = DXBDLR_WRITE_DELAY((write >> (i * 4)) & 0xf) |
-                     DXBDLR_READ_DELAY(((read >> (i * 4)) & 0xf) * 2);
-
-               for (j = DXBDLR_DQ(0); j <= DXBDLR_DM; j++)
-                       writel(val, &mctl_ctl->dx[i].bdlr[j]);
-       }
 
        clrbits_le32(&mctl_ctl->pgcr[0], 1 << 26);
 
-       for (i = 0; i < 4; i++) {
-               val = DXBDLR_WRITE_DELAY((write >> (16 + i * 4)) & 0xf) |
-                     DXBDLR_READ_DELAY((read >> (16 + i * 4)) & 0xf);
+       for (i = 0; i < 4; i++)
+               for (j = 0; j < 11; j++)
+                       writel(DXBDLR_WRITE_DELAY(para->dx_write_delays[i][j]) |
+                              DXBDLR_READ_DELAY(para->dx_read_delays[i][j]),
+                              &mctl_ctl->dx[i].bdlr[j]);
 
-               writel(val, &mctl_ctl->dx[i].bdlr[DXBDLR_DQS]);
-               writel(val, &mctl_ctl->dx[i].bdlr[DXBDLR_DQSN]);
-       }
+       for (i = 0; i < 31; i++)
+               writel(ACBDLR_WRITE_DELAY(para->ac_delays[i]),
+                      &mctl_ctl->acbdlr[i]);
 
        setbits_le32(&mctl_ctl->pgcr[0], 1 << 26);
-
-       udelay(1);
 }
 
 static void mctl_set_master_priority(void)

@@ -372,11 +364,8 @@ static int mctl_channel_init(struct dram_para *para)
        clrsetbits_le32(&mctl_ctl->dtcr, 0xf << 24,
                        (para->dual_rank ? 0x3 : 0x1) << 24);
 
-
-       if (para->read_delays || para->write_delays) {
-               mctl_dq_delay(para->read_delays, para->write_delays);
-               udelay(50);
-       }
+       mctl_set_bit_delays(para);
+       udelay(50);
 
        mctl_zq_calibration(para);
 

@@ -458,12 +447,23 @@ unsigned long sunxi_dram_init(void)
                        (struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
 
        struct dram_para para = {
-               .read_delays = 0x00007979,      /* dram_tpr12 */
-               .write_delays = 0x6aaa0000,     /* dram_tpr11 */
                .dual_rank = 0,
                .bus_width = 32,
                .row_bits = 15,
                .page_size = 4096,
+
+               .dx_read_delays =  {{ 18, 18, 18, 18, 18, 18, 18, 18, 18,  0,  0 },
+                                   { 14, 14, 14, 14, 14, 14, 14, 14, 14,  0,  0 },
+                                   { 18, 18, 18, 18, 18, 18, 18, 18, 18,  0,  0 },
+                                   { 14, 14, 14, 14, 14, 14, 14, 14, 14,  0,  0 }},
+               .dx_write_delays = {{  0,  0,  0,  0,  0,  0,  0,  0,  0, 10, 10 },
+                                   {  0,  0,  0,  0,  0,  0,  0,  0,  0, 10, 10 },
+                                   {  0,  0,  0,  0,  0,  0,  0,  0,  0, 10, 10 },
+                                   {  0,  0,  0,  0,  0,  0,  0,  0,  0,  6,  6 }},
+               .ac_delays = {  0,  0,  0,  0,  0,  0,  0,  0,
+                               0,  0,  0,  0,  0,  0,  0,  0,
+                               0,  0,  0,  0,  0,  0,  0,  0,
+                               0,  0,  0,  0,  0,  0,  0      },
        };
 

Quoted 4 lines

arch/arm/mach-sunxi/dram_sun8i_h3.c +0 -1

        mctl_sys_init(&para);
--

2.8.2

I wonder if there is value in moving this to device tree with of-platdata?

Quoted 124 lines · Simon Glass

Hi Andre,

Quoted 11 lines · Andre Przywara

From: Jens Kuske <[email protected]>

Instead of setting the delay for whole bytes allow setting it for each individual bit. Also add support for address/command lane delays.

Signed-off-by Jens Kuske <[email protected]>

Signed-off-by Andre Przywara <[email protected]>

---
 arch/arm/mach-sunxi/dram_sun8i_h3.c | 54 ++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

ACBDLR_WRITE_DELAY_SHIFT

Quoted 6 lines

arch/arm/mach-sunxi/dram_sun8i_h3.c +0 -0

index 3dd6803..1647d76 100644

arch/arm/mach-sunxi/dram_sun8i_h3.c line 23

Quoted 10 lines

arch/arm/mach-sunxi/dram_sun8i_h3.c +1 -2

@@ -16,12 +16,13 @@
 #include <linux/kconfig.h>
 
 struct dram_para {
-       u32 read_delays;
-       u32 write_delays;
        u16 page_size;
        u8 bus_width;
        u8 dual_rank;
        u8 row_bits;
+       const u8 dx_read_delays[4][11];

Can we have #defines for 4 and 11?

Quoted 88 lines

arch/arm/mach-sunxi/dram_sun8i_h3.c +26 -25

+       const u8 dx_write_delays[4][11];
+       const u8 ac_delays[31];
 };
 
 static inline int ns_to_t(int nanoseconds)

@@ -64,34 +65,25 @@ static void mctl_phy_init(u32 val)
        mctl_await_completion(&mctl_ctl->pgsr[0], PGSR_INIT_DONE, 0x1);
 }
 
-static void mctl_dq_delay(u32 read, u32 write)
+static void mctl_set_bit_delays(struct dram_para *para)
 {
        struct sunxi_mctl_ctl_reg * const mctl_ctl =
                        (struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
        int i, j;
-       u32 val;
-
-       for (i = 0; i < 4; i++) {
-               val = DXBDLR_WRITE_DELAY((write >> (i * 4)) & 0xf) |
-                     DXBDLR_READ_DELAY(((read >> (i * 4)) & 0xf) * 2);
-
-               for (j = DXBDLR_DQ(0); j <= DXBDLR_DM; j++)
-                       writel(val, &mctl_ctl->dx[i].bdlr[j]);
-       }
 
        clrbits_le32(&mctl_ctl->pgcr[0], 1 << 26);
 
-       for (i = 0; i < 4; i++) {
-               val = DXBDLR_WRITE_DELAY((write >> (16 + i * 4)) & 0xf) |
-                     DXBDLR_READ_DELAY((read >> (16 + i * 4)) & 0xf);
+       for (i = 0; i < 4; i++)
+               for (j = 0; j < 11; j++)
+                       writel(DXBDLR_WRITE_DELAY(para->dx_write_delays[i][j]) |
+                              DXBDLR_READ_DELAY(para->dx_read_delays[i][j]),
+                              &mctl_ctl->dx[i].bdlr[j]);
 
-               writel(val, &mctl_ctl->dx[i].bdlr[DXBDLR_DQS]);
-               writel(val, &mctl_ctl->dx[i].bdlr[DXBDLR_DQSN]);
-       }
+       for (i = 0; i < 31; i++)
+               writel(ACBDLR_WRITE_DELAY(para->ac_delays[i]),
+                      &mctl_ctl->acbdlr[i]);
 
        setbits_le32(&mctl_ctl->pgcr[0], 1 << 26);
-
-       udelay(1);
 }
 
 static void mctl_set_master_priority(void)

@@ -372,11 +364,8 @@ static int mctl_channel_init(struct dram_para *para)
        clrsetbits_le32(&mctl_ctl->dtcr, 0xf << 24,
                        (para->dual_rank ? 0x3 : 0x1) << 24);
 
-
-       if (para->read_delays || para->write_delays) {
-               mctl_dq_delay(para->read_delays, para->write_delays);
-               udelay(50);
-       }
+       mctl_set_bit_delays(para);
+       udelay(50);
 
        mctl_zq_calibration(para);
 

@@ -458,12 +447,23 @@ unsigned long sunxi_dram_init(void)
                        (struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
 
        struct dram_para para = {
-               .read_delays = 0x00007979,      /* dram_tpr12 */
-               .write_delays = 0x6aaa0000,     /* dram_tpr11 */
                .dual_rank = 0,
                .bus_width = 32,
                .row_bits = 15,
                .page_size = 4096,
+
+               .dx_read_delays =  {{ 18, 18, 18, 18, 18, 18, 18, 18, 18,  0,  0 },
+                                   { 14, 14, 14, 14, 14, 14, 14, 14, 14,  0,  0 },
+                                   { 18, 18, 18, 18, 18, 18, 18, 18, 18,  0,  0 },
+                                   { 14, 14, 14, 14, 14, 14, 14, 14, 14,  0,  0 }},
+               .dx_write_delays = {{  0,  0,  0,  0,  0,  0,  0,  0,  0, 10, 10 },
+                                   {  0,  0,  0,  0,  0,  0,  0,  0,  0, 10, 10 },
+                                   {  0,  0,  0,  0,  0,  0,  0,  0,  0, 10, 10 },
+                                   {  0,  0,  0,  0,  0,  0,  0,  0,  0,  6,  6 }},
+               .ac_delays = {  0,  0,  0,  0,  0,  0,  0,  0,
+                               0,  0,  0,  0,  0,  0,  0,  0,
+                               0,  0,  0,  0,  0,  0,  0,  0,
+                               0,  0,  0,  0,  0,  0,  0      },
        };
 

Quoted 6 lines

Quoted 4 lines

arch/arm/mach-sunxi/dram_sun8i_h3.c +0 -1

        mctl_sys_init(&para);
--

2.8.2

I wonder if there is value in moving this to device tree with of-platdata?

I think device tree support is unlikely to fit in SPL for sunxi. IIRC Andre already mentions the space constraints in his cover letter.

Quoted 1 line

Quoted 1 line

I wonder if there is value in moving this to device tree with of-platdata?

While I kind of like the idea of using the DT for this, there are some issues:

1) There is no binding so far for representing the DRAM data. Given the lacking documentation for the DRAM controller it sounds very hard to come up with a good binding anyway. Also we can't push this through the Linux DT binding review, since this is of no interest to the kernel. And I'd rather avoid making up some dodgy binding just for this.

There is work underway to improve the DRAM init code and make it more robust and flexible. Ideally we can use some autodetection and calibration feature the controller offers to get rid of arbitrary magic numbers. But this is quite some work ahead and shouldn't block the much sought after A64 SPL support for now.

2) If there is need, we can detect the SoC easily by reading the ID register and differentiate at runtime. This is probably less code than pulling in DT bits, also more robust.

Quoted 2 lines

I think device tree support is unlikely to fit in SPL for sunxi. IIRC Andre already mentions the space constraints in his cover letter.

3) Yes, adding DT support for the SPL makes it rather big. I think it breaks the 28K limit that the mksunxiboot tool currently has. This can (and will) be fixed later, but just for this exercise I'd rather keep it small, especially as we would use it only for the DRAM code and not for the device drivers.

Actually I have a plan to make better use of DT, but not for the SPL. To a good degree the SPL code mimics the on-SoC boot ROM operation (accessing storage devices to load code), which has to work with every board already and thus does not need a board specific DT.
I can elaborate on that if there is interest.

Hi Andre,

[...]

Quoted 24 lines

Quoted 1 line

Quoted 1 line

I wonder if there is value in moving this to device tree with of-platdata?

While I kind of like the idea of using the DT for this, there are some issues:

1) There is no binding so far for representing the DRAM data. Given the lacking documentation for the DRAM controller it sounds very hard to come up with a good binding anyway. Also we can't push this through the Linux DT binding review, since this is of no interest to the kernel. And I'd rather avoid making up some dodgy binding just for this.

There is work underway to improve the DRAM init code and make it more robust and flexible. Ideally we can use some autodetection and calibration feature the controller offers to get rid of arbitrary magic numbers. But this is quite some work ahead and shouldn't block the much sought after A64 SPL support for now.

2) If there is need, we can detect the SoC easily by reading the ID register and differentiate at runtime. This is probably less code than pulling in DT bits, also more robust.

Quoted 2 lines

I think device tree support is unlikely to fit in SPL for sunxi. IIRC Andre already mentions the space constraints in his cover letter.

Quoted 5 lines

3) Yes, adding DT support for the SPL makes it rather big. I think it breaks the 28K limit that the mksunxiboot tool currently has. This can (and will) be fixed later, but just for this exercise I'd rather keep it small, especially as we would use it only for the DRAM code and not for the device drivers.

Take a look at rk3288-firefly if you like. It has an ad-hoc device tree binding (no one has the energy to try to get this into Linux :-).

With of-platdata, DT support doesn't actually add any space (or at least very little). There is no libfdt and the only code is that needed to copy data from the of-platdata struct to the normal one.

That said, there has to be a benefit, and it's much more desirable to spend the time on this IMO:

Quoted 6 lines

Actually I have a plan to make better use of DT, but not for the SPL. To a good degree the SPL code mimics the on-SoC boot ROM operation (accessing storage devices to load code), which has to work with every board already and thus does not need a board specific DT.
I can elaborate on that if there is interest.