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_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. > > 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 > > cryo...@gmail.com -- Best regards Oleksandr Oleksandr Suvorov cryo...@gmail.com