Revert "ARM: SPL: do not set gd again"

PATCH New Tom Rini 3 participants 7 messages Sep 17, 2014 – Dec 4, 2014 Arm (32-bit)

Share & tools

Tom Rini

author

2014-09-17 00:27 UTC #0 lore raw

PATCH 1/1 Revert "ARM: SPL: do not set gd again"

At the high level, the problem is that we set gd multiple times (and still do, even after the commit we're reverting). We set important parts of gd to the copy which is not above stack but rather in the data section. For the release, we're going to revert this change and for the next release we shall correct things to only, really, set gd once to an appropriate location and ensure that comments about it are correct too.

This reverts commit f0c3a6c4ad09210d5d4aeafe87685ee75e5683d6.

Acked-by Albert Aribaud <[email protected]>

Signed-off-by Tom Rini <[email protected]>

---
 arch/arm/lib/spl.c |    3 +++
 1 file changed, 3 insertions(+)

arch/arm/lib/spl.c +3 -0

index 75ab546..dfcc596 100644
@@ -28,6 +28,9 @@ void __weak board_init_f(ulong dummy)
 	/* Clear the BSS. */
 	memset(__bss_start, 0, __bss_end - __bss_start);
 
+	/* Set global data pointer. */
+	gd = &gdata;
+
 	board_init_r(NULL, 0);
 }

Signature · 1 line

1.7.9.5

Tom Rini

author

2014-09-17 00:47 UTC #1 lore raw

PATCH 1/1 Revert "ARM: SPL: do not set gd again"

Quoted 11 lines · Tom Rini

