| Message ID | 1386543229-1542-6-git-send-email-ynvich@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sunday 08 December 2013, Sergei Ianovich wrote: > Non-dts implementation supply required DMA channel numbers as > IORESOURCE_DMA. However, there is was no way to get them from > device tree. I just wrote a lengthy reply to this email to explain in what ways you got it wrong, but then I saw that Daniel has already done all the work in the right way in August, so nevermind that. It hasn't made it upstream yet, but see http://list-archives.org/2013/08/07/linux-mtd-lists-infradead-org/patch-00-20-arm-pxa-move-core-and-drivers-to-dmaengine/f/3444199144 for how it's done. Maybe Daniel can comment on the status of his patches. Arnd
On 12/09/2013 02:33 AM, Arnd Bergmann wrote: > On Sunday 08 December 2013, Sergei Ianovich wrote: >> Non-dts implementation supply required DMA channel numbers as >> IORESOURCE_DMA. However, there is was no way to get them from >> device tree. > > I just wrote a lengthy reply to this email to explain in what ways > you got it wrong, but then I saw that Daniel has already done all > the work in the right way in August, so nevermind that. > > It hasn't made it upstream yet, but see http://list-archives.org/2013/08/07/linux-mtd-lists-infradead-org/patch-00-20-arm-pxa-move-core-and-drivers-to-dmaengine/f/3444199144 > for how it's done. Maybe Daniel can comment on the status of his > patches. I recently rebased all my patches on top of 3.13-rc3, and will resend in couple of days. The real problem here is that by reworking the DMA related PXA bits, all drivers have to be changed at once, and a gradual transition seems impossible. For that, I was asking for help in areas where I don't have any hardware to test my patches on, but unfortunately, nobody got back to me yet. In particular, the pxa camera driver is something Robert (cc) wanted to have a look at. Robert, any updates on this? Anyway, I'll have to clean up some details and will resend my patches very soon. Thanks, Daniel
On Mon, 2013-12-09 at 10:04 +0100, Daniel Mack wrote: > On 12/09/2013 02:33 AM, Arnd Bergmann wrote: > > On Sunday 08 December 2013, Sergei Ianovich wrote: > >> Non-dts implementation supply required DMA channel numbers as > >> IORESOURCE_DMA. However, there is was no way to get them from > >> device tree. > > > > I just wrote a lengthy reply to this email to explain in what ways > > you got it wrong, but then I saw that Daniel has already done all > > the work in the right way in August, so nevermind that. > > > > It hasn't made it upstream yet, but see http://list-archives.org/2013/08/07/linux-mtd-lists-infradead-org/patch-00-20-arm-pxa-move-core-and-drivers-to-dmaengine/f/3444199144 > > for how it's done. Maybe Daniel can comment on the status of his > > patches. > > I recently rebased all my patches on top of 3.13-rc3, and will resend in > couple of days. > > The real problem here is that by reworking the DMA related PXA bits, all > drivers have to be changed at once, and a gradual transition seems > impossible. For that, I was asking for help in areas where I don't have > any hardware to test my patches on, but unfortunately, nobody got back > to me yet. Nice to have Daniel in this conversation. Your patch series is a big and important work. However, I am not sure I will ever land as is exactly for this reason. DMA is a per-physical-device concept, not per-platform. Only the DMA controller is per-platform. This means we need to rewrite all platform drivers at once. This is roughly equivalent to adding a new platform support in one patch, which is not going to work. In patch 08/20 in the series above, you change 10% lines of the pxa mmc driver. If we don't count boilerplate lines, this will amount to over 50%. In other word, the output is a new driver. My proposal in to actually add new drivers for each platform device with DMA and mark new ones EXPERIMENTAL. When they receive adequate testing we remove the tag and mark the old one OBSOLETE. When we have a new driver for every device presently supported, we will drop the whole old series. This should be very close to a gradual transition. While we are in transition, I propose that we allow 'hackish' support for device tree in the existing drivers. Platform devices are declared in dtsi files, so when we move to a new driver we just change the compatible string there. Actual devices with their one dts file won't need any change. They will need a dtb recompile, though. This way we can begin a migration to device tree in pxa immediately. The patch series does just that.
On Mon, 2013-12-09 at 13:34 +0400, Sergei Ianovich wrote: > On Mon, 2013-12-09 at 10:04 +0100, Daniel Mack wrote: > > On 12/09/2013 02:33 AM, Arnd Bergmann wrote: > > > On Sunday 08 December 2013, Sergei Ianovich wrote: > > >> Non-dts implementation supply required DMA channel numbers as > > >> IORESOURCE_DMA. However, there is was no way to get them from > > >> device tree. > > > > > > I just wrote a lengthy reply to this email to explain in what ways > > > you got it wrong, but then I saw that Daniel has already done all > > > the work in the right way in August, so nevermind that. > > > > > > It hasn't made it upstream yet, but see http://list-archives.org/2013/08/07/linux-mtd-lists-infradead-org/patch-00-20-arm-pxa-move-core-and-drivers-to-dmaengine/f/3444199144 > > > for how it's done. Maybe Daniel can comment on the status of his > > > patches. > > > > I recently rebased all my patches on top of 3.13-rc3, and will resend in > > couple of days. > > > > The real problem here is that by reworking the DMA related PXA bits, all > > drivers have to be changed at once, and a gradual transition seems > > impossible. For that, I was asking for help in areas where I don't have > > any hardware to test my patches on, but unfortunately, nobody got back > > to me yet. > > Nice to have Daniel in this conversation. Your patch series is a big and > important work. However, I am not sure I will ever land as is exactly > for this reason. s/I will/it will/ > DMA is a per-physical-device concept, not per-platform. Only the DMA > controller is per-platform. This means we need to rewrite all platform > drivers at once. This is roughly equivalent to adding a new platform > support in one patch, which is not going to work. > > In patch 08/20 in the series above, you change 10% lines of the pxa mmc > driver. If we don't count boilerplate lines, this will amount to over > 50%. In other word, the output is a new driver. s/other word/other words/ > My proposal in to actually add new drivers for each platform device with > DMA and mark new ones EXPERIMENTAL. When they receive adequate testing > we remove the tag and mark the old one OBSOLETE. When we have a new > driver for every device presently supported, we will drop the whole old > series. s/proposal in/proposal is/ > This should be very close to a gradual transition. > > While we are in transition, I propose that we allow 'hackish' support > for device tree in the existing drivers. Platform devices are declared > in dtsi files, so when we move to a new driver we just change the > compatible string there. Actual devices with their one dts file won't > need any change. They will need a dtb recompile, though. This way we can > begin a migration to device tree in pxa immediately. s/their one/their own/ > The patch series does just that. Sorry for noise.
On 12/09/2013 10:34 AM, Sergei Ianovich wrote: > On Mon, 2013-12-09 at 10:04 +0100, Daniel Mack wrote: >> On 12/09/2013 02:33 AM, Arnd Bergmann wrote: >>> On Sunday 08 December 2013, Sergei Ianovich wrote: >>>> Non-dts implementation supply required DMA channel numbers as >>>> IORESOURCE_DMA. However, there is was no way to get them from >>>> device tree. >>> >>> I just wrote a lengthy reply to this email to explain in what ways >>> you got it wrong, but then I saw that Daniel has already done all >>> the work in the right way in August, so nevermind that. >>> >>> It hasn't made it upstream yet, but see http://list-archives.org/2013/08/07/linux-mtd-lists-infradead-org/patch-00-20-arm-pxa-move-core-and-drivers-to-dmaengine/f/3444199144 >>> for how it's done. Maybe Daniel can comment on the status of his >>> patches. >> >> I recently rebased all my patches on top of 3.13-rc3, and will resend in >> couple of days. >> >> The real problem here is that by reworking the DMA related PXA bits, all >> drivers have to be changed at once, and a gradual transition seems >> impossible. For that, I was asking for help in areas where I don't have >> any hardware to test my patches on, but unfortunately, nobody got back >> to me yet. > > Nice to have Daniel in this conversation. Your patch series is a big and > important work. However, I am not sure I will ever land as is exactly > for this reason. Well, I wouldn't be so certain about that statement. As I wrote in the cover letter, most of the work is actually done, and I successfully tested the new DMA support with a some of the drivers I ported. Others were ported blindly, and in case of no reaction, I'd dare to merge them and wait for people to report back in case of trouble. The only real problem is the PXA camera driver, which does tricky things like hot re-queuing of DMA descriptors. That one needs fixing before the series can land. > My proposal in to actually add new drivers for each platform device with > DMA and mark new ones EXPERIMENTAL. That would cause tree-wide cross-dependencies between drivers, because the two DMA controllers can't be used at the same time, and the PXA specific API will be unavailable when the mmp-dma driver is selected. My patch series (and the DMA controller framework for that matter) aims for the opposite - the unification of APIs and drivers. > When they receive adequate testing > we remove the tag and mark the old one OBSOLETE. When we have a new > driver for every device presently supported, we will drop the whole old > series. Well, given the feedback I got for the series so far, I fear we'll be off to maintain two drivers for each peripheral for a very long time. I'd rather propose cracking the last nut that's left, and then get the thing merged. Arnd, Haojian? Any opinion here? Daniel
On Mon, 2013-12-09 at 11:21 +0100, Daniel Mack wrote: > On 12/09/2013 10:34 AM, Sergei Ianovich wrote: > > Nice to have Daniel in this conversation. Your patch series is a big and > > important work. However, I am not sure I will ever land as is exactly > > for this reason. > > Well, I wouldn't be so certain about that statement. As I wrote in the > cover letter, most of the work is actually done, and I successfully > tested the new DMA support with a some of the drivers I ported. Others > were ported blindly, and in case of no reaction, I'd dare to merge them > and wait for people to report back in case of trouble. If breaking things is an option, I am definitely wrong. I assumed the opposite. > > My proposal in to actually add new drivers for each platform device with > > DMA and mark new ones EXPERIMENTAL. > > That would cause tree-wide cross-dependencies between drivers, because > the two DMA controllers can't be used at the same time, and the PXA > specific API will be unavailable when the mmp-dma driver is selected. My > patch series (and the DMA controller framework for that matter) aims for > the opposite - the unification of APIs and drivers. Not sure I got this point. My proposal is to keep the existing DMA intact until we are ready to remove it. I understand this approach requires considerably more work inside DMA to allow both driver to coexist than wholesale replacement. I still think big change is risky.
On Mon, 2013-12-09 at 11:21 +0100, Daniel Mack wrote: > On 12/09/2013 10:34 AM, Sergei Ianovich wrote: > > On Mon, 2013-12-09 at 10:04 +0100, Daniel Mack wrote: > >> On 12/09/2013 02:33 AM, Arnd Bergmann wrote: > >>> It hasn't made it upstream yet, but see http://list-archives.org/2013/08/07/linux-mtd-lists-infradead-org/patch-00-20-arm-pxa-move-core-and-drivers-to-dmaengine/f/3444199144 > >>> for how it's done. Maybe Daniel can comment on the status of his > >>> patches. > >> ... > The only real problem is the PXA camera driver, which does tricky things > like hot re-queuing of DMA descriptors. That one needs fixing before the > series can land. My impression is that his series is hard to land. I've expressed my concerns about big changes in a separate mail. PXA device tree support issue is practically orthogonal. As of August 2013, Daniel's dma series doesn't add any dt support. After new DMA is working, we will need to add dt support in the same drivers. Basically we have three options: A. Wait for Daniel DMA, than for PXA clock, than do dt support pros: * correct order cons: * need to wait, possibly a long time * dt boot not possible, while we wait B. Provide 'hackish' support to dt, than correct hacks when DMA is merged. pros: * parallel development * healthier kernel code (current PXA usb driver won't compile with dt enabled) cons: * wrong concept (like my patch 5/9) until DMA is working * need to recompile dtb files when kernel changes C. Use 'fake' DMA provider dt binding to emulate existing DMA until new DMA is merged, than pros: * parallel development * healthier kernel code * correct device trees from the beginning cons: * more work * still need to recompile dtbs when clock support is merged Any ideas?
On Monday 09 December 2013, Sergei Ianovich wrote: > On Mon, 2013-12-09 at 11:21 +0100, Daniel Mack wrote: > > On 12/09/2013 10:34 AM, Sergei Ianovich wrote: > > > Nice to have Daniel in this conversation. Your patch series is a big and > > > important work. However, I am not sure I will ever land as is exactly > > > for this reason. > > > > Well, I wouldn't be so certain about that statement. As I wrote in the > > cover letter, most of the work is actually done, and I successfully > > tested the new DMA support with a some of the drivers I ported. Others > > were ported blindly, and in case of no reaction, I'd dare to merge them > > and wait for people to report back in case of trouble. > > If breaking things is an option, I am definitely wrong. I assumed the > opposite. We never intentionally break things that people are using, but there are cases where doing blind changes is the best way forward. We should always try to find people to test patches on real hardware if possible, which is only possible if there are people around that have the hardware and that are going to run new kernels on them. In case of PXA dmaengine support, I think the benefits of applying the series far outweigh the breakage potential. A lot of the drivers are shared with MMP, which is going to be DT-only eventually and will require the new dmaengine code. Some PXA drivers may be shared with SA1100, which has also converted to dmaengine although there are no plans to ever support DT on sa1100. > > > My proposal in to actually add new drivers for each platform device with > > > DMA and mark new ones EXPERIMENTAL. > > > > That would cause tree-wide cross-dependencies between drivers, because > > the two DMA controllers can't be used at the same time, and the PXA > > specific API will be unavailable when the mmp-dma driver is selected. My > > patch series (and the DMA controller framework for that matter) aims for > > the opposite - the unification of APIs and drivers. > > Not sure I got this point. My proposal is to keep the existing DMA > intact until we are ready to remove it. I understand this approach > requires considerably more work inside DMA to allow both driver to > coexist than wholesale replacement. I still think big change is risky. We certainly need to be extra careful if we want to move things over at once and cannot test all the drivers. I'd be happier if it could be done gradually by having some drivers use the new interface and other drivers use the old one, but I don't see how that can be done here. Arnd
On Tue, 2013-12-10 at 01:25 +0100, Arnd Bergmann wrote: > We never intentionally break things that people are using, but there are > cases where doing blind changes is the best way forward. We should > always try to find people to test patches on real hardware if possible, > which is only possible if there are people around that have the hardware > and that are going to run new kernels on them. > > In case of PXA dmaengine support, I think the benefits of applying the > series far outweigh the breakage potential. A lot of the drivers are > shared with MMP, which is going to be DT-only eventually and will require > the new dmaengine code. Some PXA drivers may be shared with SA1100, > which has also converted to dmaengine although there are no plans to > ever support DT on sa1100. > > We certainly need to be extra careful if we want to move things over > at once and cannot test all the drivers. I'd be happier if it could > be done gradually by having some drivers use the new interface and > other drivers use the old one, but I don't see how that can be done > here. Thanks for detailed explanation. I see there is strong support for the big change. Lets hope it'll be smooth. I'll strest test Daniel's changes and report the results.
diff --git a/Documentation/devicetree/bindings/mmc/pxa-mmc.txt b/Documentation/devicetree/bindings/mmc/pxa-mmc.txt index b7025de..27447ec 100644 --- a/Documentation/devicetree/bindings/mmc/pxa-mmc.txt +++ b/Documentation/devicetree/bindings/mmc/pxa-mmc.txt @@ -5,6 +5,7 @@ Driver bindings for the PXA MCI (MMC/SDIO) interfaces Required properties: - compatible: Should be "marvell,pxa-mmc". - vmmc-supply: A regulator for VMMC +- marvell,dma-channels: 2 PXA DMA channels [0] for RX, [1] for TX Optional properties: - marvell,detect-delay-ms: sets the detection delay timeout in ms. @@ -19,6 +20,7 @@ mmc0: mmc@41100000 { compatible = "marvell,pxa-mmc"; reg = <0x41100000 0x1000>; interrupts = <23>; + marvell,dma-channels = <21 22>; cd-gpios = <&gpio 23 0>; wp-gpios = <&gpio 24 0>; }; diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi index 44df554..3f97520 100644 --- a/arch/arm/boot/dts/pxa27x.dtsi +++ b/arch/arm/boot/dts/pxa27x.dtsi @@ -16,5 +16,9 @@ interrupts = <8>, <9>, <10>; interrupt-names = "gpio0", "gpio1", "gpio_mux"; }; + + mmc@41100000 { + marvell,dma-channels = <21 22>; + }; }; }; diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c index 32fe113..8c7a110 100644 --- a/drivers/mmc/host/pxamci.c +++ b/drivers/mmc/host/pxamci.c @@ -613,11 +613,37 @@ static int pxamci_of_init(struct platform_device *pdev) return 0; } + +static int pxamci_of_init_dma(struct platform_device *pdev, + struct pxamci_host *host) +{ + struct device_node *np = pdev->dev.of_node; + u32 tmp; + int ret; + + ret = of_property_read_u32_index(np, "marvell,dma-channels", 0, &tmp); + if (ret < 0) + return ret; + host->dma_drcmrrx = tmp; + + ret = of_property_read_u32_index(np, "marvell,dma-channels", 1, &tmp); + if (ret < 0) + return ret; + host->dma_drcmrtx = tmp; + + return 0; +} #else static int pxamci_of_init(struct platform_device *pdev) { return 0; } + +static int pxamci_of_init_dma(struct platform_device *pdev, + struct pxamci_host *host) +{ + return -ENODATA; +} #endif static int pxamci_probe(struct platform_device *pdev) @@ -741,19 +767,21 @@ static int pxamci_probe(struct platform_device *pdev) platform_set_drvdata(pdev, mmc); - dmarx = platform_get_resource(pdev, IORESOURCE_DMA, 0); - if (!dmarx) { - ret = -ENXIO; - goto out; - } - host->dma_drcmrrx = dmarx->start; + if (pxamci_of_init_dma(pdev, host) < 0) { + dmarx = platform_get_resource(pdev, IORESOURCE_DMA, 0); + if (!dmarx) { + ret = -ENXIO; + goto out; + } + host->dma_drcmrrx = dmarx->start; - dmatx = platform_get_resource(pdev, IORESOURCE_DMA, 1); - if (!dmatx) { - ret = -ENXIO; - goto out; + dmatx = platform_get_resource(pdev, IORESOURCE_DMA, 1); + if (!dmatx) { + ret = -ENXIO; + goto out; + } + host->dma_drcmrtx = dmatx->start; } - host->dma_drcmrtx = dmatx->start; if (host->pdata) { gpio_cd = host->pdata->gpio_card_detect;
Non-dts implementation supply required DMA channel numbers as IORESOURCE_DMA. However, there is was no way to get them from device tree. Signed-off-by: Sergei Ianovich <ynvich@gmail.com> CC: Daniel Mack <zonque@gmail.com> CC: Arnd Bergmann <arnd@arndb.de> --- Documentation/devicetree/bindings/mmc/pxa-mmc.txt | 2 + arch/arm/boot/dts/pxa27x.dtsi | 4 ++ drivers/mmc/host/pxamci.c | 50 ++++++++++++++++++----- 3 files changed, 45 insertions(+), 11 deletions(-)