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" <cont...@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 <lokeshvu...@ti.com> wrote: > > > > > > > On 7/25/2017 7:38 AM, Siarhei Siamashka wrote: > > > > > On Fri, 14 Jul 2017 08:53:20 -0500 > > > > > Adam Ford <aford...@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 <lokeshvu...@ti.com> > > > > > > Signed-off-by: Adam Ford <aford...@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/
signature.asc
Description: This is a digitally signed message part
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot