[PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara rao]

Inorder to debug issues with fpga's users would
like to read the fpga configuration information.
This patch adds readback support for fpga configuration data
in the framework through debugfs interface.

Usage:
        cat /sys/kernel/debug/fpga/fpga0/image

Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
---
Changes for v4:
--> None.
Changes for v3:
--> None.
Changes for v2:
--> Fixed debug attribute path and name as suggested by Alan
--> Add config entry for DEBUG as suggested by Alan
--> Fixed trival coding style issues.

 drivers/fpga/Kconfig          |  7 +++++
 drivers/fpga/fpga-mgr.c       | 68 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/fpga/fpga-mgr.h |  5 ++++
 3 files changed, 80 insertions(+)

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 53d3f55..838ad4e 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -11,6 +11,13 @@ menuconfig FPGA
 
 if FPGA
 
+config FPGA_MGR_DEBUG_FS
+	tristate "FPGA Debug fs"
+	select DEBUG_FS
+	help
+	  FPGA manager debug provides support for reading fpga configuration
+	  information.
+
 config FPGA_MGR_SOCFPGA
 	tristate "Altera SOCFPGA FPGA Manager"
 	depends on ARCH_SOCFPGA || COMPILE_TEST
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 9939d2c..4bea860 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+#include <linux/debugfs.h>
+
+static int fpga_mgr_read(struct seq_file *s, void *data)
+{
+	struct fpga_manager *mgr = (struct fpga_manager *)s->private;
+	int ret = 0;
+
+	if (!mgr->mops->read)
+		return -ENOENT;
+
+	if (!mutex_trylock(&mgr->ref_mutex))
+		return -EBUSY;
+
+	if (mgr->state != FPGA_MGR_STATE_OPERATING) {
+		ret = -EPERM;
+		goto err_unlock;
+	}
+
+	/* Read the FPGA configuration data from the fabric */
+	ret = mgr->mops->read(mgr, s);
+	if (ret)
+		dev_err(&mgr->dev, "Error while reading configuration data from FPGA\n");
+
+err_unlock:
+	mutex_unlock(&mgr->ref_mutex);
+
+	return ret;
+}
+
+static int fpga_mgr_read_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, fpga_mgr_read, inode->i_private);
+}
+
+static const struct file_operations fpga_mgr_ops_image = {
+	.owner = THIS_MODULE,
+	.open = fpga_mgr_read_open,
+	.read = seq_read,
+};
+#endif
+
 /**
  * fpga_mgr_lock - Lock FPGA manager for exclusive use
  * @mgr:	fpga manager
@@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const char *name,
 	if (ret)
 		goto error_device;
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+	struct dentry *d, *parent;
+
+	mgr->dir = debugfs_create_dir("fpga", NULL);
+	if (!mgr->dir)
+		goto error_device;
+
+	parent = mgr->dir;
+	d = debugfs_create_dir(mgr->dev.kobj.name, parent);
+	if (!d) {
+		debugfs_remove_recursive(parent);
+		goto error_device;
+	}
+
+	parent = d;
+	d = debugfs_create_file("image", 0644, parent, mgr,
+				&fpga_mgr_ops_image);
+	if (!d) {
+		debugfs_remove_recursive(mgr->dir);
+		goto error_device;
+	}
+#endif
+
 	dev_info(&mgr->dev, "%s registered\n", mgr->name);
 
 	return 0;
@@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
 
 	dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+	debugfs_remove_recursive(mgr->dir);
+#endif
 	/*
 	 * If the low level driver provides a method for putting fpga into
 	 * a desired state upon unregister, do it.
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 3c6de23..e9e17a9 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -114,6 +114,7 @@ struct fpga_image_info {
  * @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
+ * @read: optional: read FPGA configuration information
  * @fpga_remove: optional: Set FPGA into a specific state during driver remove
  * @groups: optional attribute groups.
  *
@@ -131,6 +132,7 @@ struct fpga_manager_ops {
 	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
 	int (*write_complete)(struct fpga_manager *mgr,
 			      struct fpga_image_info *info);
+	int (*read)(struct fpga_manager *mgr, struct seq_file *s);
 	void (*fpga_remove)(struct fpga_manager *mgr);
 	const struct attribute_group **groups;
 };
@@ -151,6 +153,9 @@ struct fpga_manager {
 	enum fpga_mgr_states state;
 	const struct fpga_manager_ops *mops;
 	void *priv;
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+	struct dentry *dir;
+#endif
 };
 
 #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
-- 
2.7.4

[PATCH v4 2/2] fpga: zynq-fpga: Add support for readback of FPGA configuration data and registers

[Appana Durga Kedareswara rao]

This patch adds support for readback of FPGA configuration data and registers.

Usage:
Readback of PL configuration data
        cat /sys/kernel/debug/fpga/fpga0/image
Readback of PL configuration registers
        cat /sys/kernel/debug/fpga/f8007000.devcfg/cfg_reg

Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
---
Changes for v4:
--> Improved commit message description as suggested by Moritz.
--> Used GENMASK and BIT() Macros wherever applicable as suggested by Moritz.
--> Fixed commenting sytle issues as suggested by Moritz and Alan.
--> Get rid of the readback_type module param as suggested by Alan and Moritz.
Changes for v3:
--> Added support for pl configuration data readback
--> Improved the pl configuration register readback logic.
Changes for v2:
--> Removed locks from the read_ops as lock handling is done in the framework.

 drivers/fpga/zynq-fpga.c | 430 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 430 insertions(+)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 70b15b3..1746d18 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -31,6 +31,7 @@
 #include <linux/regmap.h>
 #include <linux/string.h>
 #include <linux/scatterlist.h>
+#include <linux/seq_file.h>
 
 /* Offsets into SLCR regmap */
 
@@ -127,6 +128,69 @@
 /* Disable global resets */
 #define FPGA_RST_NONE_MASK		0x0
 
+/**
+ * struct zynq_configreg - Configuration register offsets
+ * @reg:	Name of the configuration register.
+ * @offset:	Register offset.
+ */
+struct zynq_configreg {
+	char *reg;
+	u32 offset;
+};
+
+static struct zynq_configreg cfgreg[] = {
+	{.reg = "CRC",		.offset = 0},
+	{.reg = "FAR",		.offset = 1},
+	{.reg = "FDRI",		.offset = 2},
+	{.reg = "FDRO",		.offset = 3},
+	{.reg = "CMD",		.offset = 4},
+	{.reg = "CTRL0",	.offset = 5},
+	{.reg = "MASK",		.offset = 6},
+	{.reg = "STAT",		.offset = 7},
+	{.reg = "LOUT",		.offset = 8},
+	{.reg = "COR0",		.offset = 9},
+	{.reg = "MFWR",		.offset = 10},
+	{.reg = "CBC",		.offset = 11},
+	{.reg = "IDCODE",	.offset = 12},
+	{.reg = "AXSS",		.offset = 13},
+	{.reg = "COR1",		.offset = 14},
+	{.reg = "WBSTR",	.offset = 16},
+	{.reg = "TIMER",	.offset = 17},
+	{.reg = "BOOTSTS",	.offset = 22},
+	{.reg = "CTRL1",	.offset = 24},
+	{}
+};
+
+/* Masks for Configuration registers */
+#define FAR_ADDR_MASK		0x00000000
+#define RCFG_CMD_MASK		BIT(2)
+#define START_CMD_MASK		BIT(2) + BIT(0)
+#define RCRC_CMD_MASK		GENMASK(2, 0)
+#define SHUTDOWN_CMD_MASK	GENMASK(1, 0) + BIT(3)
+#define DESYNC_WORD_MASK	GENMASK(2, 3) + BIT(0)
+#define BUSWIDTH_SYNCWORD_MASK	0x000000BB
+#define NOOP_WORD_MASK		BIT(29)
+#define BUSWIDTH_DETECT_MASK	0x11220044
+#define SYNC_WORD_MASK		0xAA995566
+#define DUMMY_WORD_MASK		GENMASK(31, 0)
+
+#define TYPE_HDR_SHIFT		29
+#define TYPE_REG_SHIFT		13
+#define TYPE_OP_SHIFT		27
+#define TYPE_OPCODE_NOOP	0
+#define TYPE_OPCODE_READ	1
+#define TYPE_OPCODE_WRITE	2
+#define TYPE_FAR_OFFSET		1
+#define TYPE_FDRO_OFFSET	3
+#define TYPE_CMD_OFFSET		4
+
+#define READ_STEP5_NOOPS	6
+#define READ_STEP9_NOOPS	32
+
+#define READ_DMA_SIZE		0x200
+#define DUMMY_FRAMES_SIZE	0x28
+#define SLCR_PCAP_FREQ		10000000
+
 struct zynq_fpga_priv {
 	int irq;
 	struct clk *clk;
@@ -140,6 +204,11 @@ struct zynq_fpga_priv {
 	struct scatterlist *cur_sg;
 
 	struct completion dma_done;
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+	struct mutex ref_mutex;
+	struct dentry *dir;
+#endif
+	u32 size;
 };
 
 static inline void zynq_fpga_write(struct zynq_fpga_priv *priv, u32 offset,
@@ -164,6 +233,27 @@ static inline void zynq_fpga_set_irq(struct zynq_fpga_priv *priv, u32 enable)
 	zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
 }
 
+static void zynq_fpga_dma_xfer(struct zynq_fpga_priv *priv, u32 srcaddr,
+			       u32 srclen, u32 dstaddr, u32 dstlen)
+{
+	zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, srcaddr);
+	zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, dstaddr);
+	zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, srclen);
+	zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, dstlen);
+}
+
+static int zynq_fpga_wait_fordone(struct zynq_fpga_priv *priv)
+{
+	u32 status;
+	int ret;
+
+	ret = zynq_fpga_poll_timeout(priv, INT_STS_OFFSET, status,
+				     status & IXR_D_P_DONE_MASK,
+				     INIT_POLL_DELAY,
+				     INIT_POLL_TIMEOUT);
+	return ret;
+}
+
 /* Must be called with dma_lock held */
 static void zynq_step_dma(struct zynq_fpga_priv *priv)
 {
@@ -192,6 +282,7 @@ static void zynq_step_dma(struct zynq_fpga_priv *priv)
 			priv->dma_elm++;
 		}
 
+		priv->size += len;
 		zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, addr);
 		zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, DMA_INVALID_ADDRESS);
 		zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, len / 4);
@@ -401,6 +492,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr, struct sg_table *sgt)
 	int i;
 
 	priv = mgr->priv;
+	priv->size = 0;
 
 	/* The hardware can only DMA multiples of 4 bytes, and it requires the
 	 * starting addresses to be aligned to 64 bits (UG585 pg 212).
@@ -546,12 +638,327 @@ static enum fpga_mgr_states zynq_fpga_ops_state(struct fpga_manager *mgr)
 	return FPGA_MGR_STATE_UNKNOWN;
 }
 
+static int zynq_type1_pkt(u8 reg, u8 opcode, u16 size)
+{
+	/*
+	 * Type 1 Packet Header Format
+	 * The header section is always a 32-bit word.
+	 *
+	 * HeaderType | Opcode | Register Address | Reserved | Word Count
+	 * [31:29]      [28:27]         [26:13]      [12:11]     [10:0]
+	 * --------------------------------------------------------------
+	 *   001          xx      RRRRRRRRRxxxxx        RR      xxxxxxxxxxx
+	 *
+	 * @R: means the bit is not used and reserved for future use.
+	 * The reserved bits should be written as 0s.
+	 *
+	 * Generating the Type 1 packet header which involves shifting of Type1
+	 * Header Mask, Register value and the OpCode which is 01 in this case
+	 * as only read operation is to be carried out and then performing OR
+	 * operation with the Word Length.
+	 * For more details refer ug470 Packet Types section Table 5-20.
+	 */
+	return (((1 << TYPE_HDR_SHIFT) |
+		(reg << TYPE_REG_SHIFT) |
+		(opcode << TYPE_OP_SHIFT)) | size);
+
+}
+
+static int zynq_type2_pkt(u8 opcode, u32 size)
+{
+	/*
+	 * Type 2 Packet Header Format
+	 * The header section is always a 32-bit word.
+	 *
+	 * HeaderType | Opcode |  Word Count
+	 * [31:29]      [28:27]         [26:0]
+	 * --------------------------------------------------------------
+	 *   010          xx      xxxxxxxxxxxxx
+	 *
+	 * @R: means the bit is not used and reserved for future use.
+	 * The reserved bits should be written as 0s.
+	 *
+	 * Generating the Type 2 packet header which involves shifting of Type 2
+	 * Header Mask, OpCode and then performing OR operation with the Word
+	 * Length. For more details refer ug470 Packet Types section
+	 * Table 5-22.
+	 */
+	return (((2 << TYPE_HDR_SHIFT) |
+		(opcode << TYPE_OP_SHIFT)) | size);
+}
+
+static int zynq_fpga_ops_read_image(struct fpga_manager *mgr,
+				    struct seq_file *s)
+{
+	struct zynq_fpga_priv *priv = mgr->priv;
+	int ret = 0, cmdindex, clk_rate;
+	u32 intr_status, status, i;
+	dma_addr_t dma_addr;
+	unsigned int *buf;
+	size_t size;
+
+	ret = clk_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	size = priv->size + READ_DMA_SIZE + DUMMY_FRAMES_SIZE;
+	buf = dma_zalloc_coherent(mgr->dev.parent, size,
+				 &dma_addr, GFP_KERNEL);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto disable_clk;
+	}
+
+	seq_puts(s, "Zynq FPGA Configuration data contents are\n");
+
+	/*
+	 * There is no h/w flow control for pcap read
+	 * to prevent the FIFO from over flowing, reduce
+	 * the PCAP operating frequency.
+	 */
+	clk_rate = clk_get_rate(priv->clk);
+	ret = clk_set_rate(priv->clk, SLCR_PCAP_FREQ);
+	if (ret) {
+		dev_err(&mgr->dev, "Unable to reduce the PCAP freq\n");
+		goto free_dmabuf;
+	}
+
+	ret = zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status,
+				     status & STATUS_PCFG_INIT_MASK,
+				     INIT_POLL_DELAY,
+				     INIT_POLL_TIMEOUT);
+	if (ret) {
+		dev_err(&mgr->dev, "Timeout waiting for PCFG_INIT\n");
+		goto restore_pcap_clk;
+	}
+
+	cmdindex = 0;
+	buf[cmdindex++] = DUMMY_WORD_MASK;
+	buf[cmdindex++] = BUSWIDTH_SYNCWORD_MASK;
+	buf[cmdindex++] = BUSWIDTH_DETECT_MASK;
+	buf[cmdindex++] = DUMMY_WORD_MASK;
+	buf[cmdindex++] = SYNC_WORD_MASK;
+	buf[cmdindex++] = NOOP_WORD_MASK;
+	buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET, TYPE_OPCODE_WRITE,
+					 1);
+	buf[cmdindex++] = SHUTDOWN_CMD_MASK;
+	buf[cmdindex++] = NOOP_WORD_MASK;
+	buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET, TYPE_OPCODE_WRITE,
+					 1);
+	buf[cmdindex++] = RCRC_CMD_MASK;
+	for (i = 0; i < READ_STEP5_NOOPS; i++)
+		buf[cmdindex++] = NOOP_WORD_MASK;
+	buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET, TYPE_OPCODE_WRITE,
+					 1);
+	buf[cmdindex++] = RCFG_CMD_MASK;
+	buf[cmdindex++] = NOOP_WORD_MASK;
+	buf[cmdindex++] = zynq_type1_pkt(TYPE_FAR_OFFSET, TYPE_OPCODE_WRITE,
+					 1);
+	buf[cmdindex++] = FAR_ADDR_MASK;
+	buf[cmdindex++] = zynq_type1_pkt(TYPE_FDRO_OFFSET, TYPE_OPCODE_READ,
+					 0);
+	buf[cmdindex++] = zynq_type2_pkt(TYPE_OPCODE_READ, priv->size/4);
+	for (i = 0; i < READ_STEP9_NOOPS; i++)
+		buf[cmdindex++] = NOOP_WORD_MASK;
+
+	intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
+	zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
+
+	/* Write to PCAP */
+	zynq_fpga_dma_xfer(priv, dma_addr, cmdindex,
+			   DMA_INVALID_ADDRESS, 0);
+	ret = zynq_fpga_wait_fordone(priv);
+	if (ret) {
+		dev_err(&mgr->dev, "SRCDMA: Timeout waiting for D_P_DONE\n");
+		goto restore_pcap_clk;
+	}
+	intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
+	zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
+
+	/* Read From PACP */
+	zynq_fpga_dma_xfer(priv, DMA_INVALID_ADDRESS, 0,
+			   dma_addr + READ_DMA_SIZE, priv->size/4);
+	ret = zynq_fpga_wait_fordone(priv);
+	if (ret) {
+		dev_err(&mgr->dev, "DSTDMA: Timeout waiting for D_P_DONE\n");
+		goto restore_pcap_clk;
+	}
+	intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
+	zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
+
+	/* Write to PCAP */
+	cmdindex = 0;
+	buf[cmdindex++] = NOOP_WORD_MASK;
+	buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET,
+					 TYPE_OPCODE_WRITE, 1);
+	buf[cmdindex++] = START_CMD_MASK;
+	buf[cmdindex++] = NOOP_WORD_MASK;
+
+	buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET,
+					 TYPE_OPCODE_WRITE, 1);
+	buf[cmdindex++] = RCRC_CMD_MASK;
+	buf[cmdindex++] = NOOP_WORD_MASK;
+	buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET,
+					 TYPE_OPCODE_WRITE, 1);
+	buf[cmdindex++] = DESYNC_WORD_MASK;
+	buf[cmdindex++] = NOOP_WORD_MASK;
+	buf[cmdindex++] = NOOP_WORD_MASK;
+
+	zynq_fpga_dma_xfer(priv, dma_addr, cmdindex,
+			   DMA_INVALID_ADDRESS, 0);
+	ret = zynq_fpga_wait_fordone(priv);
+	if (ret) {
+		dev_err(&mgr->dev, "SRCDMA1: Timeout waiting for D_P_DONE\n");
+		goto restore_pcap_clk;
+	}
+
+	seq_write(s, &buf[READ_DMA_SIZE/4], priv->size);
+
+restore_pcap_clk:
+	clk_set_rate(priv->clk, clk_rate);
+free_dmabuf:
+	dma_free_coherent(mgr->dev.parent, size, buf,
+			  dma_addr);
+disable_clk:
+	clk_disable(priv->clk);
+	return ret;
+}
+
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+#include <linux/debugfs.h>
+
+static int zynq_fpga_getconfigreg(struct fpga_manager *mgr, u8 reg,
+				  dma_addr_t dma_addr, int *buf)
+{
+	struct zynq_fpga_priv *priv = mgr->priv;
+	int ret = 0, cmdindex, src_dmaoffset;
+	u32 intr_status, status;
+
+	src_dmaoffset = 0x8;
+	cmdindex = 2;
+	buf[cmdindex++] = DUMMY_WORD_MASK;
+	buf[cmdindex++] = BUSWIDTH_SYNCWORD_MASK;
+	buf[cmdindex++] = BUSWIDTH_DETECT_MASK;
+	buf[cmdindex++] = DUMMY_WORD_MASK;
+	buf[cmdindex++] = SYNC_WORD_MASK;
+	buf[cmdindex++] = NOOP_WORD_MASK;
+	buf[cmdindex++] = zynq_type1_pkt(reg, TYPE_OPCODE_READ, 1);
+	buf[cmdindex++] = NOOP_WORD_MASK;
+	buf[cmdindex++] = NOOP_WORD_MASK;
+
+	ret = zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status,
+				     status & STATUS_PCFG_INIT_MASK,
+				     INIT_POLL_DELAY,
+				     INIT_POLL_TIMEOUT);
+	if (ret) {
+		dev_err(&mgr->dev, "Timeout waiting for PCFG_INIT\n");
+		goto out;
+	}
+
+	/* Write to PCAP */
+	intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
+	zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
+
+	zynq_fpga_dma_xfer(priv, dma_addr + src_dmaoffset, cmdindex,
+			   DMA_INVALID_ADDRESS, 0);
+	ret = zynq_fpga_wait_fordone(priv);
+	if (ret) {
+		dev_err(&mgr->dev, "SRCDMA: Timeout waiting for D_P_DONE\n");
+		goto out;
+	}
+	zynq_fpga_set_irq(priv, intr_status);
+
+	/* Read from PACP */
+	zynq_fpga_dma_xfer(priv, DMA_INVALID_ADDRESS, 0, dma_addr, 1);
+	ret = zynq_fpga_wait_fordone(priv);
+	if (ret) {
+		dev_err(&mgr->dev, "DSTDMA: Timeout waiting for D_P_DONE\n");
+		goto out;
+	}
+
+	/* Write to PCAP */
+	cmdindex = 2;
+	buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET,
+					 TYPE_OPCODE_WRITE, 1);
+	buf[cmdindex++] = DESYNC_WORD_MASK;
+	buf[cmdindex++] = NOOP_WORD_MASK;
+	buf[cmdindex++] = NOOP_WORD_MASK;
+	zynq_fpga_dma_xfer(priv, dma_addr + src_dmaoffset, cmdindex,
+			   DMA_INVALID_ADDRESS, 0);
+	ret = zynq_fpga_wait_fordone(priv);
+	if (ret)
+		dev_err(&mgr->dev, "SRCDMA1: Timeout waiting for D_P_DONE\n");
+out:
+	return ret;
+}
+
+static int zynq_fpga_read_cfg_reg(struct seq_file *s, void *data)
+{
+	struct fpga_manager *mgr = (struct fpga_manager *)s->private;
+	struct zynq_fpga_priv *priv = mgr->priv;
+	struct zynq_configreg *p = cfgreg;
+	dma_addr_t dma_addr;
+	unsigned int *buf;
+	int ret = 0;
+
+	if (!mutex_trylock(&priv->ref_mutex))
+		return -EBUSY;
+
+	if (mgr->state != FPGA_MGR_STATE_OPERATING) {
+		ret = -EPERM;
+		goto err_unlock;
+	}
+
+	ret = clk_enable(priv->clk);
+	if (ret)
+		goto err_unlock;
+
+	buf = dma_zalloc_coherent(mgr->dev.parent, READ_DMA_SIZE,
+				 &dma_addr, GFP_KERNEL);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto disable_clk;
+	}
+
+	seq_puts(s, "Zynq FPGA Configuration register contents are\n");
+
+	while (p->reg) {
+		ret = zynq_fpga_getconfigreg(mgr, p->offset, dma_addr, buf);
+		if (ret)
+			goto free_dmabuf;
+		seq_printf(s, "%s --> \t %x \t\r\n", p->reg, buf[0]);
+		p++;
+	}
+
+free_dmabuf:
+	dma_free_coherent(mgr->dev.parent, READ_DMA_SIZE, buf,
+			  dma_addr);
+disable_clk:
+	clk_disable(priv->clk);
+err_unlock:
+	mutex_unlock(&priv->ref_mutex);
+	return ret;
+}
+
+static int zynq_fpga_read_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, zynq_fpga_read_cfg_reg, inode->i_private);
+}
+
+static const struct file_operations zynq_fpga_ops_cfg_reg = {
+	.owner = THIS_MODULE,
+	.open = zynq_fpga_read_open,
+	.read = seq_read,
+};
+#endif
+
 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,
+	.read = zynq_fpga_ops_read_image,
 };
 
 static int zynq_fpga_probe(struct platform_device *pdev)
@@ -621,6 +1028,26 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 		return err;
 	}
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+	struct dentry *d;
+	struct fpga_manager *mgr;
+
+	mgr = platform_get_drvdata(pdev);
+	mutex_init(&priv->ref_mutex);
+
+	d = debugfs_create_dir(pdev->dev.kobj.name, mgr->dir);
+	if (!d)
+		return err;
+
+	priv->dir = d;
+	d = debugfs_create_file("cfg_reg", 0644, priv->dir, mgr,
+				&zynq_fpga_ops_cfg_reg);
+	if (!d) {
+		debugfs_remove_recursive(mgr->dir);
+		return err;
+	}
+#endif
+
 	return 0;
 }
 
@@ -632,6 +1059,9 @@ static int zynq_fpga_remove(struct platform_device *pdev)
 	mgr = platform_get_drvdata(pdev);
 	priv = mgr->priv;
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+	debugfs_remove_recursive(priv->dir);
+#endif
 	fpga_mgr_unregister(&pdev->dev);
 
 	clk_unprepare(priv->clk);
-- 
2.7.4

Re: [PATCH v4 2/2] fpga: zynq-fpga: Add support for readback of FPGA configuration data and registers

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3929 bytes --]

Hi Appana,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on sof-driver-fuweitax/master]
[cannot apply to v4.18-rc6 next-20180727]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Appana-Durga-Kedareswara-rao/fpga-fpga-mgr-Add-readback-support/20180728-034920
base:   https://github.com/fuweitax/linux master
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   drivers/fpga/zynq-fpga.c: In function 'zynq_fpga_probe':
>> drivers/fpga/zynq-fpga.c:1032:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
     struct dentry *d;
     ^~~~~~

vim +1032 drivers/fpga/zynq-fpga.c

   963	
   964	static int zynq_fpga_probe(struct platform_device *pdev)
   965	{
   966		struct device *dev = &pdev->dev;
   967		struct zynq_fpga_priv *priv;
   968		struct resource *res;
   969		int err;
   970	
   971		priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
   972		if (!priv)
   973			return -ENOMEM;
   974		spin_lock_init(&priv->dma_lock);
   975	
   976		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   977		priv->io_base = devm_ioremap_resource(dev, res);
   978		if (IS_ERR(priv->io_base))
   979			return PTR_ERR(priv->io_base);
   980	
   981		priv->slcr = syscon_regmap_lookup_by_phandle(dev->of_node,
   982			"syscon");
   983		if (IS_ERR(priv->slcr)) {
   984			dev_err(dev, "unable to get zynq-slcr regmap\n");
   985			return PTR_ERR(priv->slcr);
   986		}
   987	
   988		init_completion(&priv->dma_done);
   989	
   990		priv->irq = platform_get_irq(pdev, 0);
   991		if (priv->irq < 0) {
   992			dev_err(dev, "No IRQ available\n");
   993			return priv->irq;
   994		}
   995	
   996		priv->clk = devm_clk_get(dev, "ref_clk");
   997		if (IS_ERR(priv->clk)) {
   998			dev_err(dev, "input clock not found\n");
   999			return PTR_ERR(priv->clk);
  1000		}
  1001	
  1002		err = clk_prepare_enable(priv->clk);
  1003		if (err) {
  1004			dev_err(dev, "unable to enable clock\n");
  1005			return err;
  1006		}
  1007	
  1008		/* unlock the device */
  1009		zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
  1010	
  1011		zynq_fpga_set_irq(priv, 0);
  1012		zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
  1013		err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0, dev_name(dev),
  1014				       priv);
  1015		if (err) {
  1016			dev_err(dev, "unable to request IRQ\n");
  1017			clk_disable_unprepare(priv->clk);
  1018			return err;
  1019		}
  1020	
  1021		clk_disable(priv->clk);
  1022	
  1023		err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
  1024					&zynq_fpga_ops, priv);
  1025		if (err) {
  1026			dev_err(dev, "unable to register FPGA manager\n");
  1027			clk_unprepare(priv->clk);
  1028			return err;
  1029		}
  1030	
  1031	#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> 1032		struct dentry *d;
  1033		struct fpga_manager *mgr;
  1034	
  1035		mgr = platform_get_drvdata(pdev);
  1036		mutex_init(&priv->ref_mutex);
  1037	
  1038		d = debugfs_create_dir(pdev->dev.kobj.name, mgr->dir);
  1039		if (!d)
  1040			return err;
  1041	
  1042		priv->dir = d;
  1043		d = debugfs_create_file("cfg_reg", 0644, priv->dir, mgr,
  1044					&zynq_fpga_ops_cfg_reg);
  1045		if (!d) {
  1046			debugfs_remove_recursive(mgr->dir);
  1047			return err;
  1048		}
  1049	#endif
  1050	
  1051		return 0;
  1052	}
  1053	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52967 bytes --]

Re: [PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Alan Tull]

On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao
<appana.durga.rao@xilinx.com> wrote:

Hi Appana,

There should be some documentation for the debugfs added under
Documentation/driver-api/fpga/

Also there are a lot of #ifdefs that were added due to the
CONFIG_FPGA_MGR_DEBUG_FS.  This has caused a kernel robot complaint.
The way to fix that is to have a separate c file for the debugfs
implementation.  I see a lot of other kernel drivers have done it this
way.  I have an implementation that I haven't submitted yet, it
exposes different functionality (exposing the image firmware file name
and writing to the image file).  It's on the downstream
github.com/altera-opensource repo [1].  I'll clean this up and submit
it to the mailing list, it may take a minute for me to get to that.
Once that's done, your added functionality can be a patch on top of
that.

Alan

[1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-4.17/drivers/fpga/fpga-mgr-debugfs.c
https://github.com/altera-opensource/linux-socfpga/blob/socfpga-4.17/drivers/fpga/fpga-mgr-debugfs.h


> Inorder to debug issues with fpga's users would
> like to read the fpga configuration information.
> This patch adds readback support for fpga configuration data
> in the framework through debugfs interface.
>
> Usage:
>         cat /sys/kernel/debug/fpga/fpga0/image
>
> Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> ---
> Changes for v4:
> --> None.
> Changes for v3:
> --> None.
> Changes for v2:
> --> Fixed debug attribute path and name as suggested by Alan
> --> Add config entry for DEBUG as suggested by Alan
> --> Fixed trival coding style issues.
>
>  drivers/fpga/Kconfig          |  7 +++++
>  drivers/fpga/fpga-mgr.c       | 68 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fpga/fpga-mgr.h |  5 ++++
>  3 files changed, 80 insertions(+)
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 53d3f55..838ad4e 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -11,6 +11,13 @@ menuconfig FPGA
>
>  if FPGA
>
> +config FPGA_MGR_DEBUG_FS
> +       tristate "FPGA Debug fs"
> +       select DEBUG_FS
> +       help
> +         FPGA manager debug provides support for reading fpga configuration
> +         information.
> +
>  config FPGA_MGR_SOCFPGA
>         tristate "Altera SOCFPGA FPGA Manager"
>         depends on ARCH_SOCFPGA || COMPILE_TEST
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 9939d2c..4bea860 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_put);
>
> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> +#include <linux/debugfs.h>
> +
> +static int fpga_mgr_read(struct seq_file *s, void *data)
> +{
> +       struct fpga_manager *mgr = (struct fpga_manager *)s->private;
> +       int ret = 0;
> +
> +       if (!mgr->mops->read)
> +               return -ENOENT;
> +
> +       if (!mutex_trylock(&mgr->ref_mutex))
> +               return -EBUSY;
> +
> +       if (mgr->state != FPGA_MGR_STATE_OPERATING) {
> +               ret = -EPERM;
> +               goto err_unlock;
> +       }
> +
> +       /* Read the FPGA configuration data from the fabric */
> +       ret = mgr->mops->read(mgr, s);
> +       if (ret)
> +               dev_err(&mgr->dev, "Error while reading configuration data from FPGA\n");
> +
> +err_unlock:
> +       mutex_unlock(&mgr->ref_mutex);
> +
> +       return ret;
> +}
> +
> +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
> +{
> +       return single_open(file, fpga_mgr_read, inode->i_private);
> +}
> +
> +static const struct file_operations fpga_mgr_ops_image = {
> +       .owner = THIS_MODULE,
> +       .open = fpga_mgr_read_open,
> +       .read = seq_read,
> +};
> +#endif
> +
>  /**
>   * fpga_mgr_lock - Lock FPGA manager for exclusive use
>   * @mgr:       fpga manager
> @@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const char *name,
>         if (ret)
>                 goto error_device;
>
> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> +       struct dentry *d, *parent;
> +
> +       mgr->dir = debugfs_create_dir("fpga", NULL);
> +       if (!mgr->dir)
> +               goto error_device;
> +
> +       parent = mgr->dir;
> +       d = debugfs_create_dir(mgr->dev.kobj.name, parent);
> +       if (!d) {
> +               debugfs_remove_recursive(parent);
> +               goto error_device;
> +       }
> +
> +       parent = d;
> +       d = debugfs_create_file("image", 0644, parent, mgr,
> +                               &fpga_mgr_ops_image);
> +       if (!d) {
> +               debugfs_remove_recursive(mgr->dir);
> +               goto error_device;
> +       }
> +#endif
> +
>         dev_info(&mgr->dev, "%s registered\n", mgr->name);
>
>         return 0;
> @@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
>
>         dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>
> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> +       debugfs_remove_recursive(mgr->dir);
> +#endif
>         /*
>          * If the low level driver provides a method for putting fpga into
>          * a desired state upon unregister, do it.
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 3c6de23..e9e17a9 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -114,6 +114,7 @@ struct fpga_image_info {
>   * @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
> + * @read: optional: read FPGA configuration information
>   * @fpga_remove: optional: Set FPGA into a specific state during driver remove
>   * @groups: optional attribute groups.
>   *
> @@ -131,6 +132,7 @@ struct fpga_manager_ops {
>         int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
>         int (*write_complete)(struct fpga_manager *mgr,
>                               struct fpga_image_info *info);
> +       int (*read)(struct fpga_manager *mgr, struct seq_file *s);
>         void (*fpga_remove)(struct fpga_manager *mgr);
>         const struct attribute_group **groups;
>  };
> @@ -151,6 +153,9 @@ struct fpga_manager {
>         enum fpga_mgr_states state;
>         const struct fpga_manager_ops *mops;
>         void *priv;
> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> +       struct dentry *dir;
> +#endif
>  };
>
>  #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara Rao]

Hi Alan,

	Thanks for the review... 

<Snip>
> On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao
> <appana.durga.rao@xilinx.com> wrote:
> 
> Hi Appana,
> 
> There should be some documentation for the debugfs added under
> Documentation/driver-api/fpga/
> 
> Also there are a lot of #ifdefs that were added due to the
> CONFIG_FPGA_MGR_DEBUG_FS.  This has caused a kernel robot complaint.
> The way to fix that is to have a separate c file for the debugfs
> implementation.  I see a lot of other kernel drivers have done it this way.  I
> have an implementation that I haven't submitted yet, it exposes different
> functionality (exposing the image firmware file name and writing to the
> image file).  It's on the downstream github.com/altera-opensource repo [1].
> I'll clean this up and submit it to the mailing list, it may take a minute for me
> to get to that.
> Once that's done, your added functionality can be a patch on top of that.

Sounds good...
Let me know if you need any help on testing this will do.

Regards,
Kedar.
> 
> Alan
> 
> [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.c
> https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.h
> 
> 
> > Inorder to debug issues with fpga's users would like to read the fpga
> > configuration information.
> > This patch adds readback support for fpga configuration data in the
> > framework through debugfs interface.
> >
> > Usage:
> >         cat /sys/kernel/debug/fpga/fpga0/image
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > <appana.durga.rao@xilinx.com>
> > ---
> > Changes for v4:
> > --> None.
> > Changes for v3:
> > --> None.
> > Changes for v2:
> > --> Fixed debug attribute path and name as suggested by Alan Add
> > --> config entry for DEBUG as suggested by Alan Fixed trival coding
> > --> style issues.
> >
> >  drivers/fpga/Kconfig          |  7 +++++
> >  drivers/fpga/fpga-mgr.c       | 68
> +++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/fpga/fpga-mgr.h |  5 ++++
> >  3 files changed, 80 insertions(+)
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > 53d3f55..838ad4e 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -11,6 +11,13 @@ menuconfig FPGA
> >
> >  if FPGA
> >
> > +config FPGA_MGR_DEBUG_FS
> > +       tristate "FPGA Debug fs"
> > +       select DEBUG_FS
> > +       help
> > +         FPGA manager debug provides support for reading fpga
> configuration
> > +         information.
> > +
> >  config FPGA_MGR_SOCFPGA
> >         tristate "Altera SOCFPGA FPGA Manager"
> >         depends on ARCH_SOCFPGA || COMPILE_TEST diff --git
> > a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> > 9939d2c..4bea860 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)  }
> > EXPORT_SYMBOL_GPL(fpga_mgr_put);
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +#include <linux/debugfs.h>
> > +
> > +static int fpga_mgr_read(struct seq_file *s, void *data) {
> > +       struct fpga_manager *mgr = (struct fpga_manager *)s->private;
> > +       int ret = 0;
> > +
> > +       if (!mgr->mops->read)
> > +               return -ENOENT;
> > +
> > +       if (!mutex_trylock(&mgr->ref_mutex))
> > +               return -EBUSY;
> > +
> > +       if (mgr->state != FPGA_MGR_STATE_OPERATING) {
> > +               ret = -EPERM;
> > +               goto err_unlock;
> > +       }
> > +
> > +       /* Read the FPGA configuration data from the fabric */
> > +       ret = mgr->mops->read(mgr, s);
> > +       if (ret)
> > +               dev_err(&mgr->dev, "Error while reading configuration
> > + data from FPGA\n");
> > +
> > +err_unlock:
> > +       mutex_unlock(&mgr->ref_mutex);
> > +
> > +       return ret;
> > +}
> > +
> > +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
> > +{
> > +       return single_open(file, fpga_mgr_read, inode->i_private); }
> > +
> > +static const struct file_operations fpga_mgr_ops_image = {
> > +       .owner = THIS_MODULE,
> > +       .open = fpga_mgr_read_open,
> > +       .read = seq_read,
> > +};
> > +#endif
> > +
> >  /**
> >   * fpga_mgr_lock - Lock FPGA manager for exclusive use
> >   * @mgr:       fpga manager
> > @@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const
> char *name,
> >         if (ret)
> >                 goto error_device;
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +       struct dentry *d, *parent;
> > +
> > +       mgr->dir = debugfs_create_dir("fpga", NULL);
> > +       if (!mgr->dir)
> > +               goto error_device;
> > +
> > +       parent = mgr->dir;
> > +       d = debugfs_create_dir(mgr->dev.kobj.name, parent);
> > +       if (!d) {
> > +               debugfs_remove_recursive(parent);
> > +               goto error_device;
> > +       }
> > +
> > +       parent = d;
> > +       d = debugfs_create_file("image", 0644, parent, mgr,
> > +                               &fpga_mgr_ops_image);
> > +       if (!d) {
> > +               debugfs_remove_recursive(mgr->dir);
> > +               goto error_device;
> > +       }
> > +#endif
> > +
> >         dev_info(&mgr->dev, "%s registered\n", mgr->name);
> >
> >         return 0;
> > @@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
> >
> >         dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +       debugfs_remove_recursive(mgr->dir);
> > +#endif
> >         /*
> >          * If the low level driver provides a method for putting fpga into
> >          * a desired state upon unregister, do it.
> > diff --git a/include/linux/fpga/fpga-mgr.h
> > b/include/linux/fpga/fpga-mgr.h index 3c6de23..e9e17a9 100644
> > --- a/include/linux/fpga/fpga-mgr.h
> > +++ b/include/linux/fpga/fpga-mgr.h
> > @@ -114,6 +114,7 @@ struct fpga_image_info {
> >   * @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
> > + * @read: optional: read FPGA configuration information
> >   * @fpga_remove: optional: Set FPGA into a specific state during driver
> remove
> >   * @groups: optional attribute groups.
> >   *
> > @@ -131,6 +132,7 @@ struct fpga_manager_ops {
> >         int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
> >         int (*write_complete)(struct fpga_manager *mgr,
> >                               struct fpga_image_info *info);
> > +       int (*read)(struct fpga_manager *mgr, struct seq_file *s);
> >         void (*fpga_remove)(struct fpga_manager *mgr);
> >         const struct attribute_group **groups;  }; @@ -151,6 +153,9 @@
> > struct fpga_manager {
> >         enum fpga_mgr_states state;
> >         const struct fpga_manager_ops *mops;
> >         void *priv;
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +       struct dentry *dir;
> > +#endif
> >  };
> >
> >  #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fpga"
> > in the body of a message to majordomo@vger.kernel.org More
> majordomo
> > info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara Rao]

Hi Alan,

Did you get a chance to send your framework changes to upstream?
@Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series??
Please let me know your thoughts on this. 

Regards,
Kedar.
> On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao
> <appana.durga.rao@xilinx.com> wrote:
> 
> Hi Appana,
> 
> There should be some documentation for the debugfs added under
> Documentation/driver-api/fpga/
> 
> Also there are a lot of #ifdefs that were added due to the
> CONFIG_FPGA_MGR_DEBUG_FS.  This has caused a kernel robot complaint.
> The way to fix that is to have a separate c file for the debugfs implementation.
> I see a lot of other kernel drivers have done it this way.  I have an
> implementation that I haven't submitted yet, it exposes different functionality
> (exposing the image firmware file name and writing to the image file).  It's on
> the downstream github.com/altera-opensource repo [1].  I'll clean this up and
> submit it to the mailing list, it may take a minute for me to get to that.
> Once that's done, your added functionality can be a patch on top of that.
> 
> Alan
> 
> [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.c
> https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.h
> 
> 
> > Inorder to debug issues with fpga's users would like to read the fpga
> > configuration information.
> > This patch adds readback support for fpga configuration data in the
> > framework through debugfs interface.
> >
> > Usage:
> >         cat /sys/kernel/debug/fpga/fpga0/image
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > <appana.durga.rao@xilinx.com>
> > ---
> > Changes for v4:
> > --> None.
> > Changes for v3:
> > --> None.
> > Changes for v2:
> > --> Fixed debug attribute path and name as suggested by Alan Add
> > --> config entry for DEBUG as suggested by Alan Fixed trival coding
> > --> style issues.
> >
> >  drivers/fpga/Kconfig          |  7 +++++
> >  drivers/fpga/fpga-mgr.c       | 68
> +++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/fpga/fpga-mgr.h |  5 ++++
> >  3 files changed, 80 insertions(+)
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > 53d3f55..838ad4e 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -11,6 +11,13 @@ menuconfig FPGA
> >
> >  if FPGA
> >
> > +config FPGA_MGR_DEBUG_FS
> > +       tristate "FPGA Debug fs"
> > +       select DEBUG_FS
> > +       help
> > +         FPGA manager debug provides support for reading fpga configuration
> > +         information.
> > +
> >  config FPGA_MGR_SOCFPGA
> >         tristate "Altera SOCFPGA FPGA Manager"
> >         depends on ARCH_SOCFPGA || COMPILE_TEST diff --git
> > a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> > 9939d2c..4bea860 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)  }
> > EXPORT_SYMBOL_GPL(fpga_mgr_put);
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +#include <linux/debugfs.h>
> > +
> > +static int fpga_mgr_read(struct seq_file *s, void *data) {
> > +       struct fpga_manager *mgr = (struct fpga_manager *)s->private;
> > +       int ret = 0;
> > +
> > +       if (!mgr->mops->read)
> > +               return -ENOENT;
> > +
> > +       if (!mutex_trylock(&mgr->ref_mutex))
> > +               return -EBUSY;
> > +
> > +       if (mgr->state != FPGA_MGR_STATE_OPERATING) {
> > +               ret = -EPERM;
> > +               goto err_unlock;
> > +       }
> > +
> > +       /* Read the FPGA configuration data from the fabric */
> > +       ret = mgr->mops->read(mgr, s);
> > +       if (ret)
> > +               dev_err(&mgr->dev, "Error while reading configuration
> > + data from FPGA\n");
> > +
> > +err_unlock:
> > +       mutex_unlock(&mgr->ref_mutex);
> > +
> > +       return ret;
> > +}
> > +
> > +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
> > +{
> > +       return single_open(file, fpga_mgr_read, inode->i_private); }
> > +
> > +static const struct file_operations fpga_mgr_ops_image = {
> > +       .owner = THIS_MODULE,
> > +       .open = fpga_mgr_read_open,
> > +       .read = seq_read,
> > +};
> > +#endif
> > +
> >  /**
> >   * fpga_mgr_lock - Lock FPGA manager for exclusive use
> >   * @mgr:       fpga manager
> > @@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const
> char *name,
> >         if (ret)
> >                 goto error_device;
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +       struct dentry *d, *parent;
> > +
> > +       mgr->dir = debugfs_create_dir("fpga", NULL);
> > +       if (!mgr->dir)
> > +               goto error_device;
> > +
> > +       parent = mgr->dir;
> > +       d = debugfs_create_dir(mgr->dev.kobj.name, parent);
> > +       if (!d) {
> > +               debugfs_remove_recursive(parent);
> > +               goto error_device;
> > +       }
> > +
> > +       parent = d;
> > +       d = debugfs_create_file("image", 0644, parent, mgr,
> > +                               &fpga_mgr_ops_image);
> > +       if (!d) {
> > +               debugfs_remove_recursive(mgr->dir);
> > +               goto error_device;
> > +       }
> > +#endif
> > +
> >         dev_info(&mgr->dev, "%s registered\n", mgr->name);
> >
> >         return 0;
> > @@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
> >
> >         dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +       debugfs_remove_recursive(mgr->dir);
> > +#endif
> >         /*
> >          * If the low level driver provides a method for putting fpga into
> >          * a desired state upon unregister, do it.
> > diff --git a/include/linux/fpga/fpga-mgr.h
> > b/include/linux/fpga/fpga-mgr.h index 3c6de23..e9e17a9 100644
> > --- a/include/linux/fpga/fpga-mgr.h
> > +++ b/include/linux/fpga/fpga-mgr.h
> > @@ -114,6 +114,7 @@ struct fpga_image_info {
> >   * @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
> > + * @read: optional: read FPGA configuration information
> >   * @fpga_remove: optional: Set FPGA into a specific state during driver
> remove
> >   * @groups: optional attribute groups.
> >   *
> > @@ -131,6 +132,7 @@ struct fpga_manager_ops {
> >         int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
> >         int (*write_complete)(struct fpga_manager *mgr,
> >                               struct fpga_image_info *info);
> > +       int (*read)(struct fpga_manager *mgr, struct seq_file *s);
> >         void (*fpga_remove)(struct fpga_manager *mgr);
> >         const struct attribute_group **groups;  }; @@ -151,6 +153,9 @@
> > struct fpga_manager {
> >         enum fpga_mgr_states state;
> >         const struct fpga_manager_ops *mops;
> >         void *priv;
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +       struct dentry *dir;
> > +#endif
> >  };
> >
> >  #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fpga"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Thor Thayer]

Hi Kedar & Moritz,

On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote:
> Hi Alan,
> 
> Did you get a chance to send your framework changes to upstream?
> @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series??
> Please let me know your thoughts on this.
> 
> Regards,
> Kedar.


I'd like to see some mechanism added as well. Our CvP driver needs a way 
to load images to the FPGA over the PCIe bus.

It wasn't clear to me from the discussion on Alan's patchset[1] that the 
debugfs was acceptable to the mainline. I'd be happy to resurrect that 
patchset with the suggested changes.

Since debugfs isn't enabled for production, are there any alternatives?

Alan sent a RFC [2] for loading FIT images through the sysfs.

The RFC described a FIT image that included FPGA image specific 
information to be included with the image (for systems running without 
device tree like our PCIe bus FPGA CvP).

Unfortunately, I believe this has the same uphill battle that the Device 
Tree Overlay patches[3] have to getting accepted.

Thor

[1] https://patchwork.kernel.org/patch/10566865/

[2] https://patchwork.kernel.org/patch/9860183/
     https://patchwork.kernel.org/patch/9860193/
     https://patchwork.kernel.org/patch/9860191/
     https://patchwork.kernel.org/patch/9860189/
     https://patchwork.kernel.org/patch/9860187/

[3] https://lore.kernel.org/patchwork/cover/511851/

>> On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao
>> <appana.durga.rao@xilinx.com> wrote:
>>
>> Hi Appana,
>>
>> There should be some documentation for the debugfs added under
>> Documentation/driver-api/fpga/
>>
>> Also there are a lot of #ifdefs that were added due to the
>> CONFIG_FPGA_MGR_DEBUG_FS.  This has caused a kernel robot complaint.
>> The way to fix that is to have a separate c file for the debugfs implementation.
>> I see a lot of other kernel drivers have done it this way.  I have an
>> implementation that I haven't submitted yet, it exposes different functionality
>> (exposing the image firmware file name and writing to the image file).  It's on
>> the downstream github.com/altera-opensource repo [1].  I'll clean this up and
>> submit it to the mailing list, it may take a minute for me to get to that.
>> Once that's done, your added functionality can be a patch on top of that.
>>
>> Alan
>>
>> [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
>> 4.17/drivers/fpga/fpga-mgr-debugfs.c
>> https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
>> 4.17/drivers/fpga/fpga-mgr-debugfs.h
>>
>>
>>> Inorder to debug issues with fpga's users would like to read the fpga
>>> configuration information.
>>> This patch adds readback support for fpga configuration data in the
>>> framework through debugfs interface.
>>>
>>> Usage:
>>>          cat /sys/kernel/debug/fpga/fpga0/image
>>>
>>> Signed-off-by: Appana Durga Kedareswara rao
>>> <appana.durga.rao@xilinx.com>
>>> ---
>>> Changes for v4:
>>> --> None.
>>> Changes for v3:
>>> --> None.
>>> Changes for v2:
>>> --> Fixed debug attribute path and name as suggested by Alan Add
>>> --> config entry for DEBUG as suggested by Alan Fixed trival coding
>>> --> style issues.
>>>
>>>   drivers/fpga/Kconfig          |  7 +++++
>>>   drivers/fpga/fpga-mgr.c       | 68
>> +++++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/fpga/fpga-mgr.h |  5 ++++
>>>   3 files changed, 80 insertions(+)
>>>
>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
>>> 53d3f55..838ad4e 100644
>>> --- a/drivers/fpga/Kconfig
>>> +++ b/drivers/fpga/Kconfig
>>> @@ -11,6 +11,13 @@ menuconfig FPGA
>>>
>>>   if FPGA
>>>
>>> +config FPGA_MGR_DEBUG_FS
>>> +       tristate "FPGA Debug fs"
>>> +       select DEBUG_FS
>>> +       help
>>> +         FPGA manager debug provides support for reading fpga configuration
>>> +         information.
>>> +
>>>   config FPGA_MGR_SOCFPGA
>>>          tristate "Altera SOCFPGA FPGA Manager"
>>>          depends on ARCH_SOCFPGA || COMPILE_TEST diff --git
>>> a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
>>> 9939d2c..4bea860 100644
>>> --- a/drivers/fpga/fpga-mgr.c
>>> +++ b/drivers/fpga/fpga-mgr.c
>>> @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)  }
>>> EXPORT_SYMBOL_GPL(fpga_mgr_put);
>>>
>>> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
>>> +#include <linux/debugfs.h>
>>> +
>>> +static int fpga_mgr_read(struct seq_file *s, void *data) {
>>> +       struct fpga_manager *mgr = (struct fpga_manager *)s->private;
>>> +       int ret = 0;
>>> +
>>> +       if (!mgr->mops->read)
>>> +               return -ENOENT;
>>> +
>>> +       if (!mutex_trylock(&mgr->ref_mutex))
>>> +               return -EBUSY;
>>> +
>>> +       if (mgr->state != FPGA_MGR_STATE_OPERATING) {
>>> +               ret = -EPERM;
>>> +               goto err_unlock;
>>> +       }
>>> +
>>> +       /* Read the FPGA configuration data from the fabric */
>>> +       ret = mgr->mops->read(mgr, s);
>>> +       if (ret)
>>> +               dev_err(&mgr->dev, "Error while reading configuration
>>> + data from FPGA\n");
>>> +
>>> +err_unlock:
>>> +       mutex_unlock(&mgr->ref_mutex);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
>>> +{
>>> +       return single_open(file, fpga_mgr_read, inode->i_private); }
>>> +
>>> +static const struct file_operations fpga_mgr_ops_image = {
>>> +       .owner = THIS_MODULE,
>>> +       .open = fpga_mgr_read_open,
>>> +       .read = seq_read,
>>> +};
>>> +#endif
>>> +
>>>   /**
>>>    * fpga_mgr_lock - Lock FPGA manager for exclusive use
>>>    * @mgr:       fpga manager
>>> @@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const
>> char *name,
>>>          if (ret)
>>>                  goto error_device;
>>>
>>> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
>>> +       struct dentry *d, *parent;
>>> +
>>> +       mgr->dir = debugfs_create_dir("fpga", NULL);
>>> +       if (!mgr->dir)
>>> +               goto error_device;
>>> +
>>> +       parent = mgr->dir;
>>> +       d = debugfs_create_dir(mgr->dev.kobj.name, parent);
>>> +       if (!d) {
>>> +               debugfs_remove_recursive(parent);
>>> +               goto error_device;
>>> +       }
>>> +
>>> +       parent = d;
>>> +       d = debugfs_create_file("image", 0644, parent, mgr,
>>> +                               &fpga_mgr_ops_image);
>>> +       if (!d) {
>>> +               debugfs_remove_recursive(mgr->dir);
>>> +               goto error_device;
>>> +       }
>>> +#endif
>>> +
>>>          dev_info(&mgr->dev, "%s registered\n", mgr->name);
>>>
>>>          return 0;
>>> @@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
>>>
>>>          dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>>>
>>> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
>>> +       debugfs_remove_recursive(mgr->dir);
>>> +#endif
>>>          /*
>>>           * If the low level driver provides a method for putting fpga into
>>>           * a desired state upon unregister, do it.
>>> diff --git a/include/linux/fpga/fpga-mgr.h
>>> b/include/linux/fpga/fpga-mgr.h index 3c6de23..e9e17a9 100644
>>> --- a/include/linux/fpga/fpga-mgr.h
>>> +++ b/include/linux/fpga/fpga-mgr.h
>>> @@ -114,6 +114,7 @@ struct fpga_image_info {
>>>    * @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
>>> + * @read: optional: read FPGA configuration information
>>>    * @fpga_remove: optional: Set FPGA into a specific state during driver
>> remove
>>>    * @groups: optional attribute groups.
>>>    *
>>> @@ -131,6 +132,7 @@ struct fpga_manager_ops {
>>>          int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
>>>          int (*write_complete)(struct fpga_manager *mgr,
>>>                                struct fpga_image_info *info);
>>> +       int (*read)(struct fpga_manager *mgr, struct seq_file *s);
>>>          void (*fpga_remove)(struct fpga_manager *mgr);
>>>          const struct attribute_group **groups;  }; @@ -151,6 +153,9 @@
>>> struct fpga_manager {
>>>          enum fpga_mgr_states state;
>>>          const struct fpga_manager_ops *mops;
>>>          void *priv;
>>> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
>>> +       struct dentry *dir;
>>> +#endif
>>>   };
>>>
>>>   #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
>>> --
>>> 2.7.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-fpga"
>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>> info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Moritz Fischer]

Thor,

On Fri, Sep 27, 2019 at 09:32:11AM -0500, Thor Thayer wrote:
> Hi Kedar & Moritz,
> 
> On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote:
> > Hi Alan,
> > 
> > Did you get a chance to send your framework changes to upstream?
No they weren't upstreamed.

> > @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series??
> > Please let me know your thoughts on this.

Alan had some comments RE: #defines, I'll have to take another look.
> > 
> > Regards,
> > Kedar.
> 
> 
> I'd like to see some mechanism added as well. Our CvP driver needs a way to
> load images to the FPGA over the PCIe bus.

Can you elaborate a bit on the CvP use-case and how that would work? Who
would use the device how after loading the bitstream?

Generally there are several use cases that I have collected mentally
over the years:

I) DFL use case:
  - Mixed-set of drivers: Kernel and Userspace
  - FPGA logic is discoverable through DFL
  - Userspace application wants to reprogram FPGA

II) DT configfs use case:
  - Mixed-set of drivers: Kernel and Userspace
  - FPGA logic is *not* discoverable (hence DT overlay)
  - Userspace application wants to reprogram FPGA

III) Thomas' case:
  - Kernel only drivers (pcie bridge, pcie drivers, ...)
  - FPGA logic is fully discoverable (i.e. PCIe endpoint
    implemented in FPGA, connected to SoC via PCIe)
  - Userspace application wants to reprogram FPGA

IV) VFIO case:
  - Usually exposes either entire device via vfio-pci or part via
    vfio-mdev
  - Loading (basic) bitstream at boot from flash
  - vfio-mdev case can use FPGA region interface + ioctl
  - Full VFIO case is similar to III)

How does your CvP use case fit in? Collecting all the use-cases would
help with moving forward on coming up with an API :)

> 
> It wasn't clear to me from the discussion on Alan's patchset[1] that the
> debugfs was acceptable to the mainline. I'd be happy to resurrect that
> patchset with the suggested changes.

Back then we decided to not move forward with the debugfs patchset since
it's essentially cat foo.bin > /dev/xdevcfg / cat bar.rbf > /dev/fpga0
in disguise. Which is why I vetoed it back then.

> Since debugfs isn't enabled for production, are there any alternatives?
> 
> Alan sent a RFC [2] for loading FIT images through the sysfs.
> 
> The RFC described a FIT image that included FPGA image specific information
> to be included with the image (for systems running without device tree like
> our PCIe bus FPGA CvP).

Yeah I had originally suggested that as a mechanim to make FPGA images
discoverable by the kernel. I still think the idea has merit, however it
will run into the same issues that the configfs interface ran into w.r.t
using dt-overlays.

Generally I'd like to see a solution that exposes the *same* interface
to DT and not DT systems to userspace.

Using FIT headers one could go ahead and design something along the
lines of what DFL is doing, except for instead of parsing the DFL in the
logic, one would parse the FIT header to create subdevices.

> Unfortunately, I believe this has the same uphill battle that the Device
> Tree Overlay patches[3] have to getting accepted.

See above. While I'm happy to discuss this more I atm don't have the
bandwidth to push the DT work any further.

Thanks,
Moritz

Re: [PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Thor Thayer]

Hi Moritz,

On 9/27/19 1:23 PM, Moritz Fischer wrote:
> Thor,
> 
> On Fri, Sep 27, 2019 at 09:32:11AM -0500, Thor Thayer wrote:
>> Hi Kedar & Moritz,
>>
>> On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote:
>>> Hi Alan,
>>>
>>> Did you get a chance to send your framework changes to upstream?
> No they weren't upstreamed.
> 
>>> @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series??
>>> Please let me know your thoughts on this.
> 
> Alan had some comments RE: #defines, I'll have to take another look.
>>>
>>> Regards,
>>> Kedar.
>>
>>
>> I'd like to see some mechanism added as well. Our CvP driver needs a way to
>> load images to the FPGA over the PCIe bus.
> 
> Can you elaborate a bit on the CvP use-case and how that would work? Who
> would use the device how after loading the bitstream?
> 
> Generally there are several use cases that I have collected mentally
> over the years:
> 
> I) DFL use case:
>    - Mixed-set of drivers: Kernel and Userspace
>    - FPGA logic is discoverable through DFL
>    - Userspace application wants to reprogram FPGA
> 
> II) DT configfs use case:
>    - Mixed-set of drivers: Kernel and Userspace
>    - FPGA logic is *not* discoverable (hence DT overlay)
>    - Userspace application wants to reprogram FPGA
> 
> III) Thomas' case:
>    - Kernel only drivers (pcie bridge, pcie drivers, ...)
>    - FPGA logic is fully discoverable (i.e. PCIe endpoint
>      implemented in FPGA, connected to SoC via PCIe)
>    - Userspace application wants to reprogram FPGA
> 
> IV) VFIO case:
>    - Usually exposes either entire device via vfio-pci or part via
>      vfio-mdev
>    - Loading (basic) bitstream at boot from flash
>    - vfio-mdev case can use FPGA region interface + ioctl
>    - Full VFIO case is similar to III)
> 
> How does your CvP use case fit in? Collecting all the use-cases would
> help with moving forward on coming up with an API :)
> 
The CvP case is the same as III) Thomas' case. The FPGA configuration 
bitstream is downloaded over the PCIe.

The one difference in my case is that there isn't an SoC. This is a 
Intel host processor connecting to a non-SoC Stratix10/Arria10. The 
non-SoC A10/S10, boots a minimal image (CvP) setting up the peripheral 
pins and enabling the PCIe endpoint for CvP downloads.

The host can then download bitstreams using the FPGA Manager through 
debugFS and when the bitstream finishes downloading and the FPGA enters 
User Mode, the functionality is available for the host to use.

>>
>> It wasn't clear to me from the discussion on Alan's patchset[1] that the
>> debugfs was acceptable to the mainline. I'd be happy to resurrect that
>> patchset with the suggested changes.
> 
> Back then we decided to not move forward with the debugfs patchset since
> it's essentially cat foo.bin > /dev/xdevcfg / cat bar.rbf > /dev/fpga0
> in disguise. Which is why I vetoed it back then.
> 
>> Since debugfs isn't enabled for production, are there any alternatives?
>>
>> Alan sent a RFC [2] for loading FIT images through the sysfs.
>>
>> The RFC described a FIT image that included FPGA image specific information
>> to be included with the image (for systems running without device tree like
>> our PCIe bus FPGA CvP).
> 
> Yeah I had originally suggested that as a mechanim to make FPGA images
> discoverable by the kernel. I still think the idea has merit, however it
> will run into the same issues that the configfs interface ran into w.r.t
> using dt-overlays.
> 
> Generally I'd like to see a solution that exposes the *same* interface
> to DT and not DT systems to userspace.
> 
> Using FIT headers one could go ahead and design something along the
> lines of what DFL is doing, except for instead of parsing the DFL in the
> logic, one would parse the FIT header to create subdevices.
> 
>> Unfortunately, I believe this has the same uphill battle that the Device
>> Tree Overlay patches[3] have to getting accepted.
> 
> See above. While I'm happy to discuss this more I atm don't have the
> bandwidth to push the DT work any further.

Understood. I'm still coming up to speed on the variations of FPGA 
enablement but I'm happy to help where I can.

Thanks.

> 
> Thanks,
> Moritz
> 

Re: [PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Moritz Fischer]

Hi Thor,

On Mon, Oct 07, 2019 at 01:06:51PM -0500, Thor Thayer wrote:
> Hi Moritz,
> 
> On 9/27/19 1:23 PM, Moritz Fischer wrote:
> > Thor,
> > 
> > On Fri, Sep 27, 2019 at 09:32:11AM -0500, Thor Thayer wrote:
> > > Hi Kedar & Moritz,
> > > 
> > > On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote:
> > > > Hi Alan,
> > > > 
> > > > Did you get a chance to send your framework changes to upstream?
> > No they weren't upstreamed.
> > 
> > > > @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series??
> > > > Please let me know your thoughts on this.
> > 
> > Alan had some comments RE: #defines, I'll have to take another look.
> > > > 
> > > > Regards,
> > > > Kedar.
> > > 
> > > 
> > > I'd like to see some mechanism added as well. Our CvP driver needs a way to
> > > load images to the FPGA over the PCIe bus.
> > 
> > Can you elaborate a bit on the CvP use-case and how that would work? Who
> > would use the device how after loading the bitstream?
> > 
> > Generally there are several use cases that I have collected mentally
> > over the years:
> > 
> > I) DFL use case:
> >    - Mixed-set of drivers: Kernel and Userspace
> >    - FPGA logic is discoverable through DFL
> >    - Userspace application wants to reprogram FPGA
> > 
> > II) DT configfs use case:
> >    - Mixed-set of drivers: Kernel and Userspace
> >    - FPGA logic is *not* discoverable (hence DT overlay)
> >    - Userspace application wants to reprogram FPGA
> > 
> > III) Thomas' case:
> >    - Kernel only drivers (pcie bridge, pcie drivers, ...)
> >    - FPGA logic is fully discoverable (i.e. PCIe endpoint
> >      implemented in FPGA, connected to SoC via PCIe)
> >    - Userspace application wants to reprogram FPGA
> > 
> > IV) VFIO case:
> >    - Usually exposes either entire device via vfio-pci or part via
> >      vfio-mdev
> >    - Loading (basic) bitstream at boot from flash
> >    - vfio-mdev case can use FPGA region interface + ioctl
> >    - Full VFIO case is similar to III)
> > 
> > How does your CvP use case fit in? Collecting all the use-cases would
> > help with moving forward on coming up with an API :)
> > 
> The CvP case is the same as III) Thomas' case. The FPGA configuration
> bitstream is downloaded over the PCIe.
> 
> The one difference in my case is that there isn't an SoC. This is a Intel
> host processor connecting to a non-SoC Stratix10/Arria10. The non-SoC
> A10/S10, boots a minimal image (CvP) setting up the peripheral pins and
> enabling the PCIe endpoint for CvP downloads.
> 
> The host can then download bitstreams using the FPGA Manager through debugFS
> and when the bitstream finishes downloading and the FPGA enters User Mode,
> the functionality is available for the host to use.

I am generally confused by this driver. How does it work exactly? What
happens after altera-cvp binds a PCI device?

You can use it to download a bitstream (say we had the debugfs
interface), and then what happens next? How do I use the device? It
already has a PCI driver bound to it at that point?

What happens next?

Please tell me that not the only use-case for this is /dev/mem :)

Thomas' use-case is different in that behind the FPGA device there are
actual other *discoverable* PCI devices that will get enumerated and
bind to separate drivers.

Thanks,
Moritz

PS: I'll be out this week on vacation starting tmr so responses might be delayed

Re: [PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Thor Thayer]

Hi Moritz,

On 10/7/19 4:20 PM, Moritz Fischer wrote:
> Hi Thor,
> 
> On Mon, Oct 07, 2019 at 01:06:51PM -0500, Thor Thayer wrote:
>> Hi Moritz,
>>
>> On 9/27/19 1:23 PM, Moritz Fischer wrote:
>>> Thor,
>>>
>>> On Fri, Sep 27, 2019 at 09:32:11AM -0500, Thor Thayer wrote:
>>>> Hi Kedar & Moritz,
>>>>
>>>> On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote:
>>>>> Hi Alan,
>>>>>
>>>>> Did you get a chance to send your framework changes to upstream?
>>> No they weren't upstreamed.
>>>
>>>>> @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series??
>>>>> Please let me know your thoughts on this.
>>>
>>> Alan had some comments RE: #defines, I'll have to take another look.
>>>>>
>>>>> Regards,
>>>>> Kedar.
>>>>
>>>>
>>>> I'd like to see some mechanism added as well. Our CvP driver needs a way to
>>>> load images to the FPGA over the PCIe bus.
>>>
>>> Can you elaborate a bit on the CvP use-case and how that would work? Who
>>> would use the device how after loading the bitstream?
>>>
>>> Generally there are several use cases that I have collected mentally
>>> over the years:
>>>
>>> I) DFL use case:
>>>     - Mixed-set of drivers: Kernel and Userspace
>>>     - FPGA logic is discoverable through DFL
>>>     - Userspace application wants to reprogram FPGA
>>>
>>> II) DT configfs use case:
>>>     - Mixed-set of drivers: Kernel and Userspace
>>>     - FPGA logic is *not* discoverable (hence DT overlay)
>>>     - Userspace application wants to reprogram FPGA
>>>
>>> III) Thomas' case:
>>>     - Kernel only drivers (pcie bridge, pcie drivers, ...)
>>>     - FPGA logic is fully discoverable (i.e. PCIe endpoint
>>>       implemented in FPGA, connected to SoC via PCIe)
>>>     - Userspace application wants to reprogram FPGA
>>>
>>> IV) VFIO case:
>>>     - Usually exposes either entire device via vfio-pci or part via
>>>       vfio-mdev
>>>     - Loading (basic) bitstream at boot from flash
>>>     - vfio-mdev case can use FPGA region interface + ioctl
>>>     - Full VFIO case is similar to III)
>>>
>>> How does your CvP use case fit in? Collecting all the use-cases would
>>> help with moving forward on coming up with an API :)
>>>
>> The CvP case is the same as III) Thomas' case. The FPGA configuration
>> bitstream is downloaded over the PCIe.
>>
>> The one difference in my case is that there isn't an SoC. This is a Intel
>> host processor connecting to a non-SoC Stratix10/Arria10. The non-SoC
>> A10/S10, boots a minimal image (CvP) setting up the peripheral pins and
>> enabling the PCIe endpoint for CvP downloads.
>>
>> The host can then download bitstreams using the FPGA Manager through debugFS
>> and when the bitstream finishes downloading and the FPGA enters User Mode,
>> the functionality is available for the host to use.
> 
> I am generally confused by this driver. How does it work exactly? What
> happens after altera-cvp binds a PCI device?
> 
> You can use it to download a bitstream (say we had the debugfs
> interface), and then what happens next? How do I use the device? It
> already has a PCI driver bound to it at that point?

Sorry for the delay. In the CvP case, I reboot the host device leaving 
the FPGA board powered so the new PCI interface is enumerated.

There may be a better way. I'll need to research Thomas' use case. There 
may be some good lessons to learn there.

Thanks,

Thor

> 
> What happens next?
> 
> Please tell me that not the only use-case for this is /dev/mem :)
> 
> Thomas' use-case is different in that behind the FPGA device there are
> actual other *discoverable* PCI devices that will get enumerated and
> bind to separate drivers.
> 
> Thanks,
> Moritz
> 
> PS: I'll be out this week on vacation starting tmr so responses might be delayed
>