[PATCH 0/3] dma: add support for DMA multiplexer DT nodes

[PATCH 0/3] dma: add support for DMA multiplexer DT nodes

[Guennadi Liakhovetski]

Next re-send of an earlier work. This series adds support for multiple 
compatible DMAC instances, capable of serving the same DMA slaves. 
Currently to specify such a configuration in DT, slaves would have to add 
DMA tuples for each requested channel for each suitable DMAC. This 
needlessly clutters the Device Tree. Instead we group such DMAC DT nodes 
together under a common parent. Then slaves can just reference that parent 
in their DMA bindings.

Cc: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>

Guennadi Liakhovetski (3):
  OF: add a new phandle parsing function for grouped nodes
  dmaengine: add support for DMA multiplexer DT nodes
  DMA: shdma: add DT support

 Documentation/devicetree/bindings/dma/dma.txt |   44 +++++++++++++++++++++
 drivers/dma/of-dma.c                          |   31 +++++++++++----
 drivers/dma/sh/shdma-base.c                   |   51 ++++++++++++++++++++++---
 drivers/dma/sh/shdma.c                        |   44 ++++++++++++++++++---
 drivers/of/base.c                             |   28 ++++++++++++-
 include/linux/of.h                            |   16 ++++++++
 include/linux/shdma-base.h                    |    5 ++
 7 files changed, 196 insertions(+), 23 deletions(-)

-- 
1.7.2.5

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH 1/3] OF: add a new phandle parsing function for grouped nodes

[Guennadi Liakhovetski]

Sometimes it is useful to group similar DT nodes under a common parent,
when a different node can reference the group, meaning, that any subnode
will do the job. An example of such a group is a DMA multiplexer, when a
DMA slave can be served by any DMA controller from the group. This patch
slightly extends an internal __of_parse_phandle_with_args() function to
accept one more optional parameter and exports a new API function
of_parse_phandle_with_child_args(), which allows the caller to provide a
compatibility string for a group. In this case the function returns the
first matching node. Once a matching node is found, standard DT iterators
can be used to look for further matches.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/of/base.c  |   28 +++++++++++++++++++++++++---
 include/linux/of.h |   16 ++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index f53b992..33d4f3a 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1108,11 +1108,16 @@ EXPORT_SYMBOL(of_parse_phandle);
  * @cells_name:	property name that specifies phandles' arguments count
  * @index:	index of a phandle to parse out
  * @out_args:	optional pointer to output arguments structure (will be filled)
+ * @parent:	optional parent-compatibility string
  *
  * This function is useful to parse lists of phandles and their arguments.
  * Returns 0 on success and fills out_args, on error returns appropriate
  * errno value.
  *
+ * If @parent is specified and the DT node, pointed by the currently iterated
+ * phandle is compatible with it, it is assumed, that the phandle is a parent of
+ * DT nodes with equal cell counts. In that case the first child is taken.
+ *
  * Caller is responsible to call of_node_put() on the returned out_args->node
  * pointer.
  *
@@ -1136,7 +1141,8 @@ EXPORT_SYMBOL(of_parse_phandle);
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name, int index,
-					struct of_phandle_args *out_args)
+					struct of_phandle_args *out_args,
+					const char *parent)
 {
 	const __be32 *list, *list_end;
 	int rc = 0, size, cur_index = 0;
@@ -1171,6 +1177,12 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 					 np->full_name);
 				goto err;
 			}
