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

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

[Gerhard Sittig]

blurb is below the stats

Alexander Popov (1):
  powerpc: mpc512x_dma: add support for data transfers between memory
    and i/o memory

Gerhard Sittig (6):
  dma: mpc512x: fix start condition in execute()
  dma: mpc512x: support 'terminate all' control request
  dts: mpc512x: prepare for preprocessor support
  dma: mpc512x: use symbolic specifiers for DMA channels
  dma: mpc512x: register for device tree channel lookup
  HACK mmc: mxcmmc: enable clocks for the MPC512x

Lars-Peter Clausen (1):
  dma: of: Add common xlate function for matching by channel id

 .../devicetree/bindings/dma/mpc512x-dma.txt        |   55 ++++++
 arch/powerpc/boot/dts/ac14xx.dts                   |    2 +-
 arch/powerpc/boot/dts/include/dt-bindings          |    1 +
 arch/powerpc/boot/dts/mpc5121.dtsi                 |    7 +-
 arch/powerpc/boot/dts/mpc5121ads.dts               |    2 +-
 arch/powerpc/boot/dts/pdm360ng.dts                 |    2 +-
 drivers/dma/mpc512x_dma.c                          |  181 +++++++++++++++++++-
 drivers/dma/of-dma.c                               |   47 +++++
 drivers/mmc/host/mxcmmc.c                          |   41 +++--
 include/dt-bindings/dma/mpc512x-dma.h              |   21 +++
 include/linux/of_dma.h                             |    4 +
 11 files changed, 336 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/mpc512x-dma.txt
 create mode 120000 arch/powerpc/boot/dts/include/dt-bindings
 create mode 100644 include/dt-bindings/dma/mpc512x-dma.h


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


Alexander, I'd suggest that you pickup this series and
- squash part 2 (start condition) and optionally part 3 (terminate all)
  into part 1 (your slave s/g introduction)
- drop part 8 (SD card demo) as it was already NAKed and will be
  obsolete with correct COMMON_CLK support, but was useful during test
  to have one more DMA client on the MPC512x platform
- put your SCLPC support on top of the then OF capable DMA driver
- please leave the preprocessor support for DTS files separate from the
  DMA related DTS part, as it will be part of another series as well
  (clocks) or might get picked up separately for its being useful or
  even a dependency in other ways (GPIOs, interrupts)

Lars, there is a checkpatch warning about a line longer than 80
characters in the common xlate part -- I didn't dare to change your
submission, and the part included here is verbatim from your patchwork
2331091 (original submission) and 2555701 (resend), is there a followup
which you can provide or point us to?

Arnd, this version addresses your concerns in Anatolij's earlier
submissions:
- the DMA controller really maps peripheral requesters 1:1 to DMA
  channels, there's no other mapping or flexibility involved
- OF registration in the DMA controller's driver now is non-fatal for
  compatibility with older device trees
- the device tree binding is documented (even if it's in line with the
  generic DMA binding, being explicit is good, I guess)

Vinod, would you take these DMA related patches when Alexander's updated
version has passed review?  That's what I understood from Lars so far.


the series is based on v3.10, each step in the series was build-tested,
the series' result including the mxcmmc hack was run-tested with the
commands below, moving 170MB of data and using DMA during transfers, the
numbers translate to 1.6MB/s throughput for the SD card and some 62.5KB
per DMA IRQ on average (does this help verify plausibility?)

  $ find /media/mmcblk0p1/ -type f | xargs time wc
    ...
    532086   3032411 173164032 total
  real    1m 47.41s
  user    0m 8.08s
  sys     0m 1.69s
  $ grep -i dma /proc/interrupts 
   65:       2716      IPIC Level     mpc512x_dma
  $ grep -i mmc /proc/interrupts 
   20:       5489      IPIC Level     mxc-mmc

-- 
1.7.10.4

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

[Gerhard Sittig]

blurb is below the stats

Alexander Popov (1):
  powerpc: mpc512x_dma: add support for data transfers between memory
    and i/o memory

Gerhard Sittig (6):
  dma: mpc512x: fix start condition in execute()
  dma: mpc512x: support 'terminate all' control request
  dts: mpc512x: prepare for preprocessor support
  dma: mpc512x: use symbolic specifiers for DMA channels
  dma: mpc512x: register for device tree channel lookup
  HACK mmc: mxcmmc: enable clocks for the MPC512x

Lars-Peter Clausen (1):
  dma: of: Add common xlate function for matching by channel id

 .../devicetree/bindings/dma/mpc512x-dma.txt        |   55 ++++++
 arch/powerpc/boot/dts/ac14xx.dts                   |    2 +-
 arch/powerpc/boot/dts/include/dt-bindings          |    1 +
 arch/powerpc/boot/dts/mpc5121.dtsi                 |    7 +-
 arch/powerpc/boot/dts/mpc5121ads.dts               |    2 +-
 arch/powerpc/boot/dts/pdm360ng.dts                 |    2 +-
 drivers/dma/mpc512x_dma.c                          |  181 +++++++++++++++++++-
 drivers/dma/of-dma.c                               |   47 +++++
 drivers/mmc/host/mxcmmc.c                          |   41 +++--
 include/dt-bindings/dma/mpc512x-dma.h              |   21 +++
 include/linux/of_dma.h                             |    4 +
 11 files changed, 336 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/mpc512x-dma.txt
 create mode 120000 arch/powerpc/boot/dts/include/dt-bindings
 create mode 100644 include/dt-bindings/dma/mpc512x-dma.h


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


Alexander, I'd suggest that you pickup this series and
- squash part 2 (start condition) and optionally part 3 (terminate all)
  into part 1 (your slave s/g introduction)
- drop part 8 (SD card demo) as it was already NAKed and will be
  obsolete with correct COMMON_CLK support, but was useful during test
  to have one more DMA client on the MPC512x platform
- put your SCLPC support on top of the then OF capable DMA driver
- please leave the preprocessor support for DTS files separate from the
  DMA related DTS part, as it will be part of another series as well
  (clocks) or might get picked up separately for its being useful or
  even a dependency in other ways (GPIOs, interrupts)

Lars, there is a checkpatch warning about a line longer than 80
characters in the common xlate part -- I didn't dare to change your
submission, and the part included here is verbatim from your patchwork
2331091 (original submission) and 2555701 (resend), is there a followup
which you can provide or point us to?

Arnd, this version addresses your concerns in Anatolij's earlier
submissions:
- the DMA controller really maps peripheral requesters 1:1 to DMA
  channels, there's no other mapping or flexibility involved
- OF registration in the DMA controller's driver now is non-fatal for
  compatibility with older device trees
- the device tree binding is documented (even if it's in line with the
  generic DMA binding, being explicit is good, I guess)

Vinod, would you take these DMA related patches when Alexander's updated
version has passed review?  That's what I understood from Lars so far.


the series is based on v3.10, each step in the series was build-tested,
the series' result including the mxcmmc hack was run-tested with the
commands below, moving 170MB of data and using DMA during transfers, the
numbers translate to 1.6MB/s throughput for the SD card and some 62.5KB
per DMA IRQ on average (does this help verify plausibility?)

  $ find /media/mmcblk0p1/ -type f | xargs time wc
    ...
    532086   3032411 173164032 total
  real    1m 47.41s
  user    0m 8.08s
  sys     0m 1.69s
  $ grep -i dma /proc/interrupts 
   65:       2716      IPIC Level     mpc512x_dma
  $ grep -i mmc /proc/interrupts 
   20:       5489      IPIC Level     mxc-mmc

-- 
1.7.10.4

[PATCH RFC 1/8] powerpc: mpc512x_dma: add support for data transfers between memory and i/o memory

[Gerhard Sittig]

From: Alexander Popov <a13xp0p0v88@gmail.com>

From: Alexander Popov <a13xp0p0v88@gmail.com>

Data transfers between memory and i/o memory require more delicate TCD
(Transfer Control Descriptor) configuration and DMA channel service requests
via hardware.

dma_device.device_control callback function is needed to configure
DMA channel to work with i/o memory.

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

diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index 2d95673..f90b717 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
@@ -28,11 +29,6 @@
  * file called COPYING.
  */
 
-/*
- * This is initial version of MPC5121 DMA driver. Only memory to memory
- * transfers are supported (tested using dmatest module).
- */
-
 #include <linux/module.h>
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
@@ -190,9 +186,13 @@ struct mpc_dma_chan {
 	struct list_head		completed;
 	struct mpc_dma_tcd		*tcd;
 	dma_addr_t			tcd_paddr;
+	u32				tcd_nunits;
 
 	/* Lock for this structure */
 	spinlock_t			lock;
+
+	/* Channel's peripheral fifo address */
+	dma_addr_t			per_paddr;
 };
 
 struct mpc_dma {
@@ -256,7 +256,9 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
 
 		prev->tcd->dlast_sga = mdesc->tcd_paddr;
 		prev->tcd->e_sg = 1;
-		mdesc->tcd->start = 1;
+		/* only start explicitly on MDDRC channel */
+		if (cid == 32)
+			mdesc->tcd->start = 1;
 
 		prev = mdesc;
 	}
@@ -268,7 +270,15 @@ 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);
+
+	switch (cid) {
+	case 26:
+		out_8(&mdma->regs->dmaserq, cid);
+		break;
+	case 32:
+		out_8(&mdma->regs->dmassrt, cid);
+		break;
+	}
 }
 
 /* Handle interrupt on one half of DMA controller (32 channels) */
