linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] fpga: fpga-mgr: Add readback support
@ 2018-07-24 14:17 Appana Durga Kedareswara rao
  2018-07-24 14:17 ` [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback Appana Durga Kedareswara rao
  0 siblings, 1 reply; 10+ messages in thread
From: Appana Durga Kedareswara rao @ 2018-07-24 14:17 UTC (permalink / raw)
  To: atull, mdf, navam, michals
  Cc: linux-fpga, linux-arm-kernel, linux-kernel, 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 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback
  2018-07-24 14:17 [PATCH v3 1/2] fpga: fpga-mgr: Add readback support Appana Durga Kedareswara rao
@ 2018-07-24 14:17 ` Appana Durga Kedareswara rao
  2018-07-24 16:32   ` Moritz Fischer
  2018-07-24 18:52   ` Alan Tull
  0 siblings, 2 replies; 10+ messages in thread
From: Appana Durga Kedareswara rao @ 2018-07-24 14:17 UTC (permalink / raw)
  To: atull, mdf, navam, michals
  Cc: linux-fpga, linux-arm-kernel, linux-kernel, Appana Durga Kedareswara rao

This patch does the below
--> Adds support for readback of pl configuration data
--> Adds support for readback of pl configuration registers

Usage:
Readback of PL configuration registers
        cat /sys/kernel/debug/fpga/fpga0/image
Readback of PL configuration data
        echo 1 > /sys/module/zynqmp_fpga/parameters/readback_type
        cat /sys/kernel/debug/fpga/fpga0/image

Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
---
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 | 400 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 400 insertions(+)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 70b15b3..5f1a1aa 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,72 @@
 /* Disable global resets */
 #define FPGA_RST_NONE_MASK		0x0
 
+static bool readback_type;
+module_param(readback_type, bool, 0644);
+MODULE_PARM_DESC(readback_type,
+		"readback_type 0-configuration register read "
+		"1- configuration data read (default: 0)");
+
+/**
+ * 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		0x00000004
+#define START_CMD_MASK		0x00000005
+#define RCRC_CMD_MASK		0x00000007
+#define SHUTDOWN_CMD_MASK	0x0000000B
+#define DESYNC_WORD_MASK	0x0000000D
+#define BUSWIDTH_SYNCWORD_MASK	0x000000BB
+#define NOOP_WORD_MASK		0x20000000
+#define BUSWIDTH_DETECT_MASK	0x11220044
+#define SYNC_WORD_MASK		0xAA995566
+#define DUMMY_WORD_MASK		0xFFFFFFFF
+
+#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_DMA_SIZE		0x200
+#define DUMMY_FRAMES_SIZE	0x28
+#define SLCR_PCAP_FREQ		10000000
+
 struct zynq_fpga_priv {
 	int irq;
 	struct clk *clk;
@@ -140,6 +207,7 @@ struct zynq_fpga_priv {
 	struct scatterlist *cur_sg;
 
 	struct completion dma_done;
+	u32 size;
 };
 
 static inline void zynq_fpga_write(struct zynq_fpga_priv *priv, u32 offset,
@@ -164,6 +232,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 +281,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 +491,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 +637,321 @@ 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 sifting 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.
+	 */
+	return (((1 << TYPE_HDR_SHIFT) |
+		(reg << TYPE_REG_SHIFT) |
+		(opcode << TYPE_OP_SHIFT)) | size);
+
+}
+
+/****************************************************************************/
+/**
+ *
+ * Generates a Type 2 packet header that reads back the requested Configuration
+ * register.
+ *
+ * @param        OpCode is the read/write operation code.
+ * @param        Size is the size of the word to be read.
+ *
+ * @return       Type 2 packet header to read the specified register
+ *
+ * @note         None.
+ *
+ *****************************************************************************/
+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 sifting of Type 2
+	 * Header Mask, OpCode and then performing OR
+	 * operation with the Word Length.
+	 */
+	return (((2 << TYPE_HDR_SHIFT) |
+		(OpCode << TYPE_OP_SHIFT)) | Size);
+}
+
+static int zynq_fpga_read_cfgdata(struct fpga_manager *mgr,
+				  struct seq_file *s)
+{
+	struct zynq_fpga_priv *priv;
+	int ret = 0, i, cmdindex, clk_rate;
+	unsigned int *buf;
+	dma_addr_t dma_addr;
+	u32 intr_status, status;
+	size_t size;
+
+	priv = mgr->priv;
+	size = priv->size + READ_DMA_SIZE + DUMMY_FRAMES_SIZE;
+	buf = dma_zalloc_coherent(mgr->dev.parent, size,
+				 &dma_addr, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	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 < 6; 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 < 32; 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);
+	return ret;
+}
+
+static int zynq_fpga_getconfigreg(struct fpga_manager *mgr, u8 reg,
+				  dma_addr_t dma_addr, int *buf)
+{
+	struct zynq_fpga_priv *priv;
+	int ret = 0, cmdindex, src_dmaoffset;
+	u32 intr_status, status;
+
+	priv = mgr->priv;
+
+	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_cfgreg(struct fpga_manager *mgr,
+				 struct seq_file *s)
+{
+	int ret = 0;
+	unsigned int *buf;
+	dma_addr_t dma_addr;
+	struct zynq_configreg *p = cfgreg;
+
+	buf = dma_zalloc_coherent(mgr->dev.parent, READ_DMA_SIZE,
+				 &dma_addr, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	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);
+	return ret;
+}
+
+static int zynq_fpga_ops_read(struct fpga_manager *mgr, struct seq_file *s)
+{
+	struct zynq_fpga_priv *priv;
+	int ret;
+
+	priv = mgr->priv;
+
+	ret = clk_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	if (readback_type)
+		ret = zynq_fpga_read_cfgdata(mgr, s);
+	else
+		ret = zynq_fpga_read_cfgreg(mgr, s);
+
+	clk_disable(priv->clk);
+
+	return ret;
+}
+
 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,
 };
 
 static int zynq_fpga_probe(struct platform_device *pdev)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback
  2018-07-24 14:17 ` [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback Appana Durga Kedareswara rao
@ 2018-07-24 16:32   ` Moritz Fischer
  2018-07-24 18:31     ` Appana Durga Kedareswara Rao
  2018-07-24 18:52   ` Alan Tull
  1 sibling, 1 reply; 10+ messages in thread
From: Moritz Fischer @ 2018-07-24 16:32 UTC (permalink / raw)
  To: Appana Durga Kedareswara rao
  Cc: Alan Tull, navam, Michal Simek, linux-fpga, linux-arm-kernel,
	Linux Kernel Mailing List

Hi Appana,

On Tue, Jul 24, 2018 at 7:17 AM, Appana Durga Kedareswara rao
<appana.durga.rao@xilinx.com> wrote:
> This patch does the below
> --> Adds support for readback of pl configuration data
> --> Adds support for readback of pl configuration registers

Can you please make the commit message such that you have full sentences?

"Add support for readback of FPGA configuration data and registers" of example.

>
> Usage:
> Readback of PL configuration registers
>         cat /sys/kernel/debug/fpga/fpga0/image
> Readback of PL configuration data
>         echo 1 > /sys/module/zynqmp_fpga/parameters/readback_type

The patch below is for Zynq if I'm not mistaken, not ZynqMP?

>         cat /sys/kernel/debug/fpga/fpga0/image
>
> Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> ---
> 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 | 400 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 400 insertions(+)
>
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index 70b15b3..5f1a1aa 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,72 @@
>  /* Disable global resets */
>  #define FPGA_RST_NONE_MASK             0x0
>
> +static bool readback_type;
> +module_param(readback_type, bool, 0644);
> +MODULE_PARM_DESC(readback_type,
> +               "readback_type 0-configuration register read "
> +               "1- configuration data read (default: 0)");

Not sure a module param is the best solution here.
> +
> +/**
> + * 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          0x00000004
> +#define START_CMD_MASK         0x00000005
> +#define RCRC_CMD_MASK          0x00000007

BIT() and GENMASK() would work here.

> +#define SHUTDOWN_CMD_MASK      0x0000000B
> +#define DESYNC_WORD_MASK       0x0000000D
> +#define BUSWIDTH_SYNCWORD_MASK 0x000000BB
> +#define NOOP_WORD_MASK         0x20000000
> +#define BUSWIDTH_DETECT_MASK   0x11220044
> +#define SYNC_WORD_MASK         0xAA995566
> +#define DUMMY_WORD_MASK                0xFFFFFFFF

GENMASK() would probably work for most of the above ones
> +
> +#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_DMA_SIZE          0x200
> +#define DUMMY_FRAMES_SIZE      0x28
> +#define SLCR_PCAP_FREQ         10000000
> +
>  struct zynq_fpga_priv {
>         int irq;
>         struct clk *clk;
> @@ -140,6 +207,7 @@ struct zynq_fpga_priv {
>         struct scatterlist *cur_sg;
>
>         struct completion dma_done;
> +       u32 size;
>  };
>
>  static inline void zynq_fpga_write(struct zynq_fpga_priv *priv, u32 offset,
> @@ -164,6 +232,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 +281,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 +491,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 +637,321 @@ 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 sifting of Type1
Shifting? Maybe you can just reference the section that documents this in the
TRM?
> +        * 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.
> +        */
> +       return (((1 << TYPE_HDR_SHIFT) |
> +               (reg << TYPE_REG_SHIFT) |
> +               (opcode << TYPE_OP_SHIFT)) | size);
> +
> +}
> +
> +/****************************************************************************/
> +/**
> + *
> + * Generates a Type 2 packet header that reads back the requested Configuration
> + * register.
> + *
> + * @param        OpCode is the read/write operation code.
> + * @param        Size is the size of the word to be read.
> + *
> + * @return       Type 2 packet header to read the specified register
> + *
> + * @note         None.
> + *
> + *****************************************************************************/
> +static int zynq_type2_pkt(u8 OpCode, u32 Size)

No camel case please, can you make Size and OpCode small caps please?
Also please make the two functions consistent.
> +{
> +       /*
> +        * 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 sifting of Type 2

s/sifting/shifting/
> +        * Header Mask, OpCode and then performing OR
> +        * operation with the Word Length.
> +        */
> +       return (((2 << TYPE_HDR_SHIFT) |
> +               (OpCode << TYPE_OP_SHIFT)) | Size);
> +}
> +
> +static int zynq_fpga_read_cfgdata(struct fpga_manager *mgr,
> +                                 struct seq_file *s)
> +{
> +       struct zynq_fpga_priv *priv;
> +       int ret = 0, i, cmdindex, clk_rate;
> +       unsigned int *buf;
> +       dma_addr_t dma_addr;
> +       u32 intr_status, status;
> +       size_t size;

Reverse xmas tree please
> +
> +       priv = mgr->priv;
> +       size = priv->size + READ_DMA_SIZE + DUMMY_FRAMES_SIZE;
> +       buf = dma_zalloc_coherent(mgr->dev.parent, size,
> +                                &dma_addr, GFP_KERNEL);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       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 < 6; i++)
Magic constant?
> +               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 < 32; i++)
Magic constant?
> +               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);
> +       return ret;
> +}
> +
> +static int zynq_fpga_getconfigreg(struct fpga_manager *mgr, u8 reg,
> +                                 dma_addr_t dma_addr, int *buf)
zynq_fpga_get_config_reg maybe?
> +{
> +       struct zynq_fpga_priv *priv;
> +       int ret = 0, cmdindex, src_dmaoffset;
> +       u32 intr_status, status;

Same here.
> +
> +       priv = mgr->priv;
> +
> +       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 */

READ -> Read ?
> +       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_cfgreg(struct fpga_manager *mgr,
> +                                struct seq_file *s)
> +{
> +       int ret = 0;
> +       unsigned int *buf;
> +       dma_addr_t dma_addr;
> +       struct zynq_configreg *p = cfgreg;

And here.
> +
> +       buf = dma_zalloc_coherent(mgr->dev.parent, READ_DMA_SIZE,
> +                                &dma_addr, GFP_KERNEL);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       seq_puts(s, "zynq FPGA Configuration register contents are\n");

Zynq FPGA, maybe caps here...
> +
> +       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);
> +       return ret;
> +}
> +
> +static int zynq_fpga_ops_read(struct fpga_manager *mgr, struct seq_file *s)
> +{
> +       struct zynq_fpga_priv *priv;
> +       int ret;
> +
> +       priv = mgr->priv;
> +
> +       ret = clk_enable(priv->clk);
> +       if (ret)
> +               return ret;
> +
> +       if (readback_type)
> +               ret = zynq_fpga_read_cfgdata(mgr, s);
> +       else
> +               ret = zynq_fpga_read_cfgreg(mgr, s);
> +
> +       clk_disable(priv->clk);
> +
> +       return ret;
> +}
> +
>  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,
>  };
>
>  static int zynq_fpga_probe(struct platform_device *pdev)
> --
> 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

