Zynq FPGA Manager Improvements

Signed-off-by Jason Gunthorpe <[email protected]>

---
 drivers/fpga/zynq-fpga.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

drivers/fpga/zynq-fpga.c +11 -11

index c2fb4120bd62..e72340ea7323 100644
@@ -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;
 	}

Quoted 1 line · Jason Gunthorpe

Signed-off-by Jason Gunthorpe <[email protected]>

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

Quoted 5 lines · Matthias Brugger

Quoted 1 line · Jason Gunthorpe

Signed-off-by Jason Gunthorpe <[email protected]>

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.

Quoted 4 lines

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.

Quoted 5 lines · Moritz Fischer

Quoted 4 lines

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.

Quoted 1 line

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

Quoted 9 lines · Jason Gunthorpe

Quoted 6 lines · Moritz Fischer

Quoted 4 lines

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?

Quoted 5 lines

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.

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.

Quoted 5 lines · Matthias Brugger

Quoted 1 line · Jason Gunthorpe

Signed-off-by Jason Gunthorpe <[email protected]>

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.

drivers/fpga/zynq-fpga.c +8 -0

@@ -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;
+	}

drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, · line 217

Quoted 3 lines

drivers/fpga/zynq-fpga.c +3 -0

+		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).

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 <[email protected]>

---
 drivers/fpga/zynq-fpga.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

drivers/fpga/zynq-fpga.c +0 -7

index 86f4377e2b52..40cf0feaca7c 100644
@@ -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");

drivers/fpga/zynq-fpga.c +3 -1

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

2.1.4

Quoted 1 line

+ zynq_fpga_write(priv, INT_MASK_OFFSET, 0xFFFFFFFF);

Named constant please.

drivers/fpga/zynq-fpga.c static int zynq_fpga_probe(struct platform_device *pdev) · line 484

Quoted 6 lines

drivers/fpga/zynq-fpga.c +6 -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);
+       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.

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 <[email protected]>

---
 drivers/fpga/zynq-fpga.c | 55 +++++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

drivers/fpga/zynq-fpga.c +0 -0

index 40cf0feaca7c..3ffc5fcc3072 100644

drivers/fpga/zynq-fpga.c +2 -7

 #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);
-}

drivers/fpga/zynq-fpga.c +2 -1

 }
 
 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;

drivers/fpga/zynq-fpga.c +15 -2

        /* 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",

drivers/fpga/zynq-fpga.c +1 -2

        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

drivers/fpga/zynq-fpga.c line 92

Quoted 4 lines

/* FPGA programmed */
#define IXR_PCFG_DONE_MASK BIT(2)

drivers/fpga/zynq-fpga.c +1 -1

-#define IXR_ERROR_FLAGS_MASK           0x00F0F860
+#define IXR_ERROR_FLAGS_MASK           0x00F0C860

True. Ouch.

Quoted 6 lines

drivers/fpga/zynq-fpga.c +2 -3

-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;
-

drivers/fpga/zynq-fpga.c static inline u32 zynq_fpga_read(const struct zynq_fpga_priv *priv, · line 150

Quoted 5 lines

drivers/fpga/zynq-fpga.c +1 -4

-       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

drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write(struct fpga_manager *mgr, · line 361

Quoted 3 lines

drivers/fpga/zynq-fpga.c +1 -1

+out_clk:
        clk_disable(priv->clk);
-

Personally found it more readable with newline.

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 <[email protected]>

---
 drivers/fpga/zynq-fpga.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

drivers/fpga/zynq-fpga.c +10 -12

index 3ffc5fcc3072..ac2deae92dbd 100644
@@ -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))

Signature · 1 line

2.1.4

On Wed, 9 Nov 2016, Jason Gunthorpe wrote:

Hi Jason,

Acked-by Alan Tull <[email protected]>

Alan

Quoted 5 lines

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 <[email protected]>

Quoted 102 lines

---
 drivers/fpga/zynq-fpga.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

