[PATCH] dma: imx-dma: Add oftree support

[PATCH] dma: imx-dma: Add oftree support

[Markus Pargmann]

Adding devicetree support for imx-dma driver. Use driver name for
function 'imx_dma_is_general_purpose' because the devicename for
devicetree initialized devices is different.

Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 .../devicetree/bindings/dma/fsl-imx-dma.txt        | 13 ++++++++++++
 drivers/dma/imx-dma.c                              | 23 ++++++++++++++++++++++
 include/linux/platform_data/dma-imx.h              |  3 ++-
 3 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/dma/fsl-imx-dma.txt

diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-dma.txt b/Documentation/devicetree/bindings/dma/fsl-imx-dma.txt
new file mode 100644
index 0000000..9a1df37
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/fsl-imx-dma.txt
@@ -0,0 +1,13 @@
+* Freescale Direct Memory Access (DMA) Controller for i.MX
+
+Required properties:
+- compatible : Should be "fsl,<chip>-dma". chip can be imx1, imx21 or imx27
+- reg : Should contain DMA registers location and length
+- interrupts : First item should be DMA interrupt, second one is optional and
+  should contain DMA Error interrupt
+
+dma: dma@10001000 {
+	compatible = "fsl,imx27-dma";
+	reg = <0x10001000 0x1000>;
+	interrupts = <32 33>;
+};
diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index a7dcf78..73eb59c 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -26,6 +26,7 @@
 #include <linux/clk.h>
 #include <linux/dmaengine.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 
 #include <asm/irq.h>
 #include <linux/platform_data/dma-imx.h>
@@ -201,6 +202,22 @@ static struct platform_device_id imx_dma_devtype[] = {
 };
 MODULE_DEVICE_TABLE(platform, imx_dma_devtype);
 
