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@xxxxxxxxxxxxx> wrote:

> On 2017/10/05 16:31, Boris Brezillon wrote:
> > On Thu, 5 Oct 2017 16:24:08 +0900
> > KOBAYASHI Yoshitake <yoshitake.kobayashi@xxxxxxxxxxxxx> 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
>