[PATCH v2 03/16] ARM: dts: provide DMA config to pxamci

[PATCH v2 03/16] ARM: dts: provide DMA config to pxamci

[Sergei Ianovich]

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>
---
   v1..v2
   * add binding for next-gen dma controller
   * use correct dma declararion
   * number changed from 5 to 3

 Documentation/devicetree/bindings/mmc/pxa-mmc.txt |  5 ++
 arch/arm/boot/dts/pxa27x.dtsi                     | 14 ++++++
 drivers/mmc/host/pxamci.c                         | 59 ++++++++++++++++++-----
 3 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/pxa-mmc.txt b/Documentation/devicetree/bindings/mmc/pxa-mmc.txt
index b7025de..9f54c69 100644
--- a/Documentation/devicetree/bindings/mmc/pxa-mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/pxa-mmc.txt
@@ -5,6 +5,8 @@ Driver bindings for the PXA MCI (MMC/SDIO) interfaces
 Required properties:
 - compatible: Should be "marvell,pxa-mmc".
 - vmmc-supply: A regulator for VMMC
+- dmas: Should be DMA specifiers for RX and TX
+- dma-names: Should be "rx" and "tx"
 
 Optional properties:
 - marvell,detect-delay-ms: sets the detection delay timeout in ms.
@@ -21,5 +23,8 @@ mmc0: mmc@41100000 {
 	interrupts = <23>;
 	cd-gpios = <&gpio 23 0>;
 	wp-gpios = <&gpio 24 0>;
+	dmas = <&dma 21
+		&dma 22>;
+	dma-names = "rx", "tx";
 };
 
diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
index 44df554..c2a90c5 100644
--- a/arch/arm/boot/dts/pxa27x.dtsi
+++ b/arch/arm/boot/dts/pxa27x.dtsi
@@ -11,10 +11,24 @@
 			marvell,intc-nr-irqs = <34>;
 		};
 
+		dma: dma-controller@40000000 {
+			compatible = "marvell,pdma-1.0";
+			reg = <0x40000000 0x10000>;
+			interrupts = <25>;
+			#dma-cells = <1>;
+			dma-channels = <32>;
+		};
+
 		gpio: gpio@40e00000 {
 			compatible = "intel,pxa27x-gpio";
 			interrupts = <8>, <9>, <10>;
 			interrupt-names = "gpio0", "gpio1", "gpio_mux";
 		};
