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

[Michal Simek]

On 20.2.2017 15:30, Dalon Westergreen wrote:
> On Mon, 2017-02-20 at 10:22 +0100, Michal Simek wrote:
>> On 19.2.2017 21:58, 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.
>>
>> We did that for zynq when we did that for the first time. But not for
>> zynqmp. Zynq is maybe still using it but it shouldn't now. It is not
>> 100% reliable way. Definitely having DT property is the best option
>> because you can add sort of "nop" which extend bitstream size and does
>> nothing which breaks that checking.
>>
>> For full u-boot there is loadb, loadbp, load and loadp to distinguish it.
> 
> That brings up an interesting point, right now the fpga_loadbitstream doesn't
> follow the same method as fpga_load for allowing multiple FPGA types to be
> supported simultaneously.  Would it not be prudent to move in that direction?
> I believe only xilinx implements this right now.

loadbitstream is there for a long time which is in general just header
bitstream header parser written in pretty bad way.
Right now we are adding support for encrypted and authenticated images
and for that we are adding fpga loads command where you specify by flag
if you want to pass encrypted or authenticated image and keys.

There is also loadmk which should be improved a lot exactly by what you
are trying to do. It means we should wrap bitstreams with flags which
says what it is inside.
On the other hand having separate commands it is easy for testing.

Definitely I am open to improve this subsystem to make it more flexible.

Thanks,
Michal