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

[Michal Simek]

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

> + * @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.


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

3. maybe before overlays we can add support for simple cases as I
supported now.

Definitely there is a lot of work which need to be done.

Thanks,
Michal