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
--
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
