Messages in this thread | | | Subject | Re: [PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND) | From | KOBAYASHI Yoshitake <> | Date | Fri, 20 Oct 2017 13:52:32 +0900 |
| |
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 >> > > >
| |