[PATCH fpga 0/9] Zynq FPGA Manager Improvements

[PATCH fpga 0/9] Zynq FPGA Manager Improvements

[Jason Gunthorpe]

This series is all the changes I made while reviewing the Zynq driver.

The first five patches are independent straight forward changes.

The next four patches rework the FPGA manager core code and all drivers to
only use scatter lists for bitfile storage. This is an essential change
looking forward as the high order physical and vmalloc allocations currently
used are simply unreliable and unusable in many cases.

This does not fully fix the request_firmware path, as that needs changes to
the firmware core, but does all the low level work needed in this
subsystem. Zynq sees a significant improvement as it no longer needs large
amounts of physically contiguous DMA coherent memory.

Other users who can use the new sg interface have no limitations, and could
zero-copy DMA directly out of the page cache, for instance.

I do not have socfpga hardware, so those straightfoward changes are untested,
but I have tested the Zynq driver extensively now.

Jason Gunthorpe (9):
  fpga zynq: Add missing \n to messages
  fpga zynq: Check the bitstream for validity
  fpga zynq: Fix incorrect ISR state on bootup
  fpga zynq: Check for errors after completing DMA
  fpga zynq: Remove priv->dev
  fpga: Add scatterlist based write ops to the driver ops
  fpga zynq: Use the scatterlist interface
  fpga socfpga: Use the scatterlist interface
  fpga: Remove support for non-sg drivers

 drivers/fpga/fpga-mgr.c       |  87 +++++++++++--
 drivers/fpga/socfpga.c        |  56 ++++++---
 drivers/fpga/zynq-fpga.c      | 278 ++++++++++++++++++++++++++++++------------
 include/linux/fpga/fpga-mgr.h |  14 ++-
 4 files changed, 327 insertions(+), 108 deletions(-)

-- 
2.1.4

[PATCH fpga 1/9] fpga zynq: Add missing \n to messages

[Jason Gunthorpe]

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/fpga/zynq-fpga.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index c2fb4120bd62..e72340ea7323 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -217,7 +217,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 					     INIT_POLL_DELAY,
 					     INIT_POLL_TIMEOUT);
 		if (err) {
-			dev_err(priv->dev, "Timeout waiting for PCFG_INIT");
+			dev_err(priv->dev, "Timeout waiting for PCFG_INIT\n");
 			goto out_err;
 		}
 
@@ -231,7 +231,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 					     INIT_POLL_DELAY,
 					     INIT_POLL_TIMEOUT);
 		if (err) {
-			dev_err(priv->dev, "Timeout waiting for !PCFG_INIT");
+			dev_err(priv->dev, "Timeout waiting for !PCFG_INIT\n");
 			goto out_err;
 		}
 
@@ -245,7 +245,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 					     INIT_POLL_DELAY,
 					     INIT_POLL_TIMEOUT);
 		if (err) {
-			dev_err(priv->dev, "Timeout waiting for PCFG_INIT");
+			dev_err(priv->dev, "Timeout waiting for PCFG_INIT\n");
 			goto out_err;
 		}
 	}
@@ -262,7 +262,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 	/* check that we have room in the command queue */
 	status = zynq_fpga_read(priv, STATUS_OFFSET);
 	if (status & STATUS_DMA_Q_F) {
-		dev_err(priv->dev, "DMA command queue full");
+		dev_err(priv->dev, "DMA command queue full\n");
 		err = -EBUSY;
 		goto out_err;
 	}
@@ -331,7 +331,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 	zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
 
 	if (!((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)) {
-		dev_err(priv->dev, "Error configuring FPGA");
+		dev_err(priv->dev, "Error configuring FPGA\n");
 		err = -EFAULT;
 	}
 
@@ -426,7 +426,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 	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");
+		dev_err(dev, "unable to get zynq-slcr regmap\n");
 		return PTR_ERR(priv->slcr);
 	}
 
@@ -434,26 +434,26 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 
 	priv->irq = platform_get_irq(pdev, 0);
 	if (priv->irq < 0) {
-		dev_err(dev, "No IRQ available");
+		dev_err(dev, "No IRQ available\n");
 		return priv->irq;
 	}
 
 	err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0,
 			       dev_name(dev), priv);
 	if (err) {
-		dev_err(dev, "unable to request IRQ");
+		dev_err(dev, "unable to request IRQ\n");
 		return err;
 	}
 
 	priv->clk = devm_clk_get(dev, "ref_clk");
 	if (IS_ERR(priv->clk)) {
-		dev_err(dev, "input clock not found");
+		dev_err(dev, "input clock not found\n");
 		return PTR_ERR(priv->clk);
 	}
 
 	err = clk_prepare_enable(priv->clk);
 	if (err) {
-		dev_err(dev, "unable to enable clock");
+		dev_err(dev, "unable to enable clock\n");
 		return err;
 	}
 
@@ -465,7 +465,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 	err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
 				&zynq_fpga_ops, priv);
 	if (err) {
-		dev_err(dev, "unable to register FPGA manager");
+		dev_err(dev, "unable to register FPGA manager\n");
 		clk_unprepare(priv->clk);
 		return err;
 	}
-- 
2.1.4

[PATCH fpga 2/9] fpga zynq: Check the bitstream for validity

[Jason Gunthorpe]

There is no sense in sending a bitstream we know will not work, and
with the variety of options for bitstream generation in Xilinx tools
it is not terribly clear or very well documented what the correct
input should be, especially since auto-detection was removed from this
driver.

All Zynq full configuration bitstreams must start with the sync word in
the correct byte order.

Zynq is also only able to DMA dword quantities, so bitstreams must be
a multiple of 4 bytes. This also fixes a DMA-past the end bug.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/fpga/zynq-fpga.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index e72340ea7323..86f4377e2b52 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -175,6 +175,19 @@ static irqreturn_t zynq_fpga_isr(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+/* Sanity check the proposed bitstream. It must start with the sync word in
+ * the correct byte order. The input is a Xilinx .bin file with every 32 bit
+ * quantity swapped.
+ */
+static bool zynq_fpga_has_sync(const char *buf, size_t count)
+{
+	for (; count > 4; buf += 4, count -= 4)
+		if (buf[0] == 0x66 && buf[1] == 0x55 && buf[2] == 0x99 &&
+		    buf[3] == 0xaa)
+			return true;
+	return false;
+}
+
 static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 				    const char *buf, size_t count)
 {
@@ -184,12 +197,28 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 
 	priv = mgr->priv;
 
+	/* The hardware can only DMA multiples of 4 bytes, and we need at
+	 * least the sync word and something else to do anything.
+	 */
+	if (count <= 4 || (count % 4) != 0) {
+		dev_err(priv->dev,
+			"Invalid bitstream size, must be multiples of 4 bytes\n");
+		return -EINVAL;
+	}
+
 	err = clk_enable(priv->clk);
 	if (err)
 		return err;
 
 	/* don't globally reset PL if we're doing partial reconfig */
 	if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
+		if (!zynq_fpga_has_sync(buf, count)) {
+			dev_err(priv->dev,
+				"Invalid bitstream, could not find a sync word. Bitstream must be a byte swaped .bin file\n");
+			err = -EINVAL;
+			goto out_err;
+		}
+
 		/* assert AXI interface resets */
 		regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
 			     FPGA_RST_ALL_MASK);
@@ -287,12 +316,9 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 	struct zynq_fpga_priv *priv;
 	int err;
 	char *kbuf;
-	size_t in_count;
 	dma_addr_t dma_addr;
-	u32 transfer_length;
 	u32 intr_status;
 
-	in_count = count;
 	priv = mgr->priv;
 
 	kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr, GFP_KERNEL);
@@ -318,11 +344,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 	 */
 	zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, (u32)(dma_addr) + 1);
 	zynq_fpga_write(priv, DMA_DST_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_SRC_LEN_OFFSET, count / 4);
 	zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, 0);
 
 	wait_for_completion(&priv->dma_done);
@@ -338,7 +360,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 	clk_disable(priv->clk);
 
 out_free:
-	dma_free_coherent(priv->dev, in_count, kbuf, dma_addr);
+	dma_free_coherent(priv->dev, count, kbuf, dma_addr);
 
 	return err;
 }
-- 
2.1.4

[PATCH fpga 3/9] fpga zynq: Fix incorrect ISR state on bootup

[Jason Gunthorpe]

It is best practice to clear and mask all interrupts before
associating the IRQ, and this should be done after the clock
is enabled.

This corrects a bad result from zynq_fpga_ops_state on bootup
where left over latched values in INT_STS_OFFSET caused it to
report an unconfigured FPGA as configured.

After this change the boot up operating state for an unconfigured
FPGA reports 'unknown'.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/fpga/zynq-fpga.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 86f4377e2b52..40cf0feaca7c 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -460,13 +460,6 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 		return priv->irq;
 	}
 
-	err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0,
-			       dev_name(dev), priv);
-	if (err) {
-		dev_err(dev, "unable to request IRQ\n");
-		return err;
-	}
-
 	priv->clk = devm_clk_get(dev, "ref_clk");
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "input clock not found\n");
@@ -482,6 +475,16 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 	/* unlock the device */
 	zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
 
+	zynq_fpga_write(priv, INT_MASK_OFFSET, 0xFFFFFFFF);
+	zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
+	err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0, dev_name(dev),
+			       priv);
+	if (err) {
+		dev_err(dev, "unable to request IRQ\n");
+		clk_unprepare(priv->clk);
+		return err;
+	}
+
 	clk_disable(priv->clk);
 
 	err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
-- 
2.1.4

[PATCH fpga 4/9] fpga zynq: Check for errors after completing DMA

[Jason Gunthorpe]

The completion did not check the interrupt status to see if any error
bits were asserted, check error bits and dump some registers if things
went wrong.

A few fixes are needed to make this work, the IXR_ERROR_FLAGS_MASK was
wrong, it included the done bits, which shows a bug in mask/unmask_irqs
which were using the wrong bits, simplify all of this stuff.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/fpga/zynq-fpga.c | 55 +++++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 40cf0feaca7c..3ffc5fcc3072 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -89,7 +89,7 @@
 #define IXR_D_P_DONE_MASK		BIT(12)
  /* FPGA programmed */
 #define IXR_PCFG_DONE_MASK		BIT(2)
-#define IXR_ERROR_FLAGS_MASK		0x00F0F860
+#define IXR_ERROR_FLAGS_MASK		0x00F0C860
 #define IXR_ALL_MASK			0xF8F7F87F
 
 /* Miscellaneous constant values */
@@ -144,23 +144,10 @@ static inline u32 zynq_fpga_read(const struct zynq_fpga_priv *priv,
 	readl_poll_timeout(priv->io_base + addr, val, cond, sleep_us, \
 			   timeout_us)
 
-static void zynq_fpga_mask_irqs(struct zynq_fpga_priv *priv)
+static inline void zynq_fpga_set_irq_mask(struct zynq_fpga_priv *priv,
+					  u32 enable)
 {
-	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));
+	zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
 }
 
 static irqreturn_t zynq_fpga_isr(int irq, void *data)
@@ -168,7 +155,7 @@ static irqreturn_t zynq_fpga_isr(int irq, void *data)
 	struct zynq_fpga_priv *priv = data;
 
 	/* disable DMA and error IRQs */
-	zynq_fpga_mask_irqs(priv);
+	zynq_fpga_set_irq_mask(priv, 0);
 
 	complete(&priv->dma_done);
 
@@ -314,6 +301,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 			       const char *buf, size_t count)
 {
 	struct zynq_fpga_priv *priv;
+	const char *why;
 	int err;
 	char *kbuf;
 	dma_addr_t dma_addr;
@@ -337,7 +325,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 	reinit_completion(&priv->dma_done);
 
 	/* enable DMA and error IRQs */
-	zynq_fpga_unmask_irqs(priv);
+	zynq_fpga_set_irq_mask(priv, IXR_D_P_DONE_MASK | IXR_ERROR_FLAGS_MASK);
 
 	/* the +1 in the src addr is used to hold off on DMA_DONE IRQ
 	 * until both AXI and PCAP are done ...
@@ -352,16 +340,35 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 	intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
 	zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
 
+	if (intr_status & IXR_ERROR_FLAGS_MASK) {
+		why = "DMA reported error";
+		err = -EIO;
+		goto out_report;
+	}
+
 	if (!((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)) {
-		dev_err(priv->dev, "Error configuring FPGA\n");
-		err = -EFAULT;
+		why = "DMA did not complete";
+		err = -EIO;
+		goto out_report;
 	}
 
+	err = 0;
+	goto out_clk;
+
+out_report:
+	dev_err(priv->dev,
+		"%s: INT_STS:0x%x CTRL:0x%x LOCK:0x%x INT_MASK:0x%x STATUS:0x%x MCTRL:0x%x\n",
+		why,
+		intr_status,
+		zynq_fpga_read(priv, CTRL_OFFSET),
+		zynq_fpga_read(priv, LOCK_OFFSET),
+		zynq_fpga_read(priv, INT_MASK_OFFSET),
+		zynq_fpga_read(priv, STATUS_OFFSET),
+		zynq_fpga_read(priv, MCTRL_OFFSET));
+out_clk:
 	clk_disable(priv->clk);
-
 out_free:
 	dma_free_coherent(priv->dev, count, kbuf, dma_addr);
-
 	return err;
 }
 
@@ -475,7 +482,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 	/* unlock the device */
 	zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
 
-	zynq_fpga_write(priv, INT_MASK_OFFSET, 0xFFFFFFFF);
+	zynq_fpga_set_irq_mask(priv, 0);
 	zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
 	err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0, dev_name(dev),
 			       priv);
-- 
2.1.4

[PATCH fpga 5/9] fpga zynq: Remove priv->dev

[Jason Gunthorpe]