+
+		mmc@41100000 {
+			dmas = <&dma 21
+				&dma 22>;
+			dma-names = "rx", "tx";
+		};
 	};
 };
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 32fe113..6b67764 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -613,11 +613,46 @@ 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 i;
+	int ret;
+
+	i = of_property_match_string(np, "dma-names", "rx");
+	if (i < 0)
+		return i;
+
+	ret = of_property_read_u32_index(np, "dmas", 2 * i + 1, &tmp);
+	if (ret < 0)
+		return ret;
+	host->dma_drcmrrx = tmp;
+
+	i = of_property_match_string(np, "dma-names", "tx");
+	if (i < 0)
+		return i;
+
+	ret = of_property_read_u32_index(np, "dmas", 2 * i + 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 +776,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;
-- 
1.8.4.3

Re: [PATCH v2 03/16] ARM: dts: provide DMA config to pxamci

[Arnd Bergmann]

On Friday 13 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.

Please update this changeset comment, I think it still refers to
the earlier version.
 
> Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
> CC: Daniel Mack <zonque@gmail.com>
> CC: Arnd Bergmann <arnd@arndb.de>

The patch looks ok in case we are merging your patches for 3.14
and Daniel's patches later than that. If they end up in the
same merge window however, we'd have to be care to resolve
the obvious conflict in a proper way.

	Arnd

Re: [PATCH v2 03/16] ARM: dts: provide DMA config to pxamci

[Sergei Ianovich]

On Sat, 2013-12-14 at 20:06 +0100, Arnd Bergmann wrote:
> On Friday 13 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.
> 
> Please update this changeset comment, I think it still refers to
> the earlier version.

---
Non-dts implementation supply required DMA channel numbers as
IORESOURCE_DMA. We can also get them from the device tree, if it
is present.
---

Is this fine?

Should I post v3, if so? 

> > Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
> > CC: Daniel Mack <zonque@gmail.com>
> > CC: Arnd Bergmann <arnd@arndb.de>
> 
> The patch looks ok in case we are merging your patches for 3.14
> and Daniel's patches later than that. If they end up in the
> same merge window however, we'd have to be care to resolve
> the obvious conflict in a proper way.

The most recently published Daniel's patch (Aug 2013) wraps
IORESOURCE_DMA handling on DT presence in a similar way, but doesn't do
any actual DT processing. So there is no conflict at the moment. Daniel
can comment better on this, though.

Re: [PATCH v2 03/16] ARM: dts: provide DMA config to pxamci

[Arnd Bergmann]

On Saturday 14 December 2013, Sergei Ianovich wrote:
> On Sat, 2013-12-14 at 20:06 +0100, Arnd Bergmann wrote:
> > On Friday 13 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.
> > 
> > Please update this changeset comment, I think it still refers to
> > the earlier version.
> 
> ---
> Non-dts implementation supply required DMA channel numbers as
> IORESOURCE_DMA. We can also get them from the device tree, if it
> is present.
> ---
> 
> Is this fine?

I would mention that the binding is designed to work with the dmaengine
framework as soon as we have figured out how to make that work without
the problems you found.

> Should I post v3, if so? 

I'd wait a little longer for more comments.

	Arnd

Re: [PATCH v2 03/16] ARM: dts: provide DMA config to pxamci

[Daniel Mack]

On 12/14/2013 08:34 PM, Sergei Ianovich wrote:
> On Sat, 2013-12-14 at 20:06 +0100, Arnd Bergmann wrote:

>> The patch looks ok in case we are merging your patches for 3.14
>> and Daniel's patches later than that. If they end up in the
>> same merge window however, we'd have to be care to resolve
>> the obvious conflict in a proper way.
> 
> The most recently published Daniel's patch (Aug 2013) wraps
> IORESOURCE_DMA handling on DT presence in a similar way,

Erm, no it doesn't. My patch uses dma_request_slave_channel_compat() in
DT case, and that works fine with the current version of pdma, and
there's no need to read the "dmas" properties directly.

If you want to provide a way to simply denote the dma channel numbers,
without looking at the actual phandle, then yes, we could merge this
patch first, but it would be effectively reverted a proper implementation.


Daniel

Re: [PATCH v2 03/16] ARM: dts: provide DMA config to pxamci

[Sergei Ianovich]

On Mon, 2013-12-16 at 10:58 +0100, Daniel Mack wrote:
> On 12/14/2013 08:34 PM, Sergei Ianovich wrote:
> > On Sat, 2013-12-14 at 20:06 +0100, Arnd Bergmann wrote:
> 
> >> The patch looks ok in case we are merging your patches for 3.14
> >> and Daniel's patches later than that. If they end up in the
> >> same merge window however, we'd have to be care to resolve
> >> the obvious conflict in a proper way.
> > 
> > The most recently published Daniel's patch (Aug 2013) wraps
> > IORESOURCE_DMA handling on DT presence in a similar way,
> 
> Erm, no it doesn't. My patch uses dma_request_slave_channel_compat() in
> DT case, and that works fine with the current version of pdma, and
> there's no need to read the "dmas" properties directly.
> 
> If you want to provide a way to simply denote the dma channel numbers,
> without looking at the actual phandle, then yes, we could merge this
> patch first, but it would be effectively reverted a proper implementation.

Daniel is right. His patch doesn't need to read "dmas" directly. So my
patch won't need to change drivers/mmc/host/pxamci.c at all, if it is
applied after Daniel's one.

What are we going to do in this context?

Re: [PATCH v2 03/16] ARM: dts: provide DMA config to pxamci

[Lars-Peter Clausen]

On 12/16/2013 12:47 PM, Sergei Ianovich wrote:
> On Mon, 2013-12-16 at 10:58 +0100, Daniel Mack wrote:
>> On 12/14/2013 08:34 PM, Sergei Ianovich wrote:
>>> On Sat, 2013-12-14 at 20:06 +0100, Arnd Bergmann wrote:
>>
>>>> The patch looks ok in case we are merging your patches for 3.14
>>>> and Daniel's patches later than that. If they end up in the
>>>> same merge window however, we'd have to be care to resolve
>>>> the obvious conflict in a proper way.
>>>
>>> The most recently published Daniel's patch (Aug 2013) wraps
>>> IORESOURCE_DMA handling on DT presence in a similar way,
>>
>> Erm, no it doesn't. My patch uses dma_request_slave_channel_compat() in
>> DT case, and that works fine with the current version of pdma, and
>> there's no need to read the "dmas" properties directly.
>>
>> If you want to provide a way to simply denote the dma channel numbers,
>> without looking at the actual phandle, then yes, we could merge this
>> patch first, but it would be effectively reverted a proper implementation.
> 
> Daniel is right. His patch doesn't need to read "dmas" directly. So my
> patch won't need to change drivers/mmc/host/pxamci.c at all, if it is
> applied after Daniel's one.
> 

Btw. any driver that parses the dmas property manually should be considered
broken. This is a classical layering violation. The layout of the dma
specifier is DMA controller specific and should be completely transparent to
the device driver.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 03/16] ARM: dts: provide DMA config to pxamci

