Re: [PATCH] NAND: fix reading/writing OOB for syndrome: next iteration

[Vitaly Wool <vwool@ru.mvista.com>]

Thomas Gleixner wrote:
> *smdcmd ?
>
> s/void nand_.../int nand.../
>   
Agreed.
>   
>> +		chip->cmdfunc(mtd, NAND_CMD_READOOB, offs, page);
>> +		*sndcmd = 0;
>> +	}
>> +	chip->read_buf(mtd, buf, length);
>> +}
>> +static void (*nand_read_oob_swecc) (struct mtd_info *mtd,
>> +				    struct nand_chip *chip, uint8_t *buf,
>> +				    int offs, int length, int page,
>> +				    int *sndcmd) =
>> +    &nand_read_oob_raw;
>>     
>
> What is this for ? We can put nand_read_oob_raw into the chip->ecc.xxx
> pointer directly
>   
Agreed.
>> +static void nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
>> +				   uint8_t *buf, int offs, int length,
>> +				   int page, int *sndcmd)
>> +{
>> +	int portion = chip->ecc.bytes + chip->ecc.prepad + chip->ecc.postpad;
>> +	uint8_t *bufpoi = buf;
>> +	int i, toread;
>> +
>> +	for (i = 0; i < chip->ecc.steps; i++) {
>> +		if (offs) {
>> +			while (portion < offs) {
>> +				offs -= portion;
>> +				i++;
>> +			}
>> +		}
>>     
>
> That breaks, if portion is < oobsize / ecc.steps and the offset is in
> the last area. I gave you a hint for the correct code before.
>   
Yup, thanks for pointing that out.
>   
>> +		chip->cmdfunc(mtd, NAND_CMD_READ0,
>> +				(i + 1) * chip->ecc.size + i * portion + offs,
>> +				page);
>>     
>
> We should use the random in command for the second read, as it is faster
>   
Ok
>   
>> +		toread = min_t(int, length, portion - offs);
>> +		chip->read_buf(mtd, bufpoi, toread);
>> +		bufpoi += toread;
>> +		length -= toread;
>> +		offs = 0;
>> +	}
>> +	if (length > 0)
>> +		chip->read_buf(mtd, bufpoi, length);
>>     
>
> When offset is far enough off, you need to reposition the flash
> pointer. 
>   
Depends on the previous code

>> +	chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
>> +	status = chip->waitfunc(mtd, chip, FL_WRITING);
>> +
>> +	/* See if device thinks it succeeded */
>> +	if (status & NAND_STATUS_FAIL)
>> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Failed write, page 0x%08x\n",
>> +		      __FUNCTION__, page);
>> +
>> +	return status & NAND_STATUS_FAIL ? -EIO : 0;
>>     
>
> Why is the pageprog command and the status wait in every implementation
> and not in the calling code ?
>   
It's because the READ/READOOB and SEQIN commands are to be in every 
implementation, so it's keeping the integrity.
Having SEQIN in every implementation and PAGEPROG elsewhere would have 
been misleading IMHO.

Vitaly