[PATCH v7 0/7] fpga: zynqmp: Adding support of loading authenticated images

[PATCH v7 0/7] fpga: zynqmp: Adding support of loading authenticated images

[Adrian Fiergolski]

This patchset introduces support for the authenticated FPGA images
on ZynqMP boards, besides that introducing common way to pass the
compatible property to any fpga driver.

It bases on the initial work by Jorge Ramirez-Ortiz <jorge@foundries.io>
https://patchwork.ozlabs.org/project/uboot/patch/20211015091506.2602-1-jorge@foundries.io/
https://patchwork.ozlabs.org/project/uboot/patch/20211005111324.19749-3-jorge@foundries.io/

Changed in v7:
- apply Michal Simek's suggestions
  As I applied changes on Oleksandr's patches, I indicated it by specifying myself as co-author
  in the commits logs. I am not sure if that is the convention of marking it.

Changed in v6:
- add support for the encrypted bitfiles

Changes in v5:
- replace ifdef with if() where it's possible

Changes in v4:
- change interface to xilinx_desc->operations->open() callback.
- fix a bug from previous version of the patchset in dereferencing
  of a parent fpga_desc structure.

Changes in v3:
- remove the patch which introduced CMD_SPL_FPGA_LOAD_SECURE.
- fix mixing definitions/declarations.
- replace strcmp() calls with more secure strncmp().
- document the "u-boot,zynqmp-fpga-ddrauth" compatible string.
- fix code style by check-patch recommendations.

Changes in v2:
- add function fit_fpga_load() to simplify calls of fpga_load()
  from contexts without a compatible attribute.
- move all ZynqMP-specific logic to drivers/fpga/zynqmppl.c
- prepare for passing a "compatible" FDT property to any fpga driver.

Oleksandr Suvorov (6):
  fpga: add option for loading FPGA secure bitstreams
  fpga: add fit_fpga_load function
  fpga: xilinx: pass an address of xilinx_desc in fpga_desc
  fpga: xilinx: add missed identifier names
  fpga: xilinx: pass xilinx_desc pointer address into load() ops
  fpga: zynqmp: support loading authenticated images

Adrian Fiergolski (1):
  fpga: zynqmp: support loading encrypted bitfiles

 boot/Kconfig                          |  4 +--
 cmd/Kconfig                           |  3 ++-
 common/spl/spl_fit.c                  |  6 ++---
 doc/uImage.FIT/source_file_format.txt |  7 +++++-
 drivers/fpga/Kconfig                  | 14 +++++++++++
 drivers/fpga/fpga.c                   | 29 ++++++++++++++++++---
 drivers/fpga/spartan2.c               |  3 ++-
 drivers/fpga/spartan3.c               |  3 ++-
 drivers/fpga/versalpl.c               |  2 +-
 drivers/fpga/virtex2.c                |  3 ++-
 drivers/fpga/xilinx.c                 |  8 +++---
 drivers/fpga/zynqmppl.c               | 36 ++++++++++++++++++++++++---
 drivers/fpga/zynqpl.c                 |  3 ++-
 include/fpga.h                        |  5 ++++
 include/xilinx.h                      | 12 +++++----
 15 files changed, 109 insertions(+), 29 deletions(-)

-- 
2.35.1

[PATCH v7 1/7] fpga: add option for loading FPGA secure bitstreams

[Adrian Fiergolski]

From: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>

It allows using this feature without enabling the "fpga loads"
command.

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>
---
 cmd/Kconfig             |  3 ++-
 drivers/fpga/Kconfig    | 14 ++++++++++++++
 drivers/fpga/fpga.c     |  2 +-
 drivers/fpga/xilinx.c   |  2 +-
 drivers/fpga/zynqmppl.c |  4 ++--
 5 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 5e25e45fd2..604ab37f3b 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -949,8 +949,9 @@ config CMD_FPGA_LOADP
 	  a partial bitstream.
 
 config CMD_FPGA_LOAD_SECURE
-	bool "fpga loads - loads secure bitstreams (Xilinx only)"
+	bool "fpga loads - loads secure bitstreams"
 	depends on CMD_FPGA
+	select FPGA_LOAD_SECURE
 	help
 	  Enables the fpga loads command which is used to load secure
 	  (authenticated or encrypted or both) bitstreams on to FPGA.
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index dc0b3dd31b..6f8ef7b8db 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -85,4 +85,18 @@ config FPGA_ZYNQPL
 	  Enable FPGA driver for loading bitstream in BIT and BIN format
 	  on Xilinx Zynq devices.
 
+config FPGA_LOAD_SECURE
+	bool "Enable loading secure bitstreams"
+	depends on FPGA
+	help
+	  Enables the fpga loads() functions that are used to load secure
+	  (authenticated or encrypted or both) bitstreams on to FPGA.
+
+config SPL_FPGA_LOAD_SECURE
+	bool "Enable loading secure bitstreams for SPL"
+	depends on SPL_FPGA
+	help
+	  Enables the fpga loads() functions that are used to load secure
+	  (authenticated or encrypted or both) bitstreams on to FPGA.
+
 endmenu
diff --git a/drivers/fpga/fpga.c b/drivers/fpga/fpga.c
index fe3dfa1233..3b0a44b242 100644
--- a/drivers/fpga/fpga.c
+++ b/drivers/fpga/fpga.c
@@ -220,7 +220,7 @@ int fpga_fsload(int devnum, const void *buf, size_t size,
 }
 #endif
 
