From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Mon, 16 Mar 2015 16:30:29 -0500 Subject: [U-Boot] [PATCH v5 2/8] lpc32xx: mtd: nand: add MLC NAND controller In-Reply-To: <20150314152735.312f6c03.albert.aribaud@3adev.fr> References: <1426233873-12690-1-git-send-email-albert.aribaud@3adev.fr> <1426233873-12690-2-git-send-email-albert.aribaud@3adev.fr> <1426233873-12690-3-git-send-email-albert.aribaud@3adev.fr> <1426283853.27998.11.camel@freescale.com> <20150314152735.312f6c03.albert.aribaud@3adev.fr> Message-ID: <1426541429.27998.61.camel@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Sat, 2015-03-14 at 15:27 +0100, Albert ARIBAUD wrote: > Bonjour Scott, > > Le Fri, 13 Mar 2015 16:57:33 -0500, Scott Wood > a ?crit : > > > On Fri, 2015-03-13 at 09:04 +0100, Albert ARIBAUD (3ADEV) wrote: > > > + /* go through all four small pages */ > > > + for (i = 0; i < 4; i++) { > > > + /* start auto decode (reads 528 NAND bytes) */ > > > + writel(0, &lpc32xx_nand_mlc_registers->ecc_auto_dec_reg); > > > + /* wait for controller to return to ready state */ > > > + timeout = LPC32X_NAND_TIMEOUT; > > > + do { > > > + if (timeout-- == 0) > > > + return -1; > > > + status = readl(&lpc32xx_nand_mlc_registers->isr); > > > + } while (!(status & ISR_CONTROLLER_READY)); > > > > How much time does 10000 reads of this register equate to? Are you sure > > it's enough? Timeouts should generally be in terms of time, not loop > > iterations. > > I followed the examples in several drivers where timeouts are by > iteration. Note that -- while this does not void your point -- I did > not use 10000 but 100000, which at a CPU clock of 208 MHz, and assuming > an optimistic one instruction per cycle and two instructions per loop, > makes the loop last at least 960 us, well over the 600 us which the > NAND takes for any page programming. What if this driver ends up being used on hardware that runs significantly faster than 208 MHz? I could understand if it's hugely space-constrained SPL code (like the ones that have to fit in 4K), but otherwise why not make use of the timekeeping code that exists in U-Boot (either by reading the timer, or by putting udelay(1) in the loop body)? > > > +#define LARGE_PAGE_SIZE 2048 > > > + > > > +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst) > > > +{ > > > + struct lpc32xx_oob oob; > > > + unsigned int page = offs / LARGE_PAGE_SIZE; > > > + unsigned int left = DIV_ROUND_UP(size, LARGE_PAGE_SIZE); > > > + > > > + while (left) { > > > + int res = read_single_page(dst, page, &oob); > > > + page++; > > > + /* if read succeeded, even if fixed by ECC */ > > > + if (res >= 0) { > > > + /* skip bad block */ > > > + if (oob.free[0].free_oob_bytes[0] != 0xff) > > > + continue; > > > + if (oob.free[0].free_oob_bytes[1] != 0xff) > > > + continue; > > > + /* page is good, keep it */ > > > + dst += LARGE_PAGE_SIZE; > > > + left--; > > > + } > > > > You should be checking the designated page(s) of the block, rather than > > the current page, for the bad block markers -- and skipping the entire > > block if it's bad. > > Will fix this -- is there any helper function in the bad block > management code for this? I could not find one, but I'm no NAND expert. I don't know of any such helper -- outside of SPL it's handled via the BBT. fsl_ifc_spl.c is an example that checks for bad block markers, but it's hardcoded to assume the first two pages of a block which is a bit simpler than checking at the end of the block. -Scott