u-boot/spl: Add u-boot-spl.img to u-boot targets

PATCH RESEND New Vipin Kumar 3 participants 13 messages Dec 6, 2012 – Dec 17, 2012

Subsystems (2)

Arm (32-bit) Kbuild

lore

Share & tools

lore

Vipin Kumar

author

2012-12-06 09:29 UTC #0 lore raw

PATCH RESEND 1/2 u-boot/spl: Add u-boot-spl.img to u-boot targets

Signed-off-by Vipin Kumar <[email protected]>

---
 Makefile | 10 ++++++++++
 1 file changed, 10 insertions(+)

Makefile +10 -0

index 8a04727..7f416f4 100644
@@ -499,6 +499,13 @@ $(obj)u-boot.sb:       $(obj)u-boot.bin $(obj)spl/u-boot-spl.bin
 		elftosb -zdf $(ELFTOSB_TARGET-y) -c $(TOPDIR)/$(CPUDIR)/$(SOC)/u-boot-$(ELFTOSB_TARGET-y).bd \
 			-o $(obj)u-boot.sb
 
+$(obj)spl/u-boot-spl.img:	$(obj)spl/u-boot-spl.bin
+		$(obj)tools/mkimage -A $(ARCH) -T firmware -C none \
+		-a $(CONFIG_SPL_TEXT_BASE) -e $(CONFIG_SPL_TEXT_BASE) \
+		-n $(shell sed -n -e 's/.*U_BOOT_SPL_VERSION//p' $(VERSION_FILE) | \
+			sed -e 's/"[	 ]*$$/ for $(BOARD) board"/') \
+		-d $(obj)spl/u-boot-spl.bin $(obj)spl/u-boot-spl.img
+
 # On x600 (SPEAr600) U-Boot is appended to U-Boot SPL.
 # Both images are created using mkimage (crc etc), so that the ROM
 # bootloader can check its integrity. Padding needs to be done to the

@@ -707,6 +714,9 @@ $(VERSION_FILE):
 			"$(U_BOOT_VERSION)" "$${localvers}" ; \
 		   printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \
 			"$(U_BOOT_VERSION)" "$${localvers}" ; \
+		   printf '#define U_BOOT_SPL_VERSION "%s %s%s"\n' \
+			$(if $(CONFIG_SPL_IMAGENAME),$(CONFIG_SPL_IMAGENAME),"U-Boot SPL") \
+			"$(U_BOOT_VERSION)" "$${localvers}" ; \
 		) > [email protected]
 		@( printf '#define CC_VERSION_STRING "%s"\n' \
 		 '$(shell $(CC) --version | head -n 1)' )>>  [email protected]

Signature · 1 line

1.8.0

Vipin Kumar

author

2012-12-06 09:29 UTC #1 lore raw

PATCH RESEND 2/2 arm/boards: Define a new config option CONFIG_BOOT_PARAMS_P

A lot of ARM boards are using board_init routine just to initialize boot_params variable in the global data structure.

This patch lets the board config files to define a CONFIG_BOOT_PARAMS_P option which is assigned to gd->bd->bi_boot_params automatically

Consequently, many board_init routines would not be required in the respective board directories and a weak definition becomes necessary before their removal from the code.

Signed-off-by Vipin Kumar <[email protected]>

---
 README               |  6 ++++++
 arch/arm/lib/board.c | 12 ++++++++++++
 2 files changed, 18 insertions(+)

README +6 -0

index 037513a..2077c3b 100644
@@ -550,6 +550,12 @@ The following options need to be configured:
 		in a single configuration file and the machine type is
 		runtime discoverable, do not have to use this setting.
 
+		CONFIG_BOOT_PARAMS_P		[relevant for ARM only]
+
+		This config option can provide a way to initialize
+		bi_boot_params from the u-boot infrastructure itself. The
+		board still has the option to override it in board_init routine
+
 - vxWorks boot parameters:
 
 		bootvx constructs a valid bootline using the following

