[U-Boot] [PATCH] ARM: at91: Fix 'boot.bin' generation when CONFIG_SD_BOOT is enabled
Derald D. Woods
woods.technical at gmail.com
Tue Dec 11 05:48:25 UTC 2018
On Mon, Dec 10, 2018 at 03:14:05PM +0000, Eugen.Hristev at microchip.com wrote:
>
>
> 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 ?
>
I am simply working on a proper solution. I have the SAMA5D3-XPLD board
and use it occasionally. The original post to the mailing list simply
peaked my interest. I had the board pinned at v2017.09 in my personal
build environment for the same issue. I am just digging deeper this time
around.
> 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
>
I began looking into converting the following to Kconfig:
[scripts/config_whitelist.txt] (REMOVING ITEMS)
------------------------------------------------------------------------
CONFIG_ATMEL_NAND_HWECC
CONFIG_ATMEL_NAND_HW_PMECC
CONFIG_PMECC_CAP
CONFIG_PMECC_SECTOR_SIZE
CONFIG_SPL_GENERATE_ATMEL_PMECC_HEADER
------------------------------------------------------------------------
[arch/arm/mach-at91/Kconfig]
------------------------------------------------------------------------
config ATMEL_NAND_HWECC
bool "Atmel Hardware ECC"
default n
config ATMEL_NAND_HW_PMECC
bool "Atmel Programmable Multibit ECC (PMECC)"
select ATMEL_NAND_HWECC
default n
help
The Programmable Multibit ECC (PMECC) controller is a programmable
binary BCH(Bose, Chaudhuri and Hocquenghem) encoder and decoder.
config PMECC_CAP
int "PMECC Correctable ECC Bits"
depends on ATMEL_NAND_HW_PMECC
default 2
help
Correctable ECC bits, can be 2, 4, 8, 12, and 24.
config PMECC_SECTOR_SIZE
int "PMECC Sector Size"
depends on ATMEL_NAND_HW_PMECC
default 512
help
Sector size, in bytes, can be 512 or 1024.
config SPL_GENERATE_ATMEL_PMECC_HEADER
bool "Atmel PMECC Header Generation"
select ATMEL_NAND_HWECC
select ATMEL_NAND_HW_PMECC
default n
help
Generate Programmable Multibit ECC (PMECC) header for SPL image.
------------------------------------------------------------------------
The issue is that the PMECC configuration items are used along with
other NAND configuration items outside of a NAND_BOOT scenario. I would
like to get the proper Kconfig selections up and running. This is just a
start. I will not have any more time until this weekend. I will send
another patch at that time.
Cheers,
Derald
>
>
> >
> >
> > >
> > > 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