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

[Scott Wood <scottwood@freescale.com>]

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.

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.

>  
> +	/* 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?

> + * 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.

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

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?

> +	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?

> +
> +	if (nand_next_page_req(*rstate))
> +		chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page+1);

Spaces around binary operators.

> 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.

-Scott