Hi Scott, Le Mon, 9 Mar 2015 18:51:03 -0500, Scott Wood <scottw...@freescale.com> a écrit :
> On Thu, 2015-03-05 at 07:46 +0100, Albert ARIBAUD (3ADEV) wrote: > > diff --git a/drivers/mtd/nand/lpc32xx_nand_mlc.c > > b/drivers/mtd/nand/lpc32xx_nand_mlc.c > > new file mode 100644 > > index 0000000..cb23972 > > --- /dev/null > > +++ b/drivers/mtd/nand/lpc32xx_nand_mlc.c > > @@ -0,0 +1,589 @@ > > +/* > > + * LPC32xx MLC NAND flash controller driver > > + * > > + * (C) Copyright 2014 3ADEV <http://3adev.com> > > + * Written by Albert ARIBAUD <albert.arib...@3adev.fr> > > + * > > + * SPDX-License-Identifier: GPL-2.0+ > > + * > > + * NOTE: > > + * > > + * The MLC NAND flash controller provides hardware Reed-Solomon ECC > > + * covering in- and out-of-band data together. Therefore, in- and out- > > + * of-band data must be written together in order to have a valid ECC. > > + * > > + * Consequently, pages with meaningful in-band data are written with > > + * blank (all-ones) out-of-band data and a valid ECC, and any later > > + * out-of-band data write will void the ECC. > > + * > > + * Therefore, code which reads such late-written out-of-band data > > + * should not rely on the ECC validity. > > + */ > > When are you expecting later out-of-band writes? I am not actually expecting OOB writes to happen, but they might do regardless of my expectations (bad block marking, for instance?); However, I do know the once an OOB write happens, OOB as well as in-band reads will fail ECC, so I am just warning whoever it might happen to. > > +static struct lpc32xx_nand_mlc_registers *lpc32xx_nand_mlc_registers > > + = (struct lpc32xx_nand_mlc_registers *)MLC_NAND_BASE; > > __iomem > > > + unsigned int ctrl) > > +{ > > + if (cmd == NAND_CMD_NONE) > > + return; > > + > > + if (ctrl & NAND_CLE) > > + writeb(cmd & 0Xff, &lpc32xx_nand_mlc_registers->cmd); > > + else if (ctrl & NAND_ALE) > > + writeb(cmd & 0Xff, &lpc32xx_nand_mlc_registers->addr); > > + else > > + return; > > +} > > That last return is superfluous. Will fix in v4. > > +/** > > + * ECC layout -- this is needed whatever ECC mode we are using. > > + * In a 2KB (4*512B) page, R/S codes occupy 40 (4*10) bytes. > > + * To make U-Boot's life easier, we pack 'useable' OOB at the > > + * front and R/S ECC at the back. > > + */ > > + > > +static struct nand_ecclayout lpc32xx_largepage_ecclayout = { > > + .eccbytes = 40, > > + .eccpos = {24, 25, 26, 27, 28, 29, 30, 31, 32, 33, > > + 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, > > + 44, 45, 46, 47, 48, 48, 50, 51, 52, 53, > > + 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, > > + }, > > + .oobfree = { > > + { > > + .offset = 0, > > + .length = 24 > > + }, > > + } > > +}; > > Is offset zero really free? Where does the bad block marker go? My bad -- I had not realized that the ECC layout struct had to factor out the bad block marker location. Since this is a large block NAND, that should be... .offset = 2, .length = 22 ... Right? Will fix in v4. > > + for (i = 0; i < 4; i++) { > > + /* start data input */ > > + oob[6*i] = ((0xfe << i) & 0xff); > > + oob[6*i+5] = ((0x7f >> i) & 0xff); > > + chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x200+0x210*i, page); > > + /* copy 6 non-ECC out-of-band bytes directly into NAND */ > > + memcpy(lpc32xx_nand_mlc_registers->data, oob+6*i, 6); > > Lots of magic numbers... Will turn these into symbols in v4. > > + /* wait for NAND to return to ready state */ > > + while (!(readl(&lpc32xx_nand_mlc_registers->isr) > > + & ISR_NAND_READY)) > > + ; > > Timeout? Will fix in v4. > > + } > > + return 0; > > +} > > + > > +/** > > + * lpc32xx_waitfunc - wait until a command is done > > + * @mtd: MTD device structure > > + * @chip: NAND chip structure > > + * > > + * Wait for controller and FLASH to both be ready. > > + */ > > + > > +static int lpc32xx_waitfunc(struct mtd_info *mtd, struct nand_chip *chip) > > +{ > > + /* FIXME: put a time-out here */ > > + int status; > > + /* wait until both controller and NAND are ready */ > > + do { > > + status = readl(&lpc32xx_nand_mlc_registers->isr); > > + } while ((status & (ISR_CONTROLLER_READY || ISR_NAND_READY)) > > + != (ISR_CONTROLLER_READY || ISR_NAND_READY)); > > Timeout? Etc. Will fix in v4. > > +/* > > + * Initialize the controller > > + */ > > + > > +int board_nand_init(struct nand_chip *nand) > > +{ > > It'd be nice if new NAND drivers used CONFIG_SYS_NAND_SELF_INIT. Will use in v4. > > + > > + /* BBT options: read from last two pages, don't write */ > > + nand->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_LASTBLOCK > > + | NAND_BBT_SCANLASTPAGE | NAND_BBT_SCAN2NDPAGE > > + | NAND_BBT_WRITE; > > Don't write? Obsolete comment. Will fix comment in v4. > > +#define LARGE_PAGE_SIZE 2048 > > + > > +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst) > > +{ > > + unsigned int page = offs / LARGE_PAGE_SIZE; > > + unsigned int left = (size + LARGE_PAGE_SIZE - 1) / LARGE_PAGE_SIZE; > > DIV_ROUND_UP() Will use in v4. > > + while (left) { > > + if (read_single_page(dst, page) >= 0) { > > + dst += LARGE_PAGE_SIZE; > > + page++; > > + left--; > > + } > > + } > > No bad block skipping? Hmm... actually the 'left--' should be just after the 'if' block, otherwise not only will the code not skip a bad block, it will actually loop infinitely trying to read it. Will fix in v4. Thanks for pointing this out! > -Scott Thanks for your review. Cordialement, Albert ARIBAUD 3ADEV _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot