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

[Eugen.Hristev at microchip.com <Eugen.Hristev@microchip.com>]

On 10.12.2018 15:01, Derald D. Woods wrote:
> 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@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)
>>>
>