Hi Dalon,
On 02/22/2017 06:00 AM, Simon Glass wrote:
> Hi Dalon,
>
> On 20 February 2017 at 07:56, Dalon Westergreen <dwest...@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 <dwest...@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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
