arm: omap3: Detect boot mode very early

Fixes 4bd754d8abef ("arm: omap: Detect boot mode very early") where the intent was to store the boot params information in a known location and pass it to SPL very early. Unfortunately it didn't account for OMAP3 boards.

This patch adds adds this functionality back into OMAP3 boards.

Reviewed-by Lokesh Vutla <[email protected]>

Signed-off-by Adam Ford <[email protected]>

arch/arm/mach-omap2/omap3/board.c +0 -0

index cd8e302..a61b933 100644
@@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
 {
 	early_system_init();

Quoted 17 lines · On Fri, 14 Jul 2017 08:53:20 -0500 Adam Ford <[email protected]> wrote:

Fixes 4bd754d8abef ("arm: omap: Detect boot mode very early") where the intent was to store the boot params information in a known location and pass it to SPL very early. Unfortunately it didn't account for OMAP3 boards.

This patch adds adds this functionality back into OMAP3 boards.

Reviewed-by Lokesh Vutla <[email protected]>

Signed-off-by Adam Ford <[email protected]>

arch/arm/mach-omap2/omap3/board.c +0 -0

index cd8e302..a61b933 100644
@@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
 {
 	early_system_init();

arch/arm/mach-omap2/omap3/board.c void board_init_f(ulong dummy) · line 223

Quoted 10 lines

arch/arm/mach-omap2/omap3/board.c +6 -0

 	mem_init();
+	/*
+	* Save the boot parameters passed from romcode.
+	* We cannot delay the saving further than this,
+	* to prevent overwrites.
+	*/
+	save_omap_boot_params();
 }
 #endif
 

Hello,

Something seems to be fishy here.

The 4bd754d8abef patch from Lokesh Vutla basically replicated the same chunk of code for every OMAP variant rather than keeping it in one common location. The justification for this code duplication is that the boot parameters may be overwritten. Can we have a bit more details about how and when this overwrite actually happens in practice?

I don't quite like the fact that we still have the code, which relies on OMAP_SRAM_SCRATCH_BOOT_PARAMS in the "jump_to_image_no_args" function very late in the SPL:

Quoted 33 lines · Siarhei Siamashka

Quoted 17 lines · On Fri, 14 Jul 2017 08:53:20 -0500 Adam Ford <[email protected]> wrote:

Fixes 4bd754d8abef ("arm: omap: Detect boot mode very early") where the intent was to store the boot params information in a known location and pass it to SPL very early. Unfortunately it didn't account for OMAP3 boards.

This patch adds adds this functionality back into OMAP3 boards.

Reviewed-by Lokesh Vutla <[email protected]>

Signed-off-by Adam Ford <[email protected]>

arch/arm/mach-omap2/omap3/board.c +0 -0

index cd8e302..a61b933 100644
@@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
 {
 	early_system_init();

arch/arm/mach-omap2/omap3/board.c void board_init_f(ulong dummy) · line 223

Quoted 10 lines

arch/arm/mach-omap2/omap3/board.c +6 -0

 	mem_init();
+	/*
+	* Save the boot parameters passed from romcode.
+	* We cannot delay the saving further than this,
+	* to prevent overwrites.
+	*/
+	save_omap_boot_params();
 }
 #endif
 

Hello,

Quoted 7 lines

Something seems to be fishy here.

The 4bd754d8abef patch from Lokesh Vutla basically replicated the same chunk of code for every OMAP variant rather than keeping it in one common location. The justification for this code duplication is that the boot parameters may be overwritten. Can we have a bit more details about how and when this overwrite actually happens in practice?

ROM stores the boot_params info in SRAM and passed this address to SPL. For SPL, all the dtb, malloc, stack, and scratch area are in SRAM. There is high probability that it might override the boot params info.

So, we need to parse this info asap. I did see this is being overwritten on dra7 evm

so I had to introduce this patch.

You can always dump R0 at the beiginning of SPL and compare it with objdump of spl.

Quoted 4 lines

I don't quite like the fact that we still have the code, which relies on OMAP_SRAM_SCRATCH_BOOT_PARAMS in the "jump_to_image_no_args" function very late in the SPL:

I don't think U-Boot uses this information at all, so till now there is no effect. You can as well not pass anything.

Quoted 47 lines · On Tue, 25 Jul 2017 12:00:05 +0530 Lokesh Vutla <[email protected]> wrote:

Quoted 37 lines · Siarhei Siamashka

Quoted 19 lines · On Fri, 14 Jul 2017 08:53:20 -0500 Adam Ford <[email protected]> wrote:

Fixes 4bd754d8abef ("arm: omap: Detect boot mode very early") where
the intent was to store the boot params information in a known location and pass it to SPL very early. Unfortunately it didn't account for OMAP3 boards.

This patch adds adds this functionality back into OMAP3 boards.

Reviewed-by Lokesh Vutla <[email protected]>

Signed-off-by Adam Ford <[email protected]>

arch/arm/mach- renamed from arch/arm/mach-omap2/omap3/board.c +0 -0

omap2/omap3/board.c
index cd8e302..a61b933 100644

arch/arm/mach-omap2/omap3/board.c +0 -0

@@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
 {
 	early_system_init();

arch/arm/mach-omap2/omap3/board.c void board_init_f(ulong dummy) · line 223

Quoted 10 lines

arch/arm/mach-omap2/omap3/board.c +6 -0

 	mem_init();
+	/*
+	* Save the boot parameters passed from romcode.
+	* We cannot delay the saving further than this,
+	* to prevent overwrites.
+	*/
+	save_omap_boot_params();
 }
 #endif
 

Hello,

Something seems to be fishy here.

Quoted 8 lines

The 4bd754d8abef patch from Lokesh Vutla basically replicated the same
chunk of code for every OMAP variant rather than keeping it in one common location. The justification for this code duplication is that
the boot parameters may be overwritten. Can we have a bit more details
about how and when this overwrite actually happens in practice?

Quoted 5 lines

ROM stores the boot_params info in SRAM and passed this address to SPL.
For SPL, all the dtb, malloc, stack, and scratch area are in SRAM. There
is high probability that it might override the boot params info.

This is a somewhat vague description. We are supposed to know where all this stuff is located and have a reasonably good control over it.

OMAP_SRAM_SCRATCH_BOOT_PARAMS appears to be a part of the 1K scratch area in the SRAM, which is located right above the loaded SPL image. So if something is able to unexpectedly overwrite OMAP_SRAM_SCRATCH_BOOT_PARAMS, then there might be a risk that some other important code or data near the end of the SPL image may be overwritten too.

Quoted 3 lines

So, we need to parse this info asap. I did see this is being overwritten on
dra7 evm

Thanks for providing this information. You did not mention it in your commit message before. This dra7 evm is an OMAP5 board, right? And in 'arch/arm/include/asm/arch-omap5/omap.h' I can see the following defines:

/*
* In all cases, the TRM defines the RAM Memory Map for the processor
* and indicates the area for the downloaded image. We use all of
that
* space for download and once up and running may use other parts of
the
* map for our needs. We set a scratch space that is at the end of
the
* OMAP5 download area, but within the DRA7xx download area (as it is
* much larger) and do not, at this time, make use of the additional
* space.
*/
#if defined(CONFIG_DRA7XX)
#define NON_SECURE_SRAM_START 0x40300000
#define NON_SECURE_SRAM_END 0x40380000 /* Not inclusive
*/
#define NON_SECURE_SRAM_IMG_END 0x4037C000
#else
#define NON_SECURE_SRAM_START 0x40300000
#define NON_SECURE_SRAM_END 0x40320000 /* Not inclusive
*/
#define NON_SECURE_SRAM_IMG_END 0x4031E000
#endif
#define SRAM_SCRATCH_SPACE_ADDR (NON_SECURE_SRAM_IMG_END -
SZ_1K)

