[U-Boot] [PATCH 1/2] common: image: update boot_get_fpga to support arbitrary fpga image

[Marek Vasut]

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

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