[U-Boot] [PATCH v6 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 v5 to v6: * Addressed Vladimir's comments on LPC32xx NAND SLC driver: - move DMAC registers definitions from NAND SLC to "dma.h". - update NAND SLC to use the new DMA channel API. - generate the fatal BUG on the client side when attempting a transfer on an unallocated DMA channel. - kept the original order for read/write byte/buf functions. * Following feedback on maillist from Vladimir, the following update were done: - the ECC size / ECC bytes are fix to 256 & 3 in the driver. - add comments regarding the ECC correction done by page when using the DMA controller scatter/gather mode through linked list (i.e. ECC correction process). - definition used for managing small vs. large page were updated inside the driver to make them more clear. - definition used for the ECC process when the DMA is enable were updated to avoid any confusion with basic definition. - possible to select small page NAND using a single define. - use define instead of value for internal process logic.
Changes from v4 to v5: * Addressed Marek's comments on LPC32xx NAND SLC 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 SLC 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 | 382 +++++++++++++++++++++++++++++++++++- 1 file changed, 378 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/lpc32xx_nand_slc.c b/drivers/mtd/nand/lpc32xx_nand_slc.c index 719a74d..2a32264 100644 --- a/drivers/mtd/nand/lpc32xx_nand_slc.c +++ b/drivers/mtd/nand/lpc32xx_nand_slc.c @@ -3,15 +3,48 @@ * * (C) Copyright 2015 Vladimir Zapolskiy vz@mleia.com * + * Hardware ECC support original source code + * Copyright (C) 2008 by NXP Semiconductors + * Author: Kevin Wells + * + * Copyright (c) 2015 Tyco Fire Protection Products. + * * 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 page size (large page), if target did not. */ +#if !defined(CONFIG_SYS_NAND_PAGE_SIZE) +#define CONFIG_SYS_NAND_PAGE_SIZE 2048 +#endif + +#define NAND_LARGE_BLOCK_PAGE_SIZE 2048 +#define NAND_SMALL_BLOCK_PAGE_SIZE 512 + +/* Verify if a valid page size was define and configure OOB size. */ +#if (CONFIG_SYS_NAND_PAGE_SIZE == NAND_LARGE_BLOCK_PAGE_SIZE) +#define CONFIG_SYS_NAND_OOBSIZE 64 +#elif (CONFIG_SYS_NAND_PAGE_SIZE == NAND_SMALL_BLOCK_PAGE_SIZE) +#define CONFIG_SYS_NAND_OOBSIZE 16 +#else +#error "CONFIG_SYS_NAND_PAGE_SIZE set to an invalid value" +#endif + +#define CONFIG_SYS_NAND_ECCBYTES 3 +#define CONFIG_SYS_NAND_ECCSIZE 256
struct lpc32xx_nand_slc_regs { u32 data; @@ -33,11 +66,18 @@ struct lpc32xx_nand_slc_regs {
/* CFG register */ #define CFG_CE_LOW (1 << 5) +#define CFG_DMA_ECC (1 << 4) /* Enable DMA ECC bit */ +#define CFG_ECC_EN (1 << 3) /* ECC enable bit */ +#define CFG_DMA_BURST (1 << 2) /* DMA burst bit */ +#define CFG_DMA_DIR (1 << 1) /* DMA write(0)/read(1) bit */
/* CTRL register */ #define CTRL_SW_RESET (1 << 2) +#define CTRL_ECC_CLEAR (1 << 1) /* Reset ECC bit */ +#define CTRL_DMA_START (1 << 0) /* Start DMA channel bit */
/* STAT register */ +#define STAT_DMA_FIFO (1 << 2) /* DMA FIFO has data bit */ #define STAT_NAND_READY (1 << 0)
/* INT_STAT register */ @@ -54,6 +94,33 @@ 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)
+#if defined(CONFIG_DMA_LPC32XX) +/* Total ECC bytes and size; refer to board_nand_init() for details. */ +#define ECCSTEPS (CONFIG_SYS_NAND_PAGE_SIZE / CONFIG_SYS_NAND_ECCSIZE) +#define TOTAL_ECCBYTES (CONFIG_SYS_NAND_ECCBYTES * ECCSTEPS) +#define TOTAL_ECCSIZE (CONFIG_SYS_NAND_ECCSIZE * ECCSTEPS) + +/* + * 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[ECCSTEPS * 2 + 1]; +static u32 ecc_buffer[8]; /* MAX ECC size */ +static unsigned int dmachan = (unsigned int)-1; /* Invalid channel */ + +/* + * 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). + * - to assign the DMA data register to DMA source or destination address. + * - to assign the ECC register to DMA source or destination address. + */ +#define lpc32xx_dmac_next_lli(x) ((u32)x) +#define lpc32xx_dmac_set_dma_data() ((u32)&lpc32xx_nand_slc_regs->dma_data) +#define lpc32xx_dmac_set_ecc() ((u32)&lpc32xx_nand_slc_regs->ecc) +#endif + static struct lpc32xx_nand_slc_regs __iomem *lpc32xx_nand_slc_regs = (struct lpc32xx_nand_slc_regs __iomem *)SLC_NAND_BASE;
@@ -108,22 +175,278 @@ static int lpc32xx_nand_dev_ready(struct mtd_info *mtd) return readl(&lpc32xx_nand_slc_regs->stat) & STAT_NAND_READY; }
+#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) +{ + u32 i, dmasrc, ctrl, ecc_ctrl, oob_ctrl, dmadst; + 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 = (CONFIG_SYS_NAND_ECCSIZE / 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 = lpc32xx_dmac_set_dma_data(); + dmadst = (u32)buffer; + ctrl |= DMAC_CHAN_DEST_AUTOINC; + } else { + dmadst = lpc32xx_dmac_set_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/CONFIG_SYS_NAND_ECCSIZE; 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 = lpc32xx_dmac_set_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 = lpc32xx_dmac_set_dma_data(); + dmadst = (u32)buffer; + oob_ctrl |= DMAC_CHAN_DEST_AUTOINC; + } else { + dmadst = lpc32xx_dmac_set_dma_data(); + 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 void lpc32xx_nand_xfer(struct mtd_info *mtd, const u8 *buf, + int len, int read) +{ + struct nand_chip *chip = mtd->priv; + u32 config; + int ret; + + /* 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, CFG_DMA_DIR); + else /* NAND_ECC_WRITE */ + clrbits_le32(&lpc32xx_nand_slc_regs->cfg, CFG_DMA_DIR); + setbits_le32(&lpc32xx_nand_slc_regs->cfg, CFG_DMA_BURST); + + /* Write length for new transfers */ + if (!((readl(&lpc32xx_nand_slc_regs->stat) & STAT_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, CTRL_DMA_START); + + /* Start DMA transfers */ + ret = lpc32xx_dma_start_xfer(dmachan, dmalist, config); + if (unlikely(ret < 0)) + BUG(); + + + /* 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, CTRL_DMA_START); + clrbits_le32(&lpc32xx_nand_slc_regs->cfg, + CFG_DMA_DIR | CFG_DMA_BURST | CFG_ECC_EN | CFG_DMA_ECC); +} + +static u32 slc_ecc_copy_to_buffer(u8 *spare, const u32 *ecc, int count) +{ + int i; + for (i = 0; i < (count * CONFIG_SYS_NAND_ECCBYTES); + i += CONFIG_SYS_NAND_ECCBYTES) { + u32 ce = ecc[i / CONFIG_SYS_NAND_ECCBYTES]; + 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, ECCSTEPS); +} + +/* + * 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(CTRL_ECC_CLEAR, &lpc32xx_nand_slc_regs->ctrl); + + /* Setup SLC controller for H/W ECC operations */ + setbits_le32(&lpc32xx_nand_slc_regs->cfg, CFG_ECC_EN | CFG_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) +{ + unsigned int i; + int ret1, ret2 = 0; + u_char *r = read_ecc; + u_char *c = calc_ecc; + u16 data_offset = 0; + + for (i = 0 ; i < ECCSTEPS ; i++) { + r += CONFIG_SYS_NAND_ECCBYTES; + c += CONFIG_SYS_NAND_ECCBYTES; + data_offset += CONFIG_SYS_NAND_ECCSIZE; + + ret1 = nand_correct_data(mtd, dat + data_offset, r, c); + if (ret1 < 0) + return -EBADMSG; + else + ret2 += ret1; + } + + return ret2; +} +#endif + +#if defined(CONFIG_DMA_LPC32XX) +static void lpc32xx_dma_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) +{ + lpc32xx_nand_xfer(mtd, buf, len, 1); +} +#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); } +#endif
static uint8_t lpc32xx_read_byte(struct mtd_info *mtd) { return readl(&lpc32xx_nand_slc_regs->data); }
+#if defined(CONFIG_DMA_LPC32XX) +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_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) { while (len-- > 0) writel(*buf++, &lpc32xx_nand_slc_regs->data); } +#endif
static void lpc32xx_write_byte(struct mtd_info *mtd, uint8_t byte) { @@ -137,10 +460,61 @@ 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) + int ret; + + /* Acquire a channel for our use */ + ret = lpc32xx_dma_get_channel(); + if (unlikely(ret < 0)) { + pr_info("Unable to get free DMA channel for NAND transfers\n"); + return -1; + } + dmachan = (unsigned int)ret; +#endif + lpc32xx_chip->cmd_ctrl = lpc32xx_nand_cmd_ctrl; lpc32xx_chip->dev_ready = lpc32xx_nand_dev_ready;
/* + * 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; + +#if defined(CONFIG_DMA_LPC32XX) + /* + * Hardware ECC calculation is supported when + * DMA driver is selected. + */ + lpc32xx_chip->ecc.mode = NAND_ECC_HW; + + 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; + + /* + * ECC correction is done by page when using the DMA controller + * scatter/gather mode through linked list; + * refer to UM10326, "LPC32x0 and LPC32x0/01 User manual" - Rev. 3 + * section 9.7 and tables 173 notes for details. + * + * The error correction is done on each page and process in multiple + * steps of 256 bytes inside the driver. + * + * The total ECC size and bytes for a page are used; + * NAND base code (HW ECC) will call the read / write buffer functions + * using the total ECC size and bytes for a page as a single step. + */ + lpc32xx_chip->ecc.size = TOTAL_ECCSIZE; + lpc32xx_chip->ecc.bytes = TOTAL_ECCBYTES; +#else + /* * Hardware ECC calculation is not supported by the driver, * because it requires DMA support, see LPC32x0 User Manual, * note after SLC_ECC register description (UM10326, p.198) @@ -153,16 +527,16 @@ int board_nand_init(struct nand_chip *lpc32xx_chip) * is strictly 32-bit aligned. */ lpc32xx_chip->read_buf = lpc32xx_read_buf; - lpc32xx_chip->read_byte = lpc32xx_read_byte; lpc32xx_chip->write_buf = lpc32xx_write_buf; - lpc32xx_chip->write_byte = lpc32xx_write_byte;
/* * Use default ECC layout, but these values are predefined * for both small and large page NAND flash devices. */ - lpc32xx_chip->ecc.size = 256; - lpc32xx_chip->ecc.bytes = 3; + lpc32xx_chip->ecc.size = CONFIG_SYS_NAND_ECCSIZE; + lpc32xx_chip->ecc.bytes = CONFIG_SYS_NAND_ECCBYTES; +#endif + lpc32xx_chip->ecc.strength = 1;
#if defined(CONFIG_SYS_NAND_USE_FLASH_BBT)
Hi Sylvain,
On 10.08.2015 15:16, 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 v5 to v6:
- Addressed Vladimir's comments on LPC32xx NAND SLC driver:
- move DMAC registers definitions from NAND SLC to "dma.h".
- update NAND SLC to use the new DMA channel API.
- generate the fatal BUG on the client side when attempting a transfer on an unallocated DMA channel.
- kept the original order for read/write byte/buf functions.
- Following feedback on maillist from Vladimir, the following update were done:
- the ECC size / ECC bytes are fix to 256 & 3 in the driver.
- add comments regarding the ECC correction done by page when using the DMA controller scatter/gather mode through linked list (i.e. ECC correction process).
- definition used for managing small vs. large page were updated inside the driver to make them more clear.
- definition used for the ECC process when the DMA is enable were updated to avoid any confusion with basic definition.
- possible to select small page NAND using a single define.
- use define instead of value for internal process logic.
Changes from v4 to v5:
- Addressed Marek's comments on LPC32xx NAND SLC 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 SLC 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:
- Fixed checkpatch script output in legacy code.
- Use u-boot API for register access to remove the volatile in register definition taken from "lpc3250.h" header file.
- 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 | 382 +++++++++++++++++++++++++++++++++++- 1 file changed, 378 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/lpc32xx_nand_slc.c b/drivers/mtd/nand/lpc32xx_nand_slc.c index 719a74d..2a32264 100644 --- a/drivers/mtd/nand/lpc32xx_nand_slc.c +++ b/drivers/mtd/nand/lpc32xx_nand_slc.c @@ -3,15 +3,48 @@
- (C) Copyright 2015 Vladimir Zapolskiy vz@mleia.com
- Hardware ECC support original source code
- Copyright (C) 2008 by NXP Semiconductors
- Author: Kevin Wells
- Copyright (c) 2015 Tyco Fire Protection Products.
*/
- 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 page size (large page), if target did not. */ +#if !defined(CONFIG_SYS_NAND_PAGE_SIZE) +#define CONFIG_SYS_NAND_PAGE_SIZE 2048 +#endif
+#define NAND_LARGE_BLOCK_PAGE_SIZE 2048 +#define NAND_SMALL_BLOCK_PAGE_SIZE 512
It makes sense to reorder block of lines above to get
#if !defined(CONFIG_SYS_NAND_PAGE_SIZE) #define CONFIG_SYS_NAND_PAGE_SIZE NAND_LARGE_BLOCK_PAGE_SIZE #endif
+/* Verify if a valid page size was define and configure OOB size. */ +#if (CONFIG_SYS_NAND_PAGE_SIZE == NAND_LARGE_BLOCK_PAGE_SIZE) +#define CONFIG_SYS_NAND_OOBSIZE 64 +#elif (CONFIG_SYS_NAND_PAGE_SIZE == NAND_SMALL_BLOCK_PAGE_SIZE) +#define CONFIG_SYS_NAND_OOBSIZE 16 +#else +#error "CONFIG_SYS_NAND_PAGE_SIZE set to an invalid value" +#endif
+#define CONFIG_SYS_NAND_ECCBYTES 3 +#define CONFIG_SYS_NAND_ECCSIZE 256
These defines are OK, but see my bottom comment below related to this subject.
Also it's worth to mention that these defines are conflicting with the same defines from update to my board: http://lists.denx.de/pipermail/u-boot/2015-July/219251.html -- I don't understand why my changes are still not present in U-boot/master e.g. to catch this kind of problems.
So, CONFIG_SYS_NAND_ECCSIZE, CONFIG_SYS_NAND_ECCBYTES and CONFIG_SYS_NAND_OOBSIZE are used outside of the driver code (to be precise they are used in drivers/mtd/nand/nand_spl_simple.c), and therefore this is not the right place to define them IMHO.
struct lpc32xx_nand_slc_regs { u32 data; @@ -33,11 +66,18 @@ struct lpc32xx_nand_slc_regs {
/* CFG register */ #define CFG_CE_LOW (1 << 5) +#define CFG_DMA_ECC (1 << 4) /* Enable DMA ECC bit */ +#define CFG_ECC_EN (1 << 3) /* ECC enable bit */ +#define CFG_DMA_BURST (1 << 2) /* DMA burst bit */ +#define CFG_DMA_DIR (1 << 1) /* DMA write(0)/read(1) bit */
/* CTRL register */ #define CTRL_SW_RESET (1 << 2) +#define CTRL_ECC_CLEAR (1 << 1) /* Reset ECC bit */ +#define CTRL_DMA_START (1 << 0) /* Start DMA channel bit */
/* STAT register */ +#define STAT_DMA_FIFO (1 << 2) /* DMA FIFO has data bit */ #define STAT_NAND_READY (1 << 0)
/* INT_STAT register */ @@ -54,6 +94,33 @@ 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)
+#if defined(CONFIG_DMA_LPC32XX) +/* Total ECC bytes and size; refer to board_nand_init() for details. */ +#define ECCSTEPS (CONFIG_SYS_NAND_PAGE_SIZE / CONFIG_SYS_NAND_ECCSIZE) +#define TOTAL_ECCBYTES (CONFIG_SYS_NAND_ECCBYTES * ECCSTEPS) +#define TOTAL_ECCSIZE (CONFIG_SYS_NAND_ECCSIZE * ECCSTEPS)
See my comment below regarding these two defines.
+/*
- 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[ECCSTEPS * 2 + 1]; +static u32 ecc_buffer[8]; /* MAX ECC size */ +static unsigned int dmachan = (unsigned int)-1; /* Invalid channel */
+/*
- 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).
- to assign the DMA data register to DMA source or destination address.
- to assign the ECC register to DMA source or destination address.
- */
+#define lpc32xx_dmac_next_lli(x) ((u32)x) +#define lpc32xx_dmac_set_dma_data() ((u32)&lpc32xx_nand_slc_regs->dma_data) +#define lpc32xx_dmac_set_ecc() ((u32)&lpc32xx_nand_slc_regs->ecc) +#endif
static struct lpc32xx_nand_slc_regs __iomem *lpc32xx_nand_slc_regs = (struct lpc32xx_nand_slc_regs __iomem *)SLC_NAND_BASE;
@@ -108,22 +175,278 @@ static int lpc32xx_nand_dev_ready(struct mtd_info *mtd) return readl(&lpc32xx_nand_slc_regs->stat) & STAT_NAND_READY; }
+#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)+{
- u32 i, dmasrc, ctrl, ecc_ctrl, oob_ctrl, dmadst;
- 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 = (CONFIG_SYS_NAND_ECCSIZE / 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 = lpc32xx_dmac_set_dma_data();dmadst = (u32)buffer;ctrl |= DMAC_CHAN_DEST_AUTOINC;- } else {
dmadst = lpc32xx_dmac_set_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/CONFIG_SYS_NAND_ECCSIZE; 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 = lpc32xx_dmac_set_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 = lpc32xx_dmac_set_dma_data();dmadst = (u32)buffer;oob_ctrl |= DMAC_CHAN_DEST_AUTOINC;- } else {
dmadst = lpc32xx_dmac_set_dma_data();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 void lpc32xx_nand_xfer(struct mtd_info *mtd, const u8 *buf,
int len, int read)+{
- struct nand_chip *chip = mtd->priv;
- u32 config;
- int ret;
- /* 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, CFG_DMA_DIR);- else /* NAND_ECC_WRITE */
clrbits_le32(&lpc32xx_nand_slc_regs->cfg, CFG_DMA_DIR);- setbits_le32(&lpc32xx_nand_slc_regs->cfg, CFG_DMA_BURST);
- /* Write length for new transfers */
- if (!((readl(&lpc32xx_nand_slc_regs->stat) & STAT_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, CTRL_DMA_START);
- /* Start DMA transfers */
- ret = lpc32xx_dma_start_xfer(dmachan, dmalist, config);
- if (unlikely(ret < 0))
BUG();- /* 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, CTRL_DMA_START);
- clrbits_le32(&lpc32xx_nand_slc_regs->cfg,
CFG_DMA_DIR | CFG_DMA_BURST | CFG_ECC_EN | CFG_DMA_ECC);+}
+static u32 slc_ecc_copy_to_buffer(u8 *spare, const u32 *ecc, int count) +{
- int i;
- for (i = 0; i < (count * CONFIG_SYS_NAND_ECCBYTES);
i += CONFIG_SYS_NAND_ECCBYTES) {u32 ce = ecc[i / CONFIG_SYS_NAND_ECCBYTES];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, ECCSTEPS);
+}
+/*
- 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(CTRL_ECC_CLEAR, &lpc32xx_nand_slc_regs->ctrl);
- /* Setup SLC controller for H/W ECC operations */
- setbits_le32(&lpc32xx_nand_slc_regs->cfg, CFG_ECC_EN | CFG_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)+{
- unsigned int i;
- int ret1, ret2 = 0;
- u_char *r = read_ecc;
- u_char *c = calc_ecc;
- u16 data_offset = 0;
- for (i = 0 ; i < ECCSTEPS ; i++) {
r += CONFIG_SYS_NAND_ECCBYTES;c += CONFIG_SYS_NAND_ECCBYTES;data_offset += CONFIG_SYS_NAND_ECCSIZE;ret1 = nand_correct_data(mtd, dat + data_offset, r, c);if (ret1 < 0)return -EBADMSG;elseret2 += ret1;- }
- return ret2;
+} +#endif
+#if defined(CONFIG_DMA_LPC32XX) +static void lpc32xx_dma_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) +{
- lpc32xx_nand_xfer(mtd, buf, len, 1);
+} +#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); } +#endif
static uint8_t lpc32xx_read_byte(struct mtd_info *mtd) { return readl(&lpc32xx_nand_slc_regs->data); }
+#if defined(CONFIG_DMA_LPC32XX) +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_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) { while (len-- > 0) writel(*buf++, &lpc32xx_nand_slc_regs->data); } +#endif
Ok.
static void lpc32xx_write_byte(struct mtd_info *mtd, uint8_t byte) { @@ -137,10 +460,61 @@ 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)
- int ret;
- /* Acquire a channel for our use */
- ret = lpc32xx_dma_get_channel();
- if (unlikely(ret < 0)) {
pr_info("Unable to get free DMA channel for NAND transfers\n");return -1;- }
- dmachan = (unsigned int)ret;
+#endif
lpc32xx_chip->cmd_ctrl = lpc32xx_nand_cmd_ctrl; lpc32xx_chip->dev_ready = lpc32xx_nand_dev_ready;
/*
* 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;
+#if defined(CONFIG_DMA_LPC32XX)
- /*
* Hardware ECC calculation is supported when* DMA driver is selected.*/- lpc32xx_chip->ecc.mode = NAND_ECC_HW;
- 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;
- /*
* ECC correction is done by page when using the DMA controller* scatter/gather mode through linked list;* refer to UM10326, "LPC32x0 and LPC32x0/01 User manual" - Rev. 3* section 9.7 and tables 173 notes for details.** The error correction is done on each page and process in multiple* steps of 256 bytes inside the driver.** The total ECC size and bytes for a page are used;* NAND base code (HW ECC) will call the read / write buffer functions* using the total ECC size and bytes for a page as a single step.*/- lpc32xx_chip->ecc.size = TOTAL_ECCSIZE;
- lpc32xx_chip->ecc.bytes = TOTAL_ECCBYTES;
I still don't quite understand, why TOTAL_ECCSIZE and TOTAL_ECCBYTES are not equal to CONFIG_SYS_NAND_ECCSIZE and CONFIG_SYS_NAND_ECCBYTES correspondingly. IOW why total values are used here?
This indicates a bug.
+#else
- /*
- Hardware ECC calculation is not supported by the driver,
- because it requires DMA support, see LPC32x0 User Manual,
- note after SLC_ECC register description (UM10326, p.198)
@@ -153,16 +527,16 @@ int board_nand_init(struct nand_chip *lpc32xx_chip) * is strictly 32-bit aligned. */ lpc32xx_chip->read_buf = lpc32xx_read_buf;
lpc32xx_chip->read_byte = lpc32xx_read_byte; lpc32xx_chip->write_buf = lpc32xx_write_buf;
lpc32xx_chip->write_byte = lpc32xx_write_byte;
/*
- Use default ECC layout, but these values are predefined
- for both small and large page NAND flash devices.
*/
lpc32xx_chip->ecc.size = 256;
lpc32xx_chip->ecc.bytes = 3;
- lpc32xx_chip->ecc.size = CONFIG_SYS_NAND_ECCSIZE;
- lpc32xx_chip->ecc.bytes = CONFIG_SYS_NAND_ECCBYTES;
+#endif
- lpc32xx_chip->ecc.strength = 1;
#if defined(CONFIG_SYS_NAND_USE_FLASH_BBT)
-- With best wishes, Vladimir
-----Original Message----- From: Vladimir Zapolskiy [mailto:vz@mleia.com]
Hi Sylvain,
On 10.08.2015 15:16, 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
[...]
diff --git a/drivers/mtd/nand/lpc32xx_nand_slc.c b/drivers/mtd/nand/lpc32xx_nand_slc.c index 719a74d..2a32264 100644 --- a/drivers/mtd/nand/lpc32xx_nand_slc.c +++ b/drivers/mtd/nand/lpc32xx_nand_slc.c @@ -3,15 +3,48 @@
[...]
+#define CONFIG_SYS_NAND_ECCBYTES 3 +#define CONFIG_SYS_NAND_ECCSIZE 256
These defines are OK, but see my bottom comment below related to this subject.
Also it's worth to mention that these defines are conflicting with the same defines from update to my board: http://lists.denx.de/pipermail/u-boot/2015-July/219251.html -- I don't understand why my changes are still not present in U-boot/master e.g. to catch this kind of problems.
So, CONFIG_SYS_NAND_ECCSIZE, CONFIG_SYS_NAND_ECCBYTES and CONFIG_SYS_NAND_OOBSIZE are used outside of the driver code (to be precise they are used in drivers/mtd/nand/nand_spl_simple.c), and therefore this is not the right place to define them IMHO.
I missed the change; I applied only patch 2/4 of your series (NAND SLC driver).
I can add an #if !defined() statement for both of them, as it was in previous version of the patch. This way, you don't need to define them in your board config if you are not using the SPL.
struct lpc32xx_nand_slc_regs { u32 data; @@ -33,11 +66,18 @@ struct lpc32xx_nand_slc_regs {
[...]
+#if defined(CONFIG_DMA_LPC32XX) +/* Total ECC bytes and size; refer to board_nand_init() for details. */ +#define ECCSTEPS (CONFIG_SYS_NAND_PAGE_SIZE / CONFIG_SYS_NAND_ECCSIZE) +#define TOTAL_ECCBYTES (CONFIG_SYS_NAND_ECCBYTES * ECCSTEPS) +#define TOTAL_ECCSIZE (CONFIG_SYS_NAND_ECCSIZE * ECCSTEPS)
See my comment below regarding these two defines.
[...]
- /*
- ECC correction is done by page when using the DMA controller
- scatter/gather mode through linked list;
- refer to UM10326, "LPC32x0 and LPC32x0/01 User manual" - Rev. 3
- section 9.7 and tables 173 notes for details.
- The error correction is done on each page and process in multiple
- steps of 256 bytes inside the driver.
- The total ECC size and bytes for a page are used;
- NAND base code (HW ECC) will call the read / write buffer functions
- using the total ECC size and bytes for a page as a single step.
- */
- lpc32xx_chip->ecc.size = TOTAL_ECCSIZE;
- lpc32xx_chip->ecc.bytes = TOTAL_ECCBYTES;
I still don't quite understand, why TOTAL_ECCSIZE and TOTAL_ECCBYTES are not equal to CONFIG_SYS_NAND_ECCSIZE and CONFIG_SYS_NAND_ECCBYTES correspondingly. IOW why total values are used here?
This indicates a bug.
The ECC correction is done by page when using the DMA controller scatter/gather mode through linked list; the DMA is configured to process 2 (small) or 8 (large) pages of 256 bytes and the ECC read without any software intervention, resulting in a single DMA transfer.
As per the "LPC32x0 and LPC32x0/01 User manual" table 173 notes and section 9.7, the NAND SLC & DMA allowed single DMA transaction of a page size (512 or 2048) that manage the ECC within that single transaction, resulting in an ECCSIZE of 512 (small page) or 2048 (large page) size.
[...]
-- With best wishes, Vladimir
________________________________
This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you are not the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any action in respect of any information contained in it. If you have received this e-mail in error, please notify the sender immediately by e-mail and immediately destroy this e-mail and its attachments.
On 10.08.2015 21:40, LEMIEUX, SYLVAIN wrote:
-----Original Message----- From: Vladimir Zapolskiy [mailto:vz@mleia.com]
Hi Sylvain,
On 10.08.2015 15:16, 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
[...]
diff --git a/drivers/mtd/nand/lpc32xx_nand_slc.c b/drivers/mtd/nand/lpc32xx_nand_slc.c index 719a74d..2a32264 100644 --- a/drivers/mtd/nand/lpc32xx_nand_slc.c +++ b/drivers/mtd/nand/lpc32xx_nand_slc.c @@ -3,15 +3,48 @@
[...]
+#define CONFIG_SYS_NAND_ECCBYTES 3 +#define CONFIG_SYS_NAND_ECCSIZE 256
These defines are OK, but see my bottom comment below related to this subject.
Also it's worth to mention that these defines are conflicting with the same defines from update to my board: http://lists.denx.de/pipermail/u-boot/2015-July/219251.html -- I don't understand why my changes are still not present in U-boot/master e.g. to catch this kind of problems.
So, CONFIG_SYS_NAND_ECCSIZE, CONFIG_SYS_NAND_ECCBYTES and CONFIG_SYS_NAND_OOBSIZE are used outside of the driver code (to be precise they are used in drivers/mtd/nand/nand_spl_simple.c), and therefore this is not the right place to define them IMHO.
I missed the change; I applied only patch 2/4 of your series (NAND SLC driver).
It is not your fault, please understand that the maintainers are very busy, some of the changes are not merged timely.
I can add an #if !defined() statement for both of them, as it was in previous version of the patch. This way, you don't need to define them in your board config if you are not using the SPL.
This is ugly. It seems that an update to arch-lpc32xx/config.h is required here, I'll send a change tomorrow.
struct lpc32xx_nand_slc_regs { u32 data; @@ -33,11 +66,18 @@ struct lpc32xx_nand_slc_regs {
[...]
+#if defined(CONFIG_DMA_LPC32XX) +/* Total ECC bytes and size; refer to board_nand_init() for details. */ +#define ECCSTEPS (CONFIG_SYS_NAND_PAGE_SIZE / CONFIG_SYS_NAND_ECCSIZE) +#define TOTAL_ECCBYTES (CONFIG_SYS_NAND_ECCBYTES * ECCSTEPS) +#define TOTAL_ECCSIZE (CONFIG_SYS_NAND_ECCSIZE * ECCSTEPS)
See my comment below regarding these two defines.
[...]
- /*
- ECC correction is done by page when using the DMA controller
- scatter/gather mode through linked list;
- refer to UM10326, "LPC32x0 and LPC32x0/01 User manual" - Rev. 3
- section 9.7 and tables 173 notes for details.
- The error correction is done on each page and process in multiple
- steps of 256 bytes inside the driver.
- The total ECC size and bytes for a page are used;
- NAND base code (HW ECC) will call the read / write buffer functions
- using the total ECC size and bytes for a page as a single step.
- */
- lpc32xx_chip->ecc.size = TOTAL_ECCSIZE;
- lpc32xx_chip->ecc.bytes = TOTAL_ECCBYTES;
I still don't quite understand, why TOTAL_ECCSIZE and TOTAL_ECCBYTES are not equal to CONFIG_SYS_NAND_ECCSIZE and CONFIG_SYS_NAND_ECCBYTES correspondingly. IOW why total values are used here?
This indicates a bug.
The ECC correction is done by page when using the DMA controller scatter/gather mode through linked list; the DMA is configured to process 2 (small) or 8 (large) pages of 256 bytes and the ECC read without any software intervention, resulting in a single DMA transfer.
As per the "LPC32x0 and LPC32x0/01 User manual" table 173 notes and section 9.7, the NAND SLC & DMA allowed single DMA transaction of a page size (512 or 2048) that manage the ECC within that single transaction, resulting in an ECCSIZE of 512 (small page) or 2048 (large page) size.
Here we are talking about ecc.size and ecc.bytes. I do believe that the change works correctly due to two compensating issues -- one is misusage of ecc.size and ecc.bytes and another one is exploitation of this misusage. Both must be corrected.
If you open include/linux/mtd/nand.h you may note the following description:
struct nand_ecc_ctrl - Control structure for ECC ... @bytes: ECC bytes per step @size: data bytes per ECC step ...
Per ECC step, not total.
Please fix it.
-- With best wishes, Vladimir
participants (3)
-
LEMIEUX, SYLVAIN -
slemieux.tycoï¼ gmail.com -
Vladimir Zapolskiy