Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On 02/20/2017 06:35 PM, Moritz Fischer wrote: > Hi all, > > On Mon, Feb 20, 2017 at 6:59 AM, Michal Simekwrote: > >> Definitely I am open to improve this subsystem to make it more flexible. > > Over at linux-fpga ([1]) we're currently discussing the idea of coming > up with a header > format (vendor neutral) that we stitch onto the fpga image to allow the > fpga-mgr > to derive certain features of the image from the image itself (i.e. is > the image encrypted, > compressed, ...). Can you CC me on that discussion ? I think it'd be for the best to use DT for the header format, it's documented and vendor-neutral already. > We're trying to come up with an extensible format that would allow us > to add stuff like > encryption keys etc. Nothing like a NIH format :) > Cheers, > > Moritz > > > [1] https://patchwork.kernel.org/patch/9574399/ > -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
Hi all, On Mon, Feb 20, 2017 at 6:59 AM, Michal Simekwrote: > Definitely I am open to improve this subsystem to make it more flexible. Over at linux-fpga ([1]) we're currently discussing the idea of coming up with a header format (vendor neutral) that we stitch onto the fpga image to allow the fpga-mgr to derive certain features of the image from the image itself (i.e. is the image encrypted, compressed, ...). We're trying to come up with an extensible format that would allow us to add stuff like encryption keys etc. Cheers, Moritz [1] https://patchwork.kernel.org/patch/9574399/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On 20.2.2017 15:30, Dalon Westergreen wrote: > On Mon, 2017-02-20 at 10:22 +0100, Michal Simek wrote: >> On 19.2.2017 21:58, Dalon Westergreen wrote: >>> >>> On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote: On 02/19/2017 09:43 PM, Dalon Westergreen wrote: > > > On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: >> >> >> On 02/19/2017 08:49 PM, Dalon Westergreen wrote: >>> >>> >>> >>> The implementation of boot_get_fpga only supported one fpga family. >>> This modification allows for any of the fpga devices supported by >>> fpga_load to be used. >>> >>> Signed-off-by: Dalon Westergreen>> >> +CC Xilinx friends :) >> >>> >>> >>> >>> --- >>> common/image.c | 37 ++--- >>> 1 file changed, 22 insertions(+), 15 deletions(-) >>> >>> diff --git a/common/image.c b/common/image.c >>> index 0f88984..792d371 100644 >>> --- a/common/image.c >>> +++ b/common/image.c >>> @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, >>> uint8_t >>> arch, >>> } >>> >>> #if IMAGE_ENABLE_FIT >>> -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) >>> +#if defined(CONFIG_FPGA) >>> int boot_get_fpga(int argc, char * const argv[], bootm_headers_t >>> *images, >>> uint8_t arch, const ulong *ld_start, ulong * >>> const >>> ld_len) >>> { >>> @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const >>> argv[], >>> bootm_headers_t *images, >>> int err; >>> int devnum = 0; /* TODO support multi fpga platforms */ >>> const fpga_desc * const desc = fpga_get_desc(devnum); >>> - xilinx_desc *desc_xilinx = desc->devdesc; >>> + xilinx_desc *desc_xilinx; >>> + bitstream_type bstype; >>> >>> /* Check to see if the images struct has a FIT >>> configuration */ >>> if (!genimg_has_config(images)) { >>> @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const >>> argv[], >>> bootm_headers_t *images, >>> return fit_img_result; >>> } >>> >>> - if (img_len >= desc_xilinx->size) { >>> + switch (desc->devtype) { >> >> Do we need the switch statement at all ? We can have full >> configuration >> as a default mode of operation and have something like >> >> if (xilinx) { >> if (partial reconfiguration) { >> do_special_setup(); >> } >> } > > I only did the switch stuff b/c i envisioned a need for partial image > support for socfpga. That'd be seriously cool :) > > > That said, i would suggest, as you mention, moving > this to platform specific code and perhaps an indication of the image > type > in the fitimage. driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ? >>> >>> as far as i understand it, it is all in the blob. All that is needed is >>> knowing >>> whether the blob is a full or partial image. X seems to just use the image >>> size >>> to determine this, but that means having a table of all devices and their >>> respective full image size. seems simpler to just specify the image type is >>> partial or not in the fitimage. >> >> We did that for zynq when we did that for the first time. But not for >> zynqmp. Zynq is maybe still using it but it shouldn't now. It is not >> 100% reliable way. Definitely having DT property is the best option >> because you can add sort of "nop" which extend bitstream size and does >> nothing which breaks that checking. >> >> For full u-boot there is loadb, loadbp, load and loadp to distinguish it. > > That brings up an interesting point, right now the fpga_loadbitstream doesn't > follow the same method as fpga_load for allowing multiple FPGA types to be > supported simultaneously. Would it not be prudent to move in that direction? > I believe only xilinx implements this right now. loadbitstream is there for a long time which is in general just header bitstream header parser written in pretty bad way. Right now we are adding support for encrypted and authenticated images and for that we are adding fpga loads command where you specify by flag if you want to pass encrypted or authenticated image and keys. There is also loadmk which should be improved a lot exactly by what you are trying to do. It means we should wrap bitstreams with flags which says what it is inside. On the other hand having separate commands it is easy for testing. Definitely I am open to improve this subsystem to make
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On Mon, 2017-02-20 at 10:22 +0100, Michal Simek wrote: > On 19.2.2017 21:58, Dalon Westergreen wrote: > > > > On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote: > > > > > > On 02/19/2017 09:43 PM, Dalon Westergreen wrote: > > > > > > > > > > > > On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: > > > > > > > > > > > > > > > On 02/19/2017 08:49 PM, Dalon Westergreen wrote: > > > > > > > > > > > > > > > > > > > > > > > > The implementation of boot_get_fpga only supported one fpga family. > > > > > > This modification allows for any of the fpga devices supported by > > > > > > fpga_load to be used. > > > > > > > > > > > > Signed-off-by: Dalon Westergreen> > > > > > > > > > +CC Xilinx friends :) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > common/image.c | 37 ++--- > > > > > > 1 file changed, 22 insertions(+), 15 deletions(-) > > > > > > > > > > > > diff --git a/common/image.c b/common/image.c > > > > > > index 0f88984..792d371 100644 > > > > > > --- a/common/image.c > > > > > > +++ b/common/image.c > > > > > > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, > > > > > > uint8_t > > > > > > arch, > > > > > > } > > > > > > > > > > > > #if IMAGE_ENABLE_FIT > > > > > > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) > > > > > > +#if defined(CONFIG_FPGA) > > > > > > int boot_get_fpga(int argc, char * const argv[], bootm_headers_t > > > > > > *images, > > > > > > uint8_t arch, const ulong *ld_start, ulong * > > > > > > const > > > > > > ld_len) > > > > > > { > > > > > > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const > > > > > > argv[], > > > > > > bootm_headers_t *images, > > > > > > int err; > > > > > > int devnum = 0; /* TODO support multi fpga platforms */ > > > > > > const fpga_desc * const desc = fpga_get_desc(devnum); > > > > > > - xilinx_desc *desc_xilinx = desc->devdesc; > > > > > > + xilinx_desc *desc_xilinx; > > > > > > + bitstream_type bstype; > > > > > > > > > > > > /* Check to see if the images struct has a FIT > > > > > > configuration */ > > > > > > if (!genimg_has_config(images)) { > > > > > > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const > > > > > > argv[], > > > > > > bootm_headers_t *images, > > > > > > return fit_img_result; > > > > > > } > > > > > > > > > > > > - if (img_len >= desc_xilinx->size) { > > > > > > + switch (desc->devtype) { > > > > > > > > > > Do we need the switch statement at all ? We can have full > > > > > configuration > > > > > as a default mode of operation and have something like > > > > > > > > > > if (xilinx) { > > > > > if (partial reconfiguration) { > > > > > do_special_setup(); > > > > > } > > > > > } > > > > > > > > I only did the switch stuff b/c i envisioned a need for partial image > > > > support for socfpga. > > > > > > That'd be seriously cool :) > > > > > > > > > > > > > > > That said, i would suggest, as you mention, moving > > > > this to platform specific code and perhaps an indication of the image > > > > type > > > > in the fitimage. > > > > > > driver-specific code . It doesn't need to know the imagetype, just that > > > the blob that you passed in is a partial-reconfiguration blob. I never > > > really worked with P/R though, do you need some other metadata for that > > > or is it contained in that P/R bitstream blob already ? > > > > as far as i understand it, it is all in the blob. All that is needed is > > knowing > > whether the blob is a full or partial image. X seems to just use the image > > size > > to determine this, but that means having a table of all devices and their > > respective full image size. seems simpler to just specify the image type is > > partial or not in the fitimage. > > We did that for zynq when we did that for the first time. But not for > zynqmp. Zynq is maybe still using it but it shouldn't now. It is not > 100% reliable way. Definitely having DT property is the best option > because you can add sort of "nop" which extend bitstream size and does > nothing which breaks that checking. > > For full u-boot there is loadb, loadbp, load and loadp to distinguish it. That brings up an interesting point, right now the fpga_loadbitstream doesn't follow the same method as fpga_load for allowing multiple FPGA types to be supported simultaneously. Would it not be prudent to move in that direction? I believe only xilinx implements this right now. --dalon > Thanks, > Michal > > Thanks, > Michal > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On Mon, 2017-02-20 at 10:24 +0100, Michal Simek wrote: > On 19.2.2017 22:26, Marek Vasut wrote: > > > > On 02/19/2017 10:21 PM, Dalon Westergreen wrote: > > > > > > On Sun, 2017-02-19 at 22:12 +0100, Marek Vasut wrote: > > > > > > > > On 02/19/2017 09:58 PM, Dalon Westergreen wrote: > > > > > > > > > > > > > > > On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote: > > > > > > > > > > > > > > > > > > On 02/19/2017 09:43 PM, Dalon Westergreen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 02/19/2017 08:49 PM, Dalon Westergreen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The implementation of boot_get_fpga only supported one fpga > > > > > > > > > family. > > > > > > > > > This modification allows for any of the fpga devices supported > > > > > > > > > by > > > > > > > > > fpga_load to be used. > > > > > > > > > > > > > > > > > > Signed-off-by: Dalon Westergreen> > > > > > > > > > > > > > > > +CC Xilinx friends :) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > common/image.c | 37 ++--- > > > > > > > > > 1 file changed, 22 insertions(+), 15 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/common/image.c b/common/image.c > > > > > > > > > index 0f88984..792d371 100644 > > > > > > > > > --- a/common/image.c > > > > > > > > > +++ b/common/image.c > > > > > > > > > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t > > > > > > > > > *images, > > > > > > > > > uint8_t > > > > > > > > > arch, > > > > > > > > > } > > > > > > > > > > > > > > > > > > #if IMAGE_ENABLE_FIT > > > > > > > > > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) > > > > > > > > > +#if defined(CONFIG_FPGA) > > > > > > > > > int boot_get_fpga(int argc, char * const argv[], > > > > > > > > > bootm_headers_t > > > > > > > > > *images, > > > > > > > > > uint8_t arch, const ulong *ld_start, ulong > > > > > > > > > * > > > > > > > > > const > > > > > > > > > ld_len) > > > > > > > > > { > > > > > > > > > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const > > > > > > > > > argv[], > > > > > > > > > bootm_headers_t *images, > > > > > > > > > int err; > > > > > > > > > int devnum = 0; /* TODO support multi fpga platforms > > > > > > > > > */ > > > > > > > > > const fpga_desc * const desc = fpga_get_desc(devnum); > > > > > > > > > - xilinx_desc *desc_xilinx = desc->devdesc; > > > > > > > > > + xilinx_desc *desc_xilinx; > > > > > > > > > + bitstream_type bstype; > > > > > > > > > > > > > > > > > > /* Check to see if the images struct has a FIT > > > > > > > > > configuration */ > > > > > > > > > if (!genimg_has_config(images)) { > > > > > > > > > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * > > > > > > > > > const > > > > > > > > > argv[], > > > > > > > > > bootm_headers_t *images, > > > > > > > > > return fit_img_result; > > > > > > > > > } > > > > > > > > > > > > > > > > > > - if (img_len >= desc_xilinx->size) { > > > > > > > > > + switch (desc->devtype) { > > > > > > > > > > > > > > > > Do we need the switch statement at all ? We can have full > > > > > > > > configuration > > > > > > > > as a default mode of operation and have something like > > > > > > > > > > > > > > > > if (xilinx) { > > > > > > > > if (partial reconfiguration) { > > > > > > > > do_special_setup(); > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > I only did the switch stuff b/c i envisioned a need for partial > > > > > > > image > > > > > > > support for socfpga. > > > > > > > > > > > > That'd be seriously cool :) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > That said, i would suggest, as you mention, moving > > > > > > > this to platform specific code and perhaps an indication of the > > > > > > > image > > > > > > > type > > > > > > > in the fitimage. > > > > > > > > > > > > driver-specific code . It doesn't need to know the imagetype, just > > > > > > that > > > > > > the blob that you passed in is a partial-reconfiguration blob. I > > > > > > never > > > > > > really worked with P/R though, do you need some other metadata for > > > > > > that > > > > > > or is it contained in that P/R bitstream blob already ? > > > > > > > > > > as far as i understand it, it is all in the blob. All that is needed > > > > > is > > > > > knowing > > > > > whether the blob is a full or partial image. X seems to just use the > > > > > image > > > > > size > > > > > to determine this, but that means having a table of all devices and > > > > > their > > > > > respective full image size. seems simpler to just specify the
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On 19.2.2017 21:58, Dalon Westergreen wrote: > On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote: >> On 02/19/2017 09:43 PM, Dalon Westergreen wrote: >>> >>> On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: On 02/19/2017 08:49 PM, Dalon Westergreen wrote: > > > The implementation of boot_get_fpga only supported one fpga family. > This modification allows for any of the fpga devices supported by > fpga_load to be used. > > Signed-off-by: Dalon Westergreen+CC Xilinx friends :) > > > --- > common/image.c | 37 ++--- > 1 file changed, 22 insertions(+), 15 deletions(-) > > diff --git a/common/image.c b/common/image.c > index 0f88984..792d371 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, > uint8_t > arch, > } > > #if IMAGE_ENABLE_FIT > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) > +#if defined(CONFIG_FPGA) > int boot_get_fpga(int argc, char * const argv[], bootm_headers_t > *images, > uint8_t arch, const ulong *ld_start, ulong * const > ld_len) > { > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], > bootm_headers_t *images, > int err; > int devnum = 0; /* TODO support multi fpga platforms */ > const fpga_desc * const desc = fpga_get_desc(devnum); > - xilinx_desc *desc_xilinx = desc->devdesc; > + xilinx_desc *desc_xilinx; > + bitstream_type bstype; > > /* Check to see if the images struct has a FIT configuration */ > if (!genimg_has_config(images)) { > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], > bootm_headers_t *images, > return fit_img_result; > } > > - if (img_len >= desc_xilinx->size) { > + switch (desc->devtype) { Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like if (xilinx) { if (partial reconfiguration) { do_special_setup(); } } >>> >>> I only did the switch stuff b/c i envisioned a need for partial image >>> support for socfpga. >> >> That'd be seriously cool :) >> >>> >>> That said, i would suggest, as you mention, moving >>> this to platform specific code and perhaps an indication of the image type >>> in the fitimage. >> >> driver-specific code . It doesn't need to know the imagetype, just that >> the blob that you passed in is a partial-reconfiguration blob. I never >> really worked with P/R though, do you need some other metadata for that >> or is it contained in that P/R bitstream blob already ? > > as far as i understand it, it is all in the blob. All that is needed is > knowing > whether the blob is a full or partial image. X seems to just use the image > size > to determine this, but that means having a table of all devices and their > respective full image size. seems simpler to just specify the image type is > partial or not in the fitimage. We did that for zynq when we did that for the first time. But not for zynqmp. Zynq is maybe still using it but it shouldn't now. It is not 100% reliable way. Definitely having DT property is the best option because you can add sort of "nop" which extend bitstream size and does nothing which breaks that checking. For full u-boot there is loadb, loadbp, load and loadp to distinguish it. Thanks, Michal Thanks, Michal ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On 19.2.2017 22:26, Marek Vasut wrote: > On 02/19/2017 10:21 PM, Dalon Westergreen wrote: >> On Sun, 2017-02-19 at 22:12 +0100, Marek Vasut wrote: >>> On 02/19/2017 09:58 PM, Dalon Westergreen wrote: On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote: > > On 02/19/2017 09:43 PM, Dalon Westergreen wrote: >> >> >> On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: >>> >>> >>> On 02/19/2017 08:49 PM, Dalon Westergreen wrote: The implementation of boot_get_fpga only supported one fpga family. This modification allows for any of the fpga devices supported by fpga_load to be used. Signed-off-by: Dalon Westergreen>>> >>> +CC Xilinx friends :) >>> --- common/image.c | 37 ++--- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/common/image.c b/common/image.c index 0f88984..792d371 100644 --- a/common/image.c +++ b/common/image.c @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, } #if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, const ulong *ld_start, ulong * const ld_len) { @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, int err; int devnum = 0; /* TODO support multi fpga platforms */ const fpga_desc * const desc = fpga_get_desc(devnum); - xilinx_desc *desc_xilinx = desc->devdesc; + xilinx_desc *desc_xilinx; + bitstream_type bstype; /* Check to see if the images struct has a FIT configuration */ if (!genimg_has_config(images)) { @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, return fit_img_result; } - if (img_len >= desc_xilinx->size) { + switch (desc->devtype) { >>> >>> Do we need the switch statement at all ? We can have full >>> configuration >>> as a default mode of operation and have something like >>> >>> if (xilinx) { >>> if (partial reconfiguration) { >>> do_special_setup(); >>> } >>> } >> >> I only did the switch stuff b/c i envisioned a need for partial image >> support for socfpga. > > That'd be seriously cool :) > >> >> >> That said, i would suggest, as you mention, moving >> this to platform specific code and perhaps an indication of the image >> type >> in the fitimage. > > driver-specific code . It doesn't need to know the imagetype, just that > the blob that you passed in is a partial-reconfiguration blob. I never > really worked with P/R though, do you need some other metadata for that > or is it contained in that P/R bitstream blob already ? as far as i understand it, it is all in the blob. All that is needed is knowing whether the blob is a full or partial image. X seems to just use the image size to determine this, but that means having a table of all devices and their respective full image size. seems simpler to just specify the image type is partial or not in the fitimage. >>> >>> Can't you extract that info from the RBF though ? But then again, >>> extending the fitImage format for this would be fine IMO. >>> >>> btw is spelling the X in it's entirety somehow forbidden in A ? :-) >> >> Ah, but you forget, we are now I... :) > > So it's now A+I ... 愛 [1] ? :-) > [1] http://jisho.org/search/%E6%84%9B > > Looking forward to V2 of this , we seem to have some good stuff coming up. I can only have some thoughts about it but if you work for I you should use that I email. The same in A case and I do something for X that's why I use X email. Thanks, Michal ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On 02/19/2017 10:21 PM, Dalon Westergreen wrote: > On Sun, 2017-02-19 at 22:12 +0100, Marek Vasut wrote: >> On 02/19/2017 09:58 PM, Dalon Westergreen wrote: >>> >>> On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote: On 02/19/2017 09:43 PM, Dalon Westergreen wrote: > > > On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: >> >> >> On 02/19/2017 08:49 PM, Dalon Westergreen wrote: >>> >>> >>> >>> The implementation of boot_get_fpga only supported one fpga family. >>> This modification allows for any of the fpga devices supported by >>> fpga_load to be used. >>> >>> Signed-off-by: Dalon Westergreen>> >> +CC Xilinx friends :) >> >>> >>> >>> >>> --- >>> common/image.c | 37 ++--- >>> 1 file changed, 22 insertions(+), 15 deletions(-) >>> >>> diff --git a/common/image.c b/common/image.c >>> index 0f88984..792d371 100644 >>> --- a/common/image.c >>> +++ b/common/image.c >>> @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, >>> uint8_t >>> arch, >>> } >>> >>> #if IMAGE_ENABLE_FIT >>> -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) >>> +#if defined(CONFIG_FPGA) >>> int boot_get_fpga(int argc, char * const argv[], bootm_headers_t >>> *images, >>> uint8_t arch, const ulong *ld_start, ulong * >>> const >>> ld_len) >>> { >>> @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const >>> argv[], >>> bootm_headers_t *images, >>> int err; >>> int devnum = 0; /* TODO support multi fpga platforms */ >>> const fpga_desc * const desc = fpga_get_desc(devnum); >>> - xilinx_desc *desc_xilinx = desc->devdesc; >>> + xilinx_desc *desc_xilinx; >>> + bitstream_type bstype; >>> >>> /* Check to see if the images struct has a FIT >>> configuration */ >>> if (!genimg_has_config(images)) { >>> @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const >>> argv[], >>> bootm_headers_t *images, >>> return fit_img_result; >>> } >>> >>> - if (img_len >= desc_xilinx->size) { >>> + switch (desc->devtype) { >> >> Do we need the switch statement at all ? We can have full >> configuration >> as a default mode of operation and have something like >> >> if (xilinx) { >> if (partial reconfiguration) { >> do_special_setup(); >> } >> } > > I only did the switch stuff b/c i envisioned a need for partial image > support for socfpga. That'd be seriously cool :) > > > That said, i would suggest, as you mention, moving > this to platform specific code and perhaps an indication of the image > type > in the fitimage. driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ? >>> >>> as far as i understand it, it is all in the blob. All that is needed is >>> knowing >>> whether the blob is a full or partial image. X seems to just use the image >>> size >>> to determine this, but that means having a table of all devices and their >>> respective full image size. seems simpler to just specify the image type is >>> partial or not in the fitimage. >> >> Can't you extract that info from the RBF though ? But then again, >> extending the fitImage format for this would be fine IMO. >> >> btw is spelling the X in it's entirety somehow forbidden in A ? :-) > > Ah, but you forget, we are now I... :) So it's now A+I ... 愛 [1] ? :-) [1] http://jisho.org/search/%E6%84%9B Looking forward to V2 of this , we seem to have some good stuff coming up. -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On Sun, 2017-02-19 at 22:12 +0100, Marek Vasut wrote: > On 02/19/2017 09:58 PM, Dalon Westergreen wrote: > > > > On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote: > > > > > > On 02/19/2017 09:43 PM, Dalon Westergreen wrote: > > > > > > > > > > > > On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: > > > > > > > > > > > > > > > On 02/19/2017 08:49 PM, Dalon Westergreen wrote: > > > > > > > > > > > > > > > > > > > > > > > > The implementation of boot_get_fpga only supported one fpga family. > > > > > > This modification allows for any of the fpga devices supported by > > > > > > fpga_load to be used. > > > > > > > > > > > > Signed-off-by: Dalon Westergreen> > > > > > > > > > +CC Xilinx friends :) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > common/image.c | 37 ++--- > > > > > > 1 file changed, 22 insertions(+), 15 deletions(-) > > > > > > > > > > > > diff --git a/common/image.c b/common/image.c > > > > > > index 0f88984..792d371 100644 > > > > > > --- a/common/image.c > > > > > > +++ b/common/image.c > > > > > > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, > > > > > > uint8_t > > > > > > arch, > > > > > > } > > > > > > > > > > > > #if IMAGE_ENABLE_FIT > > > > > > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) > > > > > > +#if defined(CONFIG_FPGA) > > > > > > int boot_get_fpga(int argc, char * const argv[], bootm_headers_t > > > > > > *images, > > > > > > uint8_t arch, const ulong *ld_start, ulong * > > > > > > const > > > > > > ld_len) > > > > > > { > > > > > > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const > > > > > > argv[], > > > > > > bootm_headers_t *images, > > > > > > int err; > > > > > > int devnum = 0; /* TODO support multi fpga platforms */ > > > > > > const fpga_desc * const desc = fpga_get_desc(devnum); > > > > > > - xilinx_desc *desc_xilinx = desc->devdesc; > > > > > > + xilinx_desc *desc_xilinx; > > > > > > + bitstream_type bstype; > > > > > > > > > > > > /* Check to see if the images struct has a FIT > > > > > > configuration */ > > > > > > if (!genimg_has_config(images)) { > > > > > > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const > > > > > > argv[], > > > > > > bootm_headers_t *images, > > > > > > return fit_img_result; > > > > > > } > > > > > > > > > > > > - if (img_len >= desc_xilinx->size) { > > > > > > + switch (desc->devtype) { > > > > > > > > > > Do we need the switch statement at all ? We can have full > > > > > configuration > > > > > as a default mode of operation and have something like > > > > > > > > > > if (xilinx) { > > > > > if (partial reconfiguration) { > > > > > do_special_setup(); > > > > > } > > > > > } > > > > > > > > I only did the switch stuff b/c i envisioned a need for partial image > > > > support for socfpga. > > > > > > That'd be seriously cool :) > > > > > > > > > > > > > > > That said, i would suggest, as you mention, moving > > > > this to platform specific code and perhaps an indication of the image > > > > type > > > > in the fitimage. > > > > > > driver-specific code . It doesn't need to know the imagetype, just that > > > the blob that you passed in is a partial-reconfiguration blob. I never > > > really worked with P/R though, do you need some other metadata for that > > > or is it contained in that P/R bitstream blob already ? > > > > as far as i understand it, it is all in the blob. All that is needed is > > knowing > > whether the blob is a full or partial image. X seems to just use the image > > size > > to determine this, but that means having a table of all devices and their > > respective full image size. seems simpler to just specify the image type is > > partial or not in the fitimage. > > Can't you extract that info from the RBF though ? But then again, > extending the fitImage format for this would be fine IMO. > > btw is spelling the X in it's entirety somehow forbidden in A ? :-) Ah, but you forget, we are now I... :) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > But even better would be to move this platform-dependent stuff into > > > > > drivers/fpga/ or somewhere there. This is common code, so it shouldn't > > > > > be here in the first place. > > > > > > > > My preference would be to only call fpga_load and have the platform > > > > > > s/platform/driver/ > > > > > > > > > > > > > > > specific stuff figure out what they want to do. > > > > > > Agreed > > > > > > > > > > > > > > > My next comment would be > > > > that perhaps it is best to add an fpgap type or some such in the > > > > fitimage > > > > to specify the image is a partial image rather than looking at the image > > > > size? > > > > > > H, see my question above. If the driver cannot discern it from the > > > blob, maybe a DT-alike property would
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On 02/19/2017 09:58 PM, Dalon Westergreen wrote: > On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote: >> On 02/19/2017 09:43 PM, Dalon Westergreen wrote: >>> >>> On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: On 02/19/2017 08:49 PM, Dalon Westergreen wrote: > > > The implementation of boot_get_fpga only supported one fpga family. > This modification allows for any of the fpga devices supported by > fpga_load to be used. > > Signed-off-by: Dalon Westergreen+CC Xilinx friends :) > > > --- > common/image.c | 37 ++--- > 1 file changed, 22 insertions(+), 15 deletions(-) > > diff --git a/common/image.c b/common/image.c > index 0f88984..792d371 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, > uint8_t > arch, > } > > #if IMAGE_ENABLE_FIT > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) > +#if defined(CONFIG_FPGA) > int boot_get_fpga(int argc, char * const argv[], bootm_headers_t > *images, > uint8_t arch, const ulong *ld_start, ulong * const > ld_len) > { > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], > bootm_headers_t *images, > int err; > int devnum = 0; /* TODO support multi fpga platforms */ > const fpga_desc * const desc = fpga_get_desc(devnum); > - xilinx_desc *desc_xilinx = desc->devdesc; > + xilinx_desc *desc_xilinx; > + bitstream_type bstype; > > /* Check to see if the images struct has a FIT configuration */ > if (!genimg_has_config(images)) { > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], > bootm_headers_t *images, > return fit_img_result; > } > > - if (img_len >= desc_xilinx->size) { > + switch (desc->devtype) { Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like if (xilinx) { if (partial reconfiguration) { do_special_setup(); } } >>> >>> I only did the switch stuff b/c i envisioned a need for partial image >>> support for socfpga. >> >> That'd be seriously cool :) >> >>> >>> That said, i would suggest, as you mention, moving >>> this to platform specific code and perhaps an indication of the image type >>> in the fitimage. >> >> driver-specific code . It doesn't need to know the imagetype, just that >> the blob that you passed in is a partial-reconfiguration blob. I never >> really worked with P/R though, do you need some other metadata for that >> or is it contained in that P/R bitstream blob already ? > > as far as i understand it, it is all in the blob. All that is needed is > knowing > whether the blob is a full or partial image. X seems to just use the image > size > to determine this, but that means having a table of all devices and their > respective full image size. seems simpler to just specify the image type is > partial or not in the fitimage. Can't you extract that info from the RBF though ? But then again, extending the fitImage format for this would be fine IMO. btw is spelling the X in it's entirety somehow forbidden in A ? :-) >>> But even better would be to move this platform-dependent stuff into drivers/fpga/ or somewhere there. This is common code, so it shouldn't be here in the first place. >>> >>> My preference would be to only call fpga_load and have the platform >> >> s/platform/driver/ >> >>> >>> specific stuff figure out what they want to do. >> >> Agreed >> >>> >>> My next comment would be >>> that perhaps it is best to add an fpgap type or some such in the fitimage >>> to specify the image is a partial image rather than looking at the image >>> size? >> >> H, see my question above. If the driver cannot discern it from the >> blob, maybe a DT-alike property would work. ie. the image entry would >> also have a "xlnx,partial-reconfiguration" property or somesuch . > > I dont think the property need be fpga vendor specific. Indeed. [...] -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote: > On 02/19/2017 09:43 PM, Dalon Westergreen wrote: > > > > On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: > > > > > > On 02/19/2017 08:49 PM, Dalon Westergreen wrote: > > > > > > > > > > > > The implementation of boot_get_fpga only supported one fpga family. > > > > This modification allows for any of the fpga devices supported by > > > > fpga_load to be used. > > > > > > > > Signed-off-by: Dalon Westergreen> > > > > > +CC Xilinx friends :) > > > > > > > > > > > > > > > --- > > > > common/image.c | 37 ++--- > > > > 1 file changed, 22 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/common/image.c b/common/image.c > > > > index 0f88984..792d371 100644 > > > > --- a/common/image.c > > > > +++ b/common/image.c > > > > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, > > > > uint8_t > > > > arch, > > > > } > > > > > > > > #if IMAGE_ENABLE_FIT > > > > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) > > > > +#if defined(CONFIG_FPGA) > > > > int boot_get_fpga(int argc, char * const argv[], bootm_headers_t > > > > *images, > > > > uint8_t arch, const ulong *ld_start, ulong * const > > > > ld_len) > > > > { > > > > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], > > > > bootm_headers_t *images, > > > > int err; > > > > int devnum = 0; /* TODO support multi fpga platforms */ > > > > const fpga_desc * const desc = fpga_get_desc(devnum); > > > > - xilinx_desc *desc_xilinx = desc->devdesc; > > > > + xilinx_desc *desc_xilinx; > > > > + bitstream_type bstype; > > > > > > > > /* Check to see if the images struct has a FIT configuration */ > > > > if (!genimg_has_config(images)) { > > > > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], > > > > bootm_headers_t *images, > > > > return fit_img_result; > > > > } > > > > > > > > - if (img_len >= desc_xilinx->size) { > > > > + switch (desc->devtype) { > > > > > > Do we need the switch statement at all ? We can have full configuration > > > as a default mode of operation and have something like > > > > > > if (xilinx) { > > > if (partial reconfiguration) { > > > do_special_setup(); > > > } > > > } > > > > I only did the switch stuff b/c i envisioned a need for partial image > > support for socfpga. > > That'd be seriously cool :) > > > > > That said, i would suggest, as you mention, moving > > this to platform specific code and perhaps an indication of the image type > > in the fitimage. > > driver-specific code . It doesn't need to know the imagetype, just that > the blob that you passed in is a partial-reconfiguration blob. I never > really worked with P/R though, do you need some other metadata for that > or is it contained in that P/R bitstream blob already ? as far as i understand it, it is all in the blob. All that is needed is knowing whether the blob is a full or partial image. X seems to just use the image size to determine this, but that means having a table of all devices and their respective full image size. seems simpler to just specify the image type is partial or not in the fitimage. > > > > > > > > > > > But even better would be to move this platform-dependent stuff into > > > drivers/fpga/ or somewhere there. This is common code, so it shouldn't > > > be here in the first place. > > > > My preference would be to only call fpga_load and have the platform > > s/platform/driver/ > > > > > specific stuff figure out what they want to do. > > Agreed > > > > > My next comment would be > > that perhaps it is best to add an fpgap type or some such in the fitimage > > to specify the image is a partial image rather than looking at the image > > size? > > H, see my question above. If the driver cannot discern it from the > blob, maybe a DT-alike property would work. ie. the image entry would > also have a "xlnx,partial-reconfiguration" property or somesuch . I dont think the property need be fpga vendor specific. --dalon > > > > Also a consideration is that there should be a means of specifying the fpga > > devnum somehow in the fitimage? it is plausible that a system could have > > multiple fpgas, no? > > H, yees, system with multiple FPGAs, I like that :-) Probably a > similar DT prop in the fitimage indicating where this bitstream goes > would do, like loadaddress for kernel, it'd be FPGA devid for bitstream. > > > > > --dalon > > > > > > > > > > > > > > > > > + case fpga_xilinx: > > > > + desc_xilinx = desc->devdesc; > > > > + if (img_len >= desc_xilinx->size) { > > > > + name = "full"; > > > > + bstype = BIT_FULL; > > > > +
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On 02/19/2017 09:43 PM, Dalon Westergreen wrote: > On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: >> On 02/19/2017 08:49 PM, Dalon Westergreen wrote: >>> >>> The implementation of boot_get_fpga only supported one fpga family. >>> This modification allows for any of the fpga devices supported by >>> fpga_load to be used. >>> >>> Signed-off-by: Dalon Westergreen>> >> +CC Xilinx friends :) >> >>> >>> --- >>> common/image.c | 37 ++--- >>> 1 file changed, 22 insertions(+), 15 deletions(-) >>> >>> diff --git a/common/image.c b/common/image.c >>> index 0f88984..792d371 100644 >>> --- a/common/image.c >>> +++ b/common/image.c >>> @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t >>> arch, >>> } >>> >>> #if IMAGE_ENABLE_FIT >>> -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) >>> +#if defined(CONFIG_FPGA) >>> int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, >>> uint8_t arch, const ulong *ld_start, ulong * const >>> ld_len) >>> { >>> @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], >>> bootm_headers_t *images, >>> int err; >>> int devnum = 0; /* TODO support multi fpga platforms */ >>> const fpga_desc * const desc = fpga_get_desc(devnum); >>> - xilinx_desc *desc_xilinx = desc->devdesc; >>> + xilinx_desc *desc_xilinx; >>> + bitstream_type bstype; >>> >>> /* Check to see if the images struct has a FIT configuration */ >>> if (!genimg_has_config(images)) { >>> @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], >>> bootm_headers_t *images, >>> return fit_img_result; >>> } >>> >>> - if (img_len >= desc_xilinx->size) { >>> + switch (desc->devtype) { >> >> Do we need the switch statement at all ? We can have full configuration >> as a default mode of operation and have something like >> >> if (xilinx) { >> if (partial reconfiguration) { >> do_special_setup(); >> } >> } > > I only did the switch stuff b/c i envisioned a need for partial image > support for socfpga. That'd be seriously cool :) > That said, i would suggest, as you mention, moving > this to platform specific code and perhaps an indication of the image type > in the fitimage. driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ? >> >> But even better would be to move this platform-dependent stuff into >> drivers/fpga/ or somewhere there. This is common code, so it shouldn't >> be here in the first place. > > My preference would be to only call fpga_load and have the platform s/platform/driver/ > specific stuff figure out what they want to do. Agreed > My next comment would be > that perhaps it is best to add an fpgap type or some such in the fitimage > to specify the image is a partial image rather than looking at the image > size? H, see my question above. If the driver cannot discern it from the blob, maybe a DT-alike property would work. ie. the image entry would also have a "xlnx,partial-reconfiguration" property or somesuch . > Also a consideration is that there should be a means of specifying the fpga > devnum somehow in the fitimage? it is plausible that a system could have > multiple fpgas, no? H, yees, system with multiple FPGAs, I like that :-) Probably a similar DT prop in the fitimage indicating where this bitstream goes would do, like loadaddress for kernel, it'd be FPGA devid for bitstream. > --dalon > >>> >>> + case fpga_xilinx: >>> + desc_xilinx = desc->devdesc; >>> + if (img_len >= desc_xilinx->size) { >>> + name = "full"; >>> + bstype = BIT_FULL; >>> + } else { >>> + name = "partial"; >>> + bstype = BIT_PARTIAL; >>> + } >>> + break; >>> + default: >>> name = "full"; >>> - err = fpga_loadbitstream(devnum, (char *)img_data, >>> -img_len, BIT_FULL); >>> - if (err) >>> - err = fpga_load(devnum, (const void >>> *)img_data, >>> - img_len, BIT_FULL); >>> - } else { >>> - name = "partial"; >>> - err = fpga_loadbitstream(devnum, (char *)img_data, >>> -img_len, BIT_PARTIAL); >>> - if (err) >>> - err = fpga_load(devnum, (const void >>> *)img_data, >>> - img_len, BIT_PARTIAL); >>> +
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: > On 02/19/2017 08:49 PM, Dalon Westergreen wrote: > > > > The implementation of boot_get_fpga only supported one fpga family. > > This modification allows for any of the fpga devices supported by > > fpga_load to be used. > > > > Signed-off-by: Dalon Westergreen> > +CC Xilinx friends :) > > > > > --- > > common/image.c | 37 ++--- > > 1 file changed, 22 insertions(+), 15 deletions(-) > > > > diff --git a/common/image.c b/common/image.c > > index 0f88984..792d371 100644 > > --- a/common/image.c > > +++ b/common/image.c > > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t > > arch, > > } > > > > #if IMAGE_ENABLE_FIT > > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) > > +#if defined(CONFIG_FPGA) > > int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, > > uint8_t arch, const ulong *ld_start, ulong * const > > ld_len) > > { > > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], > > bootm_headers_t *images, > > int err; > > int devnum = 0; /* TODO support multi fpga platforms */ > > const fpga_desc * const desc = fpga_get_desc(devnum); > > - xilinx_desc *desc_xilinx = desc->devdesc; > > + xilinx_desc *desc_xilinx; > > + bitstream_type bstype; > > > > /* Check to see if the images struct has a FIT configuration */ > > if (!genimg_has_config(images)) { > > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], > > bootm_headers_t *images, > > return fit_img_result; > > } > > > > - if (img_len >= desc_xilinx->size) { > > + switch (desc->devtype) { > > Do we need the switch statement at all ? We can have full configuration > as a default mode of operation and have something like > > if (xilinx) { > if (partial reconfiguration) { > do_special_setup(); > } > } I only did the switch stuff b/c i envisioned a need for partial image support for socfpga. That said, i would suggest, as you mention, moving this to platform specific code and perhaps an indication of the image type in the fitimage. > > But even better would be to move this platform-dependent stuff into > drivers/fpga/ or somewhere there. This is common code, so it shouldn't > be here in the first place. My preference would be to only call fpga_load and have the platform specific stuff figure out what they want to do. My next comment would be that perhaps it is best to add an fpgap type or some such in the fitimage to specify the image is a partial image rather than looking at the image size? Also a consideration is that there should be a means of specifying the fpga devnum somehow in the fitimage? it is plausible that a system could have multiple fpgas, no? --dalon > > > > + case fpga_xilinx: > > + desc_xilinx = desc->devdesc; > > + if (img_len >= desc_xilinx->size) { > > + name = "full"; > > + bstype = BIT_FULL; > > + } else { > > + name = "partial"; > > + bstype = BIT_PARTIAL; > > + } > > + break; > > + default: > > name = "full"; > > - err = fpga_loadbitstream(devnum, (char *)img_data, > > - img_len, BIT_FULL); > > - if (err) > > - err = fpga_load(devnum, (const void > > *)img_data, > > - img_len, BIT_FULL); > > - } else { > > - name = "partial"; > > - err = fpga_loadbitstream(devnum, (char *)img_data, > > - img_len, BIT_PARTIAL); > > - if (err) > > - err = fpga_load(devnum, (const void > > *)img_data, > > - img_len, BIT_PARTIAL); > > + bstype = BIT_FULL; > > } > > > > + err = fpga_loadbitstream(devnum, (char *)img_data, > > + img_len, bstype); > > + if (err) > > + err = fpga_load(devnum, (const void *)img_data, > > + img_len, bstype); > > + > > if (err) > > return err; > > > > > > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
On 02/19/2017 08:49 PM, Dalon Westergreen wrote: > The implementation of boot_get_fpga only supported one fpga family. > This modification allows for any of the fpga devices supported by > fpga_load to be used. > > Signed-off-by: Dalon Westergreen+CC Xilinx friends :) > --- > common/image.c | 37 ++--- > 1 file changed, 22 insertions(+), 15 deletions(-) > > diff --git a/common/image.c b/common/image.c > index 0f88984..792d371 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t > arch, > } > > #if IMAGE_ENABLE_FIT > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) > +#if defined(CONFIG_FPGA) > int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, > uint8_t arch, const ulong *ld_start, ulong * const ld_len) > { > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], > bootm_headers_t *images, > int err; > int devnum = 0; /* TODO support multi fpga platforms */ > const fpga_desc * const desc = fpga_get_desc(devnum); > - xilinx_desc *desc_xilinx = desc->devdesc; > + xilinx_desc *desc_xilinx; > + bitstream_type bstype; > > /* Check to see if the images struct has a FIT configuration */ > if (!genimg_has_config(images)) { > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], > bootm_headers_t *images, > return fit_img_result; > } > > - if (img_len >= desc_xilinx->size) { > + switch (desc->devtype) { Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like if (xilinx) { if (partial reconfiguration) { do_special_setup(); } } But even better would be to move this platform-dependent stuff into drivers/fpga/ or somewhere there. This is common code, so it shouldn't be here in the first place. > + case fpga_xilinx: > + desc_xilinx = desc->devdesc; > + if (img_len >= desc_xilinx->size) { > + name = "full"; > + bstype = BIT_FULL; > + } else { > + name = "partial"; > + bstype = BIT_PARTIAL; > + } > + break; > + default: > name = "full"; > - err = fpga_loadbitstream(devnum, (char *)img_data, > - img_len, BIT_FULL); > - if (err) > - err = fpga_load(devnum, (const void *)img_data, > - img_len, BIT_FULL); > - } else { > - name = "partial"; > - err = fpga_loadbitstream(devnum, (char *)img_data, > - img_len, BIT_PARTIAL); > - if (err) > - err = fpga_load(devnum, (const void *)img_data, > - img_len, BIT_PARTIAL); > + bstype = BIT_FULL; > } > > + err = fpga_loadbitstream(devnum, (char *)img_data, > + img_len, bstype); > + if (err) > + err = fpga_load(devnum, (const void *)img_data, > + img_len, bstype); > + > if (err) > return err; > > -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
The implementation of boot_get_fpga only supported one fpga family. This modification allows for any of the fpga devices supported by fpga_load to be used. Signed-off-by: Dalon Westergreen--- common/image.c | 37 ++--- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/common/image.c b/common/image.c index 0f88984..792d371 100644 --- a/common/image.c +++ b/common/image.c @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, } #if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, const ulong *ld_start, ulong * const ld_len) { @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, int err; int devnum = 0; /* TODO support multi fpga platforms */ const fpga_desc * const desc = fpga_get_desc(devnum); - xilinx_desc *desc_xilinx = desc->devdesc; + xilinx_desc *desc_xilinx; + bitstream_type bstype; /* Check to see if the images struct has a FIT configuration */ if (!genimg_has_config(images)) { @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, return fit_img_result; } - if (img_len >= desc_xilinx->size) { + switch (desc->devtype) { + case fpga_xilinx: + desc_xilinx = desc->devdesc; + if (img_len >= desc_xilinx->size) { + name = "full"; + bstype = BIT_FULL; + } else { + name = "partial"; + bstype = BIT_PARTIAL; + } + break; + default: name = "full"; - err = fpga_loadbitstream(devnum, (char *)img_data, -img_len, BIT_FULL); - if (err) - err = fpga_load(devnum, (const void *)img_data, - img_len, BIT_FULL); - } else { - name = "partial"; - err = fpga_loadbitstream(devnum, (char *)img_data, -img_len, BIT_PARTIAL); - if (err) - err = fpga_load(devnum, (const void *)img_data, - img_len, BIT_PARTIAL); + bstype = BIT_FULL; } + err = fpga_loadbitstream(devnum, (char *)img_data, +img_len, bstype); + if (err) + err = fpga_load(devnum, (const void *)img_data, + img_len, bstype); + if (err) return err; -- 2.7.4 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot