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 <dwest...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to