Patchwork [RFC,v5,4/5] dma: mpc512x: register for device tree channel lookup

login
register
mail settings
Submitter Alexander Popov
Date Nov. 1, 2013, 7:19 a.m.
Message ID <1383290374-17484-5-git-send-email-a13xp0p0v88@gmail.com>
Download mbox | patch
Permalink /patch/287713/
State Superseded
Delegated to: Anatolij Gustschin
Headers show

Comments

Alexander Popov - Nov. 1, 2013, 7:19 a.m.
From: Gerhard Sittig <gsi@denx.de>

register the controller for device tree based lookup of DMA channels
(non-fatal for backwards compatibility with older device trees), provide
the '#dma-cells' property in the shared mpc5121.dtsi file, and introduce
a bindings document for the MPC512x DMA controller

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 .../devicetree/bindings/dma/mpc512x-dma.txt        | 55 ++++++++++++++++++++++
 arch/powerpc/boot/dts/mpc5121.dtsi                 |  1 +
 drivers/dma/mpc512x_dma.c                          | 21 +++++++--
 3 files changed, 74 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/mpc512x-dma.txt
Mark Rutland - Nov. 18, 2013, 12:09 p.m.
On Fri, Nov 01, 2013 at 07:19:33AM +0000, Alexander Popov wrote:
> From: Gerhard Sittig <gsi@denx.de>
> 
> register the controller for device tree based lookup of DMA channels
> (non-fatal for backwards compatibility with older device trees), provide
> the '#dma-cells' property in the shared mpc5121.dtsi file, and introduce
> a bindings document for the MPC512x DMA controller
> 
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
>  .../devicetree/bindings/dma/mpc512x-dma.txt        | 55 ++++++++++++++++++++++
>  arch/powerpc/boot/dts/mpc5121.dtsi                 |  1 +
>  drivers/dma/mpc512x_dma.c                          | 21 +++++++--
>  3 files changed, 74 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/dma/mpc512x-dma.txt
> 
> diff --git a/Documentation/devicetree/bindings/dma/mpc512x-dma.txt b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
> new file mode 100644
> index 0000000..a4867d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
> @@ -0,0 +1,55 @@
> +* Freescale MPC512x DMA Controller
> +
> +The DMA controller in the Freescale MPC512x SoC can move blocks of
> +memory contents between memory and peripherals or memory to memory.
> +
> +Refer to the "Generic DMA Controller and DMA request bindings" description
> +in the dma.txt file for a more detailled discussion of the binding.  The
> +MPC512x DMA engine binding follows the common scheme, but doesn't provide
> +support for the optional channels and requests counters (those values are
> +derived from the detected hardware features) and has a fixed client
> +specifier length of 1 integer cell (the value is the DMA channel, since
> +the DMA controller uses a fixed assignment of request lines per channel).

The fact that #dma-cells must be <1> isn't a difference from the
standard binding, and needs not be described here. The meaning of the
value should be in your description of #dma-cells below.

I'm not sure it's worth mentioning optional channels / request counters.
If anything, it would be better to update dma.txt to move the "Optional
properties" to something like "Suggested properties"...

> +
> +
> +DMA controller node properties:
> +
> +Required properties:
> +- compatible:		should be "fsl,mpc5121-dma"
> +- reg:			address and size of the DMA controller's register set
> +- interrupts:		interrupt spec for the DMA controller
> +
> +Optional properties:
> +- #dma-cells:		must be <1>, describes the number of integer cells
> +			needed to specify the 'dmas' property in client nodes,
> +			strongly recommended since common client helper code
> +			uses this property

Rather than redundantly describing what the standard property means, it
would be better to describe what the cell actually means (i.e. which
value maps to which DMA channel in hardware, how many their are, etc).

> +
> +Example:
> +
> +	dma0: dma@14000 {
> +		compatible = "fsl,mpc5121-dma";
> +		reg = <0x14000 0x1800>;
> +		interrupts = <65 0x8>;
> +		#dma-cells = <1>;
> +	};
> +
> +
> +Client node properties:
> +
> +Required properties:
> +- dmas:			list of DMA specifiers, consisting each of a handle
> +			for the DMA controller and integer cells to specify
> +			the channel used within the DMA controller
> +- dma-names:		list of identifier strings for the DMA specifiers,
> +			client device driver code uses these strings to
> +			have DMA channels looked up at the controller
> +

