| Message ID | 1487602565-6349-2-git-send-email-dwesterg@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Michal Simek |
| Headers | show |
On 20.2.2017 15: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. > > 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. > + * > + * returns: > + * 0, on devnum not found > + * value, on devnum found > + */ > +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); > + if (value == NULL || len != sizeof(int)) > + *devnum = 0; > + else > + *devnum = *value; > + > + return 0; > +} > + > +/** > + * fit_image_fpga_is_partial - is partial fpga bitstream. > + * @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 > + */ > +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)) > + 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) { > + 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); > 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" tbh I am not quite sure if we should introduce this. Please look below. > +#define FIT_FPGA_PARTIAL_PROP "fpga-partial-image" I would suggest to use "partial-fpga-config" because this is what Alan is pushing to mainline kernel. Make no sense to use something different. > > /* 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); > + > /** > * fit_add_verification_data() - add verification data to FIT image nodes > * > I have not a problem with this patch. I didn't test it yet but I will do it. It should probably go through my tree anyway. There are some things which should be good to keep in mind. 1. DM - all fpga drivers are using legacy model and we should really consider to move that stuff to DM and use binding for it. 2. overlays. DT overlays were added recently to u-boot and we should add support for it. Then in general fpga commands can go away entirely and we will just apply overlays directly and this will require DM. / { fragment { target = <&fpga_region0>; __overlay__ { #address-cells = <1>; #size-cells = <1>; firmware-name = "XXXX"; partial-fpga-config; gpio_on: gpio@412c0000 { ... }; }; }; }; Here is visible that target replaces devnum. 3. maybe before overlays we can add support for simple cases as I supported now. Definitely there is a lot of work which need to be done. Thanks, Michal
On Mon, 2017-02-20 at 16:16 +0100, Michal Simek wrote: > On 20.2.2017 15: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. > > > > 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. > > + * > > + * returns: > > + * 0, on devnum not found > > + * value, on devnum found > > + */ > > +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); > > + if (value == NULL || len != sizeof(int)) > > + *devnum = 0; > > + else > > + *devnum = *value; > > + > > + return 0; > > +} > > + > > +/** > > + * fit_image_fpga_is_partial - is partial fpga > > bitstream. will do. > > > > + * @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 > > + */ > > +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)) > > + 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) { > > + 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); > > 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" > > tbh I am not quite sure if we should introduce this. > Please look below. > > > > > > +#define FIT_FPGA_PARTIAL_PROP "fpga-partial-image" > > I would suggest to use "partial-fpga-config" because this is what Alan > is pushing to mainline kernel. Make no sense to use something different. Good point. > > > > > > /* 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); > > + > > /** > > * fit_add_verification_data() - add verification data to FIT image nodes > > * > > > > I have not a problem with this patch. I didn't test it yet but I will do > it. It should probably go through my tree anyway. > > There are some things which should be good to keep in mind. > 1. DM - all fpga drivers are using legacy model and we should really > consider to move that stuff to DM and use binding for it. > > 2. overlays. DT overlays were added recently to u-boot and we should add > support for it. Then in general fpga commands can go away entirely and > we will just apply overlays directly and this will require DM. > > / { > fragment { > target = <&fpga_region0>; > __overlay__ { > #address-cells = <1>; > #size-cells = <1>; > firmware-name = "XXXX"; > partial-fpga-config; > gpio_on: gpio@412c0000 { > ... > }; > }; > }; > }; > > Here is visible that target replaces devnum. I do love overlays, but am not sure if target would replace devnum. My intent for devnum is to support multiple fpgas, not multiple fpga regions. That said, nothing says an fpga region couldnt point to fpga managers for the different fgpa devices... I also think that even if support for overlays is implemented in uboot there is a case for the simpler method where the end os is not linux. Maybe after DM is implemented the fitimage can use target instead of devnum? --dalon > 3. maybe before overlays we can add support for simple cases as I > supported now. > > Definitely there is a lot of work which need to be done. > > Thanks, > Michal
On 20.2.2017 16:46, Dalon Westergreen wrote: > On Mon, 2017-02-20 at 16:16 +0100, Michal Simek wrote: >> On 20.2.2017 15: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. >>> >>> 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. >>> + * >>> + * returns: >>> + * 0, on devnum not found >>> + * value, on devnum found >>> + */ >>> +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); >>> + if (value == NULL || len != sizeof(int)) >>> + *devnum = 0; >>> + else >>> + *devnum = *value; >>> + >>> + return 0; >>> +} >>> + >>> +/** >>> + * fit_image_fpga_is_partial - is partial fpga >> >> bitstream. > > will do. > >>> >>> + * @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 >>> + */ >>> +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)) >>> + 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) { >>> + 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); >>> 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" >> >> tbh I am not quite sure if we should introduce this. >> Please look below. >> >> >>> >>> +#define FIT_FPGA_PARTIAL_PROP "fpga-partial-image" >> >> I would suggest to use "partial-fpga-config" because this is what Alan >> is pushing to mainline kernel. Make no sense to use something different. > > Good point. > >>> >>> >>> /* 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); >>> + >>> /** >>> * fit_add_verification_data() - add verification data to FIT image nodes >>> * >>> >> >> I have not a problem with this patch. I didn't test it yet but I will do >> it. It should probably go through my tree anyway. >> >> There are some things which should be good to keep in mind. >> 1. DM - all fpga drivers are using legacy model and we should really >> consider to move that stuff to DM and use binding for it. >> >> 2. overlays. DT overlays were added recently to u-boot and we should add >> support for it. Then in general fpga commands can go away entirely and >> we will just apply overlays directly and this will require DM. >> >> / { >> fragment { >> target = <&fpga_region0>; >> __overlay__ { >> #address-cells = <1>; >> #size-cells = <1>; >> firmware-name = "XXXX"; >> partial-fpga-config; >> gpio_on: gpio@412c0000 { >> ... >> }; >> }; >> }; >> }; >> >> Here is visible that target replaces devnum. > > I do love overlays, but am not sure if target would replace devnum. My > intent for devnum is to support multiple fpgas, not multiple fpga regions. > That said, nothing says an fpga region couldnt point to fpga managers for > the different fgpa devices... right. fpga region has fpga-mgr property and in general you can use this link as target instead. > > I also think that even if support for overlays is implemented in uboot > there is a case for the simpler method where the end os is not linux. > Maybe after DM is implemented the fitimage can use target instead of > devnum? not sure if you have system with multiple fpgas to load. Definitely it is easy to set it up. If you don't have this setup I would add this patch without devnum specified and 0 is used all the time which is common case. And let's start to look at DM where we can get that link for others fpgas. Thanks, Michal
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. > + * > + * returns: > + * 0, on devnum not found > + * value, on devnum found > + */ > +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); > 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@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot Regards, Simon
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. >> + * >> + * 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? >> + */ >> +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? 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@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot > > Regards, > Simon > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
On Tue, 2017-02-21 at 21:00 -0700, 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/ sure thing. > > > > > > 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. > > + * > > + * returns: > > + * 0, on devnum not found > > + * value, on devnum found > > + */ > > +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? Yes, i will elaboate > > > > > + * @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? Bad cut and paste... i will fix this. > > > > + */ > > +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? This enum type is defined in fpga.h and is lower case there as well. > > > > + 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); > > 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? sure thing. Thanks for the feedback. --dalon > > > > + > > /** > > * fit_add_verification_data() - add verification data to FIT image nodes > > * > > -- > > 2.7.4 > > > > _______________________________________________ > > U-Boot mailing list > > U-Boot@lists.denx.de > > http://lists.denx.de/mailman/listinfo/u-boot > > Regards, > Simon
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@lists.denx.de > > > http://lists.denx.de/mailman/listinfo/u-boot > > > > Regards, > > Simon > > _______________________________________________ > > U-Boot mailing list > > U-Boot@lists.denx.de > > http://lists.denx.de/mailman/listinfo/u-boot > >
On Tue, 2017-02-21 at 21:00 -0700, 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. > [snip ...] > > > > /* 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? Doing this would be counter to every other function in the c file these are added to, so i think it best to leave as is. --dalon > > > > + > > /** > > * fit_add_verification_data() - add verification data to FIT image nodes > > * > > -- > > 2.7.4 > > > > _______________________________________________ > > U-Boot mailing list > > U-Boot@lists.denx.de > > http://lists.denx.de/mailman/listinfo/u-boot > > Regards, > Simon
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. + * + * returns: + * 0, on devnum not found + * value, on devnum found + */ +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); + if (value == NULL || len != sizeof(int)) + *devnum = 0; + else + *devnum = *value; + + return 0; +} + +/** + * fit_image_fpga_is_partial - is partial fpga + * @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 + */ +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)) + 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) { + 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); 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); + /** * fit_add_verification_data() - add verification data to FIT image nodes *
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@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(-)