Thanks,

Moritz

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback
  2018-07-24 16:32   ` Moritz Fischer
@ 2018-07-24 18:31     ` Appana Durga Kedareswara Rao
  2018-07-24 18:49       ` Alan Tull
  0 siblings, 1 reply; 10+ messages in thread
From: Appana Durga Kedareswara Rao @ 2018-07-24 18:31 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Alan Tull, Nava kishore Manne, Michal Simek, linux-fpga,
	linux-arm-kernel, Linux Kernel Mailing List

Hi Moritz,

	Thanks for the review...

<Snip> 
> Can you please make the commit message such that you have full sentences?
> 
> "Add support for readback of FPGA configuration data and registers" of 
> example.

Sure will fix in v4.

> 
> >
> > Usage:
> > Readback of PL configuration registers
> >         cat /sys/kernel/debug/fpga/fpga0/image
> > Readback of PL configuration data
> >         echo 1 > /sys/module/zynqmp_fpga/parameters/readback_type
> 
> The patch below is for Zynq if I'm not mistaken, not ZynqMP?

Yes it is for Zynq not ZynqMP by mistake I have kept zynqmp here, thanks for pointing  will fix in v4...

<Snip> 
> > +static bool readback_type;
> > +module_param(readback_type, bool, 0644); 
> > +MODULE_PARM_DESC(readback_type,
> > +               "readback_type 0-configuration register read "
> > +               "1- configuration data read (default: 0)");
> 
> Not sure a module param is the best solution here.

Need to provide flexibility to the user to switch between reading of FPGA registers and configuration data.
One other option is sysfs. Do you want me to implement sysfs path??? 
Any other suggestions??? 

<Snip> 
> > +/* Masks for Configuration registers */
> > +#define FAR_ADDR_MASK          0x00000000
> > +#define RCFG_CMD_MASK          0x00000004
> > +#define START_CMD_MASK         0x00000005
> > +#define RCRC_CMD_MASK          0x00000007
> 
> BIT() and GENMASK() would work here.

Sure will fix in v4... 

> 
> > +#define SHUTDOWN_CMD_MASK      0x0000000B
> > +#define DESYNC_WORD_MASK       0x0000000D
> > +#define BUSWIDTH_SYNCWORD_MASK 0x000000BB
> > +#define NOOP_WORD_MASK         0x20000000
> > +#define BUSWIDTH_DETECT_MASK   0x11220044
> > +#define SYNC_WORD_MASK         0xAA995566
> > +#define DUMMY_WORD_MASK                0xFFFFFFFF
> 
> GENMASK() would probably work for most of the above ones

Sure will use GENMAK wherever applicable will fix in v4.

<Snip> 
> > +        * Generating the Type 1 packet header which involves 
> > +sifting of Type1
> Shifting? Maybe you can just reference the section that documents this 
> in the TRM?

Sure will fix in v4... 

<Snip>
> ******
> > +********/ static int zynq_type2_pkt(u8 OpCode, u32 Size)
> 
> No camel case please, can you make Size and OpCode small caps please?
> Also please make the two functions consistent.

Sure will fix in v4...

> > +{
> > +       /*
> > +        * 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 
> > +sifting of Type 2
> 
> s/sifting/shifting/

Sure will fix in v4...

> > +        * Header Mask, OpCode and then performing OR
> > +        * operation with the Word Length.
> > +        */
> > +       return (((2 << TYPE_HDR_SHIFT) |
> > +               (OpCode << TYPE_OP_SHIFT)) | Size); }
> > +
> > +static int zynq_fpga_read_cfgdata(struct fpga_manager *mgr,
> > +                                 struct seq_file *s) {
> > +       struct zynq_fpga_priv *priv;
> > +       int ret = 0, i, cmdindex, clk_rate;
> > +       unsigned int *buf;
> > +       dma_addr_t dma_addr;
> > +       u32 intr_status, status;
> > +       size_t size;
> 
> Reverse xmas tree please

Ok sure will v4... 

> > +
> > +       priv = mgr->priv;
> > +       size = priv->size + READ_DMA_SIZE + DUMMY_FRAMES_SIZE;
> > +       buf = dma_zalloc_coherent(mgr->dev.parent, size,
> > +                                &dma_addr, GFP_KERNEL);
> > +       if (!buf)
> > +               return -ENOMEM;
> > +
> > +       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 < 6; i++)
> Magic constant?

Sure will fix in v4....

> > +               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 < 32; i++)
> Magic constant?

Sure will fix in v4....

> > +               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);
> > +       return ret;
> > +}
> > +
> > +static int zynq_fpga_getconfigreg(struct fpga_manager *mgr, u8 reg,
> > +                                 dma_addr_t dma_addr, int *buf)
> zynq_fpga_get_config_reg maybe?

Ok will fix in v4...

> > +{
> > +       struct zynq_fpga_priv *priv;
> > +       int ret = 0, cmdindex, src_dmaoffset;
> > +       u32 intr_status, status;
> 
> Same here.

Sure will fix in v4....

> > +
> > +       priv = mgr->priv;
> > +
> > +       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 */
> 
> READ -> Read ?

Ok sure will fix in v4....

> > +       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_cfgreg(struct fpga_manager *mgr,
> > +                                struct seq_file *s) {
> > +       int ret = 0;
> > +       unsigned int *buf;
> > +       dma_addr_t dma_addr;
> > +       struct zynq_configreg *p = cfgreg;
> 
> And here.

Sure will fix in v4....

> > +
> > +       buf = dma_zalloc_coherent(mgr->dev.parent, READ_DMA_SIZE,
> > +                                &dma_addr, GFP_KERNEL);
> > +       if (!buf)
> > +               return -ENOMEM;
> > +
> > +       seq_puts(s, "zynq FPGA Configuration register contents 
> > + are\n");
> 
> Zynq FPGA, maybe caps here...

Sure will fix in v4....

Thanks and Regards,
Kedar.

> > +
> > +       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);
> > +       return ret;
> > +}
> > +
> > +static int zynq_fpga_ops_read(struct fpga_manager *mgr, struct 
> > +seq_file *s) {
> > +       struct zynq_fpga_priv *priv;
> > +       int ret;
> > +
> > +       priv = mgr->priv;
> > +
> > +       ret = clk_enable(priv->clk);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (readback_type)
> > +               ret = zynq_fpga_read_cfgdata(mgr, s);
> > +       else
> > +               ret = zynq_fpga_read_cfgreg(mgr, s);
> > +
> > +       clk_disable(priv->clk);
> > +
> > +       return ret;
> > +}
> > +
> >  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,
> >  };
> >
> >  static int zynq_fpga_probe(struct platform_device *pdev)
> > --
> > 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
> 
> Thanks,
> 
> Moritz

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback
  2018-07-24 18:31     ` Appana Durga Kedareswara Rao
