Skip to contentSkip to conversation

mtd: sh_flctl: Setup and release DMA channels

PATCH In review Bastian Hecht 2 participants 7 messages Sep 24, 2012 – Sep 29, 2012

Subsystems (3)
Memory technology devices (MTD) SuperH NAND flash
lore
Share & tools
lore

Bastian Hecht

author
#0 lore raw

PATCH v2 1/2 mtd: sh_flctl: Setup and release DMA channels

The code probes if DMA channels can get allocated and tears them down at removal/failure if needed.

Based on Guennadi Liakhovetski's code from the sh_mmcif driver.

Signed-off-by Bastian Hecht <[email protected]>

---
v2: incorporated Vikram Narayanan's thoughts
- added a missing "static".
- reordered things to get rid of a forward declaration

Based on l2-mtd with reverted commit e26c113b4130aefa1d8446602bb5b05cfd646bfe.

 drivers/mtd/nand/sh_flctl.c  |   74 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/sh_flctl.h |   10 ++++++
 2 files changed, 84 insertions(+), 0 deletions(-)
drivers/mtd/nand/sh_flctl.c +74 -0
index 4fbfe96..9659483 100644
@@ -106,6 +106,76 @@ static void wait_completion(struct sh_flctl *flctl)
 	writeb(0x0, FLTRCR(flctl));
 }
 
