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

[Oleksandr Suvorov]

Hi Michal,

On Mon, May 16, 2022 at 5:25 PM Michal Simek <michal.simek at xilinx.com> wrote:
>
>
>
> On 5/9/22 15:34, Adrian Fiergolski wrote:
> > 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 at 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 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?
> >> 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?
>
> I think you should convert compatible string from fit image to flags and don't
> parse any compatible string in the driver but only work with this flag. Maybe
> make sense also to record in the driver structure all supported flags by driver
> and let core to check if driver provides that feature or not. With conversion to
> DM this can allow to enable multiple drivers that one handle only simple
> unencrypted not authenticated bitstreams and others different types or different
> transport mechanisms.
>

I've done this in a legacy drive, please look at the next patchset v8.
I started adding DM fpga uclass and DM xilinix driver, but this work may take
extra time. So could you review the current solution, please?

> Thanks,
> Michal
>
>
>


-- 
Best regards
Oleksandr

Oleksandr Suvorov
cryosay at gmail.com