lkml.org 
[lkml]   [2017]   [Oct]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)
From
Date
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
>>
>
>
>

\
 
 \ /
  Last update: 2017-10-22 17:34    [W:0.052 / U:1.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site