From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Sun, 19 Feb 2017 22:12:28 +0100 Subject: [U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image In-Reply-To: <1487537924.13957.20.camel@gmail.com> 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> <1487537924.13957.20.camel@gmail.com> Message-ID: <3ce21661-ab01-ab00-ce01-3e9c3d5f57b8@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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? >> >> 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. Indeed. [...] -- Best regards, Marek Vasut