[U-Boot,v5,2/5] nand: lpc32xx: add hardware ECC support

From: Sylvain Lemieux <slemieux@tycoint.com>

Incorporate NAND SLC hardware ECC support from legacy
LPCLinux NXP BSP.
The code taken from the legacy patch is:
- lpc32xx SLC NAND driver (hardware ECC support)
- lpc3250 header file missing SLC NAND registers definition

The legacy driver code was updated to integrate with the existing NAND SLC driver.

Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
---
Changes from v4 to v5:
* Addressed Marek's comments on LPC32xx DMA driver:
  - Move macro use by the DMA client to write the next
    DMA linked list item address to NAND SLC driver.
* Updated multiline comments style.
* Use "uxx" instead of "uintxx_t" (new code),
  note: keep uintxx_t for API; to match initial patch.

Changes from v3 to v4:
* No changes.

Changes from v2 to v3:
* As per feedback on mailing list from Vladimir,
  - add warning when DMA and SPL build option are selected.
  - add another DMA specific read and write buffer functions.
* Addressed Marek's comments on LPC32xx NAND driver
  removed typecast and update variable type, when possible.
* As per discussion on mailing list with Vladimir & Marek,
  updated conditional compile option (DMA only) to build
  the original NAND SLC code using software ECC.
* Re-organized the conditional compile code inside
  "board_nand_init()" - DMA vs. non-DMA initialization.
* Added original source code credit (copyright & author).

Changes from v1 to v2:
* Moved the NAND SLC patch as the second patch of the series.
* As per discussion on mailing list with Vladimir,
  incorporate NAND SLC hardware ECC support into the following
  NAND SLC patch: https://patchwork.ozlabs.org/patch/497308/
* As per discussion on mailing list with Vladimir & Albert,
  add conditional compile option to build the original
  NAND SLC code using software ECC for SPL build.
* Removed ECC layout for small page NAND from this patch.

Update to the legacy code to integrate with the NAND SLC patch:
1) Fixed checkpatch script output in legacy code.
2) Use u-boot API for register access to remove the volatile
   in register definition taken from "lpc3250.h" header file.
3) Use register definition from the NAND SLC patch.

The legacy BSP patch (u-boot-2009.03_lpc32x0-v1.07.patch.tar.bz2)
was downloaded from the LPCLinux Web site.

 drivers/mtd/nand/lpc32xx_nand_slc.c | 369 +++++++++++++++++++++++++++++++++++-
 1 file changed, 364 insertions(+), 5 deletions(-)

Hi Sylvain,

On 05.08.2015 21:31, slemieux.tyco@gmail.com wrote:
> From: Sylvain Lemieux <slemieux@tycoint.com>
> 
> Incorporate NAND SLC hardware ECC support from legacy
> LPCLinux NXP BSP.
> The code taken from the legacy patch is:
> - lpc32xx SLC NAND driver (hardware ECC support)
> - lpc3250 header file missing SLC NAND registers definition
> 
> The legacy driver code was updated to integrate with the existing NAND SLC driver.
> 
> Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
> ---
> Changes from v4 to v5:
> * Addressed Marek's comments on LPC32xx DMA driver:
>   - Move macro use by the DMA client to write the next
>     DMA linked list item address to NAND SLC driver.
> * Updated multiline comments style.
> * Use "uxx" instead of "uintxx_t" (new code),
>   note: keep uintxx_t for API; to match initial patch.
> 
> Changes from v3 to v4:
> * No changes.
> 
> Changes from v2 to v3:
> * As per feedback on mailing list from Vladimir,
>   - add warning when DMA and SPL build option are selected.
>   - add another DMA specific read and write buffer functions.
> * Addressed Marek's comments on LPC32xx NAND driver
>   removed typecast and update variable type, when possible.
> * As per discussion on mailing list with Vladimir & Marek,
>   updated conditional compile option (DMA only) to build
>   the original NAND SLC code using software ECC.
> * Re-organized the conditional compile code inside
>   "board_nand_init()" - DMA vs. non-DMA initialization.
> * Added original source code credit (copyright & author).
> 
> Changes from v1 to v2:
> * Moved the NAND SLC patch as the second patch of the series.
> * As per discussion on mailing list with Vladimir,
>   incorporate NAND SLC hardware ECC support into the following
>   NAND SLC patch: https://patchwork.ozlabs.org/patch/497308/
> * As per discussion on mailing list with Vladimir & Albert,
>   add conditional compile option to build the original
>   NAND SLC code using software ECC for SPL build.
> * Removed ECC layout for small page NAND from this patch.
> 
> Update to the legacy code to integrate with the NAND SLC patch:
> 1) Fixed checkpatch script output in legacy code.
> 2) Use u-boot API for register access to remove the volatile
>    in register definition taken from "lpc3250.h" header file.
> 3) Use register definition from the NAND SLC patch.
> 
> The legacy BSP patch (u-boot-2009.03_lpc32x0-v1.07.patch.tar.bz2)
> was downloaded from the LPCLinux Web site.
> 
>  drivers/mtd/nand/lpc32xx_nand_slc.c | 369 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 364 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/nand/lpc32xx_nand_slc.c b/drivers/mtd/nand/lpc32xx_nand_slc.c
> index 719a74d..715152d 100644
> --- a/drivers/mtd/nand/lpc32xx_nand_slc.c
> +++ b/drivers/mtd/nand/lpc32xx_nand_slc.c
> @@ -3,15 +3,38 @@
>   *
>   * (C) Copyright 2015 Vladimir Zapolskiy <vz@mleia.com>
>   *
> + * Hardware ECC support original source code
> + * Copyright (C) 2008 by NXP Semiconductors
> + * Author: Kevin Wells
> + *
>   * SPDX-License-Identifier:	GPL-2.0+
>   */
>  
>  #include <common.h>
>  #include <nand.h>
> +#include <linux/mtd/nand_ecc.h>
>  #include <asm/errno.h>
>  #include <asm/io.h>
>  #include <asm/arch/clk.h>
>  #include <asm/arch/sys_proto.h>
> +#include <asm/arch/dma.h>
> +#include <asm/arch/cpu.h>
> +
> +#if defined(CONFIG_DMA_LPC32XX) && defined(CONFIG_SPL_BUILD)
> +#warning "DMA support in SPL image is not tested"
> +#endif
> +
> +/* Provide default for ECC size / bytes / OOB size for large page)
> + * if target did not. */
> +#if !defined(CONFIG_SYS_NAND_ECCSIZE)
> +#define CONFIG_SYS_NAND_ECCSIZE 2048
> +#endif

The definition above looks wrong...

s/CONFIG_SYS_NAND_ECCSIZE/CONFIG_SYS_NAND_PAGE_SIZE/g

CONFIG_SYS_NAND_ECCSIZE is hardcoded to 0x100 for LPC32xx NAND SLC.

> +#if !defined(CONFIG_SYS_NAND_ECCBYTES)
> +#define CONFIG_SYS_NAND_ECCBYTES 24
> +#endif
> +#if !defined(CONFIG_SYS_NAND_OOBSIZE)
> +#define CONFIG_SYS_NAND_OOBSIZE 64
> +#endif
> 
>  struct lpc32xx_nand_slc_regs {
>  	u32 data;
> @@ -33,11 +56,18 @@ struct lpc32xx_nand_slc_regs {
>  
>  /* CFG register */
>  #define CFG_CE_LOW		(1 << 5)
> +#define SLCCFG_DMA_ECC		(1 << 4) /* Enable DMA ECC bit */
> +#define SLCCFG_ECC_EN		(1 << 3) /* ECC enable bit */
> +#define SLCCFG_DMA_BURST	(1 << 2) /* DMA burst bit */
> +#define SLCCFG_DMA_DIR		(1 << 1) /* DMA write(0)/read(1) bit */
>  
>  /* CTRL register */
>  #define CTRL_SW_RESET		(1 << 2)
> +#define SLCCTRL_ECC_CLEAR	(1 << 1) /* Reset ECC bit */
> +#define SLCCTRL_DMA_START	(1 << 0) /* Start DMA channel bit */
>  
>  /* STAT register */
> +#define SLCSTAT_DMA_FIFO	(1 << 2) /* DMA FIFO has data bit */
>  #define STAT_NAND_READY		(1 << 0)

Do you see any chance of confusion, if "SLC" prefix is dropped from new
bit field definition? IMO it is more confusing, if there are two styles
of them at once.

>  /* INT_STAT register */
> @@ -54,6 +84,59 @@ struct lpc32xx_nand_slc_regs {
>  #define TAC_R_HOLD(n)		(max_t(uint32_t, (n), 0xF) << 4)
>  #define TAC_R_SETUP(n)		(max_t(uint32_t, (n), 0xF) << 0)
>  
> +/* control register definitions */
> +#define DMAC_CHAN_INT_TC_EN	(1 << 31) /* channel terminal count interrupt */
> +#define DMAC_CHAN_DEST_AUTOINC	(1 << 27) /* automatic destination increment */
> +#define DMAC_CHAN_SRC_AUTOINC	(1 << 26) /* automatic source increment */
> +#define DMAC_CHAN_DEST_AHB1	(1 << 25) /* AHB1 master for dest. transfer */
> +#define DMAC_CHAN_DEST_WIDTH_32	(1 << 22) /* Destination data width selection */
> +#define DMAC_CHAN_SRC_WIDTH_32	(1 << 19) /* Source data width selection */
> +#define DMAC_CHAN_DEST_BURST_1	0
> +#define DMAC_CHAN_DEST_BURST_4	(1 << 15) /* Destination data burst size */
> +#define DMAC_CHAN_SRC_BURST_1	0
> +#define DMAC_CHAN_SRC_BURST_4	(1 << 12) /* Source data burst size */
> +
> +/*
> + * config_ch register definitions
> + * DMAC_CHAN_FLOW_D_xxx: flow control with DMA as the controller
> + * DMAC_DEST_PERIP: Macro for loading destination peripheral
> + * DMAC_SRC_PERIP: Macro for loading source peripheral
> + */
> +#define DMAC_CHAN_FLOW_D_M2P	(0x1 << 11)
> +#define DMAC_CHAN_FLOW_D_P2M	(0x2 << 11)
> +#define DMAC_DEST_PERIP(n)	(((n) & 0x1F) << 6)
> +#define DMAC_SRC_PERIP(n)	(((n) & 0x1F) << 1)
> +
> +/*
> + * config_ch register definitions
> + * (source and destination peripheral ID numbers).
> + * These can be used with the DMAC_DEST_PERIP and DMAC_SRC_PERIP macros.
> + */
> +#define DMA_PERID_NAND1		1
> +
> +/* Channel enable bit */
> +#define DMAC_CHAN_ENABLE	(1 << 0)

All these DMA specific macro definitions above should go to arch/dma.h
added in 1/5, this is not the right place for any of them.

> +#define NAND_LARGE_BLOCK_PAGE_SIZE	2048

NB! See NB below.

This macro is used only in

  (CONFIG_SYS_NAND_ECCSIZE == NAND_LARGE_BLOCK_PAGE_SIZE ? 8 : 2)

cases, and actually the resulting value is always
(CONFIG_SYS_NAND_ECCSIZE / 0x100)

Here I would propose

* to add a sanity check in board_nand_init() for
(CONFIG_SYS_NAND_ECCSIZE == NAND_LARGE_BLOCK_PAGE_SIZE ||
CONFIG_SYS_NAND_ECCSIZE == NAND_SMALL_BLOCK_PAGE_SIZE),

* to replace the tristate operator case above with
(CONFIG_SYS_NAND_ECCSIZE / 0x100).

NB! Here CONFIG_SYS_NAND_ECCSIZE is misused, it must be replaced by
CONFIG_SYS_NAND_PAGE_SIZE, and 0x100 must be replaced by
CONFIG_SYS_NAND_ECCSIZE. In comments I keep the old notation to reduce
confusion, but please change the names.

> +#define NAND_SMALL_BLOCK_PAGE_SIZE	512

This macro is not used at the moment, but see the comment above.

> +#if defined(CONFIG_DMA_LPC32XX)
> +/*
> + * DMA Descriptors
> + * For Large Block: 17 descriptors = ((16 Data and ECC Read) + 1 Spare Area)
> + * For Small Block: 5 descriptors = ((4 Data and ECC Read) + 1 Spare Area)
> + */
> +static struct lpc32xx_dmac_ll dmalist[(CONFIG_SYS_NAND_ECCSIZE/256) * 2 + 1];

Aha, I see CONFIG_SYS_NAND_ECCSIZE / 0x100 again, it deserves its own macro.

> +static u32 ecc_buffer[8]; /* MAX ECC size */

And this 8 again stands for CONFIG_SYS_NAND_ECCSIZE / 0x100.

Well, you remember it must be (CONFIG_SYS_NAND_PAGE_SIZE /
CONFIG_SYS_NAND_ECCSIZE) in the corrected version, e.g. for small page
devices it is 2.

> +static int dmachan = -1;
> +
> +/*
> + * Helper macro for the DMA client (i.e. NAND SLC) to write the next
> + * DMA linked list item address (see arch/include/asm/arch-lpc32xx/dma.h).
> + */
> +#define lpc32xx_dmac_next_lli(x) ((u32)x)
> +#endif
> +
>  static struct lpc32xx_nand_slc_regs __iomem *lpc32xx_nand_slc_regs
>  	= (struct lpc32xx_nand_slc_regs __iomem *)SLC_NAND_BASE;
>  
> @@ -108,15 +191,266 @@ static int lpc32xx_nand_dev_ready(struct mtd_info *mtd)
>  	return readl(&lpc32xx_nand_slc_regs->stat) & STAT_NAND_READY;
>  }
>  
> -static void lpc32xx_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +#if defined(CONFIG_DMA_LPC32XX)
> +/*
> + * Prepares DMA descriptors for NAND RD/WR operations
> + * If the size is < 256 Bytes then it is assumed to be
> + * an OOB transfer
> + */
> +static void lpc32xx_nand_dma_configure(struct nand_chip *chip,
> +				       const u8 *buffer, int size,
> +				       int read)
>  {
> -	while (len-- > 0)
> -		*buf++ = readl(&lpc32xx_nand_slc_regs->data);
> +	u32 i, dmasrc, ctrl, ecc_ctrl, oob_ctrl, dmadst;
> +	u32 base = (u32)chip->IO_ADDR_R;

This is SLC_NAND_BASE, but the variable is not needed, see below.

> +	struct lpc32xx_dmac_ll *dmalist_cur;
> +	struct lpc32xx_dmac_ll *dmalist_cur_ecc;
> +
> +	/*
> +	 * CTRL descriptor entry for reading ECC
> +	 * Copy Multiple times to sync DMA with Flash Controller
> +	 */
> +	ecc_ctrl = 0x5 |
> +			DMAC_CHAN_SRC_BURST_1 |
> +			DMAC_CHAN_DEST_BURST_1 |
> +			DMAC_CHAN_SRC_WIDTH_32 |
> +			DMAC_CHAN_DEST_WIDTH_32 |
> +			DMAC_CHAN_DEST_AHB1;
> +
> +	/* CTRL descriptor entry for reading/writing Data */
> +	ctrl = 64 | /* 256/4 */
> +			DMAC_CHAN_SRC_BURST_4 |
> +			DMAC_CHAN_DEST_BURST_4 |
> +			DMAC_CHAN_SRC_WIDTH_32 |
> +			DMAC_CHAN_DEST_WIDTH_32 |
> +			DMAC_CHAN_DEST_AHB1;
> +
> +	/* CTRL descriptor entry for reading/writing Spare Area */
> +	oob_ctrl = (CONFIG_SYS_NAND_OOBSIZE / 4) |
> +			DMAC_CHAN_SRC_BURST_4 |
> +			DMAC_CHAN_DEST_BURST_4 |
> +			DMAC_CHAN_SRC_WIDTH_32 |
> +			DMAC_CHAN_DEST_WIDTH_32 |
> +			DMAC_CHAN_DEST_AHB1;
> +
> +	if (read) {
> +		dmasrc = base + offsetof(struct lpc32xx_nand_slc_regs,
> +					 dma_data);

This is &lpc32xx_nand_slc_regs->dma_data.

> +		dmadst = (u32)buffer;
> +		ctrl |= DMAC_CHAN_DEST_AUTOINC;
> +	} else {
> +		dmadst = base + offsetof(struct lpc32xx_nand_slc_regs,
> +					 dma_data);

This is &lpc32xx_nand_slc_regs->dma_data.

> +		dmasrc = (u32)buffer;
> +		ctrl |= DMAC_CHAN_SRC_AUTOINC;
> +	}
> +
> +	/*
> +	 * Write Operation Sequence for Small Block NAND
> +	 * ----------------------------------------------------------
> +	 * 1. X'fer 256 bytes of data from Memory to Flash.
> +	 * 2. Copy generated ECC data from Register to Spare Area
> +	 * 3. X'fer next 256 bytes of data from Memory to Flash.
> +	 * 4. Copy generated ECC data from Register to Spare Area.
> +	 * 5. X'fer 16 byets of Spare area from Memory to Flash.
> +	 * Read Operation Sequence for Small Block NAND
> +	 * ----------------------------------------------------------
> +	 * 1. X'fer 256 bytes of data from Flash to Memory.
> +	 * 2. Copy generated ECC data from Register to ECC calc Buffer.
> +	 * 3. X'fer next 256 bytes of data from Flash to Memory.
> +	 * 4. Copy generated ECC data from Register to ECC calc Buffer.
> +	 * 5. X'fer 16 bytes of Spare area from Flash to Memory.
> +	 * Write Operation Sequence for Large Block NAND
> +	 * ----------------------------------------------------------
> +	 * 1. Steps(1-4) of Write Operations repeate for four times
> +	 * which generates 16 DMA descriptors to X'fer 2048 bytes of
> +	 * data & 32 bytes of ECC data.
> +	 * 2. X'fer 64 bytes of Spare area from Memory to Flash.
> +	 * Read Operation Sequence for Large Block NAND
> +	 * ----------------------------------------------------------
> +	 * 1. Steps(1-4) of Read Operations repeate for four times
> +	 * which generates 16 DMA descriptors to X'fer 2048 bytes of
> +	 * data & 32 bytes of ECC data.
> +	 * 2. X'fer 64 bytes of Spare area from Flash to Memory.
> +	 */
> +
> +	for (i = 0; i < size/256; i++) {
> +		dmalist_cur = &dmalist[i * 2];
> +		dmalist_cur_ecc = &dmalist[(i * 2) + 1];
> +
> +		dmalist_cur->dma_src = (read ? (dmasrc) : (dmasrc + (i*256)));
> +		dmalist_cur->dma_dest = (read ? (dmadst + (i*256)) : dmadst);
> +		dmalist_cur->next_lli = lpc32xx_dmac_next_lli(dmalist_cur_ecc);
> +		dmalist_cur->next_ctrl = ctrl;
> +
> +		dmalist_cur_ecc->dma_src =
> +			base + offsetof(struct lpc32xx_nand_slc_regs, ecc);
> +		dmalist_cur_ecc->dma_dest = (u32)&ecc_buffer[i];
> +		dmalist_cur_ecc->next_lli =
> +			lpc32xx_dmac_next_lli(&dmalist[(i * 2) + 2]);
> +		dmalist_cur_ecc->next_ctrl = ecc_ctrl;
> +	}
> +
> +	if (i) { /* Data only transfer */
> +		dmalist_cur_ecc = &dmalist[(i * 2) - 1];
> +		dmalist_cur_ecc->next_lli = 0;
> +		dmalist_cur_ecc->next_ctrl |= DMAC_CHAN_INT_TC_EN;
> +		return;
> +	}
> +
> +	/* OOB only transfer */
> +	if (read) {
> +		dmasrc = base + offsetof(struct lpc32xx_nand_slc_regs,
> +					 dma_data);

This is &lpc32xx_nand_slc_regs->dma_data.

> +		dmadst = (u32)buffer;
> +		oob_ctrl |= DMAC_CHAN_DEST_AUTOINC;
> +	} else {
> +		dmadst = base + offsetof(struct lpc32xx_nand_slc_regs,
> +					 dma_data);

This is &lpc32xx_nand_slc_regs->dma_data. No "base" variable please.

> +		dmasrc = (u32)buffer;
> +		oob_ctrl |= DMAC_CHAN_SRC_AUTOINC;
> +	}
> +
> +	/* Read/ Write Spare Area Data To/From Flash */
> +	dmalist_cur = &dmalist[i * 2];
> +	dmalist_cur->dma_src = dmasrc;
> +	dmalist_cur->dma_dest = dmadst;
> +	dmalist_cur->next_lli = 0;
> +	dmalist_cur->next_ctrl = (oob_ctrl | DMAC_CHAN_INT_TC_EN);
>  }
>  
> -static uint8_t lpc32xx_read_byte(struct mtd_info *mtd)
> +static void lpc32xx_nand_xfer(struct mtd_info *mtd, const u8 *buf,
> +			      int len, int read)
>  {
> -	return readl(&lpc32xx_nand_slc_regs->data);
> +	struct nand_chip *chip = mtd->priv;
> +	u32 config;
> +
> +	/* DMA Channel Configuration */
> +	config = (read ? DMAC_CHAN_FLOW_D_P2M : DMAC_CHAN_FLOW_D_M2P) |
> +		(read ? DMAC_DEST_PERIP(0) : DMAC_DEST_PERIP(DMA_PERID_NAND1)) |
> +		(read ? DMAC_SRC_PERIP(DMA_PERID_NAND1) : DMAC_SRC_PERIP(0)) |
> +		DMAC_CHAN_ENABLE;
> +
> +	/* Prepare DMA descriptors */
> +	lpc32xx_nand_dma_configure(chip, buf, len, read);
> +
> +	/* Setup SLC controller and start transfer */
> +	if (read)
> +		setbits_le32(&lpc32xx_nand_slc_regs->cfg, SLCCFG_DMA_DIR);
> +	else  /* NAND_ECC_WRITE */
> +		clrbits_le32(&lpc32xx_nand_slc_regs->cfg, SLCCFG_DMA_DIR);
> +	setbits_le32(&lpc32xx_nand_slc_regs->cfg, SLCCFG_DMA_BURST);
> +
> +	/* Write length for new transfers */
> +	if (!((readl(&lpc32xx_nand_slc_regs->stat) & SLCSTAT_DMA_FIFO) |
> +	      readl(&lpc32xx_nand_slc_regs->tc))) {
> +		int tmp = (len != mtd->oobsize) ? mtd->oobsize : 0;
> +		writel(len + tmp, &lpc32xx_nand_slc_regs->tc);
> +	}
> +
> +	setbits_le32(&lpc32xx_nand_slc_regs->ctrl, SLCCTRL_DMA_START);
> +
> +	/* Start DMA transfers */
> +	lpc32xx_dma_start_xfer(dmachan, dmalist, config);
> +
> +	/* Wait for NAND to be ready */
> +	while (!lpc32xx_nand_dev_ready(mtd))
> +		;
> +
> +	/* Wait till DMA transfer is DONE */
> +	if (lpc32xx_dma_wait_status(dmachan))
> +		pr_err("NAND DMA transfer error!\r\n");
> +
> +	/* Stop DMA & HW ECC */
> +	clrbits_le32(&lpc32xx_nand_slc_regs->ctrl, SLCCTRL_DMA_START);
> +	clrbits_le32(&lpc32xx_nand_slc_regs->cfg,
> +		     SLCCFG_DMA_DIR | SLCCFG_DMA_BURST |
> +		     SLCCFG_ECC_EN | SLCCFG_DMA_ECC);
> +}
> +
> +static u32 slc_ecc_copy_to_buffer(u8 *spare, const u32 *ecc, int count)
> +{
> +	int i;
> +	for (i = 0; i < (count * 3); i += 3) {
> +		u32 ce = ecc[i/3];
> +		ce = ~(ce << 2) & 0xFFFFFF;
> +		spare[i+2] = (u8)(ce & 0xFF); ce >>= 8;
> +		spare[i+1] = (u8)(ce & 0xFF); ce >>= 8;
> +		spare[i]   = (u8)(ce & 0xFF);
> +	}
> +	return 0;
> +}
> +
> +static int lpc32xx_ecc_calculate(struct mtd_info *mtd, const uint8_t *dat,
> +				 uint8_t *ecc_code)
> +{
> +	return slc_ecc_copy_to_buffer(ecc_code, ecc_buffer,
> +				      CONFIG_SYS_NAND_ECCSIZE
> +				      == NAND_LARGE_BLOCK_PAGE_SIZE ? 8 : 2);
> +}
> +
> +/*
> + * Enables and prepares SLC NAND controller
> + * for doing data transfers with H/W ECC enabled.
> + */
> +static void lpc32xx_hwecc_enable(struct mtd_info *mtd, int mode)
> +{
> +	/* Clear ECC */
> +	writel(SLCCTRL_ECC_CLEAR, &lpc32xx_nand_slc_regs->ctrl);
> +
> +	/* Setup SLC controller for H/W ECC operations */
> +	setbits_le32(&lpc32xx_nand_slc_regs->cfg,
> +		     SLCCFG_ECC_EN | SLCCFG_DMA_ECC);
> +}
> +
> +/*
> + * lpc32xx_correct_data - [NAND Interface] Detect and correct bit error(s)
> + * mtd:	MTD block structure
> + * dat:	raw data read from the chip
> + * read_ecc:	ECC from the chip
> + * calc_ecc:	the ECC calculated from raw data
> + *
> + * Detect and correct a 1 bit error for 256 byte block
> + */
> +int lpc32xx_correct_data(struct mtd_info *mtd, u_char *dat,
> +			 u_char *read_ecc, u_char *calc_ecc)
> +{
> +	u8 i, nb_ecc256;
> +	int ret1, ret2 = 0;
> +	u_char *r = read_ecc;
> +	u_char *c = calc_ecc;
> +	u16 data_offset = 0;
> +
> +	nb_ecc256 = (CONFIG_SYS_NAND_ECCSIZE == NAND_LARGE_BLOCK_PAGE_SIZE
> +		     ? 8 : 2);
> +
> +	for (i = 0 ; i < nb_ecc256 ; i++ , r += 3, c += 3, data_offset += 256) {
> +		ret1 = nand_correct_data(mtd, dat + data_offset, r, c);
> +
> +		if (ret1 < 0)
> +			return -EBADMSG;
> +		else
> +			ret2 += ret1;
> +	}
> +
> +	return ret2;
> +}
> +
> +static void lpc32xx_dma_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +	lpc32xx_nand_xfer(mtd, buf, len, 1);
> +}
> +
> +static void lpc32xx_dma_write_buf(struct mtd_info *mtd, const uint8_t *buf,
> +				  int len)
> +{
> +	lpc32xx_nand_xfer(mtd, buf, len, 0);
> +}
> +#else
> +static void lpc32xx_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +	while (len-- > 0)
> +		*buf++ = readl(&lpc32xx_nand_slc_regs->data);
>  }

Please preserve the original order:

lpc32xx_read_buf()
lpc32xx_read_byte()
lpc32xx_write_buf()
lpc32xx_write_byte()

I'd like to see $OP_byte() follows $OP_buf() as a motivator to measure
performance degradation one day, if $OP_byte() body is replaced by
$OP_buf(..., 1).

>  static void lpc32xx_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
> @@ -124,6 +458,12 @@ static void lpc32xx_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
>  	while (len-- > 0)
>  		writel(*buf++, &lpc32xx_nand_slc_regs->data);
>  }
> +#endif
> +
> +static uint8_t lpc32xx_read_byte(struct mtd_info *mtd)
> +{
> +	return readl(&lpc32xx_nand_slc_regs->data);
> +}
>  
>  static void lpc32xx_write_byte(struct mtd_info *mtd, uint8_t byte)
>  {
> @@ -137,9 +477,45 @@ static void lpc32xx_write_byte(struct mtd_info *mtd, uint8_t byte)
>   */
>  int board_nand_init(struct nand_chip *lpc32xx_chip)
>  {
> +#if defined(CONFIG_DMA_LPC32XX)
> +	/* Acquire a channel for our use */
> +	dmachan = lpc32xx_dma_get_channel();
> +	if (unlikely(dmachan < 0)) {
> +		pr_info("Unable to get free DMA channel for NAND transfers\n");
> +		return -1;
> +	}
> +#endif
> +
>  	lpc32xx_chip->cmd_ctrl  = lpc32xx_nand_cmd_ctrl;
>  	lpc32xx_chip->dev_ready = lpc32xx_nand_dev_ready;
>  
> +#if defined(CONFIG_DMA_LPC32XX)
> +	/*
> +	 * Hardware ECC calculation is supported when
> +	 * DMA driver is selected.
> +	 */
> +	lpc32xx_chip->ecc.mode = NAND_ECC_HW;
> +
> +	/*
> +	 * The implementation of these functions is quite common, but
> +	 * they MUST be defined, because access to data register
> +	 * is strictly 32-bit aligned.
> +	 */
> +	lpc32xx_chip->read_byte		= lpc32xx_read_byte;
> +	lpc32xx_chip->write_byte	= lpc32xx_write_byte;

.read_byte() / .write_byte() are shared, these two lines should be
outside of #if defined(CONFIG_DMA_LPC32XX)

> +	lpc32xx_chip->read_buf		= lpc32xx_dma_read_buf;
> +	lpc32xx_chip->write_buf		= lpc32xx_dma_write_buf;
> +
> +	lpc32xx_chip->ecc.calculate	= lpc32xx_ecc_calculate;
> +	lpc32xx_chip->ecc.correct	= lpc32xx_correct_data;
> +	lpc32xx_chip->ecc.hwctl		= lpc32xx_hwecc_enable;
> +	lpc32xx_chip->chip_delay	= 2000;
> +
> +	lpc32xx_chip->ecc.bytes		= CONFIG_SYS_NAND_ECCBYTES;
> +	lpc32xx_chip->ecc.size		= CONFIG_SYS_NAND_ECCSIZE;
> +	lpc32xx_chip->ecc.strength	= 1;

Please double check, because this is wrong, it should be the same as
below for PIO:

 	lpc32xx_chip->ecc.size     = CONFIG_SYS_NAND_ECCSIZE;
 	lpc32xx_chip->ecc.bytes    = CONFIG_SYS_NAND_ECCBYTES /
(CONFIG_SYS_NAND_PAGE_SIZE / CONFIG_SYS_NAND_ECCSIZE);
 	lpc32xx_chip->ecc.strength = 1;

and therefore this chunk should be placed outside of #if
defined(CONFIG_DMA_LPC32XX)/ #else / #endif block.

> +#else
>  	/*
>  	 * Hardware ECC calculation is not supported by the driver,
>  	 * because it requires DMA support, see LPC32x0 User Manual,
> @@ -164,6 +540,7 @@ int board_nand_init(struct nand_chip *lpc32xx_chip)
>  	lpc32xx_chip->ecc.size     = 256;
>  	lpc32xx_chip->ecc.bytes    = 3;
>  	lpc32xx_chip->ecc.strength = 1;

^^^ It is the same as for DMA branch.

> +#endif
>  
>  #if defined(CONFIG_SYS_NAND_USE_FLASH_BBT)
>  	lpc32xx_chip->bbt_options |= NAND_BBT_USE_FLASH;
> 

--
With best wishes,
Vladimir