dma: mv_xor_v2: new driver

Hi Thomas,

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.5-rc4 next-20160215]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Thomas-Petazzoni/dma-mv_xor_v2-new-driver/20160215-160120
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next
config: openrisc-allyesconfig (attached as .config)

reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=openrisc

All errors (new ones prefixed by >>):

In file included from kernel/irq/chip.c:14:0:

Quoted 1 line

Quoted 1 line

include/linux/msi.h:174:21: fatal error: asm/msi.h: No such file or directory

compilation terminated.

vim +174 include/linux/msi.h

c2791b80 Yijing Wang    2014-11-11  168  	void (*teardown_irq)(struct msi_controller *chip, unsigned int irq);
0cbdcfcf Thierry Reding 2013-08-09  169  };
0cbdcfcf Thierry Reding 2013-08-09  170  
f3cf8bb0 Jiang Liu      2014-11-12  171  #ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
d9109698 Jiang Liu      2014-11-15  172  
aeeb5965 Jiang Liu      2014-11-15  173  #include <linux/irqhandler.h>
d9109698 Jiang Liu      2014-11-15 @174  #include <asm/msi.h>
d9109698 Jiang Liu      2014-11-15  175  
f3cf8bb0 Jiang Liu      2014-11-12  176  struct irq_domain;
552c494a Marc Zyngier   2015-11-23  177  struct irq_domain_ops;

:::::: The code at line 174 was first introduced by commit
:::::: d9109698be6e7439e6082aa00d79d4556114739b genirq: Introduce msi_domain_alloc/free_irqs()

:::::: TO: Jiang Liu <[email protected]> :::::: CC: Thomas Gleixner <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Marc,

This doesn't seem to be an issue introduced by my patch itself, but rather a problem with the MSI subsystem itself. Is this already a known issue (possibly already fixed), or should I investigate?

Thanks!

Thomas

Quoted 43 lines · kbuild test robot

Hi Thomas,

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.5-rc4 next-20160215]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Thomas-Petazzoni/dma-mv_xor_v2-new-driver/20160215-160120
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next
config: openrisc-allyesconfig (attached as .config)

reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=openrisc

All errors (new ones prefixed by >>):

In file included from kernel/irq/chip.c:14:0:

Quoted 1 line

Quoted 1 line

include/linux/msi.h:174:21: fatal error: asm/msi.h: No such file or directory

compilation terminated.

vim +174 include/linux/msi.h

c2791b80 Yijing Wang    2014-11-11  168  	void (*teardown_irq)(struct msi_controller *chip, unsigned int irq);
0cbdcfcf Thierry Reding 2013-08-09  169  };
0cbdcfcf Thierry Reding 2013-08-09  170  
f3cf8bb0 Jiang Liu      2014-11-12  171  #ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
d9109698 Jiang Liu      2014-11-15  172  
aeeb5965 Jiang Liu      2014-11-15  173  #include <linux/irqhandler.h>
d9109698 Jiang Liu      2014-11-15 @174  #include <asm/msi.h>
d9109698 Jiang Liu      2014-11-15  175  
f3cf8bb0 Jiang Liu      2014-11-12  176  struct irq_domain;
552c494a Marc Zyngier   2015-11-23  177  struct irq_domain_ops;

:::::: The code at line 174 was first introduced by commit
:::::: d9109698be6e7439e6082aa00d79d4556114739b genirq: Introduce msi_domain_alloc/free_irqs()

:::::: TO: Jiang Liu <[email protected]> :::::: CC: Thomas Gleixner <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Seems like something is selecting GENERIC_MSI_IRQ_DOMAIN on a system that doesn't implement the required bits.

It may be that we need to implement something like HAS_GENERIC_MSI_IRQ_DOMAIN, and make GENERIC_MSI_IRQ_DOMAIN conditional on that. Or just whitelist the architectures that do implement this.

Any chance you could have a closer look?