+static const struct of_device_id imx_dma_of_dev_id[] = {
+	{
+		.compatible = "fsl,imx1-dma",
+		.data = &imx_dma_devtype[IMX1_DMA],
+	}, {
+		.compatible = "fsl,imx21-dma",
+		.data = &imx_dma_devtype[IMX21_DMA],
+	}, {
+		.compatible = "fsl,imx27-dma",
+		.data = &imx_dma_devtype[IMX27_DMA],
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, imx_dma_of_dev_id);
+
 static inline int is_imx1_dma(struct imxdma_engine *imxdma)
 {
 	return imxdma->devtype == IMX1_DMA;
@@ -1000,9 +1017,14 @@ static int __init imxdma_probe(struct platform_device *pdev)
 	{
 	struct imxdma_engine *imxdma;
 	struct resource *res;
+	const struct of_device_id *of_id;
 	int ret, i;
 	int irq, irq_err;
 
+	of_id = of_match_device(imx_dma_of_dev_id, &pdev->dev);
+	if (of_id)
+		pdev->id_entry = of_id->data;
+
 	imxdma = devm_kzalloc(&pdev->dev, sizeof(*imxdma), GFP_KERNEL);
 	if (!imxdma)
 		return -ENOMEM;
@@ -1159,6 +1181,7 @@ static int __exit imxdma_remove(struct platform_device *pdev)
 static struct platform_driver imxdma_driver = {
 	.driver		= {
 		.name	= "imx-dma",
+		.of_match_table = imx_dma_of_dev_id,
 	},
 	.id_table	= imx_dma_devtype,
 	.remove		= __exit_p(imxdma_remove),
diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
index f6d30cc..2ce4aba 100644
--- a/include/linux/platform_data/dma-imx.h
+++ b/include/linux/platform_data/dma-imx.h
@@ -63,7 +63,8 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
 	return strstr(dev_name(chan->device->dev), "sdma") ||
 		!strcmp(dev_name(chan->device->dev), "imx1-dma") ||
 		!strcmp(dev_name(chan->device->dev), "imx21-dma") ||
-		!strcmp(dev_name(chan->device->dev), "imx27-dma");
+		!strcmp(dev_name(chan->device->dev), "imx27-dma") ||
+		!strcmp(chan->device->dev->driver->name, "imx-dma");
 }
 
 #endif
-- 
1.8.1.2

Re: [PATCH] dma: imx-dma: Add oftree support

[Arnd Bergmann]

On Friday 22 February 2013, Markus Pargmann wrote:
> Adding devicetree support for imx-dma driver. Use driver name for
> function 'imx_dma_is_general_purpose' because the devicename for
> devicetree initialized devices is different.
> 
> Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Hi Markus,

Please make sure you are following the generic dma binding from
Documentation/devicetree/bindings/dma/dma.txt that is getting added
in Linux-3.9.

> +++ b/Documentation/devicetree/bindings/dma/fsl-imx-dma.txt
> @@ -0,0 +1,13 @@
> +* Freescale Direct Memory Access (DMA) Controller for i.MX
> +
> +Required properties:
> +- compatible : Should be "fsl,<chip>-dma". chip can be imx1, imx21 or imx27
> +- reg : Should contain DMA registers location and length
> +- interrupts : First item should be DMA interrupt, second one is optional and
> +  should contain DMA Error interrupt
> +
> +dma: dma@10001000 {
> +       compatible = "fsl,imx27-dma";
> +       reg = <0x10001000 0x1000>;
> +       interrupts = <32 33>;
> +};

In particular, the "#dma-cells" property is required here, along with
dma-channels and dma-requests. You also need to describe the format
of the "dmas" property in slave drivers referring to this node.
In a lot of cases, you only need a single cell there, which is the
request line number.

	Arnd

Re: [PATCH] dma: imx-dma: Add oftree support

[Markus Pargmann]

Hi Arnd,

On Fri, Feb 22, 2013 at 02:46:06PM +0000, Arnd Bergmann wrote:
> On Friday 22 February 2013, Markus Pargmann wrote:
> > Adding devicetree support for imx-dma driver. Use driver name for
> > function 'imx_dma_is_general_purpose' because the devicename for
> > devicetree initialized devices is different.
> > 
> > Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 
> Hi Markus,
> 
> Please make sure you are following the generic dma binding from
> Documentation/devicetree/bindings/dma/dma.txt that is getting added
> in Linux-3.9.
> 
> > +++ b/Documentation/devicetree/bindings/dma/fsl-imx-dma.txt
> > @@ -0,0 +1,13 @@
> > +* Freescale Direct Memory Access (DMA) Controller for i.MX
> > +
> > +Required properties:
> > +- compatible : Should be "fsl,<chip>-dma". chip can be imx1, imx21 or imx27
> > +- reg : Should contain DMA registers location and length
> > +- interrupts : First item should be DMA interrupt, second one is optional and
> > +  should contain DMA Error interrupt
> > +
> > +dma: dma@10001000 {
> > +       compatible = "fsl,imx27-dma";
> > +       reg = <0x10001000 0x1000>;
> > +       interrupts = <32 33>;
> > +};
> 
> In particular, the "#dma-cells" property is required here, along with
> dma-channels and dma-requests. You also need to describe the format
> of the "dmas" property in slave drivers referring to this node.
> In a lot of cases, you only need a single cell there, which is the
> request line number.
> 
> 	Arnd

Thanks, I changed the patch to use the generic DMA bindings, see V2.
However I couldn't find any information about the number of supported
DMA requests.

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH V2] dma: imx-dma: Add oftree support

[Markus Pargmann]

Adding devicetree support for imx-dma driver. Use driver name for
function 'imx_dma_is_general_purpose' because the devicename for
devicetree initialized devices is different.

Changes in V2:
- Change the driver to use generic DMA DT bindings.
- Add a imx-dma filter function that sets the dma request line in
  private data.

Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
 .../devicetree/bindings/dma/fsl-imx-dma.txt        | 48 ++++++++++++++++
 drivers/dma/imx-dma.c                              | 64 +++++++++++++++++++++-
 include/linux/platform_data/dma-imx.h              |  6 +-
 3 files changed, 116 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/fsl-imx-dma.txt

diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-dma.txt b/Documentation/devicetree/bindings/dma/fsl-imx-dma.txt
new file mode 100644
index 0000000..04d8e40
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/fsl-imx-dma.txt
@@ -0,0 +1,48 @@
+* Freescale Direct Memory Access (DMA) Controller for i.MX
+
+This document will only describe differences to the generic DMA Controller and
+DMA request bindings.
+
+* DMA controller
+
+Required properties:
+- compatible : Should be "fsl,<chip>-dma". chip can be imx1, imx21 or imx27
+- reg : Should contain DMA registers location and length
+- interrupts : First item should be DMA interrupt, second one is optional and
+    should contain DMA Error interrupt
+- #dma-cells : Has to be 1. imx-dma does not support anything else.
+
+Optional properties:
+- #dma-channels : Number of DMA channels supported. Should be 16.
+- #dma-requests : Number of DMA requests supported.
+
+Example:
+
+	dma: dma@10001000 {
+		compatible = "fsl,imx27-dma";
+		reg = <0x10001000 0x1000>;
+		interrupts = <32 33>;
+		#dma-cells = <1>;
+		#dma-channels = <16>;
+	};
+
+
+* DMA client
+
+Clients have to specify the DMA requests with phandles in a list.
+
+Required properties:
+- dmas: List of one or more DMA request specifiers. One DMA request specifier
+    consists of a phandle to the DMA controller followed by the integer
+    specifiying the request line.
+- dma-names: List of string identifiers for the DMA requests. For the correct
+    names, have a look at the specific client driver.
+
+Example:
+
+	sdhci1: sdhci@10013000 {
+		...
+		dmas = <&dma 7>;
+		dma-names = "rx-tx";
+		...
+	};
diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index a7dcf78..58cef09 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -26,6 +26,8 @@
 #include <linux/clk.h>
 #include <linux/dmaengine.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_dma.h>
 
 #include <asm/irq.h>
 #include <linux/platform_data/dma-imx.h>
@@ -183,6 +185,7 @@ struct imxdma_engine {
 	struct imx_dma_2d_config	slots_2d[IMX_DMA_2D_SLOTS];
 	struct imxdma_channel		channel[IMX_DMA_CHANNELS];
 	enum imx_dma_type		devtype;
+	struct of_dma_filter_info	of_filter_info;
 };
 
 static struct platform_device_id imx_dma_devtype[] = {
@@ -201,6 +204,22 @@ static struct platform_device_id imx_dma_devtype[] = {
 };
 MODULE_DEVICE_TABLE(platform, imx_dma_devtype);
 
+static const struct of_device_id imx_dma_of_dev_id[] = {
+	{
+		.compatible = "fsl,imx1-dma",
+		.data = &imx_dma_devtype[IMX1_DMA],
+	}, {
+		.compatible = "fsl,imx21-dma",
+		.data = &imx_dma_devtype[IMX21_DMA],
+	}, {
+		.compatible = "fsl,imx27-dma",
+		.data = &imx_dma_devtype[IMX27_DMA],
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, imx_dma_of_dev_id);
+
 static inline int is_imx1_dma(struct imxdma_engine *imxdma)
 {
 	return imxdma->devtype == IMX1_DMA;
@@ -734,8 +753,13 @@ static int imxdma_alloc_chan_resources(struct dma_chan *chan)
 	struct imxdma_channel *imxdmac = to_imxdma_chan(chan);
 	struct imx_dma_data *data = chan->private;
 
-	if (data != NULL)
+	if (data != NULL) {
 		imxdmac->dma_request = data->dma_request;
+		if (data->alloc_ctl_filter) {
+			kfree(data);
+			chan->private = 0;
+		}
+	}
 
 	while (imxdmac->descs_allocated < IMXDMA_MAX_CHAN_DESCRIPTORS) {
 		struct imxdma_desc *desc;
@@ -996,13 +1020,33 @@ static void imxdma_issue_pending(struct dma_chan *chan)
 	spin_unlock_irqrestore(&imxdma->lock, flags);
 }
 
+bool imxdma_filter_fn(struct dma_chan *chan, void *param)
+{
+	struct imx_dma_data *data = kzalloc(sizeof(*data), GFP_KERNEL);
+	printk("%s\n", __func__);
+
+	if (!data)
+		return false;
+
+	data->dma_request = *(unsigned *) param;
+	data->alloc_ctl_filter = true;
+	chan->private = data;
+
+	return true;
+}
+
 static int __init imxdma_probe(struct platform_device *pdev)
 	{
 	struct imxdma_engine *imxdma;
 	struct resource *res;
+	const struct of_device_id *of_id;
 	int ret, i;
 	int irq, irq_err;
 
+	of_id = of_match_device(imx_dma_of_dev_id, &pdev->dev);
+	if (of_id)
+		pdev->id_entry = of_id->data;
+
 	imxdma = devm_kzalloc(&pdev->dev, sizeof(*imxdma), GFP_KERNEL);
 	if (!imxdma)
 		return -ENOMEM;
@@ -1136,8 +1180,22 @@ static int __init imxdma_probe(struct platform_device *pdev)
 		goto err;
 	}
 
+	if (pdev->dev.of_node) {
+		imxdma->of_filter_info.dma_cap = imxdma->dma_device.cap_mask;
+		imxdma->of_filter_info.filter_fn = imxdma_filter_fn;
+		ret = of_dma_controller_register(pdev->dev.of_node,
+				of_dma_simple_xlate,
+				&imxdma->of_filter_info);
+		if (ret) {
+			dev_err(&pdev->dev, "unable to register of_dma_controller\n");
+			goto err_of_dma_controller;
+		}
+	}
+
 	return 0;
 
+err_of_dma_controller:
+	dma_async_device_unregister(&imxdma->dma_device);
 err:
 	clk_disable_unprepare(imxdma->dma_ipg);
 	clk_disable_unprepare(imxdma->dma_ahb);
@@ -1150,6 +1208,9 @@ static int __exit imxdma_remove(struct platform_device *pdev)
 
         dma_async_device_unregister(&imxdma->dma_device);
 
+	if (pdev->dev.of_node)
+		of_dma_controller_free(pdev->dev.of_node);
+
 	clk_disable_unprepare(imxdma->dma_ipg);
 	clk_disable_unprepare(imxdma->dma_ahb);
 
@@ -1159,6 +1220,7 @@ static int __exit imxdma_remove(struct platform_device *pdev)
 static struct platform_driver imxdma_driver = {
 	.driver		= {
 		.name	= "imx-dma",
+		.of_match_table = imx_dma_of_dev_id,
 	},
 	.id_table	= imx_dma_devtype,
 	.remove		= __exit_p(imxdma_remove),
diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
index f6d30cc..762a7d0 100644
--- a/include/linux/platform_data/dma-imx.h
+++ b/include/linux/platform_data/dma-imx.h
@@ -51,6 +51,9 @@ struct imx_dma_data {
 	int dma_request; /* DMA request line */
 	enum sdma_peripheral_type peripheral_type;
 	int priority;
+
+	/* Did the controller's filter function allocated this object? */
+	bool alloc_ctl_filter;
 };
 
 static inline int imx_dma_is_ipu(struct dma_chan *chan)
@@ -63,7 +66,8 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
 	return strstr(dev_name(chan->device->dev), "sdma") ||
 		!strcmp(dev_name(chan->device->dev), "imx1-dma") ||
 		!strcmp(dev_name(chan->device->dev), "imx21-dma") ||
-		!strcmp(dev_name(chan->device->dev), "imx27-dma");
+		!strcmp(dev_name(chan->device->dev), "imx27-dma") ||
+		!strcmp(chan->device->dev->driver->name, "imx-dma");
 }
 
 #endif
-- 
1.8.1.2

Re: [PATCH V2] dma: imx-dma: Add oftree support

[Arnd Bergmann]

On Saturday 23 February 2013, Markus Pargmann wrote:

> +Required properties:
> +- compatible : Should be "fsl,<chip>-dma". chip can be imx1, imx21 or imx27
> +- reg : Should contain DMA registers location and length
> +- interrupts : First item should be DMA interrupt, second one is optional and
> +    should contain DMA Error interrupt
> +- #dma-cells : Has to be 1. imx-dma does not support anything else.

Hmm, so #dma-cells is 1

> @@ -996,13 +1020,33 @@ static void imxdma_issue_pending(struct dma_chan *chan)
>  	spin_unlock_irqrestore(&imxdma->lock, flags);
>  }
>  
> +bool imxdma_filter_fn(struct dma_chan *chan, void *param)
> +{
> +	struct imx_dma_data *data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	printk("%s\n", __func__);
> +
> +	if (!data)
> +		return false;
> +
> +	data->dma_request = *(unsigned *) param;
> +	data->alloc_ctl_filter = true;
> +	chan->private = data;
> +
> +	return true;
> +}

which matches the usage here, but 

> diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
> index f6d30cc..762a7d0 100644
> --- a/include/linux/platform_data/dma-imx.h
> +++ b/include/linux/platform_data/dma-imx.h
> @@ -51,6 +51,9 @@ struct imx_dma_data {
>  	int dma_request; /* DMA request line */
>  	enum sdma_peripheral_type peripheral_type;
>  	int priority;
> +
> +	/* Did the controller's filter function allocated this object? */
> +	bool alloc_ctl_filter;
>  };

There are actually two more members in the imx_dma_data structure. Shouldn't those
be encoded in the dma specifier as well?
  
>  static inline int imx_dma_is_ipu(struct dma_chan *chan)
> @@ -63,7 +66,8 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
>  	return strstr(dev_name(chan->device->dev), "sdma") ||
>  		!strcmp(dev_name(chan->device->dev), "imx1-dma") ||
>  		!strcmp(dev_name(chan->device->dev), "imx21-dma") ||
> -		!strcmp(dev_name(chan->device->dev), "imx27-dma");
> +		!strcmp(dev_name(chan->device->dev), "imx27-dma") ||
> +		!strcmp(chan->device->dev->driver->name, "imx-dma");
>  }

Also, your filter function does not actually check
imx_dma_is_general_purpose() as the old style filter functions
in the slave drivers do, which breaks when you have more than one dma engine
in the system.

I think you have to provide your own xlate function, and pass the controller
and multiple cells into the filter function, or use no filter at all but instead
find a way to get a channel directly.

	Arnd

Re: [PATCH V2] dma: imx-dma: Add oftree support

[Markus Pargmann]

On Sat, Feb 23, 2013 at 10:16:21PM +0000, Arnd Bergmann wrote:
> On Saturday 23 February 2013, Markus Pargmann wrote:
> 
> > +Required properties:
> > +- compatible : Should be "fsl,<chip>-dma". chip can be imx1, imx21 or imx27
> > +- reg : Should contain DMA registers location and length
> > +- interrupts : First item should be DMA interrupt, second one is optional and
> > +    should contain DMA Error interrupt
> > +- #dma-cells : Has to be 1. imx-dma does not support anything else.
> 
> Hmm, so #dma-cells is 1
> 
> > @@ -996,13 +1020,33 @@ static void imxdma_issue_pending(struct dma_chan *chan)
> >  	spin_unlock_irqrestore(&imxdma->lock, flags);
> >  }
> >  
> > +bool imxdma_filter_fn(struct dma_chan *chan, void *param)
> > +{
> > +	struct imx_dma_data *data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +	printk("%s\n", __func__);
> > +
> > +	if (!data)
> > +		return false;
> > +
> > +	data->dma_request = *(unsigned *) param;
> > +	data->alloc_ctl_filter = true;
> > +	chan->private = data;
> > +
> > +	return true;
> > +}
> 
> which matches the usage here, but 
> 
> > diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
> > index f6d30cc..762a7d0 100644
> > --- a/include/linux/platform_data/dma-imx.h
> > +++ b/include/linux/platform_data/dma-imx.h
> > @@ -51,6 +51,9 @@ struct imx_dma_data {
> >  	int dma_request; /* DMA request line */
> >  	enum sdma_peripheral_type peripheral_type;
> >  	int priority;
> > +
> > +	/* Did the controller's filter function allocated this object? */
> > +	bool alloc_ctl_filter;
> >  };
> 
> There are actually two more members in the imx_dma_data structure. Shouldn't those
> be encoded in the dma specifier as well?

imx_dma_data is used by imx-dma and imx-sdma, but imx-dma does not use
peripheral_type and priority. When not loaded from devicetree
imx_dma_data is constructed by imx drivers without knowledge about the
dma driver. This patch moves the initialization to the driver, so I
think there is no need to fill all fields. I didn't want to use a new
struct because the imx drivers could still use this old way.

>   
> >  static inline int imx_dma_is_ipu(struct dma_chan *chan)
> > @@ -63,7 +66,8 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
> >  	return strstr(dev_name(chan->device->dev), "sdma") ||
> >  		!strcmp(dev_name(chan->device->dev), "imx1-dma") ||
> >  		!strcmp(dev_name(chan->device->dev), "imx21-dma") ||
> > -		!strcmp(dev_name(chan->device->dev), "imx27-dma");
> > +		!strcmp(dev_name(chan->device->dev), "imx27-dma") ||
> > +		!strcmp(chan->device->dev->driver->name, "imx-dma");
> >  }
> 
> Also, your filter function does not actually check
> imx_dma_is_general_purpose() as the old style filter functions
> in the slave drivers do, which breaks when you have more than one dma engine
> in the system.

Oh yes, in the filter function should be a device driver comparison. But
I would prefer a direct check instead of imx_dma_is_general_purpose because
I think the driver should not define a filter function that initializes data
for another driver (sdma).

Thanks

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH V2] dma: imx-dma: Add oftree support

[Arnd Bergmann]

On Sunday 24 February 2013, Markus Pargmann wrote:
> On Sat, Feb 23, 2013 at 10:16:21PM +0000, Arnd Bergmann wrote:
> > On Saturday 23 February 2013, Markus Pargmann wrote:

> > 
> > There are actually two more members in the imx_dma_data structure. Shouldn't those
> > be encoded in the dma specifier as well?
> 
> imx_dma_data is used by imx-dma and imx-sdma, but imx-dma does not use
> peripheral_type and priority. When not loaded from devicetree
> imx_dma_data is constructed by imx drivers without knowledge about the
> dma driver. This patch moves the initialization to the driver, so I
> think there is no need to fill all fields. I didn't want to use a new
> struct because the imx drivers could still use this old way.

Ok, got it.

> > >  static inline int imx_dma_is_ipu(struct dma_chan *chan)
> > > @@ -63,7 +66,8 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
> > >  	return strstr(dev_name(chan->device->dev), "sdma") ||
> > >  		!strcmp(dev_name(chan->device->dev), "imx1-dma") ||
> > >  		!strcmp(dev_name(chan->device->dev), "imx21-dma") ||
> > > -		!strcmp(dev_name(chan->device->dev), "imx27-dma");
> > > +		!strcmp(dev_name(chan->device->dev), "imx27-dma") ||
> > > +		!strcmp(chan->device->dev->driver->name, "imx-dma");
> > >  }
> > 
> > Also, your filter function does not actually check
> > imx_dma_is_general_purpose() as the old style filter functions
> > in the slave drivers do, which breaks when you have more than one dma engine
> > in the system.
> 
> Oh yes, in the filter function should be a device driver comparison. But
> I would prefer a direct check instead of imx_dma_is_general_purpose because
> I think the driver should not define a filter function that initializes data
> for another driver (sdma).

Yes, that is also how I did it in the dwdma patches. Basically  I have
the dma engine object passed into xlate and then from there into the
filter, where I can compare it against chan->device.

	Arnd

[PATCH v3 1/2] dma: imx-dma: Add oftree support

[Markus Pargmann]

Adding devicetree support for imx-dma driver. Use driver name for
function 'imx_dma_is_general_purpose' because the devicename for
devicetree initialized devices is different.

Changes in V3:
- Create a imxdma translate function to pass the device object into the
  filter function.

Changes in V2:
- Change the driver to use generic DMA DT bindings.
- Add a imx-dma filter function that sets the dma request line in
  private data.

Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
 .../devicetree/bindings/dma/fsl-imx-dma.txt        | 48 ++++++++++++
 drivers/dma/imx-dma.c                              | 87 +++++++++++++++++++++-
 include/linux/platform_data/dma-imx.h              |  6 +-
 3 files changed, 139 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/fsl-imx-dma.txt

diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-dma.txt b/Documentation/devicetree/bindings/dma/fsl-imx-dma.txt
new file mode 100644
index 0000000..04d8e40
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/fsl-imx-dma.txt
@@ -0,0 +1,48 @@
+* Freescale Direct Memory Access (DMA) Controller for i.MX
+
+This document will only describe differences to the generic DMA Controller and
+DMA request bindings.
+
+* DMA controller
+
+Required properties:
+- compatible : Should be "fsl,<chip>-dma". chip can be imx1, imx21 or imx27
+- reg : Should contain DMA registers location and length
+- interrupts : First item should be DMA interrupt, second one is optional and
+    should contain DMA Error interrupt
+- #dma-cells : Has to be 1. imx-dma does not support anything else.
+
+Optional properties:
+- #dma-channels : Number of DMA channels supported. Should be 16.
+- #dma-requests : Number of DMA requests supported.
+
+Example:
+
+	dma: dma@10001000 {
+		compatible = "fsl,imx27-dma";
+		reg = <0x10001000 0x1000>;
+		interrupts = <32 33>;
+		#dma-cells = <1>;
+		#dma-channels = <16>;
+	};
+
+
+* DMA client
+
+Clients have to specify the DMA requests with phandles in a list.
+
+Required properties:
+- dmas: List of one or more DMA request specifiers. One DMA request specifier
+    consists of a phandle to the DMA controller followed by the integer
+    specifiying the request line.
+- dma-names: List of string identifiers for the DMA requests. For the correct
+    names, have a look at the specific client driver.
+
+Example:
+
+	sdhci1: sdhci@10013000 {
+		...
+		dmas = <&dma 7>;
+		dma-names = "rx-tx";
+		...
+	};
diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index a7dcf78..5856117 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -26,6 +26,8 @@
 #include <linux/clk.h>
 #include <linux/dmaengine.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_dma.h>
 
 #include <asm/irq.h>
 #include <linux/platform_data/dma-imx.h>
@@ -185,6 +187,11 @@ struct imxdma_engine {
 	enum imx_dma_type		devtype;
 };
 
+struct imxdma_filter_data {
+	struct imxdma_engine	*imxdma;
+	int			 request;
+};
+
 static struct platform_device_id imx_dma_devtype[] = {
 	{
 		.name = "imx1-dma",
@@ -201,6 +208,22 @@ static struct platform_device_id imx_dma_devtype[] = {
 };
 MODULE_DEVICE_TABLE(platform, imx_dma_devtype);
 
+static const struct of_device_id imx_dma_of_dev_id[] = {
+	{
+		.compatible = "fsl,imx1-dma",
+		.data = &imx_dma_devtype[IMX1_DMA],
+	}, {
+		.compatible = "fsl,imx21-dma",
+		.data = &imx_dma_devtype[IMX21_DMA],
+	}, {
+		.compatible = "fsl,imx27-dma",
+		.data = &imx_dma_devtype[IMX27_DMA],
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, imx_dma_of_dev_id);
+
 static inline int is_imx1_dma(struct imxdma_engine *imxdma)
 {
 	return imxdma->devtype == IMX1_DMA;
@@ -734,8 +757,13 @@ static int imxdma_alloc_chan_resources(struct dma_chan *chan)
 	struct imxdma_channel *imxdmac = to_imxdma_chan(chan);
 	struct imx_dma_data *data = chan->private;
 
-	if (data != NULL)
+	if (data != NULL) {
 		imxdmac->dma_request = data->dma_request;
+		if (data->alloc_ctl_filter) {
+			kfree(data);
+			chan->private = 0;
+		}
+	}
 
 	while (imxdmac->descs_allocated < IMXDMA_MAX_CHAN_DESCRIPTORS) {
 		struct imxdma_desc *desc;
@@ -996,13 +1024,55 @@ static void imxdma_issue_pending(struct dma_chan *chan)
 	spin_unlock_irqrestore(&imxdma->lock, flags);
 }
 
+static bool imxdma_filter_fn(struct dma_chan *chan, void *param)
+{
+	struct imxdma_filter_data *fdata = param;
+	struct imx_dma_data *data;
+
+	if (chan->device->dev != fdata->imxdma->dev)
+		return false;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+
+	if (!data)
+		return false;
+
+	data->dma_request = fdata->request;
+	data->alloc_ctl_filter = true;
+	chan->private = data;
+
+	return true;
+}
+
+static struct dma_chan *imxdma_xlate(struct of_phandle_args *dma_spec,
+						struct of_dma *ofdma)
+{
+	int count = dma_spec->args_count;
+	struct imxdma_engine *imxdma = ofdma->of_dma_data;
+	struct imxdma_filter_data fdata = {
+		.imxdma = imxdma,
+		.request = *(unsigned *)&dma_spec->args[0],
+	};
+
+	if (count != 1)
+		return NULL;
+
+	return dma_request_channel(imxdma->dma_device.cap_mask,
+					imxdma_filter_fn, &fdata);
+}
+
 static int __init imxdma_probe(struct platform_device *pdev)
 	{
 	struct imxdma_engine *imxdma;
 	struct resource *res;
+	const struct of_device_id *of_id;
 	int ret, i;
 	int irq, irq_err;
 
+	of_id = of_match_device(imx_dma_of_dev_id, &pdev->dev);
+	if (of_id)
+		pdev->id_entry = of_id->data;
+
 	imxdma = devm_kzalloc(&pdev->dev, sizeof(*imxdma), GFP_KERNEL);
 	if (!imxdma)
 		return -ENOMEM;
@@ -1136,8 +1206,19 @@ static int __init imxdma_probe(struct platform_device *pdev)
 		goto err;
 	}
 
+	if (pdev->dev.of_node) {
+		ret = of_dma_controller_register(pdev->dev.of_node,
+				imxdma_xlate, imxdma);
+		if (ret) {
+			dev_err(&pdev->dev, "unable to register of_dma_controller\n");
+			goto err_of_dma_controller;
+		}
+	}
+
 	return 0;
 
+err_of_dma_controller:
+	dma_async_device_unregister(&imxdma->dma_device);
 err:
 	clk_disable_unprepare(imxdma->dma_ipg);
 	clk_disable_unprepare(imxdma->dma_ahb);
@@ -1150,6 +1231,9 @@ static int __exit imxdma_remove(struct platform_device *pdev)
 
         dma_async_device_unregister(&imxdma->dma_device);
 
+	if (pdev->dev.of_node)
+		of_dma_controller_free(pdev->dev.of_node);
+
 	clk_disable_unprepare(imxdma->dma_ipg);
 	clk_disable_unprepare(imxdma->dma_ahb);
 
@@ -1159,6 +1243,7 @@ static int __exit imxdma_remove(struct platform_device *pdev)
 static struct platform_driver imxdma_driver = {
 	.driver		= {
 		.name	= "imx-dma",
+		.of_match_table = imx_dma_of_dev_id,
 	},
 	.id_table	= imx_dma_devtype,
 	.remove		= __exit_p(imxdma_remove),
diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
index f6d30cc..762a7d0 100644
--- a/include/linux/platform_data/dma-imx.h
+++ b/include/linux/platform_data/dma-imx.h
@@ -51,6 +51,9 @@ struct imx_dma_data {
 	int dma_request; /* DMA request line */
 	enum sdma_peripheral_type peripheral_type;
 	int priority;
+
+	/* Did the controller's filter function allocated this object? */
+	bool alloc_ctl_filter;
 };
 
 static inline int imx_dma_is_ipu(struct dma_chan *chan)
@@ -63,7 +66,8 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
 	return strstr(dev_name(chan->device->dev), "sdma") ||
 		!strcmp(dev_name(chan->device->dev), "imx1-dma") ||
 		!strcmp(dev_name(chan->device->dev), "imx21-dma") ||
-		!strcmp(dev_name(chan->device->dev), "imx27-dma");
+		!strcmp(dev_name(chan->device->dev), "imx27-dma") ||
+		!strcmp(chan->device->dev->driver->name, "imx-dma");
 }
 
 #endif
-- 
1.8.1.2

[PATCH v3 2/2] DMA: imx-dma: imxdma->dev used uninitialized

[Markus Pargmann]

imxdma->dev is used for dev_warn before it was set.

Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/dma/imx-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index 5856117..5830a0c 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -1077,6 +1077,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
 	if (!imxdma)
 		return -ENOMEM;
 
+	imxdma->dev = &pdev->dev;
 	imxdma->devtype = pdev->id_entry->driver_data;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1181,7 +1182,6 @@ static int __init imxdma_probe(struct platform_device *pdev)
 			      &imxdma->dma_device.channels);
 	}
 
-	imxdma->dev = &pdev->dev;
 	imxdma->dma_device.dev = &pdev->dev;
 
 	imxdma->dma_device.device_alloc_chan_resources = imxdma_alloc_chan_resources;
-- 
1.8.1.2

Re: [PATCH v3 1/2] dma: imx-dma: Add oftree support

[Arnd Bergmann]

On Monday 25 February 2013, Markus Pargmann wrote:
> Adding devicetree support for imx-dma driver. Use driver name for
> function 'imx_dma_is_general_purpose' because the devicename for
> devicetree initialized devices is different.
> 
> Changes in V3:
> - Create a imxdma translate function to pass the device object into the
>   filter function.
> 
> Changes in V2:
> - Change the driver to use generic DMA DT bindings.
> - Add a imx-dma filter function that sets the dma request line in
>   private data.
> 
> Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

Looks correct now,

Reviewed-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

I have one idea for an optimization though:

> +static bool imxdma_filter_fn(struct dma_chan *chan, void *param)
> +{
> +	struct imxdma_filter_data *fdata = param;
> +	struct imx_dma_data *data;
> +
> +	if (chan->device->dev != fdata->imxdma->dev)
> +		return false;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +
> +	if (!data)
> +		return false;
> +
> +	data->dma_request = fdata->request;
> +	data->alloc_ctl_filter = true;
> +	chan->private = data;
> +
> +	return true;
> +}

There is already a "dma_request" member in "struct imxdma_channel", so
you could use that instead of allocating the imx_dma_data here.

Then you don't need to keep track of freeing that memory again, but
you might have to change a few other functions that look at data->dma_request
rather than chan->dma_request at the moment and change them to
use the latter instead. You can copy data->dma_request over in the
alloc_chan_resources functions for the legacy case.

	Arnd

Re: [PATCH v3 1/2] dma: imx-dma: Add oftree support

[Markus Pargmann]

On Mon, Feb 25, 2013 at 04:24:51PM +0000, Arnd Bergmann wrote:
> On Monday 25 February 2013, Markus Pargmann wrote:
> > Adding devicetree support for imx-dma driver. Use driver name for
> > function 'imx_dma_is_general_purpose' because the devicename for
> > devicetree initialized devices is different.
> > 
> > Changes in V3:
> > - Create a imxdma translate function to pass the device object into the
> >   filter function.
> > 
> > Changes in V2:
> > - Change the driver to use generic DMA DT bindings.
> > - Add a imx-dma filter function that sets the dma request line in
> >   private data.
> > 
> > Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> Looks correct now,
> 
> Reviewed-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> I have one idea for an optimization though:
> 
> > +static bool imxdma_filter_fn(struct dma_chan *chan, void *param)
> > +{
> > +	struct imxdma_filter_data *fdata = param;
> > +	struct imx_dma_data *data;
> > +
> > +	if (chan->device->dev != fdata->imxdma->dev)
> > +		return false;
> > +
> > +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +
> > +	if (!data)
> > +		return false;
> > +
> > +	data->dma_request = fdata->request;
> > +	data->alloc_ctl_filter = true;
> > +	chan->private = data;
> > +
> > +	return true;
> > +}
> 
> There is already a "dma_request" member in "struct imxdma_channel", so
> you could use that instead of allocating the imx_dma_data here.
> 
> Then you don't need to keep track of freeing that memory again, but
> you might have to change a few other functions that look at data->dma_request
> rather than chan->dma_request at the moment and change them to
> use the latter instead. You can copy data->dma_request over in the
> alloc_chan_resources functions for the legacy case.

Thanks, that is much better than the current use of kzalloc. I updated it for v4.

Markus

> 
> 	Arnd

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 1/2] dma: imx-dma: Add oftree support

[Shawn Guo]

On Mon, Feb 25, 2013 at 05:04:14PM +0100, Markus Pargmann wrote:
> Adding devicetree support for imx-dma driver. Use driver name for
> function 'imx_dma_is_general_purpose' because the devicename for
> devicetree initialized devices is different.

...

> @@ -63,7 +66,8 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
>  	return strstr(dev_name(chan->device->dev), "sdma") ||
>  		!strcmp(dev_name(chan->device->dev), "imx1-dma") ||
>  		!strcmp(dev_name(chan->device->dev), "imx21-dma") ||
> -		!strcmp(dev_name(chan->device->dev), "imx27-dma");
> +		!strcmp(dev_name(chan->device->dev), "imx27-dma") ||
> +		!strcmp(chan->device->dev->driver->name, "imx-dma");
>  }

