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

[Siarhei Siamashka]

On Tue, 25 Jul 2017 12:00:05 +0530
Lokesh Vutla <lokeshvutla at 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 at 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 at ti.com>
> >> Signed-off-by: Adam Ford <aford173 at 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