./ykman-gui/ykman-gui Hi Michal, On Mon, May 16, 2022 at 5:25 PM Michal Simek <michal.si...@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.fiergol...@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.fiergol...@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.suvo...@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 <jo...@foundries.io> is reused. > >>>>>>> > >>>>>>> Signed-off-by: Oleksandr Suvorov <oleksandr.suvo...@foundries.io> > >>>>>>> Tested-by: Ricardo Salveti <rica...@foundries.io> > >>>>>>> Co-developed-by: Adrian Fiergolski <adrian.fiergol...@fastree3d.com> > >>>>>>> Signed-off-by: Adrian Fiergolski <adrian.fiergol...@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_deprecate./ykman-gui/ykman-gui d("'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.
Thanks, Michal, the plan looks perfect. So I'm reworking the patchset this weekend. > Thanks, > Michal -- Best regards Oleksandr Oleksandr Suvorov cryo...@gmail.com