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