[U-Boot-Users] [PATCH 2/7 v3] NAND_CMD_READOOB is not supported by all chips, read OOB with the page instead

[Guennadi Liakhovetski]

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 at denx.de