[U-Boot] [PATCH] ARM: at91: Fix 'boot.bin' generation when CONFIG_SD_BOOT is enabled
Eugen.Hristev at microchip.com
Eugen.Hristev at microchip.com
Mon Dec 10 15:14:05 UTC 2018
On 10.12.2018 16:54, Derald Woods wrote:
>
>
> On Mon, Dec 10, 2018 at 8:03 AM <Eugen.Hristev at microchip.com
> <mailto:Eugen.Hristev at 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 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
> <mailto:photonthunder at gmail.com>>
> >>> Cc: Robert Nelson <robertcnelson at gmail.com
> <mailto:robertcnelson at gmail.com>>
> >>> Cc: Eugen Hristev <eugen.hristev at microchip.com
> <mailto:eugen.hristev at microchip.com>>
> >>> Cc: Wenyou Yang <wenyou.yang at microchip.com
> <mailto:wenyou.yang at microchip.com>>
> >>> Signed-off-by: Derald D. Woods <woods.technical at gmail.com
> <mailto: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.
>
> 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)
> >>>
> >
>
More information about the U-Boot
mailing list