+			if (parent && of_device_is_compatible(node, parent)) {
+				struct device_node *child =
+					of_get_next_available_child(node, NULL);
+				of_node_put(node);
+				node = child;
+			}
 			if (of_property_read_u32(node, cells_name, &count)) {
 				pr_err("%s: could not get %s for %s\n",
 					 np->full_name, cells_name,
@@ -1241,10 +1253,20 @@ int of_parse_phandle_with_args(const struct device_node *np, const char *list_na
 {
 	if (index < 0)
 		return -EINVAL;
-	return __of_parse_phandle_with_args(np, list_name, cells_name, index, out_args);
+	return __of_parse_phandle_with_args(np, list_name, cells_name, index, out_args, NULL);
 }
 EXPORT_SYMBOL(of_parse_phandle_with_args);
 
+int of_parse_phandle_with_child_args(const struct device_node *np, const char *list_name,
+				     const char *cells_name, int index,
+				     struct of_phandle_args *out_args, const char *parent)
+{
+	if (index < 0)
+		return -EINVAL;
+	return __of_parse_phandle_with_args(np, list_name, cells_name, index, out_args, parent);
+}
+EXPORT_SYMBOL(of_parse_phandle_with_child_args);
+
 /**
  * of_count_phandle_with_args() - Find the number of phandles references in a property
  * @np:		pointer to a device tree node containing a list
@@ -1263,7 +1285,7 @@ EXPORT_SYMBOL(of_parse_phandle_with_args);
 int of_count_phandle_with_args(const struct device_node *np, const char *list_name,
 				const char *cells_name)
 {
-	return __of_parse_phandle_with_args(np, list_name, cells_name, -1, NULL);
+	return __of_parse_phandle_with_args(np, list_name, cells_name, -1, NULL, NULL);
 }
 EXPORT_SYMBOL(of_count_phandle_with_args);
 
diff --git a/include/linux/of.h b/include/linux/of.h
index 6e10f8f..d3fdce2 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -280,6 +280,12 @@ extern struct device_node *of_parse_phandle(const struct device_node *np,
 extern int of_parse_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name, int index,
 	struct of_phandle_args *out_args);
+extern int of_parse_phandle_with_child_args(const struct device_node *np,
+					    const char *list_name,
+					    const char *cells_name,
+					    int index,
+					    struct of_phandle_args *out_args,
+					    const char *parent);
 extern int of_count_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name);
 
@@ -488,6 +494,16 @@ static inline int of_parse_phandle_with_args(const struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline int of_parse_phandle_with_child_args(const struct device_node *np,
+						   const char *list_name,
+						   const char *cells_name,
+						   int index,
+						   struct of_phandle_args *out_args,
+						   const char *parent)
+{
+	return -ENOSYS;
+}
+
 static inline int of_count_phandle_with_args(const struct device_node *np,
 					     const char *list_name,
 					     const char *cells_name)
-- 
1.7.2.5

[PATCH 2/3] dmaengine: add support for DMA multiplexer DT nodes

[Guennadi Liakhovetski]

If a slave can use any of several DMA controllers on the system, multiple
DMA descriptors can be listed in its "dmas" DT property with the same
channel name and different DMA controller phandles. However, if multiple
such slaves can use any of a set of DMA controllers on the system, listing
them all in each slave's "dmas" property becomes counterproductive. This
patch adds support for a "dma-mux" DT node, whose sole purpose is to group
such DMA controller DT nodes. Slaves can then specify the group's phandle
in their "dmas" property. DMA controllers, belonging to the same group
must have the same #dma-cells number and use the same slave IDs.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 Documentation/devicetree/bindings/dma/dma.txt |   44 +++++++++++++++++++++++++
 drivers/dma/of-dma.c                          |   31 +++++++++++++----
 2 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/dma.txt b/Documentation/devicetree/bindings/dma/dma.txt
index 8f504e6..a861298 100644
--- a/Documentation/devicetree/bindings/dma/dma.txt
+++ b/Documentation/devicetree/bindings/dma/dma.txt
@@ -31,6 +31,50 @@ Example:
 		dma-requests = <127>;
 	};
 
+* DMA multiplexer
+
+Several DMA controllers with the same #dma-cells number and the same request
+line IDs can be grouped under a DMA multiplexer super-node, if slaves can use
+DMA channels on any of them.
+
+Required property:
+- compatible:		Must include "dma-mux".
+
+Some standard optional properties can be helpful:
+
+Optional properties:
+- compatible:		You will probably also want to include compatibility
+			with "simple-bus" to automatically create platform
+			devices from subnodes.
+- ranges:		Map DMA controller memory areas in the parent address
+			space.
+- #address-cells:	Number of address cells in case automatic propagation
+			with the help of "ranges" doesn't work.
+- #size-cells:		Number of size cells.
+
+Example:
+
+	dmac: dma-mux {
+		compatible = "simple-bus", "dma-mux";
+		ranges;
+
+		dma0: dma@10000000 {
+			#dma-cells = <1>;
+			...
+		};
+
+		dma1: dma@20000000 {
+			#dma-cells = <1>;
+			...
+		};
+	};
+
+	mmc0: mmc@30000000 {
+			dmas = <&dmac 1
+				&dmac 2>;
+			dma-names = "tx", "rx";
+			...
+	};
 
 * DMA client
 
diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 7aa0864..ec58022 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -146,8 +146,8 @@ static int of_dma_match_channel(struct device_node *np, const char *name,
 	if (strcmp(name, s))
 		return -ENODEV;
 
-	if (of_parse_phandle_with_args(np, "dmas", "#dma-cells", index,
-				       dma_spec))
+	if (of_parse_phandle_with_child_args(np, "dmas", "#dma-cells", index,
+					     dma_spec, "dma-mux"))
 		return -ENODEV;
 
 	return 0;
@@ -165,7 +165,7 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 {
 	struct of_phandle_args	dma_spec;
 	struct of_dma		*ofdma;
-	struct dma_chan		*chan;
+	struct dma_chan		*chan = NULL;
 	int			count, i;
 
 	if (!np || !name) {
@@ -180,20 +180,35 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 	}
 
 	for (i = 0; i < count; i++) {
+		struct device_node *dma = NULL, *parent;
+		bool is_mux;
+
 		if (of_dma_match_channel(np, name, i, &dma_spec))
 			continue;
 
 		mutex_lock(&of_dma_lock);
-		ofdma = of_dma_find_controller(&dma_spec);
+		parent = of_node_get(dma_spec.np->parent);
+		is_mux = of_device_is_compatible(parent, "dma-mux");
+
+		do {
+			/* If we're in a mux, try all nodes */
+			if (is_mux) {
+				dma = of_get_next_available_child(parent, dma);
+				if (!dma)
+					break;
+				dma_spec.np = dma;
+			}
+
+			ofdma = of_dma_find_controller(&dma_spec);
 
-		if (ofdma)
-			chan = ofdma->of_dma_xlate(&dma_spec, ofdma);
-		else
-			chan = NULL;
+			if (ofdma)
+				chan = ofdma->of_dma_xlate(&dma_spec, ofdma);
+		} while (dma && !chan);
 
 		mutex_unlock(&of_dma_lock);
 
 		of_node_put(dma_spec.np);
+		of_node_put(parent);
 
 		if (chan)
 			return chan;
-- 
1.7.2.5

[PATCH v2 3/3] DMA: shdma: add DT support

[Guennadi Liakhovetski]

This patch adds Device Tree support to the shdma driver. No special DT
properties are used, only standard DMA DT bindings are implemented. Since
shdma controllers reside on SoCs, their configuration is SoC-specific and
shall be passed to the driver from the SoC platform data, using the
auxdata procedure.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
---

v2: merge DT binding documentation into the patch

 Documentation/devicetree/bindings/dma/shdma.txt |   61 +++++++++++++++++++++++
 drivers/dma/sh/shdma-base.c                     |   51 +++++++++++++++++--
 drivers/dma/sh/shdma.c                          |   44 ++++++++++++++--
 include/linux/shdma-base.h                      |    5 ++
 4 files changed, 149 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/shdma.txt

diff --git a/Documentation/devicetree/bindings/dma/shdma.txt b/Documentation/devicetree/bindings/dma/shdma.txt
new file mode 100644
index 0000000..f99618e
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/shdma.txt
@@ -0,0 +1,61 @@
+* SHDMA Device Tree bindings
+
+Only generic DMA controller bindings are used for SHDMA DT nodes.
+
+* DMA controller
+
+Required properties:
+- compatible:	should be "renesas,shdma"
+- #dma-cells:	should be <1>, see "dmas" property below
+
+Optional properties (currently unused):
+- dma-channels:	number of DMA channels
+- dma-requests:	number of DMA request signals
+
+Example:
+	dma0: shdma@fe000020 {
+		compatible = "renesas,shdma";
+		reg = <0xfe000020 0x89e0>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 129 4
+				0 109 4
+				0 110 4
+				0 111 4
+				0 112 4
+				0 113 4
+				0 114 4
+				0 115 4
+				0 116 4
+				0 117 4
+				0 118 4
+				0 119 4
+				0 120 4
+				0 121 4
+				0 122 4
+				0 123 4
+				0 124 4
+				0 125 4
+				0 126 4
+				0 127 4
+				0 128 4>;
+		interrupt-names = "error",
+				"ch0", "ch1", "ch2", "ch3",
+				"ch4", "ch5", "ch6", "ch7",
+				"ch8", "ch9", "ch10", "ch11",
+				"ch12", "ch13", "ch14", "ch15",
+				"ch16", "ch17", "ch18", "ch19";
+		#dma-cells = <1>;
+		dma-channels = <20>;
+		dma-requests = <256>;
+	};
+
+* DMA client
+
+Required properties:
+- dmas:		a list of <[DMA controller phandle] [MID/RID value]> pairs
+- dma-names:	a list of DMA channel names, one per "dmas" entry
+
+Example:
+	dmas = <&dma0 0xd1
+		&dma0 0xd2>;
+	dma-names = "tx", "rx";
diff --git a/drivers/dma/sh/shdma-base.c b/drivers/dma/sh/shdma-base.c
index 4acb85a..4fd8293 100644
--- a/drivers/dma/sh/shdma-base.c
+++ b/drivers/dma/sh/shdma-base.c
@@ -14,6 +14,8 @@
  */
 
 #include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/of_dma.h>
 #include <linux/shdma-base.h>
 #include <linux/dmaengine.h>
 #include <linux/init.h>
@@ -175,7 +177,18 @@ static int shdma_setup_slave(struct shdma_chan *schan, int slave_id)
 {
 	struct shdma_dev *sdev = to_shdma_dev(schan->dma_chan.device);
 	const struct shdma_ops *ops = sdev->ops;
-	int ret;
+	int ret, match;
+
+	if (schan->dev->of_node) {
+		match = schan->hw_req;
+		ret = ops->set_slave(schan, match, true);
+		if (ret < 0)
+			return ret;
+
+		slave_id = schan->slave_id;
+	} else {
+		match = slave_id;
+	}
 
 	if (slave_id < 0 || slave_id >= slave_num)
 		return -EINVAL;
@@ -183,7 +196,7 @@ static int shdma_setup_slave(struct shdma_chan *schan, int slave_id)
 	if (test_and_set_bit(slave_id, shdma_slave_used))
 		return -EBUSY;
 
-	ret = ops->set_slave(schan, slave_id, false);
+	ret = ops->set_slave(schan, match, false);
 	if (ret < 0) {
 		clear_bit(slave_id, shdma_slave_used);
 		return ret;
@@ -206,23 +219,26 @@ static int shdma_setup_slave(struct shdma_chan *schan, int slave_id)
  * services would have to provide their own filters, which first would check
  * the device driver, similar to how other DMAC drivers, e.g., sa11x0-dma.c, do
  * this, and only then, in case of a match, call this common filter.
+ * NOTE 2: This filter function is also used in the DT case by shdma_xlate().
+ * In that case the MID-RID value is used for slave channel filtering and is
+ * passed to this function in the "arg" parameter.
  */
 bool shdma_chan_filter(struct dma_chan *chan, void *arg)
 {
 	struct shdma_chan *schan = to_shdma_chan(chan);
 	struct shdma_dev *sdev = to_shdma_dev(schan->dma_chan.device);
 	const struct shdma_ops *ops = sdev->ops;
-	int slave_id = (int)arg;
+	int match = (int)arg;
 	int ret;
 
-	if (slave_id < 0)
+	if (match < 0)
 		/* No slave requested - arbitrary channel */
 		return true;
 
-	if (slave_id >= slave_num)
+	if (!schan->dev->of_node && match >= slave_num)
 		return false;
 
-	ret = ops->set_slave(schan, slave_id, true);
+	ret = ops->set_slave(schan, match, true);
 	if (ret < 0)
 		return false;
 
@@ -867,6 +883,29 @@ void shdma_chan_remove(struct shdma_chan *schan)
 }
 EXPORT_SYMBOL(shdma_chan_remove);
 
+struct dma_chan *shdma_xlate(struct of_phandle_args *dma_spec,
+				 struct of_dma *ofdma)
+{
+	struct shdma_dev *sdev = ofdma->of_dma_data;
+	u32 id = dma_spec->args[0];
+	dma_cap_mask_t mask;
+	struct dma_chan *chan;
+
+	if (dma_spec->args_count != 1 || !sdev)
+		return NULL;
+
+	dma_cap_zero(mask);
+	/* Only slave DMA channels can be allocated via DT */
+	dma_cap_set(DMA_SLAVE, mask);
+
+	chan = dma_request_channel(mask, shdma_chan_filter, (void *)id);
+	if (chan)
+		to_shdma_chan(chan)->hw_req = id;
+
+	return chan;
+}
+EXPORT_SYMBOL(shdma_xlate);
+
 int shdma_init(struct device *dev, struct shdma_dev *sdev,
 		    int chan_num)
 {
diff --git a/drivers/dma/sh/shdma.c b/drivers/dma/sh/shdma.c
index a5a1887..db497d6 100644
--- a/drivers/dma/sh/shdma.c
+++ b/drivers/dma/sh/shdma.c
@@ -20,6 +20,8 @@
 
 #include <linux/init.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_dma.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/dmaengine.h>
@@ -301,20 +303,32 @@ static void sh_dmae_setup_xfer(struct shdma_chan *schan,
 	}
 }
 
+/*
+ * Find a slave channel configuration from the contoller list by either a slave
+ * ID in the non-DT case, or by a MID/RID value in the DT case
+ */
 static const struct sh_dmae_slave_config *dmae_find_slave(
-	struct sh_dmae_chan *sh_chan, int slave_id)
+	struct sh_dmae_chan *sh_chan, int match)
 {
 	struct sh_dmae_device *shdev = to_sh_dev(sh_chan);
 	struct sh_dmae_pdata *pdata = shdev->pdata;
 	const struct sh_dmae_slave_config *cfg;
 	int i;
 
-	if (slave_id >= SH_DMA_SLAVE_NUMBER)
-		return NULL;
+	if (!sh_chan->shdma_chan.dev->of_node) {
+		if (match >= SH_DMA_SLAVE_NUMBER)
+			return NULL;
 
-	for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
-		if (cfg->slave_id == slave_id)
-			return cfg;
+		for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
+			if (cfg->slave_id == match)
+				return cfg;
+	} else {
+		for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
+			if (cfg->mid_rid == match) {
+				sh_chan->shdma_chan.slave_id = cfg->slave_id;
+				return cfg;
+			}
+	}
 
 	return NULL;
 }
@@ -841,6 +855,14 @@ static int sh_dmae_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto edmadevreg;
 
+	if (pdev->dev.of_node) {
+		int of_err = of_dma_controller_register(pdev->dev.of_node,
+							shdma_xlate, shdev);
+		if (of_err < 0 && of_err != -ENODEV)
+			dev_err(&pdev->dev,
+				"could not register of_dma_controller\n");
+	}
+
 	return err;
 
 edmadevreg:
@@ -889,6 +911,9 @@ static int sh_dmae_remove(struct platform_device *pdev)
 
 	dma_async_device_unregister(dma_dev);
 
+	if (pdev->dev.of_node)
+		of_dma_controller_free(pdev->dev.of_node);
+
 	if (errirq > 0)
 		free_irq(errirq, shdev);
 
@@ -920,11 +945,18 @@ static int sh_dmae_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id sh_dmae_of_match[] = {
+	{ .compatible = "renesas,shdma", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sh_dmae_of_match);
+
 static struct platform_driver sh_dmae_driver = {
 	.driver 	= {
 		.owner	= THIS_MODULE,
 		.pm	= &sh_dmae_pm,
 		.name	= SH_DMAE_DRV_NAME,
+		.of_match_table = sh_dmae_of_match,
 	},
 	.remove		= sh_dmae_remove,
 	.shutdown	= sh_dmae_shutdown,
diff --git a/include/linux/shdma-base.h b/include/linux/shdma-base.h
index 9a93897..0815a0e 100644
--- a/include/linux/shdma-base.h
+++ b/include/linux/shdma-base.h
@@ -19,6 +19,7 @@
 #include <linux/dmaengine.h>
 #include <linux/interrupt.h>
 #include <linux/list.h>
+#include <linux/of_dma.h>
 #include <linux/types.h>
 
 /**
@@ -68,6 +69,8 @@ struct shdma_chan {
 	int id;				/* Raw id of this channel */
 	int irq;			/* Channel IRQ */
 	int slave_id;			/* Client ID for slave DMA */
+	int hw_req;			/* DMA request line for slave DMA - same
+					 * as MID/RID, used with DT */
 	enum shdma_pm_state pm_state;
 };
 
@@ -123,5 +126,7 @@ int shdma_init(struct device *dev, struct shdma_dev *sdev,
 		    int chan_num);
 void shdma_cleanup(struct shdma_dev *sdev);
 bool shdma_chan_filter(struct dma_chan *chan, void *arg);
+struct dma_chan *shdma_xlate(struct of_phandle_args *dma_spec,
+			     struct of_dma *ofdma);
 
 #endif
-- 
1.7.2.5

Re: [PATCH 0/3] dma: add support for DMA multiplexer DT nodes

[Vinod Koul]

On Thu, Jun 06, 2013 at 05:47:22PM +0200, Guennadi Liakhovetski wrote:
> Next re-send of an earlier work. This series adds support for multiple 
> compatible DMAC instances, capable of serving the same DMA slaves. 
> Currently to specify such a configuration in DT, slaves would have to add 
> DMA tuples for each requested channel for each suitable DMAC. This 
> needlessly clutters the Device Tree. Instead we group such DMAC DT nodes 
> together under a common parent. Then slaves can just reference that parent 
> in their DMA bindings.
> 
> Cc: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
wasnt this series Acked by Anrd previosuly.

Neverthless, Anrd do you mind reviwing this

--
~Vinod
> 
> Guennadi Liakhovetski (3):
>   OF: add a new phandle parsing function for grouped nodes
>   dmaengine: add support for DMA multiplexer DT nodes
>   DMA: shdma: add DT support
> 
>  Documentation/devicetree/bindings/dma/dma.txt |   44 +++++++++++++++++++++
>  drivers/dma/of-dma.c                          |   31 +++++++++++----
>  drivers/dma/sh/shdma-base.c                   |   51 ++++++++++++++++++++++---
>  drivers/dma/sh/shdma.c                        |   44 ++++++++++++++++++---
>  drivers/of/base.c                             |   28 ++++++++++++-
>  include/linux/of.h                            |   16 ++++++++
>  include/linux/shdma-base.h                    |    5 ++
>  7 files changed, 196 insertions(+), 23 deletions(-)
> 
> -- 
> 1.7.2.5
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/

-- 

Re: [PATCH 0/3] dma: add support for DMA multiplexer DT nodes

[Vinod Koul]

On Wed, Jun 12, 2013 at 02:53:49PM +0530, Vinod Koul wrote:
> On Thu, Jun 06, 2013 at 05:47:22PM +0200, Guennadi Liakhovetski wrote:
> > Next re-send of an earlier work. This series adds support for multiple 
> > compatible DMAC instances, capable of serving the same DMA slaves. 
> > Currently to specify such a configuration in DT, slaves would have to add 
> > DMA tuples for each requested channel for each suitable DMAC. This 
> > needlessly clutters the Device Tree. Instead we group such DMAC DT nodes 
> > together under a common parent. Then slaves can just reference that parent 
> > in their DMA bindings.
> > 
> > Cc: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
+ arnd now..

> wasnt this series Acked by Anrd previosuly.
> 
> Neverthless, Anrd do you mind reviwing this
> 
> --
> ~Vinod
> > 
> > Guennadi Liakhovetski (3):
> >   OF: add a new phandle parsing function for grouped nodes
> >   dmaengine: add support for DMA multiplexer DT nodes
> >   DMA: shdma: add DT support
> > 
> >  Documentation/devicetree/bindings/dma/dma.txt |   44 +++++++++++++++++++++
> >  drivers/dma/of-dma.c                          |   31 +++++++++++----
> >  drivers/dma/sh/shdma-base.c                   |   51 ++++++++++++++++++++++---
> >  drivers/dma/sh/shdma.c                        |   44 ++++++++++++++++++---
> >  drivers/of/base.c                             |   28 ++++++++++++-
> >  include/linux/of.h                            |   16 ++++++++
> >  include/linux/shdma-base.h                    |    5 ++
> >  7 files changed, 196 insertions(+), 23 deletions(-)
> > 
> > -- 
> > 1.7.2.5
> > 
> > Thanks
> > Guennadi
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> 
> -- 

-- 

Re: [PATCH 0/3] dma: add support for DMA multiplexer DT nodes

[Guennadi Liakhovetski]

On Wed, 12 Jun 2013, Vinod Koul wrote:

> On Wed, Jun 12, 2013 at 02:53:49PM +0530, Vinod Koul wrote:
> > On Thu, Jun 06, 2013 at 05:47:22PM +0200, Guennadi Liakhovetski wrote:
> > > Next re-send of an earlier work. This series adds support for multiple 
> > > compatible DMAC instances, capable of serving the same DMA slaves. 
> > > Currently to specify such a configuration in DT, slaves would have to add 
> > > DMA tuples for each requested channel for each suitable DMAC. This 
> > > needlessly clutters the Device Tree. Instead we group such DMAC DT nodes 
> > > together under a common parent. Then slaves can just reference that parent 
> > > in their DMA bindings.
> > > 
> > > Cc: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> + arnd now..
> 
> > wasnt this series Acked by Anrd previosuly.

Well, he requested documentation, which I first posted as a separate 
patch, but integrated in the actual DT-adding patch in this version. Back 
then Arnd wrote "With a binding document added, the patch seems ok." which 
kinda is an Ack... :)

Thanks
Guennadi

> > 
> > Neverthless, Anrd do you mind reviwing this
> > 
> > --
> > ~Vinod
> > > 
> > > Guennadi Liakhovetski (3):
> > >   OF: add a new phandle parsing function for grouped nodes
> > >   dmaengine: add support for DMA multiplexer DT nodes
> > >   DMA: shdma: add DT support
> > > 
> > >  Documentation/devicetree/bindings/dma/dma.txt |   44 +++++++++++++++++++++
> > >  drivers/dma/of-dma.c                          |   31 +++++++++++----
> > >  drivers/dma/sh/shdma-base.c                   |   51 ++++++++++++++++++++++---
> > >  drivers/dma/sh/shdma.c                        |   44 ++++++++++++++++++---
> > >  drivers/of/base.c                             |   28 ++++++++++++-
> > >  include/linux/of.h                            |   16 ++++++++
> > >  include/linux/shdma-base.h                    |    5 ++
> > >  7 files changed, 196 insertions(+), 23 deletions(-)
> > > 
> > > -- 
> > > 1.7.2.5
> > > 
> > > Thanks
> > > Guennadi
> > > ---
> > > Guennadi Liakhovetski, Ph.D.
> > > Freelance Open-Source Software Developer
> > > http://www.open-technology.de/
> > 
> > -- 
> 
> -- 
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 0/3] dma: add support for DMA multiplexer DT nodes

[Vinod Koul]

On Wed, Jun 12, 2013 at 12:38:44PM +0200, Guennadi Liakhovetski wrote:
> On Wed, 12 Jun 2013, Vinod Koul wrote:
> 
> > On Wed, Jun 12, 2013 at 02:53:49PM +0530, Vinod Koul wrote:
> > > On Thu, Jun 06, 2013 at 05:47:22PM +0200, Guennadi Liakhovetski wrote:
> > > > Next re-send of an earlier work. This series adds support for multiple 
> > > > compatible DMAC instances, capable of serving the same DMA slaves. 
> > > > Currently to specify such a configuration in DT, slaves would have to add 
> > > > DMA tuples for each requested channel for each suitable DMAC. This 
> > > > needlessly clutters the Device Tree. Instead we group such DMAC DT nodes 
> > > > together under a common parent. Then slaves can just reference that parent 
> > > > in their DMA bindings.
> > > > 
> > > > Cc: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> > + arnd now..
> > 
> > > wasnt this series Acked by Anrd previosuly.
> 
> Well, he requested documentation, which I first posted as a separate 
> patch, but integrated in the actual DT-adding patch in this version. Back 
> then Arnd wrote "With a binding document added, the patch seems ok." which 
> kinda is an Ack... :)
I would prefer an explict ACK from Arnd before applying the DT changes

Arnd, can you pls do the needful

--
~Vinod
> 
> Thanks
> Guennadi
> 
> > > 
> > > Neverthless, Anrd do you mind reviwing this
> > > 
> > > --
> > > ~Vinod
> > > > 
> > > > Guennadi Liakhovetski (3):
> > > >   OF: add a new phandle parsing function for grouped nodes
> > > >   dmaengine: add support for DMA multiplexer DT nodes
> > > >   DMA: shdma: add DT support
> > > > 
> > > >  Documentation/devicetree/bindings/dma/dma.txt |   44 +++++++++++++++++++++
> > > >  drivers/dma/of-dma.c                          |   31 +++++++++++----
> > > >  drivers/dma/sh/shdma-base.c                   |   51 ++++++++++++++++++++++---
> > > >  drivers/dma/sh/shdma.c                        |   44 ++++++++++++++++++---
> > > >  drivers/of/base.c                             |   28 ++++++++++++-
> > > >  include/linux/of.h                            |   16 ++++++++
> > > >  include/linux/shdma-base.h                    |    5 ++
> > > >  7 files changed, 196 insertions(+), 23 deletions(-)
> > > > 
> > > > -- 
> > > > 1.7.2.5
> > > > 
> > > > Thanks
> > > > Guennadi
> > > > ---
> > > > Guennadi Liakhovetski, Ph.D.
> > > > Freelance Open-Source Software Developer
> > > > http://www.open-technology.de/
> > > 
> > > -- 
> > 
> > -- 
> > 
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/

-- 

Re: [PATCH v2 3/3] DMA: shdma: add DT support

[Arnd Bergmann]

On Thursday 06 June 2013, Guennadi Liakhovetski wrote:
> +Required properties:
> +- dmas:		a list of <[DMA controller phandle] [MID/RID value]> pairs
> +- dma-names:	a list of DMA channel names, one per "dmas" entry

Looks ok to me, although it might be helpful to clarify what MID/RID means,
by expanding the acronym and describing whether one needs to pass both
or just one of them. If both, what is the bit pattern?

>   * services would have to provide their own filters, which first would check
>   * the device driver, similar to how other DMAC drivers, e.g., sa11x0-dma.c, do
>   * this, and only then, in case of a match, call this common filter.
> + * NOTE 2: This filter function is also used in the DT case by shdma_xlate().
> + * In that case the MID-RID value is used for slave channel filtering and is
> + * passed to this function in the "arg" parameter.
>   */
>  bool shdma_chan_filter(struct dma_chan *chan, void *arg)
>  {
>  	struct shdma_chan *schan = to_shdma_chan(chan);
>  	struct shdma_dev *sdev = to_shdma_dev(schan->dma_chan.device);
>  	const struct shdma_ops *ops = sdev->ops;
> -	int slave_id = (int)arg;
> +	int match = (int)arg;
>  	int ret;
>  
> -	if (slave_id < 0)
> +	if (match < 0)
>  		/* No slave requested - arbitrary channel */
>  		return true;
>  
> -	if (slave_id >= slave_num)
> +	if (!schan->dev->of_node && match >= slave_num)
>  		return false;
>  
> -	ret = ops->set_slave(schan, slave_id, true);
> +	ret = ops->set_slave(schan, match, true);
>  	if (ret < 0)
>  		return false;

This is complicated by the fact that you are using the same function for
two entirely different purposes. It would be easier to have a separate
filter for the DT case, rather than reusing the one that uses the slave_id
as an argument.

> @@ -867,6 +883,29 @@ void shdma_chan_remove(struct shdma_chan *schan)
>  }
>  EXPORT_SYMBOL(shdma_chan_remove);
>  
> +struct dma_chan *shdma_xlate(struct of_phandle_args *dma_spec,
> +				 struct of_dma *ofdma)
> +{
> +	struct shdma_dev *sdev = ofdma->of_dma_data;
> +	u32 id = dma_spec->args[0];
> +	dma_cap_mask_t mask;
> +	struct dma_chan *chan;
> +
> +	if (dma_spec->args_count != 1 || !sdev)
> +		return NULL;
> +
> +	dma_cap_zero(mask);
> +	/* Only slave DMA channels can be allocated via DT */
> +	dma_cap_set(DMA_SLAVE, mask);
> +
> +	chan = dma_request_channel(mask, shdma_chan_filter, (void *)id);
> +	if (chan)
> +		to_shdma_chan(chan)->hw_req = id;
> +
> +	return chan;
> +}
> +EXPORT_SYMBOL(shdma_xlate);

I would suggest keeping this to the drivers/dma/sh/shdma.c file
and not exporting it. The sudma would use a different binding anyway.

> +/*
> + * Find a slave channel configuration from the contoller list by either a slave
> + * ID in the non-DT case, or by a MID/RID value in the DT case
> + */
>  static const struct sh_dmae_slave_config *dmae_find_slave(
> -	struct sh_dmae_chan *sh_chan, int slave_id)
> +	struct sh_dmae_chan *sh_chan, int match)
>  {
>  	struct sh_dmae_device *shdev = to_sh_dev(sh_chan);
>  	struct sh_dmae_pdata *pdata = shdev->pdata;
>  	const struct sh_dmae_slave_config *cfg;
>  	int i;
>  
> -	if (slave_id >= SH_DMA_SLAVE_NUMBER)
> -		return NULL;
> +	if (!sh_chan->shdma_chan.dev->of_node) {
> +		if (match >= SH_DMA_SLAVE_NUMBER)
> +			return NULL;
>  
> -	for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
> -		if (cfg->slave_id == slave_id)
> -			return cfg;
> +		for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
> +			if (cfg->slave_id == match)
> +				return cfg;
> +	} else {
> +		for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
> +			if (cfg->mid_rid == match) {
> +				sh_chan->shdma_chan.slave_id = cfg->slave_id;
> +				return cfg;
> +			}
> +	}

The pdata and slave_id should really not be needed here for the lookup in the DT
case. Is this just temporary until all slave drivers use dmaenging_slave_config
to pass the address? That should be clarified in a comment.

	Arnd

Re: [PATCH 0/3] dma: add support for DMA multiplexer DT nodes

[Arnd Bergmann]

On Wednesday 12 June 2013, Vinod Koul wrote:
> On Thu, Jun 06, 2013 at 05:47:22PM +0200, Guennadi Liakhovetski wrote:
> > Next re-send of an earlier work. This series adds support for multiple 
> > compatible DMAC instances, capable of serving the same DMA slaves. 
> > Currently to specify such a configuration in DT, slaves would have to add 
> > DMA tuples for each requested channel for each suitable DMAC. This 
> > needlessly clutters the Device Tree. Instead we group such DMAC DT nodes 
> > together under a common parent. Then slaves can just reference that parent 
> > in their DMA bindings.
> > 
> > Cc: Guennadi Liakhovetski <g.liakhovetski+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> wasnt this series Acked by Anrd previosuly.
> 
> Neverthless, Anrd do you mind reviwing this

Sorry about the delay. The binding in patch 3 looks fine, but I have
a few comments about the implementation. Feel free to ignore those
if you think the code is good enough.

Trying to wrap my head around the mux binding now, to see if there
is a better way to do that.

	Arnd

Re: [PATCH 2/3] dmaengine: add support for DMA multiplexer DT nodes

[Arnd Bergmann]

On Thursday 06 June 2013, Guennadi Liakhovetski wrote:
>  Documentation/devicetree/bindings/dma/dma.txt |   44 +++++++++++++++++++++++++
>  drivers/dma/of-dma.c                          |   31 +++++++++++++----
>  2 files changed, 67 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/dma.txt b/Documentation/devicetree/bindings/dma/dma.txt
> index 8f504e6..a861298 100644
> --- a/Documentation/devicetree/bindings/dma/dma.txt
> +++ b/Documentation/devicetree/bindings/dma/dma.txt
> @@ -31,6 +31,50 @@ Example:
>  		dma-requests = <127>;
>  	};
>  
> +* DMA multiplexer
> +
> +Several DMA controllers with the same #dma-cells number and the same request
> +line IDs can be grouped under a DMA multiplexer super-node, if slaves can use
> +DMA channels on any of them.
> +
> +Required property:
> +- compatible:		Must include "dma-mux".
> +
> +Some standard optional properties can be helpful:
> +
> +Optional properties:
> +- compatible:		You will probably also want to include compatibility
> +			with "simple-bus" to automatically create platform
> +			devices from subnodes.
> +- ranges:		Map DMA controller memory areas in the parent address
> +			space.
> +- #address-cells:	Number of address cells in case automatic propagation
> +			with the help of "ranges" doesn't work.
> +- #size-cells:		Number of size cells.
> +
> +Example:
> +
> +	dmac: dma-mux {
> +		compatible = "simple-bus", "dma-mux";
> +		ranges;
> +
> +		dma0: dma@10000000 {
> +			#dma-cells = <1>;
> +			...
> +		};
> +
> +		dma1: dma@20000000 {
> +			#dma-cells = <1>;
> +			...
> +		};
> +	};
> +
> +	mmc0: mmc@30000000 {
> +			dmas = <&dmac 1
> +				&dmac 2>;
> +			dma-names = "tx", "rx";
> +			...
> +	};
>  
>  * DMA client

Hmm, you've clearly shown that this can work, but it feels like a really odd way to
do this. I don't think we should do it this way, because it tries to be really
generic but then cannot some of the interesting cases, e.g.

1. you have multiplexed dma engines, but they need different #dma-cells.
1. you have multiplexed dma engines, but they need different dma specifiers.
2. The mux is between devices that are not on the same parent bus.

I think this should either not try to be fully generic and just limited to
the case you care about, i.e. shdma, or it should be more abstract and
multiplex between the individual channels. In either case, I guess
it would not need a change like this to the of_dma_request_slave_channel()
function, and the node pointer used by the slave would be the same node
that defines the address space for the channel descriptions with #dma-cells.

I think the easiest way would be the first of those two, so it would
essentially look like 

	dmac: dma-mux {
		compatible = "renesas,shdma-mux"; /* not simple-bus! */
		#dma-cells = <1>;
		#address-cells = <1>;
		#size-cells = <1>;
	
		dma@10000000 {
			compatible = "renesas,shdma";
			reg = <0x10000000 0x1000>;
			interrupts = <10>;
		};
		dma@20000000 {
			compatible = "renesas,shdma";
			reg = <0x10000000 0x1000>;
			interrupts = <10>;
		};
	}

You then register a device driver for the master device, which
will call of_dma_controller_register() for itself and create
its child devices.

	Arnd

Re: [PATCH v2 3/3] DMA: shdma: add DT support

[Guennadi Liakhovetski]

Hi Arnd

On Mon, 17 Jun 2013, Arnd Bergmann wrote:

> On Thursday 06 June 2013, Guennadi Liakhovetski wrote:
> > +Required properties:
> > +- dmas:		a list of <[DMA controller phandle] [MID/RID value]> pairs
> > +- dma-names:	a list of DMA channel names, one per "dmas" entry
> 
> Looks ok to me, although it might be helpful to clarify what MID/RID means,
> by expanding the acronym and describing whether one needs to pass both
> or just one of them. If both, what is the bit pattern?

One word: magic. MID/RID is what that value is called in the datasheet. 
E.g. on APE6 (r8a73a4) it is indeed divided into 2 fields - 2 and 6 bits 
for RID and MID respectively, I _guess_ 2 bits are to select a channel 
within a slave device and 6 bits to pick up one of slaves, but that 
doesn't fit with a slave with 8 channels (HSI), there are also slave 
devices with different MID values, so, those values are really better 
considered as a single magic value - an 8-bit channel request handle, 
which is also how they are listed in a reference table.

> >   * services would have to provide their own filters, which first would check
> >   * the device driver, similar to how other DMAC drivers, e.g., sa11x0-dma.c, do
> >   * this, and only then, in case of a match, call this common filter.
> > + * NOTE 2: This filter function is also used in the DT case by shdma_xlate().
> > + * In that case the MID-RID value is used for slave channel filtering and is
> > + * passed to this function in the "arg" parameter.
> >   */
> >  bool shdma_chan_filter(struct dma_chan *chan, void *arg)
> >  {
> >  	struct shdma_chan *schan = to_shdma_chan(chan);
> >  	struct shdma_dev *sdev = to_shdma_dev(schan->dma_chan.device);
> >  	const struct shdma_ops *ops = sdev->ops;
> > -	int slave_id = (int)arg;
> > +	int match = (int)arg;
> >  	int ret;
> >  
> > -	if (slave_id < 0)
> > +	if (match < 0)
> >  		/* No slave requested - arbitrary channel */
> >  		return true;
> >  
> > -	if (slave_id >= slave_num)
> > +	if (!schan->dev->of_node && match >= slave_num)
> >  		return false;
> >  
> > -	ret = ops->set_slave(schan, slave_id, true);
> > +	ret = ops->set_slave(schan, match, true);
> >  	if (ret < 0)
> >  		return false;
> 
> This is complicated by the fact that you are using the same function for
> two entirely different purposes. It would be easier to have a separate
> filter for the DT case, rather than reusing the one that uses the slave_id
> as an argument.

Hm, yes, I was considering either making 2 functions or reusing one, but 
it's really the same code with only difference - the DT version wouldn't 
have the "> slave_num" check. So, I decided to use a single function 
renaming "slave_id" to a neutral "match." You really think it's become too 
complex and should be copied for clarity?

> > @@ -867,6 +883,29 @@ void shdma_chan_remove(struct shdma_chan *schan)
> >  }
> >  EXPORT_SYMBOL(shdma_chan_remove);
> >  
> > +struct dma_chan *shdma_xlate(struct of_phandle_args *dma_spec,
> > +				 struct of_dma *ofdma)
> > +{
> > +	struct shdma_dev *sdev = ofdma->of_dma_data;
> > +	u32 id = dma_spec->args[0];
> > +	dma_cap_mask_t mask;
> > +	struct dma_chan *chan;
> > +
> > +	if (dma_spec->args_count != 1 || !sdev)
> > +		return NULL;
> > +
> > +	dma_cap_zero(mask);
> > +	/* Only slave DMA channels can be allocated via DT */
> > +	dma_cap_set(DMA_SLAVE, mask);
> > +
> > +	chan = dma_request_channel(mask, shdma_chan_filter, (void *)id);
> > +	if (chan)
> > +		to_shdma_chan(chan)->hw_req = id;
> > +
> > +	return chan;
> > +}
> > +EXPORT_SYMBOL(shdma_xlate);
> 
> I would suggest keeping this to the drivers/dma/sh/shdma.c file
> and not exporting it. The sudma would use a different binding anyway.

