From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dalon Westergreen Date: Wed, 22 Feb 2017 06:26:47 -0800 Subject: [U-Boot] [PATCH v3 1/2] common: image: update boot_get_fpga to support arbitrary fpga image In-Reply-To: References: <1487602565-6349-1-git-send-email-dwesterg@gmail.com> <1487602565-6349-2-git-send-email-dwesterg@gmail.com> Message-ID: <1487773607.782.9.camel@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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 > > > > > > -- > > > 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 > >