[U-Boot] [PATCH] arm: omap3: Detect boot mode very early

[U-Boot] [PATCH] arm: omap3: Detect boot mode very early

[Adam Ford]

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 <lokeshvutla@ti.com>
Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/arch/arm/mach-omap2/omap3/board.c b/arch/arm/mach-omap2/omap3/board.c
index cd8e302..a61b933 100644
--- a/arch/arm/mach-omap2/omap3/board.c
+++ b/arch/arm/mach-omap2/omap3/board.c
@@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
 {
 	early_system_init();
 	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
 
-- 
2.7.4

[U-Boot] arm: omap3: Detect boot mode very early

[Tom Rini]

On Fri, Jul 14, 2017 at 08:53:20AM -0500, Adam Ford 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 <lokeshvutla@ti.com>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/arch/arm/mach-omap2/omap3/board.c b/arch/arm/mach-omap2/omap3/board.c
> index cd8e302..a61b933 100644

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170724/1debb197/attachment.sig>

[U-Boot] [PATCH] arm: omap3: Detect boot mode very early

[Siarhei Siamashka]

On Fri, 14 Jul 2017 08:53:20 -0500
Adam Ford <aford173@gmail.com> 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 <lokeshvutla@ti.com>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/arch/arm/mach-omap2/omap3/board.c b/arch/arm/mach-omap2/omap3/board.c
> index cd8e302..a61b933 100644
> --- a/arch/arm/mach-omap2/omap3/board.c
> +++ b/arch/arm/mach-omap2/omap3/board.c
> @@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
>  {
>  	early_system_init();
>  	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:

void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
{
	typedef void __noreturn (*image_entry_noargs_t)(u32 *);
	image_entry_noargs_t image_entry =
			(image_entry_noargs_t) spl_image->entry_point;

	u32 boot_params = *((u32 *)OMAP_SRAM_SCRATCH_BOOT_PARAMS);

	debug("image entry point: 0x%lX\n", spl_image->entry_point);
	/* Pass the saved boot_params from rom code */
	image_entry((u32 *)boot_params);
}


This code had been introduced by Paul Kocialkowski in the old
patch 60c7c30aa084588ef9746 ("omap-common: Common boot code
OMAP3 support and cleanup") and still exists in U-Boot. And
the commit message from that patch implied that no overwrite of
this OMAP_SRAM_SCRATCH_BOOT_PARAMS data happens during the whole
SPL lifetime:
    http://git.denx.de/?p=u-boot.git;a=commitdiff;h=60c7c30aa084588ef974663be3d22a1de7f66cbb

So what's going on? Can Lokesh or Paul comment?

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH] arm: omap3: Detect boot mode very early

[Lokesh Vutla]

On 7/25/2017 7:38 AM, Siarhei Siamashka wrote:
> On Fri, 14 Jul 2017 08:53:20 -0500
> Adam Ford <aford173@gmail.com> 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 <lokeshvutla@ti.com>
>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>
>> diff --git a/arch/arm/mach-omap2/omap3/board.c b/arch/arm/mach-omap2/omap3/board.c
>> index cd8e302..a61b933 100644
>> --- a/arch/arm/mach-omap2/omap3/board.c
>> +++ b/arch/arm/mach-omap2/omap3/board.c
>> @@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
>>  {
>>  	early_system_init();
>>  	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?

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.

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

Thanks and regards,
Lokesh

[U-Boot] [PATCH] arm: omap3: Detect boot mode very early

[Siarhei Siamashka]

On Tue, 25 Jul 2017 12:00:05 +0530
Lokesh Vutla <lokeshvutla@ti.com> wrote:

> On 7/25/2017 7:38 AM, Siarhei Siamashka wrote:
> > On Fri, 14 Jul 2017 08:53:20 -0500
> > Adam Ford <aford173@gmail.com> 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 <lokeshvutla@ti.com>
> >> Signed-off-by: Adam Ford <aford173@gmail.com>
> >>
> >> diff --git a/arch/arm/mach-omap2/omap3/board.c b/arch/arm/mach-omap2/omap3/board.c
> >> index cd8e302..a61b933 100644
> >> --- a/arch/arm/mach-omap2/omap3/board.c
> >> +++ b/arch/arm/mach-omap2/omap3/board.c
> >> @@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
> >>  {
> >>  	early_system_init();
> >>  	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?  
> 
> 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.

> 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

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

> You can always dump R0 at the beiginning of SPL and compare it
> with objdump of spl.
> 
> > 
> > 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.

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

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH] arm: omap3: Detect boot mode very early

[Paul Kocialkowski]

Le mardi 25 juillet 2017 à 13:19 +0300, Siarhei Siamashka a écrit :
> On Tue, 25 Jul 2017 12:00:05 +0530
> Lokesh Vutla <lokeshvutla@ti.com> wrote:
> 
> > On 7/25/2017 7:38 AM, Siarhei Siamashka wrote:
> > > On Fri, 14 Jul 2017 08:53:20 -0500
> > > Adam Ford <aford173@gmail.com> 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 <lokeshvutla@ti.com>
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > 
> > > > diff --git a/arch/arm/mach-omap2/omap3/board.c b/arch/arm/mach-
> > > > omap2/omap3/board.c
> > > > index cd8e302..a61b933 100644
> > > > --- a/arch/arm/mach-omap2/omap3/board.c
> > > > +++ b/arch/arm/mach-omap2/omap3/board.c
> > > > @@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
> > > >  {
> > > >  	early_system_init();
> > > >  	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?  
> > 
> > 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.
> 
> > 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
> 
> > 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.
> 
> > You can always dump R0 at the beiginning of SPL and compare it
> > with objdump of spl.
> > 
> > > 
> > > 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.
> 
> 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.

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.

-- 
Paul Kocialkowski,

developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170727/3cb92f10/attachment.sig>

[U-Boot] [PATCH] arm: omap3: Detect boot mode very early

[Adam Ford]

On Jul 27, 2017 3:40 PM, "Paul Kocialkowski" <contact@paulk.fr> wrote:

Le mardi 25 juillet 2017 à 13:19 +0300, Siarhei Siamashka a écrit :
> On Tue, 25 Jul 2017 12:00:05 +0530
> Lokesh Vutla <lokeshvutla@ti.com> wrote:
>
> > On 7/25/2017 7:38 AM, Siarhei Siamashka wrote:
> > > On Fri, 14 Jul 2017 08:53:20 -0500
> > > Adam Ford <aford173@gmail.com> 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 <lokeshvutla@ti.com>
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > >
> > > > diff --git a/arch/arm/mach-omap2/omap3/board.c b/arch/arm/mach-
> > > > omap2/omap3/board.c
> > > > index cd8e302..a61b933 100644
> > > > --- a/arch/arm/mach-omap2/omap3/board.c
> > > > +++ b/arch/arm/mach-omap2/omap3/board.c
> > > > @@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
> > > >  {
> > > >         early_system_init();
> > > >         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?
> >
> > 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.
>
> > 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
>
> > 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.
>
> > You can always dump R0 at the beiginning of SPL and compare it
> > with objdump of spl.
> >
> > >
> > > 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.
>
> 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.


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



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.

--
Paul Kocialkowski,

developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

[U-Boot] [PATCH] arm: omap3: Detect boot mode very early

[Paul Kocialkowski]

Hi,

I'd like to point out that HTML messages are not welcome in mailing
lists, so please make sure you send plain-text in the future!

Le jeudi 27 juillet 2017 à 15:44 -0500, Adam Ford a écrit :
> On Jul 27, 2017 3:40 PM, "Paul Kocialkowski" <contact@paulk.fr> wrote:
> > Le mardi 25 juillet 2017 à 13:19 +0300, Siarhei Siamashka a écrit :
> > > On Tue, 25 Jul 2017 12:00:05 +0530
> > > Lokesh Vutla <lokeshvutla@ti.com> wrote:
> > >
> > > > On 7/25/2017 7:38 AM, Siarhei Siamashka wrote:
> > > > > On Fri, 14 Jul 2017 08:53:20 -0500
> > > > > Adam Ford <aford173@gmail.com> 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 <lokeshvutla@ti.com>
> > > > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > > >
> > > > > > diff --git a/arch/arm/mach-omap2/omap3/board.c
> > b/arch/arm/mach-
> > > > > > omap2/omap3/board.c
> > > > > > index cd8e302..a61b933 100644
> > > > > > --- a/arch/arm/mach-omap2/omap3/board.c
> > > > > > +++ b/arch/arm/mach-omap2/omap3/board.c
> > > > > > @@ -212,6 +212,12 @@ void board_init_f(ulong dummy)
> > > > > >  {
> > > > > >         early_system_init();
> > > > > >         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?
> > > >
> > > > 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.
> > >
> > > > 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
> > >
> > > > 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.
> > >
> > > > You can always dump R0 at the beiginning of SPL and compare it
> > > > with objdump of spl.
> > > >
> > > > >
> > > > > 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.
> > >
> > > 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.
> > 
> > 
> > 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

No, I'm talking about passing OMAP_SRAM_SCRATCH_BOOT_PARAMS to
jump_to_image_no_args. It looks perfectly unnecessary.

Your patch on the other hand is required and I would definitely like to
see it merged!

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

-- 
Paul Kocialkowski,

developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170730/bf507d0b/attachment.sig>