Ok, can do that and then see, how different sudma's .xlate() function will 
be. If it's the same we can make this common again.

> > +/*
> > + * Find a slave channel configuration from the contoller list by either a slave
> > + * ID in the non-DT case, or by a MID/RID value in the DT case
> > + */
> >  static const struct sh_dmae_slave_config *dmae_find_slave(
> > -	struct sh_dmae_chan *sh_chan, int slave_id)
> > +	struct sh_dmae_chan *sh_chan, int match)
> >  {
> >  	struct sh_dmae_device *shdev = to_sh_dev(sh_chan);
> >  	struct sh_dmae_pdata *pdata = shdev->pdata;
> >  	const struct sh_dmae_slave_config *cfg;
> >  	int i;
> >  
> > -	if (slave_id >= SH_DMA_SLAVE_NUMBER)
> > -		return NULL;
> > +	if (!sh_chan->shdma_chan.dev->of_node) {
> > +		if (match >= SH_DMA_SLAVE_NUMBER)
> > +			return NULL;
> >  
> > -	for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
> > -		if (cfg->slave_id == slave_id)
> > -			return cfg;
> > +		for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
> > +			if (cfg->slave_id == match)
> > +				return cfg;
> > +	} else {
> > +		for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
> > +			if (cfg->mid_rid == match) {
> > +				sh_chan->shdma_chan.slave_id = cfg->slave_id;
> > +				return cfg;
> > +			}
> > +	}
> 
> The pdata and slave_id should really not be needed here for the lookup in the DT
> case. Is this just temporary until all slave drivers use dmaenging_slave_config
> to pass the address? That should be clarified in a comment.

Also with DT we still use platform data, passed to the driver using 
auxdata. This function finds a suitable struct sh_dmae_slave_config 
channel configuration entry in the platform data and returns it to the 
caller. I don't think this can be avoided without carrying all the 
platform data over to DT.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/3] dmaengine: add support for DMA multiplexer DT nodes

[Guennadi Liakhovetski]

Hi Arnd

Thanks for your comments

On Mon, 17 Jun 2013, Arnd Bergmann wrote:

> On Thursday 06 June 2013, Guennadi Liakhovetski wrote:
> >  Documentation/devicetree/bindings/dma/dma.txt |   44 +++++++++++++++++++++++++
> >  drivers/dma/of-dma.c                          |   31 +++++++++++++----
> >  2 files changed, 67 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/dma.txt b/Documentation/devicetree/bindings/dma/dma.txt
> > index 8f504e6..a861298 100644
> > --- a/Documentation/devicetree/bindings/dma/dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/dma.txt
> > @@ -31,6 +31,50 @@ Example:
> >  		dma-requests = <127>;
> >  	};
> >  
> > +* DMA multiplexer
> > +
> > +Several DMA controllers with the same #dma-cells number and the same request
> > +line IDs can be grouped under a DMA multiplexer super-node, if slaves can use
> > +DMA channels on any of them.
> > +
> > +Required property:
> > +- compatible:		Must include "dma-mux".
> > +
> > +Some standard optional properties can be helpful:
> > +
> > +Optional properties:
> > +- compatible:		You will probably also want to include compatibility
> > +			with "simple-bus" to automatically create platform
> > +			devices from subnodes.
> > +- ranges:		Map DMA controller memory areas in the parent address
> > +			space.
> > +- #address-cells:	Number of address cells in case automatic propagation
> > +			with the help of "ranges" doesn't work.
> > +- #size-cells:		Number of size cells.
> > +
> > +Example:
> > +
> > +	dmac: dma-mux {
> > +		compatible = "simple-bus", "dma-mux";
> > +		ranges;
> > +
> > +		dma0: dma@10000000 {
> > +			#dma-cells = <1>;
> > +			...
> > +		};
> > +
> > +		dma1: dma@20000000 {
> > +			#dma-cells = <1>;
> > +			...
> > +		};
> > +	};
> > +
> > +	mmc0: mmc@30000000 {
> > +			dmas = <&dmac 1
> > +				&dmac 2>;
> > +			dma-names = "tx", "rx";
> > +			...
> > +	};
> >  
> >  * DMA client
> 
> Hmm, you've clearly shown that this can work, but it feels like a really odd way to
> do this. I don't think we should do it this way, because it tries to be really
> generic but then cannot some of the interesting cases, e.g.
> 
> 1. you have multiplexed dma engines, but they need different #dma-cells.
> 1. you have multiplexed dma engines, but they need different dma specifiers.
> 2. The mux is between devices that are not on the same parent bus.

