Re: [PATCH v5 06/23] mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS

[Boris Brezillon]

On Wed,  7 Jun 2017 20:52:15 +0900
Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:

> The denali_cmdfunc() actually does nothing valuable for
> NAND_CMD_{PAGEPROG,READ0,SEQIN}.
> 
> For NAND_CMD_{READ0,SEQIN}, it copies "page" to "denali->page", then
> denali_read_page() and denali_read_page_raw() compare them to check
> if the NAND framework called the callbacks in correct order.
> (Inconsistently, this check is missing from the denali_write_page()
> and denali_write_page_raw().)
> 
> The framework is widely tested by many drivers, so this kind of
> sanity check is unneeded.  The Denali controller is equipped with
> high level interface for read/write, so let's skip unneeded call
> of cmdfunc().

I recently changed the semantic of ecc->write_page[_raw]() when
NAND_ECC_CUSTOM_PAGE_ACCESS is set [1]. I'm not sure your driver waits
for the program command to finish.
I think you should wait for INTR_STATUS__PROGRAM_COMP instead of
INTR_STATUS__DMA_CMD_COMP in write_page() [2], as is done in
write_oob_data().

Note that, even though you listen to INTR_STATUS__PROGRAM_FAIL, you
never test the value of irq_status when it != 0, which means you don't
detect PROG failures.

[1]http://git.infradead.org/l2-mtd.git/commit/41145649f4acb30249b636b945053db50c9331c5
[2]http://elixir.free-electrons.com/linux/latest/source/drivers/mtd/nand/denali.c#L1029

> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> ---
> 
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
>   - Newly added
> 
>  drivers/mtd/nand/denali.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 991924b9ae2c..1897fe238290 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -998,7 +998,7 @@ static void denali_setup_dma(struct denali_nand_info *denali, int op)
>   * configuration details.
>   */
>  static int write_page(struct mtd_info *mtd, struct nand_chip *chip,
> -			const uint8_t *buf, bool raw_xfer)
> +			const uint8_t *buf, int page, bool raw_xfer)
>  {
>  	struct denali_nand_info *denali = mtd_to_denali(mtd);
>  	dma_addr_t addr = denali->buf.dma_buf;
> @@ -1006,6 +1006,8 @@ static int write_page(struct mtd_info *mtd, struct nand_chip *chip,
>  	uint32_t irq_status;
>  	uint32_t irq_mask = INTR__DMA_CMD_COMP | INTR__PROGRAM_FAIL;
>  
> +	denali->page = page;
> +
>  	/*
>  	 * if it is a raw xfer, we want to disable ecc and send the spare area.
>  	 * !raw_xfer - enable ecc
> @@ -1059,7 +1061,7 @@ static int denali_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>  	 * for regular page writes, we let HW handle all the ECC
>  	 * data written to the device.
>  	 */
> -	return write_page(mtd, chip, buf, false);
> +	return write_page(mtd, chip, buf, page, false);
>  }
>  
>  /*
> @@ -1075,7 +1077,7 @@ static int denali_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
>  	 * for raw page writes, we want to disable ECC and simply write
>  	 * whatever data is in the buffer.
>  	 */
> -	return write_page(mtd, chip, buf, true);
> +	return write_page(mtd, chip, buf, page, true);
>  }
>  
>  static int denali_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> @@ -1105,12 +1107,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>  	unsigned long uncor_ecc_flags = 0;
>  	int stat = 0;
>  
> -	if (page != denali->page) {
> -		dev_err(denali->dev,
> -			"IN %s: page %d is not equal to denali->page %d",
> -			__func__, page, denali->page);
> -		BUG();
> -	}
> +	denali->page = page;
>  
>  	setup_ecc_for_xfer(denali, true, false);
>  
> @@ -1154,12 +1151,7 @@ static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
>  	size_t size = mtd->writesize + mtd->oobsize;
>  	uint32_t irq_mask = INTR__DMA_CMD_COMP;
>  
> -	if (page != denali->page) {
> -		dev_err(denali->dev,
> -			"IN %s: page %d is not equal to denali->page %d",
> -			__func__, page, denali->page);
> -		BUG();
> -	}
> +	denali->page = page;
>  
>  	setup_ecc_for_xfer(denali, false, true);
>  	denali_enable_dma(denali, true);
> @@ -1238,8 +1230,6 @@ static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col,
>  	int i;
>  
>  	switch (cmd) {
> -	case NAND_CMD_PAGEPROG:
> -		break;
>  	case NAND_CMD_STATUS:
>  		read_status(denali);
>  		break;
> @@ -1259,10 +1249,6 @@ static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col,
>  			write_byte_to_buf(denali, id);
>  		}
>  		break;
> -	case NAND_CMD_READ0:
> -	case NAND_CMD_SEQIN:
> -		denali->page = page;
> -		break;
>  	case NAND_CMD_RESET:
>  		reset_bank(denali);
>  		break;
> @@ -1603,6 +1589,7 @@ int denali_init(struct denali_nand_info *denali)
>  
>  	mtd_set_ooblayout(mtd, &denali_ooblayout_ops);
>  
> +	chip->ecc.options |= NAND_ECC_CUSTOM_PAGE_ACCESS;
>  	chip->ecc.read_page = denali_read_page;
>  	chip->ecc.read_page_raw = denali_read_page_raw;
>  	chip->ecc.write_page = denali_write_page;