| Message ID | 1492585618-13655-2-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Boris Brezillon |
| Headers | show |
Hi Masahiro, Sorry for the delay, I was busy with non-NAND/MTD stuff lately. On Wed, 19 Apr 2017 16:06:57 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Each driver has been responsible for: > - Check if ECC setting specified (mostly by DT) is valid > - Meet the chip's required ECC strength > - Maximize the strength when NAND_ECC_MAXIMIZE flag is set > > The logic can be generalized by factoring out driver-specific > parameters. A driver provides: > - Array of (step_size, strength) supported on the controller > - A hook that calculates ECC bytes from the combination of > (step_size, strength). > > Then, this commit provides 3 helper functions: > nand_check_ecc_caps - Check if preset (step_size, strength) is valid > nand_try_to_match_ecc_req - Match the chip's requirement > nand_try_to_maximize_ecc - Maximize the ECC strength > > These helpers will save duplicated code among drivers. Thanks for working on this. My comments below. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > drivers/mtd/nand/nand_base.c | 178 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/mtd/nand.h | 31 ++++++++ > 2 files changed, 209 insertions(+) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 0670e13..ee43e5e 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -4539,6 +4539,184 @@ static int nand_set_ecc_soft_ops(struct mtd_info *mtd) > } > } > > +/** > + * nand_check_ecc_caps - check if ECC step size and strength is supported > + * @mtd: mtd info structure > + * @chip: nand chip info structure > + * @caps: ECC engine caps info structure > + * > + * When ECC step size and strength are set, check if the combination is really > + * supported by the controller and fit within the chip's OOB. On success, > + * ECC bytes per step is set. > + */ > +int nand_check_ecc_caps(struct mtd_info *mtd, struct nand_chip *chip, > + const struct nand_ecc_engine_caps *caps) > +{ > + const struct nand_ecc_setting *setting; > + int preset_step = chip->ecc.size; > + int preset_strength = chip->ecc.strength; > + int ecc_bytes; > + > + if (!preset_step || !preset_strength) > + return -ENODATA; > + > + for (setting = caps->ecc_settings; setting->step; setting++) { > + if (setting->step != preset_step || > + setting->strength != preset_strength) > + continue; > + > + ecc_bytes = caps->calc_ecc_bytes(setting); > + if (WARN_ON_ONCE(ecc_bytes < 0)) > + continue; > + > + if (ecc_bytes * mtd->writesize / setting->step > > + caps->avail_oobsize) { > + pr_err("ECC (step, strength) = (%d, %d) does not fit in OOB", > + setting->step, setting->strength); > + return -ENOSPC; > + } > + > + chip->ecc.bytes = ecc_bytes; > + return 0; > + } > + > + pr_err("ECC (step, strength) = (%d, %d) not supported on this controller", > + preset_step, preset_strength); > + > + return -ENOTSUPP; > +} > + > +/** > + * nand_try_to_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 chip's ECC requirement is available, try to meet it with the least number > + * number of ECC bytes (i.e. with the largest number of OOB-free bytes). > + * On success, the chosen ECC settings are set. > + */ > +int nand_try_to_match_ecc_req(struct mtd_info *mtd, struct nand_chip *chip, > + const struct nand_ecc_engine_caps *caps) > +{ > + const struct nand_ecc_setting *setting, *best_setting = NULL; > + int req_step = chip->ecc_step_ds; > + int req_strength = chip->ecc_strength_ds; > + int req_corr, steps, ecc_bytes, ecc_bytes_total; > + int best_ecc_bytes, best_ecc_bytes_total = INT_MAX; > + > + /* 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 (setting = caps->ecc_settings; setting->step; setting++) { > + /* If chip->ecc.size is already set, respect it. */ > + if (chip->ecc.size && setting->step != chip->ecc.size) > + continue; > + > + /* If chip->ecc.strength is already set, respect it. */ > + if (chip->ecc.strength && > + setting->strength != chip->ecc.strength) > + continue; Hm, I don't get it. If chip->ecc.strength and chip->ecc.size are explicitly set, you should just call nand_check_ecc_caps() and skip nand_try_to_match_ecc_req(). Why would you call nand_try_to_match_ecc_req() in this case? > + > + /* > + * If the controller's step size is smaller than the chip's > + * requirement, comparison of the strength is not simple. > + */ There's one thing we can easily do in this case: try to apply the same strength but on the smaller step size. If it fits the OOB area, we have a valid match, if it doesn't, then we can fallback to ECC maximization in case no valid settings were found after iterating over ECC settings. How about: if (setting->step < req_step && setting->strength < req_strength) continue; > + if (setting->step < req_step) > + continue; You should probably check that setting->step < mtd->writesize. > + > + steps = mtd->writesize / setting->step; Not sure it will ever happen to have a step which is not a multiple of ->writesize, but it's probably safer to do DIV_ROUND_UP(), or simply skip the ECC setting entry if mtd->writesize % setting->step != 0. > + > + ecc_bytes = caps->calc_ecc_bytes(setting); > + if (WARN_ON_ONCE(ecc_bytes < 0)) > + continue; > + ecc_bytes_total = ecc_bytes * steps; > + > + if (ecc_bytes_total > caps->avail_oobsize || > + setting->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_settings entries were in ascending order (lowest step-size and strength first), you could bail out as soon as you find a suitable config, because following settings would necessarily take more bits. > + if (ecc_bytes_total < best_ecc_bytes_total) { > + best_ecc_bytes_total = ecc_bytes_total; > + best_setting = setting; > + best_ecc_bytes = ecc_bytes; > + } > + } > + > + if (!best_setting) > + return -ENOTSUPP; > + > + chip->ecc.size = best_setting->step; > + chip->ecc.strength = best_setting->strength; > + chip->ecc.bytes = best_ecc_bytes; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(nand_try_to_match_ecc_req); > + > +/** > + * nand_try_to_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_try_to_maximize_ecc(struct mtd_info *mtd, struct nand_chip *chip, > + const struct nand_ecc_engine_caps *caps) > +{ > + const struct nand_ecc_setting *setting, *best_setting = NULL; > + int steps, ecc_bytes, corr; > + int best_corr = 0; > + int best_ecc_bytes; > + > + for (setting = caps->ecc_settings; setting->step; setting++) { > + /* If chip->ecc.size is already set, respect it. */ > + if (chip->ecc.size && setting->step != chip->ecc.size) > + continue; > + > + steps = mtd->writesize / setting->step; > + ecc_bytes = caps->calc_ecc_bytes(setting); > + if (WARN_ON_ONCE(ecc_bytes < 0)) > + continue; > + > + if (ecc_bytes * steps > caps->avail_oobsize) > + continue; > + > + corr = setting->strength * steps; > + > + /* > + * If the number of correctable bits is the same, > + * bigger ecc_step has more reliability. > + */ > + if (corr > best_corr || > + (corr == best_corr && setting->step > best_setting->step)) { > + best_corr = corr; > + best_setting = setting; > + best_ecc_bytes = ecc_bytes; > + } Same comment as earlier: you could probably skip a few entries by enforcing ordering in the ecc_settings array. > + } > + > + if (!best_setting) > + return -ENOTSUPP; > + > + chip->ecc.size = best_setting->step; > + chip->ecc.strength = best_setting->strength; > + chip->ecc.bytes = best_ecc_bytes; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(nand_try_to_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 2ae781e..394128f 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -486,6 +486,28 @@ static inline void nand_hw_control_init(struct nand_hw_control *nfc) > } > > /** > + * struct nand_ecc_setting - information of ECC step supported by ECC engine > + * @step: data bytes per ECC step > + * @bytes: ECC bytes per step > + */ > +struct nand_ecc_setting { > + int step; > + int strength; > +}; I think I already mentioned that I'd prefer to have the step and strength info separated in 2 different objects so that we can easily re-use the strength definitions for all step-size supported by the engine (ECC engines usually provide the same set of strengths for all supported step-size). struct nand_ecc_step_size_caps { int step_size; int *strengths; }; struct nand_ecc_engine_caps { const struct nand_ecc_step_size_caps *step_sizes; /* ... */ }; static int my_strengths[] = { 8, 16, 24, 0 }; static const struct nand_ecc_step_size_caps my_step_sizes[] = { { 512, my_strengths }, { 1024, my_strengths }, /* sentinel */ }; static const struct nand_ecc_engine_caps my_ecc_caps = { .step_sizes = my_step_sizes, /* ... */ }; > + > +/** > + * struct nand_ecc_engine_caps - capability of ECC engine > + * @ecc_settings: array of (step, strength) supported by this ECC engine > + * @calc_ecc_bytes: driver's hook to calculate ECC bytes per step > + * @avail_oobsize: OOB size that the ECC engine can use for ECC correction > + */ > +struct nand_ecc_engine_caps { > + const struct nand_ecc_setting *ecc_settings; > + int (*calc_ecc_bytes)(const struct nand_ecc_setting *ecc_setting); > + int avail_oobsize; avail_oobsize should be passed as an argument to the different helpers you define above, because it's completely dependent on the NAND chip, which means you would have change it dynamically, which in turn prevents you from defining this object as 'static const' in your driver. > +};
Hi Boris, 2017-04-29 1:32 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: >> + for (setting = caps->ecc_settings; setting->step; setting++) { >> + /* If chip->ecc.size is already set, respect it. */ >> + if (chip->ecc.size && setting->step != chip->ecc.size) >> + continue; >> + >> + /* If chip->ecc.strength is already set, respect it. */ >> + if (chip->ecc.strength && >> + setting->strength != chip->ecc.strength) >> + continue; > > Hm, I don't get it. If chip->ecc.strength and chip->ecc.size are > explicitly set, you should just call nand_check_ecc_caps() and skip > nand_try_to_match_ecc_req(). Why would you call > nand_try_to_match_ecc_req() in this case? I want to call this function if ecc.size is specified but ecc.strength is not (or vice versa). If both ecc.size and ecc.strength are already specified, you are right, no need to call this function. This function can check the sanity of the specified combination of (step, strength), but this is the same as what nand_check_ecc_caps() does. >> + >> + /* >> + * If the controller's step size is smaller than the chip's >> + * requirement, comparison of the strength is not simple. >> + */ > > There's one thing we can easily do in this case: try to apply the > same strength but on the smaller step size. If it fits the OOB area, we > have a valid match, if it doesn't, then we can fallback to ECC > maximization in case no valid settings were found after iterating over > ECC settings. > > How about: > > if (setting->step < req_step && > setting->strength < req_strength) > continue; Make sense. I will do this. >> + if (setting->step < req_step) >> + continue; > > You should probably check that setting->step < mtd->writesize. > >> + >> + steps = mtd->writesize / setting->step; > > Not sure it will ever happen to have a step which is not a multiple of > ->writesize, but it's probably safer to do DIV_ROUND_UP(), or simply > skip the ECC setting entry if mtd->writesize % setting->step != 0. OK. mtd->writesize % setting->step != 0 will guarantee setting->step <= mtd->writesize >> + >> + ecc_bytes = caps->calc_ecc_bytes(setting); >> + if (WARN_ON_ONCE(ecc_bytes < 0)) >> + continue; >> + ecc_bytes_total = ecc_bytes * steps; >> + >> + if (ecc_bytes_total > caps->avail_oobsize || >> + setting->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_settings entries were in ascending order (lowest step-size and > strength first), you could bail out as soon as you find a suitable > config, because following settings would necessarily take more bits. If we want to achieve this, step-size must be descending order, while strength must be ascending order. This is a bit confusing (and I have no idea how to statically check if every driver follows this order). >> + /* >> + * If the number of correctable bits is the same, >> + * bigger ecc_step has more reliability. >> + */ >> + if (corr > best_corr || >> + (corr == best_corr && setting->step > best_setting->step)) { >> + best_corr = corr; >> + best_setting = setting; >> + best_ecc_bytes = ecc_bytes; >> + } > > Same comment as earlier: you could probably skip a few entries by > enforcing ordering in the ecc_settings array. Assuming that step-size is descending order and strength is ascending order, it may be possible by iterating backward. But, I think the array should be terminated by zero or NULL. How to find the last entry we want to start iterating from? Assuming the array size is small, is this worthwhile? >> + } >> + >> + if (!best_setting) >> + return -ENOTSUPP; >> + >> + chip->ecc.size = best_setting->step; >> + chip->ecc.strength = best_setting->strength; >> + chip->ecc.bytes = best_ecc_bytes; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(nand_try_to_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 2ae781e..394128f 100644 >> --- a/include/linux/mtd/nand.h >> +++ b/include/linux/mtd/nand.h >> @@ -486,6 +486,28 @@ static inline void nand_hw_control_init(struct nand_hw_control *nfc) >> } >> >> /** >> + * struct nand_ecc_setting - information of ECC step supported by ECC engine >> + * @step: data bytes per ECC step >> + * @bytes: ECC bytes per step >> + */ >> +struct nand_ecc_setting { >> + int step; >> + int strength; >> +}; > > I think I already mentioned that I'd prefer to have the step and > strength info separated in 2 different objects so that we can easily > re-use the strength definitions for all step-size supported by the > engine (ECC engines usually provide the same set of strengths for all > supported step-size). > > struct nand_ecc_step_size_caps { > int step_size; > int *strengths; > }; > > struct nand_ecc_engine_caps { > const struct nand_ecc_step_size_caps *step_sizes; > /* ... */ > }; > > static int my_strengths[] = { 8, 16, 24, 0 }; > static const struct nand_ecc_step_size_caps my_step_sizes[] = { > { 512, my_strengths }, > { 1024, my_strengths }, > /* sentinel */ > }; > > static const struct nand_ecc_engine_caps my_ecc_caps = { > .step_sizes = my_step_sizes, > /* ... */ > }; At first, I implemented as you suggested, but I did not like the taste of the code. I want each compatible string has only one associated data array as you see in 2/2. If we follow the separate structure approach, each compatible has a chain of caps arrays. One more thing, the loop nest will become deeper, for the outer loop with ecc-step, and inter loop with strength. Just in case, I copy-pasted the compatible array I wrote first. static const int denali_socfpga_ecc_strengths[] = {8, 15, 0}; static const struct nand_ecc_step_caps denali_socfpga_ecc_step_caps[] = { { .step_size = 512, .strengths = denali_socfpga_ecc_strengths, }, {}, }; static const struct denali_dt_data denali_socfpga_data = { .caps = DENALI_CAP_HW_ECC_FIXUP, .ecc_step_caps = denali_socfpga_ecc_step_caps, }; static const int denali_uniphier_v5a_strengths[] = {8, 16, 24, 0}; static const struct nand_ecc_step_caps denali_uniphier_v5a_ecc_step_caps[] = { { .step_size = 1024, .strengths = denali_uniphier_v5a_strengths, }, {}, }; static const struct denali_dt_data denali_uniphier_v5a_data = { .caps = DENALI_CAP_HW_ECC_FIXUP | DENALI_CAP_DMA_64BIT, .ecc_step_caps = denali_uniphier_v5a_ecc_step_caps, }; static const int denali_uniphier_v5b_strengths[] = {8, 16, 0}; static const struct nand_ecc_step_caps denali_uniphier_v5b_ecc_step_caps[] = { { .step_size = 1024, .strengths = denali_uniphier_v5b_strengths, }, {}, }; static const struct denali_dt_data denali_uniphier_v5b_data = { .revision = 0x0501, .caps = DENALI_CAP_HW_ECC_FIXUP | DENALI_CAP_DMA_64BIT, .ecc_step_caps = denali_uniphier_v5b_ecc_step_caps, }; static const struct of_device_id denali_nand_dt_ids[] = { { .compatible = "altr,socfpga-denali-nand", .data = &denali_socfpga_data, }, { .compatible = "socionext,uniphier-denali-nand-v5a", .data = &denali_uniphier_v5a_data, }, { .compatible = "socionext,uniphier-denali-nand-v5b", .data = &denali_uniphier_v5b_data, }, { /* sentinel */ } }; > >> + >> +/** >> + * struct nand_ecc_engine_caps - capability of ECC engine >> + * @ecc_settings: array of (step, strength) supported by this ECC engine >> + * @calc_ecc_bytes: driver's hook to calculate ECC bytes per step >> + * @avail_oobsize: OOB size that the ECC engine can use for ECC correction >> + */ >> +struct nand_ecc_engine_caps { >> + const struct nand_ecc_setting *ecc_settings; >> + int (*calc_ecc_bytes)(const struct nand_ecc_setting *ecc_setting); >> + int avail_oobsize; > > avail_oobsize should be passed as an argument to the different helpers > you define above, because it's completely dependent on the NAND chip, > which means you would have change it dynamically, which in turn > prevents you from defining this object as 'static const' in your driver. Make sense.
Hi Masahiro, Sorry for the late reply. On Mon, 8 May 2017 12:40:47 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Boris, > > > 2017-04-29 1:32 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > > >> + for (setting = caps->ecc_settings; setting->step; setting++) { > >> + /* If chip->ecc.size is already set, respect it. */ > >> + if (chip->ecc.size && setting->step != chip->ecc.size) > >> + continue; > >> + > >> + /* If chip->ecc.strength is already set, respect it. */ > >> + if (chip->ecc.strength && > >> + setting->strength != chip->ecc.strength) > >> + continue; > > > > Hm, I don't get it. If chip->ecc.strength and chip->ecc.size are > > explicitly set, you should just call nand_check_ecc_caps() and skip > > nand_try_to_match_ecc_req(). Why would you call > > nand_try_to_match_ecc_req() in this case? > > > I want to call this function if > ecc.size is specified but ecc.strength is not > (or vice versa). That's not a valid combination. I accepted the case where nand-ecc-step-size is not defined in the DT just because sometime you only have one possible setting which is imposed by the controller. In this case ecc.size should be explicitly set by the driver not left to 0. > > > If both ecc.size and ecc.strength are already specified, > you are right, no need to call this function. > This function can check the sanity of the specified > combination of (step, strength), but this is the same > as what nand_check_ecc_caps() does. Really, this function is just about trying to match ecc_strength_ds and ecc_step_size_ds, and not a partial combination of DT def + _ds def. > > > > > >> + > >> + /* > >> + * If the controller's step size is smaller than the chip's > >> + * requirement, comparison of the strength is not simple. > >> + */ > > > > There's one thing we can easily do in this case: try to apply the > > same strength but on the smaller step size. If it fits the OOB area, we > > have a valid match, if it doesn't, then we can fallback to ECC > > maximization in case no valid settings were found after iterating over > > ECC settings. > > > > How about: > > > > if (setting->step < req_step && > > setting->strength < req_strength) > > continue; > > Make sense. I will do this. > > > > >> + if (setting->step < req_step) > >> + continue; > > > > You should probably check that setting->step < mtd->writesize. > > > >> + > >> + steps = mtd->writesize / setting->step; > > > > Not sure it will ever happen to have a step which is not a multiple of > > ->writesize, but it's probably safer to do DIV_ROUND_UP(), or simply > > skip the ECC setting entry if mtd->writesize % setting->step != 0. > > OK. > > mtd->writesize % setting->step != 0 > will guarantee > setting->step <= mtd->writesize > Indeed. > > > > > >> + > >> + ecc_bytes = caps->calc_ecc_bytes(setting); > >> + if (WARN_ON_ONCE(ecc_bytes < 0)) > >> + continue; > >> + ecc_bytes_total = ecc_bytes * steps; > >> + > >> + if (ecc_bytes_total > caps->avail_oobsize || > >> + setting->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_settings entries were in ascending order (lowest step-size and > > strength first), you could bail out as soon as you find a suitable > > config, because following settings would necessarily take more bits. > > If we want to achieve this, > step-size must be descending order, > while strength must be ascending order. > > This is a bit confusing (and I have no idea > how to statically check if every driver follows this order). Well, I was not suggesting to check the ordering statically. It's more a convention and reviews should allow us to detect offenders. This being said, maybe it's not such a big deal to iterate over all configs before taking a decision. > > > > >> + /* > >> + * If the number of correctable bits is the same, > >> + * bigger ecc_step has more reliability. > >> + */ > >> + if (corr > best_corr || > >> + (corr == best_corr && setting->step > best_setting->step)) { > >> + best_corr = corr; > >> + best_setting = setting; > >> + best_ecc_bytes = ecc_bytes; > >> + } > > > > Same comment as earlier: you could probably skip a few entries by > > enforcing ordering in the ecc_settings array. > > > Assuming that step-size is descending order and strength is ascending order, > it may be possible by iterating backward. > > But, I think the array should be terminated by zero or NULL. Well, you decided to do that. Actually I'd prefer to have an object containing both a pointer to the array and the array size. I'm really not a big fan of the sentinel entry approach, because it's error prone compared to an ARRAY_SIZE(def). > How to find the last entry we want to start iterating from? If you store the array size, it's just a matter of doing: for (i = ARRAY_SIZE(def); i >= 0; i--) ... > > Assuming the array size is small, is this worthwhile? Maybe not. > > > > >> + } > >> + > >> + if (!best_setting) > >> + return -ENOTSUPP; > >> + > >> + chip->ecc.size = best_setting->step; > >> + chip->ecc.strength = best_setting->strength; > >> + chip->ecc.bytes = best_ecc_bytes; > >> + > >> + return 0; > >> +} > >> +EXPORT_SYMBOL_GPL(nand_try_to_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 2ae781e..394128f 100644 > >> --- a/include/linux/mtd/nand.h > >> +++ b/include/linux/mtd/nand.h > >> @@ -486,6 +486,28 @@ static inline void nand_hw_control_init(struct nand_hw_control *nfc) > >> } > >> > >> /** > >> + * struct nand_ecc_setting - information of ECC step supported by ECC engine > >> + * @step: data bytes per ECC step > >> + * @bytes: ECC bytes per step > >> + */ > >> +struct nand_ecc_setting { > >> + int step; > >> + int strength; > >> +}; > > > > I think I already mentioned that I'd prefer to have the step and > > strength info separated in 2 different objects so that we can easily > > re-use the strength definitions for all step-size supported by the > > engine (ECC engines usually provide the same set of strengths for all > > supported step-size). > > > > struct nand_ecc_step_size_caps { > > int step_size; > > int *strengths; > > }; > > > > struct nand_ecc_engine_caps { > > const struct nand_ecc_step_size_caps *step_sizes; > > /* ... */ > > }; > > > > static int my_strengths[] = { 8, 16, 24, 0 }; > > static const struct nand_ecc_step_size_caps my_step_sizes[] = { > > { 512, my_strengths }, > > { 1024, my_strengths }, > > /* sentinel */ > > }; > > > > static const struct nand_ecc_engine_caps my_ecc_caps = { > > .step_sizes = my_step_sizes, > > /* ... */ > > }; > > > At first, I implemented as you suggested, but I did not like the taste > of the code. Well, I prefer it this way, and since, as you say, it's a matter of taste, I think I'll abuse of my maintainer's position and push for this approach :-). > > I want each compatible string has only one associated data array > as you see in 2/2. > > > If we follow the separate structure approach, each compatible > has a chain of caps arrays. > > > One more thing, the loop nest will become deeper, > for the outer loop with ecc-step, > and inter loop with strength. > > > Just in case, I copy-pasted the compatible array I wrote first. Note that your use case is a bit biased because the strength arrays is not reusable in the denali driver. But I have a real use case (sunxi_nand) where the strength array is exactly the same for 512 and 1024 step size. > > > static const int denali_socfpga_ecc_strengths[] = {8, 15, 0}; > static const struct nand_ecc_step_caps denali_socfpga_ecc_step_caps[] = { > { .step_size = 512, .strengths = denali_socfpga_ecc_strengths, }, > {}, > }; > > static const struct denali_dt_data denali_socfpga_data = { > .caps = DENALI_CAP_HW_ECC_FIXUP, > .ecc_step_caps = denali_socfpga_ecc_step_caps, > }; > > static const int denali_uniphier_v5a_strengths[] = {8, 16, 24, 0}; > static const struct nand_ecc_step_caps denali_uniphier_v5a_ecc_step_caps[] = { > { .step_size = 1024, .strengths = denali_uniphier_v5a_strengths, }, > {}, > }; > > static const struct denali_dt_data denali_uniphier_v5a_data = { > .caps = DENALI_CAP_HW_ECC_FIXUP | > DENALI_CAP_DMA_64BIT, > .ecc_step_caps = denali_uniphier_v5a_ecc_step_caps, > }; > > static const int denali_uniphier_v5b_strengths[] = {8, 16, 0}; > static const struct nand_ecc_step_caps denali_uniphier_v5b_ecc_step_caps[] = { > { .step_size = 1024, .strengths = denali_uniphier_v5b_strengths, }, > {}, > }; If you store the size of the array in the struct as I suggested above, you can even re-use the strength array here: static const int denali_uniphier_v5_strengths[] = {8, 16, 24}; static const struct nand_ecc_step_caps denali_uniphier_v5a_ecc_step_caps[] = { { .step_size = 1024, .nstrengths = ARRAY_SIZE(denali_uniphier_v5_strengths), .strengths = denali_uniphier_v5a_strengths, } }; static const struct nand_ecc_step_caps denali_uniphier_v5b_ecc_step_caps[] = { { .step_size = 1024, .nstrengths = ARRAY_SIZE(denali_uniphier_v5_strengths) - 1, .strengths = denali_uniphier_v5_strengths, } };
Hi Boris, 2017-05-15 20:54 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > Hi Masahiro, > > Sorry for the late reply. > > On Mon, 8 May 2017 12:40:47 +0900 > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > >> Hi Boris, >> >> >> 2017-04-29 1:32 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: >> >> >> + for (setting = caps->ecc_settings; setting->step; setting++) { >> >> + /* If chip->ecc.size is already set, respect it. */ >> >> + if (chip->ecc.size && setting->step != chip->ecc.size) >> >> + continue; >> >> + >> >> + /* If chip->ecc.strength is already set, respect it. */ >> >> + if (chip->ecc.strength && >> >> + setting->strength != chip->ecc.strength) >> >> + continue; >> > >> > Hm, I don't get it. If chip->ecc.strength and chip->ecc.size are >> > explicitly set, you should just call nand_check_ecc_caps() and skip >> > nand_try_to_match_ecc_req(). Why would you call >> > nand_try_to_match_ecc_req() in this case? >> >> >> I want to call this function if >> ecc.size is specified but ecc.strength is not >> (or vice versa). > > That's not a valid combination. I accepted the case where > nand-ecc-step-size is not defined in the DT just because sometime you > only have one possible setting which is imposed by the controller. In > this case ecc.size should be explicitly set by the driver not left to 0. > >> >> >> If both ecc.size and ecc.strength are already specified, >> you are right, no need to call this function. >> This function can check the sanity of the specified >> combination of (step, strength), but this is the same >> as what nand_check_ecc_caps() does. I am working on the next version because I really need to merge all of my Denali controller patches for my SoCs. One question about this part. /* If chip->ecc.size is already set, respect it. */ if (chip->ecc.size && step_size != chip->ecc.size) continue; Does this make sense for nand_try_to_maximize_ecc()? (In other words, can nand-ecc-maximize stand together with nand-ecc-step-size?)
On Thu, 18 May 2017 15:27:11 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Boris, > > > > 2017-05-15 20:54 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > > Hi Masahiro, > > > > Sorry for the late reply. > > > > On Mon, 8 May 2017 12:40:47 +0900 > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > > >> Hi Boris, > >> > >> > >> 2017-04-29 1:32 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > >> > >> >> + for (setting = caps->ecc_settings; setting->step; setting++) { > >> >> + /* If chip->ecc.size is already set, respect it. */ > >> >> + if (chip->ecc.size && setting->step != chip->ecc.size) > >> >> + continue; > >> >> + > >> >> + /* If chip->ecc.strength is already set, respect it. */ > >> >> + if (chip->ecc.strength && > >> >> + setting->strength != chip->ecc.strength) > >> >> + continue; > >> > > >> > Hm, I don't get it. If chip->ecc.strength and chip->ecc.size are > >> > explicitly set, you should just call nand_check_ecc_caps() and skip > >> > nand_try_to_match_ecc_req(). Why would you call > >> > nand_try_to_match_ecc_req() in this case? > >> > >> > >> I want to call this function if > >> ecc.size is specified but ecc.strength is not > >> (or vice versa). > > > > That's not a valid combination. I accepted the case where > > nand-ecc-step-size is not defined in the DT just because sometime you > > only have one possible setting which is imposed by the controller. In > > this case ecc.size should be explicitly set by the driver not left to 0. > > > >> > >> > >> If both ecc.size and ecc.strength are already specified, > >> you are right, no need to call this function. > >> This function can check the sanity of the specified > >> combination of (step, strength), but this is the same > >> as what nand_check_ecc_caps() does. > > > I am working on the next version because I really need to > merge all of my Denali controller patches for my SoCs. Okay. > > > One question about this part. > > > /* If chip->ecc.size is already set, respect it. */ > if (chip->ecc.size && step_size != chip->ecc.size) > continue; > > Does this make sense for nand_try_to_maximize_ecc()? > > (In other words, can nand-ecc-maximize stand together with nand-ecc-step-size?) It could make sense if one wants to maximize the strength for a specific step-size, but most of the time the user will let the driver choose the best step-size+strength pair.
Hi Boris, 2017-05-18 16:09 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: >> >> One question about this part. >> >> >> /* If chip->ecc.size is already set, respect it. */ >> if (chip->ecc.size && step_size != chip->ecc.size) >> continue; >> >> Does this make sense for nand_try_to_maximize_ecc()? >> >> (In other words, can nand-ecc-maximize stand together with nand-ecc-step-size?) > > It could make sense if one wants to maximize the strength for a > specific step-size, but most of the time the user will let the driver > choose the best step-size+strength pair. OK. I am keeping it here, but I do not have a strong opinion about this. I will follow your decision. Thanks.
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 0670e13..ee43e5e 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -4539,6 +4539,184 @@ static int nand_set_ecc_soft_ops(struct mtd_info *mtd) } } +/** + * nand_check_ecc_caps - check if ECC step size and strength is supported + * @mtd: mtd info structure + * @chip: nand chip info structure + * @caps: ECC engine caps info structure + * + * When ECC step size and strength are set, check if the combination is really + * supported by the controller and fit within the chip's OOB. On success, + * ECC bytes per step is set. + */ +int nand_check_ecc_caps(struct mtd_info *mtd, struct nand_chip *chip, + const struct nand_ecc_engine_caps *caps) +{ + const struct nand_ecc_setting *setting; + int preset_step = chip->ecc.size; + int preset_strength = chip->ecc.strength; + int ecc_bytes; + + if (!preset_step || !preset_strength) + return -ENODATA; + + for (setting = caps->ecc_settings; setting->step; setting++) { + if (setting->step != preset_step || + setting->strength != preset_strength) + continue; + + ecc_bytes = caps->calc_ecc_bytes(setting); + if (WARN_ON_ONCE(ecc_bytes < 0)) + continue; + + if (ecc_bytes * mtd->writesize / setting->step > + caps->avail_oobsize) { + pr_err("ECC (step, strength) = (%d, %d) does not fit in OOB", + setting->step, setting->strength); + return -ENOSPC; + } + + chip->ecc.bytes = ecc_bytes; + return 0; + } + + pr_err("ECC (step, strength) = (%d, %d) not supported on this controller", + preset_step, preset_strength); + + return -ENOTSUPP; +} + +/** + * nand_try_to_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 chip's ECC requirement is available, try to meet it with the least number + * number of ECC bytes (i.e. with the largest number of OOB-free bytes). + * On success, the chosen ECC settings are set. + */ +int nand_try_to_match_ecc_req(struct mtd_info *mtd, struct nand_chip *chip, + const struct nand_ecc_engine_caps *caps) +{ + const struct nand_ecc_setting *setting, *best_setting = NULL; + int req_step = chip->ecc_step_ds; + int req_strength = chip->ecc_strength_ds; + int req_corr, steps, ecc_bytes, ecc_bytes_total; + int best_ecc_bytes, best_ecc_bytes_total = INT_MAX; + + /* 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 (setting = caps->ecc_settings; setting->step; setting++) { + /* If chip->ecc.size is already set, respect it. */ + if (chip->ecc.size && setting->step != chip->ecc.size) + continue; + + /* If chip->ecc.strength is already set, respect it. */ + if (chip->ecc.strength && + setting->strength != chip->ecc.strength) + continue; + + /* + * If the controller's step size is smaller than the chip's + * requirement, comparison of the strength is not simple. + */ + if (setting->step < req_step) + continue; + + steps = mtd->writesize / setting->step; + + ecc_bytes = caps->calc_ecc_bytes(setting); + if (WARN_ON_ONCE(ecc_bytes < 0)) + continue; + ecc_bytes_total = ecc_bytes * steps; + + if (ecc_bytes_total > caps->avail_oobsize || + setting->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_setting = setting; + best_ecc_bytes = ecc_bytes; + } + } + + if (!best_setting) + return -ENOTSUPP; + + chip->ecc.size = best_setting->step; + chip->ecc.strength = best_setting->strength; + chip->ecc.bytes = best_ecc_bytes; + + return 0; +} +EXPORT_SYMBOL_GPL(nand_try_to_match_ecc_req); + +/** + * nand_try_to_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_try_to_maximize_ecc(struct mtd_info *mtd, struct nand_chip *chip, + const struct nand_ecc_engine_caps *caps) +{ + const struct nand_ecc_setting *setting, *best_setting = NULL; + int steps, ecc_bytes, corr; + int best_corr = 0; + int best_ecc_bytes; + + for (setting = caps->ecc_settings; setting->step; setting++) { + /* If chip->ecc.size is already set, respect it. */ + if (chip->ecc.size && setting->step != chip->ecc.size) + continue; + + steps = mtd->writesize / setting->step; + ecc_bytes = caps->calc_ecc_bytes(setting); + if (WARN_ON_ONCE(ecc_bytes < 0)) + continue; + + if (ecc_bytes * steps > caps->avail_oobsize) + continue; + + corr = setting->strength * steps; + + /* + * If the number of correctable bits is the same, + * bigger ecc_step has more reliability. + */ + if (corr > best_corr || + (corr == best_corr && setting->step > best_setting->step)) { + best_corr = corr; + best_setting = setting; + best_ecc_bytes = ecc_bytes; + } + } + + if (!best_setting) + return -ENOTSUPP; + + chip->ecc.size = best_setting->step; + chip->ecc.strength = best_setting->strength; + chip->ecc.bytes = best_ecc_bytes; + + return 0; +} +EXPORT_SYMBOL_GPL(nand_try_to_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 2ae781e..394128f 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -486,6 +486,28 @@ static inline void nand_hw_control_init(struct nand_hw_control *nfc) } /** + * struct nand_ecc_setting - information of ECC step supported by ECC engine + * @step: data bytes per ECC step + * @bytes: ECC bytes per step + */ +struct nand_ecc_setting { + int step; + int strength; +}; + +/** + * struct nand_ecc_engine_caps - capability of ECC engine + * @ecc_settings: array of (step, strength) supported by this ECC engine + * @calc_ecc_bytes: driver's hook to calculate ECC bytes per step + * @avail_oobsize: OOB size that the ECC engine can use for ECC correction + */ +struct nand_ecc_engine_caps { + const struct nand_ecc_setting *ecc_settings; + int (*calc_ecc_bytes)(const struct nand_ecc_setting *ecc_setting); + int avail_oobsize; +}; + +/** * struct nand_ecc_ctrl - Control structure for ECC * @mode: ECC mode * @algo: ECC algorithm @@ -1208,6 +1230,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_engine_caps *caps); + +int nand_try_to_match_ecc_req(struct mtd_info *mtd, struct nand_chip *chip, + const struct nand_ecc_engine_caps *caps); + +int nand_try_to_maximize_ecc(struct mtd_info *mtd, struct nand_chip *chip, + const struct nand_ecc_engine_caps *caps); + /* Default write_oob implementation */ int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip, int page);
Each driver has been responsible for: - Check if ECC setting specified (mostly by DT) is valid - Meet the chip's required ECC strength - Maximize the strength when NAND_ECC_MAXIMIZE flag is set The logic can be generalized by factoring out driver-specific parameters. A driver provides: - Array of (step_size, strength) supported on the controller - A hook that calculates ECC bytes from the combination of (step_size, strength). Then, this commit provides 3 helper functions: nand_check_ecc_caps - Check if preset (step_size, strength) is valid nand_try_to_match_ecc_req - Match the chip's requirement nand_try_to_maximize_ecc - Maximize the ECC strength These helpers will save duplicated code among drivers. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- drivers/mtd/nand/nand_base.c | 178 +++++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/nand.h | 31 ++++++++ 2 files changed, 209 insertions(+)