[PATCH 0/3] Adding Xilinx Zynq 7000 FPGA Manager driver

[PATCH 0/3] Adding Xilinx Zynq 7000 FPGA Manager driver

[Moritz Fischer]

Hi all,

this patchset adds support for the Xilinx Zynq 7000 FPGA Manager,
based on Alan's FPGA manager framework.

The patchset has lived for a while already in the Xilinx tree,
however, I don't know if anyone besides me tested it so far?

The version in their tree got ACKs by Michal and Soeren already,
but was based against an older version of Alan's patchset.

Michal, Soeren, could I get ACKs from you guys for this version?

Cheers,

    Moritz

Moritz Fischer (3):
  doc: dt: fpga: Added Documentation for Xilinx Zynq FPGA manager.
  dts: Updated devicetree bindings for Zynq 7000 platform
  fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

 .../bindings/fpga/xilinx-zynq-fpga-mgr.txt         |  26 ++
 arch/arm/boot/dts/zynq-7000.dtsi                   |   5 +
 drivers/fpga/Kconfig                               |   5 +
 drivers/fpga/Makefile                              |   1 +
 drivers/fpga/zynq-fpga.c                           | 478 +++++++++++++++++++++
 5 files changed, 515 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
 create mode 100644 drivers/fpga/zynq-fpga.c

-- 
2.6.1

[PATCH 1/3] doc: dt: fpga: Added Documentation for Xilinx Zynq FPGA manager.

[Moritz Fischer]

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 .../bindings/fpga/xilinx-zynq-fpga-mgr.txt         | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt

diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
new file mode 100644
index 0000000..82ffda8
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
@@ -0,0 +1,26 @@
+Xilinx Zynq FPGA Manager
+
+Required properties:
+- compatible:		should contain "xlnx,zynq-devcfg-1.0"
+- reg:			base address and size for memory mapped io
+- interrupt parent:	interrupt source phandle
+- interrupts:		interrupt for the FPGA manager device
+- clocks:		phandle for clocks required operation
+- syscon:		phandle for access to SLCR registers
+
+Optional properties:
+- clock-names:		names for clocks
+
+
+
+Example:
+	devcfg: devcfg@f8007000 {
+		compatible = "xlnx,zynq-devcfg-1.0";
+		reg = <0xf8007000 0x100>;
+		interrupt-parent = <&intc>;
+		interrupts = <0 8 4>;
+		clocks = <&clkc 12>;
+		clock-names = "ref_clk";
+		syscon = <&slcr>;
+	};
+
-- 
2.6.1

[PATCH 2/3] dts: Updated devicetree bindings for Zynq 7000 platform

[Moritz Fischer]

Added addtional bindings required for FPGA Manager operation
of the Xilinx Zynq Devc configuration interface.

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 arch/arm/boot/dts/zynq-7000.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index dc0457e..1a5220e 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -294,6 +294,11 @@
 		devcfg: devcfg@f8007000 {
 			compatible = "xlnx,zynq-devcfg-1.0";
 			reg = <0xf8007000 0x100>;
+			interrupt-parent = <&intc>;
+			interrupts = <0 8 4>;
+			clocks = <&clkc 12>;
+			clock-names = "ref_clk";
+			syscon = <&slcr>;
 		};
 
 		global_timer: timer@f8f00200 {
-- 
2.6.1

[PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Moritz Fischer]

This commit adds FPGA Manager support for the Xilinx Zynq chip.
The code heavily borrows from the xdevcfg driver in Xilinx'
vendor tree.

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 drivers/fpga/Kconfig     |   5 +
 drivers/fpga/Makefile    |   1 +
 drivers/fpga/zynq-fpga.c | 478 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 484 insertions(+)
 create mode 100644 drivers/fpga/zynq-fpga.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index dfc1f1e..c9b9fdf 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -19,6 +19,11 @@ config FPGA_MGR_SOCFPGA
 	help
 	  FPGA manager driver support for Altera SOCFPGA.
 
+config FPGA_MGR_ZYNQ_FPGA
+	tristate "Xilinx Zynq FPGA"
+	help
+	  FPGA manager driver support for Xilinx Zynq FPGAs.
+
 endif # FPGA
 
 endmenu
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index ba6c5c5..8d83fc6 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_FPGA)			+= fpga-mgr.o
 
 # FPGA Manager Drivers
 obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
+obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
new file mode 100644
index 0000000..7d0d703
--- /dev/null
+++ b/drivers/fpga/zynq-fpga.c
@@ -0,0 +1,478 @@
+/*
+ * Copyright (c) 2011-2015 Xilinx Inc.
+ * Copyright (c) 2015, National Instruments Corp.
+ *
+ * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
+ * in their vendor tree.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/interrupt.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+
+/* Offsets into SLCR regmap */
+#define SLCR_FPGA_RST_CTRL_OFFSET	0x240 /* FPGA Software Reset Control */
+#define SLCR_LVL_SHFTR_EN_OFFSET	0x900 /* Level Shifters Enable */
+
+/* Constant Definitions */
+#define CTRL_OFFSET		0x00 /* Control Register */
+#define LOCK_OFFSET		0x04 /* Lock Register */
+#define INT_STS_OFFSET		0x0c /* Interrupt Status Register */
+#define INT_MASK_OFFSET		0x10 /* Interrupt Mask Register */
+#define STATUS_OFFSET		0x14 /* Status Register */
+#define DMA_SRC_ADDR_OFFSET	0x18 /* DMA Source Address Register */
+#define DMA_DEST_ADDR_OFFSET	0x1c /* DMA Destination Address Reg */
+#define DMA_SRC_LEN_OFFSET	0x20 /* DMA Source Transfer Length */
+#define DMA_DEST_LEN_OFFSET	0x24 /* DMA Destination Transfer */
+#define UNLOCK_OFFSET		0x34 /* Unlock Register */
+#define MCTRL_OFFSET		0x80 /* Misc. Control Register */
+
+/* Control Register Bit definitions */
+#define CTRL_PCFG_PROG_B_MASK	BIT(30) /* Program signal to reset FPGA */
+#define CTRL_PCAP_PR_MASK	BIT(27) /* Enable PCAP for PR */
+#define CTRL_PCAP_MODE_MASK	BIT(26) /* Enable PCAP */
+
+/* Miscellaneous Control Register bit definitions */
+#define MCTRL_PCAP_LPBK_MASK	BIT(4) /* Internal PCAP loopback */
+
+/* Status register bit definitions */
+#define STATUS_PCFG_INIT_MASK	BIT(4) /* FPGA init status */
+
+/* Interrupt Status/Mask Register Bit definitions */
+#define IXR_DMA_DONE_MASK	BIT(13) /* DMA command done */
+#define IXR_D_P_DONE_MASK	BIT(12) /* DMA and PCAP cmd done */
+#define IXR_PCFG_DONE_MASK	BIT(2)  /* FPGA programmed */
+#define IXR_ERROR_FLAGS_MASK	0x00F0F860
+#define IXR_ALL_MASK		0xF8F7F87F
+
+/* Miscellaneous constant values */
+#define DMA_INVALID_ADDRESS	GENMASK(31, 0)  /* Invalid DMA address */
+#define UNLOCK_MASK		0x757bdf0d /* Used to unlock the device */
+
+/* Masks for controlling stuff in SLCR */
+#define LVL_SHFTR_DISABLE_ALL_MASK	0x0 /* Disable all Level shifters */
+#define LVL_SHFTR_ENABLE_PS_TO_PL	0xa /* Enable all Level shifters */
+#define LVL_SHFTR_ENABLE_PL_TO_PS	0xf /* Enable all Level shifters */
+#define FPGA_RST_ALL_MASK		0xf /* Enable global resets */
+#define FPGA_RST_NONE_MASK		0x0 /* Disable global resets */
+
+struct zynq_fpga_priv {
+	struct device *dev;
+	int irq;
+	struct clk *clk;
+
+	void __iomem *io_base;
+	struct regmap *slcr;
+
+	/* this protects the error flag */
+	spinlock_t lock;
+	bool error;
+
+	struct completion dma_done;
+};
+
+static inline void zynq_fpga_write(struct zynq_fpga_priv *priv, u32 offset,
+				   u32 val)
+{
+	writel(val, priv->io_base + offset);
+}
+
+static inline u32 zynq_fpga_read(const struct zynq_fpga_priv *priv,
+				 u32 offset)
+{
+	return readl(priv->io_base + offset);
+}
+
+#define zynq_fpga_poll_timeout(priv, addr, val, cond, sleep_us, timeout_us) \
+	readl_poll_timeout(priv->io_base + addr, val, cond, sleep_us, \
+			   timeout_us)
+
+static void zynq_fpga_mask_irqs(struct zynq_fpga_priv *priv)
+{
+	u32 intr_mask;
+
+	intr_mask = zynq_fpga_read(priv, INT_MASK_OFFSET);
+	zynq_fpga_write(priv, INT_MASK_OFFSET,
+			intr_mask | IXR_DMA_DONE_MASK | IXR_ERROR_FLAGS_MASK);
+}
+
+static void zynq_fpga_unmask_irqs(struct zynq_fpga_priv *priv)
+{
+	u32 intr_mask;
+
+	intr_mask = zynq_fpga_read(priv, INT_MASK_OFFSET);
+	zynq_fpga_write(priv, INT_MASK_OFFSET,
+			intr_mask
+			& ~(IXR_D_P_DONE_MASK | IXR_ERROR_FLAGS_MASK));
+}
+
+static irqreturn_t zynq_fpga_isr(int irq, void *data)
+{
+	u32 intr_status;
+	struct zynq_fpga_priv *priv = data;
+
+	spin_lock(&priv->lock);
+	intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
+
+	if (!intr_status) {
+		spin_unlock(&priv->lock);
+		return IRQ_NONE;
+	}
+
+	zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
+
+	if ((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)
+		complete(&priv->dma_done);
+
+	if ((intr_status & IXR_ERROR_FLAGS_MASK) ==
+			IXR_ERROR_FLAGS_MASK) {
+		priv->error = true;
+		dev_err(priv->dev, "%s dma error\n", __func__);
+	}
+	spin_unlock(&priv->lock);
+
+	return IRQ_HANDLED;
+}
+
+static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
+				    const char *buf, size_t count)
+{
+	struct zynq_fpga_priv *priv;
+	u32 ctrl, status;
+	int err;
+
+	priv = mgr->priv;
+
+	err = clk_enable(priv->clk);
+	if (err)
+		return err;
+
+	/* only reset if we're not doing partial reconfig */
+	if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
+		/* assert AXI interface resets */
+		regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
+			     FPGA_RST_ALL_MASK);
+
+		/* disable level shifters */
+		regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
+			     LVL_SHFTR_DISABLE_ALL_MASK);
+		/* enable output level shifters */
+		regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
+			     LVL_SHFTR_ENABLE_PS_TO_PL);
+
+		/* create a rising edge on PCFG_INIT. PCFG_INIT follows
+		 * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
+		 * to make sure the rising edge actually happens
+		 */
+		ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
+		ctrl |= CTRL_PCFG_PROG_B_MASK;
+
+		zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
+
+		zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
+				       STATUS_PCFG_INIT_MASK, 20, 0);
+
+		ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
+		ctrl &= ~CTRL_PCFG_PROG_B_MASK;
+
+		zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
+
+		zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, !(status &
+				       STATUS_PCFG_INIT_MASK), 20, 0);
+
+		ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
+		ctrl |= CTRL_PCFG_PROG_B_MASK;
+
+		zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
+
+		zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
+				       STATUS_PCFG_INIT_MASK, 20, 0);
+	}
+
+	clk_disable(priv->clk);
+
+	return 0;
+}
+
+static int zynq_fpga_ops_write(struct fpga_manager *mgr,
+			       const char *buf, size_t count)
+{
+	struct zynq_fpga_priv *priv;
+	int err;
+	char *kbuf;
+	size_t i, in_count;
+	dma_addr_t dma_addr;
+	u32 transfer_length = 0;
+	bool endian_swap = false;
+
+	in_count = count;
+	priv = mgr->priv;
+
+	kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+
+	memcpy(kbuf, buf, count);
+
+	/* look for the sync word */
+	for (i = 0; i < count - 4; i++) {
+		if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
+			dev_dbg(priv->dev, "Found normal sync word\n");
+			endian_swap = false;
+			break;
+		}
+		if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
+			dev_dbg(priv->dev, "Found swapped sync word\n");
+			endian_swap = true;
+			break;
+		}
+	}
+
+	/* remove the header, align the data on word boundary */
+	if (i != count - 4) {
+		count -= i;
+		memmove(kbuf, kbuf + i, count);
+	}
+
+	/* fixup endianness of the data */
+	if (endian_swap) {
+		for (i = 0; i < count; i += 4) {
+			u32 *p = (u32 *)&kbuf[i];
+			*p = swab32(*p);
+		}
+	}
+
+	/* enable clock */
+	err = clk_enable(priv->clk);
+	if (err)
+		goto out_free;
+
+	zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
+
+	/* enable DMA and error IRQs */
+	zynq_fpga_unmask_irqs(priv);
+
+	priv->error = false;
+
+	/* the +1 in the src addr is used to hold off on DMA_DONE IRQ */
+	/* until both AXI and PCAP are done */
+	if (count < PAGE_SIZE)
+		zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, (u32)(dma_addr + 1));
+	else
+		zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, (u32)(dma_addr));
+
+	zynq_fpga_write(priv, DMA_DEST_ADDR_OFFSET, (u32)DMA_INVALID_ADDRESS);
+
+	/* convert #bytes to #words */
+	transfer_length = (count + 3) / 4;
+
+	zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, transfer_length);
+	zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, 0);
+
+	wait_for_completion_interruptible(&priv->dma_done);
+	if (priv->error) {
+		dev_err(priv->dev, "Error configuring FPGA.\n");
+		err = -EFAULT;
+	}
+
+	/* disable DMA and error IRQs */
+	zynq_fpga_mask_irqs(priv);
+
+	clk_disable(priv->clk);
+
+out_free:
+	dma_free_coherent(priv->dev, in_count, kbuf, dma_addr);
+
+	return err;
+}
+
+static int zynq_fpga_ops_write_complete(struct fpga_manager *mgr, u32 flags)
+{
+	struct zynq_fpga_priv *priv = mgr->priv;
+	int err;
+	u32 intr_status;
+
+	err = clk_enable(priv->clk);
+	if (err)
+		return err;
+
+	err = zynq_fpga_poll_timeout(priv, INT_STS_OFFSET, intr_status,
+				     intr_status & IXR_PCFG_DONE_MASK, 20,
+				     10000);
+	clk_disable(priv->clk);
+
+	if (err)
+		return err;
+
+	/* only in not partial reconfiguration do we need to deassert reset */
+	if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
+		/* enable all level shifters */
+		regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
+			     LVL_SHFTR_ENABLE_PL_TO_PS);
+
+		/* deassert AXI interface resets */
+		regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
+			     FPGA_RST_NONE_MASK);
+	}
+
+	return 0;
+}
+
+static enum fpga_mgr_states zynq_fpga_ops_state(struct fpga_manager *mgr)
+{
+	int err;
+	u32 intr_status;
+	struct zynq_fpga_priv *priv;
+
+	priv = mgr->priv;
+
+	err = clk_enable(priv->clk);
+	if (err)
+		return FPGA_MGR_STATE_UNKNOWN;
+
+	intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
+	clk_disable(priv->clk);
+
+	if (intr_status & IXR_PCFG_DONE_MASK)
+		return FPGA_MGR_STATE_OPERATING;
+
+	return FPGA_MGR_STATE_UNKNOWN;
+}
+
+static const struct fpga_manager_ops zynq_fpga_ops = {
+	.state = zynq_fpga_ops_state,
+	.write_init = zynq_fpga_ops_write_init,
+	.write = zynq_fpga_ops_write,
+	.write_complete = zynq_fpga_ops_write_complete,
+};
+
+static int zynq_fpga_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct zynq_fpga_priv *priv;
+	struct resource *res;
+	u32 ctrl_reg;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+	priv->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->io_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->io_base))
+		return PTR_ERR(priv->io_base);
+
+	priv->slcr = syscon_regmap_lookup_by_phandle(dev->of_node,
+		"syscon");
+	if (IS_ERR(priv->slcr)) {
+		dev_err(dev, "unable to get zynq-slcr regmap");
+		return PTR_ERR(priv->slcr);
+	}
+
+	init_completion(&priv->dma_done);
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0) {
+		dev_err(dev, "No IRQ available");
+		return priv->irq;
+	}
+
+	ret = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0,
+			       dev_name(dev), priv);
+	if (IS_ERR_VALUE(ret))
+		return ret;
+
+	priv->clk = devm_clk_get(dev, "ref_clk");
+	if (IS_ERR(priv->clk)) {
+		dev_err(dev, "input clock not found\n");
+		return PTR_ERR(priv->clk);
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret) {
+		dev_err(dev, "unable to enable clock\n");
+		return ret;
+	}
+
+	/* unlock the device */
+	zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
+
+	/* set configuration register with following options:
+	* - reset FPGA
+	* - enable PCAP interface for partial reconfig
+	* - set throughput for maximum speed
+	* - set CPU in user mode
+	*/
+	ctrl_reg = zynq_fpga_read(priv, CTRL_OFFSET);
+	zynq_fpga_write(priv, CTRL_OFFSET, (CTRL_PCFG_PROG_B_MASK |
+		CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK | ctrl_reg));
+
+	/* ensure internal PCAP loopback is disabled */
+	ctrl_reg = zynq_fpga_read(priv, MCTRL_OFFSET);
+	zynq_fpga_write(priv, MCTRL_OFFSET, (~MCTRL_PCAP_LPBK_MASK & ctrl_reg));
+
+	ret = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
+				&zynq_fpga_ops, priv);
+	if (ret) {
+		dev_err(dev, "unable to register FPGA manager");
+		clk_disable_unprepare(priv->clk);
+		return ret;
+	}
+	return 0;
+}
+
+static int zynq_fpga_remove(struct platform_device *pdev)
+{
+	fpga_mgr_unregister(&pdev->dev);
+
+	return 0;
+}
+
+static const struct of_device_id zynq_fpga_of_match[] = {
+	{ .compatible = "xlnx,zynq-devcfg-1.0", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, zynq_fpga_of_match);
+
+static struct platform_driver zynq_fpga_driver = {
+	.probe = zynq_fpga_probe,
+	.remove = zynq_fpga_remove,
+	.driver = {
+		.name = "zynq_fpga_manager",
+		.of_match_table = of_match_ptr(zynq_fpga_of_match),
+	},
+};
+
+module_platform_driver(zynq_fpga_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
+MODULE_AUTHOR("Michal Simek <michal.simek@xilinx.com>");
+MODULE_DESCRIPTION("Xilinx Zynq FPGA Manager");
+MODULE_ALIAS("fpga:zynq");
-- 
2.6.1

Re: [PATCH 1/3] doc: dt: fpga: Added Documentation for Xilinx Zynq FPGA manager.

[Josh Cartwright]

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

On Fri, Oct 09, 2015 at 12:45:05AM +0200, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  .../bindings/fpga/xilinx-zynq-fpga-mgr.txt         | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> new file mode 100644
> index 0000000..82ffda8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> @@ -0,0 +1,26 @@
> +Xilinx Zynq FPGA Manager
> +
> +Required properties:
> +- compatible:		should contain "xlnx,zynq-devcfg-1.0"
> +- reg:			base address and size for memory mapped io
> +- interrupt parent:	interrupt source phandle

I think you mean 'interrupt-parent', with the hyphen.

Actually, this isn't really a 'required' property of this node, as it
could be specified in a parent node.

  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Josh Cartwright]

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

Hey Moritz-

On Fri, Oct 09, 2015 at 12:45:07AM +0200, Moritz Fischer wrote:
> This commit adds FPGA Manager support for the Xilinx Zynq chip.
> The code heavily borrows from the xdevcfg driver in Xilinx'
> vendor tree.
> 
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
[..]
> +++ b/drivers/fpga/zynq-fpga.c
[..]
> +static irqreturn_t zynq_fpga_isr(int irq, void *data)
> +{
> +	u32 intr_status;
> +	struct zynq_fpga_priv *priv = data;
> +
> +	spin_lock(&priv->lock);

I'm confused about the locking here.  You have this lock, but it's only
used in the isr.  It's claimed purpose is to protect 'error', but that
clearly isn't happening.

> +	intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
> +
> +	if (!intr_status) {
> +		spin_unlock(&priv->lock);
> +		return IRQ_NONE;
> +	}
> +
> +	zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
> +
> +	if ((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)
> +		complete(&priv->dma_done);

Just so I understand, wouldn't you also want to complete() in the error
case, too?

> +	if ((intr_status & IXR_ERROR_FLAGS_MASK) ==
> +			IXR_ERROR_FLAGS_MASK) {
> +		priv->error = true;
> +		dev_err(priv->dev, "%s dma error\n", __func__);
> +	}
> +	spin_unlock(&priv->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> +				    const char *buf, size_t count)
> +{
> +	struct zynq_fpga_priv *priv;
> +	u32 ctrl, status;
> +	int err;
> +
> +	priv = mgr->priv;
> +
> +	err = clk_enable(priv->clk);
> +	if (err)
> +		return err;
> +
> +	/* only reset if we're not doing partial reconfig */
> +	if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> +		/* assert AXI interface resets */
> +		regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
> +			     FPGA_RST_ALL_MASK);
> +
> +		/* disable level shifters */
> +		regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +			     LVL_SHFTR_DISABLE_ALL_MASK);
> +		/* enable output level shifters */
> +		regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +			     LVL_SHFTR_ENABLE_PS_TO_PL);
> +
> +		/* create a rising edge on PCFG_INIT. PCFG_INIT follows
> +		 * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
> +		 * to make sure the rising edge actually happens
> +		 */
> +		ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> +		ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> +		zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> +		zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +				       STATUS_PCFG_INIT_MASK, 20, 0);

