On 10.12.2018 15:01, Derald D. Woods wrote:
On Mon, Dec 10, 2018 at 08:32:33AM +0000, Eugen.Hristev@microchip.com wrote:
On 08.12.2018 21:49, Derald D. Woods wrote:
On AT91 platforms configured for SD_BOOT, this commit avoids the generation of the PMECC header used for booting from NAND flash. This issue was found by attempting to boot the SAMA5D3-XPLD board with the 'sama5d3_xplained_mmc_defconfig'.
[PMECC Reference] http://www.at91.com/linux4sam/bin/view/Linux4SAM/AT91Bootstrap
[Mailing List Thread] https://lists.denx.de/pipermail/u-boot/2018-December/350666.html
Fixes: 5541543f ("configs: at91: Remove CONFIG_SYS_EXTRA_OPTIONS assignment") Reported-by: Daniel Evans photonthunder@gmail.com Cc: Robert Nelson robertcnelson@gmail.com Cc: Eugen Hristev eugen.hristev@microchip.com Cc: Wenyou Yang wenyou.yang@microchip.com Signed-off-by: Derald D. Woods woods.technical@gmail.com
scripts/Makefile.spl | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 22bd8f7c27..e727cb610f 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -166,10 +166,12 @@ ifeq ($(CONFIG_SYS_SOC),"at91") MKIMAGEFLAGS_boot.bin = -T atmelimage
ifeq ($(CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER),y) +ifneq ($(CONFIG_SD_BOOT),y)
Hi Derald,
Thanks for your patch, however, I don't like that we do not use the CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER anymore... isn't this config supposed to say whether we are going to generate the header or not ?
That is not what is happening with this patch. SPL_GENERATE_ATMEL_PMECC_HEADER is not removed. The config still serves its orignal intent. If SD_BOOT is configured, then NAND is not being used. In this non-NAND case, the header is not needed.
Checking if "not sd-boot" doesn't look like a good option... we may use SPI boot or QSPI or some other type at some point and the issue will still be there.
This location and method would work for those nod-NAND cases also. See below.
I would rather fix the original patch by Wenyou, namely move the #ifdef below to not have the GENERATE_ATMEL_PMECC enabled for SDBOOT.
Does this sound good for you?
If this SPL_GENERATE_ATMEL_PMECC_HEADER only needs to be there for NAND, why not guard the 'ifdef ($(CONFIG_NAND_BOOT,y))'? Would this be better? Basically we could replace the 'ifneq ($(CONFIG_SD_BOOT),y)' with the more appropriate 'ifeq ($(CONFIG_NAND_BOOT),y)'. This would actually allow the future use-cases to be added as they become available and can be shown to actually boot with the header applied.
I will put together a proper version 2 of my patch later today.
[patch v2]
diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 22bd8f7..e727cb6 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -166,10 +166,12 @@ ifeq ($(CONFIG_SYS_SOC),"at91") MKIMAGEFLAGS_boot.bin = -T atmelimage
ifeq ($(CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER),y) +ifeq ($(CONFIG_NAND_BOOT),y) MKIMAGEFLAGS_boot.bin += -n $(shell $(obj)/../tools/atmel_pmecc_params)
boot.bin: $(obj)/../tools/atmel_pmecc_params endif +endif
boot.bin: $(obj)/u-boot-spl.bin FORCE $(call if_changed,mkimage)
This would allow other configurations to 'opt-in' to applying the header. Which I think is the direction this is truly heading.
My questions are :
If we have configured CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER, then why the header is not being applied after your patch ?
And why do we configure CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER if we do not wish the PMECC header in the image ?
With your patch, why do we generate the PMECC header w.r.t. the configuration of NAND_BOOT and not CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER ?
Or perhaps I am missing something on the purpose of CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER ?
To quote from doc/README.atmel_pmecc : <quote> To enable the generation of atmel PMECC header for SPL one need to define CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER. The required parameters are taken from board configuration and compiled into the host tools atmel_pmecc_params. This tool will be called in build process to parametrize mkimage for atmelimage type. The mkimage tool has intentionally _not_ compiled in those parameters. </quote>
So I would expect CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER to generate the PMECC header if configured, and if this flag is not present, the PMECC header is not generated. I do expect that SD card booting configurations do not select this flag.
Also, the SPL_GENERATE_ATMEL_PMECC_HEADER needs transitioning to Kconfig. Having the logic in "Makefile.spl" would help with that work too.
Derald
Thanks again,
Eugen
MKIMAGEFLAGS_boot.bin += -n $(shell $(obj)/../tools/atmel_pmecc_params)
boot.bin: $(obj)/../tools/atmel_pmecc_params endif +endif
boot.bin: $(obj)/u-boot-spl.bin FORCE $(call if_changed,mkimage)