From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dalon Westergreen Date: Sun, 19 Feb 2017 12:58:44 -0800 Subject: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image In-Reply-To: References: <1487533799-28788-1-git-send-email-dwesterg@gmail.com> <1487533799-28788-2-git-send-email-dwesterg@gmail.com> <78c71e1d-999a-d9da-0326-c34e7caae3d3@denx.de> <1487537033.13957.12.camel@gmail.com> Message-ID: <1487537924.13957.20.camel@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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? > > 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; > > > > ? > > > > > > > > > > > >