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 ?

>>
>> 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 .

> 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;
>>>  
>>>
>>
>>


-- 
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to