[PATCH RFC v6 0/5] MPC512x DMA slave s/g support, OF DMA lookup

[PATCH RFC v6 0/5] MPC512x DMA slave s/g support, OF DMA lookup

[Alexander Popov]

v2013/7/14 Gerhard Sittig <gsi@denx.de>:
> this series
> - introduces slave s/g support (that's support for DMA transfers which
>    involve peripherals in contrast to mem-to-mem transfers)
> - adds device tree based lookup support for DMA channels
> - combines floating patches and related feedback which already covered
>    several aspects of what the suggested LPB driver needs, to demonstrate
>    how integration might be done
> - carries Q&D SD card support to enable another DMA client during test,
>    while this patch needs to get dropped upon pickup

Changes in v2:
> - re-order mpc8308 related code paths for improved readability, no
>    change in behaviour, introduction of symbolic channel names here
>    already
> - squash 'execute() start condition' and 'terminate all' into the
>    introduction of 'slave s/g prep' and 'device control' support; refuse
>    s/g lists with more than one item since slave support is operational
>    yet proper s/g support is missing (can get addressed later)
> - always start transfers from software on MPC8308 as there are no
>    external request lines for peripheral flow control
> - drop dt-bindings header file and symbolic channel names in OF nodes

Changes in v3 and v4:
 Part 1/5:
 - use #define instead of enum since individual channels don't require
    special handling.
 Part 2/5:
 - add a flag "will_access_peripheral" to DMA transfer descriptor
    according recommendations of Gerhard Sittig.
    This flag is set in mpc_dma_prep_memcpy() and mpc_dma_prep_slave_sg()
    and is evaluated in mpc_dma_execute() to choose a type of start for
    the transfer.
 - prevent descriptors of transfers which involve peripherals from
    being chained together;
    each of such transfers needs hardware initiated start.
 - add locking while working with struct mpc_dma_chan
    according recommendations of Lars-Peter Clausen.
 - remove default nbytes value. Client kernel modules must set
    src_maxburst and dst_maxburst fields of struct dma_slave_config (dmaengine.h).

Changes in v5:
 Part 2/5:
 - add and improve comments;
 - improve the code moving transfer descriptors from 'queued' to 'active' list
    in mpc_dma_execute();
 - allow mpc_dma_prep_slave_sg() to run with non-empty 'active' list;
 - take 'mdesc' back to 'free' list in case of error in mpc_dma_prep_slave_sg();
 - improve checks of the transfer parameters;
 - provide the default value for 'maxburst' in mpc_dma_device_control().

Changes in v6:
 Part 2/5:
 - remove doubtful comment;
 - fix coding style issues;
 - set default value for 'maxburst' to 1 which applies to most cases;
 Part 3/5:
 - use dma_get_slave_channel() instead of dma_request_channel()
    in new function of_dma_xlate_by_chan_id() according recommendations of
    Arnd Bergmann;
 Part 4/5:
 - set DMA_PRIVATE flag for MPC512x DMA controller since its driver relies on
    of_dma_xlate_by_chan_id() which doesn't use dma_request_channel()
    any more;
 - resolve little patch conflict;
 Part 5/5:
 - resolve little patch conflict;

> known issues:
> - it's yet to get confirmed whether MPC8308 can use slave support or
>    whether the DMA controller's driver shall actively reject it, the
>    information that's available so far suggests that peripheral transfers
>    to IP bus attached I/O is useful and shall not get blocked right away
 - adding support for transfers which don't increment the RAM address or
    do increment the peripheral "port's" address is easy with
    this implementation; but which options of the common API
    should be used for specifying such transfers?


Alexander Popov (3):
  dma: mpc512x: reorder mpc8308 specific instructions
  dma: mpc512x: add support for peripheral transfers
  dma: of: Add common xlate function for matching by channel id

Gerhard Sittig (2):
  dma: mpc512x: register for device tree channel lookup
  HACK mmc: mxcmmc: enable clocks for the MPC512x

 .../devicetree/bindings/dma/mpc512x-dma.txt        |  55 ++++
 arch/powerpc/boot/dts/mpc5121.dtsi                 |   1 +
 drivers/dma/mpc512x_dma.c                          | 294 +++++++++++++++++++--
 drivers/dma/of-dma.c                               |  35 +++
 drivers/mmc/host/mxcmmc.c                          |  42 ++-
 include/linux/of_dma.h                             |   4 +
 6 files changed, 392 insertions(+), 39 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/mpc512x-dma.txt

-- 
1.8.4.2

[PATCH RFC v6 1/5] dma: mpc512x: reorder mpc8308 specific instructions

[Alexander Popov]

Concentrate the specific code for MPC8308 in the 'if' branch
and handle MPC512x in the 'else' branch.
This modification only reorders instructions but doesn't change behaviour.

Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
---
 drivers/dma/mpc512x_dma.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index 448750d..2ce248b 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -52,9 +52,17 @@
 #define MPC_DMA_DESCRIPTORS	64
 
 /* Macro definitions */
-#define MPC_DMA_CHANNELS	64
 #define MPC_DMA_TCD_OFFSET	0x1000
 
+/*
+ * Maximum channel counts for individual hardware variants
+ * and the maximum channel count over all supported controllers,
+ * used for data structure size
+ */
+#define MPC8308_DMACHAN_MAX	16
+#define MPC512x_DMACHAN_MAX	64
+#define MPC_DMA_CHANNELS	64
+
 /* Arbitration mode of group and channel */
 #define MPC_DMA_DMACR_EDCG	(1 << 31)
 #define MPC_DMA_DMACR_ERGA	(1 << 3)
@@ -710,10 +718,10 @@ static int mpc_dma_probe(struct platform_device *op)
 
 	dma = &mdma->dma;
 	dma->dev = dev;
-	if (!mdma->is_mpc8308)
-		dma->chancnt = MPC_DMA_CHANNELS;
+	if (mdma->is_mpc8308)
+		dma->chancnt = MPC8308_DMACHAN_MAX;
 	else
-		dma->chancnt = 16; /* MPC8308 DMA has only 16 channels */
+		dma->chancnt = MPC512x_DMACHAN_MAX;
 	dma->device_alloc_chan_resources = mpc_dma_alloc_chan_resources;
 	dma->device_free_chan_resources = mpc_dma_free_chan_resources;
 	dma->device_issue_pending = mpc_dma_issue_pending;
@@ -747,7 +755,19 @@ static int mpc_dma_probe(struct platform_device *op)
 	 * - Round-robin group arbitration,
 	 * - Round-robin channel arbitration.
 	 */