socfpga uses mgr->dev for debug prints, there should be consistency
here, so standardize on that. The only other use was for dma
which can be replaced with mgr->dev.parent.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/fpga/zynq-fpga.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 3ffc5fcc3072..ac2deae92dbd 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -118,7 +118,6 @@
 #define FPGA_RST_NONE_MASK		0x0
 
 struct zynq_fpga_priv {
-	struct device *dev;
 	int irq;
 	struct clk *clk;
 
@@ -188,7 +187,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 	 * least the sync word and something else to do anything.
 	 */
 	if (count <= 4 || (count % 4) != 0) {
-		dev_err(priv->dev,
+		dev_err(&mgr->dev,
 			"Invalid bitstream size, must be multiples of 4 bytes\n");
 		return -EINVAL;
 	}
@@ -200,7 +199,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 	/* don't globally reset PL if we're doing partial reconfig */
 	if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
 		if (!zynq_fpga_has_sync(buf, count)) {
-			dev_err(priv->dev,
+			dev_err(&mgr->dev,
 				"Invalid bitstream, could not find a sync word. Bitstream must be a byte swaped .bin file\n");
 			err = -EINVAL;
 			goto out_err;
@@ -233,7 +232,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 					     INIT_POLL_DELAY,
 					     INIT_POLL_TIMEOUT);
 		if (err) {
-			dev_err(priv->dev, "Timeout waiting for PCFG_INIT\n");
+			dev_err(&mgr->dev, "Timeout waiting for PCFG_INIT\n");
 			goto out_err;
 		}
 
@@ -247,7 +246,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 					     INIT_POLL_DELAY,
 					     INIT_POLL_TIMEOUT);
 		if (err) {
-			dev_err(priv->dev, "Timeout waiting for !PCFG_INIT\n");
+			dev_err(&mgr->dev, "Timeout waiting for !PCFG_INIT\n");
 			goto out_err;
 		}
 
@@ -261,7 +260,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 					     INIT_POLL_DELAY,
 					     INIT_POLL_TIMEOUT);
 		if (err) {
-			dev_err(priv->dev, "Timeout waiting for PCFG_INIT\n");
+			dev_err(&mgr->dev, "Timeout waiting for PCFG_INIT\n");
 			goto out_err;
 		}
 	}
@@ -278,7 +277,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 	/* check that we have room in the command queue */
 	status = zynq_fpga_read(priv, STATUS_OFFSET);
 	if (status & STATUS_DMA_Q_F) {
-		dev_err(priv->dev, "DMA command queue full\n");
+		dev_err(&mgr->dev, "DMA command queue full\n");
 		err = -EBUSY;
 		goto out_err;
 	}
@@ -309,7 +308,8 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 
 	priv = mgr->priv;
 
-	kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr, GFP_KERNEL);
+	kbuf =
+	    dma_alloc_coherent(mgr->dev.parent, count, &dma_addr, GFP_KERNEL);
 	if (!kbuf)
 		return -ENOMEM;
 
@@ -356,7 +356,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 	goto out_clk;
 
 out_report:
-	dev_err(priv->dev,
+	dev_err(&mgr->dev,
 		"%s: INT_STS:0x%x CTRL:0x%x LOCK:0x%x INT_MASK:0x%x STATUS:0x%x MCTRL:0x%x\n",
 		why,
 		intr_status,
@@ -368,7 +368,7 @@ out_report:
 out_clk:
 	clk_disable(priv->clk);
 out_free:
-	dma_free_coherent(priv->dev, count, kbuf, dma_addr);
+	dma_free_coherent(mgr->dev.parent, count, kbuf, dma_addr);
 	return err;
 }
 
@@ -445,8 +445,6 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
-	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))
-- 
2.1.4

[PATCH fpga 6/9] fpga: Add scatterlist based write ops to the driver ops

[Jason Gunthorpe]

Requiring contiguous kernel memory is not a good idea, this is a limited
resource and allocation can fail under normal work loads.

As a first step allow for drivers to provide a _sg write interface and
internally convert the existing contiguous mappings into a scatterlist.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/fpga/fpga-mgr.c       | 127 +++++++++++++++++++++++++++++++++++++++---
 include/linux/fpga/fpga-mgr.h |   9 ++-
 2 files changed, 128 insertions(+), 8 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 953dc9195937..c2491ffeabd3 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -25,26 +25,76 @@
 #include <linux/of.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/scatterlist.h>
+#include <linux/highmem.h>
 
 static DEFINE_IDA(fpga_mgr_ida);
 static struct class *fpga_mgr_class;
 
 /**
- * fpga_mgr_buf_load - load fpga from image in buffer
+ * fpga_mgr_buf_load_sg - load fpga from image in buffer from a scatter list
  * @mgr:	fpga manager
  * @flags:	flags setting fpga confuration modes
- * @buf:	buffer contain fpga image
- * @count:	byte count of buf
+ * @sgt:	scatterlist table
  *
  * Step the low level fpga manager through the device-specific steps of getting
  * an FPGA ready to be configured, writing the image to it, then doing whatever
  * post-configuration steps necessary.  This code assumes the caller got the
  * mgr pointer from of_fpga_mgr_get() and checked that it is not an error code.
  *
+ * This is the preferred entry point for FPGA programming, it does not require
+ * any contiguous kernel memory.
+ *
  * Return: 0 on success, negative error code otherwise.
  */
-int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
-		      size_t count)
+static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags,
+				struct sg_table *sgt)
+{
+	struct device *dev = &mgr->dev;
+	int ret;
+
+	/*
+	 * Call the low level driver's write_init function.  This will do the
+	 * device-specific things to get the FPGA into the state where it is
+	 * ready to receive an FPGA image.
+	 */
+	mgr->state = FPGA_MGR_STATE_WRITE_INIT;
+	ret = mgr->mops->write_init_sg(mgr, flags, sgt);
+	if (ret) {
+		dev_err(dev, "Error preparing FPGA for writing\n");
+		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
+		return ret;
+	}
+
+	/*
+	 * Write the FPGA image to the FPGA.
+	 */
+	mgr->state = FPGA_MGR_STATE_WRITE;
+	ret = mgr->mops->write_sg(mgr, sgt);
+	if (ret) {
+		dev_err(dev, "Error while writing image data to FPGA\n");
+		mgr->state = FPGA_MGR_STATE_WRITE_ERR;
+		return ret;
+	}
+
+	/*
+	 * After all the FPGA image has been written, do the device specific
+	 * steps to finish and set the FPGA into operating mode.
+	 */
+	mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
+	ret = mgr->mops->write_complete(mgr, flags);
+	if (ret) {
+		dev_err(dev, "Error after writing image data to FPGA\n");
+		mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
+		return ret;
+	}
+	mgr->state = FPGA_MGR_STATE_OPERATING;
+
+	return 0;
+}
+
+static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, u32 flags,
+				    const char *buf, size_t count)
 {
 	struct device *dev = &mgr->dev;
 	int ret;
@@ -88,6 +138,68 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
 
 	return 0;
 }
+
+/**
+ * fpga_mgr_buf_load - load fpga from image in buffer
+ * @mgr:	fpga manager
+ * @flags:	flags setting fpga confuration modes
+ * @buf:	buffer contain fpga image
+ * @count:	byte count of buf
+ *
+ * Step the low level fpga manager through the device-specific steps of getting
+ * an FPGA ready to be configured, writing the image to it, then doing whatever
+ * post-configuration steps necessary.  This code assumes the caller got the
+ * mgr pointer from of_fpga_mgr_get() and checked that it is not an error code.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
+		      size_t count)
+{
+	struct page **pages;
+	struct sg_table sgt;
+	const void *p;
+	int nr_pages;
+	int index;
+	int rc;
+
+	if (!mgr->mops->write_init_sg || !mgr->mops->write_sg)
+		return fpga_mgr_buf_load_mapped(mgr, flags, buf, count);
+
+	/*
+	 * Convert the linear kernel pointer into a sg_table of pages for use
+	 * by the driver.
+	 */
+	nr_pages = DIV_ROUND_UP((unsigned long)buf + count, PAGE_SIZE) -
+		   (unsigned long)buf / PAGE_SIZE;
+	pages = kmalloc_array(nr_pages, sizeof(struct page *), GFP_KERNEL);
+	if (!pages)
+		return -ENOMEM;
+
+	p = (buf - offset_in_page(p));
+	for (index = 0; index < nr_pages; index++) {
+		if (is_vmalloc_addr(p))
+			pages[index] = vmalloc_to_page(p);
+		else
+			pages[index] = kmap_to_page((void *)p);
+		p += PAGE_SIZE;
+	}
+
+	/*
+	 * The temporary pages list is used to code share the merging algorithm
+	 * in sg_alloc_table_from_pages
+	 */
+	rc = sg_alloc_table_from_pages(&sgt, pages, index, offset_in_page(buf),
+				       count, GFP_KERNEL);
+	kfree(pages);
+	if (rc)
+		return rc;
+
+	rc = fpga_mgr_buf_load_sg(mgr, flags, &sgt);
+	sg_free_table(&sgt);
+
+	return rc;
+}
 EXPORT_SYMBOL_GPL(fpga_mgr_buf_load);
 
 /**
@@ -256,8 +368,9 @@ int fpga_mgr_register(struct device *dev, const char *name,
 	struct fpga_manager *mgr;
 	int id, ret;
 
-	if (!mops || !mops->write_init || !mops->write ||
-	    !mops->write_complete || !mops->state) {
+	if (!mops || !mops->write_complete || !mops->state ||
+	    ((!mops->write_init || !mops->write) &&
+	     (!mops->write_init_sg || !mops->write_sg))) {
 		dev_err(dev, "Attempt to register without fpga_manager_ops\n");
 		return -EINVAL;
 	}
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 0940bf45e2f2..371b30ea60eb 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -22,6 +22,7 @@
 #define _LINUX_FPGA_MGR_H
 
 struct fpga_manager;
+struct sg_table;
 
 /**
  * enum fpga_mgr_states - fpga framework states
@@ -71,8 +72,11 @@ enum fpga_mgr_states {
 /**
  * struct fpga_manager_ops - ops for low level fpga manager drivers
  * @state: returns an enum value of the FPGA's state
- * @write_init: prepare the FPGA to receive confuration data
+ * @write_init: prepare the FPGA to receive confuration data (linear memory)
  * @write: write count bytes of configuration data to the FPGA
+ * @write_init_sg: prepare the FPGA to receive confuration data (scatter list
+ *                 table)
+ * @write_sg: write count bytes of configuration data to the FPGA
  * @write_complete: set FPGA to operating state after writing is done
  * @fpga_remove: optional: Set FPGA into a specific state during driver remove
  *
@@ -85,6 +89,9 @@ struct fpga_manager_ops {
 	int (*write_init)(struct fpga_manager *mgr, u32 flags,
 			  const char *buf, size_t count);
 	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
+	int (*write_init_sg)(struct fpga_manager *mgr, u32 flags,
+			     struct sg_table *sgt);
+	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
 	int (*write_complete)(struct fpga_manager *mgr, u32 flags);
 	void (*fpga_remove)(struct fpga_manager *mgr);
 };
-- 
2.1.4

[PATCH fpga 7/9] fpga zynq: Use the scatterlist interface

[Jason Gunthorpe]

This allows the driver to avoid a high order coherent DMA allocation
and memory copy. With this patch it can DMA directly from the kernel
pages that the bitfile is stored in.

Since this is now a gather DMA operation the driver uses the ISR
to feed the chips DMA queue with each entry from the SGL.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/fpga/zynq-fpga.c | 194 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 146 insertions(+), 48 deletions(-)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index ac2deae92dbd..559b4f2ab9f6 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -30,6 +30,7 @@
 #include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/string.h>
+#include <linux/scatterlist.h>
 
 /* Offsets into SLCR regmap */
 
@@ -80,6 +81,7 @@
 
 /* FPGA init status */
 #define STATUS_DMA_Q_F			BIT(31)
+#define STATUS_DMA_Q_E			BIT(30)
 #define STATUS_PCFG_INIT_MASK		BIT(4)
 
 /* Interrupt Status/Mask Register Bit definitions */
@@ -98,12 +100,14 @@
 #define DMA_INVALID_ADDRESS		GENMASK(31, 0)
 /* Used to unlock the dev */
 #define UNLOCK_MASK			0x757bdf0d
-/* Timeout for DMA to complete */
-#define DMA_DONE_TIMEOUT		msecs_to_jiffies(1000)
 /* Timeout for polling reset bits */
 #define INIT_POLL_TIMEOUT		2500000
 /* Delay for polling reset bits */
 #define INIT_POLL_DELAY			20
+/* Signal this is the last DMA transfer, wait for the AXI and PCAP before
+ * interrupting
+ */
+#define DMA_SRC_LAST_TRANSFER		1
 
 /* Masks for controlling stuff in SLCR */
 /* Disable all Level shifters */
@@ -124,6 +128,11 @@ struct zynq_fpga_priv {
 	void __iomem *io_base;
 	struct regmap *slcr;
 
+	spinlock_t dma_lock;
+	unsigned int dma_elm;
+	unsigned int dma_nelms;
+	struct scatterlist *cur_sg;
+
 	struct completion dma_done;
 };
 
@@ -149,15 +158,81 @@ static inline void zynq_fpga_set_irq_mask(struct zynq_fpga_priv *priv,
 	zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
 }
 