At the high level, the problem is that we set gd multiple times (and still do, even after the commit we're reverting). We set important parts of gd to the copy which is not above stack but rather in the data section. For the release, we're going to revert this change and for the next release we shall correct things to only, really, set gd once to an appropriate location and ensure that comments about it are correct too.
This reverts commit f0c3a6c4ad09210d5d4aeafe87685ee75e5683d6.
Acked-by Albert Aribaud <[email protected]>
Signed-off-by Tom Rini <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140916/99f7cd63/attachment.pgp>

Simon Glass

2014-11-10 22:13 UTC #2 lore raw

PATCH 1/1 Revert "ARM: SPL: do not set gd again"

+Albert

Hi Tom,

Quoted 10 lines · Tom Rini

Quoted 7 lines · Tom Rini
At the high level, the problem is that we set gd multiple times (and still do, even after the commit we're reverting). We set important parts of gd to the copy which is not above stack but rather in the data section. For the release, we're going to revert this change and for the next release we shall correct things to only, really, set gd once to an appropriate location and ensure that comments about it are correct too.

Quoted 6 lines

Quoted 4 lines
This reverts commit f0c3a6c4ad09210d5d4aeafe87685ee75e5683d6.
Acked-by Albert Aribaud <[email protected]>
Signed-off-by Tom Rini <[email protected]>
Applied to u-boot/master, thanks!

Is this going to be un-reverted? I will need this done to apply the driver model SPL series.

Regards,
Simon

Tom Rini

author

2014-11-11 21:57 UTC #3 lore raw

PATCH 1/1 Revert "ARM: SPL: do not set gd again"

Quoted 17 lines · Simon Glass

+Albert
Hi Tom,
Quoted 12 lines · Tom Rini
Quoted 9 lines · Tom Rini
At the high level, the problem is that we set gd multiple times (and still do, even after the commit we're reverting). We set important parts of gd to the copy which is not above stack but rather in the data section. For the release, we're going to revert this change and for the next release we shall correct things to only, really, set gd once to an appropriate location and ensure that comments about it are correct too.
This reverts commit f0c3a6c4ad09210d5d4aeafe87685ee75e5683d6.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Acked-by Albert Aribaud <[email protected]>
Signed-off-by Tom Rini <[email protected]>
Applied to u-boot/master, thanks!
Is this going to be un-reverted? I will need this done to apply the driver model SPL series.

So I've got am335x working again locally and now I'm trying to see if we need to introduce a SoC-specific board_init_f for SPL here or not or if I can shove save_omap_boot_params() into spl_board_init() and add preloader_console_init rather generically to the ARM board_init_f SPL function. Once I've got this clean enough I need to dust off some davinci and omap3 targets, do similar changes and then see if Hans was right about why my olimex Allwinner board was behaving badly, and if so, test the changes there too. That'll cover most of the ARM boards that re-set gd themselves when they can't with the above change re-introduced.

Signature · 8 lines

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/20141111/a2149796/attachment.pgp>

Simon Glass

2014-11-12 00:59 UTC #4 lore raw

PATCH 1/1 Revert "ARM: SPL: do not set gd again"

Hi Tom,

Quoted 26 lines · Tom Rini

Quoted 17 lines · Simon Glass
+Albert
Hi Tom,
Quoted 12 lines · Tom Rini
Quoted 9 lines · Tom Rini
At the high level, the problem is that we set gd multiple times (and still do, even after the commit we're reverting). We set important parts of gd to the copy which is not above stack but rather in the data section. For the release, we're going to revert this change and for the next release we shall correct things to only, really, set gd once to an appropriate location and ensure that comments about it are correct too.
This reverts commit f0c3a6c4ad09210d5d4aeafe87685ee75e5683d6.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Acked-by Albert Aribaud <[email protected]>
Signed-off-by Tom Rini <[email protected]>
Applied to u-boot/master, thanks!
Is this going to be un-reverted? I will need this done to apply the driver model SPL series.

Quoted 10 lines

So I've got am335x working again locally and now I'm trying to see if we need to introduce a SoC-specific board_init_f for SPL here or not or if I can shove save_omap_boot_params() into spl_board_init() and add preloader_console_init rather generically to the ARM board_init_f SPL function. Once I've got this clean enough I need to dust off some davinci and omap3 targets, do similar changes and then see if Hans was right about why my olimex Allwinner board was behaving badly, and if so, test the changes there too. That'll cover most of the ARM boards that re-set gd themselves when they can't with the above change re-introduced.

OK thanks. I might pull in some of the non-dependent patches in the meantime.

Regards,
Simon

Jeroen Hofstee

2014-11-28 21:48 UTC #5 lore raw

PATCH 1/1 Revert "ARM: SPL: do not set gd again"

In reply to Tom Rini

Hi all,

Quoted 22 lines · Tom Rini

Quoted 14 lines · Simon Glass
+Albert
Hi Tom,
Quoted 9 lines · Tom Rini
Quoted 7 lines · Tom Rini
At the high level, the problem is that we set gd multiple times (and still do, even after the commit we're reverting). We set important parts of gd to the copy which is not above stack but rather in the data section. For the release, we're going to revert this change and for the next release we shall correct things to only, really, set gd once to an appropriate location and ensure that comments about it are correct too.
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
This reverts commit f0c3a6c4ad09210d5d4aeafe87685ee75e5683d6.
Acked-by Albert Aribaud <[email protected]>
Signed-off-by Tom Rini <[email protected]>
Applied to u-boot/master, thanks!
Is this going to be un-reverted? I will need this done to apply the driver model SPL series.

Quoted 12 lines

So I've got am335x working again locally and now I'm trying to see if we need to introduce a SoC-specific board_init_f for SPL here or not or if I can shove save_omap_boot_params() into spl_board_init() and add preloader_console_init rather generically to the ARM board_init_f SPL function. Once I've got this clean enough I need to dust off some davinci and omap3 targets, do similar changes and then see if Hans was right about why my olimex Allwinner board was behaving badly, and if so, test the changes there too. That'll cover most of the ARM boards that re-set gd themselves when they can't with the above change re-introduced.

Any progress on this?

Regards,
Jeroen

Tom Rini

author

2014-12-04 14:16 UTC #6 lore raw

PATCH 1/1 Revert "ARM: SPL: do not set gd again"

Quoted 36 lines · Jeroen Hofstee

Hi all,
Quoted 22 lines · Tom Rini
Quoted 14 lines · Simon Glass
+Albert
Hi Tom,
Quoted 9 lines · Tom Rini
Quoted 7 lines · Tom Rini
At the high level, the problem is that we set gd multiple times (and still do, even after the commit we're reverting). We set important parts of gd to the copy which is not above stack but rather in the data section. For the release, we're going to revert this change and for the next release we shall correct things to only, really, set gd once to an appropriate location and ensure that comments about it are correct too.
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
This reverts commit f0c3a6c4ad09210d5d4aeafe87685ee75e5683d6.
Acked-by Albert Aribaud <[email protected]>
Signed-off-by Tom Rini <[email protected]>
Applied to u-boot/master, thanks!
Is this going to be un-reverted? I will need this done to apply the driver model SPL series.
Quoted 10 lines
So I've got am335x working again locally and now I'm trying to see if we need to introduce a SoC-specific board_init_f for SPL here or not or if I can shove save_omap_boot_params() into spl_board_init() and add preloader_console_init rather generically to the ARM board_init_f SPL function. Once I've got this clean enough I need to dust off some davinci and omap3 targets, do similar changes and then see if Hans was right about why my olimex Allwinner board was behaving badly, and if so, test the changes there too. That'll cover most of the ARM boards that re-set gd themselves when they can't with the above change re-introduced.

Quoted 1 line

Any progress on this?

Not really. I got as far as the following (which of course breaks non-TI ARMv7 boards):

arch/arm/cpu/armv7/am33xx/board.c +0 -3

index 29b1d73..7681025 100644
@@ -302,9 +302,6 @@ void s_init(void)
 	gd->baudrate = CONFIG_BAUDRATE;
 	serial_init();
 	gd->have_console = 1;
-#elif defined(CONFIG_SPL_BUILD)
-	gd = &gdata;
-	preloader_console_init();
 #endif
 #if defined(CONFIG_SPL_AM33XX_ENABLE_RTC32K_OSC)
 	/* Enable RTC32K clock */

arch/arm/lib/spl.c +10 -2

index dfcc596..1e0be2a 100644
@@ -28,8 +28,16 @@ void __weak board_init_f(ulong dummy)
 	/* Clear the BSS. */
 	memset(__bss_start, 0, __bss_end - __bss_start);
 
-	/* Set global data pointer. */
-	gd = &gdata;
+	/*
+	 * Save the boot parameters passed from romcode.
+	 * We cannot delay the saving further than this,
+	 * to prevent overwrites.
+	 */
+#ifdef CONFIG_SPL_BUILD
+	save_omap_boot_params();
+#endif
+
+	preloader_console_init();
 
 	board_init_r(NULL, 0);
 }

It needs testing on more TI boards to make sure there's nothing else missing (and there's probably some similar tweaks needed on other TI platforms even). That we can move the save_omap_boot_params call later (and duplicate calling it, even) bodes fairly well for an easy conversion to perphaps spl_board_init() on these boards.

Signature · 8 lines

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/20141204/920919a1/attachment.pgp>