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

[U-Boot] [PATCH 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.  Note though, that fitimage support for fpga
configuration only allows for configuration of the fpga of devnum 0.

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.c | 37 ++++++++++++++++++++++---------------
 2 files changed, 23 insertions(+), 16 deletions(-)

-- 
2.7.4

[U-Boot] [PATCH 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>
---
 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) {
+		case fpga_xilinx:
+			desc_xilinx = desc->devdesc;
+			if (img_len >= desc_xilinx->size) {
+				name = "full";
+				bstype = BIT_FULL;
+			} else {
+				name = "partial";
+				bstype = BIT_PARTIAL;
+			}
+			break;
+		default:
 			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);
+			bstype = BIT_FULL;
 		}
 
+		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;
 
-- 
2.7.4

[U-Boot] [PATCH 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 1/2] common: image: update boot_get_fpga to support arbitrary fpga image

[Marek Vasut]

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@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();
 }
}

But even better would be to move this platform-dependent stuff into
drivers/fpga/ or somewhere there. This is common code, so it shouldn't
be here in the first place.

> +		case fpga_xilinx:
> +			desc_xilinx = desc->devdesc;
> +			if (img_len >= desc_xilinx->size) {
> +				name = "full";
> +				bstype = BIT_FULL;
> +			} else {
> +				name = "partial";
> +				bstype = BIT_PARTIAL;
> +			}
> +			break;
> +		default:
>  			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);
> +			bstype = BIT_FULL;
>  		}
>  
> +		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;
>  
> 


-- 
Best regards,
Marek Vasut

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

[Dalon Westergreen]

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@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 said, i would suggest, as you mention, moving
this to platform specific code and perhaps an indication of the image type
in the fitimage.

> 
> But even better would be to move this platform-dependent stuff into
> drivers/fpga/ or somewhere there. This is common code, so it shouldn't
> be here in the first place.

My preference would be to only call fpga_load and have the platform
specific stuff figure out what they want to do. My next comment would be
that perhaps it is best to add an fpgap type or some such in the fitimage
to specify the image is a partial image rather than looking at the image
size?

Also a consideration is that there should be a means of specifying the fpga
devnum somehow in the fitimage? ?it is plausible that a system could have
multiple fpgas, no?

--dalon

> > 
> > +		case fpga_xilinx:
> > +			desc_xilinx = desc->devdesc;
> > +			if (img_len >= desc_xilinx->size) {
> > +				name = "full";
> > +				bstype = BIT_FULL;
> > +			} else {
> > +				name = "partial";
> > +				bstype = BIT_PARTIAL;
> > +			}
> > +			break;
> > +		default:
> > ?			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);
> > +			bstype = BIT_FULL;
> > ?		}
> > ?
> > +		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;
> > ?
> > 
> 
> 

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

[Marek Vasut]

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

>>
>> But even better would be to move this platform-dependent stuff into
>> drivers/fpga/ or somewhere there. This is common code, so it shouldn't
>> be here in the first place.
> 
> My preference would be to only call fpga_load and have the platform

s/platform/driver/

> specific stuff figure out what they want to do.

Agreed

> My next comment would be
> that perhaps it is best to add an fpgap type or some such in the fitimage
> to specify the image is a partial image rather than looking at the image
> size?

Hmmmm, see my question above. If the driver cannot discern it from the
blob, maybe a DT-alike property would work. ie. the image entry would
also have a "xlnx,partial-reconfiguration" property or somesuch .

> Also a consideration is that there should be a means of specifying the fpga
> devnum somehow in the fitimage?  it is plausible that a system could have
> multiple fpgas, no?

Hmmmm, yeeeeees, system with multiple FPGAs, I like that :-) Probably a
similar DT prop in the fitimage indicating where this bitstream goes
would do, like loadaddress for kernel, it'd be FPGA devid for bitstream.

> --dalon
> 
>>>
>>> +		case fpga_xilinx:
>>> +			desc_xilinx = desc->devdesc;
>>> +			if (img_len >= desc_xilinx->size) {
>>> +				name = "full";
>>> +				bstype = BIT_FULL;
>>> +			} else {
>>> +				name = "partial";
>>> +				bstype = BIT_PARTIAL;
>>> +			}
>>> +			break;
>>> +		default:
>>>  			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);
>>> +			bstype = BIT_FULL;
>>>  		}
>>>  
>>> +		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;
>>>  
>>>
>>
>>


-- 
Best regards,
Marek Vasut

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

[Dalon Westergreen]

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

> > 
> > > 
> > > 
> > > But even better would be to move this platform-dependent stuff into
> > > drivers/fpga/ or somewhere there. This is common code, so it shouldn't
> > > be here in the first place.
> > 
> > My preference would be to only call fpga_load and have the platform
> 
> s/platform/driver/
> 
> > 
> > specific stuff figure out what they want to do.
> 
> Agreed
> 
> > 
> > My next comment would be
> > that perhaps it is best to add an fpgap type or some such in the fitimage
> > to specify the image is a partial image rather than looking at the image
> > size?
> 
> Hmmmm, see my question above. If the driver cannot discern it from the
> blob, maybe a DT-alike property would work. ie. the image entry would
> also have a "xlnx,partial-reconfiguration" property or somesuch .

I dont think the property need be fpga vendor specific.?

--dalon

> > 
> > Also a consideration is that there should be a means of specifying the fpga
> > devnum somehow in the fitimage???it is plausible that a system could have
> > multiple fpgas, no?
> 
> Hmmmm, yeeeeees, system with multiple FPGAs, I like that :-) Probably a
> similar DT prop in the fitimage indicating where this bitstream goes
> would do, like loadaddress for kernel, it'd be FPGA devid for bitstream.
> 
> > 
> > --dalon
> > 
> > > 
> > > > 
> > > > 
> > > > +		case fpga_xilinx:
> > > > +			desc_xilinx = desc->devdesc;
> > > > +			if (img_len >= desc_xilinx->size) {
> > > > +				name = "full";
> > > > +				bstype = BIT_FULL;
> > > > +			} else {
> > > > +				name = "partial";
> > > > +				bstype = BIT_PARTIAL;
> > > > +			}
> > > > +			break;
> > > > +		default:
> > > > ?			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);
> > > > +			bstype = BIT_FULL;
> > > > ?		}
> > > > ?
> > > > +		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;
> > > > ?
> > > > 
> > > 
> > > 
> 
> 

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

[Marek Vasut]

