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 <[email protected]>
---
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
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 <[email protected]>
---
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
Hi Appana,
On Tue, Jul 24, 2018 at 7:17 AM, Appana Durga Kedareswara rao
<[email protected]> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks,
Moritz
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 [email protected] More
> majordomo
> > info at http://vger.kernel.org/majordomo-info.html
>
> Thanks,
>
> Moritz
On Tue, Jul 24, 2018 at 1:31 PM, Appana Durga Kedareswara Rao
<[email protected]> 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
On Tue, Jul 24, 2018 at 9:17 AM, Appana Durga Kedareswara rao
<[email protected]> 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
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
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
On Wed, Jul 25, 2018 at 4:12 AM, Appana Durga Kedareswara Rao
<[email protected]> 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
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