+static void flctl_release_dma(struct sh_flctl *flctl)
+{
+	if (flctl->chan_fifo0_rx) {
+		dma_release_channel(flctl->chan_fifo0_rx);
+		flctl->chan_fifo0_rx = NULL;
+	}
+	if (flctl->chan_fifo0_tx) {
+		dma_release_channel(flctl->chan_fifo0_tx);
+		flctl->chan_fifo0_tx = NULL;
+	}
+}
+
+static void flctl_setup_dma(struct sh_flctl *flctl)
+{
+	dma_cap_mask_t mask;
+	struct dma_slave_config cfg;
+	struct platform_device *pdev = flctl->pdev;
+	struct sh_flctl_platform_data *pdata = pdev->dev.platform_data;
+	int ret;
+
+	if (!pdata)
+		return;
+
+	if (pdata->slave_id_fifo0_tx <= 0 || pdata->slave_id_fifo0_rx <= 0)
+		return;
+
+	/* We can only either use DMA for both Tx and Rx or not use it at all */
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+
+	flctl->chan_fifo0_tx = dma_request_channel(mask, shdma_chan_filter,
+					    (void *)pdata->slave_id_fifo0_tx);
+	dev_dbg(&pdev->dev, "%s: TX: got channel %p\n", __func__,
+		flctl->chan_fifo0_tx);
+
+	if (!flctl->chan_fifo0_tx)
+		return;
+
+	cfg.slave_id = pdata->slave_id_fifo0_tx;
+	cfg.direction = DMA_MEM_TO_DEV;
+	cfg.dst_addr = (dma_addr_t)FLDTFIFO(flctl);
+	cfg.src_addr = 0;
+	ret = dmaengine_slave_config(flctl->chan_fifo0_tx, &cfg);
+	if (ret < 0)
+		goto err;
+
+	flctl->chan_fifo0_rx = dma_request_channel(mask, shdma_chan_filter,
+					    (void *)pdata->slave_id_fifo0_rx);
+	dev_dbg(&pdev->dev, "%s: RX: got channel %p\n", __func__,
+		flctl->chan_fifo0_rx);
+
+	if (!flctl->chan_fifo0_rx)
+		goto err;
+
+	cfg.slave_id = pdata->slave_id_fifo0_rx;
+	cfg.direction = DMA_DEV_TO_MEM;
+	cfg.dst_addr = 0;
+	cfg.src_addr = (dma_addr_t)FLDTFIFO(flctl);
+	ret = dmaengine_slave_config(flctl->chan_fifo0_rx, &cfg);
+	if (ret < 0)
+		goto err;
+
+	init_completion(&flctl->dma_complete);
+
+	return;
+
+err:
+	flctl_release_dma(flctl);
+}
+
 static void set_addr(struct mtd_info *mtd, int column, int page_addr)
 {
 	struct sh_flctl *flctl = mtd_to_flctl(mtd);

@@ -930,6 +1000,8 @@ static int __devinit flctl_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_resume(&pdev->dev);
 
+	flctl_setup_dma(flctl);
+
 	ret = nand_scan_ident(flctl_mtd, 1, NULL);
 	if (ret)
 		goto err_chip;

@@ -947,6 +1019,7 @@ static int __devinit flctl_probe(struct platform_device *pdev)
 	return 0;
 
 err_chip:
+	flctl_release_dma(flctl);
 	pm_runtime_disable(&pdev->dev);
 	free_irq(irq, flctl);
 err_flste:

@@ -960,6 +1033,7 @@ static int __devexit flctl_remove(struct platform_device *pdev)
 {
 	struct sh_flctl *flctl = platform_get_drvdata(pdev);
 
+	flctl_release_dma(flctl);
 	nand_release(&flctl->mtd);
 	pm_runtime_disable(&pdev->dev);
 	free_irq(platform_get_irq(pdev, 0), flctl);
include/linux/mtd/sh_flctl.h +10 -0
index 01e4b15..20d3f48 100644
@@ -20,10 +20,12 @@
 #ifndef __SH_FLCTL_H__
 #define __SH_FLCTL_H__
 
+#include <linux/dmaengine.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/pm_qos.h>
+#include <linux/sh_dma.h>
 
 /* FLCTL registers */
 #define FLCMNCR(f)		(f->reg + 0x0)

@@ -161,6 +163,11 @@ struct sh_flctl {
 	unsigned hwecc:1;	/* Hardware ECC (0 = disabled, 1 = enabled) */
 	unsigned holden:1;	/* Hardware has FLHOLDCR and HOLDEN is set */
 	unsigned qos_request:1;	/* QoS request to prevent deep power shutdown */
+
+	/* DMA related objects */
+	struct dma_chan		*chan_fifo0_rx;
+	struct dma_chan		*chan_fifo0_tx;
+	struct completion	dma_complete;
 };
 
 struct sh_flctl_platform_data {

@@ -170,6 +177,9 @@ struct sh_flctl_platform_data {
 
 	unsigned has_hwecc:1;
 	unsigned use_holden:1;
+
+	unsigned int            slave_id_fifo0_tx;
+	unsigned int            slave_id_fifo0_rx;
 };
 
 static inline struct sh_flctl *mtd_to_flctl(struct mtd_info *mtdinfo)
Signature · 1 line
1.7.5.4
author
#1 lore raw

PATCH v2 2/2 mtd: sh_flctl: Use DMA for data fifo FLTDFIFO when available

Map and unmap DMA buffers, trigger the DMA and wait for the completion. On failure we fallback to PIO mode.

Signed-off-by Bastian Hecht <[email protected]>

---
log v2: dropped a forward declaration

 drivers/mtd/nand/sh_flctl.c  |   97 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/sh_flctl.h |    1 +
 2 files changed, 96 insertions(+), 2 deletions(-)
drivers/mtd/nand/sh_flctl.c +94 -2
index 9659483..0d90af8 100644
@@ -24,6 +24,8 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/platform_device.h>

@@ -106,6 +108,13 @@ static void wait_completion(struct sh_flctl *flctl)
 	writeb(0x0, FLTRCR(flctl));
 }
 
+static void flctl_dma_complete(void *param)
+{
+	struct sh_flctl *flctl = param;
+
+	complete(&flctl->dma_complete);
+}
+
 static void flctl_release_dma(struct sh_flctl *flctl)
 {
 	if (flctl->chan_fifo0_rx) {

@@ -331,6 +340,69 @@ static void wait_wecfifo_ready(struct sh_flctl *flctl)
 	timeout_error(flctl, __func__);
 }
 
+static void flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
+					int len, enum dma_data_direction dir)
+{
+	struct dma_async_tx_descriptor *desc = NULL;
+	struct dma_chan *chan;
+	enum dma_transfer_direction tr_dir;
+	dma_addr_t dma_addr;
+	dma_cookie_t cookie = -EINVAL;
+	uint32_t reg;
+	int ret;
+
+	if (dir = DMA_FROM_DEVICE) {
+		chan = flctl->chan_fifo0_rx;
+		tr_dir = DMA_DEV_TO_MEM;
+	} else {
+		chan = flctl->chan_fifo0_tx;
+		tr_dir = DMA_MEM_TO_DEV;
+	}
+
+	dma_addr = dma_map_single(chan->device->dev, buf, len, dir);
+
+	if (dma_addr)
+		desc = dmaengine_prep_slave_single(chan, dma_addr, len,
+			tr_dir, DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+
+	if (desc) {
+		reg = readl(FLINTDMACR(flctl));
+		reg |= DREQ0EN;
+		writel(reg, FLINTDMACR(flctl));
+
+		desc->callback = flctl_dma_complete;
+		desc->callback_param = flctl;
+		cookie = dmaengine_submit(desc);
+
+		dma_async_issue_pending(chan);
+	}
+
+	if (!desc) {
+		/* DMA failed, fall back to PIO */
+		flctl_release_dma(flctl);
+		dev_warn(&flctl->pdev->dev,
+			 "DMA failed, falling back to PIO\n");
+		goto out;
+	}
+
+	ret +	wait_for_completion_timeout(&flctl->dma_complete,
+				msecs_to_jiffies(3000));
+
+	if (ret <= 0) {
+		chan->device->device_control(chan, DMA_TERMINATE_ALL, 0);
+		dev_err(&flctl->pdev->dev, "wait_for_completion_timeout\n");
+	}
+
+out:
+	reg = readl(FLINTDMACR(flctl));
+	reg &= ~DREQ0EN;
+	writel(reg, FLINTDMACR(flctl));
+
+	dma_unmap_single(chan->device->dev, dma_addr, len, dir);
+	init_completion(&flctl->dma_complete);
+}
+
 static void read_datareg(struct sh_flctl *flctl, int offset)
 {
 	unsigned long data;

@@ -349,6 +421,16 @@ static void read_fiforeg(struct sh_flctl *flctl, int rlen, int offset)
 
 	len_4align = (rlen + 3) / 4;
 
+	/* initiate DMA transfer */
+	if (flctl->chan_fifo0_rx && rlen >= 32) {
+		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_DEV_TO_MEM);
+		for (i = 0; i < len_4align; i++)
+			buf[i] = be32_to_cpu(buf[i]);
+
+		return;
+	}
+
+	/* do polling transfer */
 	for (i = 0; i < len_4align; i++) {
 		wait_rfifo_ready(flctl);
 		buf[i] = readl(FLDTFIFO(flctl));

@@ -378,13 +460,24 @@ static enum flctl_ecc_res_t read_ecfiforeg
 static void write_fiforeg(struct sh_flctl *flctl, int rlen, int offset)
 {
 	int i, len_4align;
-	unsigned long *data = (unsigned long *)&flctl->done_buff[offset];
+	unsigned long *buf = (unsigned long *)&flctl->done_buff[offset];
 	void *fifo_addr = (void *)FLDTFIFO(flctl);
 
 	len_4align = (rlen + 3) / 4;
+
+	/* initiate DMA transfer */
+	if (flctl->chan_fifo0_tx && rlen >= 32) {
+		for (i = 0; i < len_4align; i++)
+			buf[i] = cpu_to_be32(buf[i]);
+
+		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV);
+		return;
+	}
+
+	/* do polling transfer */
 	for (i = 0; i < len_4align; i++) {
 		wait_wfifo_ready(flctl);
-		writel(cpu_to_be32(data[i]), fifo_addr);
+		writel(cpu_to_be32(buf[i]), fifo_addr);
 	}
 }
include/linux/mtd/sh_flctl.h +1 -0
index 20d3f48..d55ec25 100644
@@ -109,6 +109,7 @@
 #define ESTERINTE	(0x1 << 24)	/* ECC error interrupt enable */
 #define AC1CLR		(0x1 << 19)	/* ECC FIFO clear */
 #define AC0CLR		(0x1 << 18)	/* Data FIFO clear */
+#define DREQ0EN		(0x1 << 16)	/* FLDTFIFODMA Request Enable */
 #define ECERB		(0x1 << 9)	/* ECC error */
 #define STERB		(0x1 << 8)	/* Status error */
 #define STERINTE	(0x1 << 4)	/* Status error enable */
Signature · 1 line
1.7.5.4
#2 lore raw

Re: [PATCH v2 2/2] mtd: sh_flctl: Use DMA for data fifo FLTDFIFO when available

Quoted 16 lines · Bastian Hecht

Map and unmap DMA buffers, trigger the DMA and wait for the completion. On failure we fallback to PIO mode.

Signed-off-by Bastian Hecht <[email protected]>

---
log v2: dropped a forward declaration

 drivers/mtd/nand/sh_flctl.c  |   97 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/sh_flctl.h |    1 +
 2 files changed, 96 insertions(+), 2 deletions(-)
drivers/mtd/nand/sh_flctl.c +0 -0
index 9659483..0d90af8 100644

drivers/mtd/nand/sh_flctl.c line 28

Quoted 5 lines
drivers/mtd/nand/sh_flctl.c +2 -0
@@ -24,6 +24,8 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>

As I suggested in my comments to patch 1/2, I would merge these patches. In any case the headers are needed already in the first patch, including sh_dma.h.

Thanks
Guennadi

Quoted 145 lines
drivers/mtd/nand/sh_flctl.c +92 -2
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/platform_device.h>

@@ -106,6 +108,13 @@ static void wait_completion(struct sh_flctl *flctl)
 	writeb(0x0, FLTRCR(flctl));
 }
 
+static void flctl_dma_complete(void *param)
+{
+	struct sh_flctl *flctl = param;
+
+	complete(&flctl->dma_complete);
+}
+
 static void flctl_release_dma(struct sh_flctl *flctl)
 {
 	if (flctl->chan_fifo0_rx) {

@@ -331,6 +340,69 @@ static void wait_wecfifo_ready(struct sh_flctl *flctl)
 	timeout_error(flctl, __func__);
 }
 
+static void flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
+					int len, enum dma_data_direction dir)
+{
+	struct dma_async_tx_descriptor *desc = NULL;
+	struct dma_chan *chan;
+	enum dma_transfer_direction tr_dir;
+	dma_addr_t dma_addr;
+	dma_cookie_t cookie = -EINVAL;
+	uint32_t reg;
+	int ret;
+
+	if (dir = DMA_FROM_DEVICE) {
+		chan = flctl->chan_fifo0_rx;
+		tr_dir = DMA_DEV_TO_MEM;
+	} else {
+		chan = flctl->chan_fifo0_tx;
+		tr_dir = DMA_MEM_TO_DEV;
+	}
+
+	dma_addr = dma_map_single(chan->device->dev, buf, len, dir);
+
+	if (dma_addr)
+		desc = dmaengine_prep_slave_single(chan, dma_addr, len,
+			tr_dir, DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+
+	if (desc) {
+		reg = readl(FLINTDMACR(flctl));
+		reg |= DREQ0EN;
+		writel(reg, FLINTDMACR(flctl));
+
+		desc->callback = flctl_dma_complete;
+		desc->callback_param = flctl;
+		cookie = dmaengine_submit(desc);
+
+		dma_async_issue_pending(chan);
+	}
+
+	if (!desc) {
+		/* DMA failed, fall back to PIO */
+		flctl_release_dma(flctl);
+		dev_warn(&flctl->pdev->dev,
+			 "DMA failed, falling back to PIO\n");
+		goto out;
+	}
+
+	ret > +	wait_for_completion_timeout(&flctl->dma_complete,
+				msecs_to_jiffies(3000));
+
+	if (ret <= 0) {
+		chan->device->device_control(chan, DMA_TERMINATE_ALL, 0);
+		dev_err(&flctl->pdev->dev, "wait_for_completion_timeout\n");
+	}
+
+out:
+	reg = readl(FLINTDMACR(flctl));
+	reg &= ~DREQ0EN;
+	writel(reg, FLINTDMACR(flctl));
+
+	dma_unmap_single(chan->device->dev, dma_addr, len, dir);
+	init_completion(&flctl->dma_complete);
+}
+
 static void read_datareg(struct sh_flctl *flctl, int offset)
 {
 	unsigned long data;

@@ -349,6 +421,16 @@ static void read_fiforeg(struct sh_flctl *flctl, int rlen, int offset)
 
 	len_4align = (rlen + 3) / 4;
 
+	/* initiate DMA transfer */
+	if (flctl->chan_fifo0_rx && rlen >= 32) {
+		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_DEV_TO_MEM);
+		for (i = 0; i < len_4align; i++)
+			buf[i] = be32_to_cpu(buf[i]);
+
+		return;
+	}
+
+	/* do polling transfer */
 	for (i = 0; i < len_4align; i++) {
 		wait_rfifo_ready(flctl);
 		buf[i] = readl(FLDTFIFO(flctl));

@@ -378,13 +460,24 @@ static enum flctl_ecc_res_t read_ecfiforeg
 static void write_fiforeg(struct sh_flctl *flctl, int rlen, int offset)
 {
 	int i, len_4align;
-	unsigned long *data = (unsigned long *)&flctl->done_buff[offset];
+	unsigned long *buf = (unsigned long *)&flctl->done_buff[offset];
 	void *fifo_addr = (void *)FLDTFIFO(flctl);
 
 	len_4align = (rlen + 3) / 4;
+
+	/* initiate DMA transfer */
+	if (flctl->chan_fifo0_tx && rlen >= 32) {
+		for (i = 0; i < len_4align; i++)
+			buf[i] = cpu_to_be32(buf[i]);
+
+		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV);
+		return;
+	}
+
+	/* do polling transfer */
 	for (i = 0; i < len_4align; i++) {
 		wait_wfifo_ready(flctl);
-		writel(cpu_to_be32(data[i]), fifo_addr);
+		writel(cpu_to_be32(buf[i]), fifo_addr);
 	}
 }
include/linux/mtd/sh_flctl.h +1 -0
index 20d3f48..d55ec25 100644
@@ -109,6 +109,7 @@
 #define ESTERINTE	(0x1 << 24)	/* ECC error interrupt enable */
 #define AC1CLR		(0x1 << 19)	/* ECC FIFO clear */
 #define AC0CLR		(0x1 << 18)	/* Data FIFO clear */
+#define DREQ0EN		(0x1 << 16)	/* FLDTFIFODMA Request Enable */
 #define ECERB		(0x1 << 9)	/* ECC error */
 #define STERB		(0x1 << 8)	/* Status error */
 #define STERINTE	(0x1 << 4)	/* Status error enable */
Signature · 1 line
1.7.5.4

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

#3 lore raw
In reply to Bastian Hecht

Hi Bastian

Quoted 2 lines · Bastian Hecht

The code probes if DMA channels can get allocated and tears them down at removal/failure if needed.

I'm not sure it makes sense to break this code into two patches - the first one makes no sense without the second one.

Quoted 20 lines

Based on Guennadi Liakhovetski's code from the sh_mmcif driver.

Signed-off-by Bastian Hecht <[email protected]>

---
v2: incorporated Vikram Narayanan's thoughts
- added a missing "static".
- reordered things to get rid of a forward declaration

Based on l2-mtd with reverted commit e26c113b4130aefa1d8446602bb5b05cfd646bfe.

 drivers/mtd/nand/sh_flctl.c  |   74 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/sh_flctl.h |   10 ++++++
 2 files changed, 84 insertions(+), 0 deletions(-)
drivers/mtd/nand/sh_flctl.c +0 -0
index 4fbfe96..9659483 100644

drivers/mtd/nand/sh_flctl.c static void wait_completion(struct sh_flctl *flctl) · line 112

Quoted 7 lines
drivers/mtd/nand/sh_flctl.c +4 -0
@@ -106,6 +106,76 @@ static void wait_completion(struct sh_flctl *flctl)
 	writeb(0x0, FLTRCR(flctl));
 }
 
+static void flctl_release_dma(struct sh_flctl *flctl)
+{
+	if (flctl->chan_fifo0_rx) {
+		dma_release_channel(flctl->chan_fifo0_rx);

You add dmaengine API calls to the file, so, you have to include header(s). dma_release_channel() requires dmaengine.h.

Quoted 17 lines
drivers/mtd/nand/sh_flctl.c +17 -0
+		flctl->chan_fifo0_rx = NULL;
+	}
+	if (flctl->chan_fifo0_tx) {
+		dma_release_channel(flctl->chan_fifo0_tx);
+		flctl->chan_fifo0_tx = NULL;
+	}
+}
+
+static void flctl_setup_dma(struct sh_flctl *flctl)
+{
+	dma_cap_mask_t mask;
+	struct dma_slave_config cfg;
+	struct platform_device *pdev = flctl->pdev;
+	struct sh_flctl_platform_data *pdata = pdev->dev.platform_data;
+	int ret;
+
+	if (!pdata)

drivers/mtd/nand/sh_flctl.c static void wait_completion(struct sh_flctl *flctl) · line 139

Quoted 10 lines
drivers/mtd/nand/sh_flctl.c +10 -0
+		return;
+
+	if (pdata->slave_id_fifo0_tx <= 0 || pdata->slave_id_fifo0_rx <= 0)
+		return;
+
+	/* We can only either use DMA for both Tx and Rx or not use it at all */
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+
+	flctl->chan_fifo0_tx = dma_request_channel(mask, shdma_chan_filter,

shdma_chan_filter() requires sh_dma.h. And no, including these headers in your header is not quite correct.

drivers/mtd/nand/sh_flctl.c static void wait_completion(struct sh_flctl *flctl) · line 151

Quoted 12 lines
drivers/mtd/nand/sh_flctl.c +12 -0
+					    (void *)pdata->slave_id_fifo0_tx);
+	dev_dbg(&pdev->dev, "%s: TX: got channel %p\n", __func__,
+		flctl->chan_fifo0_tx);
+
+	if (!flctl->chan_fifo0_tx)
+		return;
+
+	cfg.slave_id = pdata->slave_id_fifo0_tx;
+	cfg.direction = DMA_MEM_TO_DEV;
+	cfg.dst_addr = (dma_addr_t)FLDTFIFO(flctl);
+	cfg.src_addr = 0;
+	ret = dmaengine_slave_config(flctl->chan_fifo0_tx, &cfg);

Ok, you copied my mistake here:-) "cfg" is allocated on stack and never initialised. You set those its members, that are needed by the sh-dma driver, but it can change at any time to begin using further fields from struct dma_slave_config, so, it has to be zeroed first. E.g., you could do

+ struct dma_slave_config cfg = {.device_fc = false,};

Quoted 60 lines
drivers/mtd/nand/sh_flctl.c +31 -0
+	if (ret < 0)
+		goto err;
+
+	flctl->chan_fifo0_rx = dma_request_channel(mask, shdma_chan_filter,
+					    (void *)pdata->slave_id_fifo0_rx);
+	dev_dbg(&pdev->dev, "%s: RX: got channel %p\n", __func__,
+		flctl->chan_fifo0_rx);
+
+	if (!flctl->chan_fifo0_rx)
+		goto err;
+
+	cfg.slave_id = pdata->slave_id_fifo0_rx;
+	cfg.direction = DMA_DEV_TO_MEM;
+	cfg.dst_addr = 0;
+	cfg.src_addr = (dma_addr_t)FLDTFIFO(flctl);
+	ret = dmaengine_slave_config(flctl->chan_fifo0_rx, &cfg);
+	if (ret < 0)
+		goto err;
+
+	init_completion(&flctl->dma_complete);
+
+	return;
+
+err:
+	flctl_release_dma(flctl);
+}
+
 static void set_addr(struct mtd_info *mtd, int column, int page_addr)
 {
 	struct sh_flctl *flctl = mtd_to_flctl(mtd);

@@ -930,6 +1000,8 @@ static int __devinit flctl_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_resume(&pdev->dev);
 
+	flctl_setup_dma(flctl);
+
 	ret = nand_scan_ident(flctl_mtd, 1, NULL);
 	if (ret)
 		goto err_chip;

@@ -947,6 +1019,7 @@ static int __devinit flctl_probe(struct platform_device *pdev)
 	return 0;
 
 err_chip:
+	flctl_release_dma(flctl);
 	pm_runtime_disable(&pdev->dev);
 	free_irq(irq, flctl);
 err_flste:

@@ -960,6 +1033,7 @@ static int __devexit flctl_remove(struct platform_device *pdev)
 {
 	struct sh_flctl *flctl = platform_get_drvdata(pdev);
 
+	flctl_release_dma(flctl);
 	nand_release(&flctl->mtd);
 	pm_runtime_disable(&pdev->dev);
 	free_irq(platform_get_irq(pdev, 0), flctl);
include/linux/mtd/sh_flctl.h +0 -0
index 01e4b15..20d3f48 100644

include/linux/mtd/sh_flctl.h line 28

Quoted 9 lines
include/linux/mtd/sh_flctl.h +2 -0
@@ -20,10 +20,12 @@
 #ifndef __SH_FLCTL_H__
 #define __SH_FLCTL_H__
 
+#include <linux/dmaengine.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/pm_qos.h>
+#include <linux/sh_dma.h>

Here headers aren't actually needed - you can just forward-declare "struct dma_chan." OTOH, what _is_ indeed needed is <linux/completion.h>.

Quoted 28 lines
include/linux/mtd/sh_flctl.h +8 -0
 /* FLCTL registers */
 #define FLCMNCR(f)		(f->reg + 0x0)

@@ -161,6 +163,11 @@ struct sh_flctl {
 	unsigned hwecc:1;	/* Hardware ECC (0 = disabled, 1 = enabled) */
 	unsigned holden:1;	/* Hardware has FLHOLDCR and HOLDEN is set */
 	unsigned qos_request:1;	/* QoS request to prevent deep power shutdown */
+
+	/* DMA related objects */
+	struct dma_chan		*chan_fifo0_rx;
+	struct dma_chan		*chan_fifo0_tx;
+	struct completion	dma_complete;
 };
 
 struct sh_flctl_platform_data {

@@ -170,6 +177,9 @@ struct sh_flctl_platform_data {
 
 	unsigned has_hwecc:1;
 	unsigned use_holden:1;
+
+	unsigned int            slave_id_fifo0_tx;
+	unsigned int            slave_id_fifo0_rx;
 };
 
 static inline struct sh_flctl *mtd_to_flctl(struct mtd_info *mtdinfo)
Signature · 1 line
1.7.5.4

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

author
#4 lore raw

Thanks for the reviews, Guennadi! Thanks too to Vikram - I've forgot to add you as recipient for the v2, so I pull you in here now.

2012/9/27 Guennadi Liakhovetski <[email protected]>:

Quoted 9 lines

Hi Bastian

Quoted 2 lines · Bastian Hecht

The code probes if DMA channels can get allocated and tears them down at removal/failure if needed.

I'm not sure it makes sense to break this code into two patches - the first one makes no sense without the second one.

Ok, will merge them.

Quoted 23 lines
Quoted 23 lines

Based on Guennadi Liakhovetski's code from the sh_mmcif driver.

Signed-off-by Bastian Hecht <[email protected]>

---
v2: incorporated Vikram Narayanan's thoughts
- added a missing "static".
- reordered things to get rid of a forward declaration

Based on l2-mtd with reverted commit e26c113b4130aefa1d8446602bb5b05cfd646bfe.

 drivers/mtd/nand/sh_flctl.c  |   74 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/sh_flctl.h |   10 ++++++
 2 files changed, 84 insertions(+), 0 deletions(-)
drivers/mtd/nand/sh_flctl.c +0 -0
index 4fbfe96..9659483 100644
@@ -106,6 +106,76 @@ static void wait_completion(struct sh_flctl *flctl)
      writeb(0x0, FLTRCR(flctl));
 }
Quoted 7 lines

drivers/mtd/nand/sh_flctl.c static void wait_completion(struct sh_flctl *flctl) · line 112

Quoted 4 lines
drivers/mtd/nand/sh_flctl.c +4 -0
+static void flctl_release_dma(struct sh_flctl *flctl)
+{
+     if (flctl->chan_fifo0_rx) {
+             dma_release_channel(flctl->chan_fifo0_rx);

You add dmaengine API calls to the file, so, you have to include header(s). dma_release_channel() requires dmaengine.h.

Ok.

Quoted 22 lines
Quoted 22 lines
drivers/mtd/nand/sh_flctl.c +22 -0
+             flctl->chan_fifo0_rx = NULL;
+     }
+     if (flctl->chan_fifo0_tx) {
+             dma_release_channel(flctl->chan_fifo0_tx);
+             flctl->chan_fifo0_tx = NULL;
+     }
+}
+
+static void flctl_setup_dma(struct sh_flctl *flctl)
+{
+     dma_cap_mask_t mask;
+     struct dma_slave_config cfg;
+     struct platform_device *pdev = flctl->pdev;
+     struct sh_flctl_platform_data *pdata = pdev->dev.platform_data;
+     int ret;
+
+     if (!pdata)
+             return;
+
+     if (pdata->slave_id_fifo0_tx <= 0 || pdata->slave_id_fifo0_rx <= 0)
+             return;
+
Quoted 8 lines

drivers/mtd/nand/sh_flctl.c static void wait_completion(struct sh_flctl *flctl) · line 139

Quoted 5 lines
drivers/mtd/nand/sh_flctl.c +5 -0
+     /* We can only either use DMA for both Tx and Rx or not use it at all */
+     dma_cap_zero(mask);
+     dma_cap_set(DMA_SLAVE, mask);
+
+     flctl->chan_fifo0_tx = dma_request_channel(mask, shdma_chan_filter,

shdma_chan_filter() requires sh_dma.h. And no, including these headers in your header is not quite correct.

Ok.

Quoted 18 lines

drivers/mtd/nand/sh_flctl.c static void wait_completion(struct sh_flctl *flctl) · line 151

Quoted 12 lines
drivers/mtd/nand/sh_flctl.c +12 -0
+                                         (void *)pdata->slave_id_fifo0_tx);
+     dev_dbg(&pdev->dev, "%s: TX: got channel %p\n", __func__,
+             flctl->chan_fifo0_tx);
+
+     if (!flctl->chan_fifo0_tx)
+             return;
+
+     cfg.slave_id = pdata->slave_id_fifo0_tx;
+     cfg.direction = DMA_MEM_TO_DEV;
+     cfg.dst_addr = (dma_addr_t)FLDTFIFO(flctl);
+     cfg.src_addr = 0;
+     ret = dmaengine_slave_config(flctl->chan_fifo0_tx, &cfg);

Ok, you copied my mistake here:-) "cfg" is allocated on stack and never initialised. You set those its members, that are needed by the sh-dma driver, but it can change at any time to begin using further fields from struct dma_slave_config, so, it has to be zeroed first. E.g., you could do

Quoted 1 line

+ struct dma_slave_config cfg = {.device_fc = false,};

I think I'll use memset. Taking a member like .device_fc may be a bit misleading. We are not interested in it and leave it on the default value. And we have no other field we could initialize that won't be changed in the function.

Quoted 63 lines
Quoted 63 lines
drivers/mtd/nand/sh_flctl.c +31 -0
+     if (ret < 0)
+             goto err;
+
+     flctl->chan_fifo0_rx = dma_request_channel(mask, shdma_chan_filter,
+                                         (void *)pdata->slave_id_fifo0_rx);
+     dev_dbg(&pdev->dev, "%s: RX: got channel %p\n", __func__,
+             flctl->chan_fifo0_rx);
+
+     if (!flctl->chan_fifo0_rx)
+             goto err;
+
+     cfg.slave_id = pdata->slave_id_fifo0_rx;
+     cfg.direction = DMA_DEV_TO_MEM;
+     cfg.dst_addr = 0;
+     cfg.src_addr = (dma_addr_t)FLDTFIFO(flctl);
+     ret = dmaengine_slave_config(flctl->chan_fifo0_rx, &cfg);
+     if (ret < 0)
+             goto err;
+
+     init_completion(&flctl->dma_complete);
+
+     return;
+
+err:
+     flctl_release_dma(flctl);
+}
+
 static void set_addr(struct mtd_info *mtd, int column, int page_addr)
 {
      struct sh_flctl *flctl = mtd_to_flctl(mtd);

@@ -930,6 +1000,8 @@ static int __devinit flctl_probe(struct platform_device *pdev)
      pm_runtime_enable(&pdev->dev);
      pm_runtime_resume(&pdev->dev);
 
+     flctl_setup_dma(flctl);
+
      ret = nand_scan_ident(flctl_mtd, 1, NULL);
      if (ret)
              goto err_chip;

@@ -947,6 +1019,7 @@ static int __devinit flctl_probe(struct platform_device *pdev)
      return 0;
 
 err_chip:
+     flctl_release_dma(flctl);
      pm_runtime_disable(&pdev->dev);
      free_irq(irq, flctl);
 err_flste:

@@ -960,6 +1033,7 @@ static int __devexit flctl_remove(struct platform_device *pdev)
 {
      struct sh_flctl *flctl = platform_get_drvdata(pdev);
 
+     flctl_release_dma(flctl);
      nand_release(&flctl->mtd);
      pm_runtime_disable(&pdev->dev);
      free_irq(platform_get_irq(pdev, 0), flctl);
include/linux/mtd/sh_flctl.h +0 -0
index 01e4b15..20d3f48 100644
@@ -20,10 +20,12 @@
 #ifndef __SH_FLCTL_H__
 #define __SH_FLCTL_H__
Quoted 9 lines

include/linux/mtd/sh_flctl.h line 28

Quoted 6 lines
include/linux/mtd/sh_flctl.h +2 -0
+#include <linux/dmaengine.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/pm_qos.h>
+#include <linux/sh_dma.h>

Here headers aren't actually needed - you can just forward-declare "struct dma_chan." OTOH, what _is_ indeed needed is <linux/completion.h>.

Oh yes, I'll add <linux/completion.h>.
About the forward declration: Do I really need it? We use a struct pointer and I am unsure if the compiler just needs to know the name and nothing more. I've tested it and included sh_flctl.h (without any DMA headers) in some driver that has no idea about DMA. gcc didn't complain. Adding a non-pointer member
+ struct sh_dma test;
lead to an error, so struct sh_dma is really unknown.

Quoted 39 lines
Quoted 26 lines
include/linux/mtd/sh_flctl.h +8 -1
 /* FLCTL registers */
 #define FLCMNCR(f)           (f->reg + 0x0)

@@ -161,6 +163,11 @@ struct sh_flctl {
      unsigned hwecc:1;       /* Hardware ECC (0 = disabled, 1 = enabled) */
      unsigned holden:1;      /* Hardware has FLHOLDCR and HOLDEN is set */
      unsigned qos_request:1; /* QoS request to prevent deep power shutdown */
+
+     /* DMA related objects */
+     struct dma_chan         *chan_fifo0_rx;
+     struct dma_chan         *chan_fifo0_tx;
+     struct completion       dma_complete;
 };
 
 struct sh_flctl_platform_data {

@@ -170,6 +177,9 @@ struct sh_flctl_platform_data {
 
      unsigned has_hwecc:1;
      unsigned use_holden:1;
+
+     unsigned int            slave_id_fifo0_tx;
+     unsigned int            slave_id_fifo0_rx;
 };
 
 static inline struct sh_flctl *mtd_to_flctl(struct mtd_info *mtdinfo)
--
Quoted 2 lines

1.7.5.4

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw

On Fri, 28 Sep 2012, Bastian Hecht wrote:

[snip]

Quoted 18 lines
Quoted 8 lines
Quoted 8 lines
include/linux/mtd/sh_flctl.h +0 -0
index 01e4b15..20d3f48 100644
@@ -20,10 +20,12 @@
 #ifndef __SH_FLCTL_H__
 #define __SH_FLCTL_H__
Quoted 9 lines

include/linux/mtd/sh_flctl.h line 28

Quoted 6 lines
include/linux/mtd/sh_flctl.h +2 -0
+#include <linux/dmaengine.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/pm_qos.h>
+#include <linux/sh_dma.h>

Here headers aren't actually needed - you can just forward-declare "struct dma_chan." OTOH, what _is_ indeed needed is <linux/completion.h>.

Quoted 6 lines

Oh yes, I'll add <linux/completion.h>.
About the forward declration: Do I really need it? We use a struct pointer and I am unsure if the compiler just needs to know the name and nothing more. I've tested it and included sh_flctl.h (without any DMA headers) in some driver that has no idea about DMA. gcc didn't complain.

I'm not sure about this, I think, sometimes there might be cases, when gcc doesn't complain. Or (more likely) the header is pulled in via another one, or some other header has this forward-declaration. But in any case I wouldn't rely on either of those and declare what's needed explicitly.

Quoted 3 lines

Adding a non-pointer member
+ struct sh_dma test;
lead to an error, so struct sh_dma is really unknown.

Of course, the compiler has to know the size of the struct to allocate space for it:-)

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

author
#6 lore raw

2012/9/28 Guennadi Liakhovetski <[email protected]>:

Quoted 16 lines · Bastian Hecht

[snip]

Quoted 6 lines

Oh yes, I'll add <linux/completion.h>.
About the forward declration: Do I really need it? We use a struct pointer and I am unsure if the compiler just needs to know the name and nothing more. I've tested it and included sh_flctl.h (without any DMA headers) in some driver that has no idea about DMA. gcc didn't complain.

I'm not sure about this, I think, sometimes there might be cases, when gcc doesn't complain. Or (more likely) the header is pulled in via another one, or some other header has this forward-declaration. But in any case I wouldn't rely on either of those and declare what's needed explicitly.

Quoted 7 lines
Quoted 3 lines

Adding a non-pointer member
+ struct sh_dma test;
lead to an error, so struct sh_dma is really unknown.

Of course, the compiler has to know the size of the struct to allocate space for it:-)

Ah sure - I thought I catch any header pull-ins by this test but yeah
- it can be just forward declarations too...

So I will post a merged v3 under the new title: "mtd: sh_flctl: Add DMA capabilty".
It includes the critics from here plus 2 other things:
- "Fall back to PIO" now actually falls back for the current read
operation and not just for future reads
- I removed the init_completion() after each DMA read. The completion
gets set up at probe time and what I read on the internet and in researched in the sources we don't need to reset it then after each usage.

thanks,

Bastian