[U-Boot] [PATCH] ARM: at91: Fix 'boot.bin' generation when CONFIG_SD_BOOT is enabled

[Derald D. Woods]

On Mon, Dec 10, 2018 at 08:32:33AM +0000, Eugen.Hristev at 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 at gmail.com>
> > Cc: Robert Nelson <robertcnelson at gmail.com>
> > Cc: Eugen Hristev <eugen.hristev at microchip.com>
> > Cc: Wenyou Yang <wenyou.yang at microchip.com>
> > Signed-off-by: Derald D. Woods <woods.technical at 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.

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