The new mv_xor_v2 driver supports the XOR engines found in the 64-bits ARM from Marvell of the Armada 7K and Armada 8K family. This XOR engine is a completely new hardware block, entirely different from the one used on previous Marvell Armada platforms, which use the existing mv_xor driver.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 .../devicetree/bindings/dma/mv-xor-v2.txt          |  19 +
 drivers/dma/Kconfig                                |  13 +
 drivers/dma/Makefile                               |   1 +
 drivers/dma/mv_xor_v2.c                            | 880 +++++++++++++++++++++
 4 files changed, 913 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/mv-xor-v2.txt
 create mode 100644 drivers/dma/mv_xor_v2.c

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +0 -0

new file mode 100644
index 0000000..0a03dcf

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +15 -0

+- reg: Should contain registers location and length (two sets)
+    the first set is the DMA registers
+    the second set is the global registers
+- msi-parent: Phandle to the MSI-capable interrupt controller used for
+  interrupts.
+
+Example:
+
+	xor0 at 400000 {
+		compatible = "marvell,mv-xor-v2";
+		reg = <0x400000 0x1000>,
+		      <0x410000 0x1000>;
+		msi-parent = <&gic_v2m0>;
+		dma-coherent;
+	};

Quoted 2 lines

+Required properties:
+- compatible: Should be "marvell,mv-xor-v2"

and an SoC specific compatible please.

"marvell,mv" is a bit redundant.

Rob,

Quoted 4 lines · Rob Herring

Quoted 2 lines

+Required properties:
+- compatible: Should be "marvell,mv-xor-v2"

and an SoC specific compatible please.

So you mean *both* marvell,mv-xor-v2 and a SoC specific compatible?

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +9 -0

new file mode 100644
index 0000000..0a03dcf
@@ -0,0 +1,19 @@
+* Marvell XOR v2 engines
+
+Required properties:
+- compatible: Should be "marvell,mv-xor-v2"
+- reg: Should contain registers location and length (two sets)
+    the first set is the DMA registers
+    the second set is the global registers
+- msi-parent: Phandle to the MSI-capable interrupt controller used for
+  interrupts.

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +20 -0

+/* DMA Engine Registers */
+#define DMA_DESQ_BALR_OFF	0x000
+#define DMA_DESQ_BAHR_OFF	0x004
+#define DMA_DESQ_SIZE_OFF	0x008
+#define DMA_DESQ_DONE_OFF	0x00C
+#define   DMA_DESQ_DONE_PENDING_MASK		0x7FFF
+#define   DMA_DESQ_DONE_PENDING_SHIFT		0
+#define   DMA_DESQ_DONE_READ_PTR_MASK		0x1FFF
+#define   DMA_DESQ_DONE_READ_PTR_SHIFT		16
+#define DMA_DESQ_ARATTR_OFF	0x010
+#define   DMA_DESQ_ATTR_CACHE_MASK		0x3F3F
+#define   DMA_DESQ_ATTR_OUTER_SHAREABLE		0x202
+#define   DMA_DESQ_ATTR_CACHEABLE		0x3C3C
+#define DMA_IMSG_CDAT_OFF	0x014
+#define DMA_IMSG_THRD_OFF	0x018
+#define   DMA_IMSG_THRD_MASK			0x7FFF
+#define   DMA_IMSG_THRD_SHIFT			0x0
+#define DMA_DESQ_AWATTR_OFF	0x01C
+  /* Same flags as DMA_DESQ_ARATTR_OFF */
+#define DMA_DESQ_ALLOC_OFF	0x04C

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +15 -0

+
+/* XOR Global registers */
+#define GLOB_BW_CTRL		0x4
+#define   GLOB_BW_CTRL_NUM_OSTD_RD_SHIFT	0
+#define   GLOB_BW_CTRL_NUM_OSTD_RD_VAL		64
+#define   GLOB_BW_CTRL_NUM_OSTD_WR_SHIFT	8
+#define   GLOB_BW_CTRL_NUM_OSTD_WR_VAL		8
+#define   GLOB_BW_CTRL_RD_BURST_LEN_SHIFT	12
+#define   GLOB_BW_CTRL_RD_BURST_LEN_VAL		4
+#define   GLOB_BW_CTRL_WR_BURST_LEN_SHIFT	16
+#define	  GLOB_BW_CTRL_WR_BURST_LEN_VAL		4
+#define GLOB_PAUSE		0x014
+#define   GLOB_PAUSE_AXI_TIME_DIS_VAL		0x8
+#define GLOB_SYS_INT_CAUSE	0x200
+#define GLOB_SYS_INT_MASK	0x204

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +5 -0