-	if (!mdma->is_mpc8308) {
+	if (mdma->is_mpc8308) {
+		/* MPC8308 has 16 channels and lacks some registers */
+		out_be32(&mdma->regs->dmacr, MPC_DMA_DMACR_ERCA);
+
+		/* enable snooping */
+		out_be32(&mdma->regs->dmagpor, MPC_DMA_DMAGPOR_SNOOP_ENABLE);
+		/* Disable error interrupts */
+		out_be32(&mdma->regs->dmaeeil, 0);
+
+		/* Clear interrupts status */
+		out_be32(&mdma->regs->dmaintl, 0xFFFF);
+		out_be32(&mdma->regs->dmaerrl, 0xFFFF);
+	} else {
 		out_be32(&mdma->regs->dmacr, MPC_DMA_DMACR_EDCG |
 					MPC_DMA_DMACR_ERGA | MPC_DMA_DMACR_ERCA);
 
@@ -768,18 +788,6 @@ static int mpc_dma_probe(struct platform_device *op)
 		/* Route interrupts to IPIC */
 		out_be32(&mdma->regs->dmaihsa, 0);
 		out_be32(&mdma->regs->dmailsa, 0);
-	} else {
-		/* MPC8308 has 16 channels and lacks some registers */
-		out_be32(&mdma->regs->dmacr, MPC_DMA_DMACR_ERCA);
-
-		/* enable snooping */
-		out_be32(&mdma->regs->dmagpor, MPC_DMA_DMAGPOR_SNOOP_ENABLE);
-		/* Disable error interrupts */
-		out_be32(&mdma->regs->dmaeeil, 0);
-
-		/* Clear interrupts status */
-		out_be32(&mdma->regs->dmaintl, 0xFFFF);
-		out_be32(&mdma->regs->dmaerrl, 0xFFFF);
 	}
 
 	/* Register DMA engine */
-- 
1.8.4.2

[PATCH RFC v6 2/5] dma: mpc512x: add support for peripheral transfers

[Alexander Popov]

Introduce support for slave s/g transfer preparation and the associated
device control callback in the MPC512x DMA controller driver, which adds
support for data transfers between memory and peripheral I/O to the
previously supported mem-to-mem transfers.

Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
---
 drivers/dma/mpc512x_dma.c | 230 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 225 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index 2ce248b..a7e7749 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -2,6 +2,7 @@
  * Copyright (C) Freescale Semicondutor, Inc. 2007, 2008.
  * Copyright (C) Semihalf 2009
  * Copyright (C) Ilya Yanok, Emcraft Systems 2010
+ * Copyright (C) Alexander Popov, Promcontroller 2013
  *
  * Written by Piotr Ziecik <kosmo@semihalf.com>. Hardware description
  * (defines, structures and comments) was taken from MPC5121 DMA driver
@@ -29,8 +30,15 @@
  */
 
 /*
- * This is initial version of MPC5121 DMA driver. Only memory to memory
- * transfers are supported (tested using dmatest module).
+ * This version of MPC5121 DMA driver supports
+ * memory to memory data transfers (tested using dmatest module) and
+ * data transfers between memory and peripheral I/O memory
+ * by means of slave s/g with these limitations:
+ * - chunked transfers (transfers with more than one part) are refused
+ * as long as proper support for scatter/gather is missing;
+ * - transfers on MPC8308 always start from software as this SoC appears
+ * not to have external request lines for peripheral flow control;
+ * - minimal memory <-> I/O memory transfer size is 4 bytes.
  */
 
 #include <linux/module.h>
@@ -189,6 +197,7 @@ struct mpc_dma_desc {
 	dma_addr_t			tcd_paddr;
 	int				error;
 	struct list_head		node;
+	int				will_access_peripheral;
 };
 
 struct mpc_dma_chan {
@@ -201,6 +210,10 @@ struct mpc_dma_chan {
 	struct mpc_dma_tcd		*tcd;
 	dma_addr_t			tcd_paddr;
 
+	/* Settings for access to peripheral FIFO */
+	dma_addr_t			per_paddr;	/* FIFO address */
+	u32				tcd_nunits;
+
 	/* Lock for this structure */
 	spinlock_t			lock;
 };
@@ -251,8 +264,21 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
 	struct mpc_dma_desc *mdesc;
 	int cid = mchan->chan.chan_id;
 
-	/* Move all queued descriptors to active list */
-	list_splice_tail_init(&mchan->queued, &mchan->active);
+	while (!list_empty(&mchan->queued)) {
+		mdesc = list_first_entry(&mchan->queued,
+						struct mpc_dma_desc, node);
+
+		/* Grab either several mem-to-mem transfer descriptors
+		 * or one peripheral transfer descriptor,
+		 * don't mix mem-to-mem and peripheral transfer descriptors
+		 * within the same 'active' list. */
+		if (mdesc->will_access_peripheral) {
+			if (list_empty(&mchan->active))
+				list_move_tail(&mdesc->node, &mchan->active);
+			break;
+		} else
+			list_move_tail(&mdesc->node, &mchan->active);
+	}
 
 	/* Chain descriptors into one transaction */
 	list_for_each_entry(mdesc, &mchan->active, node) {
@@ -278,7 +304,17 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
 
 	if (first != prev)
 		mdma->tcd[cid].e_sg = 1;
-	out_8(&mdma->regs->dmassrt, cid);
+
+	if (mdma->is_mpc8308) {
+		/* MPC8308, no request lines, software initiated start */
+		out_8(&mdma->regs->dmassrt, cid);
+	} else if (first->will_access_peripheral) {
+		/* peripherals involved, start by external request signal */
+		out_8(&mdma->regs->dmaserq, cid);
+	} else {
+		/* memory to memory transfer, software initiated start */
+		out_8(&mdma->regs->dmassrt, cid);
+	}
 }
 
 /* Handle interrupt on one half of DMA controller (32 channels) */
@@ -596,6 +632,7 @@ mpc_dma_prep_memcpy(struct dma_chan *chan, dma_addr_t dst, dma_addr_t src,
 	}
 
 	mdesc->error = 0;
+	mdesc->will_access_peripheral = 0;
 	tcd = mdesc->tcd;
 
 	/* Prepare Transfer Control Descriptor for this transaction */
@@ -643,6 +680,186 @@ mpc_dma_prep_memcpy(struct dma_chan *chan, dma_addr_t dst, dma_addr_t src,
 	return &mdesc->desc;
 }
 
+static struct dma_async_tx_descriptor *
+mpc_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
+		unsigned int sg_len, enum dma_transfer_direction direction,
+		unsigned long flags, void *context)
+{
+	struct mpc_dma *mdma = dma_chan_to_mpc_dma(chan);
+	struct mpc_dma_chan *mchan = dma_chan_to_mpc_dma_chan(chan);
+	struct mpc_dma_desc *mdesc = NULL;
+	dma_addr_t per_paddr;
+	u32 tcd_nunits;
+	struct mpc_dma_tcd *tcd;
+	unsigned long iflags;
+	struct scatterlist *sg;
+	size_t len;
+	int iter, i;
+
+	/* currently there is no proper support for scatter/gather */
+	if (sg_len != 1)
+		return NULL;
+
+	for_each_sg(sgl, sg, sg_len, i) {
+		spin_lock_irqsave(&mchan->lock, iflags);
+
+		mdesc = list_first_entry(&mchan->free, struct mpc_dma_desc,
+									node);
+		if (!mdesc) {
+			spin_unlock_irqrestore(&mchan->lock, iflags);
+			/* try to free completed descriptors */
+			mpc_dma_process_completed(mdma);
+			return NULL;
+		}
+
+		list_del(&mdesc->node);
+
+		per_paddr = mchan->per_paddr;
+		tcd_nunits = mchan->tcd_nunits;
+
+		spin_unlock_irqrestore(&mchan->lock, iflags);
+
+		if (per_paddr == 0 || tcd_nunits == 0)
+			goto err_prep;
+
+		mdesc->error = 0;
+		mdesc->will_access_peripheral = 1;
+		tcd = mdesc->tcd;
+
+		/* Prepare Transfer Control Descriptor for this transaction */
+
+		memset(tcd, 0, sizeof(struct mpc_dma_tcd));
+
+		if (!IS_ALIGNED(sg_dma_address(sg), 4))
+			goto err_prep;
+
+		if (direction == DMA_DEV_TO_MEM) {
+			tcd->saddr = per_paddr;
+			tcd->daddr = sg_dma_address(sg);
+			tcd->soff = 0;
+			tcd->doff = 4;
+		} else if (direction == DMA_MEM_TO_DEV) {
+			tcd->saddr = sg_dma_address(sg);
+			tcd->daddr = per_paddr;
+			tcd->soff = 4;
+			tcd->doff = 0;
+		} else
+			goto err_prep;
+
+		tcd->ssize = MPC_DMA_TSIZE_4;
+		tcd->dsize = MPC_DMA_TSIZE_4;
+
+		len = sg_dma_len(sg);
+		tcd->nbytes = tcd_nunits * 4;
+		if (!IS_ALIGNED(len, tcd->nbytes))
+			goto err_prep;
+
+		iter = len / tcd->nbytes;
+		if (iter >= 1 << 15) {
+			/* len is too big */
+			goto err_prep;
+		} else {
+			/* citer_linkch contains the high bits of iter */
+			tcd->biter = iter & 0x1ff;
+			tcd->biter_linkch = iter >> 9;
+			tcd->citer = tcd->biter;
+			tcd->citer_linkch = tcd->biter_linkch;
+		}
+
+		tcd->e_sg = 0;
+		tcd->d_req = 1;
+
+		/* Place descriptor in prepared list */
+		spin_lock_irqsave(&mchan->lock, iflags);
+		list_add_tail(&mdesc->node, &mchan->prepared);
+		spin_unlock_irqrestore(&mchan->lock, iflags);
+	}
+
+	return &mdesc->desc;
+
+err_prep:
+	/* Put the descriptor back */
+	spin_lock_irqsave(&mchan->lock, iflags);
+	list_add_tail(&mdesc->node, &mchan->free);
+	spin_unlock_irqrestore(&mchan->lock, iflags);
+
+	return NULL;
+}
+
+static int mpc_dma_device_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
+							unsigned long arg)
+{
+	struct mpc_dma_chan *mchan;
+	struct mpc_dma *mdma;
+	struct dma_slave_config *cfg;
+	unsigned long flags;
+
+	mchan = dma_chan_to_mpc_dma_chan(chan);
+	switch (cmd) {
+	case DMA_TERMINATE_ALL:
+		/* disable channel requests */
+		mdma = dma_chan_to_mpc_dma(chan);
+
+		spin_lock_irqsave(&mchan->lock, flags);
+
+		out_8(&mdma->regs->dmacerq, chan->chan_id);
+		list_splice_tail_init(&mchan->prepared, &mchan->free);
+		list_splice_tail_init(&mchan->queued, &mchan->free);
+		list_splice_tail_init(&mchan->active, &mchan->free);
+
+		spin_unlock_irqrestore(&mchan->lock, flags);
+
+		return 0;
+	case DMA_SLAVE_CONFIG:
+		/* Constraints:
+		 * - only transfers between a peripheral device and
+		 * memory are supported;
+		 * - minimal transfer size is 4 bytes and consequently
+		 * source and destination addresses must be 4-byte aligned and
+		 * transfer size must be aligned on (4 * maxburst) boundary;
+		 * - RAM address is being incremented by minimal transfer size
+		 * during the transfer;
+		 * - peripheral port's address is constant during the transfer.
+		 */
+
+		cfg = (void *)arg;
+
+		if (cfg->direction != DMA_DEV_TO_MEM &&
+			cfg->direction != DMA_MEM_TO_DEV)
+			return -EINVAL;
+
+		if (cfg->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES &&
+			cfg->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
+			return -EINVAL;
+
+		spin_lock_irqsave(&mchan->lock, flags);
+
+		if (cfg->direction == DMA_DEV_TO_MEM) {
+			mchan->per_paddr = cfg->src_addr;
+			mchan->tcd_nunits = cfg->src_maxburst;
+		} else {
+			mchan->per_paddr = cfg->dst_addr;
+			mchan->tcd_nunits = cfg->dst_maxburst;
+		}
+
+		if (!IS_ALIGNED(mchan->per_paddr, 4)) {
+			spin_unlock_irqrestore(&mchan->lock, flags);
+			return -EINVAL;
+		}
+
+		if (mchan->tcd_nunits == 0)
+			mchan->tcd_nunits = 1;	/* apply default */
+
+		spin_unlock_irqrestore(&mchan->lock, flags);
+
+		return 0;
+	default:
+		return -ENOSYS;
+	}
+
+	return -EINVAL;
+}
+
 static int mpc_dma_probe(struct platform_device *op)
 {
 	struct device_node *dn = op->dev.of_node;
@@ -727,9 +944,12 @@ static int mpc_dma_probe(struct platform_device *op)
 	dma->device_issue_pending = mpc_dma_issue_pending;
 	dma->device_tx_status = mpc_dma_tx_status;
 	dma->device_prep_dma_memcpy = mpc_dma_prep_memcpy;
+	dma->device_prep_slave_sg = mpc_dma_prep_slave_sg;
+	dma->device_control = mpc_dma_device_control;
 
 	INIT_LIST_HEAD(&dma->channels);
 	dma_cap_set(DMA_MEMCPY, dma->cap_mask);
+	dma_cap_set(DMA_SLAVE, dma->cap_mask);
 
 	for (i = 0; i < dma->chancnt; i++) {
 		mchan = &mdma->channels[i];
-- 
1.8.4.2

[PATCH RFC v6 3/5] dma: of: Add common xlate function for matching by channel id

[Alexander Popov]

This patch adds a new common OF dma xlate callback function which will match a
channel by it's id. The binding expects one integer argument which it will use to
lookup the channel by the id.

Unlike of_dma_simple_xlate this function is able to handle a system with
multiple DMA controllers. When registering the of dma provider with
of_dma_controller_register a pointer to the dma_device struct which is
associated with the dt node needs to passed as the data parameter.
New function will use this pointer to match only channels which belong to the
specified DMA controller.

Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
---
 drivers/dma/of-dma.c   | 35 +++++++++++++++++++++++++++++++++++
 include/linux/of_dma.h |  4 ++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 0b88dd3..03a375e 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -215,3 +215,38 @@ struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 			&dma_spec->args[0]);
 }
 EXPORT_SYMBOL_GPL(of_dma_simple_xlate);
+
+/**
+ * of_dma_xlate_by_chan_id - Translate dt property to DMA channel by channel id
+ * @dma_spec:	pointer to DMA specifier as found in the device tree
+ * @of_dma:	pointer to DMA controller data
+ *
+ * This function can be used as the of xlate callback for DMA driver which wants
+ * to match the channel based on the channel id. When using this xlate function
+ * the #dma-cells propety of the DMA controller dt node needs to be set to 1.
+ * The data parameter of of_dma_controller_register must be a pointer to the
+ * dma_device struct the function should match upon.
+ *
+ * Returns pointer to appropriate dma channel on success or NULL on error.
+ */
+struct dma_chan *of_dma_xlate_by_chan_id(struct of_phandle_args *dma_spec,
+					 struct of_dma *ofdma)
+{
+	struct dma_device *dev = ofdma->of_dma_data;
+	struct dma_chan *chan, *candidate = NULL;
+
+	if (!dev || dma_spec->args_count != 1)
+		return NULL;
+
+	list_for_each_entry(chan, &dev->channels, device_node)
+		if (chan->chan_id == dma_spec->args[0]) {
+			candidate = chan;
+			break;
+		}
+
+	if (!candidate)
+		return NULL;
+
+	return dma_get_slave_channel(candidate);
+}
+EXPORT_SYMBOL_GPL(of_dma_xlate_by_chan_id);
diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h
index ae36298..56bc026 100644
--- a/include/linux/of_dma.h
+++ b/include/linux/of_dma.h
@@ -41,6 +41,8 @@ extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 						     const char *name);
 extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 		struct of_dma *ofdma);
+extern struct dma_chan *of_dma_xlate_by_chan_id(struct of_phandle_args *dma_spec,
+		struct of_dma *ofdma);
 #else
 static inline int of_dma_controller_register(struct device_node *np,
 		struct dma_chan *(*of_dma_xlate)
@@ -66,6 +68,8 @@ static inline struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_s
 	return NULL;
 }
 
+#define of_dma_xlate_by_chan_id NULL
+
 #endif
 
 #endif /* __LINUX_OF_DMA_H */
-- 
1.8.4.2

[PATCH RFC v6 4/5] dma: mpc512x: register for device tree channel lookup

[Alexander Popov]

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>
[ a13xp0p0v88@gmail.com: resolve little patch conflict ]
[ a13xp0p0v88@gmail.com: set DMA_PRIVATE flag for MPC512x DMA controller ]
Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
---
 .../devicetree/bindings/dma/mpc512x-dma.txt        | 55 ++++++++++++++++++++++
 arch/powerpc/boot/dts/mpc5121.dtsi                 |  1 +
 drivers/dma/mpc512x_dma.c                          | 22 +++++++--
 3 files changed, 75 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).
+
+
+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 2d7cb04..15b7860 100644
--- a/arch/powerpc/boot/dts/mpc5121.dtsi
+++ b/arch/powerpc/boot/dts/mpc5121.dtsi
@@ -389,6 +389,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 a7e7749..8fabb52 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -50,6 +50,7 @@
 #include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
+#include <linux/of_dma.h>
 #include <linux/of_platform.h>
 
 #include <linux/random.h>
@@ -950,6 +951,7 @@ static int mpc_dma_probe(struct platform_device *op)
 	INIT_LIST_HEAD(&dma->channels);
 	dma_cap_set(DMA_MEMCPY, dma->cap_mask);
 	dma_cap_set(DMA_SLAVE, dma->cap_mask);
+	dma_cap_set(DMA_PRIVATE, dma->cap_mask);
 
 	for (i = 0; i < dma->chancnt; i++) {
 		mchan = &mdma->channels[i];
@@ -1013,11 +1015,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 +1040,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);
-- 
1.8.4.2

[PATCH RFC v6 5/5] HACK mmc: mxcmmc: enable clocks for the MPC512x

[Alexander Popov]

From: Gerhard Sittig <gsi@denx.de>

Q&D HACK to enable SD card support without correct COMMON_CLK support,
best viewed with 'git diff -w -b', NOT acceptable for mainline (NAKed)

Signed-off-by: Gerhard Sittig <gsi@denx.de>
[ a13xp0p0v88@gmail.com: resolve little patch conflict ]
Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
---
 drivers/mmc/host/mxcmmc.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index f7199c8..ddefa60 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -1123,20 +1123,29 @@ static int mxcmci_probe(struct platform_device *pdev)
 	host->res = r;
 	host->irq = irq;
 
-	host->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
-	if (IS_ERR(host->clk_ipg)) {
-		ret = PTR_ERR(host->clk_ipg);
-		goto out_iounmap;
-	}
+	if (!is_mpc512x_mmc(host)) {
+		host->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
+		if (IS_ERR(host->clk_ipg)) {
+			ret = PTR_ERR(host->clk_ipg);
+			goto out_iounmap;
+		}
 
-	host->clk_per = devm_clk_get(&pdev->dev, "per");
-	if (IS_ERR(host->clk_per)) {
-		ret = PTR_ERR(host->clk_per);
-		goto out_iounmap;
+		host->clk_per = devm_clk_get(&pdev->dev, "per");
+		if (IS_ERR(host->clk_per)) {
+			ret = PTR_ERR(host->clk_per);
+			goto out_iounmap;
+		}
+	} else {
+		host->clk_per = devm_clk_get(&pdev->dev, "sdhc_clk");
+		if (IS_ERR(host->clk_per)) {
+			ret = PTR_ERR(host->clk_per);
+			goto out_iounmap;
+		}
 	}
 
 	clk_prepare_enable(host->clk_per);
-	clk_prepare_enable(host->clk_ipg);
+	if (host->clk_ipg)
+		clk_prepare_enable(host->clk_ipg);
 
 	mxcmci_softreset(host);
 
@@ -1206,7 +1215,8 @@ out_free_dma:
 		dma_release_channel(host->dma);
 out_clk_put:
 	clk_disable_unprepare(host->clk_per);
-	clk_disable_unprepare(host->clk_ipg);
+	if (host->clk_ipg)
+		clk_disable_unprepare(host->clk_ipg);
 out_iounmap:
 	iounmap(host->base);
 out_free:
@@ -1236,7 +1246,8 @@ static int mxcmci_remove(struct platform_device *pdev)
 		dma_release_channel(host->dma);
 
 	clk_disable_unprepare(host->clk_per);
-	clk_disable_unprepare(host->clk_ipg);
+	if (host->clk_ipg)
+		clk_disable_unprepare(host->clk_ipg);
 
 	release_mem_region(host->res->start, resource_size(host->res));
 
@@ -1252,7 +1263,9 @@ static int mxcmci_suspend(struct device *dev)
 	struct mxcmci_host *host = mmc_priv(mmc);
 
 	clk_disable_unprepare(host->clk_per);
-	clk_disable_unprepare(host->clk_ipg);
+	if (host->clk_ipg)
+		clk_disable_unprepare(host->clk_ipg);
+
 	return 0;
 }
 
@@ -1262,7 +1275,8 @@ static int mxcmci_resume(struct device *dev)
 	struct mxcmci_host *host = mmc_priv(mmc);
 
 	clk_prepare_enable(host->clk_per);
-	clk_prepare_enable(host->clk_ipg);
+	if (host->clk_ipg)
+		clk_prepare_enable(host->clk_ipg);
 	return 0;
 }
 
-- 
1.8.4.2

Re: [PATCH RFC v6 4/5] dma: mpc512x: register for device tree channel lookup

[Gerhard Sittig]

On Tue, Dec 24, 2013 at 16:06 +0400, Alexander Popov wrote:
> 
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
> @@ -0,0 +1,55 @@
> +* Freescale MPC512x DMA Controller
> +
> +[ ... ]
> +
> +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

Given how time has passed and that we learned something in the
meantime, I guess the device tree documentation would look
different today than what was written back then.

- I'd reference the generic interrupt bindings as well, so DTS
  authors need not guess what an interrupt spec looks like
- the #dma-cells would become less confusing is it referenced the
  generic DMA binding, and just say that the value is "the length
  of the DMA specifier" for this provider
- the property's being recommended should not get hidden in the
  description but should reflect in the group's caption
- the binding doc shold not reference implementation details of
  one specific user (common client helper code)


[ device tree binding doc policy? ]

That one Linux driver handles both MPC5121 and MPC8308 hardware
and implements the same binding in both cases should get
reflected in the documentation as well.  But I'm not certain
whether adding MPC8308 into an MPX5121 document is better than
duplicating MPC5121 information in another MPC8308 document.  But
it might be the lesser evil.

Are there opinions, established preferences?  Is an exhaustive
list of compatible strings good enough since text search will
match regardless of the document's filename in this case?

There must have been this situation before of a component being
used in one SoC and getting re-used in another SoC later, too.
What's the best document to "get inspired from", i.e. how to best
put this into binding document wording as well as filenames?

> +[ ... ]
> +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

This certainly is wrong (it was before, I just wasn't aware back
then).  The phandle is not part of the specifier.  And the
binding should not discuss what driver code does.  Since the DMA
controller implements the semantics of the common DMA binding,
it's unwise to duplicate the information here.


Let me see how I can improve this document.  Alex, it may be
useful to split the code update and the binding document into
separate patches.  The current status already mixes the code
extension and the binding update with the introduction of the
document which was missing in the first place.  That's why the
binding doc patch is that late in the series.  (Yes, my RFC
"template" was rather dirty, which is why I flagged it as "RFC"
in the first place.)

I guess that I may have to introduce a binding doc reflecting the
given current status, and update it later as new features become
available.

Or -- given that the hardware remains, all the knowledge is there
already, just the implementations' capabilities change -- I might
as well introduce a binding document including OF based DMA
lookup.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH RFC v6 4/5] dma: mpc512x: register for device tree channel lookup

[Gerhard Sittig]

[ dropping devicetree, we're DMA specific here ]

On Tue, Dec 24, 2013 at 16:06 +0400, Alexander Popov wrote:
> 
> --- a/drivers/dma/mpc512x_dma.c
> +++ b/drivers/dma/mpc512x_dma.c
> [ ... ]
> @@ -950,6 +951,7 @@ static int mpc_dma_probe(struct platform_device *op)
>  	INIT_LIST_HEAD(&dma->channels);
>  	dma_cap_set(DMA_MEMCPY, dma->cap_mask);
>  	dma_cap_set(DMA_SLAVE, dma->cap_mask);
> +	dma_cap_set(DMA_PRIVATE, dma->cap_mask);
>  
>  	for (i = 0; i < dma->chancnt; i++) {
>  		mchan = &mdma->channels[i];

What are the implications of this?  Is a comment due?

I haven't found documentation about the DMA_PRIVATE flag, only
saw commit 59b5ec21446b9 "dmaengine: introduce
dma_request_channel and private channels".  Alex, unless I'm
missing something this one-line change is quite a change in
semantics, and has dramatic influence on the code's behaviour
(ignores the DMA controller when looking for channels that can do
mem-to-mem transfers).

Please reason about this change some more, and explain what it
does and why it's needed.  Consider the fact that this driver
handles both MPC5121 as well as MPC8308 hardware.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH RFC v6 4/5] dma: mpc512x: register for device tree channel lookup

[Alexander Popov]

Hello Gerhard.
Thanks for your review.

2013/12/26 Gerhard Sittig <gsi@denx.de>:
> [ dropping devicetree, we're DMA specific here ]
>
> On Tue, Dec 24, 2013 at 16:06 +0400, Alexander Popov wrote:
>>
>> --- a/drivers/dma/mpc512x_dma.c
>> +++ b/drivers/dma/mpc512x_dma.c
>> [ ... ]
>> @@ -950,6 +951,7 @@ static int mpc_dma_probe(struct platform_device *op)
>>       INIT_LIST_HEAD(&dma->channels);
>>       dma_cap_set(DMA_MEMCPY, dma->cap_mask);
>>       dma_cap_set(DMA_SLAVE, dma->cap_mask);
>> +     dma_cap_set(DMA_PRIVATE, dma->cap_mask);
>>
>>       for (i = 0; i < dma->chancnt; i++) {
>>               mchan = &mdma->channels[i];
>
> What are the implications of this?  Is a comment due?

I've involved DMA_PRIVATE flag because new of_dma_xlate_by_chan_id()
uses dma_get_slave_channel() instead of dma_request_channel()
(PATCH RFC v6 3/5). This flag is implicitly set in dma_request_channel(),
but is not set in dma_get_slave_channel().

There are only two places in the mainline kernel, where
dma_get_slave_channel() is used. I've picked up the idea
at one of these places. Please look at this patch:
http://www.spinics.net/lists/arm-kernel/msg268718.html

> I haven't found documentation about the DMA_PRIVATE flag, only
> saw commit 59b5ec21446b9 "dmaengine: introduce
> dma_request_channel and private channels".

Unfortunately I didn't find any description of DMA_PRIVATE flag too.
But the comment at the beginning of drivers/dma/dmaengine.c
may give a clue. Quotation:
  * subsystem can get access to a channel by calling dmaengine_get() followed
  * by dma_find_channel(), or if it has need for an exclusive channel
it can call
  * dma_request_channel().  Once a channel is allocated a reference is taken
  * against its corresponding driver to disable removal.

DMA_PRIVATE capability flag might indicate that the DMA controller
can provide exclusive channels to its clients. Please correct me if I'm wrong.

> Alex, unless I'm
> missing something this one-line change is quite a change in
> semantics, and has dramatic influence on the code's behaviour
> (ignores the DMA controller when looking for channels that can do
> mem-to-mem transfers)

Excuse me, Gerhard, I don't see what you mean.
Could you point to the corresponding code?

> Consider the fact that this driver
> handles both MPC5121 as well as MPC8308 hardware.

Ah, yes, sorry. I should certainly fix this, if setting of DMA_PRIVATE flag
is needed at all.

Thanks!

Best regards,
Alexander

Re: [PATCH RFC v6 4/5] dma: mpc512x: register for device tree channel lookup

[Gerhard Sittig]

[ dropping devicetree from the Cc: list ]

[ what is the semantics of DMA_PRIVATE capability flag?
  is documentation available beyond the initial commit message?
  need individual channels be handled instead of controllers? ]

On Sat, Jan 04, 2014 at 00:54 +0400, Alexander Popov wrote:
> 
> Hello Gerhard.
> Thanks for your review.
> 
> 2013/12/26 Gerhard Sittig <gsi@denx.de>:
> > [ dropping devicetree, we're DMA specific here ]
> >
> > On Tue, Dec 24, 2013 at 16:06 +0400, Alexander Popov wrote:
> >>
> >> --- a/drivers/dma/mpc512x_dma.c
> >> +++ b/drivers/dma/mpc512x_dma.c
> >> [ ... ]
> >> @@ -950,6 +951,7 @@ static int mpc_dma_probe(struct platform_device *op)
> >>       INIT_LIST_HEAD(&dma->channels);
> >>       dma_cap_set(DMA_MEMCPY, dma->cap_mask);
> >>       dma_cap_set(DMA_SLAVE, dma->cap_mask);
> >> +     dma_cap_set(DMA_PRIVATE, dma->cap_mask);
> >>
> >>       for (i = 0; i < dma->chancnt; i++) {
> >>               mchan = &mdma->channels[i];
> >
> > What are the implications of this?  Is a comment due?
> 
> I've involved DMA_PRIVATE flag because new of_dma_xlate_by_chan_id()
> uses dma_get_slave_channel() instead of dma_request_channel()
> (PATCH RFC v6 3/5). This flag is implicitly set in dma_request_channel(),
> but is not set in dma_get_slave_channel().
> 
> There are only two places in the mainline kernel, where
> dma_get_slave_channel() is used. I've picked up the idea
> at one of these places. Please look at this patch:
> http://www.spinics.net/lists/arm-kernel/msg268718.html

I agree that the change looks simple, and there is no doubt that
other drivers apply the flag.  None of this I questioned.  Yet
I'm afraid that the implications are rather huge.

Unless I miss something, I'd happily learn where I'm wrong.

> > I haven't found documentation about the DMA_PRIVATE flag, only
> > saw commit 59b5ec21446b9 "dmaengine: introduce
> > dma_request_channel and private channels".
> 
> Unfortunately I didn't find any description of DMA_PRIVATE flag too.
> But the comment at the beginning of drivers/dma/dmaengine.c
> may give a clue. Quotation:
>   * subsystem can get access to a channel by calling dmaengine_get() followed
>   * by dma_find_channel(), or if it has need for an exclusive channel
> it can call
>   * dma_request_channel().  Once a channel is allocated a reference is taken
>   * against its corresponding driver to disable removal.
> 
> DMA_PRIVATE capability flag might indicate that the DMA controller
> can provide exclusive channels to its clients. Please correct me if I'm wrong.
> 
> > Alex, unless I'm
> > missing something this one-line change is quite a change in
> > semantics, and has dramatic influence on the code's behaviour
> > (ignores the DMA controller when looking for channels that can do
> > mem-to-mem transfers)
> 
> Excuse me, Gerhard, I don't see what you mean.
> Could you point to the corresponding code?

You did see `git show 59b5ec21446b9`, didn't you?  The commit
message strongly suggests that DMA_PRIVATE applies to the whole
DMA controller and excludes _all_ of its channels from the
general purpose allocator which mem-to-mem transfers appear to be
using.  It's not just a hint, but an active decision to reject
requests.

Not only checking code references, but doing a text search,
reveals one more comment on the DMA_PRIVATE flag in a crypto
related document, which supports my interpretation:
Documentation/crypto/async-tx-api.txt:203


Can somebody ACK or NAK my interpretation?  Dan, you committed
this change which introduced the DMA_PRIVATE logic.  What was the
motivation for it, or the goal to achieve?  Do other platforms
have several dedicated DMA controllers, some for peripherals and
some for memory transfers?  Should the "private" flag apply to
channels and not whole controllers?  Am I over-estimating the
benefit or importance of DMA supported memory transfers?


Still I see a difference in the lookup approaches:  Yours applies
DMA_PRIVATE globally and in advance, preventing _any_ use of DMA
for memory transfers.  While the __dma_request_channel() routine
only applies it _temporarily_ around a dma_chan_get() operation.
Allowing for use of DMA channels by both individual peripherals
as well as memory transfers.


> > Consider the fact that this driver
> > handles both MPC5121 as well as MPC8308 hardware.
> 
> Ah, yes, sorry. I should certainly fix this, if setting of DMA_PRIVATE flag
> is needed at all.

What I meant here is that implications for all affected platforms
should be considered.  There is one driver source, but the driver
applies to more than one platform (another issue of the driver is
that this is not apparent from the doc nor the compat strings).

MPC512x has one (GP) DMA controller, of which one channel is
dedicated to DDR, and all other channels can get used for memory
transfers as well.  In addition to most channels being connected
to a specific peripheral for flow control.  Which your patch set
introduces initial support for.

MPC8308 has _all_ channels for memory transfers exclusively (or
at least none of its channels supports flow control).

So blocking memory transfers in mpc512x_dma.c is a total breakage
for MPC8308 (removes the only previous feature and adds nothing),
and is a regression for MPC512x (removes the previously supported
memory transfers, while it may add peripheral supports with very
few users).


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH RFC v6 4/5] dma: mpc512x: register for device tree channel lookup

[Vinod Koul]

On Wed, Jan 08, 2014 at 05:47:19PM +0100, Gerhard Sittig wrote:
> [ dropping devicetree from the Cc: list ]
> 
> [ what is the semantics of DMA_PRIVATE capability flag?
>   is documentation available beyond the initial commit message?
>   need individual channels be handled instead of controllers? ]

The DMA_PRIVATE means that your channels are not to be used for global memcpy,
as one can do in async cases (this is hwere DMAengine came into existence)

If the device has the capablity of doing genric memcpy then it should not set
this. For slave dma usage the dam channel can transfer data to a specfic
slave device(s), hence we should use this is geric fashion so setting
DMA_PRIVATE makes sense in those cases.

> 
> On Sat, Jan 04, 2014 at 00:54 +0400, Alexander Popov wrote:
> > 
> > Hello Gerhard.
> > Thanks for your review.
> > 
> > 2013/12/26 Gerhard Sittig <gsi@denx.de>:
> > > [ dropping devicetree, we're DMA specific here ]
> > >
> > > On Tue, Dec 24, 2013 at 16:06 +0400, Alexander Popov wrote:
> > >>
> > >> --- a/drivers/dma/mpc512x_dma.c
> > >> +++ b/drivers/dma/mpc512x_dma.c
> > >> [ ... ]
> > >> @@ -950,6 +951,7 @@ static int mpc_dma_probe(struct platform_device *op)
> > >>       INIT_LIST_HEAD(&dma->channels);
> > >>       dma_cap_set(DMA_MEMCPY, dma->cap_mask);
> > >>       dma_cap_set(DMA_SLAVE, dma->cap_mask);
> > >> +     dma_cap_set(DMA_PRIVATE, dma->cap_mask);
> > >>
> > >>       for (i = 0; i < dma->chancnt; i++) {
> > >>               mchan = &mdma->channels[i];
> > >
> > > What are the implications of this?  Is a comment due?
> > 
> > I've involved DMA_PRIVATE flag because new of_dma_xlate_by_chan_id()
> > uses dma_get_slave_channel() instead of dma_request_channel()
> > (PATCH RFC v6 3/5). This flag is implicitly set in dma_request_channel(),
> > but is not set in dma_get_slave_channel().
Which makes me thing you are targetting slave usages. Do you intend to use for
mempcy too on all controllers you support. in that case you should set it
selectively.

> > There are only two places in the mainline kernel, where
> > dma_get_slave_channel() is used. I've picked up the idea
> > at one of these places. Please look at this patch:
> > http://www.spinics.net/lists/arm-kernel/msg268718.html
> 
> I agree that the change looks simple, and there is no doubt that
> other drivers apply the flag.  None of this I questioned.  Yet
> I'm afraid that the implications are rather huge.
> 
> Unless I miss something, I'd happily learn where I'm wrong.
> 
> > > I haven't found documentation about the DMA_PRIVATE flag, only
> > > saw commit 59b5ec21446b9 "dmaengine: introduce
> > > dma_request_channel and private channels".
> > 
> > Unfortunately I didn't find any description of DMA_PRIVATE flag too.
> > But the comment at the beginning of drivers/dma/dmaengine.c
> > may give a clue. Quotation:
> >   * subsystem can get access to a channel by calling dmaengine_get() followed
> >   * by dma_find_channel(), or if it has need for an exclusive channel
> > it can call
> >   * dma_request_channel().  Once a channel is allocated a reference is taken
> >   * against its corresponding driver to disable removal.
> > 
> > DMA_PRIVATE capability flag might indicate that the DMA controller
> > can provide exclusive channels to its clients. Please correct me if I'm wrong.
> > 
> > > Alex, unless I'm
> > > missing something this one-line change is quite a change in
> > > semantics, and has dramatic influence on the code's behaviour
> > > (ignores the DMA controller when looking for channels that can do
> > > mem-to-mem transfers)
> > 
> > Excuse me, Gerhard, I don't see what you mean.
> > Could you point to the corresponding code?
> 
> You did see `git show 59b5ec21446b9`, didn't you?  The commit
> message strongly suggests that DMA_PRIVATE applies to the whole
> DMA controller and excludes _all_ of its channels from the
> general purpose allocator which mem-to-mem transfers appear to be
> using.  It's not just a hint, but an active decision to reject
> requests.
> 
> Not only checking code references, but doing a text search,
> reveals one more comment on the DMA_PRIVATE flag in a crypto
> related document, which supports my interpretation:
> Documentation/crypto/async-tx-api.txt:203
> 
> 
> Can somebody ACK or NAK my interpretation?  Dan, you committed
> this change which introduced the DMA_PRIVATE logic.  What was the
> motivation for it, or the goal to achieve?  Do other platforms
> have several dedicated DMA controllers, some for peripherals and
> some for memory transfers?  Should the "private" flag apply to
> channels and not whole controllers?  Am I over-estimating the
> benefit or importance of DMA supported memory transfers?

The DMA_PRIVATE flag is more on how the channel is allocated and will it be used
by generic allocator or not. You cna still use mecpy ops for a controller with
DMA_PRIVATE flag if the controller supports.
> 
> 
> Still I see a difference in the lookup approaches:  Yours applies
> DMA_PRIVATE globally and in advance, preventing _any_ use of DMA
> for memory transfers.  While the __dma_request_channel() routine
> only applies it _temporarily_ around a dma_chan_get() operation.
> Allowing for use of DMA channels by both individual peripherals
> as well as memory transfers.
> 
No it doesnt prevent. You can still use it for memcpy once you have the channel.

--
~Vinod
> 
> > > Consider the fact that this driver
> > > handles both MPC5121 as well as MPC8308 hardware.
> > 
> > Ah, yes, sorry. I should certainly fix this, if setting of DMA_PRIVATE flag
> > is needed at all.
> 
> What I meant here is that implications for all affected platforms
> should be considered.  There is one driver source, but the driver
> applies to more than one platform (another issue of the driver is
> that this is not apparent from the doc nor the compat strings).
> 
> MPC512x has one (GP) DMA controller, of which one channel is
> dedicated to DDR, and all other channels can get used for memory
> transfers as well.  In addition to most channels being connected
> to a specific peripheral for flow control.  Which your patch set
> introduces initial support for.
> 
> MPC8308 has _all_ channels for memory transfers exclusively (or
> at least none of its channels supports flow control).
> 
> So blocking memory transfers in mpc512x_dma.c is a total breakage
> for MPC8308 (removes the only previous feature and adds nothing),
> and is a regression for MPC512x (removes the previously supported
> memory transfers, while it may add peripheral supports with very
> few users).
> 
> 
> virtually yours
> Gerhard Sittig
> -- 
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

-- 

Re: [PATCH RFC v6 4/5] dma: mpc512x: register for device tree channel lookup

[Alexander Popov]

Thanks for your replies, Gerhard and Vinod.

2014/1/9 Vinod Koul <vinod.koul@intel.com>:
> On Wed, Jan 08, 2014 at 05:47:19PM +0100, Gerhard Sittig wrote:
>> [ what is the semantics of DMA_PRIVATE capability flag?
>>   is documentation available beyond the initial commit message?
>>   need individual channels be handled instead of controllers? ]
>
> The DMA_PRIVATE means that your channels are not to be used for global memcpy,
> as one can do in async cases (this is hwere DMAengine came into existence)
>
> If the device has the capablity of doing genric memcpy then it should not set
> this. For slave dma usage the dam channel can transfer data to a specfic
> slave device(s), hence we should use this is geric fashion so setting
> DMA_PRIVATE makes sense in those cases.

Each DMA channel of MPC512x DMA controller can do _both_
mem-to-mem transfers and transfers between mem and some slave peripheral
(only one DMA channel is fully dedicated to DDR).
All DMA channels of MPC512x DMA controller belong to one dma_device.
So we _don't_ need setting DMA_PRIVATE flag for this dma_device at all, do we?

>> On Sat, Jan 04, 2014 at 00:54 +0400, Alexander Popov wrote:
>> > I've involved DMA_PRIVATE flag because new of_dma_xlate_by_chan_id()
>> > uses dma_get_slave_channel() instead of dma_request_channel()
>> > (PATCH RFC v6 3/5). This flag is implicitly set in dma_request_channel(),
>> > but is not set in dma_get_slave_channel().
> Which makes me thing you are targetting slave usages. Do you intend to use for
> mempcy too on all controllers you support. in that case you should set it
> selectively.

Vinod, please correct me if I'm wrong.
As I could understand from your comments and the code,
DMA_PRIVATE flag is needed for dma_devices with DMA channels
which _can_ work with slave peripheral but _can't_ do mem-to-mem transfers.
If DMA_PRIVATE flag is set for some dma_device before
dma_async_device_register()
then its DMA channels are not published in tables for kernel slab allocator
(because these channels are simply useless for memcpy).

>> Still I see a difference in the lookup approaches:  Yours applies
>> DMA_PRIVATE globally and in advance, preventing _any_ use of DMA
>> for memory transfers.  While the __dma_request_channel() routine
>> only applies it _temporarily_ around a dma_chan_get() operation.
>> Allowing for use of DMA channels by both individual peripherals
>> as well as memory transfers.
>>
> No it doesnt prevent. You can still use it for memcpy once you have the channel.

Excuse me, I don't completely understand why dma_request_channel()
needs to set DMA_PRIVATE flag.
If dma_request_channel() for some dma_device without DMA_PRIVATE
is called before the first dmaengine_get()
then no DMA channels of this dma_device will become available for memcpy
by slab allocator.
Could you give me a clue?

>> > > Consider the fact that this driver
>> > > handles both MPC5121 as well as MPC8308 hardware.
>> >
>> > Ah, yes, sorry. I should certainly fix this, if setting of DMA_PRIVATE flag
>> > is needed at all.
>>
>> What I meant here is that implications for all affected platforms
>> should be considered.  There is one driver source, but the driver
>> applies to more than one platform (another issue of the driver is
>> that this is not apparent from the doc nor the compat strings).

I'll add a comment with information about the supported platforms to
mpc512x_dma.c
in RFC PATCH 1/5. Ok?

>> So blocking memory transfers in mpc512x_dma.c is a total breakage
>> for MPC8308 (removes the only previous feature and adds nothing),
>> and is a regression for MPC512x (removes the previously supported
>> memory transfers, while it may add peripheral supports with very
>> few users).

Yes, I see. MPC512x and MPC8308 should be treated differently.

Thanks!
Alexander

Re: [PATCH RFC v6 4/5] dma: mpc512x: register for device tree channel lookup

[Gerhard Sittig]

On Mon, Jan 13, 2014 at 12:17 +0400, Alexander Popov wrote:
> 
> Thanks for your replies, Gerhard and Vinod.
> 
> 2014/1/9 Vinod Koul <vinod.koul@intel.com>:
> > On Wed, Jan 08, 2014 at 05:47:19PM +0100, Gerhard Sittig wrote:
> >> [ what is the semantics of DMA_PRIVATE capability flag?
> >>   is documentation available beyond the initial commit message?
> >>   need individual channels be handled instead of controllers? ]
> >
> > The DMA_PRIVATE means that your channels are not to be used for global memcpy,
> > as one can do in async cases (this is hwere DMAengine came into existence)
> >
> > If the device has the capablity of doing genric memcpy then it should not set
> > this. For slave dma usage the dam channel can transfer data to a specfic
> > slave device(s), hence we should use this is geric fashion so setting
> > DMA_PRIVATE makes sense in those cases.
> 
> Each DMA channel of MPC512x DMA controller can do _both_
> mem-to-mem transfers and transfers between mem and some slave peripheral
> (only one DMA channel is fully dedicated to DDR).
> All DMA channels of MPC512x DMA controller belong to one dma_device.
> So we _don't_ need setting DMA_PRIVATE flag for this dma_device at all, do we?

I'd phrase it a little stronger.  It's not that we don't _need_
the DMA_PRIVATE flag, it's actually that we _must_not_ use it
(unless I'm being dense, and keep missing something).  With the
DMA_PRIVATE flag set, the generic allocator will refuse to use
any channel of the only DMA controller, which totally eliminates
general use, and only leaves us with explicitly configured uses
(that would be MMC only in mainline, and nothing else).

> >> Still I see a difference in the lookup approaches:  Yours applies
> >> DMA_PRIVATE globally and in advance, preventing _any_ use of DMA
> >> for memory transfers.  While the __dma_request_channel() routine
> >> only applies it _temporarily_ around a dma_chan_get() operation.
> >> Allowing for use of DMA channels by both individual peripherals
> >> as well as memory transfers.
> >>
> > No it doesnt prevent. You can still use it for memcpy once you have the channel.

Vinod, what am I missing here?  Before probe() there is no DMA
controller.  After probe() the DMA_PRIVATE flag is set and thus
general allocation won't happen.  How exactly does one get to
"have the channel" for memory transfers?  Aren't the channel
references acquired upon demand, as the need arises?  While the
DMA controller has no means to know whether "all memory transfer
channel aquisition was done" or whether "all slave peripherals
have their channel" (if at all such a situation exists, given we
have dynamically loadable modules), such that the DMA_PRIVATE
toggle could get thrown one way or another?

This brings me back to a question I raised earlier:  Am I
overestimating the benefit or importance of DMA supported memory
transfers?  Am I wrong assuming that there are users of this
feature which need not get configured explicitly (i.e. they
operate in transparent ways, using whatever they find to be
available), and that the set of these users and their consumption
of DMA resources is something that is dynamic (i.e. driven by
demand, instead of pre-allocated and then probably inappropriate
for the workload they see)?

> Excuse me, I don't completely understand why dma_request_channel()
> needs to set DMA_PRIVATE flag.
> If dma_request_channel() for some dma_device without DMA_PRIVATE
> is called before the first dmaengine_get()
> then no DMA channels of this dma_device will become available for memcpy
> by slab allocator.
> Could you give me a clue?
> 
> >> > > Consider the fact that this driver
> >> > > handles both MPC5121 as well as MPC8308 hardware.
> >> >
> >> > Ah, yes, sorry. I should certainly fix this, if setting of DMA_PRIVATE flag
> >> > is needed at all.
> >>
> >> What I meant here is that implications for all affected platforms
> >> should be considered.  There is one driver source, but the driver
> >> applies to more than one platform (another issue of the driver is
> >> that this is not apparent from the doc nor the compat strings).
> 
> I'll add a comment with information about the supported platforms to
> mpc512x_dma.c
> in RFC PATCH 1/5. Ok?
> 
> >> So blocking memory transfers in mpc512x_dma.c is a total breakage
> >> for MPC8308 (removes the only previous feature and adds nothing),
> >> and is a regression for MPC512x (removes the previously supported
> >> memory transfers, while it may add peripheral supports with very
> >> few users).
> 
> Yes, I see. MPC512x and MPC8308 should be treated differently.

Alexander, are you suggesting to treat 512x and 8308 differently,
and did you decide how to do that?  Previous review feedback
raised the question whether this is needed or appropriate, while
there has not been an answer yet AFAICT.  I would not jump to
conclusions here, especially when you cannot test what you
change.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de