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

[Paul Kocialkowski <contact@paulk.fr>]

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>