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 <[email protected]>
Reported-by: Balaji T K <[email protected]>
Cc: Pantel Antoniou <[email protected]>
Signed-off-by: Joel Fernandes <[email protected]>
---
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
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 <[email protected]>
> Reported-by: Balaji T K <[email protected]>
> Cc: Pantel Antoniou <[email protected]>
> Signed-off-by: Joel Fernandes <[email protected]>
> ---
> 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
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 <[email protected]>
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 <[email protected]>
Reported-by: Balaji T K <[email protected]>
Cc: Pantel Antoniou <[email protected]>
Signed-off-by: Joel Fernandes <[email protected]>
---
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
Hi Joel,
On Saturday 14 September 2013 06:27 AM, Joel Fernandes wrote:
> From: Joel Fernandes <[email protected]>
> 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 <[email protected]>
> Reported-by: Balaji T K <[email protected]>
> Cc: Pantel Antoniou <[email protected]>
> Signed-off-by: Joel Fernandes <[email protected]>
> ---
> 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
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 <[email protected]>
>> 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 <[email protected]>
>> Reported-by: Balaji T K <[email protected]>
>> Cc: Pantel Antoniou <[email protected]>
>> Signed-off-by: Joel Fernandes <[email protected]>
>> ---
>> 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 <[email protected]>
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 <[email protected]>
Reported-by: Balaji T K <[email protected]>
Cc: Pantel Antoniou <[email protected]>
Signed-off-by: Joel Fernandes <[email protected]>
---
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
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 <[email protected]>
>>> 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 <[email protected]>
>>> Reported-by: Balaji T K <[email protected]>
>>> Cc: Pantel Antoniou <[email protected]>
>>> Signed-off-by: Joel Fernandes <[email protected]>
>>> ---
>>> 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
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
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
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 <[email protected]>
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 <[email protected]>
Reported-by: Balaji T K <[email protected]>
Cc: Pantel Antoniou <[email protected]>
Signed-off-by: Joel Fernandes <[email protected]>
---
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
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 <[email protected]>
> 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