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 v4:
--> None.
Changes for v3:
--> None.
Changes for v2:
--> Fixed debug attribute path and name as suggested by Alan
--> Add config entry for DEBUG as suggested by Alan
--> Fixed trival coding style issues.
drivers/fpga/Kconfig | 7 +++++
drivers/fpga/fpga-mgr.c | 68 +++++++++++++++++++++++++++++++++++++++++++
include/linux/fpga/fpga-mgr.h | 5 ++++
3 files changed, 80 insertions(+)
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 53d3f55..838ad4e 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -11,6 +11,13 @@ menuconfig FPGA
if FPGA
+config FPGA_MGR_DEBUG_FS
+ tristate "FPGA Debug fs"
+ select DEBUG_FS
+ help
+ FPGA manager debug provides support for reading fpga configuration
+ information.
+
config FPGA_MGR_SOCFPGA
tristate "Altera SOCFPGA FPGA Manager"
depends on ARCH_SOCFPGA || COMPILE_TEST
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 9939d2c..4bea860 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)
}
EXPORT_SYMBOL_GPL(fpga_mgr_put);
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+#include <linux/debugfs.h>
+
+static int fpga_mgr_read(struct seq_file *s, void *data)
+{
+ struct fpga_manager *mgr = (struct fpga_manager *)s->private;
+ int ret = 0;
+
+ if (!mgr->mops->read)
+ return -ENOENT;
+
+ if (!mutex_trylock(&mgr->ref_mutex))
+ return -EBUSY;
+
+ if (mgr->state != FPGA_MGR_STATE_OPERATING) {
+ ret = -EPERM;
+ goto err_unlock;
+ }
+
+ /* Read the FPGA configuration data from the fabric */
+ ret = mgr->mops->read(mgr, s);
+ if (ret)
+ dev_err(&mgr->dev, "Error while reading configuration data from FPGA\n");
+
+err_unlock:
+ mutex_unlock(&mgr->ref_mutex);
+
+ return ret;
+}
+
+static int fpga_mgr_read_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, fpga_mgr_read, inode->i_private);
+}
+
+static const struct file_operations fpga_mgr_ops_image = {
+ .owner = THIS_MODULE,
+ .open = fpga_mgr_read_open,
+ .read = seq_read,
+};
+#endif
+
/**
* fpga_mgr_lock - Lock FPGA manager for exclusive use
* @mgr: fpga manager
@@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const char *name,
if (ret)
goto error_device;
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+ struct dentry *d, *parent;
+
+ mgr->dir = debugfs_create_dir("fpga", NULL);
+ if (!mgr->dir)
+ goto error_device;
+
+ parent = mgr->dir;
+ d = debugfs_create_dir(mgr->dev.kobj.name, parent);
+ if (!d) {
+ debugfs_remove_recursive(parent);
+ goto error_device;
+ }
+
+ parent = d;
+ d = debugfs_create_file("image", 0644, parent, mgr,
+ &fpga_mgr_ops_image);
+ if (!d) {
+ debugfs_remove_recursive(mgr->dir);
+ goto error_device;
+ }
+#endif
+
dev_info(&mgr->dev, "%s registered\n", mgr->name);
return 0;
@@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+ debugfs_remove_recursive(mgr->dir);
+#endif
/*
* If the low level driver provides a method for putting fpga into
* a desired state upon unregister, do it.
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 3c6de23..e9e17a9 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -114,6 +114,7 @@ struct fpga_image_info {
* @write: write count bytes of configuration data to the FPGA
* @write_sg: write the scatter list of configuration data to the FPGA
* @write_complete: set FPGA to operating state after writing is done
+ * @read: optional: read FPGA configuration information
* @fpga_remove: optional: Set FPGA into a specific state during driver remove
* @groups: optional attribute groups.
*
@@ -131,6 +132,7 @@ struct fpga_manager_ops {
int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
int (*write_complete)(struct fpga_manager *mgr,
struct fpga_image_info *info);
+ int (*read)(struct fpga_manager *mgr, struct seq_file *s);
void (*fpga_remove)(struct fpga_manager *mgr);
const struct attribute_group **groups;
};
@@ -151,6 +153,9 @@ struct fpga_manager {
enum fpga_mgr_states state;
const struct fpga_manager_ops *mops;
void *priv;
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+ struct dentry *dir;
+#endif
};
#define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
--
2.7.4
This patch adds support for readback of FPGA configuration data and registers.
Usage:
Readback of PL configuration data
cat /sys/kernel/debug/fpga/fpga0/image
Readback of PL configuration registers
cat /sys/kernel/debug/fpga/f8007000.devcfg/cfg_reg
Signed-off-by: Appana Durga Kedareswara rao <[email protected]>
---
Changes for v4:
--> Improved commit message description as suggested by Moritz.
--> Used GENMASK and BIT() Macros wherever applicable as suggested by Moritz.
--> Fixed commenting sytle issues as suggested by Moritz and Alan.
--> Get rid of the readback_type module param as suggested by Alan and Moritz.
Changes for v3:
--> Added support for pl configuration data readback
--> Improved the pl configuration register readback logic.
Changes for v2:
--> Removed locks from the read_ops as lock handling is done in the framework.
drivers/fpga/zynq-fpga.c | 430 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 430 insertions(+)
diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 70b15b3..1746d18 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -31,6 +31,7 @@
#include <linux/regmap.h>
#include <linux/string.h>
#include <linux/scatterlist.h>
+#include <linux/seq_file.h>
/* Offsets into SLCR regmap */
@@ -127,6 +128,69 @@
/* Disable global resets */
#define FPGA_RST_NONE_MASK 0x0
+/**
+ * struct zynq_configreg - Configuration register offsets
+ * @reg: Name of the configuration register.
+ * @offset: Register offset.
+ */
+struct zynq_configreg {
+ char *reg;
+ u32 offset;
+};
+
+static struct zynq_configreg cfgreg[] = {
+ {.reg = "CRC", .offset = 0},
+ {.reg = "FAR", .offset = 1},
+ {.reg = "FDRI", .offset = 2},
+ {.reg = "FDRO", .offset = 3},
+ {.reg = "CMD", .offset = 4},
+ {.reg = "CTRL0", .offset = 5},
+ {.reg = "MASK", .offset = 6},
+ {.reg = "STAT", .offset = 7},
+ {.reg = "LOUT", .offset = 8},
+ {.reg = "COR0", .offset = 9},
+ {.reg = "MFWR", .offset = 10},
+ {.reg = "CBC", .offset = 11},
+ {.reg = "IDCODE", .offset = 12},
+ {.reg = "AXSS", .offset = 13},
+ {.reg = "COR1", .offset = 14},
+ {.reg = "WBSTR", .offset = 16},
+ {.reg = "TIMER", .offset = 17},
+ {.reg = "BOOTSTS", .offset = 22},
+ {.reg = "CTRL1", .offset = 24},
+ {}
+};
+
+/* Masks for Configuration registers */
+#define FAR_ADDR_MASK 0x00000000
+#define RCFG_CMD_MASK BIT(2)
+#define START_CMD_MASK BIT(2) + BIT(0)
+#define RCRC_CMD_MASK GENMASK(2, 0)
+#define SHUTDOWN_CMD_MASK GENMASK(1, 0) + BIT(3)
+#define DESYNC_WORD_MASK GENMASK(2, 3) + BIT(0)
+#define BUSWIDTH_SYNCWORD_MASK 0x000000BB
+#define NOOP_WORD_MASK BIT(29)
+#define BUSWIDTH_DETECT_MASK 0x11220044
+#define SYNC_WORD_MASK 0xAA995566
+#define DUMMY_WORD_MASK GENMASK(31, 0)
+
+#define TYPE_HDR_SHIFT 29
+#define TYPE_REG_SHIFT 13
+#define TYPE_OP_SHIFT 27
+#define TYPE_OPCODE_NOOP 0
+#define TYPE_OPCODE_READ 1
+#define TYPE_OPCODE_WRITE 2
+#define TYPE_FAR_OFFSET 1
+#define TYPE_FDRO_OFFSET 3
+#define TYPE_CMD_OFFSET 4
+
+#define READ_STEP5_NOOPS 6
+#define READ_STEP9_NOOPS 32
+
+#define READ_DMA_SIZE 0x200
+#define DUMMY_FRAMES_SIZE 0x28
+#define SLCR_PCAP_FREQ 10000000
+
struct zynq_fpga_priv {
int irq;
struct clk *clk;
@@ -140,6 +204,11 @@ struct zynq_fpga_priv {
struct scatterlist *cur_sg;
struct completion dma_done;
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+ struct mutex ref_mutex;
+ struct dentry *dir;
+#endif
+ u32 size;
};
static inline void zynq_fpga_write(struct zynq_fpga_priv *priv, u32 offset,
@@ -164,6 +233,27 @@ static inline void zynq_fpga_set_irq(struct zynq_fpga_priv *priv, u32 enable)
zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
}
+static void zynq_fpga_dma_xfer(struct zynq_fpga_priv *priv, u32 srcaddr,
+ u32 srclen, u32 dstaddr, u32 dstlen)
+{
+ zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, srcaddr);
+ zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, dstaddr);
+ zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, srclen);
+ zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, dstlen);
+}
+
+static int zynq_fpga_wait_fordone(struct zynq_fpga_priv *priv)
+{
+ u32 status;
+ int ret;
+
+ ret = zynq_fpga_poll_timeout(priv, INT_STS_OFFSET, status,
+ status & IXR_D_P_DONE_MASK,
+ INIT_POLL_DELAY,
+ INIT_POLL_TIMEOUT);
+ return ret;
+}
+
/* Must be called with dma_lock held */
static void zynq_step_dma(struct zynq_fpga_priv *priv)
{
@@ -192,6 +282,7 @@ static void zynq_step_dma(struct zynq_fpga_priv *priv)
priv->dma_elm++;
}
+ priv->size += len;
zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, addr);
zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, DMA_INVALID_ADDRESS);
zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, len / 4);
@@ -401,6 +492,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr, struct sg_table *sgt)
int i;
priv = mgr->priv;
+ priv->size = 0;
/* The hardware can only DMA multiples of 4 bytes, and it requires the
* starting addresses to be aligned to 64 bits (UG585 pg 212).
@@ -546,12 +638,327 @@ static enum fpga_mgr_states zynq_fpga_ops_state(struct fpga_manager *mgr)
return FPGA_MGR_STATE_UNKNOWN;
}
+static int zynq_type1_pkt(u8 reg, u8 opcode, u16 size)
+{
+ /*
+ * Type 1 Packet Header Format
+ * The header section is always a 32-bit word.
+ *
+ * HeaderType | Opcode | Register Address | Reserved | Word Count
+ * [31:29] [28:27] [26:13] [12:11] [10:0]
+ * --------------------------------------------------------------
+ * 001 xx RRRRRRRRRxxxxx RR xxxxxxxxxxx
+ *
+ * @R: means the bit is not used and reserved for future use.
+ * The reserved bits should be written as 0s.
+ *
+ * Generating the Type 1 packet header which involves shifting of Type1
+ * Header Mask, Register value and the OpCode which is 01 in this case
+ * as only read operation is to be carried out and then performing OR
+ * operation with the Word Length.
+ * For more details refer ug470 Packet Types section Table 5-20.
+ */
+ return (((1 << TYPE_HDR_SHIFT) |
+ (reg << TYPE_REG_SHIFT) |
+ (opcode << TYPE_OP_SHIFT)) | size);
+
+}
+
+static int zynq_type2_pkt(u8 opcode, u32 size)
+{
+ /*
+ * Type 2 Packet Header Format
+ * The header section is always a 32-bit word.
+ *
+ * HeaderType | Opcode | Word Count
+ * [31:29] [28:27] [26:0]
+ * --------------------------------------------------------------
+ * 010 xx xxxxxxxxxxxxx
+ *
+ * @R: means the bit is not used and reserved for future use.
+ * The reserved bits should be written as 0s.
+ *
+ * Generating the Type 2 packet header which involves shifting of Type 2
+ * Header Mask, OpCode and then performing OR operation with the Word
+ * Length. For more details refer ug470 Packet Types section
+ * Table 5-22.
+ */
+ return (((2 << TYPE_HDR_SHIFT) |
+ (opcode << TYPE_OP_SHIFT)) | size);
+}
+
+static int zynq_fpga_ops_read_image(struct fpga_manager *mgr,
+ struct seq_file *s)
+{
+ struct zynq_fpga_priv *priv = mgr->priv;
+ int ret = 0, cmdindex, clk_rate;
+ u32 intr_status, status, i;
+ dma_addr_t dma_addr;
+ unsigned int *buf;
+ size_t size;
+
+ ret = clk_enable(priv->clk);
+ if (ret)
+ return ret;
+
+ size = priv->size + READ_DMA_SIZE + DUMMY_FRAMES_SIZE;
+ buf = dma_zalloc_coherent(mgr->dev.parent, size,
+ &dma_addr, GFP_KERNEL);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto disable_clk;
+ }
+
+ seq_puts(s, "Zynq FPGA Configuration data contents are\n");
+
+ /*
+ * There is no h/w flow control for pcap read
+ * to prevent the FIFO from over flowing, reduce
+ * the PCAP operating frequency.
+ */
+ clk_rate = clk_get_rate(priv->clk);
+ ret = clk_set_rate(priv->clk, SLCR_PCAP_FREQ);
+ if (ret) {
+ dev_err(&mgr->dev, "Unable to reduce the PCAP freq\n");
+ goto free_dmabuf;
+ }
+
+ ret = zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status,
+ status & STATUS_PCFG_INIT_MASK,
+ INIT_POLL_DELAY,
+ INIT_POLL_TIMEOUT);
+ if (ret) {
+ dev_err(&mgr->dev, "Timeout waiting for PCFG_INIT\n");
+ goto restore_pcap_clk;
+ }
+
+ cmdindex = 0;
+ buf[cmdindex++] = DUMMY_WORD_MASK;
+ buf[cmdindex++] = BUSWIDTH_SYNCWORD_MASK;
+ buf[cmdindex++] = BUSWIDTH_DETECT_MASK;
+ buf[cmdindex++] = DUMMY_WORD_MASK;
+ buf[cmdindex++] = SYNC_WORD_MASK;
+ buf[cmdindex++] = NOOP_WORD_MASK;
+ buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET, TYPE_OPCODE_WRITE,
+ 1);
+ buf[cmdindex++] = SHUTDOWN_CMD_MASK;
+ buf[cmdindex++] = NOOP_WORD_MASK;
+ buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET, TYPE_OPCODE_WRITE,
+ 1);
+ buf[cmdindex++] = RCRC_CMD_MASK;
+ for (i = 0; i < READ_STEP5_NOOPS; i++)
+ buf[cmdindex++] = NOOP_WORD_MASK;
+ buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET, TYPE_OPCODE_WRITE,
+ 1);
+ buf[cmdindex++] = RCFG_CMD_MASK;
+ buf[cmdindex++] = NOOP_WORD_MASK;
+ buf[cmdindex++] = zynq_type1_pkt(TYPE_FAR_OFFSET, TYPE_OPCODE_WRITE,
+ 1);
+ buf[cmdindex++] = FAR_ADDR_MASK;
+ buf[cmdindex++] = zynq_type1_pkt(TYPE_FDRO_OFFSET, TYPE_OPCODE_READ,
+ 0);
+ buf[cmdindex++] = zynq_type2_pkt(TYPE_OPCODE_READ, priv->size/4);
+ for (i = 0; i < READ_STEP9_NOOPS; i++)
+ buf[cmdindex++] = NOOP_WORD_MASK;
+
+ intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
+ zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
+
+ /* Write to PCAP */
+ zynq_fpga_dma_xfer(priv, dma_addr, cmdindex,
+ DMA_INVALID_ADDRESS, 0);
+ ret = zynq_fpga_wait_fordone(priv);
+ if (ret) {
+ dev_err(&mgr->dev, "SRCDMA: Timeout waiting for D_P_DONE\n");
+ goto restore_pcap_clk;
+ }
+ intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
+ zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
+
+ /* Read From PACP */
+ zynq_fpga_dma_xfer(priv, DMA_INVALID_ADDRESS, 0,
+ dma_addr + READ_DMA_SIZE, priv->size/4);
+ ret = zynq_fpga_wait_fordone(priv);
+ if (ret) {
+ dev_err(&mgr->dev, "DSTDMA: Timeout waiting for D_P_DONE\n");
+ goto restore_pcap_clk;
+ }
+ intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
+ zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
+
+ /* Write to PCAP */
+ cmdindex = 0;
+ buf[cmdindex++] = NOOP_WORD_MASK;
+ buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET,
+ TYPE_OPCODE_WRITE, 1);
+ buf[cmdindex++] = START_CMD_MASK;
+ buf[cmdindex++] = NOOP_WORD_MASK;
+
+ buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET,
+ TYPE_OPCODE_WRITE, 1);
+ buf[cmdindex++] = RCRC_CMD_MASK;
+ buf[cmdindex++] = NOOP_WORD_MASK;
+ buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET,
+ TYPE_OPCODE_WRITE, 1);
+ buf[cmdindex++] = DESYNC_WORD_MASK;
+ buf[cmdindex++] = NOOP_WORD_MASK;
+ buf[cmdindex++] = NOOP_WORD_MASK;
+
+ zynq_fpga_dma_xfer(priv, dma_addr, cmdindex,
+ DMA_INVALID_ADDRESS, 0);
+ ret = zynq_fpga_wait_fordone(priv);
+ if (ret) {
+ dev_err(&mgr->dev, "SRCDMA1: Timeout waiting for D_P_DONE\n");
+ goto restore_pcap_clk;
+ }
+
+ seq_write(s, &buf[READ_DMA_SIZE/4], priv->size);
+
+restore_pcap_clk:
+ clk_set_rate(priv->clk, clk_rate);
+free_dmabuf:
+ dma_free_coherent(mgr->dev.parent, size, buf,
+ dma_addr);
+disable_clk:
+ clk_disable(priv->clk);
+ return ret;
+}
+
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+#include <linux/debugfs.h>
+
+static int zynq_fpga_getconfigreg(struct fpga_manager *mgr, u8 reg,
+ dma_addr_t dma_addr, int *buf)
+{
+ struct zynq_fpga_priv *priv = mgr->priv;
+ int ret = 0, cmdindex, src_dmaoffset;
+ u32 intr_status, status;
+
+ src_dmaoffset = 0x8;
+ cmdindex = 2;
+ buf[cmdindex++] = DUMMY_WORD_MASK;
+ buf[cmdindex++] = BUSWIDTH_SYNCWORD_MASK;
+ buf[cmdindex++] = BUSWIDTH_DETECT_MASK;
+ buf[cmdindex++] = DUMMY_WORD_MASK;
+ buf[cmdindex++] = SYNC_WORD_MASK;
+ buf[cmdindex++] = NOOP_WORD_MASK;
+ buf[cmdindex++] = zynq_type1_pkt(reg, TYPE_OPCODE_READ, 1);
+ buf[cmdindex++] = NOOP_WORD_MASK;
+ buf[cmdindex++] = NOOP_WORD_MASK;
+
+ ret = zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status,
+ status & STATUS_PCFG_INIT_MASK,
+ INIT_POLL_DELAY,
+ INIT_POLL_TIMEOUT);
+ if (ret) {
+ dev_err(&mgr->dev, "Timeout waiting for PCFG_INIT\n");
+ goto out;
+ }
+
+ /* Write to PCAP */
+ intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
+ zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
+
+ zynq_fpga_dma_xfer(priv, dma_addr + src_dmaoffset, cmdindex,
+ DMA_INVALID_ADDRESS, 0);
+ ret = zynq_fpga_wait_fordone(priv);
+ if (ret) {
+ dev_err(&mgr->dev, "SRCDMA: Timeout waiting for D_P_DONE\n");
+ goto out;
+ }
+ zynq_fpga_set_irq(priv, intr_status);
+
+ /* Read from PACP */
+ zynq_fpga_dma_xfer(priv, DMA_INVALID_ADDRESS, 0, dma_addr, 1);
+ ret = zynq_fpga_wait_fordone(priv);
+ if (ret) {
+ dev_err(&mgr->dev, "DSTDMA: Timeout waiting for D_P_DONE\n");
+ goto out;
+ }
+
+ /* Write to PCAP */
+ cmdindex = 2;
+ buf[cmdindex++] = zynq_type1_pkt(TYPE_CMD_OFFSET,
+ TYPE_OPCODE_WRITE, 1);
+ buf[cmdindex++] = DESYNC_WORD_MASK;
+ buf[cmdindex++] = NOOP_WORD_MASK;
+ buf[cmdindex++] = NOOP_WORD_MASK;
+ zynq_fpga_dma_xfer(priv, dma_addr + src_dmaoffset, cmdindex,
+ DMA_INVALID_ADDRESS, 0);
+ ret = zynq_fpga_wait_fordone(priv);
+ if (ret)
+ dev_err(&mgr->dev, "SRCDMA1: Timeout waiting for D_P_DONE\n");
+out:
+ return ret;
+}
+
+static int zynq_fpga_read_cfg_reg(struct seq_file *s, void *data)
+{
+ struct fpga_manager *mgr = (struct fpga_manager *)s->private;
+ struct zynq_fpga_priv *priv = mgr->priv;
+ struct zynq_configreg *p = cfgreg;
+ dma_addr_t dma_addr;
+ unsigned int *buf;
+ int ret = 0;
+
+ if (!mutex_trylock(&priv->ref_mutex))
+ return -EBUSY;
+
+ if (mgr->state != FPGA_MGR_STATE_OPERATING) {
+ ret = -EPERM;
+ goto err_unlock;
+ }
+
+ ret = clk_enable(priv->clk);
+ if (ret)
+ goto err_unlock;
+
+ buf = dma_zalloc_coherent(mgr->dev.parent, READ_DMA_SIZE,
+ &dma_addr, GFP_KERNEL);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto disable_clk;
+ }
+
+ seq_puts(s, "Zynq FPGA Configuration register contents are\n");
+
+ while (p->reg) {
+ ret = zynq_fpga_getconfigreg(mgr, p->offset, dma_addr, buf);
+ if (ret)
+ goto free_dmabuf;
+ seq_printf(s, "%s --> \t %x \t\r\n", p->reg, buf[0]);
+ p++;
+ }
+
+free_dmabuf:
+ dma_free_coherent(mgr->dev.parent, READ_DMA_SIZE, buf,
+ dma_addr);
+disable_clk:
+ clk_disable(priv->clk);
+err_unlock:
+ mutex_unlock(&priv->ref_mutex);
+ return ret;
+}
+
+static int zynq_fpga_read_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, zynq_fpga_read_cfg_reg, inode->i_private);
+}
+
+static const struct file_operations zynq_fpga_ops_cfg_reg = {
+ .owner = THIS_MODULE,
+ .open = zynq_fpga_read_open,
+ .read = seq_read,
+};
+#endif
+
static const struct fpga_manager_ops zynq_fpga_ops = {
.initial_header_size = 128,
.state = zynq_fpga_ops_state,
.write_init = zynq_fpga_ops_write_init,
.write_sg = zynq_fpga_ops_write,
.write_complete = zynq_fpga_ops_write_complete,
+ .read = zynq_fpga_ops_read_image,
};
static int zynq_fpga_probe(struct platform_device *pdev)
@@ -621,6 +1028,26 @@ static int zynq_fpga_probe(struct platform_device *pdev)
return err;
}
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+ struct dentry *d;
+ struct fpga_manager *mgr;
+
+ mgr = platform_get_drvdata(pdev);
+ mutex_init(&priv->ref_mutex);
+
+ d = debugfs_create_dir(pdev->dev.kobj.name, mgr->dir);
+ if (!d)
+ return err;
+
+ priv->dir = d;
+ d = debugfs_create_file("cfg_reg", 0644, priv->dir, mgr,
+ &zynq_fpga_ops_cfg_reg);
+ if (!d) {
+ debugfs_remove_recursive(mgr->dir);
+ return err;
+ }
+#endif
+
return 0;
}
@@ -632,6 +1059,9 @@ static int zynq_fpga_remove(struct platform_device *pdev)
mgr = platform_get_drvdata(pdev);
priv = mgr->priv;
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+ debugfs_remove_recursive(priv->dir);
+#endif
fpga_mgr_unregister(&pdev->dev);
clk_unprepare(priv->clk);
--
2.7.4
Hi Appana,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on sof-driver-fuweitax/master]
[cannot apply to v4.18-rc6 next-20180727]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Appana-Durga-Kedareswara-rao/fpga-fpga-mgr-Add-readback-support/20180728-034920
base: https://github.com/fuweitax/linux master
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=xtensa
All warnings (new ones prefixed by >>):
drivers/fpga/zynq-fpga.c: In function 'zynq_fpga_probe':
>> drivers/fpga/zynq-fpga.c:1032:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
struct dentry *d;
^~~~~~
vim +1032 drivers/fpga/zynq-fpga.c
963
964 static int zynq_fpga_probe(struct platform_device *pdev)
965 {
966 struct device *dev = &pdev->dev;
967 struct zynq_fpga_priv *priv;
968 struct resource *res;
969 int err;
970
971 priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
972 if (!priv)
973 return -ENOMEM;
974 spin_lock_init(&priv->dma_lock);
975
976 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
977 priv->io_base = devm_ioremap_resource(dev, res);
978 if (IS_ERR(priv->io_base))
979 return PTR_ERR(priv->io_base);
980
981 priv->slcr = syscon_regmap_lookup_by_phandle(dev->of_node,
982 "syscon");
983 if (IS_ERR(priv->slcr)) {
984 dev_err(dev, "unable to get zynq-slcr regmap\n");
985 return PTR_ERR(priv->slcr);
986 }
987
988 init_completion(&priv->dma_done);
989
990 priv->irq = platform_get_irq(pdev, 0);
991 if (priv->irq < 0) {
992 dev_err(dev, "No IRQ available\n");
993 return priv->irq;
994 }
995
996 priv->clk = devm_clk_get(dev, "ref_clk");
997 if (IS_ERR(priv->clk)) {
998 dev_err(dev, "input clock not found\n");
999 return PTR_ERR(priv->clk);
1000 }
1001
1002 err = clk_prepare_enable(priv->clk);
1003 if (err) {
1004 dev_err(dev, "unable to enable clock\n");
1005 return err;
1006 }
1007
1008 /* unlock the device */
1009 zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
1010
1011 zynq_fpga_set_irq(priv, 0);
1012 zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
1013 err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0, dev_name(dev),
1014 priv);
1015 if (err) {
1016 dev_err(dev, "unable to request IRQ\n");
1017 clk_disable_unprepare(priv->clk);
1018 return err;
1019 }
1020
1021 clk_disable(priv->clk);
1022
1023 err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
1024 &zynq_fpga_ops, priv);
1025 if (err) {
1026 dev_err(dev, "unable to register FPGA manager\n");
1027 clk_unprepare(priv->clk);
1028 return err;
1029 }
1030
1031 #ifdef CONFIG_FPGA_MGR_DEBUG_FS
> 1032 struct dentry *d;
1033 struct fpga_manager *mgr;
1034
1035 mgr = platform_get_drvdata(pdev);
1036 mutex_init(&priv->ref_mutex);
1037
1038 d = debugfs_create_dir(pdev->dev.kobj.name, mgr->dir);
1039 if (!d)
1040 return err;
1041
1042 priv->dir = d;
1043 d = debugfs_create_file("cfg_reg", 0644, priv->dir, mgr,
1044 &zynq_fpga_ops_cfg_reg);
1045 if (!d) {
1046 debugfs_remove_recursive(mgr->dir);
1047 return err;
1048 }
1049 #endif
1050
1051 return 0;
1052 }
1053
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao
<[email protected]> wrote:
Hi Appana,
There should be some documentation for the debugfs added under
Documentation/driver-api/fpga/
Also there are a lot of #ifdefs that were added due to the
CONFIG_FPGA_MGR_DEBUG_FS. This has caused a kernel robot complaint.
The way to fix that is to have a separate c file for the debugfs
implementation. I see a lot of other kernel drivers have done it this
way. I have an implementation that I haven't submitted yet, it
exposes different functionality (exposing the image firmware file name
and writing to the image file). It's on the downstream
github.com/altera-opensource repo [1]. I'll clean this up and submit
it to the mailing list, it may take a minute for me to get to that.
Once that's done, your added functionality can be a patch on top of
that.
Alan
[1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-4.17/drivers/fpga/fpga-mgr-debugfs.c
https://github.com/altera-opensource/linux-socfpga/blob/socfpga-4.17/drivers/fpga/fpga-mgr-debugfs.h
> Inorder to debug issues with fpga's users would
> like to read the fpga configuration information.
> This patch adds readback support for fpga configuration data
> in the framework through debugfs interface.
>
> Usage:
> cat /sys/kernel/debug/fpga/fpga0/image
>
> Signed-off-by: Appana Durga Kedareswara rao <[email protected]>
> ---
> Changes for v4:
> --> None.
> Changes for v3:
> --> None.
> Changes for v2:
> --> Fixed debug attribute path and name as suggested by Alan
> --> Add config entry for DEBUG as suggested by Alan
> --> Fixed trival coding style issues.
>
> drivers/fpga/Kconfig | 7 +++++
> drivers/fpga/fpga-mgr.c | 68 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/fpga/fpga-mgr.h | 5 ++++
> 3 files changed, 80 insertions(+)
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 53d3f55..838ad4e 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -11,6 +11,13 @@ menuconfig FPGA
>
> if FPGA
>
> +config FPGA_MGR_DEBUG_FS
> + tristate "FPGA Debug fs"
> + select DEBUG_FS
> + help
> + FPGA manager debug provides support for reading fpga configuration
> + information.
> +
> config FPGA_MGR_SOCFPGA
> tristate "Altera SOCFPGA FPGA Manager"
> depends on ARCH_SOCFPGA || COMPILE_TEST
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 9939d2c..4bea860 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)
> }
> EXPORT_SYMBOL_GPL(fpga_mgr_put);
>
> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> +#include <linux/debugfs.h>
> +
> +static int fpga_mgr_read(struct seq_file *s, void *data)
> +{
> + struct fpga_manager *mgr = (struct fpga_manager *)s->private;
> + int ret = 0;
> +
> + if (!mgr->mops->read)
> + return -ENOENT;
> +
> + if (!mutex_trylock(&mgr->ref_mutex))
> + return -EBUSY;
> +
> + if (mgr->state != FPGA_MGR_STATE_OPERATING) {
> + ret = -EPERM;
> + goto err_unlock;
> + }
> +
> + /* Read the FPGA configuration data from the fabric */
> + ret = mgr->mops->read(mgr, s);
> + if (ret)
> + dev_err(&mgr->dev, "Error while reading configuration data from FPGA\n");
> +
> +err_unlock:
> + mutex_unlock(&mgr->ref_mutex);
> +
> + return ret;
> +}
> +
> +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, fpga_mgr_read, inode->i_private);
> +}
> +
> +static const struct file_operations fpga_mgr_ops_image = {
> + .owner = THIS_MODULE,
> + .open = fpga_mgr_read_open,
> + .read = seq_read,
> +};
> +#endif
> +
> /**
> * fpga_mgr_lock - Lock FPGA manager for exclusive use
> * @mgr: fpga manager
> @@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const char *name,
> if (ret)
> goto error_device;
>
> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> + struct dentry *d, *parent;
> +
> + mgr->dir = debugfs_create_dir("fpga", NULL);
> + if (!mgr->dir)
> + goto error_device;
> +
> + parent = mgr->dir;
> + d = debugfs_create_dir(mgr->dev.kobj.name, parent);
> + if (!d) {
> + debugfs_remove_recursive(parent);
> + goto error_device;
> + }
> +
> + parent = d;
> + d = debugfs_create_file("image", 0644, parent, mgr,
> + &fpga_mgr_ops_image);
> + if (!d) {
> + debugfs_remove_recursive(mgr->dir);
> + goto error_device;
> + }
> +#endif
> +
> dev_info(&mgr->dev, "%s registered\n", mgr->name);
>
> return 0;
> @@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
>
> dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>
> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> + debugfs_remove_recursive(mgr->dir);
> +#endif
> /*
> * If the low level driver provides a method for putting fpga into
> * a desired state upon unregister, do it.
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 3c6de23..e9e17a9 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -114,6 +114,7 @@ struct fpga_image_info {
> * @write: write count bytes of configuration data to the FPGA
> * @write_sg: write the scatter list of configuration data to the FPGA
> * @write_complete: set FPGA to operating state after writing is done
> + * @read: optional: read FPGA configuration information
> * @fpga_remove: optional: Set FPGA into a specific state during driver remove
> * @groups: optional attribute groups.
> *
> @@ -131,6 +132,7 @@ struct fpga_manager_ops {
> int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
> int (*write_complete)(struct fpga_manager *mgr,
> struct fpga_image_info *info);
> + int (*read)(struct fpga_manager *mgr, struct seq_file *s);
> void (*fpga_remove)(struct fpga_manager *mgr);
> const struct attribute_group **groups;
> };
> @@ -151,6 +153,9 @@ struct fpga_manager {
> enum fpga_mgr_states state;
> const struct fpga_manager_ops *mops;
> void *priv;
> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> + struct dentry *dir;
> +#endif
> };
>
> #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan,
Thanks for the review...
<Snip>
> On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao
> <[email protected]> wrote:
>
> Hi Appana,
>
> There should be some documentation for the debugfs added under
> Documentation/driver-api/fpga/
>
> Also there are a lot of #ifdefs that were added due to the
> CONFIG_FPGA_MGR_DEBUG_FS. This has caused a kernel robot complaint.
> The way to fix that is to have a separate c file for the debugfs
> implementation. I see a lot of other kernel drivers have done it this way. I
> have an implementation that I haven't submitted yet, it exposes different
> functionality (exposing the image firmware file name and writing to the
> image file). It's on the downstream github.com/altera-opensource repo [1].
> I'll clean this up and submit it to the mailing list, it may take a minute for me
> to get to that.
> Once that's done, your added functionality can be a patch on top of that.
Sounds good...
Let me know if you need any help on testing this will do.
Regards,
Kedar.
>
> Alan
>
> [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.c
> https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.h
>
>
> > Inorder to debug issues with fpga's users would like to read the fpga
> > configuration information.
> > This patch adds readback support for fpga configuration data in the
> > framework through debugfs interface.
> >
> > Usage:
> > cat /sys/kernel/debug/fpga/fpga0/image
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > <[email protected]>
> > ---
> > Changes for v4:
> > --> None.
> > Changes for v3:
> > --> None.
> > Changes for v2:
> > --> Fixed debug attribute path and name as suggested by Alan Add
> > --> config entry for DEBUG as suggested by Alan Fixed trival coding
> > --> style issues.
> >
> > drivers/fpga/Kconfig | 7 +++++
> > drivers/fpga/fpga-mgr.c | 68
> +++++++++++++++++++++++++++++++++++++++++++
> > include/linux/fpga/fpga-mgr.h | 5 ++++
> > 3 files changed, 80 insertions(+)
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > 53d3f55..838ad4e 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -11,6 +11,13 @@ menuconfig FPGA
> >
> > if FPGA
> >
> > +config FPGA_MGR_DEBUG_FS
> > + tristate "FPGA Debug fs"
> > + select DEBUG_FS
> > + help
> > + FPGA manager debug provides support for reading fpga
> configuration
> > + information.
> > +
> > config FPGA_MGR_SOCFPGA
> > tristate "Altera SOCFPGA FPGA Manager"
> > depends on ARCH_SOCFPGA || COMPILE_TEST diff --git
> > a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> > 9939d2c..4bea860 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr) }
> > EXPORT_SYMBOL_GPL(fpga_mgr_put);
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +#include <linux/debugfs.h>
> > +
> > +static int fpga_mgr_read(struct seq_file *s, void *data) {
> > + struct fpga_manager *mgr = (struct fpga_manager *)s->private;
> > + int ret = 0;
> > +
> > + if (!mgr->mops->read)
> > + return -ENOENT;
> > +
> > + if (!mutex_trylock(&mgr->ref_mutex))
> > + return -EBUSY;
> > +
> > + if (mgr->state != FPGA_MGR_STATE_OPERATING) {
> > + ret = -EPERM;
> > + goto err_unlock;
> > + }
> > +
> > + /* Read the FPGA configuration data from the fabric */
> > + ret = mgr->mops->read(mgr, s);
> > + if (ret)
> > + dev_err(&mgr->dev, "Error while reading configuration
> > + data from FPGA\n");
> > +
> > +err_unlock:
> > + mutex_unlock(&mgr->ref_mutex);
> > +
> > + return ret;
> > +}
> > +
> > +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
> > +{
> > + return single_open(file, fpga_mgr_read, inode->i_private); }
> > +
> > +static const struct file_operations fpga_mgr_ops_image = {
> > + .owner = THIS_MODULE,
> > + .open = fpga_mgr_read_open,
> > + .read = seq_read,
> > +};
> > +#endif
> > +
> > /**
> > * fpga_mgr_lock - Lock FPGA manager for exclusive use
> > * @mgr: fpga manager
> > @@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const
> char *name,
> > if (ret)
> > goto error_device;
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > + struct dentry *d, *parent;
> > +
> > + mgr->dir = debugfs_create_dir("fpga", NULL);
> > + if (!mgr->dir)
> > + goto error_device;
> > +
> > + parent = mgr->dir;
> > + d = debugfs_create_dir(mgr->dev.kobj.name, parent);
> > + if (!d) {
> > + debugfs_remove_recursive(parent);
> > + goto error_device;
> > + }
> > +
> > + parent = d;
> > + d = debugfs_create_file("image", 0644, parent, mgr,
> > + &fpga_mgr_ops_image);
> > + if (!d) {
> > + debugfs_remove_recursive(mgr->dir);
> > + goto error_device;
> > + }
> > +#endif
> > +
> > dev_info(&mgr->dev, "%s registered\n", mgr->name);
> >
> > return 0;
> > @@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
> >
> > dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > + debugfs_remove_recursive(mgr->dir);
> > +#endif
> > /*
> > * If the low level driver provides a method for putting fpga into
> > * a desired state upon unregister, do it.
> > diff --git a/include/linux/fpga/fpga-mgr.h
> > b/include/linux/fpga/fpga-mgr.h index 3c6de23..e9e17a9 100644
> > --- a/include/linux/fpga/fpga-mgr.h
> > +++ b/include/linux/fpga/fpga-mgr.h
> > @@ -114,6 +114,7 @@ struct fpga_image_info {
> > * @write: write count bytes of configuration data to the FPGA
> > * @write_sg: write the scatter list of configuration data to the FPGA
> > * @write_complete: set FPGA to operating state after writing is done
> > + * @read: optional: read FPGA configuration information
> > * @fpga_remove: optional: Set FPGA into a specific state during driver
> remove
> > * @groups: optional attribute groups.
> > *
> > @@ -131,6 +132,7 @@ struct fpga_manager_ops {
> > int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
> > int (*write_complete)(struct fpga_manager *mgr,
> > struct fpga_image_info *info);
> > + int (*read)(struct fpga_manager *mgr, struct seq_file *s);
> > void (*fpga_remove)(struct fpga_manager *mgr);
> > const struct attribute_group **groups; }; @@ -151,6 +153,9 @@
> > struct fpga_manager {
> > enum fpga_mgr_states state;
> > const struct fpga_manager_ops *mops;
> > void *priv;
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > + struct dentry *dir;
> > +#endif
> > };
> >
> > #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fpga"
> > in the body of a message to [email protected] More
> majordomo
> > info at http://vger.kernel.org/majordomo-info.html
Hi Alan,
Did you get a chance to send your framework changes to upstream?
@Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series??
Please let me know your thoughts on this.
Regards,
Kedar.
> On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao
> <[email protected]> wrote:
>
> Hi Appana,
>
> There should be some documentation for the debugfs added under
> Documentation/driver-api/fpga/
>
> Also there are a lot of #ifdefs that were added due to the
> CONFIG_FPGA_MGR_DEBUG_FS. This has caused a kernel robot complaint.
> The way to fix that is to have a separate c file for the debugfs implementation.
> I see a lot of other kernel drivers have done it this way. I have an
> implementation that I haven't submitted yet, it exposes different functionality
> (exposing the image firmware file name and writing to the image file). It's on
> the downstream github.com/altera-opensource repo [1]. I'll clean this up and
> submit it to the mailing list, it may take a minute for me to get to that.
> Once that's done, your added functionality can be a patch on top of that.
>
> Alan
>
> [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.c
> https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.h
>
>
> > Inorder to debug issues with fpga's users would like to read the fpga
> > configuration information.
> > This patch adds readback support for fpga configuration data in the
> > framework through debugfs interface.
> >
> > Usage:
> > cat /sys/kernel/debug/fpga/fpga0/image
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > <[email protected]>
> > ---
> > Changes for v4:
> > --> None.
> > Changes for v3:
> > --> None.
> > Changes for v2:
> > --> Fixed debug attribute path and name as suggested by Alan Add
> > --> config entry for DEBUG as suggested by Alan Fixed trival coding
> > --> style issues.
> >
> > drivers/fpga/Kconfig | 7 +++++
> > drivers/fpga/fpga-mgr.c | 68
> +++++++++++++++++++++++++++++++++++++++++++
> > include/linux/fpga/fpga-mgr.h | 5 ++++
> > 3 files changed, 80 insertions(+)
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > 53d3f55..838ad4e 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -11,6 +11,13 @@ menuconfig FPGA
> >
> > if FPGA
> >
> > +config FPGA_MGR_DEBUG_FS
> > + tristate "FPGA Debug fs"
> > + select DEBUG_FS
> > + help
> > + FPGA manager debug provides support for reading fpga configuration
> > + information.
> > +
> > config FPGA_MGR_SOCFPGA
> > tristate "Altera SOCFPGA FPGA Manager"
> > depends on ARCH_SOCFPGA || COMPILE_TEST diff --git
> > a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> > 9939d2c..4bea860 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr) }
> > EXPORT_SYMBOL_GPL(fpga_mgr_put);
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +#include <linux/debugfs.h>
> > +
> > +static int fpga_mgr_read(struct seq_file *s, void *data) {
> > + struct fpga_manager *mgr = (struct fpga_manager *)s->private;
> > + int ret = 0;
> > +
> > + if (!mgr->mops->read)
> > + return -ENOENT;
> > +
> > + if (!mutex_trylock(&mgr->ref_mutex))
> > + return -EBUSY;
> > +
> > + if (mgr->state != FPGA_MGR_STATE_OPERATING) {
> > + ret = -EPERM;
> > + goto err_unlock;
> > + }
> > +
> > + /* Read the FPGA configuration data from the fabric */
> > + ret = mgr->mops->read(mgr, s);
> > + if (ret)
> > + dev_err(&mgr->dev, "Error while reading configuration
> > + data from FPGA\n");
> > +
> > +err_unlock:
> > + mutex_unlock(&mgr->ref_mutex);
> > +
> > + return ret;
> > +}
> > +
> > +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
> > +{
> > + return single_open(file, fpga_mgr_read, inode->i_private); }
> > +
> > +static const struct file_operations fpga_mgr_ops_image = {
> > + .owner = THIS_MODULE,
> > + .open = fpga_mgr_read_open,
> > + .read = seq_read,
> > +};
> > +#endif
> > +
> > /**
> > * fpga_mgr_lock - Lock FPGA manager for exclusive use
> > * @mgr: fpga manager
> > @@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const
> char *name,
> > if (ret)
> > goto error_device;
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > + struct dentry *d, *parent;
> > +
> > + mgr->dir = debugfs_create_dir("fpga", NULL);
> > + if (!mgr->dir)
> > + goto error_device;
> > +
> > + parent = mgr->dir;
> > + d = debugfs_create_dir(mgr->dev.kobj.name, parent);
> > + if (!d) {
> > + debugfs_remove_recursive(parent);
> > + goto error_device;
> > + }
> > +
> > + parent = d;
> > + d = debugfs_create_file("image", 0644, parent, mgr,
> > + &fpga_mgr_ops_image);
> > + if (!d) {
> > + debugfs_remove_recursive(mgr->dir);
> > + goto error_device;
> > + }
> > +#endif
> > +
> > dev_info(&mgr->dev, "%s registered\n", mgr->name);
> >
> > return 0;
> > @@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
> >
> > dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > + debugfs_remove_recursive(mgr->dir);
> > +#endif
> > /*
> > * If the low level driver provides a method for putting fpga into
> > * a desired state upon unregister, do it.
> > diff --git a/include/linux/fpga/fpga-mgr.h
> > b/include/linux/fpga/fpga-mgr.h index 3c6de23..e9e17a9 100644
> > --- a/include/linux/fpga/fpga-mgr.h
> > +++ b/include/linux/fpga/fpga-mgr.h
> > @@ -114,6 +114,7 @@ struct fpga_image_info {
> > * @write: write count bytes of configuration data to the FPGA
> > * @write_sg: write the scatter list of configuration data to the FPGA
> > * @write_complete: set FPGA to operating state after writing is done
> > + * @read: optional: read FPGA configuration information
> > * @fpga_remove: optional: Set FPGA into a specific state during driver
> remove
> > * @groups: optional attribute groups.
> > *
> > @@ -131,6 +132,7 @@ struct fpga_manager_ops {
> > int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
> > int (*write_complete)(struct fpga_manager *mgr,
> > struct fpga_image_info *info);
> > + int (*read)(struct fpga_manager *mgr, struct seq_file *s);
> > void (*fpga_remove)(struct fpga_manager *mgr);
> > const struct attribute_group **groups; }; @@ -151,6 +153,9 @@
> > struct fpga_manager {
> > enum fpga_mgr_states state;
> > const struct fpga_manager_ops *mops;
> > void *priv;
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > + struct dentry *dir;
> > +#endif
> > };
> >
> > #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fpga"
> > in the body of a message to [email protected] More majordomo
> > info at http://vger.kernel.org/majordomo-info.html
Hi Kedar & Moritz,
On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote:
> Hi Alan,
>
> Did you get a chance to send your framework changes to upstream?
> @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series??
> Please let me know your thoughts on this.
>
> Regards,
> Kedar.
I'd like to see some mechanism added as well. Our CvP driver needs a way
to load images to the FPGA over the PCIe bus.
It wasn't clear to me from the discussion on Alan's patchset[1] that the
debugfs was acceptable to the mainline. I'd be happy to resurrect that
patchset with the suggested changes.
Since debugfs isn't enabled for production, are there any alternatives?
Alan sent a RFC [2] for loading FIT images through the sysfs.
The RFC described a FIT image that included FPGA image specific
information to be included with the image (for systems running without
device tree like our PCIe bus FPGA CvP).
Unfortunately, I believe this has the same uphill battle that the Device
Tree Overlay patches[3] have to getting accepted.
Thor
[1] https://patchwork.kernel.org/patch/10566865/
[2] https://patchwork.kernel.org/patch/9860183/
https://patchwork.kernel.org/patch/9860193/
https://patchwork.kernel.org/patch/9860191/
https://patchwork.kernel.org/patch/9860189/
https://patchwork.kernel.org/patch/9860187/
[3] https://lore.kernel.org/patchwork/cover/511851/
>> On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao
>> <[email protected]> wrote:
>>
>> Hi Appana,
>>
>> There should be some documentation for the debugfs added under
>> Documentation/driver-api/fpga/
>>
>> Also there are a lot of #ifdefs that were added due to the
>> CONFIG_FPGA_MGR_DEBUG_FS. This has caused a kernel robot complaint.
>> The way to fix that is to have a separate c file for the debugfs implementation.
>> I see a lot of other kernel drivers have done it this way. I have an
>> implementation that I haven't submitted yet, it exposes different functionality
>> (exposing the image firmware file name and writing to the image file). It's on
>> the downstream github.com/altera-opensource repo [1]. I'll clean this up and
>> submit it to the mailing list, it may take a minute for me to get to that.
>> Once that's done, your added functionality can be a patch on top of that.
>>
>> Alan
>>
>> [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
>> 4.17/drivers/fpga/fpga-mgr-debugfs.c
>> https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
>> 4.17/drivers/fpga/fpga-mgr-debugfs.h
>>
>>
>>> Inorder to debug issues with fpga's users would like to read the fpga
>>> configuration information.
>>> This patch adds readback support for fpga configuration data in the
>>> framework through debugfs interface.
>>>
>>> Usage:
>>> cat /sys/kernel/debug/fpga/fpga0/image
>>>
>>> Signed-off-by: Appana Durga Kedareswara rao
>>> <[email protected]>
>>> ---
>>> Changes for v4:
>>> --> None.
>>> Changes for v3:
>>> --> None.
>>> Changes for v2:
>>> --> Fixed debug attribute path and name as suggested by Alan Add
>>> --> config entry for DEBUG as suggested by Alan Fixed trival coding
>>> --> style issues.
>>>
>>> drivers/fpga/Kconfig | 7 +++++
>>> drivers/fpga/fpga-mgr.c | 68
>> +++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/fpga/fpga-mgr.h | 5 ++++
>>> 3 files changed, 80 insertions(+)
>>>
>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
>>> 53d3f55..838ad4e 100644
>>> --- a/drivers/fpga/Kconfig
>>> +++ b/drivers/fpga/Kconfig
>>> @@ -11,6 +11,13 @@ menuconfig FPGA
>>>
>>> if FPGA
>>>
>>> +config FPGA_MGR_DEBUG_FS
>>> + tristate "FPGA Debug fs"
>>> + select DEBUG_FS
>>> + help
>>> + FPGA manager debug provides support for reading fpga configuration
>>> + information.
>>> +
>>> config FPGA_MGR_SOCFPGA
>>> tristate "Altera SOCFPGA FPGA Manager"
>>> depends on ARCH_SOCFPGA || COMPILE_TEST diff --git
>>> a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
>>> 9939d2c..4bea860 100644
>>> --- a/drivers/fpga/fpga-mgr.c
>>> +++ b/drivers/fpga/fpga-mgr.c
>>> @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr) }
>>> EXPORT_SYMBOL_GPL(fpga_mgr_put);
>>>
>>> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
>>> +#include <linux/debugfs.h>
>>> +
>>> +static int fpga_mgr_read(struct seq_file *s, void *data) {
>>> + struct fpga_manager *mgr = (struct fpga_manager *)s->private;
>>> + int ret = 0;
>>> +
>>> + if (!mgr->mops->read)
>>> + return -ENOENT;
>>> +
>>> + if (!mutex_trylock(&mgr->ref_mutex))
>>> + return -EBUSY;
>>> +
>>> + if (mgr->state != FPGA_MGR_STATE_OPERATING) {
>>> + ret = -EPERM;
>>> + goto err_unlock;
>>> + }
>>> +
>>> + /* Read the FPGA configuration data from the fabric */
>>> + ret = mgr->mops->read(mgr, s);
>>> + if (ret)
>>> + dev_err(&mgr->dev, "Error while reading configuration
>>> + data from FPGA\n");
>>> +
>>> +err_unlock:
>>> + mutex_unlock(&mgr->ref_mutex);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
>>> +{
>>> + return single_open(file, fpga_mgr_read, inode->i_private); }
>>> +
>>> +static const struct file_operations fpga_mgr_ops_image = {
>>> + .owner = THIS_MODULE,
>>> + .open = fpga_mgr_read_open,
>>> + .read = seq_read,
>>> +};
>>> +#endif
>>> +
>>> /**
>>> * fpga_mgr_lock - Lock FPGA manager for exclusive use
>>> * @mgr: fpga manager
>>> @@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const
>> char *name,
>>> if (ret)
>>> goto error_device;
>>>
>>> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
>>> + struct dentry *d, *parent;
>>> +
>>> + mgr->dir = debugfs_create_dir("fpga", NULL);
>>> + if (!mgr->dir)
>>> + goto error_device;
>>> +
>>> + parent = mgr->dir;
>>> + d = debugfs_create_dir(mgr->dev.kobj.name, parent);
>>> + if (!d) {
>>> + debugfs_remove_recursive(parent);
>>> + goto error_device;
>>> + }
>>> +
>>> + parent = d;
>>> + d = debugfs_create_file("image", 0644, parent, mgr,
>>> + &fpga_mgr_ops_image);
>>> + if (!d) {
>>> + debugfs_remove_recursive(mgr->dir);
>>> + goto error_device;
>>> + }
>>> +#endif
>>> +
>>> dev_info(&mgr->dev, "%s registered\n", mgr->name);
>>>
>>> return 0;
>>> @@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
>>>
>>> dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>>>
>>> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
>>> + debugfs_remove_recursive(mgr->dir);
>>> +#endif
>>> /*
>>> * If the low level driver provides a method for putting fpga into
>>> * a desired state upon unregister, do it.
>>> diff --git a/include/linux/fpga/fpga-mgr.h
>>> b/include/linux/fpga/fpga-mgr.h index 3c6de23..e9e17a9 100644
>>> --- a/include/linux/fpga/fpga-mgr.h
>>> +++ b/include/linux/fpga/fpga-mgr.h
>>> @@ -114,6 +114,7 @@ struct fpga_image_info {
>>> * @write: write count bytes of configuration data to the FPGA
>>> * @write_sg: write the scatter list of configuration data to the FPGA
>>> * @write_complete: set FPGA to operating state after writing is done
>>> + * @read: optional: read FPGA configuration information
>>> * @fpga_remove: optional: Set FPGA into a specific state during driver
>> remove
>>> * @groups: optional attribute groups.
>>> *
>>> @@ -131,6 +132,7 @@ struct fpga_manager_ops {
>>> int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
>>> int (*write_complete)(struct fpga_manager *mgr,
>>> struct fpga_image_info *info);
>>> + int (*read)(struct fpga_manager *mgr, struct seq_file *s);
>>> void (*fpga_remove)(struct fpga_manager *mgr);
>>> const struct attribute_group **groups; }; @@ -151,6 +153,9 @@
>>> struct fpga_manager {
>>> enum fpga_mgr_states state;
>>> const struct fpga_manager_ops *mops;
>>> void *priv;
>>> +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
>>> + struct dentry *dir;
>>> +#endif
>>> };
>>>
>>> #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
>>> --
>>> 2.7.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-fpga"
>>> in the body of a message to [email protected] More majordomo
>>> info at http://vger.kernel.org/majordomo-info.html
Thor,
On Fri, Sep 27, 2019 at 09:32:11AM -0500, Thor Thayer wrote:
> Hi Kedar & Moritz,
>
> On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote:
> > Hi Alan,
> >
> > Did you get a chance to send your framework changes to upstream?
No they weren't upstreamed.
> > @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series??
> > Please let me know your thoughts on this.
Alan had some comments RE: #defines, I'll have to take another look.
> >
> > Regards,
> > Kedar.
>
>
> I'd like to see some mechanism added as well. Our CvP driver needs a way to
> load images to the FPGA over the PCIe bus.
Can you elaborate a bit on the CvP use-case and how that would work? Who
would use the device how after loading the bitstream?
Generally there are several use cases that I have collected mentally
over the years:
I) DFL use case:
- Mixed-set of drivers: Kernel and Userspace
- FPGA logic is discoverable through DFL
- Userspace application wants to reprogram FPGA
II) DT configfs use case:
- Mixed-set of drivers: Kernel and Userspace
- FPGA logic is *not* discoverable (hence DT overlay)
- Userspace application wants to reprogram FPGA
III) Thomas' case:
- Kernel only drivers (pcie bridge, pcie drivers, ...)
- FPGA logic is fully discoverable (i.e. PCIe endpoint
implemented in FPGA, connected to SoC via PCIe)
- Userspace application wants to reprogram FPGA
IV) VFIO case:
- Usually exposes either entire device via vfio-pci or part via
vfio-mdev
- Loading (basic) bitstream at boot from flash
- vfio-mdev case can use FPGA region interface + ioctl
- Full VFIO case is similar to III)
How does your CvP use case fit in? Collecting all the use-cases would
help with moving forward on coming up with an API :)
>
> It wasn't clear to me from the discussion on Alan's patchset[1] that the
> debugfs was acceptable to the mainline. I'd be happy to resurrect that
> patchset with the suggested changes.
Back then we decided to not move forward with the debugfs patchset since
it's essentially cat foo.bin > /dev/xdevcfg / cat bar.rbf > /dev/fpga0
in disguise. Which is why I vetoed it back then.
> Since debugfs isn't enabled for production, are there any alternatives?
>
> Alan sent a RFC [2] for loading FIT images through the sysfs.
>
> The RFC described a FIT image that included FPGA image specific information
> to be included with the image (for systems running without device tree like
> our PCIe bus FPGA CvP).
Yeah I had originally suggested that as a mechanim to make FPGA images
discoverable by the kernel. I still think the idea has merit, however it
will run into the same issues that the configfs interface ran into w.r.t
using dt-overlays.
Generally I'd like to see a solution that exposes the *same* interface
to DT and not DT systems to userspace.
Using FIT headers one could go ahead and design something along the
lines of what DFL is doing, except for instead of parsing the DFL in the
logic, one would parse the FIT header to create subdevices.
> Unfortunately, I believe this has the same uphill battle that the Device
> Tree Overlay patches[3] have to getting accepted.
See above. While I'm happy to discuss this more I atm don't have the
bandwidth to push the DT work any further.
Thanks,
Moritz
Hi Moritz,
On 9/27/19 1:23 PM, Moritz Fischer wrote:
> Thor,
>
> On Fri, Sep 27, 2019 at 09:32:11AM -0500, Thor Thayer wrote:
>> Hi Kedar & Moritz,
>>
>> On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote:
>>> Hi Alan,
>>>
>>> Did you get a chance to send your framework changes to upstream?
> No they weren't upstreamed.
>
>>> @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series??
>>> Please let me know your thoughts on this.
>
> Alan had some comments RE: #defines, I'll have to take another look.
>>>
>>> Regards,
>>> Kedar.
>>
>>
>> I'd like to see some mechanism added as well. Our CvP driver needs a way to
>> load images to the FPGA over the PCIe bus.
>
> Can you elaborate a bit on the CvP use-case and how that would work? Who
> would use the device how after loading the bitstream?
>
> Generally there are several use cases that I have collected mentally
> over the years:
>
> I) DFL use case:
> - Mixed-set of drivers: Kernel and Userspace
> - FPGA logic is discoverable through DFL
> - Userspace application wants to reprogram FPGA
>
> II) DT configfs use case:
> - Mixed-set of drivers: Kernel and Userspace
> - FPGA logic is *not* discoverable (hence DT overlay)
> - Userspace application wants to reprogram FPGA
>
> III) Thomas' case:
> - Kernel only drivers (pcie bridge, pcie drivers, ...)
> - FPGA logic is fully discoverable (i.e. PCIe endpoint
> implemented in FPGA, connected to SoC via PCIe)
> - Userspace application wants to reprogram FPGA
>
> IV) VFIO case:
> - Usually exposes either entire device via vfio-pci or part via
> vfio-mdev
> - Loading (basic) bitstream at boot from flash
> - vfio-mdev case can use FPGA region interface + ioctl
> - Full VFIO case is similar to III)
>
> How does your CvP use case fit in? Collecting all the use-cases would
> help with moving forward on coming up with an API :)
>
The CvP case is the same as III) Thomas' case. The FPGA configuration
bitstream is downloaded over the PCIe.
The one difference in my case is that there isn't an SoC. This is a
Intel host processor connecting to a non-SoC Stratix10/Arria10. The
non-SoC A10/S10, boots a minimal image (CvP) setting up the peripheral
pins and enabling the PCIe endpoint for CvP downloads.
The host can then download bitstreams using the FPGA Manager through
debugFS and when the bitstream finishes downloading and the FPGA enters
User Mode, the functionality is available for the host to use.
>>
>> It wasn't clear to me from the discussion on Alan's patchset[1] that the
>> debugfs was acceptable to the mainline. I'd be happy to resurrect that
>> patchset with the suggested changes.
>
> Back then we decided to not move forward with the debugfs patchset since
> it's essentially cat foo.bin > /dev/xdevcfg / cat bar.rbf > /dev/fpga0
> in disguise. Which is why I vetoed it back then.
>
>> Since debugfs isn't enabled for production, are there any alternatives?
>>
>> Alan sent a RFC [2] for loading FIT images through the sysfs.
>>
>> The RFC described a FIT image that included FPGA image specific information
>> to be included with the image (for systems running without device tree like
>> our PCIe bus FPGA CvP).
>
> Yeah I had originally suggested that as a mechanim to make FPGA images
> discoverable by the kernel. I still think the idea has merit, however it
> will run into the same issues that the configfs interface ran into w.r.t
> using dt-overlays.
>
> Generally I'd like to see a solution that exposes the *same* interface
> to DT and not DT systems to userspace.
>
> Using FIT headers one could go ahead and design something along the
> lines of what DFL is doing, except for instead of parsing the DFL in the
> logic, one would parse the FIT header to create subdevices.
>
>> Unfortunately, I believe this has the same uphill battle that the Device
>> Tree Overlay patches[3] have to getting accepted.
>
> See above. While I'm happy to discuss this more I atm don't have the
> bandwidth to push the DT work any further.
Understood. I'm still coming up to speed on the variations of FPGA
enablement but I'm happy to help where I can.
Thanks.
>
> Thanks,
> Moritz
>
Hi Thor,
On Mon, Oct 07, 2019 at 01:06:51PM -0500, Thor Thayer wrote:
> Hi Moritz,
>
> On 9/27/19 1:23 PM, Moritz Fischer wrote:
> > Thor,
> >
> > On Fri, Sep 27, 2019 at 09:32:11AM -0500, Thor Thayer wrote:
> > > Hi Kedar & Moritz,
> > >
> > > On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote:
> > > > Hi Alan,
> > > >
> > > > Did you get a chance to send your framework changes to upstream?
> > No they weren't upstreamed.
> >
> > > > @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series??
> > > > Please let me know your thoughts on this.
> >
> > Alan had some comments RE: #defines, I'll have to take another look.
> > > >
> > > > Regards,
> > > > Kedar.
> > >
> > >
> > > I'd like to see some mechanism added as well. Our CvP driver needs a way to
> > > load images to the FPGA over the PCIe bus.
> >
> > Can you elaborate a bit on the CvP use-case and how that would work? Who
> > would use the device how after loading the bitstream?
> >
> > Generally there are several use cases that I have collected mentally
> > over the years:
> >
> > I) DFL use case:
> > - Mixed-set of drivers: Kernel and Userspace
> > - FPGA logic is discoverable through DFL
> > - Userspace application wants to reprogram FPGA
> >
> > II) DT configfs use case:
> > - Mixed-set of drivers: Kernel and Userspace
> > - FPGA logic is *not* discoverable (hence DT overlay)
> > - Userspace application wants to reprogram FPGA
> >
> > III) Thomas' case:
> > - Kernel only drivers (pcie bridge, pcie drivers, ...)
> > - FPGA logic is fully discoverable (i.e. PCIe endpoint
> > implemented in FPGA, connected to SoC via PCIe)
> > - Userspace application wants to reprogram FPGA
> >
> > IV) VFIO case:
> > - Usually exposes either entire device via vfio-pci or part via
> > vfio-mdev
> > - Loading (basic) bitstream at boot from flash
> > - vfio-mdev case can use FPGA region interface + ioctl
> > - Full VFIO case is similar to III)
> >
> > How does your CvP use case fit in? Collecting all the use-cases would
> > help with moving forward on coming up with an API :)
> >
> The CvP case is the same as III) Thomas' case. The FPGA configuration
> bitstream is downloaded over the PCIe.
>
> The one difference in my case is that there isn't an SoC. This is a Intel
> host processor connecting to a non-SoC Stratix10/Arria10. The non-SoC
> A10/S10, boots a minimal image (CvP) setting up the peripheral pins and
> enabling the PCIe endpoint for CvP downloads.
>
> The host can then download bitstreams using the FPGA Manager through debugFS
> and when the bitstream finishes downloading and the FPGA enters User Mode,
> the functionality is available for the host to use.
I am generally confused by this driver. How does it work exactly? What
happens after altera-cvp binds a PCI device?
You can use it to download a bitstream (say we had the debugfs
interface), and then what happens next? How do I use the device? It
already has a PCI driver bound to it at that point?
What happens next?
Please tell me that not the only use-case for this is /dev/mem :)
Thomas' use-case is different in that behind the FPGA device there are
actual other *discoverable* PCI devices that will get enumerated and
bind to separate drivers.
Thanks,
Moritz
PS: I'll be out this week on vacation starting tmr so responses might be delayed
Hi Moritz,
On 10/7/19 4:20 PM, Moritz Fischer wrote:
> Hi Thor,
>
> On Mon, Oct 07, 2019 at 01:06:51PM -0500, Thor Thayer wrote:
>> Hi Moritz,
>>
>> On 9/27/19 1:23 PM, Moritz Fischer wrote:
>>> Thor,
>>>
>>> On Fri, Sep 27, 2019 at 09:32:11AM -0500, Thor Thayer wrote:
>>>> Hi Kedar & Moritz,
>>>>
>>>> On 9/27/19 12:13 AM, Appana Durga Kedareswara Rao wrote:
>>>>> Hi Alan,
>>>>>
>>>>> Did you get a chance to send your framework changes to upstream?
>>> No they weren't upstreamed.
>>>
>>>>> @Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch series??
>>>>> Please let me know your thoughts on this.
>>>
>>> Alan had some comments RE: #defines, I'll have to take another look.
>>>>>
>>>>> Regards,
>>>>> Kedar.
>>>>
>>>>
>>>> I'd like to see some mechanism added as well. Our CvP driver needs a way to
>>>> load images to the FPGA over the PCIe bus.
>>>
>>> Can you elaborate a bit on the CvP use-case and how that would work? Who
>>> would use the device how after loading the bitstream?
>>>
>>> Generally there are several use cases that I have collected mentally
>>> over the years:
>>>
>>> I) DFL use case:
>>> - Mixed-set of drivers: Kernel and Userspace
>>> - FPGA logic is discoverable through DFL
>>> - Userspace application wants to reprogram FPGA
>>>
>>> II) DT configfs use case:
>>> - Mixed-set of drivers: Kernel and Userspace
>>> - FPGA logic is *not* discoverable (hence DT overlay)
>>> - Userspace application wants to reprogram FPGA
>>>
>>> III) Thomas' case:
>>> - Kernel only drivers (pcie bridge, pcie drivers, ...)
>>> - FPGA logic is fully discoverable (i.e. PCIe endpoint
>>> implemented in FPGA, connected to SoC via PCIe)
>>> - Userspace application wants to reprogram FPGA
>>>
>>> IV) VFIO case:
>>> - Usually exposes either entire device via vfio-pci or part via
>>> vfio-mdev
>>> - Loading (basic) bitstream at boot from flash
>>> - vfio-mdev case can use FPGA region interface + ioctl
>>> - Full VFIO case is similar to III)
>>>
>>> How does your CvP use case fit in? Collecting all the use-cases would
>>> help with moving forward on coming up with an API :)
>>>
>> The CvP case is the same as III) Thomas' case. The FPGA configuration
>> bitstream is downloaded over the PCIe.
>>
>> The one difference in my case is that there isn't an SoC. This is a Intel
>> host processor connecting to a non-SoC Stratix10/Arria10. The non-SoC
>> A10/S10, boots a minimal image (CvP) setting up the peripheral pins and
>> enabling the PCIe endpoint for CvP downloads.
>>
>> The host can then download bitstreams using the FPGA Manager through debugFS
>> and when the bitstream finishes downloading and the FPGA enters User Mode,
>> the functionality is available for the host to use.
>
> I am generally confused by this driver. How does it work exactly? What
> happens after altera-cvp binds a PCI device?
>
> You can use it to download a bitstream (say we had the debugfs
> interface), and then what happens next? How do I use the device? It
> already has a PCI driver bound to it at that point?
Sorry for the delay. In the CvP case, I reboot the host device leaving
the FPGA board powered so the new PCI interface is enumerated.
There may be a better way. I'll need to research Thomas' use case. There
may be some good lessons to learn there.
Thanks,
Thor
>
> What happens next?
>
> Please tell me that not the only use-case for this is /dev/mem :)
>
> Thomas' use-case is different in that behind the FPGA device there are
> actual other *discoverable* PCI devices that will get enumerated and
> bind to separate drivers.
>
> Thanks,
> Moritz
>
> PS: I'll be out this week on vacation starting tmr so responses might be delayed
>