From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eugen.Hristev at microchip.com Date: Mon, 10 Dec 2018 14:03:47 +0000 Subject: [U-Boot] [PATCH] ARM: at91: Fix 'boot.bin' generation when CONFIG_SD_BOOT is enabled In-Reply-To: <20181210130114.GA7182@ethiopia> References: <20181208194905.21014-1-woods.technical@gmail.com> <20181210130114.GA7182@ethiopia> Message-ID: <00edfbbc-3748-623c-fb98-479378ca3f1a@microchip.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>> Cc: Robert Nelson >>> Cc: Eugen Hristev >>> Cc: Wenyou Yang >>> Signed-off-by: Derald D. Woods >>> --- >>> 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 : 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. 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) >>> >