| Message ID | 1542796908-7947-5-git-send-email-tien.fong.chee@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | Add support for loading FPGA bitstream | expand |
On 11/21/2018 11:41 AM, tien.fong.chee@intel.com wrote: > From: Tien Fong Chee <tien.fong.chee@intel.com> Did you change Author:ship of the patch ? > Bundle U-Boot fitImage containing U-Boot and FPGA bitstream into the > u-boot-with-spl.sfp on Arria10. This lets U-Boot operate in a very > similar fashion to Gen5, where the U-Boot binary got loaded by the > SPL from a uImage concatenated at the end of the SPL SFP image. On > Gen10, the U-Boot is in fitImage which contains the FPGA bitstream > as well. In this case, the SPL can load the FPGA bitstream first and > load the U-Boot afterward in the same manner. This is nonetheless a > stopgap measure until there is a proper firmware loader in U-Boot. Right, this is a stopgap measure until FW loader is present. Why is this patch needed at all in this series ? > Signed-off-by: Marek Vasut <marex@denx.de> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com> > --- > Makefile | 9 +++++++-- > include/configs/socfpga_common.h | 4 ++++ > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index a55915d..4ecc19d 100644 > --- a/Makefile > +++ b/Makefile > @@ -1212,9 +1212,14 @@ ifneq ($(CONFIG_ARCH_SOCFPGA),) > quiet_cmd_socboot = SOCBOOT $@ > cmd_socboot = cat spl/u-boot-spl.sfp spl/u-boot-spl.sfp \ > spl/u-boot-spl.sfp spl/u-boot-spl.sfp \ > - u-boot.img > $@ || rm -f $@ > + $2 > $@ || rm -f $@ > +ifdef CONFIG_TARGET_SOCFPGA_ARRIA10 > +u-boot-with-spl.sfp: spl/u-boot-spl.sfp u-boot.itb FORCE > + $(call if_changed,socboot,u-boot.itb) > +else > u-boot-with-spl.sfp: spl/u-boot-spl.sfp u-boot.img FORCE > - $(call if_changed,socboot) > + $(call if_changed,socboot,u-boot.img) > +endif > endif > > ifeq ($(CONFIG_MPC85xx)$(CONFIG_OF_SEPARATE),yy) > diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h > index bd8f5c8..ffdc6eb 100644 > --- a/include/configs/socfpga_common.h > +++ b/include/configs/socfpga_common.h > @@ -268,7 +268,11 @@ unsigned int cm_get_qspi_controller_clk_hz(void); > /* SPL SDMMC boot support */ > #ifdef CONFIG_SPL_MMC_SUPPORT > #if defined(CONFIG_SPL_FAT_SUPPORT) || defined(CONFIG_SPL_EXT_SUPPORT) > +#if CONFIG_SPL_FIT > +#define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME "u-boot.itb" > +#else > #define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME "u-boot-dtb.img" > +#endif > #define CONFIG_SYS_MMCSD_FS_BOOT_PARTITION 1 > #endif > #else >
On Wed, 2018-11-21 at 15:21 +0100, Marek Vasut wrote: > On 11/21/2018 11:41 AM, tien.fong.chee@intel.com wrote: > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > Did you change Author:ship of the patch ? > > > > > Bundle U-Boot fitImage containing U-Boot and FPGA bitstream into > > the > > u-boot-with-spl.sfp on Arria10. This lets U-Boot operate in a very > > similar fashion to Gen5, where the U-Boot binary got loaded by the > > SPL from a uImage concatenated at the end of the SPL SFP image. On > > Gen10, the U-Boot is in fitImage which contains the FPGA bitstream > > as well. In this case, the SPL can load the FPGA bitstream first > > and > > load the U-Boot afterward in the same manner. This is nonetheless a > > stopgap measure until there is a proper firmware loader in U-Boot. > Right, this is a stopgap measure until FW loader is present. Why is > this > patch needed at all in this series ? This patch is cherry picked from the sdmmc_next custodian, so this patch is required for generating FIT image. I can remove the stopgap comment to avoid confusing. > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com> > > --- > > Makefile | 9 +++++++-- > > include/configs/socfpga_common.h | 4 ++++ > > 2 files changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index a55915d..4ecc19d 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -1212,9 +1212,14 @@ ifneq ($(CONFIG_ARCH_SOCFPGA),) > > quiet_cmd_socboot = SOCBOOT $@ > > cmd_socboot = cat spl/u-boot-spl.sfp spl/u-boot-spl.sfp > > \ > > spl/u-boot-spl.sfp spl/u-boot-spl.sfp > > \ > > - u-boot.img > $@ || rm -f $@ > > + $2 > $@ || rm -f $@ > > +ifdef CONFIG_TARGET_SOCFPGA_ARRIA10 > > +u-boot-with-spl.sfp: spl/u-boot-spl.sfp u-boot.itb FORCE > > + $(call if_changed,socboot,u-boot.itb) > > +else > > u-boot-with-spl.sfp: spl/u-boot-spl.sfp u-boot.img FORCE > > - $(call if_changed,socboot) > > + $(call if_changed,socboot,u-boot.img) > > +endif > > endif > > > > ifeq ($(CONFIG_MPC85xx)$(CONFIG_OF_SEPARATE),yy) > > diff --git a/include/configs/socfpga_common.h > > b/include/configs/socfpga_common.h > > index bd8f5c8..ffdc6eb 100644 > > --- a/include/configs/socfpga_common.h > > +++ b/include/configs/socfpga_common.h > > @@ -268,7 +268,11 @@ unsigned int > > cm_get_qspi_controller_clk_hz(void); > > /* SPL SDMMC boot support */ > > #ifdef CONFIG_SPL_MMC_SUPPORT > > #if defined(CONFIG_SPL_FAT_SUPPORT) || > > defined(CONFIG_SPL_EXT_SUPPORT) > > +#if CONFIG_SPL_FIT > > +#define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME "u- > > boot.itb" > > +#else > > #define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME "u-boot- > > dtb.img" > > +#endif > > #define CONFIG_SYS_MMCSD_FS_BOOT_PARTITION 1 > > #endif > > #else > > >
On 11/23/2018 10:54 AM, Chee, Tien Fong wrote: > On Wed, 2018-11-21 at 15:21 +0100, Marek Vasut wrote: >> On 11/21/2018 11:41 AM, tien.fong.chee@intel.com wrote: >>> >>> From: Tien Fong Chee <tien.fong.chee@intel.com> >> Did you change Author:ship of the patch ? I believe you did, so please fix that. >>> Bundle U-Boot fitImage containing U-Boot and FPGA bitstream into >>> the >>> u-boot-with-spl.sfp on Arria10. This lets U-Boot operate in a very >>> similar fashion to Gen5, where the U-Boot binary got loaded by the >>> SPL from a uImage concatenated at the end of the SPL SFP image. On >>> Gen10, the U-Boot is in fitImage which contains the FPGA bitstream >>> as well. In this case, the SPL can load the FPGA bitstream first >>> and >>> load the U-Boot afterward in the same manner. This is nonetheless a >>> stopgap measure until there is a proper firmware loader in U-Boot. >> Right, this is a stopgap measure until FW loader is present. Why is >> this >> patch needed at all in this series ? > This patch is cherry picked from the sdmmc_next custodian, so this > patch is required for generating FIT image. I can remove the stopgap > comment to avoid confusing. But why is this patch needed at all ? You use the firmware loader to load the FPGA bitstream. Where does the fitImage come into play ? The fitImage was used to circumvent the missing FW loader, when I needed to load multiple files (bitstream and u-boot binary). Now there is no such requirement anymore, so the entire fitImage machinery is probably not needed ?
On Fri, 2018-11-23 at 13:40 +0100, Marek Vasut wrote: > On 11/23/2018 10:54 AM, Chee, Tien Fong wrote: > > > > On Wed, 2018-11-21 at 15:21 +0100, Marek Vasut wrote: > > > > > > On 11/21/2018 11:41 AM, tien.fong.chee@intel.com wrote: > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > Did you change Author:ship of the patch ? > I believe you did, so please fix that. Very sorry. I din't realize the author name was changed. > > > > > > > > > > > > > > Bundle U-Boot fitImage containing U-Boot and FPGA bitstream > > > > into > > > > the > > > > u-boot-with-spl.sfp on Arria10. This lets U-Boot operate in a > > > > very > > > > similar fashion to Gen5, where the U-Boot binary got loaded by > > > > the > > > > SPL from a uImage concatenated at the end of the SPL SFP image. > > > > On > > > > Gen10, the U-Boot is in fitImage which contains the FPGA > > > > bitstream > > > > as well. In this case, the SPL can load the FPGA bitstream > > > > first > > > > and > > > > load the U-Boot afterward in the same manner. This is > > > > nonetheless a > > > > stopgap measure until there is a proper firmware loader in U- > > > > Boot. > > > Right, this is a stopgap measure until FW loader is present. Why > > > is > > > this > > > patch needed at all in this series ? > > This patch is cherry picked from the sdmmc_next custodian, so this > > patch is required for generating FIT image. I can remove the > > stopgap > > comment to avoid confusing. > But why is this patch needed at all ? You use the firmware loader to > load the FPGA bitstream. Where does the fitImage come into play ? > > The fitImage was used to circumvent the missing FW loader, when I > needed > to load multiple files (bitstream and u-boot binary). Now there is no > such requirement anymore, so the entire fitImage machinery is > probably > not needed ? Loading issue is not the reason we choose the fitImage. We choose it because it allows more flexibility in handling various type images, especially it allows user more choices to enhance integrity and security protection. Although we plan to use fitImage as our default implementation, but this series of patches are still allow fw loading individual bitstream image in filesystem partition. So, it is up to user to made the choice. >
On 11/26/2018 11:30 AM, Chee, Tien Fong wrote: > On Fri, 2018-11-23 at 13:40 +0100, Marek Vasut wrote: >> On 11/23/2018 10:54 AM, Chee, Tien Fong wrote: >>> >>> On Wed, 2018-11-21 at 15:21 +0100, Marek Vasut wrote: >>>> >>>> On 11/21/2018 11:41 AM, tien.fong.chee@intel.com wrote: >>>>> >>>>> >>>>> From: Tien Fong Chee <tien.fong.chee@intel.com> >>>> Did you change Author:ship of the patch ? >> I believe you did, so please fix that. > Very sorry. I din't realize the author name was changed. Please be careful next time. >> >>> >>>> >>>>> >>>>> Bundle U-Boot fitImage containing U-Boot and FPGA bitstream >>>>> into >>>>> the >>>>> u-boot-with-spl.sfp on Arria10. This lets U-Boot operate in a >>>>> very >>>>> similar fashion to Gen5, where the U-Boot binary got loaded by >>>>> the >>>>> SPL from a uImage concatenated at the end of the SPL SFP image. >>>>> On >>>>> Gen10, the U-Boot is in fitImage which contains the FPGA >>>>> bitstream >>>>> as well. In this case, the SPL can load the FPGA bitstream >>>>> first >>>>> and >>>>> load the U-Boot afterward in the same manner. This is >>>>> nonetheless a >>>>> stopgap measure until there is a proper firmware loader in U- >>>>> Boot. >>>> Right, this is a stopgap measure until FW loader is present. Why >>>> is >>>> this >>>> patch needed at all in this series ? >>> This patch is cherry picked from the sdmmc_next custodian, so this >>> patch is required for generating FIT image. I can remove the >>> stopgap >>> comment to avoid confusing. >> But why is this patch needed at all ? You use the firmware loader to >> load the FPGA bitstream. Where does the fitImage come into play ? >> >> The fitImage was used to circumvent the missing FW loader, when I >> needed >> to load multiple files (bitstream and u-boot binary). Now there is no >> such requirement anymore, so the entire fitImage machinery is >> probably >> not needed ? > Loading issue is not the reason we choose the fitImage. We choose it > because it allows more flexibility in handling various type images, > especially it allows user more choices to enhance integrity and > security protection. Do you need to load multiple images at all ? Do you need the extra flexibility or does it only bloat and slow down the boot process for no benefit at all? If a user needs it, they can enable it, but do we need it by default ? > Although we plan to use fitImage as our default implementation, but > this series of patches are still allow fw loading individual bitstream > image in filesystem partition. So, it is up to user to made the choice. Right, so is the fitImage needed at all ?
On Mon, 2018-11-26 at 12:22 +0100, Marek Vasut wrote: > On 11/26/2018 11:30 AM, Chee, Tien Fong wrote: > > > > On Fri, 2018-11-23 at 13:40 +0100, Marek Vasut wrote: > > > > > > On 11/23/2018 10:54 AM, Chee, Tien Fong wrote: > > > > > > > > > > > > On Wed, 2018-11-21 at 15:21 +0100, Marek Vasut wrote: > > > > > > > > > > > > > > > On 11/21/2018 11:41 AM, tien.fong.chee@intel.com wrote: > > > > > > > > > > > > > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > > > Did you change Author:ship of the patch ? > > > I believe you did, so please fix that. > > Very sorry. I din't realize the author name was changed. > Please be careful next time. Sure. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Bundle U-Boot fitImage containing U-Boot and FPGA bitstream > > > > > > into > > > > > > the > > > > > > u-boot-with-spl.sfp on Arria10. This lets U-Boot operate in > > > > > > a > > > > > > very > > > > > > similar fashion to Gen5, where the U-Boot binary got loaded > > > > > > by > > > > > > the > > > > > > SPL from a uImage concatenated at the end of the SPL SFP > > > > > > image. > > > > > > On > > > > > > Gen10, the U-Boot is in fitImage which contains the FPGA > > > > > > bitstream > > > > > > as well. In this case, the SPL can load the FPGA bitstream > > > > > > first > > > > > > and > > > > > > load the U-Boot afterward in the same manner. This is > > > > > > nonetheless a > > > > > > stopgap measure until there is a proper firmware loader in > > > > > > U- > > > > > > Boot. > > > > > Right, this is a stopgap measure until FW loader is present. > > > > > Why > > > > > is > > > > > this > > > > > patch needed at all in this series ? > > > > This patch is cherry picked from the sdmmc_next custodian, so > > > > this > > > > patch is required for generating FIT image. I can remove the > > > > stopgap > > > > comment to avoid confusing. > > > But why is this patch needed at all ? You use the firmware loader > > > to > > > load the FPGA bitstream. Where does the fitImage come into play ? > > > > > > The fitImage was used to circumvent the missing FW loader, when I > > > needed > > > to load multiple files (bitstream and u-boot binary). Now there > > > is no > > > such requirement anymore, so the entire fitImage machinery is > > > probably > > > not needed ? > > Loading issue is not the reason we choose the fitImage. We choose > > it > > because it allows more flexibility in handling various type images, > > especially it allows user more choices to enhance integrity and > > security protection. > Do you need to load multiple images at all ? Do you need the extra > flexibility or does it only bloat and slow down the boot process for > no > benefit at all? If a user needs it, they can enable it, but do we > need > it by default ? Okay, then we add in the fitImage support and let user to enable it. So, without CONFIG_SPL_FIT is defined, then the boot process would be with individual files such as u-boot-dtb.img instead of u-boot.itb. > > > > > Although we plan to use fitImage as our default implementation, but > > this series of patches are still allow fw loading individual > > bitstream > > image in filesystem partition. So, it is up to user to made the > > choice. > Right, so is the fitImage needed at all ? >
On 11/27/2018 10:00 AM, Chee, Tien Fong wrote: > On Mon, 2018-11-26 at 12:22 +0100, Marek Vasut wrote: >> On 11/26/2018 11:30 AM, Chee, Tien Fong wrote: >>> >>> On Fri, 2018-11-23 at 13:40 +0100, Marek Vasut wrote: >>>> >>>> On 11/23/2018 10:54 AM, Chee, Tien Fong wrote: >>>>> >>>>> >>>>> On Wed, 2018-11-21 at 15:21 +0100, Marek Vasut wrote: >>>>>> >>>>>> >>>>>> On 11/21/2018 11:41 AM, tien.fong.chee@intel.com wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com> >>>>>> Did you change Author:ship of the patch ? >>>> I believe you did, so please fix that. >>> Very sorry. I din't realize the author name was changed. >> Please be careful next time. > Sure. >> >>> >>>> >>>> >>>>> >>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>> Bundle U-Boot fitImage containing U-Boot and FPGA bitstream >>>>>>> into >>>>>>> the >>>>>>> u-boot-with-spl.sfp on Arria10. This lets U-Boot operate in >>>>>>> a >>>>>>> very >>>>>>> similar fashion to Gen5, where the U-Boot binary got loaded >>>>>>> by >>>>>>> the >>>>>>> SPL from a uImage concatenated at the end of the SPL SFP >>>>>>> image. >>>>>>> On >>>>>>> Gen10, the U-Boot is in fitImage which contains the FPGA >>>>>>> bitstream >>>>>>> as well. In this case, the SPL can load the FPGA bitstream >>>>>>> first >>>>>>> and >>>>>>> load the U-Boot afterward in the same manner. This is >>>>>>> nonetheless a >>>>>>> stopgap measure until there is a proper firmware loader in >>>>>>> U- >>>>>>> Boot. >>>>>> Right, this is a stopgap measure until FW loader is present. >>>>>> Why >>>>>> is >>>>>> this >>>>>> patch needed at all in this series ? >>>>> This patch is cherry picked from the sdmmc_next custodian, so >>>>> this >>>>> patch is required for generating FIT image. I can remove the >>>>> stopgap >>>>> comment to avoid confusing. >>>> But why is this patch needed at all ? You use the firmware loader >>>> to >>>> load the FPGA bitstream. Where does the fitImage come into play ? >>>> >>>> The fitImage was used to circumvent the missing FW loader, when I >>>> needed >>>> to load multiple files (bitstream and u-boot binary). Now there >>>> is no >>>> such requirement anymore, so the entire fitImage machinery is >>>> probably >>>> not needed ? >>> Loading issue is not the reason we choose the fitImage. We choose >>> it >>> because it allows more flexibility in handling various type images, >>> especially it allows user more choices to enhance integrity and >>> security protection. >> Do you need to load multiple images at all ? Do you need the extra >> flexibility or does it only bloat and slow down the boot process for >> no >> benefit at all? If a user needs it, they can enable it, but do we >> need >> it by default ? > Okay, then we add in the fitImage support and let user to enable it. > So, without CONFIG_SPL_FIT is defined, then the boot process would be > with individual files such as u-boot-dtb.img instead of u-boot.itb. Yes, so all these fitImage patches can be dropped for now ?
On Tue, 2018-11-27 at 13:09 +0100, Marek Vasut wrote: > On 11/27/2018 10:00 AM, Chee, Tien Fong wrote: > > > > On Mon, 2018-11-26 at 12:22 +0100, Marek Vasut wrote: > > > > > > On 11/26/2018 11:30 AM, Chee, Tien Fong wrote: > > > > > > > > > > > > On Fri, 2018-11-23 at 13:40 +0100, Marek Vasut wrote: > > > > > > > > > > > > > > > On 11/23/2018 10:54 AM, Chee, Tien Fong wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2018-11-21 at 15:21 +0100, Marek Vasut wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 11/21/2018 11:41 AM, tien.fong.chee@intel.com wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > > > > > Did you change Author:ship of the patch ? > > > > > I believe you did, so please fix that. > > > > Very sorry. I din't realize the author name was changed. > > > Please be careful next time. > > Sure. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Bundle U-Boot fitImage containing U-Boot and FPGA > > > > > > > > bitstream > > > > > > > > into > > > > > > > > the > > > > > > > > u-boot-with-spl.sfp on Arria10. This lets U-Boot > > > > > > > > operate in > > > > > > > > a > > > > > > > > very > > > > > > > > similar fashion to Gen5, where the U-Boot binary got > > > > > > > > loaded > > > > > > > > by > > > > > > > > the > > > > > > > > SPL from a uImage concatenated at the end of the SPL > > > > > > > > SFP > > > > > > > > image. > > > > > > > > On > > > > > > > > Gen10, the U-Boot is in fitImage which contains the > > > > > > > > FPGA > > > > > > > > bitstream > > > > > > > > as well. In this case, the SPL can load the FPGA > > > > > > > > bitstream > > > > > > > > first > > > > > > > > and > > > > > > > > load the U-Boot afterward in the same manner. This is > > > > > > > > nonetheless a > > > > > > > > stopgap measure until there is a proper firmware loader > > > > > > > > in > > > > > > > > U- > > > > > > > > Boot. > > > > > > > Right, this is a stopgap measure until FW loader is > > > > > > > present. > > > > > > > Why > > > > > > > is > > > > > > > this > > > > > > > patch needed at all in this series ? > > > > > > This patch is cherry picked from the sdmmc_next custodian, > > > > > > so > > > > > > this > > > > > > patch is required for generating FIT image. I can remove > > > > > > the > > > > > > stopgap > > > > > > comment to avoid confusing. > > > > > But why is this patch needed at all ? You use the firmware > > > > > loader > > > > > to > > > > > load the FPGA bitstream. Where does the fitImage come into > > > > > play ? > > > > > > > > > > The fitImage was used to circumvent the missing FW loader, > > > > > when I > > > > > needed > > > > > to load multiple files (bitstream and u-boot binary). Now > > > > > there > > > > > is no > > > > > such requirement anymore, so the entire fitImage machinery is > > > > > probably > > > > > not needed ? > > > > Loading issue is not the reason we choose the fitImage. We > > > > choose > > > > it > > > > because it allows more flexibility in handling various type > > > > images, > > > > especially it allows user more choices to enhance integrity and > > > > security protection. > > > Do you need to load multiple images at all ? Do you need the > > > extra > > > flexibility or does it only bloat and slow down the boot process > > > for > > > no > > > benefit at all? If a user needs it, they can enable it, but do we > > > need > > > it by default ? > > Okay, then we add in the fitImage support and let user to enable > > it. > > So, without CONFIG_SPL_FIT is defined, then the boot process would > > be > > with individual files such as u-boot-dtb.img instead of u-boot.itb. > Yes, so all these fitImage patches can be dropped for now ? This patch can be dropped. But i don't know it is good idea to reserve the patch 5-8, this would be easier for user to enable CONFIG_SPL_FIT in future. >
On 11/28/2018 03:43 PM, Chee, Tien Fong wrote: > On Tue, 2018-11-27 at 13:09 +0100, Marek Vasut wrote: >> On 11/27/2018 10:00 AM, Chee, Tien Fong wrote: >>> >>> On Mon, 2018-11-26 at 12:22 +0100, Marek Vasut wrote: >>>> >>>> On 11/26/2018 11:30 AM, Chee, Tien Fong wrote: >>>>> >>>>> >>>>> On Fri, 2018-11-23 at 13:40 +0100, Marek Vasut wrote: >>>>>> >>>>>> >>>>>> On 11/23/2018 10:54 AM, Chee, Tien Fong wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Wed, 2018-11-21 at 15:21 +0100, Marek Vasut wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 11/21/2018 11:41 AM, tien.fong.chee@intel.com wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com> >>>>>>>> Did you change Author:ship of the patch ? >>>>>> I believe you did, so please fix that. >>>>> Very sorry. I din't realize the author name was changed. >>>> Please be careful next time. >>> Sure. >>>> >>>> >>>>> >>>>> >>>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Bundle U-Boot fitImage containing U-Boot and FPGA >>>>>>>>> bitstream >>>>>>>>> into >>>>>>>>> the >>>>>>>>> u-boot-with-spl.sfp on Arria10. This lets U-Boot >>>>>>>>> operate in >>>>>>>>> a >>>>>>>>> very >>>>>>>>> similar fashion to Gen5, where the U-Boot binary got >>>>>>>>> loaded >>>>>>>>> by >>>>>>>>> the >>>>>>>>> SPL from a uImage concatenated at the end of the SPL >>>>>>>>> SFP >>>>>>>>> image. >>>>>>>>> On >>>>>>>>> Gen10, the U-Boot is in fitImage which contains the >>>>>>>>> FPGA >>>>>>>>> bitstream >>>>>>>>> as well. In this case, the SPL can load the FPGA >>>>>>>>> bitstream >>>>>>>>> first >>>>>>>>> and >>>>>>>>> load the U-Boot afterward in the same manner. This is >>>>>>>>> nonetheless a >>>>>>>>> stopgap measure until there is a proper firmware loader >>>>>>>>> in >>>>>>>>> U- >>>>>>>>> Boot. >>>>>>>> Right, this is a stopgap measure until FW loader is >>>>>>>> present. >>>>>>>> Why >>>>>>>> is >>>>>>>> this >>>>>>>> patch needed at all in this series ? >>>>>>> This patch is cherry picked from the sdmmc_next custodian, >>>>>>> so >>>>>>> this >>>>>>> patch is required for generating FIT image. I can remove >>>>>>> the >>>>>>> stopgap >>>>>>> comment to avoid confusing. >>>>>> But why is this patch needed at all ? You use the firmware >>>>>> loader >>>>>> to >>>>>> load the FPGA bitstream. Where does the fitImage come into >>>>>> play ? >>>>>> >>>>>> The fitImage was used to circumvent the missing FW loader, >>>>>> when I >>>>>> needed >>>>>> to load multiple files (bitstream and u-boot binary). Now >>>>>> there >>>>>> is no >>>>>> such requirement anymore, so the entire fitImage machinery is >>>>>> probably >>>>>> not needed ? >>>>> Loading issue is not the reason we choose the fitImage. We >>>>> choose >>>>> it >>>>> because it allows more flexibility in handling various type >>>>> images, >>>>> especially it allows user more choices to enhance integrity and >>>>> security protection. >>>> Do you need to load multiple images at all ? Do you need the >>>> extra >>>> flexibility or does it only bloat and slow down the boot process >>>> for >>>> no >>>> benefit at all? If a user needs it, they can enable it, but do we >>>> need >>>> it by default ? >>> Okay, then we add in the fitImage support and let user to enable >>> it. >>> So, without CONFIG_SPL_FIT is defined, then the boot process would >>> be >>> with individual files such as u-boot-dtb.img instead of u-boot.itb. >> Yes, so all these fitImage patches can be dropped for now ? > This patch can be dropped. But i don't know it is good idea to reserve > the patch 5-8, this would be easier for user to enable CONFIG_SPL_FIT > in future. Drop them for now, let's revisit this later.
diff --git a/Makefile b/Makefile index a55915d..4ecc19d 100644 --- a/Makefile +++ b/Makefile @@ -1212,9 +1212,14 @@ ifneq ($(CONFIG_ARCH_SOCFPGA),) quiet_cmd_socboot = SOCBOOT $@ cmd_socboot = cat spl/u-boot-spl.sfp spl/u-boot-spl.sfp \ spl/u-boot-spl.sfp spl/u-boot-spl.sfp \ - u-boot.img > $@ || rm -f $@ + $2 > $@ || rm -f $@ +ifdef CONFIG_TARGET_SOCFPGA_ARRIA10 +u-boot-with-spl.sfp: spl/u-boot-spl.sfp u-boot.itb FORCE + $(call if_changed,socboot,u-boot.itb) +else u-boot-with-spl.sfp: spl/u-boot-spl.sfp u-boot.img FORCE - $(call if_changed,socboot) + $(call if_changed,socboot,u-boot.img) +endif endif ifeq ($(CONFIG_MPC85xx)$(CONFIG_OF_SEPARATE),yy) diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h index bd8f5c8..ffdc6eb 100644 --- a/include/configs/socfpga_common.h +++ b/include/configs/socfpga_common.h @@ -268,7 +268,11 @@ unsigned int cm_get_qspi_controller_clk_hz(void); /* SPL SDMMC boot support */ #ifdef CONFIG_SPL_MMC_SUPPORT #if defined(CONFIG_SPL_FAT_SUPPORT) || defined(CONFIG_SPL_EXT_SUPPORT) +#if CONFIG_SPL_FIT +#define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME "u-boot.itb" +#else #define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME "u-boot-dtb.img" +#endif #define CONFIG_SYS_MMCSD_FS_BOOT_PARTITION 1 #endif #else