[PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)

[PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)

[KOBAYASHI Yoshitake]

This patch enables support for Toshiba BENAND.
The current implementation does not support vondor specific command
TOSHIBA_NAND_CMD_ECC_STATUS. I would like to add the command, when
the exec_op() [1] infrastructure is implemented.

[1] https://github.com/bbrezillon/linux/commits/nand/exec_op1

Changelog[v2]:
    Rewrite to adapt the mtd "separate vendor specific code from
    core" cleanup process that based on comments from the following
    discussion.
    http://patchwork.ozlabs.org/patch/767191/

Signed-off-by: KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp>
---
 drivers/mtd/nand/nand_toshiba.c | 100 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/drivers/mtd/nand/nand_toshiba.c b/drivers/mtd/nand/nand_toshiba.c
index 57df857..7a99bbe 100644
--- a/drivers/mtd/nand/nand_toshiba.c
+++ b/drivers/mtd/nand/nand_toshiba.c
@@ -17,6 +17,72 @@
 
 #include <linux/mtd/rawnand.h>
 
+/* ECC Status Read Command for BENAND */
+#define TOSHIBA_NAND_CMD_ECC_STATUS	0x7A
+
+/* Recommended to rewrite for BENAND */
+#define TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED	BIT(3)
+
+static int toshiba_nand_benand_status_chk(struct mtd_info *mtd,
+					  struct nand_chip *chip)
+{
+	unsigned int max_bitflips = 0;
+	u8 status;
+
+	/* Check Read Status */
+	chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
+	status = chip->read_byte(mtd);
+
+	/*
+	 * TOSHIBA_NAND_CMD_ECC_STATUS is vendor specific command.
+	 * We will rewrite this code, after the ->exec_op() infrastructure
+	 * is implemented
+	 * Now, we set max_bitflips mtd->bitflip_threshold.
+	 */
+	if (status & NAND_STATUS_FAIL) {
+		/* uncorrectable */
+		mtd->ecc_stats.failed++;
+	} else if (status & TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED) {
+		/* correctable */
+		max_bitflips = mtd->bitflip_threshold;
+		mtd->ecc_stats.corrected += max_bitflips;
+	}
+
+	return max_bitflips;
+}
+
+static int
+toshiba_nand_read_page_benand(struct mtd_info *mtd,
+			      struct nand_chip *chip, uint8_t *buf,
+			      int oob_required, int page)
+{
+	unsigned int max_bitflips = 0;
+
+	chip->ecc.read_page_raw(mtd, chip, buf, oob_required, page);
+	max_bitflips = toshiba_nand_benand_status_chk(mtd, chip);
+
+	return max_bitflips;
+}
+
+static int
+toshiba_nand_read_subpage_benand(struct mtd_info *mtd,
+				 struct nand_chip *chip, uint32_t data_offs,
+				 uint32_t readlen, uint8_t *bufpoi, int page)
+{
+	uint8_t *p;
+	unsigned int max_bitflips = 0;
+
+	if (data_offs != 0)
+		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_offs, -1);
+
+	p = bufpoi + data_offs;
+	chip->read_buf(mtd, p, readlen);
+
+	max_bitflips = toshiba_nand_benand_status_chk(mtd, chip);
+
+	return max_bitflips;
+}
+
 static void toshiba_nand_decode_id(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
@@ -39,9 +105,43 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
 
 static int toshiba_nand_init(struct nand_chip *chip)
 {
+	struct mtd_info *mtd = nand_to_mtd(chip);
+
 	if (nand_is_slc(chip))
 		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
 
+	if (nand_is_slc(chip) && (chip->id.data[4] & 0x80)) {
+		/* BENAND */
+
+		/*
+		 * We can't disable the internal ECC engine, the user
+		 * has to use on-die ECC, there is no alternative.
+		 */
+		if (chip->ecc.mode != NAND_ECC_ON_DIE) {
+			pr_err("On-die ECC should be selected.\n");
+			return -EINVAL;
+		}
+
+		/*
+		 * On BENAND, all OOB reginon can be used by user (driver).
+		 * The calculated ECC bytes are stored into other isolated
+		 * area which is ubable to access from user.
+		 * This is why chip->ecc.bytes = 0.
+		 */
+		chip->ecc.bytes = 0;
+		chip->ecc.size = 512;
+		chip->ecc.strength = 8;
+		chip->ecc.read_page = toshiba_nand_read_page_benand;
+		chip->ecc.read_subpage = toshiba_nand_read_subpage_benand;
+		chip->ecc.write_page = nand_write_page_raw;
+		chip->ecc.read_page_raw = nand_read_page_raw;
+		chip->ecc.write_page_raw = nand_write_page_raw;
+
+		chip->options |= NAND_SUBPAGE_READ;
+
+		mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
+	}
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)

[Boris Brezillon]

On Thu, 21 Sep 2017 14:32:02 +0900
KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:

> This patch enables support for Toshiba BENAND.
> The current implementation does not support vondor specific command

					       ^ vendor

> TOSHIBA_NAND_CMD_ECC_STATUS. I would like to add the command, when
> the exec_op() [1] infrastructure is implemented.

It's not a good idea to reference a branch that is likely to disappear
in a commit message. Just say that you can't properly support the
TOSHIBA_NAND_CMD_ECC_STATUS operation right and that it might be
addressed in the future.

> 
> [1] https://github.com/bbrezillon/linux/commits/nand/exec_op1
> 
> Changelog[v2]:
>     Rewrite to adapt the mtd "separate vendor specific code from
>     core" cleanup process that based on comments from the following
>     discussion.
>     http://patchwork.ozlabs.org/patch/767191/

I know it's different for each subsystem, but for MTD, we don't put
changelogs in the commit message.

> 
> Signed-off-by: KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp>
> ---

Move the changelog here.

>  drivers/mtd/nand/nand_toshiba.c | 100 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand_toshiba.c b/drivers/mtd/nand/nand_toshiba.c
> index 57df857..7a99bbe 100644
> --- a/drivers/mtd/nand/nand_toshiba.c
> +++ b/drivers/mtd/nand/nand_toshiba.c
> @@ -17,6 +17,72 @@
>  
>  #include <linux/mtd/rawnand.h>
>  
> +/* ECC Status Read Command for BENAND */
> +#define TOSHIBA_NAND_CMD_ECC_STATUS	0x7A
> +
> +/* Recommended to rewrite for BENAND */
> +#define TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED	BIT(3)
> +
> +static int toshiba_nand_benand_status_chk(struct mtd_info *mtd,
> +					  struct nand_chip *chip)
> +{
> +	unsigned int max_bitflips = 0;
> +	u8 status;
> +
> +	/* Check Read Status */
> +	chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> +	status = chip->read_byte(mtd);
> +
> +	/*
> +	 * TOSHIBA_NAND_CMD_ECC_STATUS is vendor specific command.
> +	 * We will rewrite this code, after the ->exec_op() infrastructure
> +	 * is implemented

Missing period at the end of your sentence. And again, I would not name
the future stuff here. Just say that currently you have no way to send
arbitrary sequences of CMD+ADDR+DATA cycles and thus cannot support this
custom TOSHIBA_NAND_CMD_ECC_STATUS operation. 

> +	 * Now, we set max_bitflips mtd->bitflip_threshold.

	   For now,

> +	 */
> +	if (status & NAND_STATUS_FAIL) {
> +		/* uncorrectable */
> +		mtd->ecc_stats.failed++;
> +	} else if (status & TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED) {
> +		/* correctable */
> +		max_bitflips = mtd->bitflip_threshold;
> +		mtd->ecc_stats.corrected += max_bitflips;
> +	}

Is this working correctly when you read more than one ECC chunk? The
ECC step size is 512 bytes and the page is bigger than that, which means
you have more than one ECC chunk per page. What happens to the
NAND_STATUS_FAIL flag if the first chunk is uncorrectable but
following ones are correctable (or do not contain bitflips at all)? 

> +
> +	return max_bitflips;
> +}
> +
> +static int
> +toshiba_nand_read_page_benand(struct mtd_info *mtd,
> +			      struct nand_chip *chip, uint8_t *buf,
> +			      int oob_required, int page)
> +{
> +	unsigned int max_bitflips = 0;
> +
> +	chip->ecc.read_page_raw(mtd, chip, buf, oob_required, page);

Call nand_read_page_raw() directly.

> +	max_bitflips = toshiba_nand_benand_status_chk(mtd, chip);

	return toshiba_nand_benand_status_chk(mtd, chip);

> +
> +	return max_bitflips;
> +}
> +
> +static int
> +toshiba_nand_read_subpage_benand(struct mtd_info *mtd,
> +				 struct nand_chip *chip, uint32_t data_offs,
> +				 uint32_t readlen, uint8_t *bufpoi, int page)
> +{
> +	uint8_t *p;
> +	unsigned int max_bitflips = 0;
> +
> +	if (data_offs != 0)
> +		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_offs, -1);
> +
> +	p = bufpoi + data_offs;
> +	chip->read_buf(mtd, p, readlen);
> +
> +	max_bitflips = toshiba_nand_benand_status_chk(mtd, chip);

	return toshiba_nand_benand_status_chk(mtd, chip);

> +
> +	return max_bitflips;
> +}
> +
>  static void toshiba_nand_decode_id(struct nand_chip *chip)
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
> @@ -39,9 +105,43 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
>  
>  static int toshiba_nand_init(struct nand_chip *chip)
>  {
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +
>  	if (nand_is_slc(chip))
>  		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
>  
> +	if (nand_is_slc(chip) && (chip->id.data[4] & 0x80)) {
> +		/* BENAND */
> +
> +		/*
> +		 * We can't disable the internal ECC engine, the user
> +		 * has to use on-die ECC, there is no alternative.
> +		 */
> +		if (chip->ecc.mode != NAND_ECC_ON_DIE) {
> +			pr_err("On-die ECC should be selected.\n");
> +			return -EINVAL;
> +		}

According to your previous explanation that's not exactly true. Since
ECC bytes are stored in a separate area, the user can decide to use
another mode without trouble. Just skip the BENAND initialization when
mode != NAND_ECC_ON_DIE and we should be good, or am I missing something?

> +
> +		/*
> +		 * On BENAND, all OOB reginon can be used by user (driver).

			      the entire OOB region can be used by the
		   MTD user.

I'd drop the '(driver)' part since it's not really clear what the
driver is. If you're talking about the NAND controller driver then it's
wrong (at least most of the time), the real users of free OOB bytes are
upper layers (like JFFS2).

> +		 * The calculated ECC bytes are stored into other isolated
> +		 * area which is ubable to access from user.

			which is not accessible to users.

> +		 * This is why chip->ecc.bytes = 0.
> +		 */
> +		chip->ecc.bytes = 0;
> +		chip->ecc.size = 512;
> +		chip->ecc.strength = 8;
> +		chip->ecc.read_page = toshiba_nand_read_page_benand;
> +		chip->ecc.read_subpage = toshiba_nand_read_subpage_benand;
> +		chip->ecc.write_page = nand_write_page_raw;
> +		chip->ecc.read_page_raw = nand_read_page_raw;
> +		chip->ecc.write_page_raw = nand_write_page_raw;
> +
> +		chip->options |= NAND_SUBPAGE_READ;
> +
> +		mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);

Can you please move this code block in a separate toshiba_benand_init()
function in order to keep the toshiba_nand_init() small/readable.

> +	}
> +
>  	return 0;
>  }
>  

Re: [PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)

[KOBAYASHI Yoshitake]

On 2017/09/21 16:28, Boris Brezillon wrote:
> On Thu, 21 Sep 2017 14:32:02 +0900
> KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:
> 
>> This patch enables support for Toshiba BENAND.
>> The current implementation does not support vondor specific command
> 
> 					       ^ vendor
> 
>> TOSHIBA_NAND_CMD_ECC_STATUS. I would like to add the command, when
>> the exec_op() [1] infrastructure is implemented.
> 
> It's not a good idea to reference a branch that is likely to disappear
> in a commit message. Just say that you can't properly support the
> TOSHIBA_NAND_CMD_ECC_STATUS operation right and that it might be
> addressed in the future.

Thanks. I'll change the comment.

>> +	 */
>> +	if (status & NAND_STATUS_FAIL) {
>> +		/* uncorrectable */
>> +		mtd->ecc_stats.failed++;
>> +	} else if (status & TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED) {
>> +		/* correctable */
>> +		max_bitflips = mtd->bitflip_threshold;
>> +		mtd->ecc_stats.corrected += max_bitflips;
>> +	}
> 
> Is this working correctly when you read more than one ECC chunk? The
> ECC step size is 512 bytes and the page is bigger than that, which means
> you have more than one ECC chunk per page. What happens to the
> NAND_STATUS_FAIL flag if the first chunk is uncorrectable but
> following ones are correctable (or do not contain bitflips at all)? 