drivers/fpga/zynq-fpga.c +10 -10

index 3ffc5fcc3072..ac2deae92dbd 100644
@@ -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;
 

Quoted 9 lines

drivers/fpga/zynq-fpga.c +0 -3

-     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

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 Wed, 9 Nov 2016, Jason Gunthorpe wrote:

Hi Jason,

Acked-by Alan Tull <[email protected]>

Alan

Quoted 5 lines

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 <[email protected]>

Quoted 102 lines

---
 drivers/fpga/zynq-fpga.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

drivers/fpga/zynq-fpga.c +10 -10

index 3ffc5fcc3072..ac2deae92dbd 100644
@@ -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;
 

Quoted 9 lines

drivers/fpga/zynq-fpga.c +0 -2

-	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))

Signature · 1 line

2.1.4

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 <[email protected]>

---
 drivers/fpga/socfpga.c | 56 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 19 deletions(-)

drivers/fpga/socfpga.c +28 -17

index 27d2ff28132c..f3f390b2eecf 100644
@@ -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;

drivers/fpga/socfpga.c +2 -2

@@ -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,
 };
 

Signature · 1 line

2.1.4

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.

Quoted 2 lines · atull

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.

Quoted 3 lines

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?

Quoted 4 lines

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

Quoted 3 lines

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.

Quoted 5 lines · atull

Quoted 3 lines

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 ..

From 'fpga-mgr.txt':

Quoted 3 lines

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

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.

Quoted 8 lines · atull

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

Quoted 3 lines

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

Quoted 2 lines

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)

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.

Quoted 5 lines · atull

Quoted 2 lines

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?

Quoted 6 lines

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

Quoted 7 lines · Jason Gunthorpe

Quoted 5 lines · atull

Quoted 2 lines

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)

Quoted 8 lines

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.

Quoted 10 lines

Quoted 6 lines

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.

Quoted 2 lines

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

Quoted 4 lines

Jason

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 <[email protected]>

---
 drivers/fpga/fpga-mgr.c       | 62 +++++++------------------------------------
 include/linux/fpga/fpga-mgr.h |  7 ++---
 2 files changed, 11 insertions(+), 58 deletions(-)

drivers/fpga/fpga-mgr.c +9 -53

index c2491ffeabd3..4ba22925d9d5 100644
@@ -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;
 	}

include/linux/fpga/fpga-mgr.h +0 -5

index 371b30ea60eb..5c698c8fe71b 100644
@@ -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);

Quoted 15 lines

struct fpga_manager_ops {

include/linux/fpga/fpga-mgr.h +2 -3

 	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);
 

include/linux/fpga/fpga-mgr.h struct fpga_manager { · line 120

Quoted 2 lines

include/linux/fpga/fpga-mgr.h +0 -0

 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?

Quoted 15 lines

Quoted 15 lines

struct fpga_manager_ops {

include/linux/fpga/fpga-mgr.h +2 -3

 	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);
 

Quoted 7 lines

include/linux/fpga/fpga-mgr.h struct fpga_manager { · line 120

Quoted 2 lines

include/linux/fpga/fpga-mgr.h +0 -0

 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.

Quoted 4 lines

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.

Quoted 1 line

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

Fundamentally no.

Jason

Quoted 35 lines · Jason Gunthorpe

Quoted 15 lines

Quoted 15 lines

struct fpga_manager_ops {

include/linux/fpga/fpga-mgr.h +2 -3

 	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);
 

Quoted 7 lines

include/linux/fpga/fpga-mgr.h struct fpga_manager { · line 120

Quoted 2 lines

include/linux/fpga/fpga-mgr.h +0 -0

 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.

Quoted 4 lines

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.

Quoted 9 lines

Quoted 4 lines

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.

Quoted 1 line

Quoted 4 lines

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.

Quoted 2 lines

Quoted 1 line

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

Fundamentally no.

Quoted 2 lines

Jason

Joshua

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

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.