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

[PATCH v3] 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.

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. For this purpose
we use the of_* helpers to parse the arguments in the dmas phandle list.

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>
---
This patch is a repost of v2 with minor change of return value.

As such this patch fixes DMA breakages on all dependent peripherals (DMA, AES,
MMC, McASP..) so should go in for -rc cycle.

 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 117f955..88ea067 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 0;
+	}
 
-	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 v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources

[Sekhar Nori]

On Wednesday 11 September 2013 12:22 AM, 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.
> 
> 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. For this purpose
> we use the of_* helpers to parse the arguments in the dmas phandle list.
> 
> 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>
> ---
> This patch is a repost of v2 with minor change of return value.

Is this patch meant for merging? If yes, I see no resolution of the
comments given in this thread:

https://lkml.org/lkml/2013/7/30/9

It is better to send one version with all comments fixed. Helps avoid
confusion.

Thanks,
Sekhar

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

[Joel Fernandes]

On 09/12/2013 04:58 AM, Sekhar Nori wrote:
> On Wednesday 11 September 2013 12:22 AM, 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.
[..]
> It is better to send one version with all comments fixed. Helps avoid
> confusion.

Ok, here is the final version with all comments fixed, please apply this one:

--->8---
From: Joel Fernandes <joelf@ti.com>
Subject: [PATCH v4] ARM: EDMA: Fix clearing of unused list for DT DMA resources

HWMOD removal for MMC is breaking edma_start as the events are being manually
triggered due to unused channel list not being clear.

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. For this purpose
we use the of_* helpers to parse the arguments in the dmas phandle list.

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>
---
Changes since v1, in v2 and v3:
- Reduced indentation of non-of case by returning from of-case
- Using of_* helpers for parsing

Note:
This patch should go into the merge window as it is a critical bug fix.

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

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 39ad030..43c7b22 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -560,14 +560,33 @@ 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, count, ctlr;
+	struct of_phandle_args  dma_spec;

+	if (dev->of_node) {
+		count = of_property_count_strings(dev->of_node, "dma-names");
+		if (count < 0)
+			return 0;
+		for (i = 0; i < count; i++) {
+			if (of_parse_phandle_with_args(dev->of_node, "dmas",
+						       "#dma-cells", i,
+						       &dma_spec))
+				continue;
+
+			ctlr = EDMA_CTLR(dma_spec.args[0]);
+			clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
+				  edma_cc[ctlr]->edma_unused);
+		}
+		return 0;
+	}
+
+	/* For non-OF case */
 	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);
+				  edma_cc[ctlr]->edma_unused);
 		}
 	}

-- 
1.8.1.2

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

[Sekhar Nori]

Hi Joel,

On Saturday 14 September 2013 06:27 AM, Joel Fernandes wrote:
> From: Joel Fernandes <joelf@ti.com>
> Subject: [PATCH v4] ARM: EDMA: Fix clearing of unused list for DT DMA resources
> 
> HWMOD removal for MMC is breaking edma_start as the events are being manually
> triggered due to unused channel list not being clear.
> 
> 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. For this purpose
> we use the of_* helpers to parse the arguments in the dmas phandle list.
> 
> 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>
> ---
> Changes since v1, in v2 and v3:
> - Reduced indentation of non-of case by returning from of-case
> - Using of_* helpers for parsing
> 
> Note:
> This patch should go into the merge window as it is a critical bug fix.

I still cannot find any users of edma in the device tree sources either
in linux-next or linus/master. Why cannot this wait until v3.13?

> 
>  arch/arm/common/edma.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
> index 39ad030..43c7b22 100644
> --- a/arch/arm/common/edma.c
> +++ b/arch/arm/common/edma.c
> @@ -560,14 +560,33 @@ 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, count, ctlr;
> +	struct of_phandle_args  dma_spec;
> 
> +	if (dev->of_node) {
> +		count = of_property_count_strings(dev->of_node, "dma-names");
> +		if (count < 0)
> +			return 0;
> +		for (i = 0; i < count; i++) {
> +			if (of_parse_phandle_with_args(dev->of_node, "dmas",
> +						       "#dma-cells", i,
> +						       &dma_spec))
> +				continue;

This will break for the case where devices on platform bus use non-EDMA
dma controllers like SDMA or CPPI (DRA7x has both EDMA and SDMA on the
same chip). You need to do an additional check to make sure the dma
controller is indeed EDMA. Something like.

	if(!of_device_is_compatible(dma_spec.np, "ti,edma3"))
		continue;

Don forget to call of_node_put() on dma_spec.np (something that needs to
be done even with your current code).

> +
> +			ctlr = EDMA_CTLR(dma_spec.args[0]);
> +			clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
> +				  edma_cc[ctlr]->edma_unused);

We don't support the second controller when using DT and the controller
number is not really encoded in the argument to edma phandle. So just
simplify this to:

	clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), 	
		  edma_cc[0]->edma_unused);

> +		}
> +		return 0;
> +	}
> +
> +	/* For non-OF case */
>  	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);
> +				  edma_cc[ctlr]->edma_unused);

This is a useful change and I am okay with it happening in this
otherwise unrelated patch, but please mention this in changelog.

Thanks,
Sekhar

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

[Joel Fernandes]

On 09/16/2013 06:48 AM, Sekhar Nori wrote:
> Hi Joel,
> 
> On Saturday 14 September 2013 06:27 AM, Joel Fernandes wrote:
>> From: Joel Fernandes <joelf@ti.com>
>> Subject: [PATCH v4] ARM: EDMA: Fix clearing of unused list for DT DMA resources
>>
>> HWMOD removal for MMC is breaking edma_start as the events are being manually
>> triggered due to unused channel list not being clear.
>>
>> 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. For this purpose
>> we use the of_* helpers to parse the arguments in the dmas phandle list.
>>
>> 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>
>> ---
>> Changes since v1, in v2 and v3:
>> - Reduced indentation of non-of case by returning from of-case
>> - Using of_* helpers for parsing
>>
>> Note:
>> This patch should go into the merge window as it is a critical bug fix.
> 
> I still cannot find any users of edma in the device tree sources either
> in linux-next or linus/master. Why cannot this wait until v3.13?

I understand this affects only DT users of EDMA. But I get so many private
reports of breakage due to this patch not being there that I think it will save
everyone a lot of pain, specially folks creating integration trees to have this
patch available by default.

Further, EDMA DT enabling is surely to go in for 3.13, so its best if this is
applied in advance here.

I feel we shouldn't leave code intentionally broken just because it is not yet
enabled in DTS, specially when it is about to be enabled in DT. For example, a
potential problem is MMC/SD file system corruption due to DMA failure.

