[U-Boot] [PATCH RFC] NAND: Improve read performance from Large Page NAND devices

[Nick Thompson]

On 01/12/09 00:55, Scott Wood wrote:
> Nick Thompson wrote:
>> Improve read performance from Large Page NAND devices.
>>
>> This patch employs the following concepts to produce a ~37% improvement in
>> oob_first read speed (on a 300MHz ARM9). The time for a mid-buffer 2k page
>> read is now 260us, 7.88MB/s (was 357us, 5.74MB/s). oob_first is probably
>> the best case improvement.
>>
>> Provides a new config option to allow building for large page devices only.
>> reducing code size by ~800 bytes. [CONFIG_SYS_NAND_NO_SMALL_PAGE]
>> This almost exactly compensates for the code increase due to other changes.
> 
> Could we make it more orthogonal?  I.e. CONFIG_NAND_512B_PAGE, 
> CONFIG_NAND_2K_PAGE, CONFIG_NAND_4K_PAGE?  As is, it does nothing to 
> keep things from growing for small-page-only boards.

I guess that would be possible. The proposed usage is to determine
if the incompatible small page command set is supported as well as
the ONFI large page command set. So it excludes nand_command, which
should now have an unused attribute, and optimises away conditionals
in many other functions.

It's not currently there to decided what pages sizes are supported.

I could put in a CONFIG_NAND_NO_LARGE_PAGE as well, but since small
page devices are EOL now I was only looking to make it easier to
remove legacy code (a bit like the Museum IDs in nand_ids.c).

> 
> As it would determine what support is present rather than what the 
> hardware actually is, I don't think it would go in CONFIG_SYS.

Right, that's fine.

>> +	/* The chip might be ready by now, don't lose anymore time */
>> +	if (this->dev_ready) {
>> +		if (this->dev_ready(mtd))
>> +			goto ready;
>> +	} else {
>> +		if (this->read_byte(mtd) & NAND_STATUS_READY)
>> +			goto ready;
>> +	}
> 
> Does it really take a noticeable amount of time to do reset_timer() and 
> get_timer() once?

On ARM the timer functions use divides to adjust the timer values.
it takes several microseconds longer to spot the ready status when
the chip is in fact already ready. It's not major, but it is easily
measurable.

> 
>> + * Wait for cache ready after read request.
>> + *
>> + * Returns to read state before returning.
>> + *
>> + * @mtd:	mtd info structure
>> + * @chip:	nand chip info structure
>> + */
>> +static int nand_wait_cache_load(struct mtd_info *mtd, struct nand_chip *chip)
>> +{
>> +	int state = nand_wait(mtd, chip);
>> +	chip->cmd_ctrl(mtd, NAND_CMD_READSTART, NAND_CLE | NAND_CTRL_CLE |
>> +						NAND_CTRL_CHANGE);
> 
> NAND_CTRL_CLE includes NAND_CLE.

Yes, will fix.

> 
> Why nand_wait() before READSTART?  The existing nand_command_lp() 
> doesn't do this AFAICT.

In fact I'm only after the functionality in nand_wait. nand_wait always
sends a "read status" command. To get out of read status state you have
to send a new command and, if waiting for a read to complete, the obvious
command is read start as this puts the chip back into read state.

The full sequence is "read", "read start", (chip goes busy), "read
status", (polling status reads...), (chip goes ready), "read start",
(data reads...).

Currently nand_command_lp always uses a the "ready busy" read function, or
a timer if the read busy pin is not accessible. Using a timer is out of the
question here, since (we are hoping) significant time has already elapsed.

> This change will break drivers that support large page and use the 
> default read_page functions, but do not implement cmd_ctrl (they replace 
> cmdfunc instead).  This includes fsl_elbc_nand, mxc_nand, and 
> mpc5121_nfc.  While I'd like to move them to implementing their own 
> read_page-type functions instead of cmdfunc, is there any way to make it 
> a smoother transition?

Yes, as it stands they would need modifying simultaneously and I have no
way to test such a change myself. The only required change in cmdfunc is
not to wait after a read0 request. You maybe in a better position to decide
if this has wider repercussions, but I will take a look at the above
drivers as well. [This is the main reason I made this an RFC].

> 
>> +	chip->cmd_ctrl(mtd, NAND_CMD_NONE, NAND_CLE | NAND_CTRL_CHANGE);
> 
> Shouldn't this be NAND_NCE | NAND_CTRL_CHANGE?  Don't we want to drop 
> CLE here?

You are right, will fix.

> 
>> +
>> +	if (nand_next_page_req(*rstate))
>> +		chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page+1);
> 
> Spaces around binary operators.

Okay.

> 
>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>> index cb7c19a..85b7c3c 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -269,17 +269,20 @@ struct nand_ecc_ctrl {
>>  					   uint8_t *calc_ecc);
>>  	int			(*read_page_raw)(struct mtd_info *mtd,
>>  						 struct nand_chip *chip,
>> -						 uint8_t *buf, int page);
>> +						 uint8_t *buf, int page,
>> +						 uint32_t *rstate);
>>  	void			(*write_page_raw)(struct mtd_info *mtd,
>>  						  struct nand_chip *chip,
>>  						  const uint8_t *buf);
>>  	int			(*read_page)(struct mtd_info *mtd,
>>  					     struct nand_chip *chip,
>> -					     uint8_t *buf, int page);
>> +					     uint8_t *buf, int page,
>> +					     uint32_t *rstate);
>>  	int			(*read_subpage)(struct mtd_info *mtd,
>>  					     struct nand_chip *chip,
>>  					     uint32_t offs, uint32_t len,
>> -					     uint8_t *buf);
>> +					     uint8_t *buf, int page,
>> +					     uint32_t *rstate);
> 
> Does rstate really need to be a parameter, or could it be part of the 
> mtd struct?  I'd really like nand_do_read_ops() to not have to know 
> about any of this, and have it be internal to the read_page functions -- 
> other than perhaps clearing the value on exit, or some other way to let 
> read_page know that its context has changed.
> 
> If we need to communicate to the read_page function whether this is the 
> last page, then that can be a separate flag that isn't tied up with what 
> the hardware is capable of, or whether a boundary is being spanned.

Only do_read_ops knows if this is the last page read, so that does need
to be passed to read page. I didn't want to add such low level stuff into
mtd struct as it is very transitory information and bloats the struct.

The page read function also needs to know if this is the first page read.
Currently rstate's main use is this first (INIT) and/or last (NO_REQ)
state.

The page read functions can handle the boundaries and autoinc status, but
only at the expense of repeating the same tests in each read page function.
I'm okay with that, if its acceptable. It will make the code slightly
larger, but I don't really like the way I'm forcing a "last page" state to
avoid sending reads in the autoinc case as it is masking the true purpose
of the flag.

Thanks,
Nick.