[U-Boot] [PATCH v3 0/2] common: fitimage support for arbitrary fpga type

[U-Boot] [PATCH v3 0/2] common: fitimage support for arbitrary fpga type

[Dalon Westergreen]

The intent of these patches is to modify existing fitimage support for
fpga configuration to allow configuration of any fpga type supported
by the fpga_load command.
 
In the fpga node, two additional optional parameters are used to indicate
the fpga device number (for systems with multiple fpgas) and whether the fpga
image is a full or partial image.
 
fpga-devnum: FPGA device number, defaults to 0
fpga-partial-image: 0 = full, other = partial, defaults to full

Changes in v3:
 - Fix typos and caps in comments 
Changes in v2:
 - Add support for devnum and partial
 - for backward compatibility, do check of image size for xilinx to determine
   if the image is a partial image

Dalon Westergreen (2):
  common: image: update boot_get_fpga to support arbitrary fpga image
  common: bootm: add support for arbitrary fgpa configuration

 common/bootm.c     |  2 +-
 common/image-fit.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 common/image.c     | 51 ++++++++++++++++++++++++++++++++-------------------
 include/image.h    |  5 +++++
 4 files changed, 89 insertions(+), 20 deletions(-)

-- 
2.7.4

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

[Dalon Westergreen]

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
+ * @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"
+#define FIT_FPGA_PARTIAL_PROP	"fpga-partial-image"
 
 /* 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
  *
-- 
2.7.4

[U-Boot] [PATCH v3 2/2] common: bootm: add support for arbitrary fgpa configuration

[Dalon Westergreen]

This adds support for fpga configuration data in fitimages for
any fpga device supported by fpga_load.  At this point fitimages
only support configuration of fpga images for fpga devnum 0.

Signed-off-by: Dalon Westergreen <dwesterg@gmail.com>
---
 common/bootm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/bootm.c b/common/bootm.c
index b2c0912..4a4b47c 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -248,7 +248,7 @@ int bootm_find_images(int flag, int argc, char * const argv[])
 #endif
 
 #if IMAGE_ENABLE_FIT
-#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX)
+#if defined(CONFIG_FPGA)
 	/* find bitstreams */
 	ret = boot_get_fpga(argc, argv, &images, IH_ARCH_DEFAULT,
 			    NULL, NULL);
-- 
2.7.4

[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@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

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

[Dalon Westergreen]

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

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?

--dalon

> 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

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

[Michal Simek]

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

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

[Simon Glass]

Hi Dalon,

On 20 February 2017 at 07:56, Dalon Westergreen <dwesterg@gmail.com> 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.

Can you add some docs somewhere to explain how this is used? E.g. you
could update something in doc/uImage.FIT/

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

Can you use fdtdec_get_int()? It handles the endian conversion automatically.

> +       if (value == NULL || len != sizeof(int))
> +               *devnum = 0;
> +       else
> +               *devnum = *value;
> +
> +       return 0;
> +}
> +
> +/**
> + * fit_image_fpga_is_partial - is partial fpga

This doesn't explain very much - can you rephrase?

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

But it seems to return 0 or 1?

> + */
> +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))

Is this boolean? Could you use fdtdec_get_bool()?

> +               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) {

Can we use 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"
> +#define FIT_FPGA_PARTIAL_PROP  "fpga-partial-image"
>
>  /* 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);

Can you put the function comments here instead of in the C file?

> +
>  /**
>   * fit_add_verification_data() - add verification data to FIT image nodes
>   *
> --
> 2.7.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Regards,
Simon

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

[Tomas Melin]

Hi Dalon,

On 02/22/2017 06:00 AM, Simon Glass wrote:
> Hi Dalon,
> 
> On 20 February 2017 at 07:56, Dalon Westergreen <dwesterg@gmail.com> 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.
> 
> Can you add some docs somewhere to explain how this is used? E.g. you
> could update something in doc/uImage.FIT/
> 
>>
>> 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.

Repeating these function names in the decriptions could IMHO be avoided. Also
please check double spacing.

>> + *
>> + * returns:
>> + *     0, on devnum not found
>> + *     value, on devnum found

This seems to always return 0. Should it instead of providing devnum as an
argument return devnum?

>> + */
>> +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);
> 
> Can you use fdtdec_get_int()? It handles the endian conversion automatically.
> 
>> +       if (value == NULL || len != sizeof(int))
>> +               *devnum = 0;
>> +       else
>> +               *devnum = *value;
>> +
>> +       return 0;
>> +}
>> +
>> +/**
>> + * fit_image_fpga_is_partial - is partial fpga
> 
> This doesn't explain very much - can you rephrase?
> 
>> + * @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
> 
> But it seems to return 0 or 1?
> 
>> + */
>> +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))
> 
> Is this boolean? Could you use fdtdec_get_bool()?
> 
>> +               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) {
> 
> Can we use 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);

At this point the bitstream is already programmed. Should the above print
reflect this? Also, why the extra spacing?

BR,
Tomas


