Hi Scott,
Le Mon, 16 Mar 2015 16:30:29 -0500, Scott Wood
<scottw...@freescale.com> a écrit :
> 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)?
The udelay(1) in the loop is ok with me -- I'll put it in v6.
> > > > +#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.
Thanks -- I'll dig into the BBT code to see how it's handled in my
target and put a fix in v6 based on that.
> -Scott
Cordialement,
Albert ARIBAUD
3ADEV
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