I don't think its necessary or helpful to describe the client binding
here, it's in dma.txt

Thanks,
Mark.
Arnd Bergmann - Nov. 18, 2013, 2:31 p.m.
On Monday 18 November 2013, Mark Rutland wrote:
> > +++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
> > @@ -0,0 +1,55 @@
> > +* Freescale MPC512x DMA Controller
> > +
> > +The DMA controller in the Freescale MPC512x SoC can move blocks of
> > +memory contents between memory and peripherals or memory to memory.
> > +
> > +Refer to the "Generic DMA Controller and DMA request bindings" description
> > +in the dma.txt file for a more detailled discussion of the binding.  The
> > +MPC512x DMA engine binding follows the common scheme, but doesn't provide
> > +support for the optional channels and requests counters (those values are
> > +derived from the detected hardware features) and has a fixed client
> > +specifier length of 1 integer cell (the value is the DMA channel, since
> > +the DMA controller uses a fixed assignment of request lines per channel).
> 
> The fact that #dma-cells must be <1> isn't a difference from the
> standard binding, and needs not be described here. The meaning of the
> value should be in your description of #dma-cells below.

I think the value it has to be in there, and I have in the past asked other
people to add this. Note that in the generic binding, it says that it must
be "at least 1". You can have controllers that require a larger number, or
that can use 1 or 2 alternatively, depending on how the device is wired
up, e.g. when a dma controller has two master ports you would need a
second cell to specify the port number, but only if more than one port
is actually connected to a slave.

> I'm not sure it's worth mentioning optional channels / request counters.
> If anything, it would be better to update dma.txt to move the "Optional
> properties" to something like "Suggested properties"...

These are less clearly defined. In the generic binding, it's mostly a matter
of "if you need to pass this information, use these properties". The individual
binding can then make them mandatory if needed.

	Arnd
Mark Rutland - Nov. 18, 2013, 3:23 p.m.
On Mon, Nov 18, 2013 at 02:31:54PM +0000, Arnd Bergmann wrote:
> On Monday 18 November 2013, Mark Rutland wrote:
> > > +++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
> > > @@ -0,0 +1,55 @@
> > > +* Freescale MPC512x DMA Controller
> > > +
> > > +The DMA controller in the Freescale MPC512x SoC can move blocks of
> > > +memory contents between memory and peripherals or memory to memory.
> > > +
> > > +Refer to the "Generic DMA Controller and DMA request bindings" description
> > > +in the dma.txt file for a more detailled discussion of the binding.  The
> > > +MPC512x DMA engine binding follows the common scheme, but doesn't provide
> > > +support for the optional channels and requests counters (those values are
> > > +derived from the detected hardware features) and has a fixed client
> > > +specifier length of 1 integer cell (the value is the DMA channel, since
> > > +the DMA controller uses a fixed assignment of request lines per channel).
> > 
> > The fact that #dma-cells must be <1> isn't a difference from the
> > standard binding, and needs not be described here. The meaning of the
> > value should be in your description of #dma-cells below.
> 
> I think the value it has to be in there, and I have in the past asked other
> people to add this. Note that in the generic binding, it says that it must
> be "at least 1". You can have controllers that require a larger number, or
> that can use 1 or 2 alternatively, depending on how the device is wired
> up, e.g. when a dma controller has two master ports you would need a
> second cell to specify the port number, but only if more than one port
> is actually connected to a slave.

The number of cells required should be described. My points were that it
should be described at the property description rather than in the introduction,
and that the fact that a specific value was required was not a
difference from the bindings as the paragraph implied.

> 
> > I'm not sure it's worth mentioning optional channels / request counters.
> > If anything, it would be better to update dma.txt to move the "Optional
> > properties" to something like "Suggested properties"...
> 
> These are less clearly defined. In the generic binding, it's mostly a matter
> of "if you need to pass this information, use these properties". The individual
> binding can then make them mandatory if needed.

Agreed. I'd prefer that bindings described the suggested properties they
used, rather than those they don't.

Thanks,
Mark.

Patch

diff --git a/Documentation/devicetree/bindings/dma/mpc512x-dma.txt b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
new file mode 100644
index 0000000..a4867d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
@@ -0,0 +1,55 @@ 
+* Freescale MPC512x DMA Controller
+
+The DMA controller in the Freescale MPC512x SoC can move blocks of
+memory contents between memory and peripherals or memory to memory.
+
+Refer to the "Generic DMA Controller and DMA request bindings" description
+in the dma.txt file for a more detailled discussion of the binding.  The
+MPC512x DMA engine binding follows the common scheme, but doesn't provide
+support for the optional channels and requests counters (those values are
+derived from the detected hardware features) and has a fixed client
+specifier length of 1 integer cell (the value is the DMA channel, since
+the DMA controller uses a fixed assignment of request lines per channel).
+
+
+DMA controller node properties:
+
+Required properties:
+- compatible:		should be "fsl,mpc5121-dma"
+- reg:			address and size of the DMA controller's register set
+- interrupts:		interrupt spec for the DMA controller
+
+Optional properties:
+- #dma-cells:		must be <1>, describes the number of integer cells
+			needed to specify the 'dmas' property in client nodes,
+			strongly recommended since common client helper code
+			uses this property
+
+Example:
+
+	dma0: dma@14000 {
+		compatible = "fsl,mpc5121-dma";
+		reg = <0x14000 0x1800>;
+		interrupts = <65 0x8>;
+		#dma-cells = <1>;
+	};
+
+
+Client node properties:
+
+Required properties:
+- dmas:			list of DMA specifiers, consisting each of a handle
+			for the DMA controller and integer cells to specify
+			the channel used within the DMA controller
+- dma-names:		list of identifier strings for the DMA specifiers,
+			client device driver code uses these strings to
+			have DMA channels looked up at the controller
+
+Example:
+
+	sdhc@1500 {
+		compatible = "fsl,mpc5121-sdhc";
+		/* ... */
+		dmas = <&dma0 30>;
+		dma-names = "rx-tx";
+	};
diff --git a/arch/powerpc/boot/dts/mpc5121.dtsi b/arch/powerpc/boot/dts/mpc5121.dtsi
index bd14c00..eb0058f 100644
--- a/arch/powerpc/boot/dts/mpc5121.dtsi
+++ b/arch/powerpc/boot/dts/mpc5121.dtsi
@@ -390,6 +390,7 @@ 
 			compatible = "fsl,mpc5121-dma";
 			reg = <0x14000 0x1800>;
 			interrupts = <65 0x8>;
+			#dma-cells = <1>;
 		};
 	};
 
diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index d2ec42d..9929cdc 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -48,6 +48,7 @@ 
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/of_device.h>
+#include <linux/of_dma.h>
 #include <linux/of_platform.h>
 
 #include <linux/random.h>
@@ -1013,11 +1014,23 @@  static int mpc_dma_probe(struct platform_device *op)
 	/* Register DMA engine */
 	dev_set_drvdata(dev, mdma);
 	retval = dma_async_device_register(dma);
-	if (retval) {
-		devm_free_irq(dev, mdma->irq, mdma);
-		irq_dispose_mapping(mdma->irq);
+	if (retval)
+		goto out_irq;
+
+	/* register with OF helpers for DMA lookups (nonfatal) */
+	if (dev->of_node) {
+		retval = of_dma_controller_register(dev->of_node,
+						    of_dma_xlate_by_chan_id,
+						    mdma);
+		if (retval)
+			dev_warn(dev, "could not register for OF lookup\n");
 	}
 
+	return 0;
+
+out_irq:
+	devm_free_irq(dev, mdma->irq, mdma);
+	irq_dispose_mapping(mdma->irq);
 	return retval;
 }
 
@@ -1026,6 +1039,8 @@  static int mpc_dma_remove(struct platform_device *op)
 	struct device *dev = &op->dev;
 	struct mpc_dma *mdma = dev_get_drvdata(dev);
 
+	if (dev->of_node)
+		of_dma_controller_free(dev->of_node);
 	dma_async_device_unregister(&mdma->dma);
 	devm_free_irq(dev, mdma->irq, mdma);
 	irq_dispose_mapping(mdma->irq);