Unlike what we have in a similar header for OMAP3, LOW_LEVEL_SRAM_STACK
is not defined anywhere. Instead of it, we end up having:

#define CONFIG_SYS_INIT_SP_ADDR (NON_SECURE_SRAM_END - GENERATED_GBL_DATA_SIZE)

And it appears to be set to 0x4037FF20 in the SPL binary (at least in my dra7xx_evm_defconfig build). So the initial stack is above the scratch area and has size 0x3F20. The SPL code explicitly initializes the SP register in the very beginning. Except for one glitch, which I have reported in a separate e-mail:

    https://lists.denx.de/pipermail/u-boot/2017-July/299446.html

Quoted 1 line

so I had to introduce this patch.

Still I would like to know what exactly is overwriting the boot parameters on your dra7 evm board and why this did not happen on other boards.

Quoted 9 lines

You can always dump R0 at the beiginning of SPL and compare it with objdump of spl.

Quoted 5 lines

I don't quite like the fact that we still have the code, which relies
on OMAP_SRAM_SCRATCH_BOOT_PARAMS in the "jump_to_image_no_args" function very late in the SPL:

Quoted 3 lines

I don't think U-Boot uses this information at all, so till now there is
no effect. You can as well not pass anything.

If it is not used, then maybe we should remove this code?

