On Mon, 4 Aug 2008, Scott Wood wrote:
> On Mon, Aug 04, 2008 at 02:45:33PM +0200, Guennadi Liakhovetski wrote:
> > I _think_ this should work with all NAND chips. Otherwise we might have to
> > introduce a configuration variable.
>
> Which small-page NAND chips can't handle READOOB? On large page devices,
> nand_command changes it to READ0.
It's a large-page device. And, as far as I understand the datasheet, to
read data at arbitrary offset in a page, you first have to issue a READ
PAGE (READ0) for _the_ _whole_ page, then you can use RANDOM DATA READ to
read arbitrary data within this page. Whereas, the driver attempts to use
READ0 to read the bad-block marker directly, which doesn't work with this
chip. At least this is my understanding.
> That said, doing it all at once could result in smaller, faster, and
> simpler code.
>
> > @@ -150,8 +135,6 @@ static int nand_read_page(struct mtd_info *mtd, int
> > block, int page, uchar *dst)
> > u_char *ecc_code;
> > u_char *oob_data;
> > int i;
> > - int eccsize = CFG_NAND_ECCSIZE;
> > - int eccbytes = CFG_NAND_ECCBYTES;
>
> Any particular reason for this change? It's more readable as is, IMHO.
Acually, it was to improve readability:-) First, this way you can easier
grep. Secondly, when I see an assignment to a _variable_, I expect, that
this variable's value can indeed _vary_. So, it makes extra work looking
through the code and verifying what other values this variable takes.
Thus, at the very least I would add "const" to the definition. And, I do
think using constants directly makes it clearer...
> > @@ -195,6 +180,7 @@ static int nand_load(struct mtd_info *mtd, int offs,
> > int uboot_size, uchar *dst)
> > int block;
> > int blockcopy_count;
> > int page;
> > + unsigned read = 0;
>
> "unsigned int", please.
Ok...
> > + int badblock = 0;
> > + for (page = 0; page < CFG_NAND_PAGE_COUNT; page++) {
> > + nand_read_page(mtd, block, page, dst);
> > + if ((!page
> > +#ifdef CFG_NAND_BBT_2NDPAGE
> > + || page == 1
> > +#endif
>
> Please use page == 0 rather than !page when checking for an actual value
> of zero as opposed to a zero that means "not valid" or similar.
Ok...
> > + ) && dst[CFG_NAND_PAGE_SIZE] != 0xff) {
> > + badblock = 1;
> > + break;
> > }
> > + /* Overwrite skipped pages */
> > + if (read >= offs)
> > + dst += CFG_NAND_PAGE_SIZE;
> > + read += CFG_NAND_PAGE_SIZE;
> > + }
>
> I don't follow the logic here -- you're discarding a number of good
> blocks equal to the offset? That might make sense if we were starting at
> block zero, and defining the offset as not including any bad blocks
> before the image -- but the first block we read is at the start of the
> image, not the start of flash.
Right, that's a bug. Hope, it's fixed now.
> > @@ -241,12 +239,18 @@ void nand_boot(void)
> > nand_chip.dev_ready = NULL; /* preset to NULL */
> > board_nand_init(&nand_chip);
> >
> > + if (nand_chip.select_chip)
> > + nand_chip.select_chip(&nand_info, 0);
> > +
> > /*
> > * Load U-Boot image from NAND into RAM
> > */
> > ret = nand_load(&nand_info, CFG_NAND_U_BOOT_OFFS, CFG_NAND_U_BOOT_SIZE,
> > (uchar *)CFG_NAND_U_BOOT_DST);
> >
> > + if (nand_chip.select_chip)
> > + nand_chip.select_chip(&nand_info, -1);
> > +
>
> This seems like an unrelated change, that wasn't described in the
> changelog.
Ok, will describe in the changelog.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: [EMAIL PROTECTED]
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
U-Boot-Users mailing list
U-Boot-Users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users
