[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Joel Fernandes]

HWMOD removal for MMC and Crypto is breaking edma_start as the events are
being manually triggered due to unused channel list not being clear. Atleast
breakage has been seen on these peripherals, but it is expected Audio (McASP)
maybe breaking too.

This patch fixes the issue, by reading the "dmas" property from the DT node if
it exists and clearing the bits in the unused channel list so that these channels
are not manually triggered.

v2 changes:
Reduced indendation by returning from if block.

Reviewed-by: Sekhar Nori <nsekhar@ti.com>
Reported-by: Balaji T K <balajitk@ti.com>
Cc: Pantel Antoniou <panto@antoniou-consulting.com>
Signed-off-by: Joel Fernandes <joelf@ti.com>
---
Note:
Patch should go in for -rc cycle as it fixes existing crypto drivers.

 arch/arm/common/edma.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 39ad030..3867e7e 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -560,14 +560,30 @@ static int reserve_contiguous_slots(int ctlr, unsigned int id,
 static int prepare_unused_channel_list(struct device *dev, void *data)
 {
 	struct platform_device *pdev = to_platform_device(dev);
-	int i, ctlr;
+	int i = 0, ctlr;
+	u32 dma_chan;
+	const __be32 *dma_chan_p;
+	struct property *prop;
+
+	if (dev->of_node) {
+		of_property_for_each_u32(dev->of_node, "dmas", prop,
+					 dma_chan_p, dma_chan) {
+			if (i++ & 1) {
+				ctlr = EDMA_CTLR(dma_chan);
+				clear_bit(EDMA_CHAN_SLOT(dma_chan),
+					  edma_cc[ctlr]->edma_unused);
+			}
+		}
+		return;
+	}
 
-	for (i = 0; i < pdev->num_resources; i++) {
+	/* For non-OF case */
+	for (; i < pdev->num_resources; i++) {
 		if ((pdev->resource[i].flags & IORESOURCE_DMA) &&
 				(int)pdev->resource[i].start >= 0) {
 			ctlr = EDMA_CTLR(pdev->resource[i].start);
 			clear_bit(EDMA_CHAN_SLOT(pdev->resource[i].start),
-					edma_cc[ctlr]->edma_unused);
+				  edma_cc[ctlr]->edma_unused);
 		}
 	}
 
-- 
1.8.1.2

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Sekhar Nori]

On Saturday 24 August 2013 01:23 AM, Joel Fernandes wrote:
> HWMOD removal for MMC and Crypto is breaking edma_start as the events are
> being manually triggered due to unused channel list not being clear. Atleast
> breakage has been seen on these peripherals, but it is expected Audio (McASP)
> maybe breaking too.
> 
> This patch fixes the issue, by reading the "dmas" property from the DT node if
> it exists and clearing the bits in the unused channel list so that these channels
> are not manually triggered.
> 
> v2 changes:
> Reduced indendation by returning from if block.

Is this a v2 or v3 since you already sent a v2 about a month back?

> 
> Reviewed-by: Sekhar Nori <nsekhar@ti.com>
> Reported-by: Balaji T K <balajitk@ti.com>
> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
> Signed-off-by: Joel Fernandes <joelf@ti.com>
> ---
> Note:
> Patch should go in for -rc cycle as it fixes existing crypto drivers.

We agreed the patch is not needed in -rc cycle since there are no
current EDMA users in DT-boot?

> 
>  arch/arm/common/edma.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
> index 39ad030..3867e7e 100644
> --- a/arch/arm/common/edma.c
> +++ b/arch/arm/common/edma.c
> @@ -560,14 +560,30 @@ static int reserve_contiguous_slots(int ctlr, unsigned int id,
>  static int prepare_unused_channel_list(struct device *dev, void *data)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> -	int i, ctlr;
> +	int i = 0, ctlr;
> +	u32 dma_chan;
> +	const __be32 *dma_chan_p;
> +	struct property *prop;
> +
> +	if (dev->of_node) {
> +		of_property_for_each_u32(dev->of_node, "dmas", prop,
> +					 dma_chan_p, dma_chan) {
> +			if (i++ & 1) {
> +				ctlr = EDMA_CTLR(dma_chan);
> +				clear_bit(EDMA_CHAN_SLOT(dma_chan),
> +					  edma_cc[ctlr]->edma_unused);
> +			}

I thought we agreed to do this differently using
of_property_count_strings() and of_parse_phandle_with_args(). I seemed
to have missed any discussion on why this cannot be done (if such a
discussion took place on the list).

Thanks,
Sekhar

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Joel Fernandes]

On 08/26/2013 05:46 AM, Sekhar Nori wrote:
> On Saturday 24 August 2013 01:23 AM, Joel Fernandes wrote:
>> HWMOD removal for MMC and Crypto is breaking edma_start as the events are
>> being manually triggered due to unused channel list not being clear. Atleast
>> breakage has been seen on these peripherals, but it is expected Audio (McASP)
>> maybe breaking too.
>>
>> This patch fixes the issue, by reading the "dmas" property from the DT node if
>> it exists and clearing the bits in the unused channel list so that these channels
>> are not manually triggered.
>>
>> v2 changes:
>> Reduced indendation by returning from if block.
> 
> Is this a v2 or v3 since you already sent a v2 about a month back?

No, there aren't any changes since v2, I just resubmitted the same patch.

>>
>> Reviewed-by: Sekhar Nori <nsekhar@ti.com>
>> Reported-by: Balaji T K <balajitk@ti.com>
>> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>> ---
>> Note:
>> Patch should go in for -rc cycle as it fixes existing crypto drivers.
> 
> We agreed the patch is not needed in -rc cycle since there are no
> current EDMA users in DT-boot?

There is now, crypto and EDMA DT patches are being merged in.

>>
>>  arch/arm/common/edma.c | 22 +++++++++++++++++++---
>>  1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
>> index 39ad030..3867e7e 100644
>> --- a/arch/arm/common/edma.c
>> +++ b/arch/arm/common/edma.c
>> @@ -560,14 +560,30 @@ static int reserve_contiguous_slots(int ctlr, unsigned int id,
>>  static int prepare_unused_channel_list(struct device *dev, void *data)
>>  {
>>  	struct platform_device *pdev = to_platform_device(dev);
>> -	int i, ctlr;
>> +	int i = 0, ctlr;
>> +	u32 dma_chan;
>> +	const __be32 *dma_chan_p;
>> +	struct property *prop;
>> +
>> +	if (dev->of_node) {
>> +		of_property_for_each_u32(dev->of_node, "dmas", prop,
>> +					 dma_chan_p, dma_chan) {
>> +			if (i++ & 1) {
>> +				ctlr = EDMA_CTLR(dma_chan);
>> +				clear_bit(EDMA_CHAN_SLOT(dma_chan),
>> +					  edma_cc[ctlr]->edma_unused);
>> +			}
> 
> I thought we agreed to do this differently using
> of_property_count_strings() and of_parse_phandle_with_args(). I seemed
> to have missed any discussion on why this cannot be done (if such a
> discussion took place on the list).

I kind of missed that particular review comment after reading [1]. Because I
thought only change left was the indentation. Let me work on that comment and
resubmit as v3.

Regards,

-Joel

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Sekhar Nori]

On 8/26/2013 10:22 PM, Joel Fernandes wrote:
> On 08/26/2013 05:46 AM, Sekhar Nori wrote:
>> On Saturday 24 August 2013 01:23 AM, Joel Fernandes wrote:
>>> HWMOD removal for MMC and Crypto is breaking edma_start as the events are
>>> being manually triggered due to unused channel list not being clear. Atleast
>>> breakage has been seen on these peripherals, but it is expected Audio (McASP)
>>> maybe breaking too.
>>>
>>> This patch fixes the issue, by reading the "dmas" property from the DT node if
>>> it exists and clearing the bits in the unused channel list so that these channels
>>> are not manually triggered.
>>>
>>> v2 changes:
>>> Reduced indendation by returning from if block.
>>
>> Is this a v2 or v3 since you already sent a v2 about a month back?
> 
> No, there aren't any changes since v2, I just resubmitted the same patch.

Heh, it does seem to contain some indentation changes per your
documentation :)

> 
>>>
>>> Reviewed-by: Sekhar Nori <nsekhar@ti.com>
>>> Reported-by: Balaji T K <balajitk@ti.com>
>>> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
>>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>>> ---
>>> Note:
>>> Patch should go in for -rc cycle as it fixes existing crypto drivers.
>>
>> We agreed the patch is not needed in -rc cycle since there are no
>> current EDMA users in DT-boot?
> 
> There is now, crypto and EDMA DT patches are being merged in.

They are being merged in v3.12, right? So you meant the -rc cycle of
v3.12? If yes, no need to wait for a broken v3.12-rc1 to come out. It is
possible to queue fixes during merge window too.

Thanks,
Sekhar

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Mark Jackson]

On 23/08/13 20:53, Joel Fernandes wrote:
> HWMOD removal for MMC and Crypto is breaking edma_start as the events are
> being manually triggered due to unused channel list not being clear. Atleast
> breakage has been seen on these peripherals, but it is expected Audio (McASP)
> maybe breaking too.
> 
> This patch fixes the issue, by reading the "dmas" property from the DT node if
> it exists and clearing the bits in the unused channel list so that these channels
> are not manually triggered.
> 
> v2 changes:
> Reduced indendation by returning from if block.
> 
> Reviewed-by: Sekhar Nori <nsekhar@ti.com>
> Reported-by: Balaji T K <balajitk@ti.com>
> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
> Signed-off-by: Joel Fernandes <joelf@ti.com>
> ---
> Note:
> Patch should go in for -rc cycle as it fixes existing crypto drivers.
> 
>  arch/arm/common/edma.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
> index 39ad030..3867e7e 100644
> --- a/arch/arm/common/edma.c
> +++ b/arch/arm/common/edma.c
> @@ -560,14 +560,30 @@ static int reserve_contiguous_slots(int ctlr, unsigned int id,
>  static int prepare_unused_channel_list(struct device *dev, void *data)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> -	int i, ctlr;
> +	int i = 0, ctlr;
> +	u32 dma_chan;
> +	const __be32 *dma_chan_p;
> +	struct property *prop;
> +
> +	if (dev->of_node) {
> +		of_property_for_each_u32(dev->of_node, "dmas", prop,
> +					 dma_chan_p, dma_chan) {
> +			if (i++ & 1) {
> +				ctlr = EDMA_CTLR(dma_chan);
> +				clear_bit(EDMA_CHAN_SLOT(dma_chan),
> +					  edma_cc[ctlr]->edma_unused);
> +			}
> +		}
> +		return;

This should return something here, otherwise we get:-

arch/arm/common/edma.c: In function 'prepare_unused_channel_list':
arch/arm/common/edma.c:577:3: warning: 'return' with no value, in function returning non-void [-Wreturn-type]

Mark J.

> +	}
>  
> -	for (i = 0; i < pdev->num_resources; i++) {
> +	/* For non-OF case */
> +	for (; i < pdev->num_resources; i++) {
>  		if ((pdev->resource[i].flags & IORESOURCE_DMA) &&
>  				(int)pdev->resource[i].start >= 0) {
>  			ctlr = EDMA_CTLR(pdev->resource[i].start);
>  			clear_bit(EDMA_CHAN_SLOT(pdev->resource[i].start),
> -					edma_cc[ctlr]->edma_unused);
> +				  edma_cc[ctlr]->edma_unused);
>  		}
>  	}

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Mark Jackson]

On 06/09/13 20:13, Mark Jackson wrote:
> On 23/08/13 20:53, Joel Fernandes wrote:
>> HWMOD removal for MMC and Crypto is breaking edma_start as the events are
>> being manually triggered due to unused channel list not being clear. Atleast
>> breakage has been seen on these peripherals, but it is expected Audio (McASP)
>> maybe breaking too.
>>
>> This patch fixes the issue, by reading the "dmas" property from the DT node if
>> it exists and clearing the bits in the unused channel list so that these channels
>> are not manually triggered.
>>
>> v2 changes:
>> Reduced indendation by returning from if block.
>>
>> Reviewed-by: Sekhar Nori <nsekhar@ti.com>
>> Reported-by: Balaji T K <balajitk@ti.com>
>> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>> ---
>> Note:
>> Patch should go in for -rc cycle as it fixes existing crypto drivers.
>>
>>  arch/arm/common/edma.c | 22 +++++++++++++++++++---
>>  1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
>> index 39ad030..3867e7e 100644
>> --- a/arch/arm/common/edma.c
>> +++ b/arch/arm/common/edma.c
>> @@ -560,14 +560,30 @@ static int reserve_contiguous_slots(int ctlr, unsigned int id,
>>  static int prepare_unused_channel_list(struct device *dev, void *data)
>>  {
>>  	struct platform_device *pdev = to_platform_device(dev);
>> -	int i, ctlr;
>> +	int i = 0, ctlr;
>> +	u32 dma_chan;
>> +	const __be32 *dma_chan_p;
>> +	struct property *prop;
>> +
>> +	if (dev->of_node) {
>> +		of_property_for_each_u32(dev->of_node, "dmas", prop,
>> +					 dma_chan_p, dma_chan) {
>> +			if (i++ & 1) {
>> +				ctlr = EDMA_CTLR(dma_chan);
>> +				clear_bit(EDMA_CHAN_SLOT(dma_chan),
>> +					  edma_cc[ctlr]->edma_unused);
>> +			}
>> +		}
>> +		return;
> 
> This should return something here, otherwise we get:-
> 
> arch/arm/common/edma.c: In function 'prepare_unused_channel_list':
> arch/arm/common/edma.c:577:3: warning: 'return' with no value, in function returning non-void [-Wreturn-type]

Other than that I can confirm it fixes the issue for me ...

Tested-by: Mark Jackson <mpfj@newflow.co.uk>

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Joel Fernandes]

On 09/06/2013 02:15 PM, Mark Jackson wrote:
> On 06/09/13 20:13, Mark Jackson wrote:
>> On 23/08/13 20:53, Joel Fernandes wrote:
>>> HWMOD removal for MMC and Crypto is breaking edma_start as the events are
>>> being manually triggered due to unused channel list not being clear. Atleast
>>> breakage has been seen on these peripherals, but it is expected Audio (McASP)
>>> maybe breaking too.
>>>
>>> This patch fixes the issue, by reading the "dmas" property from the DT node if
>>> it exists and clearing the bits in the unused channel list so that these channels
>>> are not manually triggered.
>>>
>>> v2 changes:
>>> Reduced indendation by returning from if block.
>>>
>>> Reviewed-by: Sekhar Nori <nsekhar@ti.com>
>>> Reported-by: Balaji T K <balajitk@ti.com>
>>> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
>>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>>> ---
>>> Note:
>>> Patch should go in for -rc cycle as it fixes existing crypto drivers.
>>>
>>>  arch/arm/common/edma.c | 22 +++++++++++++++++++---
>>>  1 file changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
>>> index 39ad030..3867e7e 100644
>>> --- a/arch/arm/common/edma.c
>>> +++ b/arch/arm/common/edma.c
>>> @@ -560,14 +560,30 @@ static int reserve_contiguous_slots(int ctlr, unsigned int id,
>>>  static int prepare_unused_channel_list(struct device *dev, void *data)
>>>  {
>>>  	struct platform_device *pdev = to_platform_device(dev);
>>> -	int i, ctlr;
>>> +	int i = 0, ctlr;
>>> +	u32 dma_chan;
>>> +	const __be32 *dma_chan_p;
>>> +	struct property *prop;
>>> +
>>> +	if (dev->of_node) {
>>> +		of_property_for_each_u32(dev->of_node, "dmas", prop,
>>> +					 dma_chan_p, dma_chan) {
>>> +			if (i++ & 1) {
>>> +				ctlr = EDMA_CTLR(dma_chan);
>>> +				clear_bit(EDMA_CHAN_SLOT(dma_chan),
>>> +					  edma_cc[ctlr]->edma_unused);
>>> +			}
>>> +		}
>>> +		return;
>>
>> This should return something here, otherwise we get:-
>>
>> arch/arm/common/edma.c: In function 'prepare_unused_channel_list':
>> arch/arm/common/edma.c:577:3: warning: 'return' with no value, in function returning non-void [-Wreturn-type]
> 
> Other than that I can confirm it fixes the issue for me ...

Thanks for pointing that out. Will fix it in the next revision.


Regards,

-Joel

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Joel Fernandes]

HWMOD removal for MMC is breaking edma_start as the events are being manually
triggered due to unused channel list not being clear, Thanks to Balaji TK for
finding this issue.

This patch fixes the issue, by reading the "dmas" property from the DT node if
it exists and clearing the bits in the unused channel list.

Cc: Balaji T K <balajitk@ti.com>
Cc: Pantel Antoniou <panto@antoniou-consulting.com>
Signed-off-by: Joel Fernandes <joelf@ti.com>
---
v2 changes: Fixed compiler warning

 arch/arm/common/edma.c |   31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index a432e6c..765d578 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -561,14 +561,29 @@ static int reserve_contiguous_slots(int ctlr, unsigned int id,
 static int prepare_unused_channel_list(struct device *dev, void *data)
 {
 	struct platform_device *pdev = to_platform_device(dev);
-	int i, ctlr;
-
-	for (i = 0; i < pdev->num_resources; i++) {
-		if ((pdev->resource[i].flags & IORESOURCE_DMA) &&
-				(int)pdev->resource[i].start >= 0) {
-			ctlr = EDMA_CTLR(pdev->resource[i].start);
-			clear_bit(EDMA_CHAN_SLOT(pdev->resource[i].start),
-					edma_cc[ctlr]->edma_unused);
+	int i = 0, ctlr;
+	u32 dma_chan;
+	const __be32 *dma_chan_p;
+	struct property *prop;
+
+	if (dev->of_node) {
+		of_property_for_each_u32(dev->of_node, "dmas", prop, \
+					 dma_chan_p, dma_chan) {
+			if (i++ & 1) {
+				ctlr = EDMA_CTLR(dma_chan);
+				clear_bit(EDMA_CHAN_SLOT(dma_chan),
+						edma_cc[ctlr]->edma_unused);
+			}
+		}
+	} else {
+		for (; i < pdev->num_resources; i++) {
+			if ((pdev->resource[i].flags & IORESOURCE_DMA) &&
+			    (int)pdev->resource[i].start >= 0) {
+				ctlr = EDMA_CTLR(pdev->resource[i].start);
+				clear_bit(EDMA_CHAN_SLOT(
+					  pdev->resource[i].start),
+					  edma_cc[ctlr]->edma_unused);
+			}
 		}
 	}
 
-- 
1.7.9.5

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Joel Fernandes]

Hi Tony or Sekhar,

If this patch looks ok, could you pick it up for -rc cycle?

It fixes DMA breakages after the merge window for devices for which DMA
resources are being populated in device tree instead pdev.

Thanks,

-Joel


On 07/22/2013 12:59 PM, Joel Fernandes wrote:
> HWMOD removal for MMC is breaking edma_start as the events are being manually
> triggered due to unused channel list not being clear, Thanks to Balaji TK for
> finding this issue.
> 
> This patch fixes the issue, by reading the "dmas" property from the DT node if
> it exists and clearing the bits in the unused channel list.
> 
> Cc: Balaji T K <balajitk@ti.com>
> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
> Signed-off-by: Joel Fernandes <joelf@ti.com>
> ---
> v2 changes: Fixed compiler warning
> 
>  arch/arm/common/edma.c |   31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
> index a432e6c..765d578 100644
> --- a/arch/arm/common/edma.c
> +++ b/arch/arm/common/edma.c
> @@ -561,14 +561,29 @@ static int reserve_contiguous_slots(int ctlr, unsigned int id,
>  static int prepare_unused_channel_list(struct device *dev, void *data)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> -	int i, ctlr;
> -
> -	for (i = 0; i < pdev->num_resources; i++) {
> -		if ((pdev->resource[i].flags & IORESOURCE_DMA) &&
> -				(int)pdev->resource[i].start >= 0) {
> -			ctlr = EDMA_CTLR(pdev->resource[i].start);
> -			clear_bit(EDMA_CHAN_SLOT(pdev->resource[i].start),
> -					edma_cc[ctlr]->edma_unused);
> +	int i = 0, ctlr;
> +	u32 dma_chan;
> +	const __be32 *dma_chan_p;
> +	struct property *prop;
> +
> +	if (dev->of_node) {
> +		of_property_for_each_u32(dev->of_node, "dmas", prop, \
> +					 dma_chan_p, dma_chan) {
> +			if (i++ & 1) {
> +				ctlr = EDMA_CTLR(dma_chan);
> +				clear_bit(EDMA_CHAN_SLOT(dma_chan),
> +						edma_cc[ctlr]->edma_unused);
> +			}
> +		}
> +	} else {
> +		for (; i < pdev->num_resources; i++) {
> +			if ((pdev->resource[i].flags & IORESOURCE_DMA) &&
> +			    (int)pdev->resource[i].start >= 0) {
> +				ctlr = EDMA_CTLR(pdev->resource[i].start);
> +				clear_bit(EDMA_CHAN_SLOT(
> +					  pdev->resource[i].start),
> +					  edma_cc[ctlr]->edma_unused);
> +			}
>  		}
>  	}
>  
> 

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Sekhar Nori]

On Sunday 28 July 2013 05:02 AM, Joel Fernandes wrote:
> Hi Tony or Sekhar,
> 
> If this patch looks ok, could you pick it up for -rc cycle?
> 
> It fixes DMA breakages after the merge window for devices for which DMA
> resources are being populated in device tree instead pdev.

But which DT-enabled platform in kernel is using EDMA? A grep for edma
over arch/arm/boot/dts/* brings up nothing.

Does this really have to go into the -rc cycle or can it wait till v3.12?

Thanks,
Sekhar

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Joel Fernandes]

On 07/29/2013 02:04 AM, Sekhar Nori wrote:
> On Sunday 28 July 2013 05:02 AM, Joel Fernandes wrote:
>> Hi Tony or Sekhar,
>>
>> If this patch looks ok, could you pick it up for -rc cycle?
>>
>> It fixes DMA breakages after the merge window for devices for which DMA
>> resources are being populated in device tree instead pdev.
> 
> But which DT-enabled platform in kernel is using EDMA? A grep for edma
> over arch/arm/boot/dts/* brings up nothing.
> Does this really have to go into the -rc cycle or can it wait till
> v3.12?


But unused channel list is also populated for private EDMA callers like
davinci-pcm no? IIRC, HWMOD data is also removed for McASP so EDMA for
those may break.

Thanks,

-Joel

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Sekhar Nori]

On Tuesday 30 July 2013 09:23 AM, Joel Fernandes wrote:
> On 07/29/2013 02:04 AM, Sekhar Nori wrote:
>> On Sunday 28 July 2013 05:02 AM, Joel Fernandes wrote:
>>> Hi Tony or Sekhar,
>>>
>>> If this patch looks ok, could you pick it up for -rc cycle?
>>>
>>> It fixes DMA breakages after the merge window for devices for which DMA
>>> resources are being populated in device tree instead pdev.
>>
>> But which DT-enabled platform in kernel is using EDMA? A grep for edma
>> over arch/arm/boot/dts/* brings up nothing.
>> Does this really have to go into the -rc cycle or can it wait till
>> v3.12?
> 
> 
> But unused channel list is also populated for private EDMA callers like
> davinci-pcm no? IIRC, HWMOD data is also removed for McASP so EDMA for
> those may break.

Yes, but which DT-enabled platform has davinci-pcm working today?
AFAICS, this bug affects DT-enabled platforms only.

Thanks,
Sekhar

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Joel Fernandes]

On 07/29/2013 11:52 PM, Sekhar Nori wrote:> On Tuesday 30 July 2013
09:23 AM, Joel Fernandes wrote:
>> On 07/29/2013 02:04 AM, Sekhar Nori wrote:
>>> On Sunday 28 July 2013 05:02 AM, Joel Fernandes wrote:
>>>> Hi Tony or Sekhar,
>>>>
>>>> If this patch looks ok, could you pick it up for -rc cycle?
>>>>
>>>> It fixes DMA breakages after the merge window for devices for which DMA
>>>> resources are being populated in device tree instead pdev.
>>>
>>> But which DT-enabled platform in kernel is using EDMA? A grep for edma
>>> over arch/arm/boot/dts/* brings up nothing.
>>> Does this really have to go into the -rc cycle or can it wait till
>>> v3.12?
>>
>>
>> But unused channel list is also populated for private EDMA callers like
>> davinci-pcm no? IIRC, HWMOD data is also removed for McASP so EDMA for
>> those may break.
>
> Yes, but which DT-enabled platform has davinci-pcm working today?
> AFAICS, this bug affects DT-enabled platforms only.

Yes, you're right. I didn't realize nothing else is working yet. I guess
its fine then for next kernel release.

Thanks,

-Joel

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Sekhar Nori]

On Monday 22 July 2013 11:29 PM, Joel Fernandes wrote:
> HWMOD removal for MMC is breaking edma_start as the events are being manually
> triggered due to unused channel list not being clear, Thanks to Balaji TK for
> finding this issue.

So, Reported-by: Balaji T K <balajitk@ti.com>

?

> 
> This patch fixes the issue, by reading the "dmas" property from the DT node if
> it exists and clearing the bits in the unused channel list.
> 
> Cc: Balaji T K <balajitk@ti.com>
> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
> Signed-off-by: Joel Fernandes <joelf@ti.com>
> ---
> v2 changes: Fixed compiler warning
> 
>  arch/arm/common/edma.c |   31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
> index a432e6c..765d578 100644
> --- a/arch/arm/common/edma.c
> +++ b/arch/arm/common/edma.c
> @@ -561,14 +561,29 @@ static int reserve_contiguous_slots(int ctlr, unsigned int id,
>  static int prepare_unused_channel_list(struct device *dev, void *data)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> -	int i, ctlr;
> -
> -	for (i = 0; i < pdev->num_resources; i++) {
> -		if ((pdev->resource[i].flags & IORESOURCE_DMA) &&
> -				(int)pdev->resource[i].start >= 0) {
> -			ctlr = EDMA_CTLR(pdev->resource[i].start);
> -			clear_bit(EDMA_CHAN_SLOT(pdev->resource[i].start),
> -					edma_cc[ctlr]->edma_unused);
> +	int i = 0, ctlr;
> +	u32 dma_chan;
> +	const __be32 *dma_chan_p;
> +	struct property *prop;
> +
> +	if (dev->of_node) {
> +		of_property_for_each_u32(dev->of_node, "dmas", prop, \
> +					 dma_chan_p, dma_chan) {
> +			if (i++ & 1) {
> +				ctlr = EDMA_CTLR(dma_chan);
> +				clear_bit(EDMA_CHAN_SLOT(dma_chan),
> +						edma_cc[ctlr]->edma_unused);
> +			}

I think it will be cleaner if you use of_property_count_strings() on
dma-names to get the count of DMA channels for this device and then use
of_parse_phandle_with_args() to get access to the args. Honestly, I have
not used these APIs myself, so if this doesn?t work the way I think it
should then let me know and I will try to do some experiments myself.

The current way of skipping over even fields really depends on
#dma-cells for EDMA to be 1. That in itself is not such a bad
assumption, but if you have APIs which already take care of this for
you, why not use them?

> +		}
> +	} else {
> +		for (; i < pdev->num_resources; i++) {
> +			if ((pdev->resource[i].flags & IORESOURCE_DMA) &&
> +			    (int)pdev->resource[i].start >= 0) {
> +				ctlr = EDMA_CTLR(pdev->resource[i].start);
> +				clear_bit(EDMA_CHAN_SLOT(
> +					  pdev->resource[i].start),
> +					  edma_cc[ctlr]->edma_unused);
> +			}

So there is very little in common between OF and non-OF versions of this
function. Why not have two different versions of this function for the
two cases? The OF version can reside under the CONFIG_OF conditional
already in use in the file. This will also save you the ugly line breaks
you had to resort to due to too deep indentation.

Thanks,
Sekhar

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Joel Fernandes]

On 07/29/2013 02:01 AM, Sekhar Nori wrote:> On Monday 22 July 2013 11:29
PM, Joel Fernandes wrote:
>> HWMOD removal for MMC is breaking edma_start as the events are being
manually
>> triggered due to unused channel list not being clear, Thanks to
Balaji TK for
>> finding this issue.
>
> So, Reported-by: Balaji T K <balajitk@ti.com>
>
> ?

Sure, I'll add that as well.

>
>>
>> This patch fixes the issue, by reading the "dmas" property from the
DT node if
>> it exists and clearing the bits in the unused channel list.
>>
>> Cc: Balaji T K <balajitk@ti.com>
>> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>> ---
>> v2 changes: Fixed compiler warning
>>
>>  arch/arm/common/edma.c |   31 +++++++++++++++++++++++--------
>>  1 file changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
>> index a432e6c..765d578 100644
>> --- a/arch/arm/common/edma.c
>> +++ b/arch/arm/common/edma.c
>> @@ -561,14 +561,29 @@ static int reserve_contiguous_slots(int ctlr,
unsigned int id,
>>  static int prepare_unused_channel_list(struct device *dev, void *data)
>>  {
>>  	struct platform_device *pdev = to_platform_device(dev);
>> -	int i, ctlr;
>> -
>> -	for (i = 0; i < pdev->num_resources; i++) {
>> -		if ((pdev->resource[i].flags & IORESOURCE_DMA) &&
>> -				(int)pdev->resource[i].start >= 0) {
>> -			ctlr = EDMA_CTLR(pdev->resource[i].start);
>> -			clear_bit(EDMA_CHAN_SLOT(pdev->resource[i].start),
>> -					edma_cc[ctlr]->edma_unused);
>> +	int i = 0, ctlr;
>> +	u32 dma_chan;
>> +	const __be32 *dma_chan_p;
>> +	struct property *prop;
>> +
>> +	if (dev->of_node) {
>> +		of_property_for_each_u32(dev->of_node, "dmas", prop, \
>> +					 dma_chan_p, dma_chan) {
>> +			if (i++ & 1) {
>> +				ctlr = EDMA_CTLR(dma_chan);
>> +				clear_bit(EDMA_CHAN_SLOT(dma_chan),
>> +						edma_cc[ctlr]->edma_unused);
>> +			}
>
> I think it will be cleaner if you use of_property_count_strings() on
> dma-names to get the count of DMA channels for this device and then use
> of_parse_phandle_with_args() to get access to the args. Honestly, I have
> not used these APIs myself, so if this doesn?t work the way I think it
> should then let me know and I will try to do some experiments myself.
>
> The current way of skipping over even fields really depends on
> #dma-cells for EDMA to be 1. That in itself is not such a bad
> assumption, but if you have APIs which already take care of this for
> you, why not use them?

Sure, looks like of_dma_request_slave_channel does same thing in
drive/dma/of-dma.c. Let me follow up with a patch that does it this way.
I agree its cleaner.

>> +		}
>> +	} else {
>> +		for (; i < pdev->num_resources; i++) {
>> +			if ((pdev->resource[i].flags & IORESOURCE_DMA) &&
>> +			    (int)pdev->resource[i].start >= 0) {
>> +				ctlr = EDMA_CTLR(pdev->resource[i].start);
>> +				clear_bit(EDMA_CHAN_SLOT(
>> +					  pdev->resource[i].start),
>> +					  edma_cc[ctlr]->edma_unused);
>> +			}
>
> So there is very little in common between OF and non-OF versions of this
> function. Why not have two different versions of this function for the
> two cases? The OF version can reside under the CONFIG_OF conditional
> already in use in the file. This will also save you the ugly line breaks
> you had to resort to due to too deep indentation.

Actually those line breaks are not necessary and wouldn't result in
compilation errors. I was planning to drop them. I'll go ahead and split
it out anyway, now that also the OF version of the function is going to
be bit longer if we use the of_parse functions.

Thanks for your review,

Regards,

-Joel

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Sekhar Nori]

On 7/30/2013 9:17 AM, Joel Fernandes wrote:

>>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
>>> index a432e6c..765d578 100644
>>> --- a/arch/arm/common/edma.c
>>> +++ b/arch/arm/common/edma.c

>>> +	} else {
>>> +		for (; i < pdev->num_resources; i++) {
>>> +			if ((pdev->resource[i].flags & IORESOURCE_DMA) &&
>>> +			    (int)pdev->resource[i].start >= 0) {
>>> +				ctlr = EDMA_CTLR(pdev->resource[i].start);
>>> +				clear_bit(EDMA_CHAN_SLOT(
>>> +					  pdev->resource[i].start),
>>> +					  edma_cc[ctlr]->edma_unused);
>>> +			}
>>
>> So there is very little in common between OF and non-OF versions of this
>> function. Why not have two different versions of this function for the
>> two cases? The OF version can reside under the CONFIG_OF conditional
>> already in use in the file. This will also save you the ugly line breaks
>> you had to resort to due to too deep indentation.
> 
> Actually those line breaks are not necessary and wouldn't result in
> compilation errors. I was planning to drop them. I'll go ahead and split
> it out anyway, now that also the OF version of the function is going to
> be bit longer if we use the of_parse functions.
> 
> Thanks for your review,

It turns out, I gave a bad idea. What I suggested will break the case of
non-DT boot with CONFIG_OF enabled. So what you had was fine. May be
just return from "if (dev->of_node)" so you don't need to have an else
block and can save on the indentation.

Thanks,
Sekhar

[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Joel Fernandes]

On 07/30/2013 11:29 AM, Sekhar Nori wrote:
> On 7/30/2013 9:17 AM, Joel Fernandes wrote:
> 
>>>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
>>>> index a432e6c..765d578 100644
>>>> --- a/arch/arm/common/edma.c
>>>> +++ b/arch/arm/common/edma.c
> 
>>>> +	} else {
>>>> +		for (; i < pdev->num_resources; i++) {
>>>> +			if ((pdev->resource[i].flags & IORESOURCE_DMA) &&
>>>> +			    (int)pdev->resource[i].start >= 0) {
>>>> +				ctlr = EDMA_CTLR(pdev->resource[i].start);
>>>> +				clear_bit(EDMA_CHAN_SLOT(
>>>> +					  pdev->resource[i].start),
>>>> +					  edma_cc[ctlr]->edma_unused);
>>>> +			}
>>>
>>> So there is very little in common between OF and non-OF versions of this
>>> function. Why not have two different versions of this function for the
>>> two cases? The OF version can reside under the CONFIG_OF conditional
>>> already in use in the file. This will also save you the ugly line breaks
>>> you had to resort to due to too deep indentation.
>>
>> Actually those line breaks are not necessary and wouldn't result in
>> compilation errors. I was planning to drop them. I'll go ahead and split
>> it out anyway, now that also the OF version of the function is going to
>> be bit longer if we use the of_parse functions.
>>
>> Thanks for your review,
> 
> It turns out, I gave a bad idea. What I suggested will break the case of
> non-DT boot with CONFIG_OF enabled. So what you had was fine. May be
> just return from "if (dev->of_node)" so you don't need to have an else
> block and can save on the indentation.>

Ok, sure. I will go ahead and return from the if block.

Thanks,

-Joel