@@ -641,6 +651,126 @@ 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;
+	struct mpc_dma_tcd *tcd;
+	unsigned long iflags;
+	struct scatterlist *sg;
+	size_t len;
+	int iter, i;
+
+	if (!list_empty(&mchan->active))
+		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);
+
+		spin_unlock_irqrestore(&mchan->lock, iflags);
+
+		mdesc->error = 0;
+		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))
+			return NULL;
+
+		if (direction == DMA_DEV_TO_MEM) {
+			tcd->saddr = mchan->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 = mchan->per_paddr;
+			tcd->soff = 4;
+			tcd->doff = 0;
+		} else {
+			return NULL;
+		}
+		tcd->ssize = MPC_DMA_TSIZE_4;
+		tcd->dsize = MPC_DMA_TSIZE_4;
+
+		len = sg_dma_len(sg);
+
+		if (mchan->tcd_nunits)
+			tcd->nbytes = mchan->tcd_nunits * 4;
+		else
+			tcd->nbytes = 64;
+
+		if (!IS_ALIGNED(len, tcd->nbytes))
+			return NULL;
+
+		iter = len / tcd->nbytes;
+		if (iter > ((1 << 15) - 1)) {   /* maximum biter */
+			return NULL; /* len is too big */
+		} 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 the last descriptor */
+	return &mdesc->desc;
+}
+
+static int mpc_dma_device_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
+				  unsigned long arg)
+{
+	struct mpc_dma_chan *mchan = dma_chan_to_mpc_dma_chan(chan);
+	struct dma_slave_config *cfg = (void *)arg;
+
+	switch (cmd) {
+	case DMA_SLAVE_CONFIG:
+		if (cfg->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES &&
+		    cfg->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
+			return -EINVAL;
+
+		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;
+		}
+
+		return 0;
+	default:
+		return -ENOSYS;
+	}
+
+	return -EINVAL;
+}
+
 static int mpc_dma_probe(struct platform_device *op)
 {
 	struct device_node *dn = op->dev.of_node;
@@ -725,9 +855,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.7.10.4

[PATCH RFC 2/8] dma: mpc512x: fix start condition in execute()

[Gerhard Sittig]

adjust the conditions how submitted DMA jobs get started: memory transfers
need to get initiated by an explicit software request, all transfers which
involve peripherals need to reference the external requester line

Signed-off-by: Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org>
---
 drivers/dma/mpc512x_dma.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index f90b717..df10a48 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -272,10 +272,12 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
 		mdma->tcd[cid].e_sg = 1;
 
 	switch (cid) {
-	case 26:
+	default:
+		/* peripherals involved, use external request */
 		out_8(&mdma->regs->dmaserq, cid);
 		break;
 	case 32:
+		/* memory transfer, software provided start signal */
 		out_8(&mdma->regs->dmassrt, cid);
 		break;
 	}
-- 
1.7.10.4

[PATCH RFC 2/8] dma: mpc512x: fix start condition in execute()

[Gerhard Sittig]

adjust the conditions how submitted DMA jobs get started: memory transfers
need to get initiated by an explicit software request, all transfers which
involve peripherals need to reference the external requester line

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 drivers/dma/mpc512x_dma.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index f90b717..df10a48 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -272,10 +272,12 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
 		mdma->tcd[cid].e_sg = 1;
 
 	switch (cid) {
-	case 26:
+	default:
+		/* peripherals involved, use external request */
 		out_8(&mdma->regs->dmaserq, cid);
 		break;
 	case 32:
+		/* memory transfer, software provided start signal */
 		out_8(&mdma->regs->dmassrt, cid);
 		break;
 	}
-- 
1.7.10.4

[PATCH RFC 3/8] dma: mpc512x: support 'terminate all' control request

[Gerhard Sittig]

implement the TERMINATE_ALL request in the device_control() callback
of the DMA engine driver for the MPC512x DMA controller

reword variable initialization to better follow the code path and to
avoid artificial diffs later on (this style change vanishes when this
patch gets squashed with the device_control() routine's introduction)

Signed-off-by: Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org>
---
 drivers/dma/mpc512x_dma.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index df10a48..0053ff8 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -748,11 +748,22 @@ static struct dma_async_tx_descriptor *mpc_dma_prep_slave_sg(
 static int mpc_dma_device_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 				  unsigned long arg)
 {
-	struct mpc_dma_chan *mchan = dma_chan_to_mpc_dma_chan(chan);
-	struct dma_slave_config *cfg = (void *)arg;
+	struct mpc_dma_chan *mchan;
+	struct mpc_dma *mdma;
+	struct dma_slave_config *cfg;
 
+	mchan = dma_chan_to_mpc_dma_chan(chan);
 	switch (cmd) {
+	case DMA_TERMINATE_ALL:
+		/* disable channel requests */
+		mdma = dma_chan_to_mpc_dma(chan);
+		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);
+		return 0;
 	case DMA_SLAVE_CONFIG:
+		cfg = (void *)arg;
 		if (cfg->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES &&
 		    cfg->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
 			return -EINVAL;
-- 
1.7.10.4

[PATCH RFC 3/8] dma: mpc512x: support 'terminate all' control request

[Gerhard Sittig]

implement the TERMINATE_ALL request in the device_control() callback
of the DMA engine driver for the MPC512x DMA controller

reword variable initialization to better follow the code path and to
avoid artificial diffs later on (this style change vanishes when this
patch gets squashed with the device_control() routine's introduction)

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 drivers/dma/mpc512x_dma.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index df10a48..0053ff8 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -748,11 +748,22 @@ static struct dma_async_tx_descriptor *mpc_dma_prep_slave_sg(
 static int mpc_dma_device_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 				  unsigned long arg)
 {
-	struct mpc_dma_chan *mchan = dma_chan_to_mpc_dma_chan(chan);
-	struct dma_slave_config *cfg = (void *)arg;
+	struct mpc_dma_chan *mchan;
+	struct mpc_dma *mdma;
+	struct dma_slave_config *cfg;
 
+	mchan = dma_chan_to_mpc_dma_chan(chan);
 	switch (cmd) {
+	case DMA_TERMINATE_ALL:
+		/* disable channel requests */
+		mdma = dma_chan_to_mpc_dma(chan);
+		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);
+		return 0;
 	case DMA_SLAVE_CONFIG:
+		cfg = (void *)arg;
 		if (cfg->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES &&
 		    cfg->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
 			return -EINVAL;
-- 
1.7.10.4

[PATCH RFC 4/8] dts: mpc512x: prepare for preprocessor support

[Gerhard Sittig]

prepare C preprocessor support when processing MPC512x DTS files
- switch from DTS syntax to CPP syntax for include specs
- create a symlink such that DTS processing can reference includes

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 arch/powerpc/boot/dts/ac14xx.dts          |    2 +-
 arch/powerpc/boot/dts/include/dt-bindings |    1 +
 arch/powerpc/boot/dts/mpc5121ads.dts      |    2 +-
 arch/powerpc/boot/dts/pdm360ng.dts        |    2 +-
 4 files changed, 4 insertions(+), 3 deletions(-)
 create mode 120000 arch/powerpc/boot/dts/include/dt-bindings

diff --git a/arch/powerpc/boot/dts/ac14xx.dts b/arch/powerpc/boot/dts/ac14xx.dts
index a27a460..a543c40 100644
--- a/arch/powerpc/boot/dts/ac14xx.dts
+++ b/arch/powerpc/boot/dts/ac14xx.dts
@@ -10,7 +10,7 @@
  */
 
 
-/include/ "mpc5121.dtsi"
+#include <mpc5121.dtsi>
 
 / {
 	model = "ac14xx";
diff --git a/arch/powerpc/boot/dts/include/dt-bindings b/arch/powerpc/boot/dts/include/dt-bindings
new file mode 120000
index 0000000..08c00e4
--- /dev/null
+++ b/arch/powerpc/boot/dts/include/dt-bindings
@@ -0,0 +1 @@
+../../../../../include/dt-bindings
\ No newline at end of file
diff --git a/arch/powerpc/boot/dts/mpc5121ads.dts b/arch/powerpc/boot/dts/mpc5121ads.dts
index 7d3cb79..c228a0a 100644
--- a/arch/powerpc/boot/dts/mpc5121ads.dts
+++ b/arch/powerpc/boot/dts/mpc5121ads.dts
@@ -9,7 +9,7 @@
  * option) any later version.
  */
 
-/include/ "mpc5121.dtsi"
+#include <mpc5121.dtsi>
 
 / {
 	model = "mpc5121ads";
diff --git a/arch/powerpc/boot/dts/pdm360ng.dts b/arch/powerpc/boot/dts/pdm360ng.dts
index 7433740..871c16d 100644
--- a/arch/powerpc/boot/dts/pdm360ng.dts
+++ b/arch/powerpc/boot/dts/pdm360ng.dts
@@ -13,7 +13,7 @@
  * option) any later version.
  */
 
-/include/ "mpc5121.dtsi"
+#include <mpc5121.dtsi>
 
 / {
 	model = "pdm360ng";
-- 
1.7.10.4

[PATCH RFC 5/8] dma: mpc512x: use symbolic specifiers for DMA channels

[Gerhard Sittig]

for the DMA controller of the MPC512x SoC, DMA channels directly
correspond to specific peripherals, since requester lines directly map
to channels while no additional flexibility or mapping is involved

introduce a dt-bindings header file for MPC512x DMA channels, and make
the shared DT specs in the mpc5121.dtsi as well as the DMA engine driver
use those names instead of numbers

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 arch/powerpc/boot/dts/mpc5121.dtsi    |    6 +++++-
 drivers/dma/mpc512x_dma.c             |    6 ++++--
 include/dt-bindings/dma/mpc512x-dma.h |   21 +++++++++++++++++++++
 3 files changed, 30 insertions(+), 3 deletions(-)
 create mode 100644 include/dt-bindings/dma/mpc512x-dma.h

diff --git a/arch/powerpc/boot/dts/mpc5121.dtsi b/arch/powerpc/boot/dts/mpc5121.dtsi
index bd14c00..384e692 100644
--- a/arch/powerpc/boot/dts/mpc5121.dtsi
+++ b/arch/powerpc/boot/dts/mpc5121.dtsi
@@ -9,6 +9,8 @@
  * option) any later version.
  */
 
+#include <dt-bindings/dma/mpc512x-dma.h>
+
 /dts-v1/;
 
 / {
@@ -152,7 +154,7 @@
 			compatible = "fsl,mpc5121-sdhc";
 			reg = <0x1500 0x100>;
 			interrupts = <8 0x8>;
-			dmas = <&dma0 30>;
+			dmas = <&dma0 MPC512x_DMACHAN_SDHC>;
 			dma-names = "rx-tx";
 		};
 
@@ -262,6 +264,8 @@
 		lpc@10000 {
 			compatible = "fsl,mpc5121-lpc";
 			reg = <0x10000 0x200>;
+			dmas = <&dma0 MPC512x_DMACHAN_SCLPC>;
+			dma-names = "rx-tx";
 		};
 
 		pata@10200 {
diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index 0053ff8..8f6d545 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -29,6 +29,8 @@
  * file called COPYING.
  */
 
+#include <dt-bindings/dma/mpc512x-dma.h>
+
 #include <linux/module.h>
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
@@ -257,7 +259,7 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
 		prev->tcd->dlast_sga = mdesc->tcd_paddr;
 		prev->tcd->e_sg = 1;
 		/* only start explicitly on MDDRC channel */
-		if (cid == 32)
+		if (cid == MPC512x_DMACHAN_MDDRC)
 			mdesc->tcd->start = 1;
 
 		prev = mdesc;
@@ -276,7 +278,7 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
 		/* peripherals involved, use external request */
 		out_8(&mdma->regs->dmaserq, cid);
 		break;
-	case 32:
+	case MPC512x_DMACHAN_MDDRC:
 		/* memory transfer, software provided start signal */
 		out_8(&mdma->regs->dmassrt, cid);
 		break;
diff --git a/include/dt-bindings/dma/mpc512x-dma.h b/include/dt-bindings/dma/mpc512x-dma.h
new file mode 100644
index 0000000..56b06d1
--- /dev/null
+++ b/include/dt-bindings/dma/mpc512x-dma.h
@@ -0,0 +1,21 @@
+/*
+ * This header file provides symbolic specifiers for DMA channels
+ * within the MPC512x SoC's DMA controller.  Since requester lines
+ * directly map to channel numbers and no additional flexibility
+ * is involved, DMA channels can be considered directly associated
+ * with individual peripherals.
+ *
+ * This header file gets shared among DT bindings which provide
+ * hardware specs, and driver code which implements supporting logic.
+ */
+
+#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H
+#define _DT_BINDINGS_DMA_MPC512x_DMA_H
+
+#define MPC512x_DMACHAN_SCLPC		26
+#define MPC512x_DMACHAN_SDHC		30
+#define MPC512x_DMACHAN_MDDRC		32
+
+#define MPC512x_DMACHAN_MAX		64
+
+#endif
-- 
1.7.10.4

[PATCH RFC 6/8] dma: of: Add common xlate function for matching by channel id

[Gerhard Sittig]

From: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>

From: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>

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. The filter
function will use this pointer to match only channels which belong to the
specified DMA controller.

Signed-off-by: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
---
 drivers/dma/of-dma.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_dma.h |    4 ++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 7aa0864..d5d528e 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -229,3 +229,50 @@ struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 			&dma_spec->args[0]);
 }
 EXPORT_SYMBOL_GPL(of_dma_simple_xlate);
+
+struct of_dma_filter_by_chan_id_args {
+	struct dma_device *dev;
+	unsigned int chan_id;
+};
+
+static bool of_dma_filter_by_chan_id(struct dma_chan *chan, void *params)
+{
+	struct of_dma_filter_by_chan_id_args *args = params;
+
+	return chan->device == args->dev && chan->chan_id == args->chan_id;
+}
+
+/**
+ * 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 of_dma_filter_by_chan_id_args args;
+	dma_cap_mask_t cap;
+
+	args.dev = ofdma->of_dma_data;
+	if (!args.dev)
+		return NULL;
+
+	if (dma_spec->args_count != 1)
+		return NULL;
+
+	dma_cap_zero(cap);
+	dma_cap_set(DMA_SLAVE, cap);
+
+	args.chan_id = dma_spec->args[0];
+
+	return dma_request_channel(cap, of_dma_filter_by_chan_id, &args);
+}
+EXPORT_SYMBOL_GPL(of_dma_xlate_by_chan_id);
diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h
index 364dda7..b7cf614 100644
--- a/include/linux/of_dma.h
+++ b/include/linux/of_dma.h
@@ -42,6 +42,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)
@@ -67,6 +69,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.7.10.4

[PATCH RFC 6/8] dma: of: Add common xlate function for matching by channel id

[Gerhard Sittig]

From: Lars-Peter Clausen <lars@metafoo.de>

From: Lars-Peter Clausen <lars@metafoo.de>

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. The filter
function will use this pointer to match only channels which belong to the
specified DMA controller.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/dma/of-dma.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_dma.h |    4 ++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 7aa0864..d5d528e 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -229,3 +229,50 @@ struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 			&dma_spec->args[0]);
 }
 EXPORT_SYMBOL_GPL(of_dma_simple_xlate);
+
+struct of_dma_filter_by_chan_id_args {
+	struct dma_device *dev;
+	unsigned int chan_id;
+};
+
+static bool of_dma_filter_by_chan_id(struct dma_chan *chan, void *params)
+{
+	struct of_dma_filter_by_chan_id_args *args = params;
+
+	return chan->device == args->dev && chan->chan_id == args->chan_id;
+}
+
+/**
+ * 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 of_dma_filter_by_chan_id_args args;
+	dma_cap_mask_t cap;
+
+	args.dev = ofdma->of_dma_data;
+	if (!args.dev)
+		return NULL;
+
+	if (dma_spec->args_count != 1)
+		return NULL;
+
+	dma_cap_zero(cap);
+	dma_cap_set(DMA_SLAVE, cap);
+
+	args.chan_id = dma_spec->args[0];
+
+	return dma_request_channel(cap, of_dma_filter_by_chan_id, &args);
+}
+EXPORT_SYMBOL_GPL(of_dma_xlate_by_chan_id);
diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h
index 364dda7..b7cf614 100644
--- a/include/linux/of_dma.h
+++ b/include/linux/of_dma.h
@@ -42,6 +42,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)
@@ -67,6 +69,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.7.10.4

[PATCH RFC 7/8] dma: mpc512x: register for device tree channel lookup

[Gerhard Sittig]

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

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 .../devicetree/bindings/dma/mpc512x-dma.txt        |   55 ++++++++++++++++++++
 arch/powerpc/boot/dts/mpc5121.dtsi                 |    1 +
 drivers/dma/mpc512x_dma.c                          |   21 ++++++--
 3 files changed, 74 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/mpc512x-dma.txt

diff --git a/Documentation/devicetree/bindings/dma/mpc512x-dma.txt b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
new file mode 100644
index 0000000..a4867d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
@@ -0,0 +1,55 @@
+* Freescale MPC512x DMA Controller
+
+The DMA controller in the Freescale MPC512x SoC can move blocks of
+memory contents between memory and peripherals or memory to memory.
+
+Refer to the "Generic DMA Controller and DMA request bindings" description
+in the dma.txt file for a more detailled discussion of the binding.  The
+MPC512x DMA engine binding follows the common scheme, but doesn't provide
+support for the optional channels and requests counters (those values are
+derived from the detected hardware features) and has a fixed client
+specifier length of 1 integer cell (the value is the DMA channel, since
+the DMA controller uses a fixed assignment of request lines per channel).
+
+
+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 384e692..dae99b7 100644
--- a/arch/powerpc/boot/dts/mpc5121.dtsi
+++ b/arch/powerpc/boot/dts/mpc5121.dtsi
@@ -394,6 +394,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 8f6d545..3d79e3a 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -38,6 +38,7 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/of_device.h>
+#include <linux/of_dma.h>
 #include <linux/of_platform.h>
 
 #include <linux/random.h>
@@ -939,11 +940,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;
 }
 
@@ -952,6 +965,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.7.10.4

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

[Gerhard Sittig]

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>
---
 drivers/mmc/host/mxcmmc.c |   41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index d503635..b9d21cc 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -1121,20 +1121,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);
 
@@ -1204,7 +1213,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));
 
@@ -1255,7 +1266,8 @@ static int mxcmci_suspend(struct device *dev)
 	if (mmc)
 		ret = mmc_suspend_host(mmc);
 	clk_disable_unprepare(host->clk_per);
-	clk_disable_unprepare(host->clk_ipg);
+	if (host->clk_ipg)
+		clk_disable_unprepare(host->clk_ipg);
 
 	return ret;
 }
@@ -1267,7 +1279,8 @@ static int mxcmci_resume(struct device *dev)
 	int ret = 0;
 
 	clk_prepare_enable(host->clk_per);
-	clk_prepare_enable(host->clk_ipg);
+	if (host->clk_ipg)
+		clk_prepare_enable(host->clk_ipg);
 	if (mmc)
 		ret = mmc_resume_host(mmc);
 
-- 
1.7.10.4

Re: [PATCH RFC 0/8] MPC512x DMA slave s/g support, OF DMA lookup

[Lars-Peter Clausen]

On 07/12/2013 05:26 PM, Gerhard Sittig wrote:
[...]
> Lars, there is a checkpatch warning about a line longer than 80
> characters in the common xlate part -- I didn't dare to change your
> submission, and the part included here is verbatim from your patchwork
> 2331091 (original submission) and 2555701 (resend), is there a followup
> which you can provide or point us to?

Well the line is 81 characters long, so I wouldn't worry to much about that.
But we should probably remove the extern from all the function declarations
in dma-of.h, since the extern is kind of implicit for functions declarations.

- Lars

Re: [PATCH RFC 0/8] MPC512x DMA slave s/g support, OF DMA lookup

[Lars-Peter Clausen]

On 07/12/2013 05:26 PM, Gerhard Sittig wrote:
[...]
> Lars, there is a checkpatch warning about a line longer than 80
> characters in the common xlate part -- I didn't dare to change your
> submission, and the part included here is verbatim from your patchwork
> 2331091 (original submission) and 2555701 (resend), is there a followup
> which you can provide or point us to?

Well the line is 81 characters long, so I wouldn't worry to much about that.
But we should probably remove the extern from all the function declarations
in dma-of.h, since the extern is kind of implicit for functions declarations.

- Lars

Re: [PATCH RFC 5/8] dma: mpc512x: use symbolic specifiers for DMA channels

[Arnd Bergmann]

On Friday 12 July 2013, Gerhard Sittig wrote:
> +++ b/include/dt-bindings/dma/mpc512x-dma.h
> @@ -0,0 +1,21 @@
> +/*
> + * This header file provides symbolic specifiers for DMA channels
> + * within the MPC512x SoC's DMA controller.  Since requester lines
> + * directly map to channel numbers and no additional flexibility
> + * is involved, DMA channels can be considered directly associated
> + * with individual peripherals.
> + *
> + * This header file gets shared among DT bindings which provide
> + * hardware specs, and driver code which implements supporting logic.
> + */
> +
> +#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H
> +#define _DT_BINDINGS_DMA_MPC512x_DMA_H
> +
> +#define MPC512x_DMACHAN_SCLPC          26
> +#define MPC512x_DMACHAN_SDHC           30
> +#define MPC512x_DMACHAN_MDDRC          32
> +
> +#define MPC512x_DMACHAN_MAX            64
> +

I think these should not be in the header and should not bve part of the
binding either. They are specific to an SoC that happens to be using this
DMA controller but would be completely different for any other SoC with
the same DMA engine. These belong into the dma descriptors of the slave
drivers and don't need symbolic names.

	Arnd

Re: [PATCH RFC 5/8] dma: mpc512x: use symbolic specifiers for DMA channels

[Arnd Bergmann]

On Friday 12 July 2013, Gerhard Sittig wrote:
> +++ b/include/dt-bindings/dma/mpc512x-dma.h
> @@ -0,0 +1,21 @@
> +/*
> + * This header file provides symbolic specifiers for DMA channels
> + * within the MPC512x SoC's DMA controller.  Since requester lines
> + * directly map to channel numbers and no additional flexibility
> + * is involved, DMA channels can be considered directly associated
> + * with individual peripherals.
> + *
> + * This header file gets shared among DT bindings which provide
> + * hardware specs, and driver code which implements supporting logic.
> + */
> +
> +#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H
> +#define _DT_BINDINGS_DMA_MPC512x_DMA_H
> +
> +#define MPC512x_DMACHAN_SCLPC          26
> +#define MPC512x_DMACHAN_SDHC           30
> +#define MPC512x_DMACHAN_MDDRC          32
> +
> +#define MPC512x_DMACHAN_MAX            64
> +

I think these should not be in the header and should not bve part of the
binding either. They are specific to an SoC that happens to be using this
DMA controller but would be completely different for any other SoC with
the same DMA engine. These belong into the dma descriptors of the slave
drivers and don't need symbolic names.

	Arnd

Re: [PATCH RFC 5/8] dma: mpc512x: use symbolic specifiers for DMA channels

[Gerhard Sittig]

[ MPC8308 knowledge required, see below ]

On Sat, Jul 13, 2013 at 09:17 +0200, Arnd Bergmann wrote:
> 
> On Friday 12 July 2013, Gerhard Sittig wrote:
> > +++ b/include/dt-bindings/dma/mpc512x-dma.h
> > @@ -0,0 +1,21 @@
> > +/*
> > + * This header file provides symbolic specifiers for DMA channels
> > + * within the MPC512x SoC's DMA controller.  Since requester lines
> > + * directly map to channel numbers and no additional flexibility
> > + * is involved, DMA channels can be considered directly associated
> > + * with individual peripherals.
> > + *
> > + * This header file gets shared among DT bindings which provide
> > + * hardware specs, and driver code which implements supporting logic.
> > + */
> > +
> > +#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H
> > +#define _DT_BINDINGS_DMA_MPC512x_DMA_H
> > +
> > +#define MPC512x_DMACHAN_SCLPC          26
> > +#define MPC512x_DMACHAN_SDHC           30
> > +#define MPC512x_DMACHAN_MDDRC          32
> > +
> > +#define MPC512x_DMACHAN_MAX            64
> > +
> 
> I think these should not be in the header and should not bve part of the
> binding either. They are specific to an SoC that happens to be using this
> DMA controller but would be completely different for any other SoC with
> the same DMA engine. These belong into the dma descriptors of the slave
> drivers and don't need symbolic names.

Thank you for the feedback.

OK, so not adding the dt-bindings header leads to no change in
the DTS nodes, which in turn collapses 5/8 into something local
to the .c driver source (introduce an enum and replace a few
magic numbers with names), and obsoletes 4/8 as a prerequisite.
This will further reduce the patch set's size.

Your feedback made me re-visit the driver source and notice that
it is shared among the MPC512x as well as the MPC8308 hardware.
The latter only has 16 channels, and appears to _not_ have its
channels dedicated to peripherals.  It's even uncertain to me
whether it can cope with peripherals at all and how so.

I scanned chapter 12 (DMA controller) in the MPC8308 reference
manual (rev 0 as of 2010-04) several times and could not find any
hint about peripherals, request lines, or anything else related
to flow control.  Searching in the whole RM won't give a hint
either.  Does this suggest that the MPC8308 DMA controller's
channels are "free" in their assignment to transfer tasks?  Or
are they "memory transfers only"?  Or do they happily accept any
XLB address (internal and external RAM, IMMR and IP bus space)
but don't apply flow control, i.e. expect either peripherals to
already hold the RX data, or peripherals to keep up with being
fed random amounts of TX data?  I tend to read the doc as the
latter.

Can somebody with MPC8308 knowledge confirm my suspicion that the
MPC8308 DMA channels aren't associated with peripherals, and thus
always need the software initiated start condition (_if_ they are
used for data transfer to/from peripherals)?  Can somebody with
access to an MPC8308 based board test later versions of the
series, to verify we don't break DMA operation on that platform?

Regardless of whether MPC8308 can't handle peripheral access, or
doesn't differ from memory transfers there, the patch series
needs an update.  Part 1/8 in its current form is either wrong or
incomplete, works for MPC512x but not for all hardware that this
driver is responsible for.


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 5/8] dma: mpc512x: use symbolic specifiers for DMA channels

[Arnd Bergmann]

On Saturday 13 July 2013, Gerhard Sittig wrote:
> [ MPC8308 knowledge required, see below ]
> 
> On Sat, Jul 13, 2013 at 09:17 +0200, Arnd Bergmann wrote:
> > 
> > On Friday 12 July 2013, Gerhard Sittig wrote:
> > > +++ b/include/dt-bindings/dma/mpc512x-dma.h
> > > @@ -0,0 +1,21 @@
> > > +/*
> > > + * This header file provides symbolic specifiers for DMA channels
> > > + * within the MPC512x SoC's DMA controller.  Since requester lines
> > > + * directly map to channel numbers and no additional flexibility
> > > + * is involved, DMA channels can be considered directly associated
> > > + * with individual peripherals.
> > > + *
> > > + * This header file gets shared among DT bindings which provide
> > > + * hardware specs, and driver code which implements supporting logic.
> > > + */
> > > +
> > > +#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H
> > > +#define _DT_BINDINGS_DMA_MPC512x_DMA_H
> > > +
> > > +#define MPC512x_DMACHAN_SCLPC          26
> > > +#define MPC512x_DMACHAN_SDHC           30
> > > +#define MPC512x_DMACHAN_MDDRC          32
> > > +
> > > +#define MPC512x_DMACHAN_MAX            64
> > > +
> > 
> > I think these should not be in the header and should not bve part of the
> > binding either. They are specific to an SoC that happens to be using this
> > DMA controller but would be completely different for any other SoC with
> > the same DMA engine. These belong into the dma descriptors of the slave
> > drivers and don't need symbolic names.
> 
> Thank you for the feedback.
> 
> OK, so not adding the dt-bindings header leads to no change in
> the DTS nodes, which in turn collapses 5/8 into something local
> to the .c driver source (introduce an enum and replace a few
> magic numbers with names), and obsoletes 4/8 as a prerequisite.
> This will further reduce the patch set's size.

Actually I think you will need extra changes: The dma-engine driver
should not require knowledge of any channel-specific settings.
I did not notice you had them until you mentioned the above, but
from what I can tell, you need a few flags in the dma-specifier
to replace code like

                /* only start explicitly on MDDRC channel */
-               if (cid == 32)
+               if (cid == MPC512x_DMACHAN_MDDRC)
                        mdesc->tcd->start = 1;

with

		mdesc->tcd->start = dmaspec->explicit_start;

or something along these lines, where dmaspec is a data structure
derived from the fields in the DT dma specifier of the child
node.		

> I scanned chapter 12 (DMA controller) in the MPC8308 reference
> manual (rev 0 as of 2010-04) several times and could not find any
> hint about peripherals, request lines, or anything else related
> to flow control.  Searching in the whole RM won't give a hint
> either.  Does this suggest that the MPC8308 DMA controller's
> channels are "free" in their assignment to transfer tasks?  Or
> are they "memory transfers only"?  Or do they happily accept any
> XLB address (internal and external RAM, IMMR and IP bus space)
> but don't apply flow control, i.e. expect either peripherals to
> already hold the RX data, or peripherals to keep up with being
> fed random amounts of TX data?  I tend to read the doc as the
> latter.

It sounds to me that they are memory-to-memory only, which means
you probably want to allow #dma-cells=<0> as a special case to
describe an instance that has no slave API support.

	Arnd

Re: [PATCH RFC 5/8] dma: mpc512x: use symbolic specifiers for DMA channels

[Lars-Peter Clausen]

On 07/14/2013 10:50 AM, Arnd Bergmann wrote:
> On Saturday 13 July 2013, Gerhard Sittig wrote:
>> [ MPC8308 knowledge required, see below ]
>>
>> On Sat, Jul 13, 2013 at 09:17 +0200, Arnd Bergmann wrote:
>>>
>>> On Friday 12 July 2013, Gerhard Sittig wrote:
>>>> +++ b/include/dt-bindings/dma/mpc512x-dma.h
>>>> @@ -0,0 +1,21 @@
>>>> +/*
>>>> + * This header file provides symbolic specifiers for DMA channels
>>>> + * within the MPC512x SoC's DMA controller.  Since requester lines
>>>> + * directly map to channel numbers and no additional flexibility
>>>> + * is involved, DMA channels can be considered directly associated
>>>> + * with individual peripherals.
>>>> + *
>>>> + * This header file gets shared among DT bindings which provide
>>>> + * hardware specs, and driver code which implements supporting logic.
>>>> + */
>>>> +
>>>> +#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H
>>>> +#define _DT_BINDINGS_DMA_MPC512x_DMA_H
>>>> +
>>>> +#define MPC512x_DMACHAN_SCLPC          26
>>>> +#define MPC512x_DMACHAN_SDHC           30
>>>> +#define MPC512x_DMACHAN_MDDRC          32
>>>> +
>>>> +#define MPC512x_DMACHAN_MAX            64
>>>> +
>>>
>>> I think these should not be in the header and should not bve part of the
>>> binding either. They are specific to an SoC that happens to be using this
>>> DMA controller but would be completely different for any other SoC with
>>> the same DMA engine. These belong into the dma descriptors of the slave
>>> drivers and don't need symbolic names.
>>
>> Thank you for the feedback.
>>
>> OK, so not adding the dt-bindings header leads to no change in
>> the DTS nodes, which in turn collapses 5/8 into something local
>> to the .c driver source (introduce an enum and replace a few
>> magic numbers with names), and obsoletes 4/8 as a prerequisite.
>> This will further reduce the patch set's size.
> 
> Actually I think you will need extra changes: The dma-engine driver
> should not require knowledge of any channel-specific settings.
> I did not notice you had them until you mentioned the above, but
> from what I can tell, you need a few flags in the dma-specifier
> to replace code like
> 
>                 /* only start explicitly on MDDRC channel */
> -               if (cid == 32)
> +               if (cid == MPC512x_DMACHAN_MDDRC)
>                         mdesc->tcd->start = 1;
> 
> with
> 
> 		mdesc->tcd->start = dmaspec->explicit_start;
> 
> or something along these lines, where dmaspec is a data structure
> derived from the fields in the DT dma specifier of the child
> node.		
> 

I think the MDDRC channel is used for mem-to-mem transfers. So there is no
peripheral that is going to request it by handle. If the channel that is
used for mem-to-mem transfers differs between different instances of the
controller (e.g. on different SoCs) it should probably be a property of the
dma controllers DT node.

- Lars

Re: [PATCH RFC 1/8] powerpc: mpc512x_dma: add support for data transfers between memory and i/o memory

[Lars-Peter Clausen]

On 07/12/2013 05:26 PM, Gerhard Sittig wrote:
[...]
]
> +		if (mchan->tcd_nunits)
> +			tcd->nbytes = mchan->tcd_nunits * 4;
> +		else
> +			tcd->nbytes = 64;

Just wondering where does the magic 64 come from?

Re: [PATCH RFC 5/8] dma: mpc512x: use symbolic specifiers for DMA channels

[Gerhard Sittig]

On Sun, Jul 14, 2013 at 10:50 +0200, Arnd Bergmann wrote:
> 
> On Saturday 13 July 2013, Gerhard Sittig wrote:
> > 
> > [ ... ]
> > 
> > Thank you for the feedback.
> > 
> > OK, so not adding the dt-bindings header leads to no change in
> > the DTS nodes, which in turn collapses 5/8 into something local
> > to the .c driver source (introduce an enum and replace a few
> > magic numbers with names), and obsoletes 4/8 as a prerequisite.
> > This will further reduce the patch set's size.
> 
> Actually I think you will need extra changes: The dma-engine driver
> should not require knowledge of any channel-specific settings.
> I did not notice you had them until you mentioned the above, but
> from what I can tell, you need a few flags in the dma-specifier
> to replace code like
> 
>                 /* only start explicitly on MDDRC channel */
> -               if (cid == 32)
> +               if (cid == MPC512x_DMACHAN_MDDRC)
>                         mdesc->tcd->start = 1;
> 
> with
> 
> 		mdesc->tcd->start = dmaspec->explicit_start;
> 
> or something along these lines, where dmaspec is a data structure
> derived from the fields in the DT dma specifier of the child
> node.		

The above change applies to the mpc512x_dma.c file, this is the
very driver source which implements the DMA engine API, and deals
with the specific details of the SoC.  So I consider this the
most appropriate location to handle requirements of specific
channels in the DMA controller's driver, while the generic DMA
engine subsystem just finds the provider's API.

The driver takes care of _one_ DMA controller which has several
channels while these channels in turn might have _different_
characteristics.

Most channels of the MPC512x SoC's DMA controller have request
lines for peripherals to apply flow control, while the 'MDDRC'
channel is dedicated to mem-to-mem transfers and requires a
software initiated start in the absence of an external request
line.

The channels of the MPC8308 SoC's DMA controller appear to not
have any request lines, which makes them perfectly fit for
mem-to-mem transfers and always requires software initiated
start.  Yet this does not absolutely prevent their use for
peripheral access, _provided_ that the peripheral's FIFO is deep
enough or the data volume is appropriately low (or wire speed is
rather high, whatever obsoletes flow control).

I'm in the process of preparing v2 of the series, which keeps
compatibility with the MPC8308 and approaches things slightly
differently than v1 although in the same spirit.


Regarding the device tree binding, I was under the impression
that backwards compatibility is a must.  Reading channel counts
from DT nodes instead of encoding them in the driver would have
been nice upon introduction of OF support, but would break
compatibility these days.


> > I scanned chapter 12 (DMA controller) in the MPC8308 reference
> > manual (rev 0 as of 2010-04) several times and could not find any
> > hint about peripherals, request lines, or anything else related
> > to flow control.  Searching in the whole RM won't give a hint
> > either.  Does this suggest that the MPC8308 DMA controller's
> > channels are "free" in their assignment to transfer tasks?  Or
> > are they "memory transfers only"?  Or do they happily accept any
> > XLB address (internal and external RAM, IMMR and IP bus space)
> > but don't apply flow control, i.e. expect either peripherals to
> > already hold the RX data, or peripherals to keep up with being
> > fed random amounts of TX data?  I tend to read the doc as the
> > latter.
> 
> It sounds to me that they are memory-to-memory only, which means
> you probably want to allow #dma-cells=<0> as a special case to
> describe an instance that has no slave API support.

This would impact the current OF registration, which
unconditionally assumes one-cell specs.  Before changing this to
one-cell for MPC512x and zero-zell for MPC8308, I'd love to learn
whether slave support on MPC8308 is inapplicable or whether it's
doable yet constraints apply (ATM I assume the latter, would not
want to enforce non-availability just to re-introduce it later).


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 1/8] powerpc: mpc512x_dma: add support for data transfers between memory and i/o memory

[Gerhard Sittig]

On Sun, Jul 14, 2013 at 12:05 +0200, Lars-Peter Clausen wrote:
> 
> On 07/12/2013 05:26 PM, Gerhard Sittig wrote:
> [...]
> ]
> > +		if (mchan->tcd_nunits)
> > +			tcd->nbytes = mchan->tcd_nunits * 4;
> > +		else
> > +			tcd->nbytes = 64;
> 
> Just wondering where does the magic 64 come from?

"Inherited" from Alexander's submission.  By coincidence the SDHC
client happened to work with this constraint, as did Alexander's
LPB test suite.  I'm not aware of other DMA clients as of now
(except for the memory transfer, which runs a different prep
routine).

It looks like there is more potential for generalization and auto
adapting to user input (adjusting configurable inner-loop
parameters to exactly fullfill the overall job from the user's
spec yet meet the chunking constraints of the 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-ynQEQJNshbs@public.gmane.org

Re: [PATCH RFC 1/8] powerpc: mpc512x_dma: add support for data transfers between memory and i/o memory

[Gerhard Sittig]

On Sun, Jul 14, 2013 at 12:05 +0200, Lars-Peter Clausen wrote:
> 
> On 07/12/2013 05:26 PM, Gerhard Sittig wrote:
> [...]
> ]
> > +		if (mchan->tcd_nunits)
> > +			tcd->nbytes = mchan->tcd_nunits * 4;
> > +		else
> > +			tcd->nbytes = 64;
> 
> Just wondering where does the magic 64 come from?

"Inherited" from Alexander's submission.  By coincidence the SDHC
client happened to work with this constraint, as did Alexander's
LPB test suite.  I'm not aware of other DMA clients as of now
(except for the memory transfer, which runs a different prep
routine).

It looks like there is more potential for generalization and auto
adapting to user input (adjusting configurable inner-loop
parameters to exactly fullfill the overall job from the user's
spec yet meet the chunking constraints of the 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

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

[Gerhard Sittig]

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 since v1:
- 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

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
- currently encoded constraints do work for SD card and LPB test suite
  (all known DMA clients ATM), but will need more tuning or support for
  automatic adjustment for transfers of arbitrary length

oh, and I'd like to get feedback on whether attribution handling is
appropriate, as I had to squash patches from several authors to not
break bisectability (compilation had worked, but the code had not run
successfully on all previously supported hardware)

Alexander, when you pickup this series and improve slave s/g support and
adjust it for more general use, you have my OK to incorporate and
further adjust the parts that I've contributed


Alexander Popov (1):
  dma: mpc512x: add support for peripheral transfers

Gerhard Sittig (3):
  dma: mpc512x: re-order mpc8308 specific instructions
  dma: mpc512x: register for device tree channel lookup
  HACK mmc: mxcmmc: enable clocks for the MPC512x

Lars-Peter Clausen (1):
  dma: of: Add common xlate function for matching by channel id

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

-- 
1.7.10.4

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

[Gerhard Sittig]

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 since v1:
- 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

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
- currently encoded constraints do work for SD card and LPB test suite
  (all known DMA clients ATM), but will need more tuning or support for
  automatic adjustment for transfers of arbitrary length

oh, and I'd like to get feedback on whether attribution handling is
appropriate, as I had to squash patches from several authors to not
break bisectability (compilation had worked, but the code had not run
successfully on all previously supported hardware)

Alexander, when you pickup this series and improve slave s/g support and
adjust it for more general use, you have my OK to incorporate and
further adjust the parts that I've contributed


Alexander Popov (1):
  dma: mpc512x: add support for peripheral transfers

Gerhard Sittig (3):
  dma: mpc512x: re-order mpc8308 specific instructions
  dma: mpc512x: register for device tree channel lookup
  HACK mmc: mxcmmc: enable clocks for the MPC512x

Lars-Peter Clausen (1):
  dma: of: Add common xlate function for matching by channel id

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

-- 
1.7.10.4

[PATCH RFC v2 1/5] dma: mpc512x: re-order mpc8308 specific instructions

[Gerhard Sittig]

it's rather unexpected to have the MPC8308 specific code in the 'else'
branch so distant from the "is MPC8308?" check

concentrate the test and the specific code for MPC8308 in the 'if'
branch and handle MPC512x in the 'else' branch; use a symbolic channel
count which in combination with the re-ordering obsoletes a comment

this modification only re-orders instructions but doesn't change behaviour

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 drivers/dma/mpc512x_dma.c |   48 +++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index 2d95673..4e03f1e 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -50,9 +50,23 @@
 #define MPC_DMA_DESCRIPTORS	64
 
 /* Macro definitions */
-#define MPC_DMA_CHANNELS	64
 #define MPC_DMA_TCD_OFFSET	0x1000
 
+/*
+ * the maximum channel count, and specific channels which need
+ * special processing, for individual hardware variants
+ *
+ * and the maximum channel count over all supported controllers,
+ * used for data structure sizes
+ */
+enum mpc8308_dmachan_id_t {
+	MPC8308_DMACHAN_MAX = 16,
+};
+enum mpc512x_dmachan_id_t {
+	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)
@@ -716,10 +730,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;
@@ -753,7 +767,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);
 
@@ -774,18 +800,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.7.10.4

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

[Gerhard Sittig]

From: Alexander Popov <a13xp0p0v88@gmail.com>

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

refuse to prepare chunked transfers (transfers with more than one part)
as long as proper support for scatter/gather is lacking

keep MPC8308 operational by always starting transfers from software,
this SoC appears to not have request lines for flow control when
peripherals are involved in transfers

[ introduction of slave s/g preparation and device control ]
Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
[ execute() start condition, mpc8308 compat, terminate all, s/g length check, reworded commit msg ]
Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 drivers/dma/mpc512x_dma.c |  168 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 161 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index 4e03f1e..55e6a87 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
@@ -28,11 +29,6 @@
  * file called COPYING.
  */
 
-/*
- * This is initial version of MPC5121 DMA driver. Only memory to memory
- * transfers are supported (tested using dmatest module).
- */
-
 #include <linux/module.h>
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
@@ -63,6 +59,7 @@ enum mpc8308_dmachan_id_t {
 	MPC8308_DMACHAN_MAX = 16,
 };
 enum mpc512x_dmachan_id_t {
+	MPC512x_DMACHAN_MDDRC = 32,
 	MPC512x_DMACHAN_MAX = 64,
 };
 #define MPC_DMA_CHANNELS	64
@@ -204,9 +201,13 @@ struct mpc_dma_chan {
 	struct list_head		completed;
 	struct mpc_dma_tcd		*tcd;
 	dma_addr_t			tcd_paddr;
+	u32				tcd_nunits;
 
 	/* Lock for this structure */
 	spinlock_t			lock;
+
+	/* Channel's peripheral fifo address */
+	dma_addr_t			per_paddr;
 };
 
 struct mpc_dma {
@@ -270,7 +271,12 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
 
 		prev->tcd->dlast_sga = mdesc->tcd_paddr;
 		prev->tcd->e_sg = 1;
-		mdesc->tcd->start = 1;
+
+		/* software start for mem-to-mem transfers */
+		if (mdma->is_mpc8308)
+			mdesc->tcd->start = 1;
+		else if (cid == MPC512x_DMACHAN_MDDRC)
+			mdesc->tcd->start = 1;
 
 		prev = mdesc;
 	}
@@ -282,7 +288,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 (cid == MPC512x_DMACHAN_MDDRC) {
+		/* memory to memory transfer, software initiated start */
+		out_8(&mdma->regs->dmassrt, cid);
+	} else {
+		/* peripherals involved, use external request line */
+		out_8(&mdma->regs->dmaserq, cid);
+	}
 }
 
 /* Handle interrupt on one half of DMA controller (32 channels) */
@@ -655,6 +671,141 @@ 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;
+	struct mpc_dma_tcd *tcd;
+	unsigned long iflags;
+	struct scatterlist *sg;
+	size_t len;
+	int iter, i;
+
+	if (!list_empty(&mchan->active))
+		return NULL;
+
+	/* 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);
+
+		spin_unlock_irqrestore(&mchan->lock, iflags);
+
+		mdesc->error = 0;
+		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))
+			return NULL;
+
+		if (direction == DMA_DEV_TO_MEM) {
+			tcd->saddr = mchan->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 = mchan->per_paddr;
+			tcd->soff = 4;
+			tcd->doff = 0;
+		} else {
+			return NULL;
+		}
+		tcd->ssize = MPC_DMA_TSIZE_4;
+		tcd->dsize = MPC_DMA_TSIZE_4;
+
+		len = sg_dma_len(sg);
+
+		if (mchan->tcd_nunits)
+			tcd->nbytes = mchan->tcd_nunits * 4;
+		else
+			tcd->nbytes = 64;
+
+		if (!IS_ALIGNED(len, tcd->nbytes))
+			return NULL;
+
+		iter = len / tcd->nbytes;
+		if (iter > ((1 << 15) - 1)) {   /* maximum biter */
+			return NULL; /* len is too big */
+		} 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 the last descriptor */
+	return &mdesc->desc;
+}
+
+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;
+
+	mchan = dma_chan_to_mpc_dma_chan(chan);
+	switch (cmd) {
+	case DMA_TERMINATE_ALL:
+		/* disable channel requests */
+		mdma = dma_chan_to_mpc_dma(chan);
+		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);
+		return 0;
+	case DMA_SLAVE_CONFIG:
+		cfg = (void *)arg;
+		if (cfg->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES &&
+		    cfg->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
+			return -EINVAL;
+
+		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;
+		}
+
+		return 0;
+	default:
+		return -ENOSYS;
+	}
+
+	return -EINVAL;
+}
+
 static int mpc_dma_probe(struct platform_device *op)
 {
 	struct device_node *dn = op->dev.of_node;
@@ -739,9 +890,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.7.10.4

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

[Gerhard Sittig]

From: Lars-Peter Clausen <lars@metafoo.de>

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. The filter
function will use this pointer to match only channels which belong to the
specified DMA controller.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 drivers/dma/of-dma.c   |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_dma.h |    4 ++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 7aa0864..d5d528e 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -229,3 +229,50 @@ struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 			&dma_spec->args[0]);
 }
 EXPORT_SYMBOL_GPL(of_dma_simple_xlate);
+
+struct of_dma_filter_by_chan_id_args {
+	struct dma_device *dev;
+	unsigned int chan_id;
+};
+
+static bool of_dma_filter_by_chan_id(struct dma_chan *chan, void *params)
+{
+	struct of_dma_filter_by_chan_id_args *args = params;
+
+	return chan->device == args->dev && chan->chan_id == args->chan_id;
+}
+
+/**
+ * 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 of_dma_filter_by_chan_id_args args;
+	dma_cap_mask_t cap;
+
+	args.dev = ofdma->of_dma_data;
+	if (!args.dev)
+		return NULL;
+
+	if (dma_spec->args_count != 1)
+		return NULL;
+
+	dma_cap_zero(cap);
+	dma_cap_set(DMA_SLAVE, cap);
+
+	args.chan_id = dma_spec->args[0];
+
+	return dma_request_channel(cap, of_dma_filter_by_chan_id, &args);
+}
+EXPORT_SYMBOL_GPL(of_dma_xlate_by_chan_id);
diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h
index 364dda7..b7cf614 100644
--- a/include/linux/of_dma.h
+++ b/include/linux/of_dma.h
@@ -42,6 +42,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)
@@ -67,6 +69,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.7.10.4

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

[Gerhard Sittig]

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

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 .../devicetree/bindings/dma/mpc512x-dma.txt        |   55 ++++++++++++++++++++
 arch/powerpc/boot/dts/mpc5121.dtsi                 |    1 +
 drivers/dma/mpc512x_dma.c                          |   21 ++++++--
 3 files changed, 74 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/mpc512x-dma.txt

diff --git a/Documentation/devicetree/bindings/dma/mpc512x-dma.txt b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
new file mode 100644
index 0000000..a4867d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
@@ -0,0 +1,55 @@
+* Freescale MPC512x DMA Controller
+
+The DMA controller in the Freescale MPC512x SoC can move blocks of
+memory contents between memory and peripherals or memory to memory.
+
+Refer to the "Generic DMA Controller and DMA request bindings" description
+in the dma.txt file for a more detailled discussion of the binding.  The
+MPC512x DMA engine binding follows the common scheme, but doesn't provide
+support for the optional channels and requests counters (those values are
+derived from the detected hardware features) and has a fixed client
+specifier length of 1 integer cell (the value is the DMA channel, since
+the DMA controller uses a fixed assignment of request lines per channel).
+
+
+DMA controller node properties:
+
+Required properties:
+- compatible:		should be "fsl,mpc5121-dma"
+- reg:			address and size of the DMA controller's register set
+- interrupts:		interrupt spec for the DMA controller
+
+Optional properties:
+- #dma-cells:		must be <1>, describes the number of integer cells
+			needed to specify the 'dmas' property in client nodes,
+			strongly recommended since common client helper code
+			uses this property
+
+Example:
+
+	dma0: dma@14000 {
+		compatible = "fsl,mpc5121-dma";
+		reg = <0x14000 0x1800>;
+		interrupts = <65 0x8>;
+		#dma-cells = <1>;
+	};
+
+
+Client node properties:
+
+Required properties:
+- dmas:			list of DMA specifiers, consisting each of a handle
+			for the DMA controller and integer cells to specify
+			the channel used within the DMA controller
+- dma-names:		list of identifier strings for the DMA specifiers,
+			client device driver code uses these strings to
+			have DMA channels looked up at the controller
+
+Example:
+
+	sdhc@1500 {
+		compatible = "fsl,mpc5121-sdhc";
+		/* ... */
+		dmas = <&dma0 30>;
+		dma-names = "rx-tx";
+	};
diff --git a/arch/powerpc/boot/dts/mpc5121.dtsi b/arch/powerpc/boot/dts/mpc5121.dtsi
index bd14c00..eb0058f 100644
--- a/arch/powerpc/boot/dts/mpc5121.dtsi
+++ b/arch/powerpc/boot/dts/mpc5121.dtsi
@@ -390,6 +390,7 @@
 			compatible = "fsl,mpc5121-dma";
 			reg = <0x14000 0x1800>;
 			interrupts = <65 0x8>;
+			#dma-cells = <1>;
 		};
 	};
 
diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index 55e6a87..84fd07b 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -36,6 +36,7 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/of_device.h>
+#include <linux/of_dma.h>
 #include <linux/of_platform.h>
 
 #include <linux/random.h>
@@ -959,11 +960,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;
 }
 
@@ -972,6 +985,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.7.10.4

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

[Gerhard Sittig]

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>
---
 drivers/mmc/host/mxcmmc.c |   41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index d503635..b9d21cc 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -1121,20 +1121,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);
 
@@ -1204,7 +1213,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));
 
@@ -1255,7 +1266,8 @@ static int mxcmci_suspend(struct device *dev)
 	if (mmc)
 		ret = mmc_suspend_host(mmc);
 	clk_disable_unprepare(host->clk_per);
-	clk_disable_unprepare(host->clk_ipg);
+	if (host->clk_ipg)
+		clk_disable_unprepare(host->clk_ipg);
 
 	return ret;
 }
@@ -1267,7 +1279,8 @@ static int mxcmci_resume(struct device *dev)
 	int ret = 0;
 
 	clk_prepare_enable(host->clk_per);
-	clk_prepare_enable(host->clk_ipg);
+	if (host->clk_ipg)
+		clk_prepare_enable(host->clk_ipg);
 	if (mmc)
 		ret = mmc_resume_host(mmc);
 
-- 
1.7.10.4

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

[Alexander Popov]

Hello everyone!

Hello Gerhard!
Thanks for your work.

2013/7/14 Gerhard Sittig <gsi@denx.de>:
> known issues:
> - currently encoded constraints do work for SD card and LPB test suite
>   (all known DMA clients ATM), but will need more tuning or support for
>   automatic adjustment for transfers of arbitrary length

NBYTES of data is read from / written to DMA client's port in one burst
after DMA controller receives service request from that DMA client.

Different DMA clients want different NBYTES:
SCLPC wants 4 and SD card wants 64, other clients might want something
different.
So having default case with magic number 64 is totally wrong.

What if we simply remove it from [PATCH RFC v2 2/5]:

               len = sg_dma_len(sg);

               if (mchan->tcd_nunits)
                       tcd->nbytes = mchan->tcd_nunits * 4;
-               else
-                       tcd->nbytes = 64;

               if (!IS_ALIGNED(len, tcd->nbytes))
                       return NULL;

and make SD card driver use fields
src_addr_width, dst_addr_width and src_maxburst / dst_maxburst
of dma_slave_config which is a part of standard API (dmaengine.h):

#define DEFAULT_WORDS_PER_TRANSFER    16
...

struct dma_slave_config dma_conf = {};
...

if (dma_conf.direction = DMA_MEM_TO_DEV) {
        dma_conf.dst_maxburst = DEFAULT_WORDS_PER_TRANSFER;
} else {
        dma_conf.src_maxburst = DEFAULT_WORDS_PER_TRANSFER;
}

dma_conf.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
dma_conf.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
...

if (dma_dev->device_control(chan, DMA_SLAVE_CONFIG,
                (unsigned long)&dma_conf)) {
        goto err_dma_prep;
}

This code for SD card driver would be very similar to the code from
SCLPC driver.

Best regards,
Alexander

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

[Lars-Peter Clausen]

On 07/14/2013 02:01 PM, Gerhard Sittig wrote:
> From: Alexander Popov <a13xp0p0v88@gmail.com>
> 
> 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
> 
> refuse to prepare chunked transfers (transfers with more than one part)
> as long as proper support for scatter/gather is lacking
> 
> keep MPC8308 operational by always starting transfers from software,
> this SoC appears to not have request lines for flow control when
> peripherals are involved in transfers

I had a look at the current driver and it seems that any channel can be used
for memcpy operation not only the MDDRC channel. Since the dmaengine API
will just pick one of the currently free channels when performing a memcpy
operation I think this patch breaks memcpy operations. You probably need to
register two dma controllers, one for memcpy operations one for slave
operations, that way you can ensure that only the MDDRC channel is used for
memcpy operations.

> 
> [ introduction of slave s/g preparation and device control ]
> Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
> [ execute() start condition, mpc8308 compat, terminate all, s/g length check, reworded commit msg ]
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
[...]
> +
> +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;
> +
> +	mchan = dma_chan_to_mpc_dma_chan(chan);
> +	switch (cmd) {
> +	case DMA_TERMINATE_ALL:
> +		/* disable channel requests */
> +		mdma = dma_chan_to_mpc_dma(chan);
> +		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);

This probably need locking.

> +		return 0;
[...]
> +}
> +

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

[Gerhard Sittig]

On Tue, Jul 16, 2013 at 12:37 +0200, Lars-Peter Clausen wrote:
> 
> On 07/14/2013 02:01 PM, Gerhard Sittig wrote:
> > From: Alexander Popov <a13xp0p0v88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 
> > 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
> > 
> > refuse to prepare chunked transfers (transfers with more than one part)
> > as long as proper support for scatter/gather is lacking
> > 
> > keep MPC8308 operational by always starting transfers from software,
> > this SoC appears to not have request lines for flow control when
> > peripherals are involved in transfers
> 
> I had a look at the current driver and it seems that any channel can be used
> for memcpy operation not only the MDDRC channel. Since the dmaengine API
> will just pick one of the currently free channels when performing a memcpy
> operation I think this patch breaks memcpy operations. You probably need to
> register two dma controllers, one for memcpy operations one for slave
> operations, that way you can ensure that only the MDDRC channel is used for
> memcpy operations.

OK, so the need for explicit start in software or external
request by the peripheral remains, but the condition for the
choice is different.  It might become a flag attached to the DMA
channel, that gets setup in the prep routines (memory and slave
access each use their own prep calls) and gets evaluated in
execute.  Something like "will access memory", or "will access
peripheral" (the latter may be preferrable since slave support is
the new feature).

This assumes that the non-MDDRC channels can get used for memory
transfers as well, which your description of the the former use
suggests, when slave support was absent.

Making the MDDRC channel preferrable for memcpy operations is
orthogonal to that.

> > 
> > [ introduction of slave s/g preparation and device control ]
> > Signed-off-by: Alexander Popov <a13xp0p0v88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > [ execute() start condition, mpc8308 compat, terminate all, s/g length check, reworded commit msg ]
> > Signed-off-by: Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org>
> > ---
> [...]


> > +
> > +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;
> > +
> > +	mchan = dma_chan_to_mpc_dma_chan(chan);
> > +	switch (cmd) {
> > +	case DMA_TERMINATE_ALL:
> > +		/* disable channel requests */
> > +		mdma = dma_chan_to_mpc_dma(chan);
> > +		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);
> 
> This probably need locking.

Ah, yes.  It needs to grab the same lock as all the other list
manipulations in the driver's source.


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-ynQEQJNshbs@public.gmane.org

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

[Gerhard Sittig]

On Tue, Jul 16, 2013 at 12:37 +0200, Lars-Peter Clausen wrote:
> 
> On 07/14/2013 02:01 PM, Gerhard Sittig wrote:
> > From: Alexander Popov <a13xp0p0v88@gmail.com>
> > 
> > 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
> > 
> > refuse to prepare chunked transfers (transfers with more than one part)
> > as long as proper support for scatter/gather is lacking
> > 
> > keep MPC8308 operational by always starting transfers from software,
> > this SoC appears to not have request lines for flow control when
> > peripherals are involved in transfers
> 
> I had a look at the current driver and it seems that any channel can be used
> for memcpy operation not only the MDDRC channel. Since the dmaengine API
> will just pick one of the currently free channels when performing a memcpy
> operation I think this patch breaks memcpy operations. You probably need to
> register two dma controllers, one for memcpy operations one for slave
> operations, that way you can ensure that only the MDDRC channel is used for
> memcpy operations.

OK, so the need for explicit start in software or external
request by the peripheral remains, but the condition for the
choice is different.  It might become a flag attached to the DMA
channel, that gets setup in the prep routines (memory and slave
access each use their own prep calls) and gets evaluated in
execute.  Something like "will access memory", or "will access
peripheral" (the latter may be preferrable since slave support is
the new feature).

This assumes that the non-MDDRC channels can get used for memory
transfers as well, which your description of the the former use
suggests, when slave support was absent.

Making the MDDRC channel preferrable for memcpy operations is
orthogonal to that.

> > 
> > [ introduction of slave s/g preparation and device control ]
> > Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
> > [ execute() start condition, mpc8308 compat, terminate all, s/g length check, reworded commit msg ]
> > Signed-off-by: Gerhard Sittig <gsi@denx.de>
> > ---
> [...]


> > +
> > +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;
> > +
> > +	mchan = dma_chan_to_mpc_dma_chan(chan);
> > +	switch (cmd) {
> > +	case DMA_TERMINATE_ALL:
> > +		/* disable channel requests */
> > +		mdma = dma_chan_to_mpc_dma(chan);
> > +		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);
> 
> This probably need locking.

Ah, yes.  It needs to grab the same lock as all the other list
manipulations in the driver's source.


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 v2 2/5] dma: mpc512x: add support for peripheral transfers

[Alexander Popov]

Hello everyone!

I've just sent v3 of part 1 and 2 of RFC series:
https://patchwork.kernel.org/patch/2836123/
https://patchwork.kernel.org/patch/2836124/


2013/7/17 Gerhard Sittig <gsi@denx.de>:
> OK, so the need for explicit start in software or external
> request by the peripheral remains, but the condition for the
> choice is different. It might become a flag attached to the DMA
> channel, that gets setup in the prep routines (memory and slave
> access each use their own prep calls) and gets evaluated in
> execute.

I made a flag "will_access_peripheral" be set up in the device control callback.


> non-MDDRC channels can get used for memory
> transfers as well, which your description of the the former use
> suggests, when slave support was absent.

I tested v3 on MPC5125 with SCLPC driver (transfers between dev and
mem work fine)
and dmatest module (all 64 DMA channels can perform mem-to-mem transfers).


>> > +   mchan = dma_chan_to_mpc_dma_chan(chan);
>> > +   switch (cmd) {
>> > +   case DMA_TERMINATE_ALL:
>> > +           /* disable channel requests */
>> > +           mdma = dma_chan_to_mpc_dma(chan);
>> > +           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);
>>
>> This probably need locking.
>
> Ah, yes.  It needs to grab the same lock as all the other list
> manipulations in the driver's source.

Fixed that too.

Best regards,
Alexander.

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

[Alexander Popov]

Hello everyone!

Changes offered in this letter:

- Adding 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().

- Adding locking and removing default nbytes value according
recommendations of Lars-Peter Clausen.

I tested these changes on MPC5125
with SCLPC driver (transfers between dev and mem work fine)
and dmatest module (all 64 DMA channels can perform mem-to-mem transfers
which can be chained in one DMA transaction).


2013/7/14 Gerhard Sittig <gsi@denx.de>:
> From: Alexander Popov <a13xp0p0v88@gmail.com>
>
> 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
>
> refuse to prepare chunked transfers (transfers with more than one part)
> as long as proper support for scatter/gather is lacking
>
> keep MPC8308 operational by always starting transfers from software,
> this SoC appears to not have request lines for flow control when
> peripherals are involved in transfers
>
> [ introduction of slave s/g preparation and device control ]
> Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
> [ execute() start condition, mpc8308 compat, terminate all, s/g length check, reworded commit msg ]
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
>  drivers/dma/mpc512x_dma.c |  168 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 161 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
> index 4e03f1e..55e6a87 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
> @@ -28,11 +29,6 @@
>   * file called COPYING.
>   */
>
> -/*
> - * This is initial version of MPC5121 DMA driver. Only memory to memory
> - * transfers are supported (tested using dmatest module).
> - */
> -
>  #include <linux/module.h>
>  #include <linux/dmaengine.h>
>  #include <linux/dma-mapping.h>
> @@ -63,6 +59,7 @@ enum mpc8308_dmachan_id_t {
>         MPC8308_DMACHAN_MAX = 16,
>  };
>  enum mpc512x_dmachan_id_t {
> +       MPC512x_DMACHAN_MDDRC = 32,
>         MPC512x_DMACHAN_MAX = 64,
>  };
>  #define MPC_DMA_CHANNELS       64

Adding the flag:

@@ -193,6 +183,7 @@ struct mpc_dma_desc {
     dma_addr_t            tcd_paddr;
     int                error;
     struct list_head        node;
+    int                will_access_peripheral;
 };

 struct mpc_dma_chan {


> @@ -204,9 +201,13 @@ struct mpc_dma_chan {
>         struct list_head                completed;
>         struct mpc_dma_tcd              *tcd;
>         dma_addr_t                      tcd_paddr;
> +       u32                             tcd_nunits;
>
>         /* Lock for this structure */
>         spinlock_t                      lock;
> +
> +       /* Channel's peripheral fifo address */
> +       dma_addr_t                      per_paddr;
>  };
>
>  struct mpc_dma {

We can't chain together descriptors of transfers which involve peripherals
because each of such transfers needs hardware initiated start.

@@ -253,10 +248,27 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
     struct mpc_dma_desc *first = NULL;
     struct mpc_dma_desc *prev = NULL;
     struct mpc_dma_desc *mdesc;
+    int staffed = 0;
     int cid = mchan->chan.chan_id;

-    /* Move all queued descriptors to active list */
-    list_splice_tail_init(&mchan->queued, &mchan->active);
+    /*
+     * Mem-to-mem transfers can be chained
+     * together into one transaction.
+     * But each transfer which involves peripherals
+     * must be executed separately.
+     */
+    while (!staffed) {
+        mdesc = list_first_entry(&mchan->queued,
+                        struct mpc_dma_desc, node);
+
+        if (!mdesc->will_access_peripheral)
+            list_move_tail(&mdesc->node, &mchan->active);
+        else {
+            staffed = 1;
+            if (list_empty(&mchan->active))
+                list_move_tail(&mdesc->node, &mchan->active);
+        }
+    }

     /* Chain descriptors into one transaction */
     list_for_each_entry(mdesc, &mchan->active, node) {


> @@ -270,7 +271,12 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
>
>                 prev->tcd->dlast_sga = mdesc->tcd_paddr;
>                 prev->tcd->e_sg = 1;
> -               mdesc->tcd->start = 1;
> +
> +               /* software start for mem-to-mem transfers */
> +               if (mdma->is_mpc8308)
> +                       mdesc->tcd->start = 1;
> +               else if (cid == MPC512x_DMACHAN_MDDRC)
> +                       mdesc->tcd->start = 1;
>
>                 prev = mdesc;
>         }

We don't need this change. Now only mem-to-mem transfers are chained:

@@ -270,6 +282,8 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)

         prev->tcd->dlast_sga = mdesc->tcd_paddr;
         prev->tcd->e_sg = 1;
+
+        /* software start for mem-to-mem transfers */
         mdesc->tcd->start = 1;

         prev = mdesc;


> @@ -282,7 +288,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 (cid == MPC512x_DMACHAN_MDDRC) {

This condition should be changed to:
+    } else if (!first->will_access_peripheral) {

> +               /* memory to memory transfer, software initiated start */
> +               out_8(&mdma->regs->dmassrt, cid);
> +       } else {
> +               /* peripherals involved, use external request line */
> +               out_8(&mdma->regs->dmaserq, cid);
> +       }
>  }
>
>  /* Handle interrupt on one half of DMA controller (32 channels) */

Setting the flag:

@@ -608,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 */


> @@ -655,6 +671,141 @@ 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;

Will be used when the spinlock is grabbed:
+    dma_addr_t per_paddr;
+    u32 tcd_nunits = 0;

> +       struct mpc_dma_tcd *tcd;
> +       unsigned long iflags;
> +       struct scatterlist *sg;
> +       size_t len;
> +       int iter, i;
> +
> +       if (!list_empty(&mchan->active))
> +               return NULL;
> +
> +       /* 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);

While spinlock is grabbed:
+        per_paddr = mchan->per_paddr;
+        tcd_nunits = mchan->tcd_nunits;

> +
> +               spin_unlock_irqrestore(&mchan->lock, iflags);
> +
> +               mdesc->error = 0;

Setting the flag here:
+        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))
> +                       return NULL;
> +
> +               if (direction == DMA_DEV_TO_MEM) {
> +                       tcd->saddr = mchan->per_paddr;

This line should be changed to:
+                       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 = mchan->per_paddr;

Ditto:
+                       tcd->daddr = per_paddr;

> +                       tcd->soff = 4;
> +                       tcd->doff = 0;
> +               } else {
> +                       return NULL;
> +               }
> +               tcd->ssize = MPC_DMA_TSIZE_4;
> +               tcd->dsize = MPC_DMA_TSIZE_4;
> +
> +               len = sg_dma_len(sg);
> +
> +               if (mchan->tcd_nunits)
> +                       tcd->nbytes = mchan->tcd_nunits * 4;
> +               else
> +                       tcd->nbytes = 64;

These 4 lines should be changed to:

+               if (tcd_nunits)
+                       tcd->nbytes = tcd_nunits * 4;
+               else
+                       return NULL;

The last line means that client kernel modules must set
src_maxburst and dst_maxburst fields of dma_slave_config (dmaengine.h).

> +
> +               if (!IS_ALIGNED(len, tcd->nbytes))
> +                       return NULL;
> +
> +               iter = len / tcd->nbytes;
> +               if (iter > ((1 << 15) - 1)) {   /* maximum biter */
> +                       return NULL; /* len is too big */
> +               } 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 the last descriptor */

Offer to remove this comment.

> +       return &mdesc->desc;
> +}
> +
> +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;

Locking:
+    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:
> +               cfg = (void *)arg;
> +               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;
> +               }

+        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;
> @@ -739,9 +890,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.7.10.4
>

Best regards,
Alexander.

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

[Alexander Popov]

Hello everyone!

Changes offered in this letter:

- Adding 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().

- Adding locking and removing default nbytes value according
recommendations of Lars-Peter Clausen.

I tested these changes on MPC5125
with SCLPC driver (transfers between dev and mem work fine)
and dmatest module (all 64 DMA channels can perform mem-to-mem transfers
which can be chained in one DMA transaction).


2013/7/14 Gerhard Sittig <gsi@denx.de>:
> From: Alexander Popov <a13xp0p0v88@gmail.com>
>
> 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
>
> refuse to prepare chunked transfers (transfers with more than one part)
> as long as proper support for scatter/gather is lacking
>
> keep MPC8308 operational by always starting transfers from software,
> this SoC appears to not have request lines for flow control when
> peripherals are involved in transfers
>
> [ introduction of slave s/g preparation and device control ]
> Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
> [ execute() start condition, mpc8308 compat, terminate all, s/g length check, reworded commit msg ]
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
>  drivers/dma/mpc512x_dma.c |  168 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 161 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
> index 4e03f1e..55e6a87 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
> @@ -28,11 +29,6 @@
>   * file called COPYING.
>   */
>
> -/*
> - * This is initial version of MPC5121 DMA driver. Only memory to memory
> - * transfers are supported (tested using dmatest module).
> - */
> -
>  #include <linux/module.h>
>  #include <linux/dmaengine.h>
>  #include <linux/dma-mapping.h>
> @@ -63,6 +59,7 @@ enum mpc8308_dmachan_id_t {
>         MPC8308_DMACHAN_MAX = 16,
>  };
>  enum mpc512x_dmachan_id_t {
> +       MPC512x_DMACHAN_MDDRC = 32,
>         MPC512x_DMACHAN_MAX = 64,
>  };
>  #define MPC_DMA_CHANNELS       64

Adding the flag:

@@ -193,6 +183,7 @@ struct mpc_dma_desc {
     dma_addr_t            tcd_paddr;
     int                error;
     struct list_head        node;
+    int                will_access_peripheral;
 };

 struct mpc_dma_chan {


> @@ -204,9 +201,13 @@ struct mpc_dma_chan {
>         struct list_head                completed;
>         struct mpc_dma_tcd              *tcd;
>         dma_addr_t                      tcd_paddr;
> +       u32                             tcd_nunits;
>
>         /* Lock for this structure */
>         spinlock_t                      lock;
> +
> +       /* Channel's peripheral fifo address */
> +       dma_addr_t                      per_paddr;
>  };
>
>  struct mpc_dma {

We can't chain together descriptors of transfers which involve peripherals
because each of such transfers needs hardware initiated start.

@@ -253,10 +248,27 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
     struct mpc_dma_desc *first = NULL;
     struct mpc_dma_desc *prev = NULL;
     struct mpc_dma_desc *mdesc;
+    int staffed = 0;
     int cid = mchan->chan.chan_id;

-    /* Move all queued descriptors to active list */
-    list_splice_tail_init(&mchan->queued, &mchan->active);
+    /*
+     * Mem-to-mem transfers can be chained
+     * together into one transaction.
+     * But each transfer which involves peripherals
+     * must be executed separately.
+     */
+    while (!staffed) {
+        mdesc = list_first_entry(&mchan->queued,
+                        struct mpc_dma_desc, node);
+
+        if (!mdesc->will_access_peripheral)
+            list_move_tail(&mdesc->node, &mchan->active);
+        else {
+            staffed = 1;
+            if (list_empty(&mchan->active))
+                list_move_tail(&mdesc->node, &mchan->active);
+        }
+    }

     /* Chain descriptors into one transaction */
     list_for_each_entry(mdesc, &mchan->active, node) {


> @@ -270,7 +271,12 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
>
>                 prev->tcd->dlast_sga = mdesc->tcd_paddr;
>                 prev->tcd->e_sg = 1;
> -               mdesc->tcd->start = 1;
> +
> +               /* software start for mem-to-mem transfers */
> +               if (mdma->is_mpc8308)
> +                       mdesc->tcd->start = 1;
> +               else if (cid == MPC512x_DMACHAN_MDDRC)
> +                       mdesc->tcd->start = 1;
>
>                 prev = mdesc;
>         }

We don't need this change. Now only mem-to-mem transfers are chained:

@@ -270,6 +282,8 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)

         prev->tcd->dlast_sga = mdesc->tcd_paddr;
         prev->tcd->e_sg = 1;
+
+        /* software start for mem-to-mem transfers */
         mdesc->tcd->start = 1;

         prev = mdesc;


> @@ -282,7 +288,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 (cid == MPC512x_DMACHAN_MDDRC) {

This condition should be changed to:
+    } else if (!first->will_access_peripheral) {

> +               /* memory to memory transfer, software initiated start */
> +               out_8(&mdma->regs->dmassrt, cid);
> +       } else {
> +               /* peripherals involved, use external request line */
> +               out_8(&mdma->regs->dmaserq, cid);
> +       }
>  }
>
>  /* Handle interrupt on one half of DMA controller (32 channels) */

Setting the flag:

@@ -608,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 */


> @@ -655,6 +671,141 @@ 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;

Will be used when the spinlock is grabbed:
+    dma_addr_t per_paddr;
+    u32 tcd_nunits = 0;

> +       struct mpc_dma_tcd *tcd;
> +       unsigned long iflags;
> +       struct scatterlist *sg;
> +       size_t len;
> +       int iter, i;
> +
> +       if (!list_empty(&mchan->active))
> +               return NULL;
> +
> +       /* 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);

While spinlock is grabbed:
+        per_paddr = mchan->per_paddr;
+        tcd_nunits = mchan->tcd_nunits;

> +
> +               spin_unlock_irqrestore(&mchan->lock, iflags);
> +
> +               mdesc->error = 0;

Setting the flag here:
+        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))
> +                       return NULL;
> +
> +               if (direction == DMA_DEV_TO_MEM) {
> +                       tcd->saddr = mchan->per_paddr;

This line should be changed to:
+                       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 = mchan->per_paddr;

Ditto:
+                       tcd->daddr = per_paddr;

> +                       tcd->soff = 4;
> +                       tcd->doff = 0;
> +               } else {
> +                       return NULL;
> +               }
> +               tcd->ssize = MPC_DMA_TSIZE_4;
> +               tcd->dsize = MPC_DMA_TSIZE_4;
> +
> +               len = sg_dma_len(sg);
> +
> +               if (mchan->tcd_nunits)
> +                       tcd->nbytes = mchan->tcd_nunits * 4;
> +               else
> +                       tcd->nbytes = 64;

These 4 lines should be changed to:

+               if (tcd_nunits)
+                       tcd->nbytes = tcd_nunits * 4;
+               else
+                       return NULL;

The last line means that client kernel modules must set
src_maxburst and dst_maxburst fields of dma_slave_config (dmaengine.h).

> +
> +               if (!IS_ALIGNED(len, tcd->nbytes))
> +                       return NULL;
> +
> +               iter = len / tcd->nbytes;
> +               if (iter > ((1 << 15) - 1)) {   /* maximum biter */
> +                       return NULL; /* len is too big */
> +               } 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 the last descriptor */

Offer to remove this comment.

> +       return &mdesc->desc;
> +}
> +
> +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;

Locking:
+    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:
> +               cfg = (void *)arg;
> +               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;
> +               }

+        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;
> @@ -739,9 +890,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.7.10.4
>

Best regards,
Alexander.

Re: [PATCH RFC v2 1/5] dma: mpc512x: re-order mpc8308 specific instructions

[Alexander Popov]

Hello everyone!

2013/7/14 Gerhard Sittig <gsi@denx.de>:
> @@ -50,9 +50,23 @@
>  #define MPC_DMA_DESCRIPTORS    64
>
>  /* Macro definitions */
> -#define MPC_DMA_CHANNELS       64
>  #define MPC_DMA_TCD_OFFSET     0x1000
>
> +/*
> + * the maximum channel count, and specific channels which need
> + * special processing, for individual hardware variants
> + *
> + * and the maximum channel count over all supported controllers,
> + * used for data structure sizes
> + */
> +enum mpc8308_dmachan_id_t {
> +       MPC8308_DMACHAN_MAX = 16,
> +};
> +enum mpc512x_dmachan_id_t {
> +       MPC512x_DMACHAN_MAX = 64,
> +};
> +#define MPC_DMA_CHANNELS       64
> +

I offer to use #define instead of enum here
since individual channels don't require special handling.

Best regards,
Alexander.

Re: [PATCH RFC v2 1/5] dma: mpc512x: re-order mpc8308 specific instructions

[Alexander Popov]

Hello everyone!

2013/7/14 Gerhard Sittig <gsi@denx.de>:
> @@ -50,9 +50,23 @@
>  #define MPC_DMA_DESCRIPTORS    64
>
>  /* Macro definitions */
> -#define MPC_DMA_CHANNELS       64
>  #define MPC_DMA_TCD_OFFSET     0x1000
>
> +/*
> + * the maximum channel count, and specific channels which need
> + * special processing, for individual hardware variants
> + *
> + * and the maximum channel count over all supported controllers,
> + * used for data structure sizes
> + */
> +enum mpc8308_dmachan_id_t {
> +       MPC8308_DMACHAN_MAX = 16,
> +};
> +enum mpc512x_dmachan_id_t {
> +       MPC512x_DMACHAN_MAX = 64,
> +};
> +#define MPC_DMA_CHANNELS       64
> +

I offer to use #define instead of enum here
since individual channels don't require special handling.

Best regards,
Alexander.

Re: [PATCH RFC v2 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 since v1:
> - 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 since v2 (RFC v3 was badly formed, excuse me for that):
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).
Part 6/8:
  unchanged.
Part 7/8:
  unchanged.
Part 8/8:
  unchanged.

These changes are tested on MPC5125
- with SCLPC driver (transfers between dev and mem work fine).
- with dmatest module (all 64 DMA channels can perform mem-to-mem transfers
  which can be chained in one DMA transaction).

> 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
>

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

[Alexander Popov]

From: Lars-Peter Clausen <lars@metafoo.de>

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. The filter
function will use this pointer to match only channels which belong to the
specified DMA controller.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 drivers/dma/of-dma.c   |   47 ++++++++++++++++++++++++++++++
+++++++++++++++++
 include/linux/of_dma.h |    4 ++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 7aa0864..d5d528e 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -229,3 +229,50 @@ struct dma_chan *of_dma_simple_xlate(struct
of_phandle_args *dma_spec,
                        &dma_spec->args[0]);
 }
 EXPORT_SYMBOL_GPL(of_dma_simple_xlate);
+
+struct of_dma_filter_by_chan_id_args {
+       struct dma_device *dev;
+       unsigned int chan_id;
+};
+
+static bool of_dma_filter_by_chan_id(struct dma_chan *chan, void *params)
+{
+       struct of_dma_filter_by_chan_id_args *args = params;
+
+       return chan->device == args->dev && chan->chan_id == args->chan_id;
+}
+
+/**
+ * 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 of_dma_filter_by_chan_id_args args;
+       dma_cap_mask_t cap;
+
+       args.dev = ofdma->of_dma_data;
+       if (!args.dev)
+               return NULL;
+
+       if (dma_spec->args_count != 1)
+               return NULL;
+
+       dma_cap_zero(cap);
+       dma_cap_set(DMA_SLAVE, cap);
+
+       args.chan_id = dma_spec->args[0];
+
+       return dma_request_channel(cap, of_dma_filter_by_chan_id, &args);
+}
+EXPORT_SYMBOL_GPL(of_dma_xlate_by_chan_id);
diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h
index 364dda7..b7cf614 100644
--- a/include/linux/of_dma.h
+++ b/include/linux/of_dma.h
@@ -42,6 +42,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)
@@ -67,6 +69,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.7.10.4

Re: [PATCH RFC v2 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>
---
 .../devicetree/bindings/dma/
mpc512x-dma.txt        |   55 ++++++++++++++++++++
 arch/powerpc/boot/dts/mpc5121.dtsi                 |    1 +
 drivers/dma/mpc512x_dma.c                          |   21 ++++++--
 3 files changed, 74 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/mpc512x-dma.txt

diff --git a/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
new file mode 100644
index 0000000..a4867d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
@@ -0,0 +1,55 @@
+* Freescale MPC512x DMA Controller
+
+The DMA controller in the Freescale MPC512x SoC can move blocks of
+memory contents between memory and peripherals or memory to memory.
+
+Refer to the "Generic DMA Controller and DMA request bindings" description
+in the dma.txt file for a more detailled discussion of the binding.  The
+MPC512x DMA engine binding follows the common scheme, but doesn't provide
+support for the optional channels and requests counters (those values are
+derived from the detected hardware features) and has a fixed client
+specifier length of 1 integer cell (the value is the DMA channel, since
+the DMA controller uses a fixed assignment of request lines per channel).
+
+
+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 384e692..dae99b7 100644
--- a/arch/powerpc/boot/dts/mpc5121.dtsi
+++ b/arch/powerpc/boot/dts/mpc5121.dtsi
@@ -394,6 +394,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 8f6d545..3d79e3a 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -38,6 +38,7 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/of_device.h>
+#include <linux/of_dma.h>
 #include <linux/of_platform.h>

 #include <linux/random.h>
@@ -939,11 +940,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;
 }

@@ -952,6 +965,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.7.10.4

Re: [PATCH RFC v2 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>
---
 drivers/mmc/host/mxcmmc.c |   41 +++++++++++++++++++++++++++---
-----------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index d503635..b9d21cc 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -1121,20 +1121,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);

@@ -1204,7 +1213,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));

@@ -1255,7 +1266,8 @@ static int mxcmci_suspend(struct device *dev)
        if (mmc)
                ret = mmc_suspend_host(mmc);
        clk_disable_unprepare(host->clk_per);
-       clk_disable_unprepare(host->clk_ipg);
+       if (host->clk_ipg)
+               clk_disable_unprepare(host->clk_ipg);

        return ret;
 }
@@ -1267,7 +1279,8 @@ static int mxcmci_resume(struct device *dev)
        int ret = 0;

        clk_prepare_enable(host->clk_per);
-       clk_prepare_enable(host->clk_ipg);
+       if (host->clk_ipg)
+               clk_prepare_enable(host->clk_ipg);
        if (mmc)
                ret = mmc_resume_host(mmc);


--
1.7.10.4

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

[Gerhard Sittig]

On Thu, Oct 03, 2013 at 18:00 +0400, Alexander Popov wrote:
> 
> 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 since v1:
> > - 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 since v2 (RFC v3 was badly formed, excuse me for that):
> 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).
> Part 6/8:
>   unchanged.
> Part 7/8:
>   unchanged.
> Part 8/8:
>   unchanged.
> 
> These changes are tested on MPC5125
> - with SCLPC driver (transfers between dev and mem work fine).
> - with dmatest module (all 64 DMA channels can perform mem-to-mem transfers
>   which can be chained in one DMA transaction).
> 
> > 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


I'm not certain whether keeping the "cover letter" in threaded
form is appropriate.  But speaking about non-existent parts
(6-8/8), not saying what the current version is, missing stats
certainly isn't right.

Your submission style adds more work to doing review and
providing feedback than what's necessary.  You assume that others
would keep the history for you, or would do the archeology and
collect individual pieces from the past, to recover what you fail
to send out.

The messages show up in an unexpected order here (3, 4, 5, 0
first on one thread, 1, 2 then in another thread, with some 2800
messages between them) and carry conflicting subjects or version
numbers.  Formatting was broken in transport, patches won't apply.

Please do consider checking what you send out, and how you can
improve the reception's side of the process.  Try to help those
people you want to receive help from.  Try to support both kinds
of review for people coming new to the subject as well as those
who have seen a former version.


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