sunxi: Allwinner A64: SPL support

Somehow an int returning function without a return statement sneaked in, fix it.
Also fix some whitespace damage on the way.

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

---
 drivers/mtd/spi/sunxi_spi_spl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/mtd/spi/sunxi_spi_spl.c +0 -0

index 67c7edd..7502314 100644

Somehow an int returning function without a return statement sneaked in, fix it.
Also fix some whitespace damage on the way.

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

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 setting up the PRCM PLL_CTLR1 register to provide the proper voltage seems to be an A31-only feature as well.
So restrict the initialization to this SoC only to avoid writing bogus values to (undefined) registers in other chips.

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

Reviewed-by Alexander Graf <[email protected]>

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

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 setting up the PRCM PLL_CTLR1 register to provide the proper voltage seems to be an A31-only feature as well.
So restrict the initialization to this SoC only to avoid writing bogus values to (undefined) registers in other chips.

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 | 5 +++++
 1 file changed, 5 insertions(+)

arch/arm/mach-sunxi/clock_sun6i.c +2 -0

index ed8cd9b..382fa94 100644
@@ -21,6 +21,8 @@ void clock_init_safe(void)
 {
 	struct sunxi_ccm_reg * const ccm =
 		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+
+#ifdef CONFIG_MACH_SUN6I
 	struct sunxi_prcm_reg * const prcm =
 		(struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
 

Quoted 27 lines · On Sun, 20 Nov 2016 14:56:56 +0000 Andre Przywara <[email protected]>…

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 setting up the PRCM PLL_CTLR1 register to provide the proper voltage seems to be an A31-only feature as well.
So restrict the initialization to this SoC only to avoid writing bogus values to (undefined) registers in other chips.

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 | 5 +++++
 1 file changed, 5 insertions(+)

arch/arm/mach-sunxi/clock_sun6i.c +2 -0

index ed8cd9b..382fa94 100644
@@ -21,6 +21,8 @@ void clock_init_safe(void)
 {
 	struct sunxi_ccm_reg * const ccm =
 		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+
+#ifdef CONFIG_MACH_SUN6I
 	struct sunxi_prcm_reg * const prcm =
 		(struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
 

arch/arm/mach-sunxi/clock_sun6i.c void clock_init_safe(void) · line 36

Quoted 5 lines

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

@@ -31,6 +33,7 @@ void clock_init_safe(void)
 		PRCM_PLL_CTRL_LDO_DIGITAL_EN | PRCM_PLL_CTRL_LDO_ANALOG_EN |
 		PRCM_PLL_CTRL_EXT_OSC_EN | PRCM_PLL_CTRL_LDO_OUT_L(1140));
 	clrbits_le32(&prcm->pll_ctrl1, PRCM_PLL_CTRL_LDO_KEY_MASK);
+#endif

PRCM is generally poorly documented, with its description sometimes entirely missing from the Allwinner manuals (while it exists in the hardware). But many SoC variants are sharing the same features and need the same code. I can confirm that this code chunk is needed on my A31s tablet (otherwise the system does not boot), so it was not entirely useless.

What about the other SoC variants? For example, I can see that the A23 manual documents this register in a roughly the same way as A31 (the PLLVDD voltage settings, etc.). But I don't have any A23 hardware to test. Basically, are we sure that we will not break A23 support with this commit?

If I understand it correctly, you primarily wanted to disable this code on A64. But disabling it everywhere other than A31 may be a bit too broad.

arch/arm/mach-sunxi/clock_sun6i.c void clock_init_safe(void) · line 49

Quoted 10 lines

arch/arm/mach-sunxi/clock_sun6i.c +2 -0

 	clock_set_pll1(408000000);
 

@@ -41,7 +44,9 @@ void clock_init_safe(void)
 	writel(AHB1_ABP1_DIV_DEFAULT, &ccm->ahb1_apb1_div);
 
 	writel(MBUS_CLK_DEFAULT, &ccm->mbus0_clk_cfg);
+#ifdef CONFIG_MACH_SUN6I
 	writel(MBUS_CLK_DEFAULT, &ccm->mbus1_clk_cfg);
+#endif

We can change this to:

       if (IS_ENABLED(CONFIG_MACH_SUN6I))
           writel(MBUS_CLK_DEFAULT, &ccm->mbus1_clk_cfg);

This saves one line of code and also looks a bit less ugly.

Hi,

Quoted 30 lines · Siarhei Siamashka

Quoted 27 lines · On Sun, 20 Nov 2016 14:56:56 +0000 Andre Przywara <[email protected]>…

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 setting up the PRCM PLL_CTLR1 register to provide the proper voltage seems to be an A31-only feature as well.
So restrict the initialization to this SoC only to avoid writing bogus values to (undefined) registers in other chips.

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 | 5 +++++
 1 file changed, 5 insertions(+)

arch/arm/mach-sunxi/clock_sun6i.c +2 -0

index ed8cd9b..382fa94 100644
@@ -21,6 +21,8 @@ void clock_init_safe(void)
 {
 	struct sunxi_ccm_reg * const ccm =
 		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
+
+#ifdef CONFIG_MACH_SUN6I
 	struct sunxi_prcm_reg * const prcm =
 		(struct sunxi_prcm_reg *)SUNXI_PRCM_BASE;
 

Quoted 12 lines

arch/arm/mach-sunxi/clock_sun6i.c void clock_init_safe(void) · line 36

Quoted 5 lines

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

@@ -31,6 +33,7 @@ void clock_init_safe(void)
 		PRCM_PLL_CTRL_LDO_DIGITAL_EN | PRCM_PLL_CTRL_LDO_ANALOG_EN |
 		PRCM_PLL_CTRL_EXT_OSC_EN | PRCM_PLL_CTRL_LDO_OUT_L(1140));
 	clrbits_le32(&prcm->pll_ctrl1, PRCM_PLL_CTRL_LDO_KEY_MASK);
+#endif

PRCM is generally poorly documented, with its description sometimes entirely missing from the Allwinner manuals (while it exists in the hardware). But many SoC variants are sharing the same features and need the same code. I can confirm that this code chunk is needed on my A31s tablet (otherwise the system does not boot), so it was not entirely useless.

Sure, in fact I was hoping for people to holler if it breaks their board.

Quoted 9 lines

What about the other SoC variants? For example, I can see that the A23 manual documents this register in a roughly the same way as A31 (the PLLVDD voltage settings, etc.). But I don't have any A23 hardware to test. Basically, are we sure that we will not break A23 support with this commit?

If I understand it correctly, you primarily wanted to disable this code on A64. But disabling it everywhere other than A31 may be a bit too broad.

Well, my impression was that this code was added for the A31, and just called clock_sun6i.c because it made sense at this time. Later on people just re-used the _clock_ code because the clocks are compatible, but missed this one - which cares about a regulator, really.
So if people can come up with a list of Socs that need this, I am happy to add this to the #ifdef. I just had the impression that boards with AXPs or I2C regulators don't need this.

Quoted 14 lines

arch/arm/mach-sunxi/clock_sun6i.c void clock_init_safe(void) · line 49

Quoted 11 lines

arch/arm/mach-sunxi/clock_sun6i.c +2 -0

 	clock_set_pll1(408000000);
 

@@ -41,7 +44,9 @@ void clock_init_safe(void)
 	writel(AHB1_ABP1_DIV_DEFAULT, &ccm->ahb1_apb1_div);
 
 	writel(MBUS_CLK_DEFAULT, &ccm->mbus0_clk_cfg);
+#ifdef CONFIG_MACH_SUN6I
 	writel(MBUS_CLK_DEFAULT, &ccm->mbus1_clk_cfg);
+#endif

We can change this to:

Quoted 4 lines

       if (IS_ENABLED(CONFIG_MACH_SUN6I))
           writel(MBUS_CLK_DEFAULT, &ccm->mbus1_clk_cfg);

This saves one line of code and also looks a bit less ugly.

Is there some "official" rationale for using IS_ENABLED vs. #ifdef? As much as I dislike this massive usage of #ifdefs, at least it gives me clear heads up that this code may not be compiled in, which can more easily be missed with IS_ENABLED.
But I don't have a strong opinion on this, so happy to change it.

Cheers,
Andre.

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

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

arch/arm/cpu/armv8/Makefile +1 -0

index dea1465..799a752 100644
@@ -25,3 +25,4 @@ obj-$(CONFIG_FSL_LAYERSCAPE) += fsl-layerscape/
 obj-$(CONFIG_S32V234) += s32v234/
 obj-$(CONFIG_ARCH_ZYNQMP) += zynqmp/
 obj-$(CONFIG_TARGET_HIKEY) += hisilicon/
+obj-$(CONFIG_ARCH_SUNXI) += lowlevel_init.o

arch/arm/cpu/armv8/lowlevel_init.S new file +29 -0

new file mode 100644
index 0000000..189e35f
@@ -0,0 +1,44 @@
+/*
+ * A lowlevel_init function that sets up the stack to call a C function to
+ * perform further init.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <asm-offsets.h>
+#include <config.h>
+#include <linux/linkage.h>
+
+ENTRY(lowlevel_init)
+	/*
+	 * Setup a temporary stack. Global data is not available yet.
+	 */
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
+	ldr	w0, =CONFIG_SPL_STACK
+#else
+	ldr	w0, =CONFIG_SYS_INIT_SP_ADDR
+#endif
+	bic	sp, x0, #0xf	/* 16-byte alignment for ABI compliance */
+
+	/*
+	 * Save the old LR(passed in x29) and the current LR to stack
+	 */
+	stp	x29, x30, [sp, #-16]!
+
+	/*
+	 * Call the very early init function. This should do only the

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

Isn't that what _main in ./arch/arm/lib/crt0_64.S is supposed to do? That should be used for the SPL flow too, no?

Quoted 58 lines

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

arch/arm/cpu/armv8/Makefile +1 -0

index dea1465..799a752 100644
@@ -25,3 +25,4 @@ obj-$(CONFIG_FSL_LAYERSCAPE) += fsl-layerscape/
 obj-$(CONFIG_S32V234) += s32v234/
 obj-$(CONFIG_ARCH_ZYNQMP) += zynqmp/
 obj-$(CONFIG_TARGET_HIKEY) += hisilicon/
+obj-$(CONFIG_ARCH_SUNXI) += lowlevel_init.o

arch/arm/cpu/armv8/lowlevel_init.S new file +2 -0

new file mode 100644
index 0000000..189e35f
@@ -0,0 +1,44 @@
+/*
+ * A lowlevel_init function that sets up the stack to call a C

function to
+ * perform further init.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */

arch/arm/cpu/armv8/lowlevel_init.S new file +28 -0

+
+#include <asm-offsets.h>
+#include <config.h>
+#include <linux/linkage.h>
+
+ENTRY(lowlevel_init)
+    /*
+     * Setup a temporary stack. Global data is not available yet.
+     */
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
+    ldr    w0, =CONFIG_SPL_STACK
+#else
+    ldr    w0, =CONFIG_SYS_INIT_SP_ADDR
+#endif
+    bic    sp, x0, #0xf    /* 16-byte alignment for ABI compliance */
+
+    /*
+     * Save the old LR(passed in x29) and the current LR to stack
+     */
+    stp    x29, x30, [sp, #-16]!
+
+    /*
+     * Call the very early init function. This should do only the
+     * absolute bare minimum to get started. It should not:
+     *
+     * - set up DRAM
+     * - use global_data
+     * - clear BSS

arch/arm/cpu/armv8/lowlevel_init.S line 39

Quoted 5 lines

arch/arm/cpu/armv8/lowlevel_init.S new file +5 -0

+     * - try to start a console
+     *
+     * For boards with SPL this should be empty since SPL can do all of
+     * this init in the SPL board_init_f() function which is called
+     * immediately after this.

So this comment says that s_init shouldn't even be used for SPL and instead we should use board_init_f which is what crt0 calls.

Hi Alex,

thanks for having a look!

Quoted 9 lines · Alexander Graf

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

Isn't that what _main in ./arch/arm/lib/crt0_64.S is supposed to do? That should be used for the SPL flow too, no?

I saw that too, but apparently this one here is a some shortcut for the SPL. TBH I didn't dare to look too closely, as this seems to be a bit entangled.

....

Quoted 58 lines

Quoted 58 lines

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

arch/arm/cpu/armv8/Makefile +1 -0

index dea1465..799a752 100644
@@ -25,3 +25,4 @@ obj-$(CONFIG_FSL_LAYERSCAPE) += fsl-layerscape/
 obj-$(CONFIG_S32V234) += s32v234/
 obj-$(CONFIG_ARCH_ZYNQMP) += zynqmp/
 obj-$(CONFIG_TARGET_HIKEY) += hisilicon/
+obj-$(CONFIG_ARCH_SUNXI) += lowlevel_init.o

arch/arm/cpu/armv8/lowlevel_init.S new file +2 -0

new file mode 100644
index 0000000..189e35f
@@ -0,0 +1,44 @@
+/*
+ * A lowlevel_init function that sets up the stack to call a C

function to
+ * perform further init.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */

arch/arm/cpu/armv8/lowlevel_init.S new file +28 -0

+
+#include <asm-offsets.h>
+#include <config.h>
+#include <linux/linkage.h>
+
+ENTRY(lowlevel_init)
+    /*
+     * Setup a temporary stack. Global data is not available yet.
+     */
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
+    ldr    w0, =CONFIG_SPL_STACK
+#else
+    ldr    w0, =CONFIG_SYS_INIT_SP_ADDR
+#endif
+    bic    sp, x0, #0xf    /* 16-byte alignment for ABI compliance */
+
+    /*
+     * Save the old LR(passed in x29) and the current LR to stack
+     */
+    stp    x29, x30, [sp, #-16]!
+
+    /*
+     * Call the very early init function. This should do only the
+     * absolute bare minimum to get started. It should not:
+     *
+     * - set up DRAM
+     * - use global_data
+     * - clear BSS

Quoted 8 lines

arch/arm/cpu/armv8/lowlevel_init.S line 39

Quoted 5 lines

arch/arm/cpu/armv8/lowlevel_init.S new file +5 -0

+     * - try to start a console
+     *
+     * For boards with SPL this should be empty since SPL can do all of
+     * this init in the SPL board_init_f() function which is called
+     * immediately after this.

So this comment says that s_init shouldn't even be used for SPL and instead we should use board_init_f which is what crt0 calls.

Yes, that is a good point. So the sunxi port seemed to ignore this recommendation and do it in its own way (TM).
Frankly I didn't want to fix this one too at this point, so I just went ahead with filling the 64-bit gaps.
But I agree that this should be fixed eventually. The line above:
+obj-$(CONFIG_ARCH_SUNXI) += lowlevel_init.o
hints already that something is not really right here, as SUNXI is the only user of this rather generic function.
If I find some spare time (haha), I might take a look, otherwise: be my guest ;-)

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.

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

---
 lib/tiny-printf.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

lib/tiny-printf.c +4 -4

index 30ac759..b01099d 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 != '%') {

lib/tiny-printf.c +5 -0

 			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;

lib/tiny-printf.c +7 -1

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

Quoted 44 lines · Andre Przywara

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.

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

---
 lib/tiny-printf.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

lib/tiny-printf.c +4 -4

index 30ac759..b01099d 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 != '%') {

lib/tiny-printf.c +1 -0

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

lib/tiny-printf.c int _vprintf(struct printf_info *info, const char *fmt, va_list va) · line 74

Quoted 4 lines

lib/tiny-printf.c +3 -0

             ch = *(fmt++);
+            if (ch == '-')
+                ch = *(fmt++);
+

What does this do? I don't see '-' mentioned in the patch description.

Quoted 22 lines

lib/tiny-printf.c +0 -0

             if (ch == '0') {
                 ch = *(fmt++);

                 lz = 1;

lib/tiny-printf.c +5 -0

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

lib/tiny-printf.c +0 -0

@@ -89,24 +98,38 @@ int _vprintf(struct printf_info *info, const char
 *fmt, va_list va)
                 goto abort;
             case 'u':
             case 'd':

lib/tiny-printf.c int _vprintf(struct printf_info *info, const char *fmt, va_list va) · line 102

Quoted 5 lines

lib/tiny-printf.c +2 -3

-                num = va_arg(va, unsigned int);
-                if (ch == 'd' && (int)num < 0) {
-                    num = -(int)num;
+                div = 1000000000;
+                if (islong) {

Check here if sizeof(long) > 4, so that the whole branch gets optimized away on 32bit.

lib/tiny-printf.c int _vprintf(struct printf_info *info, const char *fmt, va_list va) · line 111

Quoted 9 lines

lib/tiny-printf.c +9 -0

+                    num = va_arg(va, unsigned long);
+                    if (sizeof(long) > 4)
+                        div *= div * 10;
+                } else {
+                    num = va_arg(va, unsigned int);
+                }
+
+                if (ch == 'd' && (long)num < 0) {
+                    num = -(long)num;

Num is a long now and before. So if you have a 32bit signed input, it will sign extend incorrectly here. You need an additional check

  if (islong)
    num = -(long)num;
  else
    num = -(int)num;

Let's hope the compiler on 32bit is smart enough to know that it can combine those two cases :).

lib/tiny-printf.c int _vprintf(struct printf_info *info, const char *fmt, va_list va) · line 117

Quoted 7 lines

lib/tiny-printf.c +1 -1

                     out(info, '-');
                 }
                 if (!num) {
                     out_dgt(info, 0);
                 } else {
-                    for (div = 1000000000; div; div /= 10)
+                    for (; div; div /= 10)

Any particular reason for that change?

lib/tiny-printf.c int _vprintf(struct printf_info *info, const char *fmt, va_list va) · line 122

Quoted 6 lines

lib/tiny-printf.c +1 -1

                         div_out(info, &num, div);
                 }
                 break;
             case 'x':
-                num = va_arg(va, unsigned int);
+                if (islong) {

Same comment as above.

Quoted 16 lines

lib/tiny-printf.c +7 -1

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

Hi Alex,

Quoted 47 lines · Alexander Graf

Quoted 44 lines · Andre Przywara

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.

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

---
 lib/tiny-printf.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

lib/tiny-printf.c +4 -4

index 30ac759..b01099d 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 != '%') {

lib/tiny-printf.c +1 -0

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

Quoted 6 lines

lib/tiny-printf.c int _vprintf(struct printf_info *info, const char *fmt, va_list va) · line 74

Quoted 4 lines

lib/tiny-printf.c +3 -0

             ch = *(fmt++);
+            if (ch == '-')
+                ch = *(fmt++);
+

What does this do? I don't see '-' mentioned in the patch description.

Argh, apparently the comment in the commit message got lost during a patch reshuffle. Sorry, will re-add it.

We need it because some SPL printf uses '-', just ignoring it here seems fine for SPL purposes though.

Quoted 22 lines

Quoted 22 lines

lib/tiny-printf.c +0 -0

             if (ch == '0') {
                 ch = *(fmt++);

                 lz = 1;

lib/tiny-printf.c +5 -0

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

lib/tiny-printf.c +0 -0

@@ -89,24 +98,38 @@ int _vprintf(struct printf_info *info, const char
 *fmt, va_list va)
                 goto abort;
             case 'u':
             case 'd':

Quoted 8 lines

lib/tiny-printf.c int _vprintf(struct printf_info *info, const char *fmt, va_list va) · line 102

Quoted 5 lines

lib/tiny-printf.c +2 -3

-                num = va_arg(va, unsigned int);
-                if (ch == 'd' && (int)num < 0) {
-                    num = -(int)num;
+                div = 1000000000;
+                if (islong) {

Check here if sizeof(long) > 4, so that the whole branch gets optimized away on 32bit.

Good idea.

Quoted 29 lines

lib/tiny-printf.c int _vprintf(struct printf_info *info, const char *fmt, va_list va) · line 111

Quoted 9 lines

lib/tiny-printf.c +9 -0

+                    num = va_arg(va, unsigned long);
+                    if (sizeof(long) > 4)
+                        div *= div * 10;
+                } else {
+                    num = va_arg(va, unsigned int);
+                }
+
+                if (ch == 'd' && (long)num < 0) {
+                    num = -(long)num;

Num is a long now and before. So if you have a 32bit signed input, it will sign extend incorrectly here. You need an additional check

  if (islong)
    num = -(long)num;
  else
    num = -(int)num;

Let's hope the compiler on 32bit is smart enough to know that it can combine those two cases :).

Quoted 7 lines

lib/tiny-printf.c +1 -1

                     out(info, '-');
                 }
                 if (!num) {
                     out_dgt(info, 0);
                 } else {
-                    for (div = 1000000000; div; div /= 10)
+                    for (; div; div /= 10)

Quoted 1 line

Any particular reason for that change?

This algorithm so far only cared for 32-bit values, so it set the start divider to 1E9. This is not sufficient for 64-bit longs in AA64. So I compute div above, depending on the actual size of long.

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.

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

---
 lib/tiny-printf.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

lib/tiny-printf.c +33 -10

index 30ac759..b01099d 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,38 @@ 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;
+				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' && (long)num < 0) {
+					num = -(long)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;

Quoted 3 lines · On Sun, 20 Nov 2016 14:56:59 +0000 Andre Przywara <[email protected]>…

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.

I can't help but notice that the changes of this kind are in a way defeating the original purpose of tiny-printf. And it is surely not the first patch adding features to tiny-printf. I guess, in the end we may end up with a large and bloated printf implementation again :-)

A possible solution might be just a strict check when parsing format modifiers and abort with an error message (yeah, this will introduce some size increase, but hopefully the last one). This way we acknowledge the fact that tiny-printf is a reduced incomplete implementation, and that the callers need to take this into account.

As for the "l" modifier. How much does it add to the code size? IMHO this information should be mentioned in the commit message. Can the AArch64 crash dump code be modified to avoid using it? Or can we have the "l" modifier supported on 64-bit platforms only?

Quoted 4 lines

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

---
 lib/tiny-printf.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

Quoted 23 lines · Siarhei Siamashka

Quoted 3 lines · On Sun, 20 Nov 2016 14:56:59 +0000 Andre Przywara <[email protected]>…

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.

I can't help but notice that the changes of this kind are in a way defeating the original purpose of tiny-printf. And it is surely not the first patch adding features to tiny-printf. I guess, in the end we may end up with a large and bloated printf implementation again :-)

A possible solution might be just a strict check when parsing format modifiers and abort with an error message (yeah, this will introduce some size increase, but hopefully the last one). This way we acknowledge the fact that tiny-printf is a reduced incomplete implementation, and that the callers need to take this into account.

As for the "l" modifier. How much does it add to the code size? IMHO this information should be mentioned in the commit message. Can the AArch64 crash dump code be modified to avoid using it? Or can we have the "l" modifier supported on 64-bit platforms only?

Quoted 4 lines

Quoted 4 lines

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

---
 lib/tiny-printf.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

I think I tested this patch as adding 36 bytes on Thumb2 so not too terrible. But I do agree with the sentiment.

Why is aarch64 using tiny-printf? Surely all though chips have heaps of space?!

On 27/11/16 17:02, Simon Glass wrote:

Hi,

Quoted 32 lines

Quoted 18 lines · Siarhei Siamashka

Quoted 3 lines · On Sun, 20 Nov 2016 14:56:59 +0000 Andre Przywara <[email protected]>…

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.

I can't help but notice that the changes of this kind are in a way defeating the original purpose of tiny-printf. And it is surely not the first patch adding features to tiny-printf. I guess, in the end we may end up with a large and bloated printf implementation again :-)

A possible solution might be just a strict check when parsing format modifiers and abort with an error message (yeah, this will introduce some size increase, but hopefully the last one). This way we acknowledge the fact that tiny-printf is a reduced incomplete implementation, and that the callers need to take this into account.

Quoted 9 lines

As for the "l" modifier. How much does it add to the code size? IMHO this information should be mentioned in the commit message. Can the AArch64 crash dump code be modified to avoid using it? Or can we have the "l" modifier supported on 64-bit platforms only?

Quoted 4 lines

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

---
 lib/tiny-printf.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

I think I tested this patch as adding 36 bytes on Thumb2 so not too terrible. But I do agree with the sentiment.

Quoted 34 lines · Andr? Przywara

On 27/11/16 17:02, Simon Glass wrote:

Hi,

Quoted 30 lines

Quoted 27 lines · Siarhei Siamashka

Quoted 3 lines · On Sun, 20 Nov 2016 14:56:59 +0000 Andre Przywara <[email protected]>…

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.

I can't help but notice that the changes of this kind are in a way defeating the original purpose of tiny-printf. And it is surely not the first patch adding features to tiny-printf. I guess, in the end we may end up with a large and bloated printf implementation again :-)

A possible solution might be just a strict check when parsing format modifiers and abort with an error message (yeah, this will introduce some size increase, but hopefully the last one). This way we acknowledge the fact that tiny-printf is a reduced incomplete implementation, and that the callers need to take this into account.

As for the "l" modifier. How much does it add to the code size? IMHO this information should be mentioned in the commit message. Can the AArch64 crash dump code be modified to avoid using it? Or can we have the "l" modifier supported on 64-bit platforms only?

Quoted 4 lines

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

---
 lib/tiny-printf.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

Quoted 2 lines

Quoted 2 lines

I think I tested this patch as adding 36 bytes on Thumb2 so not too terrible. But I do agree with the sentiment.

Simon, what is your compiler?

Both with GCC 5.3.0 and GCC 6.2.0 I get exactly 6/4 bytes more of .text, which is not too bad for parsing (but ignoring) two new modifiers. It turns out that (at least these two versions of) GCCs are quite clever here and optimize away almost everything.
Looking closer one can see that the if and else branches become identical if sizeof(long) == sizeof(int) == 4, so the compiler happily merges the code, removes the "if (long)" check and in turn the whole long-handling code on 32-bit.
This is the patch as sent, without any further hints in the code.

If anyone really wants to save code space, I suggest to switch to a later compiler:

GCC 5.3.0:
text data bss dec hex filename
origin/master orangepi_plus_defconfig GCC 5.3.0
18881 488 232 19601 4c91 spl/u-boot-spl
758 0 0 758 2f6 spl/lib/tiny-printf.o

master+tiny_printf %l,%- orangepi_plus_defconfig GCC 5.3.0
18887 488 232 19607 4c97 spl/u-boot-spl
758 0 0 758 2f6 spl/lib/tiny-printf.o

GCC 6.2.0:
origin/master orangepi_plus_defconfig GCC 6.2.0
16871 488 232 17591 44b7 spl/u-boot-spl
698 0 0 698 2ba spl/lib/tiny-printf.o

master+tiny_printf %l,%- orangepi_plus_defconfig GCC 6.2.0
16875 488 232 17595 44bb spl/u-boot-spl
702 0 0 702 2be spl/lib/tiny-printf.o

On 64-bit (only GCC 6.2.0) this results in more code, as expected:
HEAD of patch set w/o tiny-printf %l, pine64_plus_defconfig
25824 392 360 26576 67d0 spl/u-boot-spl
1542 0 0 1542 606 spl/lib/tiny-printf.o
HEAD of patch set, pine64_plus_defconfig
25972 392 360 26724 6864 spl/u-boot-spl
1690 0 0 1690 69a spl/lib/tiny-printf.o

So this is 148 Bytes more in .text. I can trade three simple patches that cut off 80 Bytes in sunxi/armv8 to at least offset this a bit, though this isn't really a regression, as there was no SPL64 for sunxi before.
So apart from Alex' bug fix I won't change the patch, if people can live with that.

Cheers,
Andre.

Quoted 3 lines · On Sun, 20 Nov 2016 14:56:59 +0000 Andre Przywara <[email protected]>…

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.

I can't help but notice that the changes of this kind are in a way defeating the original purpose of tiny-printf. And it is surely not the first patch adding features to tiny-printf. I guess, in the end we may end up with a large and bloated printf implementation again :-)

A possible solution might be just a strict check when parsing format modifiers and abort with an error message (yeah, this will introduce some size increase, but hopefully the last one). This way we acknowledge the fact that tiny-printf is a reduced incomplete implementation, and that the callers need to take this into account.

As for the "l" modifier. How much does it add to the code size? IMHO this information should be mentioned in the commit message.

Can the AArch64 crash dump code be modified to avoid using it?

Or can we have
the "l" modifier supported on 64-bit platforms only?

Quoted 103 lines

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

---
 lib/tiny-printf.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

lib/tiny-printf.c +33 -10

index 30ac759..b01099d 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,38 @@ 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;
+				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' && (long)num < 0) {
+					num = -(long)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]>

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

---
 arch/arm/cpu/armv7/omap-common/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(-)

arch/arm/cpu/armv7/omap-common/boot-common.c +1 -1

index 385310b..7ae3d80 100644
@@ -228,7 +228,7 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 
 	u32 boot_params = *((u32 *)OMAP_SRAM_SCRATCH_BOOT_PARAMS);
 
-	debug("image entry point: 0x%X\n", spl_image->entry_point);
+	debug("image entry point: 0x%lX\n", spl_image->entry_point);
 	/* Pass the saved boot_params from rom code */
 	image_entry((u32 *)boot_params);
 }

arch/arm/mach-tegra/spl.c +1 -1

index e0f9d5b..41c88cb 100644
@@ -42,7 +42,7 @@ u32 spl_boot_device(void)
 
 void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 {
-	debug("image entry point: 0x%X\n", spl_image->entry_point);
+	debug("image entry point: 0x%lX\n", spl_image->entry_point);
 
 	start_cpu((u32)spl_image->entry_point);
 	halt_avp();

common/spl/spl.c +4 -4

index bdb165a..835eed6 100644
@@ -115,7 +115,7 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
 		}
 		spl_image->os = image_get_os(header);
 		spl_image->name = image_get_name(header);
-		debug("spl: payload image: %.*s load addr: 0x%x size: %d\n",
+		debug("spl: payload image: %.*s load addr: 0x%lx size: %d\n",
 			(int)sizeof(spl_image->name), spl_image->name,
 			spl_image->load_addr, spl_image->size);
 	} else {

@@ -140,7 +140,7 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
 			spl_image->load_addr = CONFIG_SYS_LOAD_ADDR;
 			spl_image->entry_point = CONFIG_SYS_LOAD_ADDR;
 			spl_image->size = end - start;
-			debug("spl: payload zImage, load addr: 0x%x size: %d\n",
+			debug("spl: payload zImage, load addr: 0x%lx size: %d\n",
 			      spl_image->load_addr, spl_image->size);
 			return 0;
 		}

@@ -164,9 +164,9 @@ __weak void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 	typedef void __noreturn (*image_entry_noargs_t)(void);
 
 	image_entry_noargs_t image_entry =
-		(image_entry_noargs_t)(unsigned long)spl_image->entry_point;
+		(image_entry_noargs_t)spl_image->entry_point;
 
-	debug("image entry point: 0x%X\n", spl_image->entry_point);
+	debug("image entry point: 0x%lX\n", spl_image->entry_point);
 	image_entry();
 }
 

common/spl/spl_mmc.c +1 -1

index c674e61..4d0af2d 100644
@@ -36,7 +36,7 @@ static int mmc_load_legacy(struct spl_image_info *spl_image, struct mmc *mmc,
 	/* Read the header too to avoid extra memcpy */
 	count = blk_dread(mmc_get_blk_desc(mmc), sector, image_size_sectors,
 			  (void *)(ulong)spl_image->load_addr);
-	debug("read %x sectors to %x\n", image_size_sectors,
+	debug("read %x sectors to %lx\n", image_size_sectors,
 	      spl_image->load_addr);
 	if (count != image_size_sectors)
 		return -EIO;

include/spl.h +2 -2

index e080a82..2f8c052 100644
@@ -23,8 +23,8 @@
 struct spl_image_info {
 	const char *name;
 	u8 os;
-	u32 load_addr;
-	u32 entry_point;
+	ulong load_addr;
+	ulong entry_point;
 	u32 size;
 	u32 flags;
 };

Quoted 6 lines · Andre Przywara

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

I'm surprised York didn't stumble over this yet :).

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 +13 -0

index e06c3cc..e82e9cf 100644
@@ -16,6 +16,19 @@
 #include <asm/system.h>
 #include <linux/compiler.h>
 
+/************************************************************
+ * sdelay() - simple spin loop.  Will be constant time as
+ *  its generally used in bypass conditions only.  This
+ *  is necessary until timers are accessible.
+ *
+ *  not inline to increase chances its in cache when called
+ *************************************************************/
+void sdelay(unsigned long loops)
+{
+	__asm__ volatile ("1:\n" "subs %0, %1, #1\n"
+			  "b.ne 1b":"=r" (loops):"0"(loops));
+}
+
 int cleanup_before_linux(void)
 {
 	/*

Quoted 5 lines · Andre Przywara

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

I don't think it hurts to write this in C - and I also doubt that inlining has any negative effect.

Something along the lines of

static inline void sdelay(...) {
   for (; loops; loops--)
     asm volatile("");
}

inside a header should do the trick as well and is much more readable.

Alex

Quoted 29 lines

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

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

index e06c3cc..e82e9cf 100644
@@ -16,6 +16,19 @@
 #include <asm/system.h>
 #include <linux/compiler.h>
 
+/************************************************************
+ * sdelay() - simple spin loop.  Will be constant time as
+ *  its generally used in bypass conditions only.  This
+ *  is necessary until timers are accessible.
+ *
+ *  not inline to increase chances its in cache when called
+ *************************************************************/
+void sdelay(unsigned long loops)
+{
+	__asm__ volatile ("1:\n" "subs %0, %1, #1\n"
+			  "b.ne 1b":"=r" (loops):"0"(loops));
+}
+
 int cleanup_before_linux(void)
 {
 	/*

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 +1 -0

index e06c3cc..e82e9cf 100644
@@ -16,6 +16,19 @@
 #include <asm/system.h>
 #include <linux/compiler.h>
 
+/************************************************************

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

+}
+
 int cleanup_before_linux(void)
 {
 	/*

Quoted 17 lines · On Sun, 20 Nov 2016 14:57:02 +0000 Andre Przywara <[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..e82e9cf 100644
@@ -16,6 +16,19 @@
 #include <asm/system.h>
 #include <linux/compiler.h>
 

arch/arm/cpu/armv8/cpu.c line 29

Quoted 11 lines

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

+/************************************************************
+ * sdelay() - simple spin loop.  Will be constant time as
+ *  its generally used in bypass conditions only.  This
+ *  is necessary until timers are accessible.
+ *
+ *  not inline to increase chances its in cache when called
+ *************************************************************/
+void sdelay(unsigned long loops)
+{
+	__asm__ volatile ("1:\n" "subs %0, %1, #1\n"
+			  "b.ne 1b":"=r" (loops):"0"(loops));

This inline assembly needs "cc" in the clobber list. Also don't we want to just use a single register for the counter ("subs %0, %0, #1") rather than trying to construct something excessively complicated and possibly fragile?

The https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html page provides some information.

Quoted 5 lines

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

+}
+
 int cleanup_before_linux(void)
 {
 	/*

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

Hi Andre,

Quoted 19 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(-)

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 144

Quoted 9 lines

board/sunxi/Kconfig +9 -0

+config RESERVE_ALLWINNER_BOOT0_HEADER
+	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.
+

Is there a reason you can think of to disable it?

If not, you should consider making this enabled by default, so that we don't enable it in all the defconfig for no particular reason.

Hi Maxime,

thanks for having a look!

Quoted 22 lines · Maxime Ripard

Hi Andre,

Quoted 19 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(-)

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
 

Quoted 11 lines

board/sunxi/Kconfig config MACH_SUN8I · line 144

Quoted 9 lines

board/sunxi/Kconfig +9 -0

+config RESERVE_ALLWINNER_BOOT0_HEADER
+	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.
+

Is there a reason you can think of to disable it?

We need it only for booting from boot0, so this series actually makes this whole thing obsolete. Since - apart from enlarging the U-Boot (proper) image by 1.5KB - it doesn't hurt, though, my idea was to keep it in as an option for some time until we are confident that boot0 is no longer needed.

Quoted 2 lines

If not, you should consider making this enabled by default, so that we don't enable it in all the defconfig for no particular reason.

I can change the logic, make the Kconfig entry "default y if ARM64", and any defconfig could then choose to say "# RESERVE_... is not set".

Does that make more sense to you?

What was your major concern about this? Having pointless options in various defconfigs?

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.

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 +1 -0

index 6a13db5..c31a2af 100644
@@ -4,6 +4,33 @@
  * SPDX-License-Identifier:	GPL-2.0+
  */
 
+#if defined(CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER)

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

+	.word   0xe59f0020              // ldr  r0, [pc, #32]
+	.word   0xe5810000              // str  r0, [r1]
+	.word   0xf57ff04f              // dsb  sy
+	.word   0xf57ff06f              // isb  sy
+	.word   0xee1c0f50              // mrc  15, 0, r0, cr12, cr0, {2}
+	.word   0xe3800003              // orr  r0, r0, #3
+	.word   0xee0c0f50              // mcr  15, 0, r0, cr12, cr0, {2}
+	.word   0xf57ff06f              // isb  sy
+	.word   0xe320f003              // wfi
+	.word   0xeafffffd              // b    @wfi
+#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 +5 -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.
 
+config ARM_BOOT_HOOK_RMR
+	bool
+	default y if ARM64
+	select ENABLE_ARM_SOC_BOOT0_HOOK
+
 config DRAM_TYPE
 	int "sunxi dram type"
 	depends on MACH_SUN8I_A83T

Quoted 28 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.

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

index 6a13db5..c31a2af 100644
@@ -4,6 +4,33 @@
  * SPDX-License-Identifier:    GPL-2.0+
  */
 

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

Quoted 11 lines

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

+#if defined(CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER)
 /* 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   0xe3a01617              // mov  r1, #0x1700000
+    .word   0xe38110a0              // orr  r1, r1, #0xa0

Is this address guaranteed to stay the same for newer chips?

Maybe it'd
be better to use a pc-relative load and put it in as .word like you do below for the text base address.

Alex

Quoted 36 lines

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

+    .word   0xe59f0020              // ldr  r0, [pc, #32]
+    .word   0xe5810000              // str  r0, [r1]
+    .word   0xf57ff04f              // dsb  sy
+    .word   0xf57ff06f              // isb  sy
+    .word   0xee1c0f50              // mrc  15, 0, r0, cr12, cr0, {2}
+    .word   0xe3800003              // orr  r0, r0, #3
+    .word   0xee0c0f50              // mcr  15, 0, r0, cr12, cr0, {2}
+    .word   0xf57ff06f              // isb  sy
+    .word   0xe320f003              // wfi
+    .word   0xeafffffd              // b    @wfi
+#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 +5 -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.
 
+config ARM_BOOT_HOOK_RMR
+    bool
+    default y if ARM64
+    select ENABLE_ARM_SOC_BOOT0_HOOK
+
 config DRAM_TYPE
     int "sunxi dram type"
     depends on MACH_SUN8I_A83T

Fix the output of the DRAM size on AArch64 SPLs.

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

Now that the SPL is ready to be compiled in AArch64 and the DRAM init code is ready, enable SPL support for the A64 SoC and in the Pine64 defconfig.
For now we keep the boot0 header in the U-Boot proper, as this allows to still use boot0 as an SPL replacement without hurting the SPL use case.
We disable FEL support for now, as the code isn't ready yet.

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

---
 arch/arm/include/asm/arch-sunxi/boot0.h | 4 ++--
 arch/arm/mach-sunxi/board.c             | 2 +-
 board/sunxi/Kconfig                     | 2 ++
 configs/pine64_plus_defconfig           | 2 ++
 include/configs/sunxi-common.h          | 2 ++
 5 files changed, 9 insertions(+), 3 deletions(-)

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

index c31a2af..173e042 100644
@@ -4,11 +4,11 @@
  * SPDX-License-Identifier:	GPL-2.0+
  */
 

arch/arm/mach-sunxi/board.c +1 -1

index 0f8ead9..80d4b57 100644
@@ -133,7 +133,7 @@ static int gpio_init(void)
 	return 0;
 }
 
-#ifdef CONFIG_SPL_BUILD
+#if defined(CONFIG_SPL_BOARD_LOAD_IMAGE) && defined(CONFIG_SPL_BUILD)
 static int spl_board_load_image(struct spl_image_info *spl_image,
 				struct spl_boot_device *bootdev)
 {

board/sunxi/Kconfig +2 -0

index d477925..b5246df 100644
@@ -125,6 +125,7 @@ config MACH_SUN50I
 	bool "sun50i (Allwinner A64)"
 	select ARM64
 	select SUNXI_GEN_SUN6I
+	select SUPPORT_SPL
 
 endchoice
 

@@ -187,6 +188,7 @@ config DRAM_ODT_EN
 	bool "sunxi dram odt enable"
 	default n if !MACH_SUN8I_A23
 	default y if MACH_SUN8I_A23
+	default y if MACH_SUN50I
 	---help---
 	Select this to enable dram odt (on die termination).
 

configs/pine64_plus_defconfig +0 -0

index ebc24b8..5286fee 100644

configs/pine64_plus_defconfig +1 -0

 CONFIG_DEFAULT_DEVICE_TREE="sun50i-a64-pine64-plus"
 # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
 CONFIG_CONSOLE_MUX=y
+CONFIG_SPL=y
 # CONFIG_CMD_IMLS is not set
 # CONFIG_CMD_FLASH is not set
 # CONFIG_CMD_FPGA is not set

include/configs/sunxi-common.h +2 -0

index 86b4104..f2cb174 100644
@@ -182,7 +182,9 @@
 
 #define CONFIG_SPL_FRAMEWORK
 
+#ifndef CONFIG_MACH_SUN50I
 #define CONFIG_SPL_BOARD_LOAD_IMAGE
+#endif
 
 #if defined(CONFIG_MACH_SUN9I)
 #define CONFIG_SPL_TEXT_BASE		0x10040		/* sram start+header */

Quoted 26 lines · Andre Przywara

Now that the SPL is ready to be compiled in AArch64 and the DRAM init code is ready, enable SPL support for the A64 SoC and in the Pine64 defconfig.
For now we keep the boot0 header in the U-Boot proper, as this allows to still use boot0 as an SPL replacement without hurting the SPL use case.
We disable FEL support for now, as the code isn't ready yet.

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

---
 arch/arm/include/asm/arch-sunxi/boot0.h | 4 ++--
 arch/arm/mach-sunxi/board.c             | 2 +-
 board/sunxi/Kconfig                     | 2 ++
 configs/pine64_plus_defconfig           | 2 ++
 include/configs/sunxi-common.h          | 2 ++
 5 files changed, 9 insertions(+), 3 deletions(-)

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

index c31a2af..173e042 100644
@@ -4,11 +4,11 @@
  * SPDX-License-Identifier:    GPL-2.0+
  */
 

Quoted 11 lines

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

-#if defined(CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER)
+#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)
+#elif defined(CONFIG_ARM_BOOT_HOOK_RMR) && defined(CONFIG_SPL_BUILD)
 /* switch into AArch64 if needed */
     tst     x0, x0                  // this is "b #0x84" in ARM

b reset

Shouldn't the hunk above go into the patches that introduce the options?

Quoted 33 lines

arch/arm/mach-sunxi/board.c +1 -1

index 0f8ead9..80d4b57 100644
@@ -133,7 +133,7 @@ static int gpio_init(void)
     return 0;
 }
 
-#ifdef CONFIG_SPL_BUILD
+#if defined(CONFIG_SPL_BOARD_LOAD_IMAGE) && defined(CONFIG_SPL_BUILD)
 static int spl_board_load_image(struct spl_image_info *spl_image,
                 struct spl_boot_device *bootdev)
 {

board/sunxi/Kconfig +2 -0

index d477925..b5246df 100644
@@ -125,6 +125,7 @@ config MACH_SUN50I
     bool "sun50i (Allwinner A64)"
     select ARM64
     select SUNXI_GEN_SUN6I
+    select SUPPORT_SPL
 
 endchoice
 

@@ -187,6 +188,7 @@ config DRAM_ODT_EN
     bool "sunxi dram odt enable"
     default n if !MACH_SUN8I_A23
     default y if MACH_SUN8I_A23
+    default y if MACH_SUN50I
     ---help---
     Select this to enable dram odt (on die termination).
 

configs/pine64_plus_defconfig CONFIG_ARM=y · line 5

Quoted 10 lines

configs/pine64_plus_defconfig +1 -0

index ebc24b8..5286fee 100644
@@ -2,9 +2,11 @@ CONFIG_ARM=y
 CONFIG_RESERVE_ALLWINNER_BOOT0_HEADER=y
 CONFIG_ARCH_SUNXI=y
 CONFIG_MACH_SUN50I=y
+CONFIG_DRAM_CLK=672

Do you need this?

Quoted 24 lines

configs/pine64_plus_defconfig +1 -0

 CONFIG_DEFAULT_DEVICE_TREE="sun50i-a64-pine64-plus"
 # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
 CONFIG_CONSOLE_MUX=y
+CONFIG_SPL=y
 # CONFIG_CMD_IMLS is not set
 # CONFIG_CMD_FLASH is not set
 # CONFIG_CMD_FPGA is not set

include/configs/sunxi-common.h +2 -0

index 86b4104..f2cb174 100644
@@ -182,7 +182,9 @@
 
 #define CONFIG_SPL_FRAMEWORK
 
+#ifndef CONFIG_MACH_SUN50I
 #define CONFIG_SPL_BOARD_LOAD_IMAGE
+#endif
 
 #if defined(CONFIG_MACH_SUN9I)
 #define CONFIG_SPL_TEXT_BASE        0x10040        /* sram

start+header */