On Tue, 6 Jun 2017 08:21:42 +0900 Masahiro Yamada <yamada.masah...@socionext.com> wrote:
> Driver are responsible for setting up ECC parameters correctly. > Those include: > - Check if ECC parameters specified (usually by DT) are valid > - Meet the chip's ECC requirement > - Maximize ECC strength if NAND_ECC_MAXIMIZE flag is set > > The logic can be generalized by factoring out common code. > > This commit adds 3 helpers to the NAND framework: > nand_check_ecc_caps - Check if preset step_size and strength are valid > nand_match_ecc_req - Match the chip's requirement > nand_maximize_ecc - Maximize the ECC strength > > To use the helpers above, a driver needs to provide: > - Data array of supported ECC step size and strength > - A hook that calculates ECC bytes from the combination of > step_size and strength. > > By using those helpers, code duplication among drivers will be > reduced. > > Signed-off-by: Masahiro Yamada <yamada.masah...@socionext.com> > --- > > Changes since the previous version: > > - Step size info holds an array of associated strengths > - nand_match_ecc_req() does not take care of the case > where ecc_size/strength is already set > - Reflect more comments from Boris > > Previous version: > http://patchwork.ozlabs.org/patch/752107/ > > > Changes in v4: None > Changes in v3: None > Changes in v2: None > > drivers/mtd/nand/nand_base.c | 219 > +++++++++++++++++++++++++++++++++++++++++++ > include/linux/mtd/nand.h | 35 +++++++ > 2 files changed, 254 insertions(+) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index bdfa903..f2da4f2 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -4509,6 +4509,225 @@ static int nand_set_ecc_soft_ops(struct mtd_info *mtd) > } > } > > +/** > + * nand_check_ecc_caps - check the sanity of preset ECC settings > + * @mtd: mtd info structure > + * @chip: nand chip info structure > + * @caps: ECC caps info structure > + * > + * When ECC step size and strength are already set, check if they are > supported > + * by the controller and the calculated ECC bytes fit within the chip's OOB. > + * On success, the calculated ECC bytes is set. > + */ > +int nand_check_ecc_caps(struct mtd_info *mtd, struct nand_chip *chip, > + const struct nand_ecc_caps *caps) > +{ > + const struct nand_ecc_step_info *stepinfo; > + int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes; > + int preset_step = chip->ecc.size; > + int preset_strength = chip->ecc.strength; > + int ecc_bytes; > + int i, j; > + > + if (WARN_ON(avail_oobsize < 0)) > + return -EINVAL; > + > + if (!preset_step || !preset_strength) > + return -ENODATA; > + > + for (i = 0; i < caps->nstepinfos; i++) { > + stepinfo = &caps->stepinfos[i]; > + > + if (stepinfo->stepsize != preset_step) > + continue; > + > + for (j = 0; j < stepinfo->nstrengths; j++) { > + if (stepinfo->strengths[j] == preset_strength) > + goto found; > + } > + } > + > + pr_err("ECC (step, strength) = (%d, %d) not supported on this > controller", > + preset_step, preset_strength); > + > + return -ENOTSUPP; > + > +found: I prefer something like: if (i == caps->nstepinfos) { pr_err(...); return -ENOTSUPP; } ... instead of this 'found' label. > + ecc_bytes = caps->calc_ecc_bytes(preset_step, preset_strength); > + if (WARN_ON_ONCE(ecc_bytes < 0)) > + return ecc_bytes; > + > + if (ecc_bytes * mtd->writesize / preset_step > avail_oobsize) { > + pr_err("ECC (step, strength) = (%d, %d) does not fit in OOB", > + preset_step, preset_strength); > + return -ENOSPC; > + } > + > + chip->ecc.bytes = ecc_bytes; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(nand_check_ecc_caps); > + > +/** > + * nand_match_ecc_req - meet the chip's requirement with least ECC bytes > + * @mtd: mtd info structure > + * @chip: nand chip info structure > + * @caps: ECC engine caps info structure > + * > + * If a chip's ECC requirement is provided, try to meet it with the least > + * number of ECC bytes (i.e. with the largest number of OOB-free bytes). > + * On success, the chosen ECC settings are set. > + */ > +int nand_match_ecc_req(struct mtd_info *mtd, struct nand_chip *chip, > + const struct nand_ecc_caps *caps) > +{ > + const struct nand_ecc_step_info *stepinfo; > + int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes; > + int req_step = chip->ecc_step_ds; > + int req_strength = chip->ecc_strength_ds; > + int req_corr, step_size, strength, steps, ecc_bytes, ecc_bytes_total; > + int best_step, best_strength, best_ecc_bytes; > + int best_ecc_bytes_total = INT_MAX; Just nitpicking, but why not -1 instead of INT_MAX? > + int i, j; > + > + if (WARN_ON(avail_oobsize < 0)) > + return -EINVAL; > + > + /* No information provided by the NAND chip */ > + if (!req_step || !req_strength) > + return -ENOTSUPP; > + > + /* number of correctable bits the chip requires in a page */ > + req_corr = mtd->writesize / req_step * req_strength; > + > + for (i = 0; i < caps->nstepinfos; i++) { > + stepinfo = &caps->stepinfos[i]; > + step_size = stepinfo->stepsize; > + > + for (j = 0; j < stepinfo->nstrengths; j++) { > + strength = stepinfo->strengths[j]; > + > + /* > + * If both step size and strength are smaller than the > + * chip's requirement, it is not easy to compare the > + * resulted reliability. > + */ > + if (step_size < req_step && strength < req_strength) > + continue; > + > + if (mtd->writesize % step_size) > + continue; > + > + steps = mtd->writesize / step_size; > + > + ecc_bytes = caps->calc_ecc_bytes(step_size, strength); > + if (WARN_ON_ONCE(ecc_bytes < 0)) > + continue; > + ecc_bytes_total = ecc_bytes * steps; > + > + if (ecc_bytes_total > avail_oobsize || > + strength * steps < req_corr) > + continue; > + > + /* > + * We assume the best is to meet the chip's requrement > + * with the least number of ECC bytes. > + */ > + if (ecc_bytes_total < best_ecc_bytes_total) { > + best_ecc_bytes_total = ecc_bytes_total; > + best_step = step_size; > + best_strength = strength; > + best_ecc_bytes = ecc_bytes; > + } > + } > + } > + > + if (best_ecc_bytes_total == INT_MAX) > + return -ENOTSUPP; > + > + chip->ecc.size = best_step; > + chip->ecc.strength = best_strength; > + chip->ecc.bytes = best_ecc_bytes; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(nand_match_ecc_req); > + > +/** > + * nand_maximize_ecc - choose the max ECC strength available > + * @mtd: mtd info structure > + * @chip: nand chip info structure > + * @caps: ECC engine caps info structure > + * > + * Choose the max ECC strength that is supported on the controller, and can > fit > + * within the chip's OOB. On success, the chosen ECC settings are set. > + */ > +int nand_maximize_ecc(struct mtd_info *mtd, struct nand_chip *chip, > + const struct nand_ecc_caps *caps) > +{ > + const struct nand_ecc_step_info *stepinfo; > + int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes; > + int step_size, strength, steps, ecc_bytes, corr; > + int best_corr = 0; > + int best_step = 0; > + int best_strength, best_ecc_bytes; > + int i, j; > + > + if (WARN_ON(avail_oobsize < 0)) > + return -EINVAL; > + > + for (i = 0; i < caps->nstepinfos; i++) { > + stepinfo = &caps->stepinfos[i]; > + step_size = stepinfo->stepsize; > + > + Extra blank line here. > + /* If chip->ecc.size is already set, respect it */ > + if (chip->ecc.size && step_size != chip->ecc.size) > + continue; > + > + for (j = 0; j < stepinfo->nstrengths; j++) { > + strength = stepinfo->strengths[j]; > + > + if (mtd->writesize % step_size) > + continue; > + > + steps = mtd->writesize / step_size; > + > + ecc_bytes = caps->calc_ecc_bytes(step_size, strength); > + if (WARN_ON_ONCE(ecc_bytes < 0)) > + continue; > + > + if (ecc_bytes * steps > avail_oobsize) > + continue; > + > + corr = strength * steps; > + > + /* > + * If the number of correctable bits is the same, > + * bigger step_size has more reliability. > + */ > + if (corr > best_corr || > + (corr == best_corr && step_size > best_step)) { > + best_corr = corr; > + best_step = step_size; > + best_strength = strength; > + best_ecc_bytes = ecc_bytes; > + } > + } > + } > + > + if (!best_corr) > + return -ENOTSUPP; > + > + chip->ecc.size = best_step; > + chip->ecc.strength = best_strength; > + chip->ecc.bytes = best_ecc_bytes; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(nand_maximize_ecc); > + > /* > * Check if the chip configuration meet the datasheet requirements. > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 8f67b15..97ccb76 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -477,6 +477,32 @@ static inline void nand_hw_control_init(struct > nand_hw_control *nfc) > } > > /** > + * struct nand_ecc_step_info - ECC step information of ECC engine > + * @stepsize: data bytes per ECC step > + * @strengths: array of supported strengths > + * @nstrengths: number of supported strengths > + */ > +struct nand_ecc_step_info { > + int stepsize; > + const int *strengths; > + int nstrengths; > +}; > + > +/** > + * struct nand_ecc_caps - capability of ECC engine > + * @stepinfos: array of ECC step information > + * @nstepinfos: number of ECC step information > + * @calc_ecc_bytes: driver's hook to calculate ECC bytes per step > + * @oob_reserve_bytes: number of bytes in OOB that must be reserved > + */ > +struct nand_ecc_caps { > + const struct nand_ecc_step_info *stepinfos; > + int nstepinfos; > + int (*calc_ecc_bytes)(int step_size, int strength); > + int oob_reserve_bytes; Why is this needed? I thought we agreed on passing oobavail as an argument to these helper funcs. If a driver needs to reserve a few OOB bytes, then doing mtd->oobsize - rsvd_bytes is not such a big deal. > +}; > + > +/** > * struct nand_ecc_ctrl - Control structure for ECC > * @mode: ECC mode > * @algo: ECC algorithm > @@ -1244,6 +1270,15 @@ int nand_check_erased_ecc_chunk(void *data, int > datalen, > void *extraoob, int extraooblen, > int threshold); > > +int nand_check_ecc_caps(struct mtd_info *mtd, struct nand_chip *chip, > + const struct nand_ecc_caps *caps); > + > +int nand_match_ecc_req(struct mtd_info *mtd, struct nand_chip *chip, > + const struct nand_ecc_caps *caps); > + > +int nand_maximize_ecc(struct mtd_info *mtd, struct nand_chip *chip, > + const struct nand_ecc_caps *caps); > + > /* Default write_oob implementation */ > int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip, int > page); >