[Sergei Ianovich]

On Mon, 2013-12-16 at 12:58 +0100, Lars-Peter Clausen wrote:
> Btw. any driver that parses the dmas property manually should be considered
> broken. This is a classical layering violation. The layout of the dma
> specifier is DMA controller specific and should be completely transparent to
> the device driver.

PXA as a whole is broken in this respect at the moment. The problem is
how to move to a "fixed" state without breaking things which work now.

[PATCH v3 07/21] ARM: dts: parse DMA config in pxamci

[Sergei Ianovich]

The existing PXA MMC driver cannot get DMA channels in a proper
way from the respective device tree binding.

This patch provides temporary workaround which allows using the
existing driver in DT machines by pointing to the proper
dmaengine-based "marvell,pdma-1.0" DMA.

Even though the new DMA provider is not present we can parse device node
attributes manually and use channel numbers to acquire DMA channel
from the existing non-dmaengine provider.

When Daniel's DMA series is merged there will be no need to manually
parse for "dmas" and this patch can be safely reverted.

Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
CC: Daniel Mack <zonque@gmail.com>
CC: Arnd Bergmann <arnd@arndb.de>
---
   v2..v3
   * split into good (PATCH 07/21) and temporary (this one) parts

   v1..v2
   * add binding for next-gen dma controller
   * use correct dma declararion
   * number changed from 5 to 3

 drivers/mmc/host/pxamci.c | 59 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 32fe113..6b67764 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -613,11 +613,46 @@ 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 i;
+	int ret;
+
+	i = of_property_match_string(np, "dma-names", "rx");
+	if (i < 0)
+		return i;
+
+	ret = of_property_read_u32_index(np, "dmas", 2 * i + 1, &tmp);
+	if (ret < 0)
+		return ret;
+	host->dma_drcmrrx = tmp;
+
+	i = of_property_match_string(np, "dma-names", "tx");
+	if (i < 0)
+		return i;
+
+	ret = of_property_read_u32_index(np, "dmas", 2 * i + 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 +776,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;
-- 
1.8.4.2

Re: [PATCH v3 07/21] ARM: dts: parse DMA config in pxamci

[Sergei Ianovich]

Daniel,

On Tue, 2013-12-17 at 23:37 +0400, Sergei Ianovich wrote:
> The existing PXA MMC driver cannot get DMA channels in a proper
> way from the respective device tree binding.
> 
> This patch provides temporary workaround which allows using the
> existing driver in DT machines by pointing to the proper
> dmaengine-based "marvell,pdma-1.0" DMA.
> 
> Even though the new DMA provider is not present we can parse device node
> attributes manually and use channel numbers to acquire DMA channel
> from the existing non-dmaengine provider.
> 
> When Daniel's DMA series is merged there will be no need to manually
> parse for "dmas" and this patch can be safely reverted.
> 
> Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
> CC: Daniel Mack <zonque@gmail.com>
> CC: Arnd Bergmann <arnd@arndb.de>
> ---
>    v2..v3
>    * split into good (PATCH 07/21) and temporary (this one) parts
> 
>    v1..v2
>    * add binding for next-gen dma controller
>    * use correct dma declararion
>    * number changed from 5 to 3

Could you please comment on this solution for PXA DMA?

There may appear new, but it remains the last open review comment for
the series at the moment.