Re: [PATCH v3 4/6] DMA: PL330: Add device tree support

2011-09-14 Thread Thomas Abraham
Hi Grant,

On 14 September 2011 21:54, Grant Likely  wrote:
> On Mon, Sep 12, 2011 at 11:59:23PM +0530, Thomas Abraham wrote:
>> For PL330 dma controllers instantiated from device tree, the channel
>> lookup is based on phandle of the dma controller and dma request id
>> specified by the client node. During probe, the private data of each
>> channel of the controller is set to point to the device node of the
>> dma controller. The 'chan_id' of the each channel is used as the
>> dma request id.
>>
>> Client driver requesting dma channels specify the phandle of the
>> dma controller and the request id. The pl330 filter function
>> converts the phandle to the device node pointer and matches that
>> with channel's private data. If a match is found, the request id
>> from the client node and the 'chan_id' of the channel is matched.
>> A channel is found if both the values match.
>>
>> Cc: Jassi Brar 
>> Cc: Boojin Kim 
>> Signed-off-by: Thomas Abraham 
>> Reviewed-by: Rob Herring 
>> ---
>>  .../devicetree/bindings/dma/arm-pl330.txt          |   29 +
>>  drivers/dma/pl330.c                                |   33 
>> +--
>>  2 files changed, 58 insertions(+), 4 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/dma/arm-pl330.txt
>>
>> diff --git a/Documentation/devicetree/bindings/dma/arm-pl330.txt 
>> b/Documentation/devicetree/bindings/dma/arm-pl330.txt
>> new file mode 100644
>> index 000..05b007d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt
>> @@ -0,0 +1,29 @@
>> +* ARM PrimeCell PL330 DMA Controller
>> +
>> +The ARM PrimeCell PL330 DMA controller can move blocks of memory contents
>> +between memory and peripherals or memory to memory.
>> +
>> +Required properties:
>> +  - compatible: should include both "arm,pl330" and "arm,primecell".
>> +  - reg: physical base address of the controller and length of memory mapped
>> +    region.
>> +  - interrupts: interrupt number to the cpu.
>> +
>> +Example: (from Samsung's Exynos4 processor dtsi file)
>> +
>> +     pdma0: pdma@1268 {
>> +             compatible = "arm,pl330", "arm,primecell";
>> +             reg = <0x1268 0x1000>;
>> +             interrupts = <99>;
>> +     };
>> +
>> +Client drivers (device nodes requiring dma transfers from dev-to-mem or
>> +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]>;
>> +
>> +      where 'dma request id' is the dma request number which is connected
>> +      to the client controller.
>> +
>> +  Example:  tx-dma-channel = <&pdma0 12>;
>
> Looks good to me.  You may want to specify that the property name
> should be in the form: -dma-channel just to enforce a bit of
> convention on the users.

Ok. This will be added as a suggestion for property name specifying
the dma channel.

>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 992bf82..7a4ebf1 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -19,6 +19,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #define NR_DEFAULT_DESC      16
>>
>> @@ -277,6 +278,20 @@ bool pl330_filter(struct dma_chan *chan, void *param)
>>       if (chan->device->dev->driver != &pl330_driver.drv)
>>               return false;
>>
>> +#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)));
>
> Don't open code this.  There is already a function to decode a phandle
> property.  of_parse_phandle() should do the trick.

The parameter to this function 'void *param' is already pointing to a
'struct property'. That 'struct property' has a value of type
<[phandle of dma controller] [channel id]>.

Since the property is already known, the of_get_property() call within
the of_parse_phandle() would complicate the above code. And,
of_parse_phandle requires a 'property name' inside the dma client
device node, which the above code fragment does not know about
(property names are defined by client drivers).

So, I would prefer to continue with the above implementation.

>
> Otherwise looks good to me.
>
> g.


Thanks for your review.

Regards,
Thomas.

[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 4/6] DMA: PL330: Add device tree support

2011-09-14 Thread Grant Likely
On Mon, Sep 12, 2011 at 11:59:23PM +0530, Thomas Abraham wrote:
> For PL330 dma controllers instantiated from device tree, the channel
> lookup is based on phandle of the dma controller and dma request id
> specified by the client node. During probe, the private data of each
> channel of the controller is set to point to the device node of the
> dma controller. The 'chan_id' of the each channel is used as the
> dma request id.
> 
> Client driver requesting dma channels specify the phandle of the
> dma controller and the request id. The pl330 filter function
> converts the phandle to the device node pointer and matches that
> with channel's private data. If a match is found, the request id
> from the client node and the 'chan_id' of the channel is matched.
> A channel is found if both the values match.
> 
> Cc: Jassi Brar 
> Cc: Boojin Kim 
> Signed-off-by: Thomas Abraham 
> Reviewed-by: Rob Herring 
> ---
>  .../devicetree/bindings/dma/arm-pl330.txt  |   29 +
>  drivers/dma/pl330.c|   33 +--
>  2 files changed, 58 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/dma/arm-pl330.txt
> 
> diff --git a/Documentation/devicetree/bindings/dma/arm-pl330.txt 
> b/Documentation/devicetree/bindings/dma/arm-pl330.txt
> new file mode 100644
> index 000..05b007d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt
> @@ -0,0 +1,29 @@
> +* ARM PrimeCell PL330 DMA Controller
> +
> +The ARM PrimeCell PL330 DMA controller can move blocks of memory contents
> +between memory and peripherals or memory to memory.
> +
> +Required properties:
> +  - compatible: should include both "arm,pl330" and "arm,primecell".
> +  - reg: physical base address of the controller and length of memory mapped
> +region.
> +  - interrupts: interrupt number to the cpu.
> +
> +Example: (from Samsung's Exynos4 processor dtsi file)
> +
> + pdma0: pdma@1268 {
> + compatible = "arm,pl330", "arm,primecell";
> + reg = <0x1268 0x1000>;
> + interrupts = <99>;
> + };
> +
> +Client drivers (device nodes requiring dma transfers from dev-to-mem or
> +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]>;
> +
> +  where 'dma request id' is the dma request number which is connected
> +  to the client controller.
> +
> +  Example:  tx-dma-channel = <&pdma0 12>;

Looks good to me.  You may want to specify that the property name
should be in the form: -dma-channel just to enforce a bit of
convention on the users.

> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 992bf82..7a4ebf1 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -19,6 +19,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define NR_DEFAULT_DESC  16
>  
> @@ -277,6 +278,20 @@ bool pl330_filter(struct dma_chan *chan, void *param)
>   if (chan->device->dev->driver != &pl330_driver.drv)
>   return false;
>  
> +#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)));

Don't open code this.  There is already a function to decode a phandle
property.  of_parse_phandle() should do the trick.

Otherwise looks good to me.

g.

> + }
> +#endif
> +
>   peri_id = chan->private;
>   return *peri_id == (unsigned)param;
>  }
> @@ -855,12 +870,17 @@ pl330_probe(struct amba_device *adev, const struct 
> amba_id *id)
>   INIT_LIST_HEAD(&pd->channels);
>  
>   /* Initialize channel parameters */
> - num_chan = max(pdat ? pdat->nr_valid_peri : 0, (u8)pi->pcfg.num_chan);
> + num_chan = max(pdat ? pdat->nr_valid_peri : (u8)pi->pcfg.num_peri,
> + (u8)pi->pcfg.num_chan);
>   pdmac->peripherals = kzalloc(num_chan * sizeof(*pch), GFP_KERNEL);
>  
>   for (i = 0; i < num_chan; i++) {
>   pch = &pdmac->peripherals[i];
> - pch->chan.private = pdat ? &pdat->peri_id[i] : NULL;
> + if (!adev->dev.of_node)
> + pch->chan.private = pdat ? &pdat->peri_id[i] : NULL;
> + else
> + pch->chan.private = adev->dev.of_node;
> +
>   INIT_LIST_HEAD(&pch->work_list);
>   spin_lock_init(&pch->lock);
>   pch->pl330_chid = NULL;
> @@ -874,10 +894,15 @@ pl330_probe(struct amba_device *adev, const struct 
> amba_id *id)
>   }
>  
>   pd->dev = &adev->dev;
> - if (pdat)

Re: [PATCH v3 4/6] DMA: PL330: Add device tree support

2011-09-12 Thread Jassi Brar
On Mon, Sep 12, 2011 at 11:59 PM, Thomas Abraham
 wrote:
> For PL330 dma controllers instantiated from device tree, the channel
> lookup is based on phandle of the dma controller and dma request id
> specified by the client node. During probe, the private data of each
> channel of the controller is set to point to the device node of the
> dma controller. The 'chan_id' of the each channel is used as the
> dma request id.
>
> Client driver requesting dma channels specify the phandle of the
> dma controller and the request id. The pl330 filter function
> converts the phandle to the device node pointer and matches that
> with channel's private data. If a match is found, the request id
> from the client node and the 'chan_id' of the channel is matched.
> A channel is found if both the values match.
>
> Cc: Jassi Brar 
> Cc: Boojin Kim 
> Signed-off-by: Thomas Abraham 
> Reviewed-by: Rob Herring 
> ---

Acked-by: Jassi Brar 

thanks
-jassi
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v3 4/6] DMA: PL330: Add device tree support

2011-09-12 Thread Thomas Abraham
For PL330 dma controllers instantiated from device tree, the channel
lookup is based on phandle of the dma controller and dma request id
specified by the client node. During probe, the private data of each
channel of the controller is set to point to the device node of the
dma controller. The 'chan_id' of the each channel is used as the
dma request id.

