Re: [PATCH v7 2/7] fpga: add fit_fpga_load function

[Adrian Fiergolski <adrian.fiergolski@fastree3d.com>]

Hi Oleksandr,

On 03.05.2022 09:42, Michal Simek wrote:
>
>
> On 4/11/22 20:00, Adrian Fiergolski wrote:
>> From: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
>>
>> Introduce a function which passes an fpga compatible string from
>> FIT images to FPGA drivers. This lets the different implementations
>> decide how to handle it.
>>
>> Some code of Jorge Ramirez-Ortiz <jorge@foundries.io> is reused.
>>
>> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
>> Tested-by: Ricardo Salveti <ricardo@foundries.io>
>> Co-developed-by: Adrian Fiergolski <adrian.fiergolski@fastree3d.com>
>> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@fastree3d.com>
>> ---
>>   common/spl/spl_fit.c |  6 ++----
>>   drivers/fpga/fpga.c  | 23 ++++++++++++++++++++++-
>>   include/fpga.h       |  4 ++++
>>   3 files changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index 1bbf824684..0e3c2a94b6 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -588,11 +588,9 @@ static int spl_fit_upload_fpga(struct 
>> spl_fit_info *ctx, int node,
>>       compatible = fdt_getprop(ctx->fit, node, "compatible", NULL);
>>       if (!compatible)
>>           warn_deprecated("'fpga' image without 'compatible' property");
>> -    else if (strcmp(compatible, "u-boot,fpga-legacy"))
>> -        printf("Ignoring compatible = %s property\n", compatible);
>>   -    ret = fpga_load(0, (void *)fpga_image->load_addr, 
>> fpga_image->size,
>> -            BIT_FULL);
>> +    ret = fit_fpga_load(0, (void *)fpga_image->load_addr, 
>> fpga_image->size,
>> +                BIT_FULL, compatible);
>>       if (ret) {
>>           printf("%s: Cannot load the image to the FPGA\n", __func__);
>>           return ret;
>> diff --git a/drivers/fpga/fpga.c b/drivers/fpga/fpga.c
>> index 3b0a44b242..a306dd81f9 100644
>> --- a/drivers/fpga/fpga.c
>> +++ b/drivers/fpga/fpga.c
>> @@ -249,8 +249,23 @@ int fpga_loads(int devnum, const void *buf, 
>> size_t size,
>>   }
>>   #endif
>>   +int fit_fpga_load(int devnum, const void *buf, size_t bsize,
>> +          bitstream_type bstype, const char *compatible)
>> +{
>> +    fpga_desc *desc = fpga_get_desc(devnum);
>
> this generates warning which you should fix.
>
> +  fpga_desc *desc = fpga_get_desc(devnum);
> +                    ^~~~~~~~~~~~~
> w+../drivers/fpga/fpga.c: In function ‘fit_fpga_load’:
> w+../drivers/fpga/fpga.c:255:20: warning: initialization discards 
> ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]

As it's your patch, could you address it?

The 'compatible' field can't be set here, as fpga_get_desc returns 
'const fpga_desc * const'. The descriptors table is populated (fpga_add) 
in board_init. At that stage, I am afraid, we don't have access to the 
fit image information yet to set a proper 'compatible' (would need to 
check the code more carefully). Moreover, IMHO it's not the cleanest way 
to change the 'compatible' field after the driver's initialisation.

>
>
>> +
>> +    /*
>> +     * Store the compatible string to proceed it in underlying
>> +     * functions
>> +     */
>> +    desc->compatible = (char *)compatible;
>> +
>> +    return fpga_load(devnum, buf, bsize, bstype);
>> +}
>> +
>>   /*
>> - * Generic multiplexing code
>> + * Generic multiplexing code:
>> + * Each architecture must handle the mandatory FPGA DT compatible 
>> property.
>>    */
>>   int fpga_load(int devnum, const void *buf, size_t bsize, 
>> bitstream_type bstype)
>>   {
>> @@ -270,6 +285,9 @@ int fpga_load(int devnum, const void *buf, size_t 
>> bsize, bitstream_type bstype)
>>               break;
>>           case fpga_altera:
>>   #if defined(CONFIG_FPGA_ALTERA)
>> +            if (strncmp(desc->compatible, "u-boot,fpga-legacy", 18))
>> +                printf("Ignoring compatible = %s property\n",
>> +                       desc->compatible);
>>               ret_val = altera_load(desc->devdesc, buf, bsize);
>>   #else
>>               fpga_no_sup((char *)__func__, "Altera devices");
>> @@ -277,6 +295,9 @@ int fpga_load(int devnum, const void *buf, size_t 
>> bsize, bitstream_type bstype)
>>               break;
>>           case fpga_lattice:
>>   #if defined(CONFIG_FPGA_LATTICE)
>> +            if (strncmp(desc->compatible, "u-boot,fpga-legacy", 18))
>> +                printf("Ignoring compatible = %s property\n",
>> +                       desc->compatible);
>>               ret_val = lattice_load(desc->devdesc, buf, bsize);
>>   #else
>>               fpga_no_sup((char *)__func__, "Lattice devices");
>> diff --git a/include/fpga.h b/include/fpga.h
>> index ec5144334d..2891f32106 100644
>> --- a/include/fpga.h
>> +++ b/include/fpga.h
>> @@ -35,6 +35,7 @@ typedef enum {            /* typedef fpga_type */
>>   typedef struct {        /* typedef fpga_desc */
>>       fpga_type devtype;    /* switch value to select sub-functions */
>>       void *devdesc;        /* real device descriptor */
>> +    char *compatible;    /* device compatible string */
>>   } fpga_desc;            /* end, typedef fpga_desc */
>>     typedef struct {                /* typedef fpga_desc */
>> @@ -63,6 +64,9 @@ int fpga_add(fpga_type devtype, void *desc);
>>   int fpga_count(void);
>>   const fpga_desc *const fpga_get_desc(int devnum);
>>   int fpga_is_partial_data(int devnum, size_t img_len);
>> +/* the DT compatible property must be handled by the different FPGA 
>> archs */
>> +int fit_fpga_load(int devnum, const void *buf, size_t bsize,
>> +          bitstream_type bstype, const char *compatible);
>>   int fpga_load(int devnum, const void *buf, size_t bsize,
>>             bitstream_type bstype);
>>   int fpga_fsload(int devnum, const void *buf, size_t size,
>
> M
Adrian