diff mbox

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

Message ID 1377287613-16491-1-git-send-email-joelf@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Joel Fernandes Aug. 23, 2013, 7:53 p.m. UTC
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(-)

Comments

Sekhar Nori Aug. 26, 2013, 10:46 a.m. UTC | #1
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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joel Fernandes Aug. 26, 2013, 4:52 p.m. UTC | #2
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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sekhar Nori Aug. 26, 2013, 7:34 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Jackson Sept. 6, 2013, 7:13 p.m. UTC | #4
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);
>  		}
>  	}

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Jackson Sept. 6, 2013, 7:15 p.m. UTC | #5
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>

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joel Fernandes Sept. 6, 2013, 7:51 p.m. UTC | #6
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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);
 		}
 	}