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: office@denx.de