| Message ID | 20181208194905.21014-1-woods.technical@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] ARM: at91: Fix 'boot.bin' generation when CONFIG_SD_BOOT is enabled | expand |
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 ? 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. 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? 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) >
On Mon, Dec 10, 2018 at 08:32:33AM +0000, 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> > > 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. 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) > >
On 10.12.2018 15:01, Derald D. Woods wrote: > On Mon, Dec 10, 2018 at 08:32:33AM +0000, 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> >>> 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) >>> >
On Mon, Dec 10, 2018 at 8:03 AM <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@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. > > 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 > > > > > 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) > >>> > > >
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@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) > >>> > > >
On Mon, Dec 10, 2018 at 03:14:05PM +0000, Eugen.Hristev@microchip.com wrote: > > > 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@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 ? > 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) > > >>> > > > > >
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) 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)
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(+)