Hi Sylvain,

On 05.08.2015 21:31, slemieux.t...@gmail.com wrote:
> From: Sylvain Lemieux <slemi...@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 <slemi...@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 <v...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to