[PATCH 0/4] Add generic DMA DT binding support

[PATCH 0/4] Add generic DMA DT binding support

[Padmavathi Venna]

This patch set adds support for generic dma device tree bindings for
Samsung platforms and is dependent on the following patches from
Vinod Koul next branch
1)of: Add generic device tree DMA helpers
2)dmaengine: add helper function to request a slave DMA channel

This patch set is made based Kukjin Kim for-next branch

Padmavathi Venna (4):
  DMA: PL330: Add xlate function
  DMA: PL330: Register the DMA controller with the generic DMA helpers
  ARM: dts: Add #dma-cells for generic dma binding support
  DMA: PL330: Modify pl330 filter based on new generic dma dt bindings.

 .../devicetree/bindings/dma/arm-pl330.txt          |    9 +++-
 arch/arm/boot/dts/exynos5250.dtsi                  |    4 ++
 drivers/dma/pl330.c                                |   48 ++++++++++++++++----
 3 files changed, 49 insertions(+), 12 deletions(-)

-- 
1.7.4.4

[PATCH 1/4] DMA: PL330: Add xlate function

[Padmavathi Venna]

Add xlate to translate the device-tree binding information into
the appropriate format. The filter function requires the dma
controller device node and dma channel number as filter_params.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
---
 drivers/dma/pl330.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 80680ee..b0a9080 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -25,6 +25,7 @@
 #include <linux/amba/pl330.h>
 #include <linux/scatterlist.h>
 #include <linux/of.h>
+#include <linux/of_dma.h>
 
 #include "dmaengine.h"
 #define PL330_MAX_CHAN		8
@@ -2352,6 +2353,22 @@ static void dma_pl330_rqcb(void *token, enum pl330_op_err err)
 	tasklet_schedule(&pch->task);
 }
 
+struct dma_chan *of_dma_pl330_xlate(struct of_phandle_args *dma_spec,
+						struct of_dma *ofdma)
+{
+	int count = dma_spec->args_count;
+	struct of_dma_filter_info *info = ofdma->of_dma_data;
+
+	if (!info || !info->filter_fn)
+		return NULL;
+
+	if (count != 1)
+		return NULL;
+
+	return dma_request_channel(info->dma_cap, info->filter_fn, dma_spec);
+}
+EXPORT_SYMBOL_GPL(of_dma_pl330_xlate);
+
 bool pl330_filter(struct dma_chan *chan, void *param)
 {
 	u8 *peri_id;
-- 
1.7.4.4

Re: [PATCH 1/4] DMA: PL330: Add xlate function

[Arnd Bergmann]

On Friday 18 January 2013, Padmavathi Venna wrote:
> +struct dma_chan *of_dma_pl330_xlate(struct of_phandle_args *dma_spec,
> +                                               struct of_dma *ofdma)
> +{
> +       int count = dma_spec->args_count;
> +       struct of_dma_filter_info *info = ofdma->of_dma_data;
> +
> +       if (!info || !info->filter_fn)
> +               return NULL;
> +
> +       if (count != 1)
> +               return NULL;
> +
> +       return dma_request_channel(info->dma_cap, info->filter_fn, dma_spec);
> +}
> +EXPORT_SYMBOL_GPL(of_dma_pl330_xlate);

It seems a little sad that we still have to use dma_request_channel()
to implement this, when that function will go off searching all channels
and pass them tino the filter, which then has to look for the device node
and match it with each channel. We already know the controller and should
just be able to get a channel for it, although I don't exactly know how
that is done.

Further, your function is almost identical to the of_dma_simple_xlate
function. Can't you use that one instead?

	Arnd

Re: [PATCH 1/4] DMA: PL330: Add xlate function

[Padma Venkat]

Hi,

On Mon, Jan 28, 2013 at 8:28 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 18 January 2013, Padmavathi Venna wrote:
>> +struct dma_chan *of_dma_pl330_xlate(struct of_phandle_args *dma_spec,
>> +                                               struct of_dma *ofdma)
>> +{
>> +       int count = dma_spec->args_count;
>> +       struct of_dma_filter_info *info = ofdma->of_dma_data;
>> +
>> +       if (!info || !info->filter_fn)
>> +               return NULL;
>> +
>> +       if (count != 1)
>> +               return NULL;
>> +
>> +       return dma_request_channel(info->dma_cap, info->filter_fn, dma_spec);
>> +}
>> +EXPORT_SYMBOL_GPL(of_dma_pl330_xlate);
>
> It seems a little sad that we still have to use dma_request_channel()
> to implement this, when that function will go off searching all channels
> and pass them tino the filter, which then has to look for the device node
> and match it with each channel. We already know the controller and should
> just be able to get a channel for it, although I don't exactly know how
> that is done.
>
> Further, your function is almost identical to the of_dma_simple_xlate
> function. Can't you use that one instead?

of_dma_simple_xlate is just passing the dma channel number to the
filter function. But I also need
to compare against device node as my requested channel can belong to
any of the available dma controller
on SoC. So I implemented a xlate which passes the whole dma_spec.

Thanks for reviewing the patches.

Regards
Padma

>
>         Arnd

Re: [PATCH 1/4] DMA: PL330: Add xlate function

[Arnd Bergmann]

On Tuesday 29 January 2013, Padma Venkat wrote:
> of_dma_simple_xlate is just passing the dma channel number to the
> filter function. But I also need
> to compare against device node as my requested channel can belong to
> any of the available dma controller
> on SoC. So I implemented a xlate which passes the whole dma_spec.

Hmm, this seems to be a universal requirement for anyone using the
simple xlate function. Jon, how did you expect drivers to handle
this? Assuming that there is only one DMA controller in the system
does not sound like a good idea.

Padma, please have a look at how I did this in the dw_dma patch
I posted yesterday.

	Arnd

[PATCH 2/4] DMA: PL330: Register the DMA controller with the generic DMA helpers

[Padmavathi Venna]

This patch registers the pl330 dma controller driver with the generic
device tree dma helper functions.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
---
 drivers/dma/pl330.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index b0a9080..b80ef97 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2874,6 +2874,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	struct dma_pl330_platdata *pdat;
 	struct dma_pl330_dmac *pdmac;
 	struct dma_pl330_chan *pch;
+	struct of_dma_filter_info *pfi;
 	struct pl330_info *pi;
 	struct dma_device *pd;
 	struct resource *res;
@@ -2990,6 +2991,23 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 		pi->pcfg.data_bus_width / 8, pi->pcfg.num_chan,
 		pi->pcfg.num_peri, pi->pcfg.num_events);
 