And if we timeout?

> +
> +		ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> +		ctrl &= ~CTRL_PCFG_PROG_B_MASK;
> +
> +		zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> +		zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, !(status &
> +				       STATUS_PCFG_INIT_MASK), 20, 0);

And if we timeout?

> +
> +		ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> +		ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> +		zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> +		zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +				       STATUS_PCFG_INIT_MASK, 20, 0);

And if we timeout?

> +	}
> +
> +	clk_disable(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
> +			       const char *buf, size_t count)
> +{
> +	struct zynq_fpga_priv *priv;
> +	int err;
> +	char *kbuf;
> +	size_t i, in_count;
> +	dma_addr_t dma_addr;
> +	u32 transfer_length = 0;
> +	bool endian_swap = false;
> +
> +	in_count = count;
> +	priv = mgr->priv;
> +
> +	kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr, GFP_KERNEL);
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	memcpy(kbuf, buf, count);
> +
> +	/* look for the sync word */
> +	for (i = 0; i < count - 4; i++) {
> +		if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
> +			dev_dbg(priv->dev, "Found normal sync word\n");
> +			endian_swap = false;
> +			break;
> +		}
> +		if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
> +			dev_dbg(priv->dev, "Found swapped sync word\n");
> +			endian_swap = true;
> +			break;
> +		}
> +	}

How much control do we have over mandating the format of firmware at
this point?  It'd be swell if we could just mandate a specific
endianness, and leave this munging to usermode.

> +	/* remove the header, align the data on word boundary */
> +	if (i != count - 4) {
> +		count -= i;
> +		memmove(kbuf, kbuf + i, count);
> +	}
> +
> +	/* fixup endianness of the data */
> +	if (endian_swap) {
> +		for (i = 0; i < count; i += 4) {

Aren't we writing beyond the buffer, if count isn't word-aligned?

> +			u32 *p = (u32 *)&kbuf[i];
> +			*p = swab32(*p);
> +		}
> +	}
> +
> +	/* enable clock */
> +	err = clk_enable(priv->clk);
> +	if (err)
> +		goto out_free;
> +
> +	zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
> +
> +	/* enable DMA and error IRQs */
> +	zynq_fpga_unmask_irqs(priv);
> +
> +	priv->error = false;
> +
> +	/* the +1 in the src addr is used to hold off on DMA_DONE IRQ */
> +	/* until both AXI and PCAP are done */
> +	if (count < PAGE_SIZE)
> +		zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, (u32)(dma_addr + 1));
> +	else
> +		zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, (u32)(dma_addr));
> +
> +	zynq_fpga_write(priv, DMA_DEST_ADDR_OFFSET, (u32)DMA_INVALID_ADDRESS);
> +
> +	/* convert #bytes to #words */
> +	transfer_length = (count + 3) / 4;
> +
> +	zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, transfer_length);
> +	zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, 0);
> +
> +	wait_for_completion_interruptible(&priv->dma_done);

And if we're interrupted?

> +	if (priv->error) {
> +		dev_err(priv->dev, "Error configuring FPGA.\n");
> +		err = -EFAULT;
> +	}
> +
> +	/* disable DMA and error IRQs */
> +	zynq_fpga_mask_irqs(priv);
> +
> +	clk_disable(priv->clk);
> +
> +out_free:
> +	dma_free_coherent(priv->dev, in_count, kbuf, dma_addr);
> +
> +	return err;
> +}
[..]
> +static int zynq_fpga_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct zynq_fpga_priv *priv;
> +	struct resource *res;
> +	u32 ctrl_reg;
> +	int ret;
> +
[..]
> +	priv->irq = platform_get_irq(pdev, 0);
> +	if (priv->irq < 0) {
> +		dev_err(dev, "No IRQ available");
> +		return priv->irq;
> +	}
> +
> +	ret = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0,
> +			       dev_name(dev), priv);
> +	if (IS_ERR_VALUE(ret))

This is the wrong check for error in this case.  You should check 'ret'
being non-zero.

> +		return ret;
> +
> +	priv->clk = devm_clk_get(dev, "ref_clk");
> +	if (IS_ERR(priv->clk)) {
> +		dev_err(dev, "input clock not found\n");
> +		return PTR_ERR(priv->clk);
> +	}
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret) {
> +		dev_err(dev, "unable to enable clock\n");
> +		return ret;
> +	}
> +
> +	/* unlock the device */
> +	zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
> +
> +	/* set configuration register with following options:
> +	* - reset FPGA
> +	* - enable PCAP interface for partial reconfig
> +	* - set throughput for maximum speed
> +	* - set CPU in user mode
> +	*/
> +	ctrl_reg = zynq_fpga_read(priv, CTRL_OFFSET);
> +	zynq_fpga_write(priv, CTRL_OFFSET, (CTRL_PCFG_PROG_B_MASK |
> +		CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK | ctrl_reg));
> +
> +	/* ensure internal PCAP loopback is disabled */
> +	ctrl_reg = zynq_fpga_read(priv, MCTRL_OFFSET);
> +	zynq_fpga_write(priv, MCTRL_OFFSET, (~MCTRL_PCAP_LPBK_MASK & ctrl_reg));

Why do all of this initialization in probe()?  Is it necessary to read
FPGA state()?
>
> +
> +	ret = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
> +				&zynq_fpga_ops, priv);
> +	if (ret) {
> +		dev_err(dev, "unable to register FPGA manager");
> +		clk_disable_unprepare(priv->clk);
> +		return ret;
> +	}

I would have expected the clock to have been disabled after even a
successful probe.

> +	return 0;
> +}
> +
> +static int zynq_fpga_remove(struct platform_device *pdev)
> +{
> +	fpga_mgr_unregister(&pdev->dev);

Your clock management is unbalanced.

  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Moritz Fischer]

Hi Josh,

thanks for the review!

On Fri, Oct 9, 2015 at 6:33 PM, Josh Cartwright <joshc@ni.com> wrote:
> Hey Moritz-
>
> On Fri, Oct 09, 2015 at 12:45:07AM +0200, Moritz Fischer wrote:
>> This commit adds FPGA Manager support for the Xilinx Zynq chip.
>> The code heavily borrows from the xdevcfg driver in Xilinx'
>> vendor tree.
>>
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> [..]
>> +++ b/drivers/fpga/zynq-fpga.c
> [..]
>> +static irqreturn_t zynq_fpga_isr(int irq, void *data)
>> +{
>> +     u32 intr_status;
>> +     struct zynq_fpga_priv *priv = data;
>> +
>> +     spin_lock(&priv->lock);
>
> I'm confused about the locking here.  You have this lock, but it's only
> used in the isr.  It's claimed purpose is to protect 'error', but that
> clearly isn't happening.

Ouch, yes ...
>
>> +     intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
>> +
>> +     if (!intr_status) {
>> +             spin_unlock(&priv->lock);
>> +             return IRQ_NONE;
>> +     }
>> +
>> +     zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
>> +
>> +     if ((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)
>> +             complete(&priv->dma_done);
>
> Just so I understand, wouldn't you also want to complete() in the error
> case, too?

Ehrm ... yes. Definitely.
>
>> +     if ((intr_status & IXR_ERROR_FLAGS_MASK) ==
>> +                     IXR_ERROR_FLAGS_MASK) {
>> +             priv->error = true;
>> +             dev_err(priv->dev, "%s dma error\n", __func__);
>> +     }
>> +     spin_unlock(&priv->lock);
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>> +                                 const char *buf, size_t count)
>> +{
>> +     struct zynq_fpga_priv *priv;
>> +     u32 ctrl, status;
>> +     int err;
>> +
>> +     priv = mgr->priv;
>> +
>> +     err = clk_enable(priv->clk);
>> +     if (err)
>> +             return err;
>> +
>> +     /* only reset if we're not doing partial reconfig */
>> +     if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
>> +             /* assert AXI interface resets */
>> +             regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
>> +                          FPGA_RST_ALL_MASK);
>> +
>> +             /* disable level shifters */
>> +             regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
>> +                          LVL_SHFTR_DISABLE_ALL_MASK);
>> +             /* enable output level shifters */
>> +             regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
>> +                          LVL_SHFTR_ENABLE_PS_TO_PL);
>> +
>> +             /* create a rising edge on PCFG_INIT. PCFG_INIT follows
>> +              * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
>> +              * to make sure the rising edge actually happens
>> +              */
>> +             ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> +             ctrl |= CTRL_PCFG_PROG_B_MASK;
>> +
>> +             zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>> +
>> +             zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
>> +                                    STATUS_PCFG_INIT_MASK, 20, 0);
>
> And if we timeout?

Ehrm ... then we should cleanup & return ...
>
>> +
>> +             ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> +             ctrl &= ~CTRL_PCFG_PROG_B_MASK;
>> +
>> +             zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>> +
>> +             zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, !(status &
>> +                                    STATUS_PCFG_INIT_MASK), 20, 0);
>
> And if we timeout?

See above ...
>
>> +
>> +             ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> +             ctrl |= CTRL_PCFG_PROG_B_MASK;
>> +
>> +             zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>> +
>> +             zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
>> +                                    STATUS_PCFG_INIT_MASK, 20, 0);
>
> And if we timeout?

Ok ok ... got it...
>
>> +     }
>> +
>> +     clk_disable(priv->clk);
>> +
>> +     return 0;
>> +}
>> +
>> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>> +                            const char *buf, size_t count)
>> +{
>> +     struct zynq_fpga_priv *priv;
>> +     int err;
>> +     char *kbuf;
>> +     size_t i, in_count;
>> +     dma_addr_t dma_addr;
>> +     u32 transfer_length = 0;
>> +     bool endian_swap = false;
>> +
>> +     in_count = count;
>> +     priv = mgr->priv;
>> +
>> +     kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr, GFP_KERNEL);
>> +     if (!kbuf)
>> +             return -ENOMEM;
>> +
>> +     memcpy(kbuf, buf, count);
>> +
>> +     /* look for the sync word */
>> +     for (i = 0; i < count - 4; i++) {
>> +             if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
>> +                     dev_dbg(priv->dev, "Found normal sync word\n");
>> +                     endian_swap = false;
>> +                     break;
>> +             }
>> +             if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
>> +                     dev_dbg(priv->dev, "Found swapped sync word\n");
>> +                     endian_swap = true;
>> +                     break;
>> +             }
>> +     }
>
> How much control do we have over mandating the format of firmware at
> this point?  It'd be swell if we could just mandate a specific
> endianness, and leave this munging to usermode.

That's a good question. Personally I do only care about one of both,
but that's just because I get to decide for my targets...
Opinions from the Xilinx guys?

>
>> +     /* remove the header, align the data on word boundary */
>> +     if (i != count - 4) {
>> +             count -= i;
>> +             memmove(kbuf, kbuf + i, count);
>> +     }
>> +
>> +     /* fixup endianness of the data */
>> +     if (endian_swap) {
>> +             for (i = 0; i < count; i += 4) {
>
> Aren't we writing beyond the buffer, if count isn't word-aligned?
>
>> +                     u32 *p = (u32 *)&kbuf[i];
>> +                     *p = swab32(*p);
>> +             }
>> +     }
>> +
>> +     /* enable clock */
>> +     err = clk_enable(priv->clk);
>> +     if (err)
>> +             goto out_free;
>> +
>> +     zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
>> +
>> +     /* enable DMA and error IRQs */
>> +     zynq_fpga_unmask_irqs(priv);
>> +
>> +     priv->error = false;
>> +
>> +     /* the +1 in the src addr is used to hold off on DMA_DONE IRQ */
>> +     /* until both AXI and PCAP are done */
>> +     if (count < PAGE_SIZE)
>> +             zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, (u32)(dma_addr + 1));
>> +     else
>> +             zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, (u32)(dma_addr));
>> +
>> +     zynq_fpga_write(priv, DMA_DEST_ADDR_OFFSET, (u32)DMA_INVALID_ADDRESS);
>> +
>> +     /* convert #bytes to #words */
>> +     transfer_length = (count + 3) / 4;
>> +
>> +     zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, transfer_length);
>> +     zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, 0);
>> +
>> +     wait_for_completion_interruptible(&priv->dma_done);
>
> And if we're interrupted?

One should deal with that ...
>
>> +     if (priv->error) {
>> +             dev_err(priv->dev, "Error configuring FPGA.\n");
>> +             err = -EFAULT;
>> +     }
>> +
>> +     /* disable DMA and error IRQs */
>> +     zynq_fpga_mask_irqs(priv);
>> +
>> +     clk_disable(priv->clk);
>> +
>> +out_free:
>> +     dma_free_coherent(priv->dev, in_count, kbuf, dma_addr);
>> +
>> +     return err;
>> +}
> [..]
>> +static int zynq_fpga_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct zynq_fpga_priv *priv;
>> +     struct resource *res;
>> +     u32 ctrl_reg;
>> +     int ret;
>> +
> [..]
>> +     priv->irq = platform_get_irq(pdev, 0);
>> +     if (priv->irq < 0) {
>> +             dev_err(dev, "No IRQ available");
>> +             return priv->irq;
>> +     }
>> +
>> +     ret = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0,
>> +                            dev_name(dev), priv);
>> +     if (IS_ERR_VALUE(ret))
>
> This is the wrong check for error in this case.  You should check 'ret'
> being non-zero.

Good catch, will fix ...
>
>> +             return ret;
>> +
>> +     priv->clk = devm_clk_get(dev, "ref_clk");
>> +     if (IS_ERR(priv->clk)) {
>> +             dev_err(dev, "input clock not found\n");
>> +             return PTR_ERR(priv->clk);
>> +     }
>> +
>> +     ret = clk_prepare_enable(priv->clk);
>> +     if (ret) {
>> +             dev_err(dev, "unable to enable clock\n");
>> +             return ret;
>> +     }
>> +
>> +     /* unlock the device */
>> +     zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
>> +
>> +     /* set configuration register with following options:
>> +     * - reset FPGA
>> +     * - enable PCAP interface for partial reconfig
>> +     * - set throughput for maximum speed
>> +     * - set CPU in user mode
>> +     */
>> +     ctrl_reg = zynq_fpga_read(priv, CTRL_OFFSET);
>> +     zynq_fpga_write(priv, CTRL_OFFSET, (CTRL_PCFG_PROG_B_MASK |
>> +             CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK | ctrl_reg));
>> +
>> +     /* ensure internal PCAP loopback is disabled */
>> +     ctrl_reg = zynq_fpga_read(priv, MCTRL_OFFSET);
>> +     zynq_fpga_write(priv, MCTRL_OFFSET, (~MCTRL_PCAP_LPBK_MASK & ctrl_reg));
>
> Why do all of this initialization in probe()?  Is it necessary to read
> FPGA state()?

Hmmm ... good catch, again. Will rework...
>>
>> +
>> +     ret = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
>> +                             &zynq_fpga_ops, priv);
>> +     if (ret) {
>> +             dev_err(dev, "unable to register FPGA manager");
>> +             clk_disable_unprepare(priv->clk);
>> +             return ret;
>> +     }
>
> I would have expected the clock to have been disabled after even a
> successful probe.

Yes, you're right. Will do.
>
>> +     return 0;
>> +}
>> +
>> +static int zynq_fpga_remove(struct platform_device *pdev)
>> +{
>> +     fpga_mgr_unregister(&pdev->dev);
>
> Your clock management is unbalanced.

Arghh
>
>   Josh

Thanks for reviewing, seems like I got one or two things to fix ;-)

Moritz

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[atull]

On Thu, 8 Oct 2015, Moritz Fischer wrote:

> --- /dev/null
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -0,0 +1,478 @@
> +/*
> + * Copyright (c) 2011-2015 Xilinx Inc.
> + * Copyright (c) 2015, National Instruments Corp.
> + *
> + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
> + * in their vendor tree.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/string.h>

Hi Moritz,

That was fast!  I just have a couple of very minor comments...

Please alphabetize the #includes.

> +
> +/* Offsets into SLCR regmap */
> +#define SLCR_FPGA_RST_CTRL_OFFSET	0x240 /* FPGA Software Reset Control */
> +#define SLCR_LVL_SHFTR_EN_OFFSET	0x900 /* Level Shifters Enable */
> +
> +/* Constant Definitions */
> +#define CTRL_OFFSET		0x00 /* Control Register */
> +#define LOCK_OFFSET		0x04 /* Lock Register */
> +#define INT_STS_OFFSET		0x0c /* Interrupt Status Register */
> +#define INT_MASK_OFFSET		0x10 /* Interrupt Mask Register */
> +#define STATUS_OFFSET		0x14 /* Status Register */
> +#define DMA_SRC_ADDR_OFFSET	0x18 /* DMA Source Address Register */
> +#define DMA_DEST_ADDR_OFFSET	0x1c /* DMA Destination Address Reg */
> +#define DMA_SRC_LEN_OFFSET	0x20 /* DMA Source Transfer Length */
> +#define DMA_DEST_LEN_OFFSET	0x24 /* DMA Destination Transfer */
> +#define UNLOCK_OFFSET		0x34 /* Unlock Register */
> +#define MCTRL_OFFSET		0x80 /* Misc. Control Register */

Please fix up the indenting.

> +
> +/* Control Register Bit definitions */
> +#define CTRL_PCFG_PROG_B_MASK	BIT(30) /* Program signal to reset FPGA */
> +#define CTRL_PCAP_PR_MASK	BIT(27) /* Enable PCAP for PR */
> +#define CTRL_PCAP_MODE_MASK	BIT(26) /* Enable PCAP */
> +
> +/* Miscellaneous Control Register bit definitions */
> +#define MCTRL_PCAP_LPBK_MASK	BIT(4) /* Internal PCAP loopback */
> +
> +/* Status register bit definitions */
> +#define STATUS_PCFG_INIT_MASK	BIT(4) /* FPGA init status */
> +
> +/* Interrupt Status/Mask Register Bit definitions */
> +#define IXR_DMA_DONE_MASK	BIT(13) /* DMA command done */
> +#define IXR_D_P_DONE_MASK	BIT(12) /* DMA and PCAP cmd done */
> +#define IXR_PCFG_DONE_MASK	BIT(2)  /* FPGA programmed */
> +#define IXR_ERROR_FLAGS_MASK	0x00F0F860
> +#define IXR_ALL_MASK		0xF8F7F87F
> +
> +/* Miscellaneous constant values */
> +#define DMA_INVALID_ADDRESS	GENMASK(31, 0)  /* Invalid DMA address */
> +#define UNLOCK_MASK		0x757bdf0d /* Used to unlock the device */
> +
> +/* Masks for controlling stuff in SLCR */
> +#define LVL_SHFTR_DISABLE_ALL_MASK	0x0 /* Disable all Level shifters */
> +#define LVL_SHFTR_ENABLE_PS_TO_PL	0xa /* Enable all Level shifters */
> +#define LVL_SHFTR_ENABLE_PL_TO_PS	0xf /* Enable all Level shifters */
> +#define FPGA_RST_ALL_MASK		0xf /* Enable global resets */
> +#define FPGA_RST_NONE_MASK		0x0 /* Disable global resets */
> +

> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> +				    const char *buf, size_t count)
> +{
> +	struct zynq_fpga_priv *priv;
> +	u32 ctrl, status;
> +	int err;
> +
> +	priv = mgr->priv;
> +
> +	err = clk_enable(priv->clk);
> +	if (err)
> +		return err;

You might not even need to enable/disable the clock if not doing PR.
Does this driver support both full reconfig and partial reconfig?

> +
> +	/* only reset if we're not doing partial reconfig */
> +	if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> +		/* assert AXI interface resets */
> +		regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
> +			     FPGA_RST_ALL_MASK);
> +
> +		/* disable level shifters */
> +		regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +			     LVL_SHFTR_DISABLE_ALL_MASK);
> +		/* enable output level shifters */
> +		regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +			     LVL_SHFTR_ENABLE_PS_TO_PL);
> +
> +		/* create a rising edge on PCFG_INIT. PCFG_INIT follows
> +		 * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
> +		 * to make sure the rising edge actually happens
> +		 */
> +		ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> +		ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> +		zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> +		zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +				       STATUS_PCFG_INIT_MASK, 20, 0);
> +
> +		ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> +		ctrl &= ~CTRL_PCFG_PROG_B_MASK;
> +
> +		zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> +		zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, !(status &
> +				       STATUS_PCFG_INIT_MASK), 20, 0);
> +
> +		ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> +		ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> +		zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> +		zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +				       STATUS_PCFG_INIT_MASK, 20, 0);
> +	}
> +
> +	clk_disable(priv->clk);
> +
> +	return 0;
> +}
> +

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Greg KH]

On Fri, Oct 09, 2015 at 07:09:15PM +0100, atull wrote:
> On Thu, 8 Oct 2015, Moritz Fischer wrote:
> 
> > --- /dev/null
> > +++ b/drivers/fpga/zynq-fpga.c
> > @@ -0,0 +1,478 @@
> > +/*
> > + * Copyright (c) 2011-2015 Xilinx Inc.
> > + * Copyright (c) 2015, National Instruments Corp.
> > + *
> > + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
> > + * in their vendor tree.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; version 2 of the License.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/fpga/fpga-mgr.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/module.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/pm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/string.h>
> 
> Hi Moritz,
> 
> That was fast!  I just have a couple of very minor comments...
> 
> Please alphabetize the #includes.

Bah, who cares about that, it's not a requirement at all.

greg k-h

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[atull]

On Fri, 9 Oct 2015, Greg KH wrote:

> On Fri, Oct 09, 2015 at 07:09:15PM +0100, atull wrote:
> > On Thu, 8 Oct 2015, Moritz Fischer wrote:
> > 
> > > --- /dev/null
> > > +++ b/drivers/fpga/zynq-fpga.c
> > > @@ -0,0 +1,478 @@
> > > +/*
> > > + * Copyright (c) 2011-2015 Xilinx Inc.
> > > + * Copyright (c) 2015, National Instruments Corp.
> > > + *
> > > + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
> > > + * in their vendor tree.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; version 2 of the License.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/dma-mapping.h>
> > > +#include <linux/fpga/fpga-mgr.h>
> > > +#include <linux/io.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/pm.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/string.h>
> > 
> > Hi Moritz,
> > 
> > That was fast!  I just have a couple of very minor comments...
> > 
> > Please alphabetize the #includes.
> 
> Bah, who cares about that, it's not a requirement at all.
> 
> greg k-h
> 

Nice to know!  I get slammed every time for that!

Alan

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Moritz Fischer]

Hi Alan,

thanks for your feedback!

On Fri, Oct 9, 2015 at 8:09 PM, atull <atull@opensource.altera.com> wrote:
> On Thu, 8 Oct 2015, Moritz Fischer wrote:
>
>> --- /dev/null
>> +++ b/drivers/fpga/zynq-fpga.c
>> @@ -0,0 +1,478 @@
>> +/*
>> + * Copyright (c) 2011-2015 Xilinx Inc.
>> + * Copyright (c) 2015, National Instruments Corp.
>> + *
>> + * FPGA Manager Driver for Xilinx Zynq, heavily based on xdevcfg driver
>> + * in their vendor tree.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/fpga/fpga-mgr.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/pm.h>
>> +#include <linux/regmap.h>
>> +#include <linux/string.h>
>
> Hi Moritz,
>
> That was fast!  I just have a couple of very minor comments...
>
> Please alphabetize the #includes.
>
>> +
>> +/* Offsets into SLCR regmap */
>> +#define SLCR_FPGA_RST_CTRL_OFFSET    0x240 /* FPGA Software Reset Control */
>> +#define SLCR_LVL_SHFTR_EN_OFFSET     0x900 /* Level Shifters Enable */
>> +
>> +/* Constant Definitions */
>> +#define CTRL_OFFSET          0x00 /* Control Register */
>> +#define LOCK_OFFSET          0x04 /* Lock Register */
>> +#define INT_STS_OFFSET               0x0c /* Interrupt Status Register */
>> +#define INT_MASK_OFFSET              0x10 /* Interrupt Mask Register */
>> +#define STATUS_OFFSET                0x14 /* Status Register */
>> +#define DMA_SRC_ADDR_OFFSET  0x18 /* DMA Source Address Register */
>> +#define DMA_DEST_ADDR_OFFSET 0x1c /* DMA Destination Address Reg */
>> +#define DMA_SRC_LEN_OFFSET   0x20 /* DMA Source Transfer Length */
>> +#define DMA_DEST_LEN_OFFSET  0x24 /* DMA Destination Transfer */
>> +#define UNLOCK_OFFSET                0x34 /* Unlock Register */
>> +#define MCTRL_OFFSET         0x80 /* Misc. Control Register */
>
> Please fix up the indenting.

Will do.
>
>> +
>> +/* Control Register Bit definitions */
>> +#define CTRL_PCFG_PROG_B_MASK        BIT(30) /* Program signal to reset FPGA */
>> +#define CTRL_PCAP_PR_MASK    BIT(27) /* Enable PCAP for PR */
>> +#define CTRL_PCAP_MODE_MASK  BIT(26) /* Enable PCAP */
>> +
>> +/* Miscellaneous Control Register bit definitions */
>> +#define MCTRL_PCAP_LPBK_MASK BIT(4) /* Internal PCAP loopback */
>> +
>> +/* Status register bit definitions */
>> +#define STATUS_PCFG_INIT_MASK        BIT(4) /* FPGA init status */
>> +
>> +/* Interrupt Status/Mask Register Bit definitions */
>> +#define IXR_DMA_DONE_MASK    BIT(13) /* DMA command done */
>> +#define IXR_D_P_DONE_MASK    BIT(12) /* DMA and PCAP cmd done */
>> +#define IXR_PCFG_DONE_MASK   BIT(2)  /* FPGA programmed */
>> +#define IXR_ERROR_FLAGS_MASK 0x00F0F860
>> +#define IXR_ALL_MASK         0xF8F7F87F
>> +
>> +/* Miscellaneous constant values */
>> +#define DMA_INVALID_ADDRESS  GENMASK(31, 0)  /* Invalid DMA address */
>> +#define UNLOCK_MASK          0x757bdf0d /* Used to unlock the device */
>> +
>> +/* Masks for controlling stuff in SLCR */
>> +#define LVL_SHFTR_DISABLE_ALL_MASK   0x0 /* Disable all Level shifters */
>> +#define LVL_SHFTR_ENABLE_PS_TO_PL    0xa /* Enable all Level shifters */
>> +#define LVL_SHFTR_ENABLE_PL_TO_PS    0xf /* Enable all Level shifters */
>> +#define FPGA_RST_ALL_MASK            0xf /* Enable global resets */
>> +#define FPGA_RST_NONE_MASK           0x0 /* Disable global resets */
>> +
>
>> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>> +                                 const char *buf, size_t count)
>> +{
>> +     struct zynq_fpga_priv *priv;
>> +     u32 ctrl, status;
>> +     int err;
>> +
>> +     priv = mgr->priv;
>> +
>> +     err = clk_enable(priv->clk);
>> +     if (err)
>> +             return err;
>
> You might not even need to enable/disable the clock if not doing PR.

Yeah, you're probably right.
>
>> +
>> +     /* only reset if we're not doing partial reconfig */
>> +     if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
>> +             /* assert AXI interface resets */
>> +             regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
>> +                          FPGA_RST_ALL_MASK);
>> +
>> +             /* disable level shifters */
>> +             regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
>> +                          LVL_SHFTR_DISABLE_ALL_MASK);
>> +             /* enable output level shifters */
>> +             regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
>> +                          LVL_SHFTR_ENABLE_PS_TO_PL);
>> +
>> +             /* create a rising edge on PCFG_INIT. PCFG_INIT follows
>> +              * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
>> +              * to make sure the rising edge actually happens
>> +              */
>> +             ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> +             ctrl |= CTRL_PCFG_PROG_B_MASK;
>> +
>> +             zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>> +
>> +             zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
>> +                                    STATUS_PCFG_INIT_MASK, 20, 0);
>> +
>> +             ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> +             ctrl &= ~CTRL_PCFG_PROG_B_MASK;
>> +
>> +             zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>> +
>> +             zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, !(status &
>> +                                    STATUS_PCFG_INIT_MASK), 20, 0);
>> +
>> +             ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> +             ctrl |= CTRL_PCFG_PROG_B_MASK;
>> +
>> +             zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>> +
>> +             zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
>> +                                    STATUS_PCFG_INIT_MASK, 20, 0);
>> +     }
>> +
>> +     clk_disable(priv->clk);
>> +
>> +     return 0;
>> +}
>> +

Cheers & enjoy your vacation

Moritz

Re: [PATCH 1/3] doc: dt: fpga: Added Documentation for Xilinx Zynq FPGA manager.

[Michal Simek]

On 10/09/2015 06:04 PM, Josh Cartwright wrote:
> On Fri, Oct 09, 2015 at 12:45:05AM +0200, Moritz Fischer wrote:
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  .../bindings/fpga/xilinx-zynq-fpga-mgr.txt         | 26 ++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
>>
>> diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
>> new file mode 100644
>> index 0000000..82ffda8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
>> @@ -0,0 +1,26 @@
>> +Xilinx Zynq FPGA Manager
>> +
>> +Required properties:
>> +- compatible:		should contain "xlnx,zynq-devcfg-1.0"
>> +- reg:			base address and size for memory mapped io
>> +- interrupt parent:	interrupt source phandle
> 
> I think you mean 'interrupt-parent', with the hyphen.

yes.

> 
> Actually, this isn't really a 'required' property of this node, as it
> could be specified in a parent node.

yes. Normally interrupt-parent is not listed as required property in the
binding doc. Some docs listed it as optional property.

Thanks,
Michal

Re: [PATCH 2/3] dts: Updated devicetree bindings for Zynq 7000 platform

[Michal Simek]

On 10/09/2015 12:45 AM, Moritz Fischer wrote:
> Added addtional bindings required for FPGA Manager operation
> of the Xilinx Zynq Devc configuration interface.
> 
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  arch/arm/boot/dts/zynq-7000.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> index dc0457e..1a5220e 100644
> --- a/arch/arm/boot/dts/zynq-7000.dtsi
> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> @@ -294,6 +294,11 @@
>  		devcfg: devcfg@f8007000 {
>  			compatible = "xlnx,zynq-devcfg-1.0";
>  			reg = <0xf8007000 0x100>;
> +			interrupt-parent = <&intc>;
> +			interrupts = <0 8 4>;
> +			clocks = <&clkc 12>;
> +			clock-names = "ref_clk";
> +			syscon = <&slcr>;
>  		};
>  
>  		global_timer: timer@f8f00200 {
> 

This patch should go via arm-soc tree. That's why please remove it from
this patchset.

Thanks,
Michal

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Michal Simek]

>>> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>>> +                            const char *buf, size_t count)
>>> +{
>>> +     struct zynq_fpga_priv *priv;
>>> +     int err;
>>> +     char *kbuf;
>>> +     size_t i, in_count;
>>> +     dma_addr_t dma_addr;
>>> +     u32 transfer_length = 0;
>>> +     bool endian_swap = false;
>>> +
>>> +     in_count = count;
>>> +     priv = mgr->priv;
>>> +
>>> +     kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr, GFP_KERNEL);
>>> +     if (!kbuf)
>>> +             return -ENOMEM;
>>> +
>>> +     memcpy(kbuf, buf, count);
>>> +
>>> +     /* look for the sync word */
>>> +     for (i = 0; i < count - 4; i++) {
>>> +             if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
>>> +                     dev_dbg(priv->dev, "Found normal sync word\n");
>>> +                     endian_swap = false;
>>> +                     break;
>>> +             }

This is bin format

>>> +             if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
>>> +                     dev_dbg(priv->dev, "Found swapped sync word\n");
>>> +                     endian_swap = true;
>>> +                     break;
>>> +             }

This is bit format from header

>>> +     }
>>
>> How much control do we have over mandating the format of firmware at
>> this point?  It'd be swell if we could just mandate a specific
>> endianness, and leave this munging to usermode.
> 
> That's a good question. Personally I do only care about one of both,
> but that's just because I get to decide for my targets...
> Opinions from the Xilinx guys?

Don't know full history about this but in past bitstream in BIT format
was used. Which is header (partially decoding in u-boot for example)
with data.
On zynq native format is BIN which is format without header and data is
swapped.
This code just detects which format is used. If BIT, header is skipped
and data is swapped to BIN format.

Back to origin question if this is something what can be handled from
user space. And answer is - yes it can be handled there.
But based on my experience it is very useful to be able to handle BIT
because it is built by tools by default.
Also with BIN format you are loosing record what this data bitstream
targets. Header in BIT gives you at least some ideas.

Thanks,
Michal

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Mike Looijmans]

On 12-10-15 13:16, Michal Simek wrote:
>
>>>> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>>>> +                            const char *buf, size_t count)
>>>> +{
>>>> +     struct zynq_fpga_priv *priv;
>>>> +     int err;
>>>> +     char *kbuf;
>>>> +     size_t i, in_count;
>>>> +     dma_addr_t dma_addr;
>>>> +     u32 transfer_length = 0;
>>>> +     bool endian_swap = false;
>>>> +
>>>> +     in_count = count;
>>>> +     priv = mgr->priv;
>>>> +
>>>> +     kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr, GFP_KERNEL);
>>>> +     if (!kbuf)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     memcpy(kbuf, buf, count);
>>>> +
>>>> +     /* look for the sync word */
>>>> +     for (i = 0; i < count - 4; i++) {
>>>> +             if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
>>>> +                     dev_dbg(priv->dev, "Found normal sync word\n");
>>>> +                     endian_swap = false;
>>>> +                     break;
>>>> +             }
>
> This is bin format
>
>>>> +             if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
>>>> +                     dev_dbg(priv->dev, "Found swapped sync word\n");
>>>> +                     endian_swap = true;
>>>> +                     break;
>>>> +             }
>
> This is bit format from header
>
>>>> +     }
>>>
>>> How much control do we have over mandating the format of firmware at
>>> this point?  It'd be swell if we could just mandate a specific
>>> endianness, and leave this munging to usermode.
>>
>> That's a good question. Personally I do only care about one of both,
>> but that's just because I get to decide for my targets...
>> Opinions from the Xilinx guys?
>
> Don't know full history about this but in past bitstream in BIT format
> was used. Which is header (partially decoding in u-boot for example)
> with data.
> On zynq native format is BIN which is format without header and data is
> swapped.
> This code just detects which format is used. If BIT, header is skipped
> and data is swapped to BIN format.
>
> Back to origin question if this is something what can be handled from
> user space. And answer is - yes it can be handled there.
> But based on my experience it is very useful to be able to handle BIT
> because it is built by tools by default.
> Also with BIN format you are loosing record what this data bitstream
> targets. Header in BIT gives you at least some ideas.

People should stop using "cat" to program the FPGA and use a userspace tool 
instead. I've already released such tools under GPL, so anyone can pick up on 
it and extend it as required.

The header for the "bit" format is completely ignored (you can't even use it 
to determine if the bitstream is compatible with the current device) so 
there's no point in carrying it around. On the zynq, doing the "swap" in 
userspace was measurably faster than having the driver handle it, and that was 
even without using NEON instructions for byte swapping.

I admit that being able to do "cat static.bit > /dev/xdevcfg" has had its 
uses. But it's not something that belongs in mainline Linux.

Probably one of the key reasons that the "bit" format is still popular is that 
getting the Vivado tools to create a proper "bin" that will actually work on 
the Zynq is about as easy as nailing jelly to a tree. We've been using a 
simple Python script to do the bit->bin conversion for that reason.

Using the "bin" format in the driver keeps it simple and singular. Userspace 
tools can add whatever wrappers and headers they feel appropriate to it, these 
checks don't belong in the driver since they will be application specific. For 
example, some users would want to verify that a partial bitstream actually 
matches the static part that's currently in the FPGA.

Mike.


Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Michal Simek]

Hi Mike,

On 10/12/2015 02:22 PM, Mike Looijmans wrote:
> On 12-10-15 13:16, Michal Simek wrote:
>>
>>>>> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>>>>> +                            const char *buf, size_t count)
>>>>> +{
>>>>> +     struct zynq_fpga_priv *priv;
>>>>> +     int err;
>>>>> +     char *kbuf;
>>>>> +     size_t i, in_count;
>>>>> +     dma_addr_t dma_addr;
>>>>> +     u32 transfer_length = 0;
>>>>> +     bool endian_swap = false;
>>>>> +
>>>>> +     in_count = count;
>>>>> +     priv = mgr->priv;
>>>>> +
>>>>> +     kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr,
>>>>> GFP_KERNEL);
>>>>> +     if (!kbuf)
>>>>> +             return -ENOMEM;
>>>>> +
>>>>> +     memcpy(kbuf, buf, count);
>>>>> +
>>>>> +     /* look for the sync word */
>>>>> +     for (i = 0; i < count - 4; i++) {
>>>>> +             if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
>>>>> +                     dev_dbg(priv->dev, "Found normal sync word\n");
>>>>> +                     endian_swap = false;
>>>>> +                     break;
>>>>> +             }
>>
>> This is bin format
>>
>>>>> +             if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
>>>>> +                     dev_dbg(priv->dev, "Found swapped sync word\n");
>>>>> +                     endian_swap = true;
>>>>> +                     break;
>>>>> +             }
>>
>> This is bit format from header
>>
>>>>> +     }
>>>>
>>>> How much control do we have over mandating the format of firmware at
>>>> this point?  It'd be swell if we could just mandate a specific
>>>> endianness, and leave this munging to usermode.
>>>
>>> That's a good question. Personally I do only care about one of both,
>>> but that's just because I get to decide for my targets...
>>> Opinions from the Xilinx guys?
>>
>> Don't know full history about this but in past bitstream in BIT format
>> was used. Which is header (partially decoding in u-boot for example)
>> with data.
>> On zynq native format is BIN which is format without header and data is
>> swapped.
>> This code just detects which format is used. If BIT, header is skipped
>> and data is swapped to BIN format.
>>
>> Back to origin question if this is something what can be handled from
>> user space. And answer is - yes it can be handled there.
>> But based on my experience it is very useful to be able to handle BIT
>> because it is built by tools by default.
>> Also with BIN format you are loosing record what this data bitstream
>> targets. Header in BIT gives you at least some ideas.
> 
> People should stop using "cat" to program the FPGA and use a userspace
> tool instead. I've already released such tools under GPL, so anyone can
> pick up on it and extend it as required.

Link?

This is fpga manager based driver where "cat" won't be used.

> 
> The header for the "bit" format is completely ignored (you can't even
> use it to determine if the bitstream is compatible with the current
> device) so there's no point in carrying it around. 

up2you what you want to do with it. If you work with different boards
with different FPGAs it is at least helpful to know if X.bit target this
or that board. Unfortunately I am not aware about any public document
which describe what there is written.

> On the zynq, doing
> the "swap" in userspace was measurably faster than having the driver
> handle it, and that was even without using NEON instructions for byte
> swapping.
> 
> I admit that being able to do "cat static.bit > /dev/xdevcfg" has had
> its uses. But it's not something that belongs in mainline Linux.

It is about comfort but I have really not a problem that driver will
handle just BIN format.

> Probably one of the key reasons that the "bit" format is still popular
> is that getting the Vivado tools to create a proper "bin" that will
> actually work on the Zynq is about as easy as nailing jelly to a tree.
> We've been using a simple Python script to do the bit->bin conversion
> for that reason.

In vivado it is one tcl cmd. But truth is that I don't really get why
BIN is not generated by default.

> Using the "bin" format in the driver keeps it simple and singular.
> Userspace tools can add whatever wrappers and headers they feel
> appropriate to it, these checks don't belong in the driver since they
> will be application specific. For example, some users would want to
> verify that a partial bitstream actually matches the static part that's
> currently in the FPGA.

agree.

Thanks,
Michal

Re: [PATCH 2/3] dts: Updated devicetree bindings for Zynq 7000 platform

[Sören Brinkmann]

On Fri, 2015-10-09 at 12:45AM +0200, Moritz Fischer wrote:
> Added addtional bindings required for FPGA Manager operation
> of the Xilinx Zynq Devc configuration interface.
> 
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
Reviewed-by: Sören Brinkmann <soren.brinkmann@xilinx.com>

	Sören

Re: [PATCH 1/3] doc: dt: fpga: Added Documentation for Xilinx Zynq FPGA manager.

[Sören Brinkmann]

On Fri, 2015-10-09 at 12:45AM +0200, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  .../bindings/fpga/xilinx-zynq-fpga-mgr.txt         | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> new file mode 100644
> index 0000000..82ffda8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> @@ -0,0 +1,26 @@
> +Xilinx Zynq FPGA Manager
> +
> +Required properties:
> +- compatible:		should contain "xlnx,zynq-devcfg-1.0"
> +- reg:			base address and size for memory mapped io
> +- interrupt parent:	interrupt source phandle
> +- interrupts:		interrupt for the FPGA manager device
> +- clocks:		phandle for clocks required operation
> +- syscon:		phandle for access to SLCR registers
> +
> +Optional properties:
> +- clock-names:		names for clocks

Is it optional? Currently, there is only one clock input, so a match
without specifying a clock name should work making this optional. But in
your implementation, you do specify a clock name in devm_clk_get(). I'm
not entirely sure, but that call might fail if it doesn't find the
corresponding clock-names property.
I think, either we should make this required and list the required
entries here. Or the implementation probably needs to drop the clock
name when looking up its input clock.

	Sören

Re: [PATCH 1/3] doc: dt: fpga: Added Documentation for Xilinx Zynq FPGA manager.

[Moritz Fischer]

Hi Michal, Josh,

On Mon, Oct 12, 2015 at 2:31 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 10/09/2015 06:04 PM, Josh Cartwright wrote:
>> On Fri, Oct 09, 2015 at 12:45:05AM +0200, Moritz Fischer wrote:
>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>> ---
>>>  .../bindings/fpga/xilinx-zynq-fpga-mgr.txt         | 26 ++++++++++++++++++++++
>>>  1 file changed, 26 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
>>> new file mode 100644
>>> index 0000000..82ffda8
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
>>> @@ -0,0 +1,26 @@
>>> +Xilinx Zynq FPGA Manager
>>> +
>>> +Required properties:
>>> +- compatible:               should contain "xlnx,zynq-devcfg-1.0"
>>> +- reg:                      base address and size for memory mapped io
>>> +- interrupt parent: interrupt source phandle
>>
>> I think you mean 'interrupt-parent', with the hyphen.
>
> yes.
>
>>
>> Actually, this isn't really a 'required' property of this node, as it
>> could be specified in a parent node.
>
> yes. Normally interrupt-parent is not listed as required property in the
> binding doc. Some docs listed it as optional property.

Alright, will correct typo, and mark it as optional unless someone
feels strongly against having the optional.
>
> Thanks,
> Michal

Thanks for the feedback,

Moritz

Re: [PATCH 1/3] doc: dt: fpga: Added Documentation for Xilinx Zynq FPGA manager.

[Moritz Fischer]

Soeren,

On Mon, Oct 12, 2015 at 9:33 AM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> On Fri, 2015-10-09 at 12:45AM +0200, Moritz Fischer wrote:
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  .../bindings/fpga/xilinx-zynq-fpga-mgr.txt         | 26 ++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
>>
>> diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
>> new file mode 100644
>> index 0000000..82ffda8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
>> @@ -0,0 +1,26 @@
>> +Xilinx Zynq FPGA Manager
>> +
>> +Required properties:
>> +- compatible:                should contain "xlnx,zynq-devcfg-1.0"
>> +- reg:                       base address and size for memory mapped io
>> +- interrupt parent:  interrupt source phandle
>> +- interrupts:                interrupt for the FPGA manager device
>> +- clocks:            phandle for clocks required operation
>> +- syscon:            phandle for access to SLCR registers
>> +
>> +Optional properties:
>> +- clock-names:               names for clocks
>
> Is it optional? Currently, there is only one clock input, so a match
> without specifying a clock name should work making this optional. But in
> your implementation, you do specify a clock name in devm_clk_get(). I'm
> not entirely sure, but that call might fail if it doesn't find the
> corresponding clock-names property.
> I think, either we should make this required and list the required
> entries here. Or the implementation probably needs to drop the clock
> name when looking up its input clock.

I kinda prefer the named one, so I'll probaly adapt the
implementation, any feelings either way anyone?
>someone
>         Sören

Thanks,

Moritz

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Mike Looijmans]

On 12-10-15 14:38, Michal Simek wrote:
> Hi Mike,
>
> On 10/12/2015 02:22 PM, Mike Looijmans wrote:
>> On 12-10-15 13:16, Michal Simek wrote:
>>>
>>>>>> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>>>>>> +                            const char *buf, size_t count)
>>>>>> +{
>>>>>> +     struct zynq_fpga_priv *priv;
>>>>>> +     int err;
>>>>>> +     char *kbuf;
>>>>>> +     size_t i, in_count;
>>>>>> +     dma_addr_t dma_addr;
>>>>>> +     u32 transfer_length = 0;
>>>>>> +     bool endian_swap = false;
>>>>>> +
>>>>>> +     in_count = count;
>>>>>> +     priv = mgr->priv;
>>>>>> +
>>>>>> +     kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr,
>>>>>> GFP_KERNEL);
>>>>>> +     if (!kbuf)
>>>>>> +             return -ENOMEM;
>>>>>> +
>>>>>> +     memcpy(kbuf, buf, count);
>>>>>> +
>>>>>> +     /* look for the sync word */
>>>>>> +     for (i = 0; i < count - 4; i++) {
>>>>>> +             if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
>>>>>> +                     dev_dbg(priv->dev, "Found normal sync word\n");
>>>>>> +                     endian_swap = false;
>>>>>> +                     break;
>>>>>> +             }
>>>
>>> This is bin format
>>>
>>>>>> +             if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
>>>>>> +                     dev_dbg(priv->dev, "Found swapped sync word\n");
>>>>>> +                     endian_swap = true;
>>>>>> +                     break;
>>>>>> +             }
>>>
>>> This is bit format from header
>>>
>>>>>> +     }
>>>>>
>>>>> How much control do we have over mandating the format of firmware at
>>>>> this point?  It'd be swell if we could just mandate a specific
>>>>> endianness, and leave this munging to usermode.
>>>>
>>>> That's a good question. Personally I do only care about one of both,
>>>> but that's just because I get to decide for my targets...
>>>> Opinions from the Xilinx guys?
>>>
>>> Don't know full history about this but in past bitstream in BIT format
>>> was used. Which is header (partially decoding in u-boot for example)
>>> with data.
>>> On zynq native format is BIN which is format without header and data is
>>> swapped.
>>> This code just detects which format is used. If BIT, header is skipped
>>> and data is swapped to BIN format.
>>>
>>> Back to origin question if this is something what can be handled from
>>> user space. And answer is - yes it can be handled there.
>>> But based on my experience it is very useful to be able to handle BIT
>>> because it is built by tools by default.
>>> Also with BIN format you are loosing record what this data bitstream
>>> targets. Header in BIT gives you at least some ideas.
>>
>> People should stop using "cat" to program the FPGA and use a userspace
>> tool instead. I've already released such tools under GPL, so anyone can
>> pick up on it and extend it as required.
>
> Link?

https://github.com/topic-embedded-products/dyplo-utils/blob/master/dyploprogrammer.cpp
https://github.com/topic-embedded-products/libdyplo/blob/master/hardware.cpp#L261

Will need some work to combine into a single tool though.

> This is fpga manager based driver where "cat" won't be used.

Haven't looked into it yet, but I guess at some point one will have to stream 
some data from userspace into the device, right?

>> The header for the "bit" format is completely ignored (you can't even
>> use it to determine if the bitstream is compatible with the current
>> device) so there's no point in carrying it around.
>
> up2you what you want to do with it. If you work with different boards
> with different FPGAs it is at least helpful to know if X.bit target this
> or that board. Unfortunately I am not aware about any public document
> which describe what there is written.
>
>> On the zynq, doing
>> the "swap" in userspace was measurably faster than having the driver
>> handle it, and that was even without using NEON instructions for byte
>> swapping.
>>
>> I admit that being able to do "cat static.bit > /dev/xdevcfg" has had
>> its uses. But it's not something that belongs in mainline Linux.
>
> It is about comfort but I have really not a problem that driver will
> handle just BIN format.
>
>> Probably one of the key reasons that the "bit" format is still popular
>> is that getting the Vivado tools to create a proper "bin" that will
>> actually work on the Zynq is about as easy as nailing jelly to a tree.
>> We've been using a simple Python script to do the bit->bin conversion
>> for that reason.
>
> In vivado it is one tcl cmd. But truth is that I don't really get why
> BIN is not generated by default.

If I recall correctly, Vivado strips the "bit" header but doesn't swap the 
bytes, so the resulting bin file won't work.


>> Using the "bin" format in the driver keeps it simple and singular.
>> Userspace tools can add whatever wrappers and headers they feel
>> appropriate to it, these checks don't belong in the driver since they
>> will be application specific. For example, some users would want to
>> verify that a partial bitstream actually matches the static part that's
>> currently in the FPGA.
>
> agree.
>
> Thanks,
> Michal
>
>



Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Michal Simek]

On 10/13/2015 07:33 AM, Mike Looijmans wrote:
> On 12-10-15 14:38, Michal Simek wrote:
>> Hi Mike,
>>
>> On 10/12/2015 02:22 PM, Mike Looijmans wrote:
>>> On 12-10-15 13:16, Michal Simek wrote:
>>>>
>>>>>>> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>>>>>>> +                            const char *buf, size_t count)
>>>>>>> +{
>>>>>>> +     struct zynq_fpga_priv *priv;
>>>>>>> +     int err;
>>>>>>> +     char *kbuf;
>>>>>>> +     size_t i, in_count;
>>>>>>> +     dma_addr_t dma_addr;
>>>>>>> +     u32 transfer_length = 0;
>>>>>>> +     bool endian_swap = false;
>>>>>>> +
>>>>>>> +     in_count = count;
>>>>>>> +     priv = mgr->priv;
>>>>>>> +
>>>>>>> +     kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr,
>>>>>>> GFP_KERNEL);
>>>>>>> +     if (!kbuf)
>>>>>>> +             return -ENOMEM;
>>>>>>> +
>>>>>>> +     memcpy(kbuf, buf, count);
>>>>>>> +
>>>>>>> +     /* look for the sync word */
>>>>>>> +     for (i = 0; i < count - 4; i++) {
>>>>>>> +             if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
>>>>>>> +                     dev_dbg(priv->dev, "Found normal sync
>>>>>>> word\n");
>>>>>>> +                     endian_swap = false;
>>>>>>> +                     break;
>>>>>>> +             }
>>>>
>>>> This is bin format
>>>>
>>>>>>> +             if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
>>>>>>> +                     dev_dbg(priv->dev, "Found swapped sync
>>>>>>> word\n");
>>>>>>> +                     endian_swap = true;
>>>>>>> +                     break;
>>>>>>> +             }
>>>>
>>>> This is bit format from header
>>>>
>>>>>>> +     }
>>>>>>
>>>>>> How much control do we have over mandating the format of firmware at
>>>>>> this point?  It'd be swell if we could just mandate a specific
>>>>>> endianness, and leave this munging to usermode.
>>>>>
>>>>> That's a good question. Personally I do only care about one of both,
>>>>> but that's just because I get to decide for my targets...
>>>>> Opinions from the Xilinx guys?
>>>>
>>>> Don't know full history about this but in past bitstream in BIT format
>>>> was used. Which is header (partially decoding in u-boot for example)
>>>> with data.
>>>> On zynq native format is BIN which is format without header and data is
>>>> swapped.
>>>> This code just detects which format is used. If BIT, header is skipped
>>>> and data is swapped to BIN format.
>>>>
>>>> Back to origin question if this is something what can be handled from
>>>> user space. And answer is - yes it can be handled there.
>>>> But based on my experience it is very useful to be able to handle BIT
>>>> because it is built by tools by default.
>>>> Also with BIN format you are loosing record what this data bitstream
>>>> targets. Header in BIT gives you at least some ideas.
>>>
>>> People should stop using "cat" to program the FPGA and use a userspace
>>> tool instead. I've already released such tools under GPL, so anyone can
>>> pick up on it and extend it as required.
>>
>> Link?
> 
> https://github.com/topic-embedded-products/dyplo-utils/blob/master/dyploprogrammer.cpp
> 
> https://github.com/topic-embedded-products/libdyplo/blob/master/hardware.cpp#L261
> 
> 
> Will need some work to combine into a single tool though.
> 
>> This is fpga manager based driver where "cat" won't be used.
> 
> Haven't looked into it yet, but I guess at some point one will have to
> stream some data from userspace into the device, right?

Currently loading bitstream via firmware interface is used.

> 
>>> The header for the "bit" format is completely ignored (you can't even
>>> use it to determine if the bitstream is compatible with the current
>>> device) so there's no point in carrying it around.
>>
>> up2you what you want to do with it. If you work with different boards
>> with different FPGAs it is at least helpful to know if X.bit target this
>> or that board. Unfortunately I am not aware about any public document
>> which describe what there is written.
>>
>>> On the zynq, doing
>>> the "swap" in userspace was measurably faster than having the driver
>>> handle it, and that was even without using NEON instructions for byte
>>> swapping.
>>>
>>> I admit that being able to do "cat static.bit > /dev/xdevcfg" has had
>>> its uses. But it's not something that belongs in mainline Linux.
>>
>> It is about comfort but I have really not a problem that driver will
>> handle just BIN format.
>>
>>> Probably one of the key reasons that the "bit" format is still popular
>>> is that getting the Vivado tools to create a proper "bin" that will
>>> actually work on the Zynq is about as easy as nailing jelly to a tree.
>>> We've been using a simple Python script to do the bit->bin conversion
>>> for that reason.
>>
>> In vivado it is one tcl cmd. But truth is that I don't really get why
>> BIN is not generated by default.
> 
> If I recall correctly, Vivado strips the "bit" header but doesn't swap
> the bytes, so the resulting bin file won't work.


I have built bitstream from vivado 2014.4 and if you dump bit you will
see this.

All stuff in front of 0xaa995566 at 0xa0 is header.

00000000  00 09 0f f0 0f f0 0f f0  0f f0 00 00 01 61 00 32
|.............a.2|
00000010  64 65 73 69 67 6e 5f 31  5f 77 72 61 70 70 65 72
|design_1_wrapper|
00000020  3b 55 73 65 72 49 44 3d  30 58 46 46 46 46 46 46
|;UserID=0XFFFFFF|
00000030  46 46 3b 56 65 72 73 69  6f 6e 3d 32 30 31 34 2e
|FF;Version=2014.|
00000040  34 00 62 00 0c 37 7a 30  32 30 63 6c 67 34 38 34
|4.b..7z020clg484|
00000050  00 63 00 0b 32 30 31 35  2f 31 30 2f 31 33 00 64
|.c..2015/10/13.d|
00000060  00 09 31 30 3a 35 33 3a  33 38 00 65 00 3d ba fc
|..10:53:38.e.=..|
00000070  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
|................|
*
00000090  00 00 00 bb 11 22 00 44  ff ff ff ff ff ff ff ff
|.....".D........|
000000a0  aa 99 55 66 20 00 00 00  30 02 20 01 00 00 00 00  |..Uf ...0.
.....|


On the other hand bin format has also small header which can be skipped
for devcfg usage but doesn't need to be (I have tested it)

I do use this command for bitstream in bin format generation (it was in
edk 14.7).
promgen -p bin -data_width 32 -b -u 0x0 *.bit -w

00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
|................|
*
00000020  bb 00 00 00 44 00 22 11  ff ff ff ff ff ff ff ff
|....D.".........|
00000030  66 55 99 aa 00 00 00 20  01 20 02 30 00 00 00 00  |fU..... .
.0....|
00000040  01 00 02 30 00 00 00 00  01 80 00 30 00 00 00 00
|...0.......0....|
00000050  00 00 00 20 01 80 00 30  07 00 00 00 00 00 00 20  |...
...0....... |
00000060  00 00 00 20 01 60 02 30  00 00 00 00 01 20 01 30  |...
.`.0..... .0|
00000070  e5 3f 00 02 01 c0 01 30  00 00 00 00 01 80 01 30
|.?.....0.......0|
00000080  93 70 72 03 01 80 00 30  09 00 00 00 00 00 00 20
|.pr....0....... |
00000090  01 c0 00 30 01 04 00 00  01 a0 00 30 01 05 00 00
|...0.......0....|
000000a0  01 c0 00 30 00 00 00 00  01 00 03 30 00 00 00 00
|...0.......0....|
000000b0  00 00 00 20 00 00 00 20  00 00 00 20 00 00 00 20  |... ... ...
... |

Thanks,
Michal

Re: [PATCH 2/3] dts: Updated devicetree bindings for Zynq 7000 platform

[Moritz Fischer]

Hi Michal,

On Mon, Oct 12, 2015 at 2:32 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 10/09/2015 12:45 AM, Moritz Fischer wrote:
>> Added addtional bindings required for FPGA Manager operation
>> of the Xilinx Zynq Devc configuration interface.
>>
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  arch/arm/boot/dts/zynq-7000.dtsi | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
>> index dc0457e..1a5220e 100644
>> --- a/arch/arm/boot/dts/zynq-7000.dtsi
>> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
>> @@ -294,6 +294,11 @@
>>               devcfg: devcfg@f8007000 {
>>                       compatible = "xlnx,zynq-devcfg-1.0";
>>                       reg = <0xf8007000 0x100>;
>> +                     interrupt-parent = <&intc>;
>> +                     interrupts = <0 8 4>;
>> +                     clocks = <&clkc 12>;
>> +                     clock-names = "ref_clk";
>> +                     syscon = <&slcr>;
>>               };
>>
>>               global_timer: timer@f8f00200 {
>>
>
> This patch should go via arm-soc tree. That's why please remove it from
> this patchset.

Just to clarify, you want this as a separate patch?

>
> Thanks,
> Michal

Cheers,

Moritz

Re: [PATCH 2/3] dts: Updated devicetree bindings for Zynq 7000 platform

[Michal Simek]

On 10/14/2015 04:50 AM, Moritz Fischer wrote:
> Hi Michal,
> 
> On Mon, Oct 12, 2015 at 2:32 AM, Michal Simek <michal.simek@xilinx.com> wrote:
>> On 10/09/2015 12:45 AM, Moritz Fischer wrote:
>>> Added addtional bindings required for FPGA Manager operation
>>> of the Xilinx Zynq Devc configuration interface.
>>>
>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>> ---
>>>  arch/arm/boot/dts/zynq-7000.dtsi | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
>>> index dc0457e..1a5220e 100644
>>> --- a/arch/arm/boot/dts/zynq-7000.dtsi
>>> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
>>> @@ -294,6 +294,11 @@
>>>               devcfg: devcfg@f8007000 {
>>>                       compatible = "xlnx,zynq-devcfg-1.0";
>>>                       reg = <0xf8007000 0x100>;
>>> +                     interrupt-parent = <&intc>;
>>> +                     interrupts = <0 8 4>;
>>> +                     clocks = <&clkc 12>;
>>> +                     clock-names = "ref_clk";
>>> +                     syscon = <&slcr>;
>>>               };
>>>
>>>               global_timer: timer@f8f00200 {
>>>
>>
>> This patch should go via arm-soc tree. That's why please remove it from
>> this patchset.
> 
> Just to clarify, you want this as a separate patch?

yes when binding is ACKed.

Thanks,
Michal