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
> <scottw...@freescale.com> 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