+/* Must be called with dma_lock held */
+static void zynq_step_dma(struct zynq_fpga_priv *priv)
+{
+	u32 addr;
+	u32 len;
+	bool first;
+
+	first = priv->dma_elm == 0;
+	while (priv->cur_sg) {
+		/* Feed the DMA queue until it is full. */
+		if (zynq_fpga_read(priv, STATUS_OFFSET) & STATUS_DMA_Q_F)
+			break;
+
+		addr = sg_dma_address(priv->cur_sg);
+		len = sg_dma_len(priv->cur_sg);
+		if (priv->dma_elm + 1 == priv->dma_nelms) {
+			/* The last transfer waits for the PCAP to finish too,
+			 * notice this also changes the irq_mask to ignore
+			 * IXR_DMA_DONE_MASK which ensures we do not trigger
+			 * the completion too early.
+			 */
+			addr |= DMA_SRC_LAST_TRANSFER;
+			priv->cur_sg = NULL;
+		} else {
+			priv->cur_sg = sg_next(priv->cur_sg);
+			priv->dma_elm++;
+		}
+
+		zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, addr);
+		zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, DMA_INVALID_ADDRESS);
+		zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, len / 4);
+		zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, 0);
+	}
+
+	/* Once the first transfer is queued we can turn on the ISR, future
+	 * calls to zynq_step_dma will happen from the ISR context. The
+	 * dma_lock spinlock guarentees this handover is done coherently, the
+	 * ISR enable is put at the end to avoid another CPU spinning in the
+	 * ISR on this lock.
+	 */
+	if (first && priv->cur_sg) {
+		zynq_fpga_set_irq_mask(priv, IXR_DMA_DONE_MASK |
+						 IXR_ERROR_FLAGS_MASK);
+	} else if (!priv->cur_sg) {
+		/* The last transfer changes to DMA & PCAP mode since we do
+		 * not want to continue until everything has bee flushed into
+		 * the PCAP.
+		 */
+		zynq_fpga_set_irq_mask(priv, IXR_D_P_DONE_MASK |
+						 IXR_ERROR_FLAGS_MASK);
+	}
+}
+
 static irqreturn_t zynq_fpga_isr(int irq, void *data)
 {
 	struct zynq_fpga_priv *priv = data;
+	u32 intr_status;
 
-	/* disable DMA and error IRQs */
-	zynq_fpga_set_irq_mask(priv, 0);
+	/* If anything other than DMA completion is reported stop and hand
+	 * control back to zynq_fpga_ops_write, something went wrong,
+	 * otherwise progress the DMA.
+	 */
+	spin_lock(&priv->dma_lock);
+	intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
+	if ((intr_status & IXR_ERROR_FLAGS_MASK) == 0 &&
+	    (intr_status & IXR_DMA_DONE_MASK) && priv->cur_sg) {
+		zynq_fpga_write(priv, INT_STS_OFFSET, IXR_DMA_DONE_MASK);
+		zynq_step_dma(priv);
+		spin_unlock(&priv->dma_lock);
+		return IRQ_HANDLED;
+	}
+	spin_unlock(&priv->dma_lock);
 
+	zynq_fpga_set_irq_mask(priv, 0);
 	complete(&priv->dma_done);
-
 	return IRQ_HANDLED;
 }
 
@@ -165,31 +240,47 @@ static irqreturn_t zynq_fpga_isr(int irq, void *data)
  * the correct byte order. The input is a Xilinx .bin file with every 32 bit
  * quantity swapped.
  */
-static bool zynq_fpga_has_sync(const char *buf, size_t count)
+static bool zynq_fpga_has_sync(struct sg_table *sgt)
 {
-	for (; count > 4; buf += 4, count -= 4)
-		if (buf[0] == 0x66 && buf[1] == 0x55 && buf[2] == 0x99 &&
-		    buf[3] == 0xaa)
-			return true;
+	struct sg_mapping_iter miter;
+	const u8 *buf, *end;
+
+	sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
+
+	while (sg_miter_next(&miter)) {
+		end = miter.addr + miter.length;
+		for (buf = miter.addr; buf < end; buf += 4) {
+			if (buf[0] == 0x66 && buf[1] == 0x55 &&
+			    buf[2] == 0x99 && buf[3] == 0xaa) {
+				sg_miter_stop(&miter);
+				return true;
+			}
+		}
+	}
+
+	sg_miter_stop(&miter);
 	return false;
 }
 
 static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
-				    const char *buf, size_t count)
+				    struct sg_table *sgt)
 {
 	struct zynq_fpga_priv *priv;
+	struct scatterlist *sg;
 	u32 ctrl, status;
-	int err;
+	int err, i;
 
 	priv = mgr->priv;
 
-	/* The hardware can only DMA multiples of 4 bytes, and we need at
-	 * least the sync word and something else to do anything.
+	/* The hardware can only DMA multiples of 4 bytes, and it requires the
+	 * starting address to be aligned to 64 bits (UG585 pg 212).
 	 */
-	if (count <= 4 || (count % 4) != 0) {
-		dev_err(&mgr->dev,
-			"Invalid bitstream size, must be multiples of 4 bytes\n");
-		return -EINVAL;
+	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
+		if ((sg->offset % 8) != 0 || (sg->length % 4) != 0) {
+			dev_err(&mgr->dev,
+			    "Invalid bitstream size, chunks must be aligned\n");
+			return -EINVAL;
+		}
 	}
 
 	err = clk_enable(priv->clk);
@@ -198,7 +289,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 
 	/* don't globally reset PL if we're doing partial reconfig */
 	if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
-		if (!zynq_fpga_has_sync(buf, count)) {
+		if (!zynq_fpga_has_sync(sgt)) {
 			dev_err(&mgr->dev,
 				"Invalid bitstream, could not find a sync word. Bitstream must be a byte swaped .bin file\n");
 			err = -EINVAL;
@@ -274,10 +365,11 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
 	zynq_fpga_write(priv, CTRL_OFFSET,
 			(CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK | ctrl));
 
-	/* check that we have room in the command queue */
+	/* We expect that the command queue is empty right now. */
 	status = zynq_fpga_read(priv, STATUS_OFFSET);
-	if (status & STATUS_DMA_Q_F) {
-		dev_err(&mgr->dev, "DMA command queue full\n");
+	if ((status & STATUS_DMA_Q_F) ||
+	    (status & STATUS_DMA_Q_E) != STATUS_DMA_Q_E) {
+		dev_err(&mgr->dev, "DMA command queue not right\n");
 		err = -EBUSY;
 		goto out_err;
 	}
@@ -296,49 +388,50 @@ out_err:
 	return err;
 }
 
-static int zynq_fpga_ops_write(struct fpga_manager *mgr,
-			       const char *buf, size_t count)
+static int zynq_fpga_ops_write(struct fpga_manager *mgr, struct sg_table *sgt)
 {
 	struct zynq_fpga_priv *priv;
 	const char *why;
 	int err;
-	char *kbuf;
-	dma_addr_t dma_addr;
 	u32 intr_status;
+	unsigned long timeout;
+	unsigned long flags;
 
 	priv = mgr->priv;
 
-	kbuf =
-	    dma_alloc_coherent(mgr->dev.parent, count, &dma_addr, GFP_KERNEL);
-	if (!kbuf)
+	priv->dma_nelms =
+	    dma_map_sg(mgr->dev.parent, sgt->sgl, sgt->nents, DMA_TO_DEVICE);
+	if (priv->dma_nelms == 0)
 		return -ENOMEM;
 
-	memcpy(kbuf, buf, count);
-
 	/* enable clock */
 	err = clk_enable(priv->clk);
 	if (err)
 		goto out_free;
 
 	zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
-
 	reinit_completion(&priv->dma_done);
 
-	/* enable DMA and error IRQs */
-	zynq_fpga_set_irq_mask(priv, IXR_D_P_DONE_MASK | IXR_ERROR_FLAGS_MASK);
+	/* zynq_step_dma will turn on interrupts */
+	spin_lock_irqsave(&priv->dma_lock, flags);
+	priv->dma_elm = 0;
+	priv->cur_sg = sgt->sgl;
+	zynq_step_dma(priv);
+	spin_unlock_irqrestore(&priv->dma_lock, flags);
 
-	/* the +1 in the src addr is used to hold off on DMA_DONE IRQ
-	 * until both AXI and PCAP are done ...
-	 */
-	zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, (u32)(dma_addr) + 1);
-	zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, (u32)DMA_INVALID_ADDRESS);
-	zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, count / 4);
-	zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, 0);
+	timeout = wait_for_completion_timeout(&priv->dma_done,
+					      msecs_to_jiffies(5 * 1000));
 
-	wait_for_completion(&priv->dma_done);
+	zynq_fpga_set_irq_mask(priv, 0);
 
 	intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
-	zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
+	zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
+
+	/* There doesn't seem to be a way to force cancel any DMA, so if
+	 * something went wrong we are relying on the hardware to have halted
+	 * the DMA before we get here, if there was we could use
+	 * wait_for_completion_interruptible too.
+	 */
 
 	if (intr_status & IXR_ERROR_FLAGS_MASK) {
 		why = "DMA reported error";
@@ -346,8 +439,12 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
 		goto out_report;
 	}
 
-	if (!((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)) {
-		why = "DMA did not complete";
+	if (priv->cur_sg ||
+	    !((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)) {
+		if (timeout == 0)
+			why = "DMA timed out";
+		else
+			why = "DMA did not complete";
 		err = -EIO;
 		goto out_report;
 	}
@@ -368,7 +465,7 @@ out_report:
 out_clk:
 	clk_disable(priv->clk);
 out_free:
-	dma_free_coherent(mgr->dev.parent, count, kbuf, dma_addr);
+	dma_unmap_sg(mgr->dev.parent, sgt->sgl, sgt->nents, DMA_TO_DEVICE);
 	return err;
 }
 
@@ -429,8 +526,8 @@ static enum fpga_mgr_states zynq_fpga_ops_state(struct fpga_manager *mgr)
 
 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_init_sg = zynq_fpga_ops_write_init,
+	.write_sg = zynq_fpga_ops_write,
 	.write_complete = zynq_fpga_ops_write_complete,
 };
 
@@ -444,6 +541,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
+	spin_lock_init(&priv->dma_lock);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	priv->io_base = devm_ioremap_resource(dev, res);
-- 
2.1.4

[PATCH fpga 8/9] fpga socfpga: Use the scatterlist interface

[Jason Gunthorpe]

socfpga just uses the CPU to memory copy the bitstream, so there is
no reason it needs contiguous kernel memory. Switch to use the sg
interface.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/fpga/socfpga.c | 56 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
index 27d2ff28132c..f3f390b2eecf 100644
--- a/drivers/fpga/socfpga.c
+++ b/drivers/fpga/socfpga.c
@@ -24,6 +24,7 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/pm.h>
+#include <linux/scatterlist.h>
 
 /* Register offsets */
 #define SOCFPGA_FPGMGR_STAT_OFST				0x0
@@ -408,10 +409,22 @@ static int socfpga_fpga_reset(struct fpga_manager *mgr)
  * Prepare the FPGA to receive the configuration data.
  */
 static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags,
-					   const char *buf, size_t count)
+					   struct sg_table *sgt)
 {
 	struct socfpga_fpga_priv *priv = mgr->priv;
-	int ret;
+	struct scatterlist *sg;
+	int ret, i;
+
+	/* We use the CPU to read the bitstream 32 bits at a time, and thus
+	 * require alignment.
+	 */
+	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
+		if ((sg->offset % 4) != 0) {
+			dev_err(&mgr->dev,
+				"Invalid bitstream, chunks must be aligned\n");
+			return -EINVAL;
+		}
+	}
 
 	if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
 		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
@@ -440,40 +453,45 @@ static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags,
 /*
  * Step 9: write data to the FPGA data register
  */
-static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr,
-					    const char *buf, size_t count)
+static void socfpga_write_buf(struct socfpga_fpga_priv *priv, const u32 *buf,
+			      size_t count)
 {
-	struct socfpga_fpga_priv *priv = mgr->priv;
-	u32 *buffer_32 = (u32 *)buf;
 	size_t i = 0;
 
-	if (count <= 0)
-		return -EINVAL;
-
 	/* Write out the complete 32-bit chunks. */
 	while (count >= sizeof(u32)) {
-		socfpga_fpga_data_writel(priv, buffer_32[i++]);
+		socfpga_fpga_data_writel(priv, buf[i++]);
 		count -= sizeof(u32);
 	}
 
 	/* Write out remaining non 32-bit chunks. */
 	switch (count) {
 	case 3:
-		socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x00ffffff);
+		socfpga_fpga_data_writel(priv, buf[i++] & 0x00ffffff);
 		break;
 	case 2:
-		socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x0000ffff);
+		socfpga_fpga_data_writel(priv, buf[i++] & 0x0000ffff);
 		break;
 	case 1:
-		socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x000000ff);
-		break;
-	case 0:
+		socfpga_fpga_data_writel(priv, buf[i++] & 0x000000ff);
 		break;
 	default:
-		/* This will never happen. */
-		return -EFAULT;
+		break;
 	}
+}
+
+static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr,
+					    struct sg_table *sgt)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+	struct sg_mapping_iter miter;
+
+	sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
+
+	while (sg_miter_next(&miter))
+		socfpga_write_buf(priv, miter.addr, miter.length);
 
+	sg_miter_stop(&miter);
 	return 0;
 }
 
@@ -545,8 +563,8 @@ static enum fpga_mgr_states socfpga_fpga_ops_state(struct fpga_manager *mgr)
 
 static const struct fpga_manager_ops socfpga_fpga_ops = {
 	.state = socfpga_fpga_ops_state,
-	.write_init = socfpga_fpga_ops_configure_init,
-	.write = socfpga_fpga_ops_configure_write,
+	.write_init_sg = socfpga_fpga_ops_configure_init,
+	.write_sg = socfpga_fpga_ops_configure_write,
 	.write_complete = socfpga_fpga_ops_configure_complete,
 };
 
-- 
2.1.4

[PATCH fpga 9/9] fpga: Remove support for non-sg drivers

[Jason Gunthorpe]

All drivers now use the sg interface so there is no reason to keep
the contiguous interface any more.

Now that all drivers support this interface we can also export it.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/fpga/fpga-mgr.c       | 62 +++++++------------------------------------
 include/linux/fpga/fpga-mgr.h |  7 ++---
 2 files changed, 11 insertions(+), 58 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index c2491ffeabd3..4ba22925d9d5 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -47,8 +47,8 @@ static struct class *fpga_mgr_class;
  *
  * Return: 0 on success, negative error code otherwise.
  */
-static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags,
-				struct sg_table *sgt)
+int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags,
+			 struct sg_table *sgt)
 {
 	struct device *dev = &mgr->dev;
 	int ret;
@@ -92,52 +92,7 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags,
 
 	return 0;
 }
-
-static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, u32 flags,
-				    const char *buf, size_t count)
-{
-	struct device *dev = &mgr->dev;
-	int ret;
-
-	/*
-	 * Call the low level driver's write_init function.  This will do the
-	 * device-specific things to get the FPGA into the state where it is
-	 * ready to receive an FPGA image.
-	 */
-	mgr->state = FPGA_MGR_STATE_WRITE_INIT;
-	ret = mgr->mops->write_init(mgr, flags, buf, count);
-	if (ret) {
-		dev_err(dev, "Error preparing FPGA for writing\n");
-		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
-		return ret;
-	}
-
-	/*
-	 * Write the FPGA image to the FPGA.
-	 */
-	mgr->state = FPGA_MGR_STATE_WRITE;
-	ret = mgr->mops->write(mgr, buf, count);
-	if (ret) {
-		dev_err(dev, "Error while writing image data to FPGA\n");
-		mgr->state = FPGA_MGR_STATE_WRITE_ERR;
-		return ret;
-	}
-
-	/*
-	 * After all the FPGA image has been written, do the device specific
-	 * steps to finish and set the FPGA into operating mode.
-	 */
-	mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
-	ret = mgr->mops->write_complete(mgr, flags);
-	if (ret) {
-		dev_err(dev, "Error after writing image data to FPGA\n");
-		mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
-		return ret;
-	}
-	mgr->state = FPGA_MGR_STATE_OPERATING;
-
-	return 0;
-}
+EXPORT_SYMBOL_GPL(fpga_mgr_buf_load_sg);
 
 /**
  * fpga_mgr_buf_load - load fpga from image in buffer
@@ -163,9 +118,6 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
 	int index;
 	int rc;
 
-	if (!mgr->mops->write_init_sg || !mgr->mops->write_sg)
-		return fpga_mgr_buf_load_mapped(mgr, flags, buf, count);
-
 	/*
 	 * Convert the linear kernel pointer into a sg_table of pages for use
 	 * by the driver.
@@ -226,6 +178,11 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
 
 	mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
 
+	/*
+	 * FIXME: We do not need a vmap, just a page list, but
+	 * request_firmware has no way to give us that, so this needlessly
+	 * consumes vmalloc space.
+	 */
 	ret = request_firmware(&fw, image_name, dev);
 	if (ret) {
 		mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
@@ -369,8 +326,7 @@ int fpga_mgr_register(struct device *dev, const char *name,
 	int id, ret;
 
 	if (!mops || !mops->write_complete || !mops->state ||
-	    ((!mops->write_init || !mops->write) &&
-	     (!mops->write_init_sg || !mops->write_sg))) {
+	    !mops->write_init_sg || !mops->write_sg) {
 		dev_err(dev, "Attempt to register without fpga_manager_ops\n");
 		return -EINVAL;
 	}
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 371b30ea60eb..5c698c8fe71b 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -72,8 +72,6 @@ enum fpga_mgr_states {
 /**
  * struct fpga_manager_ops - ops for low level fpga manager drivers
  * @state: returns an enum value of the FPGA's state
- * @write_init: prepare the FPGA to receive confuration data (linear memory)
- * @write: write count bytes of configuration data to the FPGA
  * @write_init_sg: prepare the FPGA to receive confuration data (scatter list
  *                 table)
  * @write_sg: write count bytes of configuration data to the FPGA
@@ -86,9 +84,6 @@ enum fpga_mgr_states {
  */
 struct fpga_manager_ops {
 	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
-	int (*write_init)(struct fpga_manager *mgr, u32 flags,
-			  const char *buf, size_t count);
-	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
 	int (*write_init_sg)(struct fpga_manager *mgr, u32 flags,
 			     struct sg_table *sgt);
 	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
@@ -118,6 +113,8 @@ struct fpga_manager {
 
 int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags,
 		      const char *buf, size_t count);
+int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags,
+			 struct sg_table *sgt);
 
 int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
 			   const char *image_name);
-- 
2.1.4

[PATCH fpga 2/9] fpga zynq: Check the bitstream for validity

[Joshua Clayton]

Hi Jason,

Minor comment below:

On 11/09/2016 02:58 PM, Jason Gunthorpe wrote:
> @@ -184,12 +197,28 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>  
>  	priv = mgr->priv;
>  
> +	/* The hardware can only DMA multiples of 4 bytes, and we need at
> +	 * least the sync word and something else to do anything.
> +	 */
> +	if (count <= 4 || (count % 4) != 0) {
> +		dev_err(priv->dev,
> +			"Invalid bitstream size, must be multiples of 4 bytes\n");
> +		return -EINVAL;
> +	}
> +
>  	err = clk_enable(priv->clk);
>  	if (err)
>  		return err;
>  
>  	/* don't globally reset PL if we're doing partial reconfig */
>  	if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> +		if (!zynq_fpga_has_sync(buf, count)) {
> +			dev_err(priv->dev,
> +				"Invalid bitstream, could not find a sync word. Bitstream must be a byte swaped .bin file\n");
Nitpick: byte swaped is a misspelling... and I'm not sure I like the second half of this message.
Maybe something like  "Bitstream must be lsb first" (if that is what is meant).
> +			err = -EINVAL;
> +			goto out_err;
> +		}
> +
>  		/* assert AXI interface resets */
>  		regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
>  			     FPGA_RST_ALL_MASK);
>
Thanks,

Joshua

[PATCH fpga 2/9] fpga zynq: Check the bitstream for validity

[Jason Gunthorpe]

On Wed, Nov 09, 2016 at 04:04:07PM -0800, Joshua Clayton wrote:

> > +		if (!zynq_fpga_has_sync(buf, count)) {
> > +			dev_err(priv->dev,
> > +				"Invalid bitstream, could not find a sync word. Bitstream must be a byte swaped .bin file\n");
> Nitpick: byte swaped is a misspelling...

oops, sure.

> and I'm not sure I like the second half of this message.  Maybe
> something like "Bitstream must be lsb first" (if that is what is
> meant).

I intended it to be a recipe to follow, not a specification. It
literally means take the .bin file and dword byte swap it to make it
work with the driver.

Describing it as LSB first would be consistent with UG470, but I doubt
many people know Xilinx tools emit the bitstream in 32 bit MSB format
and probably even fewer people know that the relavent difference
between .bit and .bin boils down to the alignment of the sync word -
which is why I prefered the recipe style..

Jason

[PATCH fpga 9/9] fpga: Remove support for non-sg drivers

[Joshua Clayton]

Hi Jason,

On 11/09/2016 02:58 PM, Jason Gunthorpe wrote:
> All drivers now use the sg interface so there is no reason to keep
> the contiguous interface any more.
>
> Now that all drivers support this interface we can also export it.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/fpga/fpga-mgr.c       | 62 +++++++------------------------------------
>  include/linux/fpga/fpga-mgr.h |  7 ++---
>  2 files changed, 11 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index c2491ffeabd3..4ba22925d9d5 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -47,8 +47,8 @@ static struct class *fpga_mgr_class;
>   *
>   * Return: 0 on success, negative error code otherwise.
>   */
> -static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags,
> -				struct sg_table *sgt)
> +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags,
> +			 struct sg_table *sgt)
>  {
>  	struct device *dev = &mgr->dev;
>  	int ret;
> @@ -92,52 +92,7 @@ static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags,
>  
>  	return 0;
>  }
> -
> -static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr, u32 flags,
> -				    const char *buf, size_t count)
> -{
> -	struct device *dev = &mgr->dev;
> -	int ret;
> -
> -	/*
> -	 * Call the low level driver's write_init function.  This will do the
> -	 * device-specific things to get the FPGA into the state where it is
> -	 * ready to receive an FPGA image.
> -	 */
> -	mgr->state = FPGA_MGR_STATE_WRITE_INIT;
> -	ret = mgr->mops->write_init(mgr, flags, buf, count);
> -	if (ret) {
> -		dev_err(dev, "Error preparing FPGA for writing\n");
> -		mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> -		return ret;
> -	}
> -
> -	/*
> -	 * Write the FPGA image to the FPGA.
> -	 */
> -	mgr->state = FPGA_MGR_STATE_WRITE;
> -	ret = mgr->mops->write(mgr, buf, count);
> -	if (ret) {
> -		dev_err(dev, "Error while writing image data to FPGA\n");
> -		mgr->state = FPGA_MGR_STATE_WRITE_ERR;
> -		return ret;
> -	}
> -
> -	/*
> -	 * After all the FPGA image has been written, do the device specific
> -	 * steps to finish and set the FPGA into operating mode.
> -	 */
> -	mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
> -	ret = mgr->mops->write_complete(mgr, flags);
> -	if (ret) {
> -		dev_err(dev, "Error after writing image data to FPGA\n");
> -		mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
> -		return ret;
> -	}
> -	mgr->state = FPGA_MGR_STATE_OPERATING;
> -
> -	return 0;
> -}
> +EXPORT_SYMBOL_GPL(fpga_mgr_buf_load_sg);
>  
>  /**
>   * fpga_mgr_buf_load - load fpga from image in buffer
> @@ -163,9 +118,6 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
>  	int index;
>  	int rc;
>  
> -	if (!mgr->mops->write_init_sg || !mgr->mops->write_sg)
> -		return fpga_mgr_buf_load_mapped(mgr, flags, buf, count);
> -
>  	/*
>  	 * Convert the linear kernel pointer into a sg_table of pages for use
>  	 * by the driver.
> @@ -226,6 +178,11 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
>  
>  	mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
>  
> +	/*
> +	 * FIXME: We do not need a vmap, just a page list, but
> +	 * request_firmware has no way to give us that, so this needlessly
> +	 * consumes vmalloc space.
> +	 */
>  	ret = request_firmware(&fw, image_name, dev);
>  	if (ret) {
>  		mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
> @@ -369,8 +326,7 @@ int fpga_mgr_register(struct device *dev, const char *name,
>  	int id, ret;
>  
>  	if (!mops || !mops->write_complete || !mops->state ||
> -	    ((!mops->write_init || !mops->write) &&
> -	     (!mops->write_init_sg || !mops->write_sg))) {
> +	    !mops->write_init_sg || !mops->write_sg) {
>  		dev_err(dev, "Attempt to register without fpga_manager_ops\n");
>  		return -EINVAL;
>  	}
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 371b30ea60eb..5c698c8fe71b 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -72,8 +72,6 @@ enum fpga_mgr_states {
>  /**
>   * struct fpga_manager_ops - ops for low level fpga manager drivers
>   * @state: returns an enum value of the FPGA's state
> - * @write_init: prepare the FPGA to receive confuration data (linear memory)
> - * @write: write count bytes of configuration data to the FPGA
>   * @write_init_sg: prepare the FPGA to receive confuration data (scatter list
>   *                 table)
>   * @write_sg: write count bytes of configuration data to the FPGA
> @@ -86,9 +84,6 @@ enum fpga_mgr_states {
>   */
>  struct fpga_manager_ops {
>  	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
> -	int (*write_init)(struct fpga_manager *mgr, u32 flags,
> -			  const char *buf, size_t count);
> -	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
>  	int (*write_init_sg)(struct fpga_manager *mgr, u32 flags,
>  			     struct sg_table *sgt);
>  	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
> @@ -118,6 +113,8 @@ struct fpga_manager {
>  
>  int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags,
>  		      const char *buf, size_t count);
> +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags,
> +			 struct sg_table *sgt);
>  
>  int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
>  			   const char *image_name);
I don't have any feeling either way about switching to scatter-gather.
(Not zynq or socfpga user)
But I do object to renaming the API.
write_init() and write() do not imply a particular implementation, nor even that
the buffer is coherent.

I am working to merge an fpga manager which uses SPI to load the bitstream
(see https://www.spinics.net/lists/arm-kernel/msg539328.html)
Any dma in use there would come from the spi driver. write_init_sg, and write_sg
don't make any sense in my case.

Would it not make sense to keep the top level API the same?

Thanks, Joshua.

[PATCH fpga 9/9] fpga: Remove support for non-sg drivers

[Jason Gunthorpe]

> >  struct fpga_manager_ops {
> >  	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
> > -	int (*write_init)(struct fpga_manager *mgr, u32 flags,
> > -			  const char *buf, size_t count);
> > -	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
> >  	int (*write_init_sg)(struct fpga_manager *mgr, u32 flags,
> >  			     struct sg_table *sgt);
> >  	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
> > @@ -118,6 +113,8 @@ struct fpga_manager {
> >  
> >  int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags,
> >  		      const char *buf, size_t count);
> > +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags,
> > +			 struct sg_table *sgt);
> >  
> >  int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
> >  			   const char *image_name);
> I don't have any feeling either way about switching to scatter-gather.
> (Not zynq or socfpga user)
> But I do object to renaming the API.
> write_init() and write() do not imply a particular implementation, nor even that
> the buffer is coherent.

Neither the sg or old linear interface imply any particular
underlying driver implementation.

All that is being changed is how the list of physical pages gets
passed to the driver. The linear interface requires them to be
contiguously mapped (eg in a vmap) while the SG interface
directly passes a list of physical page addresses.

Any alogrithm that works with the old interface can run on the new
interface, and the new interface can support much better options for
DMA drivers, while not requiring the higher layers to perform a high
order allocation (vmap or otherwise) to create the contiguous memory.

The reason the old interface is being deleted here is so the fpga mgr
API can be expanded to accept a sg list directly. Since we cannot
convert a general sg list to linear memory the liner option must be
totally removed.

> I am working to merge an fpga manager which uses SPI to load the bitstream
> (see https://www.spinics.net/lists/arm-kernel/msg539328.html)
> Any dma in use there would come from the spi driver. write_init_sg, and write_sg
> don't make any sense in my case.

No, it still make lots of sense.

SPI has been slowly transforming to use the same sort of SG scheme
universally, including facing the client. (see
6ad45a27cbe343ec8d7888e5edf6335499a4b555)

Some day your driver can just pass the SGs directly to spi and
everything will be great.

In the mean time it can do sg_miter_next to get mapped buffers.

> Would it not make sense to keep the top level API the same?

Fundamentally no.

Jason

[PATCH fpga 9/9] fpga: Remove support for non-sg drivers

[Joshua Clayton]

On 11/10/2016 08:33 AM, Jason Gunthorpe wrote:
>>>  struct fpga_manager_ops {
>>>  	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
>>> -	int (*write_init)(struct fpga_manager *mgr, u32 flags,
>>> -			  const char *buf, size_t count);
>>> -	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
>>>  	int (*write_init_sg)(struct fpga_manager *mgr, u32 flags,
>>>  			     struct sg_table *sgt);
>>>  	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
>>> @@ -118,6 +113,8 @@ struct fpga_manager {
>>>  
>>>  int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags,
>>>  		      const char *buf, size_t count);
>>> +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags,
>>> +			 struct sg_table *sgt);
>>>  
>>>  int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
>>>  			   const char *image_name);
>> I don't have any feeling either way about switching to scatter-gather.
>> (Not zynq or socfpga user)
>> But I do object to renaming the API.
>> write_init() and write() do not imply a particular implementation, nor even that
>> the buffer is coherent.
> Neither the sg or old linear interface imply any particular
> underlying driver implementation.
>
> All that is being changed is how the list of physical pages gets
> passed to the driver. The linear interface requires them to be
> contiguously mapped (eg in a vmap) while the SG interface
> directly passes a list of physical page addresses.
>
> Any alogrithm that works with the old interface can run on the new
> interface, and the new interface can support much better options for
> DMA drivers, while not requiring the higher layers to perform a high
> order allocation (vmap or otherwise) to create the contiguous memory.
>
> The reason the old interface is being deleted here is so the fpga mgr
> API can be expanded to accept a sg list directly. Since we cannot
> convert a general sg list to linear memory the liner option must be
> totally removed.
OK. That sounds reasonable.
>> I am working to merge an fpga manager which uses SPI to load the bitstream
>> (see https://www.spinics.net/lists/arm-kernel/msg539328.html)
>> Any dma in use there would come from the spi driver. write_init_sg, and write_sg
>> don't make any sense in my case.
> No, it still make lots of sense.
>
> SPI has been slowly transforming to use the same sort of SG scheme
> universally, including facing the client. (see
> 6ad45a27cbe343ec8d7888e5edf6335499a4b555)
Thanks for sharing that link.
>
> Some day your driver can just pass the SGs directly to spi and
> everything will be great.
>
> In the mean time it can do sg_miter_next to get mapped buffers.
..so I have a way forward if this series gets merged.
That was my main concern.
And as a dma n00b, learning to use dma engine structures to do
non-dma xfer was not very high on my list.
>> Would it not make sense to keep the top level API the same?
> Fundamentally no.
>
> Jason
Joshua

[PATCH fpga 3/9] fpga zynq: Fix incorrect ISR state on bootup

[Moritz Fischer]

Hi Jason,

On Wed, Nov 9, 2016 at 2:58 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> It is best practice to clear and mask all interrupts before
> associating the IRQ, and this should be done after the clock
> is enabled.
>
> This corrects a bad result from zynq_fpga_ops_state on bootup
> where left over latched values in INT_STS_OFFSET caused it to
> report an unconfigured FPGA as configured.
>
> After this change the boot up operating state for an unconfigured
> FPGA reports 'unknown'.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/fpga/zynq-fpga.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index 86f4377e2b52..40cf0feaca7c 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -460,13 +460,6 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>                 return priv->irq;
>         }
>
> -       err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0,
> -                              dev_name(dev), priv);
> -       if (err) {
> -               dev_err(dev, "unable to request IRQ\n");
> -               return err;
> -       }
> -
>         priv->clk = devm_clk_get(dev, "ref_clk");
>         if (IS_ERR(priv->clk)) {
>                 dev_err(dev, "input clock not found\n");
> @@ -482,6 +475,16 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>         /* unlock the device */
>         zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
>
> +       zynq_fpga_write(priv, INT_MASK_OFFSET, 0xFFFFFFFF);

Named constant please.

> +       zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
> +       err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0, dev_name(dev),
> +                              priv);
> +       if (err) {
> +               dev_err(dev, "unable to request IRQ\n");
> +               clk_unprepare(priv->clk);

Your enable count is off in that case. This should be a clk_disable_unprepare()
call.

<snip>

clk_prepare_enable()

if(err) {
    clk_unprepare();
    return err; /* enable count = 1? <- huh? */
}

clk_disable() enable count = 0
clk_unprepare() prepare count = 0

</snip>

> +               return err;
> +       }
> +
>         clk_disable(priv->clk);
>
>         err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
> --
> 2.1.4
>

Thanks,

Moritz

[PATCH fpga 3/9] fpga zynq: Fix incorrect ISR state on bootup

[Jason Gunthorpe]

On Thu, Nov 10, 2016 at 04:44:39PM -0800, Moritz Fischer wrote:
> > +       zynq_fpga_write(priv, INT_MASK_OFFSET, 0xFFFFFFFF);
> 
> Named constant please.

This line gets fixed in the next patch that, lets just leave it, less
churn..

> > +       zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
> > +       err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0, dev_name(dev),
> > +                              priv);
> > +       if (err) {
> > +               dev_err(dev, "unable to request IRQ\n");
> > +               clk_unprepare(priv->clk);
> 
> Your enable count is off in that case. This should be a clk_disable_unprepare()
> call.

Yep.

FWIW, it feels wrong to leave the ISR enabled but the clk
disabled..

Jason

[PATCH fpga 9/9] fpga: Remove support for non-sg drivers

[atull]

On Thu, 10 Nov 2016, Joshua Clayton wrote:

> 
> 
> On 11/10/2016 08:33 AM, Jason Gunthorpe wrote:
> >>>  struct fpga_manager_ops {
> >>>  	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
> >>> -	int (*write_init)(struct fpga_manager *mgr, u32 flags,
> >>> -			  const char *buf, size_t count);
> >>> -	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
> >>>  	int (*write_init_sg)(struct fpga_manager *mgr, u32 flags,
> >>>  			     struct sg_table *sgt);
> >>>  	int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
> >>> @@ -118,6 +113,8 @@ struct fpga_manager {
> >>>  
> >>>  int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags,
> >>>  		      const char *buf, size_t count);
> >>> +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, u32 flags,
> >>> +			 struct sg_table *sgt);
> >>>  
> >>>  int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
> >>>  			   const char *image_name);
> >> I don't have any feeling either way about switching to scatter-gather.
> >> (Not zynq or socfpga user)
> >> But I do object to renaming the API.
> >> write_init() and write() do not imply a particular implementation, nor even that
> >> the buffer is coherent.
> > Neither the sg or old linear interface imply any particular
> > underlying driver implementation.
> >
> > All that is being changed is how the list of physical pages gets
> > passed to the driver. The linear interface requires them to be
> > contiguously mapped (eg in a vmap) while the SG interface
> > directly passes a list of physical page addresses.
> >
> > Any alogrithm that works with the old interface can run on the new
> > interface, and the new interface can support much better options for
> > DMA drivers, while not requiring the higher layers to perform a high
> > order allocation (vmap or otherwise) to create the contiguous memory.
> >
> > The reason the old interface is being deleted here is so the fpga mgr
> > API can be expanded to accept a sg list directly. Since we cannot
> > convert a general sg list to linear memory the liner option must be
> > totally removed.
> OK. That sounds reasonable.
> >> I am working to merge an fpga manager which uses SPI to load the bitstream
> >> (see https://www.spinics.net/lists/arm-kernel/msg539328.html)
> >> Any dma in use there would come from the spi driver. write_init_sg, and write_sg
> >> don't make any sense in my case.
> > No, it still make lots of sense.
> >
> > SPI has been slowly transforming to use the same sort of SG scheme
> > universally, including facing the client. (see
> > 6ad45a27cbe343ec8d7888e5edf6335499a4b555)
> Thanks for sharing that link.
> >
> > Some day your driver can just pass the SGs directly to spi and
> > everything will be great.
> >
> > In the mean time it can do sg_miter_next to get mapped buffers.
> ..so I have a way forward if this series gets merged.
> That was my main concern.
> And as a dma n00b, learning to use dma engine structures to do
> non-dma xfer was not very high on my list.
> >> Would it not make sense to keep the top level API the same?
> > Fundamentally no.

NACK for now.  Let's slow down here a bit.

I don't see any rush in getting rid of the contiguous
buffer interface.

At the very least, my socfpga-a10 driver is using the old
interface and has been just applied.  And currently your
socfpga changes are untested whereas my original driver
has been in use and is known to work.

Let's wait on that patch until we have quite a few FPGA's
supported and can be sure that we won't miss the old
interface.

Alan

> >
> > Jason
> Joshua
> 

[PATCH fpga 9/9] fpga: Remove support for non-sg drivers

[Jason Gunthorpe]

On Sun, Nov 13, 2016 at 02:44:51PM -0600, atull wrote:

> > >> Would it not make sense to keep the top level API the same?
> > > Fundamentally no.
> 
> NACK for now.  Let's slow down here a bit.
> 
> I don't see any rush in getting rid of the contiguous
> buffer interface.

As I explained to Joshua, until all the drivers support sg the public
sg interface can not be sensibly added, so it actually is a hard
blocker to making progress.

Further, the longer things are left, the more code will depend on the
broken interface and the harder it will be to ultimately fix. This is
a good time because there are only 2 upstream drivers, and I can test
one, you can test the other :)

> At the very least, my socfpga-a10 driver is using the old
> interface and has been just applied.  And currently your
> socfpga changes are untested whereas my original driver
> has been in use and is known to work.

Well, looks like a10 uses the same write code as socfpga, so the patch
should trivially port over. Let me know if you need help.

Are you able to test the modified socfpga?

> Let's wait on that patch until we have quite a few FPGA's
> supported and can be sure that we won't miss the old
> interface.

Well, I am sure :) There is no reason to need this contiguous virtual
memory at the driver level. Our existing drivers demonstrate both how
to do DMA and PIO from the SG list, there really are no other transfer
modes supported by the kernel ...

But even if you are not sure, keeping the unused dead API around is
the exact opposite of the accepted kernel process. Read
stable-api-nonsense.txt and understand how that applies here.

The API can be revised again if a driver comes along that needs an
improvement, but given how intrinsically broken huge contiguous
allocations are, I can't forsee any situation where returning to this
interface would be a good idea.

Do you have comments on the other patches?

Jason

[PATCH fpga 8/9] fpga socfpga: Use the scatterlist interface

[atull]

On Wed, 9 Nov 2016, Jason Gunthorpe wrote:

> socfpga just uses the CPU to memory copy the bitstream, so there is
> no reason it needs contiguous kernel memory. Switch to use the sg
> interface.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/fpga/socfpga.c | 56 +++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 37 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> index 27d2ff28132c..f3f390b2eecf 100644
> --- a/drivers/fpga/socfpga.c
> +++ b/drivers/fpga/socfpga.c
> @@ -24,6 +24,7 @@
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
>  #include <linux/pm.h>
> +#include <linux/scatterlist.h>
>  
>  /* Register offsets */
>  #define SOCFPGA_FPGMGR_STAT_OFST				0x0
> @@ -408,10 +409,22 @@ static int socfpga_fpga_reset(struct fpga_manager *mgr)
>   * Prepare the FPGA to receive the configuration data.
>   */
>  static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags,
> -					   const char *buf, size_t count)
> +					   struct sg_table *sgt)
>  {
>  	struct socfpga_fpga_priv *priv = mgr->priv;
> -	int ret;
> +	struct scatterlist *sg;
> +	int ret, i;
> +
> +	/* We use the CPU to read the bitstream 32 bits at a time, and thus
> +	 * require alignment.
> +	 */
> +	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
> +		if ((sg->offset % 4) != 0) {
> +			dev_err(&mgr->dev,
> +				"Invalid bitstream, chunks must be aligned\n");
> +			return -EINVAL;
> +		}
> +	}
>  
>  	if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
>  		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
> @@ -440,40 +453,45 @@ static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags,
>  /*
>   * Step 9: write data to the FPGA data register
>   */
> -static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr,
> -					    const char *buf, size_t count)
> +static void socfpga_write_buf(struct socfpga_fpga_priv *priv, const u32 *buf,
> +			      size_t count)
>  {
> -	struct socfpga_fpga_priv *priv = mgr->priv;
> -	u32 *buffer_32 = (u32 *)buf;
>  	size_t i = 0;
>  
> -	if (count <= 0)
> -		return -EINVAL;
> -
>  	/* Write out the complete 32-bit chunks. */
>  	while (count >= sizeof(u32)) {
> -		socfpga_fpga_data_writel(priv, buffer_32[i++]);
> +		socfpga_fpga_data_writel(priv, buf[i++]);
>  		count -= sizeof(u32);
>  	}
>  
>  	/* Write out remaining non 32-bit chunks. */
>  	switch (count) {
>  	case 3:
> -		socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x00ffffff);
> +		socfpga_fpga_data_writel(priv, buf[i++] & 0x00ffffff);
>  		break;
>  	case 2:
> -		socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x0000ffff);
> +		socfpga_fpga_data_writel(priv, buf[i++] & 0x0000ffff);
>  		break;
>  	case 1:
> -		socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x000000ff);
> -		break;
> -	case 0:
> +		socfpga_fpga_data_writel(priv, buf[i++] & 0x000000ff);
>  		break;
>  	default:
> -		/* This will never happen. */
> -		return -EFAULT;
> +		break;
>  	}
> +}
> +
> +static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr,
> +					    struct sg_table *sgt)
> +{
> +	struct socfpga_fpga_priv *priv = mgr->priv;
> +	struct sg_mapping_iter miter;
> +
> +	sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
> +
> +	while (sg_miter_next(&miter))
> +		socfpga_write_buf(priv, miter.addr, miter.length);
>  
> +	sg_miter_stop(&miter);
>  	return 0;
>  }

Hi Jason,

Currently or soon we have 3 drivers that don't really use the sg
interface natively.  So this workaround ends up in each of them?
That's a lot of duplicated code.  Why can't this code be in the
fpga-mgr.c core for drivers that aren't using sg (to minimizing
duplication).

I will test this when I get time, may not be this week.  I just
moved to a new building and lab and am in a course all week and
so forth.

Alan

>  
> @@ -545,8 +563,8 @@ static enum fpga_mgr_states socfpga_fpga_ops_state(struct fpga_manager *mgr)
>  
>  static const struct fpga_manager_ops socfpga_fpga_ops = {
>  	.state = socfpga_fpga_ops_state,
> -	.write_init = socfpga_fpga_ops_configure_init,
> -	.write = socfpga_fpga_ops_configure_write,
> +	.write_init_sg = socfpga_fpga_ops_configure_init,
> +	.write_sg = socfpga_fpga_ops_configure_write,
>  	.write_complete = socfpga_fpga_ops_configure_complete,
>  };
>  
> -- 
> 2.1.4
> 
> 

