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

[Dalon Westergreen <dwesterg@gmail.com>]

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