[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 at gmail.com>
> 
> +CC Xilinx friends :)
> 
> > 
> > ---
> >  common/image.c | 37 ++++++++++++++++++++++---------------
> >  1 file changed, 22 insertions(+), 15 deletions(-)
> > 
> > diff --git a/common/image.c b/common/image.c
> > index 0f88984..792d371 100644
> > --- a/common/image.c
> > +++ b/common/image.c
> > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t
> > arch,
> >  }
> >  
> >  #if IMAGE_ENABLE_FIT
> > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX)
> > +#if defined(CONFIG_FPGA)
> >  int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images,
> >  		  uint8_t arch, const ulong *ld_start, ulong * const
> > ld_len)
> >  {
> > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[],
> > bootm_headers_t *images,
> >  	int err;
> >  	int devnum = 0; /* TODO support multi fpga platforms */
> >  	const fpga_desc * const desc = fpga_get_desc(devnum);
> > -	xilinx_desc *desc_xilinx = desc->devdesc;
> > +	xilinx_desc *desc_xilinx;
> > +	bitstream_type bstype;
> >  
> >  	/* Check to see if the images struct has a FIT configuration */
> >  	if (!genimg_has_config(images)) {
> > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[],
> > bootm_headers_t *images,
> >  			return fit_img_result;
> >  		}
> >  
> > -		if (img_len >= desc_xilinx->size) {
> > +		switch (desc->devtype) {
> 
> Do we need the switch statement at all ? We can have full configuration
> as a default mode of operation and have something like
> 
> if (xilinx) {
>  if (partial reconfiguration) {
>   do_special_setup();
>  }
> }

I only did the switch stuff b/c i envisioned a need for partial image
support for socfpga.  That 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;
> >  
> > 
> 
>