@ 2018-07-24 18:49       ` Alan Tull
  2018-07-25  9:12         ` Appana Durga Kedareswara Rao
  0 siblings, 1 reply; 10+ messages in thread
From: Alan Tull @ 2018-07-24 18:49 UTC (permalink / raw)
  To: Appana Durga Kedareswara Rao
  Cc: Moritz Fischer, Nava kishore Manne, Michal Simek, linux-fpga,
	linux-arm-kernel, Linux Kernel Mailing List

On Tue, Jul 24, 2018 at 1:31 PM, Appana Durga Kedareswara Rao
<appanad@xilinx.com> wrote:
> Hi Moritz,
>
>         Thanks for the review...
>
> <Snip>
>> Can you please make the commit message such that you have full sentences?
>>
>> "Add support for readback of FPGA configuration data and registers" of
>> example.
>
> Sure will fix in v4.
>
>>
>> >
>> > Usage:
>> > Readback of PL configuration registers
>> >         cat /sys/kernel/debug/fpga/fpga0/image
>> > Readback of PL configuration data
>> >         echo 1 > /sys/module/zynqmp_fpga/parameters/readback_type
>>
>> The patch below is for Zynq if I'm not mistaken, not ZynqMP?
>
> Yes it is for Zynq not ZynqMP by mistake I have kept zynqmp here, thanks for pointing  will fix in v4...
>
> <Snip>
>> > +static bool readback_type;
>> > +module_param(readback_type, bool, 0644);
>> > +MODULE_PARM_DESC(readback_type,
>> > +               "readback_type 0-configuration register read "
>> > +               "1- configuration data read (default: 0)");
>>
>> Not sure a module param is the best solution here.
>
> Need to provide flexibility to the user to switch between reading of FPGA registers and configuration data.

