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 <dwest...@gmail.com> > > > > > > +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? > > Hmmmm, 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? > > Hmmmm, yeeeeees, 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); > > > > + 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