[PATCH v7,1/3] dt-bindings: binding for jz4780-{nand,bch}

[PATCH v7,1/3] dt-bindings: binding for jz4780-{nand,bch}

[Harvey Hunt]

From: Alex Smith <alex.smith@imgtec.com>

Add DT bindings for NAND devices connected to the NEMC on JZ4780 SoCs,
as well as the hardware BCH controller, used by the jz4780_{nand,bch}
drivers.

Signed-off-by: Alex Smith <alex.smith@imgtec.com>
Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: linux-mtd@lists.infradead.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Alex Smith <alex@alex-smith.me.uk>
Signed-off-by: Harvey Hunt <harvey.hunt@imgtec.com>
---
v6 -> v7:
 - Add nand-ecc-mode to DT bindings.
 - Add nand-on-flash-bbt to DT bindings.

v5 -> v6:
 - No change.

v4 -> v5:
 - Rename ingenic,bch-device to ingenic,bch-controller to fit with
   existing convention.

v3 -> v4:
 - No change

v2 -> v3:
 - Rebase to 4.0-rc6
 - Changed ingenic,ecc-size to common nand-ecc-step-size
 - Changed ingenic,ecc-strength to common nand-ecc-strength
 - Changed ingenic,busy-gpio to common rb-gpios
 - Changed ingenic,wp-gpio to common wp-gpios

v1 -> v2:
 - Rebase to 4.0-rc3

 .../bindings/mtd/ingenic,jz4780-nand.txt           | 61 ++++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
new file mode 100644
index 0000000..44a0468
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
@@ -0,0 +1,61 @@
+* Ingenic JZ4780 NAND/BCH
+
+This file documents the device tree bindings for NAND flash devices on the
+JZ4780. NAND devices are connected to the NEMC controller (described in
+memory-controllers/ingenic,jz4780-nemc.txt), and thus NAND device nodes must
+be children of the NEMC node.
+
+Required NAND device properties:
+- compatible: Should be set to "ingenic,jz4780-nand".
+- reg: For each bank with a NAND chip attached, should specify a bank number,
+  an offset of 0 and a size of 0x1000000 (i.e. the whole NEMC bank).
+
+Optional NAND device properties:
+- ingenic,bch-controller: To make use of the hardware BCH controller, this
+  property must contain a phandle for the BCH controller node. The required
+  properties for this node are described below. If this is not specified,
+  software BCH will be used instead.
+- nand-ecc-step-size: ECC block size in bytes.
+- nand-ecc-strength: ECC strength (max number of correctable bits).
+- nand-ecc-mode: String, operation mode of the NAND ecc mode. "hw" by default
+- nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
+- rb-gpios: GPIO specifier for the busy pin.
+- wp-gpios: GPIO specifier for the write protect pin.
+
+Example:
+
+nemc: nemc@13410000 {
+	...
+
+	nand: nand@1 {
+		compatible = "ingenic,jz4780-nand";
+		reg = <1 0 0x1000000>;	/* Bank 1 */
+
+		ingenic,bch-controller = <&bch>;
+		nand-ecc-step-size = <1024>;
+		nand-ecc-strength = <24>;
+		nand-ecc-mode = "hw";
+		nand-on-flash-bbt;
+
+		rb-gpios = <&gpa 20 GPIO_ACTIVE_LOW>;
+		wp-gpios = <&gpf 22 GPIO_ACTIVE_LOW>;
+	};
+};
+
+The BCH controller is a separate SoC component used for error correction on
+NAND devices. The following is a description of the device properties for a
+BCH controller.
+
+Required BCH properties:
+- compatible: Should be set to "ingenic,jz4780-bch".
+- reg: Should specify the BCH controller registers location and length.
+- clocks: Clock for the BCH controller.
+
+Example:
+
+bch: bch@134d0000 {
+	compatible = "ingenic,jz4780-bch";
+	reg = <0x134d0000 0x10000>;
+
+	clocks = <&cgu JZ4780_CLK_BCH>;
+};
-- 
2.6.0

[PATCH v7,2/3] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs

[Harvey Hunt]

From: Alex Smith <alex.smith@imgtec.com>

Add a driver for NAND devices connected to the NEMC on JZ4780 SoCs, as
well as the hardware BCH controller. DMA is not currently implemented.

While older 47xx SoCs also have a BCH controller, they are incompatible
with the one in the 4780 due to differing register/bit positions, which
would make implementing a common driver for them quite messy.

Signed-off-by: Alex Smith <alex.smith@imgtec.com>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: linux-mtd@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: Alex Smith <alex@alex-smith.me.uk>
Signed-off-by: Harvey Hunt <harvey.hunt@imgtec.com>
---
v6 -> v7:
 - Include linux/bitops.h.
 - Default ECC mode to hw.
 - Only check BCH if we're using hw ECC mode.
 - Return -ENODEV if we're using hw ECC mode and can't find BCH.
 - Return -ENODEV if we can't find any banks.
 - Use set nand_chip->dn so we can use nand_dt_init() to read DT.

v5 -> v6:
 - Remove another printk made useless by changes in v5 (due to
   devm_ioremap_resource() printing errors itself).

v4 -> v5:
 - Bump RB_DELAY up to be sufficient for the driver to work without a
   busy GPIO available.
 - Use readl_poll_timeout instead of custom polling loop.
 - Remove useless printks.
 - Change a BUG_ON to WARN_ON.
 - Remove use of of_translate_address(), use standard platform resource
   APIs.
 - Add DRV_NAME define to avoid duplication of the same string.

v3 -> v4:
 - Rebase to 4.2-rc4
 - Change ECC_HW_OOB_FIRST to ECC_HW, reading OOB first is not necessary.
 - Fix argument to get_device() in jz4780_bch_get()

v2 -> v3:
 - Rebase to 4.0-rc6
 - Reflect binding changes
 - get/put_device in bch get/release
 - Removed empty .remove() callback
 - Removed .owner
 - Set mtd->dev.parent

v1 -> v2:
 - Fixed module license macro
 - Rebase to 4.0-rc3

 drivers/mtd/nand/Kconfig       |   7 +
 drivers/mtd/nand/Makefile      |   1 +
 drivers/mtd/nand/jz4780_bch.c  | 349 +++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/jz4780_bch.h  |  42 +++++
 drivers/mtd/nand/jz4780_nand.c | 384 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 783 insertions(+)
 create mode 100644 drivers/mtd/nand/jz4780_bch.c
 create mode 100644 drivers/mtd/nand/jz4780_bch.h
 create mode 100644 drivers/mtd/nand/jz4780_nand.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 3324281..8ffd5cd 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -508,6 +508,13 @@ config MTD_NAND_JZ4740
 	help
 		Enables support for NAND Flash on JZ4740 SoC based boards.
 
+config MTD_NAND_JZ4780
+	tristate "Support for NAND on JZ4780 SoC"
+	depends on MACH_JZ4780 && JZ4780_NEMC
+	help
+	  Enables support for NAND Flash connected to the NEMC on JZ4780 SoC
+	  based boards, using the BCH controller for hardware error correction.
+
 config MTD_NAND_FSMC
 	tristate "Support for NAND on ST Micros FSMC"
 	depends on PLAT_SPEAR || ARCH_NOMADIK || ARCH_U8500 || MACH_U300
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 075a027..604b166 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_MTD_NAND_NUC900)		+= nuc900_nand.o
 obj-$(CONFIG_MTD_NAND_MPC5121_NFC)	+= mpc5121_nfc.o
 obj-$(CONFIG_MTD_NAND_RICOH)		+= r852.o
 obj-$(CONFIG_MTD_NAND_JZ4740)		+= jz4740_nand.o
+obj-$(CONFIG_MTD_NAND_JZ4780)		+= jz4780_nand.o jz4780_bch.o
 obj-$(CONFIG_MTD_NAND_GPMI_NAND)	+= gpmi-nand/
 obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
 obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
diff --git a/drivers/mtd/nand/jz4780_bch.c b/drivers/mtd/nand/jz4780_bch.c
new file mode 100644
index 0000000..047d351
--- /dev/null
+++ b/drivers/mtd/nand/jz4780_bch.c
@@ -0,0 +1,349 @@
+/*
+ * JZ4780 BCH controller
+ *
+ * Copyright (c) 2015 Imagination Technologies
+ * Author: Alex Smith <alex.smith@imgtec.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+
+#include "jz4780_bch.h"
+
+#define BCH_BHCR			0x0
+#define BCH_BHCCR			0x8
+#define BCH_BHCNT			0xc
+#define BCH_BHDR			0x10
+#define BCH_BHPAR0			0x14
+#define BCH_BHERR0			0x84
+#define BCH_BHINT			0x184
+#define BCH_BHINTES			0x188
+#define BCH_BHINTEC			0x18c
+#define BCH_BHINTE			0x190
+
+#define BCH_BHCR_BSEL_SHIFT		4
+#define BCH_BHCR_BSEL_MASK		(0x7f << BCH_BHCR_BSEL_SHIFT)
+#define BCH_BHCR_ENCE			BIT(2)
+#define BCH_BHCR_INIT			BIT(1)
+#define BCH_BHCR_BCHE			BIT(0)
+
+#define BCH_BHCNT_PARITYSIZE_SHIFT	16
+#define BCH_BHCNT_PARITYSIZE_MASK	(0x7f << BCH_BHCNT_PARITYSIZE_SHIFT)
+#define BCH_BHCNT_BLOCKSIZE_SHIFT	0
+#define BCH_BHCNT_BLOCKSIZE_MASK	(0x7ff << BCH_BHCNT_BLOCKSIZE_SHIFT)
+
+#define BCH_BHERR_MASK_SHIFT		16
+#define BCH_BHERR_MASK_MASK		(0xffff << BCH_BHERR_MASK_SHIFT)
+#define BCH_BHERR_INDEX_SHIFT		0
+#define BCH_BHERR_INDEX_MASK		(0x7ff << BCH_BHERR_INDEX_SHIFT)
+
+#define BCH_BHINT_ERRC_SHIFT		24
+#define BCH_BHINT_ERRC_MASK		(0x7f << BCH_BHINT_ERRC_SHIFT)
+#define BCH_BHINT_TERRC_SHIFT		16
+#define BCH_BHINT_TERRC_MASK		(0x7f << BCH_BHINT_TERRC_SHIFT)
+#define BCH_BHINT_DECF			BIT(3)
+#define BCH_BHINT_ENCF			BIT(2)
+#define BCH_BHINT_UNCOR			BIT(1)
+#define BCH_BHINT_ERR			BIT(0)
+
+#define BCH_CLK_RATE			(200 * 1000 * 1000)
+
+/* Timeout for BCH calculation/correction in microseconds. */
+#define BCH_TIMEOUT			100000
+
+struct jz4780_bch {
+	void __iomem *base;
+	struct clk *clk;
+};
+
+static void jz4780_bch_init(struct jz4780_bch *bch,
+			    struct jz4780_bch_params *params, bool encode)
+{
+	uint32_t reg;
+
+	/* Clear interrupt status. */
+	writel(readl(bch->base + BCH_BHINT), bch->base + BCH_BHINT);
+
+	/* Set up BCH count register. */
+	reg = params->size << BCH_BHCNT_BLOCKSIZE_SHIFT;
+	reg |= params->bytes << BCH_BHCNT_PARITYSIZE_SHIFT;
+	writel(reg, bch->base + BCH_BHCNT);
+
+	/* Initialise and enable BCH. */
+	reg = BCH_BHCR_BCHE | BCH_BHCR_INIT;
+	reg |= params->strength << BCH_BHCR_BSEL_SHIFT;
+	if (encode)
+		reg |= BCH_BHCR_ENCE;
+	writel(reg, bch->base + BCH_BHCR);
+}
+
+static void jz4780_bch_disable(struct jz4780_bch *bch)
+{
+	writel(readl(bch->base + BCH_BHINT), bch->base + BCH_BHINT);
+	writel(BCH_BHCR_BCHE, bch->base + BCH_BHCCR);
+}
+
+static void jz4780_bch_write_data(struct jz4780_bch *bch, const void *buf,
+				  size_t size)
+{
+	size_t size32 = size / sizeof(uint32_t);
+	size_t size8 = size & (sizeof(uint32_t) - 1);
+	const uint32_t *src32;
+	const uint8_t *src8;
+
+	src32 = (const uint32_t *)buf;
+	while (size32--)
+		writel(*src32++, bch->base + BCH_BHDR);
+
+	src8 = (const uint8_t *)src32;
+	while (size8--)
+		writeb(*src8++, bch->base + BCH_BHDR);
+}
+
+static void jz4780_bch_read_parity(struct jz4780_bch *bch, void *buf,
+				   size_t size)
+{
+	size_t size32 = size / sizeof(uint32_t);
+	size_t size8 = size & (sizeof(uint32_t) - 1);
+	uint32_t *dest32;
+	uint8_t *dest8;
+	uint32_t val, offset = 0;
+
+	dest32 = (uint32_t *)buf;
+	while (size32--) {
+		*dest32++ = readl(bch->base + BCH_BHPAR0 + offset);
+		offset += sizeof(uint32_t);
+	}
+
+	dest8 = (uint8_t *)dest32;
+	val = readl(bch->base + BCH_BHPAR0 + offset);
+	switch (size8) {
+	case 3:
+		dest8[2] = (val >> 16) & 0xff;
+	case 2:
+		dest8[1] = (val >> 8) & 0xff;
+	case 1:
+		dest8[0] = val & 0xff;
+		break;
+	}
+}
+
+static bool jz4780_bch_wait_complete(struct jz4780_bch *bch, unsigned int irq,
+				     uint32_t *status)
+{
+	uint32_t reg;
+	int ret;
+
+	/*
+	 * While we could use use interrupts here and sleep until the operation
+	 * completes, the controller works fairly quickly (usually a few
+	 * microseconds), so the overhead of sleeping until we get an interrupt
+	 * actually quite noticeably decreases performance.
+	 */
+	ret = readl_poll_timeout(bch->base + BCH_BHINT, reg,
+				 (reg & irq) == irq, 0, BCH_TIMEOUT);
+	if (ret)
+		return false;
+
+	if (status)
+		*status = reg;
+
+	writel(reg, bch->base + BCH_BHINT);
+	return true;
+}
+
+/**
+ * jz4780_bch_calculate() - calculate ECC for a data buffer
+ * @dev: BCH device.
+ * @params: BCH parameters.
+ * @buf: input buffer with raw data.
+ * @ecc_code: output buffer with ECC.
+ *
+ * Return: 0 on success, -ETIMEDOUT if timed out while waiting for BCH
+ * controller.
+ */
+int jz4780_bch_calculate(struct device *dev, struct jz4780_bch_params *params,
+			 const uint8_t *buf, uint8_t *ecc_code)
+{
+	struct jz4780_bch *bch = dev_get_drvdata(dev);
+	int ret = 0;
+
+	jz4780_bch_init(bch, params, true);
+	jz4780_bch_write_data(bch, buf, params->size);
+
+	if (jz4780_bch_wait_complete(bch, BCH_BHINT_ENCF, NULL)) {
+		jz4780_bch_read_parity(bch, ecc_code, params->bytes);
+	} else {
+		dev_err(dev, "timed out while calculating ECC\n");
+		ret = -ETIMEDOUT;
+	}
+
+	jz4780_bch_disable(bch);
+	return ret;
+}
+EXPORT_SYMBOL(jz4780_bch_calculate);
+
+/**
+ * jz4780_bch_correct() - detect and correct bit errors
+ * @dev: BCH device.
+ * @params: BCH parameters.
+ * @buf: raw data read from the chip.
+ * @ecc_code: ECC read from the chip.
+ *
+ * Given the raw data and the ECC read from the NAND device, detects and
+ * corrects errors in the data.
+ *
+ * Return: the number of bit errors corrected, or -1 if there are too many
+ * errors to correct or we timed out waiting for the controller.
+ */
+int jz4780_bch_correct(struct device *dev, struct jz4780_bch_params *params,
+		       uint8_t *buf, uint8_t *ecc_code)
+{
+	struct jz4780_bch *bch = dev_get_drvdata(dev);
+	uint32_t reg, mask, index;
+	int i, ret, count;
+
+	jz4780_bch_init(bch, params, false);
+	jz4780_bch_write_data(bch, buf, params->size);
+	jz4780_bch_write_data(bch, ecc_code, params->bytes);
+
+	if (!jz4780_bch_wait_complete(bch, BCH_BHINT_DECF, &reg)) {
+		dev_err(dev, "timed out while correcting data\n");
+		ret = -1;
+		goto out;
+	}
+
+	if (reg & BCH_BHINT_UNCOR) {
+		dev_warn(dev, "uncorrectable ECC error\n");
+		ret = -1;
+		goto out;
+	}
+
+	/* Correct any detected errors. */
+	if (reg & BCH_BHINT_ERR) {
+		count = (reg & BCH_BHINT_ERRC_MASK) >> BCH_BHINT_ERRC_SHIFT;
+		ret = (reg & BCH_BHINT_TERRC_MASK) >> BCH_BHINT_TERRC_SHIFT;
+
+		for (i = 0; i < count; i++) {
+			reg = readl(bch->base + BCH_BHERR0 + (i * 4));
+			mask = (reg & BCH_BHERR_MASK_MASK) >>
+						BCH_BHERR_MASK_SHIFT;
+			index = (reg & BCH_BHERR_INDEX_MASK) >>
+						BCH_BHERR_INDEX_SHIFT;
+			buf[(index * 2) + 0] ^= mask;
+			buf[(index * 2) + 1] ^= mask >> 8;
+		}
+	} else {
+		ret = 0;
+	}
+
+out:
+	jz4780_bch_disable(bch);
+	return ret;
+}
+EXPORT_SYMBOL(jz4780_bch_correct);
+
+/**
+ * jz4780_bch_get() - get the BCH controller device
+ * @np: BCH device tree node.
+ * @dev: where to store pointer to BCH controller device.
+ *
+ * Gets the BCH controller device from the specified device tree node. The
+ * device must be released with jz4780_bch_release() when it is no longer being
+ * used.
+ *
+ * Return: 0 on success, -EPROBE_DEFER if the controller has not yet been
+ * initialised.
+ */
+int jz4780_bch_get(struct device_node *np, struct device **dev)
+{
+	struct platform_device *pdev;
+	struct jz4780_bch *bch;
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev || !platform_get_drvdata(pdev))
+		return -EPROBE_DEFER;
+
+	get_device(&pdev->dev);
+
+	bch = platform_get_drvdata(pdev);
+	clk_prepare_enable(bch->clk);
+
+	*dev = &pdev->dev;
+	return 0;
+}
+EXPORT_SYMBOL(jz4780_bch_get);
+
+/**
+ * jz4780_bch_release() - release the BCH controller device
+ * @dev: BCH device.
+ */
+void jz4780_bch_release(struct device *dev)
+{
+	struct jz4780_bch *bch = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(bch->clk);
+	put_device(dev);
+}
+EXPORT_SYMBOL(jz4780_bch_release);
+
+static int jz4780_bch_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct jz4780_bch *bch;
+	struct resource *res;
+
+	bch = devm_kzalloc(dev, sizeof(*bch), GFP_KERNEL);
+	if (!bch)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	bch->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(bch->base))
+		return PTR_ERR(bch->base);
+
+	jz4780_bch_disable(bch);
+
+	bch->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(bch->clk)) {
+		dev_err(dev, "failed to get clock: %ld\n", PTR_ERR(bch->clk));
+		return PTR_ERR(bch->clk);
+	}
+
+	clk_set_rate(bch->clk, BCH_CLK_RATE);
+
+	platform_set_drvdata(pdev, bch);
+	return 0;
+}
+
+static const struct of_device_id jz4780_bch_dt_match[] = {
+	{ .compatible = "ingenic,jz4780-bch" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, jz4780_bch_dt_match);
+
+static struct platform_driver jz4780_bch_driver = {
+	.probe		= jz4780_bch_probe,
+	.driver	= {
+		.name	= "jz4780-bch",
+		.of_match_table = of_match_ptr(jz4780_bch_dt_match),
+	},
+};
+module_platform_driver(jz4780_bch_driver);
+
+MODULE_AUTHOR("Alex Smith <alex.smith@imgtec.com>");
+MODULE_DESCRIPTION("Ingenic JZ4780 BCH error correction driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/mtd/nand/jz4780_bch.h b/drivers/mtd/nand/jz4780_bch.h
new file mode 100644
index 0000000..a5dfde5
--- /dev/null
+++ b/drivers/mtd/nand/jz4780_bch.h
@@ -0,0 +1,42 @@
+/*
+ * JZ4780 BCH controller
+ *
+ * Copyright (c) 2015 Imagination Technologies
+ * Author: Alex Smith <alex.smith@imgtec.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#ifndef __DRIVERS_MTD_NAND_JZ4780_BCH_H__
+#define __DRIVERS_MTD_NAND_JZ4780_BCH_H__
+
+#include <linux/types.h>
+
+struct device;
+struct device_node;
+
+/**
+ * struct jz4780_bch_params - BCH parameters
+ * @size: data bytes per ECC step.
+ * @bytes: ECC bytes per step.
+ * @strength: number of correctable bits per ECC step.
+ */
+struct jz4780_bch_params {
+	int size;
+	int bytes;
+	int strength;
+};
+
+extern int jz4780_bch_calculate(struct device *dev,
+				struct jz4780_bch_params *params,
+				const uint8_t *buf, uint8_t *ecc_code);
+extern int jz4780_bch_correct(struct device *dev,
+			      struct jz4780_bch_params *params, uint8_t *buf,
+			      uint8_t *ecc_code);
+
+extern int jz4780_bch_get(struct device_node *np, struct device **dev);
+extern void jz4780_bch_release(struct device *dev);
+
+#endif /* __DRIVERS_MTD_NAND_JZ4780_BCH_H__ */
diff --git a/drivers/mtd/nand/jz4780_nand.c b/drivers/mtd/nand/jz4780_nand.c
new file mode 100644
index 0000000..4100ddc9
--- /dev/null
+++ b/drivers/mtd/nand/jz4780_nand.c
@@ -0,0 +1,384 @@
+/*
+ * JZ4780 NAND driver
+ *
+ * Copyright (c) 2015 Imagination Technologies
+ * Author: Alex Smith <alex.smith@imgtec.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+#include <linux/of_mtd.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+
+#include <linux/jz4780-nemc.h>
+
+#include "jz4780_bch.h"
+
+#define DRV_NAME	"jz4780-nand"
+
+#define OFFSET_DATA	0x00000000
+#define OFFSET_CMD	0x00400000
+#define OFFSET_ADDR	0x00800000
+
+/* Command delay when there is no R/B pin (in microseconds). */
+#define RB_DELAY	100
+
+struct jz4780_nand_chip {
+	unsigned int bank;
+	void __iomem *base;
+};
+
+struct jz4780_nand {
+	struct mtd_info mtd;
+	struct nand_chip chip;
+
+	struct device *dev;
+	struct device *bch;
+
+	struct nand_ecclayout ecclayout;
+
+	int busy_gpio;
+	int wp_gpio;
+	unsigned int busy_gpio_active_low: 1;
+	unsigned int wp_gpio_active_low: 1;
+	unsigned int reading: 1;
+
+	unsigned int num_banks;
+	int selected;
+	struct jz4780_nand_chip chips[];
+};
+
+static inline struct jz4780_nand *to_jz4780_nand(struct mtd_info *mtd)
+{
+	return container_of(mtd, struct jz4780_nand, mtd);
+}
+
+static void jz4780_nand_select_chip(struct mtd_info *mtd, int chipnr)
+{
+	struct jz4780_nand *nand = to_jz4780_nand(mtd);
+	struct jz4780_nand_chip *chip;
+
+	if (chipnr == -1) {
+		/* Ensure the currently selected chip is deasserted. */
+		if (nand->selected >= 0) {
+			chip = &nand->chips[nand->selected];
+			jz4780_nemc_assert(nand->dev, chip->bank, false);
+		}
+	} else {
+		chip = &nand->chips[chipnr];
+		nand->chip.IO_ADDR_R = chip->base + OFFSET_DATA;
+		nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;
+	}
+
+	nand->selected = chipnr;
+}
+
+static void jz4780_nand_cmd_ctrl(struct mtd_info *mtd, int cmd,
+				 unsigned int ctrl)
+{
+	struct jz4780_nand *nand = to_jz4780_nand(mtd);
+	struct jz4780_nand_chip *chip;
+
+	if (WARN_ON(nand->selected < 0))
+		return;
+
+	chip = &nand->chips[nand->selected];
+
+	if (ctrl & NAND_CTRL_CHANGE) {
+		if (ctrl & NAND_ALE)
+			nand->chip.IO_ADDR_W = chip->base + OFFSET_ADDR;
+		else if (ctrl & NAND_CLE)
+			nand->chip.IO_ADDR_W = chip->base + OFFSET_CMD;
+		else
+			nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;
+
+		jz4780_nemc_assert(nand->dev, chip->bank, ctrl & NAND_NCE);
+	}
+
+	if (cmd != NAND_CMD_NONE)
+		writeb(cmd, nand->chip.IO_ADDR_W);
+}
+
+static int jz4780_nand_dev_ready(struct mtd_info *mtd)
+{
+	struct jz4780_nand *nand = to_jz4780_nand(mtd);
+
+	return !(gpio_get_value(nand->busy_gpio) ^ nand->busy_gpio_active_low);
+}
+
+static void jz4780_nand_ecc_hwctl(struct mtd_info *mtd, int mode)
+{
+	struct jz4780_nand *nand = to_jz4780_nand(mtd);
+
+	nand->reading = (mode == NAND_ECC_READ);
+}
+
+static int jz4780_nand_ecc_calculate(struct mtd_info *mtd, const uint8_t *dat,
+				     uint8_t *ecc_code)
+{
+	struct jz4780_nand *nand = to_jz4780_nand(mtd);
+	struct jz4780_bch_params params;
+
+	/*
+	 * Don't need to generate the ECC when reading, BCH does it for us as
+	 * part of decoding/correction.
+	 */
+	if (nand->reading)
+		return 0;
+
+	params.size = nand->chip.ecc.size;
+	params.bytes = nand->chip.ecc.bytes;
+	params.strength = nand->chip.ecc.strength;
+
+	return jz4780_bch_calculate(nand->bch, &params, dat, ecc_code);
+}
+
+static int jz4780_nand_ecc_correct(struct mtd_info *mtd, uint8_t *dat,
+				   uint8_t *read_ecc, uint8_t *calc_ecc)
+{
+	struct jz4780_nand *nand = to_jz4780_nand(mtd);
+	struct jz4780_bch_params params;
+
+	params.size = nand->chip.ecc.size;
+	params.bytes = nand->chip.ecc.bytes;
+	params.strength = nand->chip.ecc.strength;
+
+	return jz4780_bch_correct(nand->bch, &params, dat, read_ecc);
+}
+
+static int jz4780_nand_init_ecc(struct jz4780_nand *nand, struct device *dev)
+{
+	struct mtd_info *mtd = &nand->mtd;
+	struct nand_chip *chip = &nand->chip;
+	struct nand_ecclayout *layout = &nand->ecclayout;
+	struct device_node *bch_np;
+	int ret = 0;
+	uint32_t start, i;
+
+	if (!chip->ecc.size)
+		chip->ecc.size = 1024;
+
+	if (!chip->ecc.strength)
+		chip->ecc.strength = 24;
+
+	chip->ecc.bytes = fls(1 + 8 * chip->ecc.size) * chip->ecc.strength / 8;
+
+	dev_info(dev, "using %s BCH (strength %d, size %d, bytes %d)\n",
+		 (nand->bch) ? "hardware" : "software", chip->ecc.strength,
+		 chip->ecc.size, chip->ecc.bytes);
+
+	if (chip->ecc.mode == NAND_ECC_HW) {
+		bch_np = of_parse_phandle(dev->of_node,
+					"ingenic,bch-controller", 0);
+		if (bch_np) {
+			ret = jz4780_bch_get(bch_np, &nand->bch);
+			of_node_put(bch_np);
+			if (ret)
+				return ret;
+		} else {
+			dev_err(dev, "no bch controller in DT\n");
+			return -ENODEV;
+		}
+
+		chip->ecc.hwctl = jz4780_nand_ecc_hwctl;
+		chip->ecc.calculate = jz4780_nand_ecc_calculate;
+		chip->ecc.correct = jz4780_nand_ecc_correct;
+	}
+
+	/* Generate ECC layout. ECC codes are right aligned in the OOB area. */
+	layout->eccbytes = mtd->writesize / chip->ecc.size * chip->ecc.bytes;
+	start = mtd->oobsize - layout->eccbytes;
+	for (i = 0; i < layout->eccbytes; i++)
+		layout->eccpos[i] = start + i;
+
+	layout->oobfree[0].offset = 2;
+	layout->oobfree[0].length = mtd->oobsize - layout->eccbytes - 2;
+
+	chip->ecc.layout = layout;
+
+	return 0;
+}
+
+static int jz4780_nand_init_chips(struct jz4780_nand *nand,
+				  struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct jz4780_nand_chip *chip;
+	const __be32 *prop;
+	struct resource *res;
+	int i = 0;
+
+	/*
+	 * Iterate over each bank assigned to this device and request resources.
+	 * The bank numbers may not be consecutive, but nand_scan_ident()
+	 * expects chip numbers to be, so fill out a consecutive array of chips
+	 * which map chip number to actual bank number.
+	 */
+	while ((prop = of_get_address(dev->of_node, i, NULL, NULL))) {
+		chip = &nand->chips[i];
+		chip->bank = of_read_number(prop, 1);
+
+		jz4780_nemc_set_type(nand->dev, chip->bank,
+				     JZ4780_NEMC_BANK_NAND);
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		chip->base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(chip->base))
+			return PTR_ERR(chip->base);
+
+		i++;
+	}
+
+	return 0;
+}
+
+static int jz4780_nand_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	unsigned int num_banks;
+	struct jz4780_nand *nand;
+	struct mtd_info *mtd;
+	struct nand_chip *chip;
+	enum of_gpio_flags flags;
+	struct mtd_part_parser_data ppdata;
+	int ret;
+
+	num_banks = jz4780_nemc_num_banks(dev);
+	if (num_banks == 0) {
+		dev_err(dev, "no banks found\n");
+		return -ENODEV;
+	}
+
+	nand = devm_kzalloc(dev,
+			sizeof(*nand) + (sizeof(nand->chips[0]) * num_banks),
+			GFP_KERNEL);
+	if (!nand)
+		return -ENOMEM;
+
+	nand->dev = dev;
+	nand->num_banks = num_banks;
+	nand->selected = -1;
+
+	mtd = &nand->mtd;
+	chip = &nand->chip;
+	mtd->priv = chip;
+	mtd->owner = THIS_MODULE;
+	mtd->name = DRV_NAME;
+	mtd->dev.parent = dev;
+
+	chip->dn = dev->of_node;
+	chip->chip_delay = RB_DELAY;
+	chip->options = NAND_NO_SUBPAGE_WRITE;
+	chip->select_chip = jz4780_nand_select_chip;
+	chip->cmd_ctrl = jz4780_nand_cmd_ctrl;
+
+	nand->busy_gpio = of_get_named_gpio_flags(dev->of_node,
+						  "rb-gpios",
+						  0, &flags);
+	if (gpio_is_valid(nand->busy_gpio)) {
+		ret = devm_gpio_request(dev, nand->busy_gpio, "NAND busy");
+		if (ret) {
+			dev_err(dev, "failed to request busy GPIO %d: %d\n",
+				nand->busy_gpio, ret);
+			return ret;
+		}
+
+		nand->busy_gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
+		gpio_direction_input(nand->busy_gpio);
+
+		chip->dev_ready = jz4780_nand_dev_ready;
+	}
+
+	nand->wp_gpio = of_get_named_gpio_flags(dev->of_node, "wp-gpios",
+						0, &flags);
+	if (gpio_is_valid(nand->wp_gpio)) {
+		ret = devm_gpio_request(dev, nand->wp_gpio, "NAND WP");
+		if (ret) {
+			dev_err(dev, "failed to request WP GPIO %d: %d\n",
+				nand->wp_gpio, ret);
+			return ret;
+		}
+
+		nand->wp_gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
+		gpio_direction_output(nand->wp_gpio, nand->wp_gpio_active_low);
+	}
+
+	ret = jz4780_nand_init_chips(nand, pdev);
+	if (ret)
+		return ret;
+
+	chip->ecc.mode = NAND_ECC_HW;
+
+	ret = nand_scan_ident(mtd, num_banks, NULL);
+	if (ret)
+		return ret;
+
+	ret = jz4780_nand_init_ecc(nand, dev);
+	if (ret)
+		return ret;
+
+	ret = nand_scan_tail(mtd);
+	if (ret)
+		goto err_release_bch;
+
+	ppdata.of_node = dev->of_node;
+	ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
+	if (ret)
+		goto err_release_nand;
+
+	platform_set_drvdata(pdev, nand);
+	return 0;
+
+err_release_nand:
+	nand_release(mtd);
+
+err_release_bch:
+	if (nand->bch)
+		jz4780_bch_release(nand->bch);
+
+	return ret;
+}
+
+static int jz4780_nand_remove(struct platform_device *pdev)
+{
+	struct jz4780_nand *nand = platform_get_drvdata(pdev);
+
+	if (nand->bch)
+		jz4780_bch_release(nand->bch);
+
+	return 0;
+}
+
+static const struct of_device_id jz4780_nand_dt_match[] = {
+	{ .compatible = "ingenic,jz4780-nand" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, jz4780_nand_dt_match);
+
+static struct platform_driver jz4780_nand_driver = {
+	.probe		= jz4780_nand_probe,
+	.remove		= jz4780_nand_remove,
+	.driver	= {
+		.name	= DRV_NAME,
+		.of_match_table = of_match_ptr(jz4780_nand_dt_match),
+	},
+};
+module_platform_driver(jz4780_nand_driver);
+
+MODULE_AUTHOR("Alex Smith <alex.smith@imgtec.com>");
+MODULE_DESCRIPTION("Ingenic JZ4780 NAND driver");
+MODULE_LICENSE("GPL v2");
-- 
2.6.0

[PATCH v7,3/3] MIPS: dts: jz4780/ci20: Add NEMC, BCH and NAND device tree nodes

[Harvey Hunt]

From: Alex Smith <alex.smith@imgtec.com>

Add device tree nodes for the NEMC and BCH to the JZ4780 device tree,
and make use of them in the Ci20 device tree to add a node for the
board's NAND.

Note that since the pinctrl driver is not yet upstream, this includes
neither pin configuration nor busy/write-protect GPIO pins for the
NAND. Use of the NAND relies on the boot loader to have left the pins
configured in a usable state, which should be the case when booted
from the NAND.

Signed-off-by: Alex Smith <alex.smith@imgtec.com>
Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: linux-mtd@lists.infradead.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: Alex Smith <alex@alex-smith.me.uk>
Signed-off-by: Harvey Hunt <harvey.hunt@imgtec.com>
---
v6 -> v7:
 - Add nand-ecc-mode to DT.
 - Add nand-on-flash-bbt to DT.

v4 -> v5:
 - New patch adding DT nodes for the NAND so that the driver can be
   tested.

 arch/mips/boot/dts/ingenic/ci20.dts    | 54 ++++++++++++++++++++++++++++++++++
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 26 ++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index 9fcb9e7..453f1d3 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -42,3 +42,57 @@
 &uart4 {
 	status = "okay";
 };
+
+&nemc {
+	status = "okay";
+
+	nand: nand@1 {
+		compatible = "ingenic,jz4780-nand";
+		reg = <1 0 0x1000000>;
+
+		ingenic,nemc-tAS = <10>;
+		ingenic,nemc-tAH = <5>;
+		ingenic,nemc-tBP = <10>;
+		ingenic,nemc-tAW = <15>;
+		ingenic,nemc-tSTRV = <100>;
+
+		ingenic,bch-controller = <&bch>;
+		ingenic,ecc-size = <1024>;
+		ingenic,ecc-strength = <24>;
+
+		nand-ecc-mode = "hw";
+		nand-on-flash-bbt;
+
+		#address-cells = <2>;
+		#size-cells = <2>;
+
+		partition@0 {
+			label = "u-boot-spl";
+			reg = <0x0 0x0 0x0 0x800000>;
+		};
+
+		partition@0x800000 {
+			label = "u-boot";
+			reg = <0x0 0x800000 0x0 0x200000>;
+		};
+
+		partition@0xa00000 {
+			label = "u-boot-env";
+			reg = <0x0 0xa00000 0x0 0x200000>;
+		};
+
+		partition@0xc00000 {
+			label = "boot";
+			reg = <0x0 0xc00000 0x0 0x4000000>;
+		};
+
+		partition@0x8c00000 {
+			label = "system";
+			reg = <0x0 0x4c00000 0x1 0xfb400000>;
+		};
+	};
+};
+
+&bch {
+	status = "okay";
+};
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 65389f6..b868b42 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -108,4 +108,30 @@
 
 		status = "disabled";
 	};
+
+	nemc: nemc@13410000 {
+		compatible = "ingenic,jz4780-nemc";
+		reg = <0x13410000 0x10000>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+		ranges = <1 0 0x1b000000 0x1000000
+			  2 0 0x1a000000 0x1000000
+			  3 0 0x19000000 0x1000000
+			  4 0 0x18000000 0x1000000
+			  5 0 0x17000000 0x1000000
+			  6 0 0x16000000 0x1000000>;
+
+		clocks = <&cgu JZ4780_CLK_NEMC>;
+
+		status = "disabled";
+	};
+
+	bch: bch@134d0000 {
+		compatible = "ingenic,jz4780-bch";
+		reg = <0x134d0000 0x10000>;
+
+		clocks = <&cgu JZ4780_CLK_BCH>;
+
+		status = "disabled";
+	};
 };
-- 
2.6.0

Re: [PATCH v7,3/3] MIPS: dts: jz4780/ci20: Add NEMC, BCH and NAND device tree nodes

[Ezequiel Garcia]

On 6 October 2015 at 13:27, Harvey Hunt <harvey.hunt@imgtec.com> wrote:
> From: Alex Smith <alex.smith@imgtec.com>
>
> Add device tree nodes for the NEMC and BCH to the JZ4780 device tree,
> and make use of them in the Ci20 device tree to add a node for the
> board's NAND.
>
> Note that since the pinctrl driver is not yet upstream, this includes
> neither pin configuration nor busy/write-protect GPIO pins for the
> NAND. Use of the NAND relies on the boot loader to have left the pins
> configured in a usable state, which should be the case when booted
> from the NAND.
>
> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: Paul Burton <paul.burton@imgtec.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> Cc: Alex Smith <alex@alex-smith.me.uk>
> Signed-off-by: Harvey Hunt <harvey.hunt@imgtec.com>
> ---
> v6 -> v7:
>  - Add nand-ecc-mode to DT.
>  - Add nand-on-flash-bbt to DT.
>
> v4 -> v5:
>  - New patch adding DT nodes for the NAND so that the driver can be
>    tested.
>
>  arch/mips/boot/dts/ingenic/ci20.dts    | 54 ++++++++++++++++++++++++++++++++++
>  arch/mips/boot/dts/ingenic/jz4780.dtsi | 26 ++++++++++++++++
>  2 files changed, 80 insertions(+)
>
> diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
> index 9fcb9e7..453f1d3 100644
> --- a/arch/mips/boot/dts/ingenic/ci20.dts
> +++ b/arch/mips/boot/dts/ingenic/ci20.dts
> @@ -42,3 +42,57 @@
>  &uart4 {
>         status = "okay";
>  };
> +
> +&nemc {
> +       status = "okay";
> +
> +       nand: nand@1 {
> +               compatible = "ingenic,jz4780-nand";
> +               reg = <1 0 0x1000000>;
> +

Why is this in the ci20.dts instead of the SoC dtsi?

Seems at least compatible and reg is not board-specific.

Thanks,
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

RE: [PATCH v7,3/3] MIPS: dts: jz4780/ci20: Add NEMC, BCH and NAND device tree nodes

[Harvey Hunt]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2826 bytes --]

On 8 October 2015 at 22:23, Ezequiel Garcia < ezequiel@vanguardiasur.com.ar> wrote:
>On 6 October 2015 at 13:27, Harvey Hunt <harvey.hunt@imgtec.com> wrote:
>> From: Alex Smith <alex.smith@imgtec.com>
>>
>> Add device tree nodes for the NEMC and BCH to the JZ4780 device tree,
>> and make use of them in the Ci20 device tree to add a node for the
>> board's NAND.
>>
>> Note that since the pinctrl driver is not yet upstream, this includes
>> neither pin configuration nor busy/write-protect GPIO pins for the
>> NAND. Use of the NAND relies on the boot loader to have left the pins
>> configured in a usable state, which should be the case when booted
>> from the NAND.
>>
>> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
>> Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: Brian Norris <computersforpeace@gmail.com>
>> Cc: Paul Burton <paul.burton@imgtec.com>
>> Cc: linux-mtd@lists.infradead.org
>> Cc: devicetree@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-mips@linux-mips.org
>> Cc: Alex Smith <alex@alex-smith.me.uk>
>> Signed-off-by: Harvey Hunt <harvey.hunt@imgtec.com>
>> ---
>> v6 -> v7:
>>  - Add nand-ecc-mode to DT.
>>  - Add nand-on-flash-bbt to DT.
>>
>> v4 -> v5:
>>  - New patch adding DT nodes for the NAND so that the driver can be
>>    tested.
>>
>>  arch/mips/boot/dts/ingenic/ci20.dts    | 54 ++++++++++++++++++++++++++++++++++
>>  arch/mips/boot/dts/ingenic/jz4780.dtsi | 26 ++++++++++++++++
>>  2 files changed, 80 insertions(+)
>>
>> diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
>> index 9fcb9e7..453f1d3 100644
>> --- a/arch/mips/boot/dts/ingenic/ci20.dts
>> +++ b/arch/mips/boot/dts/ingenic/ci20.dts
>> @@ -42,3 +42,57 @@
>>  &uart4 {
>>         status = "okay";
>>  };
>> +
>> +&nemc {
>> +       status = "okay";
>> +
>> +       nand: nand@1 {
>> +               compatible = "ingenic,jz4780-nand";
>> +               reg = <1 0 0x1000000>;
>> +
>
>Why is this in the ci20.dts instead of the SoC dtsi?
>
>Seems at least compatible and reg is not board-specific.
>
>Thanks,
>-- 
>Ezequiel García, VanguardiaSur
>www.vanguardiasur.com.ar

Hi Ezequiel,

The number of NAND nodes under the NEMC node is board specific - some devices
could have 2 NAND banks and others could have none. Including the compatible
property in jz4780.dtsi would imply that all JZ4780 boards have at least one NAND bank.

The size in the reg property would be the same for all NAND devices (as it refers to the
NAND registers), however the bank number would be different, so that can also be seen
as board specific.

Thanks,

Harvey
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v7,3/3] MIPS: dts: jz4780/ci20: Add NEMC, BCH and NAND device tree nodes

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 4003 bytes --]

Hi Harvey,

On Tue, Oct 06, 2015 at 05:27:17PM +0100, Harvey Hunt wrote:
> From: Alex Smith <alex.smith@imgtec.com>
> 
> Add device tree nodes for the NEMC and BCH to the JZ4780 device tree,
> and make use of them in the Ci20 device tree to add a node for the
> board's NAND.
> 
> Note that since the pinctrl driver is not yet upstream, this includes
> neither pin configuration nor busy/write-protect GPIO pins for the
> NAND. Use of the NAND relies on the boot loader to have left the pins
> configured in a usable state, which should be the case when booted
> from the NAND.
> 
> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: Paul Burton <paul.burton@imgtec.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> Cc: Alex Smith <alex@alex-smith.me.uk>
> Signed-off-by: Harvey Hunt <harvey.hunt@imgtec.com>
> ---
> v6 -> v7:
>  - Add nand-ecc-mode to DT.
>  - Add nand-on-flash-bbt to DT.
> 
> v4 -> v5:
>  - New patch adding DT nodes for the NAND so that the driver can be
>    tested.
> 
>  arch/mips/boot/dts/ingenic/ci20.dts    | 54 ++++++++++++++++++++++++++++++++++
>  arch/mips/boot/dts/ingenic/jz4780.dtsi | 26 ++++++++++++++++
>  2 files changed, 80 insertions(+)
> 
> diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
> index 9fcb9e7..453f1d3 100644
> --- a/arch/mips/boot/dts/ingenic/ci20.dts
> +++ b/arch/mips/boot/dts/ingenic/ci20.dts
> @@ -42,3 +42,57 @@
>  &uart4 {
>  	status = "okay";
>  };
> +
> +&nemc {
> +	status = "okay";
> +
> +	nand: nand@1 {
> +		compatible = "ingenic,jz4780-nand";

Isn't the NAND a micron part? This doesn't seem right. Is the device
driver and binding already accepted upstream with that compatible
string?

Cheers
James

> +		reg = <1 0 0x1000000>;
> +
> +		ingenic,nemc-tAS = <10>;
> +		ingenic,nemc-tAH = <5>;
> +		ingenic,nemc-tBP = <10>;
> +		ingenic,nemc-tAW = <15>;
> +		ingenic,nemc-tSTRV = <100>;
> +
> +		ingenic,bch-controller = <&bch>;
> +		ingenic,ecc-size = <1024>;
> +		ingenic,ecc-strength = <24>;
> +
> +		nand-ecc-mode = "hw";
> +		nand-on-flash-bbt;
> +
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +
> +		partition@0 {
> +			label = "u-boot-spl";
> +			reg = <0x0 0x0 0x0 0x800000>;
> +		};
> +
> +		partition@0x800000 {
> +			label = "u-boot";
> +			reg = <0x0 0x800000 0x0 0x200000>;
> +		};
> +
> +		partition@0xa00000 {
> +			label = "u-boot-env";
> +			reg = <0x0 0xa00000 0x0 0x200000>;
> +		};
> +
> +		partition@0xc00000 {
> +			label = "boot";
> +			reg = <0x0 0xc00000 0x0 0x4000000>;
> +		};
> +
> +		partition@0x8c00000 {
> +			label = "system";
> +			reg = <0x0 0x4c00000 0x1 0xfb400000>;
> +		};
> +	};
> +};
> +
> +&bch {
> +	status = "okay";
> +};
> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> index 65389f6..b868b42 100644
> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> @@ -108,4 +108,30 @@
>  
>  		status = "disabled";
>  	};
> +
> +	nemc: nemc@13410000 {
> +		compatible = "ingenic,jz4780-nemc";
> +		reg = <0x13410000 0x10000>;
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		ranges = <1 0 0x1b000000 0x1000000
> +			  2 0 0x1a000000 0x1000000
> +			  3 0 0x19000000 0x1000000
> +			  4 0 0x18000000 0x1000000
> +			  5 0 0x17000000 0x1000000
> +			  6 0 0x16000000 0x1000000>;
> +
> +		clocks = <&cgu JZ4780_CLK_NEMC>;
> +
> +		status = "disabled";
> +	};
> +
> +	bch: bch@134d0000 {
> +		compatible = "ingenic,jz4780-bch";
> +		reg = <0x134d0000 0x10000>;
> +
> +		clocks = <&cgu JZ4780_CLK_BCH>;
> +
> +		status = "disabled";
> +	};
>  };
> -- 
> 2.6.0
> 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v7,3/3] MIPS: dts: jz4780/ci20: Add NEMC, BCH and NAND device tree nodes

[Alex Smith]

Hi James,

On 15 October 2015 at 09:47, James Hogan <james.hogan@imgtec.com> wrote:
>> diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
>> index 9fcb9e7..453f1d3 100644
>> --- a/arch/mips/boot/dts/ingenic/ci20.dts
>> +++ b/arch/mips/boot/dts/ingenic/ci20.dts
>> @@ -42,3 +42,57 @@
>>  &uart4 {
>>       status = "okay";
>>  };
>> +
>> +&nemc {
>> +     status = "okay";
>> +
>> +     nand: nand@1 {
>> +             compatible = "ingenic,jz4780-nand";
>
> Isn't the NAND a micron part? This doesn't seem right. Is the device
> driver and binding already accepted upstream with that compatible
> string?

This is the compatible string for the JZ4780 NAND driver, this patch
is part of the series adding that. Detection of the NAND part is
handled by the MTD subsystem.

Thanks,
Alex

Re: [PATCH v7,3/3] MIPS: dts: jz4780/ci20: Add NEMC, BCH and NAND device tree nodes

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]

Hi Alex,

On Fri, Oct 16, 2015 at 11:11:29AM +0100, Alex Smith wrote:
> Hi James,
> 
> On 15 October 2015 at 09:47, James Hogan <james.hogan@imgtec.com> wrote:
> >> diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
> >> index 9fcb9e7..453f1d3 100644
> >> --- a/arch/mips/boot/dts/ingenic/ci20.dts
> >> +++ b/arch/mips/boot/dts/ingenic/ci20.dts
> >> @@ -42,3 +42,57 @@
> >>  &uart4 {
> >>       status = "okay";
> >>  };
> >> +
> >> +&nemc {
> >> +     status = "okay";
> >> +
> >> +     nand: nand@1 {
> >> +             compatible = "ingenic,jz4780-nand";
> >
> > Isn't the NAND a micron part? This doesn't seem right. Is the device
> > driver and binding already accepted upstream with that compatible
> > string?
> 
> This is the compatible string for the JZ4780 NAND driver, this patch
> is part of the series adding that. Detection of the NAND part is
> handled by the MTD subsystem.

Right (didn't spot that it was part of a series).

The node appears to describe the NAND interface itself, i.e. a part of
the SoC, so should be in the SoC dtsi file, with overrides in the board
file if necessary for it to work with a particular NAND part
(potentially utilising status="disabled"). Would you agree?

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v7,3/3] MIPS: dts: jz4780/ci20: Add NEMC, BCH and NAND device tree nodes

[Paul Burton]

On Fri, Oct 16, 2015 at 11:31:12AM +0100, James Hogan wrote:
> > >> +
> > >> +&nemc {
> > >> +     status = "okay";
> > >> +
> > >> +     nand: nand@1 {
> > >> +             compatible = "ingenic,jz4780-nand";
> > >
> > > Isn't the NAND a micron part? This doesn't seem right. Is the device
> > > driver and binding already accepted upstream with that compatible
> > > string?
> > 
> > This is the compatible string for the JZ4780 NAND driver, this patch
> > is part of the series adding that. Detection of the NAND part is
> > handled by the MTD subsystem.
> 
> Right (didn't spot that it was part of a series).
> 
> The node appears to describe the NAND interface itself, i.e. a part of
> the SoC, so should be in the SoC dtsi file, with overrides in the board
> file if necessary for it to work with a particular NAND part
> (potentially utilising status="disabled"). Would you agree?

Hi James,

The "nemc" node there is for the Nand & External Memory Controller which
is a hardware block inside the SoC. It has 6 banks (ie. 6 chip select
pins, each associated with a different address range, that connect to
different devices). NAND flash is one such possible device, but a board
could connect it to any of the 6 chip selects, or banks. To represent
that in the SoC dtsi you'd want to have 6 NAND nodes, each disabled by
default, which doesn't make a whole lot of sense to me. Other, non-NAND
devices can connect to the NEMC too - for example the ethernet
controller on the CI20 is connected to one bank.

The NAND device nodes are sort of a mix of describing the NAND flash
(ie. Micron part as you point out) and its connections & properties, the
way the NEMC should be used to interact with it alongside the BCH block,
and the configuration for the NEMC such as timing parameters.

I imagine the most semantically correct means of describing it would
probably be for the compatible string to reflect the Micron NAND part,
and the NEMC driver to pick up on the relevant properties of its child
nodes for configuring timings, whether the device is NAND etc. However
the handling of registering NAND devices with MTD would probably then
have to be part of the NEMC driver, which feels a bit off too.

Thanks,
    Paul

Re: [PATCH v7,3/3] MIPS: dts: jz4780/ci20: Add NEMC, BCH and NAND device tree nodes

[Boris Brezillon]

Paul, Harvey,

On Fri, 16 Oct 2015 11:48:48 +0100
Paul Burton <paul.burton@imgtec.com> wrote:

> On Fri, Oct 16, 2015 at 11:31:12AM +0100, James Hogan wrote:
> > > >> +
> > > >> +&nemc {
> > > >> +     status = "okay";
> > > >> +
> > > >> +     nand: nand@1 {
> > > >> +             compatible = "ingenic,jz4780-nand";
> > > >
> > > > Isn't the NAND a micron part? This doesn't seem right. Is the device
> > > > driver and binding already accepted upstream with that compatible
> > > > string?
> > > 
> > > This is the compatible string for the JZ4780 NAND driver, this patch
> > > is part of the series adding that. Detection of the NAND part is
> > > handled by the MTD subsystem.
> > 
> > Right (didn't spot that it was part of a series).
> > 
> > The node appears to describe the NAND interface itself, i.e. a part of
> > the SoC, so should be in the SoC dtsi file, with overrides in the board
> > file if necessary for it to work with a particular NAND part
> > (potentially utilising status="disabled"). Would you agree?
> 
> Hi James,
> 
> The "nemc" node there is for the Nand & External Memory Controller which
> is a hardware block inside the SoC. It has 6 banks (ie. 6 chip select
> pins, each associated with a different address range, that connect to
> different devices). NAND flash is one such possible device, but a board
> could connect it to any of the 6 chip selects, or banks. To represent
> that in the SoC dtsi you'd want to have 6 NAND nodes, each disabled by
> default, which doesn't make a whole lot of sense to me. Other, non-NAND
> devices can connect to the NEMC too - for example the ethernet
> controller on the CI20 is connected to one bank.
> 
> The NAND device nodes are sort of a mix of describing the NAND flash
> (ie. Micron part as you point out) and its connections & properties, the
> way the NEMC should be used to interact with it alongside the BCH block,
> and the configuration for the NEMC such as timing parameters.
> 
> I imagine the most semantically correct means of describing it would
> probably be for the compatible string to reflect the Micron NAND part,
> and the NEMC driver to pick up on the relevant properties of its child
> nodes for configuring timings, whether the device is NAND etc. However
> the handling of registering NAND devices with MTD would probably then
> have to be part of the NEMC driver, which feels a bit off too.

Another solution would be to describe both the NAND controller and the
NAND chip in the DT (with the NAND chip being a chip of the NAND
controller).
Actually this is already what other binding are doing [1][2]. I know
those bindings are representing NAND controllers which can interface
with more than one NAND chip, but I think that even in the 1:1 case it
would make it clearer to represent both the NAND chip and the NAND
controller.

In your case this would give the following representation

+&nemc {
+	status = "okay";
+
+	nandc: nand-controller@1 {
+		compatible = "ingenic,jz4780-nand";
+		reg = <1 0 0x1000000>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		ingenic,bch-controller = <&bch>;
+
+		nand@0 {
+			nand-ecc-mode = "hw";
+			nand-on-flash-bbt;
+			nand-ecc-size = <1024>;
+			nand-ecc-strength = <24>;
+
+			#address-cells = <2>;
+			#size-cells = <2>;
+
+			partition@0 {
+				label = "u-boot-spl";
+				reg = <0x0 0x0 0x0 0x800000>;
+			};
+			/* ... */
+
+		};
+	};
+};

Best Regards,

Boris

[1]http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt#L119
[2]http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/sunxi-nand.txt#L28

> 
> Thanks,
>     Paul
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v7,1/3] dt-bindings: binding for jz4780-{nand,bch}

[Boris Brezillon]

Hi Harvey,

On Tue, 6 Oct 2015 17:27:15 +0100
Harvey Hunt <harvey.hunt@imgtec.com> wrote:

> From: Alex Smith <alex.smith@imgtec.com>
> 
> Add DT bindings for NAND devices connected to the NEMC on JZ4780 SoCs,
> as well as the hardware BCH controller, used by the jz4780_{nand,bch}
> drivers.

Patch 3 does not seem to follow this binding: it still uses the
ingenic,ecc-xxx properties and defines NAND timings.

Also, as answered to patch 3, I think it would be clearer to separate
the nand controller and nand chip representation.

Best Regards,

Boris

> 
> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Alex Smith <alex@alex-smith.me.uk>
> Signed-off-by: Harvey Hunt <harvey.hunt@imgtec.com>
> ---
> v6 -> v7:
>  - Add nand-ecc-mode to DT bindings.
>  - Add nand-on-flash-bbt to DT bindings.
> 
> v5 -> v6:
>  - No change.
> 
> v4 -> v5:
>  - Rename ingenic,bch-device to ingenic,bch-controller to fit with
>    existing convention.
> 
> v3 -> v4:
>  - No change
> 
> v2 -> v3:
>  - Rebase to 4.0-rc6
>  - Changed ingenic,ecc-size to common nand-ecc-step-size
>  - Changed ingenic,ecc-strength to common nand-ecc-strength
>  - Changed ingenic,busy-gpio to common rb-gpios
>  - Changed ingenic,wp-gpio to common wp-gpios
> 
> v1 -> v2:
>  - Rebase to 4.0-rc3
> 
>  .../bindings/mtd/ingenic,jz4780-nand.txt           | 61 ++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
> new file mode 100644
> index 0000000..44a0468
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
> @@ -0,0 +1,61 @@
> +* Ingenic JZ4780 NAND/BCH
> +
> +This file documents the device tree bindings for NAND flash devices on the
> +JZ4780. NAND devices are connected to the NEMC controller (described in
> +memory-controllers/ingenic,jz4780-nemc.txt), and thus NAND device nodes must
> +be children of the NEMC node.
> +
> +Required NAND device properties:
> +- compatible: Should be set to "ingenic,jz4780-nand".
> +- reg: For each bank with a NAND chip attached, should specify a bank number,
> +  an offset of 0 and a size of 0x1000000 (i.e. the whole NEMC bank).
> +
> +Optional NAND device properties:
> +- ingenic,bch-controller: To make use of the hardware BCH controller, this
> +  property must contain a phandle for the BCH controller node. The required
> +  properties for this node are described below. If this is not specified,
> +  software BCH will be used instead.
> +- nand-ecc-step-size: ECC block size in bytes.
> +- nand-ecc-strength: ECC strength (max number of correctable bits).
> +- nand-ecc-mode: String, operation mode of the NAND ecc mode. "hw" by default
> +- nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
> +- rb-gpios: GPIO specifier for the busy pin.
> +- wp-gpios: GPIO specifier for the write protect pin.
> +
> +Example:
> +
> +nemc: nemc@13410000 {
> +	...
> +
> +	nand: nand@1 {
> +		compatible = "ingenic,jz4780-nand";
> +		reg = <1 0 0x1000000>;	/* Bank 1 */
> +
> +		ingenic,bch-controller = <&bch>;
> +		nand-ecc-step-size = <1024>;
> +		nand-ecc-strength = <24>;
> +		nand-ecc-mode = "hw";
> +		nand-on-flash-bbt;
> +
> +		rb-gpios = <&gpa 20 GPIO_ACTIVE_LOW>;
> +		wp-gpios = <&gpf 22 GPIO_ACTIVE_LOW>;
> +	};
> +};
> +
> +The BCH controller is a separate SoC component used for error correction on
> +NAND devices. The following is a description of the device properties for a
> +BCH controller.
> +
> +Required BCH properties:
> +- compatible: Should be set to "ingenic,jz4780-bch".
> +- reg: Should specify the BCH controller registers location and length.
> +- clocks: Clock for the BCH controller.
> +
> +Example:
> +
> +bch: bch@134d0000 {
> +	compatible = "ingenic,jz4780-bch";
> +	reg = <0x134d0000 0x10000>;
> +
> +	clocks = <&cgu JZ4780_CLK_BCH>;
> +};



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v7, 2/3] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs

[Boris Brezillon]

Hi Harvey,

Sorry for the late review :-/.

On Tue, 6 Oct 2015 17:27:16 +0100
Harvey Hunt <harvey.hunt@imgtec.com> wrote:

> From: Alex Smith <alex.smith@imgtec.com>
> 
> Add a driver for NAND devices connected to the NEMC on JZ4780 SoCs, as
> well as the hardware BCH controller. DMA is not currently implemented.
> 
> While older 47xx SoCs also have a BCH controller, they are incompatible
> with the one in the 4780 due to differing register/bit positions, which
> would make implementing a common driver for them quite messy.
> 
> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Cc: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Alex Smith <alex@alex-smith.me.uk>
> Signed-off-by: Harvey Hunt <harvey.hunt@imgtec.com>
> ---
> v6 -> v7:
>  - Include linux/bitops.h.
>  - Default ECC mode to hw.
>  - Only check BCH if we're using hw ECC mode.
>  - Return -ENODEV if we're using hw ECC mode and can't find BCH.
>  - Return -ENODEV if we can't find any banks.
>  - Use set nand_chip->dn so we can use nand_dt_init() to read DT.
> 
> v5 -> v6:
>  - Remove another printk made useless by changes in v5 (due to
>    devm_ioremap_resource() printing errors itself).
> 
> v4 -> v5:
>  - Bump RB_DELAY up to be sufficient for the driver to work without a
>    busy GPIO available.
>  - Use readl_poll_timeout instead of custom polling loop.
>  - Remove useless printks.
>  - Change a BUG_ON to WARN_ON.
>  - Remove use of of_translate_address(), use standard platform resource
>    APIs.
>  - Add DRV_NAME define to avoid duplication of the same string.
> 
> v3 -> v4:
>  - Rebase to 4.2-rc4
>  - Change ECC_HW_OOB_FIRST to ECC_HW, reading OOB first is not necessary.
>  - Fix argument to get_device() in jz4780_bch_get()
> 
> v2 -> v3:
>  - Rebase to 4.0-rc6
>  - Reflect binding changes
>  - get/put_device in bch get/release
>  - Removed empty .remove() callback
>  - Removed .owner
>  - Set mtd->dev.parent
> 
> v1 -> v2:
>  - Fixed module license macro
>  - Rebase to 4.0-rc3
> 
>  drivers/mtd/nand/Kconfig       |   7 +
>  drivers/mtd/nand/Makefile      |   1 +
>  drivers/mtd/nand/jz4780_bch.c  | 349 +++++++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/jz4780_bch.h  |  42 +++++
>  drivers/mtd/nand/jz4780_nand.c | 384 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 783 insertions(+)
>  create mode 100644 drivers/mtd/nand/jz4780_bch.c
>  create mode 100644 drivers/mtd/nand/jz4780_bch.h
>  create mode 100644 drivers/mtd/nand/jz4780_nand.c
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 3324281..8ffd5cd 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -508,6 +508,13 @@ config MTD_NAND_JZ4740
>  	help
>  		Enables support for NAND Flash on JZ4740 SoC based boards.
>  
> +config MTD_NAND_JZ4780
> +	tristate "Support for NAND on JZ4780 SoC"
> +	depends on MACH_JZ4780 && JZ4780_NEMC
> +	help
> +	  Enables support for NAND Flash connected to the NEMC on JZ4780 SoC
> +	  based boards, using the BCH controller for hardware error correction.
> +
>  config MTD_NAND_FSMC
>  	tristate "Support for NAND on ST Micros FSMC"
>  	depends on PLAT_SPEAR || ARCH_NOMADIK || ARCH_U8500 || MACH_U300
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 075a027..604b166 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_MTD_NAND_NUC900)		+= nuc900_nand.o
>  obj-$(CONFIG_MTD_NAND_MPC5121_NFC)	+= mpc5121_nfc.o
>  obj-$(CONFIG_MTD_NAND_RICOH)		+= r852.o
>  obj-$(CONFIG_MTD_NAND_JZ4740)		+= jz4740_nand.o
> +obj-$(CONFIG_MTD_NAND_JZ4780)		+= jz4780_nand.o jz4780_bch.o
>  obj-$(CONFIG_MTD_NAND_GPMI_NAND)	+= gpmi-nand/
>  obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
>  obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
> diff --git a/drivers/mtd/nand/jz4780_bch.c b/drivers/mtd/nand/jz4780_bch.c
> new file mode 100644
> index 0000000..047d351
> --- /dev/null
> +++ b/drivers/mtd/nand/jz4780_bch.c
> @@ -0,0 +1,349 @@
> +/*
> + * JZ4780 BCH controller
> + *
> + * Copyright (c) 2015 Imagination Technologies
> + * Author: Alex Smith <alex.smith@imgtec.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +
> +#include "jz4780_bch.h"
> +
> +#define BCH_BHCR			0x0
> +#define BCH_BHCCR			0x8
> +#define BCH_BHCNT			0xc
> +#define BCH_BHDR			0x10
> +#define BCH_BHPAR0			0x14
> +#define BCH_BHERR0			0x84
> +#define BCH_BHINT			0x184
> +#define BCH_BHINTES			0x188
> +#define BCH_BHINTEC			0x18c
> +#define BCH_BHINTE			0x190
> +
> +#define BCH_BHCR_BSEL_SHIFT		4
> +#define BCH_BHCR_BSEL_MASK		(0x7f << BCH_BHCR_BSEL_SHIFT)

You can use GENMASK for these things.

> +#define BCH_BHCR_ENCE			BIT(2)
> +#define BCH_BHCR_INIT			BIT(1)
> +#define BCH_BHCR_BCHE			BIT(0)
> +
> +#define BCH_BHCNT_PARITYSIZE_SHIFT	16
> +#define BCH_BHCNT_PARITYSIZE_MASK	(0x7f << BCH_BHCNT_PARITYSIZE_SHIFT)
> +#define BCH_BHCNT_BLOCKSIZE_SHIFT	0
> +#define BCH_BHCNT_BLOCKSIZE_MASK	(0x7ff << BCH_BHCNT_BLOCKSIZE_SHIFT)
> +
> +#define BCH_BHERR_MASK_SHIFT		16
> +#define BCH_BHERR_MASK_MASK		(0xffff << BCH_BHERR_MASK_SHIFT)
> +#define BCH_BHERR_INDEX_SHIFT		0
> +#define BCH_BHERR_INDEX_MASK		(0x7ff << BCH_BHERR_INDEX_SHIFT)
> +
> +#define BCH_BHINT_ERRC_SHIFT		24
> +#define BCH_BHINT_ERRC_MASK		(0x7f << BCH_BHINT_ERRC_SHIFT)
> +#define BCH_BHINT_TERRC_SHIFT		16
> +#define BCH_BHINT_TERRC_MASK		(0x7f << BCH_BHINT_TERRC_SHIFT)
> +#define BCH_BHINT_DECF			BIT(3)
> +#define BCH_BHINT_ENCF			BIT(2)
> +#define BCH_BHINT_UNCOR			BIT(1)
> +#define BCH_BHINT_ERR			BIT(0)
> +
> +#define BCH_CLK_RATE			(200 * 1000 * 1000)
> +
> +/* Timeout for BCH calculation/correction in microseconds. */
> +#define BCH_TIMEOUT			100000

Suffixing the macro name with _MS would make it clearer.

> +
> +struct jz4780_bch {
> +	void __iomem *base;
> +	struct clk *clk;
> +};

You seem to rely on the sequencing mechanism provided by the NAND
controller struct to sequence accesses to the BCH controller.
That's fine as long as the jz4780 NAND controller is the only user of
this BCH engine, which is no longer true as soon as you define two nand
chips in your system (each of these chip is exposing its own
nand_controller, and you end up with a concurrency problem).

You have two different solutions to address that:
1/ provide a sequencing mechanism at the BCH engine level
2/ rework the NAND controller driver architecture to expose a single
NAND controller (the NAND controller can attach to several CS of the
nemc bus), and then define NAND chips as children nodes of the NAND
controller node.

Solution #1 is more future-proof, and allows you to use the BCH engine
for other purpose than just NAND controller.
Solution #2 has the benefit of making the NAND controller / NAND chip
separation clearer, which is a good thing too.

So I would recommend doing both :-).

[...]
> diff --git a/drivers/mtd/nand/jz4780_bch.h b/drivers/mtd/nand/jz4780_bch.h
> new file mode 100644
> index 0000000..a5dfde5
> --- /dev/null
> +++ b/drivers/mtd/nand/jz4780_bch.h
> @@ -0,0 +1,42 @@
> +/*
> + * JZ4780 BCH controller
> + *
> + * Copyright (c) 2015 Imagination Technologies
> + * Author: Alex Smith <alex.smith@imgtec.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#ifndef __DRIVERS_MTD_NAND_JZ4780_BCH_H__
> +#define __DRIVERS_MTD_NAND_JZ4780_BCH_H__
> +
> +#include <linux/types.h>
> +
> +struct device;
> +struct device_node;
> +
> +/**
> + * struct jz4780_bch_params - BCH parameters
> + * @size: data bytes per ECC step.
> + * @bytes: ECC bytes per step.
> + * @strength: number of correctable bits per ECC step.
> + */
> +struct jz4780_bch_params {
> +	int size;
> +	int bytes;
> +	int strength;
> +};
> +
> +extern int jz4780_bch_calculate(struct device *dev,
> +				struct jz4780_bch_params *params,
> +				const uint8_t *buf, uint8_t *ecc_code);
> +extern int jz4780_bch_correct(struct device *dev,
> +			      struct jz4780_bch_params *params, uint8_t *buf,
> +			      uint8_t *ecc_code);
> +
> +extern int jz4780_bch_get(struct device_node *np, struct device **dev);
> +extern void jz4780_bch_release(struct device *dev);
> +
> +#endif /* __DRIVERS_MTD_NAND_JZ4780_BCH_H__ */
> diff --git a/drivers/mtd/nand/jz4780_nand.c b/drivers/mtd/nand/jz4780_nand.c
> new file mode 100644
> index 0000000..4100ddc9
> --- /dev/null
> +++ b/drivers/mtd/nand/jz4780_nand.c
> @@ -0,0 +1,384 @@
> +/*
> + * JZ4780 NAND driver
> + *
> + * Copyright (c) 2015 Imagination Technologies
> + * Author: Alex Smith <alex.smith@imgtec.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_mtd.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mtd/partitions.h>
> +
> +#include <linux/jz4780-nemc.h>
> +
> +#include "jz4780_bch.h"
> +
> +#define DRV_NAME	"jz4780-nand"
> +
> +#define OFFSET_DATA	0x00000000
> +#define OFFSET_CMD	0x00400000
> +#define OFFSET_ADDR	0x00800000
> +
> +/* Command delay when there is no R/B pin (in microseconds). */
> +#define RB_DELAY	100

_US suffix?

> +
> +struct jz4780_nand_chip {
> +	unsigned int bank;
> +	void __iomem *base;
> +};
> +
> +struct jz4780_nand {
> +	struct mtd_info mtd;
> +	struct nand_chip chip;
> +
> +	struct device *dev;
> +	struct device *bch;
> +
> +	struct nand_ecclayout ecclayout;
> +
> +	int busy_gpio;
> +	int wp_gpio;

Any reason why you're not using gpio_desc here instead of integer ids?

> +	unsigned int busy_gpio_active_low: 1;
> +	unsigned int wp_gpio_active_low: 1;
> +	unsigned int reading: 1;
> +
> +	unsigned int num_banks;
> +	int selected;
> +	struct jz4780_nand_chip chips[];
> +};

As explained earlier, I would prefer to see a clean separation between
the nand_chip and the nand_controller definition. Something like:

struct jz4780_nand_cs {
	unsigned int bank;
	void __iomem *base;
};

struct jz4780_nand_controller {
	struct nand_hw_control controller;
	struct device *dev;
	struct device *bch;
	unsigned int num_banks;
	struct jz4780_nand_cs cs[];
}

static inline to_jz4780_nand_controller(struct nand_hw_control *ctrl)
{
	return container_of(ctrl, struct jz4780_nand_controller,
			    controller);
}

struct jz4780_nand {
	struct mtd_info mtd;
	struct nand_chip chip;

	struct nand_ecclayout ecclayout;

	/*
	 * Number of CS lines attached to this chip. Each CS line
	 * controls a specific die in the chip.
	*/
	unsigned int num_cs;
	/* Should point the definitions in struct nand_controller */
	struct jz4780_nand_chip **cs;

	/* RB pins definition, there can be one RB pin per die. */
	int *busy_gpios;

	/* RB pins definition, there can be one RB pin per die. */
	int *wp_gpios;

	unsigned int busy_gpio_active_low: 1;
	unsigned int wp_gpio_active_low: 1;
	unsigned int reading: 1;
};

Note that the jz4780_nand_controller pointer can be retrieved from the
nand_chip struct by doing the following:

	to_jz4780_nand_controller(chip->controller);

> +
> +static inline struct jz4780_nand *to_jz4780_nand(struct mtd_info *mtd)
> +{
> +	return container_of(mtd, struct jz4780_nand, mtd);
> +}
> +
> +static void jz4780_nand_select_chip(struct mtd_info *mtd, int chipnr)
> +{
> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> +	struct jz4780_nand_chip *chip;
> +
> +	if (chipnr == -1) {
> +		/* Ensure the currently selected chip is deasserted. */
> +		if (nand->selected >= 0) {
> +			chip = &nand->chips[nand->selected];
> +			jz4780_nemc_assert(nand->dev, chip->bank, false);
> +		}
> +	} else {
> +		chip = &nand->chips[chipnr];
> +		nand->chip.IO_ADDR_R = chip->base + OFFSET_DATA;
> +		nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;

How about providing helper functions that would use the nand->selected
+ chips information to access the correct set of registers instead of
adapting IO_ADDR_R/IO_ADDR_W values?

> +	}
> +
> +	nand->selected = chipnr;
> +}
> +
> +static void jz4780_nand_cmd_ctrl(struct mtd_info *mtd, int cmd,
> +				 unsigned int ctrl)
> +{
> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> +	struct jz4780_nand_chip *chip;
> +
> +	if (WARN_ON(nand->selected < 0))
> +		return;
> +
> +	chip = &nand->chips[nand->selected];
> +
> +	if (ctrl & NAND_CTRL_CHANGE) {
> +		if (ctrl & NAND_ALE)
> +			nand->chip.IO_ADDR_W = chip->base + OFFSET_ADDR;
> +		else if (ctrl & NAND_CLE)
> +			nand->chip.IO_ADDR_W = chip->base + OFFSET_CMD;
> +		else
> +			nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;

Ditto.

> +
> +		jz4780_nemc_assert(nand->dev, chip->bank, ctrl & NAND_NCE);
> +	}
> +
> +	if (cmd != NAND_CMD_NONE)
> +		writeb(cmd, nand->chip.IO_ADDR_W);
> +}
> +
> +static int jz4780_nand_dev_ready(struct mtd_info *mtd)
> +{
> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> +
> +	return !(gpio_get_value(nand->busy_gpio) ^ nand->busy_gpio_active_low);
> +}
> +
> +static void jz4780_nand_ecc_hwctl(struct mtd_info *mtd, int mode)
> +{
> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> +
> +	nand->reading = (mode == NAND_ECC_READ);
> +}
> +
> +static int jz4780_nand_ecc_calculate(struct mtd_info *mtd, const uint8_t *dat,
> +				     uint8_t *ecc_code)
> +{
> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> +	struct jz4780_bch_params params;
> +
> +	/*
> +	 * Don't need to generate the ECC when reading, BCH does it for us as
> +	 * part of decoding/correction.
> +	 */
> +	if (nand->reading)
> +		return 0;
> +
> +	params.size = nand->chip.ecc.size;
> +	params.bytes = nand->chip.ecc.bytes;
> +	params.strength = nand->chip.ecc.strength;
> +
> +	return jz4780_bch_calculate(nand->bch, &params, dat, ecc_code);
> +}
> +
> +static int jz4780_nand_ecc_correct(struct mtd_info *mtd, uint8_t *dat,
> +				   uint8_t *read_ecc, uint8_t *calc_ecc)
> +{
> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> +	struct jz4780_bch_params params;
> +
> +	params.size = nand->chip.ecc.size;
> +	params.bytes = nand->chip.ecc.bytes;
> +	params.strength = nand->chip.ecc.strength;
> +
> +	return jz4780_bch_correct(nand->bch, &params, dat, read_ecc);
> +}
> +
> +static int jz4780_nand_init_ecc(struct jz4780_nand *nand, struct device *dev)
> +{
> +	struct mtd_info *mtd = &nand->mtd;
> +	struct nand_chip *chip = &nand->chip;
> +	struct nand_ecclayout *layout = &nand->ecclayout;
> +	struct device_node *bch_np;
> +	int ret = 0;
> +	uint32_t start, i;
> +
> +	if (!chip->ecc.size)
> +		chip->ecc.size = 1024;
> +
> +	if (!chip->ecc.strength)
> +		chip->ecc.strength = 24;

Is there a strong reason to define a random default ECC config. I mean,
if this is not provided then there is a problem somewhere, or the NAND
does not require ECC, in which case mode should be set to NAND_ECC_NONE.

At least, if you want to automatically choose an ECC config when it's
not provided, choose it according to the NAND layout (it has to fit into
the OOB area).

> +
> +	chip->ecc.bytes = fls(1 + 8 * chip->ecc.size) * chip->ecc.strength / 8;
> +
> +	dev_info(dev, "using %s BCH (strength %d, size %d, bytes %d)\n",
> +		 (nand->bch) ? "hardware" : "software", chip->ecc.strength,
> +		 chip->ecc.size, chip->ecc.bytes);
> +
> +	if (chip->ecc.mode == NAND_ECC_HW) {
> +		bch_np = of_parse_phandle(dev->of_node,
> +					"ingenic,bch-controller", 0);
> +		if (bch_np) {
> +			ret = jz4780_bch_get(bch_np, &nand->bch);
> +			of_node_put(bch_np);
> +			if (ret)
> +				return ret;
> +		} else {
> +			dev_err(dev, "no bch controller in DT\n");
> +			return -ENODEV;
> +		}
> +
> +		chip->ecc.hwctl = jz4780_nand_ecc_hwctl;
> +		chip->ecc.calculate = jz4780_nand_ecc_calculate;
> +		chip->ecc.correct = jz4780_nand_ecc_correct;

You should probably check that
(ecc->bytes * (mtd->writesize / ecc->size)) <= mtd->oobsize - 2
and fail if it's not the case.

> +	}
> +
> +	/* Generate ECC layout. ECC codes are right aligned in the OOB area. */
> +	layout->eccbytes = mtd->writesize / chip->ecc.size * chip->ecc.bytes;
> +	start = mtd->oobsize - layout->eccbytes;
> +	for (i = 0; i < layout->eccbytes; i++)
> +		layout->eccpos[i] = start + i;
> +
> +	layout->oobfree[0].offset = 2;
> +	layout->oobfree[0].length = mtd->oobsize - layout->eccbytes - 2;

Not sure you want to fill that if mode == NAND_ECC_SOFT of
NAND_ECC_SOFT_BCH, because those fields are automatically create/filled
for you.

> +
> +	chip->ecc.layout = layout;
> +
> +	return 0;
> +}
> +
> +static int jz4780_nand_init_chips(struct jz4780_nand *nand,
> +				  struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct jz4780_nand_chip *chip;
> +	const __be32 *prop;
> +	struct resource *res;
> +	int i = 0;
> +
> +	/*
> +	 * Iterate over each bank assigned to this device and request resources.
> +	 * The bank numbers may not be consecutive, but nand_scan_ident()
> +	 * expects chip numbers to be, so fill out a consecutive array of chips
> +	 * which map chip number to actual bank number.
> +	 */
> +	while ((prop = of_get_address(dev->of_node, i, NULL, NULL))) {
> +		chip = &nand->chips[i];
> +		chip->bank = of_read_number(prop, 1);
> +
> +		jz4780_nemc_set_type(nand->dev, chip->bank,
> +				     JZ4780_NEMC_BANK_NAND);
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		chip->base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(chip->base))
> +			return PTR_ERR(chip->base);
> +
> +		i++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int jz4780_nand_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	unsigned int num_banks;
> +	struct jz4780_nand *nand;
> +	struct mtd_info *mtd;
> +	struct nand_chip *chip;
> +	enum of_gpio_flags flags;
> +	struct mtd_part_parser_data ppdata;
> +	int ret;
> +
> +	num_banks = jz4780_nemc_num_banks(dev);
> +	if (num_banks == 0) {
> +		dev_err(dev, "no banks found\n");
> +		return -ENODEV;
> +	}
> +
> +	nand = devm_kzalloc(dev,
> +			sizeof(*nand) + (sizeof(nand->chips[0]) * num_banks),
> +			GFP_KERNEL);
> +	if (!nand)
> +		return -ENOMEM;
> +
> +	nand->dev = dev;
> +	nand->num_banks = num_banks;
> +	nand->selected = -1;
> +
> +	mtd = &nand->mtd;
> +	chip = &nand->chip;
> +	mtd->priv = chip;
> +	mtd->owner = THIS_MODULE;
> +	mtd->name = DRV_NAME;
> +	mtd->dev.parent = dev;
> +
> +	chip->dn = dev->of_node;

chip->dn has recently been renamed to chip->flash_node.

BTW, If you go for the nand_controller + nand_chip approach, you'll have
to initialize the nand_controller struct and link it with your
nand_chip by doing:

	spin_lock_init(&jz4780_ctrl->controller.lock);
	init_waitqueue_head(&jz4780_ctrl->controller.wq);
	chip->controller = &jz4780_ctrl->controller;

That's all I see for now.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v7,1/3] dt-bindings: binding for jz4780-{nand,bch}

[Harvey Hunt]

Hi Boris,

On 04/11/15 07:57, Boris Brezillon wrote:
> Hi Harvey,
>
> On Tue, 6 Oct 2015 17:27:15 +0100
> Harvey Hunt <harvey.hunt@imgtec.com> wrote:
>
>> From: Alex Smith <alex.smith@imgtec.com>
>>
>> Add DT bindings for NAND devices connected to the NEMC on JZ4780 SoCs,
>> as well as the hardware BCH controller, used by the jz4780_{nand,bch}
>> drivers.
>
> Patch 3 does not seem to follow this binding: it still uses the
> ingenic,ecc-xxx properties and defines NAND timings.

I'll fix this for v8.

> Also, as answered to patch 3, I think it would be clearer to separate
> the nand controller and nand chip representation.

I agree - I'll implement this in the next version.

> Best Regards,
>
> Boris

Thanks,

Harvey Hunt

Re: [PATCH v7,1/3] dt-bindings: binding for jz4780-{nand,bch}

[Harvey Hunt]

Hi Boris,

On 04/11/15 07:57, Boris Brezillon wrote:
> Hi Harvey,
>
> On Tue, 6 Oct 2015 17:27:15 +0100
> Harvey Hunt <harvey.hunt@imgtec.com> wrote:
>
>> From: Alex Smith <alex.smith@imgtec.com>
>>
>> Add DT bindings for NAND devices connected to the NEMC on JZ4780 SoCs,
>> as well as the hardware BCH controller, used by the jz4780_{nand,bch}
>> drivers.
>
> Patch 3 does not seem to follow this binding: it still uses the
> ingenic,ecc-xxx properties and defines NAND timings.

Thanks, I'll fix that for v8.

> Also, as answered to patch 3, I think it would be clearer to separate
> the nand controller and nand chip representation.

That approach makes sense - I'll change it for the next series.

> Best Regards,
>
> Boris

Best regards,

Harvey Hunt

Re: [PATCH v7, 2/3] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs

[Harvey Hunt]

Hi Boris,

On 04/11/15 10:18, Boris Brezillon wrote:
> Hi Harvey,
>
> Sorry for the late review :-/.

Not a problem. :-)


>> +#define BCH_BHCR_BSEL_SHIFT		4
>> +#define BCH_BHCR_BSEL_MASK		(0x7f << BCH_BHCR_BSEL_SHIFT)
>
> You can use GENMASK for these things.

I'll change that.

>> +#define BCH_BHCR_ENCE			BIT(2)
>> +#define BCH_BHCR_INIT			BIT(1)
>> +#define BCH_BHCR_BCHE			BIT(0)
>> +
>> +#define BCH_BHCNT_PARITYSIZE_SHIFT	16
>> +#define BCH_BHCNT_PARITYSIZE_MASK	(0x7f << BCH_BHCNT_PARITYSIZE_SHIFT)
>> +#define BCH_BHCNT_BLOCKSIZE_SHIFT	0
>> +#define BCH_BHCNT_BLOCKSIZE_MASK	(0x7ff << BCH_BHCNT_BLOCKSIZE_SHIFT)
>> +
>> +#define BCH_BHERR_MASK_SHIFT		16
>> +#define BCH_BHERR_MASK_MASK		(0xffff << BCH_BHERR_MASK_SHIFT)
>> +#define BCH_BHERR_INDEX_SHIFT		0
>> +#define BCH_BHERR_INDEX_MASK		(0x7ff << BCH_BHERR_INDEX_SHIFT)
>> +
>> +#define BCH_BHINT_ERRC_SHIFT		24
>> +#define BCH_BHINT_ERRC_MASK		(0x7f << BCH_BHINT_ERRC_SHIFT)
>> +#define BCH_BHINT_TERRC_SHIFT		16
>> +#define BCH_BHINT_TERRC_MASK		(0x7f << BCH_BHINT_TERRC_SHIFT)
>> +#define BCH_BHINT_DECF			BIT(3)
>> +#define BCH_BHINT_ENCF			BIT(2)
>> +#define BCH_BHINT_UNCOR			BIT(1)
>> +#define BCH_BHINT_ERR			BIT(0)
>> +
>> +#define BCH_CLK_RATE			(200 * 1000 * 1000)
>> +
>> +/* Timeout for BCH calculation/correction in microseconds. */
>> +#define BCH_TIMEOUT			100000
>
> Suffixing the macro name with _MS would make it clearer.

Do you mean _US?

>> +
>> +struct jz4780_bch {
>> +	void __iomem *base;
>> +	struct clk *clk;
>> +};
>
> You seem to rely on the sequencing mechanism provided by the NAND
> controller struct to sequence accesses to the BCH controller.
> That's fine as long as the jz4780 NAND controller is the only user of
> this BCH engine, which is no longer true as soon as you define two nand
> chips in your system (each of these chip is exposing its own
> nand_controller, and you end up with a concurrency problem).
>
> You have two different solutions to address that:
> 1/ provide a sequencing mechanism at the BCH engine level
> 2/ rework the NAND controller driver architecture to expose a single
> NAND controller (the NAND controller can attach to several CS of the
> nemc bus), and then define NAND chips as children nodes of the NAND
> controller node.
>
> Solution #1 is more future-proof, and allows you to use the BCH engine
> for other purpose than just NAND controller.
> Solution #2 has the benefit of making the NAND controller / NAND chip
> separation clearer, which is a good thing too.
>
> So I would recommend doing both :-).

I'll do both.

> [...]
>> diff --git a/drivers/mtd/nand/jz4780_bch.h b/drivers/mtd/nand/jz4780_bch.h
>> new file mode 100644
>> index 0000000..a5dfde5
>> --- /dev/null
>> +++ b/drivers/mtd/nand/jz4780_bch.h
>> @@ -0,0 +1,42 @@
>> +/*
>> + * JZ4780 BCH controller
>> + *
>> + * Copyright (c) 2015 Imagination Technologies
>> + * Author: Alex Smith <alex.smith@imgtec.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> + * by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __DRIVERS_MTD_NAND_JZ4780_BCH_H__
>> +#define __DRIVERS_MTD_NAND_JZ4780_BCH_H__
>> +
>> +#include <linux/types.h>
>> +
>> +struct device;
>> +struct device_node;
>> +
>> +/**
>> + * struct jz4780_bch_params - BCH parameters
>> + * @size: data bytes per ECC step.
>> + * @bytes: ECC bytes per step.
>> + * @strength: number of correctable bits per ECC step.
>> + */
>> +struct jz4780_bch_params {
>> +	int size;
>> +	int bytes;
>> +	int strength;
>> +};
>> +
>> +extern int jz4780_bch_calculate(struct device *dev,
>> +				struct jz4780_bch_params *params,
>> +				const uint8_t *buf, uint8_t *ecc_code);
>> +extern int jz4780_bch_correct(struct device *dev,
>> +			      struct jz4780_bch_params *params, uint8_t *buf,
>> +			      uint8_t *ecc_code);
>> +
>> +extern int jz4780_bch_get(struct device_node *np, struct device **dev);
>> +extern void jz4780_bch_release(struct device *dev);
>> +
>> +#endif /* __DRIVERS_MTD_NAND_JZ4780_BCH_H__ */
>> diff --git a/drivers/mtd/nand/jz4780_nand.c b/drivers/mtd/nand/jz4780_nand.c
>> new file mode 100644
>> index 0000000..4100ddc9
>> --- /dev/null
>> +++ b/drivers/mtd/nand/jz4780_nand.c
>> @@ -0,0 +1,384 @@
>> +/*
>> + * JZ4780 NAND driver
>> + *
>> + * Copyright (c) 2015 Imagination Technologies
>> + * Author: Alex Smith <alex.smith@imgtec.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> + * by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_mtd.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/nand.h>
>> +#include <linux/mtd/partitions.h>
>> +
>> +#include <linux/jz4780-nemc.h>
>> +
>> +#include "jz4780_bch.h"
>> +
>> +#define DRV_NAME	"jz4780-nand"
>> +
>> +#define OFFSET_DATA	0x00000000
>> +#define OFFSET_CMD	0x00400000
>> +#define OFFSET_ADDR	0x00800000
>> +
>> +/* Command delay when there is no R/B pin (in microseconds). */
>> +#define RB_DELAY	100
>
> _US suffix?

I'll add that.

>
>> +
>> +struct jz4780_nand_chip {
>> +	unsigned int bank;
>> +	void __iomem *base;
>> +};
>> +
>> +struct jz4780_nand {
>> +	struct mtd_info mtd;
>> +	struct nand_chip chip;
>> +
>> +	struct device *dev;
>> +	struct device *bch;
>> +
>> +	struct nand_ecclayout ecclayout;
>> +
>> +	int busy_gpio;
>> +	int wp_gpio;
>
> Any reason why you're not using gpio_desc here instead of integer ids?

No reason - I'll fix it.

>> +	unsigned int busy_gpio_active_low: 1;
>> +	unsigned int wp_gpio_active_low: 1;
>> +	unsigned int reading: 1;
>> +
>> +	unsigned int num_banks;
>> +	int selected;
>> +	struct jz4780_nand_chip chips[];
>> +};
>
> As explained earlier, I would prefer to see a clean separation between
> the nand_chip and the nand_controller definition. Something like:
>
> struct jz4780_nand_cs {
> 	unsigned int bank;
> 	void __iomem *base;
> };
>
> struct jz4780_nand_controller {
> 	struct nand_hw_control controller;
> 	struct device *dev;
> 	struct device *bch;
> 	unsigned int num_banks;
> 	struct jz4780_nand_cs cs[];
> }
>
> static inline to_jz4780_nand_controller(struct nand_hw_control *ctrl)
> {
> 	return container_of(ctrl, struct jz4780_nand_controller,
> 			    controller);
> }
>
> struct jz4780_nand {
> 	struct mtd_info mtd;
> 	struct nand_chip chip;
>
> 	struct nand_ecclayout ecclayout;
>
> 	/*
> 	 * Number of CS lines attached to this chip. Each CS line
> 	 * controls a specific die in the chip.
> 	*/
> 	unsigned int num_cs;
> 	/* Should point the definitions in struct nand_controller */
> 	struct jz4780_nand_chip **cs;
>
> 	/* RB pins definition, there can be one RB pin per die. */
> 	int *busy_gpios;
>
> 	/* RB pins definition, there can be one RB pin per die. */
> 	int *wp_gpios;
>
> 	unsigned int busy_gpio_active_low: 1;
> 	unsigned int wp_gpio_active_low: 1;
> 	unsigned int reading: 1;
> };
>
> Note that the jz4780_nand_controller pointer can be retrieved from the
> nand_chip struct by doing the following:
>
> 	to_jz4780_nand_controller(chip->controller);
>
>> +
>> +static inline struct jz4780_nand *to_jz4780_nand(struct mtd_info *mtd)
>> +{
>> +	return container_of(mtd, struct jz4780_nand, mtd);
>> +}
>> +
>> +static void jz4780_nand_select_chip(struct mtd_info *mtd, int chipnr)
>> +{
>> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
>> +	struct jz4780_nand_chip *chip;
>> +
>> +	if (chipnr == -1) {
>> +		/* Ensure the currently selected chip is deasserted. */
>> +		if (nand->selected >= 0) {
>> +			chip = &nand->chips[nand->selected];
>> +			jz4780_nemc_assert(nand->dev, chip->bank, false);
>> +		}
>> +	} else {
>> +		chip = &nand->chips[chipnr];
>> +		nand->chip.IO_ADDR_R = chip->base + OFFSET_DATA;
>> +		nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;
>
> How about providing helper functions that would use the nand->selected
> + chips information to access the correct set of registers instead of
> adapting IO_ADDR_R/IO_ADDR_W values?

I'm not sure what you mean - are you suggesting something such as:

u8 *jz4780_nand_read_io_line(struct jz4780_nand *nand, unsigned int off)
{
	return readb(&nand->chips[nand->selected]->base + off);
}

Does the NAND core code not make use of IO_ADDR_{W,R}?

>
>> +	}
>> +
>> +	nand->selected = chipnr;
>> +}
>> +
>> +static void jz4780_nand_cmd_ctrl(struct mtd_info *mtd, int cmd,
>> +				 unsigned int ctrl)
>> +{
>> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
>> +	struct jz4780_nand_chip *chip;
>> +
>> +	if (WARN_ON(nand->selected < 0))
>> +		return;
>> +
>> +	chip = &nand->chips[nand->selected];
>> +
>> +	if (ctrl & NAND_CTRL_CHANGE) {
>> +		if (ctrl & NAND_ALE)
>> +			nand->chip.IO_ADDR_W = chip->base + OFFSET_ADDR;
>> +		else if (ctrl & NAND_CLE)
>> +			nand->chip.IO_ADDR_W = chip->base + OFFSET_CMD;
>> +		else
>> +			nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;
>
> Ditto.
>
>> +
>> +		jz4780_nemc_assert(nand->dev, chip->bank, ctrl & NAND_NCE);
>> +	}
>> +
>> +	if (cmd != NAND_CMD_NONE)
>> +		writeb(cmd, nand->chip.IO_ADDR_W);
>> +}
>> +
>> +static int jz4780_nand_dev_ready(struct mtd_info *mtd)
>> +{
>> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
>> +
>> +	return !(gpio_get_value(nand->busy_gpio) ^ nand->busy_gpio_active_low);
>> +}
>> +
>> +static void jz4780_nand_ecc_hwctl(struct mtd_info *mtd, int mode)
>> +{
>> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
>> +
>> +	nand->reading = (mode == NAND_ECC_READ);
>> +}
>> +
>> +static int jz4780_nand_ecc_calculate(struct mtd_info *mtd, const uint8_t *dat,
>> +				     uint8_t *ecc_code)
>> +{
>> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
>> +	struct jz4780_bch_params params;
>> +
>> +	/*
>> +	 * Don't need to generate the ECC when reading, BCH does it for us as
>> +	 * part of decoding/correction.
>> +	 */
>> +	if (nand->reading)
>> +		return 0;
>> +
>> +	params.size = nand->chip.ecc.size;
>> +	params.bytes = nand->chip.ecc.bytes;
>> +	params.strength = nand->chip.ecc.strength;
>> +
>> +	return jz4780_bch_calculate(nand->bch, &params, dat, ecc_code);
>> +}
>> +
>> +static int jz4780_nand_ecc_correct(struct mtd_info *mtd, uint8_t *dat,
>> +				   uint8_t *read_ecc, uint8_t *calc_ecc)
>> +{
>> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
>> +	struct jz4780_bch_params params;
>> +
>> +	params.size = nand->chip.ecc.size;
>> +	params.bytes = nand->chip.ecc.bytes;
>> +	params.strength = nand->chip.ecc.strength;
>> +
>> +	return jz4780_bch_correct(nand->bch, &params, dat, read_ecc);
>> +}
>> +
>> +static int jz4780_nand_init_ecc(struct jz4780_nand *nand, struct device *dev)
>> +{
>> +	struct mtd_info *mtd = &nand->mtd;
>> +	struct nand_chip *chip = &nand->chip;
>> +	struct nand_ecclayout *layout = &nand->ecclayout;
>> +	struct device_node *bch_np;
>> +	int ret = 0;
>> +	uint32_t start, i;
>> +
>> +	if (!chip->ecc.size)
>> +		chip->ecc.size = 1024;
>> +
>> +	if (!chip->ecc.strength)
>> +		chip->ecc.strength = 24;
>
> Is there a strong reason to define a random default ECC config. I mean,
> if this is not provided then there is a problem somewhere, or the NAND
> does not require ECC, in which case mode should be set to NAND_ECC_NONE.
>
> At least, if you want to automatically choose an ECC config when it's
> not provided, choose it according to the NAND layout (it has to fit into
> the OOB area).

Nope, I'll change it to default to NAND_ECC_NONE if nothing has been 
passed from DT.

>> +
>> +	chip->ecc.bytes = fls(1 + 8 * chip->ecc.size) * chip->ecc.strength / 8;
>> +
>> +	dev_info(dev, "using %s BCH (strength %d, size %d, bytes %d)\n",
>> +		 (nand->bch) ? "hardware" : "software", chip->ecc.strength,
>> +		 chip->ecc.size, chip->ecc.bytes);
>> +
>> +	if (chip->ecc.mode == NAND_ECC_HW) {
>> +		bch_np = of_parse_phandle(dev->of_node,
>> +					"ingenic,bch-controller", 0);
>> +		if (bch_np) {
>> +			ret = jz4780_bch_get(bch_np, &nand->bch);
>> +			of_node_put(bch_np);
>> +			if (ret)
>> +				return ret;
>> +		} else {
>> +			dev_err(dev, "no bch controller in DT\n");
>> +			return -ENODEV;
>> +		}
>> +
>> +		chip->ecc.hwctl = jz4780_nand_ecc_hwctl;
>> +		chip->ecc.calculate = jz4780_nand_ecc_calculate;
>> +		chip->ecc.correct = jz4780_nand_ecc_correct;
>
> You should probably check that
> (ecc->bytes * (mtd->writesize / ecc->size)) <= mtd->oobsize - 2
> and fail if it's not the case.

Okay, will do.

>> +	}
>> +
>> +	/* Generate ECC layout. ECC codes are right aligned in the OOB area. */
>> +	layout->eccbytes = mtd->writesize / chip->ecc.size * chip->ecc.bytes;
>> +	start = mtd->oobsize - layout->eccbytes;
>> +	for (i = 0; i < layout->eccbytes; i++)
>> +		layout->eccpos[i] = start + i;
>> +
>> +	layout->oobfree[0].offset = 2;
>> +	layout->oobfree[0].length = mtd->oobsize - layout->eccbytes - 2;
>
> Not sure you want to fill that if mode == NAND_ECC_SOFT of
> NAND_ECC_SOFT_BCH, because those fields are automatically create/filled
> for you.

Okay, I'll add a check for that.

>> +
>> +	chip->ecc.layout = layout;
>> +
>> +	return 0;
>> +}
>> +
>> +static int jz4780_nand_init_chips(struct jz4780_nand *nand,
>> +				  struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct jz4780_nand_chip *chip;
>> +	const __be32 *prop;
>> +	struct resource *res;
>> +	int i = 0;
>> +
>> +	/*
>> +	 * Iterate over each bank assigned to this device and request resources.
>> +	 * The bank numbers may not be consecutive, but nand_scan_ident()
>> +	 * expects chip numbers to be, so fill out a consecutive array of chips
>> +	 * which map chip number to actual bank number.
>> +	 */
>> +	while ((prop = of_get_address(dev->of_node, i, NULL, NULL))) {
>> +		chip = &nand->chips[i];
>> +		chip->bank = of_read_number(prop, 1);
>> +
>> +		jz4780_nemc_set_type(nand->dev, chip->bank,
>> +				     JZ4780_NEMC_BANK_NAND);
>> +
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>> +		chip->base = devm_ioremap_resource(dev, res);
>> +		if (IS_ERR(chip->base))
>> +			return PTR_ERR(chip->base);
>> +
>> +		i++;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int jz4780_nand_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	unsigned int num_banks;
>> +	struct jz4780_nand *nand;
>> +	struct mtd_info *mtd;
>> +	struct nand_chip *chip;
>> +	enum of_gpio_flags flags;
>> +	struct mtd_part_parser_data ppdata;
>> +	int ret;
>> +
>> +	num_banks = jz4780_nemc_num_banks(dev);
>> +	if (num_banks == 0) {
>> +		dev_err(dev, "no banks found\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	nand = devm_kzalloc(dev,
>> +			sizeof(*nand) + (sizeof(nand->chips[0]) * num_banks),
>> +			GFP_KERNEL);
>> +	if (!nand)
>> +		return -ENOMEM;
>> +
>> +	nand->dev = dev;
>> +	nand->num_banks = num_banks;
>> +	nand->selected = -1;
>> +
>> +	mtd = &nand->mtd;
>> +	chip = &nand->chip;
>> +	mtd->priv = chip;
>> +	mtd->owner = THIS_MODULE;
>> +	mtd->name = DRV_NAME;
>> +	mtd->dev.parent = dev;
>> +
>> +	chip->dn = dev->of_node;
>
> chip->dn has recently been renamed to chip->flash_node.

Thanks, I'll change this.

> BTW, If you go for the nand_controller + nand_chip approach, you'll have
> to initialize the nand_controller struct and link it with your
> nand_chip by doing:
>
> 	spin_lock_init(&jz4780_ctrl->controller.lock);
> 	init_waitqueue_head(&jz4780_ctrl->controller.wq);
> 	chip->controller = &jz4780_ctrl->controller;

Noted - thanks.

> That's all I see for now.
>
> Best Regards,
>
> Boris
>

Thanks again for the thorough review.

Best regards,

Harvey Hunt

Re: [PATCH v7,3/3] MIPS: dts: jz4780/ci20: Add NEMC, BCH and NAND device tree nodes

[Harvey Hunt]

Hi Boris,

On 04/11/15 07:51, Boris Brezillon wrote:
> Paul, Harvey,
>
> On Fri, 16 Oct 2015 11:48:48 +0100
> Paul Burton <paul.burton@imgtec.com> wrote:
>
>> On Fri, Oct 16, 2015 at 11:31:12AM +0100, James Hogan wrote:
>>>>>> +
>>>>>> +&nemc {
>>>>>> +     status = "okay";
>>>>>> +
>>>>>> +     nand: nand@1 {
>>>>>> +             compatible = "ingenic,jz4780-nand";
>>>>>
>>>>> Isn't the NAND a micron part? This doesn't seem right. Is the device
>>>>> driver and binding already accepted upstream with that compatible
>>>>> string?
>>>>
>>>> This is the compatible string for the JZ4780 NAND driver, this patch
>>>> is part of the series adding that. Detection of the NAND part is
>>>> handled by the MTD subsystem.
>>>
>>> Right (didn't spot that it was part of a series).
>>>
>>> The node appears to describe the NAND interface itself, i.e. a part of
>>> the SoC, so should be in the SoC dtsi file, with overrides in the board
>>> file if necessary for it to work with a particular NAND part
>>> (potentially utilising status="disabled"). Would you agree?
>>
>> Hi James,
>>
>> The "nemc" node there is for the Nand & External Memory Controller which
>> is a hardware block inside the SoC. It has 6 banks (ie. 6 chip select
>> pins, each associated with a different address range, that connect to
>> different devices). NAND flash is one such possible device, but a board
>> could connect it to any of the 6 chip selects, or banks. To represent
>> that in the SoC dtsi you'd want to have 6 NAND nodes, each disabled by
>> default, which doesn't make a whole lot of sense to me. Other, non-NAND
>> devices can connect to the NEMC too - for example the ethernet
>> controller on the CI20 is connected to one bank.
>>
>> The NAND device nodes are sort of a mix of describing the NAND flash
>> (ie. Micron part as you point out) and its connections & properties, the
>> way the NEMC should be used to interact with it alongside the BCH block,
>> and the configuration for the NEMC such as timing parameters.
>>
>> I imagine the most semantically correct means of describing it would
>> probably be for the compatible string to reflect the Micron NAND part,
>> and the NEMC driver to pick up on the relevant properties of its child
>> nodes for configuring timings, whether the device is NAND etc. However
>> the handling of registering NAND devices with MTD would probably then
>> have to be part of the NEMC driver, which feels a bit off too.
>
> Another solution would be to describe both the NAND controller and the
> NAND chip in the DT (with the NAND chip being a chip of the NAND
> controller).
> Actually this is already what other binding are doing [1][2]. I know
> those bindings are representing NAND controllers which can interface
> with more than one NAND chip, but I think that even in the 1:1 case it
> would make it clearer to represent both the NAND chip and the NAND
> controller.
>
> In your case this would give the following representation
>
> +&nemc {
> +	status = "okay";
> +
> +	nandc: nand-controller@1 {
> +		compatible = "ingenic,jz4780-nand";
> +		reg = <1 0 0x1000000>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		ingenic,bch-controller = <&bch>;
> +
> +		nand@0 {
> +			nand-ecc-mode = "hw";
> +			nand-on-flash-bbt;
> +			nand-ecc-size = <1024>;
> +			nand-ecc-strength = <24>;
> +
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +
> +			partition@0 {
> +				label = "u-boot-spl";
> +				reg = <0x0 0x0 0x0 0x800000>;
> +			};
> +			/* ... */
> +
> +		};
> +	};
> +};

I'll implement this in v8 - thanks for the example DT. :-)

> Best Regards,
>
> Boris
>
> [1]http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt#L119
> [2]http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/sunxi-nand.txt#L28
>
>>
>> Thanks,
>>      Paul
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
>
>

Best regards,

Harvey Hunt

Re: [PATCH v7, 2/3] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs

[Boris Brezillon]

Hi Harvey,

On Tue, 17 Nov 2015 16:28:59 +0000
Harvey Hunt <harvey.hunt@imgtec.com> wrote:


> >> +/* Timeout for BCH calculation/correction in microseconds. */
> >> +#define BCH_TIMEOUT			100000
> >
> > Suffixing the macro name with _MS would make it clearer.
> 
> Do you mean _US?

Yes.



> >> +static void jz4780_nand_select_chip(struct mtd_info *mtd, int chipnr)
> >> +{
> >> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
> >> +	struct jz4780_nand_chip *chip;
> >> +
> >> +	if (chipnr == -1) {
> >> +		/* Ensure the currently selected chip is deasserted. */
> >> +		if (nand->selected >= 0) {
> >> +			chip = &nand->chips[nand->selected];
> >> +			jz4780_nemc_assert(nand->dev, chip->bank, false);
> >> +		}
> >> +	} else {
> >> +		chip = &nand->chips[chipnr];
> >> +		nand->chip.IO_ADDR_R = chip->base + OFFSET_DATA;
> >> +		nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;
> >
> > How about providing helper functions that would use the nand->selected
> > + chips information to access the correct set of registers instead of
> > adapting IO_ADDR_R/IO_ADDR_W values?
> 
> I'm not sure what you mean - are you suggesting something such as:
> 
> u8 *jz4780_nand_read_io_line(struct jz4780_nand *nand, unsigned int off)
> {
> 	return readb(&nand->chips[nand->selected]->base + off);
> }
> 
> Does the NAND core code not make use of IO_ADDR_{W,R}?

Right, I missed that. It's used to implement the default
nand_read/write_buf/byte/word() functions, but I still think it would be
preferable to implement your own read/write_xxx() functions than using
those fields, but that's up to you.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com