[PATCH fpga 8/9] fpga socfpga: Use the scatterlist interface

[Jason Gunthorpe]

On Sun, Nov 13, 2016 at 05:19:34PM -0600, atull wrote:

> Currently or soon we have 3 drivers that don't really use the sg
> interface natively.  So this workaround ends up in each of them?

Thinking of the SG list as a workaround is not really right - the SG
list is a way to pass memory stored in non-contiguous pages around,
and the miter is a way to access them from the CPU.

socfpga *does* use sg natively because it is happy to process the data
from the CPU page-at-time. It just doesn't use DMA.

> That's a lot of duplicated code.  Why can't this code be in the
> fpga-mgr.c core for drivers that aren't using sg (to minimizing
> duplication).

Sure, if it is a common pattern it is a good idea to lift it.

I'd add a newop 'write_fragment' and a driver must define write_sg
write_fragment, if write_fragment is used then the core supplies
that loop.

Is there a tree with these new drivers someplace?

> I will test this when I get time, may not be this week.  I just
> moved to a new building and lab and am in a course all week and
> so forth.

Sure, I don't expect any problems, Zynq uses the same loop and it
seems fine.

Jason

[PATCH fpga 8/9] fpga socfpga: Use the scatterlist interface

[atull]

On Mon, 14 Nov 2016, Jason Gunthorpe wrote:

> On Sun, Nov 13, 2016 at 05:19:34PM -0600, atull wrote:
> 
> > Currently or soon we have 3 drivers that don't really use the sg
> > interface natively.  So this workaround ends up in each of them?
> 
> Thinking of the SG list as a workaround is not really right - the SG
> list is a way to pass memory stored in non-contiguous pages around,
> and the miter is a way to access them from the CPU.

No, I ment the other way.  The changes to socfpga.c are a workaround
to the sg-centric interface.  And other drivers will need the same
workaround.  But below I see you understand...

> 
> socfpga *does* use sg natively because it is happy to process the data
> from the CPU page-at-time. It just doesn't use DMA.
> 
> > That's a lot of duplicated code.  Why can't this code be in the
> > fpga-mgr.c core for drivers that aren't using sg (to minimizing
> > duplication).
> 
> Sure, if it is a common pattern it is a good idea to lift it.
> 
> I'd add a newop 'write_fragment' and a driver must define write_sg
> write_fragment, if write_fragment is used then the core supplies
> that loop.

Sure, but isn't that just the old op? :)
There may also be common code that you added to configure_init
that should go in the core unless it's fpga-specific.

> 
> Is there a tree with these new drivers someplace?

The arria10 driver is on linux-next master branch.  There are two
others on the mailing list now.  linux-next also contains other
changes to the FPGA mgr API that will affect your patches in minor
ways, so you should rebase you patches.

> 
> > I will test this when I get time, may not be this week.  I just
> > moved to a new building and lab and am in a course all week and
> > so forth.
> 
> Sure, I don't expect any problems, Zynq uses the same loop and it
> seems fine.
> 
> Jason
> 

[PATCH fpga 5/9] fpga zynq: Remove priv->dev

[atull]

On Wed, 9 Nov 2016, Jason Gunthorpe wrote:

Hi Jason,

    Acked-by: Alan Tull <atull@opensource.altera.com>

Alan

> socfpga uses mgr->dev for debug prints, there should be consistency
> here, so standardize on that. The only other use was for dma
> which can be replaced with mgr->dev.parent.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/fpga/zynq-fpga.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index 3ffc5fcc3072..ac2deae92dbd 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -118,7 +118,6 @@
>  #define FPGA_RST_NONE_MASK		0x0
>  
>  struct zynq_fpga_priv {
> -	struct device *dev;
>  	int irq;
>  	struct clk *clk;
>  
> @@ -188,7 +187,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>  	 * least the sync word and something else to do anything.
>  	 */
>  	if (count <= 4 || (count % 4) != 0) {
> -		dev_err(priv->dev,
> +		dev_err(&mgr->dev,
>  			"Invalid bitstream size, must be multiples of 4 bytes\n");
>  		return -EINVAL;
>  	}
> @@ -200,7 +199,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>  	/* don't globally reset PL if we're doing partial reconfig */
>  	if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
>  		if (!zynq_fpga_has_sync(buf, count)) {
> -			dev_err(priv->dev,
> +			dev_err(&mgr->dev,
>  				"Invalid bitstream, could not find a sync word. Bitstream must be a byte swaped .bin file\n");
>  			err = -EINVAL;
>  			goto out_err;
> @@ -233,7 +232,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>  					     INIT_POLL_DELAY,
>  					     INIT_POLL_TIMEOUT);
>  		if (err) {
> -			dev_err(priv->dev, "Timeout waiting for PCFG_INIT\n");
> +			dev_err(&mgr->dev, "Timeout waiting for PCFG_INIT\n");
>  			goto out_err;
>  		}
>  
> @@ -247,7 +246,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>  					     INIT_POLL_DELAY,
>  					     INIT_POLL_TIMEOUT);
>  		if (err) {
> -			dev_err(priv->dev, "Timeout waiting for !PCFG_INIT\n");
> +			dev_err(&mgr->dev, "Timeout waiting for !PCFG_INIT\n");
>  			goto out_err;
>  		}
>  
> @@ -261,7 +260,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>  					     INIT_POLL_DELAY,
>  					     INIT_POLL_TIMEOUT);
>  		if (err) {
> -			dev_err(priv->dev, "Timeout waiting for PCFG_INIT\n");
> +			dev_err(&mgr->dev, "Timeout waiting for PCFG_INIT\n");
>  			goto out_err;
>  		}
>  	}
> @@ -278,7 +277,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>  	/* check that we have room in the command queue */
>  	status = zynq_fpga_read(priv, STATUS_OFFSET);
>  	if (status & STATUS_DMA_Q_F) {
> -		dev_err(priv->dev, "DMA command queue full\n");
> +		dev_err(&mgr->dev, "DMA command queue full\n");
>  		err = -EBUSY;
>  		goto out_err;
>  	}
> @@ -309,7 +308,8 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>  
>  	priv = mgr->priv;
>  
> -	kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr, GFP_KERNEL);
> +	kbuf =
> +	    dma_alloc_coherent(mgr->dev.parent, count, &dma_addr, GFP_KERNEL);
>  	if (!kbuf)
>  		return -ENOMEM;
>  
> @@ -356,7 +356,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>  	goto out_clk;
>  
>  out_report:
> -	dev_err(priv->dev,
> +	dev_err(&mgr->dev,
>  		"%s: INT_STS:0x%x CTRL:0x%x LOCK:0x%x INT_MASK:0x%x STATUS:0x%x MCTRL:0x%x\n",
>  		why,
>  		intr_status,
> @@ -368,7 +368,7 @@ out_report:
>  out_clk:
>  	clk_disable(priv->clk);
>  out_free:
> -	dma_free_coherent(priv->dev, count, kbuf, dma_addr);
> +	dma_free_coherent(mgr->dev.parent, count, kbuf, dma_addr);
>  	return err;
>  }
>  
> @@ -445,8 +445,6 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	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))
> -- 
> 2.1.4
> 
> 

[PATCH fpga 5/9] fpga zynq: Remove priv->dev

[Moritz Fischer]

Hi Jason,

this one could be independent of the other patches, this is cleanup and
could be merged before the rest (that's still in discussion) if you pull it out.

On Mon, Nov 14, 2016 at 7:13 AM, atull <atull@opensource.altera.com> wrote:
> On Wed, 9 Nov 2016, Jason Gunthorpe wrote:
>
> Hi Jason,
>
>     Acked-by: Alan Tull <atull@opensource.altera.com>
>
> Alan
>
>> socfpga uses mgr->dev for debug prints, there should be consistency
>> here, so standardize on that. The only other use was for dma
>> which can be replaced with mgr->dev.parent.
>>
>> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Modulo the comments above:

Acked-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  drivers/fpga/zynq-fpga.c | 22 ++++++++++------------
>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
>> index 3ffc5fcc3072..ac2deae92dbd 100644
>> --- a/drivers/fpga/zynq-fpga.c
>> +++ b/drivers/fpga/zynq-fpga.c
>> @@ -118,7 +118,6 @@
>>  #define FPGA_RST_NONE_MASK           0x0
>>
>>  struct zynq_fpga_priv {
>> -     struct device *dev;
>>       int irq;
>>       struct clk *clk;
>>
>> @@ -188,7 +187,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>>        * least the sync word and something else to do anything.
>>        */
>>       if (count <= 4 || (count % 4) != 0) {
>> -             dev_err(priv->dev,
>> +             dev_err(&mgr->dev,
>>                       "Invalid bitstream size, must be multiples of 4 bytes\n");
>>               return -EINVAL;
>>       }
>> @@ -200,7 +199,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>>       /* don't globally reset PL if we're doing partial reconfig */
>>       if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
>>               if (!zynq_fpga_has_sync(buf, count)) {
>> -                     dev_err(priv->dev,
>> +                     dev_err(&mgr->dev,
>>                               "Invalid bitstream, could not find a sync word. Bitstream must be a byte swaped .bin file\n");
>>                       err = -EINVAL;
>>                       goto out_err;
>> @@ -233,7 +232,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>>                                            INIT_POLL_DELAY,
>>                                            INIT_POLL_TIMEOUT);
>>               if (err) {
>> -                     dev_err(priv->dev, "Timeout waiting for PCFG_INIT\n");
>> +                     dev_err(&mgr->dev, "Timeout waiting for PCFG_INIT\n");
>>                       goto out_err;
>>               }
>>
>> @@ -247,7 +246,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>>                                            INIT_POLL_DELAY,
>>                                            INIT_POLL_TIMEOUT);
>>               if (err) {
>> -                     dev_err(priv->dev, "Timeout waiting for !PCFG_INIT\n");
>> +                     dev_err(&mgr->dev, "Timeout waiting for !PCFG_INIT\n");
>>                       goto out_err;
>>               }
>>
>> @@ -261,7 +260,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>>                                            INIT_POLL_DELAY,
>>                                            INIT_POLL_TIMEOUT);
>>               if (err) {
>> -                     dev_err(priv->dev, "Timeout waiting for PCFG_INIT\n");
>> +                     dev_err(&mgr->dev, "Timeout waiting for PCFG_INIT\n");
>>                       goto out_err;
>>               }
>>       }
>> @@ -278,7 +277,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>>       /* check that we have room in the command queue */
>>       status = zynq_fpga_read(priv, STATUS_OFFSET);
>>       if (status & STATUS_DMA_Q_F) {
>> -             dev_err(priv->dev, "DMA command queue full\n");
>> +             dev_err(&mgr->dev, "DMA command queue full\n");
>>               err = -EBUSY;
>>               goto out_err;
>>       }
>> @@ -309,7 +308,8 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>>
>>       priv = mgr->priv;
>>
>> -     kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr, GFP_KERNEL);
>> +     kbuf =
>> +         dma_alloc_coherent(mgr->dev.parent, count, &dma_addr, GFP_KERNEL);
>>       if (!kbuf)
>>               return -ENOMEM;
>>
>> @@ -356,7 +356,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>>       goto out_clk;
>>
>>  out_report:
>> -     dev_err(priv->dev,
>> +     dev_err(&mgr->dev,
>>               "%s: INT_STS:0x%x CTRL:0x%x LOCK:0x%x INT_MASK:0x%x STATUS:0x%x MCTRL:0x%x\n",
>>               why,
>>               intr_status,
>> @@ -368,7 +368,7 @@ out_report:
>>  out_clk:
>>       clk_disable(priv->clk);
>>  out_free:
>> -     dma_free_coherent(priv->dev, count, kbuf, dma_addr);
>> +     dma_free_coherent(mgr->dev.parent, count, kbuf, dma_addr);
>>       return err;
>>  }
>>
>> @@ -445,8 +445,6 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>>       if (!priv)
>>               return -ENOMEM;
>>
>> -     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))
>> --
>> 2.1.4
>>
>>
Cheers,

Moritz

[PATCH fpga 5/9] fpga zynq: Remove priv->dev

[Jason Gunthorpe]

On Mon, Nov 14, 2016 at 09:20:38AM -0800, Moritz Fischer wrote:
> this one could be independent of the other patches, this is cleanup and
> could be merged before the rest (that's still in discussion) if you pull it out.

As I said in the cover letter, several of the patches are just fixes
and can be applied, but they are all linked by patch context so are
presented as a series.

Lets get as many reviewed as possible and then I can repost them in a
different order if necessary.

The first 5 are all straigtfoward and should be no problem to get
ackd. We've talked about the sanity check patch enough, lets just go
ahead with it now.

Jason

[PATCH fpga 8/9] fpga socfpga: Use the scatterlist interface

[Jason Gunthorpe]

On Sun, Nov 13, 2016 at 10:02:00PM -0600, atull wrote:
> > I'd add a newop 'write_fragment' and a driver must define write_sg
> > write_fragment, if write_fragment is used then the core supplies
> > that loop.
> 
> Sure, but isn't that just the old op? :)

I'm fine with that too, but it is semantically different since write
is called multiple times in this new world.. All the drivers seem fine
with that today so it is probably OK ..

> There may also be common code that you added to configure_init
> that should go in the core unless it's fpga-specific.

Sure, the alignment test is easy enough to pull out..

Jason

[PATCH fpga 1/9] fpga zynq: Add missing \n to messages

[Matthias Brugger]

On 09/11/16 23:58, Jason Gunthorpe wrote:
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Please add a commit message, although it is cristal clear what this 
patch does :)

Regards,
Matthias

> ---
>  drivers/fpga/zynq-fpga.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index c2fb4120bd62..e72340ea7323 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -217,7 +217,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>  					     INIT_POLL_DELAY,
>  					     INIT_POLL_TIMEOUT);
>  		if (err) {
> -			dev_err(priv->dev, "Timeout waiting for PCFG_INIT");
> +			dev_err(priv->dev, "Timeout waiting for PCFG_INIT\n");
>  			goto out_err;
>  		}
>
> @@ -231,7 +231,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>  					     INIT_POLL_DELAY,
>  					     INIT_POLL_TIMEOUT);
>  		if (err) {
> -			dev_err(priv->dev, "Timeout waiting for !PCFG_INIT");
> +			dev_err(priv->dev, "Timeout waiting for !PCFG_INIT\n");
>  			goto out_err;
>  		}
>
> @@ -245,7 +245,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>  					     INIT_POLL_DELAY,
>  					     INIT_POLL_TIMEOUT);
>  		if (err) {
> -			dev_err(priv->dev, "Timeout waiting for PCFG_INIT");
> +			dev_err(priv->dev, "Timeout waiting for PCFG_INIT\n");
>  			goto out_err;
>  		}
>  	}
> @@ -262,7 +262,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>  	/* check that we have room in the command queue */
>  	status = zynq_fpga_read(priv, STATUS_OFFSET);
>  	if (status & STATUS_DMA_Q_F) {
> -		dev_err(priv->dev, "DMA command queue full");
> +		dev_err(priv->dev, "DMA command queue full\n");
>  		err = -EBUSY;
>  		goto out_err;
>  	}
> @@ -331,7 +331,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>  	zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
>
>  	if (!((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)) {
> -		dev_err(priv->dev, "Error configuring FPGA");
> +		dev_err(priv->dev, "Error configuring FPGA\n");
>  		err = -EFAULT;
>  	}
>
> @@ -426,7 +426,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>  	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");
> +		dev_err(dev, "unable to get zynq-slcr regmap\n");
>  		return PTR_ERR(priv->slcr);
>  	}
>
> @@ -434,26 +434,26 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>
>  	priv->irq = platform_get_irq(pdev, 0);
>  	if (priv->irq < 0) {
> -		dev_err(dev, "No IRQ available");
> +		dev_err(dev, "No IRQ available\n");
>  		return priv->irq;
>  	}
>
>  	err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0,
>  			       dev_name(dev), priv);
>  	if (err) {
> -		dev_err(dev, "unable to request IRQ");
> +		dev_err(dev, "unable to request IRQ\n");
>  		return err;
>  	}
>
>  	priv->clk = devm_clk_get(dev, "ref_clk");
>  	if (IS_ERR(priv->clk)) {
> -		dev_err(dev, "input clock not found");
> +		dev_err(dev, "input clock not found\n");
>  		return PTR_ERR(priv->clk);
>  	}
>
>  	err = clk_prepare_enable(priv->clk);
>  	if (err) {
> -		dev_err(dev, "unable to enable clock");
> +		dev_err(dev, "unable to enable clock\n");
>  		return err;
>  	}
>
> @@ -465,7 +465,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>  	err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
>  				&zynq_fpga_ops, priv);
>  	if (err) {
> -		dev_err(dev, "unable to register FPGA manager");
> +		dev_err(dev, "unable to register FPGA manager\n");
>  		clk_unprepare(priv->clk);
>  		return err;
>  	}
>

[PATCH fpga 8/9] fpga socfpga: Use the scatterlist interface

[atull]

On Tue, 15 Nov 2016, Jason Gunthorpe wrote:

> On Sun, Nov 13, 2016 at 10:02:00PM -0600, atull wrote:
> > > I'd add a newop 'write_fragment' and a driver must define write_sg
> > > write_fragment, if write_fragment is used then the core supplies
> > > that loop.
> > 
> > Sure, but isn't that just the old op? :)
> 
> I'm fine with that too, but it is semantically different since write
> is called multiple times in this new world.. All the drivers seem fine
> with that today so it is probably OK ..

Not different.

>From 'fpga-mgr.txt':
  The programming sequence is:
   1. .write_init
   2. .write (may be called once or multiple times)
   3. .write_complete

The old write was be separate from write_init and write_complete
because I figured that in the future someone may be streaming in
the bitstream and not have the whole bitstream in memory.  So
that is keeping with the original intention that it could be
called multiple times.  The current API doesn't do that but the
fpga manager ops are intended to support that from the start.

Alan

> 
> > There may also be common code that you added to configure_init
> > that should go in the core unless it's fpga-specific.
> 
> Sure, the alignment test is easy enough to pull out..
> 
> Jason
> 

[PATCH fpga 1/9] fpga zynq: Add missing \n to messages

[Jason Gunthorpe]

On Tue, Nov 15, 2016 at 12:05:15PM +0100, Matthias Brugger wrote:
> On 09/11/16 23:58, Jason Gunthorpe wrote:
> >Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> 
> Please add a commit message, although it is cristal clear what this patch
> does :)

As you say, it is crystal clear already, and this is an acceptable commit
message.. Please suggest a text if you want to see something
different.

Jason

[PATCH fpga 8/9] fpga socfpga: Use the scatterlist interface

[Jason Gunthorpe]

On Tue, Nov 15, 2016 at 09:47:05AM -0600, atull wrote:
> Not different.
> 
> From 'fpga-mgr.txt':
>   The programming sequence is:
>    1. .write_init
>    2. .write (may be called once or multiple times)
>    3. .write_complete
> 
> The old write was be separate from write_init and write_complete
> because I figured that in the future someone may be streaming in
> the bitstream and not have the whole bitstream in memory.

What is the point of this if write_init gets a copy of the buffer -
what is that supposed to be?

If you see things this way why are you opposed to patch 9? I'll change
things around to call write multiple times and force the sg list into
write_init, which seems like what you intended anyhow..

Jason

[PATCH fpga 8/9] fpga socfpga: Use the scatterlist interface

[atull]

On Wed, 16 Nov 2016, Jason Gunthorpe wrote:

> On Tue, Nov 15, 2016 at 09:47:05AM -0600, atull wrote:
> > Not different.
> > 
> > From 'fpga-mgr.txt':
> >   The programming sequence is:
> >    1. .write_init
> >    2. .write (may be called once or multiple times)
> >    3. .write_complete
> > 
> > The old write was be separate from write_init and write_complete
> > because I figured that in the future someone may be streaming in
> > the bitstream and not have the whole bitstream in memory.
> 
> What is the point of this if write_init gets a copy of the buffer -
> what is that supposed to be?

Sometimes write_init needs to look at the header of the image.
You can see that in the socfpga-a10.c (on linux-next/master)

> 
> If you see things this way why are you opposed to patch 9? I'll change
> things around to call write multiple times and force the sg list into
> write_init, which seems like what you intended anyhow..

Not against it (and I do need to spend some more time looking
at this stuff, this is coming at a busy time).  My point there
was that there was code that needed to go into the core so that
the ICE40 and the cyclone spi driver that are on the mailing
list won't have to have the same workaround that you were
adding to the socfpga.c driver.

Alan

> 
> Jason
> 

[PATCH fpga 1/9] fpga zynq: Add missing \n to messages

[Moritz Fischer]

On Tue, Nov 15, 2016 at 11:08:13AM -0700, Jason Gunthorpe wrote:
> 
> On Tue, Nov 15, 2016 at 12:05:15PM +0100, Matthias Brugger wrote:
> > On 09/11/16 23:58, Jason Gunthorpe wrote:
> > >Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > 
> > Please add a commit message, although it is cristal clear what this patch
> > does :)

That.
> 
> As you say, it is crystal clear already, and this is an acceptable commit
> message.. Please suggest a text if you want to see something
> different.
> 

It still needs a long message. Just do it.

Thanks,

Moritz

[PATCH fpga 1/9] fpga zynq: Add missing \n to messages

[Jason Gunthorpe]

On Wed, Nov 16, 2016 at 10:39:26AM -0800, Moritz Fischer wrote:
> > As you say, it is crystal clear already, and this is an acceptable commit
> > message.. Please suggest a text if you want to see something
> > different.
> > 
> 
> It still needs a long message. Just do it.

Can I put your reviewed-by on this when I resend the series?

Will you have time to review the other patches this week?

Jason

[PATCH fpga 8/9] fpga socfpga: Use the scatterlist interface

[Jason Gunthorpe]

On Wed, Nov 16, 2016 at 09:45:23AM -0600, atull wrote:
> > What is the point of this if write_init gets a copy of the buffer -
> > what is that supposed to be?
> 
> Sometimes write_init needs to look at the header of the image.
> You can see that in the socfpga-a10.c (on linux-next/master)

I know what it is for, I'm asking what should it be if we are calling
write_init multiple times.

It feels like the driver needs to indicate the header length it wants
to inspect and the core core needs to make that much of the bitstream
available to write_init() before calling write().

Is that what you were thinking?

> at this stuff, this is coming at a busy time).  My point there
> was that there was code that needed to go into the core so that
> the ICE40 and the cyclone spi driver that are on the mailing
> list won't have to have the same workaround that you were
> adding to the socfpga.c driver.

Sure, that is easy for write() - not clear on write_init sematics?
I will send a revised series.

I'd also like to close on the zynq bitfile verification patch, did you
have any comments on that?

Jason

[PATCH fpga 1/9] fpga zynq: Add missing \n to messages

[atull]

On Wed, 16 Nov 2016, Jason Gunthorpe wrote:

> On Wed, Nov 16, 2016 at 10:39:26AM -0800, Moritz Fischer wrote:
> > > As you say, it is crystal clear already, and this is an acceptable commit
> > > message.. Please suggest a text if you want to see something
> > > different.
> > > 
> > 
> > It still needs a long message. Just do it.
> 
> Can I put your reviewed-by on this when I resend the series?
> 
> Will you have time to review the other patches this week?
> 
> Jason
> 

Hi Jason,

I agree that this needs a long description and I'm not sure
why you're pushing back on that.

You will need to rebase this series on linux-next.  I won't
have much time this week to look at this, sorry.  Not sure
what the hurry is.  I see the value of this, but things are
busy right now.  I'll be getting up to speed on how the
kernel does sg and this is kind of a fundamental change to
the API so I appreciate your patience.

Alan

[PATCH fpga 1/9] fpga zynq: Add missing \n to messages

[Moritz Fischer]

Hi Jason,

On Wed, Nov 16, 2016 at 2:28 PM, atull <atull@opensource.altera.com> wrote:
> On Wed, 16 Nov 2016, Jason Gunthorpe wrote:
>
>> On Wed, Nov 16, 2016 at 10:39:26AM -0800, Moritz Fischer wrote:
>> > > As you say, it is crystal clear already, and this is an acceptable commit
>> > > message.. Please suggest a text if you want to see something
>> > > different.
>> > >
>> >
>> > It still needs a long message. Just do it.
>>
>> Can I put your reviewed-by on this when I resend the series?

For this particular patch, feel free to add my Reviewed-By: after fixing
the long description. As Alan said there's currently a lot of patches
in flight and we're working on coming up with a better workflow.
Stay tuned.

>>
>> Will you have time to review the other patches this week?
>>
>> Jason
>>
>
> Hi Jason,
>
> I agree that this needs a long description and I'm not sure
> why you're pushing back on that.
>
> You will need to rebase this series on linux-next.  I won't
> have much time this week to look at this, sorry.  Not sure
> what the hurry is.  I see the value of this, but things are
> busy right now.  I'll be getting up to speed on how the
> kernel does sg and this is kind of a fundamental change to
> the API so I appreciate your patience.

Same here. We'll get prototypes next week, so I'm busy this week
and next week. I agree there's no massive hurry.

Thanks,

Moritz

[PATCH fpga 1/9] fpga zynq: Add missing \n to messages

[Jason Gunthorpe]

On Wed, Nov 16, 2016 at 04:28:27PM -0600, atull wrote:
> I agree that this needs a long description and I'm not sure
> why you're pushing back on that.

*shurg* it is just an unusual thing to ask for, but it is OK for a
subsystem to have a particular style like this. Other subsystems
do not.

I will add "This makes the driver prints display properly."

> You will need to rebase this series on linux-next.  I won't
> have much time this week to look at this, sorry.  Not sure
> what the hurry is.  I see the value of this, but things are
> busy right now.  I'll be getting up to speed on how the
> kernel does sg and this is kind of a fundamental change to
> the API so I appreciate your patience.

The merge window is opening soon so it would be nice to land some of
this in 4.10.

Since I have to rebase and restructure a chunk of it, I'd like to get
the bulk of the broad feedback collected before doing that..

Jason

[PATCH fpga 4/9] fpga zynq: Check for errors after completing DMA

[Moritz Fischer]

Hi Jason,

couple of minor things inline below:

On Wed, Nov 9, 2016 at 2:58 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> The completion did not check the interrupt status to see if any error
> bits were asserted, check error bits and dump some registers if things
> went wrong.
>
> A few fixes are needed to make this work, the IXR_ERROR_FLAGS_MASK was
> wrong, it included the done bits, which shows a bug in mask/unmask_irqs
> which were using the wrong bits, simplify all of this stuff.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/fpga/zynq-fpga.c | 55 +++++++++++++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index 40cf0feaca7c..3ffc5fcc3072 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -89,7 +89,7 @@
>  #define IXR_D_P_DONE_MASK              BIT(12)
>   /* FPGA programmed */
>  #define IXR_PCFG_DONE_MASK             BIT(2)
> -#define IXR_ERROR_FLAGS_MASK           0x00F0F860
> +#define IXR_ERROR_FLAGS_MASK           0x00F0C860

True. Ouch.

>  #define IXR_ALL_MASK                   0xF8F7F87F
>
>  /* Miscellaneous constant values */
> @@ -144,23 +144,10 @@ static inline u32 zynq_fpga_read(const struct zynq_fpga_priv *priv,
>         readl_poll_timeout(priv->io_base + addr, val, cond, sleep_us, \
>                            timeout_us)
>
> -static void zynq_fpga_mask_irqs(struct zynq_fpga_priv *priv)
> +static inline void zynq_fpga_set_irq_mask(struct zynq_fpga_priv *priv,
> +                                         u32 enable)
>  {
> -       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));
> +       zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);

Not a fan of this ~enable here. Function name indicates you're doing
the opposite
(see below comment).

