[U-Boot] [PATCH v3 1/2] common: image: update boot_get_fpga to support arbitrary fpga image
Dalon Westergreen
dwesterg at gmail.com
Mon Feb 20 15:46:54 UTC 2017
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 at 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 at 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
More information about the U-Boot
mailing list