+	 *  First word: Buffer-DX-Address-Low[31:0]
+	 *  Second word:Buffer-DX+1-Address-Low[31:0]
+	 *  Third word: DX+1-Buffer-Address-High[47:32] [31:16]
+	 *		DX-Buffer-Address-High[47:32] [15:0]
+	 */

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +8 -0

+	} else {
+		desc->data_buff_addr[arr_index + 1] =
+			lower_32_bits(src);
+
+		/* Clear upper 16-bits */
+		desc->data_buff_addr[arr_index + 2] &= ~0xFFFF0000;
+
+		/* Set them if we have a 64 bits DMA address */

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +12 -0

+static dma_cookie_t
+mv_xor_v2_tx_submit(struct dma_async_tx_descriptor *tx)
+{
+	int desq_ptr;
+	void *dest_hw_desc;
+	dma_cookie_t cookie;
+	struct mv_xor_v2_sw_desc *sw_desc =
+		container_of(tx, struct mv_xor_v2_sw_desc, async_tx);
+	struct mv_xor_v2_device *xor_dev =
+		container_of(tx->chan, struct mv_xor_v2_device, dmachan);
+
+	dev_dbg(xor_dev->dmadev.dev,

Quoted 19 lines · Thomas Petazzoni

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +13 -0

new file mode 100644
index 0000000..0a03dcf
@@ -0,0 +1,19 @@
+* Marvell XOR v2 engines
+
+Required properties:
+- compatible: Should be "marvell,mv-xor-v2"
+- reg: Should contain registers location and length (two sets)
+    the first set is the DMA registers
+    the second set is the global registers
+- msi-parent: Phandle to the MSI-capable interrupt controller used for
+  interrupts.
+
+Example:
+
+	xor0 at 400000 {

Documentation/devicetree/bindings/dma/mv-xor-v2.txt line 19

Quoted 6 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +6 -0

+		compatible = "marvell,mv-xor-v2";
+		reg = <0x400000 0x1000>,
+		      <0x410000 0x1000>;
+		msi-parent = <&gic_v2m0>;
+		dma-coherent;
+	};

This should be a separate patch :)

Quoted 26 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +25 -0

+/* DMA Engine Registers */
+#define DMA_DESQ_BALR_OFF	0x000
+#define DMA_DESQ_BAHR_OFF	0x004
+#define DMA_DESQ_SIZE_OFF	0x008
+#define DMA_DESQ_DONE_OFF	0x00C
+#define   DMA_DESQ_DONE_PENDING_MASK		0x7FFF
+#define   DMA_DESQ_DONE_PENDING_SHIFT		0
+#define   DMA_DESQ_DONE_READ_PTR_MASK		0x1FFF
+#define   DMA_DESQ_DONE_READ_PTR_SHIFT		16
+#define DMA_DESQ_ARATTR_OFF	0x010
+#define   DMA_DESQ_ATTR_CACHE_MASK		0x3F3F
+#define   DMA_DESQ_ATTR_OUTER_SHAREABLE		0x202
+#define   DMA_DESQ_ATTR_CACHEABLE		0x3C3C
+#define DMA_IMSG_CDAT_OFF	0x014
+#define DMA_IMSG_THRD_OFF	0x018
+#define   DMA_IMSG_THRD_MASK			0x7FFF
+#define   DMA_IMSG_THRD_SHIFT			0x0
+#define DMA_DESQ_AWATTR_OFF	0x01C
+  /* Same flags as DMA_DESQ_ARATTR_OFF */
+#define DMA_DESQ_ALLOC_OFF	0x04C
+#define   DMA_DESQ_ALLOC_WRPTR_MASK		0xFFFF
+#define   DMA_DESQ_ALLOC_WRPTR_SHIFT		16
+#define DMA_IMSG_BALR_OFF	0x050
+#define DMA_IMSG_BAHR_OFF	0x054
+#define DMA_DESQ_CTRL_OFF	0x100

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 5 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +5 -0

+#define	  DMA_DESQ_CTRL_32B			1
+#define   DMA_DESQ_CTRL_128B			7
+#define DMA_DESQ_STOP_OFF	0x800
+#define DMA_DESQ_DEALLOC_OFF	0x804
+#define DMA_DESQ_ADD_OFF	0x808

Please namespace these and others. Something like MRVL_XOR_XXX or anyother patter you like would be fine...

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 2 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +2 -0

+/* descriptors queue size */
+#define MV_XOR_V2_MAX_DESC_NUM	1024

Is this hardware defined?

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 9 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +9 -0

+static void mv_xor_v2_set_data_buffers(struct mv_xor_v2_device *xor_dev,
+					struct mv_xor_v2_descriptor *desc,
+					dma_addr_t src, int index)
+{
+	int arr_index = ((index >> 1) * 3);
+
+	/*
+	 * fill the buffer's addresses to the descriptor
+	 * The format of the buffers address for 2 sequential buffers X and X+1:

space around + please.

Quoted 9 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +9 -0

+	 *  First word: Buffer-DX-Address-Low[31:0]
+	 *  Second word:Buffer-DX+1-Address-Low[31:0]
+	 *  Third word: DX+1-Buffer-Address-High[47:32] [31:16]
+	 *		DX-Buffer-Address-High[47:32] [15:0]
+	 */
+	if ((index & 0x1) == 0) {
+		desc->data_buff_addr[arr_index] = lower_32_bits(src);
+
+		/* Clear lower 16-bits */

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 6 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +6 -0

+		desc->data_buff_addr[arr_index + 2] &= ~0xFFFF;
+
+		/* Set them if we have a 64 bits DMA address */
+		if (IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT))
+			desc->data_buff_addr[arr_index + 2] |=
+				upper_32_bits(src) & 0xFFFF;

So why should it depend on how kernel is configured?

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 4 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +4 -0

+/*
+ * Return the next available index in the DESQ.
+ */
+static inline int mv_xor_v2_get_desq_write_ptr(struct mv_xor_v2_device *xor_dev)

Pls wrap with 80 chars wherever it is reasonable

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 8 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +8 -0

+/*
+ * Allocate resources for a channel
+ */
+static int mv_xor_v2_alloc_chan_resources(struct dma_chan *chan)
+{
+	/* nothing to be done here */
+	return 0;
+}

No need for empty alloc

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 8 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +8 -0

+
+/*
+ * Free resources of a channel
+ */
+void mv_xor_v2_free_chan_resources(struct dma_chan *chan)
+{
+	/* nothing to be done here */
+}

and free as well :)

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 9 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +9 -0

+static irqreturn_t mv_xor_v2_interrupt_handler(int irq, void *data)
+{
+	struct mv_xor_v2_device *xor_dev = data;
+
+	/*
+	 * Update IMSG threshold, to disable new IMSG interrupts until
+	 * end of the tasklet
+	 */
+	mv_xor_v2_set_imsg_thrd(xor_dev, MV_XOR_V2_MAX_DESC_NUM);

You don't want to check the source of interrupt?

Quoted 17 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +17 -0

+static dma_cookie_t
+mv_xor_v2_tx_submit(struct dma_async_tx_descriptor *tx)
+{
+	int desq_ptr;
+	void *dest_hw_desc;
+	dma_cookie_t cookie;
+	struct mv_xor_v2_sw_desc *sw_desc =
+		container_of(tx, struct mv_xor_v2_sw_desc, async_tx);
+	struct mv_xor_v2_device *xor_dev =
+		container_of(tx->chan, struct mv_xor_v2_device, dmachan);
+
+	dev_dbg(xor_dev->dmadev.dev,
+		"%s sw_desc %p: async_tx %p\n",
+		__func__, sw_desc, &sw_desc->async_tx);
+
+	/* assign coookie */
+	spin_lock_bh(&xor_dev->cookie_lock);

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 5 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +5 -0

+	cookie = dma_cookie_assign(tx);
+	spin_unlock_bh(&xor_dev->cookie_lock);
+
+	/* lock enqueue DESCQ */
+	spin_lock_bh(&xor_dev->push_lock);

Why not a generic channel lock which is used in submit, issue_pending and tasklet. What is the reason for opting different locks?

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 5 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +5 -0

+	/* get the next available slot in the DESQ */
+	desq_ptr = mv_xor_v2_get_desq_write_ptr(xor_dev);
+
+	/* copy the HW descriptor from the SW descriptor to the DESQ */
+	dest_hw_desc = ((void *)xor_dev->hw_desq_virt +

cast to and away from void are not required

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 10 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +9 -0

+			(xor_dev->desc_size * desq_ptr));
+
+	memcpy(dest_hw_desc, &sw_desc->hw_desc, xor_dev->desc_size);
+
+	/* update the DMA Engine with the new descriptor */
+	mv_xor_v2_add_desc_to_desq(xor_dev, 1);
+
+	/* unlock enqueue DESCQ */
+	spin_unlock_bh(&xor_dev->push_lock);

and if IIUC, you are pushing this to HW as well, that is not quite right if thats the case. We need to do this in issue_pending

Quoted 4 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +4 -0

+static struct dma_async_tx_descriptor *
+mv_xor_v2_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src,
+		       unsigned int src_cnt, size_t len, unsigned long flags)
+{

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 6 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +6 -0

+	struct mv_xor_v2_sw_desc	*sw_desc;
+	struct mv_xor_v2_descriptor	*hw_descriptor;
+	struct mv_xor_v2_device		*xor_dev;
+	int i;
+
+	BUG_ON(src_cnt > MV_XOR_V2_CMD_LINE_NUM_MAX_D_BUF || src_cnt < 1);

why BUG_ON, is the system unusable after this?

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 8 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +8 -0

+/*
+ * poll for a transaction completion
+ */
+static enum dma_status mv_xor_v2_tx_status(struct dma_chan *chan,
+		dma_cookie_t cookie, struct dma_tx_state *txstate)
+{
+	/* return the transaction status */
+	return dma_cookie_status(chan, cookie, txstate);

why not assign this as you status callback?

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 7 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +7 -0

+static void mv_xor_v2_issue_pending(struct dma_chan *chan)
+{
+	struct mv_xor_v2_device *xor_dev =
+		container_of(chan, struct mv_xor_v2_device, dmachan);
+
+	/* Activate the channel */
+	writel(0, xor_dev->dma_base + DMA_DESQ_STOP_OFF);

what if channel is already running?

Quoted 4 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +4 -0

+static void mv_xor_v2_tasklet(unsigned long data)
+{
+	struct mv_xor_v2_device *xor_dev = (struct mv_xor_v2_device *) data;
+	int pending_ptr, num_of_pending, i;

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 6 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +6 -0

+	struct mv_xor_v2_descriptor *next_pending_hw_desc = NULL;
+	struct mv_xor_v2_sw_desc *next_pending_sw_desc = NULL;
+
+	dev_dbg(xor_dev->dmadev.dev, "%s %d\n", __func__, __LINE__);
+
+	/* get thepending descriptors parameters */

space after the pls

Documentation/devicetree/bindings/dma/mv-xor-v2.txt

Quoted 5 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +5 -0

+static struct platform_driver mv_xor_v2_driver = {
+	.probe		= mv_xor_v2_probe,
+	.remove		= mv_xor_v2_remove,
+	.driver		= {
+		.owner	= THIS_MODULE,

I dont think we need this

Hello Vinod,

Quoted 20 lines · Vinod Koul

Quoted 19 lines · Thomas Petazzoni

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +13 -0

new file mode 100644
index 0000000..0a03dcf
@@ -0,0 +1,19 @@
+* Marvell XOR v2 engines
+
+Required properties:
+- compatible: Should be "marvell,mv-xor-v2"
+- reg: Should contain registers location and length (two sets)
+    the first set is the DMA registers
+    the second set is the global registers
+- msi-parent: Phandle to the MSI-capable interrupt controller used for
+  interrupts.
+
+Example:
+
+   xor0 at 400000 {

Quoted 8 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt line 19

Quoted 6 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +6 -0

+           compatible = "marvell,mv-xor-v2";
+           reg = <0x400000 0x1000>,
+                 <0x410000 0x1000>;
+           msi-parent = <&gic_v2m0>;
+           dma-coherent;
+   };

This should be a separate patch :)

This is really a personal preference, and different maintainers have different opinions on the matter. However, if your preference is to have the DT binding documentation as a separate patch, then I'll split it, no problem :)

Quoted 1 line

So why should it depend on how kernel is configured?

Because using upper_32_bits() on a 32-bits type seems wrong? You can do a git grep for CONFIG_ARCH_DMA_ADDR_T_64BIT, and see that it is already used in drivers/dma/sh/rcar-dmac.c, drivers/gpu/drm/tegra/dc.c or drivers/iommu/tegra-smmu.c.

drivers/dma/sh/rcar-dmac.c uses it for exactly the same reason as me:
they need to know if dma_addr_t is 32 bits or 64 bits, and if it's 64 bits, then they need to take the upper 32 bits and feed them to some register.

Quoted 1 line

You don't want to check the source of interrupt?

To protect against what? An improper MSI message that ends up firing this interrupt for now reason? But only kernel space stuff (or privileged code in user-space) can write to the MSI triggering registers, so we should be able to trust that such an improper MSI message will not arrive.

That being said, I can add a read of the pending descriptors number, and if it's 0, then bail out with IRQ_NONE. Is that what you were thinking about?

Quoted 1 line

why BUG_ON, is the system unusable after this?

Well, the XOR engine cannot do XOR with 0 source buffers, or with more than MV_XOR_V2_CMD_LINE_NUM_MAX_D_BUF.

But I see that I'm already setting max_xor in struct dma_device, so one can assume that ->prep_dma_xor() will not be called with src_cnt == or src_cnt > max_xor, so this BUG_ON can be removed.

Documentation/devicetree/bindings/dma/mv-xor-v2.txt line 19

Quoted 7 lines

Documentation/devicetree/bindings/dma/mv-xor-v2.txt new file +7 -0

+	xor0 at 400000 {
+		compatible = "marvell,mv-xor-v2";
+		reg = <0x400000 0x1000>,
+		      <0x410000 0x1000>;
+		msi-parent = <&gic_v2m0>;
+		dma-coherent;
+	};  

This should be a separate patch :)

drivers/dma/mv_xor_v2.c line 56

Quoted 3 lines

drivers/dma/mv_xor_v2.c new file +3 -0

+#define DMA_DESQ_STOP_OFF	0x800
+#define DMA_DESQ_DEALLOC_OFF	0x804
+#define DMA_DESQ_ADD_OFF	0x808  

Please namespace these and others. Something like MRVL_XOR_XXX or anyother patter you like would be fine...

drivers/dma/mv_xor_v2.c line 84

Quoted 2 lines

drivers/dma/mv_xor_v2.c new file +2 -0

+/* descriptors queue size */
+#define MV_XOR_V2_MAX_DESC_NUM	1024  

Is this hardware defined?

drivers/dma/mv_xor_v2.c line 185

Quoted 3 lines

drivers/dma/mv_xor_v2.c new file +3 -0

+	/*
+	 * fill the buffer's addresses to the descriptor
+	 * The format of the buffers address for 2 sequential buffers X and X+1:  

space around + please.

drivers/dma/mv_xor_v2.c line 200

Quoted 4 lines

drivers/dma/mv_xor_v2.c new file +4 -0

+		/* Set them if we have a 64 bits DMA address */
+		if (IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT))
+			desc->data_buff_addr[arr_index + 2] |=
+				upper_32_bits(src) & 0xFFFF;  

So why should it depend on how kernel is configured?

Quoted 3 lines

+ * Return the next available index in the DESQ.
+ */
+static inline int mv_xor_v2_get_desq_write_ptr(struct mv_xor_v2_device *xor_dev)

Pls wrap with 80 chars wherever it is reasonable

drivers/dma/mv_xor_v2.c line 266

Quoted 8 lines

drivers/dma/mv_xor_v2.c new file +8 -0

+/*
+ * Allocate resources for a channel
+ */
+static int mv_xor_v2_alloc_chan_resources(struct dma_chan *chan)
+{
+	/* nothing to be done here */
+	return 0;
+}  

No need for empty alloc

drivers/dma/mv_xor_v2.c line 274

Quoted 8 lines

drivers/dma/mv_xor_v2.c new file +8 -0

+
+/*
+ * Free resources of a channel
+ */
+void mv_xor_v2_free_chan_resources(struct dma_chan *chan)
+{
+	/* nothing to be done here */
+}  

and free as well :)

drivers/dma/mv_xor_v2.c line 300

Quoted 9 lines

drivers/dma/mv_xor_v2.c new file +9 -0

+static irqreturn_t mv_xor_v2_interrupt_handler(int irq, void *data)
+{
+	struct mv_xor_v2_device *xor_dev = data;
+
+	/*
+	 * Update IMSG threshold, to disable new IMSG interrupts until
+	 * end of the tasklet
+	 */
+	mv_xor_v2_set_imsg_thrd(xor_dev, MV_XOR_V2_MAX_DESC_NUM);  

You don't want to check the source of interrupt?

drivers/dma/mv_xor_v2.c line 332

Quoted 2 lines

drivers/dma/mv_xor_v2.c new file +2 -0

+	/* lock enqueue DESCQ */
+	spin_lock_bh(&xor_dev->push_lock);  

Why not a generic channel lock which is used in submit, issue_pending and tasklet. What is the reason for opting different locks?

drivers/dma/mv_xor_v2.c line 338

Quoted 5 lines

drivers/dma/mv_xor_v2.c new file +5 -0

+	/* get the next available slot in the DESQ */
+	desq_ptr = mv_xor_v2_get_desq_write_ptr(xor_dev);
+
+	/* copy the HW descriptor from the SW descriptor to the DESQ */
+	dest_hw_desc = ((void *)xor_dev->hw_desq_virt +  

cast to and away from void are not required

Quoted 4 lines

drivers/dma/mv_xor_v2.c new file +4 -0

+			(xor_dev->desc_size * desq_ptr));
+
+	memcpy(dest_hw_desc, &sw_desc->hw_desc, xor_dev->desc_size);
+

drivers/dma/mv_xor_v2.c line 347

Quoted 5 lines

drivers/dma/mv_xor_v2.c new file +5 -0

+	/* update the DMA Engine with the new descriptor */
+	mv_xor_v2_add_desc_to_desq(xor_dev, 1);
+
+	/* unlock enqueue DESCQ */
+	spin_unlock_bh(&xor_dev->push_lock);  

and if IIUC, you are pushing this to HW as well, that is not quite right if thats the case. We need to do this in issue_pending

Quoted 4 lines

drivers/dma/mv_xor_v2.c new file +4 -0

+static struct dma_async_tx_descriptor *
+mv_xor_v2_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src,
+		       unsigned int src_cnt, size_t len, unsigned long flags)
+{

drivers/dma/mv_xor_v2.c line 450

Quoted 6 lines

drivers/dma/mv_xor_v2.c new file +6 -0

+	struct mv_xor_v2_sw_desc	*sw_desc;
+	struct mv_xor_v2_descriptor	*hw_descriptor;
+	struct mv_xor_v2_device		*xor_dev;
+	int i;
+
+	BUG_ON(src_cnt > MV_XOR_V2_CMD_LINE_NUM_MAX_D_BUF || src_cnt < 1);  

why BUG_ON, is the system unusable after this?

drivers/dma/mv_xor_v2.c line 536

Quoted 5 lines

drivers/dma/mv_xor_v2.c new file +5 -0

+static enum dma_status mv_xor_v2_tx_status(struct dma_chan *chan,
+		dma_cookie_t cookie, struct dma_tx_state *txstate)
+{
+	/* return the transaction status */
+	return dma_cookie_status(chan, cookie, txstate);  

why not assign this as you status callback?

drivers/dma/mv_xor_v2.c line 548

Quoted 7 lines

drivers/dma/mv_xor_v2.c new file +7 -0

+static void mv_xor_v2_issue_pending(struct dma_chan *chan)
+{
+	struct mv_xor_v2_device *xor_dev =
+		container_of(chan, struct mv_xor_v2_device, dmachan);
+
+	/* Activate the channel */
+	writel(0, xor_dev->dma_base + DMA_DESQ_STOP_OFF);  

what if channel is already running?

Quoted 4 lines

drivers/dma/mv_xor_v2.c new file +4 -0

+static void mv_xor_v2_tasklet(unsigned long data)
+{
+	struct mv_xor_v2_device *xor_dev = (struct mv_xor_v2_device *) data;
+	int pending_ptr, num_of_pending, i;

drivers/dma/mv_xor_v2.c line 580

Quoted 6 lines

drivers/dma/mv_xor_v2.c new file +6 -0

+	struct mv_xor_v2_descriptor *next_pending_hw_desc = NULL;
+	struct mv_xor_v2_sw_desc *next_pending_sw_desc = NULL;
+
+	dev_dbg(xor_dev->dmadev.dev, "%s %d\n", __func__, __LINE__);
+
+	/* get thepending descriptors parameters */  

space after the pls

drivers/dma/mv_xor_v2.c line 870

Quoted 6 lines

drivers/dma/mv_xor_v2.c new file +5 -0

+static struct platform_driver mv_xor_v2_driver = {
+	.probe		= mv_xor_v2_probe,
+	.remove		= mv_xor_v2_remove,
+	.driver		= {
+		.owner	= THIS_MODULE,  

I dont think we need this

Quoted 4 lines

Quoted 4 lines

drivers/dma/mv_xor_v2.c new file +4 -0

+			(xor_dev->desc_size * desq_ptr));
+
+	memcpy(dest_hw_desc, &sw_desc->hw_desc, xor_dev->desc_size);
+

Quoted 8 lines

drivers/dma/mv_xor_v2.c line 347

Quoted 5 lines

drivers/dma/mv_xor_v2.c new file +5 -0

+	/* update the DMA Engine with the new descriptor */
+	mv_xor_v2_add_desc_to_desq(xor_dev, 1);
+
+	/* unlock enqueue DESCQ */
+	spin_unlock_bh(&xor_dev->push_lock);  

and if IIUC, you are pushing this to HW as well, that is not quite right if thats the case. We need to do this in issue_pending

This is probably the only thing that I have not changed. The mv_xor driver is already using the same strategy, and enqueuing in issue_pending() would force us to add the request to a temporary linked list, which would be dequeued in issue_pending(). This is quite a bit of additional processing, while pushing the new requests directly to the engine works fine.

Quoted 6 lines

This is probably the only thing that I have not changed. The mv_xor driver is already using the same strategy, and enqueuing in issue_pending() would force us to add the request to a temporary linked list, which would be dequeued in issue_pending(). This is quite a bit of additional processing, while pushing the new requests directly to the engine works fine.

Well that is wrong! And patch is welcome for mv_xor as well :)

The DMAengine API mandates that we should submit a descriptor to a queue and then push them by invoking issue_pending.

The users are also expected to follow this

Hello,

Quoted 12 lines · Vinod Koul

Quoted 6 lines

This is probably the only thing that I have not changed. The mv_xor driver is already using the same strategy, and enqueuing in issue_pending() would force us to add the request to a temporary linked list, which would be dequeued in issue_pending(). This is quite a bit of additional processing, while pushing the new requests directly to the engine works fine.

Well that is wrong! And patch is welcome for mv_xor as well :)

The DMAengine API mandates that we should submit a descriptor to a queue and then push them by invoking issue_pending.

Quoted 1 line

The users are also expected to follow this

I have just sent a v4 that fixes this for the mv_xor_v2 driver. Now, in ->tx_submit() we only fill in the HW descriptors, but do not tell the engine to process them. It's only at ->issue_pending() time that we tell the engine that there N new descriptors to process.

I'll have a look at mv_xor a bit later.