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

[Adrian Fiergolski]

Hi Oleksandr,

On 09.05.2022 13:02, Oleksandr Suvorov wrote:
> Hi Adrian,
>
> On Wed, May 4, 2022 at 5:28 PM Adrian Fiergolski
> <adrian.fiergolski at fastree3d.com> wrote:
>> 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 at 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 at foundries.io> is reused.
>>>>
>>>> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov at foundries.io>
>>>> Tested-by: Ricardo Salveti <ricardo at foundries.io>
>>>> Co-developed-by: Adrian Fiergolski <adrian.fiergolski at fastree3d.com>
>>>> Signed-off-by: Adrian Fiergolski <adrian.fiergolski at 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.
> Obviously, the "compatible" attribute belongs to an image, not to an FPGA
> driver. Unfortunately, I don't see an easy way to stick this attribute to an
> image in the current architecture. Maybe I missed that way, so I'd appreciate
> any help with this.
>
> Anyway, we could come back to the FPGA driver subsystem later and rework
> it better and deeper.
>
> For now, I'd only fix the warning. Are you ok with this?

I haven't seen straightforward implementation in the current 
architecture as well. However, it's this series of patches which 
introduces 'compatible', so IMHO, it should be done properly. Michal, 
any ideas/opinions?

Adrian

>>>
>>>> +
>>>> +    /*
>>>> +     * 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
>
>
> --
> Best regards
> Oleksandr
>
> Oleksandr Suvorov
> cryosay at gmail.com