Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1242827 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000 Moritz Fischer <moritz.fischer@ettus.com> - 2015-10-09 00:50 +0200
    Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx  Zynq 7000 Josh Cartwright <joshc@ni.com> - 2015-10-09 18:40 +0200
      Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx  Zynq 7000 Moritz Fischer <moritz.fischer@ettus.com> - 2015-10-09 19:20 +0200
        Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx  Zynq 7000 Mike Looijmans <mike.looijmans@topic.nl> - 2015-10-12 20:10 +0200
          Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx  Zynq 7000 Mike Looijmans <mike.looijmans@topic.nl> - 2015-10-13 08:00 +0200
    Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx  Zynq 7000 Greg KH <gregkh@linuxfoundation.org> - 2015-10-09 20:20 +0200
    Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx  Zynq 7000 Moritz Fischer <moritz.fischer@ettus.com> - 2015-10-12 04:30 +0200

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web