| Message ID | bdc806b3306a13d32de18b267ae41f60d2f8eb95.1531918301.git.michal.simek@xilinx.com |
|---|---|
| State | Deferred |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | [U-Boot] spl: fpga: Implement fpga bistream loading with fpga_load | expand |
On 07/18/2018 02:51 PM, Michal Simek wrote: > There shouldn't be a need to call private spl_load_fpga_image() > because the whole sequence should be already handled by fpga_load(). > The patch let spl_load_fit_image() to load data to right location based > on "load" property in FIT and then call fpga_load(). NAK This breaks Arria10, sorry. The private loading function is needed on Arria10 as the whole bitstream is not available in RAM and needs to be loaded piece by piece, see [1] [1] http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef > The patch is partially reverting > "spl: fit: Add support for loading FPGA bitstream" > (sha1: 26a642238bdecc53527142dc043b29e21c5cc94c) > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > <debug_uart> Debug uart enabled > > U-Boot SPL 2018.07-00185-g4c2b86d7a763 (Jul 18 2018 - 14:49:35 +0200) > mmc boot > Trying to boot from MMC1 > spl_load_image_fat_os: error reading image system.dtb, err - -2 > zynq_align_dma_buffer: Bitstream is not swapped(1) - swap it > FPGA image loaded from FIT > > U-Boot 2018.07-00185-g4c2b86d7a763 (Jul 18 2018 - 14:49:35 +0200) Xilinx > Zynq ZC706 > > CPU: Zynq 7z045 > Silicon: v1.0 > Model: Zynq ZC706 Development Board > I2C: ready > DRAM: ECC disabled 1 GiB > Watchdog: Started > MMC: sdhci@e0100000: 0 > Loading Environment from SPI Flash... SF: Detected s25fl128s_64k with > page size 256 Bytes, erase size 64 KiB, total 16 MiB > *** Warning - bad CRC, using default environment > > Failed (-5) > In: serial@e0001000 > Out: serial@e0001000 > Err: serial@e0001000 > Net: ZYNQ GEM: e000b000, phyaddr 7, interface rgmii-id > > Warning: ethernet@e000b000 (eth0) using random MAC address - > 6e:3a:1c:22:aa:5f > eth0: ethernet@e000b000 > Checking if uenvcmd is set ... > Hit any key to stop autoboot: 0 > Zynq> > > --- > common/spl/spl_fit.c | 34 ++++++++++++++++------------------ > 1 file changed, 16 insertions(+), 18 deletions(-) > > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c > index 9eabb1c1058b..8ab6a2def1e2 100644 > --- a/common/spl/spl_fit.c > +++ b/common/spl/spl_fit.c > @@ -6,6 +6,7 @@ > > #include <common.h> > #include <errno.h> > +#include <fpga.h> > #include <image.h> > #include <linux/libfdt.h> > #include <spl.h> > @@ -140,14 +141,6 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size, > return (data_size + info->bl_len - 1) / info->bl_len; > } > > -#ifdef CONFIG_SPL_FPGA_SUPPORT > -__weak int spl_load_fpga_image(struct spl_load_info *info, size_t length, > - int nr_sectors, int sector_offset) > -{ > - return 0; > -} > -#endif > - > /** > * spl_load_fit_image(): load the image described in a certain FIT node > * @info: points to information about the device to load data from > @@ -169,7 +162,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector, > void *fit, ulong base_offset, int node, > struct spl_image_info *image_info) > { > - int offset, sector_offset; > + int offset; > size_t length; > int len; > ulong size; > @@ -217,16 +210,9 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector, > > overhead = get_aligned_image_overhead(info, offset); > nr_sectors = get_aligned_image_size(info, length, offset); > - sector_offset = sector + get_aligned_image_offset(info, offset); > - > -#ifdef CONFIG_SPL_FPGA_SUPPORT > - if (type == IH_TYPE_FPGA) { > - return spl_load_fpga_image(info, length, nr_sectors, > - sector_offset); > - } > -#endif > > - if (info->read(info, sector_offset, > + if (info->read(info, > + sector + get_aligned_image_offset(info, offset), > nr_sectors, (void *)load_ptr) != nr_sectors) > return -EIO; > > @@ -412,6 +398,18 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, > printf("%s: Cannot load the FPGA: %i\n", __func__, ret); > return ret; > } > + > + debug("FPGA bitstream at: %x, size: %x\n", > + (u32)spl_image->load_addr, spl_image->size); > + > + ret = fpga_load(0, (const void *)spl_image->load_addr, > + spl_image->size, BIT_FULL); > + if (ret) { > + printf("%s: Cannot load the image to the FPGA\n", > + __func__); > + return ret; > + } > + > puts("FPGA image loaded from FIT\n"); > node = -1; > } >
On 18.7.2018 14:54, Marek Vasut wrote: > On 07/18/2018 02:51 PM, Michal Simek wrote: >> There shouldn't be a need to call private spl_load_fpga_image() >> because the whole sequence should be already handled by fpga_load(). >> The patch let spl_load_fit_image() to load data to right location based >> on "load" property in FIT and then call fpga_load(). > > NAK > > This breaks Arria10, sorry. The private loading function is needed on > Arria10 as the whole bitstream is not available in RAM and needs to be > loaded piece by piece, see [1] > > [1] > http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef We are discussing this over IRC and it is clear that current solution for altera A10 is one ugly hack which is just going over current flow. Load address from ITS is ignored, the same for hashes. With small changes compression could be also possible to use without big problem which should very beneficial to speedup load from slower memories. What we are missing is do fit image reading by chunks to feed fpga. Currently spl_load_fpga_image() is dummy function which none is calling that's why the first solution could be to simply revert this patch because there is no functionality behind. The second solution is to check if load address is not 0 and call fpga_load only for that. In this case there is a need to check size for SPL. Thanks, Michal
On 07/18/2018 04:00 PM, Michal Simek wrote: > On 18.7.2018 14:54, Marek Vasut wrote: >> On 07/18/2018 02:51 PM, Michal Simek wrote: >>> There shouldn't be a need to call private spl_load_fpga_image() >>> because the whole sequence should be already handled by fpga_load(). >>> The patch let spl_load_fit_image() to load data to right location based >>> on "load" property in FIT and then call fpga_load(). >> >> NAK >> >> This breaks Arria10, sorry. The private loading function is needed on >> Arria10 as the whole bitstream is not available in RAM and needs to be >> loaded piece by piece, see [1] >> >> [1] >> http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef > > We are discussing this over IRC and it is clear that current solution > for altera A10 is one ugly hack which is just going over current flow. > Load address from ITS is ignored, the same for hashes. The load address must be ignored because it's used to load fitImage piece by piece. There is nowhere to load the whole FPGA bitstream. > With small changes compression could be also possible to use without big > problem which should very beneficial to speedup load from slower memories. > > What we are missing is do fit image reading by chunks to feed fpga. > > Currently spl_load_fpga_image() is dummy function which none is calling > that's why the first solution could be to simply revert this patch > because there is no functionality behind. See the link above, I am just holding back on pushing this mainline because it's gonna be superseded by the firmware loader once it's finished. Until then, this is needed for A10. > The second solution is to check if load address is not 0 and call > fpga_load only for that. In this case there is a need to check size for SPL. 0 is a valid load address, so no.
On 18.7.2018 16:15, Marek Vasut wrote: > On 07/18/2018 04:00 PM, Michal Simek wrote: >> On 18.7.2018 14:54, Marek Vasut wrote: >>> On 07/18/2018 02:51 PM, Michal Simek wrote: >>>> There shouldn't be a need to call private spl_load_fpga_image() >>>> because the whole sequence should be already handled by fpga_load(). >>>> The patch let spl_load_fit_image() to load data to right location based >>>> on "load" property in FIT and then call fpga_load(). >>> >>> NAK >>> >>> This breaks Arria10, sorry. The private loading function is needed on >>> Arria10 as the whole bitstream is not available in RAM and needs to be >>> loaded piece by piece, see [1] >>> >>> [1] >>> http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef >> >> We are discussing this over IRC and it is clear that current solution >> for altera A10 is one ugly hack which is just going over current flow. >> Load address from ITS is ignored, the same for hashes. > > The load address must be ignored because it's used to load fitImage > piece by piece. There is nowhere to load the whole FPGA bitstream. for your case yes. > >> With small changes compression could be also possible to use without big >> problem which should very beneficial to speedup load from slower memories. >> >> What we are missing is do fit image reading by chunks to feed fpga. >> >> Currently spl_load_fpga_image() is dummy function which none is calling >> that's why the first solution could be to simply revert this patch >> because there is no functionality behind. > > See the link above, I am just holding back on pushing this mainline > because it's gonna be superseded by the firmware loader once it's > finished. Until then, this is needed for A10. But there is no user of this interface in mainline. What you have in random custodian branch that's almost like soc vendor evil tree. :-) >> The second solution is to check if load address is not 0 and call >> fpga_load only for that. In this case there is a need to check size for SPL. > > 0 is a valid load address, so no. Then new Kconfig option is another way to go now. Thanks, Michal
On 07/18/2018 04:18 PM, Michal Simek wrote: > On 18.7.2018 16:15, Marek Vasut wrote: >> On 07/18/2018 04:00 PM, Michal Simek wrote: >>> On 18.7.2018 14:54, Marek Vasut wrote: >>>> On 07/18/2018 02:51 PM, Michal Simek wrote: >>>>> There shouldn't be a need to call private spl_load_fpga_image() >>>>> because the whole sequence should be already handled by fpga_load(). >>>>> The patch let spl_load_fit_image() to load data to right location based >>>>> on "load" property in FIT and then call fpga_load(). >>>> >>>> NAK >>>> >>>> This breaks Arria10, sorry. The private loading function is needed on >>>> Arria10 as the whole bitstream is not available in RAM and needs to be >>>> loaded piece by piece, see [1] >>>> >>>> [1] >>>> http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef [...] >>> The second solution is to check if load address is not 0 and call >>> fpga_load only for that. In this case there is a need to check size for SPL. >> >> 0 is a valid load address, so no. > > Then new Kconfig option is another way to go now. No, the firmware loader is a way to go. Sadly, it's still work in progress.
On 18.7.2018 16:24, Marek Vasut wrote: > On 07/18/2018 04:18 PM, Michal Simek wrote: >> On 18.7.2018 16:15, Marek Vasut wrote: >>> On 07/18/2018 04:00 PM, Michal Simek wrote: >>>> On 18.7.2018 14:54, Marek Vasut wrote: >>>>> On 07/18/2018 02:51 PM, Michal Simek wrote: >>>>>> There shouldn't be a need to call private spl_load_fpga_image() >>>>>> because the whole sequence should be already handled by fpga_load(). >>>>>> The patch let spl_load_fit_image() to load data to right location based >>>>>> on "load" property in FIT and then call fpga_load(). >>>>> >>>>> NAK >>>>> >>>>> This breaks Arria10, sorry. The private loading function is needed on >>>>> Arria10 as the whole bitstream is not available in RAM and needs to be >>>>> loaded piece by piece, see [1] >>>>> >>>>> [1] >>>>> http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef > > [...] >>>> The second solution is to check if load address is not 0 and call >>>> fpga_load only for that. In this case there is a need to check size for SPL. >>> >>> 0 is a valid load address, so no. >> >> Then new Kconfig option is another way to go now. > > No, the firmware loader is a way to go. Sadly, it's still work in progress. I have looked at that series and it will take some time to get it done but even that series has no user and also only support filesystems. Which is fine but not enough and support for RAW mode is necessary too. Anyway Luis sent series where this SPL fpga supported is requested to add and I had this functionality in more raw state in Xilinx tree for quite a long time and it is time to support it because on Zybo (because of i2c eeprom) and also cc108 (because of uart routing via PL) fpga load needs to be done in SPL and we need that support. I have not a problem to keep your code in SPL but I need a way to enable fpga load directly with all that features like hashes which are already available. GZIP can be added pretty easily too. That's why please suggest a way what you are comfortable with not to block functionality on these devices. Thanks, Michal
On 07/18/2018 04:57 PM, Michal Simek wrote: > On 18.7.2018 16:24, Marek Vasut wrote: >> On 07/18/2018 04:18 PM, Michal Simek wrote: >>> On 18.7.2018 16:15, Marek Vasut wrote: >>>> On 07/18/2018 04:00 PM, Michal Simek wrote: >>>>> On 18.7.2018 14:54, Marek Vasut wrote: >>>>>> On 07/18/2018 02:51 PM, Michal Simek wrote: >>>>>>> There shouldn't be a need to call private spl_load_fpga_image() >>>>>>> because the whole sequence should be already handled by fpga_load(). >>>>>>> The patch let spl_load_fit_image() to load data to right location based >>>>>>> on "load" property in FIT and then call fpga_load(). >>>>>> >>>>>> NAK >>>>>> >>>>>> This breaks Arria10, sorry. The private loading function is needed on >>>>>> Arria10 as the whole bitstream is not available in RAM and needs to be >>>>>> loaded piece by piece, see [1] >>>>>> >>>>>> [1] >>>>>> http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef >> >> [...] >>>>> The second solution is to check if load address is not 0 and call >>>>> fpga_load only for that. In this case there is a need to check size for SPL. >>>> >>>> 0 is a valid load address, so no. >>> >>> Then new Kconfig option is another way to go now. >> >> No, the firmware loader is a way to go. Sadly, it's still work in progress. > > I have looked at that series and it will take some time to get it done > but even that series has no user and also only support filesystems. > Which is fine but not enough and support for RAW mode is necessary too. It already took like a year and half I think ... well, better invest your resources in perfecting it for your usecase, for that's the way to go. > Anyway Luis sent series where this SPL fpga supported is requested to > add and I had this functionality in more raw state in Xilinx tree for > quite a long time and it is time to support it because on Zybo (because > of i2c eeprom) and also cc108 (because of uart routing via PL) fpga load > needs to be done in SPL and we need that support. Ha > I have not a problem to keep your code in SPL but I need a way to enable > fpga load directly with all that features like hashes which are already > available. GZIP can be added pretty easily too. > That's why please suggest a way what you are comfortable with not to > block functionality on these devices. Look at the A10 nand branch, it uses full fit with all the bells and whistles. Maybe that's the way to go if you have DRAM available.
On 18.7.2018 22:11, Marek Vasut wrote: > On 07/18/2018 04:57 PM, Michal Simek wrote: >> On 18.7.2018 16:24, Marek Vasut wrote: >>> On 07/18/2018 04:18 PM, Michal Simek wrote: >>>> On 18.7.2018 16:15, Marek Vasut wrote: >>>>> On 07/18/2018 04:00 PM, Michal Simek wrote: >>>>>> On 18.7.2018 14:54, Marek Vasut wrote: >>>>>>> On 07/18/2018 02:51 PM, Michal Simek wrote: >>>>>>>> There shouldn't be a need to call private spl_load_fpga_image() >>>>>>>> because the whole sequence should be already handled by fpga_load(). >>>>>>>> The patch let spl_load_fit_image() to load data to right location based >>>>>>>> on "load" property in FIT and then call fpga_load(). >>>>>>> >>>>>>> NAK >>>>>>> >>>>>>> This breaks Arria10, sorry. The private loading function is needed on >>>>>>> Arria10 as the whole bitstream is not available in RAM and needs to be >>>>>>> loaded piece by piece, see [1] >>>>>>> >>>>>>> [1] >>>>>>> http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef >>> >>> [...] >>>>>> The second solution is to check if load address is not 0 and call >>>>>> fpga_load only for that. In this case there is a need to check size for SPL. >>>>> >>>>> 0 is a valid load address, so no. >>>> >>>> Then new Kconfig option is another way to go now. >>> >>> No, the firmware loader is a way to go. Sadly, it's still work in progress. >> >> I have looked at that series and it will take some time to get it done >> but even that series has no user and also only support filesystems. >> Which is fine but not enough and support for RAW mode is necessary too. > > It already took like a year and half I think ... well, better invest > your resources in perfecting it for your usecase, for that's the way to go. I don't think so because for SPL boot we need support for a RAW mode. > >> Anyway Luis sent series where this SPL fpga supported is requested to >> add and I had this functionality in more raw state in Xilinx tree for >> quite a long time and it is time to support it because on Zybo (because >> of i2c eeprom) and also cc108 (because of uart routing via PL) fpga load >> needs to be done in SPL and we need that support. > > Ha > >> I have not a problem to keep your code in SPL but I need a way to enable >> fpga load directly with all that features like hashes which are already >> available. GZIP can be added pretty easily too. >> That's why please suggest a way what you are comfortable with not to >> block functionality on these devices. > > Look at the A10 nand branch, it uses full fit with all the bells and > whistles. Maybe that's the way to go if you have DRAM available. Enabling full fit should be possible but there is really no need to enable more and more features for load something to fpga. SPL still needs to fit to small space. Thanks, Michal
Hi, On Thu, Jul 19, 2018 at 1:58 AM Michal Simek <michal.simek@xilinx.com> wrote: > On 18.7.2018 22:11, Marek Vasut wrote: > > On 07/18/2018 04:57 PM, Michal Simek wrote: > >> On 18.7.2018 16:24, Marek Vasut wrote: > >>> On 07/18/2018 04:18 PM, Michal Simek wrote: > >>>> On 18.7.2018 16:15, Marek Vasut wrote: > >>>>> On 07/18/2018 04:00 PM, Michal Simek wrote: > >>>>>> On 18.7.2018 14:54, Marek Vasut wrote: > >>>>>>> This breaks Arria10, sorry. The private loading function is needed on > >>>>>>> Arria10 as the whole bitstream is not available in RAM and needs to be > >>>>>>> loaded piece by piece, see [1] > >>>>>>> > >>>>>>> [1] > >>>>>>> http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef > >>> > >>> [...] > >>>>>> The second solution is to check if load address is not 0 and call > >>>>>> fpga_load only for that. In this case there is a need to check size for SPL. > >>>>> > >>>>> 0 is a valid load address, so no. > >>>> > >>>> Then new Kconfig option is another way to go now. > >>> > >>> No, the firmware loader is a way to go. Sadly, it's still work in progress. > >> > >> I have looked at that series and it will take some time to get it done > >> but even that series has no user and also only support filesystems. > >> Which is fine but not enough and support for RAW mode is necessary too. > > > > It already took like a year and half I think ... well, better invest > > your resources in perfecting it for your usecase, for that's the way to go. > > I don't think so because for SPL boot we need support for a RAW mode. > > > > >> Anyway Luis sent series where this SPL fpga supported is requested to > >> add and I had this functionality in more raw state in Xilinx tree for > >> quite a long time and it is time to support it because on Zybo (because > >> of i2c eeprom) and also cc108 (because of uart routing via PL) fpga load > >> needs to be done in SPL and we need that support. > > > > Ha > > > >> I have not a problem to keep your code in SPL but I need a way to enable > >> fpga load directly with all that features like hashes which are already > >> available. GZIP can be added pretty easily too. > >> That's why please suggest a way what you are comfortable with not to > >> block functionality on these devices. > > > > Look at the A10 nand branch, it uses full fit with all the bells and > > whistles. Maybe that's the way to go if you have DRAM available. > > Enabling full fit should be possible but there is really no need to > enable more and more features for load something to fpga. SPL still > needs to fit to small space. I tested Michal's patch by replacing the fourth patch of my series [1] with his patch, and it worked fine on a Zybo Z7-20. I think that for devices with DRAM available is the way to go, but as Marek said, it will brake Arria10. I'd like to see the functionality merged, but waiting to firmware loader might delay it. So I propose the following: - Keep the spl_load_fpga_image() function for devices like the Arria10. - Apply the first three patches of my series [1], as they fix FPGA (Zynq) support on SPL. - Apply a modified version of Michal's patch, adding a temporary Kconfig option, like he suggested, to choose between the two implementations: fpga_load() for devices with DRAM available, and spl_load_fpga_image() for devices like the Arria10. - Once firmware loader is ready, convert the code to use it and unify the functions if possible. [1] https://lists.denx.de/pipermail/u-boot/2018-July/335127.html Thanks, Luis Araneda
On 19.7.2018 08:36, Luis Araneda wrote: > Hi, > > On Thu, Jul 19, 2018 at 1:58 AM Michal Simek <michal.simek@xilinx.com> wrote: >> On 18.7.2018 22:11, Marek Vasut wrote: >>> On 07/18/2018 04:57 PM, Michal Simek wrote: >>>> On 18.7.2018 16:24, Marek Vasut wrote: >>>>> On 07/18/2018 04:18 PM, Michal Simek wrote: >>>>>> On 18.7.2018 16:15, Marek Vasut wrote: >>>>>>> On 07/18/2018 04:00 PM, Michal Simek wrote: >>>>>>>> On 18.7.2018 14:54, Marek Vasut wrote: >>>>>>>>> This breaks Arria10, sorry. The private loading function is needed on >>>>>>>>> Arria10 as the whole bitstream is not available in RAM and needs to be >>>>>>>>> loaded piece by piece, see [1] >>>>>>>>> >>>>>>>>> [1] >>>>>>>>> http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef >>>>> >>>>> [...] >>>>>>>> The second solution is to check if load address is not 0 and call >>>>>>>> fpga_load only for that. In this case there is a need to check size for SPL. >>>>>>> >>>>>>> 0 is a valid load address, so no. >>>>>> >>>>>> Then new Kconfig option is another way to go now. >>>>> >>>>> No, the firmware loader is a way to go. Sadly, it's still work in progress. >>>> >>>> I have looked at that series and it will take some time to get it done >>>> but even that series has no user and also only support filesystems. >>>> Which is fine but not enough and support for RAW mode is necessary too. >>> >>> It already took like a year and half I think ... well, better invest >>> your resources in perfecting it for your usecase, for that's the way to go. >> >> I don't think so because for SPL boot we need support for a RAW mode. >> >>> >>>> Anyway Luis sent series where this SPL fpga supported is requested to >>>> add and I had this functionality in more raw state in Xilinx tree for >>>> quite a long time and it is time to support it because on Zybo (because >>>> of i2c eeprom) and also cc108 (because of uart routing via PL) fpga load >>>> needs to be done in SPL and we need that support. >>> >>> Ha >>> >>>> I have not a problem to keep your code in SPL but I need a way to enable >>>> fpga load directly with all that features like hashes which are already >>>> available. GZIP can be added pretty easily too. >>>> That's why please suggest a way what you are comfortable with not to >>>> block functionality on these devices. >>> >>> Look at the A10 nand branch, it uses full fit with all the bells and >>> whistles. Maybe that's the way to go if you have DRAM available. >> >> Enabling full fit should be possible but there is really no need to >> enable more and more features for load something to fpga. SPL still >> needs to fit to small space. > > I tested Michal's patch by replacing the fourth patch of my series [1] > with his patch, and it worked fine on a Zybo Z7-20. I think that for > devices with DRAM available is the way to go, but as Marek said, it > will brake Arria10. It doesn't break anything in mainline because there is no support for Arria10. His patch for this interface shouldn't reach mainline like it is normal style in Linux. No user, no interface. Also if there is interface already but no user then interfaces are removed like happened with SG_DMA. > > I'd like to see the functionality merged, but waiting to firmware > loader might delay it. So I propose the following: > - Keep the spl_load_fpga_image() function for devices > like the Arria10. > - Apply the first three patches of my series [1], > as they fix FPGA (Zynq) support on SPL. They are fine please send me as regular patches and I will take them. > - Apply a modified version of Michal's patch, adding > a temporary Kconfig option, like he suggested, > to choose between the two implementations: > fpga_load() for devices with DRAM available, and > spl_load_fpga_image() for devices like the Arria10. > - Once firmware loader is ready, convert the code to > use it and unify the functions if possible. Let's continue to talk with Marek on this. If there he is not open to better solution we need to create new Kconfig property to cover it. Thanks, Michal
On 07/19/2018 07:57 AM, Michal Simek wrote: > On 18.7.2018 22:11, Marek Vasut wrote: >> On 07/18/2018 04:57 PM, Michal Simek wrote: >>> On 18.7.2018 16:24, Marek Vasut wrote: >>>> On 07/18/2018 04:18 PM, Michal Simek wrote: >>>>> On 18.7.2018 16:15, Marek Vasut wrote: >>>>>> On 07/18/2018 04:00 PM, Michal Simek wrote: >>>>>>> On 18.7.2018 14:54, Marek Vasut wrote: >>>>>>>> On 07/18/2018 02:51 PM, Michal Simek wrote: >>>>>>>>> There shouldn't be a need to call private spl_load_fpga_image() >>>>>>>>> because the whole sequence should be already handled by fpga_load(). >>>>>>>>> The patch let spl_load_fit_image() to load data to right location based >>>>>>>>> on "load" property in FIT and then call fpga_load(). >>>>>>>> >>>>>>>> NAK >>>>>>>> >>>>>>>> This breaks Arria10, sorry. The private loading function is needed on >>>>>>>> Arria10 as the whole bitstream is not available in RAM and needs to be >>>>>>>> loaded piece by piece, see [1] >>>>>>>> >>>>>>>> [1] >>>>>>>> http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef >>>> >>>> [...] >>>>>>> The second solution is to check if load address is not 0 and call >>>>>>> fpga_load only for that. In this case there is a need to check size for SPL. >>>>>> >>>>>> 0 is a valid load address, so no. >>>>> >>>>> Then new Kconfig option is another way to go now. >>>> >>>> No, the firmware loader is a way to go. Sadly, it's still work in progress. >>> >>> I have looked at that series and it will take some time to get it done >>> but even that series has no user and also only support filesystems. >>> Which is fine but not enough and support for RAW mode is necessary too. >> >> It already took like a year and half I think ... well, better invest >> your resources in perfecting it for your usecase, for that's the way to go. > > I don't think so because for SPL boot we need support for a RAW mode. > >> >>> Anyway Luis sent series where this SPL fpga supported is requested to >>> add and I had this functionality in more raw state in Xilinx tree for >>> quite a long time and it is time to support it because on Zybo (because >>> of i2c eeprom) and also cc108 (because of uart routing via PL) fpga load >>> needs to be done in SPL and we need that support. >> >> Ha >> >>> I have not a problem to keep your code in SPL but I need a way to enable >>> fpga load directly with all that features like hashes which are already >>> available. GZIP can be added pretty easily too. >>> That's why please suggest a way what you are comfortable with not to >>> block functionality on these devices. >> >> Look at the A10 nand branch, it uses full fit with all the bells and >> whistles. Maybe that's the way to go if you have DRAM available. > > Enabling full fit should be possible but there is really no need to > enable more and more features for load something to fpga. SPL still > needs to fit to small space. Bitrot protection is always a good idea when loading from raw storage.
On 07/19/2018 08:44 AM, Michal Simek wrote: > On 19.7.2018 08:36, Luis Araneda wrote: >> Hi, >> >> On Thu, Jul 19, 2018 at 1:58 AM Michal Simek <michal.simek@xilinx.com> wrote: >>> On 18.7.2018 22:11, Marek Vasut wrote: >>>> On 07/18/2018 04:57 PM, Michal Simek wrote: >>>>> On 18.7.2018 16:24, Marek Vasut wrote: >>>>>> On 07/18/2018 04:18 PM, Michal Simek wrote: >>>>>>> On 18.7.2018 16:15, Marek Vasut wrote: >>>>>>>> On 07/18/2018 04:00 PM, Michal Simek wrote: >>>>>>>>> On 18.7.2018 14:54, Marek Vasut wrote: >>>>>>>>>> This breaks Arria10, sorry. The private loading function is needed on >>>>>>>>>> Arria10 as the whole bitstream is not available in RAM and needs to be >>>>>>>>>> loaded piece by piece, see [1] >>>>>>>>>> >>>>>>>>>> [1] >>>>>>>>>> http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef >>>>>> >>>>>> [...] >>>>>>>>> The second solution is to check if load address is not 0 and call >>>>>>>>> fpga_load only for that. In this case there is a need to check size for SPL. >>>>>>>> >>>>>>>> 0 is a valid load address, so no. >>>>>>> >>>>>>> Then new Kconfig option is another way to go now. >>>>>> >>>>>> No, the firmware loader is a way to go. Sadly, it's still work in progress. >>>>> >>>>> I have looked at that series and it will take some time to get it done >>>>> but even that series has no user and also only support filesystems. >>>>> Which is fine but not enough and support for RAW mode is necessary too. >>>> >>>> It already took like a year and half I think ... well, better invest >>>> your resources in perfecting it for your usecase, for that's the way to go. >>> >>> I don't think so because for SPL boot we need support for a RAW mode. >>> >>>> >>>>> Anyway Luis sent series where this SPL fpga supported is requested to >>>>> add and I had this functionality in more raw state in Xilinx tree for >>>>> quite a long time and it is time to support it because on Zybo (because >>>>> of i2c eeprom) and also cc108 (because of uart routing via PL) fpga load >>>>> needs to be done in SPL and we need that support. >>>> >>>> Ha >>>> >>>>> I have not a problem to keep your code in SPL but I need a way to enable >>>>> fpga load directly with all that features like hashes which are already >>>>> available. GZIP can be added pretty easily too. >>>>> That's why please suggest a way what you are comfortable with not to >>>>> block functionality on these devices. >>>> >>>> Look at the A10 nand branch, it uses full fit with all the bells and >>>> whistles. Maybe that's the way to go if you have DRAM available. >>> >>> Enabling full fit should be possible but there is really no need to >>> enable more and more features for load something to fpga. SPL still >>> needs to fit to small space. >> >> I tested Michal's patch by replacing the fourth patch of my series [1] >> with his patch, and it worked fine on a Zybo Z7-20. I think that for >> devices with DRAM available is the way to go, but as Marek said, it >> will brake Arria10. > > It doesn't break anything in mainline because there is no support for > Arria10. You might want to git grep a bit ? > His patch for this interface shouldn't reach mainline like it > is normal style in Linux. No user, no interface. Also if there is > interface already but no user then interfaces are removed like happened > with SG_DMA. Uhh, that's some strong wording right there. We have many other __weak functions in U-Boot which are probably never implemented either. Why don't you clean those up too instead of focusing on one which enables competing platform to work and is protected by Kconfig option, so it doesn't change anything for the other platforms ? >> I'd like to see the functionality merged, but waiting to firmware >> loader might delay it. So I propose the following: >> - Keep the spl_load_fpga_image() function for devices >> like the Arria10. >> - Apply the first three patches of my series [1], >> as they fix FPGA (Zynq) support on SPL. > > They are fine please send me as regular patches and I will take them. > >> - Apply a modified version of Michal's patch, adding >> a temporary Kconfig option, like he suggested, >> to choose between the two implementations: >> fpga_load() for devices with DRAM available, and >> spl_load_fpga_image() for devices like the Arria10. >> - Once firmware loader is ready, convert the code to >> use it and unify the functions if possible. > > Let's continue to talk with Marek on this. If there he is not open to > better solution we need to create new Kconfig property to cover it. Like CONFIG_SPL_FPGA_SUPPORT ?
On 19.7.2018 10:35, Marek Vasut wrote: > On 07/19/2018 08:44 AM, Michal Simek wrote: >> On 19.7.2018 08:36, Luis Araneda wrote: >>> Hi, >>> >>> On Thu, Jul 19, 2018 at 1:58 AM Michal Simek <michal.simek@xilinx.com> wrote: >>>> On 18.7.2018 22:11, Marek Vasut wrote: >>>>> On 07/18/2018 04:57 PM, Michal Simek wrote: >>>>>> On 18.7.2018 16:24, Marek Vasut wrote: >>>>>>> On 07/18/2018 04:18 PM, Michal Simek wrote: >>>>>>>> On 18.7.2018 16:15, Marek Vasut wrote: >>>>>>>>> On 07/18/2018 04:00 PM, Michal Simek wrote: >>>>>>>>>> On 18.7.2018 14:54, Marek Vasut wrote: >>>>>>>>>>> This breaks Arria10, sorry. The private loading function is needed on >>>>>>>>>>> Arria10 as the whole bitstream is not available in RAM and needs to be >>>>>>>>>>> loaded piece by piece, see [1] >>>>>>>>>>> >>>>>>>>>>> [1] >>>>>>>>>>> http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef >>>>>>> >>>>>>> [...] >>>>>>>>>> The second solution is to check if load address is not 0 and call >>>>>>>>>> fpga_load only for that. In this case there is a need to check size for SPL. >>>>>>>>> >>>>>>>>> 0 is a valid load address, so no. >>>>>>>> >>>>>>>> Then new Kconfig option is another way to go now. >>>>>>> >>>>>>> No, the firmware loader is a way to go. Sadly, it's still work in progress. >>>>>> >>>>>> I have looked at that series and it will take some time to get it done >>>>>> but even that series has no user and also only support filesystems. >>>>>> Which is fine but not enough and support for RAW mode is necessary too. >>>>> >>>>> It already took like a year and half I think ... well, better invest >>>>> your resources in perfecting it for your usecase, for that's the way to go. >>>> >>>> I don't think so because for SPL boot we need support for a RAW mode. >>>> >>>>> >>>>>> Anyway Luis sent series where this SPL fpga supported is requested to >>>>>> add and I had this functionality in more raw state in Xilinx tree for >>>>>> quite a long time and it is time to support it because on Zybo (because >>>>>> of i2c eeprom) and also cc108 (because of uart routing via PL) fpga load >>>>>> needs to be done in SPL and we need that support. >>>>> >>>>> Ha >>>>> >>>>>> I have not a problem to keep your code in SPL but I need a way to enable >>>>>> fpga load directly with all that features like hashes which are already >>>>>> available. GZIP can be added pretty easily too. >>>>>> That's why please suggest a way what you are comfortable with not to >>>>>> block functionality on these devices. >>>>> >>>>> Look at the A10 nand branch, it uses full fit with all the bells and >>>>> whistles. Maybe that's the way to go if you have DRAM available. >>>> >>>> Enabling full fit should be possible but there is really no need to >>>> enable more and more features for load something to fpga. SPL still >>>> needs to fit to small space. >>> >>> I tested Michal's patch by replacing the fourth patch of my series [1] >>> with his patch, and it worked fine on a Zybo Z7-20. I think that for >>> devices with DRAM available is the way to go, but as Marek said, it >>> will brake Arria10. >> >> It doesn't break anything in mainline because there is no support for >> Arria10. > > You might want to git grep a bit ? I meant arria10 spl fpga support not soc itself. > >> His patch for this interface shouldn't reach mainline like it >> is normal style in Linux. No user, no interface. Also if there is >> interface already but no user then interfaces are removed like happened >> with SG_DMA. > > Uhh, that's some strong wording right there. We have many other __weak > functions in U-Boot which are probably never implemented either. Why > don't you clean those up too instead of focusing on one which enables > competing platform to work and is protected by Kconfig option, so it > doesn't change anything for the other platforms ? I think all of them should be removed. >>> I'd like to see the functionality merged, but waiting to firmware >>> loader might delay it. So I propose the following: >>> - Keep the spl_load_fpga_image() function for devices >>> like the Arria10. >>> - Apply the first three patches of my series [1], >>> as they fix FPGA (Zynq) support on SPL. >> >> They are fine please send me as regular patches and I will take them. >> >>> - Apply a modified version of Michal's patch, adding >>> a temporary Kconfig option, like he suggested, >>> to choose between the two implementations: >>> fpga_load() for devices with DRAM available, and >>> spl_load_fpga_image() for devices like the Arria10. >>> - Once firmware loader is ready, convert the code to >>> use it and unify the functions if possible. >> >> Let's continue to talk with Marek on this. If there he is not open to >> better solution we need to create new Kconfig property to cover it. > > Like CONFIG_SPL_FPGA_SUPPORT ? One approach is to copy the whole image to DDR and use it. The second copy it by chunks. The first is what we need for Xilinx. The second what you need for Altera because you don't have DDR up and running. That's why there needs to be likely to Kconfig entries. Thanks, Michal
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 9eabb1c1058b..8ab6a2def1e2 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -6,6 +6,7 @@ #include <common.h> #include <errno.h> +#include <fpga.h> #include <image.h> #include <linux/libfdt.h> #include <spl.h> @@ -140,14 +141,6 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size, return (data_size + info->bl_len - 1) / info->bl_len; } -#ifdef CONFIG_SPL_FPGA_SUPPORT -__weak int spl_load_fpga_image(struct spl_load_info *info, size_t length, - int nr_sectors, int sector_offset) -{ - return 0; -} -#endif - /** * spl_load_fit_image(): load the image described in a certain FIT node * @info: points to information about the device to load data from @@ -169,7 +162,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector, void *fit, ulong base_offset, int node, struct spl_image_info *image_info) { - int offset, sector_offset; + int offset; size_t length; int len; ulong size; @@ -217,16 +210,9 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector, overhead = get_aligned_image_overhead(info, offset); nr_sectors = get_aligned_image_size(info, length, offset); - sector_offset = sector + get_aligned_image_offset(info, offset); - -#ifdef CONFIG_SPL_FPGA_SUPPORT - if (type == IH_TYPE_FPGA) { - return spl_load_fpga_image(info, length, nr_sectors, - sector_offset); - } -#endif - if (info->read(info, sector_offset, + if (info->read(info, + sector + get_aligned_image_offset(info, offset), nr_sectors, (void *)load_ptr) != nr_sectors) return -EIO; @@ -412,6 +398,18 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, printf("%s: Cannot load the FPGA: %i\n", __func__, ret); return ret; } + + debug("FPGA bitstream at: %x, size: %x\n", + (u32)spl_image->load_addr, spl_image->size); + + ret = fpga_load(0, (const void *)spl_image->load_addr, + spl_image->size, BIT_FULL); + if (ret) { + printf("%s: Cannot load the image to the FPGA\n", + __func__); + return ret; + } + puts("FPGA image loaded from FIT\n"); node = -1; }
There shouldn't be a need to call private spl_load_fpga_image() because the whole sequence should be already handled by fpga_load(). The patch let spl_load_fit_image() to load data to right location based on "load" property in FIT and then call fpga_load(). The patch is partially reverting "spl: fit: Add support for loading FPGA bitstream" (sha1: 26a642238bdecc53527142dc043b29e21c5cc94c) Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- <debug_uart> Debug uart enabled U-Boot SPL 2018.07-00185-g4c2b86d7a763 (Jul 18 2018 - 14:49:35 +0200) mmc boot Trying to boot from MMC1 spl_load_image_fat_os: error reading image system.dtb, err - -2 zynq_align_dma_buffer: Bitstream is not swapped(1) - swap it FPGA image loaded from FIT U-Boot 2018.07-00185-g4c2b86d7a763 (Jul 18 2018 - 14:49:35 +0200) Xilinx Zynq ZC706 CPU: Zynq 7z045 Silicon: v1.0 Model: Zynq ZC706 Development Board I2C: ready DRAM: ECC disabled 1 GiB Watchdog: Started MMC: sdhci@e0100000: 0 Loading Environment from SPI Flash... SF: Detected s25fl128s_64k with page size 256 Bytes, erase size 64 KiB, total 16 MiB *** Warning - bad CRC, using default environment Failed (-5) In: serial@e0001000 Out: serial@e0001000 Err: serial@e0001000 Net: ZYNQ GEM: e000b000, phyaddr 7, interface rgmii-id Warning: ethernet@e000b000 (eth0) using random MAC address - 6e:3a:1c:22:aa:5f eth0: ethernet@e000b000 Checking if uenvcmd is set ... Hit any key to stop autoboot: 0 Zynq> --- common/spl/spl_fit.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)