[PATCHv4 0/6] dmaengine: mv_xor fixes and improvements

[PATCHv4 0/6] dmaengine: mv_xor fixes and improvements

[Thomas Petazzoni]

Hello,

Here is a v4 of the series making some fixes and improvements to the
mv_xor driver. I'd like to see them merged for 4.3 if possible.

Changes v3 -> v4:

 * Rebased on 4.2-rc1.

Changes v2 -> v3:

 * Added Reviewed-by from Maxime Ripard on patches 1, 3, 4 and 5.

 * Added an additional patch improving the performance of the XOR
   engine by only using a subset of the available channels depending
   on the number of CPUs in the system.

Changes v1 -> v2:

 * Device Tree related changes omitted, since they have been applied
   by one of the ARM mach-mvebu maintainer.

 * Addition of one commit, "dmaengine: mv_xor: fix big endian
   operation in register mode", fixing a regression introduced by one
   the patches currently in the 'next' branch of Vinod's
   repository. It should be merged as part of 4.2.

 * Addition of two cleanup patches:

    dmaengine: mv_xor: remove mv_xor_chan->current_type field
    dmaengine: mv_xor: de-duplicate mv_chan_set_mode*()

   These patches, like the two other non-fixes, can either go in 4.2
   if it's not too late, or in 4.3.

Thanks,

Thomas

Thomas Petazzoni (6):
  dmaengine: mv_xor: fix big endian operation in register mode
  dmaengine: mv_xor: add suspend/resume support
  dmaengine: mv_xor: remove support for dmacap,* DT properties
  dmaengine: mv_xor: remove mv_xor_chan->current_type field
  dmaengine: mv_xor: de-duplicate mv_chan_set_mode*()
  dmaengine: mv_xor: optimize performance by using a subset of the XOR
    channels

 Documentation/devicetree/bindings/dma/mv-xor.txt |  10 +-
 drivers/dma/mv_xor.c                             | 121 ++++++++++++++---------
 drivers/dma/mv_xor.h                             |   2 +-
 3 files changed, 81 insertions(+), 52 deletions(-)

-- 
2.4.5

[PATCHv4 1/6] dmaengine: mv_xor: fix big endian operation in register mode

[Thomas Petazzoni]

Commit 6f166312c6ea2 ("dmaengine: mv_xor: add support for a38x command
in descriptor mode") introduced the support for a feature that
appeared in Armada 38x: specifying the operation to be performed in a
per-descriptor basis rather than globally per channel.

However, when doing so, it changed the function mv_chan_set_mode() to
use:

  if (IS_ENABLED(__BIG_ENDIAN))

instead of:

  #if defined(__BIG_ENDIAN)

While IS_ENABLED() is perfectly fine for CONFIG_* symbols, it is not
for other symbols such as __BIG_ENDIAN that is provided directly by
the compiler. Consequently, the commit broke support for big-endian,
as the XOR_DESCRIPTOR_SWAP flag was not set in the XOR channel
configuration register.

The primarily visible effect was some nasty warnings and failures
appearing during the self-test of the XOR unit:

[    1.197368] mv_xor d0060900.xor: error on chan 0. intr cause 0x00000082
[    1.197393] mv_xor d0060900.xor: config       0x00008440
[    1.197410] mv_xor d0060900.xor: activation   0x00000000
[    1.197427] mv_xor d0060900.xor: intr cause   0x00000082
[    1.197443] mv_xor d0060900.xor: intr mask    0x000003f7
[    1.197460] mv_xor d0060900.xor: error cause  0x00000000
[    1.197477] mv_xor d0060900.xor: error addr   0x00000000
[    1.197491] ------------[ cut here ]------------
[    1.197513] WARNING: CPU: 0 PID: 1 at ../drivers/dma/mv_xor.c:664 mv_xor_interrupt_handler+0x14c/0x170()

See also:

  http://storage.kernelci.org/next/next-20150617/arm-mvebu_v7_defconfig+CONFIG_CPU_BIG_ENDIAN=y/lab-khilman/boot-armada-xp-openblocks-ax3-4.txt

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Fixes: 6f166312c6ea2 ("dmaengine: mv_xor: add support for a38x command in descriptor mode")
Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
This fix should go in for 4.2.
---
 drivers/dma/mv_xor.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index fbaf1ea..f1325f6 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -162,10 +162,11 @@ static void mv_chan_set_mode(struct mv_xor_chan *chan,
 	config &= ~0x7;
 	config |= op_mode;
 
-	if (IS_ENABLED(__BIG_ENDIAN))
-		config |= XOR_DESCRIPTOR_SWAP;
-	else
-		config &= ~XOR_DESCRIPTOR_SWAP;
+#if defined(__BIG_ENDIAN)
+	config |= XOR_DESCRIPTOR_SWAP;
+#else
+	config &= ~XOR_DESCRIPTOR_SWAP;
+#endif
 
 	writel_relaxed(config, XOR_CONFIG(chan));
 	chan->current_type = type;
-- 
2.4.5

[PATCHv4 1/6] dmaengine: mv_xor: fix big endian operation in register mode

[Vinod Koul]

On Wed, Jul 08, 2015 at 04:28:14PM +0200, Thomas Petazzoni wrote:
> Commit 6f166312c6ea2 ("dmaengine: mv_xor: add support for a38x command
> in descriptor mode") introduced the support for a feature that
> appeared in Armada 38x: specifying the operation to be performed in a
> per-descriptor basis rather than globally per channel.
> 
> However, when doing so, it changed the function mv_chan_set_mode() to
> use:
> 
>   if (IS_ENABLED(__BIG_ENDIAN))
> 
> instead of:
> 
>   #if defined(__BIG_ENDIAN)
> 
> While IS_ENABLED() is perfectly fine for CONFIG_* symbols, it is not
> for other symbols such as __BIG_ENDIAN that is provided directly by
> the compiler. Consequently, the commit broke support for big-endian,
> as the XOR_DESCRIPTOR_SWAP flag was not set in the XOR channel
> configuration register.
> 
> The primarily visible effect was some nasty warnings and failures
> appearing during the self-test of the XOR unit:
> 
> [    1.197368] mv_xor d0060900.xor: error on chan 0. intr cause 0x00000082
> [    1.197393] mv_xor d0060900.xor: config       0x00008440
> [    1.197410] mv_xor d0060900.xor: activation   0x00000000
> [    1.197427] mv_xor d0060900.xor: intr cause   0x00000082
> [    1.197443] mv_xor d0060900.xor: intr mask    0x000003f7
> [    1.197460] mv_xor d0060900.xor: error cause  0x00000000
> [    1.197477] mv_xor d0060900.xor: error addr   0x00000000
> [    1.197491] ------------[ cut here ]------------
> [    1.197513] WARNING: CPU: 0 PID: 1 at ../drivers/dma/mv_xor.c:664 mv_xor_interrupt_handler+0x14c/0x170()
> 
> See also:
> 
>   http://storage.kernelci.org/next/next-20150617/arm-mvebu_v7_defconfig+CONFIG_CPU_BIG_ENDIAN=y/lab-khilman/boot-armada-xp-openblocks-ax3-4.txt
>

Applied, thanks

-- 
~Vinod

[PATCHv4 2/6] dmaengine: mv_xor: add suspend/resume support

[Thomas Petazzoni]

This commit adds suspend/resume support to the mv_xor driver. The
config and interrupt mask registers must be saved and restored, and
upon resume, the MBus windows configuration must also be done again.

Tested on Armada 388 GP, with a RAID 5 array, accessed before and
after a suspend to RAM cycle.

Based on work from Ofer Heifetz and Lior Amsalem.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/dma/mv_xor.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/dma/mv_xor.h |  1 +
 2 files changed, 48 insertions(+)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index f1325f6..0a3ddc2 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1121,6 +1121,51 @@ mv_xor_conf_mbus_windows(struct mv_xor_device *xordev,
 	writel(0, base + WINDOW_OVERRIDE_CTRL(1));
 }
 
+static int mv_xor_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct mv_xor_device *xordev = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < MV_XOR_MAX_CHANNELS; i++) {
+		struct mv_xor_chan *mv_chan = xordev->channels[i];
+
+		if (!mv_chan)
+			continue;
+
+		mv_chan->saved_config_reg =
+			readl_relaxed(XOR_CONFIG(mv_chan));
+		mv_chan->saved_int_mask_reg =
+			readl_relaxed(XOR_INTR_MASK(mv_chan));
+	}
+
+	return 0;
+}
+
+static int mv_xor_resume(struct platform_device *dev)
+{
+	struct mv_xor_device *xordev = platform_get_drvdata(dev);
+	const struct mbus_dram_target_info *dram;
+	int i;
+
+	for (i = 0; i < MV_XOR_MAX_CHANNELS; i++) {
+		struct mv_xor_chan *mv_chan = xordev->channels[i];
+
+		if (!mv_chan)
+			continue;
+
+		writel_relaxed(mv_chan->saved_config_reg,
+			       XOR_CONFIG(mv_chan));
+		writel_relaxed(mv_chan->saved_int_mask_reg,
+			       XOR_INTR_MASK(mv_chan));
+	}
+
+	dram = mv_mbus_dram_info();
+	if (dram)
+		mv_xor_conf_mbus_windows(xordev, dram);
+
+	return 0;
+}
+
 static const struct of_device_id mv_xor_dt_ids[] = {
 	{ .compatible = "marvell,orion-xor", .data = (void *)XOR_MODE_IN_REG },
 	{ .compatible = "marvell,armada-380-xor", .data = (void *)XOR_MODE_IN_DESC },
@@ -1284,6 +1329,8 @@ static int mv_xor_remove(struct platform_device *pdev)
 static struct platform_driver mv_xor_driver = {
 	.probe		= mv_xor_probe,
 	.remove		= mv_xor_remove,
+	.suspend        = mv_xor_suspend,
+	.resume         = mv_xor_resume,
 	.driver		= {
 		.name	        = MV_XOR_NAME,
 		.of_match_table = of_match_ptr(mv_xor_dt_ids),
diff --git a/drivers/dma/mv_xor.h b/drivers/dma/mv_xor.h
index b7455b4..41a2e8d 100644
--- a/drivers/dma/mv_xor.h
+++ b/drivers/dma/mv_xor.h
@@ -126,6 +126,7 @@ struct mv_xor_chan {
 	char			dummy_src[MV_XOR_MIN_BYTE_COUNT];
 	char			dummy_dst[MV_XOR_MIN_BYTE_COUNT];
 	dma_addr_t		dummy_src_addr, dummy_dst_addr;
+	u32                     saved_config_reg, saved_int_mask_reg;
 };
 
 /**
-- 
2.4.5

[PATCHv4 2/6] dmaengine: mv_xor: add suspend/resume support

[Vinod Koul]

On Wed, Jul 08, 2015 at 04:28:15PM +0200, Thomas Petazzoni wrote:
> This commit adds suspend/resume support to the mv_xor driver. The
> config and interrupt mask registers must be saved and restored, and
> upon resume, the MBus windows configuration must also be done again.
> 
> Tested on Armada 388 GP, with a RAID 5 array, accessed before and
> after a suspend to RAM cycle.
> 
> Based on work from Ofer Heifetz and Lior Amsalem.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/dma/mv_xor.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/dma/mv_xor.h |  1 +
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index f1325f6..0a3ddc2 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1121,6 +1121,51 @@ mv_xor_conf_mbus_windows(struct mv_xor_device *xordev,
>  	writel(0, base + WINDOW_OVERRIDE_CTRL(1));
>  }
>  
> +static int mv_xor_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct mv_xor_device *xordev = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < MV_XOR_MAX_CHANNELS; i++) {
> +		struct mv_xor_chan *mv_chan = xordev->channels[i];
> +
> +		if (!mv_chan)
> +			continue;
> +
> +		mv_chan->saved_config_reg =
> +			readl_relaxed(XOR_CONFIG(mv_chan));
> +		mv_chan->saved_int_mask_reg =
> +			readl_relaxed(XOR_INTR_MASK(mv_chan));
this sound fine, but I am missing the suspension of the channel. If a
transfer was active while suspend was invoked then how would this work?

-- 
~Vinod

> +	}
> +
> +	return 0;
> +}
> +
> +static int mv_xor_resume(struct platform_device *dev)
> +{
> +	struct mv_xor_device *xordev = platform_get_drvdata(dev);
> +	const struct mbus_dram_target_info *dram;
> +	int i;
> +
> +	for (i = 0; i < MV_XOR_MAX_CHANNELS; i++) {
> +		struct mv_xor_chan *mv_chan = xordev->channels[i];
> +
> +		if (!mv_chan)
> +			continue;
> +
> +		writel_relaxed(mv_chan->saved_config_reg,
> +			       XOR_CONFIG(mv_chan));
> +		writel_relaxed(mv_chan->saved_int_mask_reg,
> +			       XOR_INTR_MASK(mv_chan));
> +	}
> +
> +	dram = mv_mbus_dram_info();
> +	if (dram)
> +		mv_xor_conf_mbus_windows(xordev, dram);
> +
> +	return 0;
> +}
> +
>  static const struct of_device_id mv_xor_dt_ids[] = {
>  	{ .compatible = "marvell,orion-xor", .data = (void *)XOR_MODE_IN_REG },
>  	{ .compatible = "marvell,armada-380-xor", .data = (void *)XOR_MODE_IN_DESC },
> @@ -1284,6 +1329,8 @@ static int mv_xor_remove(struct platform_device *pdev)
>  static struct platform_driver mv_xor_driver = {
>  	.probe		= mv_xor_probe,
>  	.remove		= mv_xor_remove,
> +	.suspend        = mv_xor_suspend,
> +	.resume         = mv_xor_resume,
>  	.driver		= {
>  		.name	        = MV_XOR_NAME,
>  		.of_match_table = of_match_ptr(mv_xor_dt_ids),
> diff --git a/drivers/dma/mv_xor.h b/drivers/dma/mv_xor.h
> index b7455b4..41a2e8d 100644
> --- a/drivers/dma/mv_xor.h
> +++ b/drivers/dma/mv_xor.h
> @@ -126,6 +126,7 @@ struct mv_xor_chan {
>  	char			dummy_src[MV_XOR_MIN_BYTE_COUNT];
>  	char			dummy_dst[MV_XOR_MIN_BYTE_COUNT];
>  	dma_addr_t		dummy_src_addr, dummy_dst_addr;
> +	u32                     saved_config_reg, saved_int_mask_reg;
>  };
>  
>  /**
> -- 
> 2.4.5
> 

-- 

[PATCHv4 2/6] dmaengine: mv_xor: add suspend/resume support

[Thomas Petazzoni]

Vinod,

On Wed, 22 Jul 2015 10:40:11 +0530, Vinod Koul wrote:

> > +static int mv_xor_suspend(struct platform_device *pdev, pm_message_t state)
> > +{
> > +	struct mv_xor_device *xordev = platform_get_drvdata(pdev);
> > +	int i;
> > +
> > +	for (i = 0; i < MV_XOR_MAX_CHANNELS; i++) {
> > +		struct mv_xor_chan *mv_chan = xordev->channels[i];
> > +
> > +		if (!mv_chan)
> > +			continue;
> > +
> > +		mv_chan->saved_config_reg =
> > +			readl_relaxed(XOR_CONFIG(mv_chan));
> > +		mv_chan->saved_int_mask_reg =
> > +			readl_relaxed(XOR_INTR_MASK(mv_chan));
> this sound fine, but I am missing the suspension of the channel. If a
> transfer was active while suspend was invoked then how would this work?

There are indeed no provisions to handle this problem. Does the
dmaengine framework provide any helpers to ease with this? I have
quickly looked at other dmaengine drivers, and I haven't seen any of
them taking care of this problem. Do you have a pointer to a driver
handling this problem?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCHv4 2/6] dmaengine: mv_xor: add suspend/resume support

[Vinod Koul]

On Wed, Jul 29, 2015 at 11:34:05AM +0200, Thomas Petazzoni wrote:
> Vinod,
> 
> On Wed, 22 Jul 2015 10:40:11 +0530, Vinod Koul wrote:
> 
> > > +static int mv_xor_suspend(struct platform_device *pdev, pm_message_t state)
> > > +{
> > > +	struct mv_xor_device *xordev = platform_get_drvdata(pdev);
> > > +	int i;
> > > +
> > > +	for (i = 0; i < MV_XOR_MAX_CHANNELS; i++) {
> > > +		struct mv_xor_chan *mv_chan = xordev->channels[i];
> > > +
> > > +		if (!mv_chan)
> > > +			continue;
> > > +
> > > +		mv_chan->saved_config_reg =
> > > +			readl_relaxed(XOR_CONFIG(mv_chan));
> > > +		mv_chan->saved_int_mask_reg =
> > > +			readl_relaxed(XOR_INTR_MASK(mv_chan));
> > this sound fine, but I am missing the suspension of the channel. If a
> > transfer was active while suspend was invoked then how would this work?
> 
> There are indeed no provisions to handle this problem. Does the
> dmaengine framework provide any helpers to ease with this? I have
> quickly looked at other dmaengine drivers, and I haven't seen any of
> them taking care of this problem. Do you have a pointer to a driver
> handling this problem?

Sorry for delay...

Should this be solved at framework level? We should rather suspend the
channels, save the cfg and then restore that on resume.

-- 
~Vinod

[PATCHv4 3/6] dmaengine: mv_xor: remove support for dmacap, * DT properties

[Thomas Petazzoni]

The only reason why we had dmacap,* properties is because back when
DMA_MEMSET was supported, only one out of the two channels per engine
could do a memset operation. But this is something that the driver
already knows anyway, and since then, the DMA_MEMSET support has been
removed.

The driver is already well aware of what each channel supports and the
one to one mapping between Linux specific implementation details (such
as dmacap,interrupt enabling DMA_INTERRUPT) and DT properties is a
good indication that these DT properties are wrong.

Therefore, this commit simply gets rid of these dmacap,* properties,
they are now ignored, and the driver is responsible for knowing the
capabilities of the hardware with regard to the dmaengine subsystem
expectations.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 Documentation/devicetree/bindings/dma/mv-xor.txt | 10 ++++------
 drivers/dma/mv_xor.c                             |  9 +++------
 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/mv-xor.txt b/Documentation/devicetree/bindings/dma/mv-xor.txt
index cc29c35..276ef81 100644
--- a/Documentation/devicetree/bindings/dma/mv-xor.txt
+++ b/Documentation/devicetree/bindings/dma/mv-xor.txt
@@ -12,10 +12,13 @@ XOR engine has. Those sub-nodes have the following required
 properties:
 - interrupts: interrupt of the XOR channel
 
-And the following optional properties:
+The sub-nodes used to contain one or several of the following
+properties, but they are now deprecated:
 - dmacap,memcpy to indicate that the XOR channel is capable of memcpy operations
 - dmacap,memset to indicate that the XOR channel is capable of memset operations
 - dmacap,xor to indicate that the XOR channel is capable of xor operations
+- dmacap,interrupt to indicate that the XOR channel is capable of
+  generating interrupts
 
 Example:
 
@@ -28,13 +31,8 @@ xor at d0060900 {
 
 	xor00 {
 	      interrupts = <51>;
-	      dmacap,memcpy;
-	      dmacap,xor;
 	};
 	xor01 {
 	      interrupts = <52>;
-	      dmacap,memcpy;
-	      dmacap,xor;
-	      dmacap,memset;
 	};
 };
diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 0a3ddc2..6aedc36 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1236,12 +1236,9 @@ static int mv_xor_probe(struct platform_device *pdev)
 			op_in_desc = (int)of_id->data;
 
 			dma_cap_zero(cap_mask);
-			if (of_property_read_bool(np, "dmacap,memcpy"))
-				dma_cap_set(DMA_MEMCPY, cap_mask);
-			if (of_property_read_bool(np, "dmacap,xor"))
-				dma_cap_set(DMA_XOR, cap_mask);
-			if (of_property_read_bool(np, "dmacap,interrupt"))
-				dma_cap_set(DMA_INTERRUPT, cap_mask);
+			dma_cap_set(DMA_MEMCPY, cap_mask);
+			dma_cap_set(DMA_XOR, cap_mask);
+			dma_cap_set(DMA_INTERRUPT, cap_mask);
 
 			irq = irq_of_parse_and_map(np, 0);
 			if (!irq) {
-- 
2.4.5

[PATCHv4 3/6] dmaengine: mv_xor: remove support for dmacap,* DT properties

[Vinod Koul]

On Wed, Jul 08, 2015 at 04:28:16PM +0200, Thomas Petazzoni wrote:
> The only reason why we had dmacap,* properties is because back when
> DMA_MEMSET was supported, only one out of the two channels per engine
> could do a memset operation. But this is something that the driver
> already knows anyway, and since then, the DMA_MEMSET support has been
> removed.
> 
> The driver is already well aware of what each channel supports and the
> one to one mapping between Linux specific implementation details (such
> as dmacap,interrupt enabling DMA_INTERRUPT) and DT properties is a
> good indication that these DT properties are wrong.
> 
> Therefore, this commit simply gets rid of these dmacap,* properties,
> they are now ignored, and the driver is responsible for knowing the
> capabilities of the hardware with regard to the dmaengine subsystem
> expectations.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/dma/mv-xor.txt | 10 ++++------
>  drivers/dma/mv_xor.c                             |  9 +++------
>  2 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/mv-xor.txt b/Documentation/devicetree/bindings/dma/mv-xor.txt
> index cc29c35..276ef81 100644
> --- a/Documentation/devicetree/bindings/dma/mv-xor.txt
> +++ b/Documentation/devicetree/bindings/dma/mv-xor.txt
> @@ -12,10 +12,13 @@ XOR engine has. Those sub-nodes have the following required
>  properties:
>  - interrupts: interrupt of the XOR channel
>  
> -And the following optional properties:
> +The sub-nodes used to contain one or several of the following
> +properties, but they are now deprecated:
>  - dmacap,memcpy to indicate that the XOR channel is capable of memcpy operations
>  - dmacap,memset to indicate that the XOR channel is capable of memset operations
>  - dmacap,xor to indicate that the XOR channel is capable of xor operations
> +- dmacap,interrupt to indicate that the XOR channel is capable of
> +  generating interrupts
>  
>  Example:
>  
> @@ -28,13 +31,8 @@ xor at d0060900 {
>  
>  	xor00 {
>  	      interrupts = <51>;
> -	      dmacap,memcpy;
> -	      dmacap,xor;
>  	};
>  	xor01 {
>  	      interrupts = <52>;
> -	      dmacap,memcpy;
> -	      dmacap,xor;
> -	      dmacap,memset;
I maybe wrong but there was an assumption that DT properties, even if bad
are always there as they need to be treated as kernel ABI.

How will it work if someone has older DT implementation or older driver?

Arnd, what are the rules here and are they documented anywhere?

-- 
~Vinod
>  	};
>  };
> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 0a3ddc2..6aedc36 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1236,12 +1236,9 @@ static int mv_xor_probe(struct platform_device *pdev)
>  			op_in_desc = (int)of_id->data;
>  
>  			dma_cap_zero(cap_mask);
> -			if (of_property_read_bool(np, "dmacap,memcpy"))
> -				dma_cap_set(DMA_MEMCPY, cap_mask);
> -			if (of_property_read_bool(np, "dmacap,xor"))
> -				dma_cap_set(DMA_XOR, cap_mask);
> -			if (of_property_read_bool(np, "dmacap,interrupt"))
> -				dma_cap_set(DMA_INTERRUPT, cap_mask);
> +			dma_cap_set(DMA_MEMCPY, cap_mask);
> +			dma_cap_set(DMA_XOR, cap_mask);
> +			dma_cap_set(DMA_INTERRUPT, cap_mask);
>  
>  			irq = irq_of_parse_and_map(np, 0);
>  			if (!irq) {
> -- 
> 2.4.5
> 

-- 

[PATCHv4 3/6] dmaengine: mv_xor: remove support for dmacap,* DT properties

[Vinod Koul]

On Wed, Jul 22, 2015 at 10:46:53AM +0530, Vinod Koul wrote:
> On Wed, Jul 08, 2015 at 04:28:16PM +0200, Thomas Petazzoni wrote:

> > @@ -28,13 +31,8 @@ xor at d0060900 {
> >  
> >  	xor00 {
> >  	      interrupts = <51>;
> > -	      dmacap,memcpy;
> > -	      dmacap,xor;
> >  	};
> >  	xor01 {
> >  	      interrupts = <52>;
> > -	      dmacap,memcpy;
> > -	      dmacap,xor;
> > -	      dmacap,memset;
> I maybe wrong but there was an assumption that DT properties, even if bad
> are always there as they need to be treated as kernel ABI.
> 
> How will it work if someone has older DT implementation or older driver?
> 
> Arnd, what are the rules here and are they documented anywhere?
> 
Okay found one:
Documentation/devicetree/bindings/ABI.txt

-- 
~Vinod

[PATCHv4 3/6] dmaengine: mv_xor: remove support for dmacap,* DT properties

[Thomas Petazzoni]

Vinod,

On Wed, 22 Jul 2015 10:46:53 +0530, Vinod Koul wrote:

> >  	xor01 {
> >  	      interrupts = <52>;
> > -	      dmacap,memcpy;
> > -	      dmacap,xor;
> > -	      dmacap,memset;
> I maybe wrong but there was an assumption that DT properties, even if bad
> are always there as they need to be treated as kernel ABI.
> 
> How will it work if someone has older DT implementation or older driver?

Supporting a new DT with an old kernel has never been part of the
requirements of the DT ABI stability.

Supporting an old DT with a new kernel is the actual requirement. And
the patch I'm proposing does not break this at all: a new kernel will
simply ignore those dmacap,* properties from an old DT, and the driver
automatically knows by itself what are the capabilities of the
different XOR engines.

Therefore, there is no backward compatibility issue introduced by this
patch.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCHv4 3/6] dmaengine: mv_xor: remove support for dmacap,* DT properties

[Vinod Koul]

On Mon, Jul 27, 2015 at 12:44:05PM +0200, Thomas Petazzoni wrote:
> Vinod,
> 
> On Wed, 22 Jul 2015 10:46:53 +0530, Vinod Koul wrote:
> 
> > >  	xor01 {
> > >  	      interrupts = <52>;
> > > -	      dmacap,memcpy;
> > > -	      dmacap,xor;
> > > -	      dmacap,memset;
> > I maybe wrong but there was an assumption that DT properties, even if bad
> > are always there as they need to be treated as kernel ABI.
> > 
> > How will it work if someone has older DT implementation or older driver?
> 
> Supporting a new DT with an old kernel has never been part of the
> requirements of the DT ABI stability.
> 
> Supporting an old DT with a new kernel is the actual requirement. And
> the patch I'm proposing does not break this at all: a new kernel will
> simply ignore those dmacap,* properties from an old DT, and the driver
> automatically knows by itself what are the capabilities of the
> different XOR engines.
> 
> Therefore, there is no backward compatibility issue introduced by this
> patch.

Yes tht is correct

-- 
~Vinod

[PATCHv4 3/6] dmaengine: mv_xor: remove support for dmacap,* DT properties

[Vinod Koul]

On Wed, Jul 08, 2015 at 04:28:16PM +0200, Thomas Petazzoni wrote:
> The only reason why we had dmacap,* properties is because back when
> DMA_MEMSET was supported, only one out of the two channels per engine
> could do a memset operation. But this is something that the driver
> already knows anyway, and since then, the DMA_MEMSET support has been
> removed.
> 
> The driver is already well aware of what each channel supports and the
> one to one mapping between Linux specific implementation details (such
> as dmacap,interrupt enabling DMA_INTERRUPT) and DT properties is a
> good indication that these DT properties are wrong.
> 
> Therefore, this commit simply gets rid of these dmacap,* properties,
> they are now ignored, and the driver is responsible for knowing the
> capabilities of the hardware with regard to the dmaengine subsystem
> expectations.

Applied, thanks

-- 
~Vinod

[PATCHv4 4/6] dmaengine: mv_xor: remove mv_xor_chan->current_type field

[Thomas Petazzoni]

Since commit 3e4f52e2da9f6 ("dma: mv_xor: Simplify the DMA_MEMCPY
operation"), this field is no longer used, so get rid of it.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/dma/mv_xor.c | 1 -
 drivers/dma/mv_xor.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 6aedc36..1fe3e33 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -169,7 +169,6 @@ static void mv_chan_set_mode(struct mv_xor_chan *chan,
 #endif
 
 	writel_relaxed(config, XOR_CONFIG(chan));
-	chan->current_type = type;
 }
 
 static void mv_chan_set_mode_to_desc(struct mv_xor_chan *chan)
diff --git a/drivers/dma/mv_xor.h b/drivers/dma/mv_xor.h
index 41a2e8d..c19fe30 100644
--- a/drivers/dma/mv_xor.h
+++ b/drivers/dma/mv_xor.h
@@ -110,7 +110,6 @@ struct mv_xor_chan {
 	void __iomem		*mmr_high_base;
 	unsigned int		idx;
 	int                     irq;
-	enum dma_transaction_type	current_type;
 	struct list_head	chain;
 	struct list_head	free_slots;
 	struct list_head	allocated_slots;
-- 
2.4.5

[PATCHv4 4/6] dmaengine: mv_xor: remove mv_xor_chan->current_type field

[Vinod Koul]

On Wed, Jul 08, 2015 at 04:28:17PM +0200, Thomas Petazzoni wrote:
> Since commit 3e4f52e2da9f6 ("dma: mv_xor: Simplify the DMA_MEMCPY
> operation"), this field is no longer used, so get rid of it.

This one fails to apply, can you pls rebase

-- 
~Vinod

[PATCHv4 5/6] dmaengine: mv_xor: de-duplicate mv_chan_set_mode*()

[Thomas Petazzoni]

When commit 6f166312c6ea2 ("dmaengine: mv_xor: add support for a38x
command in descriptor mode") added support for the descriptor mode
available in Marvell Armada 38x and later SoCs, it added a new
function mv_chan_set_mode_to_desc() which allows to configure a XOR
channel to get the specific operation to be done from each individual
DMA descriptor.

However, this function was mainly a duplicate of the existing
mv_chan_set_mode(), with just the operation being different.

This commit re-organizes the code into a single mv_chan_set_mode()
function, which takes the operation mode as argument, and the
mv_xor_channel_add() function decides whether to use
XOR_OPERATION_MODE_IN_DESC or XOR_OPERATION_MODE_XOR.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/dma/mv_xor.c | 41 +++--------------------------------------
 1 file changed, 3 insertions(+), 38 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 1fe3e33..6e09d59 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -139,45 +139,10 @@ static void mv_chan_clear_err_status(struct mv_xor_chan *chan)
 }
 
 static void mv_chan_set_mode(struct mv_xor_chan *chan,
-			     enum dma_transaction_type type)
+			     u32 op_mode)
 {
-	u32 op_mode;
 	u32 config = readl_relaxed(XOR_CONFIG(chan));
 
-	switch (type) {
-	case DMA_XOR:
-		op_mode = XOR_OPERATION_MODE_XOR;
-		break;
-	case DMA_MEMCPY:
-		op_mode = XOR_OPERATION_MODE_MEMCPY;
-		break;
-	default:
-		dev_err(mv_chan_to_devp(chan),
-			"error: unsupported operation %d\n",
-			type);
-		BUG();
-		return;
-	}
-
-	config &= ~0x7;
-	config |= op_mode;
-
-#if defined(__BIG_ENDIAN)
-	config |= XOR_DESCRIPTOR_SWAP;
-#else
-	config &= ~XOR_DESCRIPTOR_SWAP;
-#endif
-
-	writel_relaxed(config, XOR_CONFIG(chan));
-}
-
-static void mv_chan_set_mode_to_desc(struct mv_xor_chan *chan)
-{
-	u32 op_mode;
-	u32 config = readl_relaxed(XOR_CONFIG(chan));
-
-	op_mode = XOR_OPERATION_MODE_IN_DESC;
-
 	config &= ~0x7;
 	config |= op_mode;
 
@@ -1042,9 +1007,9 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
 	mv_chan_unmask_interrupts(mv_chan);
 
 	if (mv_chan->op_in_desc == XOR_MODE_IN_DESC)
-		mv_chan_set_mode_to_desc(mv_chan);
+		mv_chan_set_mode(mv_chan, XOR_OPERATION_MODE_IN_DESC);
 	else
-		mv_chan_set_mode(mv_chan, DMA_XOR);
+		mv_chan_set_mode(mv_chan, XOR_OPERATION_MODE_XOR);
 
 	spin_lock_init(&mv_chan->lock);
 	INIT_LIST_HEAD(&mv_chan->chain);
-- 
2.4.5

[PATCHv4 5/6] dmaengine: mv_xor: de-duplicate mv_chan_set_mode*()

[Vinod Koul]

On Wed, Jul 08, 2015 at 04:28:18PM +0200, Thomas Petazzoni wrote:
> When commit 6f166312c6ea2 ("dmaengine: mv_xor: add support for a38x
> command in descriptor mode") added support for the descriptor mode
> available in Marvell Armada 38x and later SoCs, it added a new
> function mv_chan_set_mode_to_desc() which allows to configure a XOR
> channel to get the specific operation to be done from each individual
> DMA descriptor.
> 
> However, this function was mainly a duplicate of the existing
> mv_chan_set_mode(), with just the operation being different.
> 
> This commit re-organizes the code into a single mv_chan_set_mode()
> function, which takes the operation mode as argument, and the
> mv_xor_channel_add() function decides whether to use
> XOR_OPERATION_MODE_IN_DESC or XOR_OPERATION_MODE_XOR.

This one as well

-- 
~Vinod

[PATCHv4 6/6] dmaengine: mv_xor: optimize performance by using a subset of the XOR channels

[Thomas Petazzoni]

Due to how async_tx behaves internally, having more XOR channels than
CPUs is actually hurting performance more than it improves it, because
memcpy requests get scheduled on a different channel than the XOR
requests, but async_tx will still wait for the completion of the
memcpy requests before scheduling the XOR requests.

It is in fact more efficient to have at most one channel per CPU,
which this patch implements by limiting the number of channels per
engine, and the number of engines registered depending on the number
of availables CPUs.

Marvell platforms are currently available in one CPU, two CPUs and
four CPUs configurations:

 - in the configurations with one CPU, only one channel from one
   engine is used.

 - in the configurations with two CPUs, only one channel from each
   engine is used (they are two XOR engines)

 - in the configurations with four CPUs, both channels of both engines
   are used.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/dma/mv_xor.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 6e09d59..2cdfca7 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -26,6 +26,7 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/irqdomain.h>
+#include <linux/cpumask.h>
 #include <linux/platform_data/dma-mv_xor.h>
 
 #include "dmaengine.h"
@@ -1137,12 +1138,15 @@ static const struct of_device_id mv_xor_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, mv_xor_dt_ids);
 
+static unsigned int mv_xor_engine_count;
+
 static int mv_xor_probe(struct platform_device *pdev)
 {
 	const struct mbus_dram_target_info *dram;
 	struct mv_xor_device *xordev;
 	struct mv_xor_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct resource *res;
+	unsigned int max_engines, max_channels;
 	int i, ret;
 	int op_in_desc;
 
@@ -1186,6 +1190,21 @@ static int mv_xor_probe(struct platform_device *pdev)
 	if (!IS_ERR(xordev->clk))
 		clk_prepare_enable(xordev->clk);
 
+	/*
+	 * We don't want to have more than one channel per CPU in
+	 * order for async_tx to perform well. So we limit the number
+	 * of engines and channels so that we take into account this
+	 * constraint. Note that we also want to use channels from
+	 * separate engines when possible.
+	 */
+	max_engines = num_present_cpus();
+	max_channels = min_t(unsigned int,
+			     MV_XOR_MAX_CHANNELS,
+			     DIV_ROUND_UP(num_present_cpus(), 2));
+
+	if (mv_xor_engine_count >= max_engines)
+		return 0;
+
 	if (pdev->dev.of_node) {
 		struct device_node *np;
 		int i = 0;
@@ -1199,6 +1218,9 @@ static int mv_xor_probe(struct platform_device *pdev)
 			int irq;
 			op_in_desc = (int)of_id->data;
 
+			if (i >= max_channels)
+				continue;
+
 			dma_cap_zero(cap_mask);
 			dma_cap_set(DMA_MEMCPY, cap_mask);
 			dma_cap_set(DMA_XOR, cap_mask);
@@ -1222,7 +1244,7 @@ static int mv_xor_probe(struct platform_device *pdev)
 			i++;
 		}
 	} else if (pdata && pdata->channels) {
-		for (i = 0; i < MV_XOR_MAX_CHANNELS; i++) {
+		for (i = 0; i < max_channels; i++) {
 			struct mv_xor_channel_data *cd;
 			struct mv_xor_chan *chan;
 			int irq;
-- 
2.4.5

[PATCHv4 6/6] dmaengine: mv_xor: optimize performance by using a subset of the XOR channels

[Vinod Koul]

On Wed, Jul 08, 2015 at 04:28:19PM +0200, Thomas Petazzoni wrote:
> Due to how async_tx behaves internally, having more XOR channels than
> CPUs is actually hurting performance more than it improves it, because
> memcpy requests get scheduled on a different channel than the XOR
> requests, but async_tx will still wait for the completion of the
> memcpy requests before scheduling the XOR requests.
> 
> It is in fact more efficient to have at most one channel per CPU,
> which this patch implements by limiting the number of channels per
> engine, and the number of engines registered depending on the number
> of availables CPUs.
> 
> Marvell platforms are currently available in one CPU, two CPUs and
> four CPUs configurations:
> 
>  - in the configurations with one CPU, only one channel from one
>    engine is used.
> 
>  - in the configurations with two CPUs, only one channel from each
>    engine is used (they are two XOR engines)
> 
>  - in the configurations with four CPUs, both channels of both engines
>    are used.

Applied, thanks

-- 
~Vinod

[PATCHv4 0/6] dmaengine: mv_xor fixes and improvements

[Andrew Lunn]

On Wed, Jul 08, 2015 at 04:28:13PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> Here is a v4 of the series making some fixes and improvements to the
> mv_xor driver. I'd like to see them merged for 4.3 if possible.
> 
> Changes v3 -> v4:
> 
>  * Rebased on 4.2-rc1.

Hi Thomas

It is unlikely i'm going to retest on Kirkwood. So please feel free to
add my Tested-by: i sent for v3 to the v4/v5...

    Andrew