On 02/19/2017 09:58 PM, Dalon Westergreen wrote:
> On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
>> On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
>>>
>>> On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
>>>>
>>>> On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
>>>>>
>>>>>
>>>>> The implementation of boot_get_fpga only supported one fpga family.
>>>>> This modification allows for any of the fpga devices supported by
>>>>> fpga_load to be used.
>>>>>
>>>>> Signed-off-by: Dalon Westergreen <dwesterg@gmail.com>
>>>>
>>>> +CC Xilinx friends :)
>>>>
>>>>>
>>>>>
>>>>> ---
>>>>>  common/image.c | 37 ++++++++++++++++++++++---------------
>>>>>  1 file changed, 22 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/common/image.c b/common/image.c
>>>>> index 0f88984..792d371 100644
>>>>> --- a/common/image.c
>>>>> +++ b/common/image.c
>>>>> @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images,
>>>>> uint8_t
>>>>> arch,
>>>>>  }
>>>>>  
>>>>>  #if IMAGE_ENABLE_FIT
>>>>> -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX)
>>>>> +#if defined(CONFIG_FPGA)
>>>>>  int boot_get_fpga(int argc, char * const argv[], bootm_headers_t
>>>>> *images,
>>>>>  		  uint8_t arch, const ulong *ld_start, ulong * const
>>>>> ld_len)
>>>>>  {
>>>>> @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[],
>>>>> bootm_headers_t *images,
>>>>>  	int err;
>>>>>  	int devnum = 0; /* TODO support multi fpga platforms */
>>>>>  	const fpga_desc * const desc = fpga_get_desc(devnum);
>>>>> -	xilinx_desc *desc_xilinx = desc->devdesc;
>>>>> +	xilinx_desc *desc_xilinx;
>>>>> +	bitstream_type bstype;
>>>>>  
>>>>>  	/* Check to see if the images struct has a FIT configuration */
>>>>>  	if (!genimg_has_config(images)) {
>>>>> @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[],
>>>>> bootm_headers_t *images,
>>>>>  			return fit_img_result;
>>>>>  		}
>>>>>  
>>>>> -		if (img_len >= desc_xilinx->size) {
>>>>> +		switch (desc->devtype) {
>>>>
>>>> Do we need the switch statement at all ? We can have full configuration
>>>> as a default mode of operation and have something like
>>>>
>>>> if (xilinx) {
>>>>  if (partial reconfiguration) {
>>>>   do_special_setup();
>>>>  }
>>>> }
>>>
>>> I only did the switch stuff b/c i envisioned a need for partial image
>>> support for socfpga.
>>
>> That'd be seriously cool :)
>>
>>>
>>> That said, i would suggest, as you mention, moving
>>> this to platform specific code and perhaps an indication of the image type
>>> in the fitimage.
>>
>> driver-specific code . It doesn't need to know the imagetype, just that
>> the blob that you passed in is a partial-reconfiguration blob. I never
>> really worked with P/R though, do you need some other metadata for that
>> or is it contained in that P/R bitstream blob already ?
> 
> as far as i understand it, it is all in the blob.  All that is needed is knowing
> whether the blob is a full or partial image.  X seems to just use the image size
> to determine this, but that means having a table of all devices and their
> respective full image size.  seems simpler to just specify the image type is
> partial or not in the fitimage.

Can't you extract that info from the RBF though ? But then again,
extending the fitImage format for this would be fine IMO.

btw is spelling the X in it's entirety somehow forbidden in A ? :-)

>>>
>>>>
>>>>
>>>> But even better would be to move this platform-dependent stuff into
>>>> drivers/fpga/ or somewhere there. This is common code, so it shouldn't
>>>> be here in the first place.
>>>
>>> My preference would be to only call fpga_load and have the platform
>>
>> s/platform/driver/
>>
>>>
>>> specific stuff figure out what they want to do.
>>
>> Agreed
>>
>>>
>>> My next comment would be
>>> that perhaps it is best to add an fpgap type or some such in the fitimage
>>> to specify the image is a partial image rather than looking at the image
>>> size?
>>
>> Hmmmm, see my question above. If the driver cannot discern it from the
>> blob, maybe a DT-alike property would work. ie. the image entry would
>> also have a "xlnx,partial-reconfiguration" property or somesuch .
> 
> I dont think the property need be fpga vendor specific. 

Indeed.

[...]

-- 
Best regards,
Marek Vasut

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

[Dalon Westergreen]

On Sun, 2017-02-19 at 22:12 +0100, Marek Vasut wrote:
> On 02/19/2017 09:58 PM, Dalon Westergreen wrote:
> > 
> > On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
> > > 
> > > On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
> > > > 
> > > > 
> > > > On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > The implementation of boot_get_fpga only supported one fpga family.
> > > > > > This modification allows for any of the fpga devices supported by
> > > > > > fpga_load to be used.
> > > > > > 
> > > > > > Signed-off-by: Dalon Westergreen <dwesterg@gmail.com>
> > > > > 
> > > > > +CC Xilinx friends :)
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > ---
> > > > > > ?common/image.c | 37 ++++++++++++++++++++++---------------
> > > > > > ?1 file changed, 22 insertions(+), 15 deletions(-)
> > > > > > 
> > > > > > diff --git a/common/image.c b/common/image.c
> > > > > > index 0f88984..792d371 100644
> > > > > > --- a/common/image.c
> > > > > > +++ b/common/image.c
> > > > > > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images,
> > > > > > uint8_t
> > > > > > arch,
> > > > > > ?}
> > > > > > ?
> > > > > > ?#if IMAGE_ENABLE_FIT
> > > > > > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX)
> > > > > > +#if defined(CONFIG_FPGA)
> > > > > > ?int boot_get_fpga(int argc, char * const argv[], bootm_headers_t
> > > > > > *images,
> > > > > > ?		??uint8_t arch, const ulong *ld_start, ulong *
> > > > > > const
> > > > > > ld_len)
> > > > > > ?{
> > > > > > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const
> > > > > > argv[],
> > > > > > bootm_headers_t *images,
> > > > > > ?	int err;
> > > > > > ?	int devnum = 0; /* TODO support multi fpga platforms */
> > > > > > ?	const fpga_desc * const desc = fpga_get_desc(devnum);
> > > > > > -	xilinx_desc *desc_xilinx = desc->devdesc;
> > > > > > +	xilinx_desc *desc_xilinx;
> > > > > > +	bitstream_type bstype;
> > > > > > ?
> > > > > > ?	/* Check to see if the images struct has a FIT
> > > > > > configuration */
> > > > > > ?	if (!genimg_has_config(images)) {
> > > > > > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const
> > > > > > argv[],
> > > > > > bootm_headers_t *images,
> > > > > > ?			return fit_img_result;
> > > > > > ?		}
> > > > > > ?
> > > > > > -		if (img_len >= desc_xilinx->size) {
> > > > > > +		switch (desc->devtype) {
> > > > > 
> > > > > Do we need the switch statement at all ? We can have full
> > > > > configuration
> > > > > as a default mode of operation and have something like
> > > > > 
> > > > > if (xilinx) {
> > > > > ?if (partial reconfiguration) {
> > > > > ? do_special_setup();
> > > > > ?}
> > > > > }
> > > > 
> > > > I only did the switch stuff b/c i envisioned a need for partial image
> > > > support for socfpga.
> > > 
> > > That'd be seriously cool :)
> > > 
> > > > 
> > > > 
> > > > That said, i would suggest, as you mention, moving
> > > > this to platform specific code and perhaps an indication of the image
> > > > type
> > > > in the fitimage.
> > > 
> > > driver-specific code . It doesn't need to know the imagetype, just that
> > > the blob that you passed in is a partial-reconfiguration blob. I never
> > > really worked with P/R though, do you need some other metadata for that
> > > or is it contained in that P/R bitstream blob already ?
> > 
> > as far as i understand it, it is all in the blob.??All that is needed is
> > knowing
> > whether the blob is a full or partial image.??X seems to just use the image
> > size
> > to determine this, but that means having a table of all devices and their
> > respective full image size.??seems simpler to just specify the image type is
> > partial or not in the fitimage.
> 
> Can't you extract that info from the RBF though ? But then again,
> extending the fitImage format for this would be fine IMO.
> 
> btw is spelling the X in it's entirety somehow forbidden in A ? :-)

Ah, but you forget, we are now I... :)

> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > But even better would be to move this platform-dependent stuff into
> > > > > drivers/fpga/ or somewhere there. This is common code, so it shouldn't
> > > > > be here in the first place.
> > > > 
> > > > My preference would be to only call fpga_load and have the platform
> > > 
> > > s/platform/driver/
> > > 
> > > > 
> > > > 
> > > > specific stuff figure out what they want to do.
> > > 
> > > Agreed
> > > 
> > > > 
> > > > 
> > > > My next comment would be
> > > > that perhaps it is best to add an fpgap type or some such in the
> > > > fitimage
> > > > to specify the image is a partial image rather than looking at the image
> > > > size?
> > > 
> > > Hmmmm, see my question above. If the driver cannot discern it from the
> > > blob, maybe a DT-alike property would work. ie. the image entry would
> > > also have a "xlnx,partial-reconfiguration" property or somesuch .
> > 
> > I dont think the property need be fpga vendor specific.?
> 
> Indeed.
> 
> [...]
> 

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

[Marek Vasut]

On 02/19/2017 10:21 PM, Dalon Westergreen wrote:
> On Sun, 2017-02-19 at 22:12 +0100, Marek Vasut wrote:
>> On 02/19/2017 09:58 PM, Dalon Westergreen wrote:
>>>
>>> On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
>>>>
>>>> On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
>>>>>
>>>>>
>>>>> On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The implementation of boot_get_fpga only supported one fpga family.
>>>>>>> This modification allows for any of the fpga devices supported by
>>>>>>> fpga_load to be used.
>>>>>>>
>>>>>>> Signed-off-by: Dalon Westergreen <dwesterg@gmail.com>
>>>>>>
>>>>>> +CC Xilinx friends :)
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ---
>>>>>>>  common/image.c | 37 ++++++++++++++++++++++---------------
>>>>>>>  1 file changed, 22 insertions(+), 15 deletions(-)
>>>>>>>
>>>>>>> diff --git a/common/image.c b/common/image.c
>>>>>>> index 0f88984..792d371 100644
>>>>>>> --- a/common/image.c
>>>>>>> +++ b/common/image.c
>>>>>>> @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images,
>>>>>>> uint8_t
>>>>>>> arch,
>>>>>>>  }
>>>>>>>  
>>>>>>>  #if IMAGE_ENABLE_FIT
>>>>>>> -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX)
>>>>>>> +#if defined(CONFIG_FPGA)
>>>>>>>  int boot_get_fpga(int argc, char * const argv[], bootm_headers_t
>>>>>>> *images,
>>>>>>>  		  uint8_t arch, const ulong *ld_start, ulong *
>>>>>>> const
>>>>>>> ld_len)
>>>>>>>  {
>>>>>>> @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const
>>>>>>> argv[],
>>>>>>> bootm_headers_t *images,
>>>>>>>  	int err;
>>>>>>>  	int devnum = 0; /* TODO support multi fpga platforms */
>>>>>>>  	const fpga_desc * const desc = fpga_get_desc(devnum);
>>>>>>> -	xilinx_desc *desc_xilinx = desc->devdesc;
>>>>>>> +	xilinx_desc *desc_xilinx;
>>>>>>> +	bitstream_type bstype;
>>>>>>>  
>>>>>>>  	/* Check to see if the images struct has a FIT
>>>>>>> configuration */
>>>>>>>  	if (!genimg_has_config(images)) {
>>>>>>> @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const
>>>>>>> argv[],
>>>>>>> bootm_headers_t *images,
>>>>>>>  			return fit_img_result;
>>>>>>>  		}
>>>>>>>  
>>>>>>> -		if (img_len >= desc_xilinx->size) {
>>>>>>> +		switch (desc->devtype) {
>>>>>>
>>>>>> Do we need the switch statement at all ? We can have full
>>>>>> configuration
>>>>>> as a default mode of operation and have something like
>>>>>>
>>>>>> if (xilinx) {
>>>>>>  if (partial reconfiguration) {
>>>>>>   do_special_setup();
>>>>>>  }
>>>>>> }
>>>>>
>>>>> I only did the switch stuff b/c i envisioned a need for partial image
>>>>> support for socfpga.
>>>>
>>>> That'd be seriously cool :)
>>>>
>>>>>
>>>>>
>>>>> That said, i would suggest, as you mention, moving
>>>>> this to platform specific code and perhaps an indication of the image
>>>>> type
>>>>> in the fitimage.
>>>>
>>>> driver-specific code . It doesn't need to know the imagetype, just that
>>>> the blob that you passed in is a partial-reconfiguration blob. I never
>>>> really worked with P/R though, do you need some other metadata for that
>>>> or is it contained in that P/R bitstream blob already ?
>>>
>>> as far as i understand it, it is all in the blob.  All that is needed is
>>> knowing
>>> whether the blob is a full or partial image.  X seems to just use the image
>>> size
>>> to determine this, but that means having a table of all devices and their
>>> respective full image size.  seems simpler to just specify the image type is
>>> partial or not in the fitimage.
>>
>> Can't you extract that info from the RBF though ? But then again,
>> extending the fitImage format for this would be fine IMO.
>>
>> btw is spelling the X in it's entirety somehow forbidden in A ? :-)
> 
> Ah, but you forget, we are now I... :)

So it's now A+I ... ? [1] ? :-)
[1] http://jisho.org/search/%E6%84%9B

Looking forward to V2 of this , we seem to have some good stuff coming up.

-- 
Best regards,
Marek Vasut

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

[Michal Simek]

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

Thanks,
Michal

Thanks,
Michal

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

[Michal Simek]

On 19.2.2017 22:26, Marek Vasut wrote:
> On 02/19/2017 10:21 PM, Dalon Westergreen wrote:
>> On Sun, 2017-02-19 at 22:12 +0100, Marek Vasut wrote:
>>> On 02/19/2017 09:58 PM, Dalon Westergreen wrote:
>>>>
>>>> On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
>>>>>
>>>>> On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
>>>>>>
>>>>>>
>>>>>> On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> The implementation of boot_get_fpga only supported one fpga family.
>>>>>>>> This modification allows for any of the fpga devices supported by
>>>>>>>> fpga_load to be used.
>>>>>>>>
>>>>>>>> Signed-off-by: Dalon Westergreen <dwesterg@gmail.com>
>>>>>>>
>>>>>>> +CC Xilinx friends :)
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>  common/image.c | 37 ++++++++++++++++++++++---------------
>>>>>>>>  1 file changed, 22 insertions(+), 15 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/common/image.c b/common/image.c
>>>>>>>> index 0f88984..792d371 100644
>>>>>>>> --- a/common/image.c
>>>>>>>> +++ b/common/image.c
>>>>>>>> @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images,
>>>>>>>> uint8_t
>>>>>>>> arch,
>>>>>>>>  }
>>>>>>>>  
>>>>>>>>  #if IMAGE_ENABLE_FIT
>>>>>>>> -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX)
>>>>>>>> +#if defined(CONFIG_FPGA)
>>>>>>>>  int boot_get_fpga(int argc, char * const argv[], bootm_headers_t
>>>>>>>> *images,
>>>>>>>>  		  uint8_t arch, const ulong *ld_start, ulong *
>>>>>>>> const
>>>>>>>> ld_len)
>>>>>>>>  {
>>>>>>>> @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const
>>>>>>>> argv[],
>>>>>>>> bootm_headers_t *images,
>>>>>>>>  	int err;
>>>>>>>>  	int devnum = 0; /* TODO support multi fpga platforms */
>>>>>>>>  	const fpga_desc * const desc = fpga_get_desc(devnum);
>>>>>>>> -	xilinx_desc *desc_xilinx = desc->devdesc;
>>>>>>>> +	xilinx_desc *desc_xilinx;
>>>>>>>> +	bitstream_type bstype;
>>>>>>>>  
>>>>>>>>  	/* Check to see if the images struct has a FIT
>>>>>>>> configuration */
>>>>>>>>  	if (!genimg_has_config(images)) {
>>>>>>>> @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const
>>>>>>>> argv[],
>>>>>>>> bootm_headers_t *images,
>>>>>>>>  			return fit_img_result;
>>>>>>>>  		}
>>>>>>>>  
>>>>>>>> -		if (img_len >= desc_xilinx->size) {
>>>>>>>> +		switch (desc->devtype) {
>>>>>>>
>>>>>>> Do we need the switch statement at all ? We can have full
>>>>>>> configuration
>>>>>>> as a default mode of operation and have something like
>>>>>>>
>>>>>>> if (xilinx) {
>>>>>>>  if (partial reconfiguration) {
>>>>>>>   do_special_setup();
>>>>>>>  }
>>>>>>> }
>>>>>>
>>>>>> I only did the switch stuff b/c i envisioned a need for partial image
>>>>>> support for socfpga.
>>>>>
>>>>> That'd be seriously cool :)
>>>>>
>>>>>>
>>>>>>
>>>>>> That said, i would suggest, as you mention, moving
>>>>>> this to platform specific code and perhaps an indication of the image
>>>>>> type
>>>>>> in the fitimage.
>>>>>
>>>>> driver-specific code . It doesn't need to know the imagetype, just that
>>>>> the blob that you passed in is a partial-reconfiguration blob. I never
>>>>> really worked with P/R though, do you need some other metadata for that
>>>>> or is it contained in that P/R bitstream blob already ?
>>>>
>>>> as far as i understand it, it is all in the blob.  All that is needed is
>>>> knowing
>>>> whether the blob is a full or partial image.  X seems to just use the image
>>>> size
>>>> to determine this, but that means having a table of all devices and their
>>>> respective full image size.  seems simpler to just specify the image type is
>>>> partial or not in the fitimage.
>>>
>>> Can't you extract that info from the RBF though ? But then again,
>>> extending the fitImage format for this would be fine IMO.
>>>
>>> btw is spelling the X in it's entirety somehow forbidden in A ? :-)
>>
>> Ah, but you forget, we are now I... :)
> 
> So it's now A+I ... ? [1] ? :-)
> [1] http://jisho.org/search/%E6%84%9B
> 
> Looking forward to V2 of this , we seem to have some good stuff coming up.

I can only have some thoughts about it but if you work for I you should
use that I email. The same in A case and I do something for X that's why
I use X email.

Thanks,
Michal

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

[Dalon Westergreen]

On Mon, 2017-02-20 at 10:24 +0100, Michal Simek wrote:
> On 19.2.2017 22:26, Marek Vasut wrote:
> > 
> > On 02/19/2017 10:21 PM, Dalon Westergreen wrote:
> > > 
> > > On Sun, 2017-02-19 at 22:12 +0100, Marek Vasut wrote:
> > > > 
> > > > On 02/19/2017 09:58 PM, Dalon Westergreen wrote:
> > > > > 
> > > > > 
> > > > > On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
> > > > > > 
> > > > > > 
> > > > > > On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > The implementation of boot_get_fpga only supported one fpga
> > > > > > > > > family.
> > > > > > > > > This modification allows for any of the fpga devices supported
> > > > > > > > > by
> > > > > > > > > fpga_load to be used.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Dalon Westergreen <dwesterg@gmail.com>
> > > > > > > > 
> > > > > > > > +CC Xilinx friends :)
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > ---
> > > > > > > > > ?common/image.c | 37 ++++++++++++++++++++++---------------
> > > > > > > > > ?1 file changed, 22 insertions(+), 15 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/common/image.c b/common/image.c
> > > > > > > > > index 0f88984..792d371 100644
> > > > > > > > > --- a/common/image.c
> > > > > > > > > +++ b/common/image.c
> > > > > > > > > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t
> > > > > > > > > *images,
> > > > > > > > > uint8_t
> > > > > > > > > arch,
> > > > > > > > > ?}
> > > > > > > > > ?
> > > > > > > > > ?#if IMAGE_ENABLE_FIT
> > > > > > > > > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX)
> > > > > > > > > +#if defined(CONFIG_FPGA)
> > > > > > > > > ?int boot_get_fpga(int argc, char * const argv[],
> > > > > > > > > bootm_headers_t
> > > > > > > > > *images,
> > > > > > > > > ?		??uint8_t arch, const ulong *ld_start, ulong
> > > > > > > > > *
> > > > > > > > > const
> > > > > > > > > ld_len)
> > > > > > > > > ?{
> > > > > > > > > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const
> > > > > > > > > argv[],
> > > > > > > > > bootm_headers_t *images,
> > > > > > > > > ?	int err;
> > > > > > > > > ?	int devnum = 0; /* TODO support multi fpga platforms
> > > > > > > > > */
> > > > > > > > > ?	const fpga_desc * const desc = fpga_get_desc(devnum);
> > > > > > > > > -	xilinx_desc *desc_xilinx = desc->devdesc;
> > > > > > > > > +	xilinx_desc *desc_xilinx;
> > > > > > > > > +	bitstream_type bstype;
> > > > > > > > > ?
> > > > > > > > > ?	/* Check to see if the images struct has a FIT
> > > > > > > > > configuration */
> > > > > > > > > ?	if (!genimg_has_config(images)) {
> > > > > > > > > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char *
> > > > > > > > > const
> > > > > > > > > argv[],
> > > > > > > > > bootm_headers_t *images,
> > > > > > > > > ?			return fit_img_result;
> > > > > > > > > ?		}
> > > > > > > > > ?
> > > > > > > > > -		if (img_len >= desc_xilinx->size) {
> > > > > > > > > +		switch (desc->devtype) {
> > > > > > > > 
> > > > > > > > Do we need the switch statement at all ? We can have full
> > > > > > > > configuration
> > > > > > > > as a default mode of operation and have something like
> > > > > > > > 
> > > > > > > > if (xilinx) {
> > > > > > > > ?if (partial reconfiguration) {
> > > > > > > > ? do_special_setup();
> > > > > > > > ?}
> > > > > > > > }
> > > > > > > 
> > > > > > > I only did the switch stuff b/c i envisioned a need for partial
> > > > > > > image
> > > > > > > support for socfpga.
> > > > > > 
> > > > > > That'd be seriously cool :)
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > That said, i would suggest, as you mention, moving
> > > > > > > this to platform specific code and perhaps an indication of the
> > > > > > > image
> > > > > > > type
> > > > > > > in the fitimage.
> > > > > > 
> > > > > > driver-specific code . It doesn't need to know the imagetype, just
> > > > > > that
> > > > > > the blob that you passed in is a partial-reconfiguration blob. I
> > > > > > never
> > > > > > really worked with P/R though, do you need some other metadata for
> > > > > > that
> > > > > > or is it contained in that P/R bitstream blob already ?
> > > > > 
> > > > > as far as i understand it, it is all in the blob.??All that is needed
> > > > > is
> > > > > knowing
> > > > > whether the blob is a full or partial image.??X seems to just use the
> > > > > image
> > > > > size
> > > > > to determine this, but that means having a table of all devices and
> > > > > their
> > > > > respective full image size.??seems simpler to just specify the image
> > > > > type is
> > > > > partial or not in the fitimage.
> > > > 
> > > > Can't you extract that info from the RBF though ? But then again,
> > > > extending the fitImage format for this would be fine IMO.
> > > > 
> > > > btw is spelling the X in it's entirety somehow forbidden in A ? :-)
> > > 
> > > Ah, but you forget, we are now I... :)
> > 
> > So it's now A+I ... ? [1] ? :-)
> > [1] http://jisho.org/search/%E6%84%9B
> > 
> > Looking forward to V2 of this , we seem to have some good stuff coming up.
> 
> I can only have some thoughts about it but if you work for I you should
> use that I email. The same in A case and I do something for X that's why
> I use X email.

Thanks for the feedback, although i work for intel, i am doing this purely for
interest. ?As such, and since working on uboot is absolutely not in my job
function, i am required to use a personal email.

--dalon

> Thanks,
> Michal
> 

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

[Dalon Westergreen]

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

--dalon

> Thanks,
> Michal
> 
> Thanks,
> Michal
> 

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

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

[Moritz Fischer]

Hi all,

On Mon, Feb 20, 2017 at 6:59 AM, Michal Simek <michal.simek@xilinx.com> wrote:

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

Over at linux-fpga ([1]) we're currently discussing the idea of coming
up with a header
format (vendor neutral) that we stitch onto the fpga image to allow the fpga-mgr
to derive certain features of the image from the image itself (i.e. is
the image encrypted,
compressed, ...).

We're trying to come up with an extensible format that would allow us
to add stuff like
encryption keys etc.

Cheers,

Moritz


[1] https://patchwork.kernel.org/patch/9574399/

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

[Marek Vasut]

On 02/20/2017 06:35 PM, Moritz Fischer wrote:
> Hi all,
> 
> On Mon, Feb 20, 2017 at 6:59 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> 
>> Definitely I am open to improve this subsystem to make it more flexible.
> 
> Over at linux-fpga ([1]) we're currently discussing the idea of coming
> up with a header
> format (vendor neutral) that we stitch onto the fpga image to allow the fpga-mgr
> to derive certain features of the image from the image itself (i.e. is
> the image encrypted,
> compressed, ...).

Can you CC me on that discussion ? I think it'd be for the best to use
DT for the header format, it's documented and vendor-neutral already.

> We're trying to come up with an extensible format that would allow us
> to add stuff like
> encryption keys etc.

Nothing like a NIH format :)

> Cheers,
> 
> Moritz
> 
> 
> [1] https://patchwork.kernel.org/patch/9574399/
> 


-- 
Best regards,
Marek Vasut