Re: [RFC v2] Special handling for NAND_CMD_PAGEPROG and NAND_CMD_READ0

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Thu, 10 Nov 2016 15:47:34 +0100
Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote:

> Sample code to generate some discussion around having the framework
> *not* send I/O commands for read_page and write_page, when it is
> dealing with "high-level" NFCs that already send the commands
> themselves.
> ---
> diff from v1 to v2:
> - handle NAND_CMD_SEQIN as well
> - implement check_page_access_callbacks
> 
> If this RFC is an acceptable state, we can pick an appropriate
> name for the option, and I'll make a formal submission.
> ---
>  drivers/mtd/nand/nand_base.c  | 27 ++++++++++++++++++++++++---
>  drivers/mtd/nand/tango_nand.c |  7 ++++++-
>  include/linux/mtd/nand.h      |  8 ++++++++
>  3 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 50cdf37cb8e4..f42f79d8d0c4 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1970,7 +1970,8 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  						 __func__, buf);
>  
>  read_retry:
> -			chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
> +			if (!(chip->options & NAND_FOO))
> +				chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
>  
>  			/*
>  			 * Now read the page into the buffer.  Absent an error,
> @@ -2658,7 +2659,8 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>  	else
>  		subpage = 0;
>  
> -	chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
> +	if (!(chip->options & NAND_FOO))
> +		chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
>  
>  	if (unlikely(raw))
>  		status = chip->ecc.write_page_raw(mtd, chip, buf,
> @@ -2681,7 +2683,8 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>  
>  	if (!cached || !NAND_HAS_CACHEPROG(chip)) {
>  
> -		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> +		if (!(chip->options & NAND_FOO))
> +			chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
>  		status = chip->waitfunc(mtd, chip);
>  		/*
>  		 * See if operation failed and additional status checks are
> @@ -4539,6 +4542,21 @@ static bool nand_ecc_strength_good(struct mtd_info *mtd)
>  	return corr >= ds_corr && ecc->strength >= chip->ecc_strength_ds;
>  }
>  
> +static int check_page_access_callbacks(struct nand_chip *chip)

check_*ecc*_page_accessors() ?

> +{
> +	int err = 0;
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +
> +	if (chip->options & NAND_FOO) {
> +		err |= !ecc->read_page || !ecc->write_page;
> +		err |= !ecc->read_page_raw || !ecc->write_page_raw;
> +		err |= !ecc->read_subpage && NAND_HAS_SUBPAGE_READ(chip);
> +		err |= !ecc->write_subpage && NAND_HAS_SUBPAGE_WRITE(chip);
> +	}

Please return a real error code:

	if (err)
		return -EINVAL;

	return 0;

I think I'd prefer something more straightforward (but slightly longer):

	if (!(chip->options & NAND_FOO))
		return 0;

	/*
	 * NAND_FOO flag is set, make sure the NAND controller
	 * driver implements all the page accessors because
	 * default helpers are not suitable when the core does
	 * not send the READ0/PAGEPROG commands.
	 */
	if (!ecc->read_page || !ecc->write_page ||
	    !ecc->read_page_raw || !ecc->write_page_raw ||
	    (NAND_HAS_SUBPAGE_READ(chip) && !ecc->read_subpage) ||
	    (NAND_HAS_SUBPAGE_WRITE(chip) && !ecc->write_subpage))
		return -EINVAL;

	return 0;

> +	return err;
> +}
> +
>  /**
>   * nand_scan_tail - [NAND Interface] Scan for the NAND device
>   * @mtd: MTD device structure
> @@ -4559,6 +4577,9 @@ int nand_scan_tail(struct mtd_info *mtd)
>  		   !(chip->bbt_options & NAND_BBT_USE_FLASH)))
>  		return -EINVAL;
>  
> +	if (WARN_ON(check_page_access_callbacks(chip)))
> +		return -EINVAL;
> +
>  	if (!(chip->options & NAND_OWN_BUFFERS)) {
>  		nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize
>  				+ mtd->oobsize * 3, GFP_KERNEL);
> diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c
> index 74e39a92771c..2c9c0cac8f49 100644
> --- a/drivers/mtd/nand/tango_nand.c
> +++ b/drivers/mtd/nand/tango_nand.c
> @@ -401,13 +401,17 @@ static int raw_write(struct nand_chip *chip, const u8 *buf, const u8 *oob)
>  static int tango_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
>  		uint8_t *buf, int oob_required, int page)
>  {
> +	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
>  	return raw_read(chip, buf, chip->oob_poi);
>  }
>  
>  static int tango_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
>  		const uint8_t *buf, int oob_required, int page)
>  {
> -	return raw_write(chip, buf, chip->oob_poi);
> +	chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0, page);
> +	raw_write(chip, buf, chip->oob_poi);
> +	chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> +	return 0;
>  }
>  
>  static int tango_read_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
> @@ -527,6 +531,7 @@ static int chip_init(struct device *dev, struct device_node *np)
>  	chip->setup_data_interface = tango_set_timings;
>  	chip->options = NAND_USE_BOUNCE_BUFFER
>  		| NAND_NO_SUBPAGE_WRITE
> +		| NAND_FOO

This is an ECC engine related flag, as such it should be set in
ecc->options and defined next to NAND_ECC_GENERIC_ERASED_CHECK and
NAND_ECC_MAXIMIZE.

>  		| NAND_WAIT_TCCS;
>  	chip->controller = &nfc->hw;
>  	tchip->base = nfc->pbus_base + (cs * 256);
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 06d0c9d740f7..6999196d04f3 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -186,6 +186,7 @@ enum nand_ecc_algo {
>  /* Macros to identify the above */
>  #define NAND_HAS_CACHEPROG(chip) ((chip->options & NAND_CACHEPRG))
>  #define NAND_HAS_SUBPAGE_READ(chip) ((chip->options & NAND_SUBPAGE_READ))
> +#define NAND_HAS_SUBPAGE_WRITE(chip) (~chip->options & NAND_NO_SUBPAGE_WRITE)
>  
>  /* Non chip related options */
>  /* This option skips the bbt scan during initialization. */
> @@ -220,6 +221,13 @@ enum nand_ecc_algo {
>   */
>  #define NAND_WAIT_TCCS		0x00200000
>  
> +/*
> + * Controller sends NAND_CMD_PAGEPROG (write_page) and NAND_CMD_READ0 (read_page)
> + * therefore the framework should not send these commands.
> + * TODO: find a real name. Write a better description.
> + */
> +#define NAND_FOO		0x00400000
> +

Okay, we need to find a real name for this flag, otherwise, it looks
good to me.

Regarding the name, I have the following suggestions:

NAND_ECC_SKIP_READ_PROG_CMDS
NAND_ECC_DELEGATE_READ_PROG_CMDS
NAND_ECC_DELEGATE_PAGE_ACCESS
NAND_ECC_CUSTOM_PAGE_ACCESS


>  /* Options set by nand scan */
>  /* Nand scan has allocated controller struct */
>  #define NAND_CONTROLLER_ALLOC	0x80000000