>>  arch/arm/common/edma.c | 23 +++++++++++++++++++++--
>>  1 file changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
>> index 39ad030..43c7b22 100644
>> --- a/arch/arm/common/edma.c
>> +++ b/arch/arm/common/edma.c
>> @@ -560,14 +560,33 @@ 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, count, ctlr;
>> +	struct of_phandle_args  dma_spec;
>>
>> +	if (dev->of_node) {
>> +		count = of_property_count_strings(dev->of_node, "dma-names");
>> +		if (count < 0)
>> +			return 0;
>> +		for (i = 0; i < count; i++) {
>> +			if (of_parse_phandle_with_args(dev->of_node, "dmas",
>> +						       "#dma-cells", i,
>> +						       &dma_spec))
>> +				continue;
> 
> This will break for the case where devices on platform bus use non-EDMA
> dma controllers like SDMA or CPPI (DRA7x has both EDMA and SDMA on the
> same chip). You need to do an additional check to make sure the dma
> controller is indeed EDMA. Something like.

Ok, edma is probed earlier so I could never see any problem.
Thanks for pointing this out,

Using the below method is more future-proof than using compatible literal
strings directly. The only problem is the matches table has to be defined
earlier in the sources. What do you think?

                        if (!of_match_node(edma_of_ids, dma_spec.np) {
                                of_node_put(dma_spec.np);
                                continue;
                        }


> 	if(!of_device_is_compatible(dma_spec.np, "ti,edma3"))
> 		continue;
> 
> Don forget to call of_node_put() on dma_spec.np (something that needs to
> be done even with your current code).

Ok, will do.


>> +
>> +			ctlr = EDMA_CTLR(dma_spec.args[0]);
>> +			clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
>> +				  edma_cc[ctlr]->edma_unused);
> 
> We don't support the second controller when using DT and the controller
> number is not really encoded in the argument to edma phandle. So just
> simplify this to:
> 
> 	clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), 	
> 		  edma_cc[0]->edma_unused);

I think let's not make that assumption just incase in the future we support more
than one EDMA controller for DT-based boot. Is that ok?

> 
>> +		}
>> +		return 0;
>> +	}
>> +
>> +	/* For non-OF case */
>>  	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);
>> +				  edma_cc[ctlr]->edma_unused);
> 
> This is a useful change and I am okay with it happening in this
> otherwise unrelated patch, but please mention this in changelog.

Below is the updated version (v5), can you check and let me know if you had any
other comments?

---8<---
From: Joel Fernandes <joelf@ti.com>
Subject: [PATCH v5] ARM: EDMA: Fix clearing of unused list for DT DMA resources

HWMOD removal for MMC is breaking edma_start as the events are being manually
triggered due to unused channel list not being clear.

The above issue is fixed by reading the "dmas" property from the DT node if it
exists and clearing the bits in the unused channel list if the dma controller
used by any device is EDMA. For this purpose we use the of_* helpers to parse
the arguments in the dmas phandle list.

Also introduced is a minor clean up of a checkpatch error in old code.

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>
---
Changes since v4:
- Using of_node_put on dma_spec's node pointer.
- Update changelog with minor cleanup information.

Changes since v1, in v2 and v3:
 - Reduced indentation of non-of case by returning from of-case
 - Using of_* helpers for parsing

 arch/arm/common/edma.c | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 117f955..8b5c6ed 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -269,6 +269,11 @@ static const struct edmacc_param dummy_paramset = {
 	.ccnt = 1,
 };

+static const struct of_device_id edma_of_ids[] = {
+	{ .compatible = "ti,edma3", },
+	{}
+};
+
 /*****************************************************************************/

 static void map_dmach_queue(unsigned ctlr, unsigned ch_no,
@@ -560,14 +565,39 @@ 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, count, ctlr;
+	struct of_phandle_args  dma_spec;

+	if (dev->of_node) {
+		count = of_property_count_strings(dev->of_node, "dma-names");
+		if (count < 0)
+			return 0;
+		for (i = 0; i < count; i++) {
+			if (of_parse_phandle_with_args(dev->of_node, "dmas",
+						       "#dma-cells", i,
+						       &dma_spec))
+				continue;
+
+			if (!of_match_node(edma_of_ids, dma_spec.np)) {
+				of_node_put(dma_spec.np);
+				continue;
+			}
+
+			ctlr = EDMA_CTLR(dma_spec.args[0]);
+			clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
+				  edma_cc[ctlr]->edma_unused);
+			of_node_put(dma_spec.np);
+		}
+		return 0;
+	}
+
+	/* For non-OF case */
 	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);
+				  edma_cc[ctlr]->edma_unused);
 		}
 	}

@@ -1762,11 +1792,6 @@ static int edma_probe(struct platform_device *pdev)
 	return 0;
 }

-static const struct of_device_id edma_of_ids[] = {
-	{ .compatible = "ti,edma3", },
-	{}
-};
-
 static struct platform_driver edma_driver = {
 	.driver = {
 		.name	= "edma",
-- 
1.8.1.2

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

[Sekhar Nori]

On Monday 16 September 2013 09:56 PM, Joel Fernandes wrote:
> On 09/16/2013 06:48 AM, Sekhar Nori wrote:
>> Hi Joel,
>>
>> On Saturday 14 September 2013 06:27 AM, Joel Fernandes wrote:
>>> From: Joel Fernandes <joelf@ti.com>
>>> Subject: [PATCH v4] ARM: EDMA: Fix clearing of unused list for DT DMA resources
>>>
>>> HWMOD removal for MMC is breaking edma_start as the events are being manually
>>> triggered due to unused channel list not being clear.
>>>
>>> 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. For this purpose
>>> we use the of_* helpers to parse the arguments in the dmas phandle list.
>>>
>>> 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>
>>> ---
>>> Changes since v1, in v2 and v3:
>>> - Reduced indentation of non-of case by returning from of-case
>>> - Using of_* helpers for parsing
>>>
>>> Note:
>>> This patch should go into the merge window as it is a critical bug fix.
>>
>> I still cannot find any users of edma in the device tree sources either
>> in linux-next or linus/master. Why cannot this wait until v3.13?
> 
> I understand this affects only DT users of EDMA. But I get so many private
> reports of breakage due to this patch not being there that I think it will save
> everyone a lot of pain, specially folks creating integration trees to have this
> patch available by default.

Well, I do agree that the current DT support for EDMA is incomplete
without this patch even if there are no in-kernel users of it. I will
try sending this for the next -rc if we get to the final version in time
and after that its upto the upstreams to take it.

>>>  arch/arm/common/edma.c | 23 +++++++++++++++++++++--
>>>  1 file changed, 21 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
>>> index 39ad030..43c7b22 100644
>>> --- a/arch/arm/common/edma.c
>>> +++ b/arch/arm/common/edma.c
>>> @@ -560,14 +560,33 @@ 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, count, ctlr;
>>> +	struct of_phandle_args  dma_spec;
>>>
>>> +	if (dev->of_node) {
>>> +		count = of_property_count_strings(dev->of_node, "dma-names");
>>> +		if (count < 0)
>>> +			return 0;
>>> +		for (i = 0; i < count; i++) {
>>> +			if (of_parse_phandle_with_args(dev->of_node, "dmas",
>>> +						       "#dma-cells", i,
>>> +						       &dma_spec))
>>> +				continue;
>>
>> This will break for the case where devices on platform bus use non-EDMA
>> dma controllers like SDMA or CPPI (DRA7x has both EDMA and SDMA on the
>> same chip). You need to do an additional check to make sure the dma
>> controller is indeed EDMA. Something like.
> 
> Ok, edma is probed earlier so I could never see any problem.

This has got nothing to do with edma probe order.

> Thanks for pointing this out,
> 
> Using the below method is more future-proof than using compatible literal
> strings directly. The only problem is the matches table has to be defined
> earlier in the sources. What do you think?
> 
>                         if (!of_match_node(edma_of_ids, dma_spec.np) {
>                                 of_node_put(dma_spec.np);
>                                 continue;
>                         }

Looks fine to me.

>> 	if(!of_device_is_compatible(dma_spec.np, "ti,edma3"))
>> 		continue;
>>
>> Don forget to call of_node_put() on dma_spec.np (something that needs to
>> be done even with your current code).
> 
> Ok, will do.
> 
> 
>>> +
>>> +			ctlr = EDMA_CTLR(dma_spec.args[0]);
>>> +			clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
>>> +				  edma_cc[ctlr]->edma_unused);
>>
>> We don't support the second controller when using DT and the controller
>> number is not really encoded in the argument to edma phandle. So just
>> simplify this to:
>>
>> 	clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), 	
>> 		  edma_cc[0]->edma_unused);
> 
> I think let's not make that assumption just incase in the future we support more
> than one EDMA controller for DT-based boot. Is that ok?

We don't write future proof code in that _hope_ that it will get used
someday.  In fact this will confuse the reader into wondering if support
for second channel controller is present or not as the code will send
mixed messages. In short, we aim for consistency with situation today,
not tomorrow.

Besides, I can bet that when second CC support does get added, it is
very unlikely that the CC number is get encoded into channel number when
using DT.

Thanks,
Sekhar

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

[Joel Fernandes]

On 09/17/2013 12:08 AM, Sekhar Nori wrote:
[..]
>>> I still cannot find any users of edma in the device tree sources either
>>> in linux-next or linus/master. Why cannot this wait until v3.13?
>>
>> I understand this affects only DT users of EDMA. But I get so many private
>> reports of breakage due to this patch not being there that I think it will save
>> everyone a lot of pain, specially folks creating integration trees to have this
>> patch available by default.
> 
> Well, I do agree that the current DT support for EDMA is incomplete
> without this patch even if there are no in-kernel users of it. I will
> try sending this for the next -rc if we get to the final version in time
> and after that its upto the upstreams to take it.

Ok, except for the one minor nit below my last scissor patch is good to go.

>>>> +
>>>> +			ctlr = EDMA_CTLR(dma_spec.args[0]);
>>>> +			clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
>>>> +				  edma_cc[ctlr]->edma_unused);
>>>
>>> We don't support the second controller when using DT and the controller
>>> number is not really encoded in the argument to edma phandle. So just
>>> simplify this to:
>>>
>>> 	clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), 	
>>> 		  edma_cc[0]->edma_unused);
>>
>> I think let's not make that assumption just incase in the future we support more
>> than one EDMA controller for DT-based boot. Is that ok?
> 
> We don't write future proof code in that _hope_ that it will get used
> someday.  In fact this will confuse the reader into wondering if support
> for second channel controller is present or not as the code will send

Nope I don't agree with this at all.. EDMA CC ctrl will not be hardcoded. We
need to write future-proof code to make sure sudden regressions don't pop up
when say a second EDMA CC is introduced.. further edma_cc[ctrl] pattern is used
all through out the code so what you're asking to do doesn't make much sense in
this context. There's no reason to break out of this pattern. It actually will
confuse the reader more.

Second controller can be present in future. I don't want to come back to change
the code when we introduce more than 1 CC which is possible in the future.

> mixed messages. In short, we aim for consistency with situation today,
> not tomorrow.

What you're asking to do infact breaks consistency with the rest of the code.

> 
> Besides, I can bet that when second CC support does get added, it is
> very unlikely that the CC number is get encoded into channel number when
> using DT.

Even if it is not encoded, the data structure for edma_cc is an array and what
you're asking is to hardcode the controller number to 0 always. No way I'm going
to hard code controller number to a single value.

Different topic but anyway why wouldn't ctrl number be encoded in the channel?
That's clean, and saves variables and extra structures. Better use of the
integer bitmap making up the Ctrl and channel number of small ranges.

Regards,

-Joel

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

[Sekhar Nori]

On Tuesday 17 September 2013 11:08 AM, Joel Fernandes wrote:
> On 09/17/2013 12:08 AM, Sekhar Nori wrote:
> [..]
>>>> I still cannot find any users of edma in the device tree sources either
>>>> in linux-next or linus/master. Why cannot this wait until v3.13?
>>>
>>> I understand this affects only DT users of EDMA. But I get so many private
>>> reports of breakage due to this patch not being there that I think it will save
>>> everyone a lot of pain, specially folks creating integration trees to have this
>>> patch available by default.
>>
>> Well, I do agree that the current DT support for EDMA is incomplete
>> without this patch even if there are no in-kernel users of it. I will
>> try sending this for the next -rc if we get to the final version in time
>> and after that its upto the upstreams to take it.
> 
> Ok, except for the one minor nit below my last scissor patch is good to go.
> 
>>>>> +
>>>>> +			ctlr = EDMA_CTLR(dma_spec.args[0]);
>>>>> +			clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
>>>>> +				  edma_cc[ctlr]->edma_unused);
>>>>
>>>> We don't support the second controller when using DT and the controller
>>>> number is not really encoded in the argument to edma phandle. So just
>>>> simplify this to:
>>>>
>>>> 	clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), 	
>>>> 		  edma_cc[0]->edma_unused);
>>>
>>> I think let's not make that assumption just incase in the future we support more
>>> than one EDMA controller for DT-based boot. Is that ok?
>>
>> We don't write future proof code in that _hope_ that it will get used
>> someday.  In fact this will confuse the reader into wondering if support
>> for second channel controller is present or not as the code will send
> 
> Nope I don't agree with this at all.. EDMA CC ctrl will not be hardcoded. We
> need to write future-proof code to make sure sudden regressions don't pop up
> when say a second EDMA CC is introduced.. further edma_cc[ctrl] pattern is used

I am not sure why you call this a regression. There is no support for
second channel controller. If someone wants to add that support there is
a lot to worry about than just these two lines of code. So, no, there
wont be "sudden regressions that pop up" like you say. Look at the probe
routine. There is no way the second CC is going to get probed.

> all through out the code so what you're asking to do doesn't make much sense in
> this context. There's no reason to break out of this pattern. It actually will
> confuse the reader more.

Thats because that code is also used for existing platform data based
approach. If you are adding code paths which just get exercised with DT,
no point referring to the second channel controller since that does not
exist.

> 
> Second controller can be present in future. I don't want to come back to change
> the code when we introduce more than 1 CC which is possible in the future.

You *will* have to come back and change the code to support the second
channel controller. That is true with or without this patch. So lets
stop arguing like the second channel controller support is only blocked
because of these two lines of code.

> 
>> mixed messages. In short, we aim for consistency with situation today,
>> not tomorrow.
> 
> What you're asking to do infact breaks consistency with the rest of the code.

Well, ideally we support second CC even with DT to be consistent all
around. Since that has not happened, I don't want the code to pretend
that it it supports the second channel controller with DT that too only
in parts of code.

> 
>>
>> Besides, I can bet that when second CC support does get added, it is
>> very unlikely that the CC number is get encoded into channel number when
>> using DT.
> 
> Even if it is not encoded, the data structure for edma_cc is an array and what
> you're asking is to hardcode the controller number to 0 always. No way I'm going
> to hard code controller number to a single value.

You are missing the point. Look at the code. You are extracting
controller number from dma_spec.args[0] which is directly the value
present in DT. The binding today does not specify that the value be
encoded with controller number. So I will have not have code that
interprets it that way.

The code follows from bindings, not the other way around.

> 
> Different topic but anyway why wouldn't ctrl number be encoded in the channel?
> That's clean, and saves variables and extra structures. Better use of the
> integer bitmap making up the Ctrl and channel number of small ranges.

Lets have this discussion when you update the bindings to support the
second controller.

Thanks,
Sekhar

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

[Joel Fernandes]

On 09/17/2013 01:05 AM, Sekhar Nori wrote:
[..]
>>> mixed messages. In short, we aim for consistency with situation today,
>>> not tomorrow.
>>
>> What you're asking to do infact breaks consistency with the rest of the code.
> 
> Well, ideally we support second CC even with DT to be consistent all
> around. Since that has not happened, I don't want the code to pretend
> that it it supports the second channel controller with DT that too only
> in parts of code.

Ok, I agree that the bindings don't talk about encoding a controller number in
the channel provided from DT so it shouldn't assume that without binding
updates. Following this suggestion, I've update the patch to the below:

---8<---
From: Joel Fernandes <joelf@ti.com>
Subject: [PATCH v6] ARM: EDMA: Fix clearing of unused list for DT DMA resources

HWMOD removal for MMC is breaking edma_start as the events are being manually
triggered due to unused channel list not being clear.

The above issue is fixed by reading the "dmas" property from the DT node if it
exists and clearing the bits in the unused channel list if the dma controller
used by any device is EDMA. For this purpose we use the of_* helpers to parse
the arguments in the dmas phandle list.

Also introduced is a minor clean up of a checkpatch error in old code.

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>
---
Changes since initial version:
- Using of_node_put on dma_spec's node pointer.
- Update changelog with minor cleanup information.
- For DT-case, set controller number as 0.
- Reduced indentation of non-of case by returning from of-case
- Using of_* helpers for parsing

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

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 117f955..8e1a024 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -269,6 +269,11 @@ static const struct edmacc_param dummy_paramset = {
 	.ccnt = 1,
 };

+static const struct of_device_id edma_of_ids[] = {
+	{ .compatible = "ti,edma3", },
+	{}
+};
+
 /*****************************************************************************/

 static void map_dmach_queue(unsigned ctlr, unsigned ch_no,
@@ -560,14 +565,38 @@ 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, count, ctlr;
+	struct of_phandle_args  dma_spec;

+	if (dev->of_node) {
+		count = of_property_count_strings(dev->of_node, "dma-names");
+		if (count < 0)
+			return 0;
+		for (i = 0; i < count; i++) {
+			if (of_parse_phandle_with_args(dev->of_node, "dmas",
+						       "#dma-cells", i,
+						       &dma_spec))
+				continue;
+
+			if (!of_match_node(edma_of_ids, dma_spec.np)) {
+				of_node_put(dma_spec.np);
+				continue;
+			}
+
+			clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
+				  edma_cc[0]->edma_unused);
+			of_node_put(dma_spec.np);
+		}
+		return 0;
+	}
+
+	/* For non-OF case */
 	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);
+				  edma_cc[ctlr]->edma_unused);
 		}
 	}

@@ -1762,11 +1791,6 @@ static int edma_probe(struct platform_device *pdev)
 	return 0;
 }

-static const struct of_device_id edma_of_ids[] = {
-	{ .compatible = "ti,edma3", },
-	{}
-};
-
 static struct platform_driver edma_driver = {
 	.driver = {
 		.name	= "edma",
-- 
1.8.1.2

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

[Sekhar Nori]

On Tuesday 17 September 2013 07:59 PM, Joel Fernandes wrote:
> On 09/17/2013 01:05 AM, Sekhar Nori wrote:
> [..]
>>>> mixed messages. In short, we aim for consistency with situation today,
>>>> not tomorrow.
>>>
>>> What you're asking to do infact breaks consistency with the rest of the code.
>>
>> Well, ideally we support second CC even with DT to be consistent all
>> around. Since that has not happened, I don't want the code to pretend
>> that it it supports the second channel controller with DT that too only
>> in parts of code.
> 
> Ok, I agree that the bindings don't talk about encoding a controller number in
> the channel provided from DT so it shouldn't assume that without binding
> updates. Following this suggestion, I've update the patch to the below:
> 
> ---8<---
> From: Joel Fernandes <joelf@ti.com>
> Subject: [PATCH v6] ARM: EDMA: Fix clearing of unused list for DT DMA resources
> 
> HWMOD removal for MMC is breaking edma_start as the events are being manually
> triggered due to unused channel list not being clear.
> 
> The above issue is fixed by reading the "dmas" property from the DT node if it
> exists and clearing the bits in the unused channel list if the dma controller
> used by any device is EDMA. For this purpose we use the of_* helpers to parse
> the arguments in the dmas phandle list.
> 
> Also introduced is a minor clean up of a checkpatch error in old code.

The patch looks good to me. I made some modifications to commit message
- mostly for brevity.

    ARM: edma: dt: create unused channel list

    HWMOD removal for MMC is breaking edma_start() as the events
    are being manually triggered due to EDMA unused channel list
    not being ready.

    The above issue is fixed by reading the "dmas" property from
    the DT node if it exists and then clearing the bits in the
    unused channel list if the dma controller used by any device
    is EDMA.

    This is similar to approach taken in non-DT case.

    Also introduced is a minor clean up of a checkpatch error in
    old code.

Thanks,
Sekhar

[PATCH v3] 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.

The above issue is fixed by reading the "dmas" property from the DT node if it
exists and clearing the bits in the unused channel list if the dma controller
used by any device is EDMA. For this purpose we use the of_* helpers to parse
the arguments in the dmas phandle list.

Also introduced is a minor clean up of a checkpatch error in old code.

Reviewed-by: Sekhar Nori <nsekhar@ti.com>
Reported-by: Balaji T K <balajitk@ti.com>
Cc: Sekhar Nori <nsekhar@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Olof Johansson <olof@lixom.net>
Cc: Nishanth Menon <nm@ti.com>
Cc: Pantel Antoniou <panto@antoniou-consulting.com>
Cc: Jason Kridner <jkridner@beagleboard.org>
Cc: Koen Kooi <koen@dominion.thruhere.net>
Signed-off-by: Joel Fernandes <joelf@ti.com>
---
Just resending this patch after discussing with Sekhar and Olof.

AM335x is being booted by many users such as the beaglebone community. DT is
the only boot method available for all these users.  EDMA is required for the
operation for many common peripherals in AM335x SoC such as McASP, MMC and
Crypto.

Although EDMA DT nodes are going in only for 3.13, in reality the kernel has
been used for more than a year with EDMA code and out of tree EDMA DTS patches.
Hence though the DT nodes are still not in mainline, this patch can be still be
considered a critical fix as such and it would be great if it could be included
in 3.12-rc release. Thanks.

More details about why this broke an existing feature folks were using:
Previously the DMA resources for platform devices were being populated through
HWMOD, however with the recent clean ups with HWMOD, this data has been moved
to Device tree. The EDMA code though is not aware of this so it fails to fetch
the DMA resources correctly which it needs to prepare the unused channel list
(basically doesn't properly clear the channels that are in use, in the unused
list).

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

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 117f955..8e1a024 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -269,6 +269,11 @@ static const struct edmacc_param dummy_paramset = {
 	.ccnt = 1,
 };
 
+static const struct of_device_id edma_of_ids[] = {
+	{ .compatible = "ti,edma3", },
+	{}
+};
+
 /*****************************************************************************/
 
 static void map_dmach_queue(unsigned ctlr, unsigned ch_no,
@@ -560,14 +565,38 @@ 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, count, ctlr;
+	struct of_phandle_args  dma_spec;
 
+	if (dev->of_node) {
+		count = of_property_count_strings(dev->of_node, "dma-names");
+		if (count < 0)
+			return 0;
+		for (i = 0; i < count; i++) {
+			if (of_parse_phandle_with_args(dev->of_node, "dmas",
+						       "#dma-cells", i,
+						       &dma_spec))
+				continue;
+
+			if (!of_match_node(edma_of_ids, dma_spec.np)) {
+				of_node_put(dma_spec.np);
+				continue;
+			}
+
+			clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
+				  edma_cc[0]->edma_unused);
+			of_node_put(dma_spec.np);
+		}
+		return 0;
+	}
+
+	/* For non-OF case */
 	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);
+				  edma_cc[ctlr]->edma_unused);
 		}
 	}
 
@@ -1762,11 +1791,6 @@ static int edma_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id edma_of_ids[] = {
-	{ .compatible = "ti,edma3", },
-	{}
-};
-
 static struct platform_driver edma_driver = {
 	.driver = {
 		.name	= "edma",
-- 
1.8.1.2

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

[Olof Johansson]

On Thu, Sep 26, 2013 at 2:55 PM, Joel Fernandes <joelf@ti.com> wrote:
> HWMOD removal for MMC is breaking edma_start as the events are being manually
> triggered due to unused channel list not being clear.
>
> The above issue is fixed by reading the "dmas" property from the DT node if it
> exists and clearing the bits in the unused channel list if the dma controller
> used by any device is EDMA. For this purpose we use the of_* helpers to parse
> the arguments in the dmas phandle list.
>
> Also introduced is a minor clean up of a checkpatch error in old code.
>
> Reviewed-by: Sekhar Nori <nsekhar@ti.com>
> Reported-by: Balaji T K <balajitk@ti.com>
> Cc: Sekhar Nori <nsekhar@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
> Cc: Jason Kridner <jkridner@beagleboard.org>
> Cc: Koen Kooi <koen@dominion.thruhere.net>
> Signed-off-by: Joel Fernandes <joelf@ti.com>
> ---
> Just resending this patch after discussing with Sekhar and Olof.

Actually, the patch you talked to me about was v3 of this. It seems
that you have reposted v6 but labelled it v3. This is very confusing.

> AM335x is being booted by many users such as the beaglebone community. DT is
> the only boot method available for all these users.  EDMA is required for the
> operation for many common peripherals in AM335x SoC such as McASP, MMC and
> Crypto.
>
> Although EDMA DT nodes are going in only for 3.13, in reality the kernel has
> been used for more than a year with EDMA code and out of tree EDMA DTS patches.
> Hence though the DT nodes are still not in mainline, this patch can be still be
> considered a critical fix as such and it would be great if it could be included
> in 3.12-rc release. Thanks.

This is really the root of this problem. If you sit on code out of
tree for a year, and something breaks that we couldn't even have
detected since we didn't have the out-of-tree pieces. We'll help you
the first few times (such as now) but we will eventually stop caring.

If I was in a worse mood, then I'd just say that since your users
already has to have out-of-tree code to even use this functionality,
they could just add this fix on top of that stack of patches. But I'm
in a slightly better mood than that and I'll pick it up this time. :)

> More details about why this broke an existing feature folks were using:
> Previously the DMA resources for platform devices were being populated through
> HWMOD, however with the recent clean ups with HWMOD, this data has been moved
> to Device tree. The EDMA code though is not aware of this so it fails to fetch
> the DMA resources correctly which it needs to prepare the unused channel list
> (basically doesn't properly clear the channels that are in use, in the unused
> list).

So that we can learn for next time: What should we (as in us
maintainers and you TI) have done differently to avoid this?


-Olof

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

[Joel Fernandes]

On 09/26/2013 06:13 PM, Olof Johansson wrote:
> On Thu, Sep 26, 2013 at 2:55 PM, Joel Fernandes <joelf@ti.com> wrote:
>> HWMOD removal for MMC is breaking edma_start as the events are being manually
>> triggered due to unused channel list not being clear.
>>
>> The above issue is fixed by reading the "dmas" property from the DT node if it
>> exists and clearing the bits in the unused channel list if the dma controller
>> used by any device is EDMA. For this purpose we use the of_* helpers to parse
>> the arguments in the dmas phandle list.
>>
>> Also introduced is a minor clean up of a checkpatch error in old code.
>>
>> Reviewed-by: Sekhar Nori <nsekhar@ti.com>
>> Reported-by: Balaji T K <balajitk@ti.com>
>> Cc: Sekhar Nori <nsekhar@ti.com>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Olof Johansson <olof@lixom.net>
>> Cc: Nishanth Menon <nm@ti.com>
>> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
>> Cc: Jason Kridner <jkridner@beagleboard.org>
>> Cc: Koen Kooi <koen@dominion.thruhere.net>
>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>> ---
>> Just resending this patch after discussing with Sekhar and Olof.
> 
> Actually, the patch you talked to me about was v3 of this. It seems
> that you have reposted v6 but labelled it v3. This is very confusing.

Sorry about this. :-( This is indeed v6.

>> AM335x is being booted by many users such as the beaglebone community. DT is
>> the only boot method available for all these users.  EDMA is required for the
>> operation for many common peripherals in AM335x SoC such as McASP, MMC and
>> Crypto.
>>
>> Although EDMA DT nodes are going in only for 3.13, in reality the kernel has
>> been used for more than a year with EDMA code and out of tree EDMA DTS patches.
>> Hence though the DT nodes are still not in mainline, this patch can be still be
>> considered a critical fix as such and it would be great if it could be included
>> in 3.12-rc release. Thanks.
> 
> This is really the root of this problem. If you sit on code out of
> tree for a year, and something breaks that we couldn't even have
> detected since we didn't have the out-of-tree pieces. We'll help you
> the first few times (such as now) but we will eventually stop caring.

When I started looking at EDMA in June, I noticed that a lot had already been
merged. EDMA DMA Engine driver itself was merged last year, no worries there.
but the long pending list of fixes to be made to the driver had to written and
rewritten multiple times which took a long time.

Due to this, the EDMA device tree entries could not be merged in fear that doing
so would cause problems such as MMC/SD corruption etc.

> If I was in a worse mood, then I'd just say that since your users
> already has to have out-of-tree code to even use this functionality,
> they could just add this fix on top of that stack of patches. But I'm
> in a slightly better mood than that and I'll pick it up this time. :)

Thank you! :)

>> More details about why this broke an existing feature folks were using:
>> Previously the DMA resources for platform devices were being populated through
>> HWMOD, however with the recent clean ups with HWMOD, this data has been moved
>> to Device tree. The EDMA code though is not aware of this so it fails to fetch
>> the DMA resources correctly which it needs to prepare the unused channel list
>> (basically doesn't properly clear the channels that are in use, in the unused
>> list).
> 
> So that we can learn for next time: What should we (as in us
> maintainers and you TI) have done differently to avoid this?

I think a little on both sides can be improved.

For TI, a bit more work toward explaining patches better in change logs so that
maintainers understand and are willing to help to get the code upstream would
help. A lot of improvement to internal policies have been made for developing on
upstream, and that's certainly a good thing but there's still more room for
improvement.

For maintainers,  EDMA code would have been kept breathing all these months
(atleast 8 months) if those temporary fixes mentioned above would have been
merged into the kernel instead of not applied. With those fixes in place, dts
could have been enabled and EDMA would be fully working all these months. This
would have certainly made things a lot easier. Rewriting stuff the right way is
OK but if it is a _lot_ of effort, then to keep things alive, merging stuff "for
now" specially if they are one-liners could be made acceptable.

EDMA fixes have now been written the "right way", so those temporary fixes are
no longer needed :) but it certainly took a long time to do it the right way
during which things were dead. Only thing pending for working EDMA now is the
dts patches which are already in Benoit's for-3.13 tree and this patch that
you're picking up.

Thanks Olof for your help! :)

regards,

-Joel

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

[Sekhar Nori]

On 9/27/2013 5:58 AM, Joel Fernandes wrote:
> On 09/26/2013 06:13 PM, Olof Johansson wrote:
>> On Thu, Sep 26, 2013 at 2:55 PM, Joel Fernandes <joelf@ti.com> wrote:
>>> HWMOD removal for MMC is breaking edma_start as the events are being manually
>>> triggered due to unused channel list not being clear.
>>>
>>> The above issue is fixed by reading the "dmas" property from the DT node if it
>>> exists and clearing the bits in the unused channel list if the dma controller
>>> used by any device is EDMA. For this purpose we use the of_* helpers to parse
>>> the arguments in the dmas phandle list.
>>>
>>> Also introduced is a minor clean up of a checkpatch error in old code.
>>>
>>> Reviewed-by: Sekhar Nori <nsekhar@ti.com>
>>> Reported-by: Balaji T K <balajitk@ti.com>
>>> Cc: Sekhar Nori <nsekhar@ti.com>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>> Cc: Olof Johansson <olof@lixom.net>
>>> Cc: Nishanth Menon <nm@ti.com>
>>> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
>>> Cc: Jason Kridner <jkridner@beagleboard.org>
>>> Cc: Koen Kooi <koen@dominion.thruhere.net>
>>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>>> ---
>>> Just resending this patch after discussing with Sekhar and Olof.
>>
>> Actually, the patch you talked to me about was v3 of this. It seems
>> that you have reposted v6 but labelled it v3. This is very confusing.
> 
> Sorry about this. :-( This is indeed v6.
> 
>>> AM335x is being booted by many users such as the beaglebone community. DT is
>>> the only boot method available for all these users.  EDMA is required for the
>>> operation for many common peripherals in AM335x SoC such as McASP, MMC and
>>> Crypto.
>>>
>>> Although EDMA DT nodes are going in only for 3.13, in reality the kernel has
>>> been used for more than a year with EDMA code and out of tree EDMA DTS patches.
>>> Hence though the DT nodes are still not in mainline, this patch can be still be
>>> considered a critical fix as such and it would be great if it could be included
>>> in 3.12-rc release. Thanks.
>>
>> This is really the root of this problem. If you sit on code out of
>> tree for a year, and something breaks that we couldn't even have
>> detected since we didn't have the out-of-tree pieces. We'll help you
>> the first few times (such as now) but we will eventually stop caring.
> 
> When I started looking at EDMA in June, I noticed that a lot had already been
> merged. EDMA DMA Engine driver itself was merged last year, no worries there.
> but the long pending list of fixes to be made to the driver had to written and
> rewritten multiple times which took a long time.
> 
> Due to this, the EDMA device tree entries could not be merged in fear that doing
> so would cause problems such as MMC/SD corruption etc.
> 
>> If I was in a worse mood, then I'd just say that since your users
>> already has to have out-of-tree code to even use this functionality,
>> they could just add this fix on top of that stack of patches. But I'm
>> in a slightly better mood than that and I'll pick it up this time. :)
> 
> Thank you! :)
> 
>>> More details about why this broke an existing feature folks were using:
>>> Previously the DMA resources for platform devices were being populated through
>>> HWMOD, however with the recent clean ups with HWMOD, this data has been moved
>>> to Device tree. The EDMA code though is not aware of this so it fails to fetch
>>> the DMA resources correctly which it needs to prepare the unused channel list
>>> (basically doesn't properly clear the channels that are in use, in the unused
>>> list).
>>
>> So that we can learn for next time: What should we (as in us
>> maintainers and you TI) have done differently to avoid this?
> 
> I think a little on both sides can be improved.
> 
> For TI, a bit more work toward explaining patches better in change logs so that
> maintainers understand and are willing to help to get the code upstream would
> help. A lot of improvement to internal policies have been made for developing on
> upstream, and that's certainly a good thing but there's still more room for
> improvement.
> 
> For maintainers,  EDMA code would have been kept breathing all these months
> (atleast 8 months) if those temporary fixes mentioned above would have been
> merged into the kernel instead of not applied. With those fixes in place, dts
> could have been enabled and EDMA would be fully working all these months. This
> would have certainly made things a lot easier. Rewriting stuff the right way is
> OK but if it is a _lot_ of effort, then to keep things alive, merging stuff "for
> now" specially if they are one-liners could be made acceptable.

Joel, can you give a link to the "one-liner" "temporary fix" that was
was not merged? I am unable to put it in context.

Thanks,
Sekhar

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

[Sekhar Nori]

On 9/27/2013 5:58 AM, Joel Fernandes wrote:
> On 09/26/2013 06:13 PM, Olof Johansson wrote:
>> On Thu, Sep 26, 2013 at 2:55 PM, Joel Fernandes <joelf@ti.com> wrote:
>>> HWMOD removal for MMC is breaking edma_start as the events are being manually
>>> triggered due to unused channel list not being clear.
>>>
>>> The above issue is fixed by reading the "dmas" property from the DT node if it
>>> exists and clearing the bits in the unused channel list if the dma controller
>>> used by any device is EDMA. For this purpose we use the of_* helpers to parse
>>> the arguments in the dmas phandle list.
>>>
>>> Also introduced is a minor clean up of a checkpatch error in old code.
>>>
>>> Reviewed-by: Sekhar Nori <nsekhar@ti.com>
>>> Reported-by: Balaji T K <balajitk@ti.com>
>>> Cc: Sekhar Nori <nsekhar@ti.com>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>> Cc: Olof Johansson <olof@lixom.net>
>>> Cc: Nishanth Menon <nm@ti.com>
>>> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
>>> Cc: Jason Kridner <jkridner@beagleboard.org>
>>> Cc: Koen Kooi <koen@dominion.thruhere.net>
>>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>>> ---
>>> Just resending this patch after discussing with Sekhar and Olof.
>>
>> Actually, the patch you talked to me about was v3 of this. It seems
>> that you have reposted v6 but labelled it v3. This is very confusing.
> 
> Sorry about this. :-( This is indeed v6.
> 
>>> AM335x is being booted by many users such as the beaglebone community. DT is
>>> the only boot method available for all these users.  EDMA is required for the
>>> operation for many common peripherals in AM335x SoC such as McASP, MMC and
>>> Crypto.
>>>
>>> Although EDMA DT nodes are going in only for 3.13, in reality the kernel has
>>> been used for more than a year with EDMA code and out of tree EDMA DTS patches.
>>> Hence though the DT nodes are still not in mainline, this patch can be still be
>>> considered a critical fix as such and it would be great if it could be included
>>> in 3.12-rc release. Thanks.
>>
>> This is really the root of this problem. If you sit on code out of
>> tree for a year, and something breaks that we couldn't even have
>> detected since we didn't have the out-of-tree pieces. We'll help you
>> the first few times (such as now) but we will eventually stop caring.
> 
> When I started looking at EDMA in June, I noticed that a lot had already been
> merged. EDMA DMA Engine driver itself was merged last year, no worries there.
> but the long pending list of fixes to be made to the driver had to written and
> rewritten multiple times which took a long time.
> 
> Due to this, the EDMA device tree entries could not be merged in fear that doing
> so would cause problems such as MMC/SD corruption etc.
> 
>> If I was in a worse mood, then I'd just say that since your users
>> already has to have out-of-tree code to even use this functionality,
>> they could just add this fix on top of that stack of patches. But I'm
>> in a slightly better mood than that and I'll pick it up this time. :)
> 
> Thank you! :)
> 
>>> More details about why this broke an existing feature folks were using:
>>> Previously the DMA resources for platform devices were being populated through
>>> HWMOD, however with the recent clean ups with HWMOD, this data has been moved
>>> to Device tree. The EDMA code though is not aware of this so it fails to fetch
>>> the DMA resources correctly which it needs to prepare the unused channel list
>>> (basically doesn't properly clear the channels that are in use, in the unused
>>> list).
>>
>> So that we can learn for next time: What should we (as in us
>> maintainers and you TI) have done differently to avoid this?
> 
> I think a little on both sides can be improved.

Since we are in lessons learnt mode, I think as developers we need to
learn to prioritize fixes over other features we are working on. I went
back to the chronology of this patch series.

22nd July 2013: v2 posted
29th July 2013: Discussion on whether the patch can wait till *v3.12*
		merge window.
29th July 2013: comments given on v2
22nd Aug 2013: 	Pull request's sent by Sekhar for v3.12
24th Aug 2013: 	another v2 posted (all comments given earlier not
		addressed, received some comments on build warnings)
27th Aug 2013:	another v3 posted (all comments given on 29th July not
		addressed)
10th Sept 2013: another v3 posted (all comments given on 29th July not
		addressed)
[some discussion on comments and why this cannot wait until v3.13]
17th Sept 2013:	Final version ready for merge posted.
26th Sept 2013: Another v3 posted, this time for Olof to send into
		v3.12-rc

See, early on, the patch was actually in consideration for v3.12 merge.
The barrier of entry into -rc cycle is pretty high. So if you have an
opportunity to hit a merge window, utilize that by prioritizing this
work over anything else you may be doing.

I know you got busy with adding support for SG lists and all, but
clearly this patch is critical in your mind. Plus the comments were not
tough to fix. There is a need to keep looking at what provides the best
return on time invested.

Thanks,
Sekhar

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

[Joel Fernandes]

On 09/27/2013 02:49 AM, Sekhar Nori wrote:
> On 9/27/2013 5:58 AM, Joel Fernandes wrote:
>> On 09/26/2013 06:13 PM, Olof Johansson wrote:
>>> On Thu, Sep 26, 2013 at 2:55 PM, Joel Fernandes <joelf@ti.com> wrote:
>>>> HWMOD removal for MMC is breaking edma_start as the events are being manually
>>>> triggered due to unused channel list not being clear.
>>>>
>>>> The above issue is fixed by reading the "dmas" property from the DT node if it
>>>> exists and clearing the bits in the unused channel list if the dma controller
>>>> used by any device is EDMA. For this purpose we use the of_* helpers to parse
>>>> the arguments in the dmas phandle list.
>>>>
>>>> Also introduced is a minor clean up of a checkpatch error in old code.
>>>>
>>>> Reviewed-by: Sekhar Nori <nsekhar@ti.com>
>>>> Reported-by: Balaji T K <balajitk@ti.com>
>>>> Cc: Sekhar Nori <nsekhar@ti.com>
>>>> Cc: Tony Lindgren <tony@atomide.com>
>>>> Cc: Olof Johansson <olof@lixom.net>
>>>> Cc: Nishanth Menon <nm@ti.com>
>>>> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
>>>> Cc: Jason Kridner <jkridner@beagleboard.org>
>>>> Cc: Koen Kooi <koen@dominion.thruhere.net>
>>>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>>>> ---
>>>> Just resending this patch after discussing with Sekhar and Olof.
>>>
>>> Actually, the patch you talked to me about was v3 of this. It seems
>>> that you have reposted v6 but labelled it v3. This is very confusing.
>>
>> Sorry about this. :-( This is indeed v6.
>>
>>>> AM335x is being booted by many users such as the beaglebone community. DT is
>>>> the only boot method available for all these users.  EDMA is required for the
>>>> operation for many common peripherals in AM335x SoC such as McASP, MMC and
>>>> Crypto.
>>>>
>>>> Although EDMA DT nodes are going in only for 3.13, in reality the kernel has
>>>> been used for more than a year with EDMA code and out of tree EDMA DTS patches.
>>>> Hence though the DT nodes are still not in mainline, this patch can be still be
>>>> considered a critical fix as such and it would be great if it could be included
>>>> in 3.12-rc release. Thanks.
>>>
>>> This is really the root of this problem. If you sit on code out of
>>> tree for a year, and something breaks that we couldn't even have
>>> detected since we didn't have the out-of-tree pieces. We'll help you
>>> the first few times (such as now) but we will eventually stop caring.
>>
>> When I started looking at EDMA in June, I noticed that a lot had already been
>> merged. EDMA DMA Engine driver itself was merged last year, no worries there.
>> but the long pending list of fixes to be made to the driver had to written and
>> rewritten multiple times which took a long time.
>>
>> Due to this, the EDMA device tree entries could not be merged in fear that doing
>> so would cause problems such as MMC/SD corruption etc.
>>
>>> If I was in a worse mood, then I'd just say that since your users
>>> already has to have out-of-tree code to even use this functionality,
>>> they could just add this fix on top of that stack of patches. But I'm
>>> in a slightly better mood than that and I'll pick it up this time. :)
>>
>> Thank you! :)
>>
>>>> More details about why this broke an existing feature folks were using:
>>>> Previously the DMA resources for platform devices were being populated through
>>>> HWMOD, however with the recent clean ups with HWMOD, this data has been moved
>>>> to Device tree. The EDMA code though is not aware of this so it fails to fetch
>>>> the DMA resources correctly which it needs to prepare the unused channel list
>>>> (basically doesn't properly clear the channels that are in use, in the unused
>>>> list).
>>>
>>> So that we can learn for next time: What should we (as in us
>>> maintainers and you TI) have done differently to avoid this?
>>
>> I think a little on both sides can be improved.
>>
>> For TI, a bit more work toward explaining patches better in change logs so that
>> maintainers understand and are willing to help to get the code upstream would
>> help. A lot of improvement to internal policies have been made for developing on
>> upstream, and that's certainly a good thing but there's still more room forard
>> improvement.
>>
>> For maintainers,  EDMA code would have been kept breathing all these months
>> (atleast 8 months) if those temporary fixes mentioned above would have been
>> merged into the kernel instead of not applied. With those fixes in place, dts
>> could have been enabled and EDMA would be fully working all these months. This
>> would have certainly made things a lot easier. Rewriting stuff the right way is
>> OK but if it is a _lot_ of effort, then to keep things alive, merging stuff "for
>> now" specially if they are one-liners could be made acceptable.
> 
> Joel, can you give a link to the "one-liner" "temporary fix" that was
> was not merged? I am unable to put it in context.

