| Message ID | 20220411180046.1505209-3-adrian.fiergolski@fastree3d.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | fpga: zynqmp: Adding support of loading authenticated images | expand |
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] > + > + /* > + * 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
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. > > >> + >> + /* >> + * 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
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. Thanks for catching this. I'm addressing the issue this Friday. > > > >> + > >> + /* > >> + * 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
Hi Oleksandr, On 04.05.2022 20:26, 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. > Thanks for catching this. I'm addressing the issue this Friday. Perfect. I understand you will address all Michal's comments, including those with my SOB missing, and you will release the new version of the patches, right? Thanks, Adrian
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? > > > > > >> + > >> + /* > >> + * 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@gmail.com
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? 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@gmail.com
Hi Adrian, On Mon, May 9, 2022 at 1:30 PM Adrian Fiergolski <adrian.fiergolski@fastree3d.com> wrote: > > Hi Oleksandr, > > On 04.05.2022 20:26, 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. > > Thanks for catching this. I'm addressing the issue this Friday. > > Perfect. I understand you will address all Michal's comments, including > those with my SOB missing, and you will release the new version of the > patches, right? Yes, I've already addressed Michal's comments and going to release the new patchset version in 1-2 hours after finishing tests. > > Thanks, > Adrian >
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. > > 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@gmail.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
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@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? 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
./ykman-gui/ykman-gui Hi Michal, On Mon, May 16, 2022 at 5:25 PM Michal Simek <michal.simek@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@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_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
Hi Michal, On Mon, May 16, 2022 at 5:25 PM Michal Simek <michal.simek@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@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? > > 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 > > >
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); + + /* + * 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,