Client driver requesting dma channels specify the phandle of the
dma controller and the request id. The pl330 filter function
converts the phandle to the device node pointer and matches that
with channel's private data. If a match is found, the request id
from the client node and the 'chan_id' of the channel is matched.
A channel is found if both the values match.

Cc: Jassi Brar 
Cc: Boojin Kim 
Signed-off-by: Thomas Abraham 
Reviewed-by: Rob Herring 
---
 .../devicetree/bindings/dma/arm-pl330.txt  |   29 +
 drivers/dma/pl330.c|   33 +--
 2 files changed, 58 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/arm-pl330.txt

diff --git a/Documentation/devicetree/bindings/dma/arm-pl330.txt 
b/Documentation/devicetree/bindings/dma/arm-pl330.txt
new file mode 100644
index 000..05b007d
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt
@@ -0,0 +1,29 @@
+* ARM PrimeCell PL330 DMA Controller
+
+The ARM PrimeCell PL330 DMA controller can move blocks of memory contents
+between memory and peripherals or memory to memory.
+
+Required properties:
+  - compatible: should include both "arm,pl330" and "arm,primecell".
+  - reg: physical base address of the controller and length of memory mapped
+region.
+  - interrupts: interrupt number to the cpu.
+
+Example: (from Samsung's Exynos4 processor dtsi file)
+
+   pdma0: pdma@1268 {
+   compatible = "arm,pl330", "arm,primecell";
+   reg = <0x1268 0x1000>;
+   interrupts = <99>;
+   };
+
+Client drivers (device nodes requiring dma transfers from dev-to-mem or
+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]>;
+
+  where 'dma request id' is the dma request number which is connected
+  to the client controller.
+
+  Example:  tx-dma-channel = <&pdma0 12>;
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 992bf82..7a4ebf1 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define NR_DEFAULT_DESC16
 
@@ -277,6 +278,20 @@ bool pl330_filter(struct dma_chan *chan, void *param)
if (chan->device->dev->driver != &pl330_driver.drv)
return false;
 
+#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)));
+   }
+#endif
+
peri_id = chan->private;
return *peri_id == (unsigned)param;
 }
@@ -855,12 +870,17 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
INIT_LIST_HEAD(&pd->channels);
 
/* Initialize channel parameters */
-   num_chan = max(pdat ? pdat->nr_valid_peri : 0, (u8)pi->pcfg.num_chan);
+   num_chan = max(pdat ? pdat->nr_valid_peri : (u8)pi->pcfg.num_peri,
+   (u8)pi->pcfg.num_chan);
pdmac->peripherals = kzalloc(num_chan * sizeof(*pch), GFP_KERNEL);
 
for (i = 0; i < num_chan; i++) {
pch = &pdmac->peripherals[i];
-   pch->chan.private = pdat ? &pdat->peri_id[i] : NULL;
+   if (!adev->dev.of_node)
+   pch->chan.private = pdat ? &pdat->peri_id[i] : NULL;
+   else
+   pch->chan.private = adev->dev.of_node;
+
INIT_LIST_HEAD(&pch->work_list);
spin_lock_init(&pch->lock);
pch->pl330_chid = NULL;
@@ -874,10 +894,15 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
}
 
pd->dev = &adev->dev;
-   if (pdat)
+   if (pdat) {
pd->cap_mask = pdat->cap_mask;
-   else
+   } else {
dma_cap_set(DMA_MEMCPY, pd->cap_mask);
+   if (pi->pcfg.num_peri) {
+   dma_cap_set(DMA_SLAVE, pd->cap_mask);
+   dma_cap_set(DMA_CYCLIC, pd->cap_mask);
+   }
+   }
 
pd->device_alloc_chan_resources = pl330_alloc_chan_resources;
pd->device_free_chan_resources = pl330_free_chan_resources;
-- 
1.6.6.rc2

--
To unsubscribe from this list: s