Sure, not exactly a one-line but maybe couple of lines (see below). Also wasn't
referring to anything you're maintaining. I was just continuing the original
discussion of where we can improve as a community.

My point was very trivial changes that keep things working such as this one
should be merged in time to keep things working:
http://lkml.indiana.edu/hypermail/linux/kernel/1210.2/01476.html

That one patch was reworked several times, and then was decided to do the right
way and finally merged with the limit-less series. All I am saying is all this
while the above patch(es) could be merged to keep things alive, and that way we
could avoid a lot of the pain and frustration that folks felt.

Its just my opinion though, I don't feel strongly about it but I think this may
be a helpful approach that's all.

regards,

-Joel

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

[Joel Fernandes]

On 09/27/2013 04:04 AM, Sekhar Nori wrote:
> On 9/27/2013 5:58 AM, Joel Fernandes wrote:
>> On 09/26/2013 06:13 PM, Olof Johansson wrote:
>>> On Thu, Sep 26, 2013 at 2:55 PM, Joel Fernandes <joelf@ti.com> wrote:
>>>> HWMOD removal for MMC is breaking edma_start as the events are being manually
>>>> triggered due to unused channel list not being clear.
>>>>
>>>> The above issue is fixed by reading the "dmas" property from the DT node if it
>>>> exists and clearing the bits in the unused channel list if the dma controller
>>>> used by any device is EDMA. For this purpose we use the of_* helpers to parse
>>>> the arguments in the dmas phandle list.
>>>>
>>>> Also introduced is a minor clean up of a checkpatch error in old code.
>>>>
>>>> Reviewed-by: Sekhar Nori <nsekhar@ti.com>
>>>> Reported-by: Balaji T K <balajitk@ti.com>
>>>> Cc: Sekhar Nori <nsekhar@ti.com>
>>>> Cc: Tony Lindgren <tony@atomide.com>
>>>> Cc: Olof Johansson <olof@lixom.net>
>>>> Cc: Nishanth Menon <nm@ti.com>
>>>> Cc: Pantel Antoniou <panto@antoniou-consulting.com>
>>>> Cc: Jason Kridner <jkridner@beagleboard.org>
>>>> Cc: Koen Kooi <koen@dominion.thruhere.net>
>>>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>>>> ---
>>>> Just resending this patch after discussing with Sekhar and Olof.
>>>
>>> Actually, the patch you talked to me about was v3 of this. It seems
>>> that you have reposted v6 but labelled it v3. This is very confusing.
>>
>> Sorry about this. :-( This is indeed v6.
>>
>>>> AM335x is being booted by many users such as the beaglebone community. DT is
>>>> the only boot method available for all these users.  EDMA is required for the
>>>> operation for many common peripherals in AM335x SoC such as McASP, MMC and
>>>> Crypto.
>>>>
>>>> Although EDMA DT nodes are going in only for 3.13, in reality the kernel has
>>>> been used for more than a year with EDMA code and out of tree EDMA DTS patches.
>>>> Hence though the DT nodes are still not in mainline, this patch can be still be
>>>> considered a critical fix as such and it would be great if it could be included
>>>> in 3.12-rc release. Thanks.
>>>
>>> This is really the root of this problem. If you sit on code out of
>>> tree for a year, and something breaks that we couldn't even have
>>> detected since we didn't have the out-of-tree pieces. We'll help you
>>> the first few times (such as now) but we will eventually stop caring.
>>
>> When I started looking at EDMA in June, I noticed that a lot had already been
>> merged. EDMA DMA Engine driver itself was merged last year, no worries there.
>> but the long pending list of fixes to be made to the driver had to written and
>> rewritten multiple times which took a long time.
>>
>> Due to this, the EDMA device tree entries could not be merged in fear that doing
>> so would cause problems such as MMC/SD corruption etc.
>>
>>> If I was in a worse mood, then I'd just say that since your users
>>> already has to have out-of-tree code to even use this functionality,
>>> they could just add this fix on top of that stack of patches. But I'm
>>> in a slightly better mood than that and I'll pick it up this time. :)
>>
>> Thank you! :)
>>
>>>> More details about why this broke an existing feature folks were using:
>>>> Previously the DMA resources for platform devices were being populated through
>>>> HWMOD, however with the recent clean ups with HWMOD, this data has been moved
>>>> to Device tree. The EDMA code though is not aware of this so it fails to fetch
>>>> the DMA resources correctly which it needs to prepare the unused channel list
>>>> (basically doesn't properly clear the channels that are in use, in the unused
>>>> list).
>>>
>>> So that we can learn for next time: What should we (as in us
>>> maintainers and you TI) have done differently to avoid this?
>>
>> I think a little on both sides can be improved.
> 
> Since we are in lessons learnt mode, I think as developers we need to
> learn to prioritize fixes over other features we are working on. I went
> back to the chronology of this patch series.

Sure, I agree with this. Will definitely work on it.

> 
> 22nd July 2013: v2 posted
> 29th July 2013: Discussion on whether the patch can wait till *v3.12*
> 		merge window.
> 29th July 2013: comments given on v2
> 22nd Aug 2013: 	Pull request's sent by Sekhar for v3.12
> 24th Aug 2013: 	another v2 posted (all comments given earlier not
> 		addressed, received some comments on build warnings)
> 27th Aug 2013:	another v3 posted (all comments given on 29th July not
> 		addressed)
> 10th Sept 2013: another v3 posted (all comments given on 29th July not
> 		addressed)
> [some discussion on comments and why this cannot wait until v3.13]
> 17th Sept 2013:	Final version ready for merge posted.
> 26th Sept 2013: Another v3 posted, this time for Olof to send into
> 		v3.12-rc
> 
> See, early on, the patch was actually in consideration for v3.12 merge.
> The barrier of entry into -rc cycle is pretty high. So if you have an
> opportunity to hit a merge window, utilize that by prioritizing this
> work over anything else you may be doing.
> 
> I know you got busy with adding support for SG lists and all, but
> clearly this patch is critical in your mind. Plus the comments were not
> tough to fix. There is a need to keep looking at what provides the best
> return on time invested.


Yes, but we are not referring to this particular patch at all. Olof was asking
about the code that was not been merged and/or the reasons things are not
working. I was just responding to that. But thanks for the advice, will def keep
it in mind.

Regards,

-Joel