I suggested calling the attribute 'image' because I thought it would
always be a read back of the FPGA image information.  I don't think
that a sysfs attribute's function should change. :)

> One other option is sysfs. Do you want me to implement sysfs path???
> Any other suggestions???

You could implement another sysfs attribute for reading FPGA registers
with a separate ops callback.   Please clarify what it is doing (not
all FPGAs have this function) and what that will look like when the
user reads the from the sysfs

Alan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback
  2018-07-24 14:17 ` [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback Appana Durga Kedareswara rao
  2018-07-24 16:32   ` Moritz Fischer
@ 2018-07-24 18:52   ` Alan Tull
  2018-07-25  9:13     ` Appana Durga Kedareswara Rao
  1 sibling, 1 reply; 10+ messages in thread
From: Alan Tull @ 2018-07-24 18:52 UTC (permalink / raw)
  To: Appana Durga Kedareswara rao
  Cc: Moritz Fischer, navam, michals, linux-fpga,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

On Tue, Jul 24, 2018 at 9:17 AM, Appana Durga Kedareswara rao
<appana.durga.rao@xilinx.com> wrote:

Hi Appana,

Another minor thing.

> +
> +/****************************************************************************/

Let's keep the coding style consistent by not having
'***************************'

> +/**
> + *

Also, you don't need /** followed by '*', just take out the
essentially blank line here, please.

> + * Generates a Type 2 packet header that reads back the requested Configuration
> + * register.
> + *
> + * @param        OpCode is the read/write operation code.
> + * @param        Size is the size of the word to be read.
> + *
> + * @return       Type 2 packet header to read the specified register
> + *
> + * @note         None.
> + *
> + *****************************************************************************/
> +static int zynq_type2_pkt(u8 OpCode, u32 Size)
> +{

Thanks,
Alan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback
  2018-07-24 18:49       ` Alan Tull
@ 2018-07-25  9:12         ` Appana Durga Kedareswara Rao
  2018-07-25 15:35           ` Alan Tull
  0 siblings, 1 reply; 10+ messages in thread
From: Appana Durga Kedareswara Rao @ 2018-07-25  9:12 UTC (permalink / raw)
  To: Alan Tull
  Cc: Moritz Fischer, Nava kishore Manne, Michal Simek, linux-fpga,
	linux-arm-kernel, Linux Kernel Mailing List

Hi Alan,
	
	Thanks for the review...

<Snip> 
> >
> > <Snip>
> >> > +static bool readback_type;
> >> > +module_param(readback_type, bool, 0644);
> >> > +MODULE_PARM_DESC(readback_type,
> >> > +               "readback_type 0-configuration register read "
> >> > +               "1- configuration data read (default: 0)");
> >>
> >> Not sure a module param is the best solution here.
> >
> > Need to provide flexibility to the user to switch between reading of FPGA
> registers and configuration data.
> 
> I suggested calling the attribute 'image' because I thought it would always be
> a read back of the FPGA image information.  I don't think that a sysfs
> attribute's function should change. :)

In Zynq Case it supports two types of the readback (Configuration registers, Configuration data(fpga image))  which may not be the same case for other vendors. 
Since I need to support both the use cases I have differentiated them using module param in the zynq-fpga driver.

As you said we shouldn't change the attribute's function, So in the zynq-fpga driver,
I am planning to add another debugfs attribute for reading back of configuration registers as it is vendor specific readback type.
 (Configuration registers                   ----  Usage:  /sys/kernel/debug/fpga/zynq-fpga/cfgreg_summary) 
(Configuration data(fpga image)    ----   Usage: /sys/kernel/debug/fpga/fpga0/image)
Please let me know your thoughts for the same... 
	
> 
> > One other option is sysfs. Do you want me to implement sysfs path???
> > Any other suggestions???
> 
> You could implement another sysfs attribute for reading FPGA registers
> with a separate ops callback.   Please clarify what it is doing (not
> all FPGAs have this function) and what that will look like when the user reads
> the from the sysfs

AFAIK we shouldn't add another sysfs/debugfs attribute for vendor-specific readback type in the FPGA manager ops.
Please correct me if my understanding is wrong. 

Regards,
Kedar.

> 
> Alan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback
  2018-07-24 18:52   ` Alan Tull
@ 2018-07-25  9:13     ` Appana Durga Kedareswara Rao
  0 siblings, 0 replies; 10+ messages in thread
From: Appana Durga Kedareswara Rao @ 2018-07-25  9:13 UTC (permalink / raw)
  To: Alan Tull
  Cc: Moritz Fischer, Nava kishore Manne, Michal Simek, linux-fpga,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

Hi Alan,

	Thanks for the review...

<Snip> 
> Another minor thing.
> 
> > +
> >
> +/**************************************************************
> ******
> > +********/
> 
> Let's keep the coding style consistent by not having

Sure will fix in v4...

> '***************************'
> 
> > +/**
> > + *
> 
> Also, you don't need /** followed by '*', just take out the essentially blank
> line here, please.

Sure will fix in v4...

Regards,
Kedar.

> 
> > + * Generates a Type 2 packet header that reads back the requested
> > +Configuration
> > + * register.
> > + *
> > + * @param        OpCode is the read/write operation code.
> > + * @param        Size is the size of the word to be read.
> > + *
> > + * @return       Type 2 packet header to read the specified register
> > + *
> > + * @note         None.
> > + *
> > +
> >
> +***************************************************************
> ******
> > +********/ static int zynq_type2_pkt(u8 OpCode, u32 Size) {
> 
> Thanks,
> Alan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback
  2018-07-25  9:12         ` Appana Durga Kedareswara Rao
@ 2018-07-25 15:35           ` Alan Tull
  2018-07-26  5:20             ` Appana Durga Kedareswara Rao
  0 siblings, 1 reply; 10+ messages in thread
From: Alan Tull @ 2018-07-25 15:35 UTC (permalink / raw)
  To: Appana Durga Kedareswara Rao
  Cc: Moritz Fischer, Nava kishore Manne, Michal Simek, linux-fpga,
	linux-arm-kernel, Linux Kernel Mailing List

On Wed, Jul 25, 2018 at 4:12 AM, Appana Durga Kedareswara Rao
<appanad@xilinx.com> wrote:
> Hi Alan,
>
>         Thanks for the review...
>
> <Snip>
>> >
>> > <Snip>
>> >> > +static bool readback_type;
>> >> > +module_param(readback_type, bool, 0644);
>> >> > +MODULE_PARM_DESC(readback_type,
>> >> > +               "readback_type 0-configuration register read "
>> >> > +               "1- configuration data read (default: 0)");
>> >>
>> >> Not sure a module param is the best solution here.
>> >
>> > Need to provide flexibility to the user to switch between reading of FPGA
>> registers and configuration data.
>>
>> I suggested calling the attribute 'image' because I thought it would always be
>> a read back of the FPGA image information.  I don't think that a sysfs
>> attribute's function should change. :)
>
> In Zynq Case it supports two types of the readback (Configuration registers, Configuration data(fpga image))  which may not be the same case for other vendors.
> Since I need to support both the use cases I have differentiated them using module param in the zynq-fpga driver.
>
> As you said we shouldn't change the attribute's function, So in the zynq-fpga driver,
> I am planning to add another debugfs attribute for reading back of configuration registers as it is vendor specific readback type.
>  (Configuration registers                   ----  Usage:  /sys/kernel/debug/fpga/zynq-fpga/cfgreg_summary)

Is it called '...summary' because it is not all the registers?  Could
just be "cfg_regs"?

> (Configuration data(fpga image)    ----   Usage: /sys/kernel/debug/fpga/fpga0/image)
> Please let me know your thoughts for the same...
>
>>
>> > One other option is sysfs. Do you want me to implement sysfs path???
>> > Any other suggestions???
>>
>> You could implement another sysfs attribute for reading FPGA registers
>> with a separate ops callback.   Please clarify what it is doing (not
>> all FPGAs have this function) and what that will look like when the user reads
>> the from the sysfs
>
> AFAIK we shouldn't add another sysfs/debugfs attribute for vendor-specific readback type in the FPGA manager ops.
> Please correct me if my understanding is wrong.

You are not wrong :)

