[U-Boot] [PATCH] sunxi: Only compile board.o for CONFIG_SPL_BUILD

[U-Boot] [PATCH] sunxi: Only compile board.o for CONFIG_SPL_BUILD

[Tom Rini]

All of the code in arch/arm/cpu/armv7/sunxi/board.c was under a check
for CONFIG_SPL_BUILD so instead only build for SPL.  This lets us drop a
hunk of code that was never enabled.

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Ian Campbell <ijc@hellion.org.uk>
Signed-off-by: Tom Rini <trini@ti.com>
---
 arch/arm/cpu/armv7/sunxi/Makefile |    2 +-
 arch/arm/cpu/armv7/sunxi/board.c  |   15 ---------------
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile
index 1720f7d..1fbfb6c 100644
--- a/arch/arm/cpu/armv7/sunxi/Makefile
+++ b/arch/arm/cpu/armv7/sunxi/Makefile
@@ -8,7 +8,6 @@
 # SPDX-License-Identifier:	GPL-2.0+
 #
 obj-y	+= timer.o
-obj-y	+= board.o
 obj-y	+= clock.o
 obj-y	+= cpu_info.o
 obj-y	+= pinmux.o
@@ -30,6 +29,7 @@ endif
 endif
 
 ifdef CONFIG_SPL_BUILD
+obj-y	+= board.o
 obj-$(CONFIG_MACH_SUN4I)	+= dram_sun4i.o
 obj-$(CONFIG_MACH_SUN5I)	+= dram_sun4i.o
 obj-$(CONFIG_MACH_SUN6I)	+= dram_sun6i.o
diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c
index f4a580a..5998548 100644
--- a/arch/arm/cpu/armv7/sunxi/board.c
+++ b/arch/arm/cpu/armv7/sunxi/board.c
@@ -15,9 +15,7 @@
 #include <netdev.h>
 #include <miiphy.h>
 #include <serial.h>
-#ifdef CONFIG_SPL_BUILD
 #include <spl.h>
-#endif
 #include <asm/gpio.h>
 #include <asm/io.h>
 #include <asm/arch/clock.h>
@@ -27,10 +25,6 @@
 
 #include <linux/compiler.h>
 
-#ifdef CONFIG_SPL_BUILD
-/* Pointer to the global data structure for SPL */
-DECLARE_GLOBAL_DATA_PTR;
-
 /* The sunxi internal brom will try to loader external bootloader
  * from mmc0, nand flash, mmc2.
  * Unfortunately we can't check how SPL was loaded so assume
@@ -92,14 +86,6 @@ void board_init_f(ulong dummy)
 	 * access gets messed up (seems cache related) */
 	setbits_le32(SUNXI_SRAMC_BASE + 0x44, 0x1800);
 #endif
-#if !defined CONFIG_SPL_BUILD && (defined CONFIG_MACH_SUN7I || \
-		defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I)
-	/* Enable SMP mode for CPU0, by setting bit 6 of Auxiliary Ctl reg */
-	asm volatile(
-		"mrc p15, 0, r0, c1, c0, 1\n"
-		"orr r0, r0, #1 << 6\n"
-		"mcr p15, 0, r0, c1, c0, 1\n");
-#endif
 
 	clock_init();
 	timer_init();
@@ -186,4 +172,3 @@ int cpu_eth_init(bd_t *bis)
 
 	return 0;
 }
-#endif
-- 
1.7.9.5

[U-Boot] [PATCH] sunxi: Only compile board.o for CONFIG_SPL_BUILD

[Ian Campbell]

On Tue, 2015-01-20 at 16:40 -0500, Tom Rini wrote:

In general the change looks good, thanks.

> -#if !defined CONFIG_SPL_BUILD && (defined CONFIG_MACH_SUN7I || \
> -		defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I)
> -	/* Enable SMP mode for CPU0, by setting bit 6 of Auxiliary Ctl reg */
> -	asm volatile(
> -		"mrc p15, 0, r0, c1, c0, 1\n"
> -		"orr r0, r0, #1 << 6\n"
> -		"mcr p15, 0, r0, c1, c0, 1\n");
> -#endif

Hans has an in flight patch which moves this elsewhere. But what's
confusing me is how it ever worked at all if this wasn't even being
compiled in. ACTLR.SMP is pretty critical to correct operation of the
SoC (cache coherency etc), at least once secondary processors are up...

Ian.

[U-Boot] [PATCH] sunxi: Only compile board.o for CONFIG_SPL_BUILD

[Hans de Goede]

Hi,

On 20-01-15 22:40, Tom Rini wrote:
> All of the code in arch/arm/cpu/armv7/sunxi/board.c was under a check
> for CONFIG_SPL_BUILD so instead only build for SPL.

That is not true, the #ifdef SPL block ends at the end of board_init_f
as things currently stand in master, and even that is only the case
since the very recently merged "sunxi: Move SPL s_init() code to board_init_f()"
patch from Simon. Completely not building board.c would break the "reset"
command as well as break network booting.

Moreover I believe the changes by Simon are less then optimal.

> This lets us drop a hunk of code that was never enabled.

As for that hunk of code never being enabled, it was moved to a place
where it indeed no longer is ever enabled by the same commit from Simon,
before when it was sitting in s_init, would get called from both normal
u-boot execution and from SPL, and then it would run in the normal
u-boot call.

I realize that Simon's patches have posted quite a while back, and I
seem to have missed them, sorry about that. But I would have appreciated
a ping on this rather then merging them without any input from me.

This turns the cp15 mangling needed to get the caches going on sun6i
and later into a nop, meaning we will boot the kernel without any
caches enabling causing just the kernel extracting itself to take 5
seconds or so.

But that is not the biggest problem, the biggest problem is that on
sunxi the SPL and u-boot.bin are two separate pieces, where the second
may very will be used standalone, that is actually how we bring most
new SoC's up, first do a standalone u-boot.bin using Allwinner's
boot0 binary as SPL, and then later add SPL support. So we want
u-boot.bin to be able to work standalone, and thus it should not rely
on things like gpio setup already being done by whatever came before
it.

I'll look into fixing things up to work again properly with the recent
changes Simon made.

Regards,

Hans


>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Ian Campbell <ijc@hellion.org.uk>
> Signed-off-by: Tom Rini <trini@ti.com>
> ---
>   arch/arm/cpu/armv7/sunxi/Makefile |    2 +-
>   arch/arm/cpu/armv7/sunxi/board.c  |   15 ---------------
>   2 files changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/arch/arm/cpu/armv7/sunxi/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile
> index 1720f7d..1fbfb6c 100644
> --- a/arch/arm/cpu/armv7/sunxi/Makefile
> +++ b/arch/arm/cpu/armv7/sunxi/Makefile
> @@ -8,7 +8,6 @@
>   # SPDX-License-Identifier:	GPL-2.0+
>   #
>   obj-y	+= timer.o
> -obj-y	+= board.o
>   obj-y	+= clock.o
>   obj-y	+= cpu_info.o
>   obj-y	+= pinmux.o
> @@ -30,6 +29,7 @@ endif
>   endif
>
>   ifdef CONFIG_SPL_BUILD
> +obj-y	+= board.o
>   obj-$(CONFIG_MACH_SUN4I)	+= dram_sun4i.o
>   obj-$(CONFIG_MACH_SUN5I)	+= dram_sun4i.o
>   obj-$(CONFIG_MACH_SUN6I)	+= dram_sun6i.o
> diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c
> index f4a580a..5998548 100644
> --- a/arch/arm/cpu/armv7/sunxi/board.c
> +++ b/arch/arm/cpu/armv7/sunxi/board.c
> @@ -15,9 +15,7 @@
>   #include <netdev.h>
>   #include <miiphy.h>
>   #include <serial.h>
> -#ifdef CONFIG_SPL_BUILD
>   #include <spl.h>
> -#endif
>   #include <asm/gpio.h>
>   #include <asm/io.h>
>   #include <asm/arch/clock.h>
> @@ -27,10 +25,6 @@
>
>   #include <linux/compiler.h>
>
> -#ifdef CONFIG_SPL_BUILD
> -/* Pointer to the global data structure for SPL */
> -DECLARE_GLOBAL_DATA_PTR;
> -
>   /* The sunxi internal brom will try to loader external bootloader
>    * from mmc0, nand flash, mmc2.
>    * Unfortunately we can't check how SPL was loaded so assume
> @@ -92,14 +86,6 @@ void board_init_f(ulong dummy)
>   	 * access gets messed up (seems cache related) */
>   	setbits_le32(SUNXI_SRAMC_BASE + 0x44, 0x1800);
>   #endif
> -#if !defined CONFIG_SPL_BUILD && (defined CONFIG_MACH_SUN7I || \
> -		defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I)
> -	/* Enable SMP mode for CPU0, by setting bit 6 of Auxiliary Ctl reg */
> -	asm volatile(
> -		"mrc p15, 0, r0, c1, c0, 1\n"
> -		"orr r0, r0, #1 << 6\n"
> -		"mcr p15, 0, r0, c1, c0, 1\n");
> -#endif
>
>   	clock_init();
>   	timer_init();
> @@ -186,4 +172,3 @@ int cpu_eth_init(bd_t *bis)
>
>   	return 0;
>   }
> -#endif
>

[U-Boot] [PATCH] sunxi: Only compile board.o for CONFIG_SPL_BUILD