So the function could just look like the following?

static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
{
        return !strcmp(chan->device->dev->driver->name, "imx-sdma") ||
                !strcmp(chan->device->dev->driver->name, "imx-dma");
}

Shawn

Re: [PATCH v3 1/2] dma: imx-dma: Add oftree support

[Markus Pargmann]

On Thu, Feb 28, 2013 at 02:30:27PM +0800, Shawn Guo wrote:
> On Mon, Feb 25, 2013 at 05:04:14PM +0100, Markus Pargmann wrote:
> > Adding devicetree support for imx-dma driver. Use driver name for
> > function 'imx_dma_is_general_purpose' because the devicename for
> > devicetree initialized devices is different.
> 
> ...
> 
> > @@ -63,7 +66,8 @@ static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
> >  	return strstr(dev_name(chan->device->dev), "sdma") ||
> >  		!strcmp(dev_name(chan->device->dev), "imx1-dma") ||
> >  		!strcmp(dev_name(chan->device->dev), "imx21-dma") ||
> > -		!strcmp(dev_name(chan->device->dev), "imx27-dma");
> > +		!strcmp(dev_name(chan->device->dev), "imx27-dma") ||
> > +		!strcmp(chan->device->dev->driver->name, "imx-dma");
> >  }
> 
> So the function could just look like the following?
> 
> static inline int imx_dma_is_general_purpose(struct dma_chan *chan)
> {
>         return !strcmp(chan->device->dev->driver->name, "imx-sdma") ||
>                 !strcmp(chan->device->dev->driver->name, "imx-dma");
> }

Yes. For imx-dma there is only imx1, imx21 and imx27 that use imx-dma
driver. And the devicenames of imx-sdma are imx31-sdma and imx35-sdma,
so all devices using imx-sdma match. I will change it for v4. Thanks

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |