[PATCH 00/21] Armada 370/XP NAND support

[PATCH 00/21] Armada 370/XP NAND support

[Ezequiel Garcia]

Hi everyone,

As some of you know, I'm working on implementing the long-awaited NAND
support in Armada 370/XP SoCs.

The controller is the same as PXA3XX (NFCv1), plus some changes,
and therefore it's called NFCv2. As expected, it should be supported
by pxa3xx-nand, plus some changes.

First of all I'd like to introduce the controller and explain how it
expects the data, ECC and OOB layout within a page. We might add this
as documentation inside the driver or in Documentation/mtd, so feel free
to ask anything that looks suspicious or is not clear enough.

* Page layout

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 BCH ECC we will have
to use this layout in the pages:

  ------------------------------------------------------------------------------
  | 2048B data | 32B spare | 30B ECC || 2048B data | 32B spare | 30B ECC | ... |
  ------------------------------------------------------------------------------

The last remaining area is unusable (i.e. wasted).

To match this, my current (already working!) implementation acts like
a layout "impedance matcher" to produce in an internal driver's buffer
this layout:

  ------------------------------------------
  |     4096B data     |     64B spare     |
  ------------------------------------------

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 + 64 spare) each.

The driver accomodates this data to expose the NAND base a contiguous 4160B buffer
(4096 data + 64 spare).

* ECC

The controller has built-in hardware ECC capabilities. In addition it is completely
configurable, and we can choose between Hamming and BCH ECC. If we choose BCH ECC
then the ECC code will be calculated for each transfered chunk and expected to be
located (when reading/programming) at specific locations.

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

* 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 (aka SEQIN,
column = 4096 + PAGEPROG) the driver has to read the entire page first to the
driver's buffer. Then it should fill the OOB, and finally program the entire page,
data and oob included.

Notice, that while this is possible, I haven't included OOB only write support
(i.e. OOB write without data write) in this first patchset. I'm not sure why
would we need it, so I'd like to know others opinion on this matter.

Of course, writing to the OOB is supported, as long as this write is done
*with* data. Following the examples above, writing an entire 4096 + OOB page
is supported.

* Clocking and timings

This first patchset adds a dummy nand-clock to the device tree just to allow
the driver to get it. Timings configuration is overriden for now using the
'keep-config' DT parameter. The next round will likely include proper clock
support plus timings configuration.

* About this patchset

This has been tested on Armada 370 Mirabox and Armada XP GP boards.

Currently nandtest, mtd_pagetest, mtd_readtest pass while mtd_oobtest fails
(due to lack of OOB write support). This means that JFFS2 is not supported,
and UBIFS is a must.

The patches are based in l2-mtd's master branch and I've pushed a branch
to our github in case anyone wants to test this:

  https://github.com/MISL-EBU-System-SW/mainline-public/tree/pxa3xx-armada-nand-v1

The series is fairy complex and lengthy, so any feedback is more than welcome,
as well as questions, suggestions or flames. Also, if anyone has a PXA board
(Daniel?) to test possible regressions I would really appreciate it.

* A note about Mirabox testing

As this work is not considered completely stable, be extra careful when trying
on the Mirabox. The Mirabox has the bootloader at NAND, and there's some risk
to brick the board.

That said: I've been using the driver on my Mirabox all morning (with my own
Buildroot prepared UBIFS image) and so far everything works fine.

If despite this you happen to brick the board, Willy Tarreau has provided
excellent instructions to un-brick the Mirabox:

  http://1wt.eu/articles/mirabox-vs-guruplug/

Thanks!

Ezequiel Garcia (21):
  mtd: nand: pxa3xx: Allocate data buffer on detected flash size
  mtd: nand: pxa3xx: Disable OOB on arbitrary length commands
  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: Configure detected pages-per-block
  mtd: nand: pxa3xx: Clear cmd buffer #3 (NDCB3) on command start
  mtd: nand: pxa3xx: Make config menu show supported platforms
  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 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: Add a read/write buffers markers
  mtd: nand: pxa3xx: Introduce multiple page I/O support
  mtd: nand: pxa3xx: Add multiple chunk write support
  ARM: mvebu: Add a fixed 0Hz clock to represent NAND clock
  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

 arch/arm/boot/dts/armada-370-mirabox.dts      |  21 +
 arch/arm/boot/dts/armada-370-xp.dtsi          |  19 +
 arch/arm/boot/dts/armada-xp-gp.dts            |   8 +
 drivers/mtd/nand/Kconfig                      |   4 +-
 drivers/mtd/nand/pxa3xx_nand.c                | 546 +++++++++++++++++++++-----
 include/linux/platform_data/mtd-nand-pxa3xx.h |   3 +
 6 files changed, 493 insertions(+), 108 deletions(-)

-- 
1.8.1.5

[PATCH 01/21] mtd: nand: pxa3xx: Allocate data buffer on detected flash size

[Ezequiel Garcia]

This commit replaces the currently hardcoded buffer size, by a
dynamic detection scheme. First a small 256 bytes buffer is allocated
so the device can be detected (using READID and friends commands).

After detection, this buffer is released and a new buffer is allocated
to acommodate the page size plus out-of-band size.

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

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index dd03dfd..b4f3784 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -39,6 +39,13 @@
 #define NAND_STOP_DELAY		(2 * HZ/50)
 #define PAGE_CHUNK_SIZE		(2048)
 
+/*
+ * Define a buffer size for the initial command that detects the flash device:
+ * STATUS, READID and PARAM. The largest of these is the PARAM command,
+ * needing 256 bytes.
+ */
+#define INIT_BUFFER_SIZE	256
+
 /* registers and bit definitions */
 #define NDCR		(0x00) /* Control register */
 #define NDTR0CS0	(0x04) /* Timing Parameter 0 for CS0 */
@@ -164,6 +171,7 @@ struct pxa3xx_nand_info {
 
 	unsigned int 		buf_start;
 	unsigned int		buf_count;
+	unsigned int		buf_size;
 
 	/* DMA information */
 	int			drcmr_dat;
@@ -912,26 +920,20 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 	return 0;
 }
 
-/* the maximum possible buffer size for large page with OOB data
- * is: 2048 + 64 = 2112 bytes, allocate a page here for both the
- * data buffer and the DMA descriptor
- */
-#define MAX_BUFF_SIZE	PAGE_SIZE
-
 #ifdef ARCH_HAS_DMA
 static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
 {
 	struct platform_device *pdev = info->pdev;
-	int data_desc_offset = MAX_BUFF_SIZE - sizeof(struct pxa_dma_desc);
+	int data_desc_offset = info->buf_size - sizeof(struct pxa_dma_desc);
 
 	if (use_dma == 0) {
-		info->data_buff = kmalloc(MAX_BUFF_SIZE, GFP_KERNEL);
+		info->data_buff = kmalloc(info->buf_size, GFP_KERNEL);
 		if (info->data_buff == NULL)
 			return -ENOMEM;
 		return 0;
 	}
 
-	info->data_buff = dma_alloc_coherent(&pdev->dev, MAX_BUFF_SIZE,
+	info->data_buff = dma_alloc_coherent(&pdev->dev, info->buf_size,
 				&info->data_buff_phys, GFP_KERNEL);
 	if (info->data_buff == NULL) {
 		dev_err(&pdev->dev, "failed to allocate dma buffer\n");
@@ -945,7 +947,7 @@ static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
 				pxa3xx_nand_data_dma_irq, info);
 	if (info->data_dma_ch < 0) {
 		dev_err(&pdev->dev, "failed to request data dma\n");
-		dma_free_coherent(&pdev->dev, MAX_BUFF_SIZE,
+		dma_free_coherent(&pdev->dev, info->buf_size,
 				info->data_buff, info->data_buff_phys);
 		return info->data_dma_ch;
 	}
@@ -958,7 +960,7 @@ static void pxa3xx_nand_free_buff(struct pxa3xx_nand_info *info)
 	struct platform_device *pdev = info->pdev;
 	if (use_dma) {
 		pxa_free_dma(info->data_dma_ch);
-		dma_free_coherent(&pdev->dev, MAX_BUFF_SIZE,
+		dma_free_coherent(&pdev->dev, info->buf_size,
 				  info->data_buff, info->data_buff_phys);
 	} else {
 		kfree(info->data_buff);
@@ -967,7 +969,7 @@ static void pxa3xx_nand_free_buff(struct pxa3xx_nand_info *info)
 #else
 static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
 {
-	info->data_buff = kmalloc(MAX_BUFF_SIZE, GFP_KERNEL);
+	info->data_buff = kmalloc(info->buf_size, GFP_KERNEL);
 	if (info->data_buff == NULL)
 		return -ENOMEM;
 	return 0;
@@ -1081,7 +1083,16 @@ KEEP_CONFIG:
 	else
 		host->col_addr_cycles = 1;
 
+	/* release the initial buffer */
+	kfree(info->data_buff);
+
+	/* allocate the real data + oob buffer */
+	info->buf_size = mtd->writesize + mtd->oobsize;
+	ret = pxa3xx_nand_init_buff(info);
+	if (ret)
+		return ret;
 	info->oob_buff = info->data_buff + mtd->writesize;
+
 	if ((mtd->size >> chip->page_shift) > 65536)
 		host->row_addr_cycles = 3;
 	else
@@ -1187,9 +1198,13 @@ static int alloc_nand_resource(struct platform_device *pdev)
 	}
 	info->mmio_phys = r->start;
 
-	ret = pxa3xx_nand_init_buff(info);
-	if (ret)
+	/* Allocate a buffer to allow flash detection */
+	info->buf_size = INIT_BUFFER_SIZE;
+	info->data_buff = kmalloc(info->buf_size, GFP_KERNEL);
+	if (info->data_buff == NULL) {
+		ret = -ENOMEM;
 		goto fail_disable_clk;
+	}
 
 	/* initialize all interrupts to be disabled */
 	disable_int(info, NDSR_MASK);
@@ -1207,7 +1222,7 @@ static int alloc_nand_resource(struct platform_device *pdev)
 
 fail_free_buf:
 	free_irq(irq, info);
-	pxa3xx_nand_free_buff(info);
+	kfree(info->data_buff);
 fail_disable_clk:
 	clk_disable_unprepare(info->clk);
 	return ret;
-- 
1.8.1.5

Re: [PATCH 01/21] mtd: nand: pxa3xx: Allocate data buffer on detected flash size

[Brian Norris]

On Thu, Sep 19, 2013 at 01:01:25PM -0300, Ezequiel Garcia wrote:
> This commit replaces the currently hardcoded buffer size, by a
> dynamic detection scheme. First a small 256 bytes buffer is allocated
> so the device can be detected (using READID and friends commands).
> 
> After detection, this buffer is released and a new buffer is allocated
> to acommodate the page size plus out-of-band size.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

This is the patch that breaks Daniel's DMA setup, right? It looks a bit
off. I'll wait to comment on it much until v2.

BTW, there is a similar issue with at least one other driver (denali.c,
maybe others) where the driver uses some hard assumptions about the
maximum page/OOB sizes (NAND_MAX_PAGESIZE and NAND_MAX_OOBSIZE). This is
kind of ugly and not very sustainable/maintainable, since these
dimensions keep increasing. I appreciate your effort to avoid simply
kludging in a larger MAX_BUFF_SIZE :) I had similar plans for the other
drivers, but I don't know if we'll have much testing opportunities for
the ancient ones...

Also, it seems like your driver has a few potential leaks right now. If
alloc_nand_resource() succeeds but pxa3xx_nand_probe() later fails
(e.g., in pxa3xx_nand_scan()), you don't clean up after yourself. You
should address this soon, even if not in this patch series.

Brian

Re: [PATCH 01/21] mtd: nand: pxa3xx: Allocate data buffer on detected flash size

[Ezequiel Garcia]

On Thu, Sep 26, 2013 at 01:10:32PM -0700, Brian Norris wrote:
> On Thu, Sep 19, 2013 at 01:01:25PM -0300, Ezequiel Garcia wrote:
> > This commit replaces the currently hardcoded buffer size, by a
> > dynamic detection scheme. First a small 256 bytes buffer is allocated
> > so the device can be detected (using READID and friends commands).
> > 
> > After detection, this buffer is released and a new buffer is allocated
> > to acommodate the page size plus out-of-band size.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 
> This is the patch that breaks Daniel's DMA setup, right? It looks a bit
> off.

What do you mean by 'a bit off'?

> I'll wait to comment on it much until v2.
> 

Ok... let me re-work it then and prepare the v2.

> BTW, there is a similar issue with at least one other driver (denali.c,
> maybe others) where the driver uses some hard assumptions about the
> maximum page/OOB sizes (NAND_MAX_PAGESIZE and NAND_MAX_OOBSIZE). This is
> kind of ugly and not very sustainable/maintainable, since these
> dimensions keep increasing. I appreciate your effort to avoid simply
> kludging in a larger MAX_BUFF_SIZE :) I had similar plans for the other
> drivers, but I don't know if we'll have much testing opportunities for
> the ancient ones...
> 
> Also, it seems like your driver has a few potential leaks right now. If
> alloc_nand_resource() succeeds but pxa3xx_nand_probe() later fails
> (e.g., in pxa3xx_nand_scan()), you don't clean up after yourself. You
> should address this soon, even if not in this patch series.
> 

Hm.. are you sure about that? AFAICS, there's no leak at all.

If alloc_nand_resource() succeeds, the only leakable resources allocated
are the ones allocated at pxa3xx_nand_init_buff() and the NAND base
stuff.

If pxa3xx_nand_probe() later fails to complete, it calls pxa3xx_nand_remove()
in this part:

	if (!probe_success) {
		pxa3xx_nand_remove(pdev);
		return -ENODEV;
	}

Which takes care of cleaning both the buffers and the NAND base stuff.

Or am I missing something?

Feel free to ask more questions and thanks a lot for the feedback.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 01/21] mtd: nand: pxa3xx: Allocate data buffer on detected flash size

[Brian Norris]

On Mon, Sep 30, 2013 at 09:37:57AM -0300, Ezequiel Garcia wrote:
> On Thu, Sep 26, 2013 at 01:10:32PM -0700, Brian Norris wrote:
> > On Thu, Sep 19, 2013 at 01:01:25PM -0300, Ezequiel Garcia wrote:
> > > This commit replaces the currently hardcoded buffer size, by a
> > > dynamic detection scheme. First a small 256 bytes buffer is allocated
> > > so the device can be detected (using READID and friends commands).
> > > 
> > > After detection, this buffer is released and a new buffer is allocated
> > > to acommodate the page size plus out-of-band size.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > 
> > This is the patch that breaks Daniel's DMA setup, right? It looks a bit
> > off.
> 
> What do you mean by 'a bit off'?

The existing code allocates data_buff with kmalloc() or
dma_alloc_coherent() depending on ARCH_HAS_DMA, but your patch does the
initial fixed-size allocation only with kmalloc(). It's just what you
and Daniel already identified: that this causes problems for the DMA
case.

> > BTW, there is a similar issue with at least one other driver (denali.c,
> > maybe others) where the driver uses some hard assumptions about the
> > maximum page/OOB sizes (NAND_MAX_PAGESIZE and NAND_MAX_OOBSIZE). This is
> > kind of ugly and not very sustainable/maintainable, since these
> > dimensions keep increasing. I appreciate your effort to avoid simply
> > kludging in a larger MAX_BUFF_SIZE :) I had similar plans for the other
> > drivers, but I don't know if we'll have much testing opportunities for
> > the ancient ones...
> > 
> > Also, it seems like your driver has a few potential leaks right now. If
> > alloc_nand_resource() succeeds but pxa3xx_nand_probe() later fails
> > (e.g., in pxa3xx_nand_scan()), you don't clean up after yourself. You
> > should address this soon, even if not in this patch series.
> > 
> 
> Hm.. are you sure about that? AFAICS, there's no leak at all.

No, I'm not sure :)

> If alloc_nand_resource() succeeds, the only leakable resources allocated
> are the ones allocated at pxa3xx_nand_init_buff() and the NAND base
> stuff.
> 
> If pxa3xx_nand_probe() later fails to complete, it calls pxa3xx_nand_remove()
> in this part:
> 
> 	if (!probe_success) {
> 		pxa3xx_nand_remove(pdev);
> 		return -ENODEV;
> 	}
> 
> Which takes care of cleaning both the buffers and the NAND base stuff.
> 
> Or am I missing something?

Nope, you were correct. I just missed this point when reading.

Brian

[PATCH 02/21] mtd: nand: pxa3xx: Disable OOB on arbitrary length commands

[Ezequiel Garcia]

READID, STATUS and PARAM (aka ONFI read paramater page) don't read
the OOB area. Set the oob_size to zero and prevent it.
Also, add a comment clarifying the use of pxa3xx_set_datasize()
which is only applicable on data read/write commands.

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

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index b4f3784..a8d2ea7 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -280,6 +280,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)
 {
 	struct pxa3xx_nand_host *host = info->host[info->cs];
@@ -641,6 +646,7 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 		info->ndcb1 = (column & 0xFF);
 		info->ndcb3 = 256;
 		info->data_size = 256;
+		info->oob_size = 0;
 		break;
 
 	case NAND_CMD_READID:
@@ -651,6 +657,7 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 		info->ndcb1 = (column & 0xFF);
 
 		info->data_size = 8;
+		info->oob_size = 0;
 		break;
 	case NAND_CMD_STATUS:
 		info->buf_count = 1;
@@ -659,6 +666,7 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 				| command;
 
 		info->data_size = 8;
+		info->oob_size = 0;
 		break;
 
 	case NAND_CMD_ERASE1:
-- 
1.8.1.5

[PATCH 03/21] 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 a8d2ea7..fba91c2 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)
@@ -167,7 +168,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;
@@ -197,7 +198,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		page_size;	/* page size of attached chip */
 	unsigned int		data_size;	/* data size in FIFO */
@@ -457,7 +464,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) {
@@ -493,8 +500,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) {
@@ -523,6 +531,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;
 }
@@ -554,7 +564,6 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	info->use_ecc		= 0;
 	info->use_spare		= 1;
 	info->use_dma		= (use_dma) ? 1 : 0;
-	info->is_ready		= 0;
 	info->retcode		= ERR_NONE;
 	if (info->cs != 0)
 		info->ndcb0 = NDCB0_CSEL;
@@ -730,6 +739,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,
@@ -842,21 +853,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,
@@ -1000,7 +1017,7 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
 		return ret;
 
 	pxa3xx_nand_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

Re: [PATCH 03/21] mtd: nand: pxa3xx: Use a completion to signal device ready

[Brian Norris]

On Thu, Sep 19, 2013 at 01:01:27PM -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.

Does it really need to be an atomic type? AIUI, you can hand off exclusive
control to the hardware when you begin a command. So the rest of the driver would be
inactive when the IRQ handler is setting the ready flag. And similarly,
the hardware should be inactive (i.e., no interrupts) when you are
modifying the ready flag in the driver (i.e., in cmdfunc).

Or perhaps I'm misunderstanding something.

> 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 a8d2ea7..fba91c2 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)
> @@ -167,7 +168,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;

Do you really need two completion structures? Does 'device ready' always
follow 'command complete', so that you can just use 'device ready' all
the time? And if so, does it make sense to just use dev_ready?

>  
>  	unsigned int 		buf_start;
>  	unsigned int		buf_count;

Brian

Re: [PATCH 03/21] mtd: nand: pxa3xx: Use a completion to signal device ready

[Ezequiel Garcia]

Brian,

First of all: thanks for all your nice feedback.
I'm going through all of it right now, and just started
by this one.

On Wed, Oct 02, 2013 at 02:56:28PM -0700, Brian Norris wrote:
> On Thu, Sep 19, 2013 at 01:01:27PM -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.
> 
> Does it really need to be an atomic type? AIUI, you can hand off exclusive
> control to the hardware when you begin a command. So the rest of the driver would be
> inactive when the IRQ handler is setting the ready flag. And similarly,
> the hardware should be inactive (i.e., no interrupts) when you are
> modifying the ready flag in the driver (i.e., in cmdfunc).
> 

Hm... I think that's not the case, and that's exactly why I had to add
an atomic type. See below.

> Or perhaps I'm misunderstanding something.
> 
> > 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 a8d2ea7..fba91c2 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)
> > @@ -167,7 +168,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;
> 
> Do you really need two completion structures? Does 'device ready' always
> follow 'command complete', so that you can just use 'device ready' all
> the time? And if so, does it make sense to just use dev_ready?
> 
> >  
> >  	unsigned int 		buf_start;
> >  	unsigned int		buf_count;

The idea here is to handle the following situation. After a command is
completed (which also applies to the 'chunked' page command we want to
support later in the patchset) the controller sets a 'command done' flag
in the ISR.

The interruption handler calls the command done completion which causes
the cmdfunc to finish waiting and exit (or continue executing commands
in the 'chunked' case).

Notice that this does *not* guarantee the controller is ready since the
'ready' flag is independent of the 'command complete' flag and the IRQ can
arrive at any later point.

Let's suppose the cmdfunc() has exited, and the NAND base code is going to
continue the operation. In the PAGEPROG case, the NAND base calls waitfunc()
before continuing the operation.

At that point, we **need** to wait for the 'ready' flag because otherwise
the NAND base could issue another PAGEPROG command, while the controller
is not really ready to receive it.

However, if we enter the waitfunc() we want to read the info->is_ready flag
to check the completion has been initialized and a wait is suitable.
Otherwise, some commands that are not issued (with no wait completion
initialized) will stall at that waitfunc().

While at the waitfunc() the interruption handler might get called for
the 'ready' flag and so the info->is_ready variable gets access from two
different execution contexts.

Given the above behavior, I thought the only way of dealing with this
was to add a new completion and use atomic type for the is_ready flag.

Is the above clear? Do you think the implementation is correct for the
behavior?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 03/21] mtd: nand: pxa3xx: Use a completion to signal device ready

[Ezequiel Garcia]

On Fri, Oct 04, 2013 at 03:54:54PM -0300, Ezequiel Garcia wrote:
> Brian,
> 
> First of all: thanks for all your nice feedback.
> I'm going through all of it right now, and just started
> by this one.
> 
> On Wed, Oct 02, 2013 at 02:56:28PM -0700, Brian Norris wrote:
> > On Thu, Sep 19, 2013 at 01:01:27PM -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.
> > 
> > Does it really need to be an atomic type? AIUI, you can hand off exclusive
> > control to the hardware when you begin a command. So the rest of the driver would be
> > inactive when the IRQ handler is setting the ready flag. And similarly,
> > the hardware should be inactive (i.e., no interrupts) when you are
> > modifying the ready flag in the driver (i.e., in cmdfunc).
> > 
> 
> Hm... I think that's not the case, and that's exactly why I had to add
> an atomic type. See below.
> 
> > Or perhaps I'm misunderstanding something.
> > 
> > > 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 a8d2ea7..fba91c2 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)
> > > @@ -167,7 +168,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;
> > 
> > Do you really need two completion structures? Does 'device ready' always
> > follow 'command complete', so that you can just use 'device ready' all
> > the time? And if so, does it make sense to just use dev_ready?
> > 
> > >  
> > >  	unsigned int 		buf_start;
> > >  	unsigned int		buf_count;
> 
> The idea here is to handle the following situation. After a command is
> completed (which also applies to the 'chunked' page command we want to
> support later in the patchset) the controller sets a 'command done' flag
> in the ISR.
> 
> The interruption handler calls the command done completion which causes
> the cmdfunc to finish waiting and exit (or continue executing commands
> in the 'chunked' case).
> 
> Notice that this does *not* guarantee the controller is ready since the
> 'ready' flag is independent of the 'command complete' flag and the IRQ can
> arrive at any later point.
> 
> Let's suppose the cmdfunc() has exited, and the NAND base code is going to
> continue the operation. In the PAGEPROG case, the NAND base calls waitfunc()
> before continuing the operation.
> 
> At that point, we **need** to wait for the 'ready' flag because otherwise
> the NAND base could issue another PAGEPROG command, while the controller
> is not really ready to receive it.
> 
> However, if we enter the waitfunc() we want to read the info->is_ready flag
> to check the completion has been initialized and a wait is suitable.
> Otherwise, some commands that are not issued (with no wait completion
> initialized) will stall at that waitfunc().
> 
> While at the waitfunc() the interruption handler might get called for
> the 'ready' flag and so the info->is_ready variable gets access from two
> different execution contexts.
> 
> Given the above behavior, I thought the only way of dealing with this
> was to add a new completion and use atomic type for the is_ready flag.
> 
> Is the above clear? Do you think the implementation is correct for the
> behavior?

Brian:

I'm picking up on my NAND work, and wanted to know if you have any
comments about this patch. Otherwise, I'll just leave it as it is.

FWIW, any comments about the locking scheme is always highly appreciated
as it's a complex and important issue, so feel free to ask any questions
or suggest any improvements.

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

[PATCH 04/21] 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                | 31 +++++++++++++++++++++++++++
 include/linux/platform_data/mtd-nand-pxa3xx.h |  3 +++
 2 files changed, 34 insertions(+)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index fba91c2..a0dabe6 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
@@ -246,6 +247,29 @@ static struct pxa3xx_nand_flash builtin_flash_types[] = {
 { "256MiB 16-bit", 0xba20,  64, 2048, 16, 16, 2048, &timing[3] },
 };
 
+static u8 bb_pattern[] = {'M', 'V', 'B', 'b', 't', '0' };
+static u8 bb_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 = bb_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 = bb_mirror_pattern
+};
+
 /* Define a default flash type setting serve as flash detecting only */
 #define DEFAULT_FLASH_TYPE (&builtin_flash_types[0])
 
@@ -1100,6 +1124,12 @@ KEEP_CONFIG:
 	if (info->reg_ndcr & NDCR_DWIDTH_M)
 		chip->options |= NAND_BUSWIDTH_16;
 
+	if (pdata->flash_bbt) {
+		chip->bbt_td = &bbt_main_descr;
+		chip->bbt_md = &bbt_mirror_descr;
+		chip->bbt_options |= NAND_BBT_USE_FLASH;
+	}
+
 	if (nand_scan_ident(mtd, 1, def))
 		return -ENODEV;
 	/* calculate addressing information */
@@ -1318,6 +1348,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 05/21] 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 | 53 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index a0dabe6..86c343e 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 */
@@ -197,6 +198,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 ? */
 
@@ -270,6 +272,12 @@ static struct nand_bbt_descr bbt_mirror_descr = {
 	.pattern = bb_mirror_pattern
 };
 
+static struct nand_ecclayout ecc_layout_4KB_bch4bit = {
+	.eccbytes = 0,
+	.eccpos = {},
+	.oobfree = {}
+};
+
 /* Define a default flash type setting serve as flash detecting only */
 #define DEFAULT_FLASH_TYPE (&builtin_flash_types[0])
 
@@ -329,7 +337,10 @@ static void pxa3xx_set_datasize(struct pxa3xx_nand_info *info)
 
 	switch (host->page_size) {
 	case 2048:
-		info->oob_size = (info->use_ecc) ? 40 : 64;
+		if (info->ecc_bch)
+			info->oob_size = (info->use_ecc) ? 32 : 64;
+		else
+			info->oob_size = (info->use_ecc) ? 40 : 64;
 		break;
 	case 512:
 		info->oob_size = (info->use_ecc) ? 8 : 16;
@@ -349,10 +360,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;
@@ -1117,10 +1133,6 @@ 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 = host->page_size;
-	chip->ecc.strength = 1;
-
 	if (info->reg_ndcr & NDCR_DWIDTH_M)
 		chip->options |= NAND_BUSWIDTH_16;
 
@@ -1130,8 +1142,35 @@ KEEP_CONFIG:
 		chip->bbt_options |= NAND_BBT_USE_FLASH;
 	}
 
+	/* 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;
+
+	/* 1-step ECC over the entire detected page size */
+	chip->ecc.mode = NAND_ECC_HW;
+	chip->ecc.size = mtd->writesize;
+
+	/*
+	 * Armada370 variant supports BCH, which provides 4-bit strength
+	 * and needs to be supported in a special way.
+	 */
+	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&
+	    chip->ecc_strength_ds == 4) {
+	        chip->ecc.layout = &ecc_layout_4KB_bch4bit;
+		chip->ecc.strength = chip->ecc_strength_ds;
+		info->ecc_bch = 1;
+	} else if (mtd->writesize <= 2048) {
+		/*
+		 * This is the default ECC Hamming 1-bit strength case,
+		 * which works for page sizes of 512 and 2048 bytes.
+		 * The ECC layout will be automatically configured.
+		 */
+		chip->ecc.strength = 1;
+	}
+
 	/* calculate addressing information */
 	if (mtd->writesize >= 2048)
 		host->col_addr_cycles = 2;
-- 
1.8.1.5

Re: [PATCH 05/21] mtd: nand: pxa3xx: Add driver-specific ECC BCH support

[Brian Norris]

On Thu, Sep 19, 2013 at 01:01:29PM -0300, Ezequiel Garcia wrote:
> 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 | 53 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index a0dabe6..86c343e 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c

...

> @@ -1117,10 +1133,6 @@ 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 = host->page_size;
> -	chip->ecc.strength = 1;
> -
>  	if (info->reg_ndcr & NDCR_DWIDTH_M)
>  		chip->options |= NAND_BUSWIDTH_16;
>  
> @@ -1130,8 +1142,35 @@ KEEP_CONFIG:
>  		chip->bbt_options |= NAND_BBT_USE_FLASH;
>  	}
>  
> +	/* 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;
> +
> +	/* 1-step ECC over the entire detected page size */
> +	chip->ecc.mode = NAND_ECC_HW;
> +	chip->ecc.size = mtd->writesize;

Does the ECC really only work in one step? IOW, do you only correct
1-bit for the entire page? That is under-spec'd for most NAND (they need
1-bit/512-byte, or 4-bit/512-byte). This ecc.size should actually
reflect the correction region (typically 512B or 1024B), and even if
your buffer layout makes a step every 2KB (or 4KB or whatever), the ECC
step size *should* be smaller than that.

Or maybe I'm wrong, and you're actually using a weak 4-bit correction
over 2048 bytes.

> +
> +	/*
> +	 * Armada370 variant supports BCH, which provides 4-bit strength
> +	 * and needs to be supported in a special way.
> +	 */
> +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&

Might there be other variants eventually that support BCH? Perhaps you
want to separate the property of "can use BCH" from "will use BCH"?

> +	    chip->ecc_strength_ds == 4) {

Do you use BCH-4 only for chips that require exactly 4-bit correction?
Is there ever a need to "overclock" the ECC, and choose BCH-4 for chips
that only require 1-bit correction? Can the bootloader ever make a
choice different from yours here?

What about NAND that don't support the ecc_strength_ds field yet?
Remember, this is only filled for ONFI NAND and a few NAND that are in
the full-ID table.

> +	        chip->ecc.layout = &ecc_layout_4KB_bch4bit;

This seems to have an implicit page size assumption. In your case, it
seems like this layout should be chosen based on the page size + OOB
size, at least.

> +		chip->ecc.strength = chip->ecc_strength_ds;
> +		info->ecc_bch = 1;
> +	} else if (mtd->writesize <= 2048) {
> +		/*
> +		 * This is the default ECC Hamming 1-bit strength case,
> +		 * which works for page sizes of 512 and 2048 bytes.
> +		 * The ECC layout will be automatically configured.
> +		 */
> +		chip->ecc.strength = 1;
> +	}

What about the 'else' case? What happens with >2KB page NAND on
non-Armada370 systems? What happens if a NAND needs >4-bit correction?
Perhaps you could include a warning/error? I don't know the flexibility
of NAND types you'll find on an Armada board; maybe they're all
ONFI-compliant?

> +
>  	/* calculate addressing information */
>  	if (mtd->writesize >= 2048)
>  		host->col_addr_cycles = 2;

Brian

Re: [PATCH 05/21] mtd: nand: pxa3xx: Add driver-specific ECC BCH support

[Ezequiel Garcia]

On Wed, Oct 02, 2013 at 05:24:40PM -0700, Brian Norris wrote:
> On Thu, Sep 19, 2013 at 01:01:29PM -0300, Ezequiel Garcia wrote:
> > 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 | 53 ++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 46 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > index a0dabe6..86c343e 100644
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> 
> ...
> 
> > @@ -1117,10 +1133,6 @@ 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 = host->page_size;
> > -	chip->ecc.strength = 1;
> > -
> >  	if (info->reg_ndcr & NDCR_DWIDTH_M)
> >  		chip->options |= NAND_BUSWIDTH_16;
> >  
> > @@ -1130,8 +1142,35 @@ KEEP_CONFIG:
> >  		chip->bbt_options |= NAND_BBT_USE_FLASH;
> >  	}
> >  
> > +	/* 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;
> > +
> > +	/* 1-step ECC over the entire detected page size */
> > +	chip->ecc.mode = NAND_ECC_HW;
> > +	chip->ecc.size = mtd->writesize;
> 
> Does the ECC really only work in one step? IOW, do you only correct
> 1-bit for the entire page? That is under-spec'd for most NAND (they need
> 1-bit/512-byte, or 4-bit/512-byte). This ecc.size should actually
> reflect the correction region (typically 512B or 1024B), and even if
> your buffer layout makes a step every 2KB (or 4KB or whatever), the ECC
> step size *should* be smaller than that.
> 
> Or maybe I'm wrong, and you're actually using a weak 4-bit correction
> over 2048 bytes.
> 

I must admit I'm a bit confused by this ECC step concept. Having
described the ECC engine in the cover-letter, what do you think
the right choice should be?

IMO, given the ECC engine works on the entire FIFO (actually it works
on the transfered chunk) it's a 1-step ECC.

On the other side, a multiple step ECC, i.e. chip->ecc.size != mtd->writesize
makes the NAND base calls the subpage() function and the driver crashes
horribly when using UBIFS.

I'm not sure why subpage should be called in that case, given we don't
support that.

> > +
> > +	/*
> > +	 * Armada370 variant supports BCH, which provides 4-bit strength
> > +	 * and needs to be supported in a special way.
> > +	 */
> > +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&
> 
> Might there be other variants eventually that support BCH? Perhaps you
> want to separate the property of "can use BCH" from "will use BCH"?
> 

Yeah, probably. And maybe setting the BCH from the DT? The problem here
is that because of the ECC choice being intimately related to the OOB
location, this means the ECC choice is intimately related to the bad
block markers.

IOW, the user can't really choose among ECC.

I guess this means the ECC choice is some sort of a hardware description
and rightfully belongs to the DT. Right?

> > +	    chip->ecc_strength_ds == 4) {
> 
> Do you use BCH-4 only for chips that require exactly 4-bit correction?
> Is there ever a need to "overclock" the ECC, and choose BCH-4 for chips
> that only require 1-bit correction? Can the bootloader ever make a
> choice different from yours here?
> 
> What about NAND that don't support the ecc_strength_ds field yet?
> Remember, this is only filled for ONFI NAND and a few NAND that are in
> the full-ID table.
> 
> > +	        chip->ecc.layout = &ecc_layout_4KB_bch4bit;
> 
> This seems to have an implicit page size assumption. In your case, it
> seems like this layout should be chosen based on the page size + OOB
> size, at least.
> 
> > +		chip->ecc.strength = chip->ecc_strength_ds;
> > +		info->ecc_bch = 1;
> > +	} else if (mtd->writesize <= 2048) {
> > +		/*
> > +		 * This is the default ECC Hamming 1-bit strength case,
> > +		 * which works for page sizes of 512 and 2048 bytes.
> > +		 * The ECC layout will be automatically configured.
> > +		 */
> > +		chip->ecc.strength = 1;
> > +	}
> 
> What about the 'else' case? What happens with >2KB page NAND on
> non-Armada370 systems? What happens if a NAND needs >4-bit correction?
> Perhaps you could include a warning/error? I don't know the flexibility
> of NAND types you'll find on an Armada board; maybe they're all
> ONFI-compliant?
> 

Yeah, this 'ifs' could be fixed to better support other cases.

I'll try to rework that.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 05/21] mtd: nand: pxa3xx: Add driver-specific ECC BCH support

[Brian Norris]

On Fri, Oct 04, 2013 at 04:49:24PM -0300, Ezequiel Garcia wrote:
> On Wed, Oct 02, 2013 at 05:24:40PM -0700, Brian Norris wrote:
> > On Thu, Sep 19, 2013 at 01:01:29PM -0300, Ezequiel Garcia wrote:
> > > 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 | 53 ++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 46 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > > index a0dabe6..86c343e 100644
> > > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > 
> > ...
> > 
> > > @@ -1117,10 +1133,6 @@ 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 = host->page_size;
> > > -	chip->ecc.strength = 1;
> > > -
> > >  	if (info->reg_ndcr & NDCR_DWIDTH_M)
> > >  		chip->options |= NAND_BUSWIDTH_16;
> > >  
> > > @@ -1130,8 +1142,35 @@ KEEP_CONFIG:
> > >  		chip->bbt_options |= NAND_BBT_USE_FLASH;
> > >  	}
> > >  
> > > +	/* 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;
> > > +
> > > +	/* 1-step ECC over the entire detected page size */
> > > +	chip->ecc.mode = NAND_ECC_HW;
> > > +	chip->ecc.size = mtd->writesize;
> > 
> > Does the ECC really only work in one step? IOW, do you only correct
> > 1-bit for the entire page? That is under-spec'd for most NAND (they need
> > 1-bit/512-byte, or 4-bit/512-byte). This ecc.size should actually
> > reflect the correction region (typically 512B or 1024B), and even if
> > your buffer layout makes a step every 2KB (or 4KB or whatever), the ECC
> > step size *should* be smaller than that.
> > 
> > Or maybe I'm wrong, and you're actually using a weak 4-bit correction
> > over 2048 bytes.
> > 
> 
> I must admit I'm a bit confused by this ECC step concept. Having
> described the ECC engine in the cover-letter, what do you think
> the right choice should be?

I'm still not sure. But I'll defer that discussion to the cover-letter.

> IMO, given the ECC engine works on the entire FIFO (actually it works
> on the transfered chunk) it's a 1-step ECC.

That doesn't necessarily mean it's 1-step ECC. The step refers more to
the correction region than to the mechanics of transfer sizes.

> On the other side, a multiple step ECC, i.e. chip->ecc.size != mtd->writesize
> makes the NAND base calls the subpage() function and the driver crashes
> horribly when using UBIFS.

Which function? Read or write? AFAICT, neither relies on chip->ecc.size.

> I'm not sure why subpage should be called in that case, given we don't
> support that.

If you don't support writes, you can flag:
    chip->options |= NAND_NO_SUBPAGE_WRITE

And subpage reads are currently only enabled on soft-ECC with page size
larger than 512.

> > > +
> > > +	/*
> > > +	 * Armada370 variant supports BCH, which provides 4-bit strength
> > > +	 * and needs to be supported in a special way.
> > > +	 */
> > > +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&
> > 
> > Might there be other variants eventually that support BCH? Perhaps you
> > want to separate the property of "can use BCH" from "will use BCH"?
> > 
> 
> Yeah, probably. And maybe setting the BCH from the DT? The problem here
> is that because of the ECC choice being intimately related to the OOB
> location, this means the ECC choice is intimately related to the bad
> block markers.

Speaking of that: how do you deal with bad block markers, if your data
is actually placed in what is traditionally the "spare area"?

> IOW, the user can't really choose among ECC.

This means the choice of ECC must remain consistent across all users of
the NAND, but it doesn't mean there is no initial choice. A user could
choose to enable a higher ECC strength than what is technically required
by the flash.

> I guess this means the ECC choice is some sort of a hardware description
> and rightfully belongs to the DT. Right?

Yes, I think some description of ECC can be placed in DT. We're working
through a DT binding for Pekon's OMAP driver right now.

Brian

Re: [PATCH 05/21] mtd: nand: pxa3xx: Add driver-specific ECC BCH support

[Ezequiel Garcia]

Brian,

On Fri, Oct 04, 2013 at 05:27:13PM -0700, Brian Norris wrote:
[..]
> 
> Speaking of that: how do you deal with bad block markers, if your data
> is actually placed in what is traditionally the "spare area"?
> 

We currently don't deal with this :(

I'll be sending a v2 soon to address many of the issues you brought
and to re-work the ECC mode 'choice' to be safer.

I'll post the open issues in the v2 cover-letter, and this factory bad block
thing will be among them.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 06/21] mtd: nand: pxa3xx: Configure detected pages-per-block

[Ezequiel Garcia]

Instead of relying in the bootloader's value for the NDCR (NAND control
register), set the appropriate bits, corresponding to the detected
pages-per-block number.

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

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 86c343e..996a47d 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -71,14 +71,19 @@
 #define NDCR_PAGE_SZ		(0x1 << 24)
 #define NDCR_NCSX		(0x1 << 23)
 #define NDCR_ND_MODE		(0x3 << 21)
-#define NDCR_NAND_MODE   	(0x0)
 #define NDCR_CLR_PG_CNT		(0x1 << 20)
 #define NDCR_STOP_ON_UNCOR	(0x1 << 19)
 #define NDCR_RD_ID_CNT_MASK	(0x7 << 16)
 #define NDCR_RD_ID_CNT(x)	(((x) << 16) & NDCR_RD_ID_CNT_MASK)
 
 #define NDCR_RA_START		(0x1 << 15)
-#define NDCR_PG_PER_BLK		(0x1 << 14)
+
+#define NDCR_PG_PER_BLK_32	(0x0 << 13)
+#define NDCR_PG_PER_BLK_64	(0x2 << 13)
+#define NDCR_PG_PER_BLK_128	(0x1 << 13)
+#define NDCR_PG_PER_BLK_256	(0x3 << 13)
+#define NDCR_PG_PER_BLK_MASK	(0x3 << 13)
+
 #define NDCR_ND_ARB_EN		(0x1 << 12)
 #define NDCR_INT_MASK           (0xFFF)
 
@@ -948,7 +953,7 @@ static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
 
 	ndcr |= (pdata->enable_arbiter) ? NDCR_ND_ARB_EN : 0;
 	ndcr |= (host->col_addr_cycles == 2) ? NDCR_RA_START : 0;
-	ndcr |= (f->page_per_block == 64) ? NDCR_PG_PER_BLK : 0;
+	ndcr |= (f->page_per_block == 64) ? NDCR_PG_PER_BLK_64 : 0;
 	ndcr |= (f->page_size == 2048) ? NDCR_PAGE_SZ : 0;
 	ndcr |= (f->flash_width == 16) ? NDCR_DWIDTH_M : 0;
 	ndcr |= (f->dfc_width == 16) ? NDCR_DWIDTH_C : 0;
@@ -1074,6 +1079,7 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
 	struct nand_chip *chip = mtd->priv;
 	uint32_t id = -1;
 	uint64_t chipsize;
+	unsigned int pages_per_blk;
 	int i, ret, num;
 
 	if (pdata->keep_config && !pxa3xx_nand_detect_config(info))
@@ -1149,6 +1155,28 @@ KEEP_CONFIG:
 	if (nand_scan_ident(mtd, 1, def))
 		return -ENODEV;
 
+	pages_per_blk = mtd->erasesize / mtd->writesize;
+
+	/* Configure the pages-per-block */
+	switch (pages_per_blk) {
+	case 32:
+		info->reg_ndcr |= NDCR_PG_PER_BLK_32;
+		break;
+	case 64:
+		info->reg_ndcr |= NDCR_PG_PER_BLK_64;
+		break;
+	case 128:
+		info->reg_ndcr |= NDCR_PG_PER_BLK_128;
+		break;
+	case 256:
+		info->reg_ndcr |= NDCR_PG_PER_BLK_256;
+		break;
+	default:
+		dev_warn(&info->pdev->dev,
+			 "unsupported pages-per-block number (%d)\n",
+			 pages_per_blk);
+	}
+
 	/* 1-step ECC over the entire detected page size */
 	chip->ecc.mode = NAND_ECC_HW;
 	chip->ecc.size = mtd->writesize;
-- 
1.8.1.5

[PATCH 07/21] 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 996a47d..d8e4311 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -610,6 +610,7 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	info->use_spare		= 1;
 	info->use_dma		= (use_dma) ? 1 : 0;
 	info->retcode		= ERR_NONE;
+	info->ndcb3		= 0;
 	if (info->cs != 0)
 		info->ndcb0 = NDCB0_CSEL;
 	else
@@ -631,7 +632,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 08/21] 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 d885298..791d219 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -354,11 +354,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 09/21] 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 d8e4311..3f190fd 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -214,8 +214,8 @@ struct pxa3xx_nand_info {
 	 */
 	atomic_t		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;
 
@@ -331,16 +331,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:
 		if (info->ecc_bch)
 			info->oob_size = (info->use_ecc) ? 32 : 64;
@@ -977,9 +976,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 10/21] 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 | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 3f190fd..f3dfeba 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -157,7 +157,6 @@ struct pxa3xx_nand_host {
 	void			*info_data;
 
 	/* page size of attached chip */
-	unsigned int		page_size;
 	int			use_ecc;
 	int			cs;
 
@@ -649,12 +648,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);
 
@@ -939,7 +938,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 */
@@ -978,11 +976,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;
 	}
 
-- 
1.8.1.5

[PATCH 11/21] 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 f3dfeba..80f0105 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -588,6 +588,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)
 {
@@ -652,22 +672,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 12/21] 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 80f0105..ecf99ef 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -671,6 +671,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 13/21] 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 ecf99ef..562b968 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -608,18 +608,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;
@@ -629,10 +619,6 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	info->use_dma		= (use_dma) ? 1 : 0;
 	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:
@@ -644,14 +630,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 14/21] 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 562b968..6251939 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -610,6 +610,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;
@@ -635,6 +638,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,
@@ -676,16 +692,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 15/21] 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 6251939..0dc6fa4 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -179,6 +179,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;
@@ -328,11 +330,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) {
 		info->oob_size = 0;
 		return;
@@ -430,26 +433,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
@@ -617,6 +633,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->use_dma		= (use_dma) ? 1 : 0;
@@ -628,7 +646,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 16/21] 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 | 115 +++++++++++++++++++++++++++++++++++------
 1 file changed, 100 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 0dc6fa4..609f66e 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>
@@ -106,6 +120,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)
@@ -116,6 +132,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))
@@ -217,6 +241,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;
 	int 			retcode;
 
@@ -279,9 +304,18 @@ static struct nand_bbt_descr bbt_mirror_descr = {
 };
 
 static struct nand_ecclayout ecc_layout_4KB_bch4bit = {
-	.eccbytes = 0,
-	.eccpos = {},
-	.oobfree = {}
+	.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 = { {1, 4}, {6, 26}, { 64, 32} }
 };
 
 /* Define a default flash type setting serve as flash detecting only */
@@ -433,7 +467,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:
@@ -672,7 +706,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;
@@ -705,9 +739,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;
@@ -796,12 +841,12 @@ static int prepare_set_command(struct pxa3xx_nand_info *info, int command,
 	return exec_cmd;
 }
 
-static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
+static void pxa3xx_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;
+	int ret, exec_cmd, ext_cmd_type;
 
 	/*
 	 * if this is a x16 device ,then convert the input
@@ -822,14 +867,37 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
 		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);
 
-	info->state = STATE_PREPARED;
-	exec_cmd = prepare_set_command(info, command, column, page_addr);
-	if (exec_cmd) {
+	/*
+	 * 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);
-		init_completion(&info->dev_ready);
-		atomic_set(&info->is_ready, 0);
 		pxa3xx_nand_start(info);
 
 		ret = wait_for_completion_timeout(&info->cmd_complete,
@@ -838,8 +906,22 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
 			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;
 }
 
@@ -1028,6 +1110,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);
@@ -1234,6 +1318,7 @@ KEEP_CONFIG:
 	        chip->ecc.layout = &ecc_layout_4KB_bch4bit;
 		chip->ecc.strength = chip->ecc_strength_ds;
 		info->ecc_bch = 1;
+		info->chunk_size = 2048;
 	} else if (mtd->writesize <= 2048) {
 		/*
 		 * This is the default ECC Hamming 1-bit strength case,
-- 
1.8.1.5

[PATCH 17/21] 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 609f66e..6392923 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -760,6 +760,20 @@ 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);
+
+		/*
+		 * 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:
@@ -769,13 +783,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:
@@ -873,6 +914,15 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, const unsigned command,
 	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);
@@ -910,7 +960,16 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, const unsigned command,
 		}
 
 		/* 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) {
@@ -919,6 +978,14 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, const unsigned command,
 				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 18/21] ARM: mvebu: Add a fixed 0Hz clock to represent NAND clock

[Ezequiel Garcia]

The NAND controller in Armada 370 and Armada XP SoC has an input clock
that's either 125 MHz or 200 MHz. As a temporary representation, this
commit adds a 0Hz fixed-clock compatible DT node.

Not-Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
Of course, this is not intended for inclusion and I'm adding it to the
series only to allow the driver's probe() to get the clock.

This clock shouldn't be used anywhere, which means we must set
"marvell,keep-config" parameter in the NAND device tree node.

 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 90b1176..2e00da4 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -37,6 +37,15 @@
 		};
 	};
 
+	clocks {
+		/* NAND reference clock */
+		nandclk: nd_clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <0>;
+		};
+	};
+
 	soc {
 		#address-cells = <1>;
 		#size-cells = <1>;
-- 
1.8.1.5

[PATCH 19/21] 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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 2e00da4..5d3bce8 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -248,6 +248,16 @@
 				clocks = <&coreclk 0>;
 				status = "disabled";
 			};
+
+			nand@d0000 {
+				compatible = "marvell,armada370-nand";
+				reg = <0xd0000 0x54>;
+				#address-cells = <1>;
+				#size-cells = <1>;
+				interrupts = <113>;
+				clocks = <&nandclk>;
+				status = "disabled";
+			};
 		};
 	};
  };
-- 
1.8.1.5

[PATCH 20/21] 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 have no support for setting the timings parameters yet.

Not-Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
This patch is not ready for inclusion, and I'm only adding it here for
consistency and in case someone wants to test.

 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 c87b2de..0e8389b 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -176,6 +176,14 @@
 					status = "okay";
 				};
 			};
+
+			nand@d0000 {
+				status = "okay";
+				num-cs = <1>;
+				marvell,nand-keep-config;
+				marvell,nand-enable-arbiter;
+				nand-on-flash-bbt;
+			};
 		};
 	};
 };
-- 
1.8.1.5

[PATCH 21/21] 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.

Not-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 45b1077..dc54ba4 100644
--- a/arch/arm/boot/dts/armada-370-mirabox.dts
+++ b/arch/arm/boot/dts/armada-370-mirabox.dts
@@ -136,6 +136,27 @@
 					status = "okay";
 				};
 			};
+
+			nand@d0000 {
+				status = "okay";
+				num-cs = <1>;
+				marvell,nand-keep-config;
+				marvell,nand-enable-arbiter;
+				nand-on-flash-bbt;
+
+				partition@0 {
+					label = "U-Boot";
+					reg = <0 0x400000>;
+				};
+				partition@400000 {
+					label = "Linux";
+					reg = <0x400000 0x400000>;
+				};
+				partition@800000 {
+					label = "Filesystem";
+					reg = <0x800000 0x3f800000>;
+				};
+			};
 		};
 	};
 };
-- 
1.8.1.5

Re: [PATCH 00/21] Armada 370/XP NAND support

[Daniel Mack]

On 19.09.2013 18:01, Ezequiel Garcia wrote:

> Ezequiel Garcia (21):
>   mtd: nand: pxa3xx: Allocate data buffer on detected flash size
>   mtd: nand: pxa3xx: Disable OOB on arbitrary length commands
>   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: Configure detected pages-per-block
>   mtd: nand: pxa3xx: Clear cmd buffer #3 (NDCB3) on command start
>   mtd: nand: pxa3xx: Make config menu show supported platforms
>   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 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: Add a read/write buffers markers
>   mtd: nand: pxa3xx: Introduce multiple page I/O support
>   mtd: nand: pxa3xx: Add multiple chunk write support

Your patches work fine on my board with CONFIG_HAS_DMA=n.

However, it seems that with CONFIG_HAS_DMA=y, pxa3xx_nand_irq() is now
called before pxa3xx_nand_init_buff(), which results in a NULL pointer
dereference.  I had to tweak my pending dmaengine patch of course, but I
don't see an particular problem with that right now.

Any idea? Are you tesing with PIO mode or DMA?


Thanks,
Daniel

Re: [PATCH 00/21] Armada 370/XP NAND support

[Ezequiel Garcia]

On Thu, Sep 19, 2013 at 07:47:42PM +0200, Daniel Mack wrote:
> On 19.09.2013 18:01, Ezequiel Garcia wrote:
> 
> > Ezequiel Garcia (21):
> >   mtd: nand: pxa3xx: Allocate data buffer on detected flash size
> >   mtd: nand: pxa3xx: Disable OOB on arbitrary length commands
> >   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: Configure detected pages-per-block
> >   mtd: nand: pxa3xx: Clear cmd buffer #3 (NDCB3) on command start
> >   mtd: nand: pxa3xx: Make config menu show supported platforms
> >   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 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: Add a read/write buffers markers
> >   mtd: nand: pxa3xx: Introduce multiple page I/O support
> >   mtd: nand: pxa3xx: Add multiple chunk write support
> 
> Your patches work fine on my board with CONFIG_HAS_DMA=n.
> 

Great, this is already good news!

> However, it seems that with CONFIG_HAS_DMA=y, pxa3xx_nand_irq() is now
> called before pxa3xx_nand_init_buff(), which results in a NULL pointer
> dereference.  I had to tweak my pending dmaengine patch of course, but I
> don't see an particular problem with that right now.
> 
> Any idea? Are you tesing with PIO mode or DMA?
> 

Mm.. I'll check about that. As far as I understand, these SoCs don't
support NAND DMA operation. But I need to check this.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 00/21] Armada 370/XP NAND support

[Ezequiel Garcia]

On Thu, Sep 19, 2013 at 07:47:42PM +0200, Daniel Mack wrote:
> On 19.09.2013 18:01, Ezequiel Garcia wrote:
> 
> > Ezequiel Garcia (21):
> >   mtd: nand: pxa3xx: Allocate data buffer on detected flash size
> >   mtd: nand: pxa3xx: Disable OOB on arbitrary length commands
> >   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: Configure detected pages-per-block
> >   mtd: nand: pxa3xx: Clear cmd buffer #3 (NDCB3) on command start
> >   mtd: nand: pxa3xx: Make config menu show supported platforms
> >   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 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: Add a read/write buffers markers
> >   mtd: nand: pxa3xx: Introduce multiple page I/O support
> >   mtd: nand: pxa3xx: Add multiple chunk write support
> 
> Your patches work fine on my board with CONFIG_HAS_DMA=n.
> 
> However, it seems that with CONFIG_HAS_DMA=y, pxa3xx_nand_irq() is now
> called before pxa3xx_nand_init_buff(), which results in a NULL pointer
> dereference.

Yup, the problem is very clear: pxa3xx_nand_irq() is now called before
pxa3xx_nand_init_buff() because there must be a first device detection
to know which size the page will be. Then pxa3xx_nand_init_buff() is
called with this detected page size.

See the first patch "Allocate data buffer on detected flash size", which
removes the need to always allocate a buffer as big as the biggest page,
and tries to be smarter.

Of course, this doesn't work for DMA transfers. I guess the first device
detection should be done by PIO always.

If you agree with this I'll re-work the patches.

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

Re: [PATCH 00/21] Armada 370/XP NAND support

[Daniel Mack]

On 19.09.2013 23:17, Ezequiel Garcia wrote:
> On Thu, Sep 19, 2013 at 07:47:42PM +0200, Daniel Mack wrote:

>> However, it seems that with CONFIG_HAS_DMA=y, pxa3xx_nand_irq() is now
>> called before pxa3xx_nand_init_buff(), which results in a NULL pointer
>> dereference.
> 
> Yup, the problem is very clear: pxa3xx_nand_irq() is now called before
> pxa3xx_nand_init_buff() because there must be a first device detection
> to know which size the page will be. Then pxa3xx_nand_init_buff() is
> called with this detected page size.
> 
> See the first patch "Allocate data buffer on detected flash size", which
> removes the need to always allocate a buffer as big as the biggest page,
> and tries to be smarter.
> 
> Of course, this doesn't work for DMA transfers. I guess the first device
> detection should be done by PIO always.

Yes, that was my idea as well.

> If you agree with this I'll re-work the patches.

Please do, I'll go test again :)


Daniel

Re: [PATCH 00/21] Armada 370/XP NAND support

[Ezequiel Garcia]

Hi Brian,

On Thu, Sep 19, 2013 at 01:01:24PM -0300, Ezequiel Garcia wrote:
> Hi everyone,
> 
> As some of you know, I'm working on implementing the long-awaited NAND
> support in Armada 370/XP SoCs.
> 
> The controller is the same as PXA3XX (NFCv1), plus some changes,
> and therefore it's called NFCv2. As expected, it should be supported
> by pxa3xx-nand, plus some changes.
> 
> First of all I'd like to introduce the controller and explain how it
> expects the data, ECC and OOB layout within a page. We might add this
> as documentation inside the driver or in Documentation/mtd, so feel free
> to ask anything that looks suspicious or is not clear enough.
> 
> * Page layout
> 
> 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 BCH ECC we will have
> to use this layout in the pages:
> 
>   ------------------------------------------------------------------------------
>   | 2048B data | 32B spare | 30B ECC || 2048B data | 32B spare | 30B ECC | ... |
>   ------------------------------------------------------------------------------
> 
> The last remaining area is unusable (i.e. wasted).
> 
> To match this, my current (already working!) implementation acts like
> a layout "impedance matcher" to produce in an internal driver's buffer
> this layout:
> 
>   ------------------------------------------
>   |     4096B data     |     64B spare     |
>   ------------------------------------------
> 
> 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 + 64 spare) each.
> 
> The driver accomodates this data to expose the NAND base a contiguous 4160B buffer
> (4096 data + 64 spare).
> 
> * ECC
> 
> The controller has built-in hardware ECC capabilities. In addition it is completely
> configurable, and we can choose between Hamming and BCH ECC. If we choose BCH ECC
> then the ECC code will be calculated for each transfered chunk and expected to be
> located (when reading/programming) at specific locations.
> 
> 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 |
>   ------------------------------------
> 
> * 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 (aka SEQIN,
> column = 4096 + PAGEPROG) the driver has to read the entire page first to the
> driver's buffer. Then it should fill the OOB, and finally program the entire page,
> data and oob included.
> 
> Notice, that while this is possible, I haven't included OOB only write support
> (i.e. OOB write without data write) in this first patchset. I'm not sure why
> would we need it, so I'd like to know others opinion on this matter.
> 
> Of course, writing to the OOB is supported, as long as this write is done
> *with* data. Following the examples above, writing an entire 4096 + OOB page
> is supported.
> 
> * Clocking and timings
> 
> This first patchset adds a dummy nand-clock to the device tree just to allow
> the driver to get it. Timings configuration is overriden for now using the
> 'keep-config' DT parameter. The next round will likely include proper clock
> support plus timings configuration.
> 
> * About this patchset
> 
> This has been tested on Armada 370 Mirabox and Armada XP GP boards.
> 
> Currently nandtest, mtd_pagetest, mtd_readtest pass while mtd_oobtest fails
> (due to lack of OOB write support). This means that JFFS2 is not supported,
> and UBIFS is a must.
> 
> The patches are based in l2-mtd's master branch and I've pushed a branch
> to our github in case anyone wants to test this:
> 
>   https://github.com/MISL-EBU-System-SW/mainline-public/tree/pxa3xx-armada-nand-v1
> 
> The series is fairy complex and lengthy, so any feedback is more than welcome,
> as well as questions, suggestions or flames. Also, if anyone has a PXA board
> (Daniel?) to test possible regressions I would really appreciate it.
> 
> * A note about Mirabox testing
> 
> As this work is not considered completely stable, be extra careful when trying
> on the Mirabox. The Mirabox has the bootloader at NAND, and there's some risk
> to brick the board.
> 
> That said: I've been using the driver on my Mirabox all morning (with my own
> Buildroot prepared UBIFS image) and so far everything works fine.
> 
> If despite this you happen to brick the board, Willy Tarreau has provided
> excellent instructions to un-brick the Mirabox:
> 
>   http://1wt.eu/articles/mirabox-vs-guruplug/
> 
> Thanks!
> 
> Ezequiel Garcia (21):
>   mtd: nand: pxa3xx: Allocate data buffer on detected flash size
>   mtd: nand: pxa3xx: Disable OOB on arbitrary length commands
>   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: Configure detected pages-per-block
>   mtd: nand: pxa3xx: Clear cmd buffer #3 (NDCB3) on command start
>   mtd: nand: pxa3xx: Make config menu show supported platforms
>   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 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: Add a read/write buffers markers
>   mtd: nand: pxa3xx: Introduce multiple page I/O support
>   mtd: nand: pxa3xx: Add multiple chunk write support
>   ARM: mvebu: Add a fixed 0Hz clock to represent NAND clock
>   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
> 
>  arch/arm/boot/dts/armada-370-mirabox.dts      |  21 +
>  arch/arm/boot/dts/armada-370-xp.dtsi          |  19 +
>  arch/arm/boot/dts/armada-xp-gp.dts            |   8 +
>  drivers/mtd/nand/Kconfig                      |   4 +-
>  drivers/mtd/nand/pxa3xx_nand.c                | 546 +++++++++++++++++++++-----
>  include/linux/platform_data/mtd-nand-pxa3xx.h |   3 +
>  6 files changed, 493 insertions(+), 108 deletions(-)
> 
> 

If you're not too busy, I'd like you to take a look at the implementation.

I'm preparing a new v2 which some (very minor) improvements, mainly to
add support for timings configuration.

If there's anything to fix or to re-work, don't hesitate in asking!

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

Re: [PATCH 00/21] Armada 370/XP NAND support

[Brian Norris]

Hi Ezequiel,

On 09/24/2013 11:59 AM, Ezequiel Garcia wrote:
> On Thu, Sep 19, 2013 at 01:01:24PM -0300, Ezequiel Garcia wrote:
>> As some of you know, I'm working on implementing the long-awaited NAND
>> support in Armada 370/XP SoCs.

...

> If you're not too busy, I'd like you to take a look at the implementation.

Yes, I plan to look at this soon. I *might* have time tomorrow. But if 
my delay is too long for your taste, feel free to send v2.

> I'm preparing a new v2 which some (very minor) improvements, mainly to
> add support for timings configuration.

I believe Daniel reported a bug w/ PIO vs. DMA. I presume this will be 
in v2? If those modifications aren't too big, then I can look at the 
current series.

> If there's anything to fix or to re-work, don't hesitate in asking!

Of course!

Brian

Re: [PATCH 00/21] Armada 370/XP NAND support

[Ezequiel Garcia]

On Tue, Sep 24, 2013 at 11:27:13PM -0700, Brian Norris wrote:
> > I'm preparing a new v2 which some (very minor) improvements, mainly to
> > add support for timings configuration.
> 
> I believe Daniel reported a bug w/ PIO vs. DMA. I presume this will be 
> in v2? If those modifications aren't too big, then I can look at the 
> current series.
> 

Yes, the fix will be in v2, but it's (apparently) going to be a very
minor fix.

The current patchset contains almost all the juicy stuff.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 00/21] Armada 370/XP NAND support

[Brian Norris]

+ Huang

Hi Ezequiel,

I've mostly had a chance to study the cover letter. I'll start digging
through the patches in a bit, but I have a few initial questions.

Also, it looks like some of this ECC/OOB layout is rather similar to the
documentation I've seen from Huang for gpmi-nand. Perhaps he can review
a bit of your work here.

On Thu, Sep 19, 2013 at 01:01:24PM -0300, Ezequiel Garcia wrote:
> * Page layout
> 
> 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 BCH ECC we will have
> to use this layout in the pages:
> 
>   ------------------------------------------------------------------------------
>   | 2048B data | 32B spare | 30B ECC || 2048B data | 32B spare | 30B ECC | ... |
>   ------------------------------------------------------------------------------
> 
> The last remaining area is unusable (i.e. wasted).

Why is it wasted? With a 2176B buffer, you can fit more than 2048+32+30.
Are you simplifying things to support only NAND geometries like the
tradiditional large page 2KB+64? (If so, what happens with 4KB pages
that need higher level of ECC than can fit in 64 x 2 = 128B?) And in the
2KB+64 case, why is the 64-32-30=2B wasted?

> To match this, my current (already working!) implementation acts like
> a layout "impedance matcher" to produce in an internal driver's buffer

Maybe I'm in the wrong field, but I'm not sure how an impedance matcher
applies here.

> this layout:
> 
>   ------------------------------------------
>   |     4096B data     |     64B spare     |
>   ------------------------------------------

What is this "internal buffer"? (I'll read through your patches in a bit
and find out, perhaps?) Just the raw data + spare that you are ready to
read/write? Are you dropping the ECC from this buffer? Doesn't it need
to be returned to the callee (when the 'oob_required' flag is true)?

> 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 + 64 spare) each.

The math is a little inconsistent here and elsewhere. Do you mean two
chunks of 2080B (2048 data + 32 spare)?

> The driver accomodates this data to expose the NAND base a contiguous 4160B buffer
> (4096 data + 64 spare).

This math matches up right, if it's double the 2KB+32 instead of double
2KB+64.

> * ECC
> 
> The controller has built-in hardware ECC capabilities. In addition it is completely
> configurable, and we can choose between Hamming and BCH ECC. If we choose BCH ECC
> then the ECC code will be calculated for each transfered chunk and expected to be
> located (when reading/programming) at specific locations.

At "specific locations", but always within the 30B region that you've
been mapping out?

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

Is ECC always 30B? Or is this just the maximum size, so you always
reserve 30B?

> * 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 (aka SEQIN,
> column = 4096 + PAGEPROG) the driver has to read the entire page first to the
> driver's buffer. Then it should fill the OOB, and finally program the entire page,
> data and oob included.
> 
> Notice, that while this is possible, I haven't included OOB only write support
> (i.e. OOB write without data write) in this first patchset. I'm not sure why
> would we need it, so I'd like to know others opinion on this matter.

Depends on what you mean by "OOB only write support". You need to
support marking bad block markers, at least, which are "OOB only"
writes.

> Of course, writing to the OOB is supported, as long as this write is done
> *with* data. Following the examples above, writing an entire 4096 + OOB page
> is supported.
> 
> * Clocking and timings
> 
> This first patchset adds a dummy nand-clock to the device tree just to allow
> the driver to get it. Timings configuration is overriden for now using the
> 'keep-config' DT parameter. The next round will likely include proper clock
> support plus timings configuration.

Be sure to include devicetree@vger.kernel.org and one or more of the
maintainers for your proper DT bindings patches.

Brian

Re: [PATCH 00/21] Armada 370/XP NAND support

[Ezequiel Garcia]

Brian,

On Thu, Sep 26, 2013 at 12:56:15PM -0700, Brian Norris wrote:
> + Huang
> 
> Hi Ezequiel,
> 
> I've mostly had a chance to study the cover letter. I'll start digging
> through the patches in a bit, but I have a few initial questions.
> 
> Also, it looks like some of this ECC/OOB layout is rather similar to the
> documentation I've seen from Huang for gpmi-nand. Perhaps he can review
> a bit of your work here.
> 
> On Thu, Sep 19, 2013 at 01:01:24PM -0300, Ezequiel Garcia wrote:
> > * Page layout
> > 
> > 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 BCH ECC we will have
> > to use this layout in the pages:
> > 
> >   ------------------------------------------------------------------------------
> >   | 2048B data | 32B spare | 30B ECC || 2048B data | 32B spare | 30B ECC | ... |
> >   ------------------------------------------------------------------------------
> > 
> > The last remaining area is unusable (i.e. wasted).
> 
> Why is it wasted? With a 2176B buffer, you can fit more than 2048+32+30.
> Are you simplifying things to support only NAND geometries like the
> tradiditional large page 2KB+64? (If so, what happens with 4KB pages
> that need higher level of ECC than can fit in 64 x 2 = 128B?) And in the
> 2KB+64 case, why is the 64-32-30=2B wasted?
> 

Hm... the wasted bytes are due to an 8-byte alignment controller requirement on the
data buffer. However, you're right: in a flash with a page of 4KB+224B (e.g.) you
should be able to access at least some of the remaining OOB area.

Notice OOB handling is still a bit confusing in this driver.

> > To match this, my current (already working!) implementation acts like
> > a layout "impedance matcher" to produce in an internal driver's buffer
> 
> Maybe I'm in the wrong field, but I'm not sure how an impedance matcher
> applies here.
> 
> > this layout:
> > 
> >   ------------------------------------------
> >   |     4096B data     |     64B spare     |
> >   ------------------------------------------
> 
> What is this "internal buffer"? (I'll read through your patches in a bit
> and find out, perhaps?) Just the raw data + spare that you are ready to
> read/write? Are you dropping the ECC from this buffer? Doesn't it need
> to be returned to the callee (when the 'oob_required' flag is true)?
> 

Yes, the driver's internal buffer has the raw data + OOB that has
been read from the controller after a read operation, or that's
ready to be written to the flash before a program operation.

That said, if the command issued was READOOB then the OOB is read fully:
spare and ECC code and the buffer is filled like this:

  ----------------------------------------------------------------------------------------------
  |     4096B data     |  32B spare  |  30B ECC*  | 2x 0xff |  32B spare  |  30B ECC  | 2x 0xff |
  ----------------------------------------------------------------------------------------------

(*) We need to add two bytes (0xff) to align the 30B ECC read.

I was expecting to handle the above layout properly using this nand_ecclayout
structure:

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 = { {1, 4}, {6, 26}, { 64, 32} }
};

However, I must be doing something wrong since currently the mtd_oobtest fails
at some point and I need to investigate the issue further.

> > 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 + 64 spare) each.
> 
> The math is a little inconsistent here and elsewhere. Do you mean two
> chunks of 2080B (2048 data + 32 spare)?
> 

Yes. It should be: 2048B data + 32B spare.

> > The driver accomodates this data to expose the NAND base a contiguous 4160B buffer
> > (4096 data + 64 spare).
> 
> This math matches up right, if it's double the 2KB+32 instead of double
> 2KB+64.
> 

Yup.

> > * ECC
> > 
> > The controller has built-in hardware ECC capabilities. In addition it is completely
> > configurable, and we can choose between Hamming and BCH ECC. If we choose BCH ECC
> > then the ECC code will be calculated for each transfered chunk and expected to be
> > located (when reading/programming) at specific locations.
> 
> At "specific locations", but always within the 30B region that you've
> been mapping out?
> 

By 'specific location' I mean after the 32B spare area.

> > 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 |
> >   ------------------------------------
> 
> Is ECC always 30B? Or is this just the maximum size, so you always
> reserve 30B?
> 

In the BCH case, yes: it's always 30B.

AFAIK, the ECC works like this. When programming a page, you can
actually instruct the controller to transfer an arbitrary amount of bytes.

(Details: you do this by adding NDCB0_LEN_OVRD to the command buffer 0,
and setting the i/o length at buffer 3).

The ECC code will be written after the transfered bytes. So, when
programming a page we configure the length to be 2048B + 32B and get
the ECC after it.

Equally, we can read an arbitrary length. and the ECC code is expected
to be in the area immediate the read area.

Notice that this means you can control the ECC strength: since the ECC
is always 30B, and it's calculated on the transfered chunk you can
configure the controller to transfer the page in 1024B chunks and get a
30B ECC for each of this chunk, thus doubling the ECC strength.

This should be added to the driver, and I expect it to be fairly easy
with the infrastructure already in place.

> > * 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 (aka SEQIN,
> > column = 4096 + PAGEPROG) the driver has to read the entire page first to the
> > driver's buffer. Then it should fill the OOB, and finally program the entire page,
> > data and oob included.
> > 
> > Notice, that while this is possible, I haven't included OOB only write support
> > (i.e. OOB write without data write) in this first patchset. I'm not sure why
> > would we need it, so I'd like to know others opinion on this matter.
> 
> Depends on what you mean by "OOB only write support". You need to
> support marking bad block markers, at least, which are "OOB only"
> writes.
> 

Indeed. And that's achieved by programming an entire page with all
0xff's in the data area and only the OOB area with meaningful (non-0xff)
metadata.

Although not included in this v1, I have some patches here that support this.
I've done some testing, marking blocks as bad from within the kernel and then
re-constructing the bad block table and that appears to work OK.

So, this will be in v2. It's a very minor delta from this v1, nothing intrusive.

> > Of course, writing to the OOB is supported, as long as this write is done
> > *with* data. Following the examples above, writing an entire 4096 + OOB page
> > is supported.
> > 
> > * Clocking and timings
> > 
> > This first patchset adds a dummy nand-clock to the device tree just to allow
> > the driver to get it. Timings configuration is overriden for now using the
> > 'keep-config' DT parameter. The next round will likely include proper clock
> > support plus timings configuration.
> 
> Be sure to include devicetree@vger.kernel.org and one or more of the
> maintainers for your proper DT bindings patches.
> 

Sure. For now, I'm mostly concerned about the driver implementation and
that's why I haven't Cced the DT folks.

Thanks for your feedback and don't hesitate to ask more questions,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 00/21] Armada 370/XP NAND support

[Brian Norris]

On Mon, Sep 30, 2013 at 09:24:29AM -0300, Ezequiel Garcia wrote:
> On Thu, Sep 26, 2013 at 12:56:15PM -0700, Brian Norris wrote:
> > On Thu, Sep 19, 2013 at 01:01:24PM -0300, Ezequiel Garcia wrote:
> > > * Page layout
> > > 
> > > 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 BCH ECC we will have
> > > to use this layout in the pages:
> > > 
> > >   ------------------------------------------------------------------------------
> > >   | 2048B data | 32B spare | 30B ECC || 2048B data | 32B spare | 30B ECC | ... |
> > >   ------------------------------------------------------------------------------
> > > 
> > > The last remaining area is unusable (i.e. wasted).
> > 
> > Why is it wasted? With a 2176B buffer, you can fit more than 2048+32+30.
> > Are you simplifying things to support only NAND geometries like the
> > tradiditional large page 2KB+64? (If so, what happens with 4KB pages
> > that need higher level of ECC than can fit in 64 x 2 = 128B?) And in the
> > 2KB+64 case, why is the 64-32-30=2B wasted?
> > 
> 
> Hm... the wasted bytes are due to an 8-byte alignment controller requirement on the
> data buffer. However, you're right: in a flash with a page of 4KB+224B (e.g.) you
> should be able to access at least some of the remaining OOB area.

Alignment requirement for the start of the buffer, length of the buffer,
or both?

> Notice OOB handling is still a bit confusing in this driver.

Yes.

> > > To match this, my current (already working!) implementation acts like
> > > a layout "impedance matcher" to produce in an internal driver's buffer
> > 
> > Maybe I'm in the wrong field, but I'm not sure how an impedance matcher
> > applies here.
> > 
> > > this layout:
> > > 
> > >   ------------------------------------------
> > >   |     4096B data     |     64B spare     |
> > >   ------------------------------------------
> > 
> > What is this "internal buffer"? (I'll read through your patches in a bit
> > and find out, perhaps?) Just the raw data + spare that you are ready to
> > read/write? Are you dropping the ECC from this buffer? Doesn't it need
> > to be returned to the callee (when the 'oob_required' flag is true)?
> > 
> 
> Yes, the driver's internal buffer has the raw data + OOB that has
> been read from the controller after a read operation, or that's
> ready to be written to the flash before a program operation.
> 
> That said, if the command issued was READOOB then the OOB is read fully:
> spare and ECC code and the buffer is filled like this:
> 
>   ----------------------------------------------------------------------------------------------
>   |     4096B data     |  32B spare  |  30B ECC*  | 2x 0xff |  32B spare  |  30B ECC  | 2x 0xff |
>   ----------------------------------------------------------------------------------------------
> 
> (*) We need to add two bytes (0xff) to align the 30B ECC read.
> 
> I was expecting to handle the above layout properly using this nand_ecclayout
> structure:
> 
> 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 = { {1, 4}, {6, 26}, { 64, 32} }
> };
> 
> However, I must be doing something wrong since currently the mtd_oobtest fails
> at some point and I need to investigate the issue further.
> 
> > > 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 + 64 spare) each.
> > 
> > The math is a little inconsistent here and elsewhere. Do you mean two
> > chunks of 2080B (2048 data + 32 spare)?
> > 
> 
> Yes. It should be: 2048B data + 32B spare.
> 
> > > The driver accomodates this data to expose the NAND base a contiguous 4160B buffer
> > > (4096 data + 64 spare).
> > 
> > This math matches up right, if it's double the 2KB+32 instead of double
> > 2KB+64.
> > 
> 
> Yup.
> 
> > > * ECC
> > > 
> > > The controller has built-in hardware ECC capabilities. In addition it is completely
> > > configurable, and we can choose between Hamming and BCH ECC. If we choose BCH ECC
> > > then the ECC code will be calculated for each transfered chunk and expected to be
> > > located (when reading/programming) at specific locations.
> > 
> > At "specific locations", but always within the 30B region that you've
> > been mapping out?
> > 
> 
> By 'specific location' I mean after the 32B spare area.
> 
> > > 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 |
> > >   ------------------------------------
> > 
> > Is ECC always 30B? Or is this just the maximum size, so you always
> > reserve 30B?
> > 
> 
> In the BCH case, yes: it's always 30B.

So by "BCH" you actually are referring to BCH-4? Are there any other BCH
modes supported? Or do you expect any future revisions to expand on the
BCH options available?

> AFAIK, the ECC works like this. When programming a page, you can
> actually instruct the controller to transfer an arbitrary amount of bytes.
> 
> (Details: you do this by adding NDCB0_LEN_OVRD to the command buffer 0,
> and setting the i/o length at buffer 3).
> 
> The ECC code will be written after the transfered bytes. So, when
> programming a page we configure the length to be 2048B + 32B and get
> the ECC after it.
> 
> Equally, we can read an arbitrary length. and the ECC code is expected
> to be in the area immediate the read area.
> 
> Notice that this means you can control the ECC strength: since the ECC
> is always 30B, and it's calculated on the transfered chunk you can
> configure the controller to transfer the page in 1024B chunks and get a
> 30B ECC for each of this chunk, thus doubling the ECC strength.

This is a little odd. Your ECC HW is not parameterized by ECC strength,
but instead by ECC sector size? I think I have some comments for patch
5, relevant to this topic.

> This should be added to the driver, and I expect it to be fairly easy
> with the infrastructure already in place.

Brian

Re: [PATCH 00/21] Armada 370/XP NAND support

[Ezequiel Garcia]

On Wed, Oct 02, 2013 at 05:02:53PM -0700, Brian Norris wrote:
> On Mon, Sep 30, 2013 at 09:24:29AM -0300, Ezequiel Garcia wrote:
> > On Thu, Sep 26, 2013 at 12:56:15PM -0700, Brian Norris wrote:
> > > On Thu, Sep 19, 2013 at 01:01:24PM -0300, Ezequiel Garcia wrote:
> > > > * Page layout
> > > > 
> > > > 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 BCH ECC we will have
> > > > to use this layout in the pages:
> > > > 
> > > >   ------------------------------------------------------------------------------
> > > >   | 2048B data | 32B spare | 30B ECC || 2048B data | 32B spare | 30B ECC | ... |
> > > >   ------------------------------------------------------------------------------
> > > > 
> > > > The last remaining area is unusable (i.e. wasted).
> > > 
> > > Why is it wasted? With a 2176B buffer, you can fit more than 2048+32+30.
> > > Are you simplifying things to support only NAND geometries like the
> > > tradiditional large page 2KB+64? (If so, what happens with 4KB pages
> > > that need higher level of ECC than can fit in 64 x 2 = 128B?) And in the
> > > 2KB+64 case, why is the 64-32-30=2B wasted?
> > > 
> > 
> > Hm... the wasted bytes are due to an 8-byte alignment controller requirement on the
> > data buffer. However, you're right: in a flash with a page of 4KB+224B (e.g.) you
> > should be able to access at least some of the remaining OOB area.
> 
> Alignment requirement for the start of the buffer, length of the buffer,
> or both?
> 

The specification requires the length of a transaction (amount of data in the
data buffer) to be aligned to 8 bytes.

> > Notice OOB handling is still a bit confusing in this driver.
> 
> Yes.
> 
> > > > To match this, my current (already working!) implementation acts like
> > > > a layout "impedance matcher" to produce in an internal driver's buffer
> > > 
> > > Maybe I'm in the wrong field, but I'm not sure how an impedance matcher
> > > applies here.
> > > 
> > > > this layout:
> > > > 
> > > >   ------------------------------------------
> > > >   |     4096B data     |     64B spare     |
> > > >   ------------------------------------------
> > > 
> > > What is this "internal buffer"? (I'll read through your patches in a bit
> > > and find out, perhaps?) Just the raw data + spare that you are ready to
> > > read/write? Are you dropping the ECC from this buffer? Doesn't it need
> > > to be returned to the callee (when the 'oob_required' flag is true)?
> > > 
> > 
> > Yes, the driver's internal buffer has the raw data + OOB that has
> > been read from the controller after a read operation, or that's
> > ready to be written to the flash before a program operation.
> > 
> > That said, if the command issued was READOOB then the OOB is read fully:
> > spare and ECC code and the buffer is filled like this:
> > 
> >   ----------------------------------------------------------------------------------------------
> >   |     4096B data     |  32B spare  |  30B ECC*  | 2x 0xff |  32B spare  |  30B ECC  | 2x 0xff |
> >   ----------------------------------------------------------------------------------------------
> > 
> > (*) We need to add two bytes (0xff) to align the 30B ECC read.
> > 
> > I was expecting to handle the above layout properly using this nand_ecclayout
> > structure:
> > 
> > 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 = { {1, 4}, {6, 26}, { 64, 32} }
> > };
> > 
> > However, I must be doing something wrong since currently the mtd_oobtest fails
> > at some point and I need to investigate the issue further.
> > 
> > > > 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 + 64 spare) each.
> > > 
> > > The math is a little inconsistent here and elsewhere. Do you mean two
> > > chunks of 2080B (2048 data + 32 spare)?
> > > 
> > 
> > Yes. It should be: 2048B data + 32B spare.
> > 
> > > > The driver accomodates this data to expose the NAND base a contiguous 4160B buffer
> > > > (4096 data + 64 spare).
> > > 
> > > This math matches up right, if it's double the 2KB+32 instead of double
> > > 2KB+64.
> > > 
> > 
> > Yup.
> > 
> > > > * ECC
> > > > 
> > > > The controller has built-in hardware ECC capabilities. In addition it is completely
> > > > configurable, and we can choose between Hamming and BCH ECC. If we choose BCH ECC
> > > > then the ECC code will be calculated for each transfered chunk and expected to be
> > > > located (when reading/programming) at specific locations.
> > > 
> > > At "specific locations", but always within the 30B region that you've
> > > been mapping out?
> > > 
> > 
> > By 'specific location' I mean after the 32B spare area.
> > 
> > > > 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 |
> > > >   ------------------------------------
> > > 
> > > Is ECC always 30B? Or is this just the maximum size, so you always
> > > reserve 30B?
> > > 
> > 
> > In the BCH case, yes: it's always 30B.
> 
> So by "BCH" you actually are referring to BCH-4? Are there any other BCH
> modes supported? Or do you expect any future revisions to expand on the
> BCH options available?
> 

Mmm.. ECC BCH is always 30 bytes long, but this doesn't mean it's BCH-4.
See below.

> > AFAIK, the ECC works like this. When programming a page, you can
> > actually instruct the controller to transfer an arbitrary amount of bytes.
> > 
> > (Details: you do this by adding NDCB0_LEN_OVRD to the command buffer 0,
> > and setting the i/o length at buffer 3).
> > 
> > The ECC code will be written after the transfered bytes. So, when
> > programming a page we configure the length to be 2048B + 32B and get
> > the ECC after it.
> > 
> > Equally, we can read an arbitrary length. and the ECC code is expected
> > to be in the area immediate the read area.
> > 
> > Notice that this means you can control the ECC strength: since the ECC
> > is always 30B, and it's calculated on the transfered chunk you can
> > configure the controller to transfer the page in 1024B chunks and get a
> > 30B ECC for each of this chunk, thus doubling the ECC strength.
> 
> This is a little odd. Your ECC HW is not parameterized by ECC strength,
> but instead by ECC sector size? I think I have some comments for patch
> 5, relevant to this topic.
> 

Indeed. When ECC engine in the controller is set to do BCH, it always
store a 30-byte ECC code. This 30-byte code is calculated on the
transfered chunk, and since the chunk size is configurable we can
effectively configure the ECC strength.

The specification says the ECC BCH engine can correct up to 16-bits,
which means: if the chunk is 2048 bytes you get BCH-4, and if the chunk
is set to be 1024 bytes, you get BCH-8.

Only the former has been implemented in this v1.

Is this clearer now? Don't hesitate in asking more questions as it helps
me fill my own understanding gaps :-)
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 00/21] Armada 370/XP NAND support

[Brian Norris]

On Fri, Oct 04, 2013 at 04:42:04PM -0300, Ezequiel Garcia wrote:
> On Wed, Oct 02, 2013 at 05:02:53PM -0700, Brian Norris wrote:
> > On Mon, Sep 30, 2013 at 09:24:29AM -0300, Ezequiel Garcia wrote:
> > > Notice that this means you can control the ECC strength: since the ECC
> > > is always 30B, and it's calculated on the transfered chunk you can
> > > configure the controller to transfer the page in 1024B chunks and get a
> > > 30B ECC for each of this chunk, thus doubling the ECC strength.
> > 
> > This is a little odd. Your ECC HW is not parameterized by ECC strength,
> > but instead by ECC sector size? I think I have some comments for patch
> > 5, relevant to this topic.
> > 
> 
> Indeed. When ECC engine in the controller is set to do BCH, it always
> store a 30-byte ECC code. This 30-byte code is calculated on the
> transfered chunk, and since the chunk size is configurable we can
> effectively configure the ECC strength.
> 
> The specification says

Is there a public URL for the specification?

> the ECC BCH engine can correct up to 16-bits, which means:

Is this your interpretation, or are you quoting the spec?

> if the chunk is 2048 bytes you get BCH-4, and if the chunk
> is set to be 1024 bytes, you get BCH-8.

That doesn't really make sense to me. If you can't post a public URL,
can you paste the relevant text for this?

Most NAND specifies a required correctability of something like "correct
1 bit error per 512 bytes" or "correct 4 bits per 512 bytes". These are
not the same as "correct 1 bit per 2048 bytes" and "correct 4 bits per
2048 bytes", respectively, for obvious reasons.

So your "BCH-4" over a "2048 byte chunk" could be interpreted in
multiple ways. I'll list a few.

(1) BCH-4 usually means 4 bit correction over a 512 byte region, to
    match most SLC that might specify 4-bit correction. Applied to your
    hardware, it could possibly be that the 2048 byte region is actually
    4 smaller regions which are corrected separately, each with 4 bit
    correction. The ECC metadata just happens to be stored in 30 bytes
    of OOB (each 512-byte "sub-step" uses 7.5 bytes of spare area??)

    Another way to look at this: perhaps your ECC "chunk size" (the
    contiguous region over which your ECC is applied and transferred
    atomically) is different than the "correction region" (the region
    over which a certain number of bits may be corrected).

(2) Your BCH-4 over 2048 bytes really means what it says; it can only
    correct 4-bit in the 2048-byte area. This is sufficient for NAND
    that specify 1-bit correction per 512-bytes but not for those that
    specify 4-bit correction over 512-bytes.

Because an interpretation like (2) doesn't really match the NAND parts
you seem to be targeting, I'm inclined to think (1) is what the hardware
is doing. I also suspect (2) is wrong because the BCH algorithm (under
typical parameters) requires on the order of 6 to 8 bytes for storage,
regardless of the region it is correcting over. So "BCH-4" over 2048
bytes would only require 6 to 8 bytes of storage, not 30.

Barring more lucid documentation, you might want to verify this with
some tests. If you can support raw (no ECC) programming, this is a case
where you could induce bit errors and see how many can be corrected in
various patterns. But that's only necessary if your documentation sucks
too badly or is not public.

Regarding patch 5 (might as well mention it here), you are only checking
the ecc_strength_ds field for 4. You are not checking ecc_step_ds, which
likely will be 512 or 1024. Both parameters are relevant to determining
what ECC types are strong enough. But it's confusing enough right now to
even determine what your hardware means when you say "BCH-4", so we
might not be ready to handle this yet...

Brian

Re: [PATCH 00/21] Armada 370/XP NAND support

[Ezequiel Garcia]

Brian,

Let's continue the discussion about this controller and its ECC engine.

Sorry for the delayed reply. Also please bare with me as I'm not an MTD,
NAND or ECC expert; so feel free to correct any mis-understandings on
my side.

On Fri, Oct 04, 2013 at 05:06:49PM -0700, Brian Norris wrote:
> On Fri, Oct 04, 2013 at 04:42:04PM -0300, Ezequiel Garcia wrote:
> > On Wed, Oct 02, 2013 at 05:02:53PM -0700, Brian Norris wrote:
> > > On Mon, Sep 30, 2013 at 09:24:29AM -0300, Ezequiel Garcia wrote:
> > > > Notice that this means you can control the ECC strength: since the ECC
> > > > is always 30B, and it's calculated on the transfered chunk you can
> > > > configure the controller to transfer the page in 1024B chunks and get a
> > > > 30B ECC for each of this chunk, thus doubling the ECC strength.
> > > 
> > > This is a little odd. Your ECC HW is not parameterized by ECC strength,
> > > but instead by ECC sector size? I think I have some comments for patch
> > > 5, relevant to this topic.
> > > 
> > 
> > Indeed. When ECC engine in the controller is set to do BCH, it always
> > store a 30-byte ECC code. This 30-byte code is calculated on the
> > transfered chunk, and since the chunk size is configurable we can
> > effectively configure the ECC strength.
> > 
> > The specification says
> 
> Is there a public URL for the specification?
> 

Not yet, but we're working very hard to get the spec to be published.

> > the ECC BCH engine can correct up to 16-bits, which means:
> 
> Is this your interpretation, or are you quoting the spec?
> 

I'm quoting the spec.

> > if the chunk is 2048 bytes you get BCH-4, and if the chunk
> > is set to be 1024 bytes, you get BCH-8.
> 
> That doesn't really make sense to me. If you can't post a public URL,
> can you paste the relevant text for this?
> 

I guess the above is not clear: I didn't mean BCH-4 or BCH-8 over any region.
Instead, I meant that: the spec says the ECC BCH engine can correct up to 16 bits
'per region', where 'region' is the amount of data the controller is
configured to transfer (which is arbitrary).

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.

If you set the controller to transfer 1024B you can correct up to 16
bits on this 1024B region, or 8 bits per 512B, hence BCH-8.

> Most NAND specifies a required correctability of something like "correct
> 1 bit error per 512 bytes" or "correct 4 bits per 512 bytes". These are
> not the same as "correct 1 bit per 2048 bytes" and "correct 4 bits per
> 2048 bytes", respectively, for obvious reasons.
> 
> So your "BCH-4" over a "2048 byte chunk" could be interpreted in
> multiple ways. I'll list a few.
> 
> (1) BCH-4 usually means 4 bit correction over a 512 byte region, to
>     match most SLC that might specify 4-bit correction. Applied to your
>     hardware, it could possibly be that the 2048 byte region is actually
>     4 smaller regions which are corrected separately, each with 4 bit
>     correction. The ECC metadata just happens to be stored in 30 bytes
>     of OOB (each 512-byte "sub-step" uses 7.5 bytes of spare area??)
> 
>     Another way to look at this: perhaps your ECC "chunk size" (the
>     contiguous region over which your ECC is applied and transferred
>     atomically) is different than the "correction region" (the region
>     over which a certain number of bits may be corrected).
> 

Yes, the above (1) matches this hardware, expect for one point. The
specifications says that "the BCH engine corrects up to 16 errors
over the entire page". So there aren't any "smaller" regions, and the
hardware operates on the entire page.

However, the BCH does not operate on the "page" but rather on the
transfered region (which is configurable). So, as I explained above,
you can actually correct 16 bits per 2048B or 16 bits per 1024B depending
on the way you configure the controller.

Is this clearer now?

> 
> Barring more lucid documentation, you might want to verify this with
> some tests. If you can support raw (no ECC) programming, this is a case
> where you could induce bit errors and see how many can be corrected in
> various patterns. But that's only necessary if your documentation sucks
> too badly or is not public.
> 

Hm.. I'll see if I can do this. It sounds like an interesting test.

> Regarding patch 5 (might as well mention it here), you are only checking
> the ecc_strength_ds field for 4. You are not checking ecc_step_ds, which
> likely will be 512 or 1024. Both parameters are relevant to determining
> what ECC types are strong enough.

Yup, those checks are really crappy in this v1, I have a v2 ready to be
submitted that takes care of this in a much cleaner (and safer way).

> But it's confusing enough right now to
> even determine what your hardware means when you say "BCH-4", so we
> might not be ready to handle this yet...
> 

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.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com