Hi Scott,
Le Mon, 9 Mar 2015 18:51:03 -0500, Scott Wood scottwood@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.aribaud@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