>
> Regards,
> Kedar.
>
>>
>> Alan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback
  2018-07-25 15:35           ` Alan Tull
@ 2018-07-26  5:20             ` Appana Durga Kedareswara Rao
  0 siblings, 0 replies; 10+ messages in thread
From: Appana Durga Kedareswara Rao @ 2018-07-26  5:20 UTC (permalink / raw)
  To: Alan Tull
  Cc: Moritz Fischer, Nava kishore Manne, Michal Simek, linux-fpga,
	linux-arm-kernel, Linux Kernel Mailing List

Hi Alan,

	Thanks for the review...

<Snip>
> > In Zynq Case it supports two types of the readback (Configuration registers,
> Configuration data(fpga image))  which may not be the same case for other
> vendors.
> > Since I need to support both the use cases I have differentiated them using
> module param in the zynq-fpga driver.
> >
> > As you said we shouldn't change the attribute's function, So in the
> > zynq-fpga driver, I am planning to add another debugfs attribute for
> reading back of configuration registers as it is vendor specific readback type.
> >  (Configuration registers                   ----  Usage:
> /sys/kernel/debug/fpga/zynq-fpga/cfgreg_summary)
> 
> Is it called '...summary' because it is not all the registers?  Could just be
> "cfg_regs"?

Sure will make it cfg_regs...
Are you ok with the above proposal?? 
if you are ok will make changes accordingly and will post v4...