[Tom Rini]

On Wed, Jan 21, 2015 at 02:18:12PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 20-01-15 22:40, Tom Rini wrote:
> >All of the code in arch/arm/cpu/armv7/sunxi/board.c was under a check
> >for CONFIG_SPL_BUILD so instead only build for SPL.
> 
> That is not true, the #ifdef SPL block ends at the end of board_init_f
> as things currently stand in master, and even that is only the case
> since the very recently merged "sunxi: Move SPL s_init() code to board_init_f()"
> patch from Simon. Completely not building board.c would break the "reset"
> command as well as break network booting.
> 
> Moreover I believe the changes by Simon are less then optimal.
> 
> >This lets us drop a hunk of code that was never enabled.
> 
> As for that hunk of code never being enabled, it was moved to a place
> where it indeed no longer is ever enabled by the same commit from Simon,
> before when it was sitting in s_init, would get called from both normal
> u-boot execution and from SPL, and then it would run in the normal
> u-boot call.
> 
> I realize that Simon's patches have posted quite a while back, and I
> seem to have missed them, sorry about that. But I would have appreciated
> a ping on this rather then merging them without any input from me.

Sorry, I thought since Ian had commented and Simon had tested on some
sunxi board.

> This turns the cp15 mangling needed to get the caches going on sun6i
> and later into a nop, meaning we will boot the kernel without any
> caches enabling causing just the kernel extracting itself to take 5
> seconds or so.
> 
> But that is not the biggest problem, the biggest problem is that on
> sunxi the SPL and u-boot.bin are two separate pieces, where the second
> may very will be used standalone, that is actually how we bring most
> new SoC's up, first do a standalone u-boot.bin using Allwinner's
> boot0 binary as SPL, and then later add SPL support. So we want
> u-boot.bin to be able to work standalone, and thus it should not rely
> on things like gpio setup already being done by whatever came before
> it.
> 
> I'll look into fixing things up to work again properly with the recent
> changes Simon made.

OK, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150121/6a376154/attachment.pgp>

[U-Boot] [PATCH] sunxi: Only compile board.o for CONFIG_SPL_BUILD

[Simon Glass]

HI Hans,

On 21 January 2015 at 06:18, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 20-01-15 22:40, Tom Rini wrote:
>>
>> All of the code in arch/arm/cpu/armv7/sunxi/board.c was under a check
>> for CONFIG_SPL_BUILD so instead only build for SPL.
>
>
> That is not true, the #ifdef SPL block ends at the end of board_init_f
> as things currently stand in master, and even that is only the case
> since the very recently merged "sunxi: Move SPL s_init() code to
> board_init_f()"
> patch from Simon. Completely not building board.c would break the "reset"
> command as well as break network booting.
>
> Moreover I believe the changes by Simon are less then optimal.

Quite possibly. My objective was to sort out the global_data stuff in
SPL which we weren't able to resolve in the last merge window. This
will unblock driver model in SPL.

>
>> This lets us drop a hunk of code that was never enabled.
>
>
> As for that hunk of code never being enabled, it was moved to a place
> where it indeed no longer is ever enabled by the same commit from Simon,
> before when it was sitting in s_init, would get called from both normal
> u-boot execution and from SPL, and then it would run in the normal
> u-boot call.
>
> I realize that Simon's patches have posted quite a while back, and I
> seem to have missed them, sorry about that. But I would have appreciated
> a ping on this rather then merging them without any input from me.
>

I have a few sunxi boards, I believe I tested this with pcduino3 only,
but I don't have my notes in front of me. Also I would not have
noticed the down-stream side-effects you refer to below.

I want to thank Tom for digging into this and figuring it out. Sorry
that I broke things on sunxi, but there is still plenty of time to fix
it, and please let me know if I can help.

> This turns the cp15 mangling needed to get the caches going on sun6i
> and later into a nop, meaning we will boot the kernel without any
> caches enabling causing just the kernel extracting itself to take 5
> seconds or so.
>
> But that is not the biggest problem, the biggest problem is that on
> sunxi the SPL and u-boot.bin are two separate pieces, where the second
> may very will be used standalone, that is actually how we bring most
> new SoC's up, first do a standalone u-boot.bin using Allwinner's
> boot0 binary as SPL, and then later add SPL support. So we want
> u-boot.bin to be able to work standalone, and thus it should not rely
> on things like gpio setup already being done by whatever came before
> it.

I don't think doing things twice on boot is a good idea. If you want
to do SPL init in U-Boot proper then how about adding a CONFIG option
to enable this, for development/debugging purposes only?

Also you are double the test cases for people, by adding a new supported flow.

>
> I'll look into fixing things up to work again properly with the recent
> changes Simon made.

Thanks, it's all in a good cause.

- Simon

[snip]