Re: [RESEND PATCH v2 26/53] mtd: nand: denali: support 1024 byte ECC step size

[Masahiro Yamada]

Hi Boris,


2017-03-23 17:39 GMT+09:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>:
> On Thu, 23 Mar 2017 15:53:14 +0900
> Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:
>
>> Hi Boris,
>>
>> 2017-03-23 6:32 GMT+09:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>:
>> > On Thu, 23 Mar 2017 05:07:25 +0900
>> > Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:
>> >
>> >> This driver was originally written for the Intel MRST platform with
>> >> several platform specific parameters hard-coded.  Another thing we
>> >> need to fix is the hard-coded ECC step size.  Currently, it is
>> >> defined as follows:
>> >>
>> >>   #define ECC_SECTOR_SIZE 512
>> >>
>> >> (somehow, it is defined in both denali.c and denali.h)
>> >>
>> >> This must be avoided because the Denali IP supports 1024B ECC size
>> >> as well.  The Denali User's Guide also says supporting both 512B and
>> >> 1024B ECC sectors is possible, though it would require instantiation
>> >> of two different ECC circuits.  So, possible cases are:
>> >>
>> >>  [1] only 512B ECC size is supported
>> >>  [2] only 1024B ECC size is supported
>> >>  [3] both 512B and 1024B ECC sizes are supported
>> >>
>> >> For [3], the actually used ECC size is specified by some registers.
>> >>
>> >> Newer versions of this IP have the following registers:
>> >>   CFG_DATA_BLOCK_SIZE       (0x6b0)
>> >>   CFG_LAST_DATA_BLOCK_SIZE  (0x6c0)
>> >>   CFG_NUM_DATA_BLOCKS       (0x6d0)
>> >>
>> >> For those versions, the software should set ecc.size and ecc.steps
>> >> to these registers.  Old versions do not have such registers, but
>> >> they are "reserved", so write accesses are safely ignored.
>> >>
>> >> This commit adds new flags DENALI_CAP_ECC_SIZE_{512,1024}.
>> >>
>> >> The DT property "nand-ecc-step-size" is still optional; a reasonable
>> >> default will be chosen for [1] and [2].  For case [3], if exists, it
>> >> is recommended to specify the desired ECC size explicitly.
>> >
>> > Actually, the NAND chip gives some hints to help controller drivers
>> > decide which ecc-block-size/strength is appropriate
>> > (chip->ecc_strength_ds, chip->ecc_step_ds), so, in most cases
>> > nand-ecc-step-size is unneeded (unless you want to force a specific
>> > setting).
>>
>>
>> But, if we look at nand_flash_detect_onfi(),
>> ->ecc_step_ds is almost a fixed value, 512, right?
>>
>> if (p->ecc_bits != 0xff) {
>>         chip->ecc_strength_ds = p->ecc_bits;
>>         chip->ecc_step_ds = 512;
>>
>
> Nope, if the NAND requires a different ECC block size, you will have
> 0xff in this field and the real ECC requirements will be exposed in the
> extended parameter page [1].
>
>>
>> As far as I understood,
>> ->ecc_step_ds is not a hint for drivers to decide ->ecc.size.
>>
>> Rather, ->ecc_step_ds just specifies the unit of ->ecc_strength_ds.
>
> ->ecc_xxx_ds are encoding the minimum ECC requirements as expressed by
> the NAND chip, so yes, it is an hint for the ECC engine configuration.
>
> It does not necessarily means it has to be exactly the same, but it
> should be used to determine which setting is appropriate. For example,
> if the NAND says it requires a minimum of 4bits/512bytes but your
> controller only supports 16bits/1024bytes, then it's fine.
>
>
>
>>
>>
>>
>> >>                       int offset;
>> >>                       unsigned int flips_in_byte;
>> >>
>> >> -                     offset = (err_sector * ECC_SECTOR_SIZE + err_byte) *
>> >> +                     offset = (err_sector * ecc_size + err_byte) *
>> >>                                               denali->devnum + err_device;
>> >>
>> >>                       /* correct the ECC error */
>> >> @@ -1579,22 +1578,37 @@ int denali_init(struct denali_nand_info *denali)
>> >>       /* no subpage writes on denali */
>> >>       chip->options |= NAND_NO_SUBPAGE_WRITE;
>> >>
>> >> +     if (!chip->ecc.size) {
>> >
>> > You should set it to chip->ecc_step_ds and pick a default value only if
>> > it's still 0 after that. Same goes for ecc.strength.
>>
>> Sorry, I still do not understand this.
>>
>> ->ecc_strength_ds and ->ecc_step_ds
>> shows how often bit-flip occurs in this device.
>
> It represents the minimum ECC requirements to ensure a 'reliable' setup.
>
>>
>> So, nand_ecc_strength_good() is a typical usage of these.
>
> nand_ecc_strength_good() is complaining if you choose an ECC setting
> that is below the minimum requirements.
>
>>
>> How many sectors the driver actually splits the device into
>> is a different issue, I think.
>
> The choice is left to the ECC controller, but it should take the
> ->ecc_strength_ds and ->ecc_step_ds information into account when
> choosing the ECC settings.
>
>>
>>
>>
>> >> +             if (denali->caps & DENALI_CAP_ECC_SIZE_512)
>> >> +                     chip->ecc.size = 512;
>> >> +             if (denali->caps & DENALI_CAP_ECC_SIZE_1024)
>> >> +                     chip->ecc.size = 1024;
>> >> +             if (WARN(!chip->ecc.size, "must support at least 512 or 1024 ECC size"))
>> >> +                     goto failed_req_irq;
>> >> +     }
>> >> +
>> >> +     if ((chip->ecc.size != 512 && chip->ecc.size != 1024) ||
>> >> +         (chip->ecc.size == 512 && !(denali->caps & DENALI_CAP_ECC_SIZE_512)) ||
>> >> +         (chip->ecc.size == 1024 && !(denali->caps & DENALI_CAP_ECC_SIZE_1024))) {
>> >> +             dev_err(denali->dev, "specified ECC size %d in not supported",
>> >> +                     chip->ecc.size);
>> >> +             goto failed_req_irq;
>> >> +     }
>> >> +
>> >>       /*
>> >>        * Denali Controller only support 15bit and 8bit ECC in MRST,
>> >>        * so just let controller do 15bit ECC for MLC and 8bit ECC for
>> >>        * SLC if possible.
>> >
>> > Usually the NAND chips expose the ECC requirements, so basing our
>> > decision only on the type of NAND sounds a bit weird.
>>
>>
>> chip->ecc.size is one of the configuration of this controller IP.
>>
>> SoC vendors choose 512, 1024, or both of them
>> when they buy this IP.
>
> Yes, and that's not a problem.
>
>>
>> If both 512 and 1024 are supported, 1024 is usually a better choice
>> because bigger ecc.size uses ECC more efficiently.
>
> We agree.
>
>>
>>
>> It is unrelated to the chips' requirements.
>
> It is related to the chip requirements.
> Say you have a chip that requires a minimum of 4bits/512bytes. If you
> want to convert that to a 1024byte block setting it's perfectly fine,
> but then you'll have to meet (2 * ->ecc_strength_ds) for the
> ecc.strength parameter.

I think this example case is always fine from the
"bigger ecc.size uses ECC more efficiently" we agreed.
If 4bits/512bytes is achievable, 8bits/1024bytes is always met.


The reverse is not always true.
If the chip requires 8bits/1024bytes then controller uses 4bit/512bytes,
this could be a reliability problem.



> The nand-ecc-strength and nand-ecc-step DT properties are here to
> override the chip requirements and force a specific setting. This is
> for example needed when the bootloader hardcodes an ECC setting without
> taking the NAND chip requirements into account, and since you want to
> read/write from both the bootloader and linux, you'll have to force this
> specific ECC setting, but this case should be the exception, not the
> default behavior.

Yes, I also thought the case where the boot-loader hardcodes an ECC setting.

Moreover, the Boot ROM really hard-codes (hard-wires)
the ECC setting in some cases.   On some Socionext UniPhier boards,
users have no freedom to change the ECC settings.

So, DT property need to be supported somehow.


>
> If you want an example on how to extrapolate ECC engine settings from
> ->ecc_xxx_ds info, you can look at the sunxi_nand driver [2].
>
> [1]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_base.c#L3491
> [2]http://lxr.free-electrons.com/source/drivers/mtd/nand/sunxi_nand.c#L1946
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Masahiro Yamada