Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
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 jo...@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 nsek...@ti.com Reported-by: Balaji T K balaj...@ti.com Cc: Sekhar Nori nsek...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Olof Johansson o...@lixom.net Cc: Nishanth Menon n...@ti.com Cc: Pantel Antoniou pa...@antoniou-consulting.com Cc: Jason Kridner jkrid...@beagleboard.org Cc: Koen Kooi k...@dominion.thruhere.net Signed-off-by: Joel Fernandes jo...@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 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
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 jo...@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 nsek...@ti.com Reported-by: Balaji T K balaj...@ti.com Cc: Sekhar Nori nsek...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Olof Johansson o...@lixom.net Cc: Nishanth Menon n...@ti.com Cc: Pantel Antoniou pa...@antoniou-consulting.com Cc: Jason Kridner jkrid...@beagleboard.org Cc: Koen Kooi k...@dominion.thruhere.net Signed-off-by: Joel Fernandes jo...@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
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
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 jo...@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 nsek...@ti.com Reported-by: Balaji T K balaj...@ti.com Cc: Sekhar Nori nsek...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Olof Johansson o...@lixom.net Cc: Nishanth Menon n...@ti.com Cc: Pantel Antoniou pa...@antoniou-consulting.com Cc: Jason Kridner jkrid...@beagleboard.org Cc: Koen Kooi k...@dominion.thruhere.net Signed-off-by: Joel Fernandes jo...@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:
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
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 jo...@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 nsek...@ti.com Reported-by: Balaji T K balaj...@ti.com Cc: Sekhar Nori nsek...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Olof Johansson o...@lixom.net Cc: Nishanth Menon n...@ti.com Cc: Pantel Antoniou pa...@antoniou-consulting.com Cc: Jason Kridner jkrid...@beagleboard.org Cc: Koen Kooi k...@dominion.thruhere.net Signed-off-by: Joel Fernandes jo...@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
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
On Thu, Sep 26, 2013 at 2:55 PM, Joel Fernandes jo...@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 nsek...@ti.com Reported-by: Balaji T K balaj...@ti.com Cc: Sekhar Nori nsek...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Olof Johansson o...@lixom.net Cc: Nishanth Menon n...@ti.com Cc: Pantel Antoniou pa...@antoniou-consulting.com Cc: Jason Kridner jkrid...@beagleboard.org Cc: Koen Kooi k...@dominion.thruhere.net Signed-off-by: Joel Fernandes jo...@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 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
On 09/26/2013 06:13 PM, Olof Johansson wrote: On Thu, Sep 26, 2013 at 2:55 PM, Joel Fernandes jo...@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 nsek...@ti.com Reported-by: Balaji T K balaj...@ti.com Cc: Sekhar Nori nsek...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Olof Johansson o...@lixom.net Cc: Nishanth Menon n...@ti.com Cc: Pantel Antoniou pa...@antoniou-consulting.com Cc: Jason Kridner jkrid...@beagleboard.org Cc: Koen Kooi k...@dominion.thruhere.net Signed-off-by: Joel Fernandes jo...@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 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
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 jo...@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 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
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 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
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 jo...@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 nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Signed-off-by: Joel Fernandes jo...@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
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
Hi Joel,
On Saturday 14 September 2013 06:27 AM, Joel Fernandes wrote:
From: Joel Fernandes jo...@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 nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Signed-off-by: Joel Fernandes jo...@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
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
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 jo...@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 nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Signed-off-by: Joel Fernandes jo...@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
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
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 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
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 jo...@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 nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Signed-off-by: Joel Fernandes jo...@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
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
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 nsek...@ti.com Reported-by: Balaji T K balaj...@ti.com Cc: Pantel Antoniou pa...@antoniou-consulting.com Signed-off-by: Joel Fernandes jo...@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 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