> 
> > (Configuration data(fpga image)    ----   Usage:
> /sys/kernel/debug/fpga/fpga0/image)
> > Please let me know your thoughts for the same...
> >
> >>
> >> > One other option is sysfs. Do you want me to implement sysfs path???
> >> > Any other suggestions???
> >>
> >> You could implement another sysfs attribute for reading FPGA registers
> >> with a separate ops callback.   Please clarify what it is doing (not
> >> all FPGAs have this function) and what that will look like when the
> >> user reads the from the sysfs
> >
> > AFAIK we shouldn't add another sysfs/debugfs attribute for vendor-specific
> readback type in the FPGA manager ops.
> > Please correct me if my understanding is wrong.
> 
> You are not wrong :)

Thanks 😊 

Regards,
Kedar.

> 
> >
> > Regards,
> > Kedar.
> >
> >>
> >> Alan

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-07-26  5:21 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-24 14:17 [PATCH v3 1/2] fpga: fpga-mgr: Add readback support Appana Durga Kedareswara rao
2018-07-24 14:17 ` [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback Appana Durga Kedareswara rao
2018-07-24 16:32   ` Moritz Fischer
2018-07-24 18:31     ` Appana Durga Kedareswara Rao
2018-07-24 18:49       ` Alan Tull
2018-07-25  9:12         ` Appana Durga Kedareswara Rao
2018-07-25 15:35           ` Alan Tull
2018-07-26  5:20             ` Appana Durga Kedareswara Rao
2018-07-24 18:52   ` Alan Tull
2018-07-25  9:13     ` Appana Durga Kedareswara Rao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).