[PATCH v12 0/3] Microchip Polarfire FPGA manager

[PATCH v12 0/3] Microchip Polarfire FPGA manager

[Ivan Bornyakov]

Add support to the FPGA manager for programming Microchip Polarfire
FPGAs over slave SPI interface with .dat formatted bitsream image.

Changelog:
  v1 -> v2: fix printk formating
  v2 -> v3:
   * replace "microsemi" with "microchip"
   * replace prefix "microsemi_fpga_" with "mpf_"
   * more sensible .compatible and .name strings
   * remove unused defines STATUS_SPI_VIOLATION and STATUS_SPI_ERROR
  v3 -> v4: fix unused variable warning
    Put 'mpf_of_ids' definition under conditional compilation, so it
    would not hang unused if CONFIG_OF is not enabled.
  v4 -> v5:
   * prefix defines with MPF_
   * mdelay() -> usleep_range()
   * formatting fixes
   * add DT bindings doc
   * rework fpga_manager_ops.write() to fpga_manager_ops.write_sg()
     We can't parse image header in write_init() because image header
     size is not known beforehand. Thus parsing need to be done in
     fpga_manager_ops.write() callback, but fpga_manager_ops.write()
     also need to be reenterable. On the other hand,
     fpga_manager_ops.write_sg() is called once. Thus, rework usage of
     write() callback to write_sg().
  v5 -> v6: fix patch applying
     I forgot to clean up unrelated local changes which lead to error on
     patch 0001-fpga-microchip-spi-add-Microchip-MPF-FPGA-manager.patch
     applying on vanilla kernel.
  v6 -> v7: fix binding doc to pass dt_binding_check
  v7 -> v8: another fix for dt_binding_check warning
  v8 -> v9:
   * add another patch to support bitstream offset in FPGA image buffer
   * rework fpga_manager_ops.write_sg() back to fpga_manager_ops.write()
   * move image header parsing from write() to write_init()
  v9 -> v10:
   * add parse_header() callback to fpga_manager_ops
   * adjust fpga_mgr_write_init[_buf|_sg]() for parse_header() usage
   * implement parse_header() in microchip-spi driver
  v10 -> v11: include missing unaligned.h to microchip-spi
     fix error: implicit declaration of function 'get_unaligned_le[16|32]'
  v11 -> v12:
   * microchip-spi: double read hw status, ignore first read, because it
     can be unreliable.
   * microchip-spi: remove sleep between status readings in
     poll_status_not_busy() to save a few seconds. Status is polled on
     every 16 byte writes - that is quite often, therefore
     usleep_range() accumulate to a considerable number of seconds.

Ivan Bornyakov (3):
  fpga: fpga-mgr: support bitstream offset in image buffer
  fpga: microchip-spi: add Microchip MPF FPGA manager
  dt-bindings: fpga: add binding doc for microchip-spi fpga mgr

 .../fpga/microchip,mpf-spi-fpga-mgr.yaml      |  44 +++
 drivers/fpga/Kconfig                          |   9 +
 drivers/fpga/Makefile                         |   1 +
 drivers/fpga/fpga-mgr.c                       | 150 +++++--
 drivers/fpga/microchip-spi.c                  | 371 ++++++++++++++++++
 include/linux/fpga/fpga-mgr.h                 |  13 +-
 6 files changed, 552 insertions(+), 36 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml
 create mode 100644 drivers/fpga/microchip-spi.c

-- 
2.35.1

[PATCH v12 1/3] fpga: fpga-mgr: support bitstream offset in image buffer

[Ivan Bornyakov]

At the moment FPGA manager core loads to the device entire image
provided to fpga_mgr_load(). But it is not always whole FPGA image
buffer meant to be written to the device. In particular, .dat formatted
image for Microchip MPF contains meta info in the header that is not
meant to be written to the device. This is issue for those low level
drivers that loads data to the device with write() fpga_manager_ops
callback, since write() can be called in iterator over scatter-gather
table, not only linear image buffer. On the other hand, write_sg()
callback is provided with whole image in scatter-gather form and can
decide itself which part should be sent to the device.

Add header_size and data_size to the fpga_image_info struct and adjust
fpga_mgr_write() callers with respect to them.

  * info->header_size indicates part at the beginning of image buffer
    that is *not* meant to be written to the device. It is optional and
    can be 0.

  * info->data_size is the size of actual bitstream data that *is* meant
    to be written to the device, starting at info->header_size from the
    beginning of image buffer. It is also optional and can be 0, which
    means bitstream data is up to the end of image buffer.

Also add parse_header() callback to fpga_manager_ops, which purpose is
to set info->header_size and info->data_size. At least
initial_header_size bytes of image buffer will be passed into
parse_header() first time. If it is not enough, parse_header() should
set desired size into info->header_size and return -EAGAIN, than it will
be called again with greater part of image buffer on the input.

Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
---
 drivers/fpga/fpga-mgr.c       | 150 ++++++++++++++++++++++++++--------
 include/linux/fpga/fpga-mgr.h |  13 ++-
 2 files changed, 127 insertions(+), 36 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index a3595ecc3f79..c6ca395909a0 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -74,6 +74,15 @@ static inline int fpga_mgr_write_complete(struct fpga_manager *mgr,
 	return 0;
 }
 
+static inline int fpga_mgr_parse_header(struct fpga_manager *mgr,
+					struct fpga_image_info *info,
+					const char *buf, size_t count)
+{
+	if (buf && mgr->mops->parse_header)
+		return mgr->mops->parse_header(mgr, info, buf, count);
+	return 0;
+}
+
 static inline int fpga_mgr_write_init(struct fpga_manager *mgr,
 				      struct fpga_image_info *info,
 				      const char *buf, size_t count)
@@ -136,32 +145,61 @@ void fpga_image_info_free(struct fpga_image_info *info)
 EXPORT_SYMBOL_GPL(fpga_image_info_free);
 
 /*
- * Call the low level driver's write_init function.  This will do the
- * device-specific things to get the FPGA into the state where it is ready to
- * receive an FPGA image. The low level driver only gets to see the first
- * initial_header_size bytes in the buffer.
+ * Call the low level driver's parse_header then write_init functions.
+ * This will do the device-specific things to get the FPGA into the state
+ * where it is ready to receive an FPGA image. If parse_header sets
+ * info->header_size, the low level driver's write_init only gets to see the
+ * first info->header_size bytes in the buffer, mgr->mops->initial_header_size
+ * otherwise. If neither initial_header_size nor header_size are not set,
+ * write_init will not get any bytes of image buffer.
  */
 static int fpga_mgr_write_init_buf(struct fpga_manager *mgr,
 				   struct fpga_image_info *info,
 				   const char *buf, size_t count)
 {
+	size_t header_size;
 	int ret;
 
 	mgr->state = FPGA_MGR_STATE_WRITE_INIT;
-	if (!mgr->mops->initial_header_size) {
-		ret = fpga_mgr_write_init(mgr, info, NULL, 0);
-	} else {
-		count = min(mgr->mops->initial_header_size, count);
-		ret = fpga_mgr_write_init(mgr, info, buf, count);
+	ret = fpga_mgr_parse_header(mgr, info, buf, count);
+	if (ret) {
+		if (ret != -EAGAIN)
+			dev_err(&mgr->dev,
+				"Error while parsing FPGA image header\n");
+
+		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
+		return ret;
 	}
 
+	header_size = mgr->mops->initial_header_size;
+	if (info->header_size)
+		header_size = info->header_size;
+
+	ret = fpga_mgr_write_init(mgr, info, header_size ? buf : NULL, header_size);
 	if (ret) {
 		dev_err(&mgr->dev, "Error preparing FPGA for writing\n");
 		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
-		return ret;
 	}
 
-	return 0;
+	return ret;
+}
+
+static void *fpga_mgr_sgt_bounce_buf(struct sg_table *sgt, size_t count)
+{
+	size_t len;
+	void *buf;
+
+	buf = kmalloc(count, GFP_KERNEL);
+	if (!buf)
+		return ERR_PTR(-ENOMEM);
+
+	len = sg_copy_to_buffer(sgt->sgl, sgt->nents, buf, count);
+	if (len != count) {
+		kfree(buf);
+		return ERR_PTR(-EFAULT);
+	}
+
+	return buf;
 }
 
 static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
@@ -169,37 +207,46 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
 				  struct sg_table *sgt)
 {
 	struct sg_mapping_iter miter;
-	size_t len;
+	size_t header_size;
 	char *buf;
-	int ret;
+	int ret = -EAGAIN;
 
-	if (!mgr->mops->initial_header_size)
+	header_size = mgr->mops->initial_header_size;
+	if (!header_size)
 		return fpga_mgr_write_init_buf(mgr, info, NULL, 0);
 
-	/*
-	 * First try to use miter to map the first fragment to access the
-	 * header, this is the typical path.
-	 */
 	sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
-	if (sg_miter_next(&miter) &&
-	    miter.length >= mgr->mops->initial_header_size) {
-		ret = fpga_mgr_write_init_buf(mgr, info, miter.addr,
-					      miter.length);
-		sg_miter_stop(&miter);
-		return ret;
+	if (!sg_miter_next(&miter)) {
+		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
+		ret = -EFAULT;
 	}
-	sg_miter_stop(&miter);
 
-	/* Otherwise copy the fragments into temporary memory. */
-	buf = kmalloc(mgr->mops->initial_header_size, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
+	while (ret == -EAGAIN) {
+		/*
+		 * First try to use miter to map the first fragment to access
+		 * the header, this is the typical path.
+		 */
+		if (miter.length >= header_size) {
+			ret = fpga_mgr_write_init_buf(mgr, info, miter.addr,
+						      miter.length);
+		} else {
+			/*
+			 * Otherwise copy the fragments into temporary memory.
+			 */
+			buf = fpga_mgr_sgt_bounce_buf(sgt, header_size);
+			if (IS_ERR(buf)) {
+				mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
+				ret = PTR_ERR(buf);
+				break;
+			}
 
-	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, header_size);
+			kfree(buf);
+		}
 
-	kfree(buf);
+		header_size = info->header_size;
+	}
+	sg_miter_stop(&miter);
 
 	return ret;
 }
@@ -236,13 +283,33 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
 	if (mgr->mops->write_sg) {
 		ret = fpga_mgr_write_sg(mgr, sgt);
 	} else {
+		size_t offset, count, length, data_size;
 		struct sg_mapping_iter miter;
 
+		offset = info->header_size;
+		data_size = info->data_size;
+		count = 0;
+
 		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);
-			if (ret)
+			if (offset >= miter.length) {
+				offset -= miter.length;
+				continue;
+			}
+
+			if (data_size)
+				length = min(miter.length - offset,
+					     data_size - count);
+			else
+				length = miter.length - offset;
+
+			count += length;
+
+			ret = fpga_mgr_write(mgr, miter.addr + offset, length);
+			if (ret || count == data_size)
 				break;
+
+			offset = 0;
 		}
 		sg_miter_stop(&miter);
 	}
@@ -266,6 +333,19 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
 	if (ret)
 		return ret;
 
+	if (info->header_size + info->data_size > count) {
+		dev_err(&mgr->dev, "Bitsream data outruns FPGA image\n");
+		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
+		return -EINVAL;
+	}
+
+	if (info->data_size)
+		count = info->data_size;
+	else
+		count -= info->header_size;
+
+	buf += info->header_size;
+
 	/*
 	 * Write the FPGA image to the FPGA.
 	 */
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 0f9468771bb9..a49b97bccfa2 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -85,6 +85,8 @@ enum fpga_mgr_states {
  * @sgt: scatter/gather table containing FPGA image
  * @buf: contiguous buffer containing FPGA image
  * @count: size of buf
+ * @header_size: offset in image buffer where bitstream data starts
+ * @data_size: size of bitstream. If 0, (count - header_size) will be used.
  * @region_id: id of target region
  * @dev: device that owns this
  * @overlay: Device Tree overlay
@@ -98,6 +100,8 @@ struct fpga_image_info {
 	struct sg_table *sgt;
 	const char *buf;
 	size_t count;
+	size_t header_size;
+	size_t data_size;
 	int region_id;
 	struct device *dev;
 #ifdef CONFIG_OF
@@ -137,9 +141,13 @@ struct fpga_manager_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
+ * @initial_header_size: minimum number of bytes that should be passed into
+ *	parse_header and write_init.
  * @state: returns an enum value of the FPGA's state
  * @status: returns status of the FPGA, including reconfiguration error code
+ * @parse_header: parse FPGA image header to set info->header_size and
+ *	info->data_size. In case the input buffer is not large enough, set
+ *	requierd size to info->header_size and return -EAGAIN.
  * @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
@@ -155,6 +163,9 @@ 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 (*parse_header)(struct fpga_manager *mgr,
+			    struct fpga_image_info *info,
+			    const char *buf, size_t count);
 	int (*write_init)(struct fpga_manager *mgr,
 			  struct fpga_image_info *info,
 			  const char *buf, size_t count);
-- 
2.35.1

[PATCH v12 2/3] fpga: microchip-spi: add Microchip MPF FPGA manager

[Ivan Bornyakov]

Add support to the FPGA manager for programming Microchip Polarfire
FPGAs over slave SPI interface with .dat formatted bitsream image.

Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
---
 drivers/fpga/Kconfig         |   9 +
 drivers/fpga/Makefile        |   1 +
 drivers/fpga/microchip-spi.c | 371 +++++++++++++++++++++++++++++++++++
 3 files changed, 381 insertions(+)
 create mode 100644 drivers/fpga/microchip-spi.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 26025dbab353..75806ef5c9ea 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -248,4 +248,13 @@ config FPGA_MGR_VERSAL_FPGA
 	  configure the programmable logic(PL).
 
 	  To compile this as a module, choose M here.
+
+config FPGA_MGR_MICROCHIP_SPI
+	tristate "Microchip Polarfire SPI FPGA manager"
+	depends on SPI
+	help
+	  FPGA manager driver support for Microchip Polarfire FPGAs
+	  programming over slave SPI interface with .dat formatted
+	  bitstream image.
+
 endif # FPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index e32bfa90f968..5425a15892df 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_FPGA_MGR_XILINX_SPI)	+= xilinx-spi.o
 obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
 obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
 obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA)	+= versal-fpga.o
+obj-$(CONFIG_FPGA_MGR_MICROCHIP_SPI)	+= microchip-spi.o
 obj-$(CONFIG_ALTERA_PR_IP_CORE)		+= altera-pr-ip-core.o
 obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)	+= altera-pr-ip-core-plat.o
 
diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
new file mode 100644
index 000000000000..cb59f43000a4
--- /dev/null
+++ b/drivers/fpga/microchip-spi.c
@@ -0,0 +1,371 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Microchip Polarfire FPGA programming over slave SPI interface.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/delay.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/spi/spi.h>
+
+#define	MPF_SPI_ISC_ENABLE	0x0B
+#define	MPF_SPI_ISC_DISABLE	0x0C
+#define	MPF_SPI_READ_STATUS	0x00
+#define	MPF_SPI_READ_DATA	0x01
+#define	MPF_SPI_FRAME_INIT	0xAE
+#define	MPF_SPI_FRAME		0xEE
+#define	MPF_SPI_PRG_MODE	0x01
+#define	MPF_SPI_RELEASE		0x23
+
+#define	MPF_SPI_FRAME_SIZE	16
+
+#define	MPF_HEADER_SIZE_OFFSET	24
+#define	MPF_DATA_SIZE_OFFSET	55
+
+#define	MPF_LOOKUP_TABLE_RECORD_SIZE		9
+#define	MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET	0
+#define	MPF_LOOKUP_TABLE_BLOCK_START_OFFSET	1
+
+#define	MPF_COMPONENTS_SIZE_ID	5
+#define	MPF_BITSTREAM_ID	8
+
+#define	MPF_BITS_PER_COMPONENT_SIZE	22
+
+#define	MPF_STATUS_POLL_TIMEOUT		10000
+#define	MPF_STATUS_BUSY			BIT(0)
+#define	MPF_STATUS_READY		BIT(1)
+#define	MPF_STATUS_SPI_VIOLATION	BIT(2)
+#define	MPF_STATUS_SPI_ERROR		BIT(3)
+
+struct mpf_priv {
+	struct spi_device *spi;
+	bool program_mode;
+};
+
+static int mpf_read_status(struct spi_device *spi)
+{
+	u8 status = 0, status_command = MPF_SPI_READ_STATUS;
+	struct spi_transfer xfers[] = {
+		[0 ... 1] = {
+			.tx_buf = &status_command,
+			.rx_buf = &status,
+			.len = 1,
+			.cs_change = 1,
+		}
+	};
+	int ret = spi_sync_transfer(spi, xfers, 2);
+
+	if ((status & MPF_STATUS_SPI_VIOLATION) ||
+	    (status & MPF_STATUS_SPI_ERROR))
+		ret = -EIO;
+
+	return ret ? : status;
+}
+
+static enum fpga_mgr_states mpf_ops_state(struct fpga_manager *mgr)
+{
+	struct mpf_priv *priv = mgr->priv;
+	struct spi_device *spi;
+	bool program_mode;
+	int status;
+
+	spi = priv->spi;
+	program_mode = priv->program_mode;
+	status = mpf_read_status(spi);
+
+	if (!program_mode && !status)
+		return FPGA_MGR_STATE_OPERATING;
+
+	return FPGA_MGR_STATE_UNKNOWN;
+}
+
+static int mpf_ops_parse_header(struct fpga_manager *mgr,
+				struct fpga_image_info *info,
+				const char *buf, size_t count)
+{
+	size_t component_size_byte_num, component_size_byte_off,
+	       components_size_start = 0, bitstream_start = 0,
+	       block_id_offset, block_start_offset, i;
+	u8 header_size, blocks_num, block_id;
+	u32 block_start, component_size;
+	u16 components_num;
+
+	if (!buf) {
+		dev_err(&mgr->dev, "Image buffer is not provided\n");
+		return -EINVAL;
+	}
+
+	header_size = *(buf + MPF_HEADER_SIZE_OFFSET);
+	if (header_size > count) {
+		info->header_size = header_size;
+		return -EAGAIN;
+	}
+
+	/*
+	 * Go through look-up table to find out where actual bitstream starts
+	 * and where sizes of components of the bitstream lies.
+	 */
+	blocks_num = *(buf + header_size - 1);
+	block_id_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET;
+	block_start_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_START_OFFSET;
+
+	header_size += blocks_num * MPF_LOOKUP_TABLE_RECORD_SIZE;
+	if (header_size > count) {
+		info->header_size = header_size;
+		return -EAGAIN;
+	}
+
+	while (blocks_num--) {
+		block_id = *(buf + block_id_offset);
+		block_start = get_unaligned_le32(buf + block_start_offset);
+
+		switch (block_id) {
+		case MPF_BITSTREAM_ID:
+			info->header_size = bitstream_start = block_start;
+			if (block_start > count)
+				return -EAGAIN;
+
+			break;
+		case MPF_COMPONENTS_SIZE_ID:
+			components_size_start = block_start;
+			break;
+		default:
+			break;
+		}
+
+		if (bitstream_start && components_size_start)
+			break;
+
+		block_id_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
+		block_start_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
+	}
+
+	if (!bitstream_start || !components_size_start) {
+		dev_err(&mgr->dev, "Failed to parse header look-up table\n");
+		return -EFAULT;
+	}
+
+	/*
+	 * Parse bitstream size.
+	 * Sizes of components of the bitstream are 22-bits long placed next
+	 * to each other. Image header should be extended by now up to where
+	 * actual bitstream starts, so no need for overflow check anymore.
+	 */
+	components_num = get_unaligned_le16(buf + MPF_DATA_SIZE_OFFSET);
+
+	for (i = 0; i < components_num; i++) {
+		component_size_byte_num =
+			(i * MPF_BITS_PER_COMPONENT_SIZE) / BITS_PER_BYTE;
+		component_size_byte_off =
+			(i * MPF_BITS_PER_COMPONENT_SIZE) % BITS_PER_BYTE;
+
+		component_size = get_unaligned_le32(buf +
+						    components_size_start +
+						    component_size_byte_num);
+		component_size >>= component_size_byte_off;
+		component_size &= GENMASK(MPF_BITS_PER_COMPONENT_SIZE - 1, 0);
+
+		info->data_size += component_size * MPF_SPI_FRAME_SIZE;
+	}
+
+	return 0;
+}
+
+static int poll_status_not_busy(struct spi_device *spi, u8 mask)
+{
+	int status, timeout = MPF_STATUS_POLL_TIMEOUT;
+
+	while (timeout--) {
+		status = mpf_read_status(spi);
+		if (status < 0 ||
+		    (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
+			return status;
+	}
+
+	return -EBUSY;
+}
+
+static int mpf_spi_write(struct spi_device *spi, const void *buf, size_t buf_size)
+{
+	int status = poll_status_not_busy(spi, 0);
+
+	if (status < 0)
+		return status;
+
+	return spi_write(spi, buf, buf_size);
+}
+
+static int mpf_spi_write_then_read(struct spi_device *spi,
+				   const void *txbuf, size_t txbuf_size,
+				   void *rxbuf, size_t rxbuf_size)
+{
+	const u8 read_command[] = { MPF_SPI_READ_DATA };
+	int ret;
+
+	ret = mpf_spi_write(spi, txbuf, txbuf_size);
+	if (ret)
+		return ret;
+
+	ret = poll_status_not_busy(spi, MPF_STATUS_READY);
+	if (ret < 0)
+		return ret;
+
+	return spi_write_then_read(spi, read_command, sizeof(read_command),
+				   rxbuf, rxbuf_size);
+}
+
+static int mpf_ops_write_init(struct fpga_manager *mgr,
+			      struct fpga_image_info *info, const char *buf,
+			      size_t count)
+{
+	const u8 program_mode[] = { MPF_SPI_FRAME_INIT, MPF_SPI_PRG_MODE };
+	const u8 isc_en_command[] = { MPF_SPI_ISC_ENABLE };
+	struct mpf_priv *priv = mgr->priv;
+	struct device *dev = &mgr->dev;
+	struct spi_device *spi;
+	u32 isc_ret;
+	int ret;
+
+	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
+		dev_err(dev, "Partial reconfiguration is not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	spi = priv->spi;
+
+	ret = mpf_spi_write_then_read(spi, isc_en_command, sizeof(isc_en_command),
+				      &isc_ret, sizeof(isc_ret));
+	if (ret || isc_ret) {
+		dev_err(dev, "Failed to enable ISC: %d\n", ret ? : isc_ret);
+		return -EFAULT;
+	}
+
+	ret = mpf_spi_write(spi, program_mode, sizeof(program_mode));
+	if (ret) {
+		dev_err(dev, "Failed to enter program mode: %d\n", ret);
+		return ret;
+	}
+
+	priv->program_mode = true;
+
+	return 0;
+}
+
+static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count)
+{
+	u8 tmp_buf[MPF_SPI_FRAME_SIZE + 1] = { MPF_SPI_FRAME, };
+	struct mpf_priv *priv = mgr->priv;
+	struct device *dev = &mgr->dev;
+	struct spi_device *spi;
+	int ret, i;
+
+	if (count % MPF_SPI_FRAME_SIZE) {
+		dev_err(dev, "Bitstream size is not a multiple of %d\n",
+			MPF_SPI_FRAME_SIZE);
+		return -EINVAL;
+	}
+
+	spi = priv->spi;
+
+	for (i = 0; i < count / MPF_SPI_FRAME_SIZE; i++) {
+		memcpy(tmp_buf + 1, buf + i * MPF_SPI_FRAME_SIZE,
+		       MPF_SPI_FRAME_SIZE);
+
+		ret = mpf_spi_write(spi, tmp_buf, sizeof(tmp_buf));
+		if (ret) {
+			dev_err(dev, "Failed to write bitstream frame %d/%zd\n",
+				i, count / MPF_SPI_FRAME_SIZE);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int mpf_ops_write_complete(struct fpga_manager *mgr,
+				  struct fpga_image_info *info)
+{
+	const u8 isc_dis_command[] = { MPF_SPI_ISC_DISABLE };
+	const u8 release_command[] = { MPF_SPI_RELEASE };
+	struct mpf_priv *priv = mgr->priv;
+	struct device *dev = &mgr->dev;
+	struct spi_device *spi;
+	int ret;
+
+	spi = priv->spi;
+
+	ret = mpf_spi_write(spi, isc_dis_command, sizeof(isc_dis_command));
+	if (ret) {
+		dev_err(dev, "Failed to disable ISC: %d\n", ret);
+		return ret;
+	}
+
+	usleep_range(1000, 2000);
+
+	ret = mpf_spi_write(spi, release_command, sizeof(release_command));
+	if (ret) {
+		dev_err(dev, "Failed to exit program mode: %d\n", ret);
+		return ret;
+	}
+
+	priv->program_mode = false;
+
+	return 0;
+}
+
+static const struct fpga_manager_ops mpf_ops = {
+	.state = mpf_ops_state,
+	.initial_header_size = 71,
+	.parse_header = mpf_ops_parse_header,
+	.write_init = mpf_ops_write_init,
+	.write = mpf_ops_write,
+	.write_complete = mpf_ops_write_complete,
+};
+
+static int mpf_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct fpga_manager *mgr;
+	struct mpf_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->spi = spi;
+
+	mgr = devm_fpga_mgr_register(dev, "Microchip Polarfire SPI FPGA Manager",
+				     &mpf_ops, priv);
+
+	return PTR_ERR_OR_ZERO(mgr);
+}
+
+static const struct spi_device_id mpf_spi_ids[] = {
+	{ .name = "mpf-spi-fpga-mgr", },
+	{},
+};
+MODULE_DEVICE_TABLE(spi, mpf_spi_ids);
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id mpf_of_ids[] = {
+	{ .compatible = "microchip,mpf-spi-fpga-mgr" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mpf_of_ids);
+#endif /* IS_ENABLED(CONFIG_OF) */
+
+static struct spi_driver mpf_driver = {
+	.probe = mpf_probe,
+	.id_table = mpf_spi_ids,
+	.driver = {
+		.name = "microchip_mpf_spi_fpga_mgr",
+		.of_match_table = of_match_ptr(mpf_of_ids),
+	},
+};
+
+module_spi_driver(mpf_driver);
+
+MODULE_DESCRIPTION("Microchip Polarfire SPI FPGA Manager");
+MODULE_LICENSE("GPL");
-- 
2.35.1

[PATCH v12 3/3] dt-bindings: fpga: add binding doc for microchip-spi fpga mgr

[Ivan Bornyakov]

Add Device Tree Binding doc for Microchip Polarfire FPGA Manager using
slave SPI to load .dat formatted bitstream image.

Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../fpga/microchip,mpf-spi-fpga-mgr.yaml      | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml

diff --git a/Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml b/Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml
new file mode 100644
index 000000000000..aee45cb15592
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/fpga/microchip,mpf-spi-fpga-mgr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip Polarfire FPGA manager.
+
+maintainers:
+  - Ivan Bornyakov <i.bornyakov@metrotek.ru>
+
+description:
+  Device Tree Bindings for Microchip Polarfire FPGA Manager using slave SPI to
+  load the bitstream in .dat format.
+
+properties:
+  compatible:
+    enum:
+      - microchip,mpf-spi-fpga-mgr
+
+  reg:
+    description: SPI chip select
+    maxItems: 1
+
+  spi-max-frequency: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    spi {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            fpga_mgr@0 {
+                    compatible = "microchip,mpf-spi-fpga-mgr";
+                    spi-max-frequency = <20000000>;
+                    reg = <0>;
+            };
+    };
-- 
2.35.1

Re: [PATCH v12 0/3] Microchip Polarfire FPGA manager

[Ivan Bornyakov]

On Fri, May 13, 2022 at 07:27:52PM +0300, Ivan Bornyakov wrote:
> Add support to the FPGA manager for programming Microchip Polarfire
> FPGAs over slave SPI interface with .dat formatted bitsream image.
> 
> Changelog:
>   v1 -> v2: fix printk formating
>   v2 -> v3:
>    * replace "microsemi" with "microchip"
>    * replace prefix "microsemi_fpga_" with "mpf_"
>    * more sensible .compatible and .name strings
>    * remove unused defines STATUS_SPI_VIOLATION and STATUS_SPI_ERROR
>   v3 -> v4: fix unused variable warning
>     Put 'mpf_of_ids' definition under conditional compilation, so it
>     would not hang unused if CONFIG_OF is not enabled.
>   v4 -> v5:
>    * prefix defines with MPF_
>    * mdelay() -> usleep_range()
>    * formatting fixes
>    * add DT bindings doc
>    * rework fpga_manager_ops.write() to fpga_manager_ops.write_sg()
>      We can't parse image header in write_init() because image header
>      size is not known beforehand. Thus parsing need to be done in
>      fpga_manager_ops.write() callback, but fpga_manager_ops.write()
>      also need to be reenterable. On the other hand,
>      fpga_manager_ops.write_sg() is called once. Thus, rework usage of
>      write() callback to write_sg().
>   v5 -> v6: fix patch applying
>      I forgot to clean up unrelated local changes which lead to error on
>      patch 0001-fpga-microchip-spi-add-Microchip-MPF-FPGA-manager.patch
>      applying on vanilla kernel.
>   v6 -> v7: fix binding doc to pass dt_binding_check
>   v7 -> v8: another fix for dt_binding_check warning
>   v8 -> v9:
>    * add another patch to support bitstream offset in FPGA image buffer
>    * rework fpga_manager_ops.write_sg() back to fpga_manager_ops.write()
>    * move image header parsing from write() to write_init()
>   v9 -> v10:
>    * add parse_header() callback to fpga_manager_ops
>    * adjust fpga_mgr_write_init[_buf|_sg]() for parse_header() usage
>    * implement parse_header() in microchip-spi driver
>   v10 -> v11: include missing unaligned.h to microchip-spi
>      fix error: implicit declaration of function 'get_unaligned_le[16|32]'
>   v11 -> v12:
>    * microchip-spi: double read hw status, ignore first read, because it
>      can be unreliable.
>    * microchip-spi: remove sleep between status readings in
>      poll_status_not_busy() to save a few seconds. Status is polled on
>      every 16 byte writes - that is quite often, therefore
>      usleep_range() accumulate to a considerable number of seconds.
> 
> Ivan Bornyakov (3):
>   fpga: fpga-mgr: support bitstream offset in image buffer
>   fpga: microchip-spi: add Microchip MPF FPGA manager
>   dt-bindings: fpga: add binding doc for microchip-spi fpga mgr
> 
>  .../fpga/microchip,mpf-spi-fpga-mgr.yaml      |  44 +++
>  drivers/fpga/Kconfig                          |   9 +
>  drivers/fpga/Makefile                         |   1 +
>  drivers/fpga/fpga-mgr.c                       | 150 +++++--
>  drivers/fpga/microchip-spi.c                  | 371 ++++++++++++++++++
>  include/linux/fpga/fpga-mgr.h                 |  13 +-
>  6 files changed, 552 insertions(+), 36 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/fpga/microchip,mpf-spi-fpga-mgr.yaml
>  create mode 100644 drivers/fpga/microchip-spi.c
> 
> -- 
> 2.35.1
> 

Friendly ping.

Re: [PATCH v12 1/3] fpga: fpga-mgr: support bitstream offset in image buffer

[Xu Yilun]

On Fri, May 13, 2022 at 07:27:53PM +0300, Ivan Bornyakov wrote:
> At the moment FPGA manager core loads to the device entire image
> provided to fpga_mgr_load(). But it is not always whole FPGA image
> buffer meant to be written to the device. In particular, .dat formatted
> image for Microchip MPF contains meta info in the header that is not
> meant to be written to the device. This is issue for those low level
> drivers that loads data to the device with write() fpga_manager_ops
> callback, since write() can be called in iterator over scatter-gather
> table, not only linear image buffer. On the other hand, write_sg()
> callback is provided with whole image in scatter-gather form and can
> decide itself which part should be sent to the device.
> 
> Add header_size and data_size to the fpga_image_info struct and adjust
> fpga_mgr_write() callers with respect to them.
> 
>   * info->header_size indicates part at the beginning of image buffer
>     that is *not* meant to be written to the device. It is optional and
>     can be 0.
> 
>   * info->data_size is the size of actual bitstream data that *is* meant
>     to be written to the device, starting at info->header_size from the
>     beginning of image buffer. It is also optional and can be 0, which
>     means bitstream data is up to the end of image buffer.
> 
> Also add parse_header() callback to fpga_manager_ops, which purpose is
> to set info->header_size and info->data_size. At least
> initial_header_size bytes of image buffer will be passed into
> parse_header() first time. If it is not enough, parse_header() should
> set desired size into info->header_size and return -EAGAIN, than it will
> be called again with greater part of image buffer on the input.
> 
> Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
> ---
>  drivers/fpga/fpga-mgr.c       | 150 ++++++++++++++++++++++++++--------
>  include/linux/fpga/fpga-mgr.h |  13 ++-
>  2 files changed, 127 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index a3595ecc3f79..c6ca395909a0 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -74,6 +74,15 @@ static inline int fpga_mgr_write_complete(struct fpga_manager *mgr,
>  	return 0;
>  }
>  
> +static inline int fpga_mgr_parse_header(struct fpga_manager *mgr,
> +					struct fpga_image_info *info,
> +					const char *buf, size_t count)
> +{
> +	if (buf && mgr->mops->parse_header)
> +		return mgr->mops->parse_header(mgr, info, buf, count);
> +	return 0;
> +}
> +
>  static inline int fpga_mgr_write_init(struct fpga_manager *mgr,
>  				      struct fpga_image_info *info,
>  				      const char *buf, size_t count)
> @@ -136,32 +145,61 @@ void fpga_image_info_free(struct fpga_image_info *info)
>  EXPORT_SYMBOL_GPL(fpga_image_info_free);
>  
>  /*
> - * Call the low level driver's write_init function.  This will do the
> - * device-specific things to get the FPGA into the state where it is ready to
> - * receive an FPGA image. The low level driver only gets to see the first
> - * initial_header_size bytes in the buffer.
> + * Call the low level driver's parse_header then write_init functions.
> + * This will do the device-specific things to get the FPGA into the state
> + * where it is ready to receive an FPGA image. If parse_header sets
> + * info->header_size, the low level driver's write_init only gets to see the
> + * first info->header_size bytes in the buffer, mgr->mops->initial_header_size
> + * otherwise. If neither initial_header_size nor header_size are not set,
> + * write_init will not get any bytes of image buffer.

Could we always initialize the info->header_size = initial_header_size
at early stage, maybe in fpga_mgr_load(), and only query
info->header_size afterward. This is to make the logic for header size
simpler.

>   */
>  static int fpga_mgr_write_init_buf(struct fpga_manager *mgr,
>  				   struct fpga_image_info *info,
>  				   const char *buf, size_t count)
>  {
> +	size_t header_size;
>  	int ret;
>  
>  	mgr->state = FPGA_MGR_STATE_WRITE_INIT;
> -	if (!mgr->mops->initial_header_size) {
> -		ret = fpga_mgr_write_init(mgr, info, NULL, 0);
> -	} else {
> -		count = min(mgr->mops->initial_header_size, count);
> -		ret = fpga_mgr_write_init(mgr, info, buf, count);
> +	ret = fpga_mgr_parse_header(mgr, info, buf, count);
> +	if (ret) {
> +		if (ret != -EAGAIN)
> +			dev_err(&mgr->dev,
> +				"Error while parsing FPGA image header\n");
> +
> +		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;

Could we move the header parsing out of fpga_mgr_write_init_buf()
and introduce a dedicated function fpga_mgr_parse_head_buf().
We could also define dedicated FPGA_MGR_STATE_PARSE_HEAD &
FPGA_MGR_STATE_PARSE_HEAD_ERR states. This is to makes the flow
more clear to developers. The sg_buf flow won't try several times
on write_init() actually.

> +		return ret;
>  	}
>  
> +	header_size = mgr->mops->initial_header_size;
> +	if (info->header_size)
> +		header_size = info->header_size;
> +
> +	ret = fpga_mgr_write_init(mgr, info, header_size ? buf : NULL, header_size);

We still need to check the count vs header_size, to ensure the low level
driver won't access buf out of bounds.

>  	if (ret) {
>  		dev_err(&mgr->dev, "Error preparing FPGA for writing\n");
>  		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> -		return ret;
>  	}
>  
> -	return 0;
> +	return ret;
> +}
> +
> +static void *fpga_mgr_sgt_bounce_buf(struct sg_table *sgt, size_t count)
> +{
> +	size_t len;
> +	void *buf;
> +
> +	buf = kmalloc(count, GFP_KERNEL);
> +	if (!buf)
> +		return ERR_PTR(-ENOMEM);
> +
> +	len = sg_copy_to_buffer(sgt->sgl, sgt->nents, buf, count);
> +	if (len != count) {
> +		kfree(buf);
> +		return ERR_PTR(-EFAULT);
> +	}
> +
> +	return buf;
>  }
>  
>  static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
> @@ -169,37 +207,46 @@ static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,

Maybe we change the function name, I assume we should deal with
parse_image and write_init here. How about fpga_mgr_prepare_sg()?

>  				  struct sg_table *sgt)
>  {
>  	struct sg_mapping_iter miter;
> -	size_t len;
> +	size_t header_size;
>  	char *buf;
> -	int ret;
> +	int ret = -EAGAIN;
>  
> -	if (!mgr->mops->initial_header_size)
> +	header_size = mgr->mops->initial_header_size;
> +	if (!header_size)
>  		return fpga_mgr_write_init_buf(mgr, info, NULL, 0);
>  
> -	/*
> -	 * First try to use miter to map the first fragment to access the
> -	 * header, this is the typical path.
> -	 */
>  	sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
> -	if (sg_miter_next(&miter) &&
> -	    miter.length >= mgr->mops->initial_header_size) {
> -		ret = fpga_mgr_write_init_buf(mgr, info, miter.addr,
> -					      miter.length);
> -		sg_miter_stop(&miter);
> -		return ret;
> +	if (!sg_miter_next(&miter)) {
> +		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> +		ret = -EFAULT;
>  	}
> -	sg_miter_stop(&miter);
>  
> -	/* Otherwise copy the fragments into temporary memory. */
> -	buf = kmalloc(mgr->mops->initial_header_size, GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
> +	while (ret == -EAGAIN) {

Is do ... while() better, then we don't have to initialize the ret = -EAGAIN.

> +		/*
> +		 * First try to use miter to map the first fragment to access
> +		 * the header, this is the typical path.
> +		 */
> +		if (miter.length >= header_size) {
> +			ret = fpga_mgr_write_init_buf(mgr, info, miter.addr,
> +						      miter.length);
> +		} else {
> +			/*
> +			 * Otherwise copy the fragments into temporary memory.
> +			 */
> +			buf = fpga_mgr_sgt_bounce_buf(sgt, header_size);
> +			if (IS_ERR(buf)) {
> +				mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> +				ret = PTR_ERR(buf);
> +				break;
> +			}
>  
> -	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, header_size);
> +			kfree(buf);
> +		}
>  
> -	kfree(buf);
> +		header_size = info->header_size;
> +	}
> +	sg_miter_stop(&miter);
>  
>  	return ret;
>  }
> @@ -236,13 +283,33 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
>  	if (mgr->mops->write_sg) {
>  		ret = fpga_mgr_write_sg(mgr, sgt);
>  	} else {
> +		size_t offset, count, length, data_size;
>  		struct sg_mapping_iter miter;
>  
> +		offset = info->header_size;
> +		data_size = info->data_size;
> +		count = 0;
> +
>  		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);
> -			if (ret)
> +			if (offset >= miter.length) {
> +				offset -= miter.length;
> +				continue;
> +			}
> +
> +			if (data_size)
> +				length = min(miter.length - offset,
> +					     data_size - count);
> +			else
> +				length = miter.length - offset;
> +
> +			count += length;
> +
> +			ret = fpga_mgr_write(mgr, miter.addr + offset, length);
> +			if (ret || count == data_size)
>  				break;
> +
> +			offset = 0;

The logic for 'offset' is quite complex here.

Could we define a 'ssize count' and initialize as -(info->header_size)?
When 'count' becomes positive, we could start writing.

>  		}
>  		sg_miter_stop(&miter);
>  	}
> @@ -266,6 +333,19 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
>  	if (ret)
>  		return ret;
>  
> +	if (info->header_size + info->data_size > count) {
> +		dev_err(&mgr->dev, "Bitsream data outruns FPGA image\n");
> +		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> +		return -EINVAL;
> +	}
> +
> +	if (info->data_size)
> +		count = info->data_size;
> +	else
> +		count -= info->header_size;
> +
> +	buf += info->header_size;
> +
>  	/*
>  	 * Write the FPGA image to the FPGA.
>  	 */
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 0f9468771bb9..a49b97bccfa2 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -85,6 +85,8 @@ enum fpga_mgr_states {
>   * @sgt: scatter/gather table containing FPGA image
>   * @buf: contiguous buffer containing FPGA image
>   * @count: size of buf
> + * @header_size: offset in image buffer where bitstream data starts
> + * @data_size: size of bitstream. If 0, (count - header_size) will be used.
>   * @region_id: id of target region
>   * @dev: device that owns this
>   * @overlay: Device Tree overlay
> @@ -98,6 +100,8 @@ struct fpga_image_info {
>  	struct sg_table *sgt;
>  	const char *buf;
>  	size_t count;
> +	size_t header_size;
> +	size_t data_size;
>  	int region_id;
>  	struct device *dev;
>  #ifdef CONFIG_OF
> @@ -137,9 +141,13 @@ struct fpga_manager_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
> + * @initial_header_size: minimum number of bytes that should be passed into
> + *	parse_header and write_init.
>   * @state: returns an enum value of the FPGA's state
>   * @status: returns status of the FPGA, including reconfiguration error code
> + * @parse_header: parse FPGA image header to set info->header_size and
> + *	info->data_size. In case the input buffer is not large enough, set
> + *	requierd size to info->header_size and return -EAGAIN.

	required

Thanks,
Yilun

>   * @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
> @@ -155,6 +163,9 @@ 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 (*parse_header)(struct fpga_manager *mgr,
> +			    struct fpga_image_info *info,
> +			    const char *buf, size_t count);
>  	int (*write_init)(struct fpga_manager *mgr,
>  			  struct fpga_image_info *info,
>  			  const char *buf, size_t count);
> -- 
> 2.35.1
> 

Re: [PATCH v12 1/3] fpga: fpga-mgr: support bitstream offset in image buffer

[Xu Yilun]

On Sun, May 22, 2022 at 09:01:45PM +0800, Xu Yilun wrote:
> On Fri, May 13, 2022 at 07:27:53PM +0300, Ivan Bornyakov wrote:
> > At the moment FPGA manager core loads to the device entire image
> > provided to fpga_mgr_load(). But it is not always whole FPGA image
> > buffer meant to be written to the device. In particular, .dat formatted
> > image for Microchip MPF contains meta info in the header that is not
> > meant to be written to the device. This is issue for those low level
> > drivers that loads data to the device with write() fpga_manager_ops
> > callback, since write() can be called in iterator over scatter-gather
> > table, not only linear image buffer. On the other hand, write_sg()
> > callback is provided with whole image in scatter-gather form and can
> > decide itself which part should be sent to the device.
> > 
> > Add header_size and data_size to the fpga_image_info struct and adjust
> > fpga_mgr_write() callers with respect to them.
> > 
> >   * info->header_size indicates part at the beginning of image buffer
> >     that is *not* meant to be written to the device. It is optional and
> >     can be 0.
> > 
> >   * info->data_size is the size of actual bitstream data that *is* meant
> >     to be written to the device, starting at info->header_size from the
> >     beginning of image buffer. It is also optional and can be 0, which
> >     means bitstream data is up to the end of image buffer.
> > 
> > Also add parse_header() callback to fpga_manager_ops, which purpose is
> > to set info->header_size and info->data_size. At least
> > initial_header_size bytes of image buffer will be passed into
> > parse_header() first time. If it is not enough, parse_header() should
> > set desired size into info->header_size and return -EAGAIN, than it will
> > be called again with greater part of image buffer on the input.
> > 
> > Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
> > ---
> >  drivers/fpga/fpga-mgr.c       | 150 ++++++++++++++++++++++++++--------
> >  include/linux/fpga/fpga-mgr.h |  13 ++-
> >  2 files changed, 127 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> > index a3595ecc3f79..c6ca395909a0 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -74,6 +74,15 @@ static inline int fpga_mgr_write_complete(struct fpga_manager *mgr,
> >  	return 0;
> >  }
> >  
> > +static inline int fpga_mgr_parse_header(struct fpga_manager *mgr,
> > +					struct fpga_image_info *info,
> > +					const char *buf, size_t count)
> > +{
> > +	if (buf && mgr->mops->parse_header)
> > +		return mgr->mops->parse_header(mgr, info, buf, count);
> > +	return 0;
> > +}
> > +
> >  static inline int fpga_mgr_write_init(struct fpga_manager *mgr,
> >  				      struct fpga_image_info *info,
> >  				      const char *buf, size_t count)
> > @@ -136,32 +145,61 @@ void fpga_image_info_free(struct fpga_image_info *info)
> >  EXPORT_SYMBOL_GPL(fpga_image_info_free);
> >  
> >  /*
> > - * Call the low level driver's write_init function.  This will do the
> > - * device-specific things to get the FPGA into the state where it is ready to
> > - * receive an FPGA image. The low level driver only gets to see the first
> > - * initial_header_size bytes in the buffer.
> > + * Call the low level driver's parse_header then write_init functions.
> > + * This will do the device-specific things to get the FPGA into the state
> > + * where it is ready to receive an FPGA image. If parse_header sets
> > + * info->header_size, the low level driver's write_init only gets to see the
> > + * first info->header_size bytes in the buffer, mgr->mops->initial_header_size
> > + * otherwise. If neither initial_header_size nor header_size are not set,
> > + * write_init will not get any bytes of image buffer.
> 
> Could we always initialize the info->header_size = initial_header_size
> at early stage, maybe in fpga_mgr_load(), and only query
> info->header_size afterward. This is to make the logic for header size
> simpler.

Sorry I remember some existing drivers are using initial_header_size but
still deal with no buffer offset in write(), so we cannot force
info->header_size be filled. Forget about the previous comment.

Thanks,
Yilun

Re: [PATCH v12 2/3] fpga: microchip-spi: add Microchip MPF FPGA manager

[Xu Yilun]

On Fri, May 13, 2022 at 07:27:54PM +0300, Ivan Bornyakov wrote:
> Add support to the FPGA manager for programming Microchip Polarfire
> FPGAs over slave SPI interface with .dat formatted bitsream image.
> 
> Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
> ---
>  drivers/fpga/Kconfig         |   9 +
>  drivers/fpga/Makefile        |   1 +
>  drivers/fpga/microchip-spi.c | 371 +++++++++++++++++++++++++++++++++++
>  3 files changed, 381 insertions(+)
>  create mode 100644 drivers/fpga/microchip-spi.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 26025dbab353..75806ef5c9ea 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -248,4 +248,13 @@ config FPGA_MGR_VERSAL_FPGA
>  	  configure the programmable logic(PL).
>  
>  	  To compile this as a module, choose M here.
> +
> +config FPGA_MGR_MICROCHIP_SPI
> +	tristate "Microchip Polarfire SPI FPGA manager"
> +	depends on SPI
> +	help
> +	  FPGA manager driver support for Microchip Polarfire FPGAs
> +	  programming over slave SPI interface with .dat formatted
> +	  bitstream image.
> +
>  endif # FPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index e32bfa90f968..5425a15892df 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_FPGA_MGR_XILINX_SPI)	+= xilinx-spi.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>  obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA)	+= versal-fpga.o
> +obj-$(CONFIG_FPGA_MGR_MICROCHIP_SPI)	+= microchip-spi.o
>  obj-$(CONFIG_ALTERA_PR_IP_CORE)		+= altera-pr-ip-core.o
>  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)	+= altera-pr-ip-core-plat.o
>  
> diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> new file mode 100644
> index 000000000000..cb59f43000a4
> --- /dev/null
> +++ b/drivers/fpga/microchip-spi.c
> @@ -0,0 +1,371 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Microchip Polarfire FPGA programming over slave SPI interface.
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/spi/spi.h>
> +
> +#define	MPF_SPI_ISC_ENABLE	0x0B
> +#define	MPF_SPI_ISC_DISABLE	0x0C
> +#define	MPF_SPI_READ_STATUS	0x00
> +#define	MPF_SPI_READ_DATA	0x01
> +#define	MPF_SPI_FRAME_INIT	0xAE
> +#define	MPF_SPI_FRAME		0xEE
> +#define	MPF_SPI_PRG_MODE	0x01
> +#define	MPF_SPI_RELEASE		0x23
> +
> +#define	MPF_SPI_FRAME_SIZE	16
> +
> +#define	MPF_HEADER_SIZE_OFFSET	24
> +#define	MPF_DATA_SIZE_OFFSET	55
> +
> +#define	MPF_LOOKUP_TABLE_RECORD_SIZE		9
> +#define	MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET	0
> +#define	MPF_LOOKUP_TABLE_BLOCK_START_OFFSET	1
> +
> +#define	MPF_COMPONENTS_SIZE_ID	5
> +#define	MPF_BITSTREAM_ID	8
> +
> +#define	MPF_BITS_PER_COMPONENT_SIZE	22
> +
> +#define	MPF_STATUS_POLL_TIMEOUT		10000
> +#define	MPF_STATUS_BUSY			BIT(0)
> +#define	MPF_STATUS_READY		BIT(1)
> +#define	MPF_STATUS_SPI_VIOLATION	BIT(2)
> +#define	MPF_STATUS_SPI_ERROR		BIT(3)
> +
> +struct mpf_priv {
> +	struct spi_device *spi;
> +	bool program_mode;
> +};
> +
> +static int mpf_read_status(struct spi_device *spi)
> +{
> +	u8 status = 0, status_command = MPF_SPI_READ_STATUS;
> +	struct spi_transfer xfers[] = {
> +		[0 ... 1] = {
> +			.tx_buf = &status_command,
> +			.rx_buf = &status,
> +			.len = 1,
> +			.cs_change = 1,
> +		}
> +	};
> +	int ret = spi_sync_transfer(spi, xfers, 2);

I remember it is spi_w8r8 for the first time, why we change to
spi_sync_transfer? They behavior differently on spi bus.

And why we need to xfer the same message twice? If it relates to
some HW behavior, we'd better add some comments here.

> +
> +	if ((status & MPF_STATUS_SPI_VIOLATION) ||
> +	    (status & MPF_STATUS_SPI_ERROR))
> +		ret = -EIO;
> +
> +	return ret ? : status;
> +}
> +
> +static enum fpga_mgr_states mpf_ops_state(struct fpga_manager *mgr)
> +{
> +	struct mpf_priv *priv = mgr->priv;
> +	struct spi_device *spi;
> +	bool program_mode;
> +	int status;
> +
> +	spi = priv->spi;
> +	program_mode = priv->program_mode;
> +	status = mpf_read_status(spi);
> +
> +	if (!program_mode && !status)
> +		return FPGA_MGR_STATE_OPERATING;
> +
> +	return FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int mpf_ops_parse_header(struct fpga_manager *mgr,
> +				struct fpga_image_info *info,
> +				const char *buf, size_t count)
> +{
> +	size_t component_size_byte_num, component_size_byte_off,
> +	       components_size_start = 0, bitstream_start = 0,
> +	       block_id_offset, block_start_offset, i;

The initialization should be listed after declaration, so swap the 2
lines

Why the component_size_byte_num, component_size_byte_off, i should be size_t?

> +	u8 header_size, blocks_num, block_id;
> +	u32 block_start, component_size;
> +	u16 components_num;
> +
> +	if (!buf) {
> +		dev_err(&mgr->dev, "Image buffer is not provided\n");
> +		return -EINVAL;
> +	}
> +
> +	header_size = *(buf + MPF_HEADER_SIZE_OFFSET);
> +	if (header_size > count) {
> +		info->header_size = header_size;
> +		return -EAGAIN;
> +	}
> +
> +	/*
> +	 * Go through look-up table to find out where actual bitstream starts
> +	 * and where sizes of components of the bitstream lies.
> +	 */
> +	blocks_num = *(buf + header_size - 1);
> +	block_id_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET;
> +	block_start_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_START_OFFSET;
> +
> +	header_size += blocks_num * MPF_LOOKUP_TABLE_RECORD_SIZE;
> +	if (header_size > count) {
> +		info->header_size = header_size;
> +		return -EAGAIN;
> +	}
> +
> +	while (blocks_num--) {
> +		block_id = *(buf + block_id_offset);
> +		block_start = get_unaligned_le32(buf + block_start_offset);
> +
> +		switch (block_id) {
> +		case MPF_BITSTREAM_ID:
> +			info->header_size = bitstream_start = block_start;
> +			if (block_start > count)
> +				return -EAGAIN;
> +
> +			break;
> +		case MPF_COMPONENTS_SIZE_ID:
> +			components_size_start = block_start;

Why doesn't check the components_size_start against count, any difference
with bitstream_start?

> +			break;
> +		default:
> +			break;
> +		}
> +
> +		if (bitstream_start && components_size_start)
> +			break;
> +
> +		block_id_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
> +		block_start_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
> +	}
> +
> +	if (!bitstream_start || !components_size_start) {
> +		dev_err(&mgr->dev, "Failed to parse header look-up table\n");
> +		return -EFAULT;
> +	}
> +
> +	/*
> +	 * Parse bitstream size.
> +	 * Sizes of components of the bitstream are 22-bits long placed next
> +	 * to each other. Image header should be extended by now up to where
> +	 * actual bitstream starts, so no need for overflow check anymore.
> +	 */
> +	components_num = get_unaligned_le16(buf + MPF_DATA_SIZE_OFFSET);
> +
> +	for (i = 0; i < components_num; i++) {
> +		component_size_byte_num =
> +			(i * MPF_BITS_PER_COMPONENT_SIZE) / BITS_PER_BYTE;
> +		component_size_byte_off =
> +			(i * MPF_BITS_PER_COMPONENT_SIZE) % BITS_PER_BYTE;
> +
> +		component_size = get_unaligned_le32(buf +
> +						    components_size_start +
> +						    component_size_byte_num);

Some check against count?

> +		component_size >>= component_size_byte_off;
> +		component_size &= GENMASK(MPF_BITS_PER_COMPONENT_SIZE - 1, 0);
> +
> +		info->data_size += component_size * MPF_SPI_FRAME_SIZE;
> +	}
> +
> +	return 0;
> +}
> +
> +static int poll_status_not_busy(struct spi_device *spi, u8 mask)

The function name is confusing, it does more than polling busy bit.
mpf_poll_status is better?

> +{
> +	int status, timeout = MPF_STATUS_POLL_TIMEOUT;
> +
> +	while (timeout--) {
> +		status = mpf_read_status(spi);
> +		if (status < 0 ||
> +		    (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))

Please make the judgements easier to read.

	if (status < 0)
		return status;

	if (status & MPF_STATUS_BUSY)
		continue;

	if (!mask || (status & mask))
		return status;

Is that OK for you?
> +			return status;
> +	}
> +
> +	return -EBUSY;
> +}
> +
> +static int mpf_spi_write(struct spi_device *spi, const void *buf, size_t buf_size)
> +{
> +	int status = poll_status_not_busy(spi, 0);
> +
> +	if (status < 0)
> +		return status;
> +
> +	return spi_write(spi, buf, buf_size);
> +}
> +
> +static int mpf_spi_write_then_read(struct spi_device *spi,
> +				   const void *txbuf, size_t txbuf_size,
> +				   void *rxbuf, size_t rxbuf_size)
> +{
> +	const u8 read_command[] = { MPF_SPI_READ_DATA };
> +	int ret;
> +
> +	ret = mpf_spi_write(spi, txbuf, txbuf_size);
> +	if (ret)
> +		return ret;
> +
> +	ret = poll_status_not_busy(spi, MPF_STATUS_READY);
> +	if (ret < 0)
> +		return ret;
> +
> +	return spi_write_then_read(spi, read_command, sizeof(read_command),
> +				   rxbuf, rxbuf_size);
> +}
> +
> +static int mpf_ops_write_init(struct fpga_manager *mgr,
> +			      struct fpga_image_info *info, const char *buf,
> +			      size_t count)
> +{
> +	const u8 program_mode[] = { MPF_SPI_FRAME_INIT, MPF_SPI_PRG_MODE };
> +	const u8 isc_en_command[] = { MPF_SPI_ISC_ENABLE };
> +	struct mpf_priv *priv = mgr->priv;
> +	struct device *dev = &mgr->dev;
> +	struct spi_device *spi;
> +	u32 isc_ret;
> +	int ret;
> +
> +	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
> +		dev_err(dev, "Partial reconfiguration is not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	spi = priv->spi;
> +
> +	ret = mpf_spi_write_then_read(spi, isc_en_command, sizeof(isc_en_command),
> +				      &isc_ret, sizeof(isc_ret));
> +	if (ret || isc_ret) {
> +		dev_err(dev, "Failed to enable ISC: %d\n", ret ? : isc_ret);

%d for a u32 variable? Maybe just list the 2 values.

> +		return -EFAULT;
> +	}
> +
> +	ret = mpf_spi_write(spi, program_mode, sizeof(program_mode));
> +	if (ret) {
> +		dev_err(dev, "Failed to enter program mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	priv->program_mode = true;
> +
> +	return 0;
> +}
> +
> +static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count)
> +{
> +	u8 tmp_buf[MPF_SPI_FRAME_SIZE + 1] = { MPF_SPI_FRAME, };
> +	struct mpf_priv *priv = mgr->priv;
> +	struct device *dev = &mgr->dev;
> +	struct spi_device *spi;
> +	int ret, i;
> +
> +	if (count % MPF_SPI_FRAME_SIZE) {
> +		dev_err(dev, "Bitstream size is not a multiple of %d\n",
> +			MPF_SPI_FRAME_SIZE);
> +		return -EINVAL;
> +	}
> +
> +	spi = priv->spi;
> +
> +	for (i = 0; i < count / MPF_SPI_FRAME_SIZE; i++) {
> +		memcpy(tmp_buf + 1, buf + i * MPF_SPI_FRAME_SIZE,
> +		       MPF_SPI_FRAME_SIZE);
> +
> +		ret = mpf_spi_write(spi, tmp_buf, sizeof(tmp_buf));

As I mentioned before, is it possible we use spi_sync_transfer to avoid
memcpy the whole bitstream?

> +		if (ret) {
> +			dev_err(dev, "Failed to write bitstream frame %d/%zd\n",

%zu for size_t

> +				i, count / MPF_SPI_FRAME_SIZE);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int mpf_ops_write_complete(struct fpga_manager *mgr,
> +				  struct fpga_image_info *info)
> +{
> +	const u8 isc_dis_command[] = { MPF_SPI_ISC_DISABLE };
> +	const u8 release_command[] = { MPF_SPI_RELEASE };
> +	struct mpf_priv *priv = mgr->priv;
> +	struct device *dev = &mgr->dev;
> +	struct spi_device *spi;
> +	int ret;
> +
> +	spi = priv->spi;
> +
> +	ret = mpf_spi_write(spi, isc_dis_command, sizeof(isc_dis_command));
> +	if (ret) {
> +		dev_err(dev, "Failed to disable ISC: %d\n", ret);
> +		return ret;
> +	}
> +
> +	usleep_range(1000, 2000);
> +
> +	ret = mpf_spi_write(spi, release_command, sizeof(release_command));
> +	if (ret) {
> +		dev_err(dev, "Failed to exit program mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	priv->program_mode = false;
> +
> +	return 0;
> +}
> +
> +static const struct fpga_manager_ops mpf_ops = {
> +	.state = mpf_ops_state,
> +	.initial_header_size = 71,
> +	.parse_header = mpf_ops_parse_header,
> +	.write_init = mpf_ops_write_init,
> +	.write = mpf_ops_write,
> +	.write_complete = mpf_ops_write_complete,
> +};
> +
> +static int mpf_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct fpga_manager *mgr;
> +	struct mpf_priv *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->spi = spi;
> +
> +	mgr = devm_fpga_mgr_register(dev, "Microchip Polarfire SPI FPGA Manager",
> +				     &mpf_ops, priv);
> +
> +	return PTR_ERR_OR_ZERO(mgr);
> +}
> +
> +static const struct spi_device_id mpf_spi_ids[] = {
> +	{ .name = "mpf-spi-fpga-mgr", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(spi, mpf_spi_ids);
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id mpf_of_ids[] = {
> +	{ .compatible = "microchip,mpf-spi-fpga-mgr" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mpf_of_ids);
> +#endif /* IS_ENABLED(CONFIG_OF) */
> +
> +static struct spi_driver mpf_driver = {
> +	.probe = mpf_probe,
> +	.id_table = mpf_spi_ids,
> +	.driver = {
> +		.name = "microchip_mpf_spi_fpga_mgr",
> +		.of_match_table = of_match_ptr(mpf_of_ids),
> +	},
> +};
> +
> +module_spi_driver(mpf_driver);
> +
> +MODULE_DESCRIPTION("Microchip Polarfire SPI FPGA Manager");
> +MODULE_LICENSE("GPL");
> -- 
> 2.35.1
> 

Re: [PATCH v12 2/3] fpga: microchip-spi: add Microchip MPF FPGA manager

[Ivan Bornyakov]

On Mon, May 23, 2022 at 12:52:32AM +0800, Xu Yilun wrote:
> On Fri, May 13, 2022 at 07:27:54PM +0300, Ivan Bornyakov wrote:
> > Add support to the FPGA manager for programming Microchip Polarfire
> > FPGAs over slave SPI interface with .dat formatted bitsream image.
> > 
> > Signed-off-by: Ivan Bornyakov <i.bornyakov@metrotek.ru>
> > ---
> >  drivers/fpga/Kconfig         |   9 +
> >  drivers/fpga/Makefile        |   1 +
> >  drivers/fpga/microchip-spi.c | 371 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 381 insertions(+)
> >  create mode 100644 drivers/fpga/microchip-spi.c
> > 
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 26025dbab353..75806ef5c9ea 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -248,4 +248,13 @@ config FPGA_MGR_VERSAL_FPGA
> >  	  configure the programmable logic(PL).
> >  
> >  	  To compile this as a module, choose M here.
> > +
> > +config FPGA_MGR_MICROCHIP_SPI
> > +	tristate "Microchip Polarfire SPI FPGA manager"
> > +	depends on SPI
> > +	help
> > +	  FPGA manager driver support for Microchip Polarfire FPGAs
> > +	  programming over slave SPI interface with .dat formatted
> > +	  bitstream image.
> > +
> >  endif # FPGA
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index e32bfa90f968..5425a15892df 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -19,6 +19,7 @@ obj-$(CONFIG_FPGA_MGR_XILINX_SPI)	+= xilinx-spi.o
> >  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
> >  obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
> >  obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA)	+= versal-fpga.o
> > +obj-$(CONFIG_FPGA_MGR_MICROCHIP_SPI)	+= microchip-spi.o
> >  obj-$(CONFIG_ALTERA_PR_IP_CORE)		+= altera-pr-ip-core.o
> >  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)	+= altera-pr-ip-core-plat.o
> >  
> > diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> > new file mode 100644
> > index 000000000000..cb59f43000a4
> > --- /dev/null
> > +++ b/drivers/fpga/microchip-spi.c
> > @@ -0,0 +1,371 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Microchip Polarfire FPGA programming over slave SPI interface.
> > + */
> > +
> > +#include <asm/unaligned.h>
> > +#include <linux/delay.h>
> > +#include <linux/fpga/fpga-mgr.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#define	MPF_SPI_ISC_ENABLE	0x0B
> > +#define	MPF_SPI_ISC_DISABLE	0x0C
> > +#define	MPF_SPI_READ_STATUS	0x00
> > +#define	MPF_SPI_READ_DATA	0x01
> > +#define	MPF_SPI_FRAME_INIT	0xAE
> > +#define	MPF_SPI_FRAME		0xEE
> > +#define	MPF_SPI_PRG_MODE	0x01
> > +#define	MPF_SPI_RELEASE		0x23
> > +
> > +#define	MPF_SPI_FRAME_SIZE	16
> > +
> > +#define	MPF_HEADER_SIZE_OFFSET	24
> > +#define	MPF_DATA_SIZE_OFFSET	55
> > +
> > +#define	MPF_LOOKUP_TABLE_RECORD_SIZE		9
> > +#define	MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET	0
> > +#define	MPF_LOOKUP_TABLE_BLOCK_START_OFFSET	1
> > +
> > +#define	MPF_COMPONENTS_SIZE_ID	5
> > +#define	MPF_BITSTREAM_ID	8
> > +
> > +#define	MPF_BITS_PER_COMPONENT_SIZE	22
> > +
> > +#define	MPF_STATUS_POLL_TIMEOUT		10000
> > +#define	MPF_STATUS_BUSY			BIT(0)
> > +#define	MPF_STATUS_READY		BIT(1)
> > +#define	MPF_STATUS_SPI_VIOLATION	BIT(2)
> > +#define	MPF_STATUS_SPI_ERROR		BIT(3)
> > +
> > +struct mpf_priv {
> > +	struct spi_device *spi;
> > +	bool program_mode;
> > +};
> > +
> > +static int mpf_read_status(struct spi_device *spi)
> > +{
> > +	u8 status = 0, status_command = MPF_SPI_READ_STATUS;
> > +	struct spi_transfer xfers[] = {
> > +		[0 ... 1] = {
> > +			.tx_buf = &status_command,
> > +			.rx_buf = &status,
> > +			.len = 1,
> > +			.cs_change = 1,
> > +		}
> > +	};
> > +	int ret = spi_sync_transfer(spi, xfers, 2);
> 
> I remember it is spi_w8r8 for the first time, why we change to
> spi_sync_transfer? They behavior differently on spi bus.

On v8 Conor reported that spi_w8r8 was not correct way to read the status,
despite that my HW was giving reasonable result.

See https://lore.kernel.org/linux-fpga/7fcde9aa-c086-33e1-1619-04663bfeff85@microchip.com/#t

> 
> And why we need to xfer the same message twice? If it relates to
> some HW behavior, we'd better add some comments here.
> 

On v11 Conor reported that he observed inadequate status readings, and
double status reads fixed that. There is also a hint in Microchip's
"SPI-DirectC User Guide" that status should be read two times, but not a
clear statement.

See https://lore.kernel.org/linux-fpga/4b752147-1a09-a4af-bc5d-3b132b84ef49@conchuod.ie/#t

Anyway, I'll add some words.

> > +
> > +	if ((status & MPF_STATUS_SPI_VIOLATION) ||
> > +	    (status & MPF_STATUS_SPI_ERROR))
> > +		ret = -EIO;
> > +
> > +	return ret ? : status;
> > +}
> > +
> > +static enum fpga_mgr_states mpf_ops_state(struct fpga_manager *mgr)
> > +{
> > +	struct mpf_priv *priv = mgr->priv;
> > +	struct spi_device *spi;
> > +	bool program_mode;
> > +	int status;
> > +
> > +	spi = priv->spi;
> > +	program_mode = priv->program_mode;
> > +	status = mpf_read_status(spi);
> > +
> > +	if (!program_mode && !status)
> > +		return FPGA_MGR_STATE_OPERATING;
> > +
> > +	return FPGA_MGR_STATE_UNKNOWN;
> > +}
> > +
> > +static int mpf_ops_parse_header(struct fpga_manager *mgr,
> > +				struct fpga_image_info *info,
> > +				const char *buf, size_t count)
> > +{
> > +	size_t component_size_byte_num, component_size_byte_off,
> > +	       components_size_start = 0, bitstream_start = 0,
> > +	       block_id_offset, block_start_offset, i;
> 
> The initialization should be listed after declaration, so swap the 2
> lines
> 
> Why the component_size_byte_num, component_size_byte_off, i should be size_t?

It's a safe bet for offsets inside image buffer.

> > +	u8 header_size, blocks_num, block_id;
> > +	u32 block_start, component_size;
> > +	u16 components_num;
> > +
> > +	if (!buf) {
> > +		dev_err(&mgr->dev, "Image buffer is not provided\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	header_size = *(buf + MPF_HEADER_SIZE_OFFSET);
> > +	if (header_size > count) {
> > +		info->header_size = header_size;
> > +		return -EAGAIN;
> > +	}
> > +
> > +	/*
> > +	 * Go through look-up table to find out where actual bitstream starts
> > +	 * and where sizes of components of the bitstream lies.
> > +	 */
> > +	blocks_num = *(buf + header_size - 1);
> > +	block_id_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_ID_OFFSET;
> > +	block_start_offset = header_size + MPF_LOOKUP_TABLE_BLOCK_START_OFFSET;
> > +
> > +	header_size += blocks_num * MPF_LOOKUP_TABLE_RECORD_SIZE;
> > +	if (header_size > count) {
> > +		info->header_size = header_size;
> > +		return -EAGAIN;
> > +	}
> > +
> > +	while (blocks_num--) {
> > +		block_id = *(buf + block_id_offset);
> > +		block_start = get_unaligned_le32(buf + block_start_offset);
> > +
> > +		switch (block_id) {
> > +		case MPF_BITSTREAM_ID:
> > +			info->header_size = bitstream_start = block_start;
> > +			if (block_start > count)
> > +				return -EAGAIN;
> > +
> > +			break;
> > +		case MPF_COMPONENTS_SIZE_ID:
> > +			components_size_start = block_start;
> 
> Why doesn't check the components_size_start against count, any difference
> with bitstream_start?

bitstream_start is in fact a synonym for info->header_size, while
components_size_start is somewhere in the header. Thus, data at offset
'components_size_start' is accessed when count is not less then
info->header_size.

> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +
> > +		if (bitstream_start && components_size_start)
> > +			break;
> > +
> > +		block_id_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
> > +		block_start_offset += MPF_LOOKUP_TABLE_RECORD_SIZE;
> > +	}
> > +
> > +	if (!bitstream_start || !components_size_start) {
> > +		dev_err(&mgr->dev, "Failed to parse header look-up table\n");
> > +		return -EFAULT;
> > +	}
> > +
> > +	/*
> > +	 * Parse bitstream size.
> > +	 * Sizes of components of the bitstream are 22-bits long placed next
> > +	 * to each other. Image header should be extended by now up to where
> > +	 * actual bitstream starts, so no need for overflow check anymore.
> > +	 */
> > +	components_num = get_unaligned_le16(buf + MPF_DATA_SIZE_OFFSET);
> > +
> > +	for (i = 0; i < components_num; i++) {
> > +		component_size_byte_num =
> > +			(i * MPF_BITS_PER_COMPONENT_SIZE) / BITS_PER_BYTE;
> > +		component_size_byte_off =
> > +			(i * MPF_BITS_PER_COMPONENT_SIZE) % BITS_PER_BYTE;
> > +
> > +		component_size = get_unaligned_le32(buf +
> > +						    components_size_start +
> > +						    component_size_byte_num);
> 
> Some check against count?

Same, also see comment just above for-loop.

> > +		component_size >>= component_size_byte_off;
> > +		component_size &= GENMASK(MPF_BITS_PER_COMPONENT_SIZE - 1, 0);
> > +
> > +		info->data_size += component_size * MPF_SPI_FRAME_SIZE;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> 
> The function name is confusing, it does more than polling busy bit.
> mpf_poll_status is better?

Well, my intention was to denote that we poll status until busy bit is
not set. I have no strong feelings for this function naming, tho.

> > +{
> > +	int status, timeout = MPF_STATUS_POLL_TIMEOUT;
> > +
> > +	while (timeout--) {
> > +		status = mpf_read_status(spi);
> > +		if (status < 0 ||
> > +		    (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
> 
> Please make the judgements easier to read.
> 
> 	if (status < 0)
> 		return status;
> 
> 	if (status & MPF_STATUS_BUSY)
> 		continue;
> 
> 	if (!mask || (status & mask))
> 		return status;
> 
> Is that OK for you?

Yeah, sure.

> > +			return status;
> > +	}
> > +
> > +	return -EBUSY;
> > +}
> > +
> > +static int mpf_spi_write(struct spi_device *spi, const void *buf, size_t buf_size)
> > +{
> > +	int status = poll_status_not_busy(spi, 0);
> > +
> > +	if (status < 0)
> > +		return status;
> > +
> > +	return spi_write(spi, buf, buf_size);
> > +}
> > +
> > +static int mpf_spi_write_then_read(struct spi_device *spi,
> > +				   const void *txbuf, size_t txbuf_size,
> > +				   void *rxbuf, size_t rxbuf_size)
> > +{
> > +	const u8 read_command[] = { MPF_SPI_READ_DATA };
> > +	int ret;
> > +
> > +	ret = mpf_spi_write(spi, txbuf, txbuf_size);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = poll_status_not_busy(spi, MPF_STATUS_READY);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return spi_write_then_read(spi, read_command, sizeof(read_command),
> > +				   rxbuf, rxbuf_size);
> > +}
> > +
> > +static int mpf_ops_write_init(struct fpga_manager *mgr,
> > +			      struct fpga_image_info *info, const char *buf,
> > +			      size_t count)
> > +{
> > +	const u8 program_mode[] = { MPF_SPI_FRAME_INIT, MPF_SPI_PRG_MODE };
> > +	const u8 isc_en_command[] = { MPF_SPI_ISC_ENABLE };
> > +	struct mpf_priv *priv = mgr->priv;
> > +	struct device *dev = &mgr->dev;
> > +	struct spi_device *spi;
> > +	u32 isc_ret;
> > +	int ret;
> > +
> > +	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
> > +		dev_err(dev, "Partial reconfiguration is not supported\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	spi = priv->spi;
> > +
> > +	ret = mpf_spi_write_then_read(spi, isc_en_command, sizeof(isc_en_command),
> > +				      &isc_ret, sizeof(isc_ret));
> > +	if (ret || isc_ret) {
> > +		dev_err(dev, "Failed to enable ISC: %d\n", ret ? : isc_ret);
> 
> %d for a u32 variable? Maybe just list the 2 values.

Ok, will do.

> > +		return -EFAULT;
> > +	}
> > +
> > +	ret = mpf_spi_write(spi, program_mode, sizeof(program_mode));
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enter program mode: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	priv->program_mode = true;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count)
> > +{
> > +	u8 tmp_buf[MPF_SPI_FRAME_SIZE + 1] = { MPF_SPI_FRAME, };
> > +	struct mpf_priv *priv = mgr->priv;
> > +	struct device *dev = &mgr->dev;
> > +	struct spi_device *spi;
> > +	int ret, i;
> > +
> > +	if (count % MPF_SPI_FRAME_SIZE) {
> > +		dev_err(dev, "Bitstream size is not a multiple of %d\n",
> > +			MPF_SPI_FRAME_SIZE);
> > +		return -EINVAL;
> > +	}
> > +
> > +	spi = priv->spi;
> > +
> > +	for (i = 0; i < count / MPF_SPI_FRAME_SIZE; i++) {
> > +		memcpy(tmp_buf + 1, buf + i * MPF_SPI_FRAME_SIZE,
> > +		       MPF_SPI_FRAME_SIZE);
> > +
> > +		ret = mpf_spi_write(spi, tmp_buf, sizeof(tmp_buf));
> 
> As I mentioned before, is it possible we use spi_sync_transfer to avoid
> memcpy the whole bitstream?

Unfortunately, I didn't succeed with spi_sunc_transfer here. May be
Conor or other folks with more insight on Microchip's HW would be able
to eliminate this memcpy...

> > +		if (ret) {
> > +			dev_err(dev, "Failed to write bitstream frame %d/%zd\n",
> 
> %zu for size_t
> 
> > +				i, count / MPF_SPI_FRAME_SIZE);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mpf_ops_write_complete(struct fpga_manager *mgr,
> > +				  struct fpga_image_info *info)
> > +{
> > +	const u8 isc_dis_command[] = { MPF_SPI_ISC_DISABLE };
> > +	const u8 release_command[] = { MPF_SPI_RELEASE };
> > +	struct mpf_priv *priv = mgr->priv;
> > +	struct device *dev = &mgr->dev;
> > +	struct spi_device *spi;
> > +	int ret;
> > +
> > +	spi = priv->spi;
> > +
> > +	ret = mpf_spi_write(spi, isc_dis_command, sizeof(isc_dis_command));
> > +	if (ret) {
> > +		dev_err(dev, "Failed to disable ISC: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	usleep_range(1000, 2000);
> > +
> > +	ret = mpf_spi_write(spi, release_command, sizeof(release_command));
> > +	if (ret) {
> > +		dev_err(dev, "Failed to exit program mode: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	priv->program_mode = false;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct fpga_manager_ops mpf_ops = {
> > +	.state = mpf_ops_state,
> > +	.initial_header_size = 71,
> > +	.parse_header = mpf_ops_parse_header,
> > +	.write_init = mpf_ops_write_init,
> > +	.write = mpf_ops_write,
> > +	.write_complete = mpf_ops_write_complete,
> > +};
> > +
> > +static int mpf_probe(struct spi_device *spi)
> > +{
> > +	struct device *dev = &spi->dev;
> > +	struct fpga_manager *mgr;
> > +	struct mpf_priv *priv;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->spi = spi;
> > +
> > +	mgr = devm_fpga_mgr_register(dev, "Microchip Polarfire SPI FPGA Manager",
> > +				     &mpf_ops, priv);
> > +
> > +	return PTR_ERR_OR_ZERO(mgr);
> > +}
> > +
> > +static const struct spi_device_id mpf_spi_ids[] = {
> > +	{ .name = "mpf-spi-fpga-mgr", },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(spi, mpf_spi_ids);
> > +
> > +#if IS_ENABLED(CONFIG_OF)
> > +static const struct of_device_id mpf_of_ids[] = {
> > +	{ .compatible = "microchip,mpf-spi-fpga-mgr" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, mpf_of_ids);
> > +#endif /* IS_ENABLED(CONFIG_OF) */
> > +
> > +static struct spi_driver mpf_driver = {
> > +	.probe = mpf_probe,
> > +	.id_table = mpf_spi_ids,
> > +	.driver = {
> > +		.name = "microchip_mpf_spi_fpga_mgr",
> > +		.of_match_table = of_match_ptr(mpf_of_ids),
> > +	},
> > +};
> > +
> > +module_spi_driver(mpf_driver);
> > +
> > +MODULE_DESCRIPTION("Microchip Polarfire SPI FPGA Manager");
> > +MODULE_LICENSE("GPL");
> > -- 
> > 2.35.1
> > 

Re: [PATCH v12 2/3] fpga: microchip-spi: add Microchip MPF FPGA manager

[Conor.Dooley]

On 22/05/2022 21:22, Ivan Bornyakov wrote:
> On Mon, May 23, 2022 at 12:52:32AM +0800, Xu Yilun wrote:
>> On Fri, May 13, 2022 at 07:27:54PM +0300, Ivan Bornyakov wrote:
>>> Add support to the FPGA manager for programming Microchip Polarfire
>>> FPGAs over slave SPI interface with .dat formatted bitsream image.

---8<---

>>> +static int mpf_read_status(struct spi_device *spi)
>>> +{
>>> +	u8 status = 0, status_command = MPF_SPI_READ_STATUS;
>>> +	struct spi_transfer xfers[] = {
>>> +		[0 ... 1] = {
>>> +			.tx_buf = &status_command,
>>> +			.rx_buf = &status,
>>> +			.len = 1,
>>> +			.cs_change = 1,
>>> +		}
>>> +	};
>>> +	int ret = spi_sync_transfer(spi, xfers, 2);
>>
>> I remember it is spi_w8r8 for the first time, why we change to
>> spi_sync_transfer? They behavior differently on spi bus.
> 
> On v8 Conor reported that spi_w8r8 was not correct way to read the status,
> despite that my HW was giving reasonable result.
> 
> See https://lore.kernel.org/linux-fpga/7fcde9aa-c086-33e1-1619-04663bfeff85@microchip.com/#t

For some context, the spi slave programming was implemented to match the
jtag programming process. The status can't be read with w8r8() because it
comes directly from the hardware between the pins on the FPGA and the spi
slave & will start clocking it out onto MISO as soon as it has a clock
and its select is active.
I am not really sure how it worked that way for you.

> 
>>
>> And why we need to xfer the same message twice? If it relates to
>> some HW behavior, we'd better add some comments here.
>>
> 
> On v11 Conor reported that he observed inadequate status readings, and
> double status reads fixed that. There is also a hint in Microchip's
> "SPI-DirectC User Guide" that status should be read two times, but not a
> clear statement.
> 
> See https://lore.kernel.org/linux-fpga/4b752147-1a09-a4af-bc5d-3b132b84ef49@conchuod.ie/#t
> 
> Anyway, I'll add some words.

I'll be honest and say that the DirectC user guide is not great, I had
a look for that "hint" but it didn't stand out to me.
Unless that is you meant two screenshots of "checking hardware status"
at this URL in section 10's subsections?
https://onlinedocs.microchip.com/pr/GUID-835917AF-E521-4046-AD59-DCB458EB8466-en-US-1/index.html

If that is the case, that seems very insufficient to me. I will try to
get clarification & suggest that the documentation be improved if so.

---8<---

>>> +
>>> +static int mpf_ops_write(struct fpga_manager *mgr, const char *buf, size_t count)
>>> +{
>>> +	u8 tmp_buf[MPF_SPI_FRAME_SIZE + 1] = { MPF_SPI_FRAME, };
>>> +	struct mpf_priv *priv = mgr->priv;
>>> +	struct device *dev = &mgr->dev;
>>> +	struct spi_device *spi;
>>> +	int ret, i;
>>> +
>>> +	if (count % MPF_SPI_FRAME_SIZE) {
>>> +		dev_err(dev, "Bitstream size is not a multiple of %d\n",
>>> +			MPF_SPI_FRAME_SIZE);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	spi = priv->spi;
>>> +
>>> +	for (i = 0; i < count / MPF_SPI_FRAME_SIZE; i++) {
>>> +		memcpy(tmp_buf + 1, buf + i * MPF_SPI_FRAME_SIZE,
>>> +		       MPF_SPI_FRAME_SIZE);
>>> +
>>> +		ret = mpf_spi_write(spi, tmp_buf, sizeof(tmp_buf));
>>
>> As I mentioned before, is it possible we use spi_sync_transfer to avoid
>> memcpy the whole bitstream?
> 
> Unfortunately, I didn't succeed with spi_sunc_transfer here. May be
> Conor or other folks with more insight on Microchip's HW would be able
> to eliminate this memcpy...

Sure, I'll take a look. If I can't answer I'll try to find out.

I've been really busy the last week (and will be this coming week too) so
while I did use v12, I have not been able to properly sit down and look
at it properly yet, so apologies for the radio silence from me. Hopefully
I will get a chance this week & can give either a review or ack etc.

Thanks,
Conor.

> 
>>> +		if (ret) {
>>> +			dev_err(dev, "Failed to write bitstream frame %d/%zd\n",
>>
>> %zu for size_t
>>
>>> +				i, count / MPF_SPI_FRAME_SIZE);
>>> +			return ret;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int mpf_ops_write_complete(struct fpga_manager *mgr,
>>> +				  struct fpga_image_info *info)
>>> +{
>>> +	const u8 isc_dis_command[] = { MPF_SPI_ISC_DISABLE };
>>> +	const u8 release_command[] = { MPF_SPI_RELEASE };
>>> +	struct mpf_priv *priv = mgr->priv;
>>> +	struct device *dev = &mgr->dev;
>>> +	struct spi_device *spi;
>>> +	int ret;
>>> +
>>> +	spi = priv->spi;
>>> +
>>> +	ret = mpf_spi_write(spi, isc_dis_command, sizeof(isc_dis_command));
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to disable ISC: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	usleep_range(1000, 2000);
>>> +
>>> +	ret = mpf_spi_write(spi, release_command, sizeof(release_command));
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to exit program mode: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	priv->program_mode = false;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct fpga_manager_ops mpf_ops = {
>>> +	.state = mpf_ops_state,
>>> +	.initial_header_size = 71,
>>> +	.parse_header = mpf_ops_parse_header,
>>> +	.write_init = mpf_ops_write_init,
>>> +	.write = mpf_ops_write,
>>> +	.write_complete = mpf_ops_write_complete,
>>> +};
>>> +
>>> +static int mpf_probe(struct spi_device *spi)
>>> +{
>>> +	struct device *dev = &spi->dev;
>>> +	struct fpga_manager *mgr;
>>> +	struct mpf_priv *priv;
>>> +
>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	priv->spi = spi;
>>> +
>>> +	mgr = devm_fpga_mgr_register(dev, "Microchip Polarfire SPI FPGA Manager",
>>> +				     &mpf_ops, priv);
>>> +
>>> +	return PTR_ERR_OR_ZERO(mgr);
>>> +}
>>> +
>>> +static const struct spi_device_id mpf_spi_ids[] = {
>>> +	{ .name = "mpf-spi-fpga-mgr", },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(spi, mpf_spi_ids);
>>> +
>>> +#if IS_ENABLED(CONFIG_OF)
>>> +static const struct of_device_id mpf_of_ids[] = {
>>> +	{ .compatible = "microchip,mpf-spi-fpga-mgr" },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, mpf_of_ids);
>>> +#endif /* IS_ENABLED(CONFIG_OF) */
>>> +
>>> +static struct spi_driver mpf_driver = {
>>> +	.probe = mpf_probe,
>>> +	.id_table = mpf_spi_ids,
>>> +	.driver = {
>>> +		.name = "microchip_mpf_spi_fpga_mgr",
>>> +		.of_match_table = of_match_ptr(mpf_of_ids),
>>> +	},
>>> +};
>>> +
>>> +module_spi_driver(mpf_driver);
>>> +
>>> +MODULE_DESCRIPTION("Microchip Polarfire SPI FPGA Manager");
>>> +MODULE_LICENSE("GPL");
>>> -- 
>>> 2.35.1
>>>
>