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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