>  }
>
>  static irqreturn_t zynq_fpga_isr(int irq, void *data)
> @@ -168,7 +155,7 @@ static irqreturn_t zynq_fpga_isr(int irq, void *data)
>         struct zynq_fpga_priv *priv = data;
>
>         /* disable DMA and error IRQs */
> -       zynq_fpga_mask_irqs(priv);
> +       zynq_fpga_set_irq_mask(priv, 0);
>
>         complete(&priv->dma_done);
>
> @@ -314,6 +301,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>                                const char *buf, size_t count)
>  {
>         struct zynq_fpga_priv *priv;
> +       const char *why;
>         int err;
>         char *kbuf;
>         dma_addr_t dma_addr;
> @@ -337,7 +325,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>         reinit_completion(&priv->dma_done);
>
>         /* enable DMA and error IRQs */
> -       zynq_fpga_unmask_irqs(priv);
> +       zynq_fpga_set_irq_mask(priv, IXR_D_P_DONE_MASK | IXR_ERROR_FLAGS_MASK);

I find the naming here confusing. This sets ~(IXR_D_P_DONE_MASK |
IXR_ERROR_FLAGS_MASK)
as mask value, to enable the D_P_DONE and error IRQs yet the function
name indicates the opposite.
>
>         /* the +1 in the src addr is used to hold off on DMA_DONE IRQ
>          * until both AXI and PCAP are done ...
> @@ -352,16 +340,35 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
>         intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
>         zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
>
> +       if (intr_status & IXR_ERROR_FLAGS_MASK) {
> +               why = "DMA reported error";
> +               err = -EIO;
> +               goto out_report;
> +       }
> +
>         if (!((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)) {
> -               dev_err(priv->dev, "Error configuring FPGA\n");
> -               err = -EFAULT;
> +               why = "DMA did not complete";
> +               err = -EIO;
> +               goto out_report;
>         }
>
> +       err = 0;
> +       goto out_clk;
> +
> +out_report:
> +       dev_err(priv->dev,
> +               "%s: INT_STS:0x%x CTRL:0x%x LOCK:0x%x INT_MASK:0x%x STATUS:0x%x MCTRL:0x%x\n",
> +               why,
> +               intr_status,
> +               zynq_fpga_read(priv, CTRL_OFFSET),
> +               zynq_fpga_read(priv, LOCK_OFFSET),
> +               zynq_fpga_read(priv, INT_MASK_OFFSET),
> +               zynq_fpga_read(priv, STATUS_OFFSET),
> +               zynq_fpga_read(priv, MCTRL_OFFSET));
> +out_clk:
>         clk_disable(priv->clk);
> -

Personally found it more readable with newline.

>  out_free:
>         dma_free_coherent(priv->dev, count, kbuf, dma_addr);
> -

Same here.
>         return err;
>  }
>
> @@ -475,7 +482,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>         /* unlock the device */
>         zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
>
> -       zynq_fpga_write(priv, INT_MASK_OFFSET, 0xFFFFFFFF);
> +       zynq_fpga_set_irq_mask(priv, 0);
>         zynq_fpga_write(priv, INT_STS_OFFSET, IXR_ALL_MASK);
>         err = devm_request_irq(dev, priv->irq, zynq_fpga_isr, 0, dev_name(dev),
>                                priv);
> --
> 2.1.4
>

Thanks,

Moritz

[PATCH fpga 1/9] fpga zynq: Add missing \n to messages

[Matthias Brugger]

On 15/11/16 19:08, Jason Gunthorpe wrote:
>
> On Tue, Nov 15, 2016 at 12:05:15PM +0100, Matthias Brugger wrote:
>> On 09/11/16 23:58, Jason Gunthorpe wrote:
>>> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>
>> Please add a commit message, although it is cristal clear what this patch
>> does :)
>
> As you say, it is crystal clear already, and this is an acceptable commit
> message.. Please suggest a text if you want to see something
> different.
>

"Function dev_err doesn't add a newline at the end of the string. This 
will lead to a hard to read kernel log. This patch fixes this."

With this (or something similar added):
Reviewed-by: Matthias Brugger <mbrugger@suse.com>

Regards,
Matthias

[PATCH fpga 5/9] fpga zynq: Remove priv->dev

[Moritz Fischer]

Hi Jason,

On Mon, Nov 14, 2016 at 09:13:49AM -0600, atull wrote:
> On Wed, 9 Nov 2016, Jason Gunthorpe wrote:
> 
> Hi Jason,
> 
>     Acked-by: Alan Tull <atull@opensource.altera.com>
> 
> Alan
> 
> > socfpga uses mgr->dev for debug prints, there should be consistency
> > here, so standardize on that. The only other use was for dma
> > which can be replaced with mgr->dev.parent.
> > 
> > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

For the priv->dev removal part.

Acked-by: Moritz Fischer <moritz.fischer@ettus.com>

> > ---
> >  drivers/fpga/zynq-fpga.c | 22 ++++++++++------------
> >  1 file changed, 10 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> > index 3ffc5fcc3072..ac2deae92dbd 100644
> > --- a/drivers/fpga/zynq-fpga.c
> > +++ b/drivers/fpga/zynq-fpga.c
> > @@ -118,7 +118,6 @@
> >  #define FPGA_RST_NONE_MASK		0x0
> >  
> >  struct zynq_fpga_priv {
> > -	struct device *dev;
> >  	int irq;
> >  	struct clk *clk;
> >  
> > @@ -188,7 +187,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> >  	 * least the sync word and something else to do anything.
> >  	 */
> >  	if (count <= 4 || (count % 4) != 0) {
> > -		dev_err(priv->dev,
> > +		dev_err(&mgr->dev,
> >  			"Invalid bitstream size, must be multiples of 4 bytes\n");
> >  		return -EINVAL;
> >  	}
> > @@ -200,7 +199,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> >  	/* don't globally reset PL if we're doing partial reconfig */
> >  	if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> >  		if (!zynq_fpga_has_sync(buf, count)) {
> > -			dev_err(priv->dev,
> > +			dev_err(&mgr->dev,
> >  				"Invalid bitstream, could not find a sync word. Bitstream must be a byte swaped .bin file\n");
> >  			err = -EINVAL;
> >  			goto out_err;
> > @@ -233,7 +232,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> >  					     INIT_POLL_DELAY,
> >  					     INIT_POLL_TIMEOUT);
> >  		if (err) {
> > -			dev_err(priv->dev, "Timeout waiting for PCFG_INIT\n");
> > +			dev_err(&mgr->dev, "Timeout waiting for PCFG_INIT\n");
> >  			goto out_err;
> >  		}
> >  
> > @@ -247,7 +246,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> >  					     INIT_POLL_DELAY,
> >  					     INIT_POLL_TIMEOUT);
> >  		if (err) {
> > -			dev_err(priv->dev, "Timeout waiting for !PCFG_INIT\n");
> > +			dev_err(&mgr->dev, "Timeout waiting for !PCFG_INIT\n");
> >  			goto out_err;
> >  		}
> >  
> > @@ -261,7 +260,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> >  					     INIT_POLL_DELAY,
> >  					     INIT_POLL_TIMEOUT);
> >  		if (err) {
> > -			dev_err(priv->dev, "Timeout waiting for PCFG_INIT\n");
> > +			dev_err(&mgr->dev, "Timeout waiting for PCFG_INIT\n");
> >  			goto out_err;
> >  		}
> >  	}
> > @@ -278,7 +277,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> >  	/* check that we have room in the command queue */
> >  	status = zynq_fpga_read(priv, STATUS_OFFSET);
> >  	if (status & STATUS_DMA_Q_F) {
> > -		dev_err(priv->dev, "DMA command queue full\n");
> > +		dev_err(&mgr->dev, "DMA command queue full\n");
> >  		err = -EBUSY;
> >  		goto out_err;
> >  	}
> > @@ -309,7 +308,8 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
> >  
> >  	priv = mgr->priv;
> >  
> > -	kbuf = dma_alloc_coherent(priv->dev, count, &dma_addr, GFP_KERNEL);
> > +	kbuf =
> > +	    dma_alloc_coherent(mgr->dev.parent, count, &dma_addr, GFP_KERNEL);
> >  	if (!kbuf)
> >  		return -ENOMEM;
> >  
> > @@ -356,7 +356,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
> >  	goto out_clk;
> >  
> >  out_report:
> > -	dev_err(priv->dev,
> > +	dev_err(&mgr->dev,
> >  		"%s: INT_STS:0x%x CTRL:0x%x LOCK:0x%x INT_MASK:0x%x STATUS:0x%x MCTRL:0x%x\n",
> >  		why,
> >  		intr_status,
> > @@ -368,7 +368,7 @@ out_report:
> >  out_clk:
> >  	clk_disable(priv->clk);
> >  out_free:
> > -	dma_free_coherent(priv->dev, count, kbuf, dma_addr);
> > +	dma_free_coherent(mgr->dev.parent, count, kbuf, dma_addr);
> >  	return err;
> >  }
> >  
> > @@ -445,8 +445,6 @@ static int zynq_fpga_probe(struct platform_device *pdev)
> >  	if (!priv)
> >  		return -ENOMEM;
> >  
> > -	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))
> > -- 
> > 2.1.4
> > 
> > 

Thanks,

Moritz

[PATCH fpga 4/9] fpga zynq: Check for errors after completing DMA

[Jason Gunthorpe]

On Wed, Nov 16, 2016 at 10:10:35PM -0800, Moritz Fischer wrote:
> >   /* FPGA programmed */
> >  #define IXR_PCFG_DONE_MASK             BIT(2)
> > -#define IXR_ERROR_FLAGS_MASK           0x00F0F860
> > +#define IXR_ERROR_FLAGS_MASK           0x00F0C860
> 
> True. Ouch.

Yeah, this only works at all by accident..


> > -static void zynq_fpga_mask_irqs(struct zynq_fpga_priv *priv)
> > +static inline void zynq_fpga_set_irq_mask(struct zynq_fpga_priv *priv,
> > +                                         u32 enable)
> >  {
> > -       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);
> > -}
> > +       zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
> 
> Not a fan of this ~enable here. Function name indicates you're doing
> the opposite

Lets call it zynq_fpga_set_irq then

The ~ is best placed here because it is after the compiler has coerced
the argument into u32 so the ~ will always do the right
thing

I've been reading the IXR_*_MASK as indicating it is a bit pattern for
the INT_MASK register not as actually meaning literal masking.

> > +out_clk:
> >         clk_disable(priv->clk);
> > -
> 
> Personally found it more readable with newline.

sure

Jason

[PATCH fpga 8/9] fpga socfpga: Use the scatterlist interface

[atull]

On Wed, 16 Nov 2016, Jason Gunthorpe wrote:

> On Wed, Nov 16, 2016 at 09:45:23AM -0600, atull wrote:
> > > What is the point of this if write_init gets a copy of the buffer -
> > > what is that supposed to be?
> > 
> > Sometimes write_init needs to look at the header of the image.
> > You can see that in the socfpga-a10.c (on linux-next/master)
> 
> I know what it is for, I'm asking what should it be if we are calling
> write_init multiple times.
> 
> It feels like the driver needs to indicate the header length it wants
> to inspect and the core core needs to make that much of the bitstream
> available to write_init() before calling write().
> 
> Is that what you were thinking?

That would make sense.  socfpga-a10.c requires a certain amount
of header in write_init, but the current API didn't have a way
for socfgpa-a10.c to specify that to fpga-mgr.c core.  Should
probably happen during registration.  If you have an idea about
that, that's good, otherwise we'll get back to that separately.

> 
> > at this stuff, this is coming at a busy time).  My point there
> > was that there was code that needed to go into the core so that
> > the ICE40 and the cyclone spi driver that are on the mailing
> > list won't have to have the same workaround that you were
> > adding to the socfpga.c driver.
> 
> Sure, that is easy for write() - not clear on write_init sematics?
> I will send a revised series.
> 
> I'd also like to close on the zynq bitfile verification patch, did you
> have any comments on that?

I think Joshua had some comments.  Besides that, I'm ok with that
patch.

Alan

> 
> Jason
> 

[PATCH fpga 8/9] fpga socfpga: Use the scatterlist interface

[atull]

On Thu, 17 Nov 2016, atull wrote:

> On Wed, 16 Nov 2016, Jason Gunthorpe wrote:
> 
> > On Wed, Nov 16, 2016 at 09:45:23AM -0600, atull wrote:
> > > > What is the point of this if write_init gets a copy of the buffer -
> > > > what is that supposed to be?
> > > 
> > > Sometimes write_init needs to look at the header of the image.
> > > You can see that in the socfpga-a10.c (on linux-next/master)
> > 
> > I know what it is for, I'm asking what should it be if we are calling
> > write_init multiple times.
> > 
> > It feels like the driver needs to indicate the header length it wants
> > to inspect and the core core needs to make that much of the bitstream
> > available to write_init() before calling write().
> > 
> > Is that what you were thinking?
> 
> That would make sense.  socfpga-a10.c requires a certain amount
> of header in write_init, but the current API didn't have a way
> for socfgpa-a10.c to specify that to fpga-mgr.c core.  Should
> probably happen during registration.  If you have an idea about
> that, that's good, otherwise we'll get back to that separately.
> 
> > 
> > > at this stuff, this is coming at a busy time).  My point there
> > > was that there was code that needed to go into the core so that
> > > the ICE40 and the cyclone spi driver that are on the mailing
> > > list won't have to have the same workaround that you were
> > > adding to the socfpga.c driver.
> > 
> > Sure, that is easy for write() - not clear on write_init sematics?
> > I will send a revised series.
> > 
> > I'd also like to close on the zynq bitfile verification patch, did you
> > have any comments on that?
> 
> I think Joshua had some comments.  Besides that, I'm ok with that
> patch.
> 
> Alan

I didn't have any other specific issues with on the other
zynq patches.

For the sg patches, I don't have anything more to say
that I haven't already said.

I want to keep open the possibility of streaming an FPGA image
in.  In this case the FPGA image is not complete in memory
and write() will be called multiple times.  One case is where
maybe hopefully the firmware layer can be extended to not
allocate a large buffer (scattered or contiguous) and can
stream in the image.

If you have a nice way to move the workarounds that you had
to add to socfpga.c into the core so they don't replicated
in socfpga-a10.c, ICE40, cyclone5 spi and other new drivers
that aren't using sg natively, that would be good.

I think I've already said all this before so please figure
that into he your v2 when you rebase onto linux-next.

Alan

> 
> > 
> > Jason
> > 
>