As you mentioned, the ECC step size is 512 byte. But when 0x70 command is used
a simplified ECC status per page is generated.
In case the first chunk is uncorrectable but the following ones are correctable,
the 0x70 command can only check the status of the uncorrectable one.
Each ECC chunk status can be checked by using the 0x7A command.

>> @@ -39,9 +105,43 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
>>  
>>  static int toshiba_nand_init(struct nand_chip *chip)
>>  {
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +
>>  	if (nand_is_slc(chip))
>>  		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
>>  
>> +	if (nand_is_slc(chip) && (chip->id.data[4] & 0x80)) {
>> +		/* BENAND */
>> +
>> +		/*
>> +		 * We can't disable the internal ECC engine, the user
>> +		 * has to use on-die ECC, there is no alternative.
>> +		 */
>> +		if (chip->ecc.mode != NAND_ECC_ON_DIE) {
>> +			pr_err("On-die ECC should be selected.\n");
>> +			return -EINVAL;
>> +		}
> 
> According to your previous explanation that's not exactly true. Since
> ECC bytes are stored in a separate area, the user can decide to use
> another mode without trouble. Just skip the BENAND initialization when
> mode != NAND_ECC_ON_DIE and we should be good, or am I missing something?

I am asking to product department to confirm it.

>> +
>> +		/*
>> +		 * On BENAND, all OOB reginon can be used by user (driver).
> 
> 			      the entire OOB region can be used by the
> 		   MTD user.
> 
> I'd drop the '(driver)' part since it's not really clear what the
> driver is. If you're talking about the NAND controller driver then it's
> wrong (at least most of the time), the real users of free OOB bytes are
> upper layers (like JFFS2).
> 
>> +		 * The calculated ECC bytes are stored into other isolated
>> +		 * area which is ubable to access from user.
> 
> 			which is not accessible to users.
> 
>> +		 * This is why chip->ecc.bytes = 0.
>> +		 */
>> +		chip->ecc.bytes = 0;
>> +		chip->ecc.size = 512;
>> +		chip->ecc.strength = 8;
>> +		chip->ecc.read_page = toshiba_nand_read_page_benand;
>> +		chip->ecc.read_subpage = toshiba_nand_read_subpage_benand;
>> +		chip->ecc.write_page = nand_write_page_raw;
>> +		chip->ecc.read_page_raw = nand_read_page_raw;
>> +		chip->ecc.write_page_raw = nand_write_page_raw;
>> +
>> +		chip->options |= NAND_SUBPAGE_READ;
>> +
>> +		mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
> 
> Can you please move this code block in a separate toshiba_benand_init()
> function in order to keep the toshiba_nand_init() small/readable.

Sure. I will resend.

-- Yoshi

Re: [PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)

[Boris Brezillon]

On Tue, 26 Sep 2017 18:18:04 +0900
KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:

> On 2017/09/21 16:28, Boris Brezillon wrote:
> > On Thu, 21 Sep 2017 14:32:02 +0900
> > KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:
> >   
> >> This patch enables support for Toshiba BENAND.
> >> The current implementation does not support vondor specific command  
> > 
> > 					       ^ vendor
> >   
> >> TOSHIBA_NAND_CMD_ECC_STATUS. I would like to add the command, when
> >> the exec_op() [1] infrastructure is implemented.  
> > 
> > It's not a good idea to reference a branch that is likely to disappear
> > in a commit message. Just say that you can't properly support the
> > TOSHIBA_NAND_CMD_ECC_STATUS operation right and that it might be
> > addressed in the future.  
> 
> Thanks. I'll change the comment.
> 
> >> +	 */
> >> +	if (status & NAND_STATUS_FAIL) {
> >> +		/* uncorrectable */
> >> +		mtd->ecc_stats.failed++;
> >> +	} else if (status & TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED) {
> >> +		/* correctable */
> >> +		max_bitflips = mtd->bitflip_threshold;
> >> +		mtd->ecc_stats.corrected += max_bitflips;
> >> +	}  
> > 
> > Is this working correctly when you read more than one ECC chunk? The
> > ECC step size is 512 bytes and the page is bigger than that, which means
> > you have more than one ECC chunk per page. What happens to the
> > NAND_STATUS_FAIL flag if the first chunk is uncorrectable but
> > following ones are correctable (or do not contain bitflips at all)?   
> 
> As you mentioned, the ECC step size is 512 byte. But when 0x70 command is used
> a simplified ECC status per page is generated.

I'm fine with that as long as uncorrectable errors are detected
correctly and TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED is set as soon as
one of the ECC chunk exposes too many bitflips.

> In case the first chunk is uncorrectable but the following ones are correctable,
> the 0x70 command can only check the status of the uncorrectable one.

Sounds good.

> Each ECC chunk status can be checked by using the 0x7A command.
> 
> >> @@ -39,9 +105,43 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
> >>  
> >>  static int toshiba_nand_init(struct nand_chip *chip)
> >>  {
> >> +	struct mtd_info *mtd = nand_to_mtd(chip);
> >> +
> >>  	if (nand_is_slc(chip))
> >>  		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
> >>  
> >> +	if (nand_is_slc(chip) && (chip->id.data[4] & 0x80)) {
> >> +		/* BENAND */
> >> +
> >> +		/*
> >> +		 * We can't disable the internal ECC engine, the user
> >> +		 * has to use on-die ECC, there is no alternative.
> >> +		 */
> >> +		if (chip->ecc.mode != NAND_ECC_ON_DIE) {
> >> +			pr_err("On-die ECC should be selected.\n");
> >> +			return -EINVAL;
> >> +		}  
> > 
> > According to your previous explanation that's not exactly true. Since
> > ECC bytes are stored in a separate area, the user can decide to use
> > another mode without trouble. Just skip the BENAND initialization when
> > mode != NAND_ECC_ON_DIE and we should be good, or am I missing something?  
> 
> I am asking to product department to confirm it.

I'm almost sure this is the case ;-).

> 
> >> +
> >> +		/*
> >> +		 * On BENAND, all OOB reginon can be used by user (driver).  
> > 
> > 			      the entire OOB region can be used by the
> > 		   MTD user.
> > 
> > I'd drop the '(driver)' part since it's not really clear what the
> > driver is. If you're talking about the NAND controller driver then it's
> > wrong (at least most of the time), the real users of free OOB bytes are
> > upper layers (like JFFS2).
> >   
> >> +		 * The calculated ECC bytes are stored into other isolated
> >> +		 * area which is ubable to access from user.  
> > 
> > 			which is not accessible to users.
> >   
> >> +		 * This is why chip->ecc.bytes = 0.
> >> +		 */
> >> +		chip->ecc.bytes = 0;
> >> +		chip->ecc.size = 512;
> >> +		chip->ecc.strength = 8;
> >> +		chip->ecc.read_page = toshiba_nand_read_page_benand;
> >> +		chip->ecc.read_subpage = toshiba_nand_read_subpage_benand;
> >> +		chip->ecc.write_page = nand_write_page_raw;
> >> +		chip->ecc.read_page_raw = nand_read_page_raw;
> >> +		chip->ecc.write_page_raw = nand_write_page_raw;
> >> +
> >> +		chip->options |= NAND_SUBPAGE_READ;
> >> +
> >> +		mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);  
> > 
> > Can you please move this code block in a separate toshiba_benand_init()
> > function in order to keep the toshiba_nand_init() small/readable.  
> 
> Sure. I will resend.
> 
> -- Yoshi
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)

[KOBAYASHI Yoshitake]

On 2017/09/27 6:15, Boris Brezillon wrote:
> On Tue, 26 Sep 2017 18:18:04 +0900
> KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:
> 
>> On 2017/09/21 16:28, Boris Brezillon wrote:
>>> On Thu, 21 Sep 2017 14:32:02 +0900
>>> KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:
>>>   
>>>> This patch enables support for Toshiba BENAND.
>>>> The current implementation does not support vondor specific command  
>>>
>>> 					       ^ vendor
>>>   
>>>> TOSHIBA_NAND_CMD_ECC_STATUS. I would like to add the command, when
>>>> the exec_op() [1] infrastructure is implemented.  
>>>
>>> It's not a good idea to reference a branch that is likely to disappear
>>> in a commit message. Just say that you can't properly support the
>>> TOSHIBA_NAND_CMD_ECC_STATUS operation right and that it might be
>>> addressed in the future.  
>>
>> Thanks. I'll change the comment.
>>
>>>> +	 */
>>>> +	if (status & NAND_STATUS_FAIL) {
>>>> +		/* uncorrectable */
>>>> +		mtd->ecc_stats.failed++;
>>>> +	} else if (status & TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED) {
>>>> +		/* correctable */
>>>> +		max_bitflips = mtd->bitflip_threshold;
>>>> +		mtd->ecc_stats.corrected += max_bitflips;
>>>> +	}  
>>>
>>> Is this working correctly when you read more than one ECC chunk? The
>>> ECC step size is 512 bytes and the page is bigger than that, which means
>>> you have more than one ECC chunk per page. What happens to the
>>> NAND_STATUS_FAIL flag if the first chunk is uncorrectable but
>>> following ones are correctable (or do not contain bitflips at all)?   
>>
>> As you mentioned, the ECC step size is 512 byte. But when 0x70 command is used
>> a simplified ECC status per page is generated.
> 
> I'm fine with that as long as uncorrectable errors are detected
> correctly and TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED is set as soon as
> one of the ECC chunk exposes too many bitflips.
> 
>> In case the first chunk is uncorrectable but the following ones are correctable,
>> the 0x70 command can only check the status of the uncorrectable one.
> 
> Sounds good.
> 
>> Each ECC chunk status can be checked by using the 0x7A command.
>>
>>>> @@ -39,9 +105,43 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
>>>>  
>>>>  static int toshiba_nand_init(struct nand_chip *chip)
>>>>  {
>>>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>>>> +
>>>>  	if (nand_is_slc(chip))
>>>>  		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
>>>>  
>>>> +	if (nand_is_slc(chip) && (chip->id.data[4] & 0x80)) {
>>>> +		/* BENAND */
>>>> +
>>>> +		/*
>>>> +		 * We can't disable the internal ECC engine, the user
>>>> +		 * has to use on-die ECC, there is no alternative.
>>>> +		 */
>>>> +		if (chip->ecc.mode != NAND_ECC_ON_DIE) {
>>>> +			pr_err("On-die ECC should be selected.\n");
>>>> +			return -EINVAL;
>>>> +		}  
>>>
>>> According to your previous explanation that's not exactly true. Since
>>> ECC bytes are stored in a separate area, the user can decide to use
>>> another mode without trouble. Just skip the BENAND initialization when
>>> mode != NAND_ECC_ON_DIE and we should be good, or am I missing something?  
>>
>> I am asking to product department to confirm it.
> 
> I'm almost sure this is the case ;-).

According to the command sequence written in BENAND's datasheet, the status
of the internal ECC must be checked after reading. To do that, ecc.mode has been
set to NAND_ECC_ON_DIE and the status of the internal ECC is checked through 
the 0x70 or 0x7A command. That's the reason we are returning EINVAL here.

-- Yoshi

Re: [PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)

[Boris Brezillon]

On Thu, 5 Oct 2017 16:24:08 +0900
KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:

> On 2017/09/27 6:15, Boris Brezillon wrote:
> > On Tue, 26 Sep 2017 18:18:04 +0900
> > KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:
> >   
> >> On 2017/09/21 16:28, Boris Brezillon wrote:  
> >>> On Thu, 21 Sep 2017 14:32:02 +0900
> >>> KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:
> >>>     
> >>>> This patch enables support for Toshiba BENAND.
> >>>> The current implementation does not support vondor specific command    
> >>>
> >>> 					       ^ vendor
> >>>     
> >>>> TOSHIBA_NAND_CMD_ECC_STATUS. I would like to add the command, when
> >>>> the exec_op() [1] infrastructure is implemented.    
> >>>
> >>> It's not a good idea to reference a branch that is likely to disappear
> >>> in a commit message. Just say that you can't properly support the
> >>> TOSHIBA_NAND_CMD_ECC_STATUS operation right and that it might be
> >>> addressed in the future.    
> >>
> >> Thanks. I'll change the comment.
> >>  
> >>>> +	 */
> >>>> +	if (status & NAND_STATUS_FAIL) {
> >>>> +		/* uncorrectable */
> >>>> +		mtd->ecc_stats.failed++;
> >>>> +	} else if (status & TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED) {
> >>>> +		/* correctable */
> >>>> +		max_bitflips = mtd->bitflip_threshold;
> >>>> +		mtd->ecc_stats.corrected += max_bitflips;
> >>>> +	}    
> >>>
> >>> Is this working correctly when you read more than one ECC chunk? The
> >>> ECC step size is 512 bytes and the page is bigger than that, which means
> >>> you have more than one ECC chunk per page. What happens to the
> >>> NAND_STATUS_FAIL flag if the first chunk is uncorrectable but
> >>> following ones are correctable (or do not contain bitflips at all)?     
> >>
> >> As you mentioned, the ECC step size is 512 byte. But when 0x70 command is used
> >> a simplified ECC status per page is generated.  
> > 
> > I'm fine with that as long as uncorrectable errors are detected
> > correctly and TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED is set as soon as
> > one of the ECC chunk exposes too many bitflips.
> >   
> >> In case the first chunk is uncorrectable but the following ones are correctable,
> >> the 0x70 command can only check the status of the uncorrectable one.  
> > 
> > Sounds good.
> >   
> >> Each ECC chunk status can be checked by using the 0x7A command.
> >>  
> >>>> @@ -39,9 +105,43 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
> >>>>  
> >>>>  static int toshiba_nand_init(struct nand_chip *chip)
> >>>>  {
> >>>> +	struct mtd_info *mtd = nand_to_mtd(chip);
> >>>> +
> >>>>  	if (nand_is_slc(chip))
> >>>>  		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
> >>>>  
> >>>> +	if (nand_is_slc(chip) && (chip->id.data[4] & 0x80)) {
> >>>> +		/* BENAND */
> >>>> +
> >>>> +		/*
> >>>> +		 * We can't disable the internal ECC engine, the user
> >>>> +		 * has to use on-die ECC, there is no alternative.
> >>>> +		 */
> >>>> +		if (chip->ecc.mode != NAND_ECC_ON_DIE) {
> >>>> +			pr_err("On-die ECC should be selected.\n");
> >>>> +			return -EINVAL;
> >>>> +		}    
> >>>
> >>> According to your previous explanation that's not exactly true. Since
> >>> ECC bytes are stored in a separate area, the user can decide to use
> >>> another mode without trouble. Just skip the BENAND initialization when
> >>> mode != NAND_ECC_ON_DIE and we should be good, or am I missing something?    
> >>
> >> I am asking to product department to confirm it.  
> > 
> > I'm almost sure this is the case ;-).  
> 
> According to the command sequence written in BENAND's datasheet, the status
> of the internal ECC must be checked after reading. To do that, ecc.mode has been
> set to NAND_ECC_ON_DIE and the status of the internal ECC is checked through 
> the 0x70 or 0x7A command. That's the reason we are returning EINVAL here.

But the status will anyway be retrieved, and what's the point of
checking the ECC flags if the user wants to use its own ECC engine? I
mean, since you have the whole OOB area exposed why would you prevent
existing setup from working (by existing setup I mean those that already
have a BENAND but haven't modified their driver to accept ON_DIE_ECC).

Maybe I'm missing something, but AFAICT it's safe to allow users to
completely ignore the on-die ECC engine and use their own, even if
that means duplicating the work since on-die ECC cannot be disabled on
BENAND devices.

Re: [PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)

[KOBAYASHI Yoshitake]

On 2017/10/05 16:31, Boris Brezillon wrote:
> On Thu, 5 Oct 2017 16:24:08 +0900
> KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:
>>>>>> @@ -39,9 +105,43 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
>>>>>>  
>>>>>>  static int toshiba_nand_init(struct nand_chip *chip)
>>>>>>  {
>>>>>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>>>>>> +
>>>>>>  	if (nand_is_slc(chip))
>>>>>>  		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
>>>>>>  
>>>>>> +	if (nand_is_slc(chip) && (chip->id.data[4] & 0x80)) {
>>>>>> +		/* BENAND */
>>>>>> +
>>>>>> +		/*
>>>>>> +		 * We can't disable the internal ECC engine, the user
>>>>>> +		 * has to use on-die ECC, there is no alternative.
>>>>>> +		 */
>>>>>> +		if (chip->ecc.mode != NAND_ECC_ON_DIE) {
>>>>>> +			pr_err("On-die ECC should be selected.\n");
>>>>>> +			return -EINVAL;
>>>>>> +		}    
>>>>>
>>>>> According to your previous explanation that's not exactly true. Since
>>>>> ECC bytes are stored in a separate area, the user can decide to use
>>>>> another mode without trouble. Just skip the BENAND initialization when
>>>>> mode != NAND_ECC_ON_DIE and we should be good, or am I missing something?    
>>>>
>>>> I am asking to product department to confirm it.  
>>>
>>> I'm almost sure this is the case ;-).  
>>
>> According to the command sequence written in BENAND's datasheet, the status
>> of the internal ECC must be checked after reading. To do that, ecc.mode has been
>> set to NAND_ECC_ON_DIE and the status of the internal ECC is checked through 
>> the 0x70 or 0x7A command. That's the reason we are returning EINVAL here.
> 
> But the status will anyway be retrieved, and what's the point of
> checking the ECC flags if the user wants to use its own ECC engine? I
> mean, since you have the whole OOB area exposed why would you prevent
> existing setup from working (by existing setup I mean those that already
> have a BENAND but haven't modified their driver to accept ON_DIE_ECC).
> 
> Maybe I'm missing something, but AFAICT it's safe to allow users to
> completely ignore the on-die ECC engine and use their own, even if
> that means duplicating the work since on-die ECC cannot be disabled on
> BENAND devices.

If user host controller ECC engine can support 8bit ECC or more ,
Toshiba offers 24nm SLC NAND products (not BENAND).  If user host
controller ECC engine is less that 8bit ECC (for example: 1bit or
4bit ECC) Toshiba offers BENAND.  When using BENAND, checking
BENAND own ECC status (ECC flag) is required as per BENAND
product datasheet. Ignoring BENAND on-die ECC operation status,
and rely only on host 1 bit ECC or 4 bit ECC status, is not
recommended because the host ECC capability is inferior to BENAND
8bit ECC and data refresh or other operations may not work
properly.  Also when using BENAND, turning off Host ECC is
recommended because this can eliminate the latency due to double
ECC operation(by both host & BENAND).

-- YOSHI

Re: [PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)

[Boris Brezillon]

On Thu, 12 Oct 2017 22:03:23 +0900
KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:

> On 2017/10/05 16:31, Boris Brezillon wrote:
> > On Thu, 5 Oct 2017 16:24:08 +0900
> > KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:  
> >>>>>> @@ -39,9 +105,43 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
> >>>>>>  
> >>>>>>  static int toshiba_nand_init(struct nand_chip *chip)
> >>>>>>  {
> >>>>>> +	struct mtd_info *mtd = nand_to_mtd(chip);
> >>>>>> +
> >>>>>>  	if (nand_is_slc(chip))
> >>>>>>  		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
> >>>>>>  
> >>>>>> +	if (nand_is_slc(chip) && (chip->id.data[4] & 0x80)) {
> >>>>>> +		/* BENAND */
> >>>>>> +
> >>>>>> +		/*
> >>>>>> +		 * We can't disable the internal ECC engine, the user
> >>>>>> +		 * has to use on-die ECC, there is no alternative.
> >>>>>> +		 */
> >>>>>> +		if (chip->ecc.mode != NAND_ECC_ON_DIE) {
> >>>>>> +			pr_err("On-die ECC should be selected.\n");
> >>>>>> +			return -EINVAL;
> >>>>>> +		}      
> >>>>>
> >>>>> According to your previous explanation that's not exactly true. Since
> >>>>> ECC bytes are stored in a separate area, the user can decide to use
> >>>>> another mode without trouble. Just skip the BENAND initialization when
> >>>>> mode != NAND_ECC_ON_DIE and we should be good, or am I missing something?      
> >>>>
> >>>> I am asking to product department to confirm it.    
> >>>
> >>> I'm almost sure this is the case ;-).    
> >>
> >> According to the command sequence written in BENAND's datasheet, the status
> >> of the internal ECC must be checked after reading. To do that, ecc.mode has been
> >> set to NAND_ECC_ON_DIE and the status of the internal ECC is checked through 
> >> the 0x70 or 0x7A command. That's the reason we are returning EINVAL here.  
> > 
> > But the status will anyway be retrieved, and what's the point of
> > checking the ECC flags if the user wants to use its own ECC engine? I
> > mean, since you have the whole OOB area exposed why would you prevent
> > existing setup from working (by existing setup I mean those that already
> > have a BENAND but haven't modified their driver to accept ON_DIE_ECC).
> > 
> > Maybe I'm missing something, but AFAICT it's safe to allow users to
> > completely ignore the on-die ECC engine and use their own, even if
> > that means duplicating the work since on-die ECC cannot be disabled on
> > BENAND devices.  
> 
> If user host controller ECC engine can support 8bit ECC or more ,
> Toshiba offers 24nm SLC NAND products (not BENAND).  If user host
> controller ECC engine is less that 8bit ECC (for example: 1bit or
> 4bit ECC) Toshiba offers BENAND.  When using BENAND, checking
> BENAND own ECC status (ECC flag) is required as per BENAND
> product datasheet. Ignoring BENAND on-die ECC operation status,
> and rely only on host 1 bit ECC or 4 bit ECC status, is not
> recommended because the host ECC capability is inferior to BENAND
> 8bit ECC and data refresh or other operations may not work
> properly.

Well, that's not really your problem. The framework already complains
if someone tries to use an ECC that is weaker than the chip
requirement. On the other hand, it's perfectly valid to use on
host-side ECC engine that meets NAND requirements (8bit/xxxbytes).

The use case I'm trying to gracefully handle here is: your NAND
controller refuses to use anything but the host-side ECC engine and you
have a BENAND connected to this controller.
Before your patch this use case worked just fine, and the user didn't
even notice it was using a NAND chip that was capable of correcting
bitflips. After your patch it fails to probe the NAND chip and users
will have to patch their controller driver to make it work again. Sorry
but this is not really an option: we have to keep existing setup in a
working state, and that means allowing people to use their BENAND in a
degraded state where they'll just ignore the on-die ECC and use their
own ECC engine instead.

I really don't see the problem here. It's not worse than it was before
your patch, and those wanting to activate on-die ECC support will have
to patch their controller driver anyway.

> Also when using BENAND, turning off Host ECC is
> recommended because this can eliminate the latency due to double
> ECC operation(by both host & BENAND).

I thought you were not able to turn it off.

> 
> -- YOSHI
> 

Re: [PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)

[KOBAYASHI Yoshitake]

On 2017/10/12 22:26, Boris Brezillon wrote:
> On Thu, 12 Oct 2017 22:03:23 +0900
> KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:
> 
>> On 2017/10/05 16:31, Boris Brezillon wrote:
>>> On Thu, 5 Oct 2017 16:24:08 +0900
>>> KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:  
>>>>>>>> @@ -39,9 +105,43 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
>>>>>>>>  
>>>>>>>>  static int toshiba_nand_init(struct nand_chip *chip)
>>>>>>>>  {
>>>>>>>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>>>>>>>> +
>>>>>>>>  	if (nand_is_slc(chip))
>>>>>>>>  		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
>>>>>>>>  
>>>>>>>> +	if (nand_is_slc(chip) && (chip->id.data[4] & 0x80)) {
>>>>>>>> +		/* BENAND */
>>>>>>>> +
>>>>>>>> +		/*
>>>>>>>> +		 * We can't disable the internal ECC engine, the user
>>>>>>>> +		 * has to use on-die ECC, there is no alternative.
>>>>>>>> +		 */
>>>>>>>> +		if (chip->ecc.mode != NAND_ECC_ON_DIE) {
>>>>>>>> +			pr_err("On-die ECC should be selected.\n");
>>>>>>>> +			return -EINVAL;
>>>>>>>> +		}      
>>>>>>>
>>>>>>> According to your previous explanation that's not exactly true. Since
>>>>>>> ECC bytes are stored in a separate area, the user can decide to use
>>>>>>> another mode without trouble. Just skip the BENAND initialization when
>>>>>>> mode != NAND_ECC_ON_DIE and we should be good, or am I missing something?      
>>>>>>
>>>>>> I am asking to product department to confirm it.    
>>>>>
>>>>> I'm almost sure this is the case ;-).    
>>>>
>>>> According to the command sequence written in BENAND's datasheet, the status
>>>> of the internal ECC must be checked after reading. To do that, ecc.mode has been
>>>> set to NAND_ECC_ON_DIE and the status of the internal ECC is checked through 
>>>> the 0x70 or 0x7A command. That's the reason we are returning EINVAL here.  
>>>
>>> But the status will anyway be retrieved, and what's the point of
>>> checking the ECC flags if the user wants to use its own ECC engine? I
>>> mean, since you have the whole OOB area exposed why would you prevent
>>> existing setup from working (by existing setup I mean those that already
>>> have a BENAND but haven't modified their driver to accept ON_DIE_ECC).
>>>
>>> Maybe I'm missing something, but AFAICT it's safe to allow users to
>>> completely ignore the on-die ECC engine and use their own, even if
>>> that means duplicating the work since on-die ECC cannot be disabled on
>>> BENAND devices.  
>>
>> If user host controller ECC engine can support 8bit ECC or more ,
>> Toshiba offers 24nm SLC NAND products (not BENAND).  If user host
>> controller ECC engine is less that 8bit ECC (for example: 1bit or
>> 4bit ECC) Toshiba offers BENAND.  When using BENAND, checking
>> BENAND own ECC status (ECC flag) is required as per BENAND
>> product datasheet. Ignoring BENAND on-die ECC operation status,
>> and rely only on host 1 bit ECC or 4 bit ECC status, is not
>> recommended because the host ECC capability is inferior to BENAND
>> 8bit ECC and data refresh or other operations may not work
>> properly.
> 
> Well, that's not really your problem. The framework already complains
> if someone tries to use an ECC that is weaker than the chip
> requirement. On the other hand, it's perfectly valid to use on
> host-side ECC engine that meets NAND requirements (8bit/xxxbytes).

I have assumed to specify the ecc strength and size by devicetree.
Before BENAND patch updated, I would like to submit a patch which read
the ECC strength and size from the nand using extended-id like the
Samsung nand patch[1].
 [1] https://patchwork.ozlabs.org/patch/712549/

> The use case I'm trying to gracefully handle here is: your NAND
> controller refuses to use anything but the host-side ECC engine and you
> have a BENAND connected to this controller.
> Before your patch this use case worked just fine, and the user didn't
> even notice it was using a NAND chip that was capable of correcting
> bitflips. After your patch it fails to probe the NAND chip and users
> will have to patch their controller driver to make it work again. Sorry
> but this is not really an option: we have to keep existing setup in a
> working state, and that means allowing people to use their BENAND in a
> degraded state where they'll just ignore the on-die ECC and use their
> own ECC engine instead.
> 
> I really don't see the problem here. It's not worse than it was before
> your patch, and those wanting to activate on-die ECC support will have
> to patch their controller driver anyway.

If the above approach is acceptable, I will update BENAND patch according to
your idea.

-- Yoshi

>> Also when using BENAND, turning off Host ECC is
>> recommended because this can eliminate the latency due to double
>> ECC operation(by both host & BENAND).
> 
> I thought you were not able to turn it off.
> 
>>
>> -- YOSHI
>>
> 
> 
> 

Re: [PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)

[Boris Brezillon]

On Fri, 20 Oct 2017 13:52:32 +0900
KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:

> On 2017/10/12 22:26, Boris Brezillon wrote:
> > On Thu, 12 Oct 2017 22:03:23 +0900
> > KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:
> >   
> >> On 2017/10/05 16:31, Boris Brezillon wrote:  
> >>> On Thu, 5 Oct 2017 16:24:08 +0900
> >>> KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:    
> >>>>>>>> @@ -39,9 +105,43 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
> >>>>>>>>  
> >>>>>>>>  static int toshiba_nand_init(struct nand_chip *chip)
> >>>>>>>>  {
> >>>>>>>> +	struct mtd_info *mtd = nand_to_mtd(chip);
> >>>>>>>> +
> >>>>>>>>  	if (nand_is_slc(chip))
> >>>>>>>>  		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
> >>>>>>>>  
> >>>>>>>> +	if (nand_is_slc(chip) && (chip->id.data[4] & 0x80)) {
> >>>>>>>> +		/* BENAND */
> >>>>>>>> +
> >>>>>>>> +		/*
> >>>>>>>> +		 * We can't disable the internal ECC engine, the user
> >>>>>>>> +		 * has to use on-die ECC, there is no alternative.
> >>>>>>>> +		 */
> >>>>>>>> +		if (chip->ecc.mode != NAND_ECC_ON_DIE) {
> >>>>>>>> +			pr_err("On-die ECC should be selected.\n");
> >>>>>>>> +			return -EINVAL;
> >>>>>>>> +		}        
> >>>>>>>
> >>>>>>> According to your previous explanation that's not exactly true. Since
> >>>>>>> ECC bytes are stored in a separate area, the user can decide to use
> >>>>>>> another mode without trouble. Just skip the BENAND initialization when
> >>>>>>> mode != NAND_ECC_ON_DIE and we should be good, or am I missing something?        
> >>>>>>
> >>>>>> I am asking to product department to confirm it.      
> >>>>>
> >>>>> I'm almost sure this is the case ;-).      
> >>>>
> >>>> According to the command sequence written in BENAND's datasheet, the status
> >>>> of the internal ECC must be checked after reading. To do that, ecc.mode has been
> >>>> set to NAND_ECC_ON_DIE and the status of the internal ECC is checked through 
> >>>> the 0x70 or 0x7A command. That's the reason we are returning EINVAL here.    
> >>>
> >>> But the status will anyway be retrieved, and what's the point of
> >>> checking the ECC flags if the user wants to use its own ECC engine? I
> >>> mean, since you have the whole OOB area exposed why would you prevent
> >>> existing setup from working (by existing setup I mean those that already
> >>> have a BENAND but haven't modified their driver to accept ON_DIE_ECC).
> >>>
> >>> Maybe I'm missing something, but AFAICT it's safe to allow users to
> >>> completely ignore the on-die ECC engine and use their own, even if
> >>> that means duplicating the work since on-die ECC cannot be disabled on
> >>> BENAND devices.    
> >>
> >> If user host controller ECC engine can support 8bit ECC or more ,
> >> Toshiba offers 24nm SLC NAND products (not BENAND).  If user host
> >> controller ECC engine is less that 8bit ECC (for example: 1bit or
> >> 4bit ECC) Toshiba offers BENAND.  When using BENAND, checking
> >> BENAND own ECC status (ECC flag) is required as per BENAND
> >> product datasheet. Ignoring BENAND on-die ECC operation status,
> >> and rely only on host 1 bit ECC or 4 bit ECC status, is not
> >> recommended because the host ECC capability is inferior to BENAND
> >> 8bit ECC and data refresh or other operations may not work
> >> properly.  
> > 
> > Well, that's not really your problem. The framework already complains
> > if someone tries to use an ECC that is weaker than the chip
> > requirement. On the other hand, it's perfectly valid to use on
> > host-side ECC engine that meets NAND requirements (8bit/xxxbytes).  
> 
> I have assumed to specify the ecc strength and size by devicetree.
> Before BENAND patch updated, I would like to submit a patch which read
> the ECC strength and size from the nand using extended-id like the
> Samsung nand patch[1].

Sure.

>  [1] https://patchwork.ozlabs.org/patch/712549/
> 
> > The use case I'm trying to gracefully handle here is: your NAND
> > controller refuses to use anything but the host-side ECC engine and you
> > have a BENAND connected to this controller.
> > Before your patch this use case worked just fine, and the user didn't
> > even notice it was using a NAND chip that was capable of correcting
> > bitflips. After your patch it fails to probe the NAND chip and users
> > will have to patch their controller driver to make it work again. Sorry
> > but this is not really an option: we have to keep existing setup in a
> > working state, and that means allowing people to use their BENAND in a
> > degraded state where they'll just ignore the on-die ECC and use their
> > own ECC engine instead.
> > 
> > I really don't see the problem here. It's not worse than it was before
> > your patch, and those wanting to activate on-die ECC support will have
> > to patch their controller driver anyway.  
> 
> If the above approach is acceptable, I will update BENAND patch according to
> your idea.

Okay. BTW, an ->exec_op() implementation has been posted earlier
this week [1], and since you are depending on it to support your custom
TOSHIBA_NAND_CMD_ECC_STATUS command, that'd be great to have a review
from you.

[1]http://patchwork.ozlabs.org/project/linux-mtd/list/?series=8923