>>                 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"
>> +#define FIT_FPGA_PARTIAL_PROP  "fpga-partial-image"
>>
>>  /* 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);
> 
> Can you put the function comments here instead of in the C file?
> 
>> +
>>  /**
>>   * fit_add_verification_data() - add verification data to FIT image nodes
>>   *
>> --
>> 2.7.4
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
> 
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

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

[Dalon Westergreen]

On Tue, 2017-02-21 at 21:00 -0700, Simon Glass wrote:
> Hi Dalon,
> 
> On 20 February 2017 at 07:56, Dalon Westergreen <dwesterg@gmail.com> 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.
> 
> Can you add some docs somewhere to explain how this is used? E.g. you
> could update something in doc/uImage.FIT/

sure thing.?

> > 
> > 
> > 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);
> 
> Can you use fdtdec_get_int()? It handles the endian conversion automatically.
> 
> > 
> > +???????if (value == NULL || len != sizeof(int))
> > +???????????????*devnum = 0;
> > +???????else
> > +???????????????*devnum = *value;
> > +
> > +???????return 0;
> > +}
> > +
> > +/**
> > + * fit_image_fpga_is_partial - is partial fpga
> 
> This doesn't explain very much - can you rephrase?

Yes, i will elaboate
> 
> > 
> > + * @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
> 
> But it seems to return 0 or 1?

Bad cut and paste... i will fix this.

> > 
> > + */
> > +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))
> 
> Is this boolean? Could you use fdtdec_get_bool()?
> 
> > 
> > +???????????????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) {
> 
> Can we use FPGA_XILINX?

This enum type is defined in fpga.h and is lower case there as well.

> > 
> > +???????????????????????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"
> > +#define FIT_FPGA_PARTIAL_PROP??"fpga-partial-image"
> > 
> > ?/* 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);
> 
> Can you put the function comments here instead of in the C file?

sure thing.

Thanks for the feedback.

--dalon

> > 
> > +
> > ?/**
> > ? * fit_add_verification_data() - add verification data to FIT image nodes
> > ? *
> > --
> > 2.7.4
> > 
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> 
> Regards,
> Simon

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

[Dalon Westergreen]

On Wed, 2017-02-22 at 14:08 +0200, Tomas Melin wrote:
> Hi Dalon,
> 
> On 02/22/2017 06:00 AM, Simon Glass wrote:
> > 
> > Hi Dalon,
> > 
> > On 20 February 2017 at 07:56, Dalon Westergreen <dwesterg@gmail.com> 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.
> > 
> > Can you add some docs somewhere to explain how this is used? E.g. you
> > could update something in doc/uImage.FIT/
> > 
> > > 
> > > 
> > > 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.
> 
> Repeating these function names in the decriptions could IMHO be avoided. Also
> please check double spacing.

i can remove that, i just followed what seemed like the convention in the
functions surrounding this one.

> > 
> > > 
> > > + *
> > > + * returns:
> > > + *?????0, on devnum not found
> > > + *?????value, on devnum found
> 
> This seems to always return 0. Should it instead of providing devnum as an
> argument return devnum?

Yes, easy enough.?

> > 
> > > 
> > > + */
> > > +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);
> > 
> > Can you use fdtdec_get_int()? It handles the endian conversion
> > automatically.
> > 
> > > 
> > > +???????if (value == NULL || len != sizeof(int))
> > > +???????????????*devnum = 0;
> > > +???????else
> > > +???????????????*devnum = *value;
> > > +
> > > +???????return 0;
> > > +}
> > > +
> > > +/**
> > > + * fit_image_fpga_is_partial - is partial fpga
> > 
> > This doesn't explain very much - can you rephrase?
> > 
> > > 
> > > + * @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
> > 
> > But it seems to return 0 or 1?
> > 
> > > 
> > > + */
> > > +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))
> > 
> > Is this boolean? Could you use fdtdec_get_bool()?
> > 
> > > 
> > > +???????????????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) {
> > 
> > Can we use 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);
> 
> At this point the bitstream is already programmed. Should the above print
> reflect this? Also, why the extra spacing?

I agree, frankly i just left this how it was originally. ?I will reorder this.

Thanks,
Dalon

> BR,
> Tomas
> 
> 
> > 
> > > 
> > > ????????????????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"
> > > +#define FIT_FPGA_PARTIAL_PROP??"fpga-partial-image"
> > > 
> > > ?/* 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);
> > 
> > Can you put the function comments here instead of in the C file?
> > 
> > > 
> > > +
> > > ?/**
> > > ? * fit_add_verification_data() - add verification data to FIT image nodes
> > > ? *
> > > --
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > U-Boot mailing list
> > > U-Boot at lists.denx.de
> > > http://lists.denx.de/mailman/listinfo/u-boot
> > 
> > Regards,
> > Simon
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> > 

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

[Dalon Westergreen]

On Tue, 2017-02-21 at 21:00 -0700, Simon Glass wrote:
> Hi Dalon,
> 
> On 20 February 2017 at 07:56, Dalon Westergreen <dwesterg@gmail.com> 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.
> 
[snip ...]
> > 
> > ?/* 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);
> 
> Can you put the function comments here instead of in the C file?

Doing this would be counter to every other function in the c file these
are added to, so i think it best to leave as is.

--dalon

> > 
> > +
> > ?/**
> > ? * fit_add_verification_data() - add verification data to FIT image nodes
> > ? *
> > --
> > 2.7.4
> > 
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> 
> Regards,
> Simon