[PATCH v3 00/28] Armada 370/XP NAND support

[PATCH v3 00/28] Armada 370/XP NAND support

[Ezequiel Garcia]

Behold the Armada 370/XP SoCs v3 patchset!

This time the details about the controller have been added to
Documentation/mtd/nand/pxa3xx-nand.txt, so I'd appreciate if you could review
the docs as well.

It's worth noting that the *only* previously remaining issue (the bad block
factory initial detection) has been addressed by this recent patch:

  http://patchwork.ozlabs.org/patch/288482/

If the change is accepted,  I'll send a simple follow-up patch to make use
of the new option.

In other words, this series is now _complete_, and so I've switched to
"get-feedback-and-resubmit" mode :-)

Patches 1-4, adding the clock infrastructure have already been merged
and will be dropped once this series can be based on v3.13-rc1.
I'm including them just for completeness.

Based in l2-mtd's master branch. Also, I've pushed a branch to our github
in case anyone wants to test it:

  https://github.com/MISL-EBU-System-SW/mainline-public/tree/l2-mtd/upstream-nand-v3

Thanks!

* Changes from v2 (some minor fixes as per Huang's good feedback)

  * Add some more details to the commit log in patch
    "mtd: nand: pxa3xx: Early variant detection"

  * Add an empty line between variable declaration and function body
    in patch "mtd: nand: pxa3xx: Use chip->cmdfunc instead of the internal".

  * Fix a build break caused by incomplete variable replacement:
    "mtd: nand: pxa3xx: Replace host->page_size by mtd->writesize"

* Changes from v1

Aside from several changes based in Brian's feedback, the main changes
from v1 are:

  * The controller's clock source is now fully modeled, see patche 1 to 4.
    Of course, none of those patches should be taken through the mtd
    subsystem, but I'm adding them here for completeness.

  * The chip's cmdfunc() is now independently implemented in each SoC variant.
    The rationale behind this decision is that 'chunked' I/O is the only tested
    mode on the Armada370 variant, while the old 'vanilla' I/O is the only
    tested mode on the PXA variant.

    So it's safer to have an implementation for each variant.

  * Added support for BCH-8, in other words: 8-bits of correction in a 512-byte
    region. This is obtained by using a data chunk size of 1024B, thus doubling
    the ECC BCH strength, as per this ECC engine mechanism.

  * The ECC layout in use, which must be set according to the page size and
    desired ECC strength is now strictly chosen to match only the tested
    combinations.

Ezequiel Garcia (28):
  clk: mvebu: Add Core Divider clock
  ARM: mvebu: Add Core Divider clock device-tree binding
  ARM: mvebu: Add a 2 GHz fixed-clock Armada 370/XP
  ARM: mvebu: Add the core-divider clock to Armada 370/XP
  mtd: nand: pxa3xx: Make config menu show supported platforms
  mtd: nand: pxa3xx: Prevent sub-page writes
  mtd: nand: pxa3xx: Early variant detection
  mtd: nand: pxa3xx: Use chip->cmdfunc instead of the internal
  mtd: nand: pxa3xx: Split FIFO size from to-be-read FIFO count
  mtd: nand: pxa3xx: Replace host->page_size by mtd->writesize
  mtd: nand: pxa3xx: Add a nice comment to pxa3xx_set_datasize()
  mtd: nand: pxa3xx: Use a completion to signal device ready
  mtd: nand: pxa3xx: Add bad block handling
  mtd: nand: pxa3xx: Add driver-specific ECC BCH support
  mtd: nand: pxa3xx: Clear cmd buffer #3 (NDCB3) on command start
  mtd: nand: pxa3xx: Add helper function to set page address
  mtd: nand: pxa3xx: Remove READ0 switch/case falltrough
  mtd: nand: pxa3xx: Split prepare_command_pool() in two stages
  mtd: nand: pxa3xx: Move the data buffer clean to
    prepare_start_command()
  mtd: nand: pxa3xx: Fix SEQIN column address set
  mtd: nand: pxa3xx: Add a read/write buffers markers
  mtd: nand: pxa3xx: Introduce multiple page I/O support
  mtd: nand: pxa3xx: Add multiple chunk write support
  mtd: nand: pxa3xx: Add ECC BCH correctable errors detection
  ARM: mvebu: Add support for NAND controller in Armada 370/XP
  ARM: mvebu: Enable NAND controller in Armada XP GP board
  ARM: mvebu: Enable NAND controller in Armada 370 Mirabox
  mtd: nand: pxa3xx: Add documentation about the controller

 .../bindings/clock/mvebu-corediv-clock.txt         |  19 +
 Documentation/mtd/nand/pxa3xx-nand.txt             | 101 +++
 arch/arm/boot/dts/armada-370-mirabox.dts           |  21 +
 arch/arm/boot/dts/armada-370-xp.dtsi               |  26 +
 arch/arm/boot/dts/armada-xp-gp.dts                 |   8 +
 drivers/clk/mvebu/Kconfig                          |   5 +
 drivers/clk/mvebu/Makefile                         |   1 +
 drivers/clk/mvebu/clk-corediv.c                    | 223 +++++++
 drivers/mtd/nand/Kconfig                           |   4 +-
 drivers/mtd/nand/pxa3xx_nand.c                     | 683 ++++++++++++++++-----
 include/linux/platform_data/mtd-nand-pxa3xx.h      |   3 +
 11 files changed, 955 insertions(+), 139 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
 create mode 100644 Documentation/mtd/nand/pxa3xx-nand.txt
 create mode 100644 drivers/clk/mvebu/clk-corediv.c

-- 
1.8.1.5

[PATCH v3 01/28] clk: mvebu: Add Core Divider clock

[Ezequiel Garcia]

This commit introduces a new group of clocks present in Armada 370/XP
SoCs (called "Core Divider" clocks) and add a provider for them.
The only clock supported for now is the NAND clock (ndclk), but the
infrastructure to add the rest is already set.

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Acked-by: Jason Cooper <jason@lakedaemon.net>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/clk/mvebu/Kconfig       |   5 +
 drivers/clk/mvebu/Makefile      |   1 +
 drivers/clk/mvebu/clk-corediv.c | 223 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 229 insertions(+)
 create mode 100644 drivers/clk/mvebu/clk-corediv.c

diff --git a/drivers/clk/mvebu/Kconfig b/drivers/clk/mvebu/Kconfig
index 0b0f3e7..c339b82 100644
--- a/drivers/clk/mvebu/Kconfig
+++ b/drivers/clk/mvebu/Kconfig
@@ -4,15 +4,20 @@ config MVEBU_CLK_COMMON
 config MVEBU_CLK_CPU
 	bool
 
+config MVEBU_CLK_COREDIV
+	bool
+
 config ARMADA_370_CLK
 	bool
 	select MVEBU_CLK_COMMON
 	select MVEBU_CLK_CPU
+	select MVEBU_CLK_COREDIV
 
 config ARMADA_XP_CLK
 	bool
 	select MVEBU_CLK_COMMON
 	select MVEBU_CLK_CPU
+	select MVEBU_CLK_COREDIV
 
 config DOVE_CLK
 	bool
diff --git a/drivers/clk/mvebu/Makefile b/drivers/clk/mvebu/Makefile
index 1c7e70c..21bbfb4 100644
--- a/drivers/clk/mvebu/Makefile
+++ b/drivers/clk/mvebu/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_MVEBU_CLK_COMMON)	+= common.o
 obj-$(CONFIG_MVEBU_CLK_CPU) 	+= clk-cpu.o
+obj-$(CONFIG_MVEBU_CLK_COREDIV)	+= clk-corediv.o
 
 obj-$(CONFIG_ARMADA_370_CLK)	+= armada-370.o
 obj-$(CONFIG_ARMADA_XP_CLK)	+= armada-xp.o
diff --git a/drivers/clk/mvebu/clk-corediv.c b/drivers/clk/mvebu/clk-corediv.c
new file mode 100644
index 0000000..7162615
--- /dev/null
+++ b/drivers/clk/mvebu/clk-corediv.c
@@ -0,0 +1,223 @@
+/*
+ * MVEBU Core divider clock
+ *
+ * Copyright (C) 2013 Marvell
+ *
+ * Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/clk-provider.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include "common.h"
+
+#define CORE_CLK_DIV_RATIO_MASK		0xff
+#define CORE_CLK_DIV_RATIO_RELOAD	BIT(8)
+#define CORE_CLK_DIV_ENABLE_OFFSET	24
+#define CORE_CLK_DIV_RATIO_OFFSET	0x8
+
+struct clk_corediv_desc {
+	unsigned int mask;
+	unsigned int offset;
+	unsigned int fieldbit;
+};
+
+struct clk_corediv {
+	struct clk_hw hw;
+	void __iomem *reg;
+	struct clk_corediv_desc desc;
+	spinlock_t lock;
+};
+
+static struct clk_onecell_data clk_data;
+
+static const struct clk_corediv_desc mvebu_corediv_desc[] __initconst = {
+	{ .mask = 0x3f, .offset = 8, .fieldbit = 1 }, /* NAND clock */
+};
+
+#define to_corediv_clk(p) container_of(p, struct clk_corediv, hw)
+
+static int clk_corediv_is_enabled(struct clk_hw *hwclk)
+{
+	struct clk_corediv *corediv = to_corediv_clk(hwclk);
+	struct clk_corediv_desc *desc = &corediv->desc;
+	u32 enable_mask = BIT(desc->fieldbit) << CORE_CLK_DIV_ENABLE_OFFSET;
+
+	return !!(readl(corediv->reg) & enable_mask);
+}
+
+static int clk_corediv_enable(struct clk_hw *hwclk)
+{
+	struct clk_corediv *corediv = to_corediv_clk(hwclk);
+	struct clk_corediv_desc *desc = &corediv->desc;
+	unsigned long flags = 0;
+	u32 reg;
+
+	spin_lock_irqsave(&corediv->lock, flags);
+
+	reg = readl(corediv->reg);
+	reg |= (BIT(desc->fieldbit) << CORE_CLK_DIV_ENABLE_OFFSET);
+	writel(reg, corediv->reg);
+
+	spin_unlock_irqrestore(&corediv->lock, flags);
+
+	return 0;
+}
+
+static void clk_corediv_disable(struct clk_hw *hwclk)
+{
+	struct clk_corediv *corediv = to_corediv_clk(hwclk);
+	struct clk_corediv_desc *desc = &corediv->desc;
+	unsigned long flags = 0;
+	u32 reg;
+
+	spin_lock_irqsave(&corediv->lock, flags);
+
+	reg = readl(corediv->reg);
+	reg &= ~(BIT(desc->fieldbit) << CORE_CLK_DIV_ENABLE_OFFSET);
+	writel(reg, corediv->reg);
+
+	spin_unlock_irqrestore(&corediv->lock, flags);
+}
+
+static unsigned long clk_corediv_recalc_rate(struct clk_hw *hwclk,
+					 unsigned long parent_rate)
+{
+	struct clk_corediv *corediv = to_corediv_clk(hwclk);
+	struct clk_corediv_desc *desc = &corediv->desc;
+	u32 reg, div;
+
+	reg = readl(corediv->reg + CORE_CLK_DIV_RATIO_OFFSET);
+	div = (reg >> desc->offset) & desc->mask;
+	return parent_rate / div;
+}
+
+static long clk_corediv_round_rate(struct clk_hw *hwclk, unsigned long rate,
+			       unsigned long *parent_rate)
+{
+	/* Valid ratio are 1:4, 1:5, 1:6 and 1:8 */
+	u32 div;
+
+	div = *parent_rate / rate;
+	if (div < 4)
+		div = 4;
+	else if (div > 6)
+		div = 8;
+
+	return *parent_rate / div;
+}
+
+static int clk_corediv_set_rate(struct clk_hw *hwclk, unsigned long rate,
+			    unsigned long parent_rate)
+{
+	struct clk_corediv *corediv = to_corediv_clk(hwclk);
+	struct clk_corediv_desc *desc = &corediv->desc;
+	unsigned long flags = 0;
+	u32 reg, div;
+
+	div = parent_rate / rate;
+
+	spin_lock_irqsave(&corediv->lock, flags);
+
+	/* Write new divider to the divider ratio register */
+	reg = readl(corediv->reg + CORE_CLK_DIV_RATIO_OFFSET);
+	reg &= ~(desc->mask << desc->offset);
+	reg |= (div & desc->mask) << desc->offset;
+	writel(reg, corediv->reg + CORE_CLK_DIV_RATIO_OFFSET);
+
+	/* Set reload-force for this clock */
+	reg = readl(corediv->reg) | BIT(desc->fieldbit);
+	writel(reg, corediv->reg);
+
+	/* Now trigger the clock update */
+	reg = readl(corediv->reg) | CORE_CLK_DIV_RATIO_RELOAD;
+	writel(reg, corediv->reg);
+
+	/*
+	 * Wait for clocks to settle down, and then clear all the
+	 * ratios request and the reload request.
+	 */
+	udelay(1000);
+	reg &= ~(CORE_CLK_DIV_RATIO_MASK | CORE_CLK_DIV_RATIO_RELOAD);
+	writel(reg, corediv->reg);
+	udelay(1000);
+
+	spin_unlock_irqrestore(&corediv->lock, flags);
+
+	return 0;
+}
+
+static const struct clk_ops corediv_ops = {
+	.enable = clk_corediv_enable,
+	.disable = clk_corediv_disable,
+	.is_enabled = clk_corediv_is_enabled,
+	.recalc_rate = clk_corediv_recalc_rate,
+	.round_rate = clk_corediv_round_rate,
+	.set_rate = clk_corediv_set_rate,
+};
+
+static void __init mvebu_corediv_clk_init(struct device_node *node)
+{
+	struct clk_init_data init;
+	struct clk_corediv *corediv;
+	struct clk **clks;
+	void __iomem *base;
+	const char *parent_name;
+	const char *clk_name;
+	int i;
+
+	base = of_iomap(node, 0);
+	if (WARN_ON(!base))
+		return;
+
+	parent_name = of_clk_get_parent_name(node, 0);
+
+	clk_data.clk_num = ARRAY_SIZE(mvebu_corediv_desc);
+
+	/* clks holds the clock array */
+	clks = kcalloc(clk_data.clk_num, sizeof(struct clk *),
+				GFP_KERNEL);
+	if (WARN_ON(!clks))
+		goto err_unmap;
+	/* corediv holds the clock specific array */
+	corediv = kcalloc(clk_data.clk_num, sizeof(struct clk_corediv),
+				GFP_KERNEL);
+	if (WARN_ON(!corediv))
+		goto err_free_clks;
+
+	spin_lock_init(&corediv->lock);
+
+	for (i = 0; i < clk_data.clk_num; i++) {
+		of_property_read_string_index(node, "clock-output-names",
+					      i, &clk_name);
+		init.num_parents = 1;
+		init.parent_names = &parent_name;
+		init.name = clk_name;
+		init.ops = &corediv_ops;
+		init.flags = 0;
+
+		corediv[i].desc = mvebu_corediv_desc[i];
+		corediv[i].reg = base;
+		corediv[i].hw.init = &init;
+
+		clks[i] = clk_register(NULL, &corediv[i].hw);
+		WARN_ON(IS_ERR(clks[i]));
+	}
+
+	clk_data.clks = clks;
+	of_clk_add_provider(node, of_clk_src_onecell_get, &clk_data);
+	return;
+
+err_free_clks:
+	kfree(clks);
+err_unmap:
+	iounmap(base);
+}
+CLK_OF_DECLARE(mvebu_corediv_clk, "marvell,armada-370-corediv-clock",
+	       mvebu_corediv_clk_init);
-- 
1.8.1.5

[PATCH v3 02/28] ARM: mvebu: Add Core Divider clock device-tree binding

[Ezequiel Garcia]

The Armada 370/XP SoCs have a Core Divider clock providing
several clocks. For now, only the NAND clock is supported.

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 .../devicetree/bindings/clock/mvebu-corediv-clock.txt | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt b/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
new file mode 100644
index 0000000..c62391f
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
@@ -0,0 +1,19 @@
+* Core Divider Clock bindings for Marvell MVEBU SoCs
+
+The following is a list of provided IDs and clock names on Armada 370/XP:
+ 0 = nand (NAND clock)
+
+Required properties:
+- compatible : must be "marvell,armada-370-corediv-clock"
+- reg : must be the register address of Core Divider control register
+- #clock-cells : from common clock binding; shall be set to 1
+- clocks : must be set to the parent's phandle
+
+Example:
+
+corediv_clk: corediv-clocks at 18740 {
+	compatible = "marvell,armada-370-corediv-clock";
+	reg = <0x18740 0xc>;
+	#clock-cells = <1>;
+	clocks = <&pll>;
+};
-- 
1.8.1.5

[PATCH v3 03/28] ARM: mvebu: Add a 2 GHz fixed-clock Armada 370/XP

[Ezequiel Garcia]

Armada 370/XP SoCs have a 2 GHz fixed PLL that is used to feed
other clocks. This commit adds a DT representation of this clock
through a fixed-clock compatible node.

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 1de2dae..924c721 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -252,4 +252,13 @@
 
 		};
 	};
+
+	clocks {
+		/* 2 GHz fixed main PLL */
+		mainpll: mainpll {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <2000000000>;
+		};
+	};
  };
-- 
1.8.1.5

[PATCH v3 04/28] ARM: mvebu: Add the core-divider clock to Armada 370/XP

[Ezequiel Garcia]

The Armada 370/XP SoC has a clock provider called "Core Divider",
that is derived from a fixed 2 GHz PLL clock.

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 924c721..01e69fc 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -137,6 +137,14 @@
 				status = "disabled";
 			};
 
+			coredivclk: corediv-clock at 18740 {
+				compatible = "marvell,armada-370-corediv-clock";
+				reg = <0x18740 0xc>;
+				#clock-cells = <1>;
+				clocks = <&mainpll>;
+				clock-output-names = "nand";
+			};
+
 			timer at 20300 {
 				reg = <0x20300 0x30>, <0x21040 0x30>;
 				interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
-- 
1.8.1.5

[PATCH v3 05/28] mtd: nand: pxa3xx: Make config menu show supported platforms

[Ezequiel Garcia]

Since we have now support for the NFCv2 controller found on
Armada 370/XP platforms.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 93ae6a6..942ef28 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -326,11 +326,11 @@ config MTD_NAND_ATMEL
 	  on Atmel AT91 and AVR32 processors.
 
 config MTD_NAND_PXA3xx
-	tristate "Support for NAND flash devices on PXA3xx"
+	tristate "NAND support on PXA3xx and Armada 370/XP"
 	depends on PXA3xx || ARCH_MMP || PLAT_ORION
 	help
 	  This enables the driver for the NAND flash device found on
-	  PXA3xx processors
+	  PXA3xx processors (NFCv1) and also on Armada 370/XP (NFCv2).
 
 config MTD_NAND_SLC_LPC32XX
 	tristate "NXP LPC32xx SLC Controller"
-- 
1.8.1.5

[PATCH v3 06/28] mtd: nand: pxa3xx: Prevent sub-page writes

[Ezequiel Garcia]

The current driver doesn't support sub-page writing, so report
that to the NAND core.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 64c258e..3765e4e 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1143,6 +1143,7 @@ static int alloc_nand_resource(struct platform_device *pdev)
 		chip->read_byte		= pxa3xx_nand_read_byte;
 		chip->read_buf		= pxa3xx_nand_read_buf;
 		chip->write_buf		= pxa3xx_nand_write_buf;
+		chip->options		|= NAND_NO_SUBPAGE_WRITE;
 	}
 
 	spin_lock_init(&chip->controller->lock);
-- 
1.8.1.5

[PATCH v3 07/28] mtd: nand: pxa3xx: Early variant detection

[Ezequiel Garcia]

In order to customize early settings depending on the detected SoC variant,
move the detection to be before the nand_chip struct filling.

In a follow-up patch, this change is needed to detect the variant *before*
the call to alloc_nand_resource(), which allows to set a different cmdfunc()
for each variant.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 48 +++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 3765e4e..d4937b2 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -256,6 +256,29 @@ static struct pxa3xx_nand_flash builtin_flash_types[] = {
 /* convert nano-seconds to nand flash controller clock cycles */
 #define ns2cycle(ns, clk)	(int)((ns) * (clk / 1000000) / 1000)
 
+static struct of_device_id pxa3xx_nand_dt_ids[] = {
+	{
+		.compatible = "marvell,pxa3xx-nand",
+		.data       = (void *)PXA3XX_NAND_VARIANT_PXA,
+	},
+	{
+		.compatible = "marvell,armada370-nand",
+		.data       = (void *)PXA3XX_NAND_VARIANT_ARMADA370,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, pxa3xx_nand_dt_ids);
+
+static enum pxa3xx_nand_variant
+pxa3xx_nand_get_variant(struct platform_device *pdev)
+{
+	const struct of_device_id *of_id =
+			of_match_device(pxa3xx_nand_dt_ids, &pdev->dev);
+	if (!of_id)
+		return PXA3XX_NAND_VARIANT_PXA;
+	return (enum pxa3xx_nand_variant)of_id->data;
+}
+
 static void pxa3xx_nand_set_timing(struct pxa3xx_nand_host *host,
 				   const struct pxa3xx_nand_timing *t)
 {
@@ -1121,6 +1144,7 @@ static int alloc_nand_resource(struct platform_device *pdev)
 		return -ENOMEM;
 
 	info->pdev = pdev;
+	info->variant = pxa3xx_nand_get_variant(pdev);
 	for (cs = 0; cs < pdata->num_cs; cs++) {
 		mtd = (struct mtd_info *)((unsigned int)&info[1] +
 		      (sizeof(*mtd) + sizeof(*host)) * cs);
@@ -1256,29 +1280,6 @@ static int pxa3xx_nand_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct of_device_id pxa3xx_nand_dt_ids[] = {
-	{
-		.compatible = "marvell,pxa3xx-nand",
-		.data       = (void *)PXA3XX_NAND_VARIANT_PXA,
-	},
-	{
-		.compatible = "marvell,armada370-nand",
-		.data       = (void *)PXA3XX_NAND_VARIANT_ARMADA370,
-	},
-	{}
-};
-MODULE_DEVICE_TABLE(of, pxa3xx_nand_dt_ids);
-
-static enum pxa3xx_nand_variant
-pxa3xx_nand_get_variant(struct platform_device *pdev)
-{
-	const struct of_device_id *of_id =
-			of_match_device(pxa3xx_nand_dt_ids, &pdev->dev);
-	if (!of_id)
-		return PXA3XX_NAND_VARIANT_PXA;
-	return (enum pxa3xx_nand_variant)of_id->data;
-}
-
 static int pxa3xx_nand_probe_dt(struct platform_device *pdev)
 {
 	struct pxa3xx_nand_platform_data *pdata;
@@ -1335,7 +1336,6 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 	}
 
 	info = platform_get_drvdata(pdev);
-	info->variant = pxa3xx_nand_get_variant(pdev);
 	probe_success = 0;
 	for (cs = 0; cs < pdata->num_cs; cs++) {
 		struct mtd_info *mtd = info->host[cs]->mtd;
-- 
1.8.1.5

[PATCH v3 08/28] mtd: nand: pxa3xx: Use chip->cmdfunc instead of the internal

[Ezequiel Garcia]

Whenever possible, it's always better to use the generic chip->cmdfunc
instead of the internal pxa3xx_nand_cmdfunc().
In this particular case, this will allow to have multiple cmdfunc()
implementations for different SoC variants.

Reviewed-by: Huang Shijie <shijie8@gmail.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index d4937b2..12174c3 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1011,14 +1011,18 @@ static void pxa3xx_nand_free_buff(struct pxa3xx_nand_info *info)
 static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
 {
 	struct mtd_info *mtd;
+	struct nand_chip *chip;
 	int ret;
+
 	mtd = info->host[info->cs]->mtd;
+	chip = mtd->priv;
+
 	/* use the common timing to make a try */
 	ret = pxa3xx_nand_config_flash(info, &builtin_flash_types[0]);
 	if (ret)
 		return ret;
 
-	pxa3xx_nand_cmdfunc(mtd, NAND_CMD_RESET, 0, 0);
+	chip->cmdfunc(mtd, NAND_CMD_RESET, 0, 0);
 	if (info->is_ready)
 		return 0;
 
-- 
1.8.1.5

[PATCH v3 09/28] mtd: nand: pxa3xx: Split FIFO size from to-be-read FIFO count

[Ezequiel Garcia]

Introduce a fifo_size field to represent the size of the controller's
FIFO buffer, and use it to distinguish that size from the amount
of data bytes to be read from the FIFO.

This is important to support devices with pages larger than the
controller's internal FIFO, that need to read the pages in FIFO-sized
chunks.

In particular, the current code is at least confusing, for it mixes
all the different sizes involved: FIFO size, page size and data size.

This commit starts the cleaning by removing the info->page_size field
that is not currently used. The host->page_size field should also
be removed and use always mtd->writesize instead. Follow up commits
will clean this up.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 12174c3..1fc9e6d 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -199,8 +199,8 @@ struct pxa3xx_nand_info {
 	int			use_spare;	/* use spare ? */
 	int			is_ready;
 
-	unsigned int		page_size;	/* page size of attached chip */
-	unsigned int		data_size;	/* data size in FIFO */
+	unsigned int		fifo_size;	/* max. data size in the FIFO */
+	unsigned int		data_size;	/* data to be read from FIFO */
 	unsigned int		oob_size;
 	int 			retcode;
 
@@ -305,16 +305,15 @@ static void pxa3xx_nand_set_timing(struct pxa3xx_nand_host *host,
 
 static void pxa3xx_set_datasize(struct pxa3xx_nand_info *info)
 {
-	struct pxa3xx_nand_host *host = info->host[info->cs];
 	int oob_enable = info->reg_ndcr & NDCR_SPARE_EN;
 
-	info->data_size = host->page_size;
+	info->data_size = info->fifo_size;
 	if (!oob_enable) {
 		info->oob_size = 0;
 		return;
 	}
 
-	switch (host->page_size) {
+	switch (info->fifo_size) {
 	case 2048:
 		info->oob_size = (info->use_ecc) ? 40 : 64;
 		break;
@@ -929,9 +928,12 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 	uint32_t ndcr = nand_readl(info, NDCR);
 
 	if (ndcr & NDCR_PAGE_SZ) {
+		/* Controller's FIFO size */
+		info->fifo_size = 2048;
 		host->page_size = 2048;
 		host->read_id_bytes = 4;
 	} else {
+		info->fifo_size = 512;
 		host->page_size = 512;
 		host->read_id_bytes = 2;
 	}
-- 
1.8.1.5

[PATCH v3 10/28] mtd: nand: pxa3xx: Replace host->page_size by mtd->writesize

[Ezequiel Garcia]

There's no need to privately store the device page size as it's
available in mtd structure field mtd->writesize.
Also, this removes the hardcoded page size value, leaving the
auto-detected value only.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 1fc9e6d..c558cadb 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -149,7 +149,6 @@ struct pxa3xx_nand_host {
 	void			*info_data;
 
 	/* page size of attached chip */
-	unsigned int		page_size;
 	int			use_ecc;
 	int			cs;
 
@@ -612,12 +611,12 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 			info->buf_start += mtd->writesize;
 
 		/* Second command setting for large pages */
-		if (host->page_size >= PAGE_CHUNK_SIZE)
+		if (mtd->writesize >= PAGE_CHUNK_SIZE)
 			info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8);
 
 	case NAND_CMD_SEQIN:
 		/* small page addr setting */
-		if (unlikely(host->page_size < PAGE_CHUNK_SIZE)) {
+		if (unlikely(mtd->writesize < PAGE_CHUNK_SIZE)) {
 			info->ndcb1 = ((page_addr & 0xFFFFFF) << 8)
 					| (column & 0xFF);
 
@@ -891,7 +890,6 @@ static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
 	}
 
 	/* calculate flash information */
-	host->page_size = f->page_size;
 	host->read_id_bytes = (f->page_size == 2048) ? 4 : 2;
 
 	/* calculate addressing information */
@@ -930,11 +928,9 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 	if (ndcr & NDCR_PAGE_SZ) {
 		/* Controller's FIFO size */
 		info->fifo_size = 2048;
-		host->page_size = 2048;
 		host->read_id_bytes = 4;
 	} else {
 		info->fifo_size = 512;
-		host->page_size = 512;
 		host->read_id_bytes = 2;
 	}
 
@@ -1102,7 +1098,7 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
 	def = pxa3xx_flash_ids;
 KEEP_CONFIG:
 	chip->ecc.mode = NAND_ECC_HW;
-	chip->ecc.size = host->page_size;
+	chip->ecc.size = info->fifo_size;
 	chip->ecc.strength = 1;
 
 	if (info->reg_ndcr & NDCR_DWIDTH_M)
-- 
1.8.1.5

[PATCH v3 11/28] mtd: nand: pxa3xx: Add a nice comment to pxa3xx_set_datasize()

[Ezequiel Garcia]

Add a comment clarifying the use of pxa3xx_set_datasize() which is only
applicable on data read/write commands (i.e. commands with a data cycle,
such as READID, READ0, STATUS, etc.)

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index c558cadb..2fb0f38 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -302,6 +302,11 @@ static void pxa3xx_nand_set_timing(struct pxa3xx_nand_host *host,
 	nand_writel(info, NDTR1CS0, ndtr1);
 }
 
+/*
+ * Set the data and OOB size, depending on the selected
+ * spare and ECC configuration.
+ * Only applicable to READ0, READOOB and PAGEPROG commands.
+ */
 static void pxa3xx_set_datasize(struct pxa3xx_nand_info *info)
 {
 	int oob_enable = info->reg_ndcr & NDCR_SPARE_EN;
-- 
1.8.1.5

[PATCH v3 12/28] mtd: nand: pxa3xx: Use a completion to signal device ready

[Ezequiel Garcia]

Apparently, the expected behavior of the waitfunc() NAND chip call
is to wait for the device to be READY (this is a standard chip line).
However, the current implementation does almost nothing, which opens
a possibility to issue a command to a non-ready device.

Fix this by adding a new completion to wait for the ready event to arrive.

Because the "is ready" flag is cleared from the controller status
register, it's needed to store that state in the driver, and because the
field is accesed from an interruption, the field needs to be of an
atomic type.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 45 +++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 2fb0f38..e198c94 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -35,6 +35,7 @@
 
 #include <linux/platform_data/mtd-nand-pxa3xx.h>
 
+#define NAND_DEV_READY_TIMEOUT  50
 #define	CHIP_DELAY_TIMEOUT	(2 * HZ/10)
 #define NAND_STOP_DELAY		(2 * HZ/50)
 #define PAGE_CHUNK_SIZE		(2048)
@@ -166,7 +167,7 @@ struct pxa3xx_nand_info {
 	struct clk		*clk;
 	void __iomem		*mmio_base;
 	unsigned long		mmio_phys;
-	struct completion	cmd_complete;
+	struct completion	cmd_complete, dev_ready;
 
 	unsigned int 		buf_start;
 	unsigned int		buf_count;
@@ -196,7 +197,13 @@ struct pxa3xx_nand_info {
 	int			use_ecc;	/* use HW ECC ? */
 	int			use_dma;	/* use DMA ? */
 	int			use_spare;	/* use spare ? */
-	int			is_ready;
+
+	/*
+	 * The is_ready flag is accesed from several places,
+	 * including an interruption hander. We need an atomic
+	 * type to avoid races.
+	 */
+	atomic_t		is_ready;
 
 	unsigned int		fifo_size;	/* max. data size in the FIFO */
 	unsigned int		data_size;	/* data to be read from FIFO */
@@ -478,7 +485,7 @@ static void start_data_dma(struct pxa3xx_nand_info *info)
 static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 {
 	struct pxa3xx_nand_info *info = devid;
-	unsigned int status, is_completed = 0;
+	unsigned int status, is_completed = 0, is_ready = 0;
 	unsigned int ready, cmd_done;
 
 	if (info->cs == 0) {
@@ -514,8 +521,9 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 		is_completed = 1;
 	}
 	if (status & ready) {
-		info->is_ready = 1;
+		atomic_set(&info->is_ready, 1);
 		info->state = STATE_READY;
+		is_ready = 1;
 	}
 
 	if (status & NDSR_WRCMDREQ) {
@@ -544,6 +552,8 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 	nand_writel(info, NDSR, status);
 	if (is_completed)
 		complete(&info->cmd_complete);
+	if (is_ready)
+		complete(&info->dev_ready);
 NORMAL_IRQ_EXIT:
 	return IRQ_HANDLED;
 }
@@ -574,7 +584,6 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	info->oob_size		= 0;
 	info->use_ecc		= 0;
 	info->use_spare		= 1;
-	info->is_ready		= 0;
 	info->retcode		= ERR_NONE;
 	if (info->cs != 0)
 		info->ndcb0 = NDCB0_CSEL;
@@ -747,6 +756,8 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
 	exec_cmd = prepare_command_pool(info, command, column, page_addr);
 	if (exec_cmd) {
 		init_completion(&info->cmd_complete);
+		init_completion(&info->dev_ready);
+		atomic_set(&info->is_ready, 0);
 		pxa3xx_nand_start(info);
 
 		ret = wait_for_completion_timeout(&info->cmd_complete,
@@ -859,21 +870,27 @@ static int pxa3xx_nand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
 {
 	struct pxa3xx_nand_host *host = mtd->priv;
 	struct pxa3xx_nand_info *info = host->info_data;
+	int ret;
+
+	/* Need to wait? */
+	if (!atomic_read(&info->is_ready)) {
+		ret = wait_for_completion_timeout(&info->dev_ready,
+				CHIP_DELAY_TIMEOUT);
+		if (!ret) {
+			dev_err(&info->pdev->dev, "Ready time out!!!\n");
+			return NAND_STATUS_FAIL;
+		}
+	}
 
 	/* pxa3xx_nand_send_command has waited for command complete */
 	if (this->state == FL_WRITING || this->state == FL_ERASING) {
 		if (info->retcode == ERR_NONE)
 			return 0;
-		else {
-			/*
-			 * any error make it return 0x01 which will tell
-			 * the caller the erase and write fail
-			 */
-			return 0x01;
-		}
+		else
+			return NAND_STATUS_FAIL;
 	}
 
-	return 0;
+	return NAND_STATUS_READY;
 }
 
 static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
@@ -1026,7 +1043,7 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
 		return ret;
 
 	chip->cmdfunc(mtd, NAND_CMD_RESET, 0, 0);
-	if (info->is_ready)
+	if (atomic_read(&info->is_ready))
 		return 0;
 
 	return -ENODEV;
-- 
1.8.1.5

[PATCH v3 12/28] mtd: nand: pxa3xx: Use a completion to signal device ready

[Brian Norris]

On Tue, Nov 05, 2013 at 09:55:19AM -0300, Ezequiel Garcia wrote:
> Apparently, the expected behavior of the waitfunc() NAND chip call
> is to wait for the device to be READY (this is a standard chip line).
> However, the current implementation does almost nothing, which opens
> a possibility to issue a command to a non-ready device.
> 
> Fix this by adding a new completion to wait for the ready event to arrive.
> 
> Because the "is ready" flag is cleared from the controller status
> register, it's needed to store that state in the driver, and because the
> field is accesed from an interruption, the field needs to be of an
> atomic type.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 45 +++++++++++++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 2fb0f38..e198c94 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -35,6 +35,7 @@
>  
>  #include <linux/platform_data/mtd-nand-pxa3xx.h>
>  
> +#define NAND_DEV_READY_TIMEOUT  50
>  #define	CHIP_DELAY_TIMEOUT	(2 * HZ/10)
>  #define NAND_STOP_DELAY		(2 * HZ/50)
>  #define PAGE_CHUNK_SIZE		(2048)
> @@ -166,7 +167,7 @@ struct pxa3xx_nand_info {
>  	struct clk		*clk;
>  	void __iomem		*mmio_base;
>  	unsigned long		mmio_phys;
> -	struct completion	cmd_complete;
> +	struct completion	cmd_complete, dev_ready;

I still kinda think this could be consolidated into one completion,
under which cmdfunc() performs all the necessary waiting (for both the
"command complete" and "device ready" signals), but I think this is not
a big advantage right now, considering your code is not too complex
right now.

>  
>  	unsigned int 		buf_start;
>  	unsigned int		buf_count;
> @@ -196,7 +197,13 @@ struct pxa3xx_nand_info {
>  	int			use_ecc;	/* use HW ECC ? */
>  	int			use_dma;	/* use DMA ? */
>  	int			use_spare;	/* use spare ? */
> -	int			is_ready;
> +
> +	/*
> +	 * The is_ready flag is accesed from several places,
> +	 * including an interruption hander. We need an atomic
> +	 * type to avoid races.
> +	 */
> +	atomic_t		is_ready;

I believe your handling of this 'is_ready' bit is a little unwise, as
you are actually creating extra concurrency that is unneeded. I'll
summarize what you're doing with this 'is_ready' field:

  cmdfunc() -> sets info->is_ready=0 for appropriate commands
            -> kicks off the hardware

The following two sequences may then occur concurrently:

  (1) pxa3xx_nand_irq -> ready interrupt occurs
                      -> set info->is_ready=1
		      -> signal 'dev_ready' completion

  (2) waitfunc -> check info->is_ready, if it is 0...
                  |_ ... wait for the dev_ready completion

Instead of setting info->is_ready=1 under (1), you could set it in (2),
after the completion (or timeout). This avoids the concurrency, since
cmdfunc() and waitfunc() are sequential. This also avoids a benign race
in which you may not even call wait_for_completion() in (2) in the
following scenario:

  * Suppose waitfunc is delayed a long time
  * The IRQ handler...
    - receives the 'ready' interrupt
    - clears info->is_ready
    - calls complete(&info->dev_ready)
  * waitfunc() finally executes
    - because info->is_ready==1, it skips the wait_for_completion...(),
      leaving your completion unbalanced

This influences a comment I have below regarding your re-initialization
of the completion struct.

>  
>  	unsigned int		fifo_size;	/* max. data size in the FIFO */
>  	unsigned int		data_size;	/* data to be read from FIFO */
> @@ -478,7 +485,7 @@ static void start_data_dma(struct pxa3xx_nand_info *info)
>  static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  {
>  	struct pxa3xx_nand_info *info = devid;
> -	unsigned int status, is_completed = 0;
> +	unsigned int status, is_completed = 0, is_ready = 0;
>  	unsigned int ready, cmd_done;
>  
>  	if (info->cs == 0) {
> @@ -514,8 +521,9 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  		is_completed = 1;
>  	}
>  	if (status & ready) {
> -		info->is_ready = 1;
> +		atomic_set(&info->is_ready, 1);

According to my suggestions, I don't think you need to set
info->is_ready=1 here.

>  		info->state = STATE_READY;
> +		is_ready = 1;
>  	}
>  
>  	if (status & NDSR_WRCMDREQ) {
> @@ -544,6 +552,8 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  	nand_writel(info, NDSR, status);
>  	if (is_completed)
>  		complete(&info->cmd_complete);
> +	if (is_ready)
> +		complete(&info->dev_ready);
>  NORMAL_IRQ_EXIT:
>  	return IRQ_HANDLED;
>  }
> @@ -574,7 +584,6 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
>  	info->oob_size		= 0;
>  	info->use_ecc		= 0;
>  	info->use_spare		= 1;
> -	info->is_ready		= 0;
>  	info->retcode		= ERR_NONE;
>  	if (info->cs != 0)
>  		info->ndcb0 = NDCB0_CSEL;
> @@ -747,6 +756,8 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
>  	exec_cmd = prepare_command_pool(info, command, column, page_addr);
>  	if (exec_cmd) {
>  		init_completion(&info->cmd_complete);
> +		init_completion(&info->dev_ready);

Do you really need to init the completions each time you run a command?
AIUI, the only reason you would need to do this is if you aren't
matching up your calls to complete() and wait_for_completion*()
properly, so that you simply dodge the issue and reset the completion
count each time. This might be a result of the complexity of your
2-completion signalling design.

> +		atomic_set(&info->is_ready, 0);
>  		pxa3xx_nand_start(info);
>  
>  		ret = wait_for_completion_timeout(&info->cmd_complete,
> @@ -859,21 +870,27 @@ static int pxa3xx_nand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
>  {
>  	struct pxa3xx_nand_host *host = mtd->priv;
>  	struct pxa3xx_nand_info *info = host->info_data;
> +	int ret;
> +
> +	/* Need to wait? */
> +	if (!atomic_read(&info->is_ready)) {

This read of info->is_ready will no longer need to be atomic, because
you never modify it from the IRQ context--only from the cmdfunc() and
waitfunc().

> +		ret = wait_for_completion_timeout(&info->dev_ready,
> +				CHIP_DELAY_TIMEOUT);

I think you can just do a (non-atomic) info->is_ready=1 here.

> +		if (!ret) {
> +			dev_err(&info->pdev->dev, "Ready time out!!!\n");
> +			return NAND_STATUS_FAIL;
> +		}
> +	}
>  
>  	/* pxa3xx_nand_send_command has waited for command complete */
>  	if (this->state == FL_WRITING || this->state == FL_ERASING) {
>  		if (info->retcode == ERR_NONE)
>  			return 0;
> -		else {
> -			/*
> -			 * any error make it return 0x01 which will tell
> -			 * the caller the erase and write fail
> -			 */
> -			return 0x01;
> -		}
> +		else
> +			return NAND_STATUS_FAIL;
>  	}
>  
> -	return 0;
> +	return NAND_STATUS_READY;
>  }
>  
>  static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
> @@ -1026,7 +1043,7 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
>  		return ret;
>  
>  	chip->cmdfunc(mtd, NAND_CMD_RESET, 0, 0);
> -	if (info->is_ready)
> +	if (atomic_read(&info->is_ready))

Does this need to wait on the dev_ready completion? I'm not sure I can
guarantee there is no race on info->is_ready here, but then that means
I'm not sure the code is correct even with the atomic read (which I
don't think will be necessary, according to my suggestion above).

>  		return 0;
>  
>  	return -ENODEV;

Brian

[PATCH v3 12/28] mtd: nand: pxa3xx: Use a completion to signal device ready

[Ezequiel Garcia]

On Tue, Nov 05, 2013 at 11:51:36AM -0800, Brian Norris wrote:
> On Tue, Nov 05, 2013 at 09:55:19AM -0300, Ezequiel Garcia wrote:
> > Apparently, the expected behavior of the waitfunc() NAND chip call
> > is to wait for the device to be READY (this is a standard chip line).
> > However, the current implementation does almost nothing, which opens
> > a possibility to issue a command to a non-ready device.
> > 
> > Fix this by adding a new completion to wait for the ready event to arrive.
> > 
> > Because the "is ready" flag is cleared from the controller status
> > register, it's needed to store that state in the driver, and because the
> > field is accesed from an interruption, the field needs to be of an
> > atomic type.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  drivers/mtd/nand/pxa3xx_nand.c | 45 +++++++++++++++++++++++++++++-------------
> >  1 file changed, 31 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > index 2fb0f38..e198c94 100644
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -35,6 +35,7 @@
> >  
> >  #include <linux/platform_data/mtd-nand-pxa3xx.h>
> >  
> > +#define NAND_DEV_READY_TIMEOUT  50
> >  #define	CHIP_DELAY_TIMEOUT	(2 * HZ/10)
> >  #define NAND_STOP_DELAY		(2 * HZ/50)
> >  #define PAGE_CHUNK_SIZE		(2048)
> > @@ -166,7 +167,7 @@ struct pxa3xx_nand_info {
> >  	struct clk		*clk;
> >  	void __iomem		*mmio_base;
> >  	unsigned long		mmio_phys;
> > -	struct completion	cmd_complete;
> > +	struct completion	cmd_complete, dev_ready;
> 
> I still kinda think this could be consolidated into one completion,
> under which cmdfunc() performs all the necessary waiting (for both the
> "command complete" and "device ready" signals), but I think this is not
> a big advantage right now, considering your code is not too complex
> right now.
> 

Quite frankly, I'd love such solution (given this adds a lot of
complexity) but I can't see how. Maybe restarting the command done completion
at the end of cmdfunc() so waitfunc() would re-use it?

> >  
> >  	unsigned int 		buf_start;
> >  	unsigned int		buf_count;
> > @@ -196,7 +197,13 @@ struct pxa3xx_nand_info {
> >  	int			use_ecc;	/* use HW ECC ? */
> >  	int			use_dma;	/* use DMA ? */
> >  	int			use_spare;	/* use spare ? */
> > -	int			is_ready;
> > +
> > +	/*
> > +	 * The is_ready flag is accesed from several places,
> > +	 * including an interruption hander. We need an atomic
> > +	 * type to avoid races.
> > +	 */
> > +	atomic_t		is_ready;
> 
> I believe your handling of this 'is_ready' bit is a little unwise, as
> you are actually creating extra concurrency that is unneeded. I'll
> summarize what you're doing with this 'is_ready' field:
> 
>   cmdfunc() -> sets info->is_ready=0 for appropriate commands
>             -> kicks off the hardware
> 
> The following two sequences may then occur concurrently:
> 
>   (1) pxa3xx_nand_irq -> ready interrupt occurs
>                       -> set info->is_ready=1
> 		      -> signal 'dev_ready' completion
> 
>   (2) waitfunc -> check info->is_ready, if it is 0...
>                   |_ ... wait for the dev_ready completion
> 
> Instead of setting info->is_ready=1 under (1), you could set it in (2),
> after the completion (or timeout). This avoids the concurrency, since
> cmdfunc() and waitfunc() are sequential. This also avoids a benign race
> in which you may not even call wait_for_completion() in (2) in the
> following scenario:
> 
>   * Suppose waitfunc is delayed a long time
>   * The IRQ handler...
>     - receives the 'ready' interrupt
>     - clears info->is_ready
>     - calls complete(&info->dev_ready)
>   * waitfunc() finally executes
>     - because info->is_ready==1, it skips the wait_for_completion...(),
>       leaving your completion unbalanced
> 

Right.

> This influences a comment I have below regarding your re-initialization
> of the completion struct.
> 
> >  
> >  	unsigned int		fifo_size;	/* max. data size in the FIFO */
> >  	unsigned int		data_size;	/* data to be read from FIFO */
> > @@ -478,7 +485,7 @@ static void start_data_dma(struct pxa3xx_nand_info *info)
> >  static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
> >  {
> >  	struct pxa3xx_nand_info *info = devid;
> > -	unsigned int status, is_completed = 0;
> > +	unsigned int status, is_completed = 0, is_ready = 0;
> >  	unsigned int ready, cmd_done;
> >  
> >  	if (info->cs == 0) {
> > @@ -514,8 +521,9 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
> >  		is_completed = 1;
> >  	}
> >  	if (status & ready) {
> > -		info->is_ready = 1;
> > +		atomic_set(&info->is_ready, 1);
> 
> According to my suggestions, I don't think you need to set
> info->is_ready=1 here.
> 

Well, I'm not sure there's other place where this can be done, as it's the
only place where the status register is read. After this, the register
is cleared and the information is lost. (On the other side, see below)

I guess that if we could simply read the status at any time, this would be
much simpler (iow, we would read the status register in waitfunc).

> >  		info->state = STATE_READY;
> > +		is_ready = 1;
> >  	}
> >  
> >  	if (status & NDSR_WRCMDREQ) {
> > @@ -544,6 +552,8 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
> >  	nand_writel(info, NDSR, status);
> >  	if (is_completed)
> >  		complete(&info->cmd_complete);
> > +	if (is_ready)
> > +		complete(&info->dev_ready);
> >  NORMAL_IRQ_EXIT:
> >  	return IRQ_HANDLED;
> >  }
> > @@ -574,7 +584,6 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
> >  	info->oob_size		= 0;
> >  	info->use_ecc		= 0;
> >  	info->use_spare		= 1;
> > -	info->is_ready		= 0;
> >  	info->retcode		= ERR_NONE;
> >  	if (info->cs != 0)
> >  		info->ndcb0 = NDCB0_CSEL;
> > @@ -747,6 +756,8 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
> >  	exec_cmd = prepare_command_pool(info, command, column, page_addr);
> >  	if (exec_cmd) {
> >  		init_completion(&info->cmd_complete);
> > +		init_completion(&info->dev_ready);
> 
> Do you really need to init the completions each time you run a command?
> AIUI, the only reason you would need to do this is if you aren't
> matching up your calls to complete() and wait_for_completion*()
> properly, so that you simply dodge the issue and reset the completion
> count each time. This might be a result of the complexity of your
> 2-completion signalling design.
> 

Exactly, I need to reset any previous statue.

> > +		atomic_set(&info->is_ready, 0);
> >  		pxa3xx_nand_start(info);
> >  
> >  		ret = wait_for_completion_timeout(&info->cmd_complete,
> > @@ -859,21 +870,27 @@ static int pxa3xx_nand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
> >  {
> >  	struct pxa3xx_nand_host *host = mtd->priv;
> >  	struct pxa3xx_nand_info *info = host->info_data;
> > +	int ret;
> > +
> > +	/* Need to wait? */
> > +	if (!atomic_read(&info->is_ready)) {
> 
> This read of info->is_ready will no longer need to be atomic, because
> you never modify it from the IRQ context--only from the cmdfunc() and
> waitfunc().
> 

I see.

> > +		ret = wait_for_completion_timeout(&info->dev_ready,
> > +				CHIP_DELAY_TIMEOUT);
> 
> I think you can just do a (non-atomic) info->is_ready=1 here.
> 

The problem is: not every cmdfunc() is followed by a waitfunc(), right?
And even if it would be, cmdfunc() can be accesed from other places
(such as this same driver) so how do you guarantee cmdfunc() and
waitfunc() are called in pair?

Or maybe you're right and this is not an issue? I'm doing some tests
now and we'll report back later.

> > +		if (!ret) {
> > +			dev_err(&info->pdev->dev, "Ready time out!!!\n");
> > +			return NAND_STATUS_FAIL;
> > +		}
> > +	}
> >  
> >  	/* pxa3xx_nand_send_command has waited for command complete */
> >  	if (this->state == FL_WRITING || this->state == FL_ERASING) {
> >  		if (info->retcode == ERR_NONE)
> >  			return 0;
> > -		else {
> > -			/*
> > -			 * any error make it return 0x01 which will tell
> > -			 * the caller the erase and write fail
> > -			 */
> > -			return 0x01;
> > -		}
> > +		else
> > +			return NAND_STATUS_FAIL;
> >  	}
> >  
> > -	return 0;
> > +	return NAND_STATUS_READY;
> >  }
> >  
> >  static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
> > @@ -1026,7 +1043,7 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
> >  		return ret;
> >  
> >  	chip->cmdfunc(mtd, NAND_CMD_RESET, 0, 0);
> > -	if (info->is_ready)
> > +	if (atomic_read(&info->is_ready))
> 
> Does this need to wait on the dev_ready completion? I'm not sure I can
> guarantee there is no race on info->is_ready here, but then that means
> I'm not sure the code is correct even with the atomic read (which I
> don't think will be necessary, according to my suggestion above).
> 

Hm... yes, this probably should use instead of accessing is_ready.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 12/28] mtd: nand: pxa3xx: Use a completion to signal device ready

[Ezequiel Garcia]

On Tue, Nov 05, 2013 at 09:27:59PM -0300, Ezequiel Garcia wrote:
> > 
> > I believe your handling of this 'is_ready' bit is a little unwise, as
> > you are actually creating extra concurrency that is unneeded. I'll
> > summarize what you're doing with this 'is_ready' field:
> > 
> >   cmdfunc() -> sets info->is_ready=0 for appropriate commands
> >             -> kicks off the hardware
> > 
> > The following two sequences may then occur concurrently:
> > 
> >   (1) pxa3xx_nand_irq -> ready interrupt occurs
> >                       -> set info->is_ready=1
> > 		      -> signal 'dev_ready' completion
> > 
> >   (2) waitfunc -> check info->is_ready, if it is 0...
> >                   |_ ... wait for the dev_ready completion
> > 
> > Instead of setting info->is_ready=1 under (1), you could set it in (2),

So, after some more careful thought and some testings, it seems you're
correct. Setting is_read in waitfunc() results in a non-racy solution and
the atomic_t can go away.

I'll take a look at the other remaining issues and see about preparing
a v4, so we can try to converge quick enough for v3.14.

Thanks!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 13/28] mtd: nand: pxa3xx: Add bad block handling

[Ezequiel Garcia]

Add support for flash-based bad block table using Marvell's
custom in-flash bad block table layout. The support is enabled
a 'flash_bbt' platform data or device tree parameter.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c                | 33 +++++++++++++++++++++++++++
 include/linux/platform_data/mtd-nand-pxa3xx.h |  3 +++
 2 files changed, 36 insertions(+)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index e198c94..9a5913d 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_mtd.h>
 
 #if defined(CONFIG_ARCH_PXA) || defined(CONFIG_ARCH_MMP)
 #define ARCH_HAS_DMA
@@ -245,6 +246,29 @@ static struct pxa3xx_nand_flash builtin_flash_types[] = {
 { "256MiB 16-bit", 0xba20,  64, 2048, 16, 16, 2048, &timing[3] },
 };
 
+static u8 bbt_pattern[] = {'M', 'V', 'B', 'b', 't', '0' };
+static u8 bbt_mirror_pattern[] = {'1', 't', 'b', 'B', 'V', 'M' };
+
+static struct nand_bbt_descr bbt_main_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
+		| NAND_BBT_2BIT | NAND_BBT_VERSION,
+	.offs =	8,
+	.len = 6,
+	.veroffs = 14,
+	.maxblocks = 8,		/* Last 8 blocks in each chip */
+	.pattern = bbt_pattern
+};
+
+static struct nand_bbt_descr bbt_mirror_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
+		| NAND_BBT_2BIT | NAND_BBT_VERSION,
+	.offs =	8,
+	.len = 6,
+	.veroffs = 14,
+	.maxblocks = 8,		/* Last 8 blocks in each chip */
+	.pattern = bbt_mirror_pattern
+};
+
 /* Define a default flash type setting serve as flash detecting only */
 #define DEFAULT_FLASH_TYPE (&builtin_flash_types[0])
 
@@ -1128,6 +1152,14 @@ KEEP_CONFIG:
 
 	if (nand_scan_ident(mtd, 1, def))
 		return -ENODEV;
+
+	if (pdata->flash_bbt) {
+		chip->bbt_options |= NAND_BBT_USE_FLASH |
+				     NAND_BBT_NO_OOB_BBM;
+		chip->bbt_td = &bbt_main_descr;
+		chip->bbt_md = &bbt_mirror_descr;
+	}
+
 	/* calculate addressing information */
 	if (mtd->writesize >= 2048)
 		host->col_addr_cycles = 2;
@@ -1323,6 +1355,7 @@ static int pxa3xx_nand_probe_dt(struct platform_device *pdev)
 	if (of_get_property(np, "marvell,nand-keep-config", NULL))
 		pdata->keep_config = 1;
 	of_property_read_u32(np, "num-cs", &pdata->num_cs);
+	pdata->flash_bbt = of_get_nand_on_flash_bbt(np);
 
 	pdev->dev.platform_data = pdata;
 
diff --git a/include/linux/platform_data/mtd-nand-pxa3xx.h b/include/linux/platform_data/mtd-nand-pxa3xx.h
index ffb8019..a941471 100644
--- a/include/linux/platform_data/mtd-nand-pxa3xx.h
+++ b/include/linux/platform_data/mtd-nand-pxa3xx.h
@@ -55,6 +55,9 @@ struct pxa3xx_nand_platform_data {
 	/* indicate how many chip selects will be used */
 	int	num_cs;
 
+	/* use an flash-based bad block table */
+	bool	flash_bbt;
+
 	const struct mtd_partition		*parts[NUM_CHIP_SELECT];
 	unsigned int				nr_parts[NUM_CHIP_SELECT];
 
-- 
1.8.1.5

[PATCH v3 13/28] mtd: nand: pxa3xx: Add bad block handling

[Brian Norris]

Hi Ezequiel,

I wrote up some comments on your v2 series while on a plane Sunday, but
I didn't make time to send them out until now. Oh well.

On Tue, Nov 05, 2013 at 09:55:20AM -0300, Ezequiel Garcia wrote:
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -1128,6 +1152,14 @@ KEEP_CONFIG:
>  
>  	if (nand_scan_ident(mtd, 1, def))
>  		return -ENODEV;
> +
> +	if (pdata->flash_bbt) {
> +		chip->bbt_options |= NAND_BBT_USE_FLASH |
> +				     NAND_BBT_NO_OOB_BBM;

You're using NAND_BBT_NO_OOB_BBM? So you are unable to write bad block
markers to flash at all? Is this related to your independent patch for
trying to scan BBM from the data area? Could you instead write a
nand_chip.block_markbad() callback routine that would program BBM to the
appropriate data area?

Or, if you really want to avoid programming new BBMs, then you should
probably describe this decision in the patch description more clearly.

> +		chip->bbt_td = &bbt_main_descr;
> +		chip->bbt_md = &bbt_mirror_descr;
> +	}
> +
>  	/* calculate addressing information */
>  	if (mtd->writesize >= 2048)
>  		host->col_addr_cycles = 2;
> @@ -1323,6 +1355,7 @@ static int pxa3xx_nand_probe_dt(struct platform_device *pdev)
>  	if (of_get_property(np, "marvell,nand-keep-config", NULL))
>  		pdata->keep_config = 1;
>  	of_property_read_u32(np, "num-cs", &pdata->num_cs);
> +	pdata->flash_bbt = of_get_nand_on_flash_bbt(np);

Now that you're using the "nand-on-flash-bbt" property, you should
document it in Documentation/devicetree/bindings/mtd/pxa3xx-nand.txt
like the other drivers do. (It's already documented generically in
Documentation/.../nand.txt, but I think it's good practice to explicitly
note which drivers support the binding, since nand_base doesn't do this
generically for all NAND drivers.)

>  
>  	pdev->dev.platform_data = pdata;
>  

Brian

[PATCH v3 13/28] mtd: nand: pxa3xx: Add bad block handling

[Ezequiel Garcia]

On Tue, Nov 05, 2013 at 10:23:01AM -0800, Brian Norris wrote:
> Hi Ezequiel,
> 
> I wrote up some comments on your v2 series while on a plane Sunday, but
> I didn't make time to send them out until now. Oh well.
> 

No problem. I just wanted to push a new version with the minor fixes
from Huang to prevent from stalling.

> On Tue, Nov 05, 2013 at 09:55:20AM -0300, Ezequiel Garcia wrote:
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -1128,6 +1152,14 @@ KEEP_CONFIG:
> >  
> >  	if (nand_scan_ident(mtd, 1, def))
> >  		return -ENODEV;
> > +
> > +	if (pdata->flash_bbt) {
> > +		chip->bbt_options |= NAND_BBT_USE_FLASH |
> > +				     NAND_BBT_NO_OOB_BBM;
> 
> You're using NAND_BBT_NO_OOB_BBM? So you are unable to write bad block
> markers to flash at all? Is this related to your independent patch for
> trying to scan BBM from the data area?

Yes.

> Could you instead write a
> nand_chip.block_markbad() callback routine that would program BBM to the
> appropriate data area?
> 

No :-)

> Or, if you really want to avoid programming new BBMs, then you should
> probably describe this decision in the patch description more clearly.
> 

Right.

I'll have to describe a bunch of stuff about the controller so this
NO_OOB_BBM makes sense. Please bare with me and keep reading :)

The central issue and the main difficulty is the "splitted"
data/oob/data/oob way of regarding a page.

This is intrinsic to the hardware and we must learn to deal with it.
So, let's suppose we have 4K pages, and the manufacturer marks a block at
offset 4096 (the 'x' is offset 4096).

-----------------------------------------------
|                    Data           |x  OOB   |
-----------------------------------------------

When this same page is 'viewed' by the driver, and because of the
splitted layout, the data and OOB regions are now at different
locations. It would be something like this:

-----------------------------------------------
|      Data      | OOB |      Data   x  | OOB |
-----------------------------------------------

The offset *in the data region* depends in the controller configuration,
but considering we have a 32B and 30B ECC, the calculation would give:

 2048 + 2048 - 32 - 30 = 4034.

So, if I use nanddump to dump a page, I would have to look at offset
4034 to find the factory bad block marker.

For this reason, why need to use a customize bad block scanning.

In addition, this means under regular usage we will write such position
(since it belongs to the data region) and every used block is likely
to be marked as bad.

So, there's no point in marking a block as bad, because good blocks
are *also* mark as bad. We need to rely in the bad block table, and only
perform the scan in on the very first time (when the device is unused).

We're aware this sounds kind of crappy since we'll get completely screwed
in case the bad block table is somehow lost or corrupted, but we don't
care about such case.

Still, I'd like to know:

1. Do you think the bad block table could be corrupted or is this not
likely to ever happen?

2. Do you have any ideas to 'avoid' writing to the marker? or maybe to
otherwise scan the factory markers the first time, but then use some
other position for the kernel in-flash BB marker?

> > +		chip->bbt_td = &bbt_main_descr;
> > +		chip->bbt_md = &bbt_mirror_descr;
> > +	}
> > +
> >  	/* calculate addressing information */
> >  	if (mtd->writesize >= 2048)
> >  		host->col_addr_cycles = 2;
> > @@ -1323,6 +1355,7 @@ static int pxa3xx_nand_probe_dt(struct platform_device *pdev)
> >  	if (of_get_property(np, "marvell,nand-keep-config", NULL))
> >  		pdata->keep_config = 1;
> >  	of_property_read_u32(np, "num-cs", &pdata->num_cs);
> > +	pdata->flash_bbt = of_get_nand_on_flash_bbt(np);
> 
> Now that you're using the "nand-on-flash-bbt" property, you should
> document it in Documentation/devicetree/bindings/mtd/pxa3xx-nand.txt
> like the other drivers do. (It's already documented generically in
> Documentation/.../nand.txt, but I think it's good practice to explicitly
> note which drivers support the binding, since nand_base doesn't do this
> generically for all NAND drivers.)
>

Yeah, good idea.

Thanks,
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 13/28] mtd: nand: pxa3xx: Add bad block handling

[Brian Norris]

On Tue, Nov 5, 2013 at 3:40 PM, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> On Tue, Nov 05, 2013 at 10:23:01AM -0800, Brian Norris wrote:
>> On Tue, Nov 05, 2013 at 09:55:20AM -0300, Ezequiel Garcia wrote:
>> > --- a/drivers/mtd/nand/pxa3xx_nand.c
>> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
>> > @@ -1128,6 +1152,14 @@ KEEP_CONFIG:
>> >
>> >     if (nand_scan_ident(mtd, 1, def))
>> >             return -ENODEV;
>> > +
>> > +   if (pdata->flash_bbt) {
>> > +           chip->bbt_options |= NAND_BBT_USE_FLASH |
>> > +                                NAND_BBT_NO_OOB_BBM;
>>
>> You're using NAND_BBT_NO_OOB_BBM? So you are unable to write bad block
>> markers to flash at all? Is this related to your independent patch for
>> trying to scan BBM from the data area?
>
> Yes.
>
>> Could you instead write a
>> nand_chip.block_markbad() callback routine that would program BBM to the
>> appropriate data area?
>
> No :-)

Well given the reset of your comments, I guess you could write an
empty one (or one with a BUG() or WARN()?).

>> Or, if you really want to avoid programming new BBMs, then you should
>> probably describe this decision in the patch description more clearly.
>>
>
> Right.
>
> I'll have to describe a bunch of stuff about the controller so this
> NO_OOB_BBM makes sense. Please bare with me and keep reading :)

[snip nice description; I did read it!]

> So, there's no point in marking a block as bad, because good blocks
> are *also* mark as bad. We need to rely in the bad block table, and only
> perform the scan in on the very first time (when the device is unused).

Right. I didn't quite think through this whole process.

I think a short (few lines) comment in the code to describe the
justification for using NAND_BBT_NO_OOB_BBM is in order for v4. And
maybe include a bit of this in the commit message as well.

> We're aware this sounds kind of crappy since we'll get completely screwed
> in case the bad block table is somehow lost or corrupted, but we don't
> care about such case.
>
> Still, I'd like to know:
>
> 1. Do you think the bad block table could be corrupted or is this not
> likely to ever happen?

Yes, it can be. But no, I don't think it's likely. There are very few,
rare instances where we have to modify the BBT (and thereby make it
susceptible to corruption), and those instances have some level of
robustness to them. Of course, they still have room for improvement.
(I suppose there could as be corruption due to read disturb; but this
is also handled now, by scrubbing the affected BBT blocks that return
-EUCLEAN, refreshing them with clean data.)

Personally, I've experienced "corruption" primarily when I have boards
where I change the ECC configuration; then the BBT scan sees -EBADMSG
and has to rebuild the table.

> 2. Do you have any ideas to 'avoid' writing to the marker? or maybe to
> otherwise scan the factory markers the first time, but then use some
> other position for the kernel in-flash BB marker?

Hmm, not really. We've kind of co-opted the idea of the factory bad
block marker as a secondary, distributed bad block table in the
kernel. It's not really the expected use case, and it breaks in cases
like yours, since we don't support a third form of bad block
marker/table for post-initial-scan markers. I think the best solution
in this case is just to rely solely on the BBT after the first scan.

Brian

[PATCH v3 14/28] mtd: nand: pxa3xx: Add driver-specific ECC BCH support

[Ezequiel Garcia]

This commit adds the BCH ECC support available in NFCv2 controller.
Depending on the detected required strength the respective ECC layout
is selected.

This commit adds an empty ECC layout, since support to access large
pages is first required. Once that support is added, a proper ECC
layout will be added as well.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 88 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 71 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 9a5913d..92abe7c 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -56,6 +56,7 @@
 #define NDPCR		(0x18) /* Page Count Register */
 #define NDBDR0		(0x1C) /* Bad Block Register 0 */
 #define NDBDR1		(0x20) /* Bad Block Register 1 */
+#define NDECCCTRL	(0x28) /* ECC control */
 #define NDDB		(0x40) /* Data Buffer */
 #define NDCB0		(0x48) /* Command Buffer0 */
 #define NDCB1		(0x4C) /* Command Buffer1 */
@@ -196,6 +197,7 @@ struct pxa3xx_nand_info {
 
 	int			cs;
 	int			use_ecc;	/* use HW ECC ? */
+	int			ecc_bch;	/* using BCH ECC? */
 	int			use_dma;	/* use DMA ? */
 	int			use_spare;	/* use spare ? */
 
@@ -209,6 +211,8 @@ struct pxa3xx_nand_info {
 	unsigned int		fifo_size;	/* max. data size in the FIFO */
 	unsigned int		data_size;	/* data to be read from FIFO */
 	unsigned int		oob_size;
+	unsigned int		spare_size;
+	unsigned int		ecc_size;
 	int 			retcode;
 
 	/* cached register value */
@@ -343,19 +347,12 @@ static void pxa3xx_set_datasize(struct pxa3xx_nand_info *info)
 	int oob_enable = info->reg_ndcr & NDCR_SPARE_EN;
 
 	info->data_size = info->fifo_size;
-	if (!oob_enable) {
-		info->oob_size = 0;
+	if (!oob_enable)
 		return;
-	}
 
-	switch (info->fifo_size) {
-	case 2048:
-		info->oob_size = (info->use_ecc) ? 40 : 64;
-		break;
-	case 512:
-		info->oob_size = (info->use_ecc) ? 8 : 16;
-		break;
-	}
+	info->oob_size = info->spare_size;
+	if (!info->use_ecc)
+		info->oob_size += info->ecc_size;
 }
 
 /**
@@ -370,10 +367,15 @@ static void pxa3xx_nand_start(struct pxa3xx_nand_info *info)
 
 	ndcr = info->reg_ndcr;
 
-	if (info->use_ecc)
+	if (info->use_ecc) {
 		ndcr |= NDCR_ECC_EN;
-	else
+		if (info->ecc_bch)
+			nand_writel(info, NDECCCTRL, 0x1);
+	} else {
 		ndcr &= ~NDCR_ECC_EN;
+		if (info->ecc_bch)
+			nand_writel(info, NDECCCTRL, 0x0);
+	}
 
 	if (info->use_dma)
 		ndcr |= NDCR_DMA_EN;
@@ -1073,6 +1075,43 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
 	return -ENODEV;
 }
 
+static int pxa_ecc_init(struct pxa3xx_nand_info *info,
+			struct nand_ecc_ctrl *ecc,
+			int strength, int page_size)
+{
+	/*
+	 * We don't use strength here as the PXA variant
+	 * is used with non-ONFI compliant devices.
+	 */
+	if (page_size == 2048) {
+		ecc->mode = NAND_ECC_HW;
+		ecc->size = 512;
+		ecc->strength = 1;
+
+		info->spare_size = 40;
+		info->ecc_size = 24;
+		return 1;
+
+	} else if (page_size == 512) {
+		ecc->mode = NAND_ECC_HW;
+		ecc->size = 512;
+		ecc->strength = 1;
+
+		info->spare_size = 8;
+		info->ecc_size = 8;
+		return 1;
+	}
+	return 0;
+}
+
+static int armada370_ecc_init(struct pxa3xx_nand_info *info,
+			      struct nand_ecc_ctrl *ecc,
+			      int strength, int page_size)
+{
+	/* Unimplemented yet */
+	return 0;
+}
+
 static int pxa3xx_nand_scan(struct mtd_info *mtd)
 {
 	struct pxa3xx_nand_host *host = mtd->priv;
@@ -1143,13 +1182,13 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
 	pxa3xx_flash_ids[1].name = NULL;
 	def = pxa3xx_flash_ids;
 KEEP_CONFIG:
-	chip->ecc.mode = NAND_ECC_HW;
-	chip->ecc.size = info->fifo_size;
-	chip->ecc.strength = 1;
-
 	if (info->reg_ndcr & NDCR_DWIDTH_M)
 		chip->options |= NAND_BUSWIDTH_16;
 
+	/* Device detection must be done with ECC disabled */
+	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
+		nand_writel(info, NDECCCTRL, 0x0);
+
 	if (nand_scan_ident(mtd, 1, def))
 		return -ENODEV;
 
@@ -1160,6 +1199,21 @@ KEEP_CONFIG:
 		chip->bbt_md = &bbt_mirror_descr;
 	}
 
+	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
+		ret = armada370_ecc_init(info, &chip->ecc,
+				   chip->ecc_strength_ds,
+				   mtd->writesize);
+	else
+		ret = pxa_ecc_init(info, &chip->ecc,
+				   chip->ecc_strength_ds,
+				   mtd->writesize);
+	if (!ret) {
+		dev_err(&info->pdev->dev,
+			"ECC strength %d at page size %d is not supported\n",
+			chip->ecc_strength_ds, mtd->writesize);
+		return -ENODEV;
+	}
+
 	/* calculate addressing information */
 	if (mtd->writesize >= 2048)
 		host->col_addr_cycles = 2;
-- 
1.8.1.5

[PATCH v3 14/28] mtd: nand: pxa3xx: Add driver-specific ECC BCH support

[Brian Norris]

On Tue, Nov 05, 2013 at 09:55:21AM -0300, Ezequiel Garcia wrote:
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -1073,6 +1075,43 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
>  	return -ENODEV;
>  }
>  
> +static int pxa_ecc_init(struct pxa3xx_nand_info *info,
> +			struct nand_ecc_ctrl *ecc,
> +			int strength, int page_size)
> +{
> +	/*
> +	 * We don't use strength here as the PXA variant
> +	 * is used with non-ONFI compliant devices.

Hmm, rather than assuming you never get any relevant 'strength' info
from nand_base, could you check if it is zero and only ignore it if it
is zero? But if it is non-zero, you could at least use it to validate
the strength settings below.

At the same time, I would also caution against fragile ECC determination
methods that can indirectly cause regressions in the future, such as
with Huang's recent GPMI ECC layout regressions. Perhaps you'll want a
DT binding for the ECC strength to future proof this? (Not a
requirement; just a thought.)

> +	 */
> +	if (page_size == 2048) {
> +		ecc->mode = NAND_ECC_HW;
> +		ecc->size = 512;
> +		ecc->strength = 1;

I assume this is actually a 1-bit ECC, not just a dummy value? Note that
the ecc->strength and ecc->size fields are used by nand_base for some
bitflip calculations and are also exposed via sysfs.

> +
> +		info->spare_size = 40;
> +		info->ecc_size = 24;
> +		return 1;
> +
> +	} else if (page_size == 512) {
> +		ecc->mode = NAND_ECC_HW;
> +		ecc->size = 512;
> +		ecc->strength = 1;
> +
> +		info->spare_size = 8;
> +		info->ecc_size = 8;
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +static int armada370_ecc_init(struct pxa3xx_nand_info *info,
> +			      struct nand_ecc_ctrl *ecc,
> +			      int strength, int page_size)
> +{
> +	/* Unimplemented yet */
> +	return 0;
> +}
> +
>  static int pxa3xx_nand_scan(struct mtd_info *mtd)
>  {
>  	struct pxa3xx_nand_host *host = mtd->priv;

Brian

[PATCH v3 14/28] mtd: nand: pxa3xx: Add driver-specific ECC BCH support

[Ezequiel Garcia]

On Tue, Nov 05, 2013 at 10:31:56AM -0800, Brian Norris wrote:
> On Tue, Nov 05, 2013 at 09:55:21AM -0300, Ezequiel Garcia wrote:
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -1073,6 +1075,43 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
> >  	return -ENODEV;
> >  }
> >  
> > +static int pxa_ecc_init(struct pxa3xx_nand_info *info,
> > +			struct nand_ecc_ctrl *ecc,
> > +			int strength, int page_size)
> > +{
> > +	/*
> > +	 * We don't use strength here as the PXA variant
> > +	 * is used with non-ONFI compliant devices.
> 
> Hmm, rather than assuming you never get any relevant 'strength' info
> from nand_base, could you check if it is zero and only ignore it if it
> is zero? But if it is non-zero, you could at least use it to validate
> the strength settings below.
> 
> At the same time, I would also caution against fragile ECC determination
> methods that can indirectly cause regressions in the future, such as
> with Huang's recent GPMI ECC layout regressions. Perhaps you'll want a
> DT binding for the ECC strength to future proof this? (Not a
> requirement; just a thought.)
> 

Well, for the PXA path, as I don't have this hardware enabled to test
I try do clean-ups and change but without *functionality changes*.

So, you'll notice this pxa_ecc_init() is a different way of doing the
old initialization:

       chip->ecc.mode = NAND_ECC_HW;
       chip->ecc.size = info->fifo_size;
       chip->ecc.strength = 1;

And in fact...

> > +	 */
> > +	if (page_size == 2048) {
> > +		ecc->mode = NAND_ECC_HW;
> > +		ecc->size = 512;

To match the previous code, this should be "ecc->size = 2048"! :/

> > +		ecc->strength = 1;
> 
> I assume this is actually a 1-bit ECC, not just a dummy value? Note that
> the ecc->strength and ecc->size fields are used by nand_base for some
> bitflip calculations and are also exposed via sysfs.
> 

Right. Well, as I said: I tried not to change anything that I'm not able
to fully test.

> > +
> > +		info->spare_size = 40;
> > +		info->ecc_size = 24;
> > +		return 1;
> > +
> > +	} else if (page_size == 512) {
> > +		ecc->mode = NAND_ECC_HW;
> > +		ecc->size = 512;
> > +		ecc->strength = 1;
> > +
> > +		info->spare_size = 8;
> > +		info->ecc_size = 8;
> > +		return 1;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int armada370_ecc_init(struct pxa3xx_nand_info *info,
> > +			      struct nand_ecc_ctrl *ecc,
> > +			      int strength, int page_size)
> > +{
> > +	/* Unimplemented yet */
> > +	return 0;
> > +}
> > +
> >  static int pxa3xx_nand_scan(struct mtd_info *mtd)
> >  {
> >  	struct pxa3xx_nand_host *host = mtd->priv;

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 15/28] mtd: nand: pxa3xx: Clear cmd buffer #3 (NDCB3) on command start

[Ezequiel Garcia]

Command buffer #3 is not properly cleared and it keeps the last
set value. Fix this by clearing when a command is setup.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 92abe7c..6544300 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -611,6 +611,7 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	info->use_ecc		= 0;
 	info->use_spare		= 1;
 	info->retcode		= ERR_NONE;
+	info->ndcb3		= 0;
 	if (info->cs != 0)
 		info->ndcb0 = NDCB0_CSEL;
 	else
@@ -632,7 +633,6 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	default:
 		info->ndcb1 = 0;
 		info->ndcb2 = 0;
-		info->ndcb3 = 0;
 		break;
 	}
 
-- 
1.8.1.5

[PATCH v3 16/28] mtd: nand: pxa3xx: Add helper function to set page address

[Ezequiel Garcia]

Let's simplify the code by first introducing a helper function
to set the page address, as done by the READ0, READOOB and SEQIN
commands.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 6544300..7cc67b0 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -592,6 +592,26 @@ static inline int is_buf_blank(uint8_t *buf, size_t len)
 	return 1;
 }
 
+static void set_command_address(struct pxa3xx_nand_info *info,
+		unsigned int page_size, uint16_t column, int page_addr)
+{
+	/* small page addr setting */
+	if (page_size < PAGE_CHUNK_SIZE) {
+		info->ndcb1 = ((page_addr & 0xFFFFFF) << 8)
+				| (column & 0xFF);
+
+		info->ndcb2 = 0;
+	} else {
+		info->ndcb1 = ((page_addr & 0xFFFF) << 16)
+				| (column & 0xFFFF);
+
+		if (page_addr & 0xFF0000)
+			info->ndcb2 = (page_addr & 0xFF0000) >> 16;
+		else
+			info->ndcb2 = 0;
+	}
+}
+
 static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 		uint16_t column, int page_addr)
 {
@@ -655,22 +675,8 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 			info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8);
 
 	case NAND_CMD_SEQIN:
-		/* small page addr setting */
-		if (unlikely(mtd->writesize < PAGE_CHUNK_SIZE)) {
-			info->ndcb1 = ((page_addr & 0xFFFFFF) << 8)
-					| (column & 0xFF);
-
-			info->ndcb2 = 0;
-		} else {
-			info->ndcb1 = ((page_addr & 0xFFFF) << 16)
-					| (column & 0xFFFF);
-
-			if (page_addr & 0xFF0000)
-				info->ndcb2 = (page_addr & 0xFF0000) >> 16;
-			else
-				info->ndcb2 = 0;
-		}
 
+		set_command_address(info, mtd->writesize, column, page_addr);
 		info->buf_count = mtd->writesize + mtd->oobsize;
 		memset(info->data_buff, 0xFF, info->buf_count);
 
-- 
1.8.1.5

[PATCH v3 17/28] mtd: nand: pxa3xx: Remove READ0 switch/case falltrough

[Ezequiel Garcia]

READ0 and READOOB command preparation has a falltrough to SEQIN
case, where the command address is specified.
This is certainly confusing and makes the code less readable with
no added value. Let's remove it.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 7cc67b0..13047b5 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -674,6 +674,11 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 		if (mtd->writesize >= PAGE_CHUNK_SIZE)
 			info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8);
 
+		set_command_address(info, mtd->writesize, column, page_addr);
+		info->buf_count = mtd->writesize + mtd->oobsize;
+		memset(info->data_buff, 0xFF, info->buf_count);
+		break;
+
 	case NAND_CMD_SEQIN:
 
 		set_command_address(info, mtd->writesize, column, page_addr);
-- 
1.8.1.5

[PATCH v3 18/28] mtd: nand: pxa3xx: Split prepare_command_pool() in two stages

[Ezequiel Garcia]

This commit splits the prepare_command_pool() function into two
stages: prepare_start_command() / prepare_set_command().

This is a preparation patch without any functionality changes,
and is meant to allow support fir multiple page reading/writing
operations.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 44 ++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 13047b5..82aa1b9 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -612,18 +612,8 @@ static void set_command_address(struct pxa3xx_nand_info *info,
 	}
 }
 
-static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
-		uint16_t column, int page_addr)
+static void prepare_start_command(struct pxa3xx_nand_info *info, int command)
 {
-	int addr_cycle, exec_cmd;
-	struct pxa3xx_nand_host *host;
-	struct mtd_info *mtd;
-
-	host = info->host[info->cs];
-	mtd = host->mtd;
-	addr_cycle = 0;
-	exec_cmd = 1;
-
 	/* reset data and oob column point to handle data */
 	info->buf_start		= 0;
 	info->buf_count		= 0;
@@ -632,10 +622,6 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	info->use_spare		= 1;
 	info->retcode		= ERR_NONE;
 	info->ndcb3		= 0;
-	if (info->cs != 0)
-		info->ndcb0 = NDCB0_CSEL;
-	else
-		info->ndcb0 = 0;
 
 	switch (command) {
 	case NAND_CMD_READ0:
@@ -647,14 +633,32 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	case NAND_CMD_PARAM:
 		info->use_spare = 0;
 		break;
-	case NAND_CMD_SEQIN:
-		exec_cmd = 0;
-		break;
 	default:
 		info->ndcb1 = 0;
 		info->ndcb2 = 0;
 		break;
 	}
+}
+
+static int prepare_set_command(struct pxa3xx_nand_info *info, int command,
+		uint16_t column, int page_addr)
+{
+	int addr_cycle, exec_cmd;
+	struct pxa3xx_nand_host *host;
+	struct mtd_info *mtd;
+
+	host = info->host[info->cs];
+	mtd = host->mtd;
+	addr_cycle = 0;
+	exec_cmd = 1;
+
+	if (info->cs != 0)
+		info->ndcb0 = NDCB0_CSEL;
+	else
+		info->ndcb0 = 0;
+
+	if (command == NAND_CMD_SEQIN)
+		exec_cmd = 0;
 
 	addr_cycle = NDCB0_ADDR_CYC(host->row_addr_cycles
 				    + host->col_addr_cycles);
@@ -789,8 +793,10 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
 		nand_writel(info, NDTR1CS0, info->ndtr1cs0);
 	}
 
+	prepare_start_command(info, command);
+
 	info->state = STATE_PREPARED;
-	exec_cmd = prepare_command_pool(info, command, column, page_addr);
+	exec_cmd = prepare_set_command(info, command, column, page_addr);
 	if (exec_cmd) {
 		init_completion(&info->cmd_complete);
 		init_completion(&info->dev_ready);
-- 
1.8.1.5

[PATCH v3 18/28] mtd: nand: pxa3xx: Split prepare_command_pool() in two stages

[Brian Norris]

On Tue, Nov 05, 2013 at 09:55:25AM -0300, Ezequiel Garcia wrote:
> This commit splits the prepare_command_pool() function into two
> stages: prepare_start_command() / prepare_set_command().
> 
> This is a preparation patch without any functionality changes,
> and is meant to allow support fir multiple page reading/writing

s/fir/for/

> operations.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Brian

[PATCH v3 19/28] mtd: nand: pxa3xx: Move the data buffer clean to prepare_start_command()

[Ezequiel Garcia]

To allow future support of multiple page reading/writing, move the data
buffer clean out of prepare_set_command().

This is done to prevent the data buffer from being cleaned on every command
preparation, when a multiple command sequence is implemented to read/write
pages larger than the FIFO size (2 KiB).

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 82aa1b9..2e6654c 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -614,6 +614,9 @@ static void set_command_address(struct pxa3xx_nand_info *info,
 
 static void prepare_start_command(struct pxa3xx_nand_info *info, int command)
 {
+	struct pxa3xx_nand_host *host = info->host[info->cs];
+	struct mtd_info *mtd = host->mtd;
+
 	/* reset data and oob column point to handle data */
 	info->buf_start		= 0;
 	info->buf_count		= 0;
@@ -638,6 +641,19 @@ static void prepare_start_command(struct pxa3xx_nand_info *info, int command)
 		info->ndcb2 = 0;
 		break;
 	}
+
+	/*
+	 * If we are about to isse a read command, or about to set
+	 * the write address, then clean the data buffer.
+	 */
+	if (command == NAND_CMD_READ0 ||
+	    command == NAND_CMD_READOOB ||
+	    command == NAND_CMD_SEQIN) {
+
+		info->buf_count = mtd->writesize + mtd->oobsize;
+		memset(info->data_buff, 0xFF, info->buf_count);
+	}
+
 }
 
 static int prepare_set_command(struct pxa3xx_nand_info *info, int command,
@@ -679,16 +695,11 @@ static int prepare_set_command(struct pxa3xx_nand_info *info, int command,
 			info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8);
 
 		set_command_address(info, mtd->writesize, column, page_addr);
-		info->buf_count = mtd->writesize + mtd->oobsize;
-		memset(info->data_buff, 0xFF, info->buf_count);
 		break;
 
 	case NAND_CMD_SEQIN:
 
 		set_command_address(info, mtd->writesize, column, page_addr);
-		info->buf_count = mtd->writesize + mtd->oobsize;
-		memset(info->data_buff, 0xFF, info->buf_count);
-
 		break;
 
 	case NAND_CMD_PAGEPROG:
-- 
1.8.1.5

[PATCH v3 20/28] mtd: nand: pxa3xx: Fix SEQIN column address set

[Ezequiel Garcia]

This commit adds support page programming with a non-zero "column"
address setting. This is important to support OOB writing, through
command sequences such as:

  cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, ofs);
  write_buf(mtd, oob_buf, 6);
  cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 2e6654c..4999c3d 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -699,7 +699,8 @@ static int prepare_set_command(struct pxa3xx_nand_info *info, int command,
 
 	case NAND_CMD_SEQIN:
 
-		set_command_address(info, mtd->writesize, column, page_addr);
+		info->buf_start = column;
+		set_command_address(info, mtd->writesize, 0, page_addr);
 		break;
 
 	case NAND_CMD_PAGEPROG:
-- 
1.8.1.5

[PATCH v3 21/28] mtd: nand: pxa3xx: Add a read/write buffers markers

[Ezequiel Garcia]

In preparation to support multiple (aka chunked, aka splitted)
page I/O, this commit adds 'data_buff_pos' and 'oob_buff_pos' fields
to keep track of where the next read (or write) should be done.

This will allow multiple calls to handle_data_pio() to continue
the read (or write) operation.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 4999c3d..9b42832 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -174,6 +174,8 @@ struct pxa3xx_nand_info {
 	unsigned int 		buf_start;
 	unsigned int		buf_count;
 	unsigned int		buf_size;
+	unsigned int		data_buff_pos;
+	unsigned int		oob_buff_pos;
 
 	/* DMA information */
 	int			drcmr_dat;
@@ -342,11 +344,12 @@ static void pxa3xx_nand_set_timing(struct pxa3xx_nand_host *host,
  * spare and ECC configuration.
  * Only applicable to READ0, READOOB and PAGEPROG commands.
  */
-static void pxa3xx_set_datasize(struct pxa3xx_nand_info *info)
+static void pxa3xx_set_datasize(struct pxa3xx_nand_info *info,
+				struct mtd_info *mtd)
 {
 	int oob_enable = info->reg_ndcr & NDCR_SPARE_EN;
 
-	info->data_size = info->fifo_size;
+	info->data_size = mtd->writesize;
 	if (!oob_enable)
 		return;
 
@@ -434,26 +437,39 @@ static void disable_int(struct pxa3xx_nand_info *info, uint32_t int_mask)
 
 static void handle_data_pio(struct pxa3xx_nand_info *info)
 {
+	unsigned int do_bytes = min(info->data_size, info->fifo_size);
+
 	switch (info->state) {
 	case STATE_PIO_WRITING:
-		__raw_writesl(info->mmio_base + NDDB, info->data_buff,
-				DIV_ROUND_UP(info->data_size, 4));
+		__raw_writesl(info->mmio_base + NDDB,
+			      info->data_buff + info->data_buff_pos,
+			      DIV_ROUND_UP(do_bytes, 4));
+
 		if (info->oob_size > 0)
-			__raw_writesl(info->mmio_base + NDDB, info->oob_buff,
-					DIV_ROUND_UP(info->oob_size, 4));
+			__raw_writesl(info->mmio_base + NDDB,
+				      info->oob_buff + info->oob_buff_pos,
+				      DIV_ROUND_UP(info->oob_size, 4));
 		break;
 	case STATE_PIO_READING:
-		__raw_readsl(info->mmio_base + NDDB, info->data_buff,
-				DIV_ROUND_UP(info->data_size, 4));
+		__raw_readsl(info->mmio_base + NDDB,
+			     info->data_buff + info->data_buff_pos,
+			     DIV_ROUND_UP(do_bytes, 4));
+
 		if (info->oob_size > 0)
-			__raw_readsl(info->mmio_base + NDDB, info->oob_buff,
-					DIV_ROUND_UP(info->oob_size, 4));
+			__raw_readsl(info->mmio_base + NDDB,
+				     info->oob_buff + info->oob_buff_pos,
+				     DIV_ROUND_UP(info->oob_size, 4));
 		break;
 	default:
 		dev_err(&info->pdev->dev, "%s: invalid state %d\n", __func__,
 				info->state);
 		BUG();
 	}
+
+	/* Update buffer pointers for multi-page read/write */
+	info->data_buff_pos += do_bytes;
+	info->oob_buff_pos += info->oob_size;
+	info->data_size -= do_bytes;
 }
 
 #ifdef ARCH_HAS_DMA
@@ -621,6 +637,8 @@ static void prepare_start_command(struct pxa3xx_nand_info *info, int command)
 	info->buf_start		= 0;
 	info->buf_count		= 0;
 	info->oob_size		= 0;
+	info->data_buff_pos	= 0;
+	info->oob_buff_pos	= 0;
 	info->use_ecc		= 0;
 	info->use_spare		= 1;
 	info->retcode		= ERR_NONE;
@@ -631,7 +649,7 @@ static void prepare_start_command(struct pxa3xx_nand_info *info, int command)
 	case NAND_CMD_PAGEPROG:
 		info->use_ecc = 1;
 	case NAND_CMD_READOOB:
-		pxa3xx_set_datasize(info);
+		pxa3xx_set_datasize(info, mtd);
 		break;
 	case NAND_CMD_PARAM:
 		info->use_spare = 0;
-- 
1.8.1.5

[PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Ezequiel Garcia]

As preparation work to fully support large pages, this commit adds
the initial infrastructure to support splitted (aka chunked) I/O
operation. This commit adds support for read, and follow-up patches
will add write support.

When a read (aka READ0) command is issued, the driver loops issuing
the same command until all the requested data is transfered, changing
the 'extended' command field as needed.

For instance, if the driver is required to read a 4 KiB page, using a
chunk size of 2 KiB, the transaction is splitted in:
1. Monolithic read, first 2 KiB page chunk is read
2. Last naked read, second and last 2KiB page chunk is read

If ECC is enabled it is calculated on each chunk transfered and added
at a controller-fixed location after the data chunk that must be
spare area.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 192 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 185 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 9b42832..b9acd66 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -7,6 +7,20 @@
  * 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.
+ *
+ * Large pages implementation
+ * --------------------------
+ *
+ * NAND stack expects data in the following format:
+ * ---------------------------
+ * | Data (4K) | Spare | ECC |
+ * ---------------------------
+ *
+ * While NAND controller expects data to be in the following format:
+ * -----------------------------------------------------
+ * | Data (2K) | Spare | ECC | Data (2K) | Spare | ECC |
+ * -----------------------------------------------------
+ *
  */
 
 #include <linux/kernel.h>
@@ -101,6 +115,8 @@
 #define NDCB0_ST_ROW_EN         (0x1 << 26)
 #define NDCB0_AUTO_RS		(0x1 << 25)
 #define NDCB0_CSEL		(0x1 << 24)
+#define NDCB0_EXT_CMD_TYPE_MASK	(0x7 << 29)
+#define NDCB0_EXT_CMD_TYPE(x)	(((x) << 29) & NDCB0_EXT_CMD_TYPE_MASK)
 #define NDCB0_CMD_TYPE_MASK	(0x7 << 21)
 #define NDCB0_CMD_TYPE(x)	(((x) << 21) & NDCB0_CMD_TYPE_MASK)
 #define NDCB0_NC		(0x1 << 20)
@@ -111,6 +127,14 @@
 #define NDCB0_CMD1_MASK		(0xff)
 #define NDCB0_ADDR_CYC_SHIFT	(16)
 
+#define EXT_CMD_TYPE_DISPATCH	6 /* Command dispatch */
+#define EXT_CMD_TYPE_NAKED_RW	5 /* Naked read or Naked write */
+#define EXT_CMD_TYPE_READ	4 /* Read */
+#define EXT_CMD_TYPE_DISP_WR	4 /* Command dispatch with write */
+#define EXT_CMD_TYPE_FINAL	3 /* Final command */
+#define EXT_CMD_TYPE_LAST_RW	1 /* Last naked read/write */
+#define EXT_CMD_TYPE_MONO	0 /* Monolithic read/write */
+
 /* macros for registers read/write */
 #define nand_writel(info, off, val)	\
 	__raw_writel((val), (info)->mmio_base + (off))
@@ -212,6 +236,7 @@ struct pxa3xx_nand_info {
 
 	unsigned int		fifo_size;	/* max. data size in the FIFO */
 	unsigned int		data_size;	/* data to be read from FIFO */
+	unsigned int		chunk_size;	/* split commands chunk size */
 	unsigned int		oob_size;
 	unsigned int		spare_size;
 	unsigned int		ecc_size;
@@ -275,6 +300,31 @@ static struct nand_bbt_descr bbt_mirror_descr = {
 	.pattern = bbt_mirror_pattern
 };
 
+static struct nand_ecclayout ecc_layout_4KB_bch4bit = {
+	.eccbytes = 64,
+	.eccpos = {
+		32,  33,  34,  35,  36,  37,  38,  39,
+		40,  41,  42,  43,  44,  45,  46,  47,
+		48,  49,  50,  51,  52,  53,  54,  55,
+		56,  57,  58,  59,  60,  61,  62,  63,
+		96,  97,  98,  99,  100, 101, 102, 103,
+		104, 105, 106, 107, 108, 109, 110, 111,
+		112, 113, 114, 115, 116, 117, 118, 119,
+		120, 121, 122, 123, 124, 125, 126, 127},
+	/* Bootrom looks in bytes 0 & 5 for bad blocks */
+	.oobfree = { {6, 26}, { 64, 32} }
+};
+
+static struct nand_ecclayout ecc_layout_4KB_bch8bit = {
+	.eccbytes = 128,
+	.eccpos = {
+		32,  33,  34,  35,  36,  37,  38,  39,
+		40,  41,  42,  43,  44,  45,  46,  47,
+		48,  49,  50,  51,  52,  53,  54,  55,
+		56,  57,  58,  59,  60,  61,  62,  63},
+	.oobfree = { }
+};
+
 /* Define a default flash type setting serve as flash detecting only */
 #define DEFAULT_FLASH_TYPE (&builtin_flash_types[0])
 
@@ -437,7 +487,7 @@ static void disable_int(struct pxa3xx_nand_info *info, uint32_t int_mask)
 
 static void handle_data_pio(struct pxa3xx_nand_info *info)
 {
-	unsigned int do_bytes = min(info->data_size, info->fifo_size);
+	unsigned int do_bytes = min(info->data_size, info->chunk_size);
 
 	switch (info->state) {
 	case STATE_PIO_WRITING:
@@ -675,7 +725,7 @@ static void prepare_start_command(struct pxa3xx_nand_info *info, int command)
 }
 
 static int prepare_set_command(struct pxa3xx_nand_info *info, int command,
-		uint16_t column, int page_addr)
+		int ext_cmd_type, uint16_t column, int page_addr)
 {
 	int addr_cycle, exec_cmd;
 	struct pxa3xx_nand_host *host;
@@ -708,9 +758,20 @@ static int prepare_set_command(struct pxa3xx_nand_info *info, int command,
 		if (command == NAND_CMD_READOOB)
 			info->buf_start += mtd->writesize;
 
-		/* Second command setting for large pages */
-		if (mtd->writesize >= PAGE_CHUNK_SIZE)
+		/*
+		 * Multiple page read needs an 'extended command type' field,
+		 * which is either naked-read or last-read according to the
+		 * state.
+		 */
+		if (mtd->writesize == PAGE_CHUNK_SIZE) {
 			info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8);
+		} else if (mtd->writesize > PAGE_CHUNK_SIZE) {
+			info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8)
+					| NDCB0_LEN_OVRD
+					| NDCB0_EXT_CMD_TYPE(ext_cmd_type);
+			info->ndcb3 = info->chunk_size +
+				      info->oob_size;
+		}
 
 		set_command_address(info, mtd->writesize, column, page_addr);
 		break;
@@ -826,7 +887,8 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
 	prepare_start_command(info, command);
 
 	info->state = STATE_PREPARED;
-	exec_cmd = prepare_set_command(info, command, column, page_addr);
+	exec_cmd = prepare_set_command(info, command, -1, column, page_addr);
+
 	if (exec_cmd) {
 		init_completion(&info->cmd_complete);
 		init_completion(&info->dev_ready);
@@ -844,6 +906,91 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
 	info->state = STATE_IDLE;
 }
 
+static void armada370_nand_cmdfunc(struct mtd_info *mtd,
+				   const unsigned command,
+				   int column, int page_addr)
+{
+	struct pxa3xx_nand_host *host = mtd->priv;
+	struct pxa3xx_nand_info *info = host->info_data;
+	int ret, exec_cmd, ext_cmd_type;
+
+	/*
+	 * if this is a x16 device ,then convert the input
+	 * "byte" address into a "word" address appropriate
+	 * for indexing a word-oriented device
+	 */
+	if (info->reg_ndcr & NDCR_DWIDTH_M)
+		column /= 2;
+
+	/*
+	 * There may be different NAND chip hooked to
+	 * different chip select, so check whether
+	 * chip select has been changed, if yes, reset the timing
+	 */
+	if (info->cs != host->cs) {
+		info->cs = host->cs;
+		nand_writel(info, NDTR0CS0, info->ndtr0cs0);
+		nand_writel(info, NDTR1CS0, info->ndtr1cs0);
+	}
+
+	/* Select the extended command for the first command */
+	switch (command) {
+	case NAND_CMD_READ0:
+	case NAND_CMD_READOOB:
+		ext_cmd_type = EXT_CMD_TYPE_MONO;
+		break;
+	}
+
+	prepare_start_command(info, command);
+
+	/*
+	 * Prepare the "is ready" completion before starting a command
+	 * transaction sequence. If the command is not executed the
+	 * completion will be completed, see below.
+	 *
+	 * We can do that inside the loop because the command variable
+	 * is invariant and thus so is the exec_cmd.
+	 */
+	atomic_set(&info->is_ready, 0);
+	init_completion(&info->dev_ready);
+	do {
+		info->state = STATE_PREPARED;
+		exec_cmd = prepare_set_command(info, command, ext_cmd_type,
+					       column, page_addr);
+		if (!exec_cmd) {
+			atomic_set(&info->is_ready, 1);
+			complete(&info->dev_ready);
+			break;
+		}
+
+		init_completion(&info->cmd_complete);
+		pxa3xx_nand_start(info);
+
+		ret = wait_for_completion_timeout(&info->cmd_complete,
+				CHIP_DELAY_TIMEOUT);
+		if (!ret) {
+			dev_err(&info->pdev->dev, "Wait time out!!!\n");
+			/* Stop State Machine for next command cycle */
+			pxa3xx_nand_stop(info);
+			break;
+		}
+
+		/* Check if the sequence is complete */
+		if (info->data_size == 0)
+			break;
+
+		if (command == NAND_CMD_READ0 || command == NAND_CMD_READOOB) {
+			/* Last read: issue a 'last naked read' */
+			if (info->data_size == info->chunk_size)
+				ext_cmd_type = EXT_CMD_TYPE_LAST_RW;
+			else
+				ext_cmd_type = EXT_CMD_TYPE_NAKED_RW;
+		}
+	} while (1);
+
+	info->state = STATE_IDLE;
+}
+
 static int pxa3xx_nand_write_page_hwecc(struct mtd_info *mtd,
 		struct nand_chip *chip, const uint8_t *buf, int oob_required)
 {
@@ -1029,6 +1176,8 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 		host->read_id_bytes = 2;
 	}
 
+	/* Set an initial chunk size */
+	info->chunk_size = info->fifo_size;
 	info->reg_ndcr = ndcr & ~NDCR_INT_MASK;
 	info->ndtr0cs0 = nand_readl(info, NDTR0CS0);
 	info->ndtr1cs0 = nand_readl(info, NDTR1CS0);
@@ -1135,6 +1284,7 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 		ecc->size = 512;
 		ecc->strength = 1;
 
+		info->chunk_size = 2048;
 		info->spare_size = 40;
 		info->ecc_size = 24;
 		return 1;
@@ -1144,6 +1294,7 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 		ecc->size = 512;
 		ecc->strength = 1;
 
+		info->chunk_size = 512;
 		info->spare_size = 8;
 		info->ecc_size = 8;
 		return 1;
@@ -1155,7 +1306,30 @@ static int armada370_ecc_init(struct pxa3xx_nand_info *info,
 			      struct nand_ecc_ctrl *ecc,
 			      int strength, int page_size)
 {
-	/* Unimplemented yet */
+	if (strength == 4 && page_size == 4096) {
+		ecc->mode = NAND_ECC_HW;
+		ecc->size = 512;
+		ecc->layout = &ecc_layout_4KB_bch4bit;
+		ecc->strength = 4;
+
+		info->ecc_bch = 1;
+		info->chunk_size = 2048;
+		info->spare_size = 32;
+		info->ecc_size = 32;
+		return 1;
+
+	} else if (strength == 8 && page_size == 4096) {
+		ecc->mode = NAND_ECC_HW;
+		ecc->size = 512;
+		ecc->layout = &ecc_layout_4KB_bch8bit;
+		ecc->strength = 8;
+
+		info->ecc_bch = 1;
+		info->chunk_size = 1024;
+		info->spare_size = 0;
+		info->ecc_size = 32;
+		return 1;
+	}
 	return 0;
 }
 
@@ -1319,12 +1493,16 @@ static int alloc_nand_resource(struct platform_device *pdev)
 		chip->controller        = &info->controller;
 		chip->waitfunc		= pxa3xx_nand_waitfunc;
 		chip->select_chip	= pxa3xx_nand_select_chip;
-		chip->cmdfunc		= pxa3xx_nand_cmdfunc;
 		chip->read_word		= pxa3xx_nand_read_word;
 		chip->read_byte		= pxa3xx_nand_read_byte;
 		chip->read_buf		= pxa3xx_nand_read_buf;
 		chip->write_buf		= pxa3xx_nand_write_buf;
 		chip->options		|= NAND_NO_SUBPAGE_WRITE;
+
+		if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
+			chip->cmdfunc = armada370_nand_cmdfunc;
+		else
+			chip->cmdfunc = pxa3xx_nand_cmdfunc;
 	}
 
 	spin_lock_init(&chip->controller->lock);
-- 
1.8.1.5

[PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Brian Norris]

On Tue, Nov 05, 2013 at 09:55:29AM -0300, Ezequiel Garcia wrote:
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -826,7 +887,8 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
>  	prepare_start_command(info, command);
>  
>  	info->state = STATE_PREPARED;
> -	exec_cmd = prepare_set_command(info, command, column, page_addr);
> +	exec_cmd = prepare_set_command(info, command, -1, column, page_addr);

Is it safe to use -1 for the third parameter (ext_cmd_type)? AFAICT,
this doesn't make for correct input to the NDCB0_EXT_CMD_TYPE() macro.

> +
>  	if (exec_cmd) {
>  		init_completion(&info->cmd_complete);
>  		init_completion(&info->dev_ready);
> @@ -844,6 +906,91 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
>  	info->state = STATE_IDLE;
>  }
>  
> +static void armada370_nand_cmdfunc(struct mtd_info *mtd,
> +				   const unsigned command,
> +				   int column, int page_addr)
> +{
> +	struct pxa3xx_nand_host *host = mtd->priv;
> +	struct pxa3xx_nand_info *info = host->info_data;
> +	int ret, exec_cmd, ext_cmd_type;
> +
> +	/*
> +	 * if this is a x16 device ,then convert the input

Misplaced comma/whitespace.

> +	 * "byte" address into a "word" address appropriate
> +	 * for indexing a word-oriented device
> +	 */
> +	if (info->reg_ndcr & NDCR_DWIDTH_M)
> +		column /= 2;
> +
> +	/*
> +	 * There may be different NAND chip hooked to
> +	 * different chip select, so check whether
> +	 * chip select has been changed, if yes, reset the timing
> +	 */
> +	if (info->cs != host->cs) {
> +		info->cs = host->cs;
> +		nand_writel(info, NDTR0CS0, info->ndtr0cs0);
> +		nand_writel(info, NDTR1CS0, info->ndtr1cs0);
> +	}
> +
> +	/* Select the extended command for the first command */
> +	switch (command) {
> +	case NAND_CMD_READ0:
> +	case NAND_CMD_READOOB:
> +		ext_cmd_type = EXT_CMD_TYPE_MONO;
> +		break;
> +	}

You have no default case for this switch statement, leaving ext_cmd_type
uninitialized in some cases. You add the other cases in a later patch,
but this patch is temporarily broken.

> +
> +	prepare_start_command(info, command);
> +
> +	/*
> +	 * Prepare the "is ready" completion before starting a command
> +	 * transaction sequence. If the command is not executed the
> +	 * completion will be completed, see below.
> +	 *
> +	 * We can do that inside the loop because the command variable
> +	 * is invariant and thus so is the exec_cmd.
> +	 */
> +	atomic_set(&info->is_ready, 0);
> +	init_completion(&info->dev_ready);
> +	do {
> +		info->state = STATE_PREPARED;
> +		exec_cmd = prepare_set_command(info, command, ext_cmd_type,
> +					       column, page_addr);

[...]

> @@ -1155,7 +1306,30 @@ static int armada370_ecc_init(struct pxa3xx_nand_info *info,
>  			      struct nand_ecc_ctrl *ecc,
>  			      int strength, int page_size)
>  {
> -	/* Unimplemented yet */
> +	if (strength == 4 && page_size == 4096) {

You compare only to ecc_strength_ds, and not ecc_step_ds. While it is
likely that a 4-bit ECC NAND will have a 512-byte ECC step size, you
should probably check this.

What about strength < 4? Shouldn't you be able to support a 1-bit ECC
NAND with your 4-bit ECC?

Also, do you plan to support non-ONFI NAND? Remember that nand_base
doesn't guarantee giving you a non-zero ECC strength. You might need a
DT binding to specify this, if it's not automatically detectable.

> +		ecc->mode = NAND_ECC_HW;
> +		ecc->size = 512;
> +		ecc->layout = &ecc_layout_4KB_bch4bit;
> +		ecc->strength = 4;
> +
> +		info->ecc_bch = 1;
> +		info->chunk_size = 2048;
> +		info->spare_size = 32;
> +		info->ecc_size = 32;
> +		return 1;
> +
> +	} else if (strength == 8 && page_size == 4096) {
> +		ecc->mode = NAND_ECC_HW;
> +		ecc->size = 512;
> +		ecc->layout = &ecc_layout_4KB_bch8bit;
> +		ecc->strength = 8;

These ECC parameters (8-bit per 512 and 4-bit per 512) sound reasonable
and consistent with other ECC schemes I've seen. But I'm still not clear
if we are 100% certain that matches the actual hardware implementation.
Did you do any further research since the last time we talked about
this?

> +
> +		info->ecc_bch = 1;
> +		info->chunk_size = 1024;
> +		info->spare_size = 0;
> +		info->ecc_size = 32;
> +		return 1;
> +	}
>  	return 0;
>  }
>  

Brian

[PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Ezequiel Garcia]

On Tue, Nov 05, 2013 at 11:04:32AM -0800, Brian Norris wrote:
> On Tue, Nov 05, 2013 at 09:55:29AM -0300, Ezequiel Garcia wrote:
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -826,7 +887,8 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
> >  	prepare_start_command(info, command);
> >  
> >  	info->state = STATE_PREPARED;
> > -	exec_cmd = prepare_set_command(info, command, column, page_addr);
> > +	exec_cmd = prepare_set_command(info, command, -1, column, page_addr);
> 
> Is it safe to use -1 for the third parameter (ext_cmd_type)? AFAICT,
> this doesn't make for correct input to the NDCB0_EXT_CMD_TYPE() macro.
> 

Right. Probably '0' is a saner value.

> > +
> >  	if (exec_cmd) {
> >  		init_completion(&info->cmd_complete);
> >  		init_completion(&info->dev_ready);
> > @@ -844,6 +906,91 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
> >  	info->state = STATE_IDLE;
> >  }
> >  
> > +static void armada370_nand_cmdfunc(struct mtd_info *mtd,
> > +				   const unsigned command,
> > +				   int column, int page_addr)
> > +{
> > +	struct pxa3xx_nand_host *host = mtd->priv;
> > +	struct pxa3xx_nand_info *info = host->info_data;
> > +	int ret, exec_cmd, ext_cmd_type;
> > +
> > +	/*
> > +	 * if this is a x16 device ,then convert the input
> 
> Misplaced comma/whitespace.
> 

Ack.

> > +	 * "byte" address into a "word" address appropriate
> > +	 * for indexing a word-oriented device
> > +	 */
> > +	if (info->reg_ndcr & NDCR_DWIDTH_M)
> > +		column /= 2;
> > +
> > +	/*
> > +	 * There may be different NAND chip hooked to
> > +	 * different chip select, so check whether
> > +	 * chip select has been changed, if yes, reset the timing
> > +	 */
> > +	if (info->cs != host->cs) {
> > +		info->cs = host->cs;
> > +		nand_writel(info, NDTR0CS0, info->ndtr0cs0);
> > +		nand_writel(info, NDTR1CS0, info->ndtr1cs0);
> > +	}
> > +
> > +	/* Select the extended command for the first command */
> > +	switch (command) {
> > +	case NAND_CMD_READ0:
> > +	case NAND_CMD_READOOB:
> > +		ext_cmd_type = EXT_CMD_TYPE_MONO;
> > +		break;
> > +	}
> 
> You have no default case for this switch statement, leaving ext_cmd_type
> uninitialized in some cases. You add the other cases in a later patch,
> but this patch is temporarily broken.
> 

Right, I'll add a zero-valued default.

> > +
> > +	prepare_start_command(info, command);
> > +
> > +	/*
> > +	 * Prepare the "is ready" completion before starting a command
> > +	 * transaction sequence. If the command is not executed the
> > +	 * completion will be completed, see below.
> > +	 *
> > +	 * We can do that inside the loop because the command variable
> > +	 * is invariant and thus so is the exec_cmd.
> > +	 */
> > +	atomic_set(&info->is_ready, 0);
> > +	init_completion(&info->dev_ready);
> > +	do {
> > +		info->state = STATE_PREPARED;
> > +		exec_cmd = prepare_set_command(info, command, ext_cmd_type,
> > +					       column, page_addr);
> 
> [...]
> 
> > @@ -1155,7 +1306,30 @@ static int armada370_ecc_init(struct pxa3xx_nand_info *info,
> >  			      struct nand_ecc_ctrl *ecc,
> >  			      int strength, int page_size)
> >  {
> > -	/* Unimplemented yet */
> > +	if (strength == 4 && page_size == 4096) {
> 
> You compare only to ecc_strength_ds, and not ecc_step_ds. While it is
> likely that a 4-bit ECC NAND will have a 512-byte ECC step size, you
> should probably check this.
> 

OK.

> What about strength < 4? Shouldn't you be able to support a 1-bit ECC
> NAND with your 4-bit ECC?
> 

Yes, probably. The rationale behind these xxx_ecc_init() functions is
to only validate the devices I've tested or that are known to work.

That said, maybe I'm being too paranoid.

> Also, do you plan to support non-ONFI NAND?

Not for the time being. (Are there any new non-ONFI NAND devices?)

> Remember that nand_base
> doesn't guarantee giving you a non-zero ECC strength. You might need a
> DT binding to specify this, if it's not automatically detectable.
> 

Given this series is already long and complex, I'd like to push as much
features as possible to follow-up patches. That way we can support the
current boards out there now (Mirabox's a relevant example) and add support
for more later, gradually.

> > +		ecc->mode = NAND_ECC_HW;
> > +		ecc->size = 512;
> > +		ecc->layout = &ecc_layout_4KB_bch4bit;
> > +		ecc->strength = 4;
> > +
> > +		info->ecc_bch = 1;
> > +		info->chunk_size = 2048;
> > +		info->spare_size = 32;
> > +		info->ecc_size = 32;
> > +		return 1;
> > +
> > +	} else if (strength == 8 && page_size == 4096) {
> > +		ecc->mode = NAND_ECC_HW;
> > +		ecc->size = 512;
> > +		ecc->layout = &ecc_layout_4KB_bch8bit;
> > +		ecc->strength = 8;
> 
> These ECC parameters (8-bit per 512 and 4-bit per 512) sound reasonable
> and consistent with other ECC schemes I've seen. But I'm still not clear
> if we are 100% certain that matches the actual hardware implementation.
> Did you do any further research since the last time we talked about
> this?
> 

Yes, and I tried to answer your questions in detail in the cover letters
(which now is in a patch for Documentation/mtd/nand/...) and in past
discussion.

See for instance: http://permalink.gmane.org/gmane.linux.drivers.mtd/48895.

Feel free to ask any further clarification about how the ECC engine
works. If you think it'll help, I can write a separate mail describing
it (to the best of my knowledge) and we can discuss there.

In particular, the above link contains a question that is still
unanswered and that would help me understand this better.
I'll copy-paste it here:

"""
Regarding this: I'd really like to understand better this 'step' concept
and it applicability on this controller. So any clarification is welcome
:)

As far as I can see: currently the hardware does ECC corrections in a
completely transparent fashion and merely 'reports' the no. of corrected
bits through some IRQ plus some registers.
Therefore, it implements the ecc.{read,write}_page() functions.

So, why should I tell the NAND core any of the ECC details?

I see other drivers need to implement some of the functions in the
nand_ecc_ctrl struct, such as: hwctl(), calculate() or correct().
However, I can't see how any of that applies on this controller.
"""

Thanks a lot for reviewing all of this!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Brian Norris]

On Tue, Nov 5, 2013 at 5:13 PM, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> On Tue, Nov 05, 2013 at 11:04:32AM -0800, Brian Norris wrote:
>> On Tue, Nov 05, 2013 at 09:55:29AM -0300, Ezequiel Garcia wrote:
>> > --- a/drivers/mtd/nand/pxa3xx_nand.c
>> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
>> > @@ -1155,7 +1306,30 @@ static int armada370_ecc_init(struct pxa3xx_nand_info *info,
>> >                           struct nand_ecc_ctrl *ecc,
>> >                           int strength, int page_size)
>> >  {
>> > -   /* Unimplemented yet */
>> > +   if (strength == 4 && page_size == 4096) {
>>
>> You compare only to ecc_strength_ds, and not ecc_step_ds. While it is
>> likely that a 4-bit ECC NAND will have a 512-byte ECC step size, you
>> should probably check this.
>
> OK.
>
>> What about strength < 4? Shouldn't you be able to support a 1-bit ECC
>> NAND with your 4-bit ECC?
>>
>
> Yes, probably. The rationale behind these xxx_ecc_init() functions is
> to only validate the devices I've tested or that are known to work.
>
> That said, maybe I'm being too paranoid.

No, maybe paranoid is fine. I was just bringing it up. Being too
liberal in what you accept can create compatibility problems later. So
if you wait to handle generalities until you see them in practice, you
can potentially save yourself some problems.

>> Also, do you plan to support non-ONFI NAND?
>
> Not for the time being. (Are there any new non-ONFI NAND devices?)

Sure there are. I've been seeing non-ONFI parts from Toshiba and
Macronix, at least (I think Macronix is moving to ONFI soon, though).
Samsung is around still, too.

>> Remember that nand_base
>> doesn't guarantee giving you a non-zero ECC strength. You might need a
>> DT binding to specify this, if it's not automatically detectable.
>>
>
> Given this series is already long and complex, I'd like to push as much
> features as possible to follow-up patches. That way we can support the
> current boards out there now (Mirabox's a relevant example) and add support
> for more later, gradually.

Right, I agree with a follow-up, and only if necessary. I was just
trying to plant the seeds of thought for you, so we don't run into a
maintenance problem such as Huang's gpmi-nand has. And as long as you
can plan a reasonable forward/backward-compatible solution for any
currently-supported configurations, then you don't need to add the DT
binding at all, if you don't currently need it. This is where the
paranoid approach can help you, too.

>> > +           ecc->mode = NAND_ECC_HW;
>> > +           ecc->size = 512;
>> > +           ecc->layout = &ecc_layout_4KB_bch4bit;
>> > +           ecc->strength = 4;
>> > +
>> > +           info->ecc_bch = 1;
>> > +           info->chunk_size = 2048;
>> > +           info->spare_size = 32;
>> > +           info->ecc_size = 32;
>> > +           return 1;
>> > +
>> > +   } else if (strength == 8 && page_size == 4096) {
>> > +           ecc->mode = NAND_ECC_HW;
>> > +           ecc->size = 512;
>> > +           ecc->layout = &ecc_layout_4KB_bch8bit;
>> > +           ecc->strength = 8;
>>
>> These ECC parameters (8-bit per 512 and 4-bit per 512) sound reasonable
>> and consistent with other ECC schemes I've seen. But I'm still not clear
>> if we are 100% certain that matches the actual hardware implementation.
>> Did you do any further research since the last time we talked about
>> this?
>>
>
> Yes, and I tried to answer your questions in detail in the cover letters
> (which now is in a patch for Documentation/mtd/nand/...) and in past
> discussion.

I don't see a patch for Documentation/mtd/nand/... Am I missing something?

> See for instance: http://permalink.gmane.org/gmane.linux.drivers.mtd/48895.

My memory fails me, I guess. You did answer some of my questions, but
I was still left with some more. I failed to follow up properly...

>From the linked response:

"So, if you set the controller to transfer 2048B you can correct up to
16 bits on this 2048B region, or 4 bits per 512B, hence BCH-4."

Well, "16 bits per 2048" is at least as good as BCH-4 over 512B, but
it is not exactly the same. A true "16 bits per 2048" would be able to
correct 16 bitflips concentrated in a 512B region, whereas BCH-4/512B
would not.

But unless we can get better documentation/experiments with the
controller, it's hard to tell which is the case, and it's probably not
a big deal at this point. Your current code looks OK, I think.

> Feel free to ask any further clarification about how the ECC engine
> works. If you think it'll help, I can write a separate mail describing
> it (to the best of my knowledge) and we can discuss there.
>
> In particular, the above link contains a question that is still
> unanswered and that would help me understand this better.
> I'll copy-paste it here:
>
> """
> Regarding this: I'd really like to understand better this 'step' concept
> and it applicability on this controller. So any clarification is welcome
> :)
>
> As far as I can see: currently the hardware does ECC corrections in a
> completely transparent fashion and merely 'reports' the no. of corrected
> bits through some IRQ plus some registers.
> Therefore, it implements the ecc.{read,write}_page() functions.
>
> So, why should I tell the NAND core any of the ECC details?

Because MTD/NAND now implements a bitflip threshold based on the
reported ECC strength and step size. See commit:

  commit edbc4540e02c201bdd4f4d498ebb6ed517fd36e2
  Author: Mike Dunn <mikedunn@newsguy.com>
  Date:   Wed Apr 25 12:06:11 2012 -0700

      mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN

and some of the other related code and documentation, such as in this commit:

  commit d062d4ede877fcd2ecc4c6262abad09a6f32950a
  Author: Mike Dunn <mikedunn@newsguy.com>
  Date:   Wed Apr 25 12:06:08 2012 -0700

      mtd: bitflip_threshold added to mtd_info and sysfs

If you don't have the correct strength, then this may not work as
intended. (I realized now that the ECC step size doesn't play a part
in the bitflip threshold process; only the strength is required to be
correct.)

...and now that I bring this up, I see that pxa3xx_nand.c has not been
updated to report 'max_bitflips' via return code from
pxa3xx_nand_read_page_hwecc(). This needs fixed. I wonder how many
other drivers are similarly broken. I just realized that my
out-of-tree driver is broken for this feature :(

> I see other drivers need to implement some of the functions in the
> nand_ecc_ctrl struct, such as: hwctl(), calculate() or correct().
> However, I can't see how any of that applies on this controller.
> """

I was not suggesting you need to implement these functions.

If I've missed any other important questions, please bear with me and
repeat them.

Thanks,
Brian

[PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Brian Norris]

On Tue, Nov 5, 2013 at 6:20 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Tue, Nov 5, 2013 at 5:13 PM, Ezequiel Garcia <ezequiel.garcia@free-electrons.com> wrote:
>> So, why should I tell the NAND core any of the ECC details?
>
> Because MTD/NAND now implements a bitflip threshold based on the
> reported ECC strength and step size. See commit:
>
>   commit edbc4540e02c201bdd4f4d498ebb6ed517fd36e2
>   Author: Mike Dunn <mikedunn@newsguy.com>
>   Date:   Wed Apr 25 12:06:11 2012 -0700
>
>       mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN
>
> and some of the other related code and documentation, such as in this commit:
>
>   commit d062d4ede877fcd2ecc4c6262abad09a6f32950a
>   Author: Mike Dunn <mikedunn@newsguy.com>
>   Date:   Wed Apr 25 12:06:08 2012 -0700
>
>       mtd: bitflip_threshold added to mtd_info and sysfs
>
> If you don't have the correct strength, then this may not work as
> intended. (I realized now that the ECC step size doesn't play a part
> in the bitflip threshold process; only the strength is required to be
> correct.)

To clarify a bit further: the ECC step size doesn't play a part in
nand_base when dealing with max_bitflips, but it is implicit in the
specification: the driver should be returning the maximum number of
bitflips corrected in a single ECC sector of the page that was read.
So if your driver reports ECC strength of 8 over a 512 byte step, then
you should report a number in the range of 0 to 8, representing
bitflips in a 512B sector. If you don't have this granularity (which
it looks like you might not?) then maybe your strength-per-sector
should really be 16-per-2048B or 16-per-1024B, etc.

Brian

[PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Ezequiel Garcia]

On Tue, Nov 05, 2013 at 06:20:18PM -0800, Brian Norris wrote:
[..]
> >
> > Yes, and I tried to answer your questions in detail in the cover letters
> > (which now is in a patch for Documentation/mtd/nand/...) and in past
> > discussion.
> 
> I don't see a patch for Documentation/mtd/nand/... Am I missing something?
> 

Just for the record:

http://www.spinics.net/lists/arm-kernel/msg284271.html

And I'll continue replying the rest of the feedback tomorrow :-)

Thanks!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Ezequiel Garcia]

On Tue, Nov 05, 2013 at 06:20:18PM -0800, Brian Norris wrote:
[..]
> 
> >> Also, do you plan to support non-ONFI NAND?
> >
> > Not for the time being. (Are there any new non-ONFI NAND devices?)
> 
> Sure there are. I've been seeing non-ONFI parts from Toshiba and
> Macronix, at least (I think Macronix is moving to ONFI soon, though).
> Samsung is around still, too.
> 

Right. Well, for now I would just add a pr_warn("Your device is not
supported. Please ask Ezequiel to add it") or something along those
lines.

BTW: So, I guess that for non-ONFI devices we must simply put the ECC
information in the DT (being a hardware parameter)?

Having followed (part) of the OMAP DT ECC discussion, I'm thinking:
wouldn't it be good to have an 'nand-ecc-strength' property in the DT?

It would match the ecc_strength_ds field. This way, the ECC mode
selection is left to the driver and not to the user.
On the other side, this can cause some severe incompatibilities
unless such driver *always* make the *same* selection.

Anyway, none of this matters in this patchset, for the time being.

[..]
> 
> From the linked response:
> 
> "So, if you set the controller to transfer 2048B you can correct up to
> 16 bits on this 2048B region, or 4 bits per 512B, hence BCH-4."
> 
> Well, "16 bits per 2048" is at least as good as BCH-4 over 512B, but
> it is not exactly the same. A true "16 bits per 2048" would be able to
> correct 16 bitflips concentrated in a 512B region, whereas BCH-4/512B
> would not.
> 

Indeed, 16-over-2048 is stronger than 4-over-512 :)

> But unless we can get better documentation/experiments with the
> controller, it's hard to tell which is the case, and it's probably not
> a big deal at this point. Your current code looks OK, I think.
> 

Ok, great.

> > Feel free to ask any further clarification about how the ECC engine
> > works. If you think it'll help, I can write a separate mail describing
> > it (to the best of my knowledge) and we can discuss there.
> >
> > In particular, the above link contains a question that is still
> > unanswered and that would help me understand this better.
> > I'll copy-paste it here:
> >
> > """
> > Regarding this: I'd really like to understand better this 'step' concept
> > and it applicability on this controller. So any clarification is welcome
> > :)
> >
> > As far as I can see: currently the hardware does ECC corrections in a
> > completely transparent fashion and merely 'reports' the no. of corrected
> > bits through some IRQ plus some registers.
> > Therefore, it implements the ecc.{read,write}_page() functions.
> >
> > So, why should I tell the NAND core any of the ECC details?
> 
> Because MTD/NAND now implements a bitflip threshold based on the
> reported ECC strength and step size. See commit:
> 
>   commit edbc4540e02c201bdd4f4d498ebb6ed517fd36e2
>   Author: Mike Dunn <mikedunn@newsguy.com>
>   Date:   Wed Apr 25 12:06:11 2012 -0700
> 
>       mtd: driver _read() returns max_bitflips; mtd_read() returns -EUCLEAN
> 
> and some of the other related code and documentation, such as in this commit:
> 
>   commit d062d4ede877fcd2ecc4c6262abad09a6f32950a
>   Author: Mike Dunn <mikedunn@newsguy.com>
>   Date:   Wed Apr 25 12:06:08 2012 -0700
> 
>       mtd: bitflip_threshold added to mtd_info and sysfs
> 
> If you don't have the correct strength, then this may not work as
> intended. (I realized now that the ECC step size doesn't play a part
> in the bitflip threshold process; only the strength is required to be
> correct.)
> 
> ...and now that I bring this up, I see that pxa3xx_nand.c has not been
> updated to report 'max_bitflips' via return code from
> pxa3xx_nand_read_page_hwecc(). This needs fixed. I wonder how many
> other drivers are similarly broken. I just realized that my
> out-of-tree driver is broken for this feature :(
> 

Hm.. I see. Well, it's just a matter of returning the corrected bits,
which are already obtained. Furthermore, one of the last patches in
this series, namely:

"mtd: nand: pxa3xx: Add ECC BCH correctable errors detection"

Takes care of obtaining the proper corrected bits number.

(Let me paste here your other reply to this same mail)

"""
> To clarify a bit further: the ECC step size doesn't play a part in
> nand_base when dealing with max_bitflips, but it is implicit in the
> specification: the driver should be returning the maximum number of
> bitflips corrected in a single ECC sector of the page that was read.
> So if your driver reports ECC strength of 8 over a 512 byte step, then
> you should report a number in the range of 0 to 8, representing
> bitflips in a 512B sector. If you don't have this granularity (which
> it looks like you might not?) then maybe your strength-per-sector
> should really be 16-per-2048B or 16-per-1024B, etc.
"""

So, if I understand correctly and assuming the controller works as the
specification says (sorry, it's not public yet) I think the 'step' is
the chunk size and the strength is 16 bytes:

	/* In BCH mode */
	ecc->size = chunk->size;
	ecc->strength = 16;

And maybe I could try to expand some more the ECC description in the
Documentation/mtd/nand/pxa3xx-nand.txt.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Brian Norris]

Hi Ezequiel,

There's one question of yours that I think hasn't been addressed:

On Wed, Nov 06, 2013 at 08:32:11AM -0300, Ezequiel Garcia wrote:
> Having followed (part) of the OMAP DT ECC discussion, I'm thinking:
> wouldn't it be good to have an 'nand-ecc-strength' property in the DT?
> 
> It would match the ecc_strength_ds field. This way, the ECC mode
> selection is left to the driver and not to the user.
> On the other side, this can cause some severe incompatibilities
> unless such driver *always* make the *same* selection.

I'm not quite so sure about the whole question of ECC in device tree.
There seems to be 2 subtly different properties we might want to
capture:

1) What is the minimum ECC required for the flash?

2) What is the exact ECC layout/strength/type used for this flash?
   (i.e., what is the bootloader using?)

The first is quite generic: the property consists of a stength and step
size. (But this is also duplicated in ONFI, and our full-ID device
table.)

Still, I think a property for #1 could be useful, for those chips that
are not discoverable. And if we (you?) add it, it should be done at the
nand_base level, I think, so its binding is shared by all drivers.

I'm not quite sure how we identify the appropriate struct device_node
for nand_base to use, though. Maybe we should force drivers to
initialize a new mtd_info.of_node field? And then maybe this could also
be integrated into the 'ofpart' parser, which currently requires drivers
to pass a device_node via the mtd_part_parser_data struct?

Property #2 is very driver/hardware specific, and it may not be easy to
capture this information properly using the same set of properties for
all NAND drivers. For example, "BCH-8" is not the same on all systems;
and even on the same system a software BCH-X could potentially be very
different than (and incompatible with) a BCH-X as provided by hardware.
And different hardware provides wildly different choices regarding ECC,
so I'm not convinced that we could create a good generic binding for
describing #2.

But I think a property like #2 is necessary for many platforms that need
to eliminate the problem that you mention, where drivers must always
make the same selection. Essentially, we're assuming bootloader/driver
co-design, rather than properly communicating this information via a
shared data structure like device tree.

Now, it's another question as to whether we need a property for both #1
and #2. The latter would probably just override the former, but that
doesn't mean that the former is unnecessary...

Anyway, I'll stop having a conversation with myself now. If you think
something should be done, I'm willing to listen and help integrate a
solution.

Brian

[PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Ezequiel Garcia]

On Mon, Nov 18, 2013 at 10:10:09AM -0800, Brian Norris wrote:
> Hi Ezequiel,
> 
> There's one question of yours that I think hasn't been addressed:
> 
> On Wed, Nov 06, 2013 at 08:32:11AM -0300, Ezequiel Garcia wrote:
> > Having followed (part) of the OMAP DT ECC discussion, I'm thinking:
> > wouldn't it be good to have an 'nand-ecc-strength' property in the DT?
> > 
> > It would match the ecc_strength_ds field. This way, the ECC mode
> > selection is left to the driver and not to the user.
> > On the other side, this can cause some severe incompatibilities
> > unless such driver *always* make the *same* selection.
> 
> I'm not quite so sure about the whole question of ECC in device tree.
> There seems to be 2 subtly different properties we might want to
> capture:
> 
> 1) What is the minimum ECC required for the flash?
> 
> 2) What is the exact ECC layout/strength/type used for this flash?
>    (i.e., what is the bootloader using?)
> 
> The first is quite generic: the property consists of a stength and step
> size. (But this is also duplicated in ONFI, and our full-ID device
> table.)
> 
> Still, I think a property for #1 could be useful, for those chips that
> are not discoverable. And if we (you?) add it, it should be done at the
> nand_base level, I think, so its binding is shared by all drivers.
> 
> I'm not quite sure how we identify the appropriate struct device_node
> for nand_base to use, though. Maybe we should force drivers to
> initialize a new mtd_info.of_node field? And then maybe this could also
> be integrated into the 'ofpart' parser, which currently requires drivers
> to pass a device_node via the mtd_part_parser_data struct?
> 
> Property #2 is very driver/hardware specific, and it may not be easy to
> capture this information properly using the same set of properties for
> all NAND drivers. For example, "BCH-8" is not the same on all systems;
> and even on the same system a software BCH-X could potentially be very
> different than (and incompatible with) a BCH-X as provided by hardware.
> And different hardware provides wildly different choices regarding ECC,
> so I'm not convinced that we could create a good generic binding for
> describing #2.
> 
> But I think a property like #2 is necessary for many platforms that need
> to eliminate the problem that you mention, where drivers must always
> make the same selection. Essentially, we're assuming bootloader/driver
> co-design, rather than properly communicating this information via a
> shared data structure like device tree.
> 
> Now, it's another question as to whether we need a property for both #1
> and #2. The latter would probably just override the former, but that
> doesn't mean that the former is unnecessary...
> 

I completely agree with all of the above, but I'm still a bit uncertain
about how useful implementing #1 would be.

As you say, encoding the specific (per-driver) ECC information in the
devicetree seems the safest way of dealing with that.

On the other side, I fail to clearly see a valid use case for reporting
the "minimum" ECC required strength in the devicetree.

If I'm not missing anything, then I'd say just implement #2, for each driver
that needs it. I know that pxa3xx-nand should have it to avoid future issues.
This item is on top of my NAND TODO list.

Thanks for following the discussion!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Brian Norris]

Hi Ezequiel,

On Mon, Nov 18, 2013 at 03:33:26PM -0300, Ezequiel Garcia wrote:
> On Mon, Nov 18, 2013 at 10:10:09AM -0800, Brian Norris wrote:
> > There's one question of yours that I think hasn't been addressed:
> > 
> > On Wed, Nov 06, 2013 at 08:32:11AM -0300, Ezequiel Garcia wrote:
> > > Having followed (part) of the OMAP DT ECC discussion, I'm thinking:
> > > wouldn't it be good to have an 'nand-ecc-strength' property in the DT?
> > > 
> > > It would match the ecc_strength_ds field. This way, the ECC mode
> > > selection is left to the driver and not to the user.
> > > On the other side, this can cause some severe incompatibilities
> > > unless such driver *always* make the *same* selection.
> > 
> > I'm not quite so sure about the whole question of ECC in device tree.
> > There seems to be 2 subtly different properties we might want to
> > capture:
> > 
> > 1) What is the minimum ECC required for the flash?
> > 
> > 2) What is the exact ECC layout/strength/type used for this flash?
> >    (i.e., what is the bootloader using?)
> > 
> > The first is quite generic: the property consists of a stength and step
> > size. (But this is also duplicated in ONFI, and our full-ID device
> > table.)
> > 
> > Still, I think a property for #1 could be useful, for those chips that
> > are not discoverable. And if we (you?) add it, it should be done at the
> > nand_base level, I think, so its binding is shared by all drivers.
> > 
> > I'm not quite sure how we identify the appropriate struct device_node
> > for nand_base to use, though. Maybe we should force drivers to
> > initialize a new mtd_info.of_node field? And then maybe this could also
> > be integrated into the 'ofpart' parser, which currently requires drivers
> > to pass a device_node via the mtd_part_parser_data struct?
> > 
> > Property #2 is very driver/hardware specific, and it may not be easy to
> > capture this information properly using the same set of properties for
> > all NAND drivers. For example, "BCH-8" is not the same on all systems;
> > and even on the same system a software BCH-X could potentially be very
> > different than (and incompatible with) a BCH-X as provided by hardware.
> > And different hardware provides wildly different choices regarding ECC,
> > so I'm not convinced that we could create a good generic binding for
> > describing #2.
> > 
> > But I think a property like #2 is necessary for many platforms that need
> > to eliminate the problem that you mention, where drivers must always
> > make the same selection. Essentially, we're assuming bootloader/driver
> > co-design, rather than properly communicating this information via a
> > shared data structure like device tree.
> > 
> > Now, it's another question as to whether we need a property for both #1
> > and #2. The latter would probably just override the former, but that
> > doesn't mean that the former is unnecessary...
> > 
> 
> I completely agree with all of the above, but I'm still a bit uncertain
> about how useful implementing #1 would be.
> 
> As you say, encoding the specific (per-driver) ECC information in the
> devicetree seems the safest way of dealing with that.
> 
> On the other side, I fail to clearly see a valid use case for reporting
> the "minimum" ECC required strength in the devicetree.
> 
> If I'm not missing anything, then I'd say just implement #2, for each driver
> that needs it. I know that pxa3xx-nand should have it to avoid future issues.
> This item is on top of my NAND TODO list.

Well, Huang kinda dodged #2 for now in GPMI NAND, by hacking in a
'fsl,use-minimum-ecc' property. That potentially yields less work for
the bootloader; it just defers the "minimum ECC" choice to Linux. This
is certainly less specific, and it leaves the door open just a bit for
Linux to inadvertently make incompatible changes...

But the more I think about it, the more I think that #2 is really the
right way. And yes, I think pxa3xx-nand should do #2.

Huang,

Do you have a good reason why you needed GPMI NAND to choose the ECC
configuration (a la "miminum ECC") instead of fully specifying your ECC
selection in device tree? I recall most of your arguments were about
using an ECC strength that leaves room for user data in OOB (e.g., with
JFFS2). But you could have done the same thing by creating a proper DT
property to describe the desidered ECC strength, with no real
disadvantage, right?

Brian

[PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Brian Norris]

Hi Huang,

I'd like to follow up on this question, since you didn't answer it, and
it's still relevant, since we haven't yet merged your GPMI DT binding
(it's queued for the next merge window):

On Mon, Nov 18, 2013 at 10:50:59AM -0800, Brian Norris wrote:
> Do you have a good reason why you needed GPMI NAND to choose the ECC
> configuration (a la "miminum ECC") instead of fully specifying your ECC
> selection in device tree? I recall most of your arguments were about
> using an ECC strength that leaves room for user data in OOB (e.g., with
> JFFS2). But you could have done the same thing by creating a proper DT
> property to describe the desidered ECC strength, with no real
> disadvantage, right?

I'll rephrase it: why can't/don't you define a GPMI binding for the
actual ECC level, like:

  fsl,nand-ecc-strength and fsl,nand-ecc-sector

?

Then, you could still default to the old geometry if these properties
aren't present, and you don't have to rely on Linux auto-detecting ECC
properties for non-ONFI chips.

(Side note: I see you submitted patches for Hynix MLC ECC
auto-detection; I'm not sure this approach is really scalable, but it
may be OK for some chips. Anyway, I'll address these patches separately
once I get to them.)

Brian

[PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Brian Norris]

One more thing on this patch:

On Tue, Nov 05, 2013 at 09:55:29AM -0300, Ezequiel Garcia wrote:
> @@ -437,7 +487,7 @@ static void disable_int(struct pxa3xx_nand_info *info, uint32_t int_mask)
>  
>  static void handle_data_pio(struct pxa3xx_nand_info *info)
>  {
> -	unsigned int do_bytes = min(info->data_size, info->fifo_size);
> +	unsigned int do_bytes = min(info->data_size, info->chunk_size);

After this change, you no longer are using fifo_size globally (you only
use it locally in pxa3xx_nand_detect_config() to set the chunk_size).
Perhaps you can drop the field?

>  
>  	switch (info->state) {
>  	case STATE_PIO_WRITING:

Brian

[PATCH v3 23/28] mtd: nand: pxa3xx: Add multiple chunk write support

[Ezequiel Garcia]

This commit adds write support for large pages (4 KiB, 8 KiB).
Such support is implemented by issuing a multiple command sequence,
transfering a set of 2 KiB chunks per transaction.

The splitted command sequence requires to send the SEQIN command
independently of the PAGEPROG command and therefore it's set as
an execution command.

Since PAGEPROG enables ECC, each 2 KiB chunk of data is written
together with ECC code at a controller-fixed location within
the flash page.

Currently, only devices with a 4 KiB page size has been tested.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 83 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 75 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index b9acd66..45c7b7c 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -780,6 +780,20 @@ static int prepare_set_command(struct pxa3xx_nand_info *info, int command,
 
 		info->buf_start = column;
 		set_command_address(info, mtd->writesize, 0, page_addr);
+
+		/*
+		 * Multiple page programming needs to execute the initial
+		 * SEQIN command that sets the page address.
+		 */
+		if (mtd->writesize > PAGE_CHUNK_SIZE) {
+			info->ndcb0 |= NDCB0_CMD_TYPE(0x1)
+				| NDCB0_EXT_CMD_TYPE(ext_cmd_type)
+				| addr_cycle
+				| command;
+			/* No data transfer in this case */
+			info->data_size = 0;
+			exec_cmd = 1;
+		}
 		break;
 
 	case NAND_CMD_PAGEPROG:
@@ -789,13 +803,40 @@ static int prepare_set_command(struct pxa3xx_nand_info *info, int command,
 			break;
 		}
 
-		info->ndcb0 |= NDCB0_CMD_TYPE(0x1)
-				| NDCB0_AUTO_RS
-				| NDCB0_ST_ROW_EN
-				| NDCB0_DBC
-				| (NAND_CMD_PAGEPROG << 8)
-				| NAND_CMD_SEQIN
-				| addr_cycle;
+		/* Second command setting for large pages */
+		if (mtd->writesize > PAGE_CHUNK_SIZE) {
+			/*
+			 * Multiple page write uses the 'extended command'
+			 * field. This can be used to issue a command dispatch
+			 * or a naked-write depending on the current stage.
+			 */
+			info->ndcb0 |= NDCB0_CMD_TYPE(0x1)
+					| NDCB0_LEN_OVRD
+					| NDCB0_EXT_CMD_TYPE(ext_cmd_type);
+			info->ndcb3 = info->chunk_size +
+				      info->oob_size;
+
+			/*
+			 * This is the command dispatch that completes a chunked
+			 * page program operation.
+			 */
+			if (info->data_size == 0) {
+				info->ndcb0 = NDCB0_CMD_TYPE(0x1)
+					| NDCB0_EXT_CMD_TYPE(ext_cmd_type)
+					| command;
+				info->ndcb1 = 0;
+				info->ndcb2 = 0;
+				info->ndcb3 = 0;
+			}
+		} else {
+			info->ndcb0 |= NDCB0_CMD_TYPE(0x1)
+					| NDCB0_AUTO_RS
+					| NDCB0_ST_ROW_EN
+					| NDCB0_DBC
+					| (NAND_CMD_PAGEPROG << 8)
+					| NAND_CMD_SEQIN
+					| addr_cycle;
+		}
 		break;
 
 	case NAND_CMD_PARAM:
@@ -939,6 +980,15 @@ static void armada370_nand_cmdfunc(struct mtd_info *mtd,
 	case NAND_CMD_READOOB:
 		ext_cmd_type = EXT_CMD_TYPE_MONO;
 		break;
+	case NAND_CMD_SEQIN:
+		ext_cmd_type = EXT_CMD_TYPE_DISPATCH;
+		break;
+	case NAND_CMD_PAGEPROG:
+		ext_cmd_type = EXT_CMD_TYPE_NAKED_RW;
+		break;
+	default:
+		ext_cmd_type = -1;
+		break;
 	}
 
 	prepare_start_command(info, command);
@@ -976,7 +1026,16 @@ static void armada370_nand_cmdfunc(struct mtd_info *mtd,
 		}
 
 		/* Check if the sequence is complete */
-		if (info->data_size == 0)
+		if (info->data_size == 0 && command != NAND_CMD_PAGEPROG)
+			break;
+
+		/*
+		 * After a splitted program command sequence has issued
+		 * the command dispatch, the command sequence is complete.
+		 */
+		if (info->data_size == 0 &&
+		    command == NAND_CMD_PAGEPROG &&
+		    ext_cmd_type == EXT_CMD_TYPE_DISPATCH)
 			break;
 
 		if (command == NAND_CMD_READ0 || command == NAND_CMD_READOOB) {
@@ -985,6 +1044,14 @@ static void armada370_nand_cmdfunc(struct mtd_info *mtd,
 				ext_cmd_type = EXT_CMD_TYPE_LAST_RW;
 			else
 				ext_cmd_type = EXT_CMD_TYPE_NAKED_RW;
+
+		/*
+		 * If a splitted program command has no more data to transfer,
+		 * the command dispatch must be issued to complete.
+		 */
+		} else if (command == NAND_CMD_PAGEPROG &&
+			   info->data_size == 0) {
+				ext_cmd_type = EXT_CMD_TYPE_DISPATCH;
 		}
 	} while (1);
 
-- 
1.8.1.5

[PATCH v3 24/28] mtd: nand: pxa3xx: Add ECC BCH correctable errors detection

[Ezequiel Garcia]

This commit extends the ECC correctable error detection to include
ECC BCH errors. The number of BCH correctable errors can be any up to 16,
and the actual value is exposed in the NDSR register.

Therefore, we change some symbol names to refer to correctable or
uncorrectable (instead of single-bit or double-bit as it was in the
Hamming case) and while at it, cleanup the detection code slightly.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 43 +++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 45c7b7c..7945544 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -97,6 +97,9 @@
 #define NDCR_INT_MASK           (0xFFF)
 
 #define NDSR_MASK		(0xfff)
+#define NDSR_ERR_CNT_OFF	(16)
+#define NDSR_ERR_CNT_MASK       (0x1f)
+#define NDSR_ERR_CNT(sr)	((sr >> NDSR_ERR_CNT_OFF) & NDSR_ERR_CNT_MASK)
 #define NDSR_RDY                (0x1 << 12)
 #define NDSR_FLASH_RDY          (0x1 << 11)
 #define NDSR_CS0_PAGED		(0x1 << 10)
@@ -105,8 +108,8 @@
 #define NDSR_CS1_CMDD		(0x1 << 7)
 #define NDSR_CS0_BBD		(0x1 << 6)
 #define NDSR_CS1_BBD		(0x1 << 5)
-#define NDSR_DBERR		(0x1 << 4)
-#define NDSR_SBERR		(0x1 << 3)
+#define NDSR_UNCORERR		(0x1 << 4)
+#define NDSR_CORERR		(0x1 << 3)
 #define NDSR_WRDREQ		(0x1 << 2)
 #define NDSR_RDDREQ		(0x1 << 1)
 #define NDSR_WRCMDREQ		(0x1)
@@ -147,9 +150,9 @@ enum {
 	ERR_NONE	= 0,
 	ERR_DMABUSERR	= -1,
 	ERR_SENDCMD	= -2,
-	ERR_DBERR	= -3,
+	ERR_UNCORERR	= -3,
 	ERR_BBERR	= -4,
-	ERR_SBERR	= -5,
+	ERR_CORERR	= -5,
 };
 
 enum {
@@ -240,6 +243,7 @@ struct pxa3xx_nand_info {
 	unsigned int		oob_size;
 	unsigned int		spare_size;
 	unsigned int		ecc_size;
+	unsigned int		ecc_err_cnt;
 	int 			retcode;
 
 	/* cached register value */
@@ -590,10 +594,16 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 
 	status = nand_readl(info, NDSR);
 
-	if (status & NDSR_DBERR)
-		info->retcode = ERR_DBERR;
-	if (status & NDSR_SBERR)
-		info->retcode = ERR_SBERR;
+	if (status & NDSR_UNCORERR)
+		info->retcode = ERR_UNCORERR;
+	if (status & NDSR_CORERR) {
+		info->retcode = ERR_CORERR;
+		if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&
+		    info->ecc_bch)
+			info->ecc_err_cnt = NDSR_ERR_CNT(status);
+		else
+			info->ecc_err_cnt = 1;
+	}
 	if (status & (NDSR_RDDREQ | NDSR_WRDREQ)) {
 		/* whether use dma to transfer data */
 		if (info->use_dma) {
@@ -692,6 +702,7 @@ static void prepare_start_command(struct pxa3xx_nand_info *info, int command)
 	info->use_ecc		= 0;
 	info->use_spare		= 1;
 	info->retcode		= ERR_NONE;
+	info->ecc_err_cnt	= 0;
 	info->ndcb3		= 0;
 
 	switch (command) {
@@ -1077,20 +1088,14 @@ static int pxa3xx_nand_read_page_hwecc(struct mtd_info *mtd,
 	chip->read_buf(mtd, buf, mtd->writesize);
 	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
 
-	if (info->retcode == ERR_SBERR) {
-		switch (info->use_ecc) {
-		case 1:
-			mtd->ecc_stats.corrected++;
-			break;
-		case 0:
-		default:
-			break;
-		}
-	} else if (info->retcode == ERR_DBERR) {
+	if (info->retcode == ERR_CORERR && info->use_ecc) {
+		mtd->ecc_stats.corrected += info->ecc_err_cnt;
+
+	} else if (info->retcode == ERR_UNCORERR) {
 		/*
 		 * for blank page (all 0xff), HW will calculate its ECC as
 		 * 0, which is different from the ECC information within
-		 * OOB, ignore such double bit errors
+		 * OOB, ignore such ucorrectable errors
 		 */
 		if (is_buf_blank(buf, mtd->writesize))
 			info->retcode = ERR_NONE;
-- 
1.8.1.5

[PATCH v3 25/28] ARM: mvebu: Add support for NAND controller in Armada 370/XP

[Ezequiel Garcia]

The Armada 370 and Armada XP SoC have a NAND controller (aka NFCv2).
This commit adds support for it in Armada 370 and Armada XP SoC
common devicetree.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 01e69fc..b4e6898 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -258,6 +258,15 @@
 				status = "disabled";
 			};
 
+			nand at d0000 {
+				compatible = "marvell,armada370-nand";
+				reg = <0xd0000 0x54>;
+				#address-cells = <1>;
+				#size-cells = <1>;
+				interrupts = <113>;
+				clocks = <&coredivclk 0>;
+				status = "disabled";
+			};
 		};
 	};
 
-- 
1.8.1.5

[PATCH v3 25/28] ARM: mvebu: Add support for NAND controller in Armada 370/XP

[Jason Cooper]

On Tue, Nov 05, 2013 at 09:55:32AM -0300, Ezequiel Garcia wrote:
> The Armada 370 and Armada XP SoC have a NAND controller (aka NFCv2).
> This commit adds support for it in Armada 370 and Armada XP SoC
> common devicetree.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  arch/arm/boot/dts/armada-370-xp.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 01e69fc..b4e6898 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -258,6 +258,15 @@
>  				status = "disabled";
>  			};
>  
> +			nand at d0000 {
> +				compatible = "marvell,armada370-nand";

Could you please provide a separate patch updating the devicetree
binding documentation?  You can also Cc the entire series to the
devicetree ml as long as the documentation patch is easy to find in the
series.  eg 'dt: binding: ...'

thx,

Jason.

[PATCH v3 25/28] ARM: mvebu: Add support for NAND controller in Armada 370/XP

[Ezequiel Garcia]

On Tue, Nov 05, 2013 at 08:29:05AM -0500, Jason Cooper wrote:
> On Tue, Nov 05, 2013 at 09:55:32AM -0300, Ezequiel Garcia wrote:
> > The Armada 370 and Armada XP SoC have a NAND controller (aka NFCv2).
> > This commit adds support for it in Armada 370 and Armada XP SoC
> > common devicetree.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  arch/arm/boot/dts/armada-370-xp.dtsi | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> > index 01e69fc..b4e6898 100644
> > --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> > +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> > @@ -258,6 +258,15 @@
> >  				status = "disabled";
> >  			};
> >  
> > +			nand at d0000 {
> > +				compatible = "marvell,armada370-nand";
> 
> Could you please provide a separate patch updating the devicetree
> binding documentation?  You can also Cc the entire series to the
> devicetree ml as long as the documentation patch is easy to find in the
> series.  eg 'dt: binding: ...'
> 

Hm.. actually the controller already supports the new compatible string
so the binding documentation should be added now.

And I'd rather do that in a separate patch, to avoid cluttering the poor
devicetree people with an unrelated 28-piece patch :-)

BTW: who should take such a patch? I'm still a little lost regarding
who takes the binding or dts patches for a given subsystem.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 25/28] ARM: mvebu: Add support for NAND controller in Armada 370/XP

[Jason Cooper]

On Tue, Nov 05, 2013 at 10:51:46AM -0300, Ezequiel Garcia wrote:
> On Tue, Nov 05, 2013 at 08:29:05AM -0500, Jason Cooper wrote:
> > On Tue, Nov 05, 2013 at 09:55:32AM -0300, Ezequiel Garcia wrote:
> > > The Armada 370 and Armada XP SoC have a NAND controller (aka NFCv2).
> > > This commit adds support for it in Armada 370 and Armada XP SoC
> > > common devicetree.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > ---
> > >  arch/arm/boot/dts/armada-370-xp.dtsi | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> > > index 01e69fc..b4e6898 100644
> > > --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> > > +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> > > @@ -258,6 +258,15 @@
> > >  				status = "disabled";
> > >  			};
> > >  
> > > +			nand at d0000 {
> > > +				compatible = "marvell,armada370-nand";
> > 
> > Could you please provide a separate patch updating the devicetree
> > binding documentation?  You can also Cc the entire series to the
> > devicetree ml as long as the documentation patch is easy to find in the
> > series.  eg 'dt: binding: ...'
> > 
> 
> Hm.. actually the controller already supports the new compatible string
> so the binding documentation should be added now.

$ git grep -n 'marvell,armada370-nand' -- Documentation/devicetree/bindings/
$

Perhaps in v3.13-rc1?

> And I'd rather do that in a separate patch, to avoid cluttering the poor
> devicetree people with an unrelated 28-piece patch :-)

No (really), according to Grant and Mark during the closing session, I
asked this specific question.  They _do_ want the entire series so they
can refer to the corresponding code changes if necessary.  As I stated
above, we can make their job easier by making the binding a separate
patch that is clearly marked as such.

> BTW: who should take such a patch? I'm still a little lost regarding
> who takes the binding or dts patches for a given subsystem.

The appropriate sub-system maintainer still takes the patches, we simply
wait a bit for the DT binding maintainers to chime in.  If they don't
after a few weeks, we can take it without their Ack.

If the maintainer is unsure, or needs help reviewing the binding, they
can always ping the DT folks for assistance.

thx,

Jason.

[PATCH v3 25/28] ARM: mvebu: Add support for NAND controller in Armada 370/XP

[Ezequiel Garcia]

On Tue, Nov 05, 2013 at 10:15:31AM -0500, Jason Cooper wrote:
> On Tue, Nov 05, 2013 at 10:51:46AM -0300, Ezequiel Garcia wrote:
> > On Tue, Nov 05, 2013 at 08:29:05AM -0500, Jason Cooper wrote:
> > > On Tue, Nov 05, 2013 at 09:55:32AM -0300, Ezequiel Garcia wrote:
> > > > The Armada 370 and Armada XP SoC have a NAND controller (aka NFCv2).
> > > > This commit adds support for it in Armada 370 and Armada XP SoC
> > > > common devicetree.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > > ---
> > > >  arch/arm/boot/dts/armada-370-xp.dtsi | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> > > > index 01e69fc..b4e6898 100644
> > > > --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> > > > +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> > > > @@ -258,6 +258,15 @@
> > > >  				status = "disabled";
> > > >  			};
> > > >  
> > > > +			nand at d0000 {
> > > > +				compatible = "marvell,armada370-nand";
> > > 
> > > Could you please provide a separate patch updating the devicetree
> > > binding documentation?  You can also Cc the entire series to the
> > > devicetree ml as long as the documentation patch is easy to find in the
> > > series.  eg 'dt: binding: ...'
> > > 
> > 
> > Hm.. actually the controller already supports the new compatible string
> > so the binding documentation should be added now.
> 
> $ git grep -n 'marvell,armada370-nand' -- Documentation/devicetree/bindings/
> $

Well the controller supports it, but I never updated the binding:

$ git grep -n 'marvell,armada370-nand' -- drivers/mtd/nand/pxa3xx-nand.c

So that's why I think a separate patch to be taken by Brian now is more
appropriate.

> > And I'd rather do that in a separate patch, to avoid cluttering the poor
> > devicetree people with an unrelated 28-piece patch :-)
> 
> No (really), according to Grant and Mark during the closing session, I
> asked this specific question.  They _do_ want the entire series so they
> can refer to the corresponding code changes if necessary.  As I stated
> above, we can make their job easier by making the binding a separate
> patch that is clearly marked as such.
> 

Ah, good to know.

> > BTW: who should take such a patch? I'm still a little lost regarding
> > who takes the binding or dts patches for a given subsystem.
> 
> The appropriate sub-system maintainer still takes the patches, we simply
> wait a bit for the DT binding maintainers to chime in.  If they don't
> after a few weeks, we can take it without their Ack.
> 
> If the maintainer is unsure, or needs help reviewing the binding, they
> can always ping the DT folks for assistance.
> 

Ok, great.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 25/28] ARM: mvebu: Add support for NAND controller in Armada 370/XP

[Thomas Petazzoni]

Dear Jason Cooper,

On Tue, 5 Nov 2013 10:15:31 -0500, Jason Cooper wrote:

> > And I'd rather do that in a separate patch, to avoid cluttering the poor
> > devicetree people with an unrelated 28-piece patch :-)
> 
> No (really), according to Grant and Mark during the closing session, I
> asked this specific question.  They _do_ want the entire series so they
> can refer to the corresponding code changes if necessary.  As I stated
> above, we can make their job easier by making the binding a separate
> patch that is clearly marked as such.

Interesting, because I had understood exactly the opposite from the
discussions at the ARM kernel summit. But maybe the DT folks changed
their mind between the ARM kernel summit and the closing session of the
kernel summit :-)

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 25/28] ARM: mvebu: Add support for NAND controller in Armada 370/XP

[Jason Cooper]

Thomas,

Adding Grant and Mark to the Cc to make sure they don't have buyer's
remorse ;-)

On Wed, Nov 06, 2013 at 09:24:35AM +0100, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Tue, 5 Nov 2013 10:15:31 -0500, Jason Cooper wrote:
> 
> > > And I'd rather do that in a separate patch, to avoid cluttering the poor
> > > devicetree people with an unrelated 28-piece patch :-)
> > 
> > No (really), according to Grant and Mark during the closing session, I
> > asked this specific question.  They _do_ want the entire series so they
> > can refer to the corresponding code changes if necessary.  As I stated
> > above, we can make their job easier by making the binding a separate
> > patch that is clearly marked as such.
> 
> Interesting, because I had understood exactly the opposite from the
> discussions at the ARM kernel summit. But maybe the DT folks changed
> their mind between the ARM kernel summit and the closing session of the
> kernel summit :-)

Yes, that's why I asked the question.  Mark Rutland said he wanted to
see the code changes when he was reviewing a binding.  Which is indeed
the opposite of what we had arrived at during the ARM mini-summit.

So I asked Grant (last question of the Q&A) what he wanted us to tell
the contributors.  He said (bad paraphrasing here) "I have better
filters now for finding the binding changes, so send the whole series"

As him and Mark aren't the only reviewers, I took the extra step of
saying send the DT bindings as a separate patch with it clearly
identified in the subject line...

thx,

Jason.

[PATCH v3 25/28] ARM: mvebu: Add support for NAND controller in Armada 370/XP

[Thomas Petazzoni]

Dear Jason Cooper,

On Wed, 6 Nov 2013 06:42:44 -0500, Jason Cooper wrote:

> > Interesting, because I had understood exactly the opposite from the
> > discussions at the ARM kernel summit. But maybe the DT folks changed
> > their mind between the ARM kernel summit and the closing session of
> > the kernel summit :-)
> 
> Yes, that's why I asked the question.  Mark Rutland said he wanted to
> see the code changes when he was reviewing a binding.  Which is indeed
> the opposite of what we had arrived at during the ARM mini-summit.
> 
> So I asked Grant (last question of the Q&A) what he wanted us to tell
> the contributors.  He said (bad paraphrasing here) "I have better
> filters now for finding the binding changes, so send the whole series"
> 
> As him and Mark aren't the only reviewers, I took the extra step of
> saying send the DT bindings as a separate patch with it clearly
> identified in the subject line...

Ok, thanks for the clarification. Those rules seem to be changing from
one day to the next, not really easy to follow :)

Is there some .txt file in Documentation/ that explains what the DT
maintainers expect?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 25/28] ARM: mvebu: Add support for NAND controller in Armada 370/XP

[Jason Cooper]

On Wed, Nov 06, 2013 at 01:56:54PM +0100, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Wed, 6 Nov 2013 06:42:44 -0500, Jason Cooper wrote:
> 
> > > Interesting, because I had understood exactly the opposite from the
> > > discussions at the ARM kernel summit. But maybe the DT folks changed
> > > their mind between the ARM kernel summit and the closing session of
> > > the kernel summit :-)
> > 
> > Yes, that's why I asked the question.  Mark Rutland said he wanted to
> > see the code changes when he was reviewing a binding.  Which is indeed
> > the opposite of what we had arrived at during the ARM mini-summit.
> > 
> > So I asked Grant (last question of the Q&A) what he wanted us to tell
> > the contributors.  He said (bad paraphrasing here) "I have better
> > filters now for finding the binding changes, so send the whole series"
> > 
> > As him and Mark aren't the only reviewers, I took the extra step of
> > saying send the DT bindings as a separate patch with it clearly
> > identified in the subject line...
> 
> Ok, thanks for the clarification. Those rules seem to be changing from
> one day to the next, not really easy to follow :)
> 
> Is there some .txt file in Documentation/ that explains what the DT
> maintainers expect?

hmmm.  Give me a bit. :)

thx,

Jason.

[PATCH v3 26/28] ARM: mvebu: Enable NAND controller in Armada XP GP board

[Ezequiel Garcia]

The Armada XP GP board has a NAND flash, so enable it in the devicetree.

In order to skip the driver's custom device detection and use only ONFI
detection, the "marvell,keep-config" parameter is used.
This is needed because we haven't support for setting the timings
parameters yet and must rely in bootloader's.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-xp-gp.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
index 2298e4a..274e2ad 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -175,6 +175,14 @@
 					spi-max-frequency = <108000000>;
 				};
 			};
+
+			nand at d0000 {
+				status = "okay";
+				num-cs = <1>;
+				marvell,nand-keep-config;
+				marvell,nand-enable-arbiter;
+				nand-on-flash-bbt;
+			};
 		};
 	};
 };
-- 
1.8.1.5

[PATCH v3 27/28] ARM: mvebu: Enable NAND controller in Armada 370 Mirabox

[Ezequiel Garcia]

The Armada 370 Mirabox has a NAND flash, so enable it in the devicetree
and add the partitions as prepared in the factory images.

In order to skip the driver's custom device detection and use only
ONFI detection, the "marvell,keep-config" parameter is used.
This is needed because we have no support for setting the timings parameters yet.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-mirabox.dts | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts b/arch/arm/boot/dts/armada-370-mirabox.dts
index 2471d9d..1e73017 100644
--- a/arch/arm/boot/dts/armada-370-mirabox.dts
+++ b/arch/arm/boot/dts/armada-370-mirabox.dts
@@ -139,6 +139,27 @@
 					reg = <0x25>;
 				};
 			};
+
+			nand at d0000 {
+				status = "okay";
+				num-cs = <1>;
+				marvell,nand-keep-config;
+				marvell,nand-enable-arbiter;
+				nand-on-flash-bbt;
+
+				partition at 0 {
+					label = "U-Boot";
+					reg = <0 0x400000>;
+				};
+				partition at 400000 {
+					label = "Linux";
+					reg = <0x400000 0x400000>;
+				};
+				partition at 800000 {
+					label = "Filesystem";
+					reg = <0x800000 0x3f800000>;
+				};
+			};
 		};
 	};
 };
-- 
1.8.1.5

[PATCH v3 28/28] mtd: nand: pxa3xx: Add documentation about the controller

[Ezequiel Garcia]

Given there's no public specification to this date, and in order
to capture some important details and singularities about the
controller let's document them once and for good.

Cc: linux-doc at vger.kernel.org
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 Documentation/mtd/nand/pxa3xx-nand.txt | 101 +++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)
 create mode 100644 Documentation/mtd/nand/pxa3xx-nand.txt

diff --git a/Documentation/mtd/nand/pxa3xx-nand.txt b/Documentation/mtd/nand/pxa3xx-nand.txt
new file mode 100644
index 0000000..912f7c6
--- /dev/null
+++ b/Documentation/mtd/nand/pxa3xx-nand.txt
@@ -0,0 +1,101 @@
+
+About this document
+===================
+
+Some notes about Marvell's NAND controller available in PXA and Armada 370/XP
+SoC (aka NFCv1 and NFCv2), with an emphasis on the latter.
+
+NFCv2 controller background
+===========================
+
+The controller has a 2176 bytes FIFO buffer. Therefore, in order to support
+larger pages, I/O operations on 4 KiB and 8 KiB pages is done with a set of
+chunked transfers.
+
+For instance, if we choose a 2048 data chunk and set "BCH" ECC (see below)
+we'll have this layout in the pages:
+
+  ------------------------------------------------------------------------------
+  | 2048B data | 32B spare | 30B ECC || 2048B data | 32B spare | 30B ECC | ... |
+  ------------------------------------------------------------------------------
+
+The driver reads the data and spare portions independently and builds an internal
+buffer with this layout (in the 4 KiB page case):
+
+  ------------------------------------------
+  |     4096B data     |     64B spare     |
+  ------------------------------------------
+
+Also, for the READOOB command the driver disables the ECC and reads a 'spare + ECC'
+OOB, one per chunk read.
+
+  -------------------------------------------------------------------
+  |     4096B data     |  32B spare | 30B ECC | 32B spare | 30B ECC |
+  -------------------------------------------------------------------
+
+So, in order to achieve reading (for instance), we issue several READ0 commands
+(with some additional controller-specific magic) and read two chunks of 2080B
+(2048 data + 32 spare) each.
+The driver accomodates this data to expose the NAND core a contiguous buffer
+(4096 data + spare) or (4096 + spare + ECC + spare + ECC).
+
+ECC
+===
+
+The controller has built-in hardware ECC capabilities. In addition it is
+configurable between two modes: 1) Hamming, 2) BCH.
+
+Note that the actual BCH mode: BCH-4 or BCH-8 will depend on the way
+the controller is configured to transfer the data.
+
+In the BCH mode the ECC code will be calculated for each transfered chunk
+and expected to be located (when reading/programming) right after the spare
+bytes as the figure above shows.
+
+So, repeating the above scheme, a 2048B data chunk will be followed by 32B
+spare, and then the ECC controller will read/write the ECC code (30B in
+this case):
+
+  ------------------------------------
+  | 2048B data | 32B spare | 30B ECC |
+  ------------------------------------
+
+If the ECC mode is 'BCH' then the ECC is *always* 30 bytes long.
+If the ECC mode is 'Hamming' the ECC is 6 bytes long, for each 512B block.
+So in Hamming mode, a 2048B page will have a 24B ECC.
+
+Despite all of the above, the controller requires the driver to only read or
+write in multiples of 8-bytes, because the data buffer is 64-bits.
+
+OOB
+===
+
+Because of the above scheme, and because the "spare" OOB is really located in
+the middle of a page, spare OOB cannot be read or write independently of the
+data area. In other words, in order to read the OOB (aka READOOB), the entire
+page (aka READ0) has to be read.
+
+In the same sense, in order to write to the spare OOB the driver has to write
+an *entire* page.
+
+Factory bad blocks handling
+===========================
+
+Given the ECC BCH requires to layout the device's pages in a splitted
+data/OOB/data/OOB way, the controller has a view of the flash page that's
+different from the specified (aka the manufacturer's) view. In other words,
+
+Factory view:
+
+  -----------------------------------------------
+  |                    Data           |x  OOB   |
+  -----------------------------------------------
+
+Driver's view:
+
+  -----------------------------------------------
+  |      Data      | OOB |      Data   x  | OOB |
+  -----------------------------------------------
+
+It can be seen from the above, that the factory bad block marker must be
+searched within the 'data' region, and not in the usual OOB region.
-- 
1.8.1.5