Re: [PATCH 4/4] DMA: PL330: add device tree property for DMA_MEMCPYcapability
From: Bartlomiej Zolnierkiewicz
Date: Fri Nov 30 2012 - 06:08:17 EST
On Friday 09 November 2012 07:11:30 Jassi Brar wrote:
> On 30 October 2012 14:51, Bartlomiej Zolnierkiewicz
> <b.zolnierkie@xxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Monday 29 October 2012 22:45:48 Jassi Brar wrote:
> >> On Mon, Oct 29, 2012 at 10:59 AM, Bartlomiej Zolnierkiewicz
> >> <b.zolnierkie@xxxxxxxxxxx> wrote:
> >> > * Add device tree (DT) property ("pl330,dma-memcpy") for DMA_MEMCPY
> >> > capability and instead of setting this capability unconditionally
> >> > in pl330_probe() do it only when property is present.
> >> >
> >> Perhaps we should pass the array of peripheral interfaces via DT, the
> >> lack of which could imply MEMCPY capability ? (while it works, I doubt
> >> if pl330 is supposed to have SLAVE and MEMCPY capabilities in any
> >> instance)
> >
> > In case of PL330 on EXYNOS4 we have two interfaces with SLAVE capability
> > and one interface with MEMCPY capability. Could you please explain more
> > the idea of passing the array of peripherals through DT so we can detect
> > which interface has MEMCPY capability?
> >
> The DT node of a 'pdma' should have the array of indices of
> peripherals it caters to (what is currently peri_id of 'struct
> dma_pl330_platdata'). The array would be missing in the DT node of
> 'mdma' since all channels are equal.
> During probe if the array, say as property 'peri_map', is missing from
> DT node of the dmac, that would imply the dmac is 'mdma' and hence the
> pl330.c sets DMA_MEMCPY in its cap_mask. Otherwise the peri_map
> implies a 'pdma' and hence SLAVE|CYCLIC is set.
>
>
> >> That would also be a step towards discarding "struct dma_pl330_platdata".
> >
> > I don't know if getting rid of "struct dma_pl330_platdata" is possible
> > but we still need to come up with some way to pass the needed information
> > through DT. Do you have an idea how it could be done?
> >
> struct dma_pl330_platdata {
> u8 nr_valid_peri;
> u8 *peri_id;
> As explain above, these two should move to DT node of the dma controller.
>
> dma_cap_mask_t cap_mask;
> Should be set in pl330.c : MEMCPY for mdma, SLAVE|CYCLIC for pdma
>
> unsigned mcbuf_sz;
> Currently unused and already safe enough default value set in driver.
> }
Thank you for explaining it. Here is a patch implementing the idea:
From: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
Subject: [PATCH] DMA: PL330: add peripherals map to the device tree
Add device tree (DT) property ("peri-map") for storing indices
of peripherals connected to DMAC and fix DT nodes of client
drivers to use 'dma peripheral id' instead of 'dma request id'.
Also instead of setting DMA_MEMCPY capability unconditionally in
pl330_probe() do it only when "peri-map" DT property is present
(idea from Jassi Brar). It fixes the issue on ARM EXYNOS
platforms using DT where pdma controller erroneously was used
for DMA_MEMCPY operations instead of mdma one (it seems to work
correctly but at the cost of worse performance).
While at it:
- add missing kfree() to pl330_[probe,remove]()
- fix typo in samsung_dmadev_request()
Cc: Jassi Brar <jassisinghbrar@xxxxxxxxx>
Cc: Vinod Koul <vinod.koul@xxxxxxxxx>
Cc: Kukjin Kim <kgene.kim@xxxxxxxxxxx>
Cc: Rob Herring <rob.herring@xxxxxxxxxxx>
Cc: Dinh Nguyen <dinguyen@xxxxxxxxxx>
Cc: Pawel Moll <pawel.moll@xxxxxxx>
Cc: Tomasz Figa <t.figa@xxxxxxxxxxx>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
---
I wonder whether "peri-map" also needs to be added to following files:
arch/arm/boot/dts/highbank.dts
arch/arm/boot/dts/socfpga.dtsi
arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
(since they're also using pl330)?
Documentation/devicetree/bindings/dma/arm-pl330.txt | 5
arch/arm/boot/dts/exynos4.dtsi | 21 +-
arch/arm/boot/dts/exynos5250.dtsi | 20 +-
arch/arm/plat-samsung/dma-ops.c | 2
arch/arm/plat-samsung/include/plat/dma-pl330.h | 155 +++++++++-----------
drivers/dma/pl330.c | 54 +++++-
6 files changed, 152 insertions(+), 105 deletions(-)
Index: b/Documentation/devicetree/bindings/dma/arm-pl330.txt
===================================================================
--- a/Documentation/devicetree/bindings/dma/arm-pl330.txt 2012-11-28 17:41:36.997626033 +0100
+++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt 2012-11-28 17:42:23.433626905 +0100
@@ -11,6 +11,7 @@ Required properties:
Optional properties:
- dma-coherent : Present if dma operations are coherent
+- peri-map : An array of indices of peripherals connected to DMAC
Example:
@@ -24,9 +25,9 @@ Client drivers (device nodes requiring d
mem-to-dev) should specify the DMA channel numbers using a two-value pair
as shown below.
- [property name] = <[phandle of the dma controller] [dma request id]>;
+ [property name] = <[phandle of the dma controller] [dma peripheral id]>;
- where 'dma request id' is the dma request number which is connected
+ where 'dma peripheral id' is the id of peripheral which is connected
to the client controller. The 'property name' is recommended to be
of the form <name>-dma-channel.
Index: b/arch/arm/boot/dts/exynos4.dtsi
===================================================================
--- a/arch/arm/boot/dts/exynos4.dtsi 2012-11-28 17:41:37.033626034 +0100
+++ b/arch/arm/boot/dts/exynos4.dtsi 2012-11-28 17:42:23.433626905 +0100
@@ -256,8 +256,8 @@
compatible = "samsung,exynos4210-spi";
reg = <0x13920000 0x100>;
interrupts = <0 66 0>;
- tx-dma-channel = <&pdma0 7>; /* preliminary */
- rx-dma-channel = <&pdma0 6>; /* preliminary */
+ tx-dma-channel = <&pdma0 23>;
+ rx-dma-channel = <&pdma0 22>;
#address-cells = <1>;
#size-cells = <0>;
status = "disabled";
@@ -267,8 +267,8 @@
compatible = "samsung,exynos4210-spi";
reg = <0x13930000 0x100>;
interrupts = <0 67 0>;
- tx-dma-channel = <&pdma1 7>; /* preliminary */
- rx-dma-channel = <&pdma1 6>; /* preliminary */
+ tx-dma-channel = <&pdma1 25>;
+ rx-dma-channel = <&pdma1 24>;
#address-cells = <1>;
#size-cells = <0>;
status = "disabled";
@@ -278,8 +278,8 @@
compatible = "samsung,exynos4210-spi";
reg = <0x13940000 0x100>;
interrupts = <0 68 0>;
- tx-dma-channel = <&pdma0 9>; /* preliminary */
- rx-dma-channel = <&pdma0 8>; /* preliminary */
+ tx-dma-channel = <&pdma0 27>;
+ rx-dma-channel = <&pdma0 26>;
#address-cells = <1>;
#size-cells = <0>;
status = "disabled";
@@ -303,12 +303,21 @@
compatible = "arm,pl330", "arm,primecell";
reg = <0x12680000 0x1000>;
interrupts = <0 35 0>;
+ peri-map = < 37 36 41 40 60 61 22 23 26 27
+ 17 15 16 20 21 0 1 4 5 8
+ 9 46 47 52 53 56 57 28 29 30
+ 64 65 >;
+
};
pdma1: pdma@12690000 {
compatible = "arm,pl330", "arm,primecell";
reg = <0x12690000 0x1000>;
interrupts = <0 36 0>;
+ peri-map = < 37 36 39 38 62 63 24 25 17 15
+ 16 18 19 0 1 2 3 6 7 50
+ 51 54 55 58 59 48 49 33 66 67 >;
+
};
mdma1: mdma@12850000 {
Index: b/arch/arm/boot/dts/exynos5250.dtsi
===================================================================
--- a/arch/arm/boot/dts/exynos5250.dtsi 2012-11-28 17:41:37.021626034 +0100
+++ b/arch/arm/boot/dts/exynos5250.dtsi 2012-11-28 17:42:23.433626905 +0100
@@ -160,8 +160,8 @@
compatible = "samsung,exynos4210-spi";
reg = <0x12d20000 0x100>;
interrupts = <0 66 0>;
- tx-dma-channel = <&pdma0 5>; /* preliminary */
- rx-dma-channel = <&pdma0 4>; /* preliminary */
+ tx-dma-channel = <&pdma0 23>;
+ rx-dma-channel = <&pdma0 22>;
#address-cells = <1>;
#size-cells = <0>;
};
@@ -170,8 +170,8 @@
compatible = "samsung,exynos4210-spi";
reg = <0x12d30000 0x100>;
interrupts = <0 67 0>;
- tx-dma-channel = <&pdma1 5>; /* preliminary */
- rx-dma-channel = <&pdma1 4>; /* preliminary */
+ tx-dma-channel = <&pdma1 25>;
+ rx-dma-channel = <&pdma1 24>;
#address-cells = <1>;
#size-cells = <0>;
};
@@ -180,8 +180,8 @@
compatible = "samsung,exynos4210-spi";
reg = <0x12d40000 0x100>;
interrupts = <0 68 0>;
- tx-dma-channel = <&pdma0 7>; /* preliminary */
- rx-dma-channel = <&pdma0 6>; /* preliminary */
+ tx-dma-channel = <&pdma0 27>;
+ rx-dma-channel = <&pdma0 26>;
#address-cells = <1>;
#size-cells = <0>;
};
@@ -229,12 +229,20 @@
compatible = "arm,pl330", "arm,primecell";
reg = <0x121A0000 0x1000>;
interrupts = <0 34 0>;
+ peri-map = < 37 36 41 40 22 23 26 27 17 15
+ 16 20 21 0 1 4 5 8 9 46
+ 47 52 53 56 57 28 29 30 60 62
+ 64 66 >;
};
pdma1: pdma@121B0000 {
compatible = "arm,pl330", "arm,primecell";
reg = <0x121B0000 0x1000>;
interrupts = <0 35 0>;
+ peri-map = < 37 36 39 38 24 25 32 33 17 15
+ 16 18 19 0 1 2 3 6 7 50
+ 51 54 55 58 59 48 49 68 61 63
+ 65 67 >;
};
mdma0: mdma@10800000 {
Index: b/arch/arm/plat-samsung/dma-ops.c
===================================================================
--- a/arch/arm/plat-samsung/dma-ops.c 2012-11-28 17:41:37.057626035 +0100
+++ b/arch/arm/plat-samsung/dma-ops.c 2012-11-28 17:42:23.433626905 +0100
@@ -29,7 +29,7 @@ static unsigned samsung_dmadev_request(e
/*
* If a dma channel property of a device node from device tree is
- * specified, use that as the fliter parameter.
+ * specified, use that as the filter parameter.
*/
filter_param = (dma_ch == DMACH_DT_PROP) ?
(void *)param->dt_dmach_prop : (void *)dma_ch;
Index: b/arch/arm/plat-samsung/include/plat/dma-pl330.h
===================================================================
--- a/arch/arm/plat-samsung/include/plat/dma-pl330.h 2012-11-28 17:41:37.045626034 +0100
+++ b/arch/arm/plat-samsung/include/plat/dma-pl330.h 2012-11-28 17:42:23.433626905 +0100
@@ -17,88 +17,87 @@
* For the sake of consistency across client drivers,
* We keep the channel names unchanged and only add
* missing peripherals are added.
- * Order is not important since DMA PL330 API driver
- * use these just as IDs.
+ * Order is important since IDs are used by device tree.
*/
enum dma_ch {
DMACH_DT_PROP = -1,
DMACH_UART0_RX = 0,
- DMACH_UART0_TX,
- DMACH_UART1_RX,
- DMACH_UART1_TX,
- DMACH_UART2_RX,
- DMACH_UART2_TX,
- DMACH_UART3_RX,
- DMACH_UART3_TX,
- DMACH_UART4_RX,
- DMACH_UART4_TX,
- DMACH_UART5_RX,
- DMACH_UART5_TX,
- DMACH_USI_RX,
- DMACH_USI_TX,
- DMACH_IRDA,
- DMACH_I2S0_RX,
- DMACH_I2S0_TX,
- DMACH_I2S0S_TX,
- DMACH_I2S1_RX,
- DMACH_I2S1_TX,
- DMACH_I2S2_RX,
- DMACH_I2S2_TX,
- DMACH_SPI0_RX,
- DMACH_SPI0_TX,
- DMACH_SPI1_RX,
- DMACH_SPI1_TX,
- DMACH_SPI2_RX,
- DMACH_SPI2_TX,
- DMACH_AC97_MICIN,
- DMACH_AC97_PCMIN,
- DMACH_AC97_PCMOUT,
- DMACH_EXTERNAL,
- DMACH_PWM,
- DMACH_SPDIF,
- DMACH_HSI_RX,
- DMACH_HSI_TX,
- DMACH_PCM0_TX,
- DMACH_PCM0_RX,
- DMACH_PCM1_TX,
- DMACH_PCM1_RX,
- DMACH_PCM2_TX,
- DMACH_PCM2_RX,
- DMACH_MSM_REQ3,
- DMACH_MSM_REQ2,
- DMACH_MSM_REQ1,
- DMACH_MSM_REQ0,
- DMACH_SLIMBUS0_RX,
- DMACH_SLIMBUS0_TX,
- DMACH_SLIMBUS0AUX_RX,
- DMACH_SLIMBUS0AUX_TX,
- DMACH_SLIMBUS1_RX,
- DMACH_SLIMBUS1_TX,
- DMACH_SLIMBUS2_RX,
- DMACH_SLIMBUS2_TX,
- DMACH_SLIMBUS3_RX,
- DMACH_SLIMBUS3_TX,
- DMACH_SLIMBUS4_RX,
- DMACH_SLIMBUS4_TX,
- DMACH_SLIMBUS5_RX,
- DMACH_SLIMBUS5_TX,
- DMACH_MIPI_HSI0,
- DMACH_MIPI_HSI1,
- DMACH_MIPI_HSI2,
- DMACH_MIPI_HSI3,
- DMACH_MIPI_HSI4,
- DMACH_MIPI_HSI5,
- DMACH_MIPI_HSI6,
- DMACH_MIPI_HSI7,
- DMACH_DISP1,
- DMACH_MTOM_0,
- DMACH_MTOM_1,
- DMACH_MTOM_2,
- DMACH_MTOM_3,
- DMACH_MTOM_4,
- DMACH_MTOM_5,
- DMACH_MTOM_6,
- DMACH_MTOM_7,
+ DMACH_UART0_TX = 1,
+ DMACH_UART1_RX = 2,
+ DMACH_UART1_TX = 3,
+ DMACH_UART2_RX = 4,
+ DMACH_UART2_TX = 5,
+ DMACH_UART3_RX = 6,
+ DMACH_UART3_TX = 7,
+ DMACH_UART4_RX = 8,
+ DMACH_UART4_TX = 9,
+ DMACH_UART5_RX = 10,
+ DMACH_UART5_TX = 11,
+ DMACH_USI_RX = 12,
+ DMACH_USI_TX = 13,
+ DMACH_IRDA = 14,
+ DMACH_I2S0_RX = 15,
+ DMACH_I2S0_TX = 16,
+ DMACH_I2S0S_TX = 17,
+ DMACH_I2S1_RX = 18,
+ DMACH_I2S1_TX = 19,
+ DMACH_I2S2_RX = 20,
+ DMACH_I2S2_TX = 21,
+ DMACH_SPI0_RX = 22,
+ DMACH_SPI0_TX = 23,
+ DMACH_SPI1_RX = 24,
+ DMACH_SPI1_TX = 25,
+ DMACH_SPI2_RX = 26,
+ DMACH_SPI2_TX = 27,
+ DMACH_AC97_MICIN = 28,
+ DMACH_AC97_PCMIN = 29,
+ DMACH_AC97_PCMOUT = 30,
+ DMACH_EXTERNAL = 31,
+ DMACH_PWM = 32,
+ DMACH_SPDIF = 33,
+ DMACH_HSI_RX = 34,
+ DMACH_HSI_TX = 35,
+ DMACH_PCM0_TX = 36,
+ DMACH_PCM0_RX = 37,
+ DMACH_PCM1_TX = 38,
+ DMACH_PCM1_RX = 39,
+ DMACH_PCM2_TX = 40,
+ DMACH_PCM2_RX = 41,
+ DMACH_MSM_REQ3 = 42,
+ DMACH_MSM_REQ2 = 43,
+ DMACH_MSM_REQ1 = 44,
+ DMACH_MSM_REQ0 = 45,
+ DMACH_SLIMBUS0_RX = 46,
+ DMACH_SLIMBUS0_TX = 47,
+ DMACH_SLIMBUS0AUX_RX = 48,
+ DMACH_SLIMBUS0AUX_TX = 49,
+ DMACH_SLIMBUS1_RX = 50,
+ DMACH_SLIMBUS1_TX = 51,
+ DMACH_SLIMBUS2_RX = 52,
+ DMACH_SLIMBUS2_TX = 53,
+ DMACH_SLIMBUS3_RX = 54,
+ DMACH_SLIMBUS3_TX = 55,
+ DMACH_SLIMBUS4_RX = 56,
+ DMACH_SLIMBUS4_TX = 57,
+ DMACH_SLIMBUS5_RX = 58,
+ DMACH_SLIMBUS5_TX = 59,
+ DMACH_MIPI_HSI0 = 60,
+ DMACH_MIPI_HSI1 = 61,
+ DMACH_MIPI_HSI2 = 62,
+ DMACH_MIPI_HSI3 = 63,
+ DMACH_MIPI_HSI4 = 64,
+ DMACH_MIPI_HSI5 = 65,
+ DMACH_MIPI_HSI6 = 66,
+ DMACH_MIPI_HSI7 = 67,
+ DMACH_DISP1 = 68,
+ DMACH_MTOM_0 = 69,
+ DMACH_MTOM_1 = 70,
+ DMACH_MTOM_2 = 71,
+ DMACH_MTOM_3 = 72,
+ DMACH_MTOM_4 = 73,
+ DMACH_MTOM_5 = 74,
+ DMACH_MTOM_6 = 75,
+ DMACH_MTOM_7 = 76,
/* END Marker, also used to denote a reserved channel */
DMACH_MAX,
};
Index: b/drivers/dma/pl330.c
===================================================================
--- a/drivers/dma/pl330.c 2012-11-28 17:41:37.009626033 +0100
+++ b/drivers/dma/pl330.c 2012-11-28 17:50:27.301635989 +0100
@@ -306,6 +306,8 @@ struct pl330_config {
struct pl330_info {
/* Owning device */
struct device *dev;
+ /* Array of valid peripherals */
+ u32 *peri_id;
/* Size of MicroCode buffers for each channel. */
unsigned mcbufsz;
/* ioremap'ed address of PL330 registers. */
@@ -2368,8 +2370,9 @@ bool pl330_filter(struct dma_chan *chan,
prop_value = ((struct property *)param)->value;
phandle = be32_to_cpup(prop_value++);
node = of_find_node_by_phandle(phandle);
- return ((chan->private == node) &&
- (chan->chan_id == be32_to_cpup(prop_value)));
+ peri_id = chan->private;
+ return chan->device->dev->of_node == node &&
+ *peri_id == be32_to_cpup(prop_value);
}
#endif
@@ -2911,8 +2914,28 @@ pl330_probe(struct amba_device *adev, co
/* Initialize channel parameters */
if (pdat)
num_chan = max_t(int, pdat->nr_valid_peri, pi->pcfg.num_chan);
- else
- num_chan = max_t(int, pi->pcfg.num_peri, pi->pcfg.num_chan);
+ else {
+ struct device_node *np = pi->dev->of_node;
+ int nr_valid_peri = 0;
+
+ of_find_property(np, "peri-map", &nr_valid_peri);
+ if (nr_valid_peri) {
+ nr_valid_peri /= 4;
+
+ pi->peri_id = kzalloc(nr_valid_peri * 4, GFP_KERNEL);
+ if (!pi->peri_id) {
+ ret = -ENOMEM;
+ dev_err(&adev->dev,
+ "unable to allocate pi->peri_id\n");
+ goto probe_err4;
+ }
+ of_property_read_u32_array(np, "peri-map", pi->peri_id,
+ nr_valid_peri);
+ } else
+ nr_valid_peri = pi->pcfg.num_peri;
+
+ num_chan = max_t(int, nr_valid_peri, pi->pcfg.num_chan);
+ }
pdmac->peripherals = kzalloc(num_chan * sizeof(*pch), GFP_KERNEL);
if (!pdmac->peripherals) {
@@ -2923,10 +2946,11 @@ pl330_probe(struct amba_device *adev, co
for (i = 0; i < num_chan; i++) {
pch = &pdmac->peripherals[i];
- if (!adev->dev.of_node)
- pch->chan.private = pdat ? &pdat->peri_id[i] : NULL;
- else
- pch->chan.private = adev->dev.of_node;
+
+ if (pdat)
+ pch->chan.private = &pdat->peri_id[i];
+ else if (pi->peri_id)
+ pch->chan.private = &pi->peri_id[i];
INIT_LIST_HEAD(&pch->work_list);
spin_lock_init(&pch->lock);
@@ -2942,12 +2966,12 @@ pl330_probe(struct amba_device *adev, co
if (pdat) {
pd->cap_mask = pdat->cap_mask;
} else {
- dma_cap_set(DMA_MEMCPY, pd->cap_mask);
- if (pi->pcfg.num_peri) {
+ if (pi->peri_id) {
dma_cap_set(DMA_SLAVE, pd->cap_mask);
dma_cap_set(DMA_CYCLIC, pd->cap_mask);
dma_cap_set(DMA_PRIVATE, pd->cap_mask);
- }
+ } else
+ dma_cap_set(DMA_MEMCPY, pd->cap_mask);
}
pd->device_alloc_chan_resources = pl330_alloc_chan_resources;
@@ -2962,7 +2986,7 @@ pl330_probe(struct amba_device *adev, co
ret = dma_async_device_register(pd);
if (ret) {
dev_err(&adev->dev, "unable to register DMAC\n");
- goto probe_err4;
+ goto probe_err5;
}
dev_info(&adev->dev,
@@ -2975,7 +2999,10 @@ pl330_probe(struct amba_device *adev, co
return 0;
+probe_err5:
+ kfree(pdmac->peripherals);
probe_err4:
+ kfree(pi->peri_id);
pl330_del(pi);
probe_err3:
free_irq(irq, pi);
@@ -3013,8 +3040,11 @@ static int __devexit pl330_remove(struct
pl330_free_chan_resources(&pch->chan);
}
+ kfree(pdmac->peripherals);
+
pi = &pdmac->pif;
+ kfree(pi->peri_id);
pl330_del(pi);
irq = adev->irq[0];
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/