On 14/10/21, Simon Glass wrote:

Hi Jorge,

On Tue, 5 Oct 2021 at 05:13, Jorge Ramirez-Ortiz jorge@foundries.io wrote:

...

Instead of ignoring the mandatory fpga compatible string, let the different implementations decide how to handle it

Signed-off-by: Jorge Ramirez-Ortiz jorge@foundries.io

cmd/fpga.c | 8 ++++---- common/image.c | 4 ++-- common/spl/spl_fit.c | 4 +--- drivers/fpga/fpga.c | 11 +++++++++-- include/fpga.h | 2 +- 5 files changed, 17 insertions(+), 12 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

thanks, I'll add your tag

...

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c

[..]

...

index f41abca0cc..4db22efd8c 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -566,11 +566,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);
  

•                   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 fe3dfa1233..00aa4190b4 100644 --- a/drivers/fpga/fpga.c +++ b/drivers/fpga/fpga.c @@ -252,7 +252,8 @@ int fpga_loads(int devnum, const void *buf, size_t size, /*

• Generic multiplexing code

*/ -int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype) +int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype,

•         const char *compatible)
  

{ int ret_val = FPGA_FAIL; /* assume failure */ const fpga_desc *desc = fpga_validate(devnum, buf, bsize, @@ -263,13 +264,16 @@ int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype) case fpga_xilinx: #if defined(CONFIG_FPGA_XILINX) ret_val = xilinx_load(desc->devdesc, buf, bsize,

•                                         bstype);
  

•                                         bstype, compatible);
  

#else fpga_no_sup((char *)__func__, "Xilinx devices"); #endif break; case fpga_altera: #if defined(CONFIG_FPGA_ALTERA)

•                   if (strcmp(compatible, "u-boot,fpga-legacy"))
  

•                           printf("Ignoring compatible = %s property\n",
  

•                                  compatible);
                    ret_val = altera_load(desc->devdesc, buf, bsize);
  

#else fpga_no_sup((char *)__func__, "Altera devices"); @@ -277,6 +281,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 (strcmp(compatible, "u-boot,fpga-legacy"))
  

•                           printf("Ignoring compatible = %s property\n",
  

•                                  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..d85ef2cf18 100644 --- a/include/fpga.h +++ b/include/fpga.h @@ -64,7 +64,7 @@ int fpga_count(void); const fpga_desc *const fpga_get_desc(int devnum); int fpga_is_partial_data(int devnum, size_t img_len); int fpga_load(int devnum, const void *buf, size_t bsize,

•         bitstream_type bstype);
  

•         bitstream_type bstype, const char *compatible);
  

Please can you add a function comment?

ok

...

int fpga_fsload(int devnum, const void *buf, size_t size, fpga_fs_info *fpga_fsinfo); int fpga_loads(int devnum, const void *buf, size_t size, -- 2.31.1

Also the FPGA uclass is missing a sandbox driver/emulator and a test, if you have time to do that. At present FPGA tests in CI are skipped (e.g. with make qcheck).

I am really short of time ATM with another deadline coming (sorry!) I believe Oleksandr might be willing to pick this up but I'll let him add further.

Regards, Simon

On 10/5/21 1:13 PM, Jorge Ramirez-Ortiz wrote:

Add new compatible string u-boot,zynqmp-fpga-ddrauth to handle this use case.

Signed-off-by: Jorge Ramirez-Ortiz jorge@foundries.io

drivers/fpga/xilinx.c | 29 +++++++++++++++++++++++++---- drivers/fpga/zynqmppl.c | 4 ++-- include/xilinx.h | 2 +- 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c index cbebefb55f..c8035105e2 100644 --- a/drivers/fpga/xilinx.c +++ b/drivers/fpga/xilinx.c @@ -135,23 +135,44 @@ int fpga_loadbitstream(int devnum, char *fpgadata, size_t size, dataptr += 4; printf(" bytes in bitstream = %d\n", swapsize);

• return fpga_load(devnum, dataptr, swapsize, bstype);

• return fpga_load(devnum, dataptr, swapsize, bstype, NULL);

}

int xilinx_load(xilinx_desc *desc, const void *buf, size_t bsize,

• bitstream_type bstype)
  

• bitstream_type bstype, const char *compatible)
  