Yes, I know about these restrictions. I'm not sure whether we really ever 
will get DMA multiplexers with different #dma-cells or DMA specifiers, but 
in any case we can make this less generic - either keep it as a generic 
"uniform multiplexer" or making it shdma specific - I'm fine either way.

> I think this should either not try to be fully generic and just limited to
> the case you care about, i.e. shdma, or it should be more abstract and
> multiplex between the individual channels. In either case, I guess
> it would not need a change like this to the of_dma_request_slave_channel()
> function, and the node pointer used by the slave would be the same node
> that defines the address space for the channel descriptions with #dma-cells.
> 
> I think the easiest way would be the first of those two, so it would
> essentially look like 
> 
> 	dmac: dma-mux {
> 		compatible = "renesas,shdma-mux"; /* not simple-bus! */
> 		#dma-cells = <1>;
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 	
> 		dma@10000000 {
> 			compatible = "renesas,shdma";
> 			reg = <0x10000000 0x1000>;
> 			interrupts = <10>;
> 		};
> 		dma@20000000 {
> 			compatible = "renesas,shdma";
> 			reg = <0x10000000 0x1000>;
> 			interrupts = <10>;
> 		};
> 	}
> 
> You then register a device driver for the master device, which
> will call of_dma_controller_register() for itself and create
> its child devices.

Hmm, it is an interesting idea to only register one struct of_dma instance 
for all compatible shdma instances instead of one per shdma controller, 
and then call of_platform_populate() to instantiate all shdma instances. A 
couple of drawbacks:

1. we'll always have to put a mux DT node in .dts, even if there's just 
one DMAC instance on the system.

2. as AUXDATA for the new wrapper device we'll have to use an AUXDATA 
array for all child nodes, to be passed to of_platform_populate().

3. it seems confusing to me - having one ofdma instance for multiple 
dmaengine devices.

The advantage is, of course, that we don't need to extend existing OF and 
dmaengine APIs.

So, I think, it can be done this way, but do you really think it'd be an 
improvement over my original proposal?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/3] dmaengine: add support for DMA multiplexer DT nodes

[Arnd Bergmann]

On Tuesday 18 June 2013, Guennadi Liakhovetski wrote:
> > 
> > Hmm, you've clearly shown that this can work, but it feels like a really odd way to
> > do this. I don't think we should do it this way, because it tries to be really
> > generic but then cannot some of the interesting cases, e.g.
> > 
> > 1. you have multiplexed dma engines, but they need different #dma-cells.
> > 1. you have multiplexed dma engines, but they need different dma specifiers.
> > 2. The mux is between devices that are not on the same parent bus.
> 
> Yes, I know about these restrictions. I'm not sure whether we really ever 
> will get DMA multiplexers with different #dma-cells or DMA specifiers, but 
> in any case we can make this less generic - either keep it as a generic 
> "uniform multiplexer" or making it shdma specific - I'm fine either way.

Ok, let's make it shdma specific then.

> > I think this should either not try to be fully generic and just limited to
> > the case you care about, i.e. shdma, or it should be more abstract and
> > multiplex between the individual channels. In either case, I guess
> > it would not need a change like this to the of_dma_request_slave_channel()
> > function, and the node pointer used by the slave would be the same node
> > that defines the address space for the channel descriptions with #dma-cells.
> > 
> > I think the easiest way would be the first of those two, so it would
> > essentially look like 
> > 
> > 	dmac: dma-mux {
> > 		compatible = "renesas,shdma-mux"; /* not simple-bus! */
> > 		#dma-cells = <1>;
> > 		#address-cells = <1>;
> > 		#size-cells = <1>;
> > 	
> > 		dma@10000000 {
> > 			compatible = "renesas,shdma";
> > 			reg = <0x10000000 0x1000>;
> > 			interrupts = <10>;
> > 		};
> > 		dma@20000000 {
> > 			compatible = "renesas,shdma";
> > 			reg = <0x10000000 0x1000>;
> > 			interrupts = <10>;
> > 		};
> > 	}
> > 
> > You then register a device driver for the master device, which
> > will call of_dma_controller_register() for itself and create
> > its child devices.
> 
> Hmm, it is an interesting idea to only register one struct of_dma instance 
> for all compatible shdma instances instead of one per shdma controller, 
> and then call of_platform_populate() to instantiate all shdma instances. A 
> couple of drawbacks:
> 
> 1. we'll always have to put a mux DT node in .dts, even if there's just 
> one DMAC instance on the system.
>
> 2. as AUXDATA for the new wrapper device we'll have to use an AUXDATA 
> array for all child nodes, to be passed to of_platform_populate().

