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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Mon, 3 Apr 2017 12:16:34 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 
> 
> 2017-03-31 18:46 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> 
> > 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?

Well, only the denali driver knows what the hardware supports, though
having a generic function that takes a table of supported strengths
would work.

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

It's almost controller-agnostic, except for the denali_calc_ecc_bytes()
function, but I guess we could ask drivers to implement a hook that is
passed the ECC step size and strength and returns the associated
number of ECC bytes.

> 
> 
> [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.
>        */
> }
>

I like the idea, except I would do this slightly differently to avoid
declaring all combinations of stepsize and strengths

struct nand_ecc_stepsize_info {
	int stepsize;
	int nstrengths;
	int *strengths;
};

struct nand_ecc_engine_caps {
	int nstepsizes;
	struct nand_ecc_stepsize_info *stepsizes;
	int (*calc_ecc_bytes)(int stepsize, int strength);
};

int nand_try_to_match_ecc_req(struct nand_chip *chip,
			      const struct nand_ecc_engine_caps *caps,
			      struct nand_ecc_spec *spec)
{
	/*
	 * Find the most appropriate setting based on the ECC engine
	 * caps and fill the spec object accordingly.
	 * Returns 0 in case of success and a negative error code
	 * otherwise.
	 */
}

Note that nand_try_to_match_ecc_req() has to be more generic than
denali_try_to_match_ecc_req() WRT step sizes, which will probably
complexify the logic.