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

Sun Jul 30 08:39:13 UTC 2017

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

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>