dmaengine: mv_xor fixes and improvements
Subsystems (2)
v3 cover dmaengine: mv_xor fixes and improvements
Hello,
Here is a v3 of the series making some fixes and improvements to the mv_xor driver.
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(-)
Signature · 1 line
2.4.5
PATCH v3 1/6 dmaengine: mv_xor: fix big endian operation in register mode
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:
---
This fix should go in for 4.2.
--- drivers/dma/mv_xor.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/dma/mv_xor.c +5 -4
@@ -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;
Signature · 1 line
2.4.5
PATCH v3 1/6 dmaengine: mv_xor: fix big endian operation in register mode
Hi Thomas
I ran the dmatest module on a kirkwood system. No obvious regressions.
Andrew
Quoted 74 lines · 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_ENDIANthat is provided directly by the compiler. Consequently, the commit broke support for big-endian, as theXOR_DESCRIPTOR_SWAPflag 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:
Signed-off-by Thomas Petazzoni <[email protected]>Fixes 6f166312c6ea2 ("dmaengine: mv_xor: add support for a38x command in descriptor mode")Reviewed-by Maxime Ripard <[email protected]>---
This fix should go in for 4.2.--- drivers/dma/mv_xor.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)drivers/dma/mv_xor.c +5
-4@@ -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;Signature · 7 lines
2.4.5 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v3 2/6 dmaengine: mv_xor: add suspend/resume support
In reply to Thomas PetazzoniThis 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.
--- drivers/dma/mv_xor.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ drivers/dma/mv_xor.h | 1 + 2 files changed, 48 insertions(+)
drivers/dma/mv_xor.c +47 -0
@@ -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),
drivers/dma/mv_xor.h +1 -0
@@ -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; };/**
Signature · 1 line
2.4.5
PATCH v3 3/6 dmaengine: mv_xor: remove support for dmacap, * DT properties
In reply to Thomas PetazzoniThe 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.
--- Documentation/devicetree/bindings/dma/mv-xor.txt | 10 ++++------ drivers/dma/mv_xor.c | 9 +++------ 2 files changed, 7 insertions(+), 12 deletions(-)
Documentation/devicetree/bindings/dma/mv-xor.txt +4 -6
@@ -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; }; };
drivers/dma/mv_xor.c +3 -6
@@ -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) {
Signature · 1 line
2.4.5
PATCH v3 4/6 dmaengine: mv_xor: remove mv_xor_chan->current_type field
In reply to Thomas PetazzoniSince commit 3e4f52e2da9f6 ("dma: mv_xor: Simplify the DMA_MEMCPY operation"), this field is no longer used, so get rid of it.
--- drivers/dma/mv_xor.c | 1 - drivers/dma/mv_xor.h | 1 - 2 files changed, 2 deletions(-)
drivers/dma/mv_xor.c +0 -1
@@ -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)
drivers/dma/mv_xor.h +0 -1
@@ -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;
Signature · 1 line
2.4.5
PATCH v3 5/6 dmaengine: mv_xor: de-duplicate mv_chan_set_mode*()
In reply to Thomas PetazzoniWhen 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.
--- drivers/dma/mv_xor.c | 41 +++-------------------------------------- 1 file changed, 3 insertions(+), 38 deletions(-)
drivers/dma/mv_xor.c +3 -38
@@ -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);
Signature · 1 line
2.4.5
PATCH v3 6/6 dmaengine: mv_xor: optimize performance by using a subset of the XOR channels
In reply to Thomas PetazzoniDue 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.
--- drivers/dma/mv_xor.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)
drivers/dma/mv_xor.c +23 -1
@@ -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;
Signature · 1 line
2.4.5