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