Quoted 47 lines · On Tue, 25 Jul 2017 12:00:05 +0530 Lokesh Vutla <[email protected]> wrote:

Quoted 37 lines · Siarhei Siamashka

Quoted 19 lines · On Fri, 14 Jul 2017 08:53:20 -0500 Adam Ford <[email protected]> wrote:

Fixes 4bd754d8abef ("arm: omap: Detect boot mode very early") where
the intent was to store the boot params information in a known location and pass it to SPL very early. Unfortunately it didn't account for OMAP3 boards.

This patch adds adds this functionality back into OMAP3 boards.

Reviewed-by Lokesh Vutla <[email protected]>

Signed-off-by Adam Ford <[email protected]>

arch/arm/mach- renamed from arch/arm/mach-omap2/omap3/board.c +0 -0

omap2/omap3/board.c
index cd8e302..a61b933 100644

arch/arm/mach-omap2/omap3/board.c +0 -0

@@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
 {
        early_system_init();

arch/arm/mach-omap2/omap3/board.c void board_init_f(ulong dummy) · line 223

Quoted 10 lines

arch/arm/mach-omap2/omap3/board.c +6 -0

        mem_init();
+       /*
+       * Save the boot parameters passed from romcode.
+       * We cannot delay the saving further than this,
+       * to prevent overwrites.
+       */
+       save_omap_boot_params();
 }
 #endif
 

Hello,

Something seems to be fishy here.

Quoted 8 lines

The 4bd754d8abef patch from Lokesh Vutla basically replicated the same
chunk of code for every OMAP variant rather than keeping it in one common location. The justification for this code duplication is that
the boot parameters may be overwritten. Can we have a bit more details
about how and when this overwrite actually happens in practice?

Quoted 5 lines

ROM stores the boot_params info in SRAM and passed this address to SPL.
For SPL, all the dtb, malloc, stack, and scratch area are in SRAM. There
is high probability that it might override the boot params info.

This is a somewhat vague description. We are supposed to know where all this stuff is located and have a reasonably good control over it.

OMAP_SRAM_SCRATCH_BOOT_PARAMS appears to be a part of the 1K scratch area in the SRAM, which is located right above the loaded SPL image. So if something is able to unexpectedly overwrite OMAP_SRAM_SCRATCH_BOOT_PARAMS, then there might be a risk that some other important code or data near the end of the SPL image may be overwritten too.

Quoted 3 lines

So, we need to parse this info asap. I did see this is being overwritten on
dra7 evm

Thanks for providing this information. You did not mention it in your commit message before. This dra7 evm is an OMAP5 board, right? And in 'arch/arm/include/asm/arch-omap5/omap.h' I can see the following defines:

/*
* In all cases, the TRM defines the RAM Memory Map for the processor
* and indicates the area for the downloaded image. We use all of
that
* space for download and once up and running may use other parts of
the
* map for our needs. We set a scratch space that is at the end of
the
* OMAP5 download area, but within the DRA7xx download area (as it is
* much larger) and do not, at this time, make use of the additional
* space.
*/
#if defined(CONFIG_DRA7XX)
#define NON_SECURE_SRAM_START 0x40300000
#define NON_SECURE_SRAM_END 0x40380000 /* Not inclusive
*/
#define NON_SECURE_SRAM_IMG_END 0x4037C000
#else
#define NON_SECURE_SRAM_START 0x40300000
#define NON_SECURE_SRAM_END 0x40320000 /* Not inclusive
*/
#define NON_SECURE_SRAM_IMG_END 0x4031E000
#endif
#define SRAM_SCRATCH_SPACE_ADDR (NON_SECURE_SRAM_IMG_END -
SZ_1K)

Unlike what we have in a similar header for OMAP3, LOW_LEVEL_SRAM_STACK
is not defined anywhere. Instead of it, we end up having:

#define CONFIG_SYS_INIT_SP_ADDR (NON_SECURE_SRAM_END - GENERATED_GBL_DATA_SIZE)

And it appears to be set to 0x4037FF20 in the SPL binary (at least in my dra7xx_evm_defconfig build). So the initial stack is above the scratch area and has size 0x3F20. The SPL code explicitly initializes the SP register in the very beginning. Except for one glitch, which I have reported in a separate e-mail:

    https://lists.denx.de/pipermail/u-boot/2017-July/299446.html

Quoted 1 line

so I had to introduce this patch.

Still I would like to know what exactly is overwriting the boot parameters on your dra7 evm board and why this did not happen on other boards.

Quoted 9 lines

You can always dump R0 at the beiginning of SPL and compare it with objdump of spl.

Quoted 5 lines

I don't quite like the fact that we still have the code, which relies
on OMAP_SRAM_SCRATCH_BOOT_PARAMS in the "jump_to_image_no_args" function very late in the SPL:

Quoted 3 lines

I don't think U-Boot uses this information at all, so till now there is
no effect. You can as well not pass anything.

If it is not used, then maybe we should remove this code?

Quoted 170 lines · Paul Kocialkowski

Quoted 8 lines · Siarhei Siamashka

Quoted 5 lines · On Tue, 25 Jul 2017 12:00:05 +0530 Lokesh Vutla <[email protected]> wrote:

Quoted 4 lines · Siarhei Siamashka

Quoted 1 line · On Fri, 14 Jul 2017 08:53:20 -0500 Adam Ford <[email protected]> wrote:

