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

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

Michal,

On 09.05.2022 15:28, Oleksandr Suvorov wrote:
> Hi Adrian,
>
> On Mon, May 9, 2022 at 2:35 PM Adrian Fiergolski
> <adrian.fiergolski@fastree3d.com> wrote:
>> 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@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@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.
>>> 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?
> I see 2 ways to keep all "compatible"-logic in spl_fit_upload_fpga():
> - by extending each vendor-specific *_load() function with another
> parameter, which
>    will contain a unique type for any supported "compatible" value.
> - making up a better-fit structure like "fpga_bitstream", more
> specific for fpga bs only
>    and smaller than spl_image_info, packing there a bs ptr, bs size, bs
> type, and bs
>    "compatible" members.
> Both ways require changing the interface for all fpga load()-family functions.

Which option from Oleksandr's proposal do you think will be easier to 
push upstream?

Regards,
Adrian