+	pfi = devm_kzalloc(&adev->dev, sizeof(*pfi), GFP_KERNEL);
+	if (!pfi) {
+		dev_err(&adev->dev, "unable to allocate mem\n");
+		return -ENOMEM;
+	}
+
+	pfi->dma_cap = pd->cap_mask;
+	pfi->filter_fn = pl330_filter;
+
+	ret = of_dma_controller_register(adev->dev.of_node,
+					 of_dma_pl330_xlate, pfi);
+	if (ret) {
+		dev_err(&adev->dev, "unable to register DMA to the\
+					generic DT DMA helpers\n");
+		goto probe_err2;
+	}
+
 	return 0;
 
 probe_err4:
@@ -3016,6 +3034,8 @@ static int pl330_remove(struct amba_device *adev)
 	if (!pdmac)
 		return 0;
 
+	of_dma_controller_free(adev->dev.of_node);
+
 	amba_set_drvdata(adev, NULL);
 
 	/* Idle the DMAC */
-- 
1.7.4.4

Re: [PATCH 2/4] DMA: PL330: Register the DMA controller with the generic DMA helpers

[Arnd Bergmann]

On Friday 18 January 2013, Padmavathi Venna wrote:
> +       pfi = devm_kzalloc(&adev->dev, sizeof(*pfi), GFP_KERNEL);
> +       if (!pfi) {
> +               dev_err(&adev->dev, "unable to allocate mem\n");
> +               return -ENOMEM;
> +       }
> +
> +       pfi->dma_cap = pd->cap_mask;
> +       pfi->filter_fn = pl330_filter;
> +
> +       ret = of_dma_controller_register(adev->dev.of_node,
> +                                        of_dma_pl330_xlate, pfi);

Why do you pass a 'struct of_dma_filter_info' here? I would
expect that you pass the pdmac object as the third argument.

	Arnd

Re: [PATCH 2/4] DMA: PL330: Register the DMA controller with the generic DMA helpers

[Jon Hunter]

On 01/28/2013 08:51 AM, Arnd Bergmann wrote:
> On Friday 18 January 2013, Padmavathi Venna wrote:
>> +       pfi = devm_kzalloc(&adev->dev, sizeof(*pfi), GFP_KERNEL);
>> +       if (!pfi) {
>> +               dev_err(&adev->dev, "unable to allocate mem\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       pfi->dma_cap = pd->cap_mask;
>> +       pfi->filter_fn = pl330_filter;
>> +
>> +       ret = of_dma_controller_register(adev->dev.of_node,
>> +                                        of_dma_pl330_xlate, pfi);
> 
> Why do you pass a 'struct of_dma_filter_info' here? I would
> expect that you pass the pdmac object as the third argument.

I believe it is because that is the data that the xlate function is
using. Are you suggesting the data should be stored in the pdmac object
and extracted from there? That could be done too given that this
controller has its own xlate.

Cheers
Jon

Re: [PATCH 2/4] DMA: PL330: Register the DMA controller with the generic DMA helpers

[Arnd Bergmann]

On Monday 28 January 2013, Jon Hunter wrote:
> On 01/28/2013 08:51 AM, Arnd Bergmann wrote:
> > On Friday 18 January 2013, Padmavathi Venna wrote:
> >> +       pfi = devm_kzalloc(&adev->dev, sizeof(*pfi), GFP_KERNEL);
> >> +       if (!pfi) {
> >> +               dev_err(&adev->dev, "unable to allocate mem\n");
> >> +               return -ENOMEM;
> >> +       }
> >> +
> >> +       pfi->dma_cap = pd->cap_mask;
> >> +       pfi->filter_fn = pl330_filter;
> >> +
> >> +       ret = of_dma_controller_register(adev->dev.of_node,
> >> +                                        of_dma_pl330_xlate, pfi);
> > 
> > Why do you pass a 'struct of_dma_filter_info' here? I would
> > expect that you pass the pdmac object as the third argument.
> 
> I believe it is because that is the data that the xlate function is
> using. Are you suggesting the data should be stored in the pdmac object
> and extracted from there? That could be done too given that this
> controller has its own xlate.

It just seems weird that we are passing a constant cap_mask in
a data structure, and that we need our own filter function still,
but don't pass the pointer to the data structure that we actually
need in the filter function (the dma_device *).

	Arnd

Re: [PATCH 2/4] DMA: PL330: Register the DMA controller with the generic DMA helpers

[Jon Hunter]

On 01/28/2013 12:01 PM, Arnd Bergmann wrote:
> On Monday 28 January 2013, Jon Hunter wrote:
>> On 01/28/2013 08:51 AM, Arnd Bergmann wrote:
>>> On Friday 18 January 2013, Padmavathi Venna wrote:
>>>> +       pfi = devm_kzalloc(&adev->dev, sizeof(*pfi), GFP_KERNEL);
>>>> +       if (!pfi) {
>>>> +               dev_err(&adev->dev, "unable to allocate mem\n");
>>>> +               return -ENOMEM;
>>>> +       }
>>>> +
>>>> +       pfi->dma_cap = pd->cap_mask;
>>>> +       pfi->filter_fn = pl330_filter;
>>>> +
>>>> +       ret = of_dma_controller_register(adev->dev.of_node,
>>>> +                                        of_dma_pl330_xlate, pfi);
>>>
>>> Why do you pass a 'struct of_dma_filter_info' here? I would
>>> expect that you pass the pdmac object as the third argument.
>>
>> I believe it is because that is the data that the xlate function is
>> using. Are you suggesting the data should be stored in the pdmac object
>> and extracted from there? That could be done too given that this
>> controller has its own xlate.
> 
> It just seems weird that we are passing a constant cap_mask in
> a data structure, and that we need our own filter function still,
> but don't pass the pointer to the data structure that we actually
> need in the filter function (the dma_device *).

Gotcha. I was still thinking about the generic case, but in this case if
you know the dma controller, it is redundant.

Cheers
Jon

[PATCH 3/4] ARM: dts: Add #dma-cells for generic dma binding support

[Padmavathi Venna]

This patch adds #dma-cells property to PL330 DMA controller
nodes for supporting generic dma dt bindings on samsung
exynos5250 platform.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
---
 .../devicetree/bindings/dma/arm-pl330.txt          |    9 ++++++---
 arch/arm/boot/dts/exynos5250.dtsi                  |    4 ++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/arm-pl330.txt b/Documentation/devicetree/bindings/dma/arm-pl330.txt
index 36e27d5..457a233 100644
--- a/Documentation/devicetree/bindings/dma/arm-pl330.txt
+++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt
@@ -8,6 +8,8 @@ Required properties:
   - reg: physical base address of the controller and length of memory mapped
     region.
   - interrupts: interrupt number to the cpu.
+  - #dma-cells: must be at least 1. used to represent the number of integer
+    cells in the dmas property of client device.
 
 Optional properties:
 - dma-coherent      : Present if dma operations are coherent
@@ -18,6 +20,7 @@ Example:
 		compatible = "arm,pl330", "arm,primecell";
 		reg = <0x12680000 0x1000>;
 		interrupts = <99>;
+		#dma-cells = <1>;
 	};
 
 Client drivers (device nodes requiring dma transfers from dev-to-mem or
@@ -27,7 +30,7 @@ as shown below.
   [property name]  = <[phandle of the dma controller] [dma request id]>;
 
       where 'dma request id' is the dma request number which is connected
-      to the client controller. The 'property name' is recommended to be
-      of the form <name>-dma-channel.
+      to the client controller. The 'property name' is 'dmas' as recommended
+      by the generic dma device tree binding helpers.
 
-  Example:  tx-dma-channel = <&pdma0 12>;
+  Example:  dmas = <&pdma0 12>;
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 30485de..46bae01 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -286,24 +286,28 @@
 			compatible = "arm,pl330", "arm,primecell";
 			reg = <0x121A0000 0x1000>;
 			interrupts = <0 34 0>;
+			#dma-cells = <1>;
 		};
 
 		pdma1: pdma@121B0000 {
 			compatible = "arm,pl330", "arm,primecell";
 			reg = <0x121B0000 0x1000>;
 			interrupts = <0 35 0>;
+			#dma-cells = <1>;
 		};
 
 		mdma0: mdma@10800000 {
 			compatible = "arm,pl330", "arm,primecell";
 			reg = <0x10800000 0x1000>;
 			interrupts = <0 33 0>;
+			#dma-cells = <1>;
 		};
 
 		mdma1: mdma@11C10000 {
 			compatible = "arm,pl330", "arm,primecell";
 			reg = <0x11C10000 0x1000>;
 			interrupts = <0 124 0>;
+			#dma-cells = <1>;
 		};
 	};
 
