diff mbox

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

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

Commit Message

Joel Fernandes Sept. 26, 2013, 9:55 p.m. UTC
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(-)

Comments

Olof Johansson Sept. 26, 2013, 11:13 p.m. UTC | #1
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
Joel Fernandes Sept. 27, 2013, 12:28 a.m. UTC | #2
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
Sekhar Nori Sept. 27, 2013, 7:49 a.m. UTC | #3
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
Sekhar Nori Sept. 27, 2013, 9:04 a.m. UTC | #4
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
Joel Fernandes Sept. 27, 2013, 3:20 p.m. UTC | #5
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
Joel Fernandes Sept. 27, 2013, 3:25 p.m. UTC | #6
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
diff mbox

Patch

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",