[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 16:54, Derald Woods wrote:
> 
> 
> On Mon, Dec 10, 2018 at 8:03 AM <Eugen.Hristev@microchip.com 
> <mailto:Eugen.Hristev@microchip.com>> wrote:
> 
> 
> 
>     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 <mailto: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
>     <mailto:photonthunder@gmail.com>>
>      >>> Cc: Robert Nelson <robertcnelson@gmail.com
>     <mailto:robertcnelson@gmail.com>>
>      >>> Cc: Eugen Hristev <eugen.hristev@microchip.com
>     <mailto:eugen.hristev@microchip.com>>
>      >>> Cc: Wenyou Yang <wenyou.yang@microchip.com
>     <mailto:wenyou.yang@microchip.com>>
>      >>> Signed-off-by: Derald D. Woods <woods.technical@gmail.com
>     <mailto: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.
> 
> 
> 
> The patch does _not_ remove CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER or change
> its purpose or overall role. Literally. It is a very simple patch that 
> solves
> a "no-boot" issue for at least sama5d3-xpld. I read the documents before
> my patch. This is a post-build tooling issue. Makefile.spl can leverage
> configs as they are naturally selected.
> 
> Derald

Does the following change fix your problem ?

diff --git a/include/configs/sama5d3_xplained.h 
b/include/configs/sama5d3_xplained.h
index d0d8087..f2661c5 100644
--- a/include/configs/sama5d3_xplained.h
+++ b/include/configs/sama5d3_xplained.h
@@ -80,6 +80,7 @@
  #elif CONFIG_NAND_BOOT
  #define CONFIG_SPL_NAND_DRIVERS
  #define CONFIG_SPL_NAND_BASE
+#define CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER
  #endif
  #define CONFIG_SYS_NAND_U_BOOT_OFFS    0x40000
  #define CONFIG_SYS_NAND_5_ADDR_CYCLE
@@ -88,6 +89,5 @@
  #define CONFIG_SYS_NAND_OOBSIZE                64
  #define CONFIG_SYS_NAND_BLOCK_SIZE     0x20000
  #define CONFIG_SYS_NAND_BAD_BLOCK_POS  0x0
-#define CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER

  #endif



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