-#if defined(CONFIG_CMD_FPGA_LOAD_SECURE)
+#if CONFIG_IS_ENABLED(FPGA_LOAD_SECURE)
 int fpga_loads(int devnum, const void *buf, size_t size,
 	       struct fpga_secure_info *fpga_sec_info)
 {
diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c
index cbebefb55f..6bc1bc491f 100644
--- a/drivers/fpga/xilinx.c
+++ b/drivers/fpga/xilinx.c
@@ -172,7 +172,7 @@ int xilinx_loadfs(xilinx_desc *desc, const void *buf, size_t bsize,
 }
 #endif
 
-#if defined(CONFIG_CMD_FPGA_LOAD_SECURE)
+#if CONFIG_IS_ENABLED(FPGA_LOAD_SECURE)
 int xilinx_loads(xilinx_desc *desc, const void *buf, size_t bsize,
 		 struct fpga_secure_info *fpga_sec_info)
 {
diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
index 6b394869db..8ff12bf50a 100644
--- a/drivers/fpga/zynqmppl.c
+++ b/drivers/fpga/zynqmppl.c
@@ -245,7 +245,7 @@ static int zynqmp_load(xilinx_desc *desc, const void *buf, size_t bsize,
 	return ret;
 }
 
-#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) && !defined(CONFIG_SPL_BUILD)
+#if CONFIG_IS_ENABLED(FPGA_LOAD_SECURE)
 static int zynqmp_loads(xilinx_desc *desc, const void *buf, size_t bsize,
 			struct fpga_secure_info *fpga_sec_info)
 {
@@ -306,7 +306,7 @@ static int zynqmp_pcap_info(xilinx_desc *desc)
 
 struct xilinx_fpga_op zynqmp_op = {
 	.load = zynqmp_load,
-#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) && !defined(CONFIG_SPL_BUILD)
+#if CONFIG_IS_ENABLED(FPGA_LOAD_SECURE)
 	.loads = zynqmp_loads,
 #endif
 	.info = zynqmp_pcap_info,
-- 
2.35.1

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

[Adrian Fiergolski]

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);
+
+	/*
+	 * 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,
-- 
2.35.1

[PATCH v7 3/7] fpga: xilinx: pass an address of xilinx_desc in fpga_desc

[Adrian Fiergolski]

From: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>

Pass an address of xilinx_desc pointer in an fpga_desc to use parent
fpga_desc structure members inside a xilinx fpga driver.

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>
---
 drivers/fpga/fpga.c   | 4 ++--
 drivers/fpga/xilinx.c | 4 +++-
 include/xilinx.h      | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/fpga/fpga.c b/drivers/fpga/fpga.c
index a306dd81f9..5e2288a124 100644
--- a/drivers/fpga/fpga.c
+++ b/drivers/fpga/fpga.c
@@ -277,8 +277,8 @@ int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype)
 		switch (desc->devtype) {
 		case fpga_xilinx:
 #if defined(CONFIG_FPGA_XILINX)
-			ret_val = xilinx_load(desc->devdesc, buf, bsize,
-					      bstype);
+			ret_val = xilinx_load((xilinx_desc **)&desc->devdesc,
+					      buf, bsize, bstype);
 #else
 			fpga_no_sup((char *)__func__, "Xilinx devices");
 #endif
diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c
index 6bc1bc491f..640baac66e 100644
--- a/drivers/fpga/xilinx.c
+++ b/drivers/fpga/xilinx.c
@@ -138,9 +138,11 @@ int fpga_loadbitstream(int devnum, char *fpgadata, size_t size,
 	return fpga_load(devnum, dataptr, swapsize, bstype);
 }
 
-int xilinx_load(xilinx_desc *desc, const void *buf, size_t bsize,
+int xilinx_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
 		bitstream_type bstype)
 {
+	xilinx_desc *desc = *desc_ptr;
+
 	if (!xilinx_validate (desc, (char *)__FUNCTION__)) {
 		printf ("%s: Invalid device descriptor\n", __FUNCTION__);
 		return FPGA_FAIL;
diff --git a/include/xilinx.h b/include/xilinx.h
index ab4537becf..57b0e7be11 100644
--- a/include/xilinx.h
+++ b/include/xilinx.h
@@ -58,7 +58,7 @@ struct xilinx_fpga_op {
 
 /* Generic Xilinx Functions
  *********************************************************************/
-int xilinx_load(xilinx_desc *desc, const void *image, size_t size,
+int xilinx_load(xilinx_desc **desc_ptr, const void *image, size_t size,
 		bitstream_type bstype);
 int xilinx_dump(xilinx_desc *desc, const void *buf, size_t bsize);
 int xilinx_info(xilinx_desc *desc);
-- 
2.35.1

[PATCH v7 4/7] fpga: xilinx: add missed identifier names

[Adrian Fiergolski]

From: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>

Function definition arguments should also have identifier names.
Add missed ones to struct xilinx_fpga_op callbacks, unifying code.

Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
---
 include/xilinx.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/xilinx.h b/include/xilinx.h
index 57b0e7be11..06ecc9a842 100644
--- a/include/xilinx.h
+++ b/include/xilinx.h
@@ -48,12 +48,14 @@ typedef struct {		/* typedef xilinx_desc */
 } xilinx_desc;			/* end, typedef xilinx_desc */
 
 struct xilinx_fpga_op {
-	int (*load)(xilinx_desc *, const void *, size_t, bitstream_type);
-	int (*loadfs)(xilinx_desc *, const void *, size_t, fpga_fs_info *);
+	int (*load)(xilinx_desc *desc, const void *buf, size_t bsize,
+		    bitstream_type bstype);
+	int (*loadfs)(xilinx_desc *desc, const void *buf, size_t bsize,
+		      fpga_fs_info *fpga_fsinfo);
 	int (*loads)(xilinx_desc *desc, const void *buf, size_t bsize,
 		     struct fpga_secure_info *fpga_sec_info);
-	int (*dump)(xilinx_desc *, const void *, size_t);
-	int (*info)(xilinx_desc *);
+	int (*dump)(xilinx_desc *desc, const void *buf, size_t bsize);
+	int (*info)(xilinx_desc *desc);
 };
 
 /* Generic Xilinx Functions
-- 
2.35.1

[PATCH v7 5/7] fpga: xilinx: pass xilinx_desc pointer address into load() ops

[Adrian Fiergolski]

From: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>

Pass an address of xilinx_desc pointer in an fpga_desc into a load()
callback of struct xilinx_fpga_op.
It allows getting parent fpga_desc structure members inside xilinx
fpga drivers.

Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
Tested-by: Ricardo Salveti <ricardo@foundries.io>
---
 drivers/fpga/spartan2.c | 3 ++-
 drivers/fpga/spartan3.c | 3 ++-
 drivers/fpga/versalpl.c | 2 +-
 drivers/fpga/virtex2.c  | 3 ++-
 drivers/fpga/xilinx.c   | 2 +-
 drivers/fpga/zynqmppl.c | 5 +++--
 drivers/fpga/zynqpl.c   | 3 ++-
 include/xilinx.h        | 2 +-
 8 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/fpga/spartan2.c b/drivers/fpga/spartan2.c
index 3435400e58..f40edb1747 100644
--- a/drivers/fpga/spartan2.c
+++ b/drivers/fpga/spartan2.c
@@ -40,10 +40,11 @@ static int spartan2_ss_dump(xilinx_desc *desc, const void *buf, size_t bsize);
 
 /* ------------------------------------------------------------------------- */
 /* Spartan-II Generic Implementation */
-static int spartan2_load(xilinx_desc *desc, const void *buf, size_t bsize,
+static int spartan2_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
 			 bitstream_type bstype)
 {
 	int ret_val = FPGA_FAIL;
+	xilinx_desc *desc = *desc_ptr;
 
 	switch (desc->iface) {
 	case slave_serial:
diff --git a/drivers/fpga/spartan3.c b/drivers/fpga/spartan3.c
index 4850c99352..b7c1ddd40f 100644
--- a/drivers/fpga/spartan3.c
+++ b/drivers/fpga/spartan3.c
@@ -44,10 +44,11 @@ static int spartan3_ss_dump(xilinx_desc *desc, const void *buf, size_t bsize);
 
 /* ------------------------------------------------------------------------- */
 /* Spartan-II Generic Implementation */
-static int spartan3_load(xilinx_desc *desc, const void *buf, size_t bsize,
+static int spartan3_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
 			 bitstream_type bstype)
 {
 	int ret_val = FPGA_FAIL;
+	xilinx_desc *desc = *desc_ptr;
 
 	switch (desc->iface) {
 	case slave_serial:
diff --git a/drivers/fpga/versalpl.c b/drivers/fpga/versalpl.c
index c44a7d3455..9464cae22e 100644
--- a/drivers/fpga/versalpl.c
+++ b/drivers/fpga/versalpl.c
@@ -26,7 +26,7 @@ static ulong versal_align_dma_buffer(ulong *buf, u32 len)
 	return (ulong)buf;
 }
 
-static int versal_load(xilinx_desc *desc, const void *buf, size_t bsize,
+static int versal_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
 		       bitstream_type bstype)
 {
 	ulong bin_buf;
diff --git a/drivers/fpga/virtex2.c b/drivers/fpga/virtex2.c
index b3e0537bab..d0369d320b 100644
--- a/drivers/fpga/virtex2.c
+++ b/drivers/fpga/virtex2.c
@@ -93,10 +93,11 @@ static int virtex2_ssm_dump(xilinx_desc *desc, const void *buf, size_t bsize);
 static int virtex2_ss_load(xilinx_desc *desc, const void *buf, size_t bsize);
 static int virtex2_ss_dump(xilinx_desc *desc, const void *buf, size_t bsize);
 
-static int virtex2_load(xilinx_desc *desc, const void *buf, size_t bsize,
+static int virtex2_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
 			bitstream_type bstype)
 {
 	int ret_val = FPGA_FAIL;
+	xilinx_desc *desc = *desc_ptr;
 
 	switch (desc->iface) {
 	case slave_serial:
diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c
index 640baac66e..f89ae8fe10 100644
--- a/drivers/fpga/xilinx.c
+++ b/drivers/fpga/xilinx.c
@@ -153,7 +153,7 @@ int xilinx_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
 		return FPGA_FAIL;
 	}
 
-	return desc->operations->load(desc, buf, bsize, bstype);
+	return desc->operations->load(desc_ptr, buf, bsize, bstype);
 }
 
 #if defined(CONFIG_CMD_FPGA_LOADFS)
diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
index 8ff12bf50a..c7f9f4ae84 100644
--- a/drivers/fpga/zynqmppl.c
+++ b/drivers/fpga/zynqmppl.c
@@ -199,8 +199,8 @@ static int zynqmp_validate_bitstream(xilinx_desc *desc, const void *buf,
 	return 0;
 }
 
-static int zynqmp_load(xilinx_desc *desc, const void *buf, size_t bsize,
-		     bitstream_type bstype)
+static int zynqmp_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
+		       bitstream_type bstype)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(u32, bsizeptr, 1);
 	u32 swap = 0;
@@ -209,6 +209,7 @@ static int zynqmp_load(xilinx_desc *desc, const void *buf, size_t bsize,
 	u32 buf_lo, buf_hi;
 	u32 ret_payload[PAYLOAD_ARG_CNT];
 	bool xilfpga_old = false;
+	xilinx_desc *desc = *desc_ptr;
 
 	if (zynqmp_firmware_version() <= PMUFW_V1_0) {
 		puts("WARN: PMUFW v1.0 or less is detected\n");
diff --git a/drivers/fpga/zynqpl.c b/drivers/fpga/zynqpl.c
index 2de40109a8..c5d9dbcedf 100644
--- a/drivers/fpga/zynqpl.c
+++ b/drivers/fpga/zynqpl.c
@@ -370,11 +370,12 @@ static int zynq_validate_bitstream(xilinx_desc *desc, const void *buf,
 	return 0;
 }
 
-static int zynq_load(xilinx_desc *desc, const void *buf, size_t bsize,
+static int zynq_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
 		     bitstream_type bstype)
 {
 	unsigned long ts; /* Timestamp */
 	u32 isr_status, swap;
+	xilinx_desc *desc = *desc_ptr;
 
 	/*
 	 * send bsize inplace of blocksize as it was not a bitstream
diff --git a/include/xilinx.h b/include/xilinx.h
index 06ecc9a842..a7efe0e876 100644
--- a/include/xilinx.h
+++ b/include/xilinx.h
@@ -48,7 +48,7 @@ typedef struct {		/* typedef xilinx_desc */
 } xilinx_desc;			/* end, typedef xilinx_desc */
 
 struct xilinx_fpga_op {
-	int (*load)(xilinx_desc *desc, const void *buf, size_t bsize,
+	int (*load)(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
 		    bitstream_type bstype);
 	int (*loadfs)(xilinx_desc *desc, const void *buf, size_t bsize,
 		      fpga_fs_info *fpga_fsinfo);
-- 
2.35.1

[PATCH v7 6/7] fpga: zynqmp: support loading authenticated images

[Adrian Fiergolski]

From: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>

Add supporting new compatible string "u-boot,zynqmp-fpga-ddrauth" to
handle loading authenticated images (DDR).

Based on solution by Jorge Ramirez-Ortiz <jorge@foundries.io>
Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
Co-developed-by: Ricardo Salveti <ricardo@foundries.io>
Signed-off-by: Ricardo Salveti <ricardo@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>
---
 boot/Kconfig                          |  4 ++--
 doc/uImage.FIT/source_file_format.txt |  5 ++++-
 drivers/fpga/zynqmppl.c               | 21 +++++++++++++++++++++
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/boot/Kconfig b/boot/Kconfig
index b83a4e8400..f7faafb29f 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -209,8 +209,8 @@ config SPL_LOAD_FIT
 	  1. "loadables" images, other than FDTs, which do not have a "load"
 	     property will not be loaded. This limitation also applies to FPGA
 	     images with the correct "compatible" string.
-	  2. For FPGA images, only the "compatible" = "u-boot,fpga-legacy"
-	     loading method is supported.
+	  2. For FPGA images, the supported "compatible" list is in the
+	     doc/uImage.FIT/source_file_format.txt.
 	  3. FDTs are only loaded for images with an "os" property of "u-boot".
 	     "linux" images are also supported with Falcon boot mode.
 
diff --git a/doc/uImage.FIT/source_file_format.txt b/doc/uImage.FIT/source_file_format.txt
index f93ac6d1c7..461e2af2a8 100644
--- a/doc/uImage.FIT/source_file_format.txt
+++ b/doc/uImage.FIT/source_file_format.txt
@@ -184,7 +184,10 @@ the '/images' node should have the following layout:
     Mandatory for types: "firmware", and "kernel".
   - compatible : compatible method for loading image.
     Mandatory for types: "fpga", and images that do not specify a load address.
-    To use the generic fpga loading routine, use "u-boot,fpga-legacy".
+    Supported compatible methods:
+    "u-boot,fpga-legacy" - the generic fpga loading routine.
+    "u-boot,zynqmp-fpga-ddrauth" - signed non-encrypted FPGA bitstream for
+    Xilinx Zynq UltraScale+ (ZymqMP) device.
 
   Optional nodes:
   - hash-1 : Each hash sub-node represents separate hash or checksum
diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
index c7f9f4ae84..0ce641e495 100644
--- a/drivers/fpga/zynqmppl.c
+++ b/drivers/fpga/zynqmppl.c
@@ -9,6 +9,7 @@
 #include <common.h>
 #include <compiler.h>
 #include <cpu_func.h>
+#include <fpga.h>
 #include <log.h>
 #include <zynqmppl.h>
 #include <zynqmp_firmware.h>
@@ -210,6 +211,26 @@ static int zynqmp_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
 	u32 ret_payload[PAYLOAD_ARG_CNT];
 	bool xilfpga_old = false;
 	xilinx_desc *desc = *desc_ptr;
+	fpga_desc *fdesc = container_of((void *)desc_ptr, fpga_desc, devdesc);
+
+	if (fdesc && fdesc->compatible &&
+	    !strcmp(fdesc->compatible, "u-boot,zynqmp-fpga-ddrauth")) {
+		struct fpga_secure_info info = { 0 };
+
+		if (!CONFIG_IS_ENABLED(FPGA_LOAD_SECURE)) {
+			printf("No support for %s\n", fdesc->compatible);
+			return FPGA_FAIL;
+		}
+
+		if (!desc->operations->loads) {
+			printf("%s: Missing load operation\n", __func__);
+			return FPGA_FAIL;
+		}
+		/* DDR authentication */
+		info.authflag = 1;
+		info.encflag = 2;
+		return desc->operations->loads(desc, buf, bsize, &info);
+	}
 
 	if (zynqmp_firmware_version() <= PMUFW_V1_0) {
 		puts("WARN: PMUFW v1.0 or less is detected\n");
-- 
2.35.1

[PATCH v7 7/7] fpga: zynqmp: support loading encrypted bitfiles

[Adrian Fiergolski]

Add supporting new compatible string "u-boot,zynqmp-fpga-enc" to handle
loading encrypted bitfiles.

This feature requires encrypted FSBL,as according to UG1085:
"The CSU automatically locks out the AES key, stored in either BBRAM or eFUSEs,
 as a key source to the AES engine if the FSBL is not encrypted. This prevents
 using the BBRAM or eFUSE as the key source to the AES engine during run-time
 applications."

Signed-off-and-tested-by: Adrian Fiergolski <adrian.fiergolski@fastree3d.com>
---
 doc/uImage.FIT/source_file_format.txt |  2 ++
 drivers/fpga/zynqmppl.c               | 14 ++++++++++----
 include/fpga.h                        |  1 +
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/doc/uImage.FIT/source_file_format.txt b/doc/uImage.FIT/source_file_format.txt
index 461e2af2a8..6870111840 100644
--- a/doc/uImage.FIT/source_file_format.txt
+++ b/doc/uImage.FIT/source_file_format.txt
@@ -188,6 +188,8 @@ the '/images' node should have the following layout:
     "u-boot,fpga-legacy" - the generic fpga loading routine.
     "u-boot,zynqmp-fpga-ddrauth" - signed non-encrypted FPGA bitstream for
     Xilinx Zynq UltraScale+ (ZymqMP) device.
+    "u-boot,zynqmp-fpga-enc" - encrypted FPGA bitstream for Xilinx Zynq
+    UltraScale+ (ZynqMP) device.
 
   Optional nodes:
   - hash-1 : Each hash sub-node represents separate hash or checksum
diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
index 0ce641e495..1090734936 100644
--- a/drivers/fpga/zynqmppl.c
+++ b/drivers/fpga/zynqmppl.c
@@ -214,7 +214,7 @@ static int zynqmp_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
 	fpga_desc *fdesc = container_of((void *)desc_ptr, fpga_desc, devdesc);
 
 	if (fdesc && fdesc->compatible &&
-	    !strcmp(fdesc->compatible, "u-boot,zynqmp-fpga-ddrauth")) {
+	    strncmp(fdesc->compatible, "u-boot,fpga-legacy", 18)) {
 		struct fpga_secure_info info = { 0 };
 
 		if (!CONFIG_IS_ENABLED(FPGA_LOAD_SECURE)) {
@@ -226,9 +226,15 @@ static int zynqmp_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
 			printf("%s: Missing load operation\n", __func__);
 			return FPGA_FAIL;
 		}
-		/* DDR authentication */
-		info.authflag = 1;
-		info.encflag = 2;
+		if (!strncmp(fdesc->compatible+19, "enc", 3)) {
+		  /* Encryption using device key */
+		  info.authflag = FPGA_NO_ENC_OR_NO_AUTH;
+		  info.encflag = FPGA_ENC_DEV_KEY;
+		} else {
+		  /* DDR authentication */
+		  info.authflag = ZYNQMP_FPGA_AUTH_DDR;
+		  info.encflag = FPGA_NO_ENC_OR_NO_AUTH;
+		}
 		return desc->operations->loads(desc, buf, bsize, &info);
 	}
 
diff --git a/include/fpga.h b/include/fpga.h
index 2891f32106..cdf80fedf6 100644
--- a/include/fpga.h
+++ b/include/fpga.h
@@ -20,6 +20,7 @@
 /* device numbers must be non-negative */
 #define FPGA_INVALID_DEVICE	-1
 
+#define FPGA_ENC_DEV_KEY	0
 #define FPGA_ENC_USR_KEY	1
 #define FPGA_NO_ENC_OR_NO_AUTH	2
 
-- 
2.35.1

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

[Michal Simek]

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

Re: [PATCH v7 4/7] fpga: xilinx: add missed identifier names

[Michal Simek]

On 4/11/22 20:00, Adrian Fiergolski wrote:
> From: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> 
> Function definition arguments should also have identifier names.
> Add missed ones to struct xilinx_fpga_op callbacks, unifying code.
> 
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>

missing your sob line here.

M

> ---
>   include/xilinx.h | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/include/xilinx.h b/include/xilinx.h
> index 57b0e7be11..06ecc9a842 100644
> --- a/include/xilinx.h
> +++ b/include/xilinx.h
> @@ -48,12 +48,14 @@ typedef struct {		/* typedef xilinx_desc */
>   } xilinx_desc;			/* end, typedef xilinx_desc */
>   
>   struct xilinx_fpga_op {
> -	int (*load)(xilinx_desc *, const void *, size_t, bitstream_type);
> -	int (*loadfs)(xilinx_desc *, const void *, size_t, fpga_fs_info *);
> +	int (*load)(xilinx_desc *desc, const void *buf, size_t bsize,
> +		    bitstream_type bstype);
> +	int (*loadfs)(xilinx_desc *desc, const void *buf, size_t bsize,
> +		      fpga_fs_info *fpga_fsinfo);
>   	int (*loads)(xilinx_desc *desc, const void *buf, size_t bsize,
>   		     struct fpga_secure_info *fpga_sec_info);
> -	int (*dump)(xilinx_desc *, const void *, size_t);
> -	int (*info)(xilinx_desc *);
> +	int (*dump)(xilinx_desc *desc, const void *buf, size_t bsize);
> +	int (*info)(xilinx_desc *desc);
>   };
>   
>   /* Generic Xilinx Functions

Re: [PATCH v7 5/7] fpga: xilinx: pass xilinx_desc pointer address into load() ops

[Michal Simek]

On 4/11/22 20:00, Adrian Fiergolski wrote:
> From: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> 
> Pass an address of xilinx_desc pointer in an fpga_desc into a load()
> callback of struct xilinx_fpga_op.
> It allows getting parent fpga_desc structure members inside xilinx
> fpga drivers.
> 
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> Tested-by: Ricardo Salveti <ricardo@foundries.io>

missing your sob line here.

M

Re: [PATCH v7 6/7] fpga: zynqmp: support loading authenticated images

[Michal Simek]

On 4/11/22 20:00, Adrian Fiergolski wrote:
> From: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> 
> Add supporting new compatible string "u-boot,zynqmp-fpga-ddrauth" to
> handle loading authenticated images (DDR).
> 
> Based on solution by Jorge Ramirez-Ortiz <jorge@foundries.io>
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> Co-developed-by: Ricardo Salveti <ricardo@foundries.io>
> Signed-off-by: Ricardo Salveti <ricardo@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>
> ---
>   boot/Kconfig                          |  4 ++--
>   doc/uImage.FIT/source_file_format.txt |  5 ++++-
>   drivers/fpga/zynqmppl.c               | 21 +++++++++++++++++++++
>   3 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/boot/Kconfig b/boot/Kconfig
> index b83a4e8400..f7faafb29f 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -209,8 +209,8 @@ config SPL_LOAD_FIT
>   	  1. "loadables" images, other than FDTs, which do not have a "load"
>   	     property will not be loaded. This limitation also applies to FPGA
>   	     images with the correct "compatible" string.
> -	  2. For FPGA images, only the "compatible" = "u-boot,fpga-legacy"
> -	     loading method is supported.
> +	  2. For FPGA images, the supported "compatible" list is in the
> +	     doc/uImage.FIT/source_file_format.txt.
>   	  3. FDTs are only loaded for images with an "os" property of "u-boot".
>   	     "linux" images are also supported with Falcon boot mode.
>   
> diff --git a/doc/uImage.FIT/source_file_format.txt b/doc/uImage.FIT/source_file_format.txt
> index f93ac6d1c7..461e2af2a8 100644
> --- a/doc/uImage.FIT/source_file_format.txt
> +++ b/doc/uImage.FIT/source_file_format.txt
> @@ -184,7 +184,10 @@ the '/images' node should have the following layout:
>       Mandatory for types: "firmware", and "kernel".
>     - compatible : compatible method for loading image.
>       Mandatory for types: "fpga", and images that do not specify a load address.
> -    To use the generic fpga loading routine, use "u-boot,fpga-legacy".
> +    Supported compatible methods:
> +    "u-boot,fpga-legacy" - the generic fpga loading routine.
> +    "u-boot,zynqmp-fpga-ddrauth" - signed non-encrypted FPGA bitstream for
> +    Xilinx Zynq UltraScale+ (ZymqMP) device.
>   
>     Optional nodes:
>     - hash-1 : Each hash sub-node represents separate hash or checksum
> diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
> index c7f9f4ae84..0ce641e495 100644
> --- a/drivers/fpga/zynqmppl.c
> +++ b/drivers/fpga/zynqmppl.c
> @@ -9,6 +9,7 @@
>   #include <common.h>
>   #include <compiler.h>
>   #include <cpu_func.h>
> +#include <fpga.h>
>   #include <log.h>
>   #include <zynqmppl.h>
>   #include <zynqmp_firmware.h>
> @@ -210,6 +211,26 @@ static int zynqmp_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
>   	u32 ret_payload[PAYLOAD_ARG_CNT];
>   	bool xilfpga_old = false;
>   	xilinx_desc *desc = *desc_ptr;
> +	fpga_desc *fdesc = container_of((void *)desc_ptr, fpga_desc, devdesc);
> +
> +	if (fdesc && fdesc->compatible &&
> +	    !strcmp(fdesc->compatible, "u-boot,zynqmp-fpga-ddrauth")) {

I think you should use directly here what you have in 7/7. It means to check 
that it is not fpga-legacy.

> +		struct fpga_secure_info info = { 0 };
> +
> +		if (!CONFIG_IS_ENABLED(FPGA_LOAD_SECURE)) {
> +			printf("No support for %s\n", fdesc->compatible);
> +			return FPGA_FAIL;
> +		}
> +
> +		if (!desc->operations->loads) {
> +			printf("%s: Missing load operation\n", __func__);
> +			return FPGA_FAIL;
> +		}
> +		/* DDR authentication */
> +		info.authflag = 1;
> +		info.encflag = 2;
> +		return desc->operations->loads(desc, buf, bsize, &info);
> +	}
>   
>   	if (zynqmp_firmware_version() <= PMUFW_V1_0) {
>   		puts("WARN: PMUFW v1.0 or less is detected\n");

Before you start to deal with secure bitstreams you should also likely check 
this PMUFW checking before you call loads.

Thanks,
Michal

Re: [PATCH v7 0/7] fpga: zynqmp: Adding support of loading authenticated images

[Michal Simek]

On 4/11/22 20:00, Adrian Fiergolski wrote:
> This patchset introduces support for the authenticated FPGA images
> on ZynqMP boards, besides that introducing common way to pass the
> compatible property to any fpga driver.
> 
> It bases on the initial work by Jorge Ramirez-Ortiz <jorge@foundries.io>
> https://patchwork.ozlabs.org/project/uboot/patch/20211015091506.2602-1-jorge@foundries.io/
> https://patchwork.ozlabs.org/project/uboot/patch/20211005111324.19749-3-jorge@foundries.io/
> 
> Changed in v7:
> - apply Michal Simek's suggestions
>    As I applied changes on Oleksandr's patches, I indicated it by specifying myself as co-author
>    in the commits logs. I am not sure if that is the convention of marking it.
> 
> Changed in v6:
> - add support for the encrypted bitfiles
> 
> Changes in v5:
> - replace ifdef with if() where it's possible
> 
> Changes in v4:
> - change interface to xilinx_desc->operations->open() callback.
> - fix a bug from previous version of the patchset in dereferencing
>    of a parent fpga_desc structure.
> 
> Changes in v3:
> - remove the patch which introduced CMD_SPL_FPGA_LOAD_SECURE.
> - fix mixing definitions/declarations.
> - replace strcmp() calls with more secure strncmp().
> - document the "u-boot,zynqmp-fpga-ddrauth" compatible string.
> - fix code style by check-patch recommendations.
> 
> Changes in v2:
> - add function fit_fpga_load() to simplify calls of fpga_load()
>    from contexts without a compatible attribute.
> - move all ZynqMP-specific logic to drivers/fpga/zynqmppl.c
> - prepare for passing a "compatible" FDT property to any fpga driver.
> 
> Oleksandr Suvorov (6):
>    fpga: add option for loading FPGA secure bitstreams
>    fpga: add fit_fpga_load function
>    fpga: xilinx: pass an address of xilinx_desc in fpga_desc
>    fpga: xilinx: add missed identifier names
>    fpga: xilinx: pass xilinx_desc pointer address into load() ops
>    fpga: zynqmp: support loading authenticated images
> 
> Adrian Fiergolski (1):
>    fpga: zynqmp: support loading encrypted bitfiles
> 
>   boot/Kconfig                          |  4 +--
>   cmd/Kconfig                           |  3 ++-
>   common/spl/spl_fit.c                  |  6 ++---
>   doc/uImage.FIT/source_file_format.txt |  7 +++++-
>   drivers/fpga/Kconfig                  | 14 +++++++++++
>   drivers/fpga/fpga.c                   | 29 ++++++++++++++++++---
>   drivers/fpga/spartan2.c               |  3 ++-
>   drivers/fpga/spartan3.c               |  3 ++-
>   drivers/fpga/versalpl.c               |  2 +-
>   drivers/fpga/virtex2.c                |  3 ++-
>   drivers/fpga/xilinx.c                 |  8 +++---
>   drivers/fpga/zynqmppl.c               | 36 ++++++++++++++++++++++++---
>   drivers/fpga/zynqpl.c                 |  3 ++-
>   include/fpga.h                        |  5 ++++
>   include/xilinx.h                      | 12 +++++----
>   15 files changed, 109 insertions(+), 29 deletions(-)
> 

Thanks for continuing on this work. We are almost there. Just small things to 
finish and we are ready to go.

Thanks,
Michal

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

[Adrian Fiergolski]

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

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

[Oleksandr Suvorov]

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



-- 
Best regards
Oleksandr

Oleksandr Suvorov
cryosay@gmail.com

Re: [PATCH v7 6/7] fpga: zynqmp: support loading authenticated images

[Oleksandr Suvorov]

Hi Michal,

On Tue, May 3, 2022 at 10:56 AM Michal Simek <michal.simek@xilinx.com> wrote:
>
>
>
> On 4/11/22 20:00, Adrian Fiergolski wrote:
> > From: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> >
> > Add supporting new compatible string "u-boot,zynqmp-fpga-ddrauth" to
> > handle loading authenticated images (DDR).
> >
> > Based on solution by Jorge Ramirez-Ortiz <jorge@foundries.io>
> > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> > Co-developed-by: Ricardo Salveti <ricardo@foundries.io>
> > Signed-off-by: Ricardo Salveti <ricardo@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>
> > ---
> >   boot/Kconfig                          |  4 ++--
> >   doc/uImage.FIT/source_file_format.txt |  5 ++++-
> >   drivers/fpga/zynqmppl.c               | 21 +++++++++++++++++++++
> >   3 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/boot/Kconfig b/boot/Kconfig
> > index b83a4e8400..f7faafb29f 100644
> > --- a/boot/Kconfig
> > +++ b/boot/Kconfig
> > @@ -209,8 +209,8 @@ config SPL_LOAD_FIT
> >         1. "loadables" images, other than FDTs, which do not have a "load"
> >            property will not be loaded. This limitation also applies to FPGA
> >            images with the correct "compatible" string.
> > -       2. For FPGA images, only the "compatible" = "u-boot,fpga-legacy"
> > -          loading method is supported.
> > +       2. For FPGA images, the supported "compatible" list is in the
> > +          doc/uImage.FIT/source_file_format.txt.
> >         3. FDTs are only loaded for images with an "os" property of "u-boot".
> >            "linux" images are also supported with Falcon boot mode.
> >
> > diff --git a/doc/uImage.FIT/source_file_format.txt b/doc/uImage.FIT/source_file_format.txt
> > index f93ac6d1c7..461e2af2a8 100644
> > --- a/doc/uImage.FIT/source_file_format.txt
> > +++ b/doc/uImage.FIT/source_file_format.txt
> > @@ -184,7 +184,10 @@ the '/images' node should have the following layout:
> >       Mandatory for types: "firmware", and "kernel".
> >     - compatible : compatible method for loading image.
> >       Mandatory for types: "fpga", and images that do not specify a load address.
> > -    To use the generic fpga loading routine, use "u-boot,fpga-legacy".
> > +    Supported compatible methods:
> > +    "u-boot,fpga-legacy" - the generic fpga loading routine.
> > +    "u-boot,zynqmp-fpga-ddrauth" - signed non-encrypted FPGA bitstream for
> > +    Xilinx Zynq UltraScale+ (ZymqMP) device.
> >
> >     Optional nodes:
> >     - hash-1 : Each hash sub-node represents separate hash or checksum
> > diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
> > index c7f9f4ae84..0ce641e495 100644
> > --- a/drivers/fpga/zynqmppl.c
> > +++ b/drivers/fpga/zynqmppl.c
> > @@ -9,6 +9,7 @@
> >   #include <common.h>
> >   #include <compiler.h>
> >   #include <cpu_func.h>
> > +#include <fpga.h>
> >   #include <log.h>
> >   #include <zynqmppl.h>
> >   #include <zynqmp_firmware.h>
> > @@ -210,6 +211,26 @@ static int zynqmp_load(xilinx_desc **desc_ptr, const void *buf, size_t bsize,
> >       u32 ret_payload[PAYLOAD_ARG_CNT];
> >       bool xilfpga_old = false;
> >       xilinx_desc *desc = *desc_ptr;
> > +     fpga_desc *fdesc = container_of((void *)desc_ptr, fpga_desc, devdesc);
> > +
> > +     if (fdesc && fdesc->compatible &&
> > +         !strcmp(fdesc->compatible, "u-boot,zynqmp-fpga-ddrauth")) {
>
> I think you should use directly here what you have in 7/7. It means to check
> that it is not fpga-legacy.

Thanks, fixed. The fix will be introduced in the next patchset version.

>
> > +             struct fpga_secure_info info = { 0 };
> > +
> > +             if (!CONFIG_IS_ENABLED(FPGA_LOAD_SECURE)) {
> > +                     printf("No support for %s\n", fdesc->compatible);
> > +                     return FPGA_FAIL;
> > +             }
> > +
> > +             if (!desc->operations->loads) {
> > +                     printf("%s: Missing load operation\n", __func__);
> > +                     return FPGA_FAIL;
> > +             }
> > +             /* DDR authentication */
> > +             info.authflag = 1;
> > +             info.encflag = 2;
> > +             return desc->operations->loads(desc, buf, bsize, &info);
> > +     }
> >
> >       if (zynqmp_firmware_version() <= PMUFW_V1_0) {
> >               puts("WARN: PMUFW v1.0 or less is detected\n");
>
> Before you start to deal with secure bitstreams you should also likely check
> this PMUFW checking before you call loads.

Michal, this code block (for earlier PMUFW) does:

1. Set a flag ZYNQMP_FPGA_BIT_NS.
According to a description of the commit
19ed4b697b9732e0a5097bd233fba7e24dfe9146, ZYNQMP_FPGA_BIT_NS
bit should be set only for nonsecure bitstream. So not needed for
zynqmp_loads().

2. Prepare a pointer to a bitstream size (bsizeptr) instead of value (bsize).
For secure bitstream, a key address is already used instead.

3. Validate a bitstream data and placement with zynqmp_validate_bitstream().
I've not found enough information about validating non-secure/secure
bitstreams. Could you confirm the function zynqmp_validate_bitstream()
can be used to validate both legacy and secure bitstreams?

Thanks,
Oleksandr.

>
> Thanks,
> Michal



-- 
Best regards
Oleksandr

Oleksandr Suvorov
cryosay@gmail.com

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

[Adrian Fiergolski]

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

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

[Oleksandr Suvorov]

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

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

[Adrian Fiergolski]

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

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

[Oleksandr Suvorov]

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
>
-- 
Best regards
Oleksandr

Oleksandr Suvorov
cryosay@gmail.com

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

[Oleksandr Suvorov]

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



-- 
Best regards
Oleksandr

Oleksandr Suvorov
cryosay@gmail.com

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

[Adrian Fiergolski]

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

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

[Michal Simek]

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

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

[Oleksandr Suvorov]

./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

-- 
Best regards
Oleksandr

Oleksandr Suvorov
cryosay@gmail.com

Re: [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@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
>
>
>


-- 
Best regards
Oleksandr

Oleksandr Suvorov
cryosay@gmail.com