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

[Michal Simek <michal.simek@xilinx.com>]

On 20.2.2017 16:46, Dalon Westergreen wrote:
> On Mon, 2017-02-20 at 16:16 +0100, Michal Simek wrote:
>> On 20.2.2017 15:56, 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@gmail.com>
>>>
>>> --
>>> Changes in v3:
>>>  - Fix typos/caps in comments
>>> Changes in v2:
>>>  - Add fitimage support for fpga-devnum and fpga-partial-image
>>>  - Use above in boot_get_fpga
>>>  - for xilinx fpgas double check using image size to determine
>>>    if image is a partial image
>>> ---
>>>  common/image-fit.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  common/image.c     | 51 ++++++++++++++++++++++++++++++++-------------------
>>>  include/image.h    |  5 +++++
>>>  3 files changed, 88 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/common/image-fit.c b/common/image-fit.c
>>> index 109ecfa..eb0c633 100644
>>> --- a/common/image-fit.c
>>> +++ b/common/image-fit.c
>>> @@ -916,6 +916,57 @@ ulong fit_get_end(const void *fit)
>>>  }
>>>  
>>>  /**
>>> + * fit_image_fpga_get_devnum - get fpga devnum
>>> + * @fit: pointer to the FIT format image header
>>> + * @noffset: fpga node offset
>>> + * @devnum: pointer to an int, will hold fpga devnum
>>> + *
>>> + * fit_image_fpga_get_devnum() finds the fpga devnum for which the fpga
>>> data is
>>> + * intended.  If the property is not found, we default to 0.
>>> + *
>>> + * returns:
>>> + *     0, on devnum not found
>>> + *     value, on devnum found
>>> + */
>>> +int fit_image_fpga_get_devnum(const void *fit, int noffset, int *devnum)
>>> +{
>>> +	int len;
>>> +	int *value;
>>> +
>>> +	value = (int *)fdt_getprop(fit, noffset, FIT_FPGA_DEVNUM_PROP,
>>> &len);
>>> +	if (value == NULL || len != sizeof(int))
>>> +		*devnum = 0;
>>> +	else
>>> +		*devnum = *value;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/**
>>> + * fit_image_fpga_is_partial - is partial fpga
>>
>> bitstream.
> 
> will do.
> 
>>>
>>> + * @fit: pointer to the FIT format image header
>>> + * @noffset: fpga node offset
>>> + *
>>> + * fit_image_fpga_is_partial() checks if the fpga node sets the property
>>> + * indicating the data represents a partial fpga image.
>>> + *
>>> + * returns:
>>> + *     0, on devnum not found
>>> + *     value, on devnum found
>>> + */
>>> +int fit_image_fpga_is_partial(const void *fit, int noffset)
>>> +{
>>> +	int len;
>>> +	int *value;
>>> +
>>> +	value = (int *)fdt_getprop(fit, noffset, FIT_FPGA_PARTIAL_PROP,
>>> &len);
>>> +	if ((value == NULL || len != sizeof(int)) || (value == 0))
>>> +		return 0;
>>> +	else
>>> +		return 1;
>>> +}
>>> +
>>> +/**
>>>   * fit_set_timestamp - set node timestamp property
>>>   * @fit: pointer to the FIT format image header
>>>   * @noffset: node offset
>>> diff --git a/common/image.c b/common/image.c
>>> index 0f88984..6480b0a 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)
>>>  {
>>> @@ -1316,9 +1316,10 @@ int boot_get_fpga(int argc, char * const argv[],
>>> bootm_headers_t *images,
>>>  	int fit_img_result;
>>>  	const char *uname, *name;
>>>  	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;
>>> +	int devnum;
>>> +	const fpga_desc *desc;
>>> +	xilinx_desc *desc_xilinx;
>>> +	bitstream_type bstype = BIT_FULL;
>>>  
>>>  	/* Check to see if the images struct has a FIT configuration */
>>>  	if (!genimg_has_config(images)) {
>>> @@ -1365,26 +1366,38 @@ int boot_get_fpga(int argc, char * const argv[],
>>> bootm_headers_t *images,
>>>  			return fit_img_result;
>>>  		}
>>>  
>>> -		if (img_len >= desc_xilinx->size) {
>>> -			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);
>>> +		/* Get FPGA device number, defaults to 0 */
>>> +		fit_image_fpga_get_devnum(buf, conf_noffset, &devnum);
>>> +
>>> +		/* Check bitstream type */
>>> +		if (fit_image_fpga_is_partial(buf, conf_noffset))
>>> +			bstype = BIT_PARTIAL;
>>> +
>>> +		/* Legacy support detecting partial config files for Xilinx
>>> */
>>> +		desc = fpga_get_desc(devnum);
>>> +		if (desc->devtype == fpga_xilinx) {
>>> +			desc_xilinx = desc->devdesc;
>>> +			if (img_len < desc_xilinx->size)
>>> +				bstype = BIT_PARTIAL;
>>>  		}
>>>  
>>> +		/* Try bitstream format first */
>>> +		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;
>>>  
>>> -		printf("   Programming %s bitstream... OK\n", name);
>>> +		if (bstype == BIT_PARTIAL)
>>> +			name = "partial";
>>> +		else
>>> +			name = "full";
>>> +
>>> +		printf("   Programming %s bitstream into fpga %d... OK\n",
>>> +		       name, devnum);
>>>  		break;
>>>  	default:
>>>  		printf("The given image format is not supported
>>> (corrupt?)\n");
>>> diff --git a/include/image.h b/include/image.h
>>> index 1e686b7..75d2afc 100644
>>> --- a/include/image.h
>>> +++ b/include/image.h
>>> @@ -876,6 +876,8 @@ int bootz_setup(ulong image, ulong *start, ulong *end);
>>>  #define FIT_COMP_PROP		"compression"
>>>  #define FIT_ENTRY_PROP		"entry"
>>>  #define FIT_LOAD_PROP		"load"
>>> +#define FIT_FPGA_DEVNUM_PROP	"fpga-devnum"
>>
>> tbh I am not quite sure if we should introduce this.
>> Please look below.
>>
>>
>>>
>>> +#define FIT_FPGA_PARTIAL_PROP	"fpga-partial-image"
>>
>> I would suggest to use "partial-fpga-config" because this is what Alan
>> is pushing to mainline kernel. Make no sense to use something different.
> 
> Good point.
> 
>>>
>>>  
>>>  /* configuration node */
>>>  #define FIT_KERNEL_PROP		"kernel"
>>> @@ -955,6 +957,9 @@ int fit_image_hash_get_value(const void *fit, int
>>> noffset, uint8_t **value,
>>>  
>>>  int fit_set_timestamp(void *fit, int noffset, time_t timestamp);
>>>  
>>> +int fit_image_fpga_get_devnum(const void *fit, int noffset, int *devnum);
>>> +int fit_image_fpga_is_partial(const void *fit, int noffset);
>>> +
>>>  /**
>>>   * fit_add_verification_data() - add verification data to FIT image nodes
>>>   *
>>>
>>
>> I have not a problem with this patch. I didn't test it yet but I will do
>> it. It should probably go through my tree anyway.
>>
>> There are some things which should be good to keep in mind.
>> 1. DM - all fpga drivers are using legacy model and we should really
>> consider to move that stuff to DM and use binding for it.
>>
>> 2. overlays. DT overlays were added recently to u-boot and we should add
>> support for it. Then in general fpga commands can go away entirely and
>> we will just apply overlays directly and this will require DM.
>>
>> / {
>>         fragment {
>>                 target = <&fpga_region0>;
>>                 __overlay__ {
>>                         #address-cells = <1>;
>>                         #size-cells = <1>;
>>                         firmware-name = "XXXX";
>>                         partial-fpga-config;
>>                         gpio_on: gpio at 412c0000 {
>> ...
>>                         };
>>                 };
>>         };
>> };
>>
>> Here is visible that target replaces devnum.
> 
> I do love overlays, but am not sure if target would replace devnum.  My
> intent for devnum is to support multiple fpgas, not multiple fpga regions.
> That said, nothing says an fpga region couldnt point to fpga managers for
> the different fgpa devices...

right. fpga region has fpga-mgr property and in general you can use this
link as target instead.

> 
> I also think that even if support for overlays is implemented in uboot
> there is a case for the simpler method where the end os is not linux.
> Maybe after DM is implemented the fitimage can use target instead of
> devnum?

not sure if you have system with multiple fpgas to load. Definitely it
is easy to set it up.
If you don't have this setup I would add this patch without devnum
specified and 0 is used all the time which is common case.
And let's start to look at DM where we can get that link for others fpgas.

Thanks,
Michal