Fixes 4bd754d8abef ("arm: omap: Detect boot mode very

early")

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

where
the intent was to store the boot params information in a

known

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

location and pass it to SPL very early. Unfortunately it

didn't

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

account for OMAP3 boards.

This patch adds adds this functionality back into OMAP3

boards.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

Reviewed-by Lokesh Vutla <[email protected]>

Signed-off-by Adam Ford <[email protected]>

(quoted diff) +0 -0

b/arch/arm/mach-

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 7 lines

omap2/omap3/board.c
index cd8e302..a61b933 100644

arch/arm/mach-omap2/omap3/board.c +0 -0

@@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
 {
        early_system_init();

arch/arm/mach-omap2/omap3/board.c void board_init_f(ulong dummy) · line 223

Quoted 10 lines

arch/arm/mach-omap2/omap3/board.c +6 -0

        mem_init();
+       /*
+       * Save the boot parameters passed from romcode.
+       * We cannot delay the saving further than this,
+       * to prevent overwrites.
+       */
+       save_omap_boot_params();
 }
 #endif
 

Hello,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Something seems to be fishy here.

The 4bd754d8abef patch from Lokesh Vutla basically replicated

the

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

same
chunk of code for every OMAP variant rather than keeping it in

one

Quoted 1 line

Quoted 1 line

Quoted 1 line

common location. The justification for this code duplication

is

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

that
the boot parameters may be overwritten. Can we have a bit more details
about how and when this overwrite actually happens in

practice?

Quoted 2 lines

Quoted 2 lines

ROM stores the boot_params info in SRAM and passed this address

to

Quoted 2 lines

Quoted 2 lines

SPL.
For SPL, all the dtb, malloc, stack, and scratch area are in

SRAM.

Quoted 4 lines

Quoted 2 lines

There
is high probability that it might override the boot params info.

This is a somewhat vague description. We are supposed to know

where

Quoted 1 line

all this stuff is located and have a reasonably good control over

it.

Quoted 8 lines

OMAP_SRAM_SCRATCH_BOOT_PARAMS appears to be a part of the 1K scratch area in the SRAM, which is located right above the loaded SPL image. So if something is able to unexpectedly overwrite OMAP_SRAM_SCRATCH_BOOT_PARAMS, then there might be a risk that some other important code or data near the end of the SPL image may be overwritten too.

Quoted 5 lines

Quoted 3 lines

So, we need to parse this info asap. I did see this is being overwritten on
dra7 evm

Thanks for providing this information. You did not mention it in

your

Quoted 1 line

commit message before. This dra7 evm is an OMAP5 board, right? And

in

Quoted 5 lines

'arch/arm/include/asm/arch-omap5/omap.h' I can see the following defines:

/*
* In all cases, the TRM defines the RAM Memory Map for the

processor

Quoted 3 lines

* and indicates the area for the downloaded image. We use all of
that
* space for download and once up and running may use other parts

of

Quoted 2 lines

the
* map for our needs. We set a scratch space that is at the end

of

Quoted 2 lines

the
* OMAP5 download area, but within the DRA7xx download area (as it

is

Quoted 1 line

* much larger) and do not, at this time, make use of the

additional

Quoted 17 lines

* space.
*/
#if defined(CONFIG_DRA7XX)
#define NON_SECURE_SRAM_START 0x40300000
#define NON_SECURE_SRAM_END 0x40380000 /* Not inclusive
*/
#define NON_SECURE_SRAM_IMG_END 0x4037C000
#else
#define NON_SECURE_SRAM_START 0x40300000
#define NON_SECURE_SRAM_END 0x40320000 /* Not inclusive
*/
#define NON_SECURE_SRAM_IMG_END 0x4031E000
#endif
#define SRAM_SCRATCH_SPACE_ADDR (NON_SECURE_SRAM_IMG_END -
SZ_1K)

Quoted 8 lines

Unlike what we have in a similar header for OMAP3, LOW_LEVEL_SRAM_STACK
is not defined anywhere. Instead of it, we end up having:

#define CONFIG_SYS_INIT_SP_ADDR (NON_SECURE_SRAM_END - GENERATED_GBL_DATA_SIZE)

And it appears to be set to 0x4037FF20 in the SPL binary (at least

in

Quoted 2 lines

my dra7xx_evm_defconfig build). So the initial stack is above the scratch area and has size 0x3F20. The SPL code explicitly

initializes

Quoted 1 line

the SP register in the very beginning. Except for one glitch,

which I

Quoted 17 lines

have reported in a separate e-mail:

    https://lists.denx.de/pipermail/u-boot/2017-July/299446.html

Quoted 1 line

so I had to introduce this patch.

Still I would like to know what exactly is overwriting the boot parameters on your dra7 evm board and why this did not happen on other boards.

Quoted 7 lines

You can always dump R0 at the beiginning of SPL and compare it with objdump of spl.

Quoted 4 lines

I don't quite like the fact that we still have the code, which relies
on OMAP_SRAM_SCRATCH_BOOT_PARAMS in the

"jump_to_image_no_args"

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

function very late in the SPL:

I don't think U-Boot uses this information at all, so till now

there

Quoted 4 lines

Quoted 2 lines

is
no effect. You can as well not pass anything.

If it is not used, then maybe we should remove this code?

That's a good point, I don't think this is needed at all, although that
should definitely be tested on actual hardware before being submitted.

Quoted 3 lines

Are you saying we don't need the patch or we just don't need a portion of it? Without this patch, SPL cannot load U-Boot on my OMAp3630

Quoted 3 lines

From what I can see, the omap-specific save_boot_params is only ever defined in SPL context, so that information is just ignored otherwise.