{

• struct fpga_secure_info info = { 0 };
• if (!xilinx_validate (desc, (char *)__FUNCTION__)) { printf ("%s: Invalid device descriptor\n", __FUNCTION__); return FPGA_FAIL; }

• if (!desc->operations || !desc->operations->load) {

• if (!desc->operations) { printf("%s: Missing load operation\n", __func__); return FPGA_FAIL; }

• return desc->operations->load(desc, buf, bsize, bstype);

• if (!compatible || !strcmp(compatible, "u-boot,fpga-legacy")) {

• if (!desc->operations->load) {
  

• 	printf("%s: Missing load operation\n", __func__);
  

• 	return FPGA_FAIL;
  

• }
  

• return desc->operations->load(desc, buf, bsize, bstype);
  

• }
• if (!strcmp(compatible, "u-boot,zynqmp-fpga-ddrauth")) {

• if (!desc->operations->loads) {
  

• 	printf("%s: Missing load operation\n", __func__);
  

• 	return FPGA_FAIL;
  

• }
  

• /* DDR authentication */
  

• info.authflag = 1;
  

• return desc->operations->loads(desc, buf, bsize, &info);
  

• }

I am not sure about this solution. First of all it forces SPL to have additional code which just extending size. It means there must be a way to enable/disable it.

The next thing is that you have zynqmp string in generic xilinx file which doesn't look right. It would be better to deal with image types directly in driver which is capable to handle it. It means record fit image compatible string in the driver with hooks/flags and determined what should be called.

And the same style should work for SPL and also for U-Boot proper.

Thanks, Michal

On 10/5/21 4:03 PM, Jorge Ramirez-Ortiz, Foundries wrote:

On 05/10/21, Michal Simek wrote:

...

On 10/5/21 1:13 PM, Jorge Ramirez-Ortiz wrote:

...

Add new compatible string u-boot,zynqmp-fpga-ddrauth to handle this use case.

Signed-off-by: Jorge Ramirez-Ortiz jorge@foundries.io

drivers/fpga/xilinx.c | 29 +++++++++++++++++++++++++---- drivers/fpga/zynqmppl.c | 4 ++-- include/xilinx.h | 2 +- 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c index cbebefb55f..c8035105e2 100644 --- a/drivers/fpga/xilinx.c +++ b/drivers/fpga/xilinx.c @@ -135,23 +135,44 @@ int fpga_loadbitstream(int devnum, char *fpgadata, size_t size, dataptr += 4; printf(" bytes in bitstream = %d\n", swapsize);

• return fpga_load(devnum, dataptr, swapsize, bstype);

• return fpga_load(devnum, dataptr, swapsize, bstype, NULL);

}

int xilinx_load(xilinx_desc *desc, const void *buf, size_t bsize,

• bitstream_type bstype)
  

• bitstream_type bstype, const char *compatible)
  

{

• struct fpga_secure_info info = { 0 };
• if (!xilinx_validate (desc, (char *)__FUNCTION__)) { printf ("%s: Invalid device descriptor\n", __FUNCTION__); return FPGA_FAIL; }

• if (!desc->operations || !desc->operations->load) {

• if (!desc->operations) { printf("%s: Missing load operation\n", __func__); return FPGA_FAIL; }

• return desc->operations->load(desc, buf, bsize, bstype);

• if (!compatible || !strcmp(compatible, "u-boot,fpga-legacy")) {

• if (!desc->operations->load) {
  

• 	printf("%s: Missing load operation\n", __func__);
  

• 	return FPGA_FAIL;
  

• }
  

• return desc->operations->load(desc, buf, bsize, bstype);
  

• }
• if (!strcmp(compatible, "u-boot,zynqmp-fpga-ddrauth")) {

• if (!desc->operations->loads) {
  

• 	printf("%s: Missing load operation\n", __func__);
  

• 	return FPGA_FAIL;
  

• }
  

• /* DDR authentication */
  

• info.authflag = 1;
  

• return desc->operations->loads(desc, buf, bsize, &info);
  

• }

I am not sure about this solution. First of all it forces SPL to have additional code which just extending size. It means there must be a way to enable/disable it.

that can be contained of course...so not really an issue (ie compile out either fpga_load or fpga_loads depending on needs).

...

The next thing is that you have zynqmp string in generic xilinx file which doesn't look right. It would be better to deal with image types directly in driver which is capable to handle it.

sure that is easy..but the idea is the same

We just need to find out how this should be properly propagated and stored.

...

It means record fit image compatible string in the driver with hooks/flags and determined what should be called.

what about the compatible names? will you define those? or should I do it?

Just define it and let's see if this fits.

For our use case, we need DDR authentication so that must be defined. I can add others and someone else can add the support needed?

Sure. None is asking you to support everything. Just add support for regular bitstreams and your case. And adding support for something else will be just +1 if interface is right.

Thanks, Michal