Re: [PATCH v3 14/37] mtd: nand: denali: support "nand-ecc-strength" DT property

[Masahiro Yamada]

Hi Boris,



2017-03-31 18:46 GMT+09:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>:

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


Is here anything specific to Denali?


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


As a whole, this does not seem to driver-specific.


[1] A driver provides some pairs of (ecc_strength, ecc_size)
    it can support.

[2] The core framework knows the chip's requirement
    (ecc_strength_ds, ecc_size_ds).


Then, the core framework provides a function
to return a most recommended (ecc_strength, ecc_size).



struct nand_ecc_spec {
       int ecc_strength;
       int ecc_size;
};

/*
 * This function choose the most recommented (ecc_str, ecc_size)
 * "recommended" means: minimum ecc stregth that meets
 * the chip's requirment.
 *
 *
 * @chip   - nand_chip
 * @controller_ecc_spec - Array of (ecc_str, ecc_size) supported by the
                          controller. (terminated by NULL as sentinel)
 */
struct nand_ecc_spec * nand_try_to_match_ecc_req(struct nand_chip *chip,
                                                 struct nand_ecc_spec
*controller_ecc_spec)
{
      /*
       * Return the pointer to the most recommended
       * struct nand_ecc_spec.
       * If nothing suitable found, return NULL.
       */
}



Then, Denali driver can call it:


        recommended_ecc_spec = nand_try_to_match_ecc_req(chip,
                                                         denali->ecc_spec);
        if (recommended_ecc_spec) {
                chip->ecc.strength = recommended_ecc_spec.ecc_strength;
                chip->ecc.size = recommended_ecc_spec.ecc_size;
        } else {
                /*
                 * Do something  (for example, maximize the ECC)
                 */
        }





It seems weird to force this to the Denali driver.



-- 
Best Regards
Masahiro Yamada