arch/arm/lib/board.c +12 -0

index 92cad9a..fa161b8 100644
@@ -399,6 +399,11 @@ void board_init_f(ulong bootflag)
 	gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
 #endif
 
+#ifdef CONFIG_BOOT_PARAMS_P
+	/* Boot params passed to Linux */
+	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
+#endif
+
 	addr_sp -= sizeof (gd_t);
 	id = (gd_t *) addr_sp;
 	debug("Reserving %zu Bytes for Global Data at: %08lx\n",

@@ -468,6 +473,13 @@ void board_init_f(ulong bootflag)
 static char *failed = "*** failed ***\n";
 #endif
 
+static int __def_board_init(bd_t *bis)
+{
+	return -1;
+}
+
+int board_init(void) __attribute__((weak, alias("__def_board_init")));
+
 /*
  ************************************************************************
  *

Signature · 1 line

1.8.0

Stefan Roese

2012-12-06 09:44 UTC #2 lore raw

PATCH RESEND 2/2 arm/boards: Define a new config option CONFIG_BOOT_PARAMS_P

Quoted 38 lines · Vipin Kumar

A lot of ARM boards are using board_init routine just to initialize boot_params variable in the global data structure.
This patch lets the board config files to define a CONFIG_BOOT_PARAMS_P option which is assigned to gd->bd->bi_boot_params automatically
Consequently, many board_init routines would not be required in the respective board directories and a weak definition becomes necessary before their removal from the code.
Signed-off-by Vipin Kumar <[email protected]>
---
 README               |  6 ++++++
 arch/arm/lib/board.c | 12 ++++++++++++
 2 files changed, 18 insertions(+)
README +6 -0
index 037513a..2077c3b 100644
@@ -550,6 +550,12 @@ The following options need to be configured:
 		in a single configuration file and the machine type is
 		runtime discoverable, do not have to use this setting.
 
+		CONFIG_BOOT_PARAMS_P		[relevant for ARM only]
+
+		This config option can provide a way to initialize
+		bi_boot_params from the u-boot infrastructure itself. The
+		board still has the option to override it in board_init routine
+
 - vxWorks boot parameters:
 
 		bootvx constructs a valid bootline using the following
arch/arm/lib/board.c +0 -0
index 92cad9a..fa161b8 100644

arch/arm/lib/board.c void board_init_f(ulong bootflag) · line 405

Quoted 7 lines

arch/arm/lib/board.c +4 -0

@@ -399,6 +399,11 @@ void board_init_f(ulong bootflag)
 	gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
 #endif
 
+#ifdef CONFIG_BOOT_PARAMS_P
+	/* Boot params passed to Linux */
+	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
+#endif

Again an ugly #ifdef. Why not something like this instead:

Define a default earlier in the code (is 0x100 the best default?):

#ifndef CONFIG_BOOT_PARAMS_P
#define CONFIG_BOOT_PARAMS_P 0x100
#endif

then here just:

	/* Boot params passed to Linux */
	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;

without the #ifdef.

arch/arm/lib/board.c void board_init_f(ulong bootflag) · line 478

Quoted 11 lines

arch/arm/lib/board.c +4 -0

+
 	addr_sp -= sizeof (gd_t);
 	id = (gd_t *) addr_sp;
 	debug("Reserving %zu Bytes for Global Data at: %08lx\n",

@@ -468,6 +473,13 @@ void board_init_f(ulong bootflag)
 static char *failed = "*** failed ***\n";
 #endif
 
+static int __def_board_init(bd_t *bis)
+{
+	return -1;

Is -1 a good value to return as default board_init()?

arch/arm/lib/board.c void board_init_f(ulong bootflag) · line 482

Quoted 4 lines

arch/arm/lib/board.c +4 -0

+}
+
+int board_init(void) __attribute__((weak, alias("__def_board_init")));
+

Use __weak from inlcude/linux/compiler.h instead:

int __weak board_init(bd_t *bis)
...

Thanks,
Stefan

Vipin Kumar

author

2012-12-06 09:56 UTC #3 lore raw

PATCH RESEND 2/2 arm/boards: Define a new config option CONFIG_BOOT_PARAMS_P

Quoted 51 lines · Stefan Roese

Quoted 38 lines · Vipin Kumar
A lot of ARM boards are using board_init routine just to initialize boot_params variable in the global data structure.
This patch lets the board config files to define a CONFIG_BOOT_PARAMS_P option which is assigned to gd->bd->bi_boot_params automatically
Consequently, many board_init routines would not be required in the respective board directories and a weak definition becomes necessary before their removal from the code.
Signed-off-by Vipin Kumar<[email protected]>
---
  README               |  6 ++++++
  arch/arm/lib/board.c | 12 ++++++++++++
  2 files changed, 18 insertions(+)
README +6 -0
index 037513a..2077c3b 100644
@@ -550,6 +550,12 @@ The following options need to be configured:
  		in a single configuration file and the machine type is
  		runtime discoverable, do not have to use this setting.
 
+		CONFIG_BOOT_PARAMS_P		[relevant for ARM only]
+
+		This config option can provide a way to initialize
+		bi_boot_params from the u-boot infrastructure itself. The
+		board still has the option to override it in board_init routine
+
  - vxWorks boot parameters:
 
  		bootvx constructs a valid bootline using the following
arch/arm/lib/board.c +0 -0
index 92cad9a..fa161b8 100644
arch/arm/lib/board.c void board_init_f(ulong bootflag) · line 405
Quoted 7 lines
arch/arm/lib/board.c +4 -0
@@ -399,6 +399,11 @@ void board_init_f(ulong bootflag)
  	gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
  #endif
 
+#ifdef CONFIG_BOOT_PARAMS_P
+	/* Boot params passed to Linux */
+	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
+#endif
Again an ugly #ifdef. Why not something like this instead:
Define a default earlier in the code (is 0x100 the best default?):

Quoted 9 lines

#ifndef CONFIG_BOOT_PARAMS_P
#define CONFIG_BOOT_PARAMS_P 0x100
#endif
then here just:
	/* Boot params passed to Linux */
	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;

This would mean that I am forcing the boot params at 0x100 for all boards. Is that the right thing to do

Off-course, all of them might already be initializing gd->bd->bi_boot_params on their own

Quoted 10 lines

without the #ifdef.

Quoted 8 lines

arch/arm/lib/board.c +1 -0

+
  	addr_sp -= sizeof (gd_t);
  	id = (gd_t *) addr_sp;
  	debug("Reserving %zu Bytes for Global Data at: %08lx\n",

@@ -468,6 +473,13 @@ void board_init_f(ulong bootflag)
  static char *failed = "*** failed ***\n";
  #endif

Quoted 6 lines

arch/arm/lib/board.c void board_init_f(ulong bootflag) · line 478
Quoted 3 lines
arch/arm/lib/board.c +3 -0
+static int __def_board_init(bd_t *bis)
+{
+	return -1;
Is -1 a good value to return as default board_init()?

The return value is not checked as of today

Quoted 9 lines

arch/arm/lib/board.c void board_init_f(ulong bootflag) · line 482
Quoted 4 lines
arch/arm/lib/board.c +4 -0
+}
+
+int board_init(void) __attribute__((weak, alias("__def_board_init")));
+
Use __weak from inlcude/linux/compiler.h instead:
int __weak board_init(bd_t *bis)
...

OK, I would do that in v2

Vipin

Quoted 6 lines

Thanks,
Stefan
.

Stefan Roese

2012-12-06 11:26 UTC #4 lore raw

PATCH RESEND 2/2 arm/boards: Define a new config option CONFIG_BOOT_PARAMS_P

On 12/06/2012 10:56 AM, Vipin Kumar wrote:

<snip>

Quoted 19 lines

Quoted 9 lines
arch/arm/lib/board.c void board_init_f(ulong bootflag) · line 405
Quoted 4 lines
arch/arm/lib/board.c +4 -0
+#ifdef CONFIG_BOOT_PARAMS_P
+	/* Boot params passed to Linux */
+	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
+#endif
Again an ugly #ifdef. Why not something like this instead:
Define a default earlier in the code (is 0x100 the best default?):
Quoted 9 lines
#ifndef CONFIG_BOOT_PARAMS_P
#define CONFIG_BOOT_PARAMS_P 0x100
#endif
then here just:
	/* Boot params passed to Linux */
	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;

Quoted 5 lines

This would mean that I am forcing the boot params at 0x100 for all boards. Is that the right thing to do
Off-course, all of them might already be initializing gd->bd->bi_boot_params on their own

Yes. Before or after this initialization? Will the board_init code overwrite this one here?

But I also think this could be confusing. The generic way to implement this would be to remove *all* local board specific assignments of gd->bd->bi_boot_params and define CONFIG_BOOT_PARAMS_P in their board config header. But this will be quite intrusive and could be error-prone.

Quoted 10 lines

without the #ifdef.

Quoted 8 lines

arch/arm/lib/board.c +1 -0

+
  	addr_sp -= sizeof (gd_t);
  	id = (gd_t *) addr_sp;
  	debug("Reserving %zu Bytes for Global Data at: %08lx\n",

@@ -468,6 +473,13 @@ void board_init_f(ulong bootflag)
  static char *failed = "*** failed ***\n";
  #endif

Quoted 8 lines

Quoted 6 lines
arch/arm/lib/board.c void board_init_f(ulong bootflag) · line 478
Quoted 3 lines
arch/arm/lib/board.c +3 -0
+static int __def_board_init(bd_t *bis)
+{
+	return -1;
Is -1 a good value to return as default board_init()?
The return value is not checked as of today

Then I suggest to use 0.

Thanks,
Stefan

Vipin Kumar

author

2012-12-07 09:40 UTC #5 lore raw

PATCH RESEND 2/2 arm/boards: Define a new config option CONFIG_BOOT_PARAMS_P

Quoted 23 lines · Stefan Roese

On 12/06/2012 10:56 AM, Vipin Kumar wrote:
<snip>
Quoted 19 lines
Quoted 18 lines
arch/arm/lib/board.c void board_init_f(ulong bootflag) · line 405
Quoted 4 lines
arch/arm/lib/board.c +4 -0
+#ifdef CONFIG_BOOT_PARAMS_P
+	/* Boot params passed to Linux */
+	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
+#endif
Again an ugly #ifdef. Why not something like this instead:
Define a default earlier in the code (is 0x100 the best default?):
#ifndef CONFIG_BOOT_PARAMS_P
#define CONFIG_BOOT_PARAMS_P 0x100
#endif
then here just:
	/* Boot params passed to Linux */
	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;

Quoted 9 lines

Quoted 5 lines
This would mean that I am forcing the boot params at 0x100 for all boards. Is that the right thing to do
Off-course, all of them might already be initializing gd->bd->bi_boot_params on their own
Yes. Before or after this initialization? Will the board_init code overwrite this one here?

board_init would overwrite it as it is called after relocation from board_init_r while this routine is called even before relocation

Quoted 5 lines

But I also think this could be confusing. The generic way to implement this would be to remove *all* local board specific assignments of gd->bd->bi_boot_params and define CONFIG_BOOT_PARAMS_P in their board config header. But this will be quite intrusive and could be error-prone.

Yes, that is what I also thought..
Originally, I based my patch on the CONFIG_MACH_TYPE handling. If it is defined the board need not initialize gd->bd->bi_arch_number

Can you please suggest what is the best way here. Wolfgang, Albert ?

Quoted 17 lines

Quoted 17 lines
Quoted 16 lines
without the #ifdef.
arch/arm/lib/board.c void board_init_f(ulong bootflag) · line 478
Quoted 11 lines
arch/arm/lib/board.c +4 -0
+
   	addr_sp -= sizeof (gd_t);
   	id = (gd_t *) addr_sp;
   	debug("Reserving %zu Bytes for Global Data at: %08lx\n",
@@ -468,6 +473,13 @@ void board_init_f(ulong bootflag)
   static char *failed = "*** failed ***\n";
   #endif
 
+static int __def_board_init(bd_t *bis)
+{
+	return -1;
Is -1 a good value to return as default board_init()?

Quoted 4 lines

Quoted 1 line
The return value is not checked as of today
Then I suggest to use 0.

OK. Accepted

Quoted 5 lines

Thanks,
Stefan
.

Wolfgang Denk

2012-12-07 14:47 UTC #6 lore raw

PATCH RESEND 2/2 arm/boards: Define a new config option CONFIG_BOOT_PARAMS_P

Dear Vipin Kumar,

Quoted 2 lines · In message <[email protected]> you wrote:

Can you please suggest what is the best way here. Wolfgang, Albert ?

I don't like this patch at all. It introduces yet more
architecture specific stuff to lib/board.c, while we actually should be working on coming up with a common version for all architectures.

If you want to simplify code, then please not by making it worse in other places.

Best regards,

Wolfgang Denk

Signature · 8 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"It's hard to believe that something which is neither seen  nor  felt
can do so much harm."
"That's true. But an idea can't be seen or felt. And that's what kept
the Troglytes in the mines all these centuries. A mistaken idea."
	-- Vanna and Kirk, "The Cloud Minders", stardate 5819.0

Vipin Kumar

author

2012-12-17 08:15 UTC #7 lore raw

PATCH RESEND 2/2 arm/boards: Define a new config option CONFIG_BOOT_PARAMS_P

Quoted 4 lines · Wolfgang Denk

Dear Vipin Kumar,
Quoted 1 line · In message<[email protected]> you wrote:

Quoted 9 lines

Quoted 1 line
Can you please suggest what is the best way here. Wolfgang, Albert ?
I don't like this patch at all. It introduces yet more
architecture specific stuff to lib/board.c, while we actually should be working on coming up with a common version for all architectures.
If you want to simplify code, then please not by making it worse in other places.

I agree, but this code adds an option to pass boot params pointer to board descriptor.
Just two lines above the intended change, there is a mach_type initialization based on the definition of CONFIG_MACH_TYPE. Moreover, the lib/board.c exists in arch/arm as of today which means that it can contain all the stuff common for arm boards

If I am right, the patch should also be OK

Vipin

Quoted 4 lines

Best regards,
Wolfgang Denk

Stefan Roese

2012-12-06 12:08 UTC #8 lore raw

PATCH RESEND 1/2 u-boot/spl: Add u-boot-spl.img to u-boot targets

In reply to Vipin Kumar

Quoted 1 line · Vipin Kumar

Signed-off-by Vipin Kumar <[email protected]>

Is this new build target really needed? Please take a look at the recently added/renamed targets (e.g. u-boot-with-spl.bin or u-boot-img.bin).

If this does not fit, then please add a short description about this new build target and which SoC's need it and why.

Thanks,
STefan

Vipin Kumar

author

2012-12-07 09:47 UTC #9 lore raw

PATCH RESEND 1/2 u-boot/spl: Add u-boot-spl.img to u-boot targets

Quoted 6 lines · Stefan Roese

Quoted 1 line · Vipin Kumar
Signed-off-by Vipin Kumar<[email protected]>
Is this new build target really needed? Please take a look at the recently added/renamed targets (e.g. u-boot-with-spl.bin or u-boot-img.bin).

Hmm, this is an mkimage version of spl binary. This mkimage header is detected by the BootROM in spear devices for loading and booting the SPL binary

Quoted 3 lines

If this does not fit, then please add a short description about this new build target and which SoC's need it and why.

You mean add a short description in commit log or in README ?

Vipin

Quoted 4 lines

Thanks,
STefan

Stefan Roese

2012-12-07 10:44 UTC #10 lore raw

PATCH RESEND 1/2 u-boot/spl: Add u-boot-spl.img to u-boot targets

Quoted 11 lines · Vipin Kumar

Quoted 6 lines · Stefan Roese
Quoted 1 line · Vipin Kumar
Signed-off-by Vipin Kumar<[email protected]>
Is this new build target really needed? Please take a look at the recently added/renamed targets (e.g. u-boot-with-spl.bin or u-boot-img.bin).
Hmm, this is an mkimage version of spl binary. This mkimage header is detected by the BootROM in spear devices for loading and booting the SPL binary

Yes, I remember this from the SPEAr600. I introduced a new build target for this: u-boot.spr. Take a look at the commit:

94aebe6cc3111c7c9e3cd1311cc9793d01cc3ded

Makefile: Add u-boot.spr build target (SPEAr)

On x600 (SPEAr600) U-Boot is appended to U-Boot SPL. Both images are created using mkimage (crc etc), so that the ROM bootloader can check its integrity. Padding needs to be done to the SPL image (with mkimage header) and not the binary. Otherwise the resulting image which is loaded/copied by the ROM bootloader to SRAM doesn't fit. The resulting image containing both U-Boot images is called u-boot.spr.

Or does your patch "only" introduce the mkimage SPL image version?

Then please consolidate this with the u-boot.spr build target.

Thanks,
Stefan

Vipin Kumar

author

2012-12-07 11:55 UTC #11 lore raw

PATCH RESEND 1/2 u-boot/spl: Add u-boot-spl.img to u-boot targets

Quoted 9 lines · Stefan Roese

Quoted 8 lines · Vipin Kumar
Quoted 6 lines · Stefan Roese
Quoted 1 line · Vipin Kumar
Signed-off-by Vipin Kumar<[email protected]>
Is this new build target really needed? Please take a look at the recently added/renamed targets (e.g. u-boot-with-spl.bin or u-boot-img.bin).

Quoted 7 lines

Quoted 3 lines
Hmm, this is an mkimage version of spl binary. This mkimage header is detected by the BootROM in spear devices for loading and booting the SPL binary
Yes, I remember this from the SPEAr600. I introduced a new build target for this: u-boot.spr. Take a look at the commit:

Yes, I remember

Quoted 12 lines

94aebe6cc3111c7c9e3cd1311cc9793d01cc3ded
Makefile: Add u-boot.spr build target (SPEAr)
On x600 (SPEAr600) U-Boot is appended to U-Boot SPL. Both images are created using mkimage (crc etc), so that the ROM bootloader can check its integrity. Padding needs to be done to the SPL image (with mkimage header) and not the binary. Otherwise the resulting image which is loaded/copied by the ROM bootloader to SRAM doesn't fit. The resulting image containing both U-Boot images is called u-boot.spr.

Quoted 2 lines

Or does your patch "only" introduce the mkimage SPL image version?

Yes, it introduces only mkimage SPL image version. This is necessary for a few booting options where we absolutely require a different SPL binary with an mkimage header

Quoted 2 lines

Then please consolidate this with the u-boot.spr build target.

You mean u-boot.spr can use this u-boot-spl.img. Yes, I will do that in v2

Quoted 4 lines

Thanks,
Stefan
.

Vipin

Stefan Roese

2012-12-07 11:59 UTC #12 lore raw

PATCH RESEND 1/2 u-boot/spl: Add u-boot-spl.img to u-boot targets

Quoted 11 lines · Vipin Kumar

Quoted 2 lines
Or does your patch "only" introduce the mkimage SPL image version?
Yes, it introduces only mkimage SPL image version. This is necessary for a few booting options where we absolutely require a different SPL binary with an mkimage header
Quoted 2 lines
Then please consolidate this with the u-boot.spr build target.
You mean u-boot.spr can use this u-boot-spl.img. Yes, I will do that in v2

Yes, thats what I mean.

Thanks,
Stefan