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

Thu Jul 27 20:44:47 UTC 2017

On Jul 27, 2017 3:40 PM, "Paul Kocialkowski" <contact at 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 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?

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/