[PATCH 0/2] Add DMA and device tree support to the flash controller FLCTL

[PATCH 0/2] Add DMA and device tree support to the flash controller FLCTL

[Bastian Hecht]

This mini-series consists of 2 separately posted patch-sets that shrunk
to one patch each. It adds DMA support for the data part (not ECC) as
well as device tree support to the FLCTL flash controller.
As the 2nd patch is based on the 1st, I've decided to
collect them here to avoid any confusion.

The first set contained
[PATCH 1/2] mtd: sh_flctl: Setup and release DMA channels
[PATCH 2/2] mtd: sh_flctl: Use DMA for data fifo FLTDFIFO when available
and was merged after 2 reviews to
[PATCH] mtd: sh_flctl: Add DMA capabilty (same patch as in here)

The second set contained
[PATCH 1/3] mtd: sh_flctl: Probe SNAND_E flag from NAND chip
[PATCH 2/3] mtd: sh_flctl: Add device tree support
[PATCH 3/3] mtd: sh_flctl: Add sh7372 device tree config
and is merged here to
mtd: sh_flctl: Add device tree support

Patch 1 can be omitted as SNAND_E is handled in the FLCTL source anyway
correctly in set_cmd_regs() and the flag can be removed from existing board
codes without this patch.
Patch 2 without patch 3 may be confusing so I merged them too. Documentation
is added as well.

I've added linux-arm-kernel at lists.infradead.org as recipient for the
device tree part.

The patchset is based on
l2-mtd	git://git.infradead.org/users/dedekind/l2-mtd.git
with reverted commit e26c113b4130aefa1d8446602bb5b05cfd646bfe.


Bastian Hecht (2):
  mtd: sh_flctl: Add DMA capabilty
  mtd: sh_flctl: Add device tree support

 .../devicetree/bindings/mtd/flctl-nand.txt         |   37 +++
 drivers/mtd/nand/sh_flctl.c                        |  266 +++++++++++++++++++-
 include/linux/mtd/sh_flctl.h                       |   12 +
 3 files changed, 305 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/flctl-nand.txt

-- 
1.7.5.4

[PATCH 1/2] mtd: sh_flctl: Add DMA capabilty

[Bastian Hecht]

The code probes if DMA channels can get allocated and tears them down at
removal/failure if needed.
If available it uses them to transfer the data part (not ECC). On
failure we fall back to PIO mode.

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

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
 drivers/mtd/nand/sh_flctl.c  |  174 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/sh_flctl.h |   12 +++
 2 files changed, 183 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 4fbfe96..0fead2a 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -24,10 +24,13 @@
 #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>
 #include <linux/pm_runtime.h>
+#include <linux/sh_dma.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 
@@ -106,6 +109,84 @@ 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) {
+		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@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;
+
+	memset(&cfg, 0, sizeof(cfg));
+	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);
@@ -261,6 +342,70 @@ static void wait_wecfifo_ready(struct sh_flctl *flctl)
 	timeout_error(flctl, __func__);
 }
 
+static int 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);
+	} else {
+		/* DMA failed, fall back to PIO */
+		flctl_release_dma(flctl);
+		dev_warn(&flctl->pdev->dev,
+			 "DMA failed, falling back to PIO\n");
+		ret = -EIO;
+		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);
+
+	/* ret > 0 is success */
+	return ret;
+}
+
 static void read_datareg(struct sh_flctl *flctl, int offset)
 {
 	unsigned long data;
@@ -279,11 +424,20 @@ 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) > 0)
+			goto convert;	/* DMA success */
+
+	/* do polling transfer */
 	for (i = 0; i < len_4align; i++) {
 		wait_rfifo_ready(flctl);
 		buf[i] = readl(FLDTFIFO(flctl));
-		buf[i] = be32_to_cpu(buf[i]);
 	}
+
+convert:
+	for (i = 0; i < len_4align; i++)
+		buf[i] = be32_to_cpu(buf[i]);
 }
 
 static enum flctl_ecc_res_t read_ecfiforeg
@@ -308,13 +462,23 @@ 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;
+
+	for (i = 0; i < len_4align; i++)
+			buf[i] = cpu_to_be32(buf[i]);
+
+	/* initiate DMA transfer */
+	if (flctl->chan_fifo0_tx && rlen >= 32 &&
+		flctl_dma_fifo0_transfer(flctl, buf, rlen, DMA_MEM_TO_DEV) > 0)
+			return;	/* DMA success */
+
+	/* do polling transfer */
 	for (i = 0; i < len_4align; i++) {
 		wait_wfifo_ready(flctl);
-		writel(cpu_to_be32(data[i]), fifo_addr);
+		writel(buf[i], fifo_addr);
 	}
 }
 
@@ -930,6 +1094,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 +1113,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 +1127,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);
diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
index 01e4b15..e98fe7e 100644
--- a/include/linux/mtd/sh_flctl.h
+++ b/include/linux/mtd/sh_flctl.h
@@ -20,6 +20,7 @@
 #ifndef __SH_FLCTL_H__
 #define __SH_FLCTL_H__
 
+#include <linux/completion.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
@@ -107,6 +108,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 */
@@ -138,6 +140,8 @@ enum flctl_ecc_res_t {
 	FL_TIMEOUT
 };
 
+struct dma_chan;
+
 struct sh_flctl {
 	struct mtd_info		mtd;
 	struct nand_chip	chip;
@@ -161,6 +165,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 +179,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)
-- 
1.7.5.4

[PATCH 2/2] mtd: sh_flctl: Add device tree support

[Bastian Hecht]

The flctl can now be probed via device tree setup in addition to the
existing platform data way.

SoC specific setup data is set in the .data member of the OF match, so
kept within the driver itself, while board/user specific setup - like
partitioning - is taken from the device tree.

Actual configuration is added for the SoC sh7372.

Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
 .../devicetree/bindings/mtd/flctl-nand.txt         |   37 ++++++++
 drivers/mtd/nand/sh_flctl.c                        |   92 ++++++++++++++++++--
 2 files changed, 122 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/flctl-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/flctl-nand.txt b/Documentation/devicetree/bindings/mtd/flctl-nand.txt
new file mode 100644
index 0000000..f615031
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/flctl-nand.txt
@@ -0,0 +1,37 @@
+FLCTL NAND controller
+
+Required properties:
+- compatible : "renesas,shmobile-flctl-sh7372"
+- reg : Address range of the FLCTL
+- interrupts : flste IRQ number
+- nand-bus-width : bus width to NAND chip
+
+The device tree may optionally contain sub-nodes describing partitions of the
+address space. See partition.txt for more detail.
+
+Example:
+
+	flctl at e6a30000 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "renesas,shmobile-flctl-sh7372";
+		reg = <0xe6a30000 0x100>;
+		interrupts = <0x0d80>;
+
+		nand-bus-width = <16>;
+
+		system at 0 {
+			label = "system";
+			reg = <0x0 0x8000000>;
+		};
+
+		userdata at 8000000 {
+			label = "userdata";
+			reg = <0x8000000 0x10000000>;
+		};
+
+		cache at 18000000 {
+			label = "cache";
+			reg = <0x18000000 0x8000000>;
+		};
+	};
diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 0fead2a..4c7e542 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -28,6 +28,9 @@
 #include <linux/dma-mapping.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_mtd.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/sh_dma.h>
@@ -1020,6 +1023,73 @@ static irqreturn_t flctl_handle_flste(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_OF
+struct flctl_soc_config {
+	unsigned long flcmncr_val;
+	unsigned has_hwecc:1;
+	unsigned use_holden:1;
+};
+
+static struct flctl_soc_config flctl_sh7372_config = {
+	.flcmncr_val = CLK_16B_12L_4H | TYPESEL_SET | SHBUSSEL,
+	.has_hwecc = 1,
+	.use_holden = 1,
+};
+
+static const struct of_device_id of_flctl_match[] = {
+	{ .compatible = "renesas,shmobile-flctl-sh7372",
+				.data = &flctl_sh7372_config },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_flctl_match);
+
+static struct sh_flctl_platform_data *flctl_parse_dt(struct device *dev)
+{
+	const struct of_device_id *match;
+	struct flctl_soc_config *config;
+	struct sh_flctl_platform_data *pdata;
+	struct device_node *dn = dev->of_node;
+	int ret;
+
+	match = of_match_device(of_flctl_match, dev);
+	if (match)
+		config = (struct flctl_soc_config *)match->data;
+	else {
+		dev_err(dev, "%s: no OF configuration attached\n", __func__);
+		return NULL;
+	}
+
+	pdata = devm_kzalloc(dev, sizeof(struct sh_flctl_platform_data),
+								GFP_KERNEL);
+	if (!pdata) {
+		dev_err(dev, "%s: failed to allocate config data\n", __func__);
+		return NULL;
+	}
+
+	/* set SoC specific options */
+	pdata->flcmncr_val = config->flcmncr_val;
+	pdata->has_hwecc = config->has_hwecc;
+	pdata->use_holden = config->use_holden;
+
+	/* parse user defined options */
+	ret = of_get_nand_bus_width(dn);
+	if (ret == 16)
+		pdata->flcmncr_val |= SEL_16BIT;
+	else if (ret != 8) {
+		dev_err(dev, "%s: invalid bus width\n", __func__);
+		return NULL;
+	}
+
+	return pdata;
+}
+#else /* CONFIG_OF */
+#define of_flctl_match NULL
+static struct sh_flctl_platform_data *flctl_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF */
+
 static int __devinit flctl_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -1029,12 +1099,7 @@ static int __devinit flctl_probe(struct platform_device *pdev)
 	struct sh_flctl_platform_data *pdata;
 	int ret = -ENXIO;
 	int irq;
-
-	pdata = pdev->dev.platform_data;
-	if (pdata == NULL) {
-		dev_err(&pdev->dev, "no platform data defined\n");
-		return -EINVAL;
-	}
+	struct mtd_part_parser_data ppdata = {};
 
 	flctl = kzalloc(sizeof(struct sh_flctl), GFP_KERNEL);
 	if (!flctl) {
@@ -1066,6 +1131,16 @@ static int __devinit flctl_probe(struct platform_device *pdev)
 		goto err_flste;
 	}
 
+	if (pdev->dev.of_node)
+		pdata = flctl_parse_dt(&pdev->dev);
+	else
+		pdata = pdev->dev.platform_data;
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "no setup data defined\n");
+		return -EINVAL;
+	}
+
 	platform_set_drvdata(pdev, flctl);
 	flctl_mtd = &flctl->mtd;
 	nand = &flctl->chip;
@@ -1108,7 +1183,9 @@ static int __devinit flctl_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_chip;
 
-	mtd_device_register(flctl_mtd, pdata->parts, pdata->nr_parts);
+	ppdata.of_node = pdev->dev.of_node;
+	ret = mtd_device_parse_register(flctl_mtd, NULL, &ppdata, pdata->parts,
+			pdata->nr_parts);
 
 	return 0;
 
@@ -1142,6 +1219,7 @@ static struct platform_driver flctl_driver = {
 	.driver = {
 		.name	= "sh_flctl",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_flctl_match,
 	},
 };
 
-- 
1.7.5.4

[PATCH 2/2] mtd: sh_flctl: Add device tree support

[Arnd Bergmann]

On Tuesday 02 October 2012, Bastian Hecht wrote:
> +Required properties:
> +- compatible : "renesas,shmobile-flctl-sh7372"
> +- reg : Address range of the FLCTL
> +- interrupts : flste IRQ number
> +- nand-bus-width : bus width to NAND chip
> +
> +The device tree may optionally contain sub-nodes describing partitions of the
> +address space. See partition.txt for more detail.
> +
> +Example:
> +
> +       flctl at e6a30000 {
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               compatible = "renesas,shmobile-flctl-sh7372";
> +               reg = <0xe6a30000 0x100>;
> +               interrupts = <0x0d80>;
> +
> +               nand-bus-width = <16>;
> +
> +               system at 0 {
> +                       label = "system";
> +                       reg = <0x0 0x8000000>;
> +               };
> +
> +               userdata at 8000000 {
> +                       label = "userdata";
> +                       reg = <0x8000000 0x10000000>;
> +               };
> +
> +               cache at 18000000 {
> +                       label = "cache";
> +                       reg = <0x18000000 0x8000000>;
> +               };
> +       };

Since you are also adding dma-engine support, I would suggest you specify
a "dmas" and "dma-names" property as well, so the device can find the
right dma channel. The code might not do that yet while you're still
sorting out the dependencies (and the sh dmaengine code is not yet
converted to DT), but I think it would be good to nail down the binding
for this device.

	Arnd

[PATCH 1/2] mtd: sh_flctl: Add DMA capabilty

[Guennadi Liakhovetski]

On Tue, 2 Oct 2012, Bastian Hecht wrote:

> The code probes if DMA channels can get allocated and tears them down at
> removal/failure if needed.
> If available it uses them to transfer the data part (not ECC). On
> failure we fall back to PIO mode.
> 
> Based on Guennadi Liakhovetski's code from the sh_mmcif driver.
> 
> Signed-off-by: Bastian Hecht <hechtb@gmail.com>

Looks mostly good to me, just a couple of cosmetic remarks below. They are 
not critical, so, I won't be upset, if you fix them later:-) In any case

Reviewed-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

> ---
>  drivers/mtd/nand/sh_flctl.c  |  174 +++++++++++++++++++++++++++++++++++++++++-
>  include/linux/mtd/sh_flctl.h |   12 +++
>  2 files changed, 183 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index 4fbfe96..0fead2a 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -24,10 +24,13 @@
>  #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>
>  #include <linux/pm_runtime.h>
> +#include <linux/sh_dma.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  
> @@ -106,6 +109,84 @@ 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);

I think I mentioned in the first review, that it is good to include 
headers everywhere, where they are needed and not rely on pulling them in 
via other headers. So, it would be good to also include complete.h here 
directly.

[snip]

> @@ -308,13 +462,23 @@ 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;
> +
> +	for (i = 0; i < len_4align; i++)
> +			buf[i] = cpu_to_be32(buf[i]);

indentation ran away here.

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

[PATCH 2/2] mtd: sh_flctl: Add device tree support

[Guennadi Liakhovetski]

Hi Arnd

On Tue, 2 Oct 2012, Arnd Bergmann wrote:

> On Tuesday 02 October 2012, Bastian Hecht wrote:
> > +Required properties:
> > +- compatible : "renesas,shmobile-flctl-sh7372"
> > +- reg : Address range of the FLCTL
> > +- interrupts : flste IRQ number
> > +- nand-bus-width : bus width to NAND chip
> > +
> > +The device tree may optionally contain sub-nodes describing partitions of the
> > +address space. See partition.txt for more detail.
> > +
> > +Example:
> > +
> > +       flctl at e6a30000 {
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +               compatible = "renesas,shmobile-flctl-sh7372";
> > +               reg = <0xe6a30000 0x100>;
> > +               interrupts = <0x0d80>;
> > +
> > +               nand-bus-width = <16>;
> > +
> > +               system at 0 {
> > +                       label = "system";
> > +                       reg = <0x0 0x8000000>;
> > +               };
> > +
> > +               userdata at 8000000 {
> > +                       label = "userdata";
> > +                       reg = <0x8000000 0x10000000>;
> > +               };
> > +
> > +               cache at 18000000 {
> > +                       label = "cache";
> > +                       reg = <0x18000000 0x8000000>;
> > +               };
> > +       };
> 
> Since you are also adding dma-engine support, I would suggest you specify
> a "dmas" and "dma-names" property as well, so the device can find the
> right dma channel. The code might not do that yet while you're still
> sorting out the dependencies (and the sh dmaengine code is not yet
> converted to DT), but I think it would be good to nail down the binding
> for this device.

Have DMA DT bindings been accepted yet, are they fixed? Wouldn't it be 
better to wait until patches appear in a tree, at least with high 
probability heading towards the mainline, or have they already?

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

[PATCH 2/2] mtd: sh_flctl: Add device tree support

[Arnd Bergmann]

On Tuesday 02 October 2012, Guennadi Liakhovetski wrote:
> > 
> > Since you are also adding dma-engine support, I would suggest you specify
> > a "dmas" and "dma-names" property as well, so the device can find the
> > right dma channel. The code might not do that yet while you're still
> > sorting out the dependencies (and the sh dmaengine code is not yet
> > converted to DT), but I think it would be good to nail down the binding
> > for this device.
> 
> Have DMA DT bindings been accepted yet, are they fixed? Wouldn't it be 
> better to wait until patches appear in a tree, at least with high 
> probability heading towards the mainline, or have they already?
> 

I think they ended up not getting scheduled for 3.7, which is a bit disappointing
after we got an agreement on the binding.  However, I am rather confident that
we won't change the binding again, at least I don't see how they would change
in a way that would impact the binding document for a device using a DMA channel.

	Arnd

[PATCH 2/2] mtd: sh_flctl: Add device tree support

[Bastian Hecht]

Hi Arnd, hi Guennadi,

2012/10/2 Arnd Bergmann <arnd.bergmann@linaro.org>:
> On Tuesday 02 October 2012, Guennadi Liakhovetski wrote:
>> >
>> > Since you are also adding dma-engine support, I would suggest you specify
>> > a "dmas" and "dma-names" property as well, so the device can find the
>> > right dma channel. The code might not do that yet while you're still
>> > sorting out the dependencies (and the sh dmaengine code is not yet
>> > converted to DT), but I think it would be good to nail down the binding
>> > for this device.

Ok good, so I will include these field in v2. The FLCTL has support
for 4 channels, 2 for data read/write and 2 for ECC read/write.
The driver currently supports only the data part. Should I include the
ECC part nevertheless?

Further would you prefer the naming
 - fifo0_rx fifo0_tx fifo1_rx fifo1_tx (close to the datasheet) or
 - data_rx data_tx ecc_rx ecc_tx

Then the dmas field:
If I get it right, we have 3 possible DMA controllers to use on the
sh7372 and we specify the mid/rid value as second argument?

So how about this to add to the docs:

dmas = <&dma1 0x83 /* fifo0_rx */
&dma1 0x83 /* fifo0_tx */
&dma2 0x83 /* fifo0_rx */
&dma2 0x83 /* fifo0_tx */
&dma3 0x83 /* fifo0_rx */
&dma3 0x83>; /* fifo0_tx */
dma-names = "fifo0_rx", "fifo0_tx", fifo0_rx", "fifo0_tx", fifo0_rx",
"fifo0_tx";

Many thanks for the help,

 Bastian

[PATCH 1/2] mtd: sh_flctl: Add DMA capabilty

[Bastian Hecht]

Hey Guennadi,

2012/10/2 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> On Tue, 2 Oct 2012, Bastian Hecht wrote:
>
>> The code probes if DMA channels can get allocated and tears them down at
>> removal/failure if needed.
>> If available it uses them to transfer the data part (not ECC). On
>> failure we fall back to PIO mode.
>>
>> Based on Guennadi Liakhovetski's code from the sh_mmcif driver.
>>
>> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
>
> Looks mostly good to me, just a couple of cosmetic remarks below. They are
> not critical, so, I won't be upset, if you fix them later:-) In any case
>
> Reviewed-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Great, will add that to the next version!

>> ---
>>  drivers/mtd/nand/sh_flctl.c  |  174 +++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/mtd/sh_flctl.h |   12 +++
>>  2 files changed, 183 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
>> index 4fbfe96..0fead2a 100644
>> --- a/drivers/mtd/nand/sh_flctl.c
>> +++ b/drivers/mtd/nand/sh_flctl.c
>> @@ -24,10 +24,13 @@
>>  #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>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/sh_dma.h>
>>  #include <linux/slab.h>
>>  #include <linux/string.h>
>>
>> @@ -106,6 +109,84 @@ 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);
>
> I think I mentioned in the first review, that it is good to include
> headers everywhere, where they are needed and not rely on pulling them in
> via other headers. So, it would be good to also include complete.h here
> directly.

Ok sure, I will include that as well.

> [snip]



>> @@ -308,13 +462,23 @@ 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;
>> +
>> +     for (i = 0; i < len_4align; i++)
>> +                     buf[i] = cpu_to_be32(buf[i]);
>
> indentation ran away here.

Ah nice, thanks for the hawk's eye ;)

> Thanks
> Guennadi

Thanks!
 Bastian