On Fri, 31 Mar 2017 14:06:32 +0900
Masahiro Yamada <yamada.masah...@socionext.com> wrote:
> Hi Boris,
>
>
> 2017-03-30 23:02 GMT+09:00 Boris Brezillon
> <boris.brezil...@free-electrons.com>:
> > On Thu, 30 Mar 2017 15:46:00 +0900
> > Masahiro Yamada <yamada.masah...@socionext.com> wrote:
> >
> >> Historically, this driver tried to choose as big ECC strength as
> >> possible, but it would be reasonable to allow DT to set a particular
> >> ECC strength with "nand-ecc-strength" property. This is useful
> >> when a particular ECC setting is hard-coded by firmware (or hard-
> >> wired by boot ROM).
> >>
> >> If no ECC strength is specified in DT, "nand-ecc-maximize" is implied
> >> since this was the original behavior.
> >
> > You said there is currently no DT users,
>
> Right. No DT users ever in upstream.
>
>
> > so how about changing the
> > "fallback to ECC maximization" behavior for DT users, and instead of
> > maximizing the ECC strength take the NAND requirements into account
> > (chip->ecc_strength_ds).
>
> This is difficult to judge in some cases.
>
> As I said before, 4/512 and 8/1024 are not equivalent.
>
> If chip's requirement chip->ecc_step_ds matches
> to the ecc->size supported by the controller,
> this is easy.
>
>
> If a chip requests 1024B, then the controller can only support 512B chunk
> (or vice versa), it is difficult to simply compare
> ecc strength.
You can try something like that when no explicit ecc.strength and
ecc.size has been set in the DT and when ECC_MAXIMIZE was not passed.
static int
denali_get_closest_ecc_strength(struct denali_nand_info *denali,
int strength)
{
/*
* Whatever you need to select a strength that is greater than
* or equal to strength.
*/
return X;
}
static int denali_try_to_match_ecc_req(struct denali_nand_info *denali)
{
struct nand_chip *chip = &denali->nand;
struct mtd_info *mtd = nand_to_mtd(chip);
int max_ecc_bytes = mtd->oobsize - denali->bbtskipbytes;
int ecc_steps, ecc_strength, ecc_bytes;
int ecc_size = chip->ecc_step_ds;
int ecc_strength = chip->ecc_strength_ds;
/*
* No information provided by the NAND chip, let the core
* maximize the strength.
*/
if (!ecc_size || !ecc_strength)
return -ENOTSUPP;
if (ecc_size > 512)
ecc_size = 1024;
else
ecc_size = 512;
/* Adjust ECC step size based on hardware support. */
if (ecc_size == 1024 &&
!(denali->caps & DENALI_CAP_ECC_SIZE_1024))
ecc_size = 512;
else if(ecc_size == 512 &&
!(denali->caps & DENALI_CAP_ECC_SIZE_512))
ecc_size = 1024;
if (ecc_size < chip->ecc_size_ds) {
/*
* When the selected size if smaller than the expected
* one we try to use the same strength but on 512 blocks
* so that we can still fix the same number of errors
* even if they are concentrated in the first 512bytes
* of a 1024bytes portion.
*/
ecc_strength = chip->ecc_strength_ds;
ecc_strength = denali_get_closest_ecc_strength(denali,
ecc_strength);
} else {
/* Always prefer 1024bytes ECC blocks when possible. */
if (ecc_size != 1024 &&
(denali->caps & DENALI_CAP_ECC_SIZE_1024) &&
mtd->writesize > 1024)
ecc_size = 1024;
/*
* Adjust the strength based on the selected ECC step
* size.
*/
ecc_strength = DIV_ROUND_UP(ecc_size,
chip->ecc_step_ds) *
chip->ecc_strength_ds;
}
ecc_bytes = denali_calc_ecc_bytes(ecc_size,
ecc_strength);
ecc_bytes *= mtd->writesize / ecc_size;
/*
* If we don't have enough space, let the core maximize
* the strength.
*/
if (ecc_bytes > max_ecc_bytes)
return -ENOTSUPP;
chip->ecc.strength = ecc_strength;
chip->ecc.size = ecc_size;
return 0;
}
>
> Is it a bad thing if we use too strong ECC strength?
>
> The disadvantage I see is we will have less OOB-free bytes,
> but this will not be fatal, I guess.
Not a bad thing in general, but I'd prefer to leave the choice to the
user. If one doesn't need the extra-safety brought by ECC strength
maximization and wants to have more OOB bytes it's better to follow
NAND requirements.