-- 
1.7.4.4

Re: [PATCH 3/4] ARM: dts: Add #dma-cells for generic dma binding support

[Arnd Bergmann]

On Friday 18 January 2013, Padmavathi Venna wrote:
> diff --git a/Documentation/devicetree/bindings/dma/arm-pl330.txt b/Documentation/devicetree/bindings/dma/arm-pl330.txt
> index 36e27d5..457a233 100644
> --- a/Documentation/devicetree/bindings/dma/arm-pl330.txt
> +++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt
> @@ -8,6 +8,8 @@ Required properties:
>    - reg: physical base address of the controller and length of memory mapped
>      region.
>    - interrupts: interrupt number to the cpu.
> +  - #dma-cells: must be at least 1. used to represent the number of integer
> +    cells in the dmas property of client device.

The wording 'at least' seems wrong here: that is what we use in the
generic DMA binding, but in the part that is specific to one
driver, I would expect to see

	- #dma-cells: must be <1>

Since that is what this particular driver requires.

>  Client drivers (device nodes requiring dma transfers from dev-to-mem or
> @@ -27,7 +30,7 @@ as shown below.
>    [property name]  = <[phandle of the dma controller] [dma request id]>;
>  
>        where 'dma request id' is the dma request number which is connected
> -      to the client controller. The 'property name' is recommended to be
> -      of the form <name>-dma-channel.
> +      to the client controller. The 'property name' is 'dmas' as recommended
> +      by the generic dma device tree binding helpers.

s/recommended/required/

Also, the "dma-names" property is required as well.

	Arnd

Re: [PATCH 3/4] ARM: dts: Add #dma-cells for generic dma binding support

[Padma Venkat]

Hi,

On Mon, Jan 28, 2013 at 8:17 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 18 January 2013, Padmavathi Venna wrote:
>> diff --git a/Documentation/devicetree/bindings/dma/arm-pl330.txt b/Documentation/devicetree/bindings/dma/arm-pl330.txt
>> index 36e27d5..457a233 100644
>> --- a/Documentation/devicetree/bindings/dma/arm-pl330.txt
>> +++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt
>> @@ -8,6 +8,8 @@ Required properties:
>>    - reg: physical base address of the controller and length of memory mapped
>>      region.
>>    - interrupts: interrupt number to the cpu.
>> +  - #dma-cells: must be at least 1. used to represent the number of integer
>> +    cells in the dmas property of client device.
>
> The wording 'at least' seems wrong here: that is what we use in the
> generic DMA binding, but in the part that is specific to one
> driver, I would expect to see
>
>         - #dma-cells: must be <1>
>
> Since that is what this particular driver requires.

Okey. I will change this.

>
>>  Client drivers (device nodes requiring dma transfers from dev-to-mem or
>> @@ -27,7 +30,7 @@ as shown below.
>>    [property name]  = <[phandle of the dma controller] [dma request id]>;
>>
>>        where 'dma request id' is the dma request number which is connected
>> -      to the client controller. The 'property name' is recommended to be
>> -      of the form <name>-dma-channel.
>> +      to the client controller. The 'property name' is 'dmas' as recommended
>> +      by the generic dma device tree binding helpers.
>
> s/recommended/required/
>
> Also, the "dma-names" property is required as well.

OK. I will add.

Thanks
Padma

>
>         Arnd

[PATCH 4/4] DMA: PL330: Modify pl330 filter based on new generic dma dt bindings.

[Padmavathi Venna]

This patch modify the filter function to filter the required channel
based on new filter params.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
---
 drivers/dma/pl330.c |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index b80ef97..a773ea7 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2378,15 +2378,8 @@ bool pl330_filter(struct dma_chan *chan, void *param)
 
 #ifdef CONFIG_OF
 	if (chan->device->dev->of_node) {
-		const __be32 *prop_value;
-		phandle phandle;
-		struct device_node *node;
-
-		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)));
+		struct of_phandle_args *dma_spec = param;
+		return ((chan->private == dma_spec->np) && (chan->chan_id == dma_spec->args[0]));
 	}
 #endif
 
