[PATCH v5 0/4] generalize fpga_mgr_load

[PATCH v5 0/4] generalize fpga_mgr_load

[trix]

From: Tom Rix <trix@redhat.com>

Depends on
https://lore.kernel.org/linux-fpga/20210625195148.837230-9-trix@redhat.com/

A refactor of the fpga-manager to make space for the
functionality of the secure update in this thread.
https://lore.kernel.org/linux-fpga/20210517023200.52707-1-mdf@kernel.org/T/#mf3a1951d429a973c863eee079ed990c55056827c

Splits the reconfig write ops into its own ops structure and
then has an instance for the existing loading (reconfig) and the
secure update (reimage)

fpga_mgr_load uses a new bit, FPGA_MGR_REIMAGE, in fpga_info_info
to use either the reconfig or the reimage ops.

valid write op checking has moved to make the reimage path option.

Since fpga_image_info_alloc zalloc's the fpga_info_struct, the
reimage path will not be taken.

Changes since v1:
- update op names changed from
  partial_update to reconfig
  full_update to reimage
- dropped the cancel() and get_error() ops.
- add FPGA_MGR_REIMAGE bit
- refactor fpga_mgr_load to use either update ops

Changes since v2:
- Fix a missed write op change
- Stub in dfl reimage

Changes since v3
- refactor for wrapper ops patchset
- drop 0004 fpga: defer checking.., wrapper ops took care of that
- drop 0006 fpga: dfl stub in..., simplify the patchset
- add a wrapper for write_sg
- rearrange the passing of update ops to be last.
- simplify some wrapper checks, this should go in the wrapper ps.

Changes since v4
- refactor for wrapper ops patchset
- drop write_sg wrapper
- drop 'fpga: fpga-mgr: simplify mops check in wrappers'


Tom Rix (4):
  fpga: generalize updating the card
  fpga: add FPGA_MGR_REIMAGE flag
  fpga: pass fpga_manager_update_ops to the fpga_manager_write functions
  fpga: use reimage ops in fpga_mgr_load()

 drivers/fpga/altera-cvp.c        |  8 +--
 drivers/fpga/altera-pr-ip-core.c |  8 +--
 drivers/fpga/altera-ps-spi.c     |  8 +--
 drivers/fpga/dfl-fme-mgr.c       |  8 +--
 drivers/fpga/fpga-mgr.c          | 95 +++++++++++++++++++-------------
 drivers/fpga/ice40-spi.c         |  8 +--
 drivers/fpga/machxo2-spi.c       |  8 +--
 drivers/fpga/socfpga-a10.c       | 10 ++--
 drivers/fpga/socfpga.c           |  8 +--
 drivers/fpga/stratix10-soc.c     |  6 +-
 drivers/fpga/ts73xx-fpga.c       |  6 +-
 drivers/fpga/xilinx-spi.c        |  8 +--
 drivers/fpga/zynq-fpga.c         | 10 ++--
 drivers/fpga/zynqmp-fpga.c       |  6 +-
 include/linux/fpga/fpga-mgr.h    | 35 ++++++++----
 15 files changed, 132 insertions(+), 100 deletions(-)

-- 
2.26.3

[PATCH v5 1/4] fpga: generalize updating the card

[trix]

From: Tom Rix <trix@redhat.com>

There is a need to update the whole card.  An fpga can
contain non-fpga components whose firmware needs to be
updated at the same time as the fpga rtl images and
may need to be handled differently from the existing
fpga reconfiguration in the fpga manager.

Move the write_* ops out of fpga_manager_ops and
into a new fpga_manager_update_ops struct.  Add
two update_ops back to fpga_manager_ops,
reconfig for the exiting functionality and
reimage for the new functionity.

Rewire fpga devs to use reconfig ops

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/fpga/altera-cvp.c        |  8 ++++----
 drivers/fpga/altera-pr-ip-core.c |  8 ++++----
 drivers/fpga/altera-ps-spi.c     |  8 ++++----
 drivers/fpga/dfl-fme-mgr.c       |  8 ++++----
 drivers/fpga/fpga-mgr.c          | 20 ++++++++++----------
 drivers/fpga/ice40-spi.c         |  8 ++++----
 drivers/fpga/machxo2-spi.c       |  8 ++++----
 drivers/fpga/socfpga-a10.c       | 10 +++++-----
 drivers/fpga/socfpga.c           |  8 ++++----
 drivers/fpga/stratix10-soc.c     |  6 +++---
 drivers/fpga/ts73xx-fpga.c       |  6 +++---
 drivers/fpga/xilinx-spi.c        |  8 ++++----
 drivers/fpga/zynq-fpga.c         | 10 +++++-----
 drivers/fpga/zynqmp-fpga.c       |  6 +++---
 include/linux/fpga/fpga-mgr.h    | 32 +++++++++++++++++++++-----------
 15 files changed, 82 insertions(+), 72 deletions(-)

diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
index ccf4546eff297..9363353798bc9 100644
--- a/drivers/fpga/altera-cvp.c
+++ b/drivers/fpga/altera-cvp.c
@@ -516,10 +516,10 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
 }
 
 static const struct fpga_manager_ops altera_cvp_ops = {
-	.state		= altera_cvp_state,
-	.write_init	= altera_cvp_write_init,
-	.write		= altera_cvp_write,
-	.write_complete	= altera_cvp_write_complete,
+	.state                   = altera_cvp_state,
+	.reconfig.write_init     = altera_cvp_write_init,
+	.reconfig.write          = altera_cvp_write,
+	.reconfig.write_complete = altera_cvp_write_complete,
 };
 
 static const struct cvp_priv cvp_priv_v1 = {
diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-core.c
index dfdf21ed34c4e..30c7b534df95b 100644
--- a/drivers/fpga/altera-pr-ip-core.c
+++ b/drivers/fpga/altera-pr-ip-core.c
@@ -167,10 +167,10 @@ static int alt_pr_fpga_write_complete(struct fpga_manager *mgr,
 }
 
 static const struct fpga_manager_ops alt_pr_ops = {
-	.state = alt_pr_fpga_state,
-	.write_init = alt_pr_fpga_write_init,
-	.write = alt_pr_fpga_write,
-	.write_complete = alt_pr_fpga_write_complete,
+	.state                   = alt_pr_fpga_state,
+	.reconfig.write_init     = alt_pr_fpga_write_init,
+	.reconfig.write          = alt_pr_fpga_write,
+	.reconfig.write_complete = alt_pr_fpga_write_complete,
 };
 
 int alt_pr_register(struct device *dev, void __iomem *reg_base)
diff --git a/drivers/fpga/altera-ps-spi.c b/drivers/fpga/altera-ps-spi.c
index 23bfd4d1ad0f7..2b01a3c53d374 100644
--- a/drivers/fpga/altera-ps-spi.c
+++ b/drivers/fpga/altera-ps-spi.c
@@ -231,10 +231,10 @@ static int altera_ps_write_complete(struct fpga_manager *mgr,
 }
 
 static const struct fpga_manager_ops altera_ps_ops = {
-	.state = altera_ps_state,
-	.write_init = altera_ps_write_init,
-	.write = altera_ps_write,
-	.write_complete = altera_ps_write_complete,
+	.state                   = altera_ps_state,
+	.reconfig.write_init     = altera_ps_write_init,
+	.reconfig.write          = altera_ps_write,
+	.reconfig.write_complete = altera_ps_write_complete,
 };
 
 static const struct altera_ps_data *id_to_data(const struct spi_device_id *id)
diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
index 313420405d5e8..a6d2ed399e580 100644
--- a/drivers/fpga/dfl-fme-mgr.c
+++ b/drivers/fpga/dfl-fme-mgr.c
@@ -260,10 +260,10 @@ static u64 fme_mgr_status(struct fpga_manager *mgr)
 }
 
 static const struct fpga_manager_ops fme_mgr_ops = {
-	.write_init = fme_mgr_write_init,
-	.write = fme_mgr_write,
-	.write_complete = fme_mgr_write_complete,
-	.status = fme_mgr_status,
+	.status                  = fme_mgr_status,
+	.reconfig.write_init     = fme_mgr_write_init,
+	.reconfig.write          = fme_mgr_write,
+	.reconfig.write_complete = fme_mgr_write_complete,
 };
 
 static void fme_mgr_get_compat_id(void __iomem *fme_pr,
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index aa30889e23208..31c51d7e07cc8 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -47,8 +47,8 @@ static inline u64 fpga_mgr_status(struct fpga_manager *mgr)
 
 static inline int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
 {
-	if (mgr->mops->write)
-		return  mgr->mops->write(mgr, buf, count);
+	if (mgr->mops->reconfig.write)
+		return  mgr->mops->reconfig.write(mgr, buf, count);
 	return -EOPNOTSUPP;
 }
 
@@ -62,8 +62,8 @@ static inline int fpga_mgr_write_complete(struct fpga_manager *mgr,
 	int ret = 0;
 
 	mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
-	if (mgr->mops->write_complete)
-		ret = mgr->mops->write_complete(mgr, info);
+	if (mgr->mops->reconfig.write_complete)
+		ret = mgr->mops->reconfig.write_complete(mgr, info);
 	if (ret) {
 		dev_err(&mgr->dev, "Error after writing image data to FPGA\n");
 		mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
@@ -78,16 +78,16 @@ static inline int fpga_mgr_write_init(struct fpga_manager *mgr,
 				      struct fpga_image_info *info,
 				      const char *buf, size_t count)
 {
-	if (mgr->mops->write_init)
-		return  mgr->mops->write_init(mgr, info, buf, count);
+	if (mgr->mops->reconfig.write_init)
+		return  mgr->mops->reconfig.write_init(mgr, info, buf, count);
 	return 0;
 }
 
 static inline int fpga_mgr_write_sg(struct fpga_manager *mgr,
 				    struct sg_table *sgt)
 {
-	if (mgr->mops->write_sg)
-		return  mgr->mops->write_sg(mgr, sgt);
+	if (mgr->mops->reconfig.write_sg)
+		return  mgr->mops->reconfig.write_sg(mgr, sgt);
 	return -EOPNOTSUPP;
 }
 
@@ -232,7 +232,7 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
 
 	/* Write the FPGA image to the FPGA. */
 	mgr->state = FPGA_MGR_STATE_WRITE;
-	if (mgr->mops->write_sg) {
+	if (mgr->mops->reconfig.write_sg) {
 		ret = fpga_mgr_write_sg(mgr, sgt);
 	} else {
 		struct sg_mapping_iter miter;
@@ -309,7 +309,7 @@ static int fpga_mgr_buf_load(struct fpga_manager *mgr,
 	 * contiguous kernel buffer and the driver doesn't require SG, non-SG
 	 * drivers will still work on the slow path.
 	 */
-	if (mgr->mops->write)
+	if (mgr->mops->reconfig.write)
 		return fpga_mgr_buf_load_mapped(mgr, info, buf, count);
 
 	/*
diff --git a/drivers/fpga/ice40-spi.c b/drivers/fpga/ice40-spi.c
index 69dec5af23c36..3bdc3fe8ece97 100644
--- a/drivers/fpga/ice40-spi.c
+++ b/drivers/fpga/ice40-spi.c
@@ -126,10 +126,10 @@ static int ice40_fpga_ops_write_complete(struct fpga_manager *mgr,
 }
 
 static const struct fpga_manager_ops ice40_fpga_ops = {
-	.state = ice40_fpga_ops_state,
-	.write_init = ice40_fpga_ops_write_init,
-	.write = ice40_fpga_ops_write,
-	.write_complete = ice40_fpga_ops_write_complete,
+	.state                   = ice40_fpga_ops_state,
+	.reconfig.write_init     = ice40_fpga_ops_write_init,
+	.reconfig.write          = ice40_fpga_ops_write,
+	.reconfig.write_complete = ice40_fpga_ops_write_complete,
 };
 
 static int ice40_fpga_probe(struct spi_device *spi)
diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
index 114a64d2b7a4d..8b860e9a19c92 100644
--- a/drivers/fpga/machxo2-spi.c
+++ b/drivers/fpga/machxo2-spi.c
@@ -350,10 +350,10 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
 }
 
 static const struct fpga_manager_ops machxo2_ops = {
-	.state = machxo2_spi_state,
-	.write_init = machxo2_write_init,
-	.write = machxo2_write,
-	.write_complete = machxo2_write_complete,
+	.state                   = machxo2_spi_state,
+	.reconfig.write_init     = machxo2_write_init,
+	.reconfig.write          = machxo2_write,
+	.reconfig.write_complete = machxo2_write_complete,
 };
 
 static int machxo2_spi_probe(struct spi_device *spi)
diff --git a/drivers/fpga/socfpga-a10.c b/drivers/fpga/socfpga-a10.c
index 573d88bdf7307..e60bf844b4c40 100644
--- a/drivers/fpga/socfpga-a10.c
+++ b/drivers/fpga/socfpga-a10.c
@@ -458,11 +458,11 @@ static enum fpga_mgr_states socfpga_a10_fpga_state(struct fpga_manager *mgr)
 }
 
 static const struct fpga_manager_ops socfpga_a10_fpga_mgr_ops = {
-	.initial_header_size = (RBF_DECOMPRESS_OFFSET + 1) * 4,
-	.state = socfpga_a10_fpga_state,
-	.write_init = socfpga_a10_fpga_write_init,
-	.write = socfpga_a10_fpga_write,
-	.write_complete = socfpga_a10_fpga_write_complete,
+	.initial_header_size     = (RBF_DECOMPRESS_OFFSET + 1) * 4,
+	.state                   = socfpga_a10_fpga_state,
+	.reconfig.write_init     = socfpga_a10_fpga_write_init,
+	.reconfig.write          = socfpga_a10_fpga_write,
+	.reconfig.write_complete = socfpga_a10_fpga_write_complete,
 };
 
 static int socfpga_a10_fpga_probe(struct platform_device *pdev)
diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
index 1f467173fc1f3..cc752a3f742c2 100644
--- a/drivers/fpga/socfpga.c
+++ b/drivers/fpga/socfpga.c
@@ -534,10 +534,10 @@ static enum fpga_mgr_states socfpga_fpga_ops_state(struct fpga_manager *mgr)
 }
 
 static const struct fpga_manager_ops socfpga_fpga_ops = {
-	.state = socfpga_fpga_ops_state,
-	.write_init = socfpga_fpga_ops_configure_init,
-	.write = socfpga_fpga_ops_configure_write,
-	.write_complete = socfpga_fpga_ops_configure_complete,
+	.state                   = socfpga_fpga_ops_state,
+	.reconfig.write_init     = socfpga_fpga_ops_configure_init,
+	.reconfig.write          = socfpga_fpga_ops_configure_write,
+	.reconfig.write_complete = socfpga_fpga_ops_configure_complete,
 };
 
 static int socfpga_fpga_probe(struct platform_device *pdev)
diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
index 047fd7f237069..ab1941d92cf60 100644
--- a/drivers/fpga/stratix10-soc.c
+++ b/drivers/fpga/stratix10-soc.c
@@ -389,9 +389,9 @@ static int s10_ops_write_complete(struct fpga_manager *mgr,
 }
 
 static const struct fpga_manager_ops s10_ops = {
-	.write_init = s10_ops_write_init,
-	.write = s10_ops_write,
-	.write_complete = s10_ops_write_complete,
+	.reconfig.write_init     = s10_ops_write_init,
+	.reconfig.write          = s10_ops_write,
+	.reconfig.write_complete = s10_ops_write_complete,
 };
 
 static int s10_probe(struct platform_device *pdev)
diff --git a/drivers/fpga/ts73xx-fpga.c b/drivers/fpga/ts73xx-fpga.c
index 167abb0b08d40..cbbc6dec56856 100644
--- a/drivers/fpga/ts73xx-fpga.c
+++ b/drivers/fpga/ts73xx-fpga.c
@@ -93,9 +93,9 @@ static int ts73xx_fpga_write_complete(struct fpga_manager *mgr,
 }
 
 static const struct fpga_manager_ops ts73xx_fpga_ops = {
-	.write_init	= ts73xx_fpga_write_init,
-	.write		= ts73xx_fpga_write,
-	.write_complete	= ts73xx_fpga_write_complete,
+	.reconfig.write_init     = ts73xx_fpga_write_init,
+	.reconfig.write          = ts73xx_fpga_write,
+	.reconfig.write_complete = ts73xx_fpga_write_complete,
 };
 
 static int ts73xx_fpga_probe(struct platform_device *pdev)
diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index fee4d0abf6bfe..4d092f30bf700 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -214,10 +214,10 @@ static int xilinx_spi_write_complete(struct fpga_manager *mgr,
 }
 
 static const struct fpga_manager_ops xilinx_spi_ops = {
-	.state = xilinx_spi_state,
-	.write_init = xilinx_spi_write_init,
-	.write = xilinx_spi_write,
-	.write_complete = xilinx_spi_write_complete,
+	.state                   = xilinx_spi_state,
+	.reconfig.write_init     = xilinx_spi_write_init,
+	.reconfig.write          = xilinx_spi_write,
+	.reconfig.write_complete = xilinx_spi_write_complete,
 };
 
 static int xilinx_spi_probe(struct spi_device *spi)
diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 9b75bd4f93d8e..bdfc257740cff 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -543,11 +543,11 @@ static enum fpga_mgr_states zynq_fpga_ops_state(struct fpga_manager *mgr)
 }
 
 static const struct fpga_manager_ops zynq_fpga_ops = {
-	.initial_header_size = 128,
-	.state = zynq_fpga_ops_state,
-	.write_init = zynq_fpga_ops_write_init,
-	.write_sg = zynq_fpga_ops_write,
-	.write_complete = zynq_fpga_ops_write_complete,
+	.initial_header_size     = 128,
+	.state                   = zynq_fpga_ops_state,
+	.reconfig.write_init     = zynq_fpga_ops_write_init,
+	.reconfig.write_sg       = zynq_fpga_ops_write,
+	.reconfig.write_complete = zynq_fpga_ops_write_complete,
 };
 
 static int zynq_fpga_probe(struct platform_device *pdev)
diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
index 9efbd70aa6864..fbb66c1f9c871 100644
--- a/drivers/fpga/zynqmp-fpga.c
+++ b/drivers/fpga/zynqmp-fpga.c
@@ -78,9 +78,9 @@ static enum fpga_mgr_states zynqmp_fpga_ops_state(struct fpga_manager *mgr)
 }
 
 static const struct fpga_manager_ops zynqmp_fpga_ops = {
-	.state = zynqmp_fpga_ops_state,
-	.write_init = zynqmp_fpga_ops_write_init,
-	.write = zynqmp_fpga_ops_write,
+	.state                   = zynqmp_fpga_ops_state,
+	.reconfig.write_init     = zynqmp_fpga_ops_write_init,
+	.reconfig.write          = zynqmp_fpga_ops_write,
 };
 
 static int zynqmp_fpga_probe(struct platform_device *pdev)
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 474c1f5063070..53f9402d6aa17 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -106,14 +106,29 @@ struct fpga_image_info {
 };
 
 /**
- * struct fpga_manager_ops - ops for low level fpga manager drivers
- * @initial_header_size: Maximum number of bytes that should be passed into write_init
- * @state: returns an enum value of the FPGA's state
- * @status: returns status of the FPGA, including reconfiguration error code
+ * struct fpga_manager_update_ops - ops updating fpga
  * @write_init: prepare the FPGA to receive configuration data
  * @write: write count bytes of configuration data to the FPGA
  * @write_sg: write the scatter list of configuration data to the FPGA
  * @write_complete: set FPGA to operating state after writing is done
+ */
+struct fpga_manager_update_ops {
+	int (*write_init)(struct fpga_manager *mgr,
+			  struct fpga_image_info *info,
+			  const char *buf, size_t count);
+	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
+	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
+	int (*write_complete)(struct fpga_manager *mgr,
+			      struct fpga_image_info *info);
+};
+
+/**
+ * struct fpga_manager_ops - ops for low level fpga manager drivers
+ * @initial_header_size: Maximum number of bytes that should be passed into write_init
+ * @state: returns an enum value of the FPGA's state
+ * @status: returns status of the FPGA, including reconfiguration error code
+ * @partial_update: ops for doing partial reconfiguration
+ * @full_update: ops for doing a full card update, user,shell,fw ie. the works
  * @fpga_remove: optional: Set FPGA into a specific state during driver remove
  * @groups: optional attribute groups.
  *
@@ -125,13 +140,8 @@ struct fpga_manager_ops {
 	size_t initial_header_size;
 	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
 	u64 (*status)(struct fpga_manager *mgr);
-	int (*write_init)(struct fpga_manager *mgr,
-			  struct fpga_image_info *info,
-			  const char *buf, size_t count);
-	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
-	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
-	int (*write_complete)(struct fpga_manager *mgr,
-			      struct fpga_image_info *info);
+	struct fpga_manager_update_ops reconfig;
+	struct fpga_manager_update_ops reimage;
 	void (*fpga_remove)(struct fpga_manager *mgr);
 	const struct attribute_group **groups;
 };
-- 
2.26.3

Re: [PATCH v5 1/4] fpga: generalize updating the card

[Moritz Fischer]

On Fri, Jun 25, 2021 at 12:58:46PM -0700, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> There is a need to update the whole card.  An fpga can

Not sure I'd use the 'card' language. Not every FPGA sits on a plugin
card / board.

I'd suggest to frame the description around persistent vs
non-persistent.

> contain non-fpga components whose firmware needs to be
> updated at the same time as the fpga rtl images and
> may need to be handled differently from the existing
> fpga reconfiguration in the fpga manager.

Updating images beyond FPGA images is beyond the scope of the FPGA
Manager framework.
> 
> Move the write_* ops out of fpga_manager_ops and
> into a new fpga_manager_update_ops struct.  Add
> two update_ops back to fpga_manager_ops,
> reconfig for the exiting functionality and
> reimage for the new functionity.
> 
> Rewire fpga devs to use reconfig ops
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/fpga/altera-cvp.c        |  8 ++++----
>  drivers/fpga/altera-pr-ip-core.c |  8 ++++----
>  drivers/fpga/altera-ps-spi.c     |  8 ++++----
>  drivers/fpga/dfl-fme-mgr.c       |  8 ++++----
>  drivers/fpga/fpga-mgr.c          | 20 ++++++++++----------
>  drivers/fpga/ice40-spi.c         |  8 ++++----
>  drivers/fpga/machxo2-spi.c       |  8 ++++----
>  drivers/fpga/socfpga-a10.c       | 10 +++++-----
>  drivers/fpga/socfpga.c           |  8 ++++----
>  drivers/fpga/stratix10-soc.c     |  6 +++---
>  drivers/fpga/ts73xx-fpga.c       |  6 +++---
>  drivers/fpga/xilinx-spi.c        |  8 ++++----
>  drivers/fpga/zynq-fpga.c         | 10 +++++-----
>  drivers/fpga/zynqmp-fpga.c       |  6 +++---
>  include/linux/fpga/fpga-mgr.h    | 32 +++++++++++++++++++++-----------
>  15 files changed, 82 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index ccf4546eff297..9363353798bc9 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -516,10 +516,10 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
>  }
>  
>  static const struct fpga_manager_ops altera_cvp_ops = {
> -	.state		= altera_cvp_state,
> -	.write_init	= altera_cvp_write_init,
> -	.write		= altera_cvp_write,
> -	.write_complete	= altera_cvp_write_complete,
> +	.state                   = altera_cvp_state,
> +	.reconfig.write_init     = altera_cvp_write_init,
> +	.reconfig.write          = altera_cvp_write,
> +	.reconfig.write_complete = altera_cvp_write_complete,
>  };
>  
>  static const struct cvp_priv cvp_priv_v1 = {
> diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-core.c
> index dfdf21ed34c4e..30c7b534df95b 100644
> --- a/drivers/fpga/altera-pr-ip-core.c
> +++ b/drivers/fpga/altera-pr-ip-core.c
> @@ -167,10 +167,10 @@ static int alt_pr_fpga_write_complete(struct fpga_manager *mgr,
>  }
>  
>  static const struct fpga_manager_ops alt_pr_ops = {
> -	.state = alt_pr_fpga_state,
> -	.write_init = alt_pr_fpga_write_init,
> -	.write = alt_pr_fpga_write,
> -	.write_complete = alt_pr_fpga_write_complete,
> +	.state                   = alt_pr_fpga_state,
> +	.reconfig.write_init     = alt_pr_fpga_write_init,
> +	.reconfig.write          = alt_pr_fpga_write,
> +	.reconfig.write_complete = alt_pr_fpga_write_complete,
>  };
>  
>  int alt_pr_register(struct device *dev, void __iomem *reg_base)
> diff --git a/drivers/fpga/altera-ps-spi.c b/drivers/fpga/altera-ps-spi.c
> index 23bfd4d1ad0f7..2b01a3c53d374 100644
> --- a/drivers/fpga/altera-ps-spi.c
> +++ b/drivers/fpga/altera-ps-spi.c
> @@ -231,10 +231,10 @@ static int altera_ps_write_complete(struct fpga_manager *mgr,
>  }
>  
>  static const struct fpga_manager_ops altera_ps_ops = {
> -	.state = altera_ps_state,
> -	.write_init = altera_ps_write_init,
> -	.write = altera_ps_write,
> -	.write_complete = altera_ps_write_complete,
> +	.state                   = altera_ps_state,
> +	.reconfig.write_init     = altera_ps_write_init,
> +	.reconfig.write          = altera_ps_write,
> +	.reconfig.write_complete = altera_ps_write_complete,
>  };
>  
>  static const struct altera_ps_data *id_to_data(const struct spi_device_id *id)
> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> index 313420405d5e8..a6d2ed399e580 100644
> --- a/drivers/fpga/dfl-fme-mgr.c
> +++ b/drivers/fpga/dfl-fme-mgr.c
> @@ -260,10 +260,10 @@ static u64 fme_mgr_status(struct fpga_manager *mgr)
>  }
>  
>  static const struct fpga_manager_ops fme_mgr_ops = {
> -	.write_init = fme_mgr_write_init,
> -	.write = fme_mgr_write,
> -	.write_complete = fme_mgr_write_complete,
> -	.status = fme_mgr_status,
> +	.status                  = fme_mgr_status,
> +	.reconfig.write_init     = fme_mgr_write_init,
> +	.reconfig.write          = fme_mgr_write,
> +	.reconfig.write_complete = fme_mgr_write_complete,
>  };
>  
>  static void fme_mgr_get_compat_id(void __iomem *fme_pr,
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index aa30889e23208..31c51d7e07cc8 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -47,8 +47,8 @@ static inline u64 fpga_mgr_status(struct fpga_manager *mgr)
>  
>  static inline int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
>  {
> -	if (mgr->mops->write)
> -		return  mgr->mops->write(mgr, buf, count);
> +	if (mgr->mops->reconfig.write)
> +		return  mgr->mops->reconfig.write(mgr, buf, count);
>  	return -EOPNOTSUPP;
>  }
>  
> @@ -62,8 +62,8 @@ static inline int fpga_mgr_write_complete(struct fpga_manager *mgr,
>  	int ret = 0;
>  
>  	mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
> -	if (mgr->mops->write_complete)
> -		ret = mgr->mops->write_complete(mgr, info);
> +	if (mgr->mops->reconfig.write_complete)
> +		ret = mgr->mops->reconfig.write_complete(mgr, info);
>  	if (ret) {
>  		dev_err(&mgr->dev, "Error after writing image data to FPGA\n");
>  		mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
> @@ -78,16 +78,16 @@ static inline int fpga_mgr_write_init(struct fpga_manager *mgr,
>  				      struct fpga_image_info *info,
>  				      const char *buf, size_t count)
>  {
> -	if (mgr->mops->write_init)
> -		return  mgr->mops->write_init(mgr, info, buf, count);
> +	if (mgr->mops->reconfig.write_init)
> +		return  mgr->mops->reconfig.write_init(mgr, info, buf, count);
>  	return 0;
>  }
>  
>  static inline int fpga_mgr_write_sg(struct fpga_manager *mgr,
>  				    struct sg_table *sgt)
>  {
> -	if (mgr->mops->write_sg)
> -		return  mgr->mops->write_sg(mgr, sgt);
> +	if (mgr->mops->reconfig.write_sg)
> +		return  mgr->mops->reconfig.write_sg(mgr, sgt);
>  	return -EOPNOTSUPP;
>  }
>  
> @@ -232,7 +232,7 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
>  
>  	/* Write the FPGA image to the FPGA. */
>  	mgr->state = FPGA_MGR_STATE_WRITE;
> -	if (mgr->mops->write_sg) {
> +	if (mgr->mops->reconfig.write_sg) {
>  		ret = fpga_mgr_write_sg(mgr, sgt);
>  	} else {
>  		struct sg_mapping_iter miter;
> @@ -309,7 +309,7 @@ static int fpga_mgr_buf_load(struct fpga_manager *mgr,
>  	 * contiguous kernel buffer and the driver doesn't require SG, non-SG
>  	 * drivers will still work on the slow path.
>  	 */
> -	if (mgr->mops->write)
> +	if (mgr->mops->reconfig.write)
>  		return fpga_mgr_buf_load_mapped(mgr, info, buf, count);
>  
>  	/*
> diff --git a/drivers/fpga/ice40-spi.c b/drivers/fpga/ice40-spi.c
> index 69dec5af23c36..3bdc3fe8ece97 100644
> --- a/drivers/fpga/ice40-spi.c
> +++ b/drivers/fpga/ice40-spi.c
> @@ -126,10 +126,10 @@ static int ice40_fpga_ops_write_complete(struct fpga_manager *mgr,
>  }
>  
>  static const struct fpga_manager_ops ice40_fpga_ops = {
> -	.state = ice40_fpga_ops_state,
> -	.write_init = ice40_fpga_ops_write_init,
> -	.write = ice40_fpga_ops_write,
> -	.write_complete = ice40_fpga_ops_write_complete,
> +	.state                   = ice40_fpga_ops_state,
> +	.reconfig.write_init     = ice40_fpga_ops_write_init,
> +	.reconfig.write          = ice40_fpga_ops_write,
> +	.reconfig.write_complete = ice40_fpga_ops_write_complete,
>  };
>  
>  static int ice40_fpga_probe(struct spi_device *spi)
> diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
> index 114a64d2b7a4d..8b860e9a19c92 100644
> --- a/drivers/fpga/machxo2-spi.c
> +++ b/drivers/fpga/machxo2-spi.c
> @@ -350,10 +350,10 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
>  }
>  
>  static const struct fpga_manager_ops machxo2_ops = {
> -	.state = machxo2_spi_state,
> -	.write_init = machxo2_write_init,
> -	.write = machxo2_write,
> -	.write_complete = machxo2_write_complete,
> +	.state                   = machxo2_spi_state,
> +	.reconfig.write_init     = machxo2_write_init,
> +	.reconfig.write          = machxo2_write,
> +	.reconfig.write_complete = machxo2_write_complete,
>  };
>  
>  static int machxo2_spi_probe(struct spi_device *spi)
> diff --git a/drivers/fpga/socfpga-a10.c b/drivers/fpga/socfpga-a10.c
> index 573d88bdf7307..e60bf844b4c40 100644
> --- a/drivers/fpga/socfpga-a10.c
> +++ b/drivers/fpga/socfpga-a10.c
> @@ -458,11 +458,11 @@ static enum fpga_mgr_states socfpga_a10_fpga_state(struct fpga_manager *mgr)
>  }
>  
>  static const struct fpga_manager_ops socfpga_a10_fpga_mgr_ops = {
> -	.initial_header_size = (RBF_DECOMPRESS_OFFSET + 1) * 4,
> -	.state = socfpga_a10_fpga_state,
> -	.write_init = socfpga_a10_fpga_write_init,
> -	.write = socfpga_a10_fpga_write,
> -	.write_complete = socfpga_a10_fpga_write_complete,
> +	.initial_header_size     = (RBF_DECOMPRESS_OFFSET + 1) * 4,
> +	.state                   = socfpga_a10_fpga_state,
> +	.reconfig.write_init     = socfpga_a10_fpga_write_init,
> +	.reconfig.write          = socfpga_a10_fpga_write,
> +	.reconfig.write_complete = socfpga_a10_fpga_write_complete,
>  };
>  
>  static int socfpga_a10_fpga_probe(struct platform_device *pdev)
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> index 1f467173fc1f3..cc752a3f742c2 100644
> --- a/drivers/fpga/socfpga.c
> +++ b/drivers/fpga/socfpga.c
> @@ -534,10 +534,10 @@ static enum fpga_mgr_states socfpga_fpga_ops_state(struct fpga_manager *mgr)
>  }
>  
>  static const struct fpga_manager_ops socfpga_fpga_ops = {
> -	.state = socfpga_fpga_ops_state,
> -	.write_init = socfpga_fpga_ops_configure_init,
> -	.write = socfpga_fpga_ops_configure_write,
> -	.write_complete = socfpga_fpga_ops_configure_complete,
> +	.state                   = socfpga_fpga_ops_state,
> +	.reconfig.write_init     = socfpga_fpga_ops_configure_init,
> +	.reconfig.write          = socfpga_fpga_ops_configure_write,
> +	.reconfig.write_complete = socfpga_fpga_ops_configure_complete,
>  };
>  
>  static int socfpga_fpga_probe(struct platform_device *pdev)
> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> index 047fd7f237069..ab1941d92cf60 100644
> --- a/drivers/fpga/stratix10-soc.c
> +++ b/drivers/fpga/stratix10-soc.c
> @@ -389,9 +389,9 @@ static int s10_ops_write_complete(struct fpga_manager *mgr,
>  }
>  
>  static const struct fpga_manager_ops s10_ops = {
> -	.write_init = s10_ops_write_init,
> -	.write = s10_ops_write,
> -	.write_complete = s10_ops_write_complete,
> +	.reconfig.write_init     = s10_ops_write_init,
> +	.reconfig.write          = s10_ops_write,
> +	.reconfig.write_complete = s10_ops_write_complete,
>  };
>  
>  static int s10_probe(struct platform_device *pdev)
> diff --git a/drivers/fpga/ts73xx-fpga.c b/drivers/fpga/ts73xx-fpga.c
> index 167abb0b08d40..cbbc6dec56856 100644
> --- a/drivers/fpga/ts73xx-fpga.c
> +++ b/drivers/fpga/ts73xx-fpga.c
> @@ -93,9 +93,9 @@ static int ts73xx_fpga_write_complete(struct fpga_manager *mgr,
>  }
>  
>  static const struct fpga_manager_ops ts73xx_fpga_ops = {
> -	.write_init	= ts73xx_fpga_write_init,
> -	.write		= ts73xx_fpga_write,
> -	.write_complete	= ts73xx_fpga_write_complete,
> +	.reconfig.write_init     = ts73xx_fpga_write_init,
> +	.reconfig.write          = ts73xx_fpga_write,
> +	.reconfig.write_complete = ts73xx_fpga_write_complete,
>  };
>  
>  static int ts73xx_fpga_probe(struct platform_device *pdev)
> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
> index fee4d0abf6bfe..4d092f30bf700 100644
> --- a/drivers/fpga/xilinx-spi.c
> +++ b/drivers/fpga/xilinx-spi.c
> @@ -214,10 +214,10 @@ static int xilinx_spi_write_complete(struct fpga_manager *mgr,
>  }
>  
>  static const struct fpga_manager_ops xilinx_spi_ops = {
> -	.state = xilinx_spi_state,
> -	.write_init = xilinx_spi_write_init,
> -	.write = xilinx_spi_write,
> -	.write_complete = xilinx_spi_write_complete,
> +	.state                   = xilinx_spi_state,
> +	.reconfig.write_init     = xilinx_spi_write_init,
> +	.reconfig.write          = xilinx_spi_write,
> +	.reconfig.write_complete = xilinx_spi_write_complete,
>  };
>  
>  static int xilinx_spi_probe(struct spi_device *spi)
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index 9b75bd4f93d8e..bdfc257740cff 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -543,11 +543,11 @@ static enum fpga_mgr_states zynq_fpga_ops_state(struct fpga_manager *mgr)
>  }
>  
>  static const struct fpga_manager_ops zynq_fpga_ops = {
> -	.initial_header_size = 128,
> -	.state = zynq_fpga_ops_state,
> -	.write_init = zynq_fpga_ops_write_init,
> -	.write_sg = zynq_fpga_ops_write,
> -	.write_complete = zynq_fpga_ops_write_complete,
> +	.initial_header_size     = 128,
> +	.state                   = zynq_fpga_ops_state,
> +	.reconfig.write_init     = zynq_fpga_ops_write_init,
> +	.reconfig.write_sg       = zynq_fpga_ops_write,
> +	.reconfig.write_complete = zynq_fpga_ops_write_complete,
>  };
>  
>  static int zynq_fpga_probe(struct platform_device *pdev)
> diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
> index 9efbd70aa6864..fbb66c1f9c871 100644
> --- a/drivers/fpga/zynqmp-fpga.c
> +++ b/drivers/fpga/zynqmp-fpga.c
> @@ -78,9 +78,9 @@ static enum fpga_mgr_states zynqmp_fpga_ops_state(struct fpga_manager *mgr)
>  }
>  
>  static const struct fpga_manager_ops zynqmp_fpga_ops = {
> -	.state = zynqmp_fpga_ops_state,
> -	.write_init = zynqmp_fpga_ops_write_init,
> -	.write = zynqmp_fpga_ops_write,
> +	.state                   = zynqmp_fpga_ops_state,
> +	.reconfig.write_init     = zynqmp_fpga_ops_write_init,
> +	.reconfig.write          = zynqmp_fpga_ops_write,
>  };
>  
>  static int zynqmp_fpga_probe(struct platform_device *pdev)
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 474c1f5063070..53f9402d6aa17 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -106,14 +106,29 @@ struct fpga_image_info {
>  };
>  
>  /**
> - * struct fpga_manager_ops - ops for low level fpga manager drivers
> - * @initial_header_size: Maximum number of bytes that should be passed into write_init
> - * @state: returns an enum value of the FPGA's state
> - * @status: returns status of the FPGA, including reconfiguration error code
> + * struct fpga_manager_update_ops - ops updating fpga
>   * @write_init: prepare the FPGA to receive configuration data
>   * @write: write count bytes of configuration data to the FPGA
>   * @write_sg: write the scatter list of configuration data to the FPGA
>   * @write_complete: set FPGA to operating state after writing is done
> + */
> +struct fpga_manager_update_ops {
> +	int (*write_init)(struct fpga_manager *mgr,
> +			  struct fpga_image_info *info,
> +			  const char *buf, size_t count);
> +	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
> +	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
> +	int (*write_complete)(struct fpga_manager *mgr,
> +			      struct fpga_image_info *info);
> +};
> +
> +/**
> + * struct fpga_manager_ops - ops for low level fpga manager drivers
> + * @initial_header_size: Maximum number of bytes that should be passed into write_init
> + * @state: returns an enum value of the FPGA's state
> + * @status: returns status of the FPGA, including reconfiguration error code
> + * @partial_update: ops for doing partial reconfiguration
> + * @full_update: ops for doing a full card update, user,shell,fw ie. the works
>   * @fpga_remove: optional: Set FPGA into a specific state during driver remove
>   * @groups: optional attribute groups.
>   *
> @@ -125,13 +140,8 @@ struct fpga_manager_ops {
>  	size_t initial_header_size;
>  	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
>  	u64 (*status)(struct fpga_manager *mgr);
> -	int (*write_init)(struct fpga_manager *mgr,
> -			  struct fpga_image_info *info,
> -			  const char *buf, size_t count);
> -	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
> -	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
> -	int (*write_complete)(struct fpga_manager *mgr,
> -			      struct fpga_image_info *info);
> +	struct fpga_manager_update_ops reconfig;
> +	struct fpga_manager_update_ops reimage;
>  	void (*fpga_remove)(struct fpga_manager *mgr);
>  	const struct attribute_group **groups;
>  };
> -- 
> 2.26.3
> 

Thanks,
Moritz

Re: [PATCH v5 1/4] fpga: generalize updating the card

[Tom Rix]

On 6/28/21 11:44 AM, Moritz Fischer wrote:
> On Fri, Jun 25, 2021 at 12:58:46PM -0700, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> There is a need to update the whole card.  An fpga can
> Not sure I'd use the 'card' language. Not every FPGA sits on a plugin
> card / board.
>
> I'd suggest to frame the description around persistent vs
> non-persistent.
>
>> contain non-fpga components whose firmware needs to be
>> updated at the same time as the fpga rtl images and
>> may need to be handled differently from the existing
>> fpga reconfiguration in the fpga manager.
> Updating images beyond FPGA images is beyond the scope of the FPGA
> Manager framework.

This patchset is a followup for the sec-mgr patchset, seeing if 
generalizing fpga-mgr makes sense.

If it doesn't, that is fine we can drop this.


I believe a contentious part of sec-mgr patchset is its a single user 
feature being implemented as a general subsystem feature.

If generalizing fpga-mgr is out, the options as I see it are.

keep sec-mgr structure as-is.

move card updating to fw subsystem

have a dfl-sec-update.* dfl specific interface in fpga/ .

Tom


>> Move the write_* ops out of fpga_manager_ops and
>> into a new fpga_manager_update_ops struct.  Add
>> two update_ops back to fpga_manager_ops,
>> reconfig for the exiting functionality and
>> reimage for the new functionity.
>>
>> Rewire fpga devs to use reconfig ops
>>
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>>   drivers/fpga/altera-cvp.c        |  8 ++++----
>>   drivers/fpga/altera-pr-ip-core.c |  8 ++++----
>>   drivers/fpga/altera-ps-spi.c     |  8 ++++----
>>   drivers/fpga/dfl-fme-mgr.c       |  8 ++++----
>>   drivers/fpga/fpga-mgr.c          | 20 ++++++++++----------
>>   drivers/fpga/ice40-spi.c         |  8 ++++----
>>   drivers/fpga/machxo2-spi.c       |  8 ++++----
>>   drivers/fpga/socfpga-a10.c       | 10 +++++-----
>>   drivers/fpga/socfpga.c           |  8 ++++----
>>   drivers/fpga/stratix10-soc.c     |  6 +++---
>>   drivers/fpga/ts73xx-fpga.c       |  6 +++---
>>   drivers/fpga/xilinx-spi.c        |  8 ++++----
>>   drivers/fpga/zynq-fpga.c         | 10 +++++-----
>>   drivers/fpga/zynqmp-fpga.c       |  6 +++---
>>   include/linux/fpga/fpga-mgr.h    | 32 +++++++++++++++++++++-----------
>>   15 files changed, 82 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
>> index ccf4546eff297..9363353798bc9 100644
>> --- a/drivers/fpga/altera-cvp.c
>> +++ b/drivers/fpga/altera-cvp.c
>> @@ -516,10 +516,10 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
>>   }
>>   
>>   static const struct fpga_manager_ops altera_cvp_ops = {
>> -	.state		= altera_cvp_state,
>> -	.write_init	= altera_cvp_write_init,
>> -	.write		= altera_cvp_write,
>> -	.write_complete	= altera_cvp_write_complete,
>> +	.state                   = altera_cvp_state,
>> +	.reconfig.write_init     = altera_cvp_write_init,
>> +	.reconfig.write          = altera_cvp_write,
>> +	.reconfig.write_complete = altera_cvp_write_complete,
>>   };
>>   
>>   static const struct cvp_priv cvp_priv_v1 = {
>> diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-core.c
>> index dfdf21ed34c4e..30c7b534df95b 100644
>> --- a/drivers/fpga/altera-pr-ip-core.c
>> +++ b/drivers/fpga/altera-pr-ip-core.c
>> @@ -167,10 +167,10 @@ static int alt_pr_fpga_write_complete(struct fpga_manager *mgr,
>>   }
>>   
>>   static const struct fpga_manager_ops alt_pr_ops = {
>> -	.state = alt_pr_fpga_state,
>> -	.write_init = alt_pr_fpga_write_init,
>> -	.write = alt_pr_fpga_write,
>> -	.write_complete = alt_pr_fpga_write_complete,
>> +	.state                   = alt_pr_fpga_state,
>> +	.reconfig.write_init     = alt_pr_fpga_write_init,
>> +	.reconfig.write          = alt_pr_fpga_write,
>> +	.reconfig.write_complete = alt_pr_fpga_write_complete,
>>   };
>>   
>>   int alt_pr_register(struct device *dev, void __iomem *reg_base)
>> diff --git a/drivers/fpga/altera-ps-spi.c b/drivers/fpga/altera-ps-spi.c
>> index 23bfd4d1ad0f7..2b01a3c53d374 100644
>> --- a/drivers/fpga/altera-ps-spi.c
>> +++ b/drivers/fpga/altera-ps-spi.c
>> @@ -231,10 +231,10 @@ static int altera_ps_write_complete(struct fpga_manager *mgr,
>>   }
>>   
>>   static const struct fpga_manager_ops altera_ps_ops = {
>> -	.state = altera_ps_state,
>> -	.write_init = altera_ps_write_init,
>> -	.write = altera_ps_write,
>> -	.write_complete = altera_ps_write_complete,
>> +	.state                   = altera_ps_state,
>> +	.reconfig.write_init     = altera_ps_write_init,
>> +	.reconfig.write          = altera_ps_write,
>> +	.reconfig.write_complete = altera_ps_write_complete,
>>   };
>>   
>>   static const struct altera_ps_data *id_to_data(const struct spi_device_id *id)
>> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
>> index 313420405d5e8..a6d2ed399e580 100644
>> --- a/drivers/fpga/dfl-fme-mgr.c
>> +++ b/drivers/fpga/dfl-fme-mgr.c
>> @@ -260,10 +260,10 @@ static u64 fme_mgr_status(struct fpga_manager *mgr)
>>   }
>>   
>>   static const struct fpga_manager_ops fme_mgr_ops = {
>> -	.write_init = fme_mgr_write_init,
>> -	.write = fme_mgr_write,
>> -	.write_complete = fme_mgr_write_complete,
>> -	.status = fme_mgr_status,
>> +	.status                  = fme_mgr_status,
>> +	.reconfig.write_init     = fme_mgr_write_init,
>> +	.reconfig.write          = fme_mgr_write,
>> +	.reconfig.write_complete = fme_mgr_write_complete,
>>   };
>>   
>>   static void fme_mgr_get_compat_id(void __iomem *fme_pr,
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index aa30889e23208..31c51d7e07cc8 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -47,8 +47,8 @@ static inline u64 fpga_mgr_status(struct fpga_manager *mgr)
>>   
>>   static inline int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
>>   {
>> -	if (mgr->mops->write)
>> -		return  mgr->mops->write(mgr, buf, count);
>> +	if (mgr->mops->reconfig.write)
>> +		return  mgr->mops->reconfig.write(mgr, buf, count);
>>   	return -EOPNOTSUPP;
>>   }
>>   
>> @@ -62,8 +62,8 @@ static inline int fpga_mgr_write_complete(struct fpga_manager *mgr,
>>   	int ret = 0;
>>   
>>   	mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
>> -	if (mgr->mops->write_complete)
>> -		ret = mgr->mops->write_complete(mgr, info);
>> +	if (mgr->mops->reconfig.write_complete)
>> +		ret = mgr->mops->reconfig.write_complete(mgr, info);
>>   	if (ret) {
>>   		dev_err(&mgr->dev, "Error after writing image data to FPGA\n");
>>   		mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
>> @@ -78,16 +78,16 @@ static inline int fpga_mgr_write_init(struct fpga_manager *mgr,
>>   				      struct fpga_image_info *info,
>>   				      const char *buf, size_t count)
>>   {
>> -	if (mgr->mops->write_init)
>> -		return  mgr->mops->write_init(mgr, info, buf, count);
>> +	if (mgr->mops->reconfig.write_init)
>> +		return  mgr->mops->reconfig.write_init(mgr, info, buf, count);
>>   	return 0;
>>   }
>>   
>>   static inline int fpga_mgr_write_sg(struct fpga_manager *mgr,
>>   				    struct sg_table *sgt)
>>   {
>> -	if (mgr->mops->write_sg)
>> -		return  mgr->mops->write_sg(mgr, sgt);
>> +	if (mgr->mops->reconfig.write_sg)
>> +		return  mgr->mops->reconfig.write_sg(mgr, sgt);
>>   	return -EOPNOTSUPP;
>>   }
>>   
>> @@ -232,7 +232,7 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
>>   
>>   	/* Write the FPGA image to the FPGA. */
>>   	mgr->state = FPGA_MGR_STATE_WRITE;
>> -	if (mgr->mops->write_sg) {
>> +	if (mgr->mops->reconfig.write_sg) {
>>   		ret = fpga_mgr_write_sg(mgr, sgt);
>>   	} else {
>>   		struct sg_mapping_iter miter;
>> @@ -309,7 +309,7 @@ static int fpga_mgr_buf_load(struct fpga_manager *mgr,
>>   	 * contiguous kernel buffer and the driver doesn't require SG, non-SG
>>   	 * drivers will still work on the slow path.
>>   	 */
>> -	if (mgr->mops->write)
>> +	if (mgr->mops->reconfig.write)
>>   		return fpga_mgr_buf_load_mapped(mgr, info, buf, count);
>>   
>>   	/*
>> diff --git a/drivers/fpga/ice40-spi.c b/drivers/fpga/ice40-spi.c
>> index 69dec5af23c36..3bdc3fe8ece97 100644
>> --- a/drivers/fpga/ice40-spi.c
>> +++ b/drivers/fpga/ice40-spi.c
>> @@ -126,10 +126,10 @@ static int ice40_fpga_ops_write_complete(struct fpga_manager *mgr,
>>   }
>>   
>>   static const struct fpga_manager_ops ice40_fpga_ops = {
>> -	.state = ice40_fpga_ops_state,
>> -	.write_init = ice40_fpga_ops_write_init,
>> -	.write = ice40_fpga_ops_write,
>> -	.write_complete = ice40_fpga_ops_write_complete,
>> +	.state                   = ice40_fpga_ops_state,
>> +	.reconfig.write_init     = ice40_fpga_ops_write_init,
>> +	.reconfig.write          = ice40_fpga_ops_write,
>> +	.reconfig.write_complete = ice40_fpga_ops_write_complete,
>>   };
>>   
>>   static int ice40_fpga_probe(struct spi_device *spi)
>> diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
>> index 114a64d2b7a4d..8b860e9a19c92 100644
>> --- a/drivers/fpga/machxo2-spi.c
>> +++ b/drivers/fpga/machxo2-spi.c
>> @@ -350,10 +350,10 @@ static int machxo2_write_complete(struct fpga_manager *mgr,
>>   }
>>   
>>   static const struct fpga_manager_ops machxo2_ops = {
>> -	.state = machxo2_spi_state,
>> -	.write_init = machxo2_write_init,
>> -	.write = machxo2_write,
>> -	.write_complete = machxo2_write_complete,
>> +	.state                   = machxo2_spi_state,
>> +	.reconfig.write_init     = machxo2_write_init,
>> +	.reconfig.write          = machxo2_write,
>> +	.reconfig.write_complete = machxo2_write_complete,
>>   };
>>   
>>   static int machxo2_spi_probe(struct spi_device *spi)
>> diff --git a/drivers/fpga/socfpga-a10.c b/drivers/fpga/socfpga-a10.c
>> index 573d88bdf7307..e60bf844b4c40 100644
>> --- a/drivers/fpga/socfpga-a10.c
>> +++ b/drivers/fpga/socfpga-a10.c
>> @@ -458,11 +458,11 @@ static enum fpga_mgr_states socfpga_a10_fpga_state(struct fpga_manager *mgr)
>>   }
>>   
>>   static const struct fpga_manager_ops socfpga_a10_fpga_mgr_ops = {
>> -	.initial_header_size = (RBF_DECOMPRESS_OFFSET + 1) * 4,
>> -	.state = socfpga_a10_fpga_state,
>> -	.write_init = socfpga_a10_fpga_write_init,
>> -	.write = socfpga_a10_fpga_write,
>> -	.write_complete = socfpga_a10_fpga_write_complete,
>> +	.initial_header_size     = (RBF_DECOMPRESS_OFFSET + 1) * 4,
>> +	.state                   = socfpga_a10_fpga_state,
>> +	.reconfig.write_init     = socfpga_a10_fpga_write_init,
>> +	.reconfig.write          = socfpga_a10_fpga_write,
>> +	.reconfig.write_complete = socfpga_a10_fpga_write_complete,
>>   };
>>   
>>   static int socfpga_a10_fpga_probe(struct platform_device *pdev)
>> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
>> index 1f467173fc1f3..cc752a3f742c2 100644
>> --- a/drivers/fpga/socfpga.c
>> +++ b/drivers/fpga/socfpga.c
>> @@ -534,10 +534,10 @@ static enum fpga_mgr_states socfpga_fpga_ops_state(struct fpga_manager *mgr)
>>   }
>>   
>>   static const struct fpga_manager_ops socfpga_fpga_ops = {
>> -	.state = socfpga_fpga_ops_state,
>> -	.write_init = socfpga_fpga_ops_configure_init,
>> -	.write = socfpga_fpga_ops_configure_write,
>> -	.write_complete = socfpga_fpga_ops_configure_complete,
>> +	.state                   = socfpga_fpga_ops_state,
>> +	.reconfig.write_init     = socfpga_fpga_ops_configure_init,
>> +	.reconfig.write          = socfpga_fpga_ops_configure_write,
>> +	.reconfig.write_complete = socfpga_fpga_ops_configure_complete,
>>   };
>>   
>>   static int socfpga_fpga_probe(struct platform_device *pdev)
>> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
>> index 047fd7f237069..ab1941d92cf60 100644
>> --- a/drivers/fpga/stratix10-soc.c
>> +++ b/drivers/fpga/stratix10-soc.c
>> @@ -389,9 +389,9 @@ static int s10_ops_write_complete(struct fpga_manager *mgr,
>>   }
>>   
>>   static const struct fpga_manager_ops s10_ops = {
>> -	.write_init = s10_ops_write_init,
>> -	.write = s10_ops_write,
>> -	.write_complete = s10_ops_write_complete,
>> +	.reconfig.write_init     = s10_ops_write_init,
>> +	.reconfig.write          = s10_ops_write,
>> +	.reconfig.write_complete = s10_ops_write_complete,
>>   };
>>   
>>   static int s10_probe(struct platform_device *pdev)
>> diff --git a/drivers/fpga/ts73xx-fpga.c b/drivers/fpga/ts73xx-fpga.c
>> index 167abb0b08d40..cbbc6dec56856 100644
>> --- a/drivers/fpga/ts73xx-fpga.c
>> +++ b/drivers/fpga/ts73xx-fpga.c
>> @@ -93,9 +93,9 @@ static int ts73xx_fpga_write_complete(struct fpga_manager *mgr,
>>   }
>>   
>>   static const struct fpga_manager_ops ts73xx_fpga_ops = {
>> -	.write_init	= ts73xx_fpga_write_init,
>> -	.write		= ts73xx_fpga_write,
>> -	.write_complete	= ts73xx_fpga_write_complete,
>> +	.reconfig.write_init     = ts73xx_fpga_write_init,
>> +	.reconfig.write          = ts73xx_fpga_write,
>> +	.reconfig.write_complete = ts73xx_fpga_write_complete,
>>   };
>>   
>>   static int ts73xx_fpga_probe(struct platform_device *pdev)
>> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
>> index fee4d0abf6bfe..4d092f30bf700 100644
>> --- a/drivers/fpga/xilinx-spi.c
>> +++ b/drivers/fpga/xilinx-spi.c
>> @@ -214,10 +214,10 @@ static int xilinx_spi_write_complete(struct fpga_manager *mgr,
>>   }
>>   
>>   static const struct fpga_manager_ops xilinx_spi_ops = {
>> -	.state = xilinx_spi_state,
>> -	.write_init = xilinx_spi_write_init,
>> -	.write = xilinx_spi_write,
>> -	.write_complete = xilinx_spi_write_complete,
>> +	.state                   = xilinx_spi_state,
>> +	.reconfig.write_init     = xilinx_spi_write_init,
>> +	.reconfig.write          = xilinx_spi_write,
>> +	.reconfig.write_complete = xilinx_spi_write_complete,
>>   };
>>   
>>   static int xilinx_spi_probe(struct spi_device *spi)
>> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
>> index 9b75bd4f93d8e..bdfc257740cff 100644
>> --- a/drivers/fpga/zynq-fpga.c
>> +++ b/drivers/fpga/zynq-fpga.c
>> @@ -543,11 +543,11 @@ static enum fpga_mgr_states zynq_fpga_ops_state(struct fpga_manager *mgr)
>>   }
>>   
>>   static const struct fpga_manager_ops zynq_fpga_ops = {
>> -	.initial_header_size = 128,
>> -	.state = zynq_fpga_ops_state,
>> -	.write_init = zynq_fpga_ops_write_init,
>> -	.write_sg = zynq_fpga_ops_write,
>> -	.write_complete = zynq_fpga_ops_write_complete,
>> +	.initial_header_size     = 128,
>> +	.state                   = zynq_fpga_ops_state,
>> +	.reconfig.write_init     = zynq_fpga_ops_write_init,
>> +	.reconfig.write_sg       = zynq_fpga_ops_write,
>> +	.reconfig.write_complete = zynq_fpga_ops_write_complete,
>>   };
>>   
>>   static int zynq_fpga_probe(struct platform_device *pdev)
>> diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
>> index 9efbd70aa6864..fbb66c1f9c871 100644
>> --- a/drivers/fpga/zynqmp-fpga.c
>> +++ b/drivers/fpga/zynqmp-fpga.c
>> @@ -78,9 +78,9 @@ static enum fpga_mgr_states zynqmp_fpga_ops_state(struct fpga_manager *mgr)
>>   }
>>   
>>   static const struct fpga_manager_ops zynqmp_fpga_ops = {
>> -	.state = zynqmp_fpga_ops_state,
>> -	.write_init = zynqmp_fpga_ops_write_init,
>> -	.write = zynqmp_fpga_ops_write,
>> +	.state                   = zynqmp_fpga_ops_state,
>> +	.reconfig.write_init     = zynqmp_fpga_ops_write_init,
>> +	.reconfig.write          = zynqmp_fpga_ops_write,
>>   };
>>   
>>   static int zynqmp_fpga_probe(struct platform_device *pdev)
>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>> index 474c1f5063070..53f9402d6aa17 100644
>> --- a/include/linux/fpga/fpga-mgr.h
>> +++ b/include/linux/fpga/fpga-mgr.h
>> @@ -106,14 +106,29 @@ struct fpga_image_info {
>>   };
>>   
>>   /**
>> - * struct fpga_manager_ops - ops for low level fpga manager drivers
>> - * @initial_header_size: Maximum number of bytes that should be passed into write_init
>> - * @state: returns an enum value of the FPGA's state
>> - * @status: returns status of the FPGA, including reconfiguration error code
>> + * struct fpga_manager_update_ops - ops updating fpga
>>    * @write_init: prepare the FPGA to receive configuration data
>>    * @write: write count bytes of configuration data to the FPGA
>>    * @write_sg: write the scatter list of configuration data to the FPGA
>>    * @write_complete: set FPGA to operating state after writing is done
>> + */
>> +struct fpga_manager_update_ops {
>> +	int (*write_init)(struct fpga_manager *mgr,
>> +			  struct fpga_image_info *info,
>> +			  const char *buf, size_t count);
>> +	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
>> +	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
>> +	int (*write_complete)(struct fpga_manager *mgr,
>> +			      struct fpga_image_info *info);
>> +};
>> +
>> +/**
>> + * struct fpga_manager_ops - ops for low level fpga manager drivers
>> + * @initial_header_size: Maximum number of bytes that should be passed into write_init
>> + * @state: returns an enum value of the FPGA's state
>> + * @status: returns status of the FPGA, including reconfiguration error code
>> + * @partial_update: ops for doing partial reconfiguration
>> + * @full_update: ops for doing a full card update, user,shell,fw ie. the works
>>    * @fpga_remove: optional: Set FPGA into a specific state during driver remove
>>    * @groups: optional attribute groups.
>>    *
>> @@ -125,13 +140,8 @@ struct fpga_manager_ops {
>>   	size_t initial_header_size;
>>   	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
>>   	u64 (*status)(struct fpga_manager *mgr);
>> -	int (*write_init)(struct fpga_manager *mgr,
>> -			  struct fpga_image_info *info,
>> -			  const char *buf, size_t count);
>> -	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
>> -	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
>> -	int (*write_complete)(struct fpga_manager *mgr,
>> -			      struct fpga_image_info *info);
>> +	struct fpga_manager_update_ops reconfig;
>> +	struct fpga_manager_update_ops reimage;
>>   	void (*fpga_remove)(struct fpga_manager *mgr);
>>   	const struct attribute_group **groups;
>>   };
>> -- 
>> 2.26.3
>>
> Thanks,
> Moritz
>

[PATCH v5 2/4] fpga: add FPGA_MGR_REIMAGE flag

[trix]

From: Tom Rix <trix@redhat.com>

If this flag is set the reimage ops will be used otherwise the
reconfig ops will be used to write the image

Signed-off-by: Tom Rix <trix@redhat.com>
---
 include/linux/fpga/fpga-mgr.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 53f9402d6aa17..0791e22b07f88 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -67,12 +67,15 @@ enum fpga_mgr_states {
  * %FPGA_MGR_BITSTREAM_LSB_FIRST: SPI bitstream bit order is LSB first
  *
  * %FPGA_MGR_COMPRESSED_BITSTREAM: FPGA bitstream is compressed
+ *
+ * %FPGA_MGR_REIMAGE: Reimage the whole card, fpga bs and other device fw
  */
 #define FPGA_MGR_PARTIAL_RECONFIG	BIT(0)
 #define FPGA_MGR_EXTERNAL_CONFIG	BIT(1)
 #define FPGA_MGR_ENCRYPTED_BITSTREAM	BIT(2)
 #define FPGA_MGR_BITSTREAM_LSB_FIRST	BIT(3)
 #define FPGA_MGR_COMPRESSED_BITSTREAM	BIT(4)
+#define FPGA_MGR_REIMAGE                BIT(5)
 
 /**
  * struct fpga_image_info - information specific to an FPGA image
-- 
2.26.3

Re: [PATCH v5 2/4] fpga: add FPGA_MGR_REIMAGE flag

[Martin Hundebøll]

On 25/06/2021 21.58, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> If this flag is set the reimage ops will be used otherwise the
> reconfig ops will be used to write the image
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>   include/linux/fpga/fpga-mgr.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 53f9402d6aa17..0791e22b07f88 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -67,12 +67,15 @@ enum fpga_mgr_states {
>    * %FPGA_MGR_BITSTREAM_LSB_FIRST: SPI bitstream bit order is LSB first
>    *
>    * %FPGA_MGR_COMPRESSED_BITSTREAM: FPGA bitstream is compressed
> + *
> + * %FPGA_MGR_REIMAGE: Reimage the whole card, fpga bs and other device fw
>    */
>   #define FPGA_MGR_PARTIAL_RECONFIG	BIT(0)
>   #define FPGA_MGR_EXTERNAL_CONFIG	BIT(1)
>   #define FPGA_MGR_ENCRYPTED_BITSTREAM	BIT(2)
>   #define FPGA_MGR_BITSTREAM_LSB_FIRST	BIT(3)
>   #define FPGA_MGR_COMPRESSED_BITSTREAM	BIT(4)
> +#define FPGA_MGR_REIMAGE                BIT(5)

Are you mixing spaces with tabs here?

// Martin

[PATCH v5 3/4] fpga: pass fpga_manager_update_ops to the fpga_manager_write functions

[trix]

From: Tom Rix <trix@redhat.com>

Refactor fpga_manager_write* functions for reimaging, pass
the update_ops as a parameter.  Continue the passing of the update_ops
to the write wrapper functions.  Only do the reconfig ops.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/fpga/fpga-mgr.c | 90 ++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 37 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 31c51d7e07cc8..c8a6bfa037933 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -45,10 +45,12 @@ static inline u64 fpga_mgr_status(struct fpga_manager *mgr)
 	return 0;
 }
 
-static inline int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
+static inline int fpga_mgr_write(struct fpga_manager *mgr,
+				 const char *buf, size_t count,
+				 const struct fpga_manager_update_ops *uops)
 {
 	if (mgr->mops->reconfig.write)
-		return  mgr->mops->reconfig.write(mgr, buf, count);
+		return  uops->write(mgr, buf, count);
 	return -EOPNOTSUPP;
 }
 
@@ -57,13 +59,14 @@ static inline int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size
  * finish and set the FPGA into operating mode.
  */
 static inline int fpga_mgr_write_complete(struct fpga_manager *mgr,
-					  struct fpga_image_info *info)
+					  struct fpga_image_info *info,
+					  const struct fpga_manager_update_ops *uops)
 {
 	int ret = 0;
 
 	mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
-	if (mgr->mops->reconfig.write_complete)
-		ret = mgr->mops->reconfig.write_complete(mgr, info);
+	if (uops->write_complete)
+		ret = uops->write_complete(mgr, info);
 	if (ret) {
 		dev_err(&mgr->dev, "Error after writing image data to FPGA\n");
 		mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
@@ -76,18 +79,20 @@ static inline int fpga_mgr_write_complete(struct fpga_manager *mgr,
 
 static inline int fpga_mgr_write_init(struct fpga_manager *mgr,
 				      struct fpga_image_info *info,
-				      const char *buf, size_t count)
+				      const char *buf, size_t count,
+				      const struct fpga_manager_update_ops *uops)
 {
-	if (mgr->mops->reconfig.write_init)
-		return  mgr->mops->reconfig.write_init(mgr, info, buf, count);
+	if (uops->write_init)
+		return  uops->write_init(mgr, info, buf, count);
 	return 0;
 }
 
 static inline int fpga_mgr_write_sg(struct fpga_manager *mgr,
-				    struct sg_table *sgt)
+				    struct sg_table *sgt,
+				    const struct fpga_manager_update_ops *uops)
 {
-	if (mgr->mops->reconfig.write_sg)
-		return  mgr->mops->reconfig.write_sg(mgr, sgt);
+	if (uops->write_sg)
+		return  uops->write_sg(mgr, sgt);
 	return -EOPNOTSUPP;
 }
 
@@ -143,16 +148,17 @@ EXPORT_SYMBOL_GPL(fpga_image_info_free);
  */
 static int fpga_mgr_write_init_buf(struct fpga_manager *mgr,
 				   struct fpga_image_info *info,
-				   const char *buf, size_t count)
+				   const char *buf, size_t count,
+				   const struct fpga_manager_update_ops *uops)
 {
 	int ret;
 
 	mgr->state = FPGA_MGR_STATE_WRITE_INIT;
 	if (!mgr->mops->initial_header_size)
-		ret = fpga_mgr_write_init(mgr, info, NULL, 0);
+		ret = fpga_mgr_write_init(mgr, info, NULL, 0, uops);
 	else
 		ret = fpga_mgr_write_init(
-		    mgr, info, buf, min(mgr->mops->initial_header_size, count));
+		    mgr, info, buf, min(mgr->mops->initial_header_size, count), uops);
 
 	if (ret) {
 		dev_err(&mgr->dev, "Error preparing FPGA for writing\n");
@@ -165,7 +171,8 @@ static int fpga_mgr_write_init_buf(struct fpga_manager *mgr,
 
 static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
 				  struct fpga_image_info *info,
-				  struct sg_table *sgt)
+				  struct sg_table *sgt,
+				  const struct fpga_manager_update_ops *uops)
 {
 	struct sg_mapping_iter miter;
 	size_t len;
@@ -173,7 +180,7 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
 	int ret;
 
 	if (!mgr->mops->initial_header_size)
-		return fpga_mgr_write_init_buf(mgr, info, NULL, 0);
+		return fpga_mgr_write_init_buf(mgr, info, NULL, 0, uops);
 
 	/*
 	 * First try to use miter to map the first fragment to access the
@@ -183,7 +190,7 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
 	if (sg_miter_next(&miter) &&
 	    miter.length >= mgr->mops->initial_header_size) {
 		ret = fpga_mgr_write_init_buf(mgr, info, miter.addr,
-					      miter.length);
+					      miter.length, uops);
 		sg_miter_stop(&miter);
 		return ret;
 	}
@@ -196,7 +203,7 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
 
 	len = sg_copy_to_buffer(sgt->sgl, sgt->nents, buf,
 				mgr->mops->initial_header_size);
-	ret = fpga_mgr_write_init_buf(mgr, info, buf, len);
+	ret = fpga_mgr_write_init_buf(mgr, info, buf, len, uops);
 
 	kfree(buf);
 
@@ -208,6 +215,7 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
  * @mgr:	fpga manager
  * @info:	fpga image specific information
  * @sgt:	scatterlist table
+ * @uops:       which update ops to use
  *
  * Step the low level fpga manager through the device-specific steps of getting
  * an FPGA ready to be configured, writing the image to it, then doing whatever
@@ -222,24 +230,25 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
  */
 static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
 				struct fpga_image_info *info,
-				struct sg_table *sgt)
+				struct sg_table *sgt,
+				const struct fpga_manager_update_ops *uops)
 {
 	int ret;
 
-	ret = fpga_mgr_write_init_sg(mgr, info, sgt);
+	ret = fpga_mgr_write_init_sg(mgr, info, sgt, uops);
 	if (ret)
 		return ret;
 
 	/* Write the FPGA image to the FPGA. */
 	mgr->state = FPGA_MGR_STATE_WRITE;
-	if (mgr->mops->reconfig.write_sg) {
-		ret = fpga_mgr_write_sg(mgr, sgt);
+	if (uops->write_sg) {
+		ret = fpga_mgr_write_sg(mgr, sgt, uops);
 	} else {
 		struct sg_mapping_iter miter;
 
 		sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
 		while (sg_miter_next(&miter)) {
-			ret = fpga_mgr_write(mgr, miter.addr, miter.length);
+			ret = fpga_mgr_write(mgr, miter.addr, miter.length, uops);
 			if (ret)
 				break;
 		}
@@ -252,16 +261,17 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
 		return ret;
 	}
 
-	return fpga_mgr_write_complete(mgr, info);
+	return fpga_mgr_write_complete(mgr, info, uops);
 }
 
 static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
 				    struct fpga_image_info *info,
-				    const char *buf, size_t count)
+				    const char *buf, size_t count,
+				    const struct fpga_manager_update_ops *uops)
 {
 	int ret;
 
-	ret = fpga_mgr_write_init_buf(mgr, info, buf, count);
+	ret = fpga_mgr_write_init_buf(mgr, info, buf, count, uops);
 	if (ret)
 		return ret;
 
@@ -269,14 +279,14 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
 	 * Write the FPGA image to the FPGA.
 	 */
 	mgr->state = FPGA_MGR_STATE_WRITE;
-	ret = fpga_mgr_write(mgr, buf, count);
+	ret = fpga_mgr_write(mgr, buf, count, uops);
 	if (ret) {
 		dev_err(&mgr->dev, "Error while writing image data to FPGA\n");
 		mgr->state = FPGA_MGR_STATE_WRITE_ERR;
 		return ret;
 	}
 
-	return fpga_mgr_write_complete(mgr, info);
+	return fpga_mgr_write_complete(mgr, info, uops);
 }
 
 /**
@@ -285,6 +295,7 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
  * @info:	fpga image info
  * @buf:	buffer contain fpga image
  * @count:	byte count of buf
+ * @uops:       which update ops to use
  *
  * Step the low level fpga manager through the device-specific steps of getting
  * an FPGA ready to be configured, writing the image to it, then doing whatever
@@ -295,7 +306,8 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
  */
 static int fpga_mgr_buf_load(struct fpga_manager *mgr,
 			     struct fpga_image_info *info,
-			     const char *buf, size_t count)
+			     const char *buf, size_t count,
+			     const struct fpga_manager_update_ops *uops)
 {
 	struct page **pages;
 	struct sg_table sgt;
@@ -309,8 +321,8 @@ static int fpga_mgr_buf_load(struct fpga_manager *mgr,
 	 * contiguous kernel buffer and the driver doesn't require SG, non-SG
 	 * drivers will still work on the slow path.
 	 */
-	if (mgr->mops->reconfig.write)
-		return fpga_mgr_buf_load_mapped(mgr, info, buf, count);
+	if (uops && uops->write)
+		return fpga_mgr_buf_load_mapped(mgr, info, buf, count, uops);
 
 	/*
 	 * Convert the linear kernel pointer into a sg_table of pages for use
@@ -345,7 +357,7 @@ static int fpga_mgr_buf_load(struct fpga_manager *mgr,
 	if (rc)
 		return rc;
 
-	rc = fpga_mgr_buf_load_sg(mgr, info, &sgt);
+	rc = fpga_mgr_buf_load_sg(mgr, info, &sgt, uops);
 	sg_free_table(&sgt);
 
 	return rc;
@@ -356,6 +368,7 @@ static int fpga_mgr_buf_load(struct fpga_manager *mgr,
  * @mgr:	fpga manager
  * @info:	fpga image specific information
  * @image_name:	name of image file on the firmware search path
+ * @uops:       which update ops to use
  *
  * Request an FPGA image using the firmware class, then write out to the FPGA.
  * Update the state before each step to provide info on what step failed if
@@ -367,7 +380,8 @@ static int fpga_mgr_buf_load(struct fpga_manager *mgr,
  */
 static int fpga_mgr_firmware_load(struct fpga_manager *mgr,
 				  struct fpga_image_info *info,
-				  const char *image_name)
+				  const char *image_name,
+				  const struct fpga_manager_update_ops *uops)
 {
 	struct device *dev = &mgr->dev;
 	const struct firmware *fw;
@@ -384,7 +398,7 @@ static int fpga_mgr_firmware_load(struct fpga_manager *mgr,
 		return ret;
 	}
 
-	ret = fpga_mgr_buf_load(mgr, info, fw->data, fw->size);
+	ret = fpga_mgr_buf_load(mgr, info, fw->data, fw->size, uops);
 
 	release_firmware(fw);
 
@@ -403,12 +417,14 @@ static int fpga_mgr_firmware_load(struct fpga_manager *mgr,
  */
 int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
 {
+	const struct fpga_manager_update_ops *uops = &mgr->mops->reconfig;
+
 	if (info->sgt)
-		return fpga_mgr_buf_load_sg(mgr, info, info->sgt);
+		return fpga_mgr_buf_load_sg(mgr, info, info->sgt, uops);
 	if (info->buf && info->count)
-		return fpga_mgr_buf_load(mgr, info, info->buf, info->count);
+		return fpga_mgr_buf_load(mgr, info, info->buf, info->count, uops);
 	if (info->firmware_name)
-		return fpga_mgr_firmware_load(mgr, info, info->firmware_name);
+		return fpga_mgr_firmware_load(mgr, info, info->firmware_name, uops);
 	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_load);
-- 
2.26.3

Re: [PATCH v5 3/4] fpga: pass fpga_manager_update_ops to the fpga_manager_write functions

[Martin Hundebøll]

On 25/06/2021 21.58, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> Refactor fpga_manager_write* functions for reimaging, pass
> the update_ops as a parameter.  Continue the passing of the update_ops
> to the write wrapper functions.  Only do the reconfig ops.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>   drivers/fpga/fpga-mgr.c | 90 ++++++++++++++++++++++++-----------------
>   1 file changed, 53 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 31c51d7e07cc8..c8a6bfa037933 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -45,10 +45,12 @@ static inline u64 fpga_mgr_status(struct fpga_manager *mgr)
>   	return 0;
>   }
>   
> -static inline int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
> +static inline int fpga_mgr_write(struct fpga_manager *mgr,
> +				 const char *buf, size_t count,
> +				 const struct fpga_manager_update_ops *uops)
>   {
>   	if (mgr->mops->reconfig.write)

Shouldn't this check be

if (uops->write)
	return ...

?

> -		return  mgr->mops->reconfig.write(mgr, buf, count);
> +		return  uops->write(mgr, buf, count);
>   	return -EOPNOTSUPP;
>   }
>   

<snip>

> @@ -208,6 +215,7 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
>    * @mgr:	fpga manager
>    * @info:	fpga image specific information
>    * @sgt:	scatterlist table
> + * @uops:       which update ops to use

Tabs vs. spaces.

>    *
>    * Step the low level fpga manager through the device-specific steps of getting
>    * an FPGA ready to be configured, writing the image to it, then doing whatever

<snip>

> @@ -285,6 +295,7 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
>    * @info:	fpga image info
>    * @buf:	buffer contain fpga image
>    * @count:	byte count of buf
> + * @uops:       which update ops to use

Tabs vs. spaces.

>    *
>    * Step the low level fpga manager through the device-specific steps of getting
>    * an FPGA ready to be configured, writing the image to it, then doing whatever

<snip>

> @@ -356,6 +368,7 @@ static int fpga_mgr_buf_load(struct fpga_manager *mgr,
>    * @mgr:	fpga manager
>    * @info:	fpga image specific information
>    * @image_name:	name of image file on the firmware search path
> + * @uops:       which update ops to use

Tabs vs. spaces.

>    *
>    * Request an FPGA image using the firmware class, then write out to the FPGA.
>    * Update the state before each step to provide info on what step failed if


// Martin

[PATCH v5 4/4] fpga: use reimage ops in fpga_mgr_load()

[trix]

From: Tom Rix <trix@redhat.com>

If the fpga_image_info flags FPGA_MGR_REIMAGE bit is set
swap out the reconfig ops for the reimage ops and do
the load.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/fpga/fpga-mgr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index c8a6bfa037933..5e53a0508087a 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -419,6 +419,9 @@ int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
 {
 	const struct fpga_manager_update_ops *uops = &mgr->mops->reconfig;
 
+	if (info->flags & FPGA_MGR_REIMAGE)
+		uops = &mgr->mops->reimage;
+
 	if (info->sgt)
 		return fpga_mgr_buf_load_sg(mgr, info, info->sgt, uops);
 	if (info->buf && info->count)
-- 
2.26.3

Re: [PATCH v5 4/4] fpga: use reimage ops in fpga_mgr_load()

[Xu Yilun]

On Fri, Jun 25, 2021 at 12:58:49PM -0700, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> If the fpga_image_info flags FPGA_MGR_REIMAGE bit is set
> swap out the reconfig ops for the reimage ops and do
> the load.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/fpga/fpga-mgr.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index c8a6bfa037933..5e53a0508087a 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -419,6 +419,9 @@ int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
>  {
>  	const struct fpga_manager_update_ops *uops = &mgr->mops->reconfig;
>  
> +	if (info->flags & FPGA_MGR_REIMAGE)
> +		uops = &mgr->mops->reimage;
> +

So seems there is no big difference on processing 'reconfig' & 'reimage'
in FPGA mgr framework. We may not have to introduce 2 sets of ops in
fpga_mgr.

Could we just reuse the fpga_mgr_ops for reimage? The
fpga_mgr_ops.write_init() will pass the fpga_image_info to device driver,
and driver could be aware of the REIMAGE flag. Just like the handling of
other flags in fpga_image_info.

Thanks,
Yilun

>  	if (info->sgt)
>  		return fpga_mgr_buf_load_sg(mgr, info, info->sgt, uops);
>  	if (info->buf && info->count)
> -- 
> 2.26.3