My suggestion above is just one of the possible ways to do it, and I'm
less concerned about it if you make it specific to shdma. Other
options would be:

1 using phandles from the mux to the individual dma engine instances,
  but have them as independent nodes.
2 same as 1, but using phandles from the individual instances to the mux
3 Keep using the simple-bus.
4 Have just one combined device node for all shdma instances, with multiple
  'reg' and 'interrupts' properties, and handle the muxing in the shdma
  driver. You could create platform_device_create_resndata from the
  top-level driver to reuse most of the existing code.

In any of these cases you should be able to support both muxed and non-muxed
operation in the shdma driver if you want to. You'd just have two separate
ofdma registrations.

> 3. it seems confusing to me - having one ofdma instance for multiple 
> dmaengine devices.

I don't see a better way around this one, but I also don't see it as problematic.
Wiht a mux, you always end up having just one node that the slaves
point to, but multiple dma_device structures in the kernel. struct ofdma
really refers to the first one.

> The advantage is, of course, that we don't need to extend existing OF and 
> dmaengine APIs.
> 
> So, I think, it can be done this way, but do you really think it'd be an 
> improvement over my original proposal?

My main interest is to keep the shdma specifics out of the generic dmaengine
binding document, where it just complicates the common case.

	Arnd

Re: [PATCH v2 3/3] DMA: shdma: add DT support

[Arnd Bergmann]

On Tuesday 18 June 2013, Guennadi Liakhovetski wrote:
> Hi Arnd
> 
> On Mon, 17 Jun 2013, Arnd Bergmann wrote:
> 
> > On Thursday 06 June 2013, Guennadi Liakhovetski wrote:
> > > +Required properties:
> > > +- dmas:		a list of <[DMA controller phandle] [MID/RID value]> pairs
> > > +- dma-names:	a list of DMA channel names, one per "dmas" entry
> > 
> > Looks ok to me, although it might be helpful to clarify what MID/RID means,
> > by expanding the acronym and describing whether one needs to pass both
> > or just one of them. If both, what is the bit pattern?
> 
> One word: magic. MID/RID is what that value is called in the datasheet. 
> E.g. on APE6 (r8a73a4) it is indeed divided into 2 fields - 2 and 6 bits 
> for RID and MID respectively, I _guess_ 2 bits are to select a channel 
> within a slave device and 6 bits to pick up one of slaves, but that 
> doesn't fit with a slave with 8 channels (HSI), there are also slave 
> devices with different MID values, so, those values are really better 
> considered as a single magic value - an 8-bit channel request handle, 
> which is also how they are listed in a reference table.

Ok.

> > >   * services would have to provide their own filters, which first would check
> > >   * the device driver, similar to how other DMAC drivers, e.g., sa11x0-dma.c, do
> > >   * this, and only then, in case of a match, call this common filter.
> > > + * NOTE 2: This filter function is also used in the DT case by shdma_xlate().
> > > + * In that case the MID-RID value is used for slave channel filtering and is
> > > + * passed to this function in the "arg" parameter.
> > >   */
> > >  bool shdma_chan_filter(struct dma_chan *chan, void *arg)
> > >  {
> > >  	struct shdma_chan *schan = to_shdma_chan(chan);
> > >  	struct shdma_dev *sdev = to_shdma_dev(schan->dma_chan.device);
> > >  	const struct shdma_ops *ops = sdev->ops;
> > > -	int slave_id = (int)arg;
> > > +	int match = (int)arg;
> > >  	int ret;
> > >  
> > > -	if (slave_id < 0)
> > > +	if (match < 0)
> > >  		/* No slave requested - arbitrary channel */
> > >  		return true;
> > >  
> > > -	if (slave_id >= slave_num)
> > > +	if (!schan->dev->of_node && match >= slave_num)
> > >  		return false;
> > >  
> > > -	ret = ops->set_slave(schan, slave_id, true);
> > > +	ret = ops->set_slave(schan, match, true);
> > >  	if (ret < 0)
> > >  		return false;
> > 
> > This is complicated by the fact that you are using the same function for
> > two entirely different purposes. It would be easier to have a separate
> > filter for the DT case, rather than reusing the one that uses the slave_id
> > as an argument.
> 
> Hm, yes, I was considering either making 2 functions or reusing one, but 
> it's really the same code with only difference - the DT version wouldn't 
> have the "> slave_num" check. So, I decided to use a single function 
> renaming "slave_id" to a neutral "match." You really think it's become too 
> complex and should be copied for clarity?

I think it would be easier to understand if you have two functions, but
it's not very important. Especially if you make the new function specific
to shdma, that would be clearer.

> > > @@ -867,6 +883,29 @@ void shdma_chan_remove(struct shdma_chan *schan)
> > >  }
> > >  EXPORT_SYMBOL(shdma_chan_remove);
> > >  
> > > +struct dma_chan *shdma_xlate(struct of_phandle_args *dma_spec,
> > > +				 struct of_dma *ofdma)
> > > +{
> > > +	struct shdma_dev *sdev = ofdma->of_dma_data;
> > > +	u32 id = dma_spec->args[0];
> > > +	dma_cap_mask_t mask;
> > > +	struct dma_chan *chan;
> > > +
> > > +	if (dma_spec->args_count != 1 || !sdev)
> > > +		return NULL;
> > > +
> > > +	dma_cap_zero(mask);
> > > +	/* Only slave DMA channels can be allocated via DT */
> > > +	dma_cap_set(DMA_SLAVE, mask);
> > > +
> > > +	chan = dma_request_channel(mask, shdma_chan_filter, (void *)id);
> > > +	if (chan)
> > > +		to_shdma_chan(chan)->hw_req = id;
> > > +
> > > +	return chan;
> > > +}
> > > +EXPORT_SYMBOL(shdma_xlate);
> > 
> > I would suggest keeping this to the drivers/dma/sh/shdma.c file
> > and not exporting it. The sudma would use a different binding anyway.
> 
> Ok, can do that and then see, how different sudma's .xlate() function will 
> be. If it's the same we can make this common again.

I hope we can get rid of the need for calling dma_request_channel() from
xlate soon, since that is a bit silly anyway. Ideally you would just pick
the right channel of the dma_device (or the first free one, depending on
the driver) and return that from xlate.

> > The pdata and slave_id should really not be needed here for the lookup in the DT
> > case. Is this just temporary until all slave drivers use dmaenging_slave_config
> > to pass the address? That should be clarified in a comment.
> 
> Also with DT we still use platform data, passed to the driver using 
> auxdata. This function finds a suitable struct sh_dmae_slave_config 
> channel configuration entry in the platform data and returns it to the 
> caller. I don't think this can be avoided without carrying all the 
> platform data over to DT.

I think that should be done anyway. A lot of the data can just be hardcoded
in the driver based on the specific "compatible" value, e.g. the register offsets
of the individual channels.

The list of slaves that is currently passed in platform data should not be needed,
but I realize that moving the FIFO addresses into the drivers is work in progress.

	Arnd