-- 
1.7.4.4

Re: [PATCH 0/4] Add generic DMA DT binding support

[Vinod Koul]

On Fri, Jan 18, 2013 at 05:03:40PM +0530, Padmavathi Venna wrote:
> This patch set adds support for generic dma device tree bindings for
> Samsung platforms and is dependent on the following patches from
> Vinod Koul next branch
> 1)of: Add generic device tree DMA helpers
> 2)dmaengine: add helper function to request a slave DMA channel
Changes look fairly decent. I need somone with better knowldge of DT to akc this
before this is applied. Arnd...?
> 
> This patch set is made based Kukjin Kim for-next branch
> 
> Padmavathi Venna (4):
>   DMA: PL330: Add xlate function
>   DMA: PL330: Register the DMA controller with the generic DMA helpers
>   ARM: dts: Add #dma-cells for generic dma binding support
>   DMA: PL330: Modify pl330 filter based on new generic dma dt bindings.
> 
>  .../devicetree/bindings/dma/arm-pl330.txt          |    9 +++-
>  arch/arm/boot/dts/exynos5250.dtsi                  |    4 ++
>  drivers/dma/pl330.c                                |   48 ++++++++++++++++----
>  3 files changed, 49 insertions(+), 12 deletions(-)
> 
> -- 
> 1.7.4.4
> 

Re: [PATCH 0/4] Add generic DMA DT binding support

[Arnd Bergmann]

On Monday 28 January 2013, Vinod Koul wrote:
> On Fri, Jan 18, 2013 at 05:03:40PM +0530, Padmavathi Venna wrote:
> > This patch set adds support for generic dma device tree bindings for
> > Samsung platforms and is dependent on the following patches from
> > Vinod Koul next branch
> > 1)of: Add generic device tree DMA helpers
> > 2)dmaengine: add helper function to request a slave DMA channel
> Changes look fairly decent. I need somone with better knowldge of DT to akc this
> before this is applied. Arnd...?

Thanks for the pointer, it seems everthing is coming together today ;-)

The binding looks good, aside from the wording on some of the properties
in there that could be a little clearer.

The method of calling dma_request_channel() from the xlate() function
is a little bit clumsy IMHO, but it does work and is the easiest way
to retrofit generic DT support to this driver, given that it already
provides a global filter function. It's also apparently what Jon had
in mind, and what Matt is doing on AM33XX.

I'll probably propose something similar on dw_dma, since I was just
(this hour, actually) looking at the same problem there. I believe
you had something smarter in mind when we discussed it in San Diego,
but I don't remember what the idea was.  I would need something
like

	struct dma_chan *dma_channel_get(struct dma_device *);

and I assume that there is a reason for why I can't call this
from the xlate() function but instead have to go through
dma_request_channel